Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186
To:     Thierry Reding <thierry.reding@gmail.com>
Cc:     Sowjanya Komatineni <skomatineni@nvidia.com>, axboe@kernel.dk,
        jonathanh@nvidia.com, robh+dt@kernel.org, pchandru@nvidia.com,
        devicetree@vger.kernel.org, linux-ide@vger.kernel.org,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1617758731-12380-1-git-send-email-skomatineni@nvidia.com>
 <1617758731-12380-4-git-send-email-skomatineni@nvidia.com>
 <594c622e-4505-3448-1c7b-eae8f36cbad8@gmail.com>
 <49eba27e-18fa-b682-1385-2930dfff28ac@nvidia.com>
 <01fd01ab-f4e7-57ee-2ad7-2aabaeb92a0e@nvidia.com>
 <2ef2a124-9e4b-bc02-3830-8ef077638ced@gmail.com>
 <YG7/xPVoA4gPrMBf@orome.fritz.box>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <c05f6f69-b355-857a-1fe7-6c68596d141f@gmail.com>
Date:   Thu, 8 Apr 2021 17:41:21 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <YG7/xPVoA4gPrMBf@orome.fritz.box>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

08.04.2021 16:06, Thierry Reding пишет:
> On Thu, Apr 08, 2021 at 02:25:19AM +0300, Dmitry Osipenko wrote:
>> 08.04.2021 02:00, Sowjanya Komatineni пишет:
>>>
>>> On 4/7/21 3:57 PM, Sowjanya Komatineni wrote:
>>>>
>>>> On 4/7/21 2:36 PM, Dmitry Osipenko wrote:
>>>>> 07.04.2021 04:25, Sowjanya Komatineni пишет:
>>>>>> +    if (!tegra->pdev->dev.pm_domain) {
>>>>>> +        ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
>>>>>> +                            tegra->sata_clk,
>>>>>> +                            tegra->sata_rst);
>>>>>> +        if (ret)
>>>>>> +            goto disable_regulators;
>>>>>> +    }
>>>>> Hi,
>>>>>
>>>>> Why you haven't added condition for tegra_powergate_power_off()? I think
>>>>> it should break GENPD and legacy PD API isn't not supported by T186
>>>>> at all.
>>>>>
>>>>> I'm also not sure whether the power up/down sequence is correct using
>>>>> GENPD.
>>>>>
>>>>> Moreover the driver doesn't support runtime PM, so GENPD should be
>>>>> always off?
>>>>
>>>> This driver already using legacy PD API's so thought its supported and
>>>> added power domain device check during powergate_sequence_power_up and
>>>> yes same should apply for powergate_power_off as well. But if legacy
>>>> PD is not supported by T186 then not sure why original driver even
>>>> using these API's.
>>>>
>>>>
>>> Sorry just took a look and driver supports T210 and prior tegra as well.
>>> T210 and prior supports legacy PD and this check is applicable for
>>> those. So we should add power domain device check for power off as well.
>>
>> You could fix it with a follow up patch. Please try to test that
>> power-off works properly, at least try to unload the driver module and
>> re-load it.
> 
> Agreed, this should have the same check as above for
> tegra_powergate_power_off(). It currently works fine because on Tegra186
> tegra_powergate_power_off() (and all the other legacy APIs for that
> matter) will abort early since no power gates are implemented. The AHCI
> driver doesn't check for errors, so this will just fail silently. It's
> better to be symmetric, though, and add the check in both paths.

I missed that tegra_powergate_power_off() usage isn't fatal if GENPD is
used, thank you for the clarification.

>>> But for T186, we should have GENPD working once we add runtime PM
>>> support to driver.
>>>
>>> Preetham/Thierry, Can you confirm where SATA is un powergated prior to
>>> kernel?
>>>
>>>
>>>> But as RPM is not implemented yet for this driver, GENPD will be OFF
>>>> but SATA is not in power-gate by the time kernel starts and
>>>> functionally works.
>>>>
>>>> But with RPM implementation, I guess we can do proper power gate on/off.
>>>>
>>
>> I now recalled that GENPD turns ON all domains by default and then turns
>> them OFF only when driver entered into the RPM-suspended state. This
>> means that AHCI GENPD should be always-ON for T186, which should be okay
>> if this doesn't break power sequences.
> 
> Yeah, the generic PM domain will just stay enabled after probe and until
> remove. This does not impact the power sequences because they have to be
> completely implemented in the power domains code anyway. With the legacy
> API we used to need more rigorous sequences in the individual drivers,
> but with generic PM domains none of that should be necessary, though it
> also doesn't hurt, so some of the unnecessary clock enablement code is
> kept for simplicity.
> 
> To be honest, I'm not sure if it's worth adding runtime PM support for
> this driver. If this top-level layer has a way of getting notification
> when no device was detected, then it might make some sense to turn off
> the power domain and the regulators again, but I'm not sure if that's
> the case. tegra_ahci_host_stop() seems like it might be usable for that
> so yeah, that might work. We currently do turn off the powergate in that
> case, so extending that power optimization to Tegra186 using runtime PM
> makes sense.

Alright, then this all should be good as-is.