Message-ID: <586966c515e15f455973e7c55bd3ac5e.sboyd@kernel.org>
Date: Tue, 17 Sep 2024 22:28:53 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael Turquette <mturquette@...libre.com>, Rob Herring <robh@...nel.org>, Théo Lebrun <theo.lebrun@...tlin.com>
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>, Grégory Clement <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, Tawfik Bayouk <tawfik.bayouk@...ileye.com>, Théo Lebrun <theo.lebrun@...tlin.com>
Subject: Re: [PATCH RESEND v3 4/4] clk: eyeq: add driver

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.

> +       depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
> +       select AUXILIARY_BUS
> +       default MACH_EYEQ5 || MACH_EYEQ6H
> +       help
> +         This driver provides clocks found on Mobileye EyeQ5, EyeQ6L and Eye6H
> +         SoCs. Controllers live in shared register regions called OLB. Driver
> +         provides read-only PLLs, derived from the main crystal clock (which
> +         must be constant). It also exposes some divider clocks.
> +
> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> new file mode 100644
> index 000000000000..dbf912aa1217
> --- /dev/null
> +++ b/drivers/clk/clk-eyeq.c
> @@ -0,0 +1,793 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[...]
> +
> +static int eqc_pll_parse_registers(u32 r0, u32 r1, unsigned long *mult,
> +                                  unsigned long *div, unsigned long *acc)
> +{
> +       if (r0 & PCSR0_BYPASS) {
> +               *mult = 1;
> +               *div = 1;
> +               *acc = 0;
> +               return 0;
> +       }
> +
> +       if (!(r0 & PCSR0_PLL_LOCKED))
> +               return -EINVAL;
> +
> +       *mult = FIELD_GET(PCSR0_INTIN, r0);
> +       *div = FIELD_GET(PCSR0_REF_DIV, r0);
> +       if (r0 & PCSR0_FOUTPOSTDIV_EN)
> +               *div *= FIELD_GET(PCSR0_POST_DIV1, r0) * FIELD_GET(PCSR0_POST_DIV2, r0);
> +
> +       /* Fractional mode, in 2^20 (0x100000) parts. */
> +       if (r0 & PCSR0_DSM_EN) {
> +               *div *= 0x100000;

I'd think 1 << 20 is more idiomatic to represent 2^20 because we don't
have to count the zeroes to make sure this is right.

> +               *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?

> +               if (r1 & PCSR1_DOWN_SPREAD) {
> +                       /*
> +                        * Downspreading: the central frequency is half a
> +                        * spread lower.
> +                        */
> +                       *mult *= 2000 - spread;
> +                       *div *= 2000;
> +
> +                       /*
> +                        * Previous operation might overflow 32 bits. If it
> +                        * does, throw away the least amount of low bits.
> +                        */
> +                       eqc_pll_downshift_factors(mult, div);
> +               }
> +       } else {
> +               *acc = 0;

I'd de-indent by inverting this and returning early when *acc = 0.

> +       }
> +
> +       return 0;
> +}
> +
[...]
> +
> +static const struct of_device_id eqc_match_table[] = {
> +       { .compatible = "mobileye,eyeq5-olb", .data = &eqc_eyeq5_match_data },
> +       { .compatible = "mobileye,eyeq6l-olb", .data = &eqc_eyeq6l_match_data },
> +       { .compatible = "mobileye,eyeq6h-west-olb", .data = &eqc_eyeq6h_west_match_data },
> +       { .compatible = "mobileye,eyeq6h-east-olb", .data = &eqc_eyeq6h_east_match_data },
> +       { .compatible = "mobileye,eyeq6h-south-olb", .data = &eqc_eyeq6h_south_match_data },
> +       { .compatible = "mobileye,eyeq6h-ddr0-olb", .data = &eqc_eyeq6h_ddr0_match_data },
> +       { .compatible = "mobileye,eyeq6h-ddr1-olb", .data = &eqc_eyeq6h_ddr1_match_data },
> +       { .compatible = "mobileye,eyeq6h-acc-olb", .data = &eqc_eyeq6h_acc_match_data },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, eqc_match_table);

Module device table should be removed as this can't be a module.

> +
> +static struct platform_driver eqc_driver = {
> +       .probe = eqc_probe,
> +       .driver = {
> +               .name = "clk-eyeq",
> +               .of_match_table = eqc_match_table,
> +       },
> +};
> +builtin_platform_driver(eqc_driver);
> +
> +/* Required early for GIC timer (pll-cpu) and UARTs (pll-per). */
> +static const struct eqc_pll eqc_eyeq5_early_plls[] = {
> +       { .index = EQ5C_PLL_CPU, .name = "pll-cpu",  .reg64 = 0x02C },
> +       { .index = EQ5C_PLL_PER, .name = "pll-per",  .reg64 = 0x05C },
> +};
> +
> +static const struct eqc_early_match_data eqc_eyeq5_early_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq5_early_plls),
> +       .early_plls             = eqc_eyeq5_early_plls,
> +       .nb_late_clks = eqc_eyeq5_match_data.pll_count + eqc_eyeq5_match_data.div_count,
> +};
> +
> +/* Required early for GIC timer. */
> +static const struct eqc_pll eqc_eyeq6h_central_early_plls[] = {
> +       { .index = 0, .name = "pll-cpu", .reg64 = 0x02C },
> +};
> +
> +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?

> +static const struct eqc_pll eqc_eyeq6h_west_early_plls[] = {
> +       { .index = 0, .name = "pll-west", .reg64 = 0x074 },
> +};
> +
> +static const struct eqc_early_match_data eqc_eyeq6h_west_early_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq6h_west_early_plls),
> +       .early_plls             = eqc_eyeq6h_west_early_plls,
> +       .nb_late_clks = 0,
> +};
> +
> +static const struct of_device_id eqc_early_match_table[] = {

Can be __initconst

> +       {
> +               .compatible = "mobileye,eyeq5-olb",
> +               .data = &eqc_eyeq5_early_match_data,
> +       },
> +       {
> +               .compatible = "mobileye,eyeq6h-central-olb",
> +               .data = &eqc_eyeq6h_central_early_match_data,
> +       },
> +       {
> +               .compatible = "mobileye,eyeq6h-west-olb",
> +               .data = &eqc_eyeq6h_west_early_match_data,
> +       },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, eqc_early_match_table);

This isn't needed because this isn't a module.

> +
> +static void __init eqc_init(struct device_node *np)
> +{
> +       const struct eqc_early_match_data *early_data;
> +       const struct of_device_id *early_match_data;
> +       unsigned int nb_clks = 0;

Drop useless assignment please.

> +       struct eqc_priv *priv;
> +       void __iomem *base;
> +       unsigned int i;
> +       int ret;
> +
> +       early_match_data = of_match_node(eqc_early_match_table, np);

We've already matched before calling this function. I'd prefer a wrapper

	static void __init eqc_init(struct device_node *np, const struct eqc_early_match_data *early_data)
	{
	}

	static void __init eqc_eyeq5_early_init(struct device_node *np)
	{
		eqc_init(np, &eqc_eyeq5_early_match_data);
	}

then we don't need a match table, and skip the string comparison again.

> +       if (!early_match_data)
> +               return;
> +
> +       early_data = early_match_data->data;
> +       /* No reason to early init this clock provider. Delay until probe. */
> +       if (!early_data || early_data->early_pll_count == 0)
> +               return;

I suspect this code can be removed too because it's purely defensive.

> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       priv->np = np;
> +       priv->early_data = early_data;
> +
> +       nb_clks = early_data->early_pll_count + early_data->nb_late_clks;
> +       priv->cells = kzalloc(struct_size(priv->cells, hws, nb_clks), GFP_KERNEL);
> +       if (!priv->cells) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       priv->cells->num = nb_clks;
> +
> +       /*
> +        * Mark all clocks as deferred; some are registered here, the rest at
> +        * platform device probe.
> +        */
> +       for (i = 0; i < nb_clks; i++)
> +               priv->cells->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> +       /* Offsets (reg64) of early PLLs are relative to OLB block. */
> +       base = of_iomap(np, 0);
> +       if (!base) {
> +               ret = -ENODEV;
> +               goto err;
> +       }
> +
> +       for (i = 0; i < early_data->early_pll_count; i++) {
> +               const struct eqc_pll *pll = &early_data->early_plls[i];
> +               unsigned long mult, div, acc;
> +               struct clk_hw *hw;
> +               u32 r0, r1;
> +               u64 val;
> +
> +               val = readq(base + pll->reg64);
> +               r0 = val;
> +               r1 = val >> 32;
> +
> +               ret = eqc_pll_parse_registers(r0, r1, &mult, &div, &acc);
> +               if (ret) {
> +                       pr_err("failed parsing state of %s\n", pll->name);
> +                       goto err;
> +               }
> +
> +               hw = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
> +                               np, pll->name, "ref", 0, mult, div, acc);
> +               priv->cells->hws[pll->index] = hw;
> +               if (IS_ERR(hw)) {
> +                       pr_err("failed registering %s: %pe\n", pll->name, hw);
> +                       ret = PTR_ERR(hw);
> +                       goto err;
> +               }
> +       }
> +
> +       /* When providing a single clock, require no cell. */
> +       if (nb_clks == 1)
> +               ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, priv->cells->hws[0]);
> +       else
> +               ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, priv->cells);
> +       if (ret) {
> +               pr_err("failed registering clk provider: %d\n", ret);
> +               goto err;
> +       }
> +
> +       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.

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

> +       spin_unlock(&eqc_list_slock);
> +
> +       return;
> +

Powered by blists - more mailing lists