Re: [PATCH v14 20/39] pwm: tegra: Add runtime PM and OPP support

[Dmitry Osipenko <digetx@gmail.com>]

30.10.2021 03:47, Dmitry Osipenko пишет:
> 29.10.2021 21:06, Rafael J. Wysocki пишет:
> ...
>>>>> I just noticed that RPM core doesn't reset RPM-enable count of a device
>>>>> on driver's unbind (pm_runtime_reinit). It was a bad idea to use
>>>>> devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is
>>>>> disabled twice on driver's removal, and thus, RPM will never be enabled
>>>>> again.
>>>>>
>>>>> I'll fix it for PWM and other drivers in this series, in v15.
>>>>
>>>> Well, for the record, IMV using pm_runtime_force_suspend() is
>>>> generally a questionable idea.
>>>>
>>>
>>> Please clarify why it's a questionable idea.
>>
>> There are a few reasons.
>>
>> Generally speaking, it makes assumptions that may not be satisfied.
>>
>> For instance, it assumes that the driver will never have to work with
>> the ACPI PM domain, because the ACPI PM domain has a separate set of
>> callbacks for system-wide suspend and resume and they are not the same
>> as its PM-runtime callbacks, so if the driver is combined with the
>> ACPI PM domain, running pm_runtime_force_suspend() may not work as
>> expected.
> 
> ACPI is irrelevant to the drivers touched by this series.
> 
> This series is about older ARM32 Tegra SoCs which either don't have ACPI
> at all or it's unusable by Linux, like a non-standard ACPI of M$ Surface
> tablets.

Although, there are VIC and NVDEC drivers of newer Tegra SoCs touched by
this series. Maybe they could get ACPI support in the future, but this
needs to be clarified. Perhaps Thierry or Mikko could comment on it.

>> Next, it assumes that PM-runtime is actually enabled for the device
>> and the RPM_STATUS of it is valid when it is running.
> 
> Runtime PM presence is mandatory for Tegra and drivers take care of
> enabling it, should be good here.
> 
>> Further, it assumes that the PM-runtime suspend callback of the driver
>> will always be suitable for system-wide suspend which may not be the
>> case if the device can generate wakeup signals and it is not allowed
>> to wake up the system from sleep by user space.
> 
> There are no such 'wakeup' drivers in the context of this patchset.
> 
>> Next, if the driver has to work with a PM domain (other than the ACPI
>> one) or bus type that doesn't take the pm_runtime_force_suspend()
>> explicitly into account, it may end up running the runtime-suspend
>> callback provided by that entity from within its system-wide suspend
>> callback which may not work as expected.
> 
> Only platform bus and generic power domain are relevant for this patchset.
> 
>> I guess I could add a few if I had to.
>>
> 
> So far I can't see any problems.
> 
> If you have a better alternative on yours mind, please share.
>