Re: [PATCH v2 1/2] i2c: tegra: Better handle case where CPU0 is busy for a long time

From: Jon Hunter <jonathanh@nvidia.com>
To: Dmitry Osipenko <digetx@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Laxman Dewangan <ldewangan@nvidia.com>,
	"Wolfram Sang" <wsa@the-dreams.de>,
	Manikanta Maddireddy <mmaddireddy@nvidia.com>,
	Vidya Sagar <vidyas@nvidia.com>
Cc: <linux-i2c@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] i2c: tegra: Better handle case where CPU0 is busy for a long time
Date: Thu, 23 Apr 2020 11:56:13 +0100	[thread overview]
Message-ID: <a5198024-7273-74c4-b4f4-3a29d042bc36@nvidia.com> (raw)
In-Reply-To: <d2531fc1-b452-717d-af71-19497e14ef00@gmail.com>

On 22/04/2020 15:07, Dmitry Osipenko wrote:

...

>> So I think that part of the problem already existed prior to these
>> patches. Without your patches I see ...
>>
>> [   59.543528] tegra-i2c 7000d000.i2c: i2c transfer timed out
> 
> Does this I2C timeout happen with my patches? Could you please post full
> logs of an older and the recent kernel versions?

I believe that it does, but I need to check.

>> [   59.549036] vdd_sata,avdd_plle: failed to disable
>>
>> [   59.553778] Failed to disable avdd-plle: -110
>>
>> [   59.558150] tegra-pcie 3000.pcie: failed to disable regulators: -110
>>
>>
>> However, now with your patches the i2c is failing to resume and this
>> breaks system suspend completely for Tegra30. So far this is the only
>> board that appears to break.
>>
>> So the above issue needs to be fixed and I will chat to Thierry about this.
> 
> Okay

So I have been looking at this some more and this starting to all look a
bit of a mess :-(

Firstly, the Tegra PCI driver suspends late (noirq) as we know. The PCI
driver will warn if it cannot disable the regulators when suspending but
does not actually fail suspend. So this warning is just indicating that
we were unable to disable the regulators.

Now I don't see that we can ever disable the PCI regulators currently
when entering suspend because ...

1. We are in the noirq phase and so we will not get the completion
   interrupt for the I2C transfer. I know that you recently added the
   atomic I2C transfer support, but we can get the regulator framework
   to use this (I have not looked in much detail so far).

2. Even if the regulator framework supported atomic I2C transfers, we
   also have the problem that the I2C controller could be runtime-
   suspended and pm_runtime_get_sync() will not work during during this
   phase to resume it correctly. This is a problem that needs to be
   fixed indeed!

3. Then we also have the possible dependency on the DMA driver that is
   suspended during the noirq phase.

It could be argued that if the PCI regulators are never turned off
(currently) then we should not even bother and this will likely resolve
this for now. However, really we should try to fix that correctly.

What I still don't understand is why your patch breaks resume. Even if
the I2C transfer fails, and is deemed harmless by the client driver, we
should still be able to suspend and resume correctly.

Cheers
Jon

-- 
nvpublic