by Stephen Boyd

[permalink] [raw]

Subject: Re: [PATCH v2 00/11] Add Mobileye EyeQ system controller support (clk, reset, pinctrl)

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/

2024-05-31 11:06:22

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] mfd: olb: Add support for Mobileye OLB system-controller

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 <[email protected]>
> ---
> 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
>

--
Lee Jones [李琼斯]