[U-Boot] [PATCH 1/3] arm/PSCI: Removed unused code

[U-Boot] [PATCH 1/3] arm/PSCI: Removed unused code

[Zhiqiang Hou]

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Identify the PSCI node only by its name, so removed the code finding
it by compatible string.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
 arch/arm/lib/psci-dt.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c
index 8dc31d4..bcd92e7 100644
--- a/arch/arm/lib/psci-dt.c
+++ b/arch/arm/lib/psci-dt.c
@@ -51,27 +51,10 @@ int fdt_psci(void *fdt)
 		fdt_setprop_string(fdt, tmp, "enable-method", "psci");
 	}
 
-	/*
-	 * The PSCI node might be called "/psci" or might be called something
-	 * else but contain either of the compatible strings
-	 * "arm,psci"/"arm,psci-0.2"
-	 */
 	nodeoff = fdt_path_offset(fdt, "/psci");
 	if (nodeoff >= 0)
 		goto init_psci_node;
 
-	nodeoff = fdt_node_offset_by_compatible(fdt, -1, "arm,psci");
-	if (nodeoff >= 0)
-		goto init_psci_node;
-
-	nodeoff = fdt_node_offset_by_compatible(fdt, -1, "arm,psci-0.2");
-	if (nodeoff >= 0)
-		goto init_psci_node;
-
-	nodeoff = fdt_node_offset_by_compatible(fdt, -1, "arm,psci-1.0");
-	if (nodeoff >= 0)
-		goto init_psci_node;
-
 	nodeoff = fdt_path_offset(fdt, "/");
 	if (nodeoff < 0)
 		return nodeoff;
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[Zhiqiang Hou]

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Appended the compatible strings of old version PSCI to the latest version
supported. And there are some psci functions' property must be added to DT
only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.

Note:
The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware Framework.

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
 arch/arm/include/asm/psci.h |  3 +++
 arch/arm/lib/psci-dt.c      | 61 ++++++++++++++++++++++++++-------------------
 2 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
index 8aefaa7..5b8ce4d 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -18,6 +18,9 @@
 #ifndef __ARM_PSCI_H__
 #define __ARM_PSCI_H__
 
+#define ARM_PSCI_VER_1_0		(0x00010000)
+#define ARM_PSCI_VER_0_2		(0x00000002)
+
 /* PSCI 0.1 interface */
 #define ARM_PSCI_FN_BASE		0x95c1ba5e
 #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c
index bcd92e7..af49c24 100644
--- a/arch/arm/lib/psci-dt.c
+++ b/arch/arm/lib/psci-dt.c
@@ -19,7 +19,6 @@ int fdt_psci(void *fdt)
 #if defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
 	int nodeoff;
 	unsigned int psci_ver = 0;
-	char *psci_compt;
 	int tmp;
 
 	nodeoff = fdt_path_offset(fdt, "/cpus");
@@ -68,39 +67,49 @@ init_psci_node:
 	psci_ver = sec_firmware_support_psci_version();
 #endif
 	switch (psci_ver) {
-	case 0x00010000:
-		psci_compt = "arm,psci-1.0";
-		break;
-	case 0x00000002:
-		psci_compt = "arm,psci-0.2";
-		break;
+	case ARM_PSCI_VER_1_0:
+		tmp = fdt_setprop_string(fdt, nodeoff,
+				"compatible", "arm,psci-1.0");
+		if (tmp)
+			return tmp;
+	case ARM_PSCI_VER_0_2:
+		tmp = fdt_appendprop_string(fdt, nodeoff,
+				"compatible", "arm,psci-0.2");
+		if (tmp)
+			return tmp;
 	default:
-		psci_compt = "arm,psci";
+	/*
+	 * The Secure firmware framework isn't able to support PSCI version 0.1.
+	 */
+#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
+		tmp = fdt_appendprop_string(fdt, nodeoff,
+				"compatible", "arm,psci");
+		if (tmp)
+			return tmp;
+		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
+				ARM_PSCI_FN_CPU_SUSPEND);
+		if (tmp)
+			return tmp;
+		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
+				ARM_PSCI_FN_CPU_OFF);
+		if (tmp)
+			return tmp;
+		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
+				ARM_PSCI_FN_CPU_ON);
+		if (tmp)
+			return tmp;
+		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
+				ARM_PSCI_FN_MIGRATE);
+		if (tmp)
+			return tmp;
+#endif
 		break;
 	}
 
-	tmp = fdt_setprop_string(fdt, nodeoff, "compatible", psci_compt);
-	if (tmp)
-		return tmp;
 	tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc");
 	if (tmp)
 		return tmp;
 
-#ifdef CONFIG_ARMV7_PSCI
-	tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
-				ARM_PSCI_FN_CPU_SUSPEND);
-	if (tmp)
-		return tmp;
-	tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF);
-	if (tmp)
-		return tmp;
-	tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON);
-	if (tmp)
-		return tmp;
-	tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE);
-	if (tmp)
-		return tmp;
-#endif
 #endif
 	return 0;
 }
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH 3/3] arm/PSCI: Added support for creating ARMv7 PSCI version 1.0 DT node

[Zhiqiang Hou]

From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
---
 arch/arm/lib/psci-dt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c
index af49c24..baf6d70 100644
--- a/arch/arm/lib/psci-dt.c
+++ b/arch/arm/lib/psci-dt.c
@@ -65,6 +65,8 @@ int fdt_psci(void *fdt)
 init_psci_node:
 #ifdef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
 	psci_ver = sec_firmware_support_psci_version();
+#elif defined(CONFIG_ARMV7_PSCI_1_0)
+	psci_ver = ARM_PSCI_VER_1_0;
 #endif
 	switch (psci_ver) {
 	case ARM_PSCI_VER_1_0:
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[york sun]

On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Appended the compatible strings of old version PSCI to the latest version
> supported. And there are some psci functions' property must be added to DT
> only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
>
> Note:
> The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware Framework.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---

You missed version number and change log.

>  arch/arm/include/asm/psci.h |  3 +++
>  arch/arm/lib/psci-dt.c      | 61 ++++++++++++++++++++++++++-------------------
>  2 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> index 8aefaa7..5b8ce4d 100644
> --- a/arch/arm/include/asm/psci.h
> +++ b/arch/arm/include/asm/psci.h
> @@ -18,6 +18,9 @@
>  #ifndef __ARM_PSCI_H__
>  #define __ARM_PSCI_H__
>
> +#define ARM_PSCI_VER_1_0		(0x00010000)
> +#define ARM_PSCI_VER_0_2		(0x00000002)
> +
>  /* PSCI 0.1 interface */
>  #define ARM_PSCI_FN_BASE		0x95c1ba5e
>  #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
> diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c
> index bcd92e7..af49c24 100644
> --- a/arch/arm/lib/psci-dt.c
> +++ b/arch/arm/lib/psci-dt.c
> @@ -19,7 +19,6 @@ int fdt_psci(void *fdt)
>  #if defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
>  	int nodeoff;
>  	unsigned int psci_ver = 0;
> -	char *psci_compt;
>  	int tmp;
>
>  	nodeoff = fdt_path_offset(fdt, "/cpus");
> @@ -68,39 +67,49 @@ init_psci_node:
>  	psci_ver = sec_firmware_support_psci_version();
>  #endif
>  	switch (psci_ver) {
> -	case 0x00010000:
> -		psci_compt = "arm,psci-1.0";
> -		break;
> -	case 0x00000002:
> -		psci_compt = "arm,psci-0.2";
> -		break;
> +	case ARM_PSCI_VER_1_0:
> +		tmp = fdt_setprop_string(fdt, nodeoff,
> +				"compatible", "arm,psci-1.0");
> +		if (tmp)
> +			return tmp;

Add a comment "fall through".

> +	case ARM_PSCI_VER_0_2:
> +		tmp = fdt_appendprop_string(fdt, nodeoff,
> +				"compatible", "arm,psci-0.2");
> +		if (tmp)
> +			return tmp;

Add a comment "fall through".

>  	default:
> -		psci_compt = "arm,psci";
> +	/*
> +	 * The Secure firmware framework isn't able to support PSCI version 0.1.
> +	 */
> +#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
> +		tmp = fdt_appendprop_string(fdt, nodeoff,
> +				"compatible", "arm,psci");
> +		if (tmp)
> +			return tmp;
> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
> +				ARM_PSCI_FN_CPU_SUSPEND);
> +		if (tmp)
> +			return tmp;
> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
> +				ARM_PSCI_FN_CPU_OFF);
> +		if (tmp)
> +			return tmp;
> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
> +				ARM_PSCI_FN_CPU_ON);
> +		if (tmp)
> +			return tmp;
> +		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
> +				ARM_PSCI_FN_MIGRATE);
> +		if (tmp)
> +			return tmp;

This may not be a real concern, but I am curious what would happen if 
one of the above fdt_setprop_u32 fails? Would it be better to set 
"arm,psci" last?

York

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[Zhiqiang Hou]

Hi York,

Thanks for your comments!

> -----Original Message-----
> From: york sun
> Sent: 2016?7?29? 23:37
> To: Zhiqiang Hou <zhiqiang.hou@nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org; Hongbo
> Zhang <hongbo.zhang@nxp.com>; b.galvani at gmail.com
> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue
> 
> On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > Appended the compatible strings of old version PSCI to the latest
> > version supported. And there are some psci functions' property must be
> > added to DT only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
> >
> > Note:
> > The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware Framework.
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > ---
> 
> You missed version number and change log.

Should the previous Secure Firmware Framework patchset need to be resubmitted? And the version number follow the that patchset?

> >  arch/arm/include/asm/psci.h |  3 +++
> >  arch/arm/lib/psci-dt.c      | 61
> ++++++++++++++++++++++++++-------------------
> >  2 files changed, 38 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> > index 8aefaa7..5b8ce4d 100644
> > --- a/arch/arm/include/asm/psci.h
> > +++ b/arch/arm/include/asm/psci.h
> > @@ -18,6 +18,9 @@
> >  #ifndef __ARM_PSCI_H__
> >  #define __ARM_PSCI_H__
> >
> > +#define ARM_PSCI_VER_1_0		(0x00010000)
> > +#define ARM_PSCI_VER_0_2		(0x00000002)
> > +
> >  /* PSCI 0.1 interface */
> >  #define ARM_PSCI_FN_BASE		0x95c1ba5e
> >  #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
> > diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c index
> > bcd92e7..af49c24 100644
> > --- a/arch/arm/lib/psci-dt.c
> > +++ b/arch/arm/lib/psci-dt.c
> > @@ -19,7 +19,6 @@ int fdt_psci(void *fdt)  #if
> > defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
> >  	int nodeoff;
> >  	unsigned int psci_ver = 0;
> > -	char *psci_compt;
> >  	int tmp;
> >
> >  	nodeoff = fdt_path_offset(fdt, "/cpus"); @@ -68,39 +67,49 @@
> > init_psci_node:
> >  	psci_ver = sec_firmware_support_psci_version();
> >  #endif
> >  	switch (psci_ver) {
> > -	case 0x00010000:
> > -		psci_compt = "arm,psci-1.0";
> > -		break;
> > -	case 0x00000002:
> > -		psci_compt = "arm,psci-0.2";
> > -		break;
> > +	case ARM_PSCI_VER_1_0:
> > +		tmp = fdt_setprop_string(fdt, nodeoff,
> > +				"compatible", "arm,psci-1.0");
> > +		if (tmp)
> > +			return tmp;
> 
> Add a comment "fall through".
> 

Yes, will add the comment.
[] 
> > +	case ARM_PSCI_VER_0_2:
> > +		tmp = fdt_appendprop_string(fdt, nodeoff,
> > +				"compatible", "arm,psci-0.2");
> > +		if (tmp)
> > +			return tmp;
> 
> Add a comment "fall through".
> 
> >  	default:
> > -		psci_compt = "arm,psci";
> > +	/*
> > +	 * The Secure firmware framework isn't able to support PSCI version 0.1.
> > +	 */
> > +#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
> > +		tmp = fdt_appendprop_string(fdt, nodeoff,
> > +				"compatible", "arm,psci");
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
> > +				ARM_PSCI_FN_CPU_SUSPEND);
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
> > +				ARM_PSCI_FN_CPU_OFF);
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
> > +				ARM_PSCI_FN_CPU_ON);
> > +		if (tmp)
> > +			return tmp;
> > +		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
> > +				ARM_PSCI_FN_MIGRATE);
> > +		if (tmp)
> > +			return tmp;
> 
> This may not be a real concern, but I am curious what would happen if one of
> the above fdt_setprop_u32 fails? Would it be better to set "arm,psci" last?

Trend to add the error check to prevent the unexpected issue. Is there benefit to set the "arm,psci" last?

Thanks,
Zhiqiang

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[york sun]

On 07/31/2016 08:20 PM, Zhiqiang Hou wrote:
> Hi York,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: york sun
>> Sent: 2016?7?29? 23:37
>> To: Zhiqiang Hou <zhiqiang.hou@nxp.com>; u-boot at lists.denx.de;
>> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org; Hongbo
>> Zhang <hongbo.zhang@nxp.com>; b.galvani at gmail.com
>> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue
>>
>> On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
>>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>>>
>>> Appended the compatible strings of old version PSCI to the latest
>>> version supported. And there are some psci functions' property must be
>>> added to DT only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
>>>
>>> Note:
>>> The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware Framework.
>>>
>>> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>>> ---
>>
>> You missed version number and change log.
>
> Should the previous Secure Firmware Framework patchset need to be resubmitted? And the version number follow the that patchset?

Zhiqiang,

When you send a new version, reviewers are expecting updated version 
number and a change log. This helps us tracking what has been changed. 
All patches in the same set should be updated. If a patch has not 
change, a simple "no change" helps. If any dependency has 
changed/updated, please update the note as well.

>
>>>  arch/arm/include/asm/psci.h |  3 +++
>>>  arch/arm/lib/psci-dt.c      | 61
>> ++++++++++++++++++++++++++-------------------
>>>  2 files changed, 38 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
>>> index 8aefaa7..5b8ce4d 100644
>>> --- a/arch/arm/include/asm/psci.h
>>> +++ b/arch/arm/include/asm/psci.h
>>> @@ -18,6 +18,9 @@
>>>  #ifndef __ARM_PSCI_H__
>>>  #define __ARM_PSCI_H__
>>>
>>> +#define ARM_PSCI_VER_1_0		(0x00010000)
>>> +#define ARM_PSCI_VER_0_2		(0x00000002)
>>> +
>>>  /* PSCI 0.1 interface */
>>>  #define ARM_PSCI_FN_BASE		0x95c1ba5e
>>>  #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
>>> diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c index
>>> bcd92e7..af49c24 100644
>>> --- a/arch/arm/lib/psci-dt.c
>>> +++ b/arch/arm/lib/psci-dt.c
>>> @@ -19,7 +19,6 @@ int fdt_psci(void *fdt)  #if
>>> defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
>>>  	int nodeoff;
>>>  	unsigned int psci_ver = 0;
>>> -	char *psci_compt;
>>>  	int tmp;
>>>
>>>  	nodeoff = fdt_path_offset(fdt, "/cpus"); @@ -68,39 +67,49 @@
>>> init_psci_node:
>>>  	psci_ver = sec_firmware_support_psci_version();
>>>  #endif
>>>  	switch (psci_ver) {
>>> -	case 0x00010000:
>>> -		psci_compt = "arm,psci-1.0";
>>> -		break;
>>> -	case 0x00000002:
>>> -		psci_compt = "arm,psci-0.2";
>>> -		break;
>>> +	case ARM_PSCI_VER_1_0:
>>> +		tmp = fdt_setprop_string(fdt, nodeoff,
>>> +				"compatible", "arm,psci-1.0");
>>> +		if (tmp)
>>> +			return tmp;
>>
>> Add a comment "fall through".
>>
>
> Yes, will add the comment.
> []
>>> +	case ARM_PSCI_VER_0_2:
>>> +		tmp = fdt_appendprop_string(fdt, nodeoff,
>>> +				"compatible", "arm,psci-0.2");
>>> +		if (tmp)
>>> +			return tmp;
>>
>> Add a comment "fall through".
>>
>>>  	default:
>>> -		psci_compt = "arm,psci";
>>> +	/*
>>> +	 * The Secure firmware framework isn't able to support PSCI version 0.1.
>>> +	 */
>>> +#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
>>> +		tmp = fdt_appendprop_string(fdt, nodeoff,
>>> +				"compatible", "arm,psci");
>>> +		if (tmp)
>>> +			return tmp;
>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
>>> +				ARM_PSCI_FN_CPU_SUSPEND);
>>> +		if (tmp)
>>> +			return tmp;
>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
>>> +				ARM_PSCI_FN_CPU_OFF);
>>> +		if (tmp)
>>> +			return tmp;
>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
>>> +				ARM_PSCI_FN_CPU_ON);
>>> +		if (tmp)
>>> +			return tmp;
>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
>>> +				ARM_PSCI_FN_MIGRATE);
>>> +		if (tmp)
>>> +			return tmp;
>>
>> This may not be a real concern, but I am curious what would happen if one of
>> the above fdt_setprop_u32 fails? Would it be better to set "arm,psci" last?
>
> Trend to add the error check to prevent the unexpected issue. Is there benefit to set the "arm,psci" last?

I just guessed without "arm,psci" node, kernel wouldn't look for 
"cpu_on", "cpu_off" node. I could be wrong.

York

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[Zhiqiang Hou]

Hi York,

Thanks for your comments!

> -----Original Message-----
> From: york sun
> Sent: 2016?8?2? 0:10
> To: Zhiqiang Hou <zhiqiang.hou@nxp.com>; u-boot at lists.denx.de;
> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org; Hongbo
> Zhang <hongbo.zhang@nxp.com>; b.galvani at gmail.com
> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue
> 
> On 07/31/2016 08:20 PM, Zhiqiang Hou wrote:
> > Hi York,
> >
> > Thanks for your comments!
> >
> >> -----Original Message-----
> >> From: york sun
> >> Sent: 2016?7?29? 23:37
> >> To: Zhiqiang Hou <zhiqiang.hou@nxp.com>; u-boot at lists.denx.de;
> >> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org;
> Hongbo
> >> Zhang <hongbo.zhang@nxp.com>; b.galvani at gmail.com
> >> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity
> >> issue
> >>
> >> On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> >>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >>>
> >>> Appended the compatible strings of old version PSCI to the latest
> >>> version supported. And there are some psci functions' property must
> >>> be added to DT only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
> >>>
> >>> Note:
> >>> The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware
> Framework.
> >>>
> >>> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >>> ---
> >>
> >> You missed version number and change log.
> >
> > Should the previous Secure Firmware Framework patchset need to be
> resubmitted? And the version number follow the that patchset?
> 
> Zhiqiang,
> 
> When you send a new version, reviewers are expecting updated version
> number and a change log. This helps us tracking what has been changed.
> All patches in the same set should be updated. If a patch has not change, a
> simple "no change" helps. If any dependency has changed/updated, please
> update the note as well.
> 

I know what do you mean, this 3 patch was sent to u-boot at lists.denx.de the first time, please forget the versions for internal review. So I am confusing if you mean this 3 patches should be merged into the patchset '[PATCHv7 1/6] armv8: fsl-layerscape: add i/d-cache enable function to enable_caches' and then resubmit all the patches? I don't know if that is legal, because the state of that patchset has been applied to u-boot-fsl-qoriq master and awaiting upstream. 

> >
> >>>  arch/arm/include/asm/psci.h |  3 +++
> >>>  arch/arm/lib/psci-dt.c      | 61
> >> ++++++++++++++++++++++++++-------------------
> >>>  2 files changed, 38 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/psci.h
> >>> b/arch/arm/include/asm/psci.h index 8aefaa7..5b8ce4d 100644
> >>> --- a/arch/arm/include/asm/psci.h
> >>> +++ b/arch/arm/include/asm/psci.h
> >>> @@ -18,6 +18,9 @@
> >>>  #ifndef __ARM_PSCI_H__
> >>>  #define __ARM_PSCI_H__
> >>>
> >>> +#define ARM_PSCI_VER_1_0		(0x00010000)
> >>> +#define ARM_PSCI_VER_0_2		(0x00000002)
> >>> +
> >>>  /* PSCI 0.1 interface */
> >>>  #define ARM_PSCI_FN_BASE		0x95c1ba5e
> >>>  #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
> >>> diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c index
> >>> bcd92e7..af49c24 100644
> >>> --- a/arch/arm/lib/psci-dt.c
> >>> +++ b/arch/arm/lib/psci-dt.c
> >>> @@ -19,7 +19,6 @@ int fdt_psci(void *fdt)  #if
> >>> defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
> >>>  	int nodeoff;
> >>>  	unsigned int psci_ver = 0;
> >>> -	char *psci_compt;
> >>>  	int tmp;
> >>>
> >>>  	nodeoff = fdt_path_offset(fdt, "/cpus"); @@ -68,39 +67,49 @@
> >>> init_psci_node:
> >>>  	psci_ver = sec_firmware_support_psci_version();
> >>>  #endif
> >>>  	switch (psci_ver) {
> >>> -	case 0x00010000:
> >>> -		psci_compt = "arm,psci-1.0";
> >>> -		break;
> >>> -	case 0x00000002:
> >>> -		psci_compt = "arm,psci-0.2";
> >>> -		break;
> >>> +	case ARM_PSCI_VER_1_0:
> >>> +		tmp = fdt_setprop_string(fdt, nodeoff,
> >>> +				"compatible", "arm,psci-1.0");
> >>> +		if (tmp)
> >>> +			return tmp;
> >>
> >> Add a comment "fall through".
> >>
> >
> > Yes, will add the comment.
> > []
> >>> +	case ARM_PSCI_VER_0_2:
> >>> +		tmp = fdt_appendprop_string(fdt, nodeoff,
> >>> +				"compatible", "arm,psci-0.2");
> >>> +		if (tmp)
> >>> +			return tmp;
> >>
> >> Add a comment "fall through".
> >>
> >>>  	default:
> >>> -		psci_compt = "arm,psci";
> >>> +	/*
> >>> +	 * The Secure firmware framework isn't able to support PSCI version
> 0.1.
> >>> +	 */
> >>> +#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
> >>> +		tmp = fdt_appendprop_string(fdt, nodeoff,
> >>> +				"compatible", "arm,psci");
> >>> +		if (tmp)
> >>> +			return tmp;
> >>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
> >>> +				ARM_PSCI_FN_CPU_SUSPEND);
> >>> +		if (tmp)
> >>> +			return tmp;
> >>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
> >>> +				ARM_PSCI_FN_CPU_OFF);
> >>> +		if (tmp)
> >>> +			return tmp;
> >>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
> >>> +				ARM_PSCI_FN_CPU_ON);
> >>> +		if (tmp)
> >>> +			return tmp;
> >>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
> >>> +				ARM_PSCI_FN_MIGRATE);
> >>> +		if (tmp)
> >>> +			return tmp;
> >>
> >> This may not be a real concern, but I am curious what would happen if
> >> one of the above fdt_setprop_u32 fails? Would it be better to set
> "arm,psci" last?
> >
> > Trend to add the error check to prevent the unexpected issue. Is there
> benefit to set the "arm,psci" last?
> 
> I just guessed without "arm,psci" node, kernel wouldn't look for "cpu_on",
> "cpu_off" node. I could be wrong.

As it is a collective, so we'd better keep the sequence as previous.

Thanks,
Zhiqiang

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[york sun]

On 08/01/2016 08:29 PM, Zhiqiang Hou wrote:
> Hi York,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: york sun
>> Sent: 2016?8?2? 0:10
>> To: Zhiqiang Hou <zhiqiang.hou@nxp.com>; u-boot at lists.denx.de;
>> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org; Hongbo
>> Zhang <hongbo.zhang@nxp.com>; b.galvani at gmail.com
>> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue
>>
>> On 07/31/2016 08:20 PM, Zhiqiang Hou wrote:
>>> Hi York,
>>>
>>> Thanks for your comments!
>>>
>>>> -----Original Message-----
>>>> From: york sun
>>>> Sent: 2016?7?29? 23:37
>>>> To: Zhiqiang Hou <zhiqiang.hou@nxp.com>; u-boot at lists.denx.de;
>>>> albert.u.boot at aribaud.net; hdegoede at redhat.com; wens at csie.org;
>> Hongbo
>>>> Zhang <hongbo.zhang@nxp.com>; b.galvani at gmail.com
>>>> Subject: Re: [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity
>>>> issue
>>>>
>>>> On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
>>>>> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>>>>>
>>>>> Appended the compatible strings of old version PSCI to the latest
>>>>> version supported. And there are some psci functions' property must
>>>>> be added to DT only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
>>>>>
>>>>> Note:
>>>>> The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware
>> Framework.
>>>>>
>>>>> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>>>>> ---
>>>>
>>>> You missed version number and change log.
>>>
>>> Should the previous Secure Firmware Framework patchset need to be
>> resubmitted? And the version number follow the that patchset?
>>
>> Zhiqiang,
>>
>> When you send a new version, reviewers are expecting updated version
>> number and a change log. This helps us tracking what has been changed.
>> All patches in the same set should be updated. If a patch has not change, a
>> simple "no change" helps. If any dependency has changed/updated, please
>> update the note as well.
>>
>
> I know what do you mean, this 3 patch was sent to u-boot at lists.denx.de the first time, please forget the versions for internal review. So I am confusing if you mean this 3 patches should be merged into the patchset '[PATCHv7 1/6] armv8: fsl-layerscape: add i/d-cache enable function to enable_caches' and then resubmit all the patches? I don't know if that is legal, because the state of that patchset has been applied to u-boot-fsl-qoriq master and awaiting upstream.
>

I must got confused with your internal version numbers. Let it go.
Regarding your other patch set start with '[PATCHv7 1/6] armv8: 
fsl-layerscape: add i/d-cache enable function to enable_caches', they 
are already merged. No need to update.


>>>
>>>>>  arch/arm/include/asm/psci.h |  3 +++
>>>>>  arch/arm/lib/psci-dt.c      | 61
>>>> ++++++++++++++++++++++++++-------------------
>>>>>  2 files changed, 38 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/psci.h
>>>>> b/arch/arm/include/asm/psci.h index 8aefaa7..5b8ce4d 100644
>>>>> --- a/arch/arm/include/asm/psci.h
>>>>> +++ b/arch/arm/include/asm/psci.h
>>>>> @@ -18,6 +18,9 @@
>>>>>  #ifndef __ARM_PSCI_H__
>>>>>  #define __ARM_PSCI_H__
>>>>>
>>>>> +#define ARM_PSCI_VER_1_0		(0x00010000)
>>>>> +#define ARM_PSCI_VER_0_2		(0x00000002)
>>>>> +
>>>>>  /* PSCI 0.1 interface */
>>>>>  #define ARM_PSCI_FN_BASE		0x95c1ba5e
>>>>>  #define ARM_PSCI_FN(n)			(ARM_PSCI_FN_BASE + (n))
>>>>> diff --git a/arch/arm/lib/psci-dt.c b/arch/arm/lib/psci-dt.c index
>>>>> bcd92e7..af49c24 100644
>>>>> --- a/arch/arm/lib/psci-dt.c
>>>>> +++ b/arch/arm/lib/psci-dt.c
>>>>> @@ -19,7 +19,6 @@ int fdt_psci(void *fdt)  #if
>>>>> defined(CONFIG_ARMV8_PSCI) || defined(CONFIG_ARMV7_PSCI)
>>>>>  	int nodeoff;
>>>>>  	unsigned int psci_ver = 0;
>>>>> -	char *psci_compt;
>>>>>  	int tmp;
>>>>>
>>>>>  	nodeoff = fdt_path_offset(fdt, "/cpus"); @@ -68,39 +67,49 @@
>>>>> init_psci_node:
>>>>>  	psci_ver = sec_firmware_support_psci_version();
>>>>>  #endif
>>>>>  	switch (psci_ver) {
>>>>> -	case 0x00010000:
>>>>> -		psci_compt = "arm,psci-1.0";
>>>>> -		break;
>>>>> -	case 0x00000002:
>>>>> -		psci_compt = "arm,psci-0.2";
>>>>> -		break;
>>>>> +	case ARM_PSCI_VER_1_0:
>>>>> +		tmp = fdt_setprop_string(fdt, nodeoff,
>>>>> +				"compatible", "arm,psci-1.0");
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>
>>>> Add a comment "fall through".
>>>>
>>>
>>> Yes, will add the comment.
>>> []
>>>>> +	case ARM_PSCI_VER_0_2:
>>>>> +		tmp = fdt_appendprop_string(fdt, nodeoff,
>>>>> +				"compatible", "arm,psci-0.2");
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>
>>>> Add a comment "fall through".
>>>>
>>>>>  	default:
>>>>> -		psci_compt = "arm,psci";
>>>>> +	/*
>>>>> +	 * The Secure firmware framework isn't able to support PSCI version
>> 0.1.
>>>>> +	 */
>>>>> +#ifndef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
>>>>> +		tmp = fdt_appendprop_string(fdt, nodeoff,
>>>>> +				"compatible", "arm,psci");
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
>>>>> +				ARM_PSCI_FN_CPU_SUSPEND);
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off",
>>>>> +				ARM_PSCI_FN_CPU_OFF);
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on",
>>>>> +				ARM_PSCI_FN_CPU_ON);
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>> +		tmp = fdt_setprop_u32(fdt, nodeoff, "migrate",
>>>>> +				ARM_PSCI_FN_MIGRATE);
>>>>> +		if (tmp)
>>>>> +			return tmp;
>>>>
>>>> This may not be a real concern, but I am curious what would happen if
>>>> one of the above fdt_setprop_u32 fails? Would it be better to set
>> "arm,psci" last?
>>>
>>> Trend to add the error check to prevent the unexpected issue. Is there
>> benefit to set the "arm,psci" last?
>>
>> I just guessed without "arm,psci" node, kernel wouldn't look for "cpu_on",
>> "cpu_off" node. I could be wrong.
>
> As it is a collective, so we'd better keep the sequence as previous.
>

OK.

York

[U-Boot] [PATCH 1/3] arm/PSCI: Removed unused code

[york sun]

On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Identify the PSCI node only by its name, so removed the code finding
> it by compatible string.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
>  arch/arm/lib/psci-dt.c | 17 -----------------
>  1 file changed, 17 deletions(-)

Applied to fsl-qoriq master, awaiting upstream.
Thanks.

York

[U-Boot] [PATCH 2/3] arm/PSCI: Fixed the backward compatiblity issue

[york sun]

On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Appended the compatible strings of old version PSCI to the latest version
> supported. And there are some psci functions' property must be added to DT
> only for psci version 0.1, such as 'cpu_on' 'cpu_off' etc.
>
> Note:
> The PSCI version 0.1 isn't supported by ARMv8 Secure Firmware Framework.
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
>  arch/arm/include/asm/psci.h |  3 +++
>  arch/arm/lib/psci-dt.c      | 61 ++++++++++++++++++++++++++-------------------
>  2 files changed, 38 insertions(+), 26 deletions(-)

Applied to fsl-qoriq master, awaiting upstream.
Thanks.

York

[U-Boot] [PATCH 3/3] arm/PSCI: Added support for creating ARMv7 PSCI version 1.0 DT node

[york sun]

On 07/29/2016 03:37 AM, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
>  arch/arm/lib/psci-dt.c | 2 ++
>  1 file changed, 2 insertions(+)

Applied to fsl-qoriq master, awaiting upstream.
Thanks.

York