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

[Brian Masney]

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