| Message ID | 20231218-mbly-clk-v1-3-44ce54108f06@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Mobileye EyeQ5 clock controller | expand |
Hello, I've seen all your comments, thanks for that. I have a follow up about one: On Wed Dec 20, 2023 at 12:09 AM CET, Stephen Boyd wrote: > Quoting Théo Lebrun (2023-12-18 09:14:18) > > Add the Mobileye EyeQ5 clock controller driver. See the header comment > > for more information on how it works. > > "See the header" is like saying "Read the code" which is pretty obvious. > Remove this sentence and tell us why only the PLLs are supported at the > moment or something like that. > > > This driver is specific to this > > platform; it might grow to add later support of other platforms from > > Mobileye. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > MAINTAINERS | 1 + > > drivers/clk/Kconfig | 11 +++ > > drivers/clk/Makefile | 1 + > > drivers/clk/clk-eyeq5.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 224 insertions(+) > > [...] > > diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c > > new file mode 100644 > > index 000000000000..74bcb8cec5c1 > > --- /dev/null > > +++ b/drivers/clk/clk-eyeq5.c [...] > > + of_clk_add_hw_provider(np, of_clk_hw_onecell_get, data); > > +} > > + > > +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init); > > Please use a platform driver. I've been trying to do that but I had a stall at boot. I initially associated it with the UART driver acquiring a clock too early but instead it is the CPU timer clocksource driver that consumes one of our clock way earlier than any platform driver initialisation. The clocksource driver we are talking about is this one for reference: https://elixir.bootlin.com/linux/v6.6.8/source/drivers/clocksource/mips-gic-timer.c Its usage of TIMER_OF_DECLARE means it gets probed by timer_probe -> plat_time_init -> time_init -> start_kernel. This is way before any initcalls. Our prior use of CLK_OF_DECLARE_DRIVER meant that we got probed by of_clk_init -> plat_time_init. I'm guessing we are not the first one in this situation; any advice on how to deal with it? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Fri Dec 22, 2023 at 12:25 PM CET, Théo Lebrun wrote: > Hello, > > I've seen all your comments, thanks for that. I have a follow up about > one: > > On Wed Dec 20, 2023 at 12:09 AM CET, Stephen Boyd wrote: > > Quoting Théo Lebrun (2023-12-18 09:14:18) > > > Add the Mobileye EyeQ5 clock controller driver. See the header comment > > > for more information on how it works. > > > > "See the header" is like saying "Read the code" which is pretty obvious. > > Remove this sentence and tell us why only the PLLs are supported at the > > moment or something like that. > > > > > This driver is specific to this > > > platform; it might grow to add later support of other platforms from > > > Mobileye. > > > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/clk/Kconfig | 11 +++ > > > drivers/clk/Makefile | 1 + > > > drivers/clk/clk-eyeq5.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 224 insertions(+) > > > > > [...] > > > > diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c > > > new file mode 100644 > > > index 000000000000..74bcb8cec5c1 > > > --- /dev/null > > > +++ b/drivers/clk/clk-eyeq5.c > > [...] > > > > + of_clk_add_hw_provider(np, of_clk_hw_onecell_get, data); > > > +} > > > + > > > +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init); > > > > Please use a platform driver. > > I've been trying to do that but I had a stall at boot. I initially > associated it with the UART driver acquiring a clock too early but > instead it is the CPU timer clocksource driver that consumes one of our > clock way earlier than any platform driver initialisation. > > The clocksource driver we are talking about is this one for reference: > https://elixir.bootlin.com/linux/v6.6.8/source/drivers/clocksource/mips-gic-timer.c > > Its usage of TIMER_OF_DECLARE means it gets probed by timer_probe -> > plat_time_init -> time_init -> start_kernel. This is way before any > initcalls. Our prior use of CLK_OF_DECLARE_DRIVER meant that we got > probed by of_clk_init -> plat_time_init. > > I'm guessing we are not the first one in this situation; any advice on > how to deal with it? I went ahead with a V2, feeling it would be more productive to come up with something and gather comments on concrete stuff. There were many other things to address anyway. I've addressed this point by declaring a dummy fixed-clock in the devicetree that gets fed to the GIC timer. It is pretty much the same thing as using `clock-frequency` which this specific clocksource uses if `of_clk_get(node, 0)` fails. With the sent approach we have the timer appear in the clock tree as a consumer. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Quoting Théo Lebrun (2023-12-22 03:25:12) > On Wed Dec 20, 2023 at 12:09 AM CET, Stephen Boyd wrote: > > Quoting Théo Lebrun (2023-12-18 09:14:18) > > > > + of_clk_add_hw_provider(np, of_clk_hw_onecell_get, data); > > > +} > > > + > > > +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init); > > > > Please use a platform driver. > > I've been trying to do that but I had a stall at boot. I initially > associated it with the UART driver acquiring a clock too early but > instead it is the CPU timer clocksource driver that consumes one of our > clock way earlier than any platform driver initialisation. > > The clocksource driver we are talking about is this one for reference: > https://elixir.bootlin.com/linux/v6.6.8/source/drivers/clocksource/mips-gic-timer.c > > Its usage of TIMER_OF_DECLARE means it gets probed by timer_probe -> > plat_time_init -> time_init -> start_kernel. This is way before any > initcalls. Our prior use of CLK_OF_DECLARE_DRIVER meant that we got > probed by of_clk_init -> plat_time_init. > > I'm guessing we are not the first one in this situation; any advice on > how to deal with it? > You guessed correctly. In that case, use CLK_OF_DECLARE_DRIVER() and register the clk(s) needed for the timer in that function. Other clks shall be registered with a platform driver.
Quoting Théo Lebrun (2023-12-27 08:30:20) > > I went ahead with a V2, feeling it would be more productive to come up > with something and gather comments on concrete stuff. There were many > other things to address anyway. > > I've addressed this point by declaring a dummy fixed-clock in the > devicetree that gets fed to the GIC timer. It is pretty much the same > thing as using `clock-frequency` which this specific clocksource uses > if `of_clk_get(node, 0)` fails. With the sent approach we have the > timer appear in the clock tree as a consumer. > Ok, please send another round then. I was away from my computer for a week or two.
Quoting Théo Lebrun (2023-12-27 08:30:20) > > I went ahead with a V2, feeling it would be more productive to come up > with something and gather comments on concrete stuff. There were many > other things to address anyway. > > I've addressed this point by declaring a dummy fixed-clock in the > devicetree that gets fed to the GIC timer. It is pretty much the same > thing as using `clock-frequency` which this specific clocksource uses > if `of_clk_get(node, 0)` fails. With the sent approach we have the > timer appear in the clock tree as a consumer. If the frequency is fixed then this seems fine to do always.
Hello, On Wed Jan 3, 2024 at 12:43 AM CET, Stephen Boyd wrote: > Quoting Théo Lebrun (2023-12-27 08:30:20) > > > > I went ahead with a V2, feeling it would be more productive to come up > > with something and gather comments on concrete stuff. There were many > > other things to address anyway. > > > > I've addressed this point by declaring a dummy fixed-clock in the > > devicetree that gets fed to the GIC timer. It is pretty much the same > > thing as using `clock-frequency` which this specific clocksource uses > > if `of_clk_get(node, 0)` fails. With the sent approach we have the > > timer appear in the clock tree as a consumer. > > > > Ok, please send another round then. I was away from my computer for a > week or two. No worries, hope you had a nice time. On Wed Jan 3, 2024 at 12:47 AM CET, Stephen Boyd wrote: > If the frequency is fixed then this seems fine to do always. It is the case. Can you confirm that the taken approach is fine for you? One issue I see with my V2 is that I still expose the timer clk from the clk driver, even though it is not consumed by anyone and it is exposed as a fixed-rate from a devicetree node. That makes a duplicate. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/MAINTAINERS b/MAINTAINERS index 7f04fa760a4d..c75c7de1d507 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14557,6 +14557,7 @@ F: Documentation/devicetree/bindings/mips/mobileye.yaml F: arch/mips/boot/dts/mobileye/ F: arch/mips/configs/generic/board-eyeq5.config F: arch/mips/generic/board-epm5.its.S +F: drivers/clk/clk-eyeq5.c F: include/dt-bindings/clock/mobileye,eyeq5-clk.h F: include/dt-bindings/soc/mobileye,eyeq5.h diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index c30d0d396f7a..84fe0a89b8df 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 SOC_EYEQ5 || COMPILE_TEST + default SOC_EYEQ5 + help + This drivers provides the fixed 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. + 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 ed71f2e0ee36..0df0dc6dbcae 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..74bcb8cec5c1 --- /dev/null +++ b/drivers/clk/clk-eyeq5.c @@ -0,0 +1,211 @@ +// 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. The parent clock is expected to be constant. This driver is + * custom to this platform, its registers live in a shared region called OLB. + * + * We use eq5c_ as prefix, as-in "EyeQ5 Clock", but way shorter. + * + * Copyright (C) 2023 Mobileye Vision Technologies Ltd. + */ +#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/clk.h> +#include <linux/mfd/syscon.h> +#include <linux/of_device.h> +#include <linux/regmap.h> + +#include <dt-bindings/clock/mobileye,eyeq5-clk.h> + +#undef pr_fmt +#define pr_fmt(fmt) "%s: " fmt, __func__ + +/* + * PLL control & status registers, n=0..1 + * 0x02c..0x078 + */ +#define OLB_PCSR_CPU(n) (0x02C + (n) * 4) /* CPU */ +#define OLB_PCSR_VMP(n) (0x034 + (n) * 4) /* VMP */ +#define OLB_PCSR_PMA(n) (0x03C + (n) * 4) /* PMA */ +#define OLB_PCSR_VDI(n) (0x044 + (n) * 4) /* VDI */ +#define OLB_PCSR_DDR0(n) (0x04C + (n) * 4) /* DDR0 */ +#define OLB_PCSR_PCI(n) (0x054 + (n) * 4) /* PCI */ +#define OLB_PCSR_PER(n) (0x05C + (n) * 4) /* PER */ +#define OLB_PCSR_PMAC(n) (0x064 + (n) * 4) /* PMAC */ +#define OLB_PCSR_MPC(n) (0x06c + (n) * 4) /* MPC */ +#define OLB_PCSR_DDR1(n) (0x074 + (n) * 4) /* DDR1 */ + +/* In frac mode, it enables fractional noise canceling DAC. Else, no function. */ +#define OLB_PCSR0_DAC_EN BIT(0) +/* Fractional or integer mode */ +#define OLB_PCSR0_DSM_EN BIT(1) +#define OLB_PCSR0_PLL_EN BIT(2) +/* All clocks output held at 0 */ +#define OLB_PCSR0_FOUTPOSTDIV_EN BIT(3) +#define OLB_PCSR0_POST_DIV1 GENMASK(6, 4) +#define OLB_PCSR0_POST_DIV2 GENMASK(9, 7) +#define OLB_PCSR0_REF_DIV GENMASK(15, 10) +#define OLB_PCSR0_INTIN GENMASK(27, 16) +#define OLB_PCSR0_BYPASS BIT(28) +/* Bits 30..29 are reserved */ +#define OLB_PCSR0_PLL_LOCKED BIT(31) + +#define OLB_PCSR1_RESET BIT(0) +#define OLB_PCSR1_SSGC_DIV GENMASK(4, 1) +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ +#define OLB_PCSR1_SPREAD GENMASK(9, 5) +#define OLB_PCSR1_DIS_SSCG BIT(10) +/* Down-spread or center-spread */ +#define OLB_PCSR1_DOWN_SPREAD BIT(11) +#define OLB_PCSR1_FRAC_IN GENMASK(31, 12) + +static const struct eq5c_pll { + const char *name; + u32 reg; +} eq5c_plls[] = { + [EQ5C_PLL_CPU] = { .name = "pll-cpu", .reg = OLB_PCSR_CPU(0), }, + [EQ5C_PLL_VMP] = { .name = "pll-vmp", .reg = OLB_PCSR_VMP(0), }, + [EQ5C_PLL_PMA] = { .name = "pll-pma", .reg = OLB_PCSR_PMA(0), }, + [EQ5C_PLL_VDI] = { .name = "pll-vdi", .reg = OLB_PCSR_VDI(0), }, + [EQ5C_PLL_DDR0] = { .name = "pll-ddr0", .reg = OLB_PCSR_DDR0(0), }, + [EQ5C_PLL_PCI] = { .name = "pll-pci", .reg = OLB_PCSR_PCI(0), }, + [EQ5C_PLL_PER] = { .name = "pll-per", .reg = OLB_PCSR_PER(0), }, + [EQ5C_PLL_PMAC] = { .name = "pll-pmac", .reg = OLB_PCSR_PMAC(0), }, + [EQ5C_PLL_MPC] = { .name = "pll-mpc", .reg = OLB_PCSR_MPC(0), }, + [EQ5C_PLL_DDR1] = { .name = "pll-ddr1", .reg = OLB_PCSR_DDR1(0), }, +}; + +static int eq5c_pll_parse_registers(u32 r0, u32 r1, unsigned long *mult, + unsigned long *div, unsigned long *acc) +{ + if (!mult || !div || !acc) + return -EINVAL; + + if (r0 & OLB_PCSR0_BYPASS) { + *mult = 1; + *div = 1; + *acc = 0; + return 0; + } + + if (!(r0 & OLB_PCSR0_PLL_LOCKED)) + return -EINVAL; + + *mult = FIELD_GET(OLB_PCSR0_INTIN, r0); + *div = FIELD_GET(OLB_PCSR0_REF_DIV, r0); + if (r0 & OLB_PCSR0_FOUTPOSTDIV_EN) + *div *= FIELD_GET(OLB_PCSR0_POST_DIV1, r0) * + FIELD_GET(OLB_PCSR0_POST_DIV2, r0); + + /* Fractional mode, in 2^20 (0x100000) parts. */ + if (r0 & OLB_PCSR0_DSM_EN) { + *div *= 0x100000; + *mult = *mult * 0x100000 + FIELD_GET(OLB_PCSR1_FRAC_IN, r1); + } + + if (!*mult || !*div) + return -EINVAL; + + /* Spread spectrum. */ + if (!(r1 & (OLB_PCSR1_RESET | OLB_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(OLB_PCSR1_SPREAD, r1); + *acc = spread * 500000; + if (r1 & OLB_PCSR1_DOWN_SPREAD) { + /* + * Downspreading: the central frequency is half a + * spread lower. + */ + *mult *= 2000 - spread; + *div *= 2000; + } + } else { + *acc = 0; + } + + return 0; +} + +static void eq5c_init(struct device_node *np) +{ + struct device_node *parent_np = of_get_parent(np); + struct clk_hw_onecell_data *data; + unsigned long parent_clk_rate; + struct clk_hw *parent_clk_hw; + struct clk *parent_clk; + struct regmap *olb; + int i; + + data = kzalloc(struct_size(data, hws, ARRAY_SIZE(eq5c_plls)), GFP_KERNEL); + if (!data) + return; + + data->num = ARRAY_SIZE(eq5c_plls); + + /* + * TODO: currently, if OLB is not available, we log an error and early + * return. We might want to change this behavior and assume all clocks + * are in bypass mode; that is what is being done in the vendor driver. + * + * It is still unclear if there are valid situations where the OLB + * region would be inaccessible. + */ + olb = ERR_PTR(-ENODEV); + if (parent_np) + olb = syscon_node_to_regmap(parent_np); + if (IS_ERR(olb)) + olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb"); + if (IS_ERR(olb)) { + pr_err("failed getting regmap: %ld\n", PTR_ERR(olb)); + return; + } + + parent_clk = of_clk_get_by_name(np, "ref"); + if (IS_ERR_OR_NULL(parent_clk)) { + pr_err("no parent clock found\n"); + return; + } + parent_clk_hw = __clk_get_hw(parent_clk); + parent_clk_rate = clk_get_rate(parent_clk); + clk_put(parent_clk); + + 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; + + regmap_read(olb, pll->reg, &r0); + regmap_read(olb, pll->reg + sizeof(r0), &r1); + + ret = eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc); + if (ret) { + pr_warn("failed parsing state of %s\n", pll->name); + continue; + } + + /* We use fixed_rate and not fixed_factor because the latter + * does not allow reporting accuracy. The alternative is to + * create a custom clk implementation but that adds too many + * lines to the kernel for not much benefit; our parent clock + * rate won't change anyway. + */ + data->hws[i] = clk_hw_register_fixed_rate_with_accuracy_parent_hw( + NULL, pll->name, parent_clk_hw, 0, + parent_clk_rate * mult / div, acc); + if (IS_ERR_OR_NULL(data->hws[i])) { + pr_err("failed registering %s: %ld\n", + pll->name, PTR_ERR(data->hws[i])); + data->hws[i] = ERR_PTR(-ENOENT); + } + } + + of_clk_add_hw_provider(np, of_clk_hw_onecell_get, data); +} + +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
Add the Mobileye EyeQ5 clock controller driver. See the header comment for more information on how it works. This driver is specific to this platform; it might grow to add later support of other platforms from Mobileye. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- MAINTAINERS | 1 + drivers/clk/Kconfig | 11 +++ drivers/clk/Makefile | 1 + drivers/clk/clk-eyeq5.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 224 insertions(+)