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.

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

Thierry