Re: [PATCH v4 3/3] ata: ahci_tegra: Add AHCI support for Tegra186

[Dmitry Osipenko <digetx@gmail.com>]

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.