Re: [PATCH] spi: dw: assert reset before deasserting reset

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

Hi Niklas, Serge,

On Mi, 2022-03-16 at 14:11 +0000, Niklas Cassel wrote:
[...]
> > > > What about the self-reset based controllers?
> > > 
> > > Not sure what specific feature in the SPI controller you are
> > > referring to.
> > 
> > I am speaking about the reset-controller lines. They can be of two
> > types: manually asserted/de-asserted and self-deasserted. It's
> > platform-specific and mainly depends on the reset-controller
> > implementation.

I assume this is theoretical. Or are there any platforms where spi-dw-
mmio could be used with a self-deasserting reset controller?

> > Seeing you are adding a full-reset cycle anyway, I suggest to add a
> > support for the both types of reset. Like this:
> > 
> > #include <linux/delay.h>
> > ...
> > 
> > ret = reset_control_assert(dwsmmio->rstc);
> > if (ret == -ENOTSUPP) {
> >         ret = reset_control_reset(dwsmmio->rstc);
> > } else if (!ret) {
> >         udelay(2);

Where do the 2 microseconds come from?

> >         ret = reset_control_deassert(dwsmmio->rstc);
> > }
> > if (ret)
> >         goto out;
> > 
> > * Please don't forget to add the include line.
> 
> Wow, that is ugly, and I only see one other driver doing it this way.

Which one?

> (All drivers in drivers/spi simply do assert() + udelay() +
> deassert()).

assert() will return -ENOTSUPP if the reset controller is self-
deasserting. That's why they should implement proper error handling if
the driver may be used on a platform with a self-deasserting reset
controller in the future.

> If this is the way to handle both reset control types, there should
> probably be a common helper function in drivers/reset/core.c.
> 
> Right now, I'd rather drop this patch than being guilty of copy
> pasting this pattern futher. (Considering that this patch does not
> solve any known issue.)

If it doesn't solve any issue, I'd say drop it.

> Philipp Zabel, reset controller maintainer is already on CC, would be
> nice to hear his point of view.

I would be open to reset_control_assert_delay_deassert_or_reset() kind
of helpers (not with this name) if there are multiple users. But I'd
prefer such a thing to be introduced for drivers that are actually used
both with self-deasserting reset controllers and with reset controllers
with manually controllable reset lines, with an explanation why this is
better than just using reset_control_reset() and implementing .reset()
in all relevant reset controller drivers.

regards
Philipp