mbox series

[RFC,0/2] Introduce 'advanced' Energy Model in DT

Message ID 20220221225131.15836-1-lukasz.luba@arm.com
Headers show
Series Introduce 'advanced' Energy Model in DT | expand

Message

Lukasz Luba Feb. 21, 2022, 10:51 p.m. UTC
Hi all,

This patch set solves a few issues:
1. It allows to register EM from DT, when the voltage information is not
   available. (Some background of the issues present on Chromebook devices
   can be checked at [1].)
2. It allows to register 'advanced' EM from the DT, which is more accurate
   and reflects total power (dynamic + static).

Implementation details:
It adds a new callback in OPP framework to parse the "energy-model" DT
entry. The propsed DT bindings heavily draw on "operating-points" (v1)
schema.

Comments, suggestions are very welcome.

Regards,
Lukasz Luba

[1] https://lore.kernel.org/linux-pm/20220207073036.14901-2-lukasz.luba@arm.com/

Lukasz Luba (2):
  dt-bindings: power: add Energy Model bindings
  opp: Add support for 'advanced' Energy Model in DT

 .../bindings/power/energy-model.yaml          | 51 ++++++++++
 drivers/opp/of.c                              | 95 ++++++++++++++++++-
 2 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/power/energy-model.yaml

Comments

Viresh Kumar Feb. 22, 2022, 3:03 a.m. UTC | #1
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.
Lukasz Luba Feb. 22, 2022, 8:06 a.m. UTC | #2
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
Viresh Kumar Feb. 22, 2022, 9:45 a.m. UTC | #3
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.
Lukasz Luba Feb. 22, 2022, 10:03 a.m. UTC | #4
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.
Viresh Kumar Feb. 22, 2022, 10:12 a.m. UTC | #5
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.
Lukasz Luba Feb. 22, 2022, 11:03 a.m. UTC | #6
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.
Viresh Kumar Feb. 22, 2022, 11:15 a.m. UTC | #7
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.
Lukasz Luba Feb. 22, 2022, 11:23 a.m. UTC | #8
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
Rob Herring (Arm) Feb. 22, 2022, 2:22 p.m. UTC | #9
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.
Lukasz Luba Feb. 22, 2022, 2:30 p.m. UTC | #10
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