| Message ID | 20240503-mbly-olb-v2-0-95ce5a1e18fe@bootlin.com |
|---|---|
| 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/
On Fri, 03 May 2024, Théo Lebrun wrote: > Mobileye OLB system-controller gets used in EyeQ5, EyeQ6L and EyeQ6H > platforms. It hosts clock, reset and pinctrl functionality. > > Tiny iomem resources are declared for all cells. Some features are > spread apart. Pinctrl is only used on EyeQ5. > > EyeQ6H is special: it hosts seven OLB controllers, each with a > compatible. That means many clock and reset cells. Use cell->devname > for explicit device names rather than clk-eyeq.ID or clk-eyeq.ID.auto. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/mfd/Kconfig | 10 +++ > drivers/mfd/Makefile | 2 + > drivers/mfd/mobileye-olb.c | 180 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 192 insertions(+) > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 4b023ee229cf..d004a3f4d493 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1030,6 +1030,16 @@ config MFD_OCELOT > > If unsure, say N. > > +config MFD_OLB > + bool "Mobileye EyeQ OLB System Controller Support" > + select MFD_CORE > + depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST > + default MACH_EYEQ5 || MACH_EYEQ6H > + help > + Say yes here to add support for EyeQ platforms (EyeQ5, EyeQ6L and > + EyeQ6H). This core MFD platform driver provides clock, reset and > + pinctrl (only EyeQ5) support. > + > config EZX_PCAP > bool "Motorola EZXPCAP Support" > depends on SPI_MASTER > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index c66f07edcd0e..d872833966a8 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -120,6 +120,8 @@ obj-$(CONFIG_MFD_CORE) += mfd-core.o > ocelot-soc-objs := ocelot-core.o ocelot-spi.o > obj-$(CONFIG_MFD_OCELOT) += ocelot-soc.o > > +obj-$(CONFIG_MFD_OLB) += mobileye-olb.o > + > obj-$(CONFIG_EZX_PCAP) += ezx-pcap.o > obj-$(CONFIG_MFD_CPCAP) += motorola-cpcap.o > > diff --git a/drivers/mfd/mobileye-olb.c b/drivers/mfd/mobileye-olb.c > new file mode 100644 > index 000000000000..1640d63a3ddd > --- /dev/null > +++ b/drivers/mfd/mobileye-olb.c > @@ -0,0 +1,180 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * System controller multi-function device for EyeQ platforms. > + * > + * Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms have MMIO mapped registers > + * controlling core platform clocks, resets and pin control. Many other > + * features are present and not yet exposed. > + * > + * Declare cells for each compatible. Only EyeQ5 has pinctrl. > + * EyeQ6H has seven OLB instances; each has a name which we propagate to > + * sub-devices using cell->devname. > + * > + * Copyright (C) 2024 Mobileye Vision Technologies Ltd. > + */ > + > +#include <linux/array_size.h> > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/ioport.h> > +#include <linux/mfd/core.h> > +#include <linux/mod_devicetable.h> > +#include <linux/platform_device.h> > +#include <linux/property.h> > + > +#define OLB_MFD_CELL(_name, _res, _devname) \ > + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, false, NULL, _devname) The reason we provide generic MACROs is so that you don't have to define your own. > +struct olb_match_data { > + const struct mfd_cell *cells; > + int nb_cells; /* int to match devm_mfd_add_devices() argument */ > +}; > + > +#define OLB_DATA(_cells) { .cells = (_cells), .nb_cells = ARRAY_SIZE(_cells) } > + > +static int olb_probe(struct platform_device *pdev) > +{ > + const struct olb_match_data *match_data; > + struct device *dev = &pdev->dev; > + struct resource *res; > + > + match_data = device_get_match_data(dev); > + if (!match_data) > + return -ENODEV; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > + match_data->cells, match_data->nb_cells, > + res, 0, NULL); > +} > + > +static const struct resource olb_eyeq5_clk_resources[] = { > + DEFINE_RES_MEM_NAMED(0x02C, 10 * 8, "pll"), > + DEFINE_RES_MEM_NAMED(0x11C, 1 * 4, "ospi"), > +}; > + > +static const struct resource olb_eyeq5_reset_resources[] = { > + DEFINE_RES_MEM_NAMED(0x004, 2 * 4, "d0"), > + DEFINE_RES_MEM_NAMED(0x200, 13 * 4, "d1"), > + DEFINE_RES_MEM_NAMED(0x120, 1 * 4, "d2"), > +}; > + > +static const struct resource olb_eyeq5_pinctrl_resources[] = { > + DEFINE_RES_MEM_NAMED(0x0B0, 12 * 4, "pinctrl"), > +}; > + > +static const struct mfd_cell olb_eyeq5_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq5_clk_resources, NULL), > + OLB_MFD_CELL("reset-eyeq", olb_eyeq5_reset_resources, NULL), > + OLB_MFD_CELL("eyeq5-pinctrl", olb_eyeq5_pinctrl_resources, NULL), > +}; > + > +static const struct olb_match_data olb_eyeq5_match_data = OLB_DATA(olb_eyeq5_cells); > + > +static const struct resource olb_eyeq6l_clk_resources[] = { > + DEFINE_RES_MEM_NAMED(0x02C, 4 * 8, "pll"), > +}; > + > +static const struct resource olb_eyeq6l_reset_resources[] = { > + DEFINE_RES_MEM_NAMED(0x004, 2 * 4, "d0"), > + DEFINE_RES_MEM_NAMED(0x200, 13 * 4, "d1"), > +}; > + > +static const struct mfd_cell olb_eyeq6l_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6l_clk_resources, NULL), > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6l_reset_resources, NULL), > +}; > + > +static const struct olb_match_data olb_eyeq6l_match_data = OLB_DATA(olb_eyeq6l_cells); > + > +static const struct resource olb_eyeq6h_acc_clk_resources[] = { > + DEFINE_RES_MEM_NAMED(0x040, 7 * 8, "pll"), > +}; > + > +static const struct resource olb_eyeq6h_acc_reset_resources[] = { > + DEFINE_RES_MEM_NAMED(0x000, 15 * 4, "d0"), > +}; > + > +static const struct mfd_cell olb_eyeq6h_acc_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_acc_clk_resources, "clk-eyeq-acc"), The point of enumerating platform device names is to identify devices that are identical. We lose this with bespoke naming. If you want to identify devices either define a value to pass to .id or adapt the first parameter and make the clk-eyeq driver accept different device names. > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6h_acc_reset_resources, "reset-eyeq-acc"), > +}; > + > +static const struct olb_match_data olb_eyeq6h_acc_match_data = OLB_DATA(olb_eyeq6h_acc_cells); > + > +static const struct resource olb_eyeq6h_we_clk_resources[] = { > + DEFINE_RES_MEM_NAMED(0x074, 1 * 8, "pll"), > +}; > + > +static const struct resource olb_eyeq6h_we_reset_resources[] = { > + DEFINE_RES_MEM_NAMED(0x004, 4 * 4, "d0"), > +}; > + > +static const struct mfd_cell olb_eyeq6h_west_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_we_clk_resources, "clk-eyeq-west"), > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6h_we_reset_resources, "reset-eyeq-west"), > +}; > + > +static const struct olb_match_data olb_eyeq6h_west_match_data = OLB_DATA(olb_eyeq6h_west_cells); > + > +static const struct mfd_cell olb_eyeq6h_east_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_we_clk_resources, "clk-eyeq-east"), > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6h_we_reset_resources, "reset-eyeq-east"), > +}; > + > +static const struct olb_match_data olb_eyeq6h_east_match_data = OLB_DATA(olb_eyeq6h_east_cells); > + > +static const struct resource olb_eyeq6h_south_clk_resources[] = { > + DEFINE_RES_MEM_NAMED(0x000, 4 * 8, "pll"), > + DEFINE_RES_MEM_NAMED(0x070, 1 * 4, "emmc"), > + DEFINE_RES_MEM_NAMED(0x090, 1 * 4, "ospi"), > + DEFINE_RES_MEM_NAMED(0x098, 1 * 4, "tsu"), > +}; > + > +static const struct mfd_cell olb_eyeq6h_south_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_south_clk_resources, "clk-eyeq-south"), > +}; > + > +static const struct olb_match_data olb_eyeq6h_south_match_data = OLB_DATA(olb_eyeq6h_south_cells); > + > +static const struct resource olb_eyeq6h_ddr_clk_resources[] = { > + DEFINE_RES_MEM_NAMED(0x074, 1 * 8, "pll"), > +}; > + > +static const struct mfd_cell olb_eyeq6h_ddr0_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_ddr_clk_resources, "clk-eyeq-ddr0"), > +}; > + > +static const struct olb_match_data olb_eyeq6h_ddr0_match_data = OLB_DATA(olb_eyeq6h_ddr0_cells); > + > +static const struct mfd_cell olb_eyeq6h_ddr1_cells[] = { > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_ddr_clk_resources, "clk-eyeq-ddr1"), > +}; > + > +static const struct olb_match_data olb_eyeq6h_ddr1_match_data = OLB_DATA(olb_eyeq6h_ddr1_cells); > + > +static const struct of_device_id olb_of_match[] = { > + { .compatible = "mobileye,eyeq5-olb", .data = &olb_eyeq5_match_data }, We're not passing MFD init data through the OF API, sorry. Pass defined identifiers through instead and match on those please. > + { .compatible = "mobileye,eyeq6l-olb", .data = &olb_eyeq6l_match_data }, > + { .compatible = "mobileye,eyeq6h-acc-olb", .data = &olb_eyeq6h_acc_match_data }, > + /* No central: it only has an early clock handled using CLK_OF_DECLARE_DRIVER(). */ > + { .compatible = "mobileye,eyeq6h-east-olb", .data = &olb_eyeq6h_east_match_data }, > + { .compatible = "mobileye,eyeq6h-west-olb", .data = &olb_eyeq6h_west_match_data }, > + { .compatible = "mobileye,eyeq6h-south-olb", .data = &olb_eyeq6h_south_match_data }, > + { .compatible = "mobileye,eyeq6h-ddr0-olb", .data = &olb_eyeq6h_ddr0_match_data }, > + { .compatible = "mobileye,eyeq6h-ddr1-olb", .data = &olb_eyeq6h_ddr1_match_data }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, olb_of_match); > + > +static struct platform_driver olb_driver = { > + .probe = olb_probe, > + .driver = { > + .name = "olb", > + .of_match_table = olb_of_match, > + }, > +}; > +builtin_platform_driver(olb_driver); > > -- > 2.45.0 >
Hello Lee, The latest revision [0] has changed approach following advice from Stephen Boyd to rather move forward with auxiliary devices. [0]: https://lore.kernel.org/lkml/20240620-mbly-olb-v3-0-5f29f8ca289c@bootlin.com/ On Fri May 31, 2024 at 1:05 PM CEST, Lee Jones wrote: > On Fri, 03 May 2024, Théo Lebrun wrote: > > > Mobileye OLB system-controller gets used in EyeQ5, EyeQ6L and EyeQ6H > > platforms. It hosts clock, reset and pinctrl functionality. > > > > Tiny iomem resources are declared for all cells. Some features are > > spread apart. Pinctrl is only used on EyeQ5. > > > > EyeQ6H is special: it hosts seven OLB controllers, each with a > > compatible. That means many clock and reset cells. Use cell->devname > > for explicit device names rather than clk-eyeq.ID or clk-eyeq.ID.auto. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/mfd/Kconfig | 10 +++ > > drivers/mfd/Makefile | 2 + > > drivers/mfd/mobileye-olb.c | 180 +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 192 insertions(+) > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 4b023ee229cf..d004a3f4d493 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1030,6 +1030,16 @@ config MFD_OCELOT > > > > If unsure, say N. > > > > +config MFD_OLB > > + bool "Mobileye EyeQ OLB System Controller Support" > > + select MFD_CORE > > + depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST > > + default MACH_EYEQ5 || MACH_EYEQ6H > > + help > > + Say yes here to add support for EyeQ platforms (EyeQ5, EyeQ6L and > > + EyeQ6H). This core MFD platform driver provides clock, reset and > > + pinctrl (only EyeQ5) support. > > + > > config EZX_PCAP > > bool "Motorola EZXPCAP Support" > > depends on SPI_MASTER > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index c66f07edcd0e..d872833966a8 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -120,6 +120,8 @@ obj-$(CONFIG_MFD_CORE) += mfd-core.o > > ocelot-soc-objs := ocelot-core.o ocelot-spi.o > > obj-$(CONFIG_MFD_OCELOT) += ocelot-soc.o > > > > +obj-$(CONFIG_MFD_OLB) += mobileye-olb.o > > + > > obj-$(CONFIG_EZX_PCAP) += ezx-pcap.o > > obj-$(CONFIG_MFD_CPCAP) += motorola-cpcap.o > > > > diff --git a/drivers/mfd/mobileye-olb.c b/drivers/mfd/mobileye-olb.c > > new file mode 100644 > > index 000000000000..1640d63a3ddd > > --- /dev/null > > +++ b/drivers/mfd/mobileye-olb.c > > @@ -0,0 +1,180 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * System controller multi-function device for EyeQ platforms. > > + * > > + * Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms have MMIO mapped registers > > + * controlling core platform clocks, resets and pin control. Many other > > + * features are present and not yet exposed. > > + * > > + * Declare cells for each compatible. Only EyeQ5 has pinctrl. > > + * EyeQ6H has seven OLB instances; each has a name which we propagate to > > + * sub-devices using cell->devname. > > + * > > + * Copyright (C) 2024 Mobileye Vision Technologies Ltd. > > + */ > > + > > +#include <linux/array_size.h> > > +#include <linux/device.h> > > +#include <linux/errno.h> > > +#include <linux/ioport.h> > > +#include <linux/mfd/core.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/platform_device.h> > > +#include <linux/property.h> > > + > > +#define OLB_MFD_CELL(_name, _res, _devname) \ > > + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, false, NULL, _devname) > > The reason we provide generic MACROs is so that you don't have to define > your own. In this case, there was no macro that allowed setting the MFD cell devname field, which is something that was added in the series. I didn't know what generic macro to create so instead of creating loads of useless ones I created a single one, targeted at my usecase. > > > +struct olb_match_data { > > + const struct mfd_cell *cells; > > + int nb_cells; /* int to match devm_mfd_add_devices() argument */ > > +}; > > + > > +#define OLB_DATA(_cells) { .cells = (_cells), .nb_cells = ARRAY_SIZE(_cells) } > > + > > +static int olb_probe(struct platform_device *pdev) > > +{ > > + const struct olb_match_data *match_data; > > + struct device *dev = &pdev->dev; > > + struct resource *res; > > + > > + match_data = device_get_match_data(dev); > > + if (!match_data) > > + return -ENODEV; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -ENODEV; > > + > > + return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > > + match_data->cells, match_data->nb_cells, > > + res, 0, NULL); > > +} > > + > > +static const struct resource olb_eyeq5_clk_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x02C, 10 * 8, "pll"), > > + DEFINE_RES_MEM_NAMED(0x11C, 1 * 4, "ospi"), > > +}; > > + > > +static const struct resource olb_eyeq5_reset_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x004, 2 * 4, "d0"), > > + DEFINE_RES_MEM_NAMED(0x200, 13 * 4, "d1"), > > + DEFINE_RES_MEM_NAMED(0x120, 1 * 4, "d2"), > > +}; > > + > > +static const struct resource olb_eyeq5_pinctrl_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x0B0, 12 * 4, "pinctrl"), > > +}; > > + > > +static const struct mfd_cell olb_eyeq5_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq5_clk_resources, NULL), > > + OLB_MFD_CELL("reset-eyeq", olb_eyeq5_reset_resources, NULL), > > + OLB_MFD_CELL("eyeq5-pinctrl", olb_eyeq5_pinctrl_resources, NULL), > > +}; > > + > > +static const struct olb_match_data olb_eyeq5_match_data = OLB_DATA(olb_eyeq5_cells); > > + > > +static const struct resource olb_eyeq6l_clk_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x02C, 4 * 8, "pll"), > > +}; > > + > > +static const struct resource olb_eyeq6l_reset_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x004, 2 * 4, "d0"), > > + DEFINE_RES_MEM_NAMED(0x200, 13 * 4, "d1"), > > +}; > > + > > +static const struct mfd_cell olb_eyeq6l_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6l_clk_resources, NULL), > > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6l_reset_resources, NULL), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6l_match_data = OLB_DATA(olb_eyeq6l_cells); > > + > > +static const struct resource olb_eyeq6h_acc_clk_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x040, 7 * 8, "pll"), > > +}; > > + > > +static const struct resource olb_eyeq6h_acc_reset_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x000, 15 * 4, "d0"), > > +}; > > + > > +static const struct mfd_cell olb_eyeq6h_acc_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_acc_clk_resources, "clk-eyeq-acc"), > > The point of enumerating platform device names is to identify devices > that are identical. We lose this with bespoke naming. > > If you want to identify devices either define a value to pass to .id or > adapt the first parameter and make the clk-eyeq driver accept different > device names. About int IDs: those make it unclear what device we are talking about, when we have seven of them as it was the case in this series. Yes, there were 7 system-controllers, each exposing clocks (and most exposing resets). IDs from 0 thru 6 was really not clear enough when going through dmesg, which I why I added the feature. I wasn't able to change the first parameter as that gets used for matching a driver. As it is driver name, there can only be a single string per driver. Here we have multiple MFD cells that each want to probe the same driver, so they must have the same name. > > > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6h_acc_reset_resources, "reset-eyeq-acc"), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6h_acc_match_data = OLB_DATA(olb_eyeq6h_acc_cells); > > + > > +static const struct resource olb_eyeq6h_we_clk_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x074, 1 * 8, "pll"), > > +}; > > + > > +static const struct resource olb_eyeq6h_we_reset_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x004, 4 * 4, "d0"), > > +}; > > + > > +static const struct mfd_cell olb_eyeq6h_west_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_we_clk_resources, "clk-eyeq-west"), > > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6h_we_reset_resources, "reset-eyeq-west"), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6h_west_match_data = OLB_DATA(olb_eyeq6h_west_cells); > > + > > +static const struct mfd_cell olb_eyeq6h_east_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_we_clk_resources, "clk-eyeq-east"), > > + OLB_MFD_CELL("reset-eyeq", olb_eyeq6h_we_reset_resources, "reset-eyeq-east"), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6h_east_match_data = OLB_DATA(olb_eyeq6h_east_cells); > > + > > +static const struct resource olb_eyeq6h_south_clk_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x000, 4 * 8, "pll"), > > + DEFINE_RES_MEM_NAMED(0x070, 1 * 4, "emmc"), > > + DEFINE_RES_MEM_NAMED(0x090, 1 * 4, "ospi"), > > + DEFINE_RES_MEM_NAMED(0x098, 1 * 4, "tsu"), > > +}; > > + > > +static const struct mfd_cell olb_eyeq6h_south_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_south_clk_resources, "clk-eyeq-south"), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6h_south_match_data = OLB_DATA(olb_eyeq6h_south_cells); > > + > > +static const struct resource olb_eyeq6h_ddr_clk_resources[] = { > > + DEFINE_RES_MEM_NAMED(0x074, 1 * 8, "pll"), > > +}; > > + > > +static const struct mfd_cell olb_eyeq6h_ddr0_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_ddr_clk_resources, "clk-eyeq-ddr0"), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6h_ddr0_match_data = OLB_DATA(olb_eyeq6h_ddr0_cells); > > + > > +static const struct mfd_cell olb_eyeq6h_ddr1_cells[] = { > > + OLB_MFD_CELL("clk-eyeq", olb_eyeq6h_ddr_clk_resources, "clk-eyeq-ddr1"), > > +}; > > + > > +static const struct olb_match_data olb_eyeq6h_ddr1_match_data = OLB_DATA(olb_eyeq6h_ddr1_cells); > > + > > +static const struct of_device_id olb_of_match[] = { > > + { .compatible = "mobileye,eyeq5-olb", .data = &olb_eyeq5_match_data }, > > We're not passing MFD init data through the OF API, sorry. > > Pass defined identifiers through instead and match on those please. Ah, that makes sense. Thanks for the mention. I can't find any reasoning, I might be missing context? Thanks Lee, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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,