[PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[Sudeep Holla]

Currently ARM CPUs DT bindings allows different enable-method value for
PSCI based systems. On ARM 64-bit this property is required and must be
"psci" while on ARM 32-bit systems this property is optional and must
be "arm,psci" if present.

However, "arm,psci" has always been the compatible string for the PSCI
node, and was never intended to be the enable-method. So this is a bug
in the binding and not a deliberate attempt at specifying 32-bit
differently.

This is problematic if 32-bit OS is run on 64-bit system which has
"psci" as enable-method rather than the expected "arm,psci".

So let's unify the value into "psci" and remove support for "arm,psci"
before it finds any users.

Reported-by: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 1 -
 drivers/firmware/psci.c                        | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

v2->v1:
	- Added Mark and Lorenzo's ACKs
	- Updated commit message to specify that it also conflicts with
	  PSCI compatible(from Mark's reply on the thread)

Hi ARM-SoC guys,

Can you please pick up this single fix for PSCI binding/driver for v4.6 ?

Regards,
Sudeep

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index ccc62f145306..3f0cbbb8395f 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -192,7 +192,6 @@ nodes to be present and contain the properties described below.
 			  can be one of:
 			    "allwinner,sun6i-a31"
 			    "allwinner,sun8i-a23"
-			    "arm,psci"
 			    "arm,realview-smp"
 			    "brcm,bcm-nsp-smp"
 			    "brcm,brahma-b15"
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 11bfee8b79a9..b5d05807e6ec 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -360,7 +360,7 @@ static struct cpuidle_ops psci_cpuidle_ops __initdata = {
 	.init = psci_dt_cpu_init_idle,
 };
 
-CPUIDLE_METHOD_OF_DECLARE(psci, "arm,psci", &psci_cpuidle_ops);
+CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops);
 #endif
 #endif
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[Sudeep Holla]

On 22/04/16 16:18, Sudeep Holla wrote:
> Currently ARM CPUs DT bindings allows different enable-method value for
> PSCI based systems. On ARM 64-bit this property is required and must be
> "psci" while on ARM 32-bit systems this property is optional and must
> be "arm,psci" if present.
>
> However, "arm,psci" has always been the compatible string for the PSCI
> node, and was never intended to be the enable-method. So this is a bug
> in the binding and not a deliberate attempt at specifying 32-bit
> differently.
>
> This is problematic if 32-bit OS is run on 64-bit system which has
> "psci" as enable-method rather than the expected "arm,psci".
>
> So let's unify the value into "psci" and remove support for "arm,psci"
> before it finds any users.
>
> Reported-by: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> ---
>   Documentation/devicetree/bindings/arm/cpus.txt | 1 -
>   drivers/firmware/psci.c                        | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)
>
> v2->v1:
> 	- Added Mark and Lorenzo's ACKs
> 	- Updated commit message to specify that it also conflicts with
> 	  PSCI compatible(from Mark's reply on the thread)
>
> Hi ARM-SoC guys,
>
> Can you please pick up this single fix for PSCI binding/driver for v4.6 ?
>

Gentle ping to include this in next round of arm-soc fixes. It would be
good to get this fixed before v4.6 as we don't want to have user of the
binding which we want to remove in the release.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[Arnd Bergmann]

On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
> On 22/04/16 16:18, Sudeep Holla wrote:
> > Currently ARM CPUs DT bindings allows different enable-method value for
> > PSCI based systems. On ARM 64-bit this property is required and must be
> > "psci" while on ARM 32-bit systems this property is optional and must
> > be "arm,psci" if present.
> >
> > However, "arm,psci" has always been the compatible string for the PSCI
> > node, and was never intended to be the enable-method. So this is a bug
> > in the binding and not a deliberate attempt at specifying 32-bit
> > differently.
> >
> > This is problematic if 32-bit OS is run on 64-bit system which has
> > "psci" as enable-method rather than the expected "arm,psci".
> >
> > So let's unify the value into "psci" and remove support for "arm,psci"
> > before it finds any users.
> >
> > Reported-by: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
> > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > ---
> >   Documentation/devicetree/bindings/arm/cpus.txt | 1 -
> >   drivers/firmware/psci.c                        | 2 +-
> >   2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > v2->v1:
> >       - Added Mark and Lorenzo's ACKs
> >       - Updated commit message to specify that it also conflicts with
> >         PSCI compatible(from Mark's reply on the thread)
> >
> > Hi ARM-SoC guys,
> >
> > Can you please pick up this single fix for PSCI binding/driver for v4.6 ?
> >
> 
> Gentle ping to include this in next round of arm-soc fixes. It would be
> good to get this fixed before v4.6 as we don't want to have user of the
> binding which we want to remove in the release.
> 

Applied to the fixes branch. I have to admit that I don't understand
how the compatible string is really used here, so I have to trust that
this cannot break any existing machine.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[Mark Rutland]

On Tue, Apr 26, 2016 at 12:49:00PM +0200, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
> > On 22/04/16 16:18, Sudeep Holla wrote:
> > > Currently ARM CPUs DT bindings allows different enable-method value for
> > > PSCI based systems. On ARM 64-bit this property is required and must be
> > > "psci" while on ARM 32-bit systems this property is optional and must
> > > be "arm,psci" if present.
> > >
> > > However, "arm,psci" has always been the compatible string for the PSCI
> > > node, and was never intended to be the enable-method. So this is a bug
> > > in the binding and not a deliberate attempt at specifying 32-bit
> > > differently.
> > >
> > > This is problematic if 32-bit OS is run on 64-bit system which has
> > > "psci" as enable-method rather than the expected "arm,psci".
> > >
> > > So let's unify the value into "psci" and remove support for "arm,psci"
> > > before it finds any users.
> > >
> > > Reported-by: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
> > > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > ---
> > >   Documentation/devicetree/bindings/arm/cpus.txt | 1 -
> > >   drivers/firmware/psci.c                        | 2 +-
> > >   2 files changed, 1 insertion(+), 2 deletions(-)
> > >
> > > v2->v1:
> > >       - Added Mark and Lorenzo's ACKs
> > >       - Updated commit message to specify that it also conflicts with
> > >         PSCI compatible(from Mark's reply on the thread)
> > >
> > > Hi ARM-SoC guys,
> > >
> > > Can you please pick up this single fix for PSCI binding/driver for v4.6 ?
> > >
> > 
> > Gentle ping to include this in next round of arm-soc fixes. It would be
> > good to get this fixed before v4.6 as we don't want to have user of the
> > binding which we want to remove in the release.
> > 
> 
> Applied to the fixes branch. I have to admit that I don't understand
> how the compatible string is really used here, so I have to trust that
> this cannot break any existing machine.

The buggy code was merged for v4.6-rc1, so this hasn't been in a
release, and should not break an existing user.

No compatible string is used here, which is precisely the confusion this
addresses. The PSCI enable-method != the compatible string for a PSCI
node, but the two got confused in a documentation patch, and then that
confusion spread to code.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[Arnd Bergmann]

On Tuesday 26 April 2016 12:01:48 Mark Rutland wrote:
> On Tue, Apr 26, 2016 at 12:49:00PM +0200, Arnd Bergmann wrote:
> > On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:

> > > 
> > > Gentle ping to include this in next round of arm-soc fixes. It would be
> > > good to get this fixed before v4.6 as we don't want to have user of the
> > > binding which we want to remove in the release.
> > > 
> > 
> > Applied to the fixes branch. I have to admit that I don't understand
> > how the compatible string is really used here, so I have to trust that
> > this cannot break any existing machine.
> 
> The buggy code was merged for v4.6-rc1, so this hasn't been in a
> release, and should not break an existing user.
> 
> No compatible string is used here, which is precisely the confusion this
> addresses. The PSCI enable-method != the compatible string for a PSCI
> node, but the two got confused in a documentation patch, and then that
> confusion spread to code.
> 

Ok, thanks for the explanation.

	ARnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64,32}-bit systems

[Sudeep Holla]

On 26/04/16 11:49, Arnd Bergmann wrote:
> On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
>> On 22/04/16 16:18, Sudeep Holla wrote:
>>> Currently ARM CPUs DT bindings allows different enable-method value for
>>> PSCI based systems. On ARM 64-bit this property is required and must be
>>> "psci" while on ARM 32-bit systems this property is optional and must
>>> be "arm,psci" if present.
>>>
>>> However, "arm,psci" has always been the compatible string for the PSCI
>>> node, and was never intended to be the enable-method. So this is a bug
>>> in the binding and not a deliberate attempt at specifying 32-bit
>>> differently.
>>>
>>> This is problematic if 32-bit OS is run on 64-bit system which has
>>> "psci" as enable-method rather than the expected "arm,psci".
>>>
>>> So let's unify the value into "psci" and remove support for "arm,psci"
>>> before it finds any users.
>>>
>>> Reported-by: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
>>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>>> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
>>> Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
>>> ---
>>>    Documentation/devicetree/bindings/arm/cpus.txt | 1 -
>>>    drivers/firmware/psci.c                        | 2 +-
>>>    2 files changed, 1 insertion(+), 2 deletions(-)
>>>
>>> v2->v1:
>>>        - Added Mark and Lorenzo's ACKs
>>>        - Updated commit message to specify that it also conflicts with
>>>          PSCI compatible(from Mark's reply on the thread)
>>>
>>> Hi ARM-SoC guys,
>>>
>>> Can you please pick up this single fix for PSCI binding/driver for v4.6 ?
>>>
>>
>> Gentle ping to include this in next round of arm-soc fixes. It would be
>> good to get this fixed before v4.6 as we don't want to have user of the
>> binding which we want to remove in the release.
>>
>
> Applied to the fixes branch. I have to admit that I don't understand
> how the compatible string is really used here, so I have to trust that
> this cannot break any existing machine.
>

No it's not compatible string, it's the enable-method string value. So
far, I don't see any DT in the mainline using "arm,psci" as enable
method. It is indeed the compatible value for PSCI v0.1. Also, the
kernel didn't honor it even if it was present earlier until

Commit 8b6f2499ac45 ("ARM: 8511/1: ARM64: kernel: PSCI: move PSCI idle 
management code to drivers/firmware")

which was introduced in v4.6 and obeyed ARM32 DT bindings but Soby
reported issue when using 64-bit machine to run 32-bit kernel. So it was
good catch at right time. Hence we need to unify the binding though it
was never added intentionally and I assume it a mistake(git blame showed
it was not even reviewed). Luckily it was not used until v4.6, so we are
safe.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

[Jisheng Zhang]

Dear Arnd, Sudeep,

On Tue, 26 Apr 2016 12:49:00 +0200 Arnd Bergmann wrote:

> On Tuesday 26 April 2016 11:42:58 Sudeep Holla wrote:
> > On 22/04/16 16:18, Sudeep Holla wrote:  
> > > Currently ARM CPUs DT bindings allows different enable-method value for
> > > PSCI based systems. On ARM 64-bit this property is required and must be
> > > "psci" while on ARM 32-bit systems this property is optional and must
> > > be "arm,psci" if present.
> > >
> > > However, "arm,psci" has always been the compatible string for the PSCI
> > > node, and was never intended to be the enable-method. So this is a bug
> > > in the binding and not a deliberate attempt at specifying 32-bit
> > > differently.
> > >
> > > This is problematic if 32-bit OS is run on 64-bit system which has
> > > "psci" as enable-method rather than the expected "arm,psci".
> > >
> > > So let's unify the value into "psci" and remove support for "arm,psci"
> > > before it finds any users.
> > >
> > > Reported-by: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
> > > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > > Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> > > Signed-off-by: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> > > ---
> > >   Documentation/devicetree/bindings/arm/cpus.txt | 1 -
> > >   drivers/firmware/psci.c                        | 2 +-
> > >   2 files changed, 1 insertion(+), 2 deletions(-)
> > >
> > > v2->v1:
> > >       - Added Mark and Lorenzo's ACKs
> > >       - Updated commit message to specify that it also conflicts with
> > >         PSCI compatible(from Mark's reply on the thread)
> > >
> > > Hi ARM-SoC guys,
> > >
> > > Can you please pick up this single fix for PSCI binding/driver for v4.6 ?
> > >  
> > 
> > Gentle ping to include this in next round of arm-soc fixes. It would be
> > good to get this fixed before v4.6 as we don't want to have user of the
> > binding which we want to remove in the release.
> >   
> 
> Applied to the fixes branch. I have to admit that I don't understand
> how the compatible string is really used here, so I have to trust that
> this cannot break any existing machine.

To be honest, this breaks some Marvell DEBU's 32 bit SoCs' kernel, but these
SoCs' support are not mainlined yet, so it's fine to fix it as early as
possible.

Thanks,
Jisheng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers: firmware: psci: unify enable-method binding on ARM {64, 32}-bit systems

[Sudeep Holla]

Hi Jisheng,

On 26/04/16 12:02, Jisheng Zhang wrote:
> Dear Arnd, Sudeep,
>
> On Tue, 26 Apr 2016 12:49:00 +0200 Arnd Bergmann wrote:
>

[...]

>>
>> Applied to the fixes branch. I have to admit that I don't understand
>> how the compatible string is really used here, so I have to trust that
>> this cannot break any existing machine.
>
> To be honest, this breaks some Marvell DEBU's 32 bit SoCs' kernel, but these

Sorry for that, but it's better to fix it in the firmware/DT.
As I mentioned earlier it was never honored in the kernel before v4.6,
so technically nothing is broken yet :)

> SoCs' support are not mainlined yet, so it's fine to fix it as early as
> possible.
>

Good to hear that. Thanks indeed.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html