Re: [PATCH v3 7/9] reset: eyeq: add platform driver

["Théo Lebrun" <theo.lebrun@bootlin.com>]

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@bootlin.com>
> > ---
> >  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