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

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

Hello Philipp,

On Tue Jul 2, 2024 at 11:19 AM CEST, Philipp Zabel wrote:
> On Mo, 2024-07-01 at 18:19 +0200, Théo Lebrun wrote:
[...]
> > > > +#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 thought about it but checking drivers/pinctrl/ it looked like macros
were more common for container_of() encapsulation. I'll go the static
inline function. Plain, simple:

static inline struct eqr_private *eqr_rcdev_to_priv(struct reset_controller_dev *x)
{
	return container_of(x, struct eqr_private, rcdev);
}

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

Sure:

struct eqr_private {
	/*
	 * One mutex per domain for read-modify-write operations on registers.
	 * Some domains can be involved in LBIST which implies long critical
	 * sections; we wouldn't want other domains to be impacted by that.
	 */
	struct mutex			mutexes[EQR_MAX_DOMAIN_COUNT];
	void __iomem			*base;
	const struct eqr_match_data	*data;
	struct reset_controller_dev	rcdev;
};

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

Done, I added a plain comment on both assert and deassert:

	/* RST_REQUEST and CLK_REQUEST must be kept in sync. */

Thanks Philipp,

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