| Message ID | 20210608071518.93037-1-jonathanh@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | spi: tegra20-slink: Ensure SPI controller reset is deasserted | expand |
On Tue, Jun 08, 2021 at 08:15:18AM +0100, Jon Hunter wrote: > Commit 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling > clocks") removed some legacy code for handling resets on Tegra from > within the Tegra clock code. This exposed an issue in the Tegra20 slink > driver where the SPI controller reset was not being deasserted as needed > during probe. This is causing the Tegra30 Cardhu platform to hang on > boot. Fix this by ensuring the SPI controller reset is deasserted during > probe. > > Fixes: 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling clocks") While it technically fixes an issue uncovered by that patch, I would argue that the underlying issue has been present forever. So I think this should be applied regardless of the above patch. It also makes me wonder if we shouldn't drop the clock patch for now to unbreak things and avoid having to model complicated dependencies to make sure everything continues to work in v5.14-rc1. Unless perhaps if Mark applies this for v5.13, then we can merge the clock patch for v5.14-rc1 since SPI is the only IP that seems to be broken by that change. > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/spi/spi-tegra20-slink.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c > index f7c832fd4003..6a726c95ac7a 100644 > --- a/drivers/spi/spi-tegra20-slink.c > +++ b/drivers/spi/spi-tegra20-slink.c > @@ -1118,6 +1118,11 @@ static int tegra_slink_probe(struct platform_device *pdev) > pm_runtime_put_noidle(&pdev->dev); > goto exit_pm_disable; > } > + > + reset_control_assert(tspi->rst); > + udelay(2); > + reset_control_deassert(tspi->rst); > + I wonder if this doesn't break now again on suspend/resume. Should we perhaps move this into tegra_slink_runtime_resume()? Or better yet, move the reset_control_assert() into tegra_slink_runtime_suspend() and the reset_control_deassert() into tegra_slink_runtime_resume(). That should ensure the device's reset is always deasserted when runtime resumed. Thierry
On 08/06/2021 13:10, Thierry Reding wrote: > On Tue, Jun 08, 2021 at 08:15:18AM +0100, Jon Hunter wrote: >> Commit 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling >> clocks") removed some legacy code for handling resets on Tegra from >> within the Tegra clock code. This exposed an issue in the Tegra20 slink >> driver where the SPI controller reset was not being deasserted as needed >> during probe. This is causing the Tegra30 Cardhu platform to hang on >> boot. Fix this by ensuring the SPI controller reset is deasserted during >> probe. >> >> Fixes: 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling clocks") > > While it technically fixes an issue uncovered by that patch, I would > argue that the underlying issue has been present forever. So I think > this should be applied regardless of the above patch. Yes that is true and there is no dependency per-se but wanted to highlight that up until that patch there were no issues. > It also makes me wonder if we shouldn't drop the clock patch for now to > unbreak things and avoid having to model complicated dependencies to > make sure everything continues to work in v5.14-rc1. Yes but I guess we will need to revert that one now as it is part of the pull request you sent. However, that is fine with me. > Unless perhaps if Mark applies this for v5.13, then we can merge the > clock patch for v5.14-rc1 since SPI is the only IP that seems to be > broken by that change. Yes that works too. >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/spi/spi-tegra20-slink.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c >> index f7c832fd4003..6a726c95ac7a 100644 >> --- a/drivers/spi/spi-tegra20-slink.c >> +++ b/drivers/spi/spi-tegra20-slink.c >> @@ -1118,6 +1118,11 @@ static int tegra_slink_probe(struct platform_device *pdev) >> pm_runtime_put_noidle(&pdev->dev); >> goto exit_pm_disable; >> } >> + >> + reset_control_assert(tspi->rst); >> + udelay(2); >> + reset_control_deassert(tspi->rst); >> + > > I wonder if this doesn't break now again on suspend/resume. Should we > perhaps move this into tegra_slink_runtime_resume()? Or better yet, move > the reset_control_assert() into tegra_slink_runtime_suspend() and the > reset_control_deassert() into tegra_slink_runtime_resume(). That should > ensure the device's reset is always deasserted when runtime resumed. So we do test suspend/resume on Cardhu and I have seen no issues with this applied. At first I did put this in the runtime_suspend/resume handlers, but then looking at what is done in spi-tegra114.c it appears we just do this on probe. See ... commit 019194933339b3e9b486639c8cb3692020844d65 Author: Sowjanya Komatineni <skomatineni@nvidia.com> Date: Tue Mar 26 22:56:32 2019 -0700 spi: tegra114: reset controller on probe I guess moving it to the runtime_suspend/resume handlers would be more consistent with the previous code. What do you think? Jon
On Tue, Jun 08, 2021 at 01:35:11PM +0100, Jon Hunter wrote: > > On 08/06/2021 13:10, Thierry Reding wrote: > > On Tue, Jun 08, 2021 at 08:15:18AM +0100, Jon Hunter wrote: > >> Commit 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling > >> clocks") removed some legacy code for handling resets on Tegra from > >> within the Tegra clock code. This exposed an issue in the Tegra20 slink > >> driver where the SPI controller reset was not being deasserted as needed > >> during probe. This is causing the Tegra30 Cardhu platform to hang on > >> boot. Fix this by ensuring the SPI controller reset is deasserted during > >> probe. > >> > >> Fixes: 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling clocks") > > > > While it technically fixes an issue uncovered by that patch, I would > > argue that the underlying issue has been present forever. So I think > > this should be applied regardless of the above patch. > > Yes that is true and there is no dependency per-se but wanted to > highlight that up until that patch there were no issues. > > > It also makes me wonder if we shouldn't drop the clock patch for now to > > unbreak things and avoid having to model complicated dependencies to > > make sure everything continues to work in v5.14-rc1. > > Yes but I guess we will need to revert that one now as it is part of the > pull request you sent. However, that is fine with me. > > > Unless perhaps if Mark applies this for v5.13, then we can merge the > > clock patch for v5.14-rc1 since SPI is the only IP that seems to be > > broken by that change. > > Yes that works too. > > >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > >> --- > >> drivers/spi/spi-tegra20-slink.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c > >> index f7c832fd4003..6a726c95ac7a 100644 > >> --- a/drivers/spi/spi-tegra20-slink.c > >> +++ b/drivers/spi/spi-tegra20-slink.c > >> @@ -1118,6 +1118,11 @@ static int tegra_slink_probe(struct platform_device *pdev) > >> pm_runtime_put_noidle(&pdev->dev); > >> goto exit_pm_disable; > >> } > >> + > >> + reset_control_assert(tspi->rst); > >> + udelay(2); > >> + reset_control_deassert(tspi->rst); > >> + > > > > I wonder if this doesn't break now again on suspend/resume. Should we > > perhaps move this into tegra_slink_runtime_resume()? Or better yet, move > > the reset_control_assert() into tegra_slink_runtime_suspend() and the > > reset_control_deassert() into tegra_slink_runtime_resume(). That should > > ensure the device's reset is always deasserted when runtime resumed. > > So we do test suspend/resume on Cardhu and I have seen no issues with > this applied. At first I did put this in the runtime_suspend/resume > handlers, but then looking at what is done in spi-tegra114.c it appears > we just do this on probe. See ... > > commit 019194933339b3e9b486639c8cb3692020844d65 > Author: Sowjanya Komatineni <skomatineni@nvidia.com> > Date: Tue Mar 26 22:56:32 2019 -0700 > > spi: tegra114: reset controller on probe > > I guess moving it to the runtime_suspend/resume handlers would be more > consistent with the previous code. What do you think? Do we test that SPI is still functional after suspend/resume? If it is, I have no objection to this patch. I think making this part of runtime suspend/resume would be a bit more correct or robust, but it's also a bit more complicated and might introduce other problems. For example, I suspect that if we reset on runtime suspend/resume, we would likely need to reprogram the SLINK_COMMAND* registers as well. Thierry
08.06.2021 16:16, Thierry Reding пишет: >>> Unless perhaps if Mark applies this for v5.13, then we can merge the >>> clock patch for v5.14-rc1 since SPI is the only IP that seems to be >>> broken by that change. >> Yes that works too. Will be great if this SPI fix could be merged into 5.13. It took two kernel releases to fix the audio resets, so I prefer not to postpone the clk patch again. >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> --- >>>> drivers/spi/spi-tegra20-slink.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c >>>> index f7c832fd4003..6a726c95ac7a 100644 >>>> --- a/drivers/spi/spi-tegra20-slink.c >>>> +++ b/drivers/spi/spi-tegra20-slink.c >>>> @@ -1118,6 +1118,11 @@ static int tegra_slink_probe(struct platform_device *pdev) >>>> pm_runtime_put_noidle(&pdev->dev); >>>> goto exit_pm_disable; >>>> } >>>> + >>>> + reset_control_assert(tspi->rst); >>>> + udelay(2); >>>> + reset_control_deassert(tspi->rst); >>>> + >>> I wonder if this doesn't break now again on suspend/resume. Should we >>> perhaps move this into tegra_slink_runtime_resume()? Or better yet, move >>> the reset_control_assert() into tegra_slink_runtime_suspend() and the >>> reset_control_deassert() into tegra_slink_runtime_resume(). That should >>> ensure the device's reset is always deasserted when runtime resumed. >> So we do test suspend/resume on Cardhu and I have seen no issues with >> this applied. At first I did put this in the runtime_suspend/resume >> handlers, but then looking at what is done in spi-tegra114.c it appears >> we just do this on probe. See ... >> >> commit 019194933339b3e9b486639c8cb3692020844d65 >> Author: Sowjanya Komatineni <skomatineni@nvidia.com> >> Date: Tue Mar 26 22:56:32 2019 -0700 >> >> spi: tegra114: reset controller on probe >> >> I guess moving it to the runtime_suspend/resume handlers would be more >> consistent with the previous code. What do you think? > Do we test that SPI is still functional after suspend/resume? If it is, > I have no objection to this patch. I think making this part of runtime > suspend/resume would be a bit more correct or robust, but it's also a > bit more complicated and might introduce other problems. For example, > I suspect that if we reset on runtime suspend/resume, we would likely > need to reprogram the SLINK_COMMAND* registers as well. We don't support LP0 suspend state for older Tegra SoCs where hardware state is lost after suspending. Lot's of device drivers don't do suspend/resume properly. Fixing suspend/resume should be a separate patch, IMO.
On Tue, 8 Jun 2021 08:15:18 +0100, Jon Hunter wrote: > Commit 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling > clocks") removed some legacy code for handling resets on Tegra from > within the Tegra clock code. This exposed an issue in the Tegra20 slink > driver where the SPI controller reset was not being deasserted as needed > during probe. This is causing the Tegra30 Cardhu platform to hang on > boot. Fix this by ensuring the SPI controller reset is deasserted during > probe. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: tegra20-slink: Ensure SPI controller reset is deasserted commit: aceda401e84115bf9121454828f9da63c2a94482 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c index f7c832fd4003..6a726c95ac7a 100644 --- a/drivers/spi/spi-tegra20-slink.c +++ b/drivers/spi/spi-tegra20-slink.c @@ -1118,6 +1118,11 @@ static int tegra_slink_probe(struct platform_device *pdev) pm_runtime_put_noidle(&pdev->dev); goto exit_pm_disable; } + + reset_control_assert(tspi->rst); + udelay(2); + reset_control_deassert(tspi->rst); + tspi->def_command_reg = SLINK_M_S; tspi->def_command2_reg = SLINK_CS_ACTIVE_BETWEEN; tegra_slink_writel(tspi, tspi->def_command_reg, SLINK_COMMAND);
Commit 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling clocks") removed some legacy code for handling resets on Tegra from within the Tegra clock code. This exposed an issue in the Tegra20 slink driver where the SPI controller reset was not being deasserted as needed during probe. This is causing the Tegra30 Cardhu platform to hang on boot. Fix this by ensuring the SPI controller reset is deasserted during probe. Fixes: 4782c0a5dd88 ("clk: tegra: Don't deassert reset on enabling clocks") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/spi/spi-tegra20-slink.c | 5 +++++ 1 file changed, 5 insertions(+)