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