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

Brian Masney
2026-04-30T22:51:30Z

Hi Xuyang,

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.

Brian