Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller

Kevin Hilman
2019-08-23T15:06:31Z

Neil Armstrong <narmstrong@baylibre.com> writes:

[...]

>>> It's for legacy when VPU is initialized from vendor U-Boot, look at commit :
>>> 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
>>>
>>> In the case the VPU power domain has been powered on by the bootloader
>>> and no driver are attached to this power domain, the genpd will power it
>>> off after a certain amount of time, but the clocks hasn't been enabled
>>> by the kernel itself and the power-off will trigger some faults.
>>> This patch enable the clocks to have a coherent state for an eventual
>>> poweroff and switches to the pm_domain_always_on_gov governor.
>>
>> The key phrase there being "and no driver is attached". Now that we
>> have a driver, it claims this domain so I don't think it will be
>> powered off:
>>
>> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>> domain status slaves
>> /device runtime status
>> ----------------------------------------------------------------------
>> ETH on
>> /devices/platform/soc/ff3f0000.ethernet unsupported
>> AUDIO off-0
>> GE2D off-0
>> PCI off-0
>> USB on
>> /devices/platform/soc/ffe09000.usb active
>> NNA off-0
>> VPU on
>> /devices/platform/soc/ff900000.vpu unsupported
>>
>> In my tests with a framebuffer console (over HDMI), I don't see the
>> display being powered off.
>
> It's in the case where the driver is a module loaded by the post-initramfs
> system after the genpd timeout, or if the display driver is disabled.
>
> In the later I had some system failures when vendor u-boot enabled the
> display and genpd disabled the power domain later on.

OK, thanks for the explanation. I get it now.

>>
>>> I could set always-on governor only if the domain was already enabled,
>>> what do you think ?
>>
>> I don't think that's necessary now that we have a driver. We really
>> want to be able to power-down this domain when the display is not in
>> use, and if you use always_on, that will never happen.
>>
>>> And seems I'm also missing the "This patch enable the clocks".
>>
>> I'm not sure what patch you're referring to.
>
> It's also added in 339cd0ea082287ea8e2b7e7159a5a33665a2cbe3 "soc: amlogic: meson-gx-pwrc-vpu: fix power-off when powered by bootloader"
>
> I would like to keep the same behavior as meson-gx-pwrc-vpu, since it works fine
> and we debugged all the issues we got.

OK, that's fine with me.

We'll have to revist when we start using runtime PM enabled drviers and
want to power down the display IPs on idle, but that's fine to do later.

Thanks,

Kevin