[PATCH V2 0/6] Power: T7: add power domain driver

[PATCH V2 0/6] Power: T7: add power domain driver

[Xianwei Zhao]

First patch is that remove C3 some power domain ALWAYS_ON property.
Second patch is that add driver to support power parent node. 
Third patch is that turn on power if initial power domain with
"AWAY_ON" property state is off.

Other patchs adds power controller driver support for Amlogic T7 SoC.

Changes Since v1:
 -Fix license from "GPL-2.0-only OR .*" to "GPL-2.0-only OR MIT".
 -Modify T7_NIC flag  "ALWAYS_ON"

xianwei.zhao (6):
  soc: amlogic: modify some power domains property
  soc: amlogic: add driver to support power parent node
  soc: amlogic: init power domain state
  dt-bindings: power: add Amlogic T7 power domains
  soc: amlogic: Add support for T7 power domains controller
  arm64: dts: add support for T7 power domain controller

 .../power/amlogic,meson-sec-pwrc.yaml         |   3 +-
 arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi   |   6 +
 drivers/genpd/amlogic/meson-secure-pwrc.c     | 129 ++++++++++++++++--
 include/dt-bindings/power/amlogic,t7-pwrc.h   |  63 +++++++++
 4 files changed, 186 insertions(+), 15 deletions(-)
 create mode 100644 include/dt-bindings/power/amlogic,t7-pwrc.h


base-commit: 413f5c02929bb33042bbc4ee233166550a5fca70
-- 
2.37.1

Re: [PATCH V2 1/6] soc: amlogic: modify some power domains property

[Neil Armstrong]

Hi,

On 24/08/2023 07:59, Xianwei Zhao wrote:
> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
> 
> Some power domains for C3 can be using runtime PM,
> remove ALWAYS_ON property. And add some power domains
> description when ALWAYS_ON property.

The subject should now be:
genpd: amlogic: ....

same for patches 2 & 5.

> 
> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: None
> ---
>   drivers/genpd/amlogic/meson-secure-pwrc.c | 25 ++++++++++++-----------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c b/drivers/genpd/amlogic/meson-secure-pwrc.c
> index 89c881c56cd7..5ac2437ab8ad 100644
> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
> @@ -122,18 +122,19 @@ static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
>   };
>   
>   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_NNA,		0),
> +	SEC_PD(C3_AUDIO,	0),
> +	SEC_PD(C3_SDIOA,	0),
> +	SEC_PD(C3_EMMC,		0),
> +	SEC_PD(C3_USB_COMB,	0),
> +	SEC_PD(C3_SDCARD,	0),
> +	/* ETH is for ethernet online wakeup, and should be always on */
> +	SEC_PD(C3_ETH,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(C3_GE2D,		0),
> +	SEC_PD(C3_CVE,		0),
> +	SEC_PD(C3_GDC_WRAP,	0),
> +	SEC_PD(C3_ISP_TOP,	0),
> +	SEC_PD(C3_MIPI_ISP_WRAP, 0),
>   	SEC_PD(C3_VCODEC,	0),
>   };
>   
> 
> base-commit: 413f5c02929bb33042bbc4ee233166550a5fca70

With changed subject:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH V2 2/6] soc: amlogic: add driver to support power parent node

[Neil Armstrong]

Hi,

On 24/08/2023 07:59, Xianwei Zhao wrote:
> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
> 
> Some power domains depends on other domains, Such as Amlogic T7 SoC.
> Add parent node to support this case.
> 
> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: None
> ---
>   drivers/genpd/amlogic/meson-secure-pwrc.c | 26 ++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c b/drivers/genpd/amlogic/meson-secure-pwrc.c
> index 5ac2437ab8ad..76527f4946b4 100644
> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
> @@ -19,10 +19,12 @@
>   
>   #define PWRC_ON		1
>   #define PWRC_OFF	0
> +#define PWRC_NO_PARENT  ((1UL << (sizeof(unsigned int) * 8)) - 1)

This is fishy, just use UINT_MAX

>   
>   struct meson_secure_pwrc_domain {
>   	struct generic_pm_domain base;
>   	unsigned int index;
> +	unsigned int parent;
>   	struct meson_secure_pwrc *pwrc;
>   };
>   
> @@ -34,6 +36,7 @@ struct meson_secure_pwrc {
>   
>   struct meson_secure_pwrc_domain_desc {
>   	unsigned int index;
> +	unsigned int parent;
>   	unsigned int flags;
>   	char *name;
>   	bool (*is_off)(struct meson_secure_pwrc_domain *pwrc_domain);
> @@ -90,8 +93,19 @@ static int meson_secure_pwrc_on(struct generic_pm_domain *domain)
>   {						\
>   	.name = #__name,			\
>   	.index = PWRC_##__name##_ID,		\
> -	.is_off = pwrc_secure_is_off,	\
> +	.is_off = pwrc_secure_is_off,		\
>   	.flags = __flag,			\
> +	.parent = PWRC_NO_PARENT,		\
> +}
> +
> +#define TOP_PD(__name, __flag, __parent)	\
> +[PWRC_##__name##_ID] =				\
> +{						\
> +	.name = #__name,			\
> +	.index = PWRC_##__name##_ID,		\
> +	.is_off = pwrc_secure_is_off,		\
> +	.flags = __flag,			\
> +	.parent = __parent,			\
>   }
>   
>   static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
> @@ -202,6 +216,7 @@ static int meson_secure_pwrc_probe(struct platform_device *pdev)
>   
>   		dom->pwrc = pwrc;
>   		dom->index = match->domains[i].index;
> +		dom->parent = match->domains[i].parent;
>   		dom->base.name = match->domains[i].name;
>   		dom->base.flags = match->domains[i].flags;
>   		dom->base.power_on = meson_secure_pwrc_on;
> @@ -212,6 +227,15 @@ static int meson_secure_pwrc_probe(struct platform_device *pdev)
>   		pwrc->xlate.domains[i] = &dom->base;
>   	}
>   
> +	for (i = 0; i < match->count; i++) {
> +		struct meson_secure_pwrc_domain *dom = pwrc->domains;
> +
> +		if (!match->domains[i].name || match->domains[i].parent == PWRC_NO_PARENT)
> +			continue;
> +
> +		pm_genpd_add_subdomain(&dom[dom[i].parent].base, &dom[i].base);
> +	}
> +
>   	return of_genpd_add_provider_onecell(pdev->dev.of_node, &pwrc->xlate);
>   }
>   

Re: [PATCH V2 3/6] soc: amlogic: init power domain state

[Neil Armstrong]

Hi,

On 24/08/2023 07:59, Xianwei Zhao wrote:
> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
> 
> If initial power domain with 'AWAY_ON' property state is off,
> turn on the power.
> 
> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: None
> ---
>   drivers/genpd/amlogic/meson-secure-pwrc.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c b/drivers/genpd/amlogic/meson-secure-pwrc.c
> index 76527f4946b4..3e7e3bd25d1f 100644
> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
> @@ -55,7 +55,7 @@ static bool pwrc_secure_is_off(struct meson_secure_pwrc_domain *pwrc_domain)
>   			  pwrc_domain->index, 0, 0, 0, 0) < 0)
>   		pr_err("failed to get power domain status\n");
>   
> -	return is_off;
> +	return !!is_off;

Can you explain this ? the function returns bool, so if if_off is > 0, it would return true,
so I don't see why you would need to transform is_off into 1 or 0 using !!.

>   }
>   
>   static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
> @@ -222,6 +222,9 @@ static int meson_secure_pwrc_probe(struct platform_device *pdev)
>   		dom->base.power_on = meson_secure_pwrc_on;
>   		dom->base.power_off = meson_secure_pwrc_off;
>   
> +		if (match->domains[i].is_off(dom) && (dom->base.flags & GENPD_FLAG_ALWAYS_ON))
> +			meson_secure_pwrc_on(&dom->base);
> +
>   		pm_genpd_init(&dom->base, NULL, match->domains[i].is_off(dom));
>   
>   		pwrc->xlate.domains[i] = &dom->base;

Re: [PATCH V2 5/6] soc: amlogic: Add support for T7 power domains controller

[Neil Armstrong]

On 24/08/2023 07:59, Xianwei Zhao wrote:
> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
> 
> Add support for T7 power controller. T7 power control
> registers are in secure domain, and should be accessed by SMC.
> 
> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: Modify T7_NIC flag  "ALWAYS_ON"
> ---
>   drivers/genpd/amlogic/meson-secure-pwrc.c | 73 +++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c b/drivers/genpd/amlogic/meson-secure-pwrc.c
> index 3e7e3bd25d1f..2233f6cb7e71 100644
> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
> @@ -13,6 +13,7 @@
>   #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 <dt-bindings/power/amlogic,t7-pwrc.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/firmware/meson/meson_sm.h>
>   #include <linux/module.h>
> @@ -164,6 +165,69 @@ static struct meson_secure_pwrc_domain_desc s4_pwrc_domains[] = {
>   	SEC_PD(S4_AUDIO,	0),
>   };
>   
> +static struct meson_secure_pwrc_domain_desc t7_pwrc_domains[] = {
> +	SEC_PD(T7_DSPA,		0),
> +	SEC_PD(T7_DSPB,		0),
> +	TOP_PD(T7_DOS_HCODEC,	0, PWRC_T7_NIC3_ID),
> +	TOP_PD(T7_DOS_HEVC,	0, PWRC_T7_NIC3_ID),
> +	TOP_PD(T7_DOS_VDEC,	0, PWRC_T7_NIC3_ID),
> +	TOP_PD(T7_DOS_WAVE,	0, PWRC_T7_NIC3_ID),
> +	SEC_PD(T7_VPU_HDMI,	0),
> +	SEC_PD(T7_USB_COMB,	0),
> +	SEC_PD(T7_PCIE,		0),
> +	TOP_PD(T7_GE2D,		0, PWRC_T7_NIC3_ID),
> +	/* SRAMA is used as ATF runtime memory, and should be always on */
> +	SEC_PD(T7_SRAMA,	GENPD_FLAG_ALWAYS_ON),
> +	/* SRAMB is used as ATF runtime memory, and should be always on */
> +	SEC_PD(T7_SRAMB,	GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(T7_HDMIRX,	0),
> +	SEC_PD(T7_VI_CLK1,	0),
> +	SEC_PD(T7_VI_CLK2,	0),
> +	/* ETH is for ethernet online wakeup, and should be always on */
> +	SEC_PD(T7_ETH,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(T7_ISP,		0),
> +	SEC_PD(T7_MIPI_ISP,	0),
> +	TOP_PD(T7_GDC,		0, PWRC_T7_NIC3_ID),
> +	TOP_PD(T7_DEWARP,	0, PWRC_T7_NIC3_ID),
> +	SEC_PD(T7_SDIO_A,	0),
> +	SEC_PD(T7_SDIO_B,	0),
> +	SEC_PD(T7_EMMC,		0),
> +	TOP_PD(T7_MALI_SC0,	0, PWRC_T7_NNA_TOP_ID),
> +	TOP_PD(T7_MALI_SC1,	0, PWRC_T7_NNA_TOP_ID),
> +	TOP_PD(T7_MALI_SC2,	0, PWRC_T7_NNA_TOP_ID),
> +	TOP_PD(T7_MALI_SC3,	0, PWRC_T7_NNA_TOP_ID),
> +	SEC_PD(T7_MALI_TOP,	0),
> +	TOP_PD(T7_NNA_CORE0,	0, PWRC_T7_NNA_TOP_ID),
> +	TOP_PD(T7_NNA_CORE1,	0, PWRC_T7_NNA_TOP_ID),
> +	TOP_PD(T7_NNA_CORE2,	0, PWRC_T7_NNA_TOP_ID),
> +	TOP_PD(T7_NNA_CORE3,	0, PWRC_T7_NNA_TOP_ID),
> +	SEC_PD(T7_NNA_TOP,	0),
> +	SEC_PD(T7_DDR0,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(T7_DDR1,		GENPD_FLAG_ALWAYS_ON),
> +	/* DMC0 is for DDR PHY ana/dig and DMC, and should be always on */
> +	SEC_PD(T7_DMC0,		GENPD_FLAG_ALWAYS_ON),
> +	/* DMC1 is for DDR PHY ana/dig and DMC, and should be always on */
> +	SEC_PD(T7_DMC1,		GENPD_FLAG_ALWAYS_ON),
> +	/* NOC is related to clk bus, and should be always on */
> +	SEC_PD(T7_NOC,		GENPD_FLAG_ALWAYS_ON),
> +	/* NIC is for the Arm NIC-400 interconnect, and should be always on */
> +	SEC_PD(T7_NIC2,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(T7_NIC3,		0),
> +	/* CPU accesses the interleave data to the ddr need cci, and should be always on */
> +	SEC_PD(T7_CCI,		GENPD_FLAG_ALWAYS_ON),
> +	SEC_PD(T7_MIPI_DSI0,	0),
> +	SEC_PD(T7_SPICC0,	0),
> +	SEC_PD(T7_SPICC1,	0),
> +	SEC_PD(T7_SPICC2,	0),
> +	SEC_PD(T7_SPICC3,	0),
> +	SEC_PD(T7_SPICC4,	0),
> +	SEC_PD(T7_SPICC5,	0),
> +	SEC_PD(T7_EDP0,		0),
> +	SEC_PD(T7_EDP1,		0),
> +	SEC_PD(T7_MIPI_DSI1,	0),
> +	SEC_PD(T7_AUDIO,	0),
> +};
> +
>   static int meson_secure_pwrc_probe(struct platform_device *pdev)
>   {
>   	int i;
> @@ -257,6 +321,11 @@ static struct meson_secure_pwrc_domain_data meson_secure_s4_pwrc_data = {
>   	.count = ARRAY_SIZE(s4_pwrc_domains),
>   };
>   
> +static struct meson_secure_pwrc_domain_data amlogic_secure_t7_pwrc_data = {
> +	.domains = t7_pwrc_domains,
> +	.count = ARRAY_SIZE(t7_pwrc_domains),
> +};
> +
>   static const struct of_device_id meson_secure_pwrc_match_table[] = {
>   	{
>   		.compatible = "amlogic,meson-a1-pwrc",
> @@ -270,6 +339,10 @@ static const struct of_device_id meson_secure_pwrc_match_table[] = {
>   		.compatible = "amlogic,meson-s4-pwrc",
>   		.data = &meson_secure_s4_pwrc_data,
>   	},
> +	{
> +		.compatible = "amlogic,t7-pwrc",
> +		.data = &amlogic_secure_t7_pwrc_data,
> +	},
>   	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(of, meson_secure_pwrc_match_table);

With updated subject:

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH V2 6/6] arm64: dts: add support for T7 power domain controller

[Neil Armstrong]

On 24/08/2023 07:59, Xianwei Zhao wrote:
> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
> 
> Enable power domain controller for Amlogic T7 SoC
> 
> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
> ---
> V1 -> V2: None
> ---
>   arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> index 1423d4a79156..23cdad1b425b 100644
> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> @@ -4,6 +4,7 @@
>    */
>   
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/power/amlogic,t7-pwrc.h>
>   
>   / {
>   	interrupt-parent = <&gic>;
> @@ -118,6 +119,11 @@ psci {
>   
>   	sm: secure-monitor {
>   		compatible = "amlogic,meson-gxbb-sm";
> +
> +		pwrc: power-controller {
> +			compatible = "amlogic,t7-pwrc";
> +			#power-domain-cells = <1>;
> +		};
>   	};
>   
>   	soc {

Please change the subject to:
arm64: dts: amlogic: t7: add power domain controller node

With that:
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH V2 1/6] soc: amlogic: modify some power domains property

[Xianwei Zhao]

Hi Neil,
     Thanks for your replay.

On 2023/8/24 16:24, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 24/08/2023 07:59, Xianwei Zhao wrote:
>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>
>> Some power domains for C3 can be using runtime PM,
>> remove ALWAYS_ON property. And add some power domains
>> description when ALWAYS_ON property.
> 
> The subject should now be:
> genpd: amlogic: ....
> 
> same for patches 2 & 5.
Will do.
> 
>>
>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
>> ---
>> V1 -> V2: None
>> ---
>>   drivers/genpd/amlogic/meson-secure-pwrc.c | 25 ++++++++++++-----------
>>   1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c 
>> b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> index 89c881c56cd7..5ac2437ab8ad 100644
>> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
>> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> @@ -122,18 +122,19 @@ static struct meson_secure_pwrc_domain_desc 
>> a1_pwrc_domains[] = {
>>   };
>>
>>   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_NNA,          0),
>> +     SEC_PD(C3_AUDIO,        0),
>> +     SEC_PD(C3_SDIOA,        0),
>> +     SEC_PD(C3_EMMC,         0),
>> +     SEC_PD(C3_USB_COMB,     0),
>> +     SEC_PD(C3_SDCARD,       0),
>> +     /* ETH is for ethernet online wakeup, and should be always on */
>> +     SEC_PD(C3_ETH,          GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(C3_GE2D,         0),
>> +     SEC_PD(C3_CVE,          0),
>> +     SEC_PD(C3_GDC_WRAP,     0),
>> +     SEC_PD(C3_ISP_TOP,      0),
>> +     SEC_PD(C3_MIPI_ISP_WRAP, 0),
>>       SEC_PD(C3_VCODEC,       0),
>>   };
>>
>>
>> base-commit: 413f5c02929bb33042bbc4ee233166550a5fca70
> 
> With changed subject:
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH V2 2/6] soc: amlogic: add driver to support power parent node

[Xianwei Zhao]

Hi Neil,
    Thanks for your reply.

On 2023/8/24 16:27, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 24/08/2023 07:59, Xianwei Zhao wrote:
>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>
>> Some power domains depends on other domains, Such as Amlogic T7 SoC.
>> Add parent node to support this case.
>>
>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
>> ---
>> V1 -> V2: None
>> ---
>>   drivers/genpd/amlogic/meson-secure-pwrc.c | 26 ++++++++++++++++++++++-
>>   1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c 
>> b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> index 5ac2437ab8ad..76527f4946b4 100644
>> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
>> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> @@ -19,10 +19,12 @@
>>
>>   #define PWRC_ON             1
>>   #define PWRC_OFF    0
>> +#define PWRC_NO_PARENT  ((1UL << (sizeof(unsigned int) * 8)) - 1)
> 
> This is fishy, just use UINT_MAX
Will do, use UINT_MAX define (~0U).
> 
>>
>>   struct meson_secure_pwrc_domain {
>>       struct generic_pm_domain base;
>>       unsigned int index;
>> +     unsigned int parent;
>>       struct meson_secure_pwrc *pwrc;
>>   };
>>
>> @@ -34,6 +36,7 @@ struct meson_secure_pwrc {
>>
>>   struct meson_secure_pwrc_domain_desc {
>>       unsigned int index;
>> +     unsigned int parent;
>>       unsigned int flags;
>>       char *name;
>>       bool (*is_off)(struct meson_secure_pwrc_domain *pwrc_domain);
>> @@ -90,8 +93,19 @@ static int meson_secure_pwrc_on(struct 
>> generic_pm_domain *domain)
>>   {                                           \
>>       .name = #__name,                        \
>>       .index = PWRC_##__name##_ID,            \
>> -     .is_off = pwrc_secure_is_off,   \
>> +     .is_off = pwrc_secure_is_off,           \
>>       .flags = __flag,                        \
>> +     .parent = PWRC_NO_PARENT,               \
>> +}
>> +
>> +#define TOP_PD(__name, __flag, __parent)     \
>> +[PWRC_##__name##_ID] =                               \
>> +{                                            \
>> +     .name = #__name,                        \
>> +     .index = PWRC_##__name##_ID,            \
>> +     .is_off = pwrc_secure_is_off,           \
>> +     .flags = __flag,                        \
>> +     .parent = __parent,                     \
>>   }
>>
>>   static struct meson_secure_pwrc_domain_desc a1_pwrc_domains[] = {
>> @@ -202,6 +216,7 @@ static int meson_secure_pwrc_probe(struct 
>> platform_device *pdev)
>>
>>               dom->pwrc = pwrc;
>>               dom->index = match->domains[i].index;
>> +             dom->parent = match->domains[i].parent;
>>               dom->base.name = match->domains[i].name;
>>               dom->base.flags = match->domains[i].flags;
>>               dom->base.power_on = meson_secure_pwrc_on;
>> @@ -212,6 +227,15 @@ static int meson_secure_pwrc_probe(struct 
>> platform_device *pdev)
>>               pwrc->xlate.domains[i] = &dom->base;
>>       }
>>
>> +     for (i = 0; i < match->count; i++) {
>> +             struct meson_secure_pwrc_domain *dom = pwrc->domains;
>> +
>> +             if (!match->domains[i].name || match->domains[i].parent 
>> == PWRC_NO_PARENT)
>> +                     continue;
>> +
>> +             pm_genpd_add_subdomain(&dom[dom[i].parent].base, 
>> &dom[i].base);
>> +     }
>> +
>>       return of_genpd_add_provider_onecell(pdev->dev.of_node, 
>> &pwrc->xlate);
>>   }
>>
> 

Re: [PATCH V2 3/6] soc: amlogic: init power domain state

[Xianwei Zhao]

Hi Neil,
    Thanks for your reply.

On 2023/8/24 16:30, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 24/08/2023 07:59, Xianwei Zhao wrote:
>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>
>> If initial power domain with 'AWAY_ON' property state is off,
>> turn on the power.
>>
>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
>> ---
>> V1 -> V2: None
>> ---
>>   drivers/genpd/amlogic/meson-secure-pwrc.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c 
>> b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> index 76527f4946b4..3e7e3bd25d1f 100644
>> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
>> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> @@ -55,7 +55,7 @@ static bool pwrc_secure_is_off(struct 
>> meson_secure_pwrc_domain *pwrc_domain)
>>                         pwrc_domain->index, 0, 0, 0, 0) < 0)
>>               pr_err("failed to get power domain status\n");
>>
>> -     return is_off;
>> +     return !!is_off;
> 
> Can you explain this ? the function returns bool, so if if_off is > 0, 
> it would return true,
> so I don't see why you would need to transform is_off into 1 or 0 using !!.
> 
I will remove this modify, in next version.
>>   }
>>
>>   static int meson_secure_pwrc_off(struct generic_pm_domain *domain)
>> @@ -222,6 +222,9 @@ static int meson_secure_pwrc_probe(struct 
>> platform_device *pdev)
>>               dom->base.power_on = meson_secure_pwrc_on;
>>               dom->base.power_off = meson_secure_pwrc_off;
>>
>> +             if (match->domains[i].is_off(dom) && (dom->base.flags & 
>> GENPD_FLAG_ALWAYS_ON))
>> +                     meson_secure_pwrc_on(&dom->base);
>> +
>>               pm_genpd_init(&dom->base, NULL, 
>> match->domains[i].is_off(dom));
>>
>>               pwrc->xlate.domains[i] = &dom->base;
> 

Re: [PATCH V2 5/6] soc: amlogic: Add support for T7 power domains controller

[Xianwei Zhao]

Hi Neil,
     Thanks for your reply.

On 2023/8/24 16:30, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> On 24/08/2023 07:59, Xianwei Zhao wrote:
>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>
>> Add support for T7 power controller. T7 power control
>> registers are in secure domain, and should be accessed by SMC.
>>
>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
>> ---
>> V1 -> V2: Modify T7_NIC flag  "ALWAYS_ON"
>> ---
>>   drivers/genpd/amlogic/meson-secure-pwrc.c | 73 +++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/drivers/genpd/amlogic/meson-secure-pwrc.c 
>> b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> index 3e7e3bd25d1f..2233f6cb7e71 100644
>> --- a/drivers/genpd/amlogic/meson-secure-pwrc.c
>> +++ b/drivers/genpd/amlogic/meson-secure-pwrc.c
>> @@ -13,6 +13,7 @@
>>   #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 <dt-bindings/power/amlogic,t7-pwrc.h>
>>   #include <linux/arm-smccc.h>
>>   #include <linux/firmware/meson/meson_sm.h>
>>   #include <linux/module.h>
>> @@ -164,6 +165,69 @@ static struct meson_secure_pwrc_domain_desc 
>> s4_pwrc_domains[] = {
>>       SEC_PD(S4_AUDIO,        0),
>>   };
>>
>> +static struct meson_secure_pwrc_domain_desc t7_pwrc_domains[] = {
>> +     SEC_PD(T7_DSPA,         0),
>> +     SEC_PD(T7_DSPB,         0),
>> +     TOP_PD(T7_DOS_HCODEC,   0, PWRC_T7_NIC3_ID),
>> +     TOP_PD(T7_DOS_HEVC,     0, PWRC_T7_NIC3_ID),
>> +     TOP_PD(T7_DOS_VDEC,     0, PWRC_T7_NIC3_ID),
>> +     TOP_PD(T7_DOS_WAVE,     0, PWRC_T7_NIC3_ID),
>> +     SEC_PD(T7_VPU_HDMI,     0),
>> +     SEC_PD(T7_USB_COMB,     0),
>> +     SEC_PD(T7_PCIE,         0),
>> +     TOP_PD(T7_GE2D,         0, PWRC_T7_NIC3_ID),
>> +     /* SRAMA is used as ATF runtime memory, and should be always on */
>> +     SEC_PD(T7_SRAMA,        GENPD_FLAG_ALWAYS_ON),
>> +     /* SRAMB is used as ATF runtime memory, and should be always on */
>> +     SEC_PD(T7_SRAMB,        GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(T7_HDMIRX,       0),
>> +     SEC_PD(T7_VI_CLK1,      0),
>> +     SEC_PD(T7_VI_CLK2,      0),
>> +     /* ETH is for ethernet online wakeup, and should be always on */
>> +     SEC_PD(T7_ETH,          GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(T7_ISP,          0),
>> +     SEC_PD(T7_MIPI_ISP,     0),
>> +     TOP_PD(T7_GDC,          0, PWRC_T7_NIC3_ID),
>> +     TOP_PD(T7_DEWARP,       0, PWRC_T7_NIC3_ID),
>> +     SEC_PD(T7_SDIO_A,       0),
>> +     SEC_PD(T7_SDIO_B,       0),
>> +     SEC_PD(T7_EMMC,         0),
>> +     TOP_PD(T7_MALI_SC0,     0, PWRC_T7_NNA_TOP_ID),
>> +     TOP_PD(T7_MALI_SC1,     0, PWRC_T7_NNA_TOP_ID),
>> +     TOP_PD(T7_MALI_SC2,     0, PWRC_T7_NNA_TOP_ID),
>> +     TOP_PD(T7_MALI_SC3,     0, PWRC_T7_NNA_TOP_ID),
>> +     SEC_PD(T7_MALI_TOP,     0),
>> +     TOP_PD(T7_NNA_CORE0,    0, PWRC_T7_NNA_TOP_ID),
>> +     TOP_PD(T7_NNA_CORE1,    0, PWRC_T7_NNA_TOP_ID),
>> +     TOP_PD(T7_NNA_CORE2,    0, PWRC_T7_NNA_TOP_ID),
>> +     TOP_PD(T7_NNA_CORE3,    0, PWRC_T7_NNA_TOP_ID),
>> +     SEC_PD(T7_NNA_TOP,      0),
>> +     SEC_PD(T7_DDR0,         GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(T7_DDR1,         GENPD_FLAG_ALWAYS_ON),
>> +     /* DMC0 is for DDR PHY ana/dig and DMC, and should be always on */
>> +     SEC_PD(T7_DMC0,         GENPD_FLAG_ALWAYS_ON),
>> +     /* DMC1 is for DDR PHY ana/dig and DMC, and should be always on */
>> +     SEC_PD(T7_DMC1,         GENPD_FLAG_ALWAYS_ON),
>> +     /* NOC is related to clk bus, and should be always on */
>> +     SEC_PD(T7_NOC,          GENPD_FLAG_ALWAYS_ON),
>> +     /* NIC is for the Arm NIC-400 interconnect, and should be always 
>> on */
>> +     SEC_PD(T7_NIC2,         GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(T7_NIC3,         0),
>> +     /* CPU accesses the interleave data to the ddr need cci, and 
>> should be always on */
>> +     SEC_PD(T7_CCI,          GENPD_FLAG_ALWAYS_ON),
>> +     SEC_PD(T7_MIPI_DSI0,    0),
>> +     SEC_PD(T7_SPICC0,       0),
>> +     SEC_PD(T7_SPICC1,       0),
>> +     SEC_PD(T7_SPICC2,       0),
>> +     SEC_PD(T7_SPICC3,       0),
>> +     SEC_PD(T7_SPICC4,       0),
>> +     SEC_PD(T7_SPICC5,       0),
>> +     SEC_PD(T7_EDP0,         0),
>> +     SEC_PD(T7_EDP1,         0),
>> +     SEC_PD(T7_MIPI_DSI1,    0),
>> +     SEC_PD(T7_AUDIO,        0),
>> +};
>> +
>>   static int meson_secure_pwrc_probe(struct platform_device *pdev)
>>   {
>>       int i;
>> @@ -257,6 +321,11 @@ static struct meson_secure_pwrc_domain_data 
>> meson_secure_s4_pwrc_data = {
>>       .count = ARRAY_SIZE(s4_pwrc_domains),
>>   };
>>
>> +static struct meson_secure_pwrc_domain_data 
>> amlogic_secure_t7_pwrc_data = {
>> +     .domains = t7_pwrc_domains,
>> +     .count = ARRAY_SIZE(t7_pwrc_domains),
>> +};
>> +
>>   static const struct of_device_id meson_secure_pwrc_match_table[] = {
>>       {
>>               .compatible = "amlogic,meson-a1-pwrc",
>> @@ -270,6 +339,10 @@ static const struct of_device_id 
>> meson_secure_pwrc_match_table[] = {
>>               .compatible = "amlogic,meson-s4-pwrc",
>>               .data = &meson_secure_s4_pwrc_data,
>>       },
>> +     {
>> +             .compatible = "amlogic,t7-pwrc",
>> +             .data = &amlogic_secure_t7_pwrc_data,
>> +     },
>>       { /* sentinel */ }
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_secure_pwrc_match_table);
> 
> With updated subject:
Will do.
> 
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH V2 6/6] arm64: dts: add support for T7 power domain controller

[Xianwei Zhao]

Hi Neil,
     Thanks for your reply.

On 2023/8/24 16:32, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> On 24/08/2023 07:59, Xianwei Zhao wrote:
>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>
>> Enable power domain controller for Amlogic T7 SoC
>>
>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
>> ---
>> V1 -> V2: None
>> ---
>>   arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi 
>> b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
>> index 1423d4a79156..23cdad1b425b 100644
>> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
>> @@ -4,6 +4,7 @@
>>    */
>>
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/power/amlogic,t7-pwrc.h>
>>
>>   / {
>>       interrupt-parent = <&gic>;
>> @@ -118,6 +119,11 @@ psci {
>>
>>       sm: secure-monitor {
>>               compatible = "amlogic,meson-gxbb-sm";
>> +
>> +             pwrc: power-controller {
>> +                     compatible = "amlogic,t7-pwrc";
>> +                     #power-domain-cells = <1>;
>> +             };
>>       };
>>
>>       soc {
> 
> Please change the subject to:
> arm64: dts: amlogic: t7: add power domain controller node
> 
Will do.
> With that:
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH V2 4/6] dt-bindings: power: add Amlogic T7 power domains

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On Thu, Aug 24, 2023 at 01:59:28PM +0800, Xianwei Zhao wrote:
> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
> 
> Add devicetree binding document and related header file for
> Amlogic T7 secure power domains.
> 
> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V2 6/6] arm64: dts: add support for T7 power domain controller

[Lucas Tanure]

On 24-08-2023 09:51, Xianwei Zhao wrote:
> Hi Neil,
>     Thanks for your reply.
>
> On 2023/8/24 16:32, Neil Armstrong wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On 24/08/2023 07:59, Xianwei Zhao wrote:
>>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>>
>>> Enable power domain controller for Amlogic T7 SoC
>>>
>>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
>>> ---
>>> V1 -> V2: None
>>> ---
>>>   arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi 
>>> b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
>>> index 1423d4a79156..23cdad1b425b 100644
>>> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
>>> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
>>> @@ -4,6 +4,7 @@
>>>    */
>>>
>>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <dt-bindings/power/amlogic,t7-pwrc.h>
>>>
>>>   / {
>>>       interrupt-parent = <&gic>;
>>> @@ -118,6 +119,11 @@ psci {
>>>
>>>       sm: secure-monitor {
>>>               compatible = "amlogic,meson-gxbb-sm";
>>> +
>>> +             pwrc: power-controller {
>>> +                     compatible = "amlogic,t7-pwrc";
>>> +                     #power-domain-cells = <1>;
>>> +             };
>>>       };
>>>
>>>       soc {
>>
>> Please change the subject to:
>> arm64: dts: amlogic: t7: add power domain controller node
>>
> Will do.
>> With that:
>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Tested with Vim4 and all works fine.

Tested-by: Lucas Tanure <tanure@linux.com>

Re: [PATCH V2 4/6] dt-bindings: power: add Amlogic T7 power domains

[Lucas Tanure]

On 24-08-2023 16:07, Conor Dooley wrote:
> On Thu, Aug 24, 2023 at 01:59:28PM +0800, Xianwei Zhao wrote:
>> From: "xianwei.zhao" <xianwei.zhao@amlogic.com>
>>
>> Add devicetree binding document and related header file for
>> Amlogic T7 secure power domains.
>>
>> Signed-off-by: xianwei.zhao <xianwei.zhao@amlogic.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> Thanks,
> Conor.
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
Tested with Vim4 and all works fine.

Tested-by: Lucas Tanure <tanure@linux.com>