Re: Re: Re: Re: [PATCH v3 2/3] clk: eswin: Add eic7700 HSP clock driver

Brian Masney
2026-04-29T13:54:29Z

Hi Xuyang,

On Wed, Apr 29, 2026 at 05:38:51PM +0800, Xuyang Dong wrote:
> > >
> > > The common gate API, the HSP private API, and the reset driver all access
> > > the same register space.
> > > Therefore, they need to be protected by the same data->lock.
> > >
> >
> > If everything is accessing registers through regmap why aren't we using
> > the builtin lock with struct regmap_config::use_raw_spinlock? I don't
> > understand why we're rolling our own here.
>
> Hi Stephen,
>
> In the HSP clock driver and reset driver, there are three components that
> access the HSP register space: a common gate clock, a custom gate clock
> (i.e., 0x800), and a reset.
>
> 1. The common gate uses eswin_clk_register_gate() to register a gate clock
> via devm_clk_hw_register_gate_parent_data(). It accesses the register
> using clk_gate_endisable().
>
> static void clk_gate_endisable(struct clk_hw *hw, int enable)
> {
> struct clk_gate *gate = to_clk_gate(hw);
> unsigned long flags;
>
> if (gate->lock)
> spin_lock_irqsave(gate->lock, flags);
> else
> __acquire(gate->lock);
> ...
> if (gate->lock)
> spin_unlock_irqrestore(gate->lock, flags);
> else
> __release(gate->lock);
> }
>
> The gate->lock in use is the data->lock passed in from the clock driver.
>
> 2. The custom gate uses hsp_clk_register_gate() to register a gate clock.
> It accesses the register using hsp_clk_gate_endisable().
>
> static void hsp_clk_gate_endisable(struct clk_hw *hw, int enable)
> {
> struct eic7700_hsp_clk_gate *gate = to_gate_clk(hw);
>
> guard(spinlock_irqsave)(gate->lock);
> ...
> }
>
> The gate->lock in use is the same data->lock passed in from the clock
> driver.
>
> 3. The reset uses eic7700_hsp_reset_assert() and
> eic7700_hsp_reset_deassert(), which call regmap_assign_bits() to access
> the register.
>
> All three methods access the same register space; therefore, they must be
> protected by the same lock (data->lock).
>
> That's why we introduced eic7700_hsp_regmap_lock/unlock for
> eic7700_hsp_regmap_config.
> eic7700_hsp_regmap_config = {
> .lock = eic7700_hsp_regmap_lock,
> .unlock = eic7700_hsp_regmap_unlock,
> .lock_arg = lock_ctx,
> };
>
> The 'lock_ctx->lock' in eic7700_hsp_regmap_lock/unlock is the 'data->lock'.
> static void eic7700_hsp_regmap_lock(void *arg)
> __acquires(lock_ctx->lock)
> {
> struct eic7700_hsp_regmap_lock *const lock_ctx = arg;
> unsigned long flags;
>
> spin_lock_irqsave(lock_ctx->lock, flags);
> lock_ctx->flags = flags;
> }
>
> The similar approach can be found in clk-imx8ulp-sim-lpav.c.
>
> The annotations what we mentioned previously is the above
> "__acquires(lock_ctx->lock)".

I see what Stephen is saying. Take a look at __regmap_init() in
drivers/base/regmap/regmap.c. If the lock/unlock ops are not specified,
then the final else will automatically setup locking. By default, it'll
use a mutex, but there is the ability to use a spinlock.

So you can drop the lock/unlock ops from the driver, and add to the ops:

fast_io: 1,
use_raw_spinlock: 1,

Given the critcal nature of clks, I agree with Stephen that a raw
spinlock should be used here.

Brian