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

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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

BTW, I just figured it out. There is some incoherency in the
cleanup-on-error path of the dw_spi_mmio_probe() method. If
devm_reset_control_get_optional_exclusive() fails to get reset-control
then pclk will be left enabled. At the same time if init_func() fails
then pm_runtime_disable() will be called with not having
pm_runtime_enable() executed yet. If I don't miss something could you
fix that too?

-Sergey

> 
> 
> Kind regards,
> Niklas