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

[Jon Hunter <jonathanh@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxx>
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