| 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
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
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 9:58 AM CET, Krzysztof Kozlowski wrote: > On 31/01/2024 17:26, Théo Lebrun wrote: > > Add DT schema bindings for the EyeQ5 clock controller driver. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > No changelog, tags ignored, I scrolled through first two pages of cover > letter and also no changelog. In this case we fit into the "If a tag was not added on purpose". Sorry the changelog was not explicit enough. In my mind it fits into the first bullet point of the cover letter changelog: > - Have the three drivers access MMIO directly rather than through the > syscon & regmap. That change means important changes to the dt-bindings to adapt to this new behavior. In particular we now have reg and reg-names properties that got added and made required. I wanted to have your review on that and did not want to tag the patch as already reviewed. > > This is a friendly reminder during the review process. > > It looks like you received a tag and forgot to add it. > > If you do not know the process, here is a short explanation: > Please add Acked-by/Reviewed-by/Tested-by tags when posting new > versions, under or above your Signed-off-by tag. Tag is "received", when > provided in a message replied to you on the mailing list. Tools like b4 > can help here. However, there's no need to repost patches *only* to add > the tags. The upstream maintainer will do that for tags received on the > version they apply. > > https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577 > > If a tag was not added on purpose, please state why and what changed. As an aside, what's your preference on location for this information? Cover letter changelog? Following '---' in the specific commit message? Somewhere else? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 01/02/2024 11:38, Théo Lebrun wrote: > Hello, > > On Thu Feb 1, 2024 at 9:58 AM CET, Krzysztof Kozlowski wrote: >> On 31/01/2024 17:26, Théo Lebrun wrote: >>> Add DT schema bindings for the EyeQ5 clock controller driver. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >> >> No changelog, tags ignored, I scrolled through first two pages of cover >> letter and also no changelog. > > In this case we fit into the "If a tag was not added on purpose". Sorry > the changelog was not explicit enough. In my mind it fits into the > first bullet point of the cover letter changelog: > >> - Have the three drivers access MMIO directly rather than through the >> syscon & regmap. ... which I might not even connect to binding patches. I see only one entry regarding bindings in your changelog, so I find it not much informative. For the future, please state that you ignore tags for given reason. > > That change means important changes to the dt-bindings to adapt to this > new behavior. In particular we now have reg and reg-names properties > that got added and made required. > > I wanted to have your review on that and did not want to tag the patch > as already reviewed. Makes sense, but how can I know it? Other people often ignore the tags, so safe assumption is that it happened here as well. > >> >> This is a friendly reminder during the review process. >> >> It looks like you received a tag and forgot to add it. >> >> If you do not know the process, here is a short explanation: >> Please add Acked-by/Reviewed-by/Tested-by tags when posting new >> versions, under or above your Signed-off-by tag. Tag is "received", when >> provided in a message replied to you on the mailing list. Tools like b4 >> can help here. However, there's no need to repost patches *only* to add >> the tags. The upstream maintainer will do that for tags received on the >> version they apply. >> >> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577 >> >> If a tag was not added on purpose, please state why and what changed. > > As an aside, what's your preference on location for this information? > Cover letter changelog? Following '---' in the specific commit message? > Somewhere else? Both are accepted, but if you do it in cover letter, it should be obvious for the reader that patches XYZ were changed. It's not. Best regards, Krzysztof
Hello, On Thu Feb 1, 2024 at 12:00 PM CET, Krzysztof Kozlowski wrote: > On 01/02/2024 11:38, Théo Lebrun wrote: > > Hello, > > > > On Thu Feb 1, 2024 at 9:58 AM CET, Krzysztof Kozlowski wrote: > >> On 31/01/2024 17:26, Théo Lebrun wrote: > >>> Add DT schema bindings for the EyeQ5 clock controller driver. > >>> > >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > >>> --- > >> > >> No changelog, tags ignored, I scrolled through first two pages of cover > >> letter and also no changelog. > > > > In this case we fit into the "If a tag was not added on purpose". Sorry > > the changelog was not explicit enough. In my mind it fits into the > > first bullet point of the cover letter changelog: > > > >> - Have the three drivers access MMIO directly rather than through the > >> syscon & regmap. > > ... which I might not even connect to binding patches. I see only one > entry regarding bindings in your changelog, so I find it not much > informative. > > For the future, please state that you ignore tags for given reason. > > > > > That change means important changes to the dt-bindings to adapt to this > > new behavior. In particular we now have reg and reg-names properties > > that got added and made required. > > > > I wanted to have your review on that and did not want to tag the patch > > as already reviewed. > > Makes sense, but how can I know it? Other people often ignore the tags, > so safe assumption is that it happened here as well. I'm prepping a new revision. Should I be taking your previous Reviewed-By tags in? You sent them for the previous revision, do the changes in this V4 look good to you? Thanks Krzysztof, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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,