Re: [PATCH RESEND v3 4/4] clk: eyeq: add driver

[Théo Lebrun]

Hello Stephen,

Only answering to questions to which I have answers.
The rest is addressed for next revision.

On Wed Sep 18, 2024 at 7:28 AM CEST, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-07-30 09:04:46)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 3e9099504fad..31f48edf855c 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -218,6 +218,19 @@ config COMMON_CLK_EN7523
> >           This driver provides the fixed clocks and gates present on Airoha
> >           ARM silicon.
> >  
> > +config COMMON_CLK_EYEQ
> > +       bool "Clock driver for the Mobileye EyeQ platform"
> > +       depends on 64BIT # for readq()
> > +       depends on OF || COMPILE_TEST
>
> What's the OF build dependency? If there isn't one please remove this
> line.

There is none (at least we have a compat layer that means we can build
even with CONFIG_OF=n). Removed in next revision.

>
> > +               *mult = *mult * 0x100000 + FIELD_GET(PCSR1_FRAC_IN, r1);
> > +       }
> > +
> > +       if (!*mult || !*div)
> > +               return -EINVAL;
> > +
> > +       /* Spread spectrum. */
> > +       if (!(r1 & (PCSR1_RESET | PCSR1_DIS_SSCG))) {
> > +               /*
> > +                * Spread is 1/1000 parts of frequency, accuracy is half of
> > +                * that. To get accuracy, convert to ppb (parts per billion).
> > +                */
> > +               u32 spread = FIELD_GET(PCSR1_SPREAD, r1);
> > +
> > +               *acc = spread * 500000;
>
> Where does 500000 come from? Half a billion?

In addition to the above comment, I added this to clarify:

	 * acc = spread * 1e6 / 2
	 *   with acc in parts per billion and,
	 *        spread in parts per thousand.

[...]

> > +
> > +static const struct eqc_early_match_data eqc_eyeq6h_central_early_match_data = {
> > +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq6h_central_early_plls),
> > +       .early_plls             = eqc_eyeq6h_central_early_plls,
> > +       .nb_late_clks = 0,
> > +};
> > +
> > +/* Required early for UART. */
>
> Is this required for earlycon? Where is the UART not a device driver
> that needs to get clks early?

The UART is PL011. It is an AMBA device, those get probed before
platform devices by of_platform_bus_create(). "pll-per" must be
available at that time.

[...]

> > +static void __init eqc_init(struct device_node *np)
[...]
> > +       spin_lock(&eqc_list_slock);
>
> I don't see how the spinlock provides any value. This function will run
> before any struct devices have been created.

Indeed no clash can happen. Will remove.

>
> > +       list_add_tail(&priv->list, &eqc_list);
>
> The list is also kind of unnecessary. Set a bool in the match_data and
> move on? We could have some sort of static_assert() check to make sure
> if there's a CLK_OF_DECLARE_DRIVER() then the bool is set in the
> match_data for the driver. Such a design is cheaper than taking a lock,
> adding to a list.

This list's main goal is not to know what was early-inited. Its only
reason for existence is that we want to get, at eqc_probe(), the cells
pointer allocated at eqc_init().

struct eqc_priv {
	/* this field is why we store priv inside a linked list: */
	struct clk_hw_onecell_data	*cells;
	/* the rest, we don't care much: */
	const struct eqc_early_match_data *early_data;
	const struct eqc_match_data	*data;
	void __iomem			*base;
	struct device_node		*np;
	struct list_head		list;
};

I do not see how to do that with a bool. We could put the pointer into
the match data, but that would mean we'd have to make them writable
(currently static const data). We are talking about a linked list with
two items in the worst case (EyeQ6H), accessed twice.

The reason we store the whole of priv: simpler code and we avoid mapping
registers twice (once at eqc_init() and once at eqc_probe()).

Can you confirm the current static linked list approach (without any
spinlock) will be good for next revision?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com