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
