On Wed, 20 Mar 2024 10:02:28 -0500
Rob Herring <robh@xxxxxxxxxx> wrote:
Hi Rob,
thanks for having a look.
> On Mon, Mar 18, 2024 at 01:12:23AM +0000, Andre Przywara wrote:
> > From: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> >
> > The Allwinner H616 uses a similar NVMEM based mechanism to determine the
> > silicon revision, which is required to select the right frequency /
> > voltage pair for the OPPs.
> > However it limits the maximum frequency for some speedbins, which
> > requires to introduce the opp-supported-hw property.
> >
> > Add this property to the list of allowed properties, also drop the
> > requirement for the revision specific opp-microvolt properties, since
> > they won't be needed if using opp-supported-hw. When using this
> > property, we also might have multiple OPP nodes per frequency, so relax
> > the OPP node naming to allow a single letter suffix.
> >
> > Also use to opportunity to adjust some wording, and drop a sentence
> > referring to the Linux driver and the OPP subsystem.
> >
> > Shorten the existing example and add another example, showcasing the
> > opp-supported-hw property.
> >
> > Signed-off-by: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> > .../allwinner,sun50i-h6-operating-points.yaml | 89 ++++++++++---------
> > 1 file changed, 47 insertions(+), 42 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
> > index 51f62c3ae1947..d5439a3f696bc 100644
> > --- a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
> > +++ b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
> > @@ -13,25 +13,25 @@ maintainers:
> > description: |
> > For some SoCs, the CPU frequency subset and voltage value of each
> > OPP varies based on the silicon variant in use. Allwinner Process
> > - Voltage Scaling Tables defines the voltage and frequency value based
> > - on the speedbin blown in the efuse combination. The
> > - sun50i-cpufreq-nvmem driver reads the efuse value from the SoC to
> > - provide the OPP framework with required information.
> > + Voltage Scaling Tables define the voltage and frequency values based
> > + on the speedbin blown in the efuse combination.
> >
> > allOf:
> > - $ref: opp-v2-base.yaml#
> >
> > properties:
> > compatible:
> > - const: allwinner,sun50i-h6-operating-points
> > + enum:
> > + - allwinner,sun50i-h6-operating-points
> > + - allwinner,sun50i-h616-operating-points
> >
> > nvmem-cells:
> > description: |
> > A phandle pointing to a nvmem-cells node representing the efuse
> > - registers that has information about the speedbin that is used
> > + register that has information about the speedbin that is used
> > to select the right frequency/voltage value pair. Please refer
> > - the for nvmem-cells bindings
> > - Documentation/devicetree/bindings/nvmem/nvmem.txt and also
> > + to the nvmem-cells bindings in
> > + Documentation/devicetree/bindings/nvmem/nvmem.yaml and also the
> > examples below.
> >
> > opp-shared: true
> > @@ -41,21 +41,23 @@ required:
> > - nvmem-cells
> >
> > patternProperties:
> > - "^opp-[0-9]+$":
> > + "^opp-[0-9]+(-[a-z])?$":
> > type: object
> >
> > properties:
> > opp-hz: true
> > clock-latency-ns: true
> > + opp-microvolt: true
> > + opp-supported-hw:
> > + description: |
> > + A single 32 bit bitmap value, representing compatible HW, one
> > + bit per speed bin index.
> >
> > patternProperties:
> > "^opp-microvolt-speed[0-9]$": true
> >
> > required:
> > - opp-hz
> > - - opp-microvolt-speed0
> > - - opp-microvolt-speed1
> > - - opp-microvolt-speed2
> >
> > unevaluatedProperties: false
> >
> > @@ -77,58 +79,61 @@ examples:
> > opp-microvolt-speed2 = <800000>;
> > };
> >
> > - opp-720000000 {
> > + opp-1080000000 {
> > clock-latency-ns = <244144>; /* 8 32k periods */
> > - opp-hz = /bits/ 64 <720000000>;
> > + opp-hz = /bits/ 64 <1080000000>;
> >
> > - opp-microvolt-speed0 = <880000>;
> > - opp-microvolt-speed1 = <820000>;
> > - opp-microvolt-speed2 = <800000>;
> > + opp-microvolt-speed0 = <1060000>;
> > + opp-microvolt-speed1 = <880000>;
> > + opp-microvolt-speed2 = <840000>;
> > };
> >
> > - opp-816000000 {
> > + opp-1488000000 {
> > clock-latency-ns = <244144>; /* 8 32k periods */
> > - opp-hz = /bits/ 64 <816000000>;
> > + opp-hz = /bits/ 64 <1488000000>;
> >
> > - opp-microvolt-speed0 = <880000>;
> > - opp-microvolt-speed1 = <820000>;
> > - opp-microvolt-speed2 = <800000>;
> > + opp-microvolt-speed0 = <1160000>;
> > + opp-microvolt-speed1 = <1000000>;
> > + opp-microvolt-speed2 = <960000>;
> > };
> > + };
> > +
> > + - |
> > + opp-table {
> > + compatible = "allwinner,sun50i-h616-operating-points";
> > + nvmem-cells = <&speedbin_efuse>;
> > + opp-shared;
> >
> > - opp-888000000 {
> > + opp-480000000 {
> > clock-latency-ns = <244144>; /* 8 32k periods */
> > - opp-hz = /bits/ 64 <888000000>;
> > + opp-hz = /bits/ 64 <480000000>;
> >
> > - opp-microvolt-speed0 = <940000>;
> > - opp-microvolt-speed1 = <820000>;
> > - opp-microvolt-speed2 = <800000>;
> > + opp-microvolt = <900000>;
> > + opp-supported-hw = <0x1f>;
> > };
> >
> > - opp-1080000000 {
> > + opp-792000000-l {
> > clock-latency-ns = <244144>; /* 8 32k periods */
> > - opp-hz = /bits/ 64 <1080000000>;
> > + opp-hz = /bits/ 64 <792000000>;
> >
> > - opp-microvolt-speed0 = <1060000>;
> > - opp-microvolt-speed1 = <880000>;
> > - opp-microvolt-speed2 = <840000>;
> > + opp-microvolt = <900000>;
> > + opp-supported-hw = <0x02>;
> > };
> >
> > - opp-1320000000 {
> > + opp-792000000-h {
> > clock-latency-ns = <244144>; /* 8 32k periods */
> > - opp-hz = /bits/ 64 <1320000000>;
> > + opp-hz = /bits/ 64 <792000000>;
> >
> > - opp-microvolt-speed0 = <1160000>;
> > - opp-microvolt-speed1 = <940000>;
> > - opp-microvolt-speed2 = <900000>;
> > + opp-microvolt = <940000>;
> > + opp-supported-hw = <0x10>;
>
> So far, we've avoided multiple entries for a single frequency. I think
> it would be good to maintain that.
Fair, I wasn't super happy with that either, but it still seemed better
than the alternatives.
> Couldn't you just do:
>
> opp-supported-hw = <0>, <0x10>, <0x02>;
>
> Where the index corresponds to speed0, speed1, speed2.
>
> If not, then I don't understand how multiple entries of opp-supported-hw
> are supposed to work.
If I got this correctly, multiple cells in opp-supported-hw are to
describe various levels of hierarchy for a chip version, so like silicon
mask, metal layer revision, bin, I guess? The binding doc speaks of "cuts,
substrate and process", not really sure what that means exactly.
I think currently we cannot easily combine microvolt suffixes and
opp-supported-hw in one OPP node? I think it bails out if one
microvolt-speed<x> property is missing, but I have to double check.
But IIRC v1 of this series somehow pulled that off, so we can maybe bring
it back? To end up with:
opp-792 {
opp-hz = <792000000>;
opp-microvolt-speed1 = <900000>;
opp-microvolt-speed4 = <940000>;
opp-supported-hw = <0x12>;
};
opp-1512 {
opp-hz = <1512000000>;
opp-microvolt = <1100000>;
opp-supported-hw = <0x0a>;
};
I chose the way that's described in this patch because it seemed shorter,
but I am afraid none of the versions is really nice here. What they in
fact are is quite different OPP tables for each speedbin, with a
different set of frequencies, for unknown reasons. Is there a way to select
one of multiple *tables*, each with their individual, but simple set of
voltage/freq pairs?
This is what they look like in table format, btw:
0 1 2 3 4
480 900 900 900 900 900
600 - - - - 900
720 900 - 900 900 -
792 - 900 - - 940
936 900 - 900 900 -
1008 950 940 950 950 1020
1104 1000 - 1000 1000 -
1200 1050 1020 1050 1050 1100
1320 1100 - 1100 1100 1100
1416 1100 - 1100 1100 -
1512 - 1100 - 1100 -
I was wondering if we should fill those gaps, by putting in the voltage
from the next higher OPP? Then we could use the microvolt suffixes, except
for the last two frequencies, where we use opp-supported-hw?
Cheers,
Andre
