[PATCH 0/2] regulator: scmi: add support for registering SCMI regulators by name

[PATCH 0/2] regulator: scmi: add support for registering SCMI regulators by name

[David Collins]

Add support to register SCMI regulator subnodes based on an SCMI
Voltage Domain name specified via the 'regulator-name' device tree
property.  In doing so, make the 'reg' property optional with the
constraint that at least one of 'reg' or 'regulator-name' must be
specified.  If both are specified, then both must match the
Voltage Domain data exposed by the SCMI platform.

Name based SCMI regulator registration helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

David Collins (2):
  dt-bindings: firmware: arm,scmi: define support for name based
    regulators
  regulator: scmi: add support for registering SCMI regulators by name

 .../bindings/firmware/arm,scmi.yaml           | 11 +++-
 drivers/regulator/scmi-regulator.c            | 57 ++++++++++++++++++-
 2 files changed, 62 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[David Collins]

Allow SCMI regulator subnodes to be specified either by ID using
the "reg" property or by name using the "regulator-name" property.

Name based SCMI regulator specification helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

Signed-off-by: David Collins <quic_collinsd@quicinc.com>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml        | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5c4c6782e052..bc4a84fe25d2 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -155,7 +155,7 @@ properties:
           The list of all regulators provided by this SCMI controller.
 
         patternProperties:
-          '^regulators@[0-9a-f]+$':
+          '^regulator.+$':
             type: object
             $ref: "../regulator/regulator.yaml#"
 
@@ -164,8 +164,13 @@ properties:
                 maxItems: 1
                 description: Identifier for the voltage regulator.
 
-            required:
-              - reg
+              regulator-name: true
+
+            anyOf:
+              - required:
+                  - reg
+              - required:
+                  - regulator-name
 
 additionalProperties: false
 
-- 
2.17.1

[PATCH 2/2] regulator: scmi: add support for registering SCMI regulators by name

[David Collins]

Add support to register SCMI regulator subnodes based on an SCMI
Voltage Domain name specified via the 'regulator-name' device tree
property.  In doing so, make the 'reg' property optional with the
constraint that at least one of 'reg' or 'regulator-name' must be
specified.  If both are specified, then both must match the
Voltage Domain data exposed by the SCMI platform.

Name based SCMI regulator registration helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

Signed-off-by: David Collins <quic_collinsd@quicinc.com>
---
 drivers/regulator/scmi-regulator.c | 57 ++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
index 1f02f60ad136..c3287901975e 100644
--- a/drivers/regulator/scmi-regulator.c
+++ b/drivers/regulator/scmi-regulator.c
@@ -31,6 +31,7 @@
 #include <linux/regulator/of_regulator.h>
 #include <linux/scmi_protocol.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/types.h>
 
 static const struct scmi_voltage_proto_ops *voltage_ops;
@@ -252,16 +253,66 @@ static int scmi_regulator_common_init(struct scmi_regulator *sreg)
 	return 0;
 }
 
+static int scmi_regulator_map_name(struct scmi_protocol_handle *ph,
+				   struct scmi_regulator_info *rinfo,
+				   const char *name)
+{
+	const struct scmi_voltage_info *vinfo;
+	int i;
+
+	for (i = 0; i < rinfo->num_doms; i++) {
+		vinfo = voltage_ops->info_get(ph, i);
+		if (!vinfo)
+			continue;
+		if (!strncmp(vinfo->name, name, sizeof(vinfo->name)))
+			return i;
+	}
+
+	return -ENODEV;
+}
+
 static int process_scmi_regulator_of_node(struct scmi_device *sdev,
 					  struct scmi_protocol_handle *ph,
 					  struct device_node *np,
 					  struct scmi_regulator_info *rinfo)
 {
 	u32 dom, ret;
+	int name_dom;
+	const char *name;
 
-	ret = of_property_read_u32(np, "reg", &dom);
-	if (ret)
-		return ret;
+	dom = rinfo->num_doms;
+	if (of_find_property(np, "reg", NULL)) {
+		ret = of_property_read_u32(np, "reg", &dom);
+		if (ret)
+			return ret;
+
+		if (dom >= rinfo->num_doms)
+			return -ENODEV;
+	}
+
+	if (of_find_property(np, "regulator-name", NULL)) {
+		ret = of_property_read_string(np, "regulator-name", &name);
+		if (ret)
+			return ret;
+
+		name_dom = scmi_regulator_map_name(ph, rinfo, name);
+		if (name_dom < 0) {
+			dev_err(&sdev->dev,
+				"No SCMI Voltage Domain found named %s. Skipping: %s\n",
+				name, np->full_name);
+			return name_dom;
+		}
+
+		if (dom >= rinfo->num_doms)
+			dom = name_dom;
+
+		if (name_dom != dom) {
+			dev_err(&sdev->dev,
+				"SCMI Voltage Domain %s ID mismatch, %u (DT) != %d (firmware). Skipping: %s\n",
+				name, dom, name_dom, np->full_name);
+			return -EINVAL;
+		}
+	}
 
 	if (dom >= rinfo->num_doms)
 		return -ENODEV;
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[Rob Herring]

On Thu, 09 Dec 2021 17:54:41 -0800, David Collins wrote:
> Allow SCMI regulator subnodes to be specified either by ID using
> the "reg" property or by name using the "regulator-name" property.
> 
> Name based SCMI regulator specification helps ensure that an SCMI
> agent doesn't need to be aware of the numbering scheme used for
> Voltage Domains by the SCMI platform.  It also ensures that the
> correct Voltage Domain is selected for a given physical regulator.
> This cannot be guaranteed with numeric Voltage Domain IDs alone.
> 
> Signed-off-by: David Collins <quic_collinsd@quicinc.com>
> ---
>  .../devicetree/bindings/firmware/arm,scmi.yaml        | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[RESEND PATCH 0/2] regulator: scmi: add support for registering SCMI regulators by name

[David Collins]

Add support to register SCMI regulator subnodes based on an SCMI
Voltage Domain name specified via the 'regulator-name' device tree
property.  In doing so, make the 'reg' property optional with the
constraint that at least one of 'reg' or 'regulator-name' must be
specified.  If both are specified, then both must match the
Voltage Domain data exposed by the SCMI platform.

Name based SCMI regulator registration helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

David Collins (2):
  dt-bindings: firmware: arm,scmi: define support for name based
    regulators
  regulator: scmi: add support for registering SCMI regulators by name

 .../bindings/firmware/arm,scmi.yaml           | 11 +++-
 drivers/regulator/scmi-regulator.c            | 57 ++++++++++++++++++-
 2 files changed, 62 insertions(+), 6 deletions(-)

-- 
2.17.1

[RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[David Collins]

Allow SCMI regulator subnodes to be specified either by ID using
the "reg" property or by name using the "regulator-name" property.

Name based SCMI regulator specification helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

Signed-off-by: David Collins <quic_collinsd@quicinc.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml        | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5c4c6782e052..bc4a84fe25d2 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -155,7 +155,7 @@ properties:
           The list of all regulators provided by this SCMI controller.
 
         patternProperties:
-          '^regulators@[0-9a-f]+$':
+          '^regulator.+$':
             type: object
             $ref: "../regulator/regulator.yaml#"
 
@@ -164,8 +164,13 @@ properties:
                 maxItems: 1
                 description: Identifier for the voltage regulator.
 
-            required:
-              - reg
+              regulator-name: true
+
+            anyOf:
+              - required:
+                  - reg
+              - required:
+                  - regulator-name
 
 additionalProperties: false
 
-- 
2.17.1

[RESEND PATCH 2/2] regulator: scmi: add support for registering SCMI regulators by name

[David Collins]

Add support to register SCMI regulator subnodes based on an SCMI
Voltage Domain name specified via the 'regulator-name' device tree
property.  In doing so, make the 'reg' property optional with the
constraint that at least one of 'reg' or 'regulator-name' must be
specified.  If both are specified, then both must match the
Voltage Domain data exposed by the SCMI platform.

Name based SCMI regulator registration helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

Signed-off-by: David Collins <quic_collinsd@quicinc.com>
---
 drivers/regulator/scmi-regulator.c | 57 ++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
index 1f02f60ad136..c3287901975e 100644
--- a/drivers/regulator/scmi-regulator.c
+++ b/drivers/regulator/scmi-regulator.c
@@ -31,6 +31,7 @@
 #include <linux/regulator/of_regulator.h>
 #include <linux/scmi_protocol.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/types.h>
 
 static const struct scmi_voltage_proto_ops *voltage_ops;
@@ -252,16 +253,66 @@ static int scmi_regulator_common_init(struct scmi_regulator *sreg)
 	return 0;
 }
 
+static int scmi_regulator_map_name(struct scmi_protocol_handle *ph,
+				   struct scmi_regulator_info *rinfo,
+				   const char *name)
+{
+	const struct scmi_voltage_info *vinfo;
+	int i;
+
+	for (i = 0; i < rinfo->num_doms; i++) {
+		vinfo = voltage_ops->info_get(ph, i);
+		if (!vinfo)
+			continue;
+		if (!strncmp(vinfo->name, name, sizeof(vinfo->name)))
+			return i;
+	}
+
+	return -ENODEV;
+}
+
 static int process_scmi_regulator_of_node(struct scmi_device *sdev,
 					  struct scmi_protocol_handle *ph,
 					  struct device_node *np,
 					  struct scmi_regulator_info *rinfo)
 {
 	u32 dom, ret;
+	int name_dom;
+	const char *name;
 
-	ret = of_property_read_u32(np, "reg", &dom);
-	if (ret)
-		return ret;
+	dom = rinfo->num_doms;
+	if (of_find_property(np, "reg", NULL)) {
+		ret = of_property_read_u32(np, "reg", &dom);
+		if (ret)
+			return ret;
+
+		if (dom >= rinfo->num_doms)
+			return -ENODEV;
+	}
+
+	if (of_find_property(np, "regulator-name", NULL)) {
+		ret = of_property_read_string(np, "regulator-name", &name);
+		if (ret)
+			return ret;
+
+		name_dom = scmi_regulator_map_name(ph, rinfo, name);
+		if (name_dom < 0) {
+			dev_err(&sdev->dev,
+				"No SCMI Voltage Domain found named %s. Skipping: %s\n",
+				name, np->full_name);
+			return name_dom;
+		}
+
+		if (dom >= rinfo->num_doms)
+			dom = name_dom;
+
+		if (name_dom != dom) {
+			dev_err(&sdev->dev,
+				"SCMI Voltage Domain %s ID mismatch, %u (DT) != %d (firmware). Skipping: %s\n",
+				name, dom, name_dom, np->full_name);
+			return -EINVAL;
+		}
+	}
 
 	if (dom >= rinfo->num_doms)
 		return -ENODEV;
-- 
2.17.1

Re: [RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[Mark Brown]

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

On Wed, Feb 02, 2022 at 04:09:43PM -0800, David Collins wrote:
> On 2/2/22 9:38 AM, Mark Brown wrote:

> > This seems like a scenario where the DT should be being generated at
> > runtime along with the virtualisation of the platform?  TBH a setup
> > where this is an issue feels like it's asking for trouble.

> I'm not familiar with runtime device tree generation.  Could you please
> point to an example of it or documentation for it?  How would this
> handle kernel devices on the VM side which need a phandle to an
> scmi-regulator DT subnode in order to get a pointer to the corresponding
> regulator device at runtime via devm_regulator_get()?

I believe qemu does this for the virt machine.  I'm not sure what you're
seeing as particularly complex about generating regulator links in
particular though?

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

Re: [RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[David Collins]

On 2/2/22 9:38 AM, Mark Brown wrote:
>> The system layout that this patch is targeted for consists of an SCMI
>> platform implemented in software in the primary Linux OS on the
>> application processor and an SCMI agent in a guest VM (also running
>> Linux).  This provides paravirtualized regulator control to the guest VM
>> where full virtualization is not supported.
> 
>> During the course of development of these software images, it may be
>> necessary to add or reorder the set of SCMI voltage domains (regulators)
>> implemented on the platform side.  If the voltage domains are only
>> identified and matched based on the ID number, then it is easy for the
>> platform and agent to get out of sync.
> 
>> Using the voltage domain name instead of ID number for identification
>> and matching provides robust assurance of correct regulator usage in the
>> face of domains being added, removed, or reordered on the platform side.
> 
> This seems like a scenario where the DT should be being generated at
> runtime along with the virtualisation of the platform?  TBH a setup
> where this is an issue feels like it's asking for trouble.

I'm not familiar with runtime device tree generation.  Could you please
point to an example of it or documentation for it?  How would this
handle kernel devices on the VM side which need a phandle to an
scmi-regulator DT subnode in order to get a pointer to the corresponding
regulator device at runtime via devm_regulator_get()?

>>>> +              regulator-name: true
> 
>>> This is abusing the existing regulator-name property which is there to
>>> allow a human readable descriptive string to be attached to a regulator.
>>> It should have no effect other than being included in diagnostic output.
> 
>> Would you be ok with a new DT property being added in place of
>> "regulator-name" in this patch which serves the same matching purpose
>> (perhaps "arm,scmi-domain-name")?
> 
> Yes, it needs to be a new property.

Ok, I'll modify the series in V2 to use a different property.

Thanks,
David

Re: [RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[Mark Brown]

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

On Fri, Jan 28, 2022 at 03:09:30PM -0800, David Collins wrote:
> On 1/28/22 11:32 AM, Mark Brown wrote:
> > On Mon, Jan 24, 2022 at 04:27:35PM -0800, David Collins wrote:

> >> Name based SCMI regulator specification helps ensure that an SCMI
> >> agent doesn't need to be aware of the numbering scheme used for

> > What is a "SCMI agent" in this context?  This is changing how the DT
> > bindings are specified, at some point things are going to need to be
> > hard coded.

> The system layout that this patch is targeted for consists of an SCMI
> platform implemented in software in the primary Linux OS on the
> application processor and an SCMI agent in a guest VM (also running
> Linux).  This provides paravirtualized regulator control to the guest VM
> where full virtualization is not supported.

> During the course of development of these software images, it may be
> necessary to add or reorder the set of SCMI voltage domains (regulators)
> implemented on the platform side.  If the voltage domains are only
> identified and matched based on the ID number, then it is easy for the
> platform and agent to get out of sync.

> Using the voltage domain name instead of ID number for identification
> and matching provides robust assurance of correct regulator usage in the
> face of domains being added, removed, or reordered on the platform side.

This seems like a scenario where the DT should be being generated at
runtime along with the virtualisation of the platform?  TBH a setup
where this is an issue feels like it's asking for trouble.

> >> +              regulator-name: true

> > This is abusing the existing regulator-name property which is there to
> > allow a human readable descriptive string to be attached to a regulator.
> > It should have no effect other than being included in diagnostic output.

> Would you be ok with a new DT property being added in place of
> "regulator-name" in this patch which serves the same matching purpose
> (perhaps "arm,scmi-domain-name")?

Yes, it needs to be a new property.

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

Re: [RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[David Collins]

On 1/28/22 11:32 AM, Mark Brown wrote:
> On Mon, Jan 24, 2022 at 04:27:35PM -0800, David Collins wrote:
> 
>> Name based SCMI regulator specification helps ensure that an SCMI
>> agent doesn't need to be aware of the numbering scheme used for
> 
> What is a "SCMI agent" in this context?  This is changing how the DT
> bindings are specified, at some point things are going to need to be
> hard coded.

An SCMI agent is the entity that issues SCMI commands (i.e. the
consumer).  An SCMI platform is the entity that receives the SCMI
commands and performs the necessary operations (i.e. the provider).
This is the terminology used in the ARM SCMI spec [1].

A typical system layout could have an agent that is the application
processor (running Linux) and a platform that is an embedded controller.

The system layout that this patch is targeted for consists of an SCMI
platform implemented in software in the primary Linux OS on the
application processor and an SCMI agent in a guest VM (also running
Linux).  This provides paravirtualized regulator control to the guest VM
where full virtualization is not supported.

During the course of development of these software images, it may be
necessary to add or reorder the set of SCMI voltage domains (regulators)
implemented on the platform side.  If the voltage domains are only
identified and matched based on the ID number, then it is easy for the
platform and agent to get out of sync.

Using the voltage domain name instead of ID number for identification
and matching provides robust assurance of correct regulator usage in the
face of domains being added, removed, or reordered on the platform side.


>> +              regulator-name: true
>> +
>> +            anyOf:
>> +              - required:
>> +                  - reg
>> +              - required:
>> +                  - regulator-name
> 
> This is abusing the existing regulator-name property which is there to
> allow a human readable descriptive string to be attached to a regulator.
> It should have no effect other than being included in diagnostic output.

Would you be ok with a new DT property being added in place of
"regulator-name" in this patch which serves the same matching purpose
(perhaps "arm,scmi-domain-name")?

Thanks,
David

[1]: https://developer.arm.com/documentation/den0056/latest

Re: [RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[Mark Brown]

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

On Mon, Jan 24, 2022 at 04:27:35PM -0800, David Collins wrote:

> Name based SCMI regulator specification helps ensure that an SCMI
> agent doesn't need to be aware of the numbering scheme used for

What is a "SCMI agent" in this context?  This is changing how the DT
bindings are specified, at some point things are going to need to be
hard coded.

> +              regulator-name: true
> +
> +            anyOf:
> +              - required:
> +                  - reg
> +              - required:
> +                  - regulator-name

This is abusing the existing regulator-name property which is there to
allow a human readable descriptive string to be attached to a regulator.
It should have no effect other than being included in diagnostic output.

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

[RESEND PATCH 1/2] dt-bindings: firmware: arm,scmi: define support for name based regulators

[David Collins]

Allow SCMI regulator subnodes to be specified either by ID using
the "reg" property or by name using the "regulator-name" property.

Name based SCMI regulator specification helps ensure that an SCMI
agent doesn't need to be aware of the numbering scheme used for
Voltage Domains by the SCMI platform.  It also ensures that the
correct Voltage Domain is selected for a given physical regulator.
This cannot be guaranteed with numeric Voltage Domain IDs alone.

Signed-off-by: David Collins <quic_collinsd@quicinc.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/firmware/arm,scmi.yaml        | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5c4c6782e052..bc4a84fe25d2 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -155,7 +155,7 @@ properties:
           The list of all regulators provided by this SCMI controller.
 
         patternProperties:
-          '^regulators@[0-9a-f]+$':
+          '^regulator.+$':
             type: object
             $ref: "../regulator/regulator.yaml#"
 
@@ -164,8 +164,13 @@ properties:
                 maxItems: 1
                 description: Identifier for the voltage regulator.
 
-            required:
-              - reg
+              regulator-name: true
+
+            anyOf:
+              - required:
+                  - reg
+              - required:
+                  - regulator-name
 
 additionalProperties: false
 
-- 
2.17.1