Hello Philipp,
On Tue Jun 25, 2024 at 11:17 AM CEST, Philipp Zabel wrote:
> On Do, 2024-06-20 at 19:30 +0200, Théo Lebrun wrote:
> > Add Mobileye EyeQ reset controller driver, for EyeQ5, EyeQ6L and EyeQ6H
> > SoCs. Instances belong to a shared register region called OLB and gets
> > spawned as auxiliary device to the platform driver for clock.
> >
> > There is one OLB instance for EyeQ5 and EyeQ6L. There are seven OLB
> > instances on EyeQ6H; three have a reset controller embedded:
> > - West and east get handled by the same compatible.
> > - Acc (accelerator) is another one.
> >
> > Each instance vary in the number and types of reset domains.
> > Instances with single domain expect a single cell, others two.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > drivers/reset/Kconfig | 14 ++
> > drivers/reset/Makefile | 1 +
> > drivers/reset/reset-eyeq.c | 563 +++++++++++++++++++++++++++++++++++++++++++++
>
> Should this be called reset-eyeq-olb or reset-eyeq5, in case a
> different eyeq driver will have to be added in the future?
What about keeping reset-eyeq for the simplicity of it and using
reset-eyeq7 for a theoretical future driver that gets used by EyeQ7 and
above? Or any other revision.
Else it can be reset-eyeq5. OLB might be a concept that gets reused with
different reset blocks inside (meaning reset-eyeq-olb wouldn't
distinguish). You tell me if keeping *-eyeq is fine.
> > 4 files changed, 579 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f386e9da2cd0..36f4001c7f51 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14931,6 +14931,7 @@ F: arch/mips/boot/dts/mobileye/
> > F: arch/mips/configs/eyeq5_defconfig
> > F: arch/mips/mobileye/board-epm5.its.S
> > F: drivers/clk/clk-eyeq5.c
> > +F: drivers/reset/reset-eyeq5.c
>
> See above, and this should match the actual file name.
>
> Adding the MAINTAINERS change in the driver patches makes these patches
> depend on each other. Otherwise they could be applied independently. Do
> you intend this series to be merged together in one tree?
I'd prefer splitting it indeed.
I had thought there were two reasons the patches were interdependent:
1. MAINTAINERS file entries.
2. Kconfig: "depends on COMMON_CLK_EYEQ".
About (1): what about creating a new patch that only touches
MAINTAINERS? It would be taken as part of clk maybe (it contains the
platform driver that instantiates the other auxdevs)?
About (2): Kconfig doesn't complain the symbol doesn't exist so it looks
like a non-issue.
> > F: include/dt-bindings/clock/mobileye,eyeq5-clk.h
> >
> > MODULE SUPPORT
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 85b27c42cf65..b79c18b75674 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -66,6 +66,20 @@ config RESET_BRCMSTB_RESCAL
> > This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
> > BCM7216.
> >
> > +config RESET_EYEQ
> > + bool "Mobileye EyeQ reset controller"
> > + depends on COMMON_CLK_EYEQ
>
> Is this a real dependency? It seems to compile just fine without it.
> Please allow building under COMPILE_TEST without COMMON_CLK_EYEQ set.
Not really. This made potential users notice they want the
clk driver if they want this reset driver. I forgot
handling test builds (ie COMPILE_TEST).
Next revision will look like:
config RESET_EYEQ
bool "Mobileye EyeQ reset controller"
depends on AUXILIARY_BUS
depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
default MACH_EYEQ5 || MACH_EYEQ6H
help: ...
[...]
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/bug.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/container_of.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/lockdep.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
>
> Not needed, this being an aux driver now. Please check the other
> headers as well.
Looking at the diff, <linux/platform_device.h> is the only one.
[...]
> > +static int eqr_probe(struct auxiliary_device *adev,
> > + const struct auxiliary_device_id *id)
> > +{
> > + const struct of_device_id *match;
> > + struct device *dev = &adev->dev;
> > + struct eqr_private *priv;
> > + unsigned int i;
> > + int ret;
> > +
> > + /*
> > + * We are an auxiliary device of clk-eyeq. We do not have an OF node by
> > + * default; let's reuse our parent's OF node.
> > + */
> > + WARN_ON(dev->of_node);
> > + device_set_of_node_from_dev(dev, dev->parent);
> > + if (!dev->of_node)
> > + return -ENODEV;
> > +
> > + /*
> > + * Using our newfound OF node, we can get match data. We cannot use
> > + * device_get_match_data() because it does not match reused OF nodes.
> > + */
> > + match = of_match_node(dev->driver->of_match_table, dev->of_node);
> > + if (!match || !match->data)
> > + return -ENODEV;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->data = match->data;
> > + priv->base = dev_get_platdata(dev);
>
> drivers/reset/reset-eyeq.c:437:20: warning: incorrect type in assignment (different address spaces)
> drivers/reset/reset-eyeq.c:437:20: expected void [noderef] __iomem *base
> drivers/reset/reset-eyeq.c:437:20: got void *
>
> I'd wrap this in a struct or explicitly cast to (void __iomem *) here.
I'll cast to void iomem pointer explicitely.
Thanks for the review Philipp,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
