Add the Mobileye EyeQ5 clock controller driver. It might grow to add
support for other platforms from Mobileye.
It handles 10 read-only PLLs derived from the main crystal on board. It
exposes a table-based divider clock used for OSPI. Other platform
clocks are not configurable and therefore kept as fixed-factor
devicetree nodes.
Two PLLs are required early on and are therefore registered at
of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
UARTs.
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
drivers/clk/Kconfig | 11 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-eyeq5.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 318 insertions(+)
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 50af5fc7f570..c79b38f60b1b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -218,6 +218,17 @@ config COMMON_CLK_EN7523
This driver provides the fixed clocks and gates present on Airoha
ARM silicon.
+config COMMON_CLK_EYEQ5
+ bool "Clock driver for the Mobileye EyeQ5 platform"
+ depends on OF
+ depends on MACH_EYEQ5 || COMPILE_TEST
+ default MACH_EYEQ5
+ help
+ This driver provides the clocks found on the Mobileye EyeQ5 SoC. Its
+ registers live in a shared register region called OLB. It provides 10
+ read-only PLLs derived from the main crystal clock which must be constant
+ and one divider clock based on one PLL.
+
config COMMON_CLK_FSL_FLEXSPI
tristate "Clock driver for FlexSPI on Layerscape SoCs"
depends on ARCH_LAYERSCAPE || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 14fa8d4ecc1f..81c4d11ca437 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o
obj-$(CONFIG_COMMON_CLK_CS2000_CP) += clk-cs2000-cp.o
obj-$(CONFIG_ARCH_SPARX5) += clk-sparx5.o
obj-$(CONFIG_COMMON_CLK_EN7523) += clk-en7523.o
+obj-$(CONFIG_COMMON_CLK_EYEQ5) += clk-eyeq5.o
obj-$(CONFIG_COMMON_CLK_FIXED_MMIO) += clk-fixed-mmio.o
obj-$(CONFIG_COMMON_CLK_FSL_FLEXSPI) += clk-fsl-flexspi.o
obj-$(CONFIG_COMMON_CLK_FSL_SAI) += clk-fsl-sai.o
diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c
new file mode 100644
index 000000000000..85bf6f1c3fa3
--- /dev/null
+++ b/drivers/clk/clk-eyeq5.c
@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PLL clock driver for the Mobileye EyeQ5 platform.
+ *
+ * This controller handles 10 read-only PLLs, all derived from the same main
+ * crystal clock. It also exposes one divider clock, a child of one of the
+ * PLLs. The parent clock is expected to be constant. This driver's registers
+ * live in a shared region called OLB. Two PLLs must be initialized by
+ * of_clk_init().
+ *
+ * We use eq5c_ as prefix, as-in "EyeQ5 Clock", but way shorter.
+ *
+ * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
+ */
+
+#define pr_fmt(fmt) "clk-eyeq5: " fmt
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <dt-bindings/clock/mobileye,eyeq5-clk.h>
+
+/* In frac mode, it enables fractional noise canceling DAC. Else, no function. */
+#define PCSR0_DAC_EN BIT(0)
+/* Fractional or integer mode */
+#define PCSR0_DSM_EN BIT(1)
+#define PCSR0_PLL_EN BIT(2)
+/* All clocks output held at 0 */
+#define PCSR0_FOUTPOSTDIV_EN BIT(3)
+#define PCSR0_POST_DIV1 GENMASK(6, 4)
+#define PCSR0_POST_DIV2 GENMASK(9, 7)
+#define PCSR0_REF_DIV GENMASK(15, 10)
+#define PCSR0_INTIN GENMASK(27, 16)
+#define PCSR0_BYPASS BIT(28)
+/* Bits 30..29 are reserved */
+#define PCSR0_PLL_LOCKED BIT(31)
+
+#define PCSR1_RESET BIT(0)
+#define PCSR1_SSGC_DIV GENMASK(4, 1)
+/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */
+#define PCSR1_SPREAD GENMASK(9, 5)
+#define PCSR1_DIS_SSCG BIT(10)
+/* Down-spread or center-spread */
+#define PCSR1_DOWN_SPREAD BIT(11)
+#define PCSR1_FRAC_IN GENMASK(31, 12)
+
+static struct clk_hw_onecell_data *eq5c_clk_data;
+
+struct eq5c_pll {
+ int index;
+ const char *name;
+ u32 reg; /* next 8 bytes are r0 and r1 */
+};
+
+/* Required early for the GIC timer (pll-cpu) and UARTs (pll-per). */
+static const struct eq5c_pll eq5c_early_plls[] = {
+ { .index = EQ5C_PLL_CPU, .name = "pll-cpu", .reg = 0x00, },
+ { .index = EQ5C_PLL_PER, .name = "pll-per", .reg = 0x30, },
+};
+
+static const struct eq5c_pll eq5c_plls[] = {
+ { .index = EQ5C_PLL_VMP, .name = "pll-vmp", .reg = 0x08, },
+ { .index = EQ5C_PLL_PMA, .name = "pll-pma", .reg = 0x10, },
+ { .index = EQ5C_PLL_VDI, .name = "pll-vdi", .reg = 0x18, },
+ { .index = EQ5C_PLL_DDR0, .name = "pll-ddr0", .reg = 0x20, },
+ { .index = EQ5C_PLL_PCI, .name = "pll-pci", .reg = 0x28, },
+ { .index = EQ5C_PLL_PMAC, .name = "pll-pmac", .reg = 0x38, },
+ { .index = EQ5C_PLL_MPC, .name = "pll-mpc", .reg = 0x40, },
+ { .index = EQ5C_PLL_DDR1, .name = "pll-ddr1", .reg = 0x48, },
+};
+
+#define EQ5C_OSPI_DIV_CLK_NAME "div-ospi"
+#define EQ5C_OSPI_DIV_WIDTH 4
+
+#define EQ5C_NB_CLKS (ARRAY_SIZE(eq5c_early_plls) + ARRAY_SIZE(eq5c_plls) + 1)
+
+static const struct clk_div_table eq5c_ospi_div_table[] = {
+ { .val = 0, .div = 2 },
+ { .val = 1, .div = 4 },
+ { .val = 2, .div = 6 },
+ { .val = 3, .div = 8 },
+ { .val = 4, .div = 10 },
+ { .val = 5, .div = 12 },
+ { .val = 6, .div = 14 },
+ { .val = 7, .div = 16 },
+ {} /* sentinel */
+};
+
+static int eq5c_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;
+ *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;
+ if (r1 & PCSR1_DOWN_SPREAD) {
+ /*
+ * Downspreading: the central frequency is half a
+ * spread lower.
+ */
+ *mult *= 2000 - spread;
+ *div *= 2000;
+ }
+ } else {
+ *acc = 0;
+ }
+
+ return 0;
+}
+
+static int eq5c_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ void __iomem *base_plls, *base_ospi;
+ struct clk_hw *hw;
+ int i;
+
+ /* Return potential error from eq5c_init(). */
+ if (IS_ERR(eq5c_clk_data))
+ return PTR_ERR(eq5c_clk_data);
+ /* Return an error if eq5c_init() did not get called. */
+ else if (!eq5c_clk_data)
+ return -EINVAL;
+
+ base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
+ if (IS_ERR(base_plls))
+ return PTR_ERR(base_plls);
+
+ base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
+ if (IS_ERR(base_ospi))
+ return PTR_ERR(base_ospi);
+
+ for (i = 0; i < ARRAY_SIZE(eq5c_plls); i++) {
+ const struct eq5c_pll *pll = &eq5c_plls[i];
+ unsigned long mult, div, acc;
+ u32 r0, r1;
+ int ret;
+
+ r0 = readl(base_plls + pll->reg);
+ r1 = readl(base_plls + pll->reg + sizeof(r0));
+
+ ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
+ if (ret) {
+ dev_warn(dev, "failed parsing state of %s\n", pll->name);
+ eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
+ continue;
+ }
+
+ hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
+ pll->name, "ref", 0, mult, div, acc);
+ eq5c_clk_data->hws[pll->index] = hw;
+ if (IS_ERR(hw))
+ dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
+ pll->name);
+ }
+
+ hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME,
+ eq5c_clk_data->hws[EQ5C_PLL_PER], 0,
+ base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0,
+ eq5c_ospi_div_table, NULL);
+ eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw;
+ if (IS_ERR(hw))
+ dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
+ EQ5C_OSPI_DIV_CLK_NAME);
+
+ return 0;
+}
+
+static const struct of_device_id eq5c_match_table[] = {
+ { .compatible = "mobileye,eyeq5-clk" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, eq5c_match_table);
+
+static struct platform_driver eq5c_driver = {
+ .probe = eq5c_probe,
+ .driver = {
+ .name = "clk-eyeq5",
+ .of_match_table = eq5c_match_table,
+ },
+};
+builtin_platform_driver(eq5c_driver);
+
+static void __init eq5c_init(struct device_node *np)
+{
+ void __iomem *base_plls, *base_ospi;
+ int index_plls, index_ospi;
+ int i, ret;
+
+ eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
+ GFP_KERNEL);
+ if (!eq5c_clk_data) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ eq5c_clk_data->num = EQ5C_NB_CLKS;
+
+ /*
+ * Mark all clocks as deferred. We register some now and others at
+ * platform device probe.
+ */
+ for (i = 0; i < EQ5C_NB_CLKS; i++)
+ eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+ index_plls = of_property_match_string(np, "reg-names", "plls");
+ if (index_plls < 0) {
+ ret = index_plls;
+ goto err;
+ }
+
+ index_ospi = of_property_match_string(np, "reg-names", "ospi");
+ if (index_ospi < 0) {
+ ret = index_ospi;
+ goto err;
+ }
+
+ base_plls = of_iomap(np, index_plls);
+ base_ospi = of_iomap(np, index_ospi);
+ if (!base_plls || !base_ospi) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(eq5c_early_plls); i++) {
+ const struct eq5c_pll *pll = &eq5c_early_plls[i];
+ unsigned long mult, div, acc;
+ struct clk_hw *hw;
+ u32 r0, r1;
+
+ r0 = readl(base_plls + pll->reg);
+ r1 = readl(base_plls + pll->reg + sizeof(r0));
+
+ ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
+ if (ret) {
+ pr_warn("failed parsing state of %s\n", pll->name);
+ eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
+ continue;
+ }
+
+ hw = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
+ np, pll->name, "ref", 0, mult, div, acc);
+ eq5c_clk_data->hws[pll->index] = hw;
+ if (IS_ERR(hw))
+ pr_err("failed registering %s: %ld\n",
+ pll->name, PTR_ERR(hw));
+ }
+
+ ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, eq5c_clk_data);
+ if (ret) {
+ pr_err("failed registering clk provider: %d\n", ret);
+ goto err;
+ }
+
+ return;
+
+err:
+ kfree(eq5c_clk_data);
+ /* Signal to platform driver probe that we failed init. */
+ eq5c_clk_data = ERR_PTR(ret);
+}
+
+CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
--
2.44.0
On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
> Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> support for other platforms from Mobileye.
>
> It handles 10 read-only PLLs derived from the main crystal on board. It
If you wrap 'It' to the next line, overall text will look better.
> exposes a table-based divider clock used for OSPI. Other platform
> clocks are not configurable and therefore kept as fixed-factor
> devicetree nodes.
>
> Two PLLs are required early on and are therefore registered at
> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
Ditto for 'the'
> UARTs.
...
> +config COMMON_CLK_EYEQ5
> + bool "Clock driver for the Mobileye EyeQ5 platform"
> + depends on OF
Since it's a functional dependency, why not allow compile test without OF being
enabled?
> + depends on MACH_EYEQ5 || COMPILE_TEST
> + default MACH_EYEQ5
> + help
> + This driver provides the clocks found on the Mobileye EyeQ5 SoC. Its
> + registers live in a shared register region called OLB. It provides 10
> + read-only PLLs derived from the main crystal clock which must be constant
> + and one divider clock based on one PLL.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
+ errno.h (yes, you need both)
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
+ overflow.h
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
...
> +struct eq5c_pll {
> + int index;
Index can be negative? Any comment about this case?
> + const char *name;
> + u32 reg; /* next 8 bytes are r0 and r1 */
Not sure this comments gives any clarification to a mere reader of the code.
Perhaps you want to name this as reg64 (at least it will show that you have
8 bytes, but I have no clue what is the semantic relationship between r0 and
r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> +};
...
> +static int eq5c_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
It's used only once. Why not just use dev->of_node there?
> + void __iomem *base_plls, *base_ospi;
> + struct clk_hw *hw;
> + int i;
> +
> + /* Return potential error from eq5c_init(). */
> + if (IS_ERR(eq5c_clk_data))
> + return PTR_ERR(eq5c_clk_data);
> + /* Return an error if eq5c_init() did not get called. */
> + else if (!eq5c_clk_data)
Redundant 'else'
> + return -EINVAL;
I didn't get. If eq5c_init() was finished successfully, why do you need to
seems repeat what it already done? What did I miss?
> + base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> + if (IS_ERR(base_plls))
> + return PTR_ERR(base_plls);
> +
> + base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
> + if (IS_ERR(base_ospi))
> + return PTR_ERR(base_ospi);
> +
> + for (i = 0; i < ARRAY_SIZE(eq5c_plls); i++) {
> + const struct eq5c_pll *pll = &eq5c_plls[i];
> + unsigned long mult, div, acc;
> + u32 r0, r1;
> + int ret;
> +
> + r0 = readl(base_plls + pll->reg);
> + r1 = readl(base_plls + pll->reg + sizeof(r0));
> +
> + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> + if (ret) {
> + dev_warn(dev, "failed parsing state of %s\n", pll->name);
> + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> + continue;
> + }
> +
> + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
> + pll->name, "ref", 0, mult, div, acc);
> + eq5c_clk_data->hws[pll->index] = hw;
> + if (IS_ERR(hw))
> + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> + pll->name);
Missed return statement?
> + }
> +
> + hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME,
> + eq5c_clk_data->hws[EQ5C_PLL_PER], 0,
> + base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0,
> + eq5c_ospi_div_table, NULL);
> + eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw;
> + if (IS_ERR(hw))
> + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> + EQ5C_OSPI_DIV_CLK_NAME);
Ditto.
> + return 0;
> +}
> +static void __init eq5c_init(struct device_node *np)
> +{
> + void __iomem *base_plls, *base_ospi;
> + int index_plls, index_ospi;
> + int i, ret;
Why is i signed?
> + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS),
> + GFP_KERNEL);
> + if (!eq5c_clk_data) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + eq5c_clk_data->num = EQ5C_NB_CLKS;
> +
> + /*
> + * Mark all clocks as deferred. We register some now and others at
> + * platform device probe.
> + */
> + for (i = 0; i < EQ5C_NB_CLKS; i++)
> + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> + index_plls = of_property_match_string(np, "reg-names", "plls");
> + if (index_plls < 0) {
> + ret = index_plls;
> + goto err;
> + }
Better pattern is to avoid the output pollution in the error case. Hence
ret = of_property_match_string(np, "reg-names", "plls");
if (ret < 0)
goto err;
index_plls = ret;
> + index_ospi = of_property_match_string(np, "reg-names", "ospi");
> + if (index_ospi < 0) {
> + ret = index_ospi;
> + goto err;
> + }
Ditto.
> + base_plls = of_iomap(np, index_plls);
> + base_ospi = of_iomap(np, index_ospi);
> + if (!base_plls || !base_ospi) {
> + ret = -ENODEV;
> + goto err;
> + }
> + for (i = 0; i < ARRAY_SIZE(eq5c_early_plls); i++) {
> + const struct eq5c_pll *pll = &eq5c_early_plls[i];
> + unsigned long mult, div, acc;
> + struct clk_hw *hw;
> + u32 r0, r1;
> +
> + r0 = readl(base_plls + pll->reg);
> + r1 = readl(base_plls + pll->reg + sizeof(r0));
> +
> + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> + if (ret) {
> + pr_warn("failed parsing state of %s\n", pll->name);
> + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> + continue;
> + }
> +
> + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
> + np, pll->name, "ref", 0, mult, div, acc);
> + eq5c_clk_data->hws[pll->index] = hw;
> + if (IS_ERR(hw))
> + pr_err("failed registering %s: %ld\n",
%pe ?
> + pll->name, PTR_ERR(hw));
Is the error not critical? Is it fine? How is it supposed to work at such
circumstances?
> + }
> +
> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, eq5c_clk_data);
> + if (ret) {
> + pr_err("failed registering clk provider: %d\n", ret);
> + goto err;
> + }
> +
> + return;
> +
> +err:
> + kfree(eq5c_clk_data);
> + /* Signal to platform driver probe that we failed init. */
> + eq5c_clk_data = ERR_PTR(ret);
> +}
> +
> +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
--
With Best Regards,
Andy Shevchenko
Hello Andy,
Thanks for the review! I'll be skipping straight forward comments.
On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
> > Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> > support for other platforms from Mobileye.
[...]
> > +config COMMON_CLK_EYEQ5
> > + bool "Clock driver for the Mobileye EyeQ5 platform"
>
> > + depends on OF
>
> Since it's a functional dependency, why not allow compile test without OF being
> enabled?
I'd do this then:
depends on OF || COMPILE_TEST
Which is better than removing the depend line. I wouldn't want the
kernel to build fine with OF=n even though we need it. OK for you?
>
> > + depends on MACH_EYEQ5 || COMPILE_TEST
> > + default MACH_EYEQ5
> > + help
> > + This driver provides the clocks found on the Mobileye EyeQ5 SoC. Its
> > + registers live in a shared register region called OLB. It provides 10
> > + read-only PLLs derived from the main crystal clock which must be constant
> > + and one divider clock based on one PLL.
[...]
> > +struct eq5c_pll {
> > + int index;
>
> Index can be negative? Any comment about this case?
No it cannot. I did not care much because structs of this type are only
defined in the following static const table, using constants from
dt-bindings header.
I'll change to unsigned int.
>
> > + const char *name;
> > + u32 reg; /* next 8 bytes are r0 and r1 */
>
> Not sure this comments gives any clarification to a mere reader of the code.
> Perhaps you want to name this as reg64 (at least it will show that you have
> 8 bytes, but I have no clue what is the semantic relationship between r0 and
> r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
Clocks are defined by two 32-bit registers. We only store the first
register offset because they always follow each other.
I like the reg64 name and will remove the comment. This straight forward
code is found in the rest of the code, I don't think it is anything
hard to understand (ie does not need a comment):
u32 r0 = readl(base_plls + pll->reg);
u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
[...]
> > + return -EINVAL;
>
> I didn't get. If eq5c_init() was finished successfully, why do you need to
> seems repeat what it already done? What did I miss?
The key here is that eq5c_init() iterates on eq5c_early_plls[] while
eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
commit message:
> Two PLLs are required early on and are therefore registered at
> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> UARTs.
Doing everything in eq5c_init() is not clean because we expect all new
clock provider drivers to be standard platform drivers. Doing
everything from a platform driver probe doesn't work because some
clocks are required earlier than platform bus init. We therefore do a
mix.
This has been approved by Stephen Boyd in this email:
https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/
[...]
> > + base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> > + if (IS_ERR(base_plls))
> > + return PTR_ERR(base_plls);
> > +
> > + base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
> > + if (IS_ERR(base_ospi))
> > + return PTR_ERR(base_ospi);
> > +
> > + for (i = 0; i < ARRAY_SIZE(eq5c_plls); i++) {
> > + const struct eq5c_pll *pll = &eq5c_plls[i];
> > + unsigned long mult, div, acc;
> > + u32 r0, r1;
> > + int ret;
> > +
> > + r0 = readl(base_plls + pll->reg);
> > + r1 = readl(base_plls + pll->reg + sizeof(r0));
> > +
> > + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> > + if (ret) {
> > + dev_warn(dev, "failed parsing state of %s\n", pll->name);
> > + eq5c_clk_data->hws[pll->index] = ERR_PTR(ret);
> > + continue;
> > + }
> > +
> > + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
> > + pll->name, "ref", 0, mult, div, acc);
> > + eq5c_clk_data->hws[pll->index] = hw;
> > + if (IS_ERR(hw))
>
> > + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> > + pll->name);
>
> Missed return statement?
No, we still try to register all clocks even if one failed. I guess we
can call this being optimistic.
[...]
> > +static void __init eq5c_init(struct device_node *np)
> > +{
> > + void __iomem *base_plls, *base_ospi;
> > + int index_plls, index_ospi;
> > + int i, ret;
>
> Why is i signed?
No reason, will be changed to unsigned int.
[...]
> > + hw = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
> > + np, pll->name, "ref", 0, mult, div, acc);
> > + eq5c_clk_data->hws[pll->index] = hw;
> > + if (IS_ERR(hw))
> > + pr_err("failed registering %s: %ld\n",
>
> %pe ?
>
> > + pll->name, PTR_ERR(hw));
>
> Is the error not critical? Is it fine? How is it supposed to work at such
> circumstances?
It is a critical error, the system will stop working in a few
milliseconds. :-) This is different from probe and it should indeed
return the error.
Thanks for the review Andy.
Have a nice day,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote: > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote: [...] > > > + depends on OF > > > > Since it's a functional dependency, why not allow compile test without OF > > being enabled? > > I'd do this then: > > depends on OF || COMPILE_TEST > > Which is better than removing the depend line. I wouldn't want the > kernel to build fine with OF=n even though we need it. OK for you? Yes! [...] > > > + u32 reg; /* next 8 bytes are r0 and r1 */ > > > > Not sure this comments gives any clarification to a mere reader of the code. > > Perhaps you want to name this as reg64 (at least it will show that you have > > 8 bytes, but I have no clue what is the semantic relationship between r0 and > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1? > > Clocks are defined by two 32-bit registers. We only store the first > register offset because they always follow each other. > I like the reg64 name and will remove the comment. This straight forward > code is found in the rest of the code, I don't think it is anything > hard to understand (ie does not need a comment): > > u32 r0 = readl(base_plls + pll->reg); > u32 r1 = readl(base_plls + pll->reg + sizeof(r0)); Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) can be used in this case? It will be much better overall and be aligned with reg64 name. [...] > > I didn't get. If eq5c_init() was finished successfully, why do you need to > > seems repeat what it already done? What did I miss? > > The key here is that eq5c_init() iterates on eq5c_early_plls[] while > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the > commit message: > > > Two PLLs are required early on and are therefore registered at > > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the > > UARTs. > > Doing everything in eq5c_init() is not clean because we expect all new > clock provider drivers to be standard platform drivers. Doing > everything from a platform driver probe doesn't work because some > clocks are required earlier than platform bus init. We therefore do a > mix. Am I missing something or these two pieces are using the same IO resources? This looks like a lot of code duplication without clear benefit. Perhaps you can have a helper? > This has been approved by Stephen Boyd in this email: > https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/ OK! [...] > > > + eq5c_clk_data->hws[pll->index] = hw; > > > + if (IS_ERR(hw)) > > > > > + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n", > > > + pll->name); > > > > Missed return statement? > > No, we still try to register all clocks even if one failed. I guess we > can call this being optimistic. But how critical these clocks are? I believe we should panic it we have no critical calls be available. Otherwise, why '_err_'? Shouldn't be dev_warn()? -- With Best Regards, Andy Shevchenko
Hello, On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote: > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote: [...] > > > > + u32 reg; /* next 8 bytes are r0 and r1 */ > > > > > > Not sure this comments gives any clarification to a mere reader of the code. > > > Perhaps you want to name this as reg64 (at least it will show that you have > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1? > > > > Clocks are defined by two 32-bit registers. We only store the first > > register offset because they always follow each other. > > > I like the reg64 name and will remove the comment. This straight forward > > code is found in the rest of the code, I don't think it is anything > > hard to understand (ie does not need a comment): > > > > u32 r0 = readl(base_plls + pll->reg); > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0)); > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > can be used in this case? It will be much better overall and be aligned with > reg64 name. The doc talks in terms of 32-bit registers. I do not see a reason to work in 64-bit. If we get a 64-bit value that we need to split we need to think about the endianness of our platform, which makes things more complex than just reading both values independently. > [...] > > > > I didn't get. If eq5c_init() was finished successfully, why do you need to > > > seems repeat what it already done? What did I miss? > > > > The key here is that eq5c_init() iterates on eq5c_early_plls[] while > > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the > > commit message: > > > > > Two PLLs are required early on and are therefore registered at > > > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the > > > UARTs. > > > > Doing everything in eq5c_init() is not clean because we expect all new > > clock provider drivers to be standard platform drivers. Doing > > everything from a platform driver probe doesn't work because some > > clocks are required earlier than platform bus init. We therefore do a > > mix. > > Am I missing something or these two pieces are using the same IO resources? > This looks like a lot of code duplication without clear benefit. Perhaps > you can have a helper? There are two subtle differences that make creating a helper difficult: - Logging, pr_*() vs dev_*(). Second option is preferred but only available once a device is created. - Behavior on error: we stop the world for early clocks but keep going for normal clocks. [...] > > > > + eq5c_clk_data->hws[pll->index] = hw; > > > > + if (IS_ERR(hw)) > > > > > > > + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n", > > > > + pll->name); > > > > > > Missed return statement? > > > > No, we still try to register all clocks even if one failed. I guess we > > can call this being optimistic. > > But how critical these clocks are? I believe we should panic it we have no > critical calls be available. Otherwise, why '_err_'? Shouldn't be dev_warn()? Indeed printing should be dev_warn(), I missed that. Thanks Andy, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote:
> On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote:
> > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
[...]
> > > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > > >
> > > > Not sure this comments gives any clarification to a mere reader of the code.
> > > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> > >
> > > Clocks are defined by two 32-bit registers. We only store the first
> > > register offset because they always follow each other.
> >
> > > I like the reg64 name and will remove the comment. This straight forward
> > > code is found in the rest of the code, I don't think it is anything
> > > hard to understand (ie does not need a comment):
> > >
> > > u32 r0 = readl(base_plls + pll->reg);
> > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
> >
> > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> > can be used in this case? It will be much better overall and be aligned with
> > reg64 name.
>
> The doc talks in terms of 32-bit registers. I do not see a reason to
> work in 64-bit. If we get a 64-bit value that we need to split we need
> to think about the endianness of our platform, which makes things more
> complex than just reading both values independently.
1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO.
2) Still I see a benefit from using lo_hi_readq() and friends directly.
[...]
> > > > I didn't get. If eq5c_init() was finished successfully, why do you need to
> > > > seems repeat what it already done? What did I miss?
> > >
> > > The key here is that eq5c_init() iterates on eq5c_early_plls[] while
> > > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
> > > commit message:
> > >
> > > > Two PLLs are required early on and are therefore registered at
> > > > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> > > > UARTs.
> > >
> > > Doing everything in eq5c_init() is not clean because we expect all new
> > > clock provider drivers to be standard platform drivers. Doing
> > > everything from a platform driver probe doesn't work because some
> > > clocks are required earlier than platform bus init. We therefore do a
> > > mix.
> >
> > Am I missing something or these two pieces are using the same IO resources?
> > This looks like a lot of code duplication without clear benefit. Perhaps
> > you can have a helper?
>
> There are two subtle differences that make creating a helper difficult:
>
> - Logging, pr_*() vs dev_*(). Second option is preferred but only
> available once a device is created.
Some code uses (yeah, arguable that it's better, but depends on how much
the real deduplication takes)
if (dev)
dev_*(...);
else
pr_*(...);
> - Behavior on error: we stop the world for early clocks but keep going
> for normal clocks.
...(..., bool skip_errors)
{
...
}
(with the same caveat)?
--
With Best Regards,
Andy Shevchenko
Hello,
On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote:
> > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote:
> > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote:
>
> [...]
>
> > > > > > + u32 reg; /* next 8 bytes are r0 and r1 */
> > > > >
> > > > > Not sure this comments gives any clarification to a mere reader of the code.
> > > > > Perhaps you want to name this as reg64 (at least it will show that you have
> > > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and
> > > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1?
> > > >
> > > > Clocks are defined by two 32-bit registers. We only store the first
> > > > register offset because they always follow each other.
> > >
> > > > I like the reg64 name and will remove the comment. This straight forward
> > > > code is found in the rest of the code, I don't think it is anything
> > > > hard to understand (ie does not need a comment):
> > > >
> > > > u32 r0 = readl(base_plls + pll->reg);
> > > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0));
> > >
> > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h)
> > > can be used in this case? It will be much better overall and be aligned with
> > > reg64 name.
> >
> > The doc talks in terms of 32-bit registers. I do not see a reason to
> > work in 64-bit. If we get a 64-bit value that we need to split we need
> > to think about the endianness of our platform, which makes things more
> > complex than just reading both values independently.
>
> 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO.
Just tested, it works. No error on the memory bus. And checked assembly
generated was a single 64-bit instructions.
It might not work on other hardware revisions though. I can't remember
if memory bus is changing across them.
> 2) Still I see a benefit from using lo_hi_readq() and friends directly.
So it is:
u32 r0 = readl(base_plls + pll->reg64);
u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0));
vs:
u64 r = lo_hi_readq(base_plls + pll->regs64);
u32 r0 = r;
u32 r1 = r >> 32;
One is straight forward, the other uses an obscure helper that code
readers must understand and follows that with bit manipulation.
>
> [...]
>
> > > > > I didn't get. If eq5c_init() was finished successfully, why do you need to
> > > > > seems repeat what it already done? What did I miss?
> > > >
> > > > The key here is that eq5c_init() iterates on eq5c_early_plls[] while
> > > > eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
> > > > commit message:
> > > >
> > > > > Two PLLs are required early on and are therefore registered at
> > > > > of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> > > > > UARTs.
> > > >
> > > > Doing everything in eq5c_init() is not clean because we expect all new
> > > > clock provider drivers to be standard platform drivers. Doing
> > > > everything from a platform driver probe doesn't work because some
> > > > clocks are required earlier than platform bus init. We therefore do a
> > > > mix.
> > >
> > > Am I missing something or these two pieces are using the same IO resources?
> > > This looks like a lot of code duplication without clear benefit. Perhaps
> > > you can have a helper?
> >
> > There are two subtle differences that make creating a helper difficult:
> >
> > - Logging, pr_*() vs dev_*(). Second option is preferred but only
> > available once a device is created.
>
> Some code uses (yeah, arguable that it's better, but depends on how much
> the real deduplication takes)
>
> if (dev)
> dev_*(...);
> else
> pr_*(...);
>
> > - Behavior on error: we stop the world for early clocks but keep going
> > for normal clocks.
>
> ...(..., bool skip_errors)
> {
> ...
> }
>
> (with the same caveat)?
I started trying it out, but the combination of both flags means dealing
with errors would look like:
ret = foo();
if (ret) {
if (!skip_errors) {
if (dev)
dev_err(dev, "...");
else
pr_err("...");
return ret;
}
if (dev)
dev_warn(dev, "...");
else
pr_warn("...");
}
There are two errors to handle, that makes a mess out of the code.
Having a little bit of repetition but straight forward code is nicer in
my opinion. At least we tried!
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Quoting Théo Lebrun (2024-02-29 07:40:25) > Hello, > > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote: > > > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote: > > > 2) Still I see a benefit from using lo_hi_readq() and friends directly. > > So it is: > > u32 r0 = readl(base_plls + pll->reg64); > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); > > vs: > > u64 r = lo_hi_readq(base_plls + pll->regs64); > u32 r0 = r; > u32 r1 = r >> 32; > > One is straight forward, the other uses an obscure helper that code > readers must understand and follows that with bit manipulation. > Just use readq() and include the correct header please. We know what readq() is in the kernel.
On Thu, Feb 29, 2024 at 04:40:25PM +0100, Théo Lebrun wrote: > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote: > > > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote: [...] > > > > > > > + u32 reg; /* next 8 bytes are r0 and r1 */ > > > > > > > > > > > > Not sure this comments gives any clarification to a mere reader of the code. > > > > > > Perhaps you want to name this as reg64 (at least it will show that you have > > > > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and > > > > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1? > > > > > > > > > > Clocks are defined by two 32-bit registers. We only store the first > > > > > register offset because they always follow each other. > > > > > > > > > I like the reg64 name and will remove the comment. This straight forward > > > > > code is found in the rest of the code, I don't think it is anything > > > > > hard to understand (ie does not need a comment): > > > > > > > > > > u32 r0 = readl(base_plls + pll->reg); > > > > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0)); > > > > > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > > > > can be used in this case? It will be much better overall and be aligned with > > > > reg64 name. > > > > > > The doc talks in terms of 32-bit registers. I do not see a reason to > > > work in 64-bit. If we get a 64-bit value that we need to split we need > > > to think about the endianness of our platform, which makes things more > > > complex than just reading both values independently. > > > > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO. > > Just tested, it works. No error on the memory bus. And checked assembly > generated was a single 64-bit instructions. > > It might not work on other hardware revisions though. I can't remember > if memory bus is changing across them. > > > 2) Still I see a benefit from using lo_hi_readq() and friends directly. > > So it is: > > u32 r0 = readl(base_plls + pll->reg64); > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); > > vs: > > u64 r = lo_hi_readq(base_plls + pll->regs64); > u32 r0 = r; > u32 r1 = r >> 32; It depends to the semantics of these two. How hard do they coupled to each other semantically? I.o.w. can they always be considered as 64-bit register with the respective bitfields? (And note FIELD_GET() here is your friend.) > One is straight forward, the other uses an obscure helper that code > readers must understand and follows that with bit manipulation. [...] > There are two errors to handle, that makes a mess out of the code. > Having a little bit of repetition but straight forward code is nicer in > my opinion. At least we tried! Yes! Perhaps you can add a couple of words into commit message to explain this detail of implementation (that code in two parts is not so identical to be easily deduplicated). -- With Best Regards, Andy Shevchenko
Hello, On Thu Feb 29, 2024 at 4:48 PM CET, Andy Shevchenko wrote: > On Thu, Feb 29, 2024 at 04:40:25PM +0100, Théo Lebrun wrote: > > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > > > On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote: > > > > > > On Tue, Feb 27, 2024 at 03:55:24PM +0100, Théo Lebrun wrote: > > [...] > > > > > > > > > + u32 reg; /* next 8 bytes are r0 and r1 */ > > > > > > > > > > > > > > Not sure this comments gives any clarification to a mere reader of the code. > > > > > > > Perhaps you want to name this as reg64 (at least it will show that you have > > > > > > > 8 bytes, but I have no clue what is the semantic relationship between r0 and > > > > > > > r1, it's quite cryptic to me). Or maybe it should be reg_0_1? > > > > > > > > > > > > Clocks are defined by two 32-bit registers. We only store the first > > > > > > register offset because they always follow each other. > > > > > > > > > > > I like the reg64 name and will remove the comment. This straight forward > > > > > > code is found in the rest of the code, I don't think it is anything > > > > > > hard to understand (ie does not need a comment): > > > > > > > > > > > > u32 r0 = readl(base_plls + pll->reg); > > > > > > u32 r1 = readl(base_plls + pll->reg + sizeof(r0)); > > > > > > > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > > > > > can be used in this case? It will be much better overall and be aligned with > > > > > reg64 name. > > > > > > > > The doc talks in terms of 32-bit registers. I do not see a reason to > > > > work in 64-bit. If we get a 64-bit value that we need to split we need > > > > to think about the endianness of our platform, which makes things more > > > > complex than just reading both values independently. > > > > > > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO. > > > > Just tested, it works. No error on the memory bus. And checked assembly > > generated was a single 64-bit instructions. > > > > It might not work on other hardware revisions though. I can't remember > > if memory bus is changing across them. > > > > > 2) Still I see a benefit from using lo_hi_readq() and friends directly. > > > > So it is: > > > > u32 r0 = readl(base_plls + pll->reg64); > > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); > > > > vs: > > > > u64 r = lo_hi_readq(base_plls + pll->regs64); > > > u32 r0 = r; > > u32 r1 = r >> 32; > > It depends to the semantics of these two. How hard do they coupled to each > other semantically? I.o.w. can they always be considered as 64-bit register > with the respective bitfields? (And note FIELD_GET() here is your friend.) OLB (the memory region) has always been described as a list of 32-bit registers. The semantics lean in the camp of two readl(). > > One is straight forward, the other uses an obscure helper that code > > readers must understand and follows that with bit manipulation. > > [...] > > > There are two errors to handle, that makes a mess out of the code. > > Having a little bit of repetition but straight forward code is nicer in > > my opinion. At least we tried! > > Yes! Perhaps you can add a couple of words into commit message to explain > this detail of implementation (that code in two parts is not so identical > to be easily deduplicated). Yes, will do. I get why from a reader's point-of-view it looks like duplicate code. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
© 2016 - 2026 Red Hat, Inc.