| Message ID | 20220222140746.12293-1-lukasz.luba@arm.com |
|---|---|
| Headers | show |
| Series | Introduce 'advanced' Energy Model in DT | expand |
On 22-02-22, 14:07, Lukasz Luba wrote: > Add new entry for the OPP which provides information about power > expressed in micro-Watts. It is useful for the Energy Model framework. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > index 15a76bcd6d42..3f07a279ed2a 100644 > --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml > @@ -93,6 +93,13 @@ patternProperties: > minItems: 1 > maxItems: 8 # Should be enough regulators > > + opp-microwatt: > + description: > + Power for the OPP > + > + A value representing power for the OPP in micro-Watts. > + $ref: /schemas/types.yaml#/definitions/uint32 > + I was expecting a much larger change here. Look at how opp-microvolt and opp-microamp is defined in this file. Should this value be made per-supply/regulator, just like voltage/current ? > opp-level: > description: > A value representing the performance level of the device.
On 2/23/22 05:50, Viresh Kumar wrote: > On 22-02-22, 14:07, Lukasz Luba wrote: >> Add new entry for the OPP which provides information about power >> expressed in micro-Watts. It is useful for the Energy Model framework. >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >> index 15a76bcd6d42..3f07a279ed2a 100644 >> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml >> @@ -93,6 +93,13 @@ patternProperties: >> minItems: 1 >> maxItems: 8 # Should be enough regulators >> >> + opp-microwatt: >> + description: >> + Power for the OPP >> + >> + A value representing power for the OPP in micro-Watts. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + > > I was expecting a much larger change here. Look at how opp-microvolt and > opp-microamp is defined in this file. > > Should this value be made per-supply/regulator, just like voltage/current ? > For the EM we need only one value. If there would be some other users of this field in future we might add the multiple power values support. Currently there is no need I would say, unless it's a hard requirement to be aligned with opp-microvolt.
On 23-02-22, 08:39, Lukasz Luba wrote: > For the EM we need only one value. If there would be some other users > of this field in future we might add the multiple power values support. > Currently there is no need I would say, unless it's a hard requirement > to be aligned with opp-microvolt. This isn't really up to what the software wants to do (to some level yes it is). The DT describes the hardware and should do so in an unambiguous way and it shouldn't be required to update the bindings again and again. It should be done just once, and in the right way possible. If the power is actually per-regulator, then it should be present in that form. The EM can just sum the up later on.
Hi Lukasz, why not extend the energy model to any kind of devices? The changes are shyly proposing a new entry in the OPP table like that is the only place where power management can happen. Is the approach to describe by small pieces here and there, specific attributes and let the kernel create an energy model from that soap? I prefer the RFC approach where the energy model is described clearly but, IMHO, it should be more abstracted, without reference to frequency or whatever but index <-> power (t-uple or equation) By this way, it could be possible to describe the battery with the different charges, the LCD bright light, etc ... On 22/02/2022 15:07, Lukasz Luba wrote: > 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 OPP node entry and > read the "opp-microwatt". It's going to only work with OPP-v2, but it's > agreed to be OK. > > Comments, suggestions are very welcome. > > changelog: > v2: > - implemented Viresh idea to add "opp-microwatt" into the OPP node entry in DT > v1 [2] > > Regards, > Lukasz Luba > > [1] https://lore.kernel.org/linux-pm/20220207073036.14901-2-lukasz.luba@arm.com/ > [2] https://lore.kernel.org/linux-pm/20220221225131.15836-1-lukasz.luba@arm.com/ > > Lukasz Luba (2): > dt-bindings: opp: Add 'opp-microwatt' entry in the OPP > OPP: Add 'opp-microwatt' parsing for advanced EM registration > > .../devicetree/bindings/opp/opp-v2-base.yaml | 7 ++ > drivers/opp/of.c | 70 +++++++++++++++++++ > 2 files changed, 77 insertions(+) >
Hi Daniel, On 2/23/22 09:52, Daniel Lezcano wrote: > > Hi Lukasz, > > why not extend the energy model to any kind of devices? > > The changes are shyly proposing a new entry in the OPP table like that > is the only place where power management can happen. It was Viresh proposal to make it in the OPP v2 table. I've checked the code and this u_watt fits perfectly there. New power value looks natural there. We also have the interconnect info in the opp, so even this kind of extensions are there. It is a clean solution which meats the GPU requirements. > > Is the approach to describe by small pieces here and there, specific > attributes and let the kernel create an energy model from that soap? > > I prefer the RFC approach where the energy model is described clearly > but, IMHO, it should be more abstracted, without reference to frequency > or whatever but index <-> power (t-uple or equation) > > By this way, it could be possible to describe the battery with the > different charges, the LCD bright light, etc ... I can see your need, but I would focus on existing issues and devices. This change was motivated by existing mainline platform which wants to have EM in GPU (Chromebook) from DT. The GPU has OPP table, thus it meets this requirement. The requirements are clear for the GPU (and similar like DSP/ISP/etc which all have OPP table). This is a clean, small step forward with the OPP approach and it doesn't block your future needs and requirements. IMO it's orthogonal, devices which have OPP table naturally might provide power there. Devices which wouldn't have OPP table, but wanted to register EM via DT - it's a different story (not the existing Chromebook's GPU). This future story can be addressed in some next step. We need real devices and examples to figure out the requirements and craft something.
On 23-02-22, 10:52, Daniel Lezcano wrote:
> why not extend the energy model to any kind of devices?
FWIW, the OPP core supports a wide range of devices now, not just CPUs.
On 2/23/22 10:43, Viresh Kumar wrote: > On 23-02-22, 10:52, Daniel Lezcano wrote: >> why not extend the energy model to any kind of devices? > > FWIW, the OPP core supports a wide range of devices now, not just CPUs. > Is that the "opp-level" thing which would allow that? I can see some DT files with regulators(?) using it e.g. [1]. It looks flexible, the opp-hz is not hard requirement, the opp-level can be used instead IIUC. It might be a next step which might meet Daniel's needs. If that 'level' can be any number and frequency is not available then EM must have 'level' filed in the struct em_perf_state for this kind of new devices. I'm open for such change. We can discuss this as a next step. We would need to find some examples how this new thing would be used. [1] https://elixir.bootlin.com/linux/v5.17-rc5/source/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi#L4
On 23-02-22, 11:22, Lukasz Luba wrote: > On 2/23/22 10:43, Viresh Kumar wrote: > > On 23-02-22, 10:52, Daniel Lezcano wrote: > > > why not extend the energy model to any kind of devices? > > > > FWIW, the OPP core supports a wide range of devices now, not just CPUs. There are many other devices which still use Freq. > Is that the "opp-level" thing which would allow that? For power supplies/regulators, we don't have freq and they use level, right. Also for interconnect we use bandwidth, in a similar way. > I can see some DT files with regulators(?) using it e.g. [1]. > It looks flexible, the opp-hz is not hard requirement, > the opp-level can be used instead IIUC. Right. > It might be a next step which might meet Daniel's needs. > If that 'level' can be any number and frequency is not available > then EM must have 'level' filed in the struct em_perf_state > for this kind of new devices. I'm open for such change. > We can discuss this as a next step. We would need to find some examples > how this new thing would be used. > > [1] https://elixir.bootlin.com/linux/v5.17-rc5/source/arch/arm/boot/dts/tegra20-peripherals-opp.dtsi#L4
On 2/23/22 11:27, Viresh Kumar wrote: > On 23-02-22, 11:22, Lukasz Luba wrote: >> On 2/23/22 10:43, Viresh Kumar wrote: >>> On 23-02-22, 10:52, Daniel Lezcano wrote: >>>> why not extend the energy model to any kind of devices? >>> >>> FWIW, the OPP core supports a wide range of devices now, not just CPUs. > > There are many other devices which still use Freq. > >> Is that the "opp-level" thing which would allow that? > > For power supplies/regulators, we don't have freq and they use level, right. > > Also for interconnect we use bandwidth, in a similar way. > >> I can see some DT files with regulators(?) using it e.g. [1]. >> It looks flexible, the opp-hz is not hard requirement, >> the opp-level can be used instead IIUC. > > Right. > Looks good. It also doesn't collide with this patch set. We could have an opp entry like: opp_1: opp-1 { opp-level = <1>; opp-microwatt = <200000>; }; Daniel would that design make sense to you? If yes, we could discuss this further after this first step for fixing GPU in merged. I would need to re-think the EM em_perf_state and maybe the new ::level there.