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

Xuyang Dong
2026-04-29T09:39:05Z

> >
> > 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)".

Best regards,
Xuyang Dong