| Message ID | 20240123-mbly-clk-v3-0-392b010b8281@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for Mobileye EyeQ5 system controller | expand |
Hello, On Wed Jan 24, 2024 at 7:43 AM CET, Krzysztof Kozlowski wrote: > You miss here, in this place, the most important information which I > asked previously - dependencies/merging: > https://lore.kernel.org/all/db9b2786-29f7-4d45-9087-a9f85b770b6c@linaro.org/ Clearly I still forgot some dt-bindings dependencies. I now get what you meant at the time. Those explain the robot failures btw. Should it just be me saying "hey patches X depends on patches A, B, C?" Or are there even Git message trailers to mention dependencies? Care to give a patch link that does it the right way? Sorry about that, Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 8:03 AM CET, Krzysztof Kozlowski wrote: > On 23/01/2024 19:46, Théo Lebrun wrote: > > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > > support of other platforms from Mobileye. It belongs to a syscon region > > called OLB. > > > > Existing pins and their function live statically in the driver code > > rather than in the devicetree, see compatible match data. > > > > ... > > > +static int eq5p_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + struct device_node *parent_np = of_get_parent(np); > > + const struct eq5p_match *match = of_device_get_match_data(dev); > > + struct pinctrl_dev *pctldev; > > + struct eq5p_pinctrl *pctrl; > > + int ret; > > + > > + pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL); > > + if (!pctrl) > > + return -ENOMEM; > > + > > + pctrl->olb = ERR_PTR(-ENODEV); > > + if (parent_np) > > + pctrl->olb = syscon_node_to_regmap(parent_np); > > + if (IS_ERR(pctrl->olb)) > > + pctrl->olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb"); > > + if (IS_ERR(pctrl->olb)) > > + return PTR_ERR(pctrl->olb); > > No, we talked about this, you got comments on this. There is no > mobileye,olb. You cannot have undocumented properties. Clearly and I fully agree. It's a versioning issue on my side. It's been fixed (again, oops). > > > + > > + pctrl->regs = match->regs; > > + pctrl->funcs = match->funcs; > > + pctrl->nfuncs = match->nfuncs; > > + > > + pctrl->desc.name = dev_name(dev); > > + pctrl->desc.pins = match->pins; > > + pctrl->desc.npins = match->npins; > > + pctrl->desc.pctlops = &eq5p_pinctrl_ops; > > + pctrl->desc.pmxops = &eq5p_pinmux_ops; > > + pctrl->desc.confops = &eq5p_pinconf_ops; > > + pctrl->desc.owner = THIS_MODULE; > > + > > + ret = devm_pinctrl_register_and_init(dev, &pctrl->desc, pctrl, &pctldev); > > + if (ret) { > > + dev_err(dev, "Failed registering pinctrl device: %d\n", ret); > > + return ret; > > + } > > + > > + ret = pinctrl_enable(pctldev); > > + if (ret) { > > + dev_err(dev, "Failed enabling pinctrl device: %d\n", ret); > > + return ret; > > + } > > + > > + dev_info(dev, "probed\n"); > > I am pretty sure you got comments for these. Drop such debugs from all > of your code. Current and future. Same thing, it must have been lost in the same fixup patch as the previous mistake. [...] > > +static struct platform_driver eq5p_driver = { > > + .driver = { > > + .name = "eyeq5-pinctrl", > > + .of_match_table = eq5p_match, > > + }, > > + .probe = eq5p_probe, > > +}; > > + > > +static int __init eq5p_init(void) > > +{ > > + return platform_driver_register(&eq5p_driver); > > +} > > +core_initcall(eq5p_init); > > No, pins are not a core_initcall. This could be arch_initcall, but > considering you depend on the parent this must be module driver. > > Even from this dependency point of view your initcalls are totally wrong > and will lead to issues. Same as reset: moved to using the builtin_platform_driver() macro. No need for it to be module as it cannot be one. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 4:19 PM CET, Rob Herring wrote: > On Tue, Jan 23, 2024 at 07:46:55PM +0100, Théo Lebrun wrote: > > Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > > support of other platforms from Mobileye. It belongs to a syscon region > > called OLB. > > > > Existing pins and their function live statically in the driver code > > rather than in the devicetree, see compatible match data. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- [...] > > diff --git a/drivers/pinctrl/pinctrl-eyeq5.c b/drivers/pinctrl/pinctrl-eyeq5.c [...] > > +static const struct eq5p_match eq5p_match_a = { > > + .regs = { > > + [EQ5P_PD] = 0x0C0, > > + [EQ5P_PU] = 0x0C4, > > + [EQ5P_DS_LOW] = 0x0D0, > > + [EQ5P_DS_HIGH] = 0x0D4, > > + [EQ5P_IOCR] = 0x0B0, > > + }, > > + .pins = eq5p_pins_a, > > + .npins = ARRAY_SIZE(eq5p_pins_a), > > + .funcs = eq5p_functions_a, > > + .nfuncs = ARRAY_SIZE(eq5p_functions_a), > > +}; > > + > > +static const struct eq5p_match eq5p_match_b = { > > + .regs = { > > + [EQ5P_PD] = 0x0C8, > > + [EQ5P_PU] = 0x0CC, > > + [EQ5P_DS_LOW] = 0x0D8, > > + [EQ5P_DS_HIGH] = 0x0DC, > > + [EQ5P_IOCR] = 0x0B4, > > + }, > > These are all the same relative offsets, so you really only need to > store the base offset. Indeed, and I don't think I had even noticed. Thanks. > The use of 2 compatibles is a bit questionable as the programming model > appears to be the same and only which pins differ. Surely there are > some other pinctrl drivers handling mutiple instances. I can confirm the programming model is the same across both banks. I've addressed your comment in my answer to yours on [PATCH v3 04/17]. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 25/01/2024 12:53, Théo Lebrun wrote: > Hi, > > On Thu Jan 25, 2024 at 8:46 AM CET, Krzysztof Kozlowski wrote: >> On 24/01/2024 17:41, Théo Lebrun wrote: >>> Hello, >>> >>> On Wed Jan 24, 2024 at 8:05 AM CET, Krzysztof Kozlowski wrote: >>>> On 23/01/2024 19:46, 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 >>>>> 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. >>>>> >>>> >>>> >>>>> +#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 struct clk_hw_onecell_data *eq5c_clk_data; >>>>> +static struct regmap *eq5c_olb; >>>> >>>> Drop these two. No file-scope regmaps for drivers. Use private container >>>> structures. >>> >>> I wouldn't know how to handle the two steps then. Two clocks and the clk >>> provider are registered at of_clk_init() using CLK_OF_DECLARE_DRIVER(). >> >> Right, if some clocks have to be early, CLK_OF_DECLARE_DRIVER needs >> static ones. But your commit subject says it is a platform driver and >> all other pieces of this code is rather incompatible with this approach. > > That is my bad on the commit subject. What do you refer to by "all other > pieces of this code is rather incompatible with this approach"? That you depend on syscon. If it was regular MMIO block in SoC space, then no problem. If you depend on anything else providing you regmap, then any initcall ordering is fragile and error-prone. Avoid. > > I've tried to minimise the use of static variables. Therefore as soon as > the probe is started, we switch to the usual way of using a private > struct that contains our info. > >> >> Do not use CLK_OF_DECLARE_DRIVER for cases where you have dependencies >> because it forces you to manually order initcalls, which is exactly what >> we do not want. > > What should I be using? I got confirmation from Stephen that this > mixture of CLK_OF_DECLARE_DRIVER() + platform driver is what I should > be using as review in my V1. > > https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/ I see. In such case I believe it is error on relying on syscon. Best regards, Krzysztof
Hi, The goal of this series is to add clk, reset and pinctrl support for the Mobileye EyeQ5 platform [0]. Control of those is grouped inside a system controller block called "OLB". About clocks, we replaced the 10 fixed clocks from the initial platform support series [0] by 10 read-only fixed-factor PLLs provided by our clock driver. We also provide one table-based divider clock for OSPI. Two PLLs (for GIC timer & UARTs) are required at of_clk_init() so those are registered first, the rest comes at platform device probe. Resets are split in three domains, all dealt with by the same device. They have some behavior differences: - We busy-wait on the first two for hardware LBIST reasons (logic built-in self-test). - Domains 0 & 2 work in a bit-per-reset fashion while domain 1 works in a register-per-reset fashion. Pin control is about controlling bias, drive strength and muxing. The latter allows two functions per pin; the first function is always GPIO while the second one is pin-dependent. There exists two banks, each handled in a separate driver instance. Each pin maps to one pin group. That makes pin & group indexes the same, simplifying logic. This series inherits from the clk V2 [1], reset V1 [2] and pinctrl V1 [3]. Those were unified to simplify handling of dt-bindings. It is based on the series "[PATCH v6 00/15] Add support for the Mobileye EyeQ5 SoC" [0] rebased onto v6.8-rc1. Here is the patch list, split by subsystems: - clk: - [PATCH V3 01/17] clk: fixed-factor: add optional accuracy support - [PATCH V3 02/17] clk: fixed-factor: add fwname-based constructor functions - [PATCH V3 05/17] dt-bindings: clock: mobileye,eyeq5-clk: add bindings - [PATCH V3 08/17] clk: eyeq5: add platform driver - pinctrl: - [PATCH V3 03/17] dt-bindings: pinctrl: allow pin controller device without unit address - [PATCH V3 07/17] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings - [PATCH V3 10/17] pinctrl: eyeq5: add platform driver - MIPS: (note: dependent on the [0] series) - [PATCH V3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller - [PATCH V3 11/17] MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000 - [PATCH V3 12/17] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon - [PATCH V3 13/17] MIPS: mobileye: eyeq5: use OLB clocks controller - [PATCH V3 14/17] MIPS: mobileye: eyeq5: add OLB reset controller node - [PATCH V3 15/17] MIPS: mobileye: eyeq5: add reset properties to uarts - [PATCH V3 16/17] MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes - [PATCH V3 17/17] MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes - reset: - [PATCH V3 06/17] dt-bindings: reset: mobileye,eyeq5-reset: add bindings - [PATCH V3 09/17] reset: eyeq5: add platform driver Thanks to Krzysztof, Rob & Stephen for the previous feedback! Have a nice day, Théo Lebrun [0]: https://lore.kernel.org/lkml/20240118155252.397947-1-gregory.clement@bootlin.com/ [1]: https://lore.kernel.org/lkml/20231227-mbly-clk-v2-0-a05db63c380f@bootlin.com/ [2]: https://lore.kernel.org/lkml/20231218-mbly-reset-v1-0-b4688b916213@bootlin.com/ [3]: https://lore.kernel.org/lkml/20231218-mbly-pinctrl-v1-0-2f7d366c2051@bootlin.com/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v3: - Unified the three series into one. - clk: split driver into two for clocks registered at of_clk_init() and clocks registered at platform device probe. - reset/bindings: drop reset dt-bindings header & add comment in driver to document known valid resets in each domain. - pinctrl/bindings: fix pinctrl.yaml to allow non unit addresses for pin controller devices. - all/bindings: remove possibility to use `mobileye,olb` phandle to get syscon. All three drivers use their parent node as syscon/regmap. - MIPS/bindings: fix bindings for OLB. Have single example in parent, removing all examples in child. - all: drop the "probed" logs. - Link to v2: https://lore.kernel.org/r/20231227-mbly-clk-v2-0-a05db63c380f@bootlin.com Changes in v2: - Drop [PATCH 1/5] that was taken by Stephen for clk-next. - Add accuracy support to fixed-factor that is enabled with a flag. Register prototypes were added to exploit this feature. - Add fw_name support to fixed-factor. This allows pointing to parent clocks using the value in `clock-names` in the DT. Register prototypes were added for that. - Bindings were modified to be less dumb: a binding was added for OLB and the clock-controller is a child property of it. Removed the possibility of pointing to OLB using a phandle. $nodename is the generic `clock-controller` and not custom `clocks`. Fix dt-bindings examples. - Fix commit message for the driver patch. Add details, remove useless fluff. - Squash both driver commits together. - Declare a platform_driver instead of using CLK_OF_DECLARE_DRIVER. This also means using `dev_*` for logging, removing `pr_fmt`. We add a pointer to device in the private structure. - Use fixed-factor instead of fixed-rate for PLLs. We don't grab a reference to the parent clk, instead using newly added fixed-factor register prototypes and fwname. - NULL is not an error when registering PLLs anymore. - Now checking the return value of of_clk_add_hw_provider for errors. - Fix includes. - Remove defensive conditional at start of eq5c_pll_parse_registers. - Rename clk_hw_to_ospi_priv to clk_to_priv to avoid confusion: it is not part of the clk_hw_* family of symbols. - Fix negative returns in eq5c_ospi_div_set_rate. It was a typo highlighted by Stephen Boyd. - Declare eq5c_ospi_div_ops as static. - In devicetree, move the OLB node prior to the UARTs, as platform device probe scheduling is dependent on devicetree ordering. This is required to declare the driver as a platform driver, else it CLK_OF_DECLARE_DRIVER is required. - In device, create a core0-timer-clk fixed clock to feed to the GIC timer. It requires a clock earlier than platform bus type init. - Link to v1: https://lore.kernel.org/r/20231218-mbly-clk-v1-0-44ce54108f06@bootlin.com --- Théo Lebrun (17): clk: fixed-factor: add optional accuracy support clk: fixed-factor: add fwname-based constructor functions dt-bindings: pinctrl: allow pin controller device without unit address dt-bindings: soc: mobileye: add EyeQ5 OLB system controller dt-bindings: clock: mobileye,eyeq5-clk: add bindings dt-bindings: reset: mobileye,eyeq5-reset: add bindings dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings clk: eyeq5: add platform driver reset: eyeq5: add platform driver pinctrl: eyeq5: add platform driver MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000 MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon MIPS: mobileye: eyeq5: use OLB clocks controller MIPS: mobileye: eyeq5: add OLB reset controller node MIPS: mobileye: eyeq5: add reset properties to uarts MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes .../bindings/clock/mobileye,eyeq5-clk.yaml | 41 ++ .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 77 +++ .../devicetree/bindings/pinctrl/pinctrl.yaml | 18 +- .../bindings/reset/mobileye,eyeq5-reset.yaml | 32 ++ .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 +++ MAINTAINERS | 8 + .../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +- arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 128 +++++ arch/mips/boot/dts/mobileye/eyeq5.dtsi | 37 +- drivers/clk/Kconfig | 11 + drivers/clk/Makefile | 1 + drivers/clk/clk-eyeq5.c | 414 ++++++++++++++ drivers/clk/clk-fixed-factor.c | 103 +++- drivers/pinctrl/Kconfig | 15 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-eyeq5.c | 595 +++++++++++++++++++++ drivers/reset/Kconfig | 12 + drivers/reset/Makefile | 1 + drivers/reset/reset-eyeq5.c | 383 +++++++++++++ include/dt-bindings/clock/mobileye,eyeq5-clk.h | 22 + include/linux/clk-provider.h | 26 +- 21 files changed, 1995 insertions(+), 61 deletions(-) --- base-commit: 84f23245916391a55be31e37e48cea4da085b100 change-id: 20231023-mbly-clk-87ce5c241f08 Best regards,