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

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

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.

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.

> 
> What about the self-reset based controllers?

Not sure what specific feature in the SPI controller you are
referring to.


Kind regards,
Niklas