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

["Xuyang Dong" <dongxuyang@eswincomputing.com>]

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

Best regards,
Xuyang Dong