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

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

[Jon Hunter]

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

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);
-- 
2.25.1

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

[Thierry Reding]

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

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

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

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

[Jon Hunter]

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

-- 
nvpublic

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

[Thierry Reding]

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

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

[Dmitry Osipenko]

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.

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

[Mark Brown]

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