From d0c46b59ac82cb2c81c91188cb100401c1862d53 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 2 Nov 2022 16:19:33 +0530 Subject: [PATCH 1/6] dt-bindings: opp: Fix usage of current in microwatt property The bindings mentions "current" instead of "power". Fix it. Tested-by: James Calligeros Signed-off-by: Viresh Kumar --- Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml index 66d0ec763f0b..cb025b0a346d 100644 --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml @@ -108,7 +108,7 @@ patternProperties: The power for the OPP in micro-Watts. Entries for multiple regulators shall be provided in the same field - separated by angular brackets <>. If current values aren't required + separated by angular brackets <>. If power values aren't required for a regulator, then it shall be filled with 0. If power values aren't required for any of the regulators, then this field is not required. The OPP binding doesn't provide any provisions to relate the From 1b91dba2474ed61c34e453a48c4968833ebd68c6 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 2 Nov 2022 16:19:33 +0530 Subject: [PATCH 2/6] dt-bindings: opp: Fix named microwatt property The named microwatt- property should look exactly like microvolt and microamp properties. There were some differences, fix them. Tested-by: James Calligeros Signed-off-by: Viresh Kumar --- Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml index cb025b0a346d..cf9c2f7bddc2 100644 --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml @@ -230,9 +230,9 @@ patternProperties: minItems: 1 maxItems: 8 # Should be enough regulators - '^opp-microwatt': + '^opp-microwatt-': description: - Named opp-microwatt property. Similar to opp-microamp property, + Named opp-microwatt property. Similar to opp-microamp- property, but for microwatt instead. $ref: /schemas/types.yaml#/definitions/uint32-array minItems: 1 From 71b09429e82e5cd402bedc763c25b5962fdc9d71 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 2 Nov 2022 16:57:05 +0530 Subject: [PATCH 3/6] OPP: Parse named opp-microwatt property too We missed parsing the named opp-microwatt- property, fix that. Tested-by: James Calligeros Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 605d68673f92..e010e119c42b 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -695,9 +695,19 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, } } - /* Search for "opp-microwatt" */ - sprintf(name, "opp-microwatt"); - prop = of_find_property(opp->np, name, NULL); + /* Search for "opp-microwatt-" */ + prop = NULL; + if (opp_table->prop_name) { + snprintf(name, sizeof(name), "opp-microwatt-%s", + opp_table->prop_name); + prop = of_find_property(opp->np, name, NULL); + } + + if (!prop) { + /* Search for "opp-microwatt" */ + sprintf(name, "opp-microwatt"); + prop = of_find_property(opp->np, name, NULL); + } if (prop) { pcount = of_property_count_u32_elems(opp->np, name); From e5acb1991b5884d86f113d7841024234c465c501 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Wed, 2 Nov 2022 16:47:08 +0530 Subject: [PATCH 4/6] OPP: Simplify opp_parse_supplies() by restructuring it opp_parse_supplies() has grown into too big of a routine (~190 lines) and it is not straight-forward to understand it anymore. Break it into smaller routines and reduce code redundancy a bit by using the same code to parse properties. This shouldn't result in any logical changes. Tested-by: James Calligeros Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 266 +++++++++++++++++++---------------------------- 1 file changed, 106 insertions(+), 160 deletions(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index e010e119c42b..08d97b97c326 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -578,179 +578,126 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table, return false; } +static u32 *_parse_named_prop(struct dev_pm_opp *opp, struct device *dev, + struct opp_table *opp_table, + const char *prop_type, bool *triplet) +{ + struct property *prop = NULL; + char name[NAME_MAX]; + int count, ret; + u32 *out; + + /* Search for "opp--" */ + if (opp_table->prop_name) { + snprintf(name, sizeof(name), "opp-%s-%s", prop_type, + opp_table->prop_name); + prop = of_find_property(opp->np, name, NULL); + } + + if (!prop) { + /* Search for "opp-" */ + snprintf(name, sizeof(name), "opp-%s", prop_type); + prop = of_find_property(opp->np, name, NULL); + if (!prop) + return NULL; + } + + count = of_property_count_u32_elems(opp->np, name); + if (count < 0) { + dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, name, + count); + return ERR_PTR(count); + } + + /* + * Initialize regulator_count, if regulator information isn't provided + * by the platform. Now that one of the properties is available, fix the + * regulator_count to 1. + */ + if (unlikely(opp_table->regulator_count == -1)) + opp_table->regulator_count = 1; + + if (count != opp_table->regulator_count && + (!triplet || count != opp_table->regulator_count * 3)) { + dev_err(dev, "%s: Invalid number of elements in %s property (%u) with supplies (%d)\n", + __func__, prop_type, count, opp_table->regulator_count); + return ERR_PTR(-EINVAL); + } + + out = kmalloc_array(count, sizeof(*out), GFP_KERNEL); + if (!out) + return ERR_PTR(-EINVAL); + + ret = of_property_read_u32_array(opp->np, name, out, count); + if (ret) { + dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret); + kfree(out); + return ERR_PTR(-EINVAL); + } + + if (triplet) + *triplet = count != opp_table->regulator_count; + + return out; +} + +static u32 *opp_parse_microvolt(struct dev_pm_opp *opp, struct device *dev, + struct opp_table *opp_table, bool *triplet) +{ + u32 *microvolt; + + microvolt = _parse_named_prop(opp, dev, opp_table, "microvolt", triplet); + if (IS_ERR(microvolt)) + return microvolt; + + if (!microvolt) { + /* + * Missing property isn't a problem, but an invalid + * entry is. This property isn't optional if regulator + * information is provided. + */ + if (opp_table->regulator_count > 0) { + dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", + __func__); + return ERR_PTR(-EINVAL); + } + } + + return microvolt; +} + static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, struct opp_table *opp_table) { - u32 *microvolt, *microamp = NULL, *microwatt = NULL; - int supplies = opp_table->regulator_count; - int vcount, icount, pcount, ret, i, j; - struct property *prop = NULL; - char name[NAME_MAX]; + u32 *microvolt, *microamp, *microwatt; + int ret = 0, i, j; + bool triplet; - /* Search for "opp-microvolt-" */ - if (opp_table->prop_name) { - snprintf(name, sizeof(name), "opp-microvolt-%s", - opp_table->prop_name); - prop = of_find_property(opp->np, name, NULL); - } + microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet); + if (IS_ERR_OR_NULL(microvolt)) + return PTR_ERR(microvolt); - if (!prop) { - /* Search for "opp-microvolt" */ - sprintf(name, "opp-microvolt"); - prop = of_find_property(opp->np, name, NULL); - - /* Missing property isn't a problem, but an invalid entry is */ - if (!prop) { - if (unlikely(supplies == -1)) { - /* Initialize regulator_count */ - opp_table->regulator_count = 0; - return 0; - } - - if (!supplies) - return 0; - - dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", - __func__); - return -EINVAL; - } - } - - if (unlikely(supplies == -1)) { - /* Initialize regulator_count */ - supplies = opp_table->regulator_count = 1; - } else if (unlikely(!supplies)) { - dev_err(dev, "%s: opp-microvolt wasn't expected\n", __func__); - return -EINVAL; - } - - vcount = of_property_count_u32_elems(opp->np, name); - if (vcount < 0) { - dev_err(dev, "%s: Invalid %s property (%d)\n", - __func__, name, vcount); - return vcount; - } - - /* There can be one or three elements per supply */ - if (vcount != supplies && vcount != supplies * 3) { - dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", - __func__, name, vcount, supplies); - return -EINVAL; - } - - microvolt = kmalloc_array(vcount, sizeof(*microvolt), GFP_KERNEL); - if (!microvolt) - return -ENOMEM; - - ret = of_property_read_u32_array(opp->np, name, microvolt, vcount); - if (ret) { - dev_err(dev, "%s: error parsing %s: %d\n", __func__, name, ret); - ret = -EINVAL; + microamp = _parse_named_prop(opp, dev, opp_table, "microamp", NULL); + if (IS_ERR(microamp)) { + ret = PTR_ERR(microamp); goto free_microvolt; } - /* Search for "opp-microamp-" */ - prop = NULL; - if (opp_table->prop_name) { - snprintf(name, sizeof(name), "opp-microamp-%s", - opp_table->prop_name); - prop = of_find_property(opp->np, name, NULL); + microwatt = _parse_named_prop(opp, dev, opp_table, "microwatt", NULL); + if (IS_ERR(microwatt)) { + ret = PTR_ERR(microwatt); + goto free_microamp; } - if (!prop) { - /* Search for "opp-microamp" */ - sprintf(name, "opp-microamp"); - prop = of_find_property(opp->np, name, NULL); - } - - if (prop) { - icount = of_property_count_u32_elems(opp->np, name); - if (icount < 0) { - dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, - name, icount); - ret = icount; - goto free_microvolt; - } - - if (icount != supplies) { - dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", - __func__, name, icount, supplies); - ret = -EINVAL; - goto free_microvolt; - } - - microamp = kmalloc_array(icount, sizeof(*microamp), GFP_KERNEL); - if (!microamp) { - ret = -EINVAL; - goto free_microvolt; - } - - ret = of_property_read_u32_array(opp->np, name, microamp, - icount); - if (ret) { - dev_err(dev, "%s: error parsing %s: %d\n", __func__, - name, ret); - ret = -EINVAL; - goto free_microamp; - } - } - - /* Search for "opp-microwatt-" */ - prop = NULL; - if (opp_table->prop_name) { - snprintf(name, sizeof(name), "opp-microwatt-%s", - opp_table->prop_name); - prop = of_find_property(opp->np, name, NULL); - } - - if (!prop) { - /* Search for "opp-microwatt" */ - sprintf(name, "opp-microwatt"); - prop = of_find_property(opp->np, name, NULL); - } - - if (prop) { - pcount = of_property_count_u32_elems(opp->np, name); - if (pcount < 0) { - dev_err(dev, "%s: Invalid %s property (%d)\n", __func__, - name, pcount); - ret = pcount; - goto free_microamp; - } - - if (pcount != supplies) { - dev_err(dev, "%s: Invalid number of elements in %s property (%d) with supplies (%d)\n", - __func__, name, pcount, supplies); - ret = -EINVAL; - goto free_microamp; - } - - microwatt = kmalloc_array(pcount, sizeof(*microwatt), - GFP_KERNEL); - if (!microwatt) { - ret = -EINVAL; - goto free_microamp; - } - - ret = of_property_read_u32_array(opp->np, name, microwatt, - pcount); - if (ret) { - dev_err(dev, "%s: error parsing %s: %d\n", __func__, - name, ret); - ret = -EINVAL; - goto free_microwatt; - } - } - - for (i = 0, j = 0; i < supplies; i++) { + for (i = 0, j = 0; i < opp_table->regulator_count; i++) { opp->supplies[i].u_volt = microvolt[j++]; - if (vcount == supplies) { - opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; - opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; - } else { + if (triplet) { opp->supplies[i].u_volt_min = microvolt[j++]; opp->supplies[i].u_volt_max = microvolt[j++]; + } else { + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; } if (microamp) @@ -760,7 +707,6 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, opp->supplies[i].u_watt = microwatt[i]; } -free_microwatt: kfree(microwatt); free_microamp: kfree(microamp); From 2eedf62e66c28f6991425b580af68b3d89fa8021 Mon Sep 17 00:00:00 2001 From: James Calligeros Date: Thu, 3 Nov 2022 14:40:44 +0530 Subject: [PATCH 5/6] OPP: decouple dt properties in opp_parse_supplies() The opp-microwatt property was added with the intention of providing platforms a way to specify a precise value for the power consumption of a device at a given OPP to enable better energy-aware scheduling decisions by informing the kernel of the total static and dynamic power of a device at a given OPP, removing the reliance on the EM subsystem's often flawed estimations. This property is parsed by opp_parse_supplies(), which creates a hard dependency on the opp-microvolt property. Some platforms, such as Apple Silicon, do not describe their device's voltage regulators in the DT as they cannot be controlled by the kernel and/or rely on opaque firmware algorithms to control their voltage and current characteristics at runtime. We can, however, experimentally determine the power consumption of a given device at a given OPP, taking advantage of opp-microwatt to provide EAS on such devices as was initially intended. Allow platforms to specify and consume any subset of opp-microvolt, opp-microamp, or opp-microwatt without a hard dependency on opp-microvolt to enable this functionality on such platforms. Tested-by: James Calligeros Signed-off-by: James Calligeros Co-developed-by: Viresh Kumar Signed-off-by: Viresh Kumar --- drivers/opp/of.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 08d97b97c326..e55c6095adf0 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -654,9 +654,12 @@ static u32 *opp_parse_microvolt(struct dev_pm_opp *opp, struct device *dev, /* * Missing property isn't a problem, but an invalid * entry is. This property isn't optional if regulator - * information is provided. + * information is provided. Check only for the first OPP, as + * regulator_count may get initialized after that to a valid + * value. */ - if (opp_table->regulator_count > 0) { + if (list_empty(&opp_table->opp_list) && + opp_table->regulator_count > 0) { dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n", __func__); return ERR_PTR(-EINVAL); @@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, bool triplet; microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet); - if (IS_ERR_OR_NULL(microvolt)) + if (IS_ERR(microvolt)) return PTR_ERR(microvolt); microamp = _parse_named_prop(opp, dev, opp_table, "microamp", NULL); @@ -689,15 +692,26 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev, goto free_microamp; } - for (i = 0, j = 0; i < opp_table->regulator_count; i++) { - opp->supplies[i].u_volt = microvolt[j++]; + /* + * Initialize regulator_count if it is uninitialized and no properties + * are found. + */ + if (unlikely(opp_table->regulator_count == -1)) { + opp_table->regulator_count = 0; + return 0; + } - if (triplet) { - opp->supplies[i].u_volt_min = microvolt[j++]; - opp->supplies[i].u_volt_max = microvolt[j++]; - } else { - opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; - opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + for (i = 0, j = 0; i < opp_table->regulator_count; i++) { + if (microvolt) { + opp->supplies[i].u_volt = microvolt[j++]; + + if (triplet) { + opp->supplies[i].u_volt_min = microvolt[j++]; + opp->supplies[i].u_volt_max = microvolt[j++]; + } else { + opp->supplies[i].u_volt_min = opp->supplies[i].u_volt; + opp->supplies[i].u_volt_max = opp->supplies[i].u_volt; + } } if (microamp) From dba79b78ecc18f7788fd08eb998388e226817fb5 Mon Sep 17 00:00:00 2001 From: Serge Semin Date: Mon, 7 Nov 2022 23:43:54 +0300 Subject: [PATCH 6/6] dt-bindings: opp-v2: Fix clock-latency-ns prop in example Accidentally discovered a hidden typo in the DT-nodes defined in the opp-v2 bindings example. Instead of specifying the "clock-latency-ns" property the DT-node has been created with the "lock-latency-ns" property in it, which doesn't exist neither in opp-v2 nor in the base schemas. Let's fix the name to having the "clock-" prefix as it was originally implied and as the rest of the similar nodes has. Fixes: 94274f20f6bf ("dt-bindings: opp: Convert to DT schema") Signed-off-by: Serge Semin Signed-off-by: Viresh Kumar --- Documentation/devicetree/bindings/opp/opp-v2.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/opp/opp-v2.yaml b/Documentation/devicetree/bindings/opp/opp-v2.yaml index eaf8fba2c691..2f05920fce79 100644 --- a/Documentation/devicetree/bindings/opp/opp-v2.yaml +++ b/Documentation/devicetree/bindings/opp/opp-v2.yaml @@ -155,7 +155,7 @@ examples: opp-hz = /bits/ 64 <1200000000>; opp-microvolt = <1025000>; opp-microamp = <90000>; - lock-latency-ns = <290000>; + clock-latency-ns = <290000>; turbo-mode; }; };