Re: [PATCH v2 08/14] clk: rp1: Add support for clocks provided by RP1

[Stephen Boyd <sboyd@xxxxxxxxxx>]

Quoting Andrea della Porta (2024-10-23 08:36:06)
> Hi Stephen,
> 
> On 15:08 Wed 09 Oct     , Stephen Boyd wrote:
> > Quoting Andrea della Porta (2024-10-07 05:39:51)
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > index 299bc678ed1b..537019987f0c 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -88,6 +88,15 @@ config COMMON_CLK_RK808
> > >           These multi-function devices have two fixed-rate oscillators, clocked at 32KHz each.
> > >           Clkout1 is always on, Clkout2 can off by control register.
> > >  
> > > +config COMMON_CLK_RP1
> > > +       tristate "Raspberry Pi RP1-based clock support"
> > > +       depends on PCI || COMPILE_TEST
> > 
> > A better limit would be some ARCH_* config.
> 
> I've avoided ARCH_BCM2835 since the original intention is for this driver
> to work (in the future) also for custom PCI cards with RP1 on-board, and not
> only for Rpi5.

How will that custom PCI card work? It will need this driver to probe?
Is the iomem going to be exposed through some PCI config space?

It's not great to depend on CONFIG_PCI because then the driver is forced
to be =m if PCI ever becomes tristate (unlikely, but still makes for bad
copy/pasta). I understand this line is trying to limit the availability
of the config symbol. Maybe it should simply depend on ARM or ARM64? Or
on nothing at all.

> 
> > > diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> > > new file mode 100644
> > > index 000000000000..9016666fb27d
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-rp1.c
> > 
> > > +#include <linux/clk.h>
> > 
> > Preferably this include isn't included.
> 
> This include is currently needed by devm_clk_get_enabled() to retrieve
> the xosc. Since that clock is based on a crystal (so it's fixed and
> always enabled), I'm planning to hardcode it in the driver. This will
> not only get rid of the devm_clk_get_enabled() call (and hence of the
> clk.h include), but it'll also simplify the top devicetree. No promise
> though, I need to check a couple of things first.

A clk provider (clk-provider.h) should ideally not be a clk consumer
(clk.h).

> 
> 
> > > +
> > > +static int rp1_pll_ph_set_rate(struct clk_hw *hw,
> > > +                              unsigned long rate, unsigned long parent_rate)
> > > +{
> > > +       struct rp1_pll_ph *pll_ph = container_of(hw, struct rp1_pll_ph, hw);
> > > +       const struct rp1_pll_ph_data *data = pll_ph->data;
> > > +
> > > +       /* Nothing really to do here! */
> > 
> > Is it read-only? Don't define a set_rate function then and make the rate
> > determination function return the same value all the time.
> 
> Not 100% sure about it, maybe Raspberry Pi colleagues can explain.
> By 'rate determination function' you're referring (in this case) to
> rp1_pll_ph_recalc_rate(), right?

Yes.

> If so, that clock type seems to have
> a fixed divider but teh resulting clock depends on the parent rate, so
> it has to be calculated.

Sure, it has to be calculated, but it will return the rate that causes
no change to the hardware. When that happens, the set_rate() op should
be skipped, and you can see that with clk_divider_ro_ops not having a
set_rate() function pointer.

> 
> > > +static int rp1_clock_determine_rate(struct clk_hw *hw,
> > > +                                   struct clk_rate_request *req)
> > > +{
> > > +       struct clk_hw *parent, *best_parent = NULL;
> > > +       unsigned long best_rate = 0;
> > > +       unsigned long best_prate = 0;
> > > +       unsigned long best_rate_diff = ULONG_MAX;
> > > +       unsigned long prate, calc_rate;
> > > +       size_t i;
> > > +
> > > +       /*
> > > +        * If the NO_REPARENT flag is set, try to use existing parent.
> > > +        */
> > > +       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
> > 
> > Is this flag ever set?
> 
> Not right now, but it will be used as soon as I'll add the video clocks,
> so I thought to leave it be to avoid adding it back in the future.
> For this minimal support is not needed though, so let me know if you
> want it removed.
> 

Ok sure.

> 
> > > +
> > > +       [RP1_CLK_ETH_TSU] = REGISTER_CLK(.name = "clk_eth_tsu",
> > > +                               .parents = {"rp1-xosc"},
> > > +                               .num_std_parents = 0,
> > > +                               .num_aux_parents = 1,
> > > +                               .ctrl_reg = CLK_ETH_TSU_CTRL,
> > > +                               .div_int_reg = CLK_ETH_TSU_DIV_INT,
> > > +                               .sel_reg = CLK_ETH_TSU_SEL,
> > > +                               .div_int_max = DIV_INT_8BIT_MAX,
> > > +                               .max_freq = 50 * MHz,
> > > +                               .fc0_src = FC_NUM(5, 7),
> > > +                               ),
> > > +
> > > +       [RP1_CLK_SYS] = REGISTER_CLK(.name = "clk_sys",
> > > +                               .parents = {"rp1-xosc", "-", "pll_sys"},
> > 
> > Please use struct clk_parent_data or clk_hw directly. Don't use strings
> > to describe parents.
> 
> Describing parents as as strings allows to directly assign it to struct
> clk_init_data, as in rp1_register_clock():
> 
> const struct rp1_clock_data *clock_data = data;
> struct clk_init_data init = { };
> ...
> init.parent_names = clock_data->parents;
> 
> otherwise we should create an array and populate from clk_parent_data::name,
> which is of course feasible but a bit less compact. Are you sure you want
> to change it?
> 

Do not use strings to describe parents. That's the guiding principle
here. I agree using strings certainly makes it easy to describe things
but that doesn't mean it is acceptable.

> > > +       struct clk *clk_xosc;
> > > +       struct clk_hw **hws;
> > > +       unsigned int i;
> > > +
> > > +       clockman = devm_kzalloc(dev, struct_size(clockman, onecell.hws, asize),
> > > +                               GFP_KERNEL);
> > > +       if (!clockman)
> > > +               return -ENOMEM;
> > > +
> > > +       spin_lock_init(&clockman->regs_lock);
> > > +       clockman->dev = dev;
> > > +
> > > +       clockman->regs = devm_platform_ioremap_resource(pdev, 0);
> > > +       if (IS_ERR(clockman->regs))
> > > +               return PTR_ERR(clockman->regs);
> > > +
> > > +       clk_xosc = devm_clk_get_enabled(dev, NULL);
> > > +       if (IS_ERR(clk_xosc))
> > > +               return PTR_ERR(clk_xosc);
> > > +
> > > +       clockman->hw_xosc = __clk_get_hw(clk_xosc);
> > 
> > Please use struct clk_parent_data::index instead.
> 
> Sorry, I didn't catch what you mean here. Can you please elaborate?
> 

Don't use __clk_get_hw() at all. Also, don't use clk_get() and friends
in clk provider drivers. Use struct clk_parent_data so that the
framework can do the work for you at the right time.