| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <D2518A4HNS5R.2GQYLDGDQ82XV@bootlin.com>
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