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

[Sergio Paracuellos]

On Tue, Apr 18, 2023 at 2:50 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting Sergio Paracuellos (2023-04-13 22:49:47)
> > On Thu, Apr 13, 2023 at 8:55 PM Stephen Boyd <sboyd@xxxxxxxxxx> 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,
>         };
>

Ah, I see. Thanks. It is clear now.

> > > > +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.

Understood. Will change to use fixed and factor clocks then.

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

I'll take a look at those macros also and will take them into account, thanks.

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

I have mt7628 and rt5350 based boards where I am testing these
changes. However I don't have mt7620 which is the one needed for this
particular code.
These SoCs are commonly used in the openWRT community so we cannot
delete this code. I will use div_u64 for the next version which makes
sense. If someone complains about something wrong with the change at
some time we can just change it again in the future.

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

Sure, thanks.

I'll address all your comments and send v3, shortly.

Thanks,
    Sergio Paracuellos