HW spinlocks rework

[Fymyte]

Problem Description

The current implementation of HWSPINLOCK is flacky because it doesn't involve a standard spinlock in conjunction with HW. Currently, it is possible to be preempted by another task while holding a hwspinlock, locking other clusters while we sleep. The HW is also not protected in SMP context against multiple thread trying to acquire the ownership of the same spinlock at the same time. This may result in a double lock, or double release of the lock. Depending on the hardware, this could simply inverse the working of the spinlock.

The framework is also quite bare-bones. There are no devicetree helpers to go along, and the devicetree bindings are also missing the standard hwlocks &co properties.

Finally, hwspinlocks should not be accessed in userspace. Because hwspinlock are used to protect shared hw resources, only drivers should be consumers. If a user application needs a synchronization mechanism between clusters, it should use higher level APIs like MBOX, IPM or IPC.
Syscalls are also not generated properly right-now because of a missing call to zephyr_syscall_header() in CMakeLists.txt

Proposed Change (Summary)

I would propose the following solutions

• Remove the syscalls for hwspinlocks.
• Include a regular spinlock, embedded in the hwspinlock. It should be invisible to the user.
• Streamline the API with regular spinlocks
  • Provide a hwspinlock_dt_spec structure, similar to MBOX. It will hold the hwspinlock device, id and inner zephyr spinlock
  • Provide an api similar to regular spinlock.
• Provide devicetree helpers
  • Add standard hwlocks hwlock-names and #hwlock-cells properties in the devicetree bindings
  • Provide devicetree helper macro to extract the hwspinlock controller and ID, similar to how MBOX does it.

Proposed Change (Detailed)

Syscalls removal

As stated previously, hwspinlocks should not be accessed in userspace. They must be reserved to driver implementation in kernel space to protect shared hardware resources.

• Replace the separate __syscall and z_impl_ function with direct implementation
• Remove the hwspinlock_handlers.c file

API similar to zephyr spinlocks

• Replace

   int hwspinlock_trylock(...);

  with

   int hw_spin_trylock(..., k_spinlock_key_t *key);

• Replace

   void hwspinlock_lock(...);

  with

   k_spinlock_key_t hw_spin_lock(...);

• Replace

   void hwspinlock_unlock(...);

  with

   void hw_spin_unlock(..., k_spinlock_key_t key);

The user will call lock or trylock and get a spinlock key, and use the same key to unlock the hwspinlock

Embed a spinlock

Create a struct hwspinlock_context to hold the required spinlock. This struct will be passed to the lock/trylock/unlock APIs so the framework can manage the embedded spinlock directly.

• When calling hw_spin_lock/hw_spin_trylock, the framework first calls k_spin_lock/k_spin_trylock with the embedded spinlock, and upon success, continues with the hwspinlock api calls.
• When calling hw_spin_unlock, we first call the hwspinlock api, then call k_spin_unlock to release the lock on the inner spinlock.

Use a typedef hwspinlock_ctx_t to hide the inner working of the context. It will just be an opaque type to the user, that he will pass around the API calls.

Create a struct hwspinlock_dt_spec to hold the device pointer, hwspinlock id and context.

struct hwspinlock_dt_spec {
    const struct device *dev;
    uint32_t id;
    hwspinlock_ctx_t ctx;
};

Add helper functions suffixed with _dt to use this struct, as done in the MBOX framework.

Devicetree helpers

Add the linux standard properties in dts/bindings

• hwlocks: phandle-array, contain the list of hwspinlock
• hwlock-names: string-array, names accompanying the phandles
• #hwlock-cells: int, used by the hwspinlock controller to defines the fields of the phandles. Standardize the id field, which will be used in devicetree helper macros.

Add multiple helper macros to extract those information from the devicetree

• DT_HWSPINLOCK_CTRL_BY_IDX(node_id, idx): get the hwspinlock controller from a phandle at index
• DT_HWSPINLOCK_CTRL_BY_NAME(node_id, name): get the hwspinlock controller from a phandle by its name in hwlock-names property
• DT_HWSPINLOCK_ID_BY_IDX(node_id, idx): get the hwspinlock id from a phandle at index
• DT_HWSPINLOCK_ID_BY_NAME(node_id, name): get the hwspinlock id from a phandle by its name in hwlock-names property

Also include helpers for hwspinlock_dt_spec initialization

• HWSPINLOCK_DT_SPEC_GET_BY_IDX(node_id, idx)
• HWSPINLOCK_DT_SPEC_GET_BY_NAME(node_id, idx)
• HWSPINLOCK_DT_SPEC_GET(node_id, idx): same as HWSPINLOCK_DT_SPEC_GET_BY_IDX(node_id, 0)
• HWSPINLOCK_DT_SPEC_INST_GET_BY_IDX(inst, idx)
• HWSPINLOCK_DT_SPEC_INST_GET_BY_NAME(inst, idx)
• HWSPINLOCK_DT_SPEC_INST_GET(inst, idx): same as HWSPINLOCK_DT_SPEC_INST_GET_BY_IDX(node_id, 0)

Dependencies

Only the HWSPINLOCK framework should is impacted.

Concerns and Unresolved Questions

No response

Alternatives Considered

No response

[Fymyte]

I have a working implementation of those changes in my personal tree. They are part of changes we needed to do in my company. You can see the updates on my branch main...Fymyte:zephyr:fymyte/hwspinlock .

I can create a PR for each individual commits, addressing a single point at a time. Or I can create 2 PR, one addressing the points mentioned here, and a second updating our driver. (The driver API didn't change, but the driver was not functional)

[henrikbrixandersen]

I can create a PR for each individual commits, addressing a single point at a time. Or I can create 2 PR, one addressing the points mentioned here, and a second updating our driver. (The driver API didn't change, but the driver was not functional)

As there is currently only one hwspinlock driver in the tree, I'd suggest submitting these changes in one PR.

[Fymyte]

Went ahead and pushed all my commits in a single PR. #92738