Re: [PATCH] spi: tegra20-slink: Ensure SPI controller reset is deasserted

[Thierry Reding <thierry.reding@gmail.com>]

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

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

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