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
