Message ID | 20240131-mbly-clk-v4-0-bcd00510d6a0@bootlin.com |
---|---|
Headers | show |
Series | Add support for Mobileye EyeQ5 system controller | expand |
Hi Theo, thanks for your patches! A *new* MIPS platform, not every day I see this! On Wed, Jan 31, 2024 at 5:27 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > 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. Can the three pin control patches be merged separately? (It looks like.) That would make my job easier when we ge there. I will try to look closer at each patch! Yours, Linus Walleij
Hi Theo, thanks for your patch! On Wed, Jan 31, 2024 at 5:27 PM Théo Lebrun <theo.lebrun@bootlin.com> 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> The driver looks very nice and is using all standard features, I'm pretty sure we can merge this soon. > +static void eq5p_update_bits(const struct eq5p_pinctrl *pctrl, > + enum eq5p_bank bank, enum eq5p_regs reg, > + u32 mask, u32 val) > +{ > + void __iomem *ptr = pctrl->base + eq5p_regs[bank][reg]; > + > + writel((readl(ptr) & ~mask) | (val & mask), ptr); > +} This is in practice a reimplementation of regmap MMIO. Can't you just use regmap MMIO to access the banks then...? Maybe it doesn't add much here. I'm not sure. > +static bool eq5p_readl_bit(const struct eq5p_pinctrl *pctrl, eq5p_test_bit() maybe? that describes better what the function does. > + enum eq5p_bank bank, enum eq5p_regs reg, int bit) > +{ > + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]); > + > + return (val & BIT(bit)) != 0; > +} Maybe add a check for bit > 31? > +static int eq5p_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const unsigned int **pins, > + unsigned int *num_pins) > +{ > + *pins = &pctldev->desc->pins[selector].number; > + *num_pins = 1; > + return 0; > +} One pin per group, also known as the "qualcomm trick". (It's fine.) > + mask = 0b11 << offset; That's pretty nonstandard but it's quite readable so let's keep it! Yours, Linus Walleij
On 31/01/2024 17:26, Théo Lebrun wrote: > Node names should be generic. OLB, meaning "Other Logic Block", is a > name specific to this platform. Change the node name to the generic and > often-used "system-controller". > > See §2.2.2. "Generic Names Recommendation" in the devicetree > specification. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > arch/mips/boot/dts/mobileye/eyeq5.dtsi | 2 +- There is no such file in next-20240201 and your cover letter does not link to any dependency. Something is not right. Best regards, Krzysztof
On 01/02/2024 10:10, Krzysztof Kozlowski wrote: > On 31/01/2024 17:26, Théo Lebrun wrote: >> Node names should be generic. OLB, meaning "Other Logic Block", is a >> name specific to this platform. Change the node name to the generic and >> often-used "system-controller". >> >> See §2.2.2. "Generic Names Recommendation" in the devicetree >> specification. >> >> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >> --- >> arch/mips/boot/dts/mobileye/eyeq5.dtsi | 2 +- > > There is no such file in next-20240201 and your cover letter does not > link to any dependency. Something is not right. Ah, I found it now. Best regards, Krzysztof
On 31/01/2024 17:26, Théo Lebrun wrote: > UART nodes have been added to the devicetree by the initial platform > support patch series. Add reset properties now that the reset node is > declared. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > arch/mips/boot/dts/mobileye/eyeq5.dtsi | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi > index 06e941b0ce10..ece71cafb6ee 100644 > --- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi > +++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi > @@ -78,6 +78,7 @@ uart0: serial@800000 { > interrupts = <GIC_SHARED 6 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&uart_clk>, <&occ_periph>; > clock-names = "uartclk", "apb_pclk"; > + resets = <&reset 0 10>; You touch the same file. Squash the patch with previous one. It's the same logical change to add reset to entire SoC. You don't add half of reset, right? Best regards, Krzysztof
Hi Linus, On Wed Jan 31, 2024 at 9:55 PM CET, Linus Walleij wrote: > Hi Theo, > > thanks for your patch! > > On Wed, Jan 31, 2024 at 5:27 PM Théo Lebrun <theo.lebrun@bootlin.com> 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> > > The driver looks very nice and is using all standard features, I'm pretty sure > we can merge this soon. It is useful to get some feedback that tells me your state of mind as one involved maintainer. Thanks. > > > +static void eq5p_update_bits(const struct eq5p_pinctrl *pctrl, > > + enum eq5p_bank bank, enum eq5p_regs reg, > > + u32 mask, u32 val) > > +{ > > + void __iomem *ptr = pctrl->base + eq5p_regs[bank][reg]; > > + > > + writel((readl(ptr) & ~mask) | (val & mask), ptr); > > +} > > This is in practice a reimplementation of regmap MMIO. > > Can't you just use regmap MMIO to access the banks then...? > > Maybe it doesn't add much here. I'm not sure. Indeed, I went the minimalist route. You tell me if you'd prefer an MMIO regmap. I've not seen any helper to get a regmap based on a resource, targeting by name. Is the expected procedure to acquire the resource then create a regmap config then call devm_regmap_init_mmio()? > > > +static bool eq5p_readl_bit(const struct eq5p_pinctrl *pctrl, > > eq5p_test_bit() maybe? that describes better what the > function does. Good idea, thanks. > > > + enum eq5p_bank bank, enum eq5p_regs reg, int bit) > > +{ > > + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]); > > + > > + return (val & BIT(bit)) != 0; > > +} > > Maybe add a check for bit > 31? Will do. I like that sort of defensive programming. What behavior would you expect? - WARN_ON(bit > 31) and return false? - Just return false? - Something else? Actually looking at uses of eq5p_readl_bit() I'm thinking about a bug that might be occuring wrt the second bank and that offset. I'll make sure to fix it for next revision. > > +static int eq5p_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, > > + unsigned int selector, > > + const unsigned int **pins, > > + unsigned int *num_pins) > > +{ > > + *pins = &pctldev->desc->pins[selector].number; > > + *num_pins = 1; > > + return 0; > > +} > > One pin per group, also known as the "qualcomm trick". > > (It's fine.) :-) Thanks! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello Linus, On Wed Jan 31, 2024 at 9:44 PM CET, Linus Walleij wrote: > thanks for your patches! > > A *new* MIPS platform, not every day I see this! Indeed! According the Wikipedia it got released to market on 2021, which does sound recent from a MIPS standpoint. (The same year MIPS announced the architecture would stop being developed in favor of RISC-V.) > On Wed, Jan 31, 2024 at 5:27 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > 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. > > Can the three pin control patches be merged separately? (It looks like.) That is the goal. There are two dependencies in this series: - MIPS stuff depends on the base platform support series by Grégory, for devicetree stuff & MAINTAINERS mostly. - "dt-bindings: soc: mobileye: add EyeQ5 OLB system controller" depends on the three dt-bindings for the controllers (clk+reset+pinctrl) as it references them. That means clk+reset+pinctrl can go in separately. At least that is the goal, hoping I have not messed up along the way. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Thu Feb 1, 2024 at 10:13 AM CET, Krzysztof Kozlowski wrote: > On 31/01/2024 17:26, Théo Lebrun wrote: > > UART nodes have been added to the devicetree by the initial platform > > support patch series. Add reset properties now that the reset node is > > declared. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > arch/mips/boot/dts/mobileye/eyeq5.dtsi | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi > > index 06e941b0ce10..ece71cafb6ee 100644 > > --- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi > > +++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi > > @@ -78,6 +78,7 @@ uart0: serial@800000 { > > interrupts = <GIC_SHARED 6 IRQ_TYPE_LEVEL_HIGH>; > > clocks = <&uart_clk>, <&occ_periph>; > > clock-names = "uartclk", "apb_pclk"; > > + resets = <&reset 0 10>; > > You touch the same file. Squash the patch with previous one. It's the > same logical change to add reset to entire SoC. You don't add half of > reset, right? Makes sense. I'll update the commit message with that change. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Feb 1, 2024 at 11:24 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Can't you just use regmap MMIO to access the banks then...? > > > > Maybe it doesn't add much here. I'm not sure. > > Indeed, I went the minimalist route. You tell me if you'd prefer an MMIO > regmap. I'm not sure, because it might be that it adds more overhead than it saves and then it is pointless. > I've not seen any helper to get a regmap based on a resource, targeting > by name. Is the expected procedure to acquire the resource then create > a regmap config then call devm_regmap_init_mmio()? No... haven't seen such a thing. > > > + enum eq5p_bank bank, enum eq5p_regs reg, int bit) > > > +{ > > > + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]); > > > + > > > + return (val & BIT(bit)) != 0; > > > +} > > > > Maybe add a check for bit > 31? > > Will do. I like that sort of defensive programming. What behavior would > you expect? > - WARN_ON(bit > 31) and return false? > - Just return false? > - Something else? Your pick is as good as mine :D I let the author choose what to do there. Yours, Linus Walleij
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. The patch adding the system-controller dt-bindings ("dt-bindings: soc: mobileye: add EyeQ5 OLB system controller") is dependent on the three controllers dt-bindings: - dt-bindings: clock: mobileye,eyeq5-clk: add bindings - dt-bindings: reset: mobileye,eyeq5-reset: add bindings - dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings The parent is v6.8-rc2 with the "[PATCH v6 00/15] Add support for the Mobileye EyeQ5 SoC" series [0] rebased on top. Here is the patch list, split by subsystems: - clk: [PATCH V4 01/18] clk: fixed-factor: add optional accuracy support [PATCH V4 02/18] clk: fixed-factor: add fwname-based constructor functions [PATCH V4 04/18] dt-bindings: clock: mobileye,eyeq5-clk: add bindings [PATCH V4 08/18] clk: eyeq5: add platform driver, and init routine at of_clk_init() - pinctrl: [PATCH V4 03/18] dt-bindings: pinctrl: allow pin controller device without unit address [PATCH V4 06/18] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings [PATCH V4 10/18] pinctrl: eyeq5: add platform driver - reset: [PATCH V4 05/18] dt-bindings: reset: mobileye,eyeq5-reset: add bindings [PATCH V4 09/18] reset: eyeq5: add platform driver - MIPS: (note: dependent on the [0] series) [PATCH V4 07/18] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller [PATCH V4 11/18] MIPS: mobileye: eyeq5: rename olb@e00000 to system-controller@e00000 [PATCH V4 12/18] MIPS: mobileye: eyeq5: remove reg-io-width property from OLB syscon [PATCH V4 13/18] MIPS: mobileye: eyeq5: add memory translation inside OLB syscon [PATCH V4 14/18] MIPS: mobileye: eyeq5: use OLB clocks controller [PATCH V4 15/18] MIPS: mobileye: eyeq5: add OLB reset controller node [PATCH V4 16/18] MIPS: mobileye: eyeq5: add reset properties to UARTs [PATCH V4 17/18] MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes [PATCH V4 18/18] MIPS: mobileye: eyeq5: add pinctrl properties to UART nodes Thanks to Andrew, Krzysztof, Philipp, Rob, Sergei & 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/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v4: - Have the three drivers access MMIO directly rather than through the syscon & regmap. - pinctrl: Make the pin controller handle both banks using a single instance. - pinctrl/dt-bindings: Add if/else for each function, to strictly define possible functions. - clk: Changing to direct MMIO means we can use clk_hw_register_divider_table_parent_hw() for the OSPI table-based divider clock. - Use builtin_platform_driver() for platform driver registering instead of manual initcalls. - reset: follow Philipp & Krzysztof's feedback: - Use container_of() to get private struct. - Use '_withlock' suffix instead of the underscore prefix. - Use udelay() instead of the non-standard __udelay(). - Remove useless checks. - Use mutex guards. - Remove the ->reset() implementation. - Use devres variants for kzalloc() and reset_controller_register(). - Other small changes following feedback from reviewers. dt-bindings whitespace for pinctrl.yaml, fix pinctrl driver dt-bindings description, improve clk driver commit header, etc. - Link to v3: https://lore.kernel.org/r/20240123-mbly-clk-v3-0-392b010b8281@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 (18): 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: clock: mobileye,eyeq5-clk: add bindings dt-bindings: reset: mobileye,eyeq5-reset: add bindings dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings dt-bindings: soc: mobileye: add EyeQ5 OLB system controller clk: eyeq5: add platform driver, and init routine at of_clk_init() 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: add memory translation inside 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 | 52 ++ .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 242 +++++++++ .../devicetree/bindings/pinctrl/pinctrl.yaml | 18 +- .../bindings/reset/mobileye,eyeq5-reset.yaml | 44 ++ .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 89 ++++ MAINTAINERS | 8 + .../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +- arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 125 +++++ arch/mips/boot/dts/mobileye/eyeq5.dtsi | 39 +- drivers/clk/Kconfig | 11 + drivers/clk/Makefile | 1 + drivers/clk/clk-eyeq5.c | 289 +++++++++++ drivers/clk/clk-fixed-factor.c | 103 +++- drivers/pinctrl/Kconfig | 15 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-eyeq5.c | 567 +++++++++++++++++++++ drivers/reset/Kconfig | 12 + drivers/reset/Makefile | 1 + drivers/reset/reset-eyeq5.c | 342 +++++++++++++ include/dt-bindings/clock/mobileye,eyeq5-clk.h | 22 + include/linux/clk-provider.h | 26 +- 21 files changed, 2000 insertions(+), 61 deletions(-) --- base-commit: b93830a88d7a3a18a92ff7a1a9272934ca1bade1 change-id: 20231023-mbly-clk-87ce5c241f08 Best regards,