[PATCH 0/2] Power: meson-s4: add s4 power domain driver

[PATCH 0/2] Power: meson-s4: add s4 power domain driver

[Shunzhou Jiang]

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

[PATCH 1/2] dt-bindings: power: add Amlogic s4 power domains bindings

[Shunzhou Jiang]

Add the bindings for the Amlogic Secure power domains, controlling the
secure power domains.

The bindings targets the Amlogic s4, in which the power domains registers
are in secure world.

Signed-off-by: Shunzhou Jiang <shunzhou.jiang@amlogic.com>
---
 .../power/amlogic,meson-sec-pwrc.yaml         |  3 ++-
 include/dt-bindings/power/meson-s4-power.h    | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/power/meson-s4-power.h

diff --git a/Documentation/devicetree/bindings/power/amlogic,meson-sec-pwrc.yaml b/Documentation/devicetree/bindings/power/amlogic,meson-sec-pwrc.yaml
index 5dae04d2936c..7657721a4e96 100644
--- a/Documentation/devicetree/bindings/power/amlogic,meson-sec-pwrc.yaml
+++ b/Documentation/devicetree/bindings/power/amlogic,meson-sec-pwrc.yaml
@@ -12,13 +12,14 @@ maintainers:
   - Jianxin Pan <jianxin.pan@amlogic.com>
 
 description: |+
-  Secure Power Domains used in Meson A1/C1 SoCs, and should be the child node
+  Secure Power Domains used in Meson A1/C1/S4 SoCs, and should be the child node
   of secure-monitor.
 
 properties:
   compatible:
     enum:
       - amlogic,meson-a1-pwrc
+      - amlogic,meson-s4-pwrc
 
   "#power-domain-cells":
     const: 1
diff --git a/include/dt-bindings/power/meson-s4-power.h b/include/dt-bindings/power/meson-s4-power.h
new file mode 100644
index 000000000000..462dd2cb938b
--- /dev/null
+++ b/include/dt-bindings/power/meson-s4-power.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+/*
+ * Copyright (c) 2021 Amlogic, Inc.
+ * Author: Shunzhou Jiang <shunzhou.jiang@amlogic.com>
+ */
+
+#ifndef _DT_BINDINGS_MESON_S4_POWER_H
+#define _DT_BINDINGS_MESON_S4_POWER_H
+
+#define PWRC_S4_DOS_HEVC_ID	0
+#define PWRC_S4_DOS_VDEC_ID	1
+#define PWRC_S4_VPU_HDMI_ID	2
+#define PWRC_S4_USB_COMB_ID	3
+#define PWRC_S4_GE2D_ID		4
+#define PWRC_S4_ETH_ID		5
+#define PWRC_S4_DEMOD_ID	6
+#define PWRC_S4_AUDIO_ID	7
+
+#endif
-- 
2.34.1

[PATCH 2/2] soc: s4: Add support for power domains controller

[Shunzhou Jiang]

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 <shunzhou.jiang@amlogic.com>
---
 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

Re: [PATCH 2/2] soc: s4: Add support for power domains controller

[Kevin Hilman]

Hi Shunzhou,

Shunzhou Jiang <shunzhou.jiang@amlogic.com> 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 <shunzhou.jiang@amlogic.com>

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

Re: Re: [PATCH 2/2] soc: s4: Add support for power domains controller

[Kevin Hilman]

"shunzhou.jiang@amlogic.com" <shunzhou.jiang@amlogic.com> 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

Re: Re: [PATCH 2/2] soc: s4: Add support for power domains controller

[Kevin Hilman]

Hi Shunzhou,

"shunzhou.jiang@amlogic.com" <shunzhou.jiang@amlogic.com> 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

Re: Re: [PATCH 2/2] soc: s4: Add support for power domains controller

[Kevin Hilman]

"shunzhou.jiang@amlogic.com" <shunzhou.jiang@amlogic.com> 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: shunzhou.jiang@amlogic.com; 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,
>  
> "shunzhou.jiang@amlogic.com" <shunzhou.jiang@amlogic.com> 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
>