From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter 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 Message-ID: References: <20200324191217.1829-1-digetx@gmail.com> <20200324191217.1829-2-digetx@gmail.com> <1e259e22-c300-663a-e537-18d854e0f478@nvidia.com> <8cd085e1-f9fd-6ec0-9f7a-d5463f176a63@nvidia.com> <6f07e5c8-7916-7ea2-2fe7-d05f8f011471@nvidia.com> <77a31b2f-f525-ba9e-f1ae-2b474465bde4@gmail.com> <470b4de4-e98a-1bdc-049e-6259ad603507@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-i2c-owner@vger.kernel.org To: Dmitry Osipenko , Thierry Reding , Laxman Dewangan , Wolfram Sang , Manikanta Maddireddy , Vidya Sagar Cc: linux-i2c@vger.kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-tegra@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AEE29C2BA19 for ; Thu, 23 Apr 2020 10:56:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8DA2D2074F for ; Thu, 23 Apr 2020 10:56:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=nvidia.com header.i=@nvidia.com header.b="Ta7B/bt7" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727924AbgDWK4U (ORCPT ); Thu, 23 Apr 2020 06:56:20 -0400 Received: from hqnvemgate24.nvidia.com ([216.228.121.143]:1154 "EHLO hqnvemgate24.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727911AbgDWK4S (ORCPT ); Thu, 23 Apr 2020 06:56:18 -0400 Received: from hqpgpgate102.nvidia.com (Not Verified[216.228.121.13]) by hqnvemgate24.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Thu, 23 Apr 2020 03:54:21 -0700 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate102.nvidia.com (PGP Universal service); Thu, 23 Apr 2020 03:56:18 -0700 X-PGP-Universal: processed; by hqpgpgate102.nvidia.com on Thu, 23 Apr 2020 03:56:18 -0700 Received: from DRHQMAIL107.nvidia.com (10.27.9.16) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 23 Apr 2020 10:56:18 +0000 Received: from [10.26.73.193] (172.20.13.39) by DRHQMAIL107.nvidia.com (10.27.9.16) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 23 Apr 2020 10:56:15 +0000 Subject: Re: [PATCH v2 1/2] i2c: tegra: Better handle case where CPU0 is busy for a long time To: Dmitry Osipenko , Thierry Reding , Laxman Dewangan , "Wolfram Sang" , Manikanta Maddireddy , Vidya Sagar CC: , , References: <20200324191217.1829-1-digetx@gmail.com> <20200324191217.1829-2-digetx@gmail.com> <1e259e22-c300-663a-e537-18d854e0f478@nvidia.com> <8cd085e1-f9fd-6ec0-9f7a-d5463f176a63@nvidia.com> <6f07e5c8-7916-7ea2-2fe7-d05f8f011471@nvidia.com> <77a31b2f-f525-ba9e-f1ae-2b474465bde4@gmail.com> <470b4de4-e98a-1bdc-049e-6259ad603507@nvidia.com> From: Jon Hunter Message-ID: Date: Thu, 23 Apr 2020 11:56:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [172.20.13.39] X-ClientProxiedBy: HQMAIL107.nvidia.com (172.20.187.13) To DRHQMAIL107.nvidia.com (10.27.9.16) Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1587639261; bh=WJzV1Jv0Ce9gv3BZE2UoDPJdOFk4EzrOQ7ddtDD0IpQ=; h=X-PGP-Universal:Subject:To:CC:References:From:Message-ID:Date: User-Agent:MIME-Version:In-Reply-To:X-Originating-IP: X-ClientProxiedBy:Content-Type:Content-Language: Content-Transfer-Encoding; b=Ta7B/bt77dEvbces/XxV6IU4FQh26/tXcAY+s0a6HoRsYoUFgLQPdJ1iUgUnkG6qh 59dwFvLfpdY5/akeYk1yAvgHKsmO8FF72rXcIUEMfMJm5bL+VFaHxGn+uggWde9vRV xZ98JrcqdRWxvrEie1pyeoAJXaANRUB98c1JB+L0r045gnxV6h+aoyd+5PAx/1dm5S 9G7++0Vc3lEfyfNUefaBeaTPM1uqwv0x+sJaAbOKv+varrIR2xE5G8NkIk//MY2kaa 7SDI6q0MHo/mRXTwKggeNmgsDfTX++qNVxWXnS5iz773Ec31sel+n9zOlwvq0xR4X/ I3EjyobghTudA== Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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