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

[PATCH v2 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 "arm,scmi-domain-name" device
tree property.  In doing so, make the "reg" property optional with
the constraint that at least one of "reg" or "arm,scmi-domain-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.

Changes in v2:
- Replaced usage of DT property "regulator-name" with "arm,scmi-domain-name".

v1 of this patch series can be found at [1].

[1]: https://lore.kernel.org/lkml/cover.1643069954.git.quic_collinsd@quicinc.com/T/

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           | 15 ++++-
 drivers/regulator/scmi-regulator.c            | 58 ++++++++++++++++++-
 2 files changed, 67 insertions(+), 6 deletions(-)

-- 
2.17.1


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

[PATCH v2 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 "arm,scmi-domain-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    | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5c4c6782e052..08cb5de967ac 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,17 @@ properties:
                 maxItems: 1
                 description: Identifier for the voltage regulator.
 
-            required:
-              - reg
+              arm,scmi-domain-name:
+                description:
+                  A string matching the name of the SCMI voltage domain for this
+                  regulator.
+                $ref: "/schemas/types.yaml#/definitions/string"
+
+            anyOf:
+              - required:
+                  - reg
+              - required:
+                  - arm,scmi-domain-name
 
 additionalProperties: false
 
-- 
2.17.1


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

[PATCH v2 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 "arm,scmi-domain-name" device
tree property.  In doing so, make the "reg" property optional with
the constraint that at least one of "reg" or "arm,scmi-domain-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 | 58 ++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c
index 1f02f60ad136..15f9167b96ab 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,67 @@ 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, "arm,scmi-domain-name", NULL)) {
+		ret = of_property_read_string(np, "arm,scmi-domain-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


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

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

[Sudeep Holla]

On Mon, Mar 21, 2022 at 05:47:18PM -0700, David Collins wrote:
> Add support to register SCMI regulator subnodes based on an SCMI
> Voltage Domain name specified via the "arm,scmi-domain-name" device
> tree property.  In doing so, make the "reg" property optional with
> the constraint that at least one of "reg" or "arm,scmi-domain-name"
> must be specified.  If both are specified, then both must match the
> Voltage Domain data exposed by the SCMI platform.
>

Since the SCMI specification allows discovery of names based on the
numbering scheme, we haven't added support for the name explicitly.
However, I have heard/received couple of such feedback to provide
name based access in private so far. So good to have this discussion
in public.

> 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.

While I understand the regulator framework has a good support for name
based approach you prefer, I see other frameworks like clock rely on
numbering scheme and I see quite a few qualcomm platforms upstream use
the number scheme for clocks. So why is that a problem with regulator ?

Another main issue I have is what if the firmware and DT end up with a
mismatch say with a firmware upgrade or a DT update ? Basically out of sync
between DT and the SCMI firmware. I see this as duplication of source of
information and is always cause for the trouble. I don't want to end up with
the quirks to deal with (totally unnecessary) issues this may create in long
run.

> It also ensures that the
> correct Voltage Domain is selected for a given physical regulator.

How is that done magically if I give wrong regulator name ? Sorry the way
it is presented sounds like adding name fixes something that numerical
ID alone will always break.

> This cannot be guaranteed with numeric Voltage Domain IDs alone.
>

If the IDs are correct like the names, it is guaranteed. I see this
ID vs name is more for some maintenance convenience because somewhere
something else needs to changes or moved away from existing way of
maintenance.

That said, if others believe, this is useful, I am happy to consider
esp. if there are more *real* reasons for doing this.

Please add clock and other subsystem maintainers who also have numbering
scheme as main mechanism in the upstream so that we get feedback from them
too.

-- 
Regards,
Sudeep

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

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

[David Collins]

On 3/22/22 4:40 AM, Sudeep Holla wrote:
> On Mon, Mar 21, 2022 at 05:47:18PM -0700, David Collins wrote:
>> 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.
> 
> While I understand the regulator framework has a good support for name
> based approach youasdf prefer, I see other frameworks like clock rely on
> numbering scheme and I see quite a few qualcomm platforms upstream use
> the number scheme for clocks. So why is that a problem with regulator ?

I assume that the clocks you are referring to in upstream targets are
explicitly managed as opposed to mediated by SCMI.  In that case,
consumer devices in device tree reference particular clocks using a
tuple consisting of <&phandle_of_clock_controller clock_id>.  The
clock_id value is in a numbering space that is unique to each clock
controller.  The ID numbers are #define'd in header files shared by DT
and the clock drivers.  As far as I know, no Qualcomm targets utilize
SCMI clocks (either as a platform or agent).

Supporting clocks in Linux using SCMI has its own set of challenges.
One is that the there can only be one clk-scmi device on the agent side
(associated with the platform) and thus all of the clocks it exposes
must be in the same numbering space.  In the case where the platform is
another Linux instance, this presents a mismatch as each of its many
clock controllers has its own numbering space.

Another problem is that, as with regulators, ID numbers could
unknowingly get out of sync between the platform and the agent.  Using
clock domain names for referencing fixes both issues.  This can be
accomplished by defining wrapper clock controller devices on the agent
which define all of the clocks and specify their parent by name (which
matches a clock exposed by the clk-scmi driver).

> Another main issue I have is what if the firmware and DT end up with a
> mismatch say with a firmware upgrade or a DT update ? Basically out of sync
> between DT and the SCMI firmware. I see this as duplication of source of
> information and is always cause for the trouble. I don't want to end up with
> the quirks to deal with (totally unnecessary) issues this may create in long
> run.

This patch series is specifically intended to address the issue of
firmware (SCMI platform) configurations getting out of sync with DT
(SCMI agent) configurations where the mapping of regulators to ID
numbers isn't correctly matched.

This change allows the existing 'reg' ID number based identification of
scmi-regulator subnodes to continue without issue.  Systems don't need
to use the proposed 'arm,scmi-domain-name' property if they'd prefer to
stay with 'reg' instead.  Also, both can be specified for added
assurance if desired.

>> It also ensures that the
>> correct Voltage Domain is selected for a given physical regulator.
> 
> How is that done magically if I give wrong regulator name ? Sorry the way
> it is presented sounds like adding name fixes something that numerical
> ID alone will always break.

If an scmi-regulator subnode on the SCMI agent side specifies an
'arm,scmi-domain-name' property value which does not match the name of
any voltage domain exposed by the SCMI platform, then that regulator
will not be registered at runtime.  The only way an scmi-regulator
subnode gets registered as a Linux regulator device is if there is a
positive name match.

The name string for a regulator has an explicit meaning that clearly
maps it to a particular physical regulator without the need for any
additional context.  In a non-trivial system composed of multiple PMICs
each with multiple regulators of different types, there is no single
numbering system that intuitively and unambiguously captures the mapping
of an ID number to a physical regulator.  Such ID numbers have no
explicit meaning in the context of physical regulator identification.
Therefore, some other information is required to map the ID numbers to
physical regulators (e.g. #define constants in a header file).  This
mapping must then somehow be shared across domains (i.e. by the platform
and agent) and always change in lock-step.

>> This cannot be guaranteed with numeric Voltage Domain IDs alone.
>>
> 
> If the IDs are correct like the names, it is guaranteed. I see this
> ID vs name is more for some maintenance convenience because somewhere
> something else needs to changes or moved away from existing way of
> maintenance.

How do you quantify an ID number to physical regulator mapping as
"correct"?  What happens if the mapping must be changed on the SCMI
platform side (e.g. a PMIC was added or removed, or the order that
regulators are listed in needs to change)?  If the SCMI agent is blindly
identifying regulators solely by ID number, then it has no idea that
anything has changed on the platform side.  If instead the agent is
using names for identification of SCMI voltage domains then reordering
IDs or adding new regulators has no impact.  Removing regulators from
the platform side would correctly lead to the regulator not registering
on the agent side.

> That said, if others believe, this is useful, I am happy to consider
> esp. if there are more *real* reasons for doing this.
> 
> Please add clock and other subsystem maintainers who also have numbering
> scheme as main mechanism in the upstream so that we get feedback from them
> too.

Done.

Thanks,
David

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

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1516 bytes --]

On Tue, Mar 22, 2022 at 06:12:33PM -0700, David Collins wrote:

> Another problem is that, as with regulators, ID numbers could
> unknowingly get out of sync between the platform and the agent.  Using
> clock domain names for referencing fixes both issues.  This can be

This is just saying that the hard coded IDs that the firmware and kernel
use to communicate can get out of sync which is true no matter if those
IDs are strings or if they're numerical, either way it's an ABI which
can be broken.

> > If the IDs are correct like the names, it is guaranteed. I see this
> > ID vs name is more for some maintenance convenience because somewhere
> > something else needs to changes or moved away from existing way of
> > maintenance.

> How do you quantify an ID number to physical regulator mapping as
> "correct"?  What happens if the mapping must be changed on the SCMI
> platform side (e.g. a PMIC was added or removed, or the order that
> regulators are listed in needs to change)?  If the SCMI agent is blindly

The whole point with the numbers being an ABI is that things must never
be renumbered, just as if names are used the names can't be changed.  If
the numbering is changing that just sounds like bugs on the platform
side.  There's an implicit assumption in what you've written above that
implementation details of the firmware should affect the IDs presented
through SCMI which simply shouldn't be true, and indeed if the firmware
can assign fixed strings it can just as well assign fixed numbers.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Cristian Marussi]

On Thu, Mar 24, 2022 at 05:23:05PM +0000, Mark Brown wrote:
> On Tue, Mar 22, 2022 at 06:12:33PM -0700, David Collins wrote:
> 
> > Another problem is that, as with regulators, ID numbers could
> > unknowingly get out of sync between the platform and the agent.  Using
> > clock domain names for referencing fixes both issues.  This can be
> 
> This is just saying that the hard coded IDs that the firmware and kernel
> use to communicate can get out of sync which is true no matter if those
> IDs are strings or if they're numerical, either way it's an ABI which
> can be broken.
> 
> > > If the IDs are correct like the names, it is guaranteed. I see this
> > > ID vs name is more for some maintenance convenience because somewhere
> > > something else needs to changes or moved away from existing way of
> > > maintenance.
> 
> > How do you quantify an ID number to physical regulator mapping as
> > "correct"?  What happens if the mapping must be changed on the SCMI
> > platform side (e.g. a PMIC was added or removed, or the order that
> > regulators are listed in needs to change)?  If the SCMI agent is blindly
> 
> The whole point with the numbers being an ABI is that things must never
> be renumbered, just as if names are used the names can't be changed.  If
> the numbering is changing that just sounds like bugs on the platform
> side.  There's an implicit assumption in what you've written above that
> implementation details of the firmware should affect the IDs presented
> through SCMI which simply shouldn't be true, and indeed if the firmware
> can assign fixed strings it can just as well assign fixed numbers.

Could not agree more with Mark here...I think all the problem boils down
really to reduce maintenance burdain on the backend SCMI server as Sudeep
hinted previusly in this thread, which I am not saying is not a valid
concern, but maybe this is not the best way to address it.

My understanding, correct me if I'm wrong, is that the scenario here is one
of a backend SCMI server fw that indeed potentially manages a greater number
of resources (regulators,clocks...etc) than the ones effectively assigned to
a single OSPM agent (real or virtual), so that you have, say, 100 resources
and you are going to assign a different set of, say, 10 resources (maybe
overlapping) to each different OSPM SCMI agent running in a guest: as a
consequence you want to avoid to remap on the backend at build or
run-time this different set of 10 resources into the 0-9 set, but instead
serve these 10 different resources IDentified as they are in the backend
(say Guest1: 0-9 G2:05-14 G3:1,2,20,24-30) and then match by name in
the guest so that, say, "regulator_MAIN" is the well known regulator
for all Guests but really it could be ID 0 or 05 or 20 in the real
physical backend depending on which OSPM is askng (and similar kind of
issues in a non virtualized platform which instead has to share the same
FW between different versions of the HW)

Is my understanding correct ?

Beside these concerns expressed by Sudeep and Mark, talking specifically
about the series, I see that in V2 you introduce a common binding with
a very general 'scmi-domain-name' to be used in the above scenario with
regulators, but then you also talk about the possible need to employ this
scheme with other resources (clocks), so I was wondering, if this is the
case and if this can fly despite the above concerns,  if it was not better
to address this in a more general way at the SCMI core level, introducing
some sort of common method to be able to query a resource by name from
any SCMI driver no matter which protocol is used (perf/voltage/clock),
like as an example:

 void *.get_resource_by_node(struct scmi_protocol_handle *ph,
 			     struct device_node *np);

used in scmi-regulators to retrieve a voltage domain info by number OR
name transparently as:

    vinfo = handle->get_resource_by_node(ph, np)

so that all the logic you added in scmi-regulator to search DT and map
resources can be buried in the core SCMI and shared between all drivers
that can optionally use it.

...this will require a bit of more work in the SCMI core on my side of
course :D ...

Thanks,
Cristian

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