Re: [PATCH v2 2/9] clk: ralink: add clock and reset driver for MTMIPS SoCs

[Stephen Boyd <sboyd@kernel.org>]

Quoting Sergio Paracuellos (2023-04-13 22:49:47)
> On Thu, Apr 13, 2023 at 8:55 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Sergio Paracuellos (2023-03-20 22:00:27)
> > > diff --git a/drivers/clk/ralink/clk-mtmips.c b/drivers/clk/ralink/clk-mtmips.c
> > > new file mode 100644
> > > index 000000000000..6b4b5ae9384d
> > > --- /dev/null
> > > +++ b/drivers/clk/ralink/clk-mtmips.c
> > > @@ -0,0 +1,985 @@
[...]
> >
> > > +               .name = _name,                                  \
> > > +               .ops = &(const struct clk_ops) {                \
> >
> > Make this into a named variable? Otherwise I suspect the compiler will
> > want to duplicate it.
> 
> I am not sure if I understand this. What do you mean exactly?

	static const struct clk_ops mtmips_periph_clk_ops = {
		.recalc_rate = mtmips_pherip_clk_rate,
	};

> > > +static unsigned long rt3352_bus_recalc_rate(struct clk_hw *hw,
> > > +                                           unsigned long parent_rate)
> > > +{
> > > +       return parent_rate / 3;
> > > +}
> > > +
> > > +static unsigned long rt305x_xtal_recalc_rate(struct clk_hw *hw,
> > > +                                            unsigned long parent_rate)
> > > +{
> > > +       return 40000000;
> > > +}
> >
> > Register fixed factor and fixed rate clks in software instead of
> > duplicating the code here.
> 
> All the macros used in current code rely on the fact of having recalc
> functions so we can maintain the code shorter just using them. Is
> there a real benefit of using a fixed factor and fixed clks here?
> If possible I can avoid the duplicate here just using the same
> recalc_rate function returning the fixed stuff for both 305x and 3352
> SoCs as I am also doing for other functions.

The real benefit is less code, smaller kernel size, less maintenance
over time.

> >
> > > +       }
> > > +}
> > > +
> > > +static unsigned long rt2880_cpu_recalc_rate(struct clk_hw *hw,
> > > +                                           unsigned long xtal_clk)
> > > +{
> > > +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> > > +       struct regmap *sysc = clk->priv->sysc;
> > > +       u32 t;
> > > +
> > > +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> > > +       t = (t >> RT2880_CONFIG_CPUCLK_SHIFT) & RT2880_CONFIG_CPUCLK_MASK;
> > > +
> > > +       switch (t) {
> > > +       case RT2880_CONFIG_CPUCLK_250:
> > > +               return 250000000;
> > > +       case RT2880_CONFIG_CPUCLK_266:
> > > +               return 266000000;
> > > +       case RT2880_CONFIG_CPUCLK_280:
> > > +               return 280000000;
> > > +       case RT2880_CONFIG_CPUCLK_300:
> > > +               return 300000000;
> > > +       default:
> > > +               BUG();
> > > +       }
> > > +}
> > > +
> > > +static unsigned long rt2880_bus_recalc_rate(struct clk_hw *hw,
> > > +                                           unsigned long parent_rate)
> > > +{
> > > +       return parent_rate / 2;
> > > +}
> >
> > A fixed factor clk?
> 
> As I have said, macros rely on having recalc_rate functions. Also,
> having in this way makes pretty clear the relation between the bus
> clock and its related parent as it is in the datasheet.

The macros are your own design, right? In which case, maybe you can use
CLK_HW_INIT() and friends macros instead to show the relationship
between clks in C code?

> 
> >
> > > +
> > > +static u32 mt7620_calc_rate(u32 ref_rate, u32 mul, u32 div)
> > > +{
> > > +       u64 t;
> > > +
> > > +       t = ref_rate;
> > > +       t *= mul;
> > > +       do_div(t, div);
> >
> > Do we really need to do 64-bit math? At the least use div_u64().
> 
> This is directly extracted from arch/mips/ralink clock code, so I have
> maintained it as it is since I don't have an mt7620 SoC based board to
> test. However using div_u64 here with t being u64 makes sense.

Does anyone have the board to test? Can we simply delete it instead?

> > > +
> > > +static unsigned long mt7620_bus_recalc_rate(struct clk_hw *hw,
> > > +                                           unsigned long parent_rate)
> > > +{
> > > +       static const u32 ocp_dividers[16] = {
> > > +               [CPU_SYS_CLKCFG_OCP_RATIO_2] = 2,
> > > +               [CPU_SYS_CLKCFG_OCP_RATIO_3] = 3,
> > > +               [CPU_SYS_CLKCFG_OCP_RATIO_4] = 4,
> > > +               [CPU_SYS_CLKCFG_OCP_RATIO_5] = 5,
> > > +               [CPU_SYS_CLKCFG_OCP_RATIO_10] = 10,
> > > +       };
> > > +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> > > +       struct regmap *sysc = clk->priv->sysc;
> > > +       u32 t;
> > > +       u32 ocp_ratio;
> > > +       u32 div;
> > > +
> > > +       if (IS_ENABLED(CONFIG_USB)) {
> > > +               /*
> > > +               * When the CPU goes into sleep mode, the BUS
> > > +               * clock will be too low for USB to function properly.
> > > +               * Adjust the busses fractional divider to fix this
> > > +               */
> > > +               regmap_read(sysc, SYSC_REG_CPU_SYS_CLKCFG, &t);
> > > +               t &= ~(CLKCFG_FDIV_MASK | CLKCFG_FFRAC_MASK);
> > > +               t |= CLKCFG_FDIV_USB_VAL | CLKCFG_FFRAC_USB_VAL;
> > > +               regmap_write(sysc, SYSC_REG_CPU_SYS_CLKCFG, t);
> >
> > Why can't we do this unconditionally? And recalc_rate() shouldn't be
> > writing registers. It should be calculating the frequency of the clk
> > based on 'parent_rate' and whatever the hardware is configured for.
> 
> This code is with IS_ENABLED(CONFIG_USB) guard in the original code so
> I have maintained it as it is. Where should it be moved into instead
> of doing the register writes in this recalc function?

Can you do it unconditionally in driver probe? Or when the clk is turned
off or on can you park it at a safe frequency?