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

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

[Niklas Cassel]

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);
 	reset_control_deassert(dwsmmio->rstc);
 
 	dws->bus_num = pdev->id;
-- 
2.35.1

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

[Serge Semin]

Hello Niklas

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?

What about the self-reset based controllers?

-Sergey

>  	reset_control_deassert(dwsmmio->rstc);
>  
>  	dws->bus_num = pdev->id;
> -- 
> 2.35.1

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

[Niklas Cassel]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Fri, Mar 11, 2022 at 03:51:23PM +0000, Niklas Cassel wrote:

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

I do tend to agree that if we've got the ability to do a reset it's good
practice to use it to get the hardware into a known state, it just
guards against the possibility that there's some system or configuration
where things are left in a surprising state which ends up causing
problems.  Unless the reset is going to take a long time there should be
no real downside even if the upside is marginal.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Serge Semin]

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.

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

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

[Niklas Cassel]

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

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

[Philipp Zabel]

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

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

[Niklas Cassel]

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

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

[Philipp Zabel]

Hi Niklas,

On Di, 2022-03-22 at 10:32 +0000, Niklas Cassel wrote:
[...]
>  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
> 

Thank you.

[...]
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?

Yes, exactly. The reset-simple driver was prepared for this in commit
a9701376ed0f ("reset: simple: Add reset callback"), for example.
Several of the custom reset drivers also implement .reset() with a
fixed delay.

regards
Philipp