| Message ID | 20240503-mbly-olb-v2-0-95ce5a1e18fe@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add Mobileye EyeQ system controller support (clk, reset, pinctrl) | expand |
Quoting Théo Lebrun (2024-05-03 07:20:45) > Hello, > > This builds on previous EyeQ5 system-controller revisions[0], supporting > EyeQ5, EyeQ6L and EyeQ6H. We expose a few OLB system-controller > features here: > - Clocks: some read-only PLLs derived from main crystal and some > divider clocks based on PLLs. > - Resets. > - Pin controller, only on EyeQ5 (rest will use generic pinctrl-single). > > EyeQ6H is special in that it has seven instances of this > system-controller. Those are spread around and cannot be seen as a > single device, hence are exposed as seven DT nodes and seven > compatibles. > > This revision differs from previous in that it exposes all devices as a > single DT node. Driver-wise, a MFD registers multiple cells for each > device. Each driver is still in isolation from one another, each in > their respective subsystem. Why can't you use auxiliary device and driver APIs?
Hello, On Sat May 4, 2024 at 4:34 AM CEST, Stephen Boyd wrote: > Quoting Théo Lebrun (2024-05-03 07:20:45) > > This builds on previous EyeQ5 system-controller revisions[0], supporting > > EyeQ5, EyeQ6L and EyeQ6H. We expose a few OLB system-controller > > features here: > > - Clocks: some read-only PLLs derived from main crystal and some > > divider clocks based on PLLs. > > - Resets. > > - Pin controller, only on EyeQ5 (rest will use generic pinctrl-single). > > > > EyeQ6H is special in that it has seven instances of this > > system-controller. Those are spread around and cannot be seen as a > > single device, hence are exposed as seven DT nodes and seven > > compatibles. > > > > This revision differs from previous in that it exposes all devices as a > > single DT node. Driver-wise, a MFD registers multiple cells for each > > device. Each driver is still in isolation from one another, each in > > their respective subsystem. > > Why can't you use auxiliary device and driver APIs? Good question. Reasons I see: - I didn't know about auxdev beforehand. I discussed the rework with a few colleagues and none mentioned it either. - It feels simpler to let each device access iomem resources. From my understanding, an auxdev is supposed to make function calls to its parent without inheriting iomem access. That sounds like it will put the register logic/knowledge inside a single driver, which could or could not be a better option. Implementing a function like this feels like cheating: int olb_read(struct device *dev, u32 offset, u32 *val); With an MFD, we hand over a part of the iomem resource to each child and they deal with it however they like. - Syscon is what I picked to share parts of OLB to other devices that need it. Currently that is only for I2C speed mode but other devices have wrapping-related registers. MFD and syscon are deeply connected so an MFD felt natural. - That would require picking one device that is platform driver, the rest being all aux devices. Clock driver appears to be the one, same as two existing mpfs and starfive-jh7110 that use auxdev for clk and reset. Main reason I see for picking auxdev is that it forces devices to interact with a defined internal API. That can lead to nicer abstractions rather than inheriting resources as is being done in MFD. Are there other reasons? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Tue May 7, 2024 at 4:52 PM CEST, Théo Lebrun wrote: > On Sat May 4, 2024 at 4:34 AM CEST, Stephen Boyd wrote: > > Quoting Théo Lebrun (2024-05-03 07:20:45) > > > This builds on previous EyeQ5 system-controller revisions[0], supporting > > > EyeQ5, EyeQ6L and EyeQ6H. We expose a few OLB system-controller > > > features here: > > > - Clocks: some read-only PLLs derived from main crystal and some > > > divider clocks based on PLLs. > > > - Resets. > > > - Pin controller, only on EyeQ5 (rest will use generic pinctrl-single). > > > > > > EyeQ6H is special in that it has seven instances of this > > > system-controller. Those are spread around and cannot be seen as a > > > single device, hence are exposed as seven DT nodes and seven > > > compatibles. > > > > > > This revision differs from previous in that it exposes all devices as a > > > single DT node. Driver-wise, a MFD registers multiple cells for each > > > device. Each driver is still in isolation from one another, each in > > > their respective subsystem. > > > > Why can't you use auxiliary device and driver APIs? > > Good question. Reasons I see: > > - I didn't know about auxdev beforehand. I discussed the rework with a > few colleagues and none mentioned it either. > > - It feels simpler to let each device access iomem resources. From my > understanding, an auxdev is supposed to make function calls to its > parent without inheriting iomem access. That sounds like it will put > the register logic/knowledge inside a single driver, which could or > could not be a better option. > > Implementing a function like this feels like cheating: > int olb_read(struct device *dev, u32 offset, u32 *val); > > With an MFD, we hand over a part of the iomem resource to each child > and they deal with it however they like. > > - Syscon is what I picked to share parts of OLB to other devices that > need it. Currently that is only for I2C speed mode but other devices > have wrapping-related registers. MFD and syscon are deeply connected > so an MFD felt natural. > > - That would require picking one device that is platform driver, the > rest being all aux devices. Clock driver appears to be the one, same > as two existing mpfs and starfive-jh7110 that use auxdev for clk and > reset. > > Main reason I see for picking auxdev is that it forces devices to > interact with a defined internal API. That can lead to nicer > abstractions rather than inheriting resources as is being done in MFD. > > Are there other reasons? Self replying myself. I gave myself some time to think about that but I still have more thought now that I've written the previous email, and re-read almost all old revisions of this series. I do like this auxdev proposal. More so than current MFD revision. One really nice feature is that it centralises access to iomem. I've noticed recently a register that has most its fields for reset but one lost bit dealing with a clock mux. Logic to handle that would be in one location. Also, I just noticed you hinted at auxiliary devices in previous emails, which I thought was a generic term. I did not see it as a specific kernel infrastructure to be used. Sorry about that. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Quoting Théo Lebrun (2024-05-07 07:52:49) > On Sat May 4, 2024 at 4:34 AM CEST, Stephen Boyd wrote: > > > > Why can't you use auxiliary device and driver APIs? > > Good question. Reasons I see: > > - I didn't know about auxdev beforehand. I discussed the rework with a > few colleagues and none mentioned it either. > > - It feels simpler to let each device access iomem resources. From my > understanding, an auxdev is supposed to make function calls to its > parent without inheriting iomem access. That sounds like it will put > the register logic/knowledge inside a single driver, which could or > could not be a better option. You can pass the iomem pointer to the child device, either through the struct device platform_data void pointer or you can make a wrapper struct for struct auxiliary_device that the child device/driver, e.g. pinctrl, would know about. Or you can use a regmap and pass that through to the function that creates the auxiliary device. Either way, we don't want the iomem register logic inside a single driver. Conor recently made that change for mpfs. See this patch[1]. The syscon code uses a regmap so that register access uses a spinlock. Maybe you need that, or maybe you don't. I don't know. It depends on if the device has logical drivers that access some shared register. If that doesn't happen then letting the logical drivers map and access the registers with iomem accessors is fine. Otherwise, you want some sort of mediator function, where regmap helps make that easy to provide. > > Implementing a function like this feels like cheating: > int olb_read(struct device *dev, u32 offset, u32 *val); > > With an MFD, we hand over a part of the iomem resource to each child > and they deal with it however they like. > > - Syscon is what I picked to share parts of OLB to other devices that > need it. Currently that is only for I2C speed mode but other devices > have wrapping-related registers. MFD and syscon are deeply connected > so an MFD felt natural. > > - That would require picking one device that is platform driver, the > rest being all aux devices. Clock driver appears to be the one, same > as two existing mpfs and starfive-jh7110 that use auxdev for clk and > reset. > > Main reason I see for picking auxdev is that it forces devices to > interact with a defined internal API. That can lead to nicer > abstractions rather than inheriting resources as is being done in MFD. > The simple-mfd binding encourages sub-nodes for drivers. This is an anti-pattern because we want nodes for devices, not drivers. We should discourage the use of that compatible in my opinion. I could see the MFD subsystem gaining support for creating child auxiliary devices for some compatible string node, and passing those devices a regmap. Maybe that would be preferable to having to pick a driver subsystem to put the platform driver in. Outside of making a general purpose framework, you could put the platform driver in drivers/mfd and have that populate the child devices like clk, reset, pinctrl, etc. The overall goal is still the same. Don't make child nodes. [1] https://lore.kernel.org/linux-clk/20240424-strangle-sharpener-34755c5e6e3e@spud/
Hello Stephen, So, you should have received the latest revision [0]. It takes the approach you advised: - One driver (clk) is platform. Others (reset, pinctrl) are auxiliary drivers. - The clk driver spawns auxiliary devices, passing to it the iomem pointer using ->platform_data. - The auxdevs spawned are based on compatible match data. We don't need any info to spawn them except their name, so match data only has an optional string. No array needed even, just two pointers: plain, simple. - This means the iomem register logic is split across each driver. [0]: https://lore.kernel.org/lkml/20240620-mbly-olb-v3-0-5f29f8ca289c@bootlin.com/ On Tue May 7, 2024 at 11:48 PM CEST, Stephen Boyd wrote: > I could see the MFD subsystem gaining support for creating child > auxiliary devices for some compatible string node, and passing those > devices a regmap. Maybe that would be preferable to having to pick a > driver subsystem to put the platform driver in. Outside of making a > general purpose framework, you could put the platform driver in > drivers/mfd and have that populate the child devices like clk, reset, > pinctrl, etc. Having one of the driver be platform and spawn others reduces the amount of boilerplate (no driver that only creates sub devices). That sounds like a nice advantage; to be contrasted with having unrelated code in subsystems (eg auxdev spawning code in drivers/clk/). Thanks for your pieces of advice Stephen, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, This builds on previous EyeQ5 system-controller revisions[0], supporting EyeQ5, EyeQ6L and EyeQ6H. We expose a few OLB system-controller features here: - Clocks: some read-only PLLs derived from main crystal and some divider clocks based on PLLs. - Resets. - Pin controller, only on EyeQ5 (rest will use generic pinctrl-single). EyeQ6H is special in that it has seven instances of this system-controller. Those are spread around and cannot be seen as a single device, hence are exposed as seven DT nodes and seven compatibles. This revision differs from previous in that it exposes all devices as a single DT node. Driver-wise, a MFD registers multiple cells for each device. Each driver is still in isolation from one another, each in their respective subsystem. This has been requested during previous reviews and took time to implement; I'd be happy to get some feedback on this aspect. Patches are targeting MIPS, clk, reset, pinctrl and MFD: MIPS: - dt-bindings: clock: mobileye,eyeq5-clk: drop bindings - dt-bindings: clock: mobileye,eyeq5-reset: drop bindings - dt-bindings: soc: mobileye: add EyeQ OLB system controller - MIPS: mobileye: eyeq5: add OLB system-controller node Start by dropping already accepted dt-bindings that don't match current approach of single node for entire OLB (and no subnodes). Then add single dt-bindings that cover clk/reset/pinctrl features. Squash devicetree commits together into one. Adapted to having a single devicetree node without subnodes. MFD: - driver core: platform: Introduce platform_device_add_with_name() - mfd: Add cell device name - mfd: olb: Add support for Mobileye OLB system-controller There are seven instances of OLB on EyeQ6H. That means many clk/reset instances. Without naming devices properly it becomes a mess because integer IDs are not explicit. Add feature to name MFD sub-devices. Then add OLB MFD platform driver; a really simple driver. Most its content is iomem resources and MFD cells. clk: - clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS flag - clk: eyeq: add driver reset: - reset: eyeq: add platform driver pinctrl: - pinctrl: eyeq5: add platform driver Have a nice day, Théo [0]: https://lore.kernel.org/lkml/20240301-mbly-clk-v9-0-cbf06eb88708@bootlin.com/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v2: - dt-bindings: - Drop mobileye,eyeq5-clk and mobileye,eyeq5-reset bindings. - Update OLB bindings to handle clk/reset/pinctrl from OLB node. - MFD: - Add core driver and MFD patches to allow setting sub-devices names from MFD cell. - Add MFD OLB driver. - clk: - Change type of eqc_pll->reg64 from u32 to unsigned int. - Use resource indexes rather than names for iomem resources. - Put early PLLs into a separate match data table. Also, have store number of late clocks in early match data to properly alloc cells. - Pre-acquire all divclk resources first, then register them. This simplifies code. - Extract PLLs and divclks init to two separate functions. - Avoid variable declarations in loop bodies. - Do not register match data table to platform driver. It gets probed as MFD sub-device matching on driver name. Match data table is matched against parent OF node compatible. - Fix ugly memory corruption bug when clk count == 1. - reset: - EQR_EYEQ5_SARCR and EQR_EYEQ6H_SARCR did not use offset 0x0: do minus four to all their offsets and reduce resource sizes. - Remove resource names. Reset i uses iomem resource index i. - Simplify xlate: have two implementations for of_reset_n_cells==1 and of_reset_n_cells==2. Both call the same helper internal function. - Do not register match data table to platform driver. It gets probed as MFD sub-device matching on driver name. Match data table is matched against parent OF node compatible. - pinctrl: - Remove match data table to platform driver. It gets probed as MFD sub-device matching on driver name. Driver has single compatible. - Drop "Reviewed-by: Linus Walleij" as driver changed approach. - MIPS DTS: - Squash all commits together into a single one. - Adapt to new approach: OLB is now a single OF node. - Link to v1: https://lore.kernel.org/r/20240410-mbly-olb-v1-0-335e496d7be3@bootlin.com --- Théo Lebrun (11): dt-bindings: clock: mobileye,eyeq5-clk: drop bindings dt-bindings: clock: mobileye,eyeq5-reset: drop bindings dt-bindings: soc: mobileye: add EyeQ OLB system controller driver core: platform: Introduce platform_device_add_with_name() mfd: Add cell device name mfd: olb: Add support for Mobileye OLB system-controller clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS flag clk: eyeq: add driver reset: eyeq: add platform driver pinctrl: eyeq5: add platform driver MIPS: mobileye: eyeq5: add OLB system-controller node .../bindings/clock/mobileye,eyeq5-clk.yaml | 51 -- .../bindings/reset/mobileye,eyeq5-reset.yaml | 43 -- .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 375 +++++++++++ MAINTAINERS | 5 + .../{eyeq5-fixed-clocks.dtsi => eyeq5-clocks.dtsi} | 54 +- arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 125 ++++ arch/mips/boot/dts/mobileye/eyeq5.dtsi | 22 +- drivers/base/platform.c | 17 +- drivers/clk/Kconfig | 11 + drivers/clk/Makefile | 1 + drivers/clk/clk-divider.c | 12 +- drivers/clk/clk-eyeq.c | 690 +++++++++++++++++++++ drivers/mfd/Kconfig | 10 + drivers/mfd/Makefile | 2 + drivers/mfd/mfd-core.c | 2 +- drivers/mfd/mobileye-olb.c | 180 ++++++ drivers/pinctrl/Kconfig | 14 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-eyeq5.c | 573 +++++++++++++++++ drivers/reset/Kconfig | 13 + drivers/reset/Makefile | 1 + drivers/reset/reset-eyeq.c | 541 ++++++++++++++++ include/dt-bindings/clock/mobileye,eyeq5-clk.h | 21 + include/linux/clk-provider.h | 11 +- include/linux/mfd/core.h | 19 +- include/linux/platform_device.h | 12 +- 26 files changed, 2651 insertions(+), 155 deletions(-) --- base-commit: d5a00175dce1740a3e9d519933ba76f9ce5cbd24 change-id: 20240408-mbly-olb-75a85f5cfde3 Best regards,