Re: [PATCH 2/2] reset: eyeq: add platform driver

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Théo,

On Mo, 2024-07-01 at 18:19 +0200, Théo Lebrun wrote:
[...]
> > 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.

Thank you for the explanation.

> > 
> > > +	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.

Thinking about this again, it would be even better to turn this into a
static inline function instead.

> 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?

Yes, that would be nice. I'm not pedantic about the lock comments
because in reset drivers it's usually pretty obvious what the lock is
used for, but mentioning that the mutexes cover register read-modify-
write plus waiting for LBIST on some domains seems like a good idea.

[...]
> > 
> > > +static void eqr_assert_locked(struct eqr_private *priv, u32 domain, u32 offset)
> > > +{
[...]
> > > +	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?

Yes, please. With the knowledge that those registers must be kept in
sync, this goes from strange to obvious.


regards
Philipp