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