Re: [PATCH v6 08/15] clk: starfive: Introduce idiv ops for inverted divider clocks

[Jerome Brunet <jbrunet@baylibre.com>]

On 2026-10-14 03:16 +0000, Changhuang Liang wrote:
> Hi, Jerome
> 
> > > Some StarFive clocks only advertise the divider and invert
> > > capabilities (via JH71X0_CLK_INVERT, without ENABLE). Add the
> > > JH71X0_IDIV() macro to declare such clocks, and jh71x0_clk_idiv_ops,
> > > which combines the divider rate operations with get_phase/set_phase.
> > >
> > 
> > Please rephrase, I don't understand what you mean
> > 
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  drivers/clk/starfive/clk-starfive-jh71x0.c | 13 ++++++++++++-
> > > drivers/clk/starfive/clk-starfive-jh71x0.h |  8 ++++++++
> > >  2 files changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.c
> > > b/drivers/clk/starfive/clk-starfive-jh71x0.c
> > > index c5ee82db3c92..224fcc3d08db 100644
> > > --- a/drivers/clk/starfive/clk-starfive-jh71x0.c
> > > +++ b/drivers/clk/starfive/clk-starfive-jh71x0.c
> > > @@ -303,6 +303,15 @@ static const struct clk_ops jh71x0_clk_inv_ops = {
> > >  	.debug_init = jh71x0_clk_debug_init,  };
> > >
> > > +static const struct clk_ops jh71x0_clk_idiv_ops = {
> > > +	.get_phase = jh71x0_clk_get_phase,
> > > +	.set_phase = jh71x0_clk_set_phase,
> > > +	.recalc_rate = jh71x0_clk_recalc_rate,
> > > +	.determine_rate = jh71x0_clk_determine_rate,
> > > +	.set_rate = jh71x0_clk_set_rate,
> > > +	.debug_init = jh71x0_clk_debug_init,
> > 
> > Instead of trying to cover all the combination that could exist, can you just
> > chain 2 elements ?
> > a div then an inv ?
> 
> This may require introducing quite a few new changes.
> 
> In the current code, struct clk_hw *hw is used to obtain struct jh71x0_clk *clk through hw, and then the register offset is obtained through clk->idx.
> 
> static void jh71x0_clk_reg_rmw(struct jh71x0_clk *clk, u32 mask, u32 value)
> {
> 	struct jh71x0_clk_priv *priv = jh71x0_priv_from(clk);
> 	void __iomem *reg = priv->base + 4 * clk->idx;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&priv->rmw_lock, flags);
> 	value |= readl_relaxed(reg) & ~mask;
> 	writel_relaxed(value, reg);
> 	spin_unlock_irqrestore(&priv->rmw_lock, flags);
> }
> 
> static int jh71x0_clk_enable(struct clk_hw *hw)
> {
> 	struct jh71x0_clk *clk = jh71x0_clk_from(hw);
> 
> 	jh71x0_clk_reg_rmw(clk, JH71X0_CLK_ENABLE, JH71X0_CLK_ENABLE);
> 	return 0;
> }
> 
> Therefore, the existing code has one struct clk_hw corresponding to one register offset.
> 
> If it is split into a div and an inv, two struct clk_hw descriptions will be needed.
> The jh71x0_crg_probe() will need to implement different branches for different clocks:
> 
> if (a div and an inv) {
>     devm_clk_hw_register (div)
>     devm_clk_hw_register (inv)
> } else {
>     devm_clk_hw_register (other)
> }
> 
> Do you need me to make this part of the adjustment?

No, it is ok this time around but understand that if this patterns repeats, you will have to
fix this. Best would be to start thinking about it at least.

-- 
Jerome