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

[Dmitry Osipenko <digetx@xxxxxxxxx>]

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@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?
> 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.