| Message ID | 20220301111715.3062886-1-Niklas.Cassel@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: dw: assert reset before deasserting reset | expand |
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
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
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.
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
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
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
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
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
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;