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

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

[Xuyang Dong]

> 
> On Thu, Apr 30, 2026 at 01:58:58PM +0800, Xuyang Dong wrote:
> > > 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.
> > > 
> > 
> > Hi Stephen and Brian,
> > 
> > In the HSP clock driver, hsp_clk_gate_endisable() only accesses the 
> > registers at 0x800/0x900, and reset accesses the same registers as well, 
> > which leads to concurrent RMW (read-modify-write) races.
> > 
> > There are two approaches to solve these races.
> > 
> > The first method is the current implementation. All three functions 
> > (clk_gate_endisable(), hsp_clk_gate_endisable(), 
> > and eic7700_hsp_reset_assert()) use data->lock to prevent concurrent 
> > RMW races.
> > 
> > The second method is as Stephen said. If I understand correctly, it is to 
> > change the register read/write operations in hsp_clk_gate_endisable() to 
> > use the regmap API and use the same lock (map->raw_spinlock) as reset.
> > 
> > Is the second approach preferable?
> 
> Use the same regmap everywhere. Also you don't have to explicitly define
> the raw spinlock in your driver since the regmap API will create a raw
> spinlock for you if you use the fast_io / use_raw_spinlock options I
> described above.
> 

Hi Brian,

Thank you for your time and comments. Sorry for the late reply.
This will be fixed in the next version.

Best regards,
Xuyang Dong