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

[Niklas Cassel <Niklas.Cassel@xxxxxxx>]

On Thu, Mar 17, 2022 at 11:17:34AM +0100, Philipp Zabel wrote:
> 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?

None that I am aware of.

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

This was just some arbitrary number, to ensure that the reset was held long
enough for the device to get reset.

Two stm and two nvidia SPI controller drivers had a udelay(2) between
assert() and deassert(), so it seemed like a reasonable time to hold reset.
(Even if the stm and nvidia SPI controllers are completely different.)

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

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c?h=v5.17#n7035

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

That is the plan for now.

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

I see your point. Many drivers should probably change assert() +
udelay(x) + deassert() with a reset_control_reset(), since .reset()
implementation should have the correct delay for each SoC.
..but I guess often the manual for some hardware block states the how long
reset has to be held. So for that to work the delay in .reset() has to be
greater equal the longest reset time needed for all hardware in that SoC?


Kind regards,
Niklas