diff mbox series

[RFC] dt-bindings: firmware: scmi: Introduce compatible string

Message ID 20250226094456.2351571-1-peng.fan@oss.nxp.com (mailing list archive)
State New
Headers show
Series [RFC] dt-bindings: firmware: scmi: Introduce compatible string | expand

Commit Message

Peng Fan (OSS) Feb. 26, 2025, 9:44 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Add compatible string for the protocols by adding new nodes
The current nodename pattern is "protocol@[0-9a-f]+$", the new node
name will be "scmi-[a-z\-]+$".
With compatible string and new nodename, cpufreq and devfreq could be
separated into two nodes. And fwdevlink could correctly link suppliers
and consumers.
With compatible string, and driver updated.
- Differnet vendor drivers with same SCMI protocol ID could be built in
  without concerning vendor A's driver got probed when using vendor B's
  SoC
- NXP scmi pinctrl and ARM scmi pinctrl could be both built in, without
  concerning arm scmi platform takes nxp scmi pinctrl node as supplier.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

RFC:
 This may sounds like that adding compatible to resovle linux driver issue.
 Yes indeed. current scmi framework limitation makes it not work well with
 fwdevlink, wrong suppliers maybe linked to consumers.
 I have tried various's method to not introduce compatible, but rejected by
 fwdevlink maintainer or scmi maintainer
 There was a long discussion in [1][2][3].
 [1] https://lore.kernel.org/arm-scmi/20240729070325.2065286-1-peng.fan@oss.nxp.com/
 [2] https://lore.kernel.org/arm-scmi/20241225-scmi-fwdevlink-v1-0-e9a3a5341362@nxp.com/T/#mdd17c4b9b11af9fae0d5b6ec2e13756c2c6f977d
 [3] https://lore.kernel.org/arm-scmi/20250120-scmi-fwdevlink-v2-0-3af2fa37dbac@nxp.com/

 The binding changes are posted out to see whether DT maintainer's view on
 whether introduce compatible string is welcomed or not.
 I not include driver changes, because this is just to see whether people
 are happy with this or not.

Quote Sudeep's reply"
I am not blocking you. What I mentioned is I don't agree that DT can be used
to resolve this issue, but I don't have time or alternate solution ATM. So
if you propose DT based solution and the maintainers agree for the proposed
bindings I will take a look and help you to make that work. But I will raise
any objections I may have if the proposal has issues mainly around the
compatibility and ease of maintenance.
"

 So If compatible string is agreed, I could continue
 update driver part in next version or SCMI maintainer could help.

 .../bindings/firmware/arm,scmi.yaml           | 300 ++++++++++++++++--
 .../firmware/nxp,imx95-scmi-pinctrl.yaml      |   4 +
 .../bindings/firmware/nxp,imx95-scmi.yaml     |   6 +
 3 files changed, 275 insertions(+), 35 deletions(-)

Comments

Rob Herring (Arm) Feb. 26, 2025, 4:09 p.m. UTC | #1 
On Wed, Feb 26, 2025 at 05:44:56PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add compatible string for the protocols by adding new nodes
> The current nodename pattern is "protocol@[0-9a-f]+$", the new node
> name will be "scmi-[a-z\-]+$".
> With compatible string and new nodename, cpufreq and devfreq could be
> separated into two nodes. And fwdevlink could correctly link suppliers
> and consumers.
> With compatible string, and driver updated.
> - Differnet vendor drivers with same SCMI protocol ID could be built in
>   without concerning vendor A's driver got probed when using vendor B's
>   SoC
> - NXP scmi pinctrl and ARM scmi pinctrl could be both built in, without
>   concerning arm scmi platform takes nxp scmi pinctrl node as supplier.

How are you going to handle DTs which aren't updated and still don't 
have compatible strings? Seems like that would be messy if not 
impossible.

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> RFC:
>  This may sounds like that adding compatible to resovle linux driver issue.
>  Yes indeed. current scmi framework limitation makes it not work well with
>  fwdevlink, wrong suppliers maybe linked to consumers.
>  I have tried various's method to not introduce compatible, but rejected by
>  fwdevlink maintainer or scmi maintainer
>  There was a long discussion in [1][2][3].
>  [1] https://lore.kernel.org/arm-scmi/20240729070325.2065286-1-peng.fan@oss.nxp.com/
>  [2] https://lore.kernel.org/arm-scmi/20241225-scmi-fwdevlink-v1-0-e9a3a5341362@nxp.com/T/#mdd17c4b9b11af9fae0d5b6ec2e13756c2c6f977d
>  [3] https://lore.kernel.org/arm-scmi/20250120-scmi-fwdevlink-v2-0-3af2fa37dbac@nxp.com/
> 
>  The binding changes are posted out to see whether DT maintainer's view on
>  whether introduce compatible string is welcomed or not.
>  I not include driver changes, because this is just to see whether people
>  are happy with this or not.
> 
> Quote Sudeep's reply"
> I am not blocking you. What I mentioned is I don't agree that DT can be used
> to resolve this issue, but I don't have time or alternate solution ATM. So
> if you propose DT based solution and the maintainers agree for the proposed
> bindings I will take a look and help you to make that work. But I will raise
> any objections I may have if the proposal has issues mainly around the
> compatibility and ease of maintenance.
> "

This all looks to me like SCMI has failed to provide common interfaces.

I'm indifferent. If everyone involved thinks adding compatibles will 
solve whatever the issues are, then it's going to be fine with me 
(other than the issue above). It doesn't seem like you have that, so I 
don't know that I'd keep going down this path.

Rob
Sudeep Holla Feb. 26, 2025, 5:19 p.m. UTC | #2 
On Wed, Feb 26, 2025 at 10:09:45AM -0600, Rob Herring wrote:
> On Wed, Feb 26, 2025 at 05:44:56PM +0800, Peng Fan (OSS) wrote:
> > Quote Sudeep's reply"
> > I am not blocking you. What I mentioned is I don't agree that DT can be used
> > to resolve this issue, but I don't have time or alternate solution ATM. So
> > if you propose DT based solution and the maintainers agree for the proposed
> > bindings I will take a look and help you to make that work. But I will raise
> > any objections I may have if the proposal has issues mainly around the
> > compatibility and ease of maintenance.
> > "
>
> This all looks to me like SCMI has failed to provide common interfaces.
>

We can look into this if having such common interface can solve this problem.

> I'm indifferent. If everyone involved thinks adding compatibles will
> solve whatever the issues are, then it's going to be fine with me
> (other than the issue above). It doesn't seem like you have that, so I
> don't know that I'd keep going down this path.

Sorry if I was ambiguous with my stance as quoted above. For me, 2 devices
pointing to the same node seems implementation issue rather than fixing/
working around by extending DT bindings like this $subject patch is
attempting.

If you disagree with that and think 2 devices in the kernel shouldn't
point to the same device tree node, then yes I see this is right approach
to take. ATM I don't know which is correct and what are other developer's
include DT maintainer opinion on this. I just didn't like the way Peng
was trying to solve it with some block/allow list which wouldn't have
fixed the issue or just created new ones.

--
Regards,
Sudeep
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index abbd62f1fed0..3c6ac8ab762d 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -156,6 +156,207 @@  properties:
     description:
       Channel specifier required when using OP-TEE transport.
 
+  scmi-clock:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-clock
+
+      '#clock-cells':
+        const: 1
+
+    required:
+      - '#clock-cells'
+      - compatible
+
+  scmi-cpufreq:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-cpufreq
+
+      '#clock-cells':
+        const: 1
+
+      '#power-domain-cells':
+        const: 1
+
+    oneOf:
+      - required:
+          - '#clock-cells'
+          - compatible
+
+      - required:
+          - '#power-domain-cells'
+          - compatible
+
+  scmi-hwmon:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-hwmon
+
+      '#thermal-sensor-cells':
+        const: 1
+
+    required:
+      - '#thermal-sensor-cells'
+      - compatible
+
+  scmi-iiodev:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-iiodev
+
+    required:
+      - compatible
+
+  scmi-pinctrl:
+    type: object
+    allOf:
+      - $ref: '#/$defs/protocol-node'
+      - anyOf:
+          - $ref: /schemas/pinctrl/pinctrl.yaml
+          - $ref: /schemas/firmware/nxp,imx95-scmi-pinctrl.yaml
+
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-pinctrl
+
+    patternProperties:
+      '-pins$':
+        type: object
+        allOf:
+          - $ref: /schemas/pinctrl/pincfg-node.yaml#
+          - $ref: /schemas/pinctrl/pinmux-node.yaml#
+        unevaluatedProperties: false
+
+        description:
+          A pin multiplexing sub-node describes how to configure a
+          set of pins in some desired function.
+          A single sub-node may define several pin configurations.
+          This sub-node is using the default pinctrl bindings to configure
+          pin multiplexing and using SCMI protocol to apply a specified
+          configuration.
+
+    required:
+      - reg
+
+  scmi-power-domain:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-power-domain
+
+      '#power-domain-cells':
+        const: 1
+
+    required:
+      - compatible
+      - '#power-domain-cells'
+
+  scmi-perf:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-perf
+
+      '#power-domain-cells':
+        const: 1
+
+    required:
+      - '#power-domain-cells'
+      - compatible
+
+  scmi-powercap:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-powercap
+
+    required:
+      - compatible
+
+  scmi-regulator:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-regulator
+
+      regulators:
+        type: object
+        additionalProperties: false
+        description:
+          The list of all regulators provided by this SCMI controller.
+
+        properties:
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+        patternProperties:
+          '^regulator@[0-9a-f]+$':
+            type: object
+            $ref: /schemas/regulator/regulator.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              reg:
+                maxItems: 1
+                description: Identifier for the voltage regulator.
+
+            required:
+              - reg
+    required:
+      - compatible
+
+  scmi-reset:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-reset
+
+      '#reset-cells':
+        const: 1
+
+    required:
+      - '#reset-cells'
+      - compatible
+
+  scmi-syspower:
+    $ref: '#/$defs/protocol-node'
+    unevaluatedProperties: false
+
+    properties:
+      compatible:
+        const: arm,scmi-syspower
+
+    required:
+      - compatible
+
   protocol@11:
     $ref: '#/$defs/protocol-node'
     unevaluatedProperties: false
@@ -169,6 +370,7 @@  properties:
 
     required:
       - '#power-domain-cells'
+      - reg
 
   protocol@12:
     $ref: '#/$defs/protocol-node'
@@ -178,6 +380,9 @@  properties:
       reg:
         const: 0x12
 
+    required:
+      - reg
+
   protocol@13:
     $ref: '#/$defs/protocol-node'
     unevaluatedProperties: false
@@ -195,9 +400,11 @@  properties:
     oneOf:
       - required:
           - '#clock-cells'
+          - reg
 
       - required:
           - '#power-domain-cells'
+          - reg
 
   protocol@14:
     $ref: '#/$defs/protocol-node'
@@ -212,6 +419,7 @@  properties:
 
     required:
       - '#clock-cells'
+      - reg
 
   protocol@15:
     $ref: '#/$defs/protocol-node'
@@ -226,6 +434,7 @@  properties:
 
     required:
       - '#thermal-sensor-cells'
+      - reg
 
   protocol@16:
     $ref: '#/$defs/protocol-node'
@@ -240,6 +449,7 @@  properties:
 
     required:
       - '#reset-cells'
+      - reg
 
   protocol@17:
     $ref: '#/$defs/protocol-node'
@@ -275,6 +485,8 @@  properties:
 
             required:
               - reg
+    required:
+      - reg
 
   protocol@18:
     $ref: '#/$defs/protocol-node'
@@ -284,6 +496,9 @@  properties:
       reg:
         const: 0x18
 
+    required:
+      - reg
+
   protocol@19:
     type: object
     allOf:
@@ -358,49 +573,64 @@  $defs:
           Channel specifier required when using OP-TEE transport and
           protocol has a dedicated communication channel.
 
-    required:
-      - reg
-
 required:
   - compatible
 
-if:
-  properties:
-    compatible:
-      contains:
-        const: arm,scmi
-then:
-  properties:
-    interrupts: false
-    interrupt-names: false
-
-  required:
-    - mboxes
-    - shmem
-
-else:
-  if:
-    properties:
-      compatible:
-        contains:
-          enum:
-            - arm,scmi-smc
-            - arm,scmi-smc-param
-            - qcom,scmi-smc
-  then:
-    required:
-      - arm,smc-id
-      - shmem
-
-  else:
-    if:
+allOf:
+  - if:
       properties:
         compatible:
           contains:
-            const: linaro,scmi-optee
+            const: arm,scmi
     then:
+      properties:
+        interrupts: false
+        interrupt-names: false
+
       required:
-        - linaro,optee-channel-id
+        - mboxes
+        - shmem
+
+    else:
+      if:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - arm,scmi-smc
+                - arm,scmi-smc-param
+                - qcom,scmi-smc
+      then:
+        required:
+          - arm,smc-id
+          - shmem
+
+      else:
+        if:
+          properties:
+            compatible:
+              contains:
+                const: linaro,scmi-optee
+        then:
+          required:
+            - linaro,optee-channel-id
+
+  - if:
+      anyOf:
+        - required: [ scmi-clock ]
+        - required: [ scmi-cpufreq ]
+        - required: [ scmi-hwmon ]
+        - required: [ scmi-iiodev ]
+        - required: [ scmi-regulator ]
+        - required: [ scmi-perf ]
+        - required: [ scmi-powercap ]
+        - required: [ scmi-power-domain ]
+        - required: [ scmi-pinctrl ]
+        - required: [ scmi-reset ]
+        - required: [ scmi-syspower]
+    then:
+      patternProperties:
+        protocol@[0-9a-f]+$: false
 
 examples:
   - |
diff --git a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml
index a96fc6cce502..e9c08f1577da 100644
--- a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi-pinctrl.yaml
@@ -13,6 +13,10 @@  maintainers:
 allOf:
   - $ref: /schemas/pinctrl/pinctrl.yaml
 
+properties:
+  compatible:
+    const: nxp,imx95-scmi-pinctrl
+
 patternProperties:
   'grp$':
     type: object
diff --git a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi.yaml b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi.yaml
index 1a95010a546b..048db78c9887 100644
--- a/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/nxp,imx95-scmi.yaml
@@ -19,6 +19,9 @@  properties:
       reg:
         const: 0x81
 
+    required:
+      - reg
+
   protocol@84:
     $ref: '/schemas/firmware/arm,scmi.yaml#/$defs/protocol-node'
     unevaluatedProperties: false
@@ -40,4 +43,7 @@  properties:
         maxItems: 16
         $ref: /schemas/types.yaml#/definitions/uint32-matrix
 
+    required:
+      - reg
+
 additionalProperties: true