| Message ID | 20220221225131.15836-2-lukasz.luba@arm.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce 'advanced' Energy Model in DT | expand |
| Context | Check | Description |
|---|---|---|
| robh/patch-applied | success | |
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | fail | build log |
On 21-02-22, 22:51, Lukasz Luba wrote: > Add DT bindings for the Energy Model information. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > .../bindings/power/energy-model.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/energy-model.yaml > > diff --git a/Documentation/devicetree/bindings/power/energy-model.yaml b/Documentation/devicetree/bindings/power/energy-model.yaml > new file mode 100644 > index 000000000000..804a9b324925 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/energy-model.yaml > @@ -0,0 +1,51 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/energy-model.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Energy Model Bindings > + > +maintainers: > + - Lukasz Luba <lukasz.luba@arm.com> > + > +description: |+ > + Devices work at specific performance states (frequencies). The power which > + is used at a given performance state is an important information. A framework > + which maintains this information is Energy Model. This document defines > + bindings for these Energy Model performance states applicable across wide > + range of devices. For illustration purpose, this document uses GPU as a device. > + > + This binding only supports frequency-power pairs. > + > +select: true > + > +properties: > + operating-points: > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > + items: > + items: > + - description: Frequency in kHz > + - description: Power in uW > + > + > +additionalProperties: true > +examples: > + { > + gpu_energy_model: energy-model { > + compatible = "energy-model"; > + energy-model-entries = < > + 200000 300000 > + 297000 500000 > + 400000 800000 > + 500000 1400000 > + 600000 2000000 > + 800000 2800000 > + >; > + }; > + }; > + > + &gpu { > + energy-model = <&gpu_energy_model>; > + }; What about getting this from the OPP table instead? There is no point adding similar tables for a device.
Hi Viresh, Thanks for having a look at it. On 2/22/22 03:03, Viresh Kumar wrote: > On 21-02-22, 22:51, Lukasz Luba wrote: >> Add DT bindings for the Energy Model information. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> .../bindings/power/energy-model.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/energy-model.yaml >> >> diff --git a/Documentation/devicetree/bindings/power/energy-model.yaml b/Documentation/devicetree/bindings/power/energy-model.yaml >> new file mode 100644 >> index 000000000000..804a9b324925 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/energy-model.yaml >> @@ -0,0 +1,51 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/power/energy-model.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Energy Model Bindings >> + >> +maintainers: >> + - Lukasz Luba <lukasz.luba@arm.com> >> + >> +description: |+ >> + Devices work at specific performance states (frequencies). The power which >> + is used at a given performance state is an important information. A framework >> + which maintains this information is Energy Model. This document defines >> + bindings for these Energy Model performance states applicable across wide >> + range of devices. For illustration purpose, this document uses GPU as a device. >> + >> + This binding only supports frequency-power pairs. >> + >> +select: true >> + >> +properties: >> + operating-points: >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix >> + items: >> + items: >> + - description: Frequency in kHz >> + - description: Power in uW >> + >> + >> +additionalProperties: true >> +examples: >> + { >> + gpu_energy_model: energy-model { >> + compatible = "energy-model"; >> + energy-model-entries = < >> + 200000 300000 >> + 297000 500000 >> + 400000 800000 >> + 500000 1400000 >> + 600000 2000000 >> + 800000 2800000 >> + >; >> + }; >> + }; >> + >> + &gpu { >> + energy-model = <&gpu_energy_model>; >> + }; > > What about getting this from the OPP table instead? There is no point adding > similar tables for a device. > I'm not sure if that would be flexible enough to meet the requirement: power for each OPP might be different in one board vs. other board. Power can be different because of static power, which might vary due to different temperature that the SoC operates at a particular OPP. It can be due to: better heat sink (or no at all like on dev board), bigger PCB with fat copper layers, different location of hot devices (like PMIC) on the PCB, missing some hot devices on the PCB (fast charger), case, etc. The 'advanced' EM and this DT array allows to provide avg power from measurements for a particular board and for each OPP independently. AFAIK the OPP definition is more SoC specific. I'm particularly interested in this SC7180 SoC and it's GPU power [1]. There is also a nice OPP definition in that node. As you can see there is one SoC file and quite a few boards next to it. Some of them have a decent thermal design (inside Chromebook) but some are 'just' dev boards. The power would be different for those boards. Similar issue would be e.g. for Rk3399 SoC (Chromebook, Pine64, IoT and some dev boards). IMO the OPP table might be to much hassle and heavy for those boards. [1] https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/qcom/sc7180.dtsi#L1953
On 22-02-22, 08:06, Lukasz Luba wrote: > I'm not sure if that would be flexible enough to meet the requirement: > power for each OPP might be different in one board vs. other board. Don't DT files overload values from board files all the time ? Why wouldn't the same apply for OPP table as well ? > AFAIK the OPP definition is more SoC specific. This isn't about OPP definition as well, but just that if DT allows you to override or not. I think it will.
On 2/22/22 09:45, Viresh Kumar wrote: > On 22-02-22, 08:06, Lukasz Luba wrote: >> I'm not sure if that would be flexible enough to meet the requirement: >> power for each OPP might be different in one board vs. other board. > > Don't DT files overload values from board files all the time ? Why wouldn't the > same apply for OPP table as well ? In that SoC and family of the boards, there are no such examples. It used to be popular in arm32 boards, but I'm not sure nowadays. > >> AFAIK the OPP definition is more SoC specific. > > This isn't about OPP definition as well, but just that if DT allows you to > override or not. I think it will. > Redefining the whole OPP table, when the freq, voltage, interconnect, and other old entries don't change isn't too messy? As I said, I would prefer something lightweight, not redefining all stuff from OPP in every board file.
On 22-02-22, 10:03, Lukasz Luba wrote: > > > On 2/22/22 09:45, Viresh Kumar wrote: > > On 22-02-22, 08:06, Lukasz Luba wrote: > > > I'm not sure if that would be flexible enough to meet the requirement: > > > power for each OPP might be different in one board vs. other board. > > > > Don't DT files overload values from board files all the time ? Why wouldn't the > > same apply for OPP table as well ? > > In that SoC and family of the boards, there are no such examples. Here is one I think. arch/arm64/boot/dts/freescale/imx8mq-librem5-r3.dts > It used to be popular in arm32 boards, but I'm not sure nowadays. I think it is still common, not with OPPs though. > > > AFAIK the OPP definition is more SoC specific. > > > > This isn't about OPP definition as well, but just that if DT allows you to > > override or not. I think it will. > > > > Redefining the whole OPP table, when the freq, voltage, interconnect, > and other old entries don't change isn't too messy? I think you misunderstood what I said. The common part of the OPP table should stay in the central .dtsi file. The dts files though, should just add the power specific values to the existing OPP table. > As I said, I would prefer something lightweight, not redefining all > stuff from OPP in every board file.
On 2/22/22 10:12, Viresh Kumar wrote: > On 22-02-22, 10:03, Lukasz Luba wrote: >> >> >> On 2/22/22 09:45, Viresh Kumar wrote: >>> On 22-02-22, 08:06, Lukasz Luba wrote: >>>> I'm not sure if that would be flexible enough to meet the requirement: >>>> power for each OPP might be different in one board vs. other board. >>> >>> Don't DT files overload values from board files all the time ? Why wouldn't the >>> same apply for OPP table as well ? >> >> In that SoC and family of the boards, there are no such examples. > > Here is one I think. > > arch/arm64/boot/dts/freescale/imx8mq-librem5-r3.dts > >> It used to be popular in arm32 boards, but I'm not sure nowadays. > > I think it is still common, not with OPPs though. > >>>> AFAIK the OPP definition is more SoC specific. >>> >>> This isn't about OPP definition as well, but just that if DT allows you to >>> override or not. I think it will. >>> >> >> Redefining the whole OPP table, when the freq, voltage, interconnect, >> and other old entries don't change isn't too messy? > > I think you misunderstood what I said. The common part of the OPP table should > stay in the central .dtsi file. The dts files though, should just add the power > specific values to the existing OPP table. > OK, I misunderstood that. If that is possible than it would be great. I'm assuming you are taking about OPP v2. I can relax the requirement that I need to provide this DT-EM for arm32, since they have a legacy OPP v1. So we might have an entry similar that interconnect for the bandwidth, but for us it would be 'opp-power-uw'? Let me have a look about some examples how that could be just added/extended in the opp table but from board file. If you have some handy link, I would be grateful.
On 22-02-22, 11:03, Lukasz Luba wrote: > OK, I misunderstood that. If that is possible than it would > be great. I'm assuming you are taking about OPP v2. Yes. > I can relax the > requirement that I need to provide this DT-EM for arm32, since they > have a legacy OPP v1. OPP V2 or V1 doesn't have anything to do with arm32/64. Anyone can implement the newer V2 version (basically whoever wants something more than a simple freq/volt pair). So arm32 SoC's that want to use the DT-EM thing, should migrate to opp-v2, we can't support that with opp-v1. > So we might have an entry similar that interconnect for the > bandwidth, but for us it would be 'opp-power-uw'? I will rather say similar to opp-microvolt or opp-microamp, so it better be opp-microwatt. > Let me have a look about some examples how that could be just > added/extended in the opp table but from board file. > If you have some handy link, I would be grateful. The file I provided earlier: arch/arm64/boot/dts/freescale/imx8mq-librem5-r3.dts This is updating opp-microvolt property of a single OPP node in the whole table. Pretty much like this only.
On 2/22/22 11:15, Viresh Kumar wrote: > On 22-02-22, 11:03, Lukasz Luba wrote: >> OK, I misunderstood that. If that is possible than it would >> be great. I'm assuming you are taking about OPP v2. > > Yes. > >> I can relax the >> requirement that I need to provide this DT-EM for arm32, since they >> have a legacy OPP v1. > > OPP V2 or V1 doesn't have anything to do with arm32/64. Anyone can implement the > newer V2 version (basically whoever wants something more than a simple freq/volt > pair). So arm32 SoC's that want to use the DT-EM thing, should migrate to > opp-v2, we can't support that with opp-v1. > >> So we might have an entry similar that interconnect for the >> bandwidth, but for us it would be 'opp-power-uw'? > > I will rather say similar to opp-microvolt or opp-microamp, so it better be > opp-microwatt. > >> Let me have a look about some examples how that could be just >> added/extended in the opp table but from board file. >> If you have some handy link, I would be grateful. > > The file I provided earlier: > > arch/arm64/boot/dts/freescale/imx8mq-librem5-r3.dts > > This is updating opp-microvolt property of a single OPP node in the whole table. > Pretty much like this only. > Thanks, I'll use that example for the v2
On Mon, 21 Feb 2022 22:51:30 +0000, Lukasz Luba wrote: > Add DT bindings for the Energy Model information. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > .../bindings/power/energy-model.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/energy-model.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/power/energy-model.yaml:34:5: [warning] wrong indentation: expected 2 but found 4 (indentation) ./Documentation/devicetree/bindings/power/energy-model.yaml:35:8: [warning] wrong indentation: expected 6 but found 7 (indentation) ./Documentation/devicetree/bindings/power/energy-model.yaml:36:16: [warning] wrong indentation: expected 9 but found 15 (indentation) ./Documentation/devicetree/bindings/power/energy-model.yaml:35:39: [error] syntax error: expected ',' or '}', but got '{' (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/power/energy-model.example.dts' Traceback (most recent call last): File "/usr/local/bin/dt-extract-example", line 46, in <module> binding = yaml.load(open(args.yamlfile, encoding='utf-8').read()) File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load return constructor.get_single_data() File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 119, in get_single_data node = self.composer.get_single_node() File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event ruamel.yaml.parser.ParserError: while parsing a flow mapping in "<unicode string>", line 34, column 5 did not find expected ',' or '}' in "<unicode string>", line 35, column 39 make[1]: *** [Documentation/devicetree/bindings/Makefile:25: Documentation/devicetree/bindings/power/energy-model.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/power/energy-model.yaml: while parsing a flow mapping in "<unicode string>", line 34, column 5 did not find expected ',' or '}' in "<unicode string>", line 35, column 39 /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/energy-model.yaml: ignoring, error parsing file make: *** [Makefile:1398: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1595776 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hi Rob, On 2/22/22 14:22, Rob Herring wrote: > On Mon, 21 Feb 2022 22:51:30 +0000, Lukasz Luba wrote: >> Add DT bindings for the Energy Model information. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> .../bindings/power/energy-model.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/energy-model.yaml >> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > ./Documentation/devicetree/bindings/power/energy-model.yaml:34:5: [warning] wrong indentation: expected 2 but found 4 (indentation) > ./Documentation/devicetree/bindings/power/energy-model.yaml:35:8: [warning] wrong indentation: expected 6 but found 7 (indentation) > ./Documentation/devicetree/bindings/power/energy-model.yaml:36:16: [warning] wrong indentation: expected 9 but found 15 (indentation) > ./Documentation/devicetree/bindings/power/energy-model.yaml:35:39: [error] syntax error: expected ',' or '}', but got '{' (syntax) > > dtschema/dtc warnings/errors: > make[1]: *** Deleting file 'Documentation/devicetree/bindings/power/energy-model.example.dts' > Traceback (most recent call last): > File "/usr/local/bin/dt-extract-example", line 46, in <module> > binding = yaml.load(open(args.yamlfile, encoding='utf-8').read()) > File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load > return constructor.get_single_data() > File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 119, in get_single_data > node = self.composer.get_single_node() > File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node > File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document > File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node > File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node > File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node > File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node > File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event > ruamel.yaml.parser.ParserError: while parsing a flow mapping > in "<unicode string>", line 34, column 5 > did not find expected ',' or '}' > in "<unicode string>", line 35, column 39 > make[1]: *** [Documentation/devicetree/bindings/Makefile:25: Documentation/devicetree/bindings/power/energy-model.example.dts] Error 1 > make[1]: *** Waiting for unfinished jobs.... > ./Documentation/devicetree/bindings/power/energy-model.yaml: while parsing a flow mapping > in "<unicode string>", line 34, column 5 > did not find expected ',' or '}' > in "<unicode string>", line 35, column 39 > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/energy-model.yaml: ignoring, error parsing file > make: *** [Makefile:1398: dt_binding_check] Error 2 > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/patch/1595776 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. > This new dt bindings idea is abandoned. We would go for a new entry in the OPP node: "opp-microwatt". I've sent a v2. BTW, I've tried to run this check, but for some reason my python failed when I tried to upgrade the dtschema: 'Could not find a version that satisfies the requirement jsonschema>=4.1.2 (from dtschema)' It used to work. I'll probaby have to invest more time and figure out why my pip3 fails. Regards, Lukasz
diff --git a/Documentation/devicetree/bindings/power/energy-model.yaml b/Documentation/devicetree/bindings/power/energy-model.yaml new file mode 100644 index 000000000000..804a9b324925 --- /dev/null +++ b/Documentation/devicetree/bindings/power/energy-model.yaml @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/energy-model.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Energy Model Bindings + +maintainers: + - Lukasz Luba <lukasz.luba@arm.com> + +description: |+ + Devices work at specific performance states (frequencies). The power which + is used at a given performance state is an important information. A framework + which maintains this information is Energy Model. This document defines + bindings for these Energy Model performance states applicable across wide + range of devices. For illustration purpose, this document uses GPU as a device. + + This binding only supports frequency-power pairs. + +select: true + +properties: + operating-points: + $ref: /schemas/types.yaml#/definitions/uint32-matrix + items: + items: + - description: Frequency in kHz + - description: Power in uW + + +additionalProperties: true +examples: + { + gpu_energy_model: energy-model { + compatible = "energy-model"; + energy-model-entries = < + 200000 300000 + 297000 500000 + 400000 800000 + 500000 1400000 + 600000 2000000 + 800000 2800000 + >; + }; + }; + + &gpu { + energy-model = <&gpu_energy_model>; + }; +...
Add DT bindings for the Energy Model information. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- .../bindings/power/energy-model.yaml | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/energy-model.yaml