Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Kevin Hilman <khilman@baylibre.com>
To:     Neil Armstrong <narmstrong@baylibre.com>, ulf.hansson@linaro.org
Cc:     linux-pm@vger.kernel.org, linux-amlogic@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller
In-Reply-To: <0ac1cf30-1796-a549-e195-0f94c4a85993@baylibre.com>
References: <20190821114121.10430-1-narmstrong@baylibre.com> <20190821114121.10430-3-narmstrong@baylibre.com> <7hzhk25ct3.fsf@baylibre.com> <b6cfb770-76eb-00b1-e088-1a73b7978f33@baylibre.com> <7hzhk12b6m.fsf@baylibre.com> <0ac1cf30-1796-a549-e195-0f94c4a85993@baylibre.com>
Date:   Fri, 23 Aug 2019 08:06:26 -0700
Message-ID: <7h36hr2a6l.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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