Date: Thu, 20 Jun 2024 19:48:56 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Lee Jones" <lee@...nel.org>
Cc: "Rob Herring" <robh@...nel.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@...aro.org>, "Conor Dooley" <conor+dt@...nel.org>,
 "Michael Turquette" <mturquette@...libre.com>, "Stephen Boyd"
 <sboyd@...nel.org>, "Philipp Zabel" <p.zabel@...gutronix.de>, "Linus
 Walleij" <linus.walleij@...aro.org>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 "Thomas Bogendoerfer" <tsbogend@...ha.franken.de>,
 <linux-mips@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-clk@...r.kernel.org>,
 <linux-gpio@...r.kernel.org>, "Vladimir Kondratiev"
 <vladimir.kondratiev@...ileye.com>, "Gregory CLEMENT"
 <gregory.clement@...tlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH v2 06/11] mfd: olb: Add support for Mobileye OLB
 system-controller

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@...tlin.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

Powered by blists - more mailing lists