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

[Andrea della Porta <andrea.porta@xxxxxxxx>]

Hi Stephen,

On 14:52 Wed 23 Oct     , Stephen Boyd wrote:
> 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?

AFAICT there's no commercially available PCI card slot sporting an RP1 on-board,
but this driver should be used to probe and use that card too.

> Is the iomem going to be exposed through some PCI config space?

Yes, just as leverage in this driver through BAR1.

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

I see, Herve proposed CONFIG_MISC_RP1 that is enabled whenever this driver is
selected, and it makes a lot of sense to me.

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

Ack.

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

Ack.

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

Ack.

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

Ack.

Many thanks,
Andrea