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

[Xuyang Dong]

> > 
> > 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