Date: Mon, 01 Jul 2024 18:19:12 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Philipp Zabel" <p.zabel@...gutronix.de>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>
Cc: <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Vladimir
 Kondratiev" <vladimir.kondratiev@...ileye.com>,
 Grégory Clement <gregory.clement@...tlin.com>, "Thomas
 Petazzoni" <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH 2/2] reset: eyeq: add platform driver

Hello Philipp,

On Mon Jul 1, 2024 at 10:59 AM CEST, Philipp Zabel wrote:
> On Fr, 2024-06-28 at 18:11 +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@...tlin.com>
> > ---
> >  drivers/reset/Kconfig      |  13 ++
> >  drivers/reset/Makefile     |   1 +
> >  drivers/reset/reset-eyeq.c | 562 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 576 insertions(+)
> > 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 7112f5932609..14f3f4af0b10 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -66,6 +66,19 @@ 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 AUXILIARY_BUS
>
> This should
>
> 	select AUXILIARY_BUS
>
> instead.

Done, looking like this now:

config RESET_EYEQ
	bool "Mobileye EyeQ reset controller"
	depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
	select AUXILIARY_BUS
	default MACH_EYEQ5 || MACH_EYEQ6H
	help
	  ...

[...]

> > +#define EQR_MAX_DOMAIN_COUNT 3
> > +
> > +struct eqr_domain_descriptor {
> > +	enum eqr_domain_type	type;
> > +	u32			valid_mask;
> > +	unsigned int		offset;
> > +};
> > +
> > +struct eqr_match_data {
> > +	unsigned int				domain_count;
> > +	const struct eqr_domain_descriptor	*domains;
> > +};
> > +
> > +struct eqr_private {
> > +	struct mutex			mutexes[EQR_MAX_DOMAIN_COUNT];
>
> Is there any benefit from per-domain mutexes over just a single mutex?

Some domains can stay locked for pretty long in theory because of Logic
built-in self-test (LBIST). This is the reasoning behind
eqr_busy_wait_locked().

Other domains are not concerned by this behavior.

More concretely, on EyeQ5, SARCR and ACRP domains can lock their mutexes
for a relatively long time, and for good reasons. We wouldn't want the
PCIE domain  to suffer from that if it happens to (de)assert a reset at
the same time.

>
> > +	void __iomem			*base;
> > +	const struct eqr_match_data	*data;
> > +	struct reset_controller_dev	rcdev;
> > +};
> > +
> > +#define rcdev_to_priv(rcdev) container_of(rcdev, struct eqr_private, rcdev)
>
> Please use checkpatch --strict, and ideally mention when you ignore a
> warning on purpose. In this case, the macro parameter should named
> something else, because the last parameter to container_of must be
> "rcdev" verbatim. This only works by accident because the passed
> parameter also happens to be called called "rcdev" at all call sites.

I have let this CHECK from `checkpatch --strict` slip through indeed.
Other remaining messages, with explanations, are:

 - WARNING: added, moved or deleted file(s), does MAINTAINERS need
   updating?

   This is done in a single patch [0] in the MIPS series to avoid
   conflicts in between series.

 - CHECK: struct mutex definition without comment

   This is about the above mutexes field. Do you want a code comment
   about the reasoning for one mutex per domain?

 - WARNING: DT compatible string "[...]" appears un-documented

   Bindings are added in a single commit in the MIPS series [1],
   to avoid conflicts.

>
> > +static u32 eqr_double_readl(void __iomem *addr_a, void __iomem *addr_b,
> > +			    u32 *dest_a, u32 *dest_b)
> > +{
> > +	*dest_a = readl(addr_a);
> > +	*dest_b = readl(addr_b);
> > +	return 0; /* read_poll_timeout() op argument must return something. */
> > +}
> > +
> > +static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev,
> > +				u32 domain, u32 offset, bool assert)
> > +{
> > +	enum eqr_domain_type domain_type = priv->data->domains[domain].type;
> > +	unsigned long sleep_us, timeout_us;
> > +	u32 val, mask, val0, val1;
> > +	void __iomem *base, *reg;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&priv->mutexes[domain]);
> > +
> > +	base = priv->base + priv->data->domains[domain].offset;
> > +	sleep_us = eqr_timings[domain_type].sleep_us;
> > +	timeout_us = eqr_timings[domain_type].timeout_us;
>
> You can initialize these at the declaration.

Done, declarations will look like:

static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev,
				u32 domain, u32 offset, bool assert)
{
	void __iomem *base = priv->base + priv->data->domains[domain].offset;
	enum eqr_domain_type domain_type = priv->data->domains[domain].type;
	unsigned long timeout_us = eqr_timings[domain_type].timeout_us;
	unsigned long sleep_us = eqr_timings[domain_type].sleep_us;
	u32 val, mask, val0, val1;
	void __iomem *reg;
	int ret;

	// ...
}

>
> > +
> > +	switch (domain_type) {
> > +	case EQR_EYEQ5_SARCR:
> > +		reg = base + EQR_EYEQ5_SARCR_STATUS;
> > +		mask = BIT(offset);
> > +
> > +		ret = readl_poll_timeout(reg, val, !(val & mask) == assert,
> > +					 sleep_us, timeout_us);
> > +		break;
> > +
> > +	case EQR_EYEQ5_ACRP:
> > +		reg = base + 4 * offset;
> > +		if (assert)
> > +			mask = EQR_EYEQ5_ACRP_ST_POWER_DOWN;
> > +		else
> > +			mask = EQR_EYEQ5_ACRP_ST_ACTIVE;
> > +
> > +		ret = readl_poll_timeout(reg, val, !!(val & mask),
> > +					 sleep_us, timeout_us);
> > +		break;
> > +
> > +	case EQR_EYEQ5_PCIE:
> > +		ret = 0; /* No busy waiting. */
> > +		break;
> > +
> > +	case EQR_EYEQ6H_SARCR:
> > +		/*
> > +		 * Wait until both bits change:
> > +		 *	readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset)
> > +		 *	readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset)
> > +		 */
> > +		mask = BIT(offset);
> > +		ret = read_poll_timeout(eqr_double_readl, val,
> > +					(!(val0 & mask) == assert) &&
> > +						(!(val1 & mask) == assert),
>
> I'd remove a level of indentation here.

Indeed!

> > +					sleep_us, timeout_us, false,
> > +					base + EQR_EYEQ6H_SARCR_RST_STATUS,
> > +					base + EQR_EYEQ6H_SARCR_CLK_STATUS,
> > +					&val0, &val1);
>
> Calling these variables something like rst_status and clk_status, would
> make this a bit easier to parse.

It now looks like:

static int eqr_busy_wait_locked(struct eqr_private *priv, struct device *dev,
				u32 domain, u32 offset, bool assert)
{
	// ...

	switch (domain_type) {

	// ...

	case EQR_EYEQ6H_SARCR:
		/*
		 * Wait until both bits change:
		 *	readl(base + EQR_EYEQ6H_SARCR_RST_STATUS) & BIT(offset)
		 *	readl(base + EQR_EYEQ6H_SARCR_CLK_STATUS) & BIT(offset)
		 */
		mask = BIT(offset);
		ret = read_poll_timeout(eqr_double_readl, val,
					(!(rst_status & mask) == assert) &&
					(!(clk_status & mask) == assert),
					sleep_us, timeout_us, false,
					base + EQR_EYEQ6H_SARCR_RST_STATUS,
					base + EQR_EYEQ6H_SARCR_CLK_STATUS,
					&rst_status, &clk_status);
		break;

	// ...

	}

	// ...
}

>
> > +		break;
> > +
> > +	default:
> > +		WARN_ON(1);
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	if (ret == -ETIMEDOUT)
> > +		dev_dbg(dev, "%u-%u: timeout\n", domain, offset);
> > +	return ret;
> > +}
> > +
> > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset)
> > +{
> > +	enum eqr_domain_type domain_type = priv->data->domains[domain].type;
> > +	void __iomem *base, *reg;
> > +	u32 val;
> > +
> > +	lockdep_assert_held(&priv->mutexes[domain]);
> > +
> > +	base = priv->base + priv->data->domains[domain].offset;
> > +
> > +	switch (domain_type) {
> > +	case EQR_EYEQ5_SARCR:
> > +		reg = base + EQR_EYEQ5_SARCR_REQUEST;
> > +		writel(readl(reg) & ~BIT(offset), reg);
> > +		break;
> > +
> > +	case EQR_EYEQ5_ACRP:
> > +		reg = base + 4 * offset;
> > +		writel(readl(reg) | EQR_EYEQ5_ACRP_PD_REQ, reg);
> > +		break;
> > +
> > +	case EQR_EYEQ5_PCIE:
> > +		writel(readl(base) & ~BIT(offset), base);
> > +		break;
> > +
> > +	case EQR_EYEQ6H_SARCR:
> > +		val = readl(base + EQR_EYEQ6H_SARCR_RST_REQUEST);
> > +		val &= ~BIT(offset);
> > +		writel(val, base + EQR_EYEQ6H_SARCR_RST_REQUEST);
> > +		writel(val, base + EQR_EYEQ6H_SARCR_CLK_REQUEST);
>
> This looks peculiar. Why is it ok to write the value read from
> RST_REQUEST into CLK_REQUEST?

What is abstracted away by the hardware on EyeQ5 is not anymore on
EyeQ6H. Previously a single register was used for requests and a single
register for status. Now there are two request registers and two status
registers.

Those registers *must be kept in sync*. The register name referencing
clock is not to be confused with the clock driver of the
system-controller. It is describing a register within the reset
controller.

This hardware interface is odd, I might add a comment?

[...]

> > +static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	u32 domain = FIELD_GET(ID_DOMAIN_MASK, id);
> > +	struct eqr_private *priv = rcdev_to_priv(rcdev);
> > +	enum eqr_domain_type domain_type = priv->data->domains[domain].type;
> > +	u32 offset = FIELD_GET(ID_OFFSET_MASK, id);
>
> I'd put domain and offset declaration next to each other.

Done:

static int eqr_status(struct reset_controller_dev *rcdev, unsigned long id)
{
	u32 domain = FIELD_GET(ID_DOMAIN_MASK, id);
	u32 offset = FIELD_GET(ID_OFFSET_MASK, id);
	struct eqr_private *priv = rcdev_to_priv(rcdev);
	enum eqr_domain_type domain_type = priv->data->domains[domain].type;
	void __iomem *base, *reg;

	// ...
}

I'll be sending a new revision in the week with those fixes.

Thanks Philipp,

[0]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-3-f53f5e4c422b@bootlin.com/
[1]: https://lore.kernel.org/lkml/20240628-mbly-mips-v1-1-f53f5e4c422b@bootlin.com/

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists