This patchset adds Power controller driver support for Meson-S4 SoC
Likes Meson-A1, the power domains register only can access in secure world
Shunzhou Jiang (2):
dt-bindings: power: add Amlogic s4 power domains bindings
soc: s4: Add support for power domains controller
.../power/amlogic,meson-sec-pwrc.yaml | 3 ++-
drivers/soc/amlogic/meson-secure-pwrc.c | 21 +++++++++++++++++++
include/dt-bindings/power/meson-s4-power.h | 19 +++++++++++++++++
3 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 include/dt-bindings/power/meson-s4-power.h
--
2.34.1
Add support s4 Power controller. In s4, power control
registers are in secure domain, and should be accessed by smc.
Signed-off-by: Shunzhou Jiang <[email protected]>
---
drivers/soc/amlogic/meson-secure-pwrc.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
index 59bd195fa9c9..8fee01aabab6 100644
--- a/drivers/soc/amlogic/meson-secure-pwrc.c
+++ b/drivers/soc/amlogic/meson-secure-pwrc.c
@@ -11,6 +11,7 @@
#include <linux/platform_device.h>
#include <linux/pm_domain.h>
#include <dt-bindings/power/meson-a1-power.h>
+#include <dt-bindings/power/meson-s4-power.h>
#include <linux/arm-smccc.h>
#include <linux/firmware/meson/meson_sm.h>
#include <linux/module.h>
@@ -119,6 +120,17 @@ static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
SEC_PD(RSA, 0),
};
+static struct meson_secure_pwrc_domain_desc s4_pwrc_domains[] = {
+ SEC_PD(S4_DOS_HEVC, 0),
+ SEC_PD(S4_DOS_VDEC, 0),
+ SEC_PD(S4_VPU_HDMI, GENPD_FLAG_ALWAYS_ON),
+ SEC_PD(S4_USB_COMB, GENPD_FLAG_ALWAYS_ON),
+ SEC_PD(S4_GE2D, 0),
+ SEC_PD(S4_ETH, GENPD_FLAG_ALWAYS_ON),
+ SEC_PD(S4_DEMOD, 0),
+ SEC_PD(S4_AUDIO, 0),
+};
+
static int meson_secure_pwrc_probe(struct platform_device *pdev)
{
int i;
@@ -187,11 +199,20 @@ static struct meson_secure_pwrc_domain_data meson_secure_a1_pwrc_data = {
.count = ARRAY_SIZE(a1_pwrc_domains),
};
+static struct meson_secure_pwrc_domain_data meson_secure_s4_pwrc_data = {
+ .domains = s4_pwrc_domains,
+ .count = ARRAY_SIZE(s4_pwrc_domains),
+};
+
static const struct of_device_id meson_secure_pwrc_match_table[] = {
{
.compatible = "amlogic,meson-a1-pwrc",
.data = &meson_secure_a1_pwrc_data,
},
+ {
+ .compatible = "amlogic,meson-s4-pwrc",
+ .data = &meson_secure_s4_pwrc_data,
+ },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, meson_secure_pwrc_match_table);
--
2.34.1
Hi Shunzhou,
Shunzhou Jiang <[email protected]> writes:
> Add support s4 Power controller. In s4, power control
> registers are in secure domain, and should be accessed by smc.
>
> Signed-off-by: Shunzhou Jiang <[email protected]>
Thank you for adding support for S4 power domains.
> ---
> drivers/soc/amlogic/meson-secure-pwrc.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
> index 59bd195fa9c9..8fee01aabab6 100644
> --- a/drivers/soc/amlogic/meson-secure-pwrc.c
> +++ b/drivers/soc/amlogic/meson-secure-pwrc.c
> @@ -11,6 +11,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_domain.h>
> #include <dt-bindings/power/meson-a1-power.h>
> +#include <dt-bindings/power/meson-s4-power.h>
> #include <linux/arm-smccc.h>
> #include <linux/firmware/meson/meson_sm.h>
> #include <linux/module.h>
> @@ -119,6 +120,17 @@ static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
> SEC_PD(RSA, 0),
> };
>
> +static struct meson_secure_pwrc_domain_desc s4_pwrc_domains[] = {
> + SEC_PD(S4_DOS_HEVC, 0),
> + SEC_PD(S4_DOS_VDEC, 0),
> + SEC_PD(S4_VPU_HDMI, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(S4_USB_COMB, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(S4_GE2D, 0),
> + SEC_PD(S4_ETH, GENPD_FLAG_ALWAYS_ON),
> + SEC_PD(S4_DEMOD, 0),
> + SEC_PD(S4_AUDIO, 0),
> +};
We should avoid the GENPD_FLAG_ALWAYS_ON unless strictly necessary. If
you look at a1_pwrc_domains[] in this same driver, any use of this flag
has a comment for why it's needed, and it's usually because the domain
is used by low-level SoC/PM code not controlled by linux.
All of these appear to be domains that linux should have driver control,
so should not be set to always on.
Kevin
"[email protected]" <[email protected]> writes:
> Hi Kevin:
> Thanks your reply.
> Please refer to below comment,
>> S4_VPU_HDMI: for vpu domain, this domain provide power to many moudles(osd, vpp, hdr, dv, di), if close, will cause system crash
>> S4_USB_COMB domain: for usb, if not always on, all usb status will clear to 0, that's not right status for usb
Yes, I understand, this is teh same as for other SoCs (e.g. A1.) The
solution is not to set the domain to always on. The solution is for the
drivers for the devices in these domains use runtime PM so that when the
drivers are active, the power domain does not get shut off.
>> S4_ETH: for ethernet online wakeup, and if power down, status also not right
OK, this one makes sense for "always on" since it used by firmware for
wakeup.
Kevin
Hi Shunzhou,
"[email protected]" <[email protected]> writes:
> Hi Kevin:
> Thanks your kindly reply
You're welcome. For future reference, please avoid top-posting. See:
https://www.kernel.org/doc/html/latest/process/2.Process.html?highlight=top-posting#mailing-lists
> For those domains, default is active, we hope not close when in use or not in use, in our case,
> only runtime PM (include suspend) control this, so set always on flag to avoid domain shutdown,
my question remains: why do want to keep these powered on even when they
are not in use?
The goal of the power-domain framework + runtime PM is to be able to
save power by turnin off power domains when they are not in use.
> if you also have concern, we can control this not in kernel, but this not our expect.
My strong preference is that this is controlled by the kernel.
Kevin
"[email protected]" <[email protected]> writes:
> Hi Kevin:
> Thanks your reply
> This has been mentioned before
Yes, you mentioned what the domains are for, but you have not answered
why the domains need to remain powered on when they are not in use.
>> S4_VPU_HDMI: for vpu, this domain provide power to many moudles(osd, vpp, hdr, dv, di), if close, will cause system crash
Why does the system crash? Most likely because a driver in this domain
is still trying to access. This suggests that that the DT shoud be
updated so those devices are listed in the power domain, and also that
those drivers use runtime PM so that the power domain is turned off ONLY
when the devices are not in use.
>> S4_USB_COMB domain: for usb, if not always on, all usb status will clear to 0, that's not right status for usb
Why is this not the right status for usb?
Again, my question is: why does the power domain need to stay on when
the underlying devices are NOT used.
If the underlying devices are in use, then if the power domain turns off
it is a software bug. Devices need to be connected to the power domain
(in DT) and their drivers need to use runtime PM. If that is done
correctly, then there is no reason for the power domain to be turned off
when the devices are in use.
In other words, your solution to always keep the power domain on has a
two main problems:
1) it just hides improperly configured, or poorly written drivers
2) it wastes power and prevents low-power usecases
Kevin
> Shunzhou Jiang
> SW Department
>
> From: Kevin Hilman
> Date: 2022-02-12 02:52
> To: [email protected]; linux-arm-kernel; linux-amlogic; linux-kernel
> CC: Neil Armstrong; jbrunet; Martin Blumenstingl; jianxin.pan
> Subject: Re: Re: [PATCH 2/2] soc: s4: Add support for power domains controller
> [ EXTERNAL EMAIL ]
>
> Hi Shunzhou,
>
> "[email protected]" <[email protected]> writes:
>
>> Hi Kevin:
>> Thanks your kindly reply
>
> You're welcome. For future reference, please avoid top-posting. See:
> https://www.kernel.org/doc/html/latest/process/2.Process.html?highlight=top-posting#mailing-lists
>
>> For those domains, default is active, we hope not close when in use or not in use, in our case,
>> only runtime PM (include suspend) control this, so set always on flag to avoid domain shutdown,
>
> my question remains: why do want to keep these powered on even when they
> are not in use?
>
> The goal of the power-domain framework + runtime PM is to be able to
> save power by turnin off power domains when they are not in use.
>
>> if you also have concern, we can control this not in kernel, but this not our expect.
>
> My strong preference is that this is controlled by the kernel.
>
> Kevin
>