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

[Changhuang Liang]

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

> > +};
> > +
> >  const struct clk_ops *starfive_jh71x0_clk_ops(u32 max)  {
> >  	if (max & JH71X0_CLK_DIV_MASK) {
> > @@ -313,7 +322,9 @@ const struct clk_ops *starfive_jh71x0_clk_ops(u32
> max)
> >  		}
> >  		if (max & JH71X0_CLK_ENABLE)
> >  			return &jh71x0_clk_gdiv_ops;
> > -		if (max == JH71X0_CLK_FRAC_MAX)
> > +		else if (max & JH71X0_CLK_INVERT)
> > +			return &jh71x0_clk_idiv_ops;
> > +		else if (max == JH71X0_CLK_FRAC_MAX)
> >  			return &jh71x0_clk_fdiv_ops;
> 
> Given the branches return, if was enough and you did not need to modify the
> existing path.
> 

Best Regards,
Changhuang