diff mbox series

[V2,3/4] soc: c3: Add support for power domains controller

Message ID 20230707003710.2667989-4-xianwei.zhao@amlogic.com
State New
Headers
Series Power: C3: add power domain driver |

Commit Message

Xianwei Zhao July 7, 2023, 12:37 a.m. UTC 
  Add support for C3 Power controller. C3 power control
registers are in secure domain, and should be accessed by SMC.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
V1 -> V2: Fixed some formatting.
---
 drivers/soc/amlogic/meson-secure-pwrc.c | 26 +++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Neil Armstrong July 7, 2023, 7:50 a.m. UTC | #1 
On 07/07/2023 02:37, Xianwei Zhao wrote:
> Add support for C3 Power controller. C3 power control
> registers are in secure domain, and should be accessed by SMC.
> 
> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: Fixed some formatting.
> ---
>   drivers/soc/amlogic/meson-secure-pwrc.c | 26 +++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
> index c11d65a3e3d9..a1ffebf70de3 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/amlogic,c3-pwrc.h>
>   #include <dt-bindings/power/meson-s4-power.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/firmware/meson/meson_sm.h>
> @@ -120,6 +121,22 @@ static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
>   	SEC_PD(RSA,	0),
>   };
>   
> +static struct meson_secure_pwrc_domain_desc c3_pwrc_domains[] = {
> +	SEC_PD(C3_NNA,	0),
> +	SEC_PD(C3_AUDIO,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_SDIOA,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_EMMC,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_USB_COMB, GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_SDCARD,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_ETH,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_GE2D,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_CVE,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_GDC_WRAP,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_ISP_TOP,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_MIPI_ISP_WRAP, GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_VCODEC,	0),
> +};
> +
>   static struct meson_secure_pwrc_domain_desc s4_pwrc_domains[] = {
>   	SEC_PD(S4_DOS_HEVC,	0),
>   	SEC_PD(S4_DOS_VDEC,	0),
> @@ -202,6 +219,11 @@ 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 amlogic_secure_c3_pwrc_data = {
> +	.domains = c3_pwrc_domains,
> +	.count = ARRAY_SIZE(c3_pwrc_domains),
> +};
> +
>   static struct meson_secure_pwrc_domain_data meson_secure_s4_pwrc_data = {
>   	.domains = s4_pwrc_domains,
>   	.count = ARRAY_SIZE(s4_pwrc_domains),
> @@ -212,6 +234,10 @@ static const struct of_device_id meson_secure_pwrc_match_table[] = {
>   		.compatible = "amlogic,meson-a1-pwrc",
>   		.data = &meson_secure_a1_pwrc_data,
>   	},
> +	{
> +		.compatible = "amlogic,c3-pwrc",
> +		.data = &amlogic_secure_c3_pwrc_data,
> +	},
>   	{
>   		.compatible = "amlogic,meson-s4-pwrc",
>   		.data = &meson_secure_s4_pwrc_data,

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Kevin Hilman July 26, 2023, 10:40 p.m. UTC | #2 
Xianwei Zhao <xianwei.zhao@amlogic.com> writes:

> Add support for C3 Power controller. C3 power control
> registers are in secure domain, and should be accessed by SMC.
>
> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: Fixed some formatting.
> ---
>  drivers/soc/amlogic/meson-secure-pwrc.c | 26 +++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
> index c11d65a3e3d9..a1ffebf70de3 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/amlogic,c3-pwrc.h>
>  #include <dt-bindings/power/meson-s4-power.h>
>  #include <linux/arm-smccc.h>
>  #include <linux/firmware/meson/meson_sm.h>
> @@ -120,6 +121,22 @@ static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
>  	SEC_PD(RSA,	0),
>  };
>  
> +static struct meson_secure_pwrc_domain_desc c3_pwrc_domains[] = {
> +	SEC_PD(C3_NNA,	0),
> +	SEC_PD(C3_AUDIO,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_SDIOA,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_EMMC,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_USB_COMB, GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_SDCARD,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_ETH,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_GE2D,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_CVE,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_GDC_WRAP,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_ISP_TOP,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_MIPI_ISP_WRAP, GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_VCODEC,	0),
> +};

All of these domains being hard-coded to ALWAYS_ON looks suspicious, and
can also be an indicator that the drivers for these domains are not
(properly) using runtime PM, or not connected to the correct domains the DT.

Similar to the tables for s4 and a1 in this same file, please describe
the reason that each of these domains needs to be hard coded to be
always on.

Thanks,

Kevin
Xianwei Zhao July 27, 2023, 2:24 a.m. UTC | #3 
Hi Kevin,
      Thanks for your reply.

On 2023/7/27 06:40, Kevin Hilman wrote:
> [ EXTERNAL EMAIL ]
> 
> Xianwei Zhao <xianwei.zhao@amlogic.com> writes:
> 
>> Add support for C3 Power controller. C3 power control
>> registers are in secure domain, and should be accessed by SMC.
>>
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> ---
>> V1 -> V2: Fixed some formatting.
>> ---
>>   drivers/soc/amlogic/meson-secure-pwrc.c | 26 +++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
>> index c11d65a3e3d9..a1ffebf70de3 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/amlogic,c3-pwrc.h>
>>   #include <dt-bindings/power/meson-s4-power.h>
>>   #include <linux/arm-smccc.h>
>>   #include <linux/firmware/meson/meson_sm.h>
>> @@ -120,6 +121,22 @@ static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
>>        SEC_PD(RSA,     0),
>>   };
>>
>> +static struct meson_secure_pwrc_domain_desc c3_pwrc_domains[] = {
>> +     SEC_PD(C3_NNA,  0),
>> +     SEC_PD(C3_AUDIO,        GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_SDIOA,        GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_EMMC, GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_USB_COMB, GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_SDCARD,       GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_ETH,  GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_GE2D, GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_CVE,  GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_GDC_WRAP,     GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_ISP_TOP,              GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_MIPI_ISP_WRAP, GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_VCODEC,       0),
>> +};
> 
> All of these domains being hard-coded to ALWAYS_ON looks suspicious, and
> can also be an indicator that the drivers for these domains are not
> (properly) using runtime PM, or not connected to the correct domains the DT.
> 
> Similar to the tables for s4 and a1 in this same file, please describe
> the reason that each of these domains needs to be hard coded to be
> always on.
Will do.
> 
> Thanks,
> 
> Kevin
diff mbox series

Patch

diff --git a/drivers/soc/amlogic/meson-secure-pwrc.c b/drivers/soc/amlogic/meson-secure-pwrc.c
index c11d65a3e3d9..a1ffebf70de3 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/amlogic,c3-pwrc.h>
 #include <dt-bindings/power/meson-s4-power.h>
 #include <linux/arm-smccc.h>
 #include <linux/firmware/meson/meson_sm.h>
@@ -120,6 +121,22 @@  static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
 	SEC_PD(RSA,	0),
 };
 
+static struct meson_secure_pwrc_domain_desc c3_pwrc_domains[] = {
+	SEC_PD(C3_NNA,	0),
+	SEC_PD(C3_AUDIO,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_SDIOA,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_EMMC,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_USB_COMB, GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_SDCARD,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_ETH,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_GE2D,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_CVE,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_GDC_WRAP,	GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_ISP_TOP,		GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_MIPI_ISP_WRAP, GENPD_FLAG_ALWAYS_ON),
+	SEC_PD(C3_VCODEC,	0),
+};
+
 static struct meson_secure_pwrc_domain_desc s4_pwrc_domains[] = {
 	SEC_PD(S4_DOS_HEVC,	0),
 	SEC_PD(S4_DOS_VDEC,	0),
@@ -202,6 +219,11 @@  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 amlogic_secure_c3_pwrc_data = {
+	.domains = c3_pwrc_domains,
+	.count = ARRAY_SIZE(c3_pwrc_domains),
+};
+
 static struct meson_secure_pwrc_domain_data meson_secure_s4_pwrc_data = {
 	.domains = s4_pwrc_domains,
 	.count = ARRAY_SIZE(s4_pwrc_domains),
@@ -212,6 +234,10 @@  static const struct of_device_id meson_secure_pwrc_match_table[] = {
 		.compatible = "amlogic,meson-a1-pwrc",
 		.data = &meson_secure_a1_pwrc_data,
 	},
+	{
+		.compatible = "amlogic,c3-pwrc",
+		.data = &amlogic_secure_c3_pwrc_data,
+	},
 	{
 		.compatible = "amlogic,meson-s4-pwrc",
 		.data = &meson_secure_s4_pwrc_data,