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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Fri, Mar 11, 2022 at 08:05:58PM +0300, Serge Semin wrote:
> On Fri, Mar 11, 2022 at 03:51:23PM +0000, Niklas Cassel wrote:
> > On Fri, Mar 11, 2022 at 05:25:50PM +0300, Serge Semin wrote:
> > > Hello Niklas
> > 
> > Hello Serge,
> > 
> > > 
> > > On Tue, Mar 01, 2022 at 11:17:20AM +0000, Niklas Cassel wrote:
> > > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > > 
> > > > Simply deasserting reset just ensures that the hardware is taken out of
> > > > reset, if it was booted with the hardware reset asserted.
> > > > 
> > > > In order actually reset the SPI controller, we need to assert reset before
> > > > deasserting.
> > > > 
> > > > By doing this, we ensure that the hardware is not in some bad state, and we
> > > > ensure that the hardware registers get set to their reset default, clearing
> > > > any setting set by e.g. a bootloader.
> > > > 
> > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > > ---
> > > >  drivers/spi/spi-dw-mmio.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> > > > index 5101c4c6017b..eb1dacb45ca2 100644
> > > > --- a/drivers/spi/spi-dw-mmio.c
> > > > +++ b/drivers/spi/spi-dw-mmio.c
> > > > @@ -289,6 +289,8 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
> > > >  		ret = PTR_ERR(dwsmmio->rstc);
> > > >  		goto out_clk;
> > > >  	}
> > > 
> > > > +	reset_control_assert(dwsmmio->rstc);
> > > > +	udelay(2);
> > > 
> > > Do we really need this? dw_spi_add_host() is doing a sort of soft reset
> > > anyway by calling the dw_spi_hw_init() method. Do you have a real
> > > platform, which requires to do a full hard-reset?
> > 
> > Does this solve a real problem that I've seen with the SPI controller?
> > No.
> > 
> 
> > Which register write in dw_spi_hw_init() is doing a soft reset?
> > I assume that you mean one of the writes in dw_spi_reset_chip(),
> > probably DW_SPI_SSIENR.
> > I don't think us toggling this register will reset all registers
> > to their reset default values.
> 
> Well, you are right it isn't a true soft reset, that's why I added
> "sort of".) Anyway after calling that method the main DW SSI registers
> are supposed to be in a known state. Of course it doesn't reset the
> controller RTL logic, and some of the CSRs will still be left randomly
> initialized in case of bootloader doings.
> 
> > 
> > I think it is a good to start off with all registers in their
> > default reset values.
> > 
> > Arguably, I think it looks wrong to see a reset_control_deassert()
> > without any previous reset_control_assert().
> > 
> > Do a simple:
> > git grep -C 10 reset_control_deassert drivers/spi/
> > 
> > And you see that most SPI drivers (and most other device drivers for
> > that matter), usually assert reset before deasserting it, in order
> > to ensure that a reset pulse is actually sent to the hardware.
> > 
> > Simply deasserting reset, will have the hardware in a "fresh" state
> > if it was a cold boot (where reset is usually asserted until deasserted),
> > but will not have the hardware in a "fresh" state if booted via a boot
> > loader. This is an inconsistency, and could potentially lead to issues
> > that are only noticed if booting via a bootloader.
> > 
> 
> No objection then seeing Mark is also inclined to have a full
> hard-reset cycle here too.
> 
> > > 
> > > 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.
> 
> 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);
> 	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.
(All drivers in drivers/spi simply do assert() + udelay() + deassert()).

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

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


Kind regards,
Niklas