PSCI support for highbank

From: Rob Herring <[email protected]>

The PSCI spec from ARM has been updated to 0.2 version. Update the binding document to reflect the spec changes. For the binding, the major changes are addition of system reset and poweroff functions. The recommended function id numbering has also changed.

Signed-off-by Rob Herring <[email protected]>

Cc devicetree at vger.kernel.org

---
 Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/arm/psci.txt +0 -0

index 433afe9..b8b4d9f 100644

Documentation/devicetree/bindings/arm/psci.txt +6 -5

@@ -42,14 +45,24 @@ Main node optional properties:
 
  - migrate       : Function ID for MIGRATE operation
 
+ - system_reset  : Function ID for SYSTEM_RESET operation
+
+ - system_off    : Function ID for SYSTEM_OFF operation
+
 
 Example:
 
 	psci {
-		compatible	= "arm,psci";
+		compatible	= "arm,psci-0.2";
 		method		= "smc";
-		cpu_suspend	= <0x95c10000>;
-		cpu_off		= <0x95c10001>;
-		cpu_on		= <0x95c10002>;
-		migrate		= <0x95c10003>;
+		psci_version	= <0x84000000>;

Hi Rob,

Quoted 18 lines · Rob Herring

From: Rob Herring <[email protected]>

The PSCI spec from ARM has been updated to 0.2 version. Update the binding document to reflect the spec changes. For the binding, the major changes are addition of system reset and poweroff functions. The recommended function id numbering has also changed.

Signed-off-by Rob Herring <[email protected]>

Cc devicetree at vger.kernel.org

---
 Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/arm/psci.txt +0 -0

index 433afe9..b8b4d9f 100644

Documentation/devicetree/bindings/arm/psci.txt to #0. · line 24

Quoted 5 lines

Documentation/devicetree/bindings/arm/psci.txt +1 -1

@@ -21,7 +21,7 @@ to #0.
 
 Main node required properties:
 
- - compatible    : Must be "arm,psci"
+ - compatible    : Must be "arm,psci-0.2" or "arm,psci"

For the purposes of handling different firmware implementations (which may have different bugs to work around and may require different arguments to cpu_suspend), it may be better to state "must contain" rather than "must be".

Quoted 12 lines

Documentation/devicetree/bindings/arm/psci.txt +3 -0

  - method        : The method of calling the PSCI firmware. Permitted
                    values are:

@@ -32,6 +32,9 @@ Main node required properties:
                    "hvc" : HVC #0, with the register assignments specified
 		           in this binding.
 
+ - psci_version  : Function ID for PSCI_VERSION operation. Required for
+                   "arm,psci-0.2" compatible version or later.
+
 Main node optional properties:
 

Documentation/devicetree/bindings/arm/psci.txt Main node required properties: · line 40

Quoted 1 line

Documentation/devicetree/bindings/arm/psci.txt +0 -0

  - cpu_suspend   : Function ID for CPU_SUSPEND operation

The low bits of CPU_SUSPEND's power_state argument are platform specific and we'll need to deal with them if an implementation actually uses them for something. We can probably associate this with a specific implementation's compatible string, as we'll almost certainly need special code to handle the differences between them.

If we have a compatible string for the first platform that ignores the argument (or just uses zero), that can be shared by all those implementations that don't give any fine-grained control here.

Quoted 20 lines

Documentation/devicetree/bindings/arm/psci.txt +6 -5

@@ -42,14 +45,24 @@ Main node optional properties:
 
  - migrate       : Function ID for MIGRATE operation
 
+ - system_reset  : Function ID for SYSTEM_RESET operation
+
+ - system_off    : Function ID for SYSTEM_OFF operation
+
 
 Example:
 
 	psci {
-		compatible	= "arm,psci";
+		compatible	= "arm,psci-0.2";
 		method		= "smc";
-		cpu_suspend	= <0x95c10000>;
-		cpu_off		= <0x95c10001>;
-		cpu_on		= <0x95c10002>;
-		migrate		= <0x95c10003>;
+		psci_version	= <0x84000000>;

Documentation/devicetree/bindings/arm/psci.txt Main node optional properties: · line 68

Quoted 10 lines

Documentation/devicetree/bindings/arm/psci.txt +9 -0

+		cpu_suspend	= <0x84000001>;
+		cpu_off		= <0x84000002>;
+		cpu_on		= <0x84000003>;
+		affinity_info	= <0x84000004>; 
+		migrate		= <0x84000005>;
+		migrate_info_type = <0x84000006>; 
+		migrate_info_up_cpu = <0x84000007>; 
+		system_off	= <0x84000008>; 
+		system_reset	= <0x84000009>; 
 	};

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Quoted 44 lines · Mark Rutland

Hi Rob,

Quoted 18 lines · Rob Herring

From: Rob Herring <[email protected]>

The PSCI spec from ARM has been updated to 0.2 version. Update the binding document to reflect the spec changes. For the binding, the major changes are addition of system reset and poweroff functions. The recommended function id numbering has also changed.

Signed-off-by Rob Herring <[email protected]>

Cc devicetree at vger.kernel.org

---
 Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/arm/psci.txt +0 -0

index 433afe9..b8b4d9f 100644

Documentation/devicetree/bindings/arm/psci.txt to #0. · line 24

Quoted 5 lines

Documentation/devicetree/bindings/arm/psci.txt +1 -1

@@ -21,7 +21,7 @@ to #0.
 
 Main node required properties:
 
- - compatible    : Must be "arm,psci"
+ - compatible    : Must be "arm,psci-0.2" or "arm,psci"

For the purposes of handling different firmware implementations (which may have different bugs to work around and may require different arguments to cpu_suspend), it may be better to state "must contain" rather than "must be".

Quoted 12 lines

Documentation/devicetree/bindings/arm/psci.txt +3 -0

  - method        : The method of calling the PSCI firmware. Permitted
                    values are:

@@ -32,6 +32,9 @@ Main node required properties:
                    "hvc" : HVC #0, with the register assignments specified
 		           in this binding.
 
+ - psci_version  : Function ID for PSCI_VERSION operation. Required for
+                   "arm,psci-0.2" compatible version or later.
+
 Main node optional properties:
 

Quoted 7 lines

Documentation/devicetree/bindings/arm/psci.txt Main node required properties: · line 40

Quoted 1 line

Documentation/devicetree/bindings/arm/psci.txt +0 -0

  - cpu_suspend   : Function ID for CPU_SUSPEND operation

The low bits of CPU_SUSPEND's power_state argument are platform specific and we'll need to deal with them if an implementation actually uses them for something. We can probably associate this with a specific implementation's compatible string, as we'll almost certainly need special code to handle the differences between them.

I'm not a fan of

Quoted 42 lines

If we have a compatible string for the first platform that ignores the argument (or just uses zero), that can be shared by all those implementations that don't give any fine-grained control here.

Quoted 20 lines

Documentation/devicetree/bindings/arm/psci.txt +6 -5

@@ -42,14 +45,24 @@ Main node optional properties:
 
  - migrate       : Function ID for MIGRATE operation
 
+ - system_reset  : Function ID for SYSTEM_RESET operation
+
+ - system_off    : Function ID for SYSTEM_OFF operation
+
 
 Example:
 
 	psci {
-		compatible	= "arm,psci";
+		compatible	= "arm,psci-0.2";
 		method		= "smc";
-		cpu_suspend	= <0x95c10000>;
-		cpu_off		= <0x95c10001>;
-		cpu_on		= <0x95c10002>;
-		migrate		= <0x95c10003>;
+		psci_version	= <0x84000000>;

Documentation/devicetree/bindings/arm/psci.txt Main node optional properties: · line 68

Quoted 10 lines

Documentation/devicetree/bindings/arm/psci.txt +9 -0

+		cpu_suspend	= <0x84000001>;
+		cpu_off		= <0x84000002>;
+		cpu_on		= <0x84000003>;
+		affinity_info	= <0x84000004>; 
+		migrate		= <0x84000005>;
+		migrate_info_type = <0x84000006>; 
+		migrate_info_up_cpu = <0x84000007>; 
+		system_off	= <0x84000008>; 
+		system_reset	= <0x84000009>; 
 	};

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

Quoted 4 lines

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

A variation on this would be keep method as is and add a "#psci-cells" property to specify the number of function IDs. You can determine the 64-bit vs. 32-bit support based on the function ID itself.

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

I've added Christoffer, Marc, and Stefano to Cc in case they have any comments.

Quoted 1 line

Quoted 3 lines

A variation on this would be keep method as is and add a "#psci-cells" property to specify the number of function IDs. You can determine the 64-bit vs. 32-bit support based on the function ID itself.

I don't think that's a good idea - part of the reasoning for specifying the IDs is to cater for those not aligned with the ID guidelines in the spec, so we can't assume their choice of ID value gives us any useful information as to how they may be used.

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 31 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

Quoted 5 lines

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Quoted 47 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 36 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

Quoted 5 lines

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

Quoted 48 lines · Mark Rutland

Quoted 47 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 36 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

Quoted 8 lines

Quoted 5 lines

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

Actually, from some quick discussion with Marc and Dave, I think we can handle this better, in a way that leaves this backwards compatible.

Rather than relying on the method string to tell us the calling convention, I think we should rely on the function ID, as I proposed earlier. The existing function ids provided in the "arm,psci" binding are implicitly relying on the PSCI implementation to detect the register width and act accordingly. This is trivially true on 32bit hardware, KVM (where the same ID is used for 32-bit and 64-bit guests), and while I'm not entirely sure about Xen I believe it's true there. We can make this explicit as we extend the binding.

Having a -64 and -32 variant of each ID (while not pretty) allows us to add additional IDs for functions that might only have a 32-bit or 64-bit interface implemented, in addition to functions with common IDs:

psci {
	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
	cpu_off = <12345678>;
	cpu_on = <01234567>;
	system_reset-32 = <02222222>;
	system_reset-64 = <12222222>;
	affinity_info-64 = <15555555>;
};

This means that hypervisors could update their PSCI implementation while keeping their DTS compatible with existing kernels.

Quoted 55 lines · Mark Rutland

Quoted 54 lines · Mark Rutland

Quoted 52 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 31 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

Quoted 5 lines

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Quoted 2 lines

Quoted 2 lines

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

I would be happy with that too (Xen only uses HVC as "method").

Quoted 26 lines

Actually, from some quick discussion with Marc and Dave, I think we can handle this better, in a way that leaves this backwards compatible.

Rather than relying on the method string to tell us the calling convention, I think we should rely on the function ID, as I proposed earlier. The existing function ids provided in the "arm,psci" binding are implicitly relying on the PSCI implementation to detect the register width and act accordingly. This is trivially true on 32bit hardware, KVM (where the same ID is used for 32-bit and 64-bit guests), and while I'm not entirely sure about Xen I believe it's true there. We can make this explicit as we extend the binding.

Having a -64 and -32 variant of each ID (while not pretty) allows us to add additional IDs for functions that might only have a 32-bit or 64-bit interface implemented, in addition to functions with common IDs:

psci {
	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
	cpu_off = <12345678>;
	cpu_on = <01234567>;
	system_reset-32 = <02222222>;
	system_reset-64 = <12222222>;
	affinity_info-64 = <15555555>;
};

Quoted 2 lines

This means that hypervisors could update their PSCI implementation while keeping their DTS compatible with existing kernels.

Are you proposing of getting rid of "method" completely and therefore have 4 possible function IDs for each function:

system_reset-32-HVC
system_reset-64-SMC
system_reset-32-HVC
system_reset-64-SMC

or are you proposing of choosing just the register width via function IDs?

Honestly I think it would be cleaner to introduce a new field called "width" can that be 32 or 64 and represent the register width, rather than having an explosion of function IDs.

Quoted 56 lines · Stefano Stabellini

Quoted 55 lines · Mark Rutland

Quoted 54 lines · Mark Rutland

Quoted 52 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 31 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

     method          = "smc", "smc64";
     psci_version    = <0x84000000 0xc4000000>;
     cpu_suspend     = <0x84000001 0xc4000001>;
     cpu_off         = <0x84000002 0xc4000002>;
     cpu_on          = <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

Quoted 5 lines

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

I would be happy with that too (Xen only uses HVC as "method").

Presumably we have the same issues with hvc vs hvc32 vs hvc64 though?

And does smcX imply hvcX or does that also need to be documented here?

Quoted 37 lines

Quoted 26 lines

Actually, from some quick discussion with Marc and Dave, I think we can handle this better, in a way that leaves this backwards compatible.

Rather than relying on the method string to tell us the calling convention, I think we should rely on the function ID, as I proposed earlier. The existing function ids provided in the "arm,psci" binding are implicitly relying on the PSCI implementation to detect the register width and act accordingly. This is trivially true on 32bit hardware, KVM (where the same ID is used for 32-bit and 64-bit guests), and while I'm not entirely sure about Xen I believe it's true there. We can make this explicit as we extend the binding.

Having a -64 and -32 variant of each ID (while not pretty) allows us to add additional IDs for functions that might only have a 32-bit or 64-bit interface implemented, in addition to functions with common IDs:

psci {
    compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
    cpu_off = <12345678>;
    cpu_on = <01234567>;
    system_reset-32 = <02222222>;
    system_reset-64 = <12222222>;
    affinity_info-64 = <15555555>;
};

Quoted 2 lines

This means that hypervisors could update their PSCI implementation while keeping their DTS compatible with existing kernels.

Are you proposing of getting rid of "method" completely and therefore have 4 possible function IDs for each function:

system_reset-32-HVC
system_reset-64-SMC
system_reset-32-HVC
system_reset-64-SMC

Quoted 6 lines

or are you proposing of choosing just the register width via function IDs?

Honestly I think it would be cleaner to introduce a new field called "width" can that be 32 or 64 and represent the register width, rather than having an explosion of function IDs.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

* An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird and in any case, the device tree would use the 32-bit convention (any SMC64 call in 32-bit state should return "Unknown SMC Function Identifier" if that bit is set)

TLDR; Supply your (EL1) guests with the preferred method, not a list of all possible methods, as above. If you're the vendor and you defined any part of EL3 or EL2 you're in complete control of which method you want subordinate levels to use by way of implementation, right?

BTW according to the specs you can't have EL2 without EL3 on ARMv7. I would think the issues with hvc vs hvc64 need to be thought out; hypervisors can trap SMC from their guests and do the right thing without the guest ever knowing that a hypervisor is above it.

If it is the case that every Xen DomU kernel needs to know that it is virtualized and to be told to use HVC instead of SMC? What's the benefit of that?

Isn't this being overthought?

"method" (I would have re-used model, but that's by-the-by) should just determine which call interface to use. Only one should end up in a device tree - it doesn't make a lot of sense to specify more than one, to me, given my read of the specs and the SMC calling convention.

For AArch64 chips they should implement 64-bit SMC interface or 32-bit SMC interface. Either way you get there, the function number is encoded with the type of call anyway so AArch64 secure monitors or hypervisors will know which one was intended, given the SMC calling convention defines this "64-bit" bit pretty clearly. The "method" is an instruction to the "guest OS" in how to fill it's registers more than it is in function names.

* An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird and in any case, the device tree would use the 32-bit convention (any SMC64 call in 32-bit state should return "Unknown SMC Function Identifier" if that bit is set)

* An AArch32 guest under an AArch32 hypervisor/sm would use the 32-bit convention
* An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit convention

* An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit convention or the 32-bit convention depending on how the sm is written, but it doesn't matter which is used if both can be supported.. but you'd only want to use one of them.

TLDR; Supply your (EL1) guests with the preferred method, not a list of all possible methods, as above. If you're the vendor and you defined any part of EL3 or EL2 you're in complete control of which method you want subordinate levels to use by way of implementation, right?

If we force the use of said bit, then new kernels cannot necessarily work on existing PSCI implementations (which may not handle the use of said bit as you expect). If we make the binding for psci-0.2 explicitly require the use of this bit, then future firmware/virtualisation code cannot provide new functionality withoot breaking backwards compatiblity with existing kernels, as an "arm,psci-0.2" node would be incompatible with an "arm,psci" node (which is all an existing kernel can handle).

Quoted 4 lines

* An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird and in any case, the device tree would use the 32-bit convention (any SMC64 call in 32-bit state should return "Unknown SMC Function Identifier" if that bit is set)

This is not allowed by the architecture.

Quoted 4 lines

* An AArch32 guest under an AArch32 hypervisor/sm would use the 32-bit convention
* An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit convention

Both of these cases are trivially mandated by the architecture.

Quoted 4 lines

* An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit convention or the 32-bit convention depending on how the sm is written, but it doesn't matter which is used if both can be supported.. but you'd only want to use one of them.

Not all implementation will implement both, so there needs to be a way to describe that. Which is actually used is up to the OS.

As far as I can tell, no-one was arguing against this strategy. However, for those cases where functionality is implemented for only one register-width, or requires a different ID per register-width, this needs to be described somehow.

On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <[email protected]> wrote:

Quoted 7 lines

Quoted 4 lines

* An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit convention or the 32-bit convention depending on how the sm is written, but it doesn't matter which is used if both can be supported.. but you'd only want to use one of them.

Not all implementation will implement both, so there needs to be a way to describe that. Which is actually used is up to the OS.

Okay but putting both ways in a single node, and describing two function ids (per property or with two properties) is what I was getting at.. for the one case where you can use both at once, the device tree description is king here.

I am a little concerned that the support for this is going down the route of telling the OS all possible ways to do the same thing instead of trying to get into using a single, preferred way in the common cases.

Putting both call methods in the same node, doubling the function id property lengths, or suffixing function id properties to do it seems like putting information in the tree purely for the sake of it. In what cases would it (uncommon cases only being existing, pre-PCSI pre-SMC-conventions potentially for other things). In the case of PSCI there's an opportunity to be strict about it.. why would it be sane to allow a mixed implementation? Dictate that either support all the 64-bit versions or all the 32-bit versions or both? And if both are supported, dictate in the device tree which the OS should be using.

What about having two nodes? There is nothing in device trees that says you can't have two psci { compatible="arm,psci-0.2" } nodes, one with method=smc and one with method=smc64 or hvc64 or what have you. Parsing and setup of the PSCI code can be quit early if it's not the "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then each node follows the exact same definition, and the differentiator is the call method and not the property names or complicating their contents.

Quoted 1 line · Matt Sealey

On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <[email protected]> wrote:

[snip]

Quoted 18 lines

Quoted 7 lines

Quoted 4 lines

* An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit convention or the 32-bit convention depending on how the sm is written, but it doesn't matter which is used if both can be supported.. but you'd only want to use one of them.

Not all implementation will implement both, so there needs to be a way to describe that. Which is actually used is up to the OS.

Okay but putting both ways in a single node, and describing two function ids (per property or with two properties) is what I was getting at.. for the one case where you can use both at once, the device tree description is king here.

I am a little concerned that the support for this is going down the route of telling the OS all possible ways to do the same thing instead of trying to get into using a single, preferred way in the common cases.

Quoted 9 lines

Putting both call methods in the same node, doubling the function id property lengths, or suffixing function id properties to do it seems like putting information in the tree purely for the sake of it. In what cases would it (uncommon cases only being existing, pre-PCSI pre-SMC-conventions potentially for other things). In the case of PSCI there's an opportunity to be strict about it.. why would it be sane to allow a mixed implementation? Dictate that either support all the 64-bit versions or all the 32-bit versions or both? And if both are supported, dictate in the device tree which the OS should be using.

Quoted 8 lines

What about having two nodes? There is nothing in device trees that says you can't have two psci { compatible="arm,psci-0.2" } nodes, one with method=smc and one with method=smc64 or hvc64 or what have you. Parsing and setup of the PSCI code can be quit early if it's not the "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then each node follows the exact same definition, and the differentiator is the call method and not the property names or complicating their contents.

+1

This would certainly be easier for things like a hypervisor to parse and update.

Quoted 23 lines · Rob Herring

Quoted 1 line · Matt Sealey

On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <[email protected]> wrote:

[snip]

Quoted 18 lines

Quoted 7 lines

Quoted 4 lines

* An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit convention or the 32-bit convention depending on how the sm is written, but it doesn't matter which is used if both can be supported.. but you'd only want to use one of them.

Not all implementation will implement both, so there needs to be a way to describe that. Which is actually used is up to the OS.

Okay but putting both ways in a single node, and describing two function ids (per property or with two properties) is what I was getting at.. for the one case where you can use both at once, the device tree description is king here.

I am a little concerned that the support for this is going down the route of telling the OS all possible ways to do the same thing instead of trying to get into using a single, preferred way in the common cases.

Quoted 9 lines

Quoted 9 lines

Putting both call methods in the same node, doubling the function id property lengths, or suffixing function id properties to do it seems like putting information in the tree purely for the sake of it. In what cases would it (uncommon cases only being existing, pre-PCSI pre-SMC-conventions potentially for other things). In the case of PSCI there's an opportunity to be strict about it.. why would it be sane to allow a mixed implementation? Dictate that either support all the 64-bit versions or all the 32-bit versions or both? And if both are supported, dictate in the device tree which the OS should be using.

The DT is about description, not policy. If multiple usable flavours of the PSCI interface exist, then they exist, and there's no special reason to hide one of them from the DT that I can see.

I take the point that we shouldn't describe useless or redundant information for no good reason, though.

Quoted 11 lines

Quoted 8 lines

What about having two nodes? There is nothing in device trees that says you can't have two psci { compatible="arm,psci-0.2" } nodes, one with method=smc and one with method=smc64 or hvc64 or what have you. Parsing and setup of the PSCI code can be quit early if it's not the "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then each node follows the exact same definition, and the differentiator is the call method and not the property names or complicating their contents.

+1

Quoted 2 lines

This would certainly be easier for things like a hypervisor to parse and update.

Can you elaborate on that?

Wouldn't a hypervisor always rip out and replace PSCI node(s) with its own? Allowing the firmware's PSCI interface to "show through" to a guest VM sounds unwise.

I'm not sure why having multiple nodes makes this easier.

Having two nodes just moves information around. Does it really simplify anything?

With the multiple nodes approach, you might actually need 3 nodes if you are providing backwards compatible support for psci-0.1 callers: one for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1. Maybe not though ... I'd have to think about it a bit more.

Quoted 7 lines · Dave Martin

Quoted 5 lines · Rob Herring

Quoted 1 line · Matt Sealey

On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <[email protected]> wrote:

[snip]

Quoted 6 lines

The DT is about description, not policy. If multiple usable flavours of the PSCI interface exist, then they exist, and there's no special reason to hide one of them from the DT that I can see.

I take the point that we shouldn't describe useless or redundant information for no good reason, though.

Quoted 3 lines

Wouldn't a hypervisor always rip out and replace PSCI node(s) with its own? Allowing the firmware's PSCI interface to "show through" to a guest VM sounds unwise.

Perhaps. Minimally, the hypervisor could just replace smc with hvc for the method.

Quoted 4 lines

I'm not sure why having multiple nodes makes this easier.

Having two nodes just moves information around. Does it really simplify anything?

You can add 'status = "disabled";' easily and have it apply to the whole node. Also, if you split-up by method, then it is clear which properties apply to which method. I don't really care for the <none>, -32 and -64 distinction on function ID properties.

Quoted 4 lines

With the multiple nodes approach, you might actually need 3 nodes if you are providing backwards compatible support for psci-0.1 callers: one for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1. Maybe not though ... I'd have to think about it a bit more.

I'd hope we can transition away from psci-0.1. The things that rely on it are not really mature anyway.

Quoted 56 lines · Stefano Stabellini

Quoted 55 lines · Mark Rutland

Quoted 54 lines · Mark Rutland

Quoted 52 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 31 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

Quoted 5 lines

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

I would be happy with that too (Xen only uses HVC as "method").

Presumably we have the same issues with hvc vs hvc32 vs hvc64 though?

And does smcX imply hvcX or does that also need to be documented here?

Quoted 49 lines · Mark Rutland

Quoted 48 lines · Mark Rutland

Quoted 47 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 36 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

I would be happy with that too (Xen only uses HVC as "method").

Quoted 25 lines

Actually, from some quick discussion with Marc and Dave, I think we can handle this better, in a way that leaves this backwards compatible.

Rather than relying on the method string to tell us the calling convention, I think we should rely on the function ID, as I proposed earlier. The existing function ids provided in the "arm,psci" binding are implicitly relying on the PSCI implementation to detect the register width and act accordingly. This is trivially true on 32bit hardware, KVM (where the same ID is used for 32-bit and 64-bit guests), and while I'm not entirely sure about Xen I believe it's true there. We can make this explicit as we extend the binding.

Having a -64 and -32 variant of each ID (while not pretty) allows us to add additional IDs for functions that might only have a 32-bit or 64-bit interface implemented, in addition to functions with common IDs:

psci {
	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
	cpu_off = <12345678>;
	cpu_on = <01234567>;
	system_reset-32 = <02222222>;
	system_reset-64 = <12222222>;
	affinity_info-64 = <15555555>;
};

Quoted 2 lines

This means that hypervisors could update their PSCI implementation while keeping their DTS compatible with existing kernels.

Are you proposing of getting rid of "method" completely and therefore have 4 possible function IDs for each function:

system_reset-32-HVC
system_reset-64-SMC
system_reset-32-HVC
system_reset-64-SMC

or are you proposing of choosing just the register width via function IDs?

Honestly I think it would be cleaner to introduce a new field called "width" can that be 32 or 64 and represent the register width, rather than having an explosion of function IDs.

I can think of lots of ways it would be cleaner, but that is not what ARM defined. There are 3 cases to handle: 32-bit only calls, 64-bit only calls and both 32/64-bit calls. I think hypervisors have the same issue as firmware. Do you know the guest is 32 or 64 bit up front and can provide it a dtb based on that?

Quoted 49 lines · Mark Rutland

Quoted 48 lines · Mark Rutland

Quoted 47 lines · Rob Herring

Quoted 6 lines · Mark Rutland

Quoted 5 lines · Rob Herring

Quoted 4 lines · Mark Rutland

Hi Rob,

Quoted 1 line · Rob Herring

From: Rob Herring <[email protected]>

[snip]

Quoted 36 lines

Quoted 18 lines

Quoted 10 lines

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

Quoted 8 lines

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be compatible with the arm64 implementation, which assumes smc64 by default.

As far as I am aware, the implementations currently in use (KVM and Xen) use the same ID for both, so I think "smc" should cover an ID valid for a native register width calling convention, and "smc64" and "smc32" describing values only valid for 64-bit wide and 32-bit wide calling conventions respectively.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

The problem is that does not work for a 32-bit kernel on 64-bit h/w as native from the dts perspective is smc64. Just like the cpu bindings, the binding cannot change based on 32 or 64 bit OS. I don't think we really have to deal with that here. We can simply say "smc" is only for "arm,psci" and deprecated for "arm,psci-0.2".

Agreed. I'd be happy with only having "smc32" and "smc64" for "arm,psci-0.2".

I would be happy with that too (Xen only uses HVC as "method").

Quoted 25 lines

Actually, from some quick discussion with Marc and Dave, I think we can handle this better, in a way that leaves this backwards compatible.

Rather than relying on the method string to tell us the calling convention, I think we should rely on the function ID, as I proposed earlier. The existing function ids provided in the "arm,psci" binding are implicitly relying on the PSCI implementation to detect the register width and act accordingly. This is trivially true on 32bit hardware, KVM (where the same ID is used for 32-bit and 64-bit guests), and while I'm not entirely sure about Xen I believe it's true there. We can make this explicit as we extend the binding.

Having a -64 and -32 variant of each ID (while not pretty) allows us to add additional IDs for functions that might only have a 32-bit or 64-bit interface implemented, in addition to functions with common IDs:

psci {
	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
	cpu_off = <12345678>;
	cpu_on = <01234567>;
	system_reset-32 = <02222222>;
	system_reset-64 = <12222222>;
	affinity_info-64 = <15555555>;
};

Quoted 2 lines

This means that hypervisors could update their PSCI implementation while keeping their DTS compatible with existing kernels.

Are you proposing of getting rid of "method" completely and therefore have 4 possible function IDs for each function:

system_reset-32-HVC
system_reset-64-SMC
system_reset-32-HVC
system_reset-64-SMC

or are you proposing of choosing just the register width via function IDs?

Honestly I think it would be cleaner to introduce a new field called "width" can that be 32 or 64 and represent the register width, rather than having an explosion of function IDs.

Quoted 44 lines · Mark Rutland

Hi Rob,

Quoted 18 lines · Rob Herring

From: Rob Herring <[email protected]>

The PSCI spec from ARM has been updated to 0.2 version. Update the binding document to reflect the spec changes. For the binding, the major changes are addition of system reset and poweroff functions. The recommended function id numbering has also changed.

Signed-off-by Rob Herring <[email protected]>

Cc devicetree at vger.kernel.org

---
 Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/arm/psci.txt +0 -0

index 433afe9..b8b4d9f 100644

Documentation/devicetree/bindings/arm/psci.txt to #0. · line 24

Quoted 5 lines

Documentation/devicetree/bindings/arm/psci.txt +1 -1

@@ -21,7 +21,7 @@ to #0.
 
 Main node required properties:
 
- - compatible    : Must be "arm,psci"
+ - compatible    : Must be "arm,psci-0.2" or "arm,psci"

For the purposes of handling different firmware implementations (which may have different bugs to work around and may require different arguments to cpu_suspend), it may be better to state "must contain" rather than "must be".

Quoted 12 lines

Documentation/devicetree/bindings/arm/psci.txt +3 -0

  - method        : The method of calling the PSCI firmware. Permitted
                    values are:

@@ -32,6 +32,9 @@ Main node required properties:
                    "hvc" : HVC #0, with the register assignments specified
 		           in this binding.
 
+ - psci_version  : Function ID for PSCI_VERSION operation. Required for
+                   "arm,psci-0.2" compatible version or later.
+
 Main node optional properties:
 

Quoted 7 lines

Documentation/devicetree/bindings/arm/psci.txt Main node required properties: · line 40

Quoted 1 line

Documentation/devicetree/bindings/arm/psci.txt +0 -0

  - cpu_suspend   : Function ID for CPU_SUSPEND operation

The low bits of CPU_SUSPEND's power_state argument are platform specific and we'll need to deal with them if an implementation actually uses them for something. We can probably associate this with a specific implementation's compatible string, as we'll almost certainly need special code to handle the differences between them.

I'm not a fan of

Quoted 42 lines

If we have a compatible string for the first platform that ignores the argument (or just uses zero), that can be shared by all those implementations that don't give any fine-grained control here.

Quoted 20 lines

Documentation/devicetree/bindings/arm/psci.txt +6 -5

@@ -42,14 +45,24 @@ Main node optional properties:
 
  - migrate       : Function ID for MIGRATE operation
 
+ - system_reset  : Function ID for SYSTEM_RESET operation
+
+ - system_off    : Function ID for SYSTEM_OFF operation
+
 
 Example:
 
 	psci {
-		compatible	= "arm,psci";
+		compatible	= "arm,psci-0.2";
 		method		= "smc";
-		cpu_suspend	= <0x95c10000>;
-		cpu_off		= <0x95c10001>;
-		cpu_on		= <0x95c10002>;
-		migrate		= <0x95c10003>;
+		psci_version	= <0x84000000>;

Documentation/devicetree/bindings/arm/psci.txt Main node optional properties: · line 68

Quoted 10 lines

Documentation/devicetree/bindings/arm/psci.txt +9 -0

+		cpu_suspend	= <0x84000001>;
+		cpu_off		= <0x84000002>;
+		cpu_on		= <0x84000003>;
+		affinity_info	= <0x84000004>; 
+		migrate		= <0x84000005>;
+		migrate_info_type = <0x84000006>; 
+		migrate_info_up_cpu = <0x84000007>; 
+		system_off	= <0x84000008>; 
+		system_reset	= <0x84000009>; 
 	};

One of the things changed in PSCI 0.2 was the SMC calling convention, though this isn't clear in the PSCI document. The function IDs for 32bit and 64bit callers may differ, and we need to support describing an arbitrary configuration of the two (same ID for both, different across 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

Quoted 4 lines

I'd like to ensure the binding can deal with that from the start. We could do this by having -32 and -64 variants of each function id (e.g. cpu_off-64) , if the IDs actually differ, and use the regular combined ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was simply documenting what is already in the PSCI doc, but obviously that is not fully flushed out.

How about something like this (for the complicated case of both 32 and 64 bit):

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of methods determines the number and order of function IDs.

A variation on this would be keep method as is and add a "#psci-cells" property to specify the number of function IDs. You can determine the 64-bit vs. 32-bit support based on the function ID itself.

From: Rob Herring <[email protected]>

This updates the Calxeda cpuidle driver to use PSCI calls to powergate cores. This is needed to enable cpuidle for the ECX-2000.

This could possibly become a generic PSCI driver, but there are no other PSCI users in the kernel other than mach-virt.

Signed-off-by Rob Herring <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc linux-pm at vger.kernel.org

---

 drivers/cpuidle/cpuidle-calxeda.c | 40 ++++++---------------------------------
 1 file changed, 6 insertions(+), 34 deletions(-)

drivers/cpuidle/cpuidle-calxeda.c +6 -34

index 0e6e408..01cfecf3 100644
@@ -22,51 +22,23 @@
 
 #include <linux/cpuidle.h>
 #include <linux/init.h>
-#include <linux/io.h>
 #include <linux/of.h>
-#include <linux/time.h>
-#include <linux/delay.h>
-#include <linux/suspend.h>
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
-#include <asm/smp_scu.h>
 #include <asm/suspend.h>
-#include <asm/cacheflush.h>
-#include <asm/cp15.h>
-
-extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
-extern void *scu_base_addr;
-
-static noinline void calxeda_idle_restore(void)
-{
-	set_cr(get_cr() | CR_C);
-	set_auxcr(get_auxcr() | 0x40);
-	scu_power_mode(scu_base_addr, SCU_PM_NORMAL);
-}
+#include <asm/psci.h>
 
 static int calxeda_idle_finish(unsigned long val)
 {
-	/* Already flushed cache, but do it again as the outer cache functions
-	 * dirty the cache with spinlocks */
-	flush_cache_all();
-
-	set_auxcr(get_auxcr() & ~0x40);
-	set_cr(get_cr() & ~CR_C);
-
-	scu_power_mode(scu_base_addr, SCU_PM_DORMANT);
-
-	cpu_do_idle();
-
-	/* Restore things if we didn't enter power-gating */
-	calxeda_idle_restore();
-	return 1;
+	const struct psci_power_state ps = {
+		.type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
+	};
+	return psci_ops.cpu_suspend(ps, __pa(cpu_resume));
 }
 
 static int calxeda_pwrdown_idle(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {
-	highbank_set_cpu_jump(smp_processor_id(), cpu_resume);
 	cpu_suspend(0, calxeda_idle_finish);
 	return index;
 }

@@ -90,7 +62,7 @@ static struct cpuidle_driver calxeda_idle_driver = {
 
 static int __init calxeda_cpuidle_init(void)
 {
-	if (!of_machine_is_compatible("calxeda,highbank"))
+	if (!of_machine_is_compatible("calxeda,highbank") || !psci_ops.cpu_suspend)
 		return -ENODEV;
 
 	return cpuidle_register(&calxeda_idle_driver, NULL);

Quoted 8 lines · Rob Herring

From: Rob Herring <[email protected]>

This updates the Calxeda cpuidle driver to use PSCI calls to powergate cores. This is needed to enable cpuidle for the ECX-2000.

This could possibly become a generic PSCI driver, but there are no other PSCI users in the kernel other than mach-virt.

Quoted 5 lines

Signed-off-by Rob Herring <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc linux-pm at vger.kernel.org

---

Acked-by Daniel Lezcano <[email protected]>

Rob,

Shall I take the patch in my tree ?

Quoted 9 lines · Daniel Lezcano

Quoted 8 lines · Rob Herring

From: Rob Herring <[email protected]>

This updates the Calxeda cpuidle driver to use PSCI calls to powergate cores. This is needed to enable cpuidle for the ECX-2000.

This could possibly become a generic PSCI driver, but there are no other PSCI users in the kernel other than mach-virt.

Quoted 7 lines

Quoted 5 lines

Signed-off-by Rob Herring <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc linux-pm at vger.kernel.org

---

Acked-by Daniel Lezcano <[email protected]>

Thanks.

Quoted 1 line

Quoted 3 lines

Rob,

Shall I take the patch in my tree ?

No, there are interdependencies with the rest of the series, so I plan to send to arm-soc.

From: Rob Herring <[email protected]>

Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are different from the DT binding example because the numbering changed in revisions of the PSCI spec and are already fixed in highbank firmware. Since the numbering is transparent to the kernel, this difference is not significant.

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm/boot/dts/ecx-common.dtsi +0 -0

index e8559b7..f1dfc09 100644
@@ -19,6 +19,16 @@
 		bootargs = "console=ttyAMA0";
 	};
 

Quoted 21 lines · Rob Herring

From: Rob Herring <[email protected]>

Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are different from the DT binding example because the numbering changed in revisions of the PSCI spec and are already fixed in highbank firmware. Since the numbering is transparent to the kernel, this difference is not significant.

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm/boot/dts/ecx-common.dtsi +0 -0

index e8559b7..f1dfc09 100644
@@ -19,6 +19,16 @@
 		bootargs = "console=ttyAMA0";
 	};
 

arch/arm/boot/dts/ecx-common.dtsi line 31

Quoted 10 lines

arch/arm/boot/dts/ecx-common.dtsi +10 -0

+	psci {
+		compatible	= "arm,psci";
+		method		= "smc";
+		cpu_suspend	= <0x84000002>;
+		cpu_off		= <0x84000004>;
+		cpu_on		= <0x84000006>;
+		system_off	= <0x84000100>;
+		system_reset	= <0x84000101>;
+	};
+

I see this is somewhat beyond the original PSCI, given the addition of SYSTEM_OFF and SYSTEM_RESET, but not quite PSCI 0.2 as you don't have PSCI_VERSION.

It would be nice it the compatible string were more specific here to deal with this and any implementation-specific power_state ID values. Something like "calxeda,highbank-psci", "arm,psci" ?

Quoted 22 lines · Mark Rutland

Quoted 21 lines · Rob Herring

From: Rob Herring <[email protected]>

Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are different from the DT binding example because the numbering changed in revisions of the PSCI spec and are already fixed in highbank firmware. Since the numbering is transparent to the kernel, this difference is not significant.

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

arch/arm/boot/dts/ecx-common.dtsi +0 -0

index e8559b7..f1dfc09 100644
@@ -19,6 +19,16 @@
 		bootargs = "console=ttyAMA0";
 	};
 

Quoted 14 lines

arch/arm/boot/dts/ecx-common.dtsi line 31

Quoted 10 lines

arch/arm/boot/dts/ecx-common.dtsi +10 -0

+	psci {
+		compatible	= "arm,psci";
+		method		= "smc";
+		cpu_suspend	= <0x84000002>;
+		cpu_off		= <0x84000004>;
+		cpu_on		= <0x84000006>;
+		system_off	= <0x84000100>;
+		system_reset	= <0x84000101>;
+	};
+

I see this is somewhat beyond the original PSCI, given the addition of SYSTEM_OFF and SYSTEM_RESET, but not quite PSCI 0.2 as you don't have PSCI_VERSION.

I do actually support PSCI_VERSION. Forgot to add that to the dts.

Quoted 4 lines

It would be nice it the compatible string were more specific here to deal with this and any implementation-specific power_state ID values. Something like "calxeda,highbank-psci", "arm,psci" ?

Is that something we always want to do? You can't really know the implementation quirks up front.