opp: How to use multiple opp-supported-hw versions properly?

opp: How to use multiple opp-supported-hw versions properly?

[Stephan Gerhold]

Hi Viresh,

(an unrelated questions while I investigate the device links ;) )

I'm a bit confused about how to use "opp-supported-hw" properly
when you have multiple versions defined.

In my case I have two version numbers from 0-7, so theoretically up to
64 versions. This does not fit into a single version mask so I added
them as separate versions to the OPP table.

Now let's say I want to limit an OPP to v0.1, v1.0 and v1.1, but not
v0.0. With a single "opp-supported-hw" I think I can only say:

	opp-supported-hw = <0x3 0x3>;

but that does also include v0.0...
I think to exclude that I would need multiple version ranges, e.g.

	opp-supported-hw = <0x1 0x2>, <0x2 0x3>;

This does not seem to be supported, though.

I believe a similar situation exists in tegra20-cpu-opp.dtsi:
The way it was solved there is to duplicate many of the OPP nodes
and set them with different "opp-supported-hw" properties. e.g.

	opp@1000000000,1000 {
		clock-latency-ns = <400000>;
		opp-supported-hw = <0x02 0x0006>; // v1.1 or v1.2
		opp-hz = /bits/ 64 <1000000000>;
	};

	opp@1000000000,1000,0,2 {
		clock-latency-ns = <400000>;
		opp-supported-hw = <0x01 0x0004>; // v0.2
		opp-hz = /bits/ 64 <1000000000>;
	};

I think this is supposed to say v1.1, v1.2 or v0.2, but not v0.1.

I suppose duplicating the OPP node would also work in my case, but
personally I think this just makes the OPP table unnecessarily hard
to understand - especially when there are many more properties like
interconnects, other required-opps, ...

Wouldn't it be much cleaner to allow setting multiple version ranges
for a single "opp-supported-hw" property? The above could then become:

	opp@1000000000,1000 {
		clock-latency-ns = <400000>;
		opp-supported-hw = <0x02 0x0006>, <0x01 0x004>; // v1.1, v1.2 or v0.2
		opp-hz = /bits/ 64 <1000000000>;
	};

Or is there some other option that I'm missing?

Thanks!
Stephan

Re: opp: How to use multiple opp-supported-hw versions properly?

[Viresh Kumar]

On 25-08-20, 09:44, Stephan Gerhold wrote:
> Hi Viresh,
> 
> (an unrelated questions while I investigate the device links ;) )
> 
> I'm a bit confused about how to use "opp-supported-hw" properly
> when you have multiple versions defined.

Documentation/devicetree/bindings/opp/opp.txt gives some explanation which I
thought should be enough, maybe it isn't :(

Or maybe I am oversimplifying your problem and I myself have forgotten this
property and its usage :)

> In my case I have two version numbers from 0-7, so theoretically up to
> 64 versions. This does not fit into a single version mask

I don't think this is correct. The opp-supported-hw property must be thought of
as a bitfield where each bit represents a version.

In your case a single 32 bit value should be enough IMO.

<0x00FF00FF>, here each FF can support upto 8 versions, which is exactly what
you need. Now if you want to support version 3-3 (where version are numbered
from 0-7) for an OPP, you set this value to: <0x00080008>

Does that make sense ?

> so I added
> them as separate versions to the OPP table.
> 
> Now let's say I want to limit an OPP to v0.1, v1.0 and v1.1, but not
> v0.0. With a single "opp-supported-hw" I think I can only say:
> 
> 	opp-supported-hw = <0x3 0x3>;
> 
> but that does also include v0.0...
> I think to exclude that I would need multiple version ranges, e.g.
> 
> 	opp-supported-hw = <0x1 0x2>, <0x2 0x3>;
> 
> This does not seem to be supported, though.
> 
> I believe a similar situation exists in tegra20-cpu-opp.dtsi:

Dmitry, can you please explain why you were required to have multiple entries
for each OPP ?

-- 
viresh

Re: opp: How to use multiple opp-supported-hw versions properly?

[Stephan Gerhold]

On Tue, Aug 25, 2020 at 01:46:37PM +0530, Viresh Kumar wrote:
> On 25-08-20, 09:44, Stephan Gerhold wrote:
> > Hi Viresh,
> > 
> > (an unrelated questions while I investigate the device links ;) )
> > 
> > I'm a bit confused about how to use "opp-supported-hw" properly
> > when you have multiple versions defined.
> 
> Documentation/devicetree/bindings/opp/opp.txt gives some explanation which I
> thought should be enough, maybe it isn't :(
> 
> Or maybe I am oversimplifying your problem and I myself have forgotten this
> property and its usage :)
> 
> > In my case I have two version numbers from 0-7, so theoretically up to
> > 64 versions. This does not fit into a single version mask
> 
> I don't think this is correct. The opp-supported-hw property must be thought of
> as a bitfield where each bit represents a version.
> 
> In your case a single 32 bit value should be enough IMO.
> 

I took this from Documentation/devicetree/bindings/opp/opp.txt:

"Each level of hierarchy is represented by a 32 bit value, and so there
 can be only 32 different supported version per hierarchy. i.e. 1 bit
 per version. A value of 0xFFFFFFFF will enable the OPP for all versions
 for that hierarchy level. And a value of 0x00000000 will disable the
 OPP completely, and so we never want that to happen."

Also:

"If 32 values aren't sufficient for a version hierarchy, than that
 version hierarchy can be contained in multiple 32 bit values. i.e.
 <X Y Z1 Z2> in the above example, Z1 & Z2 refer to the version
 hierarchy Z."

but it doesn't mention anything about the problem I described
("conflicting" ranges for one of the "sub-versions").

> <0x00FF00FF>, here each FF can support upto 8 versions, which is exactly what
> you need. Now if you want to support version 3-3 (where version are numbered
> from 0-7) for an OPP, you set this value to: <0x00080008>
> 
> Does that make sense ?
> 

I have to admit that I keep getting confused with these bit masks...
I think this is also one option I considered but actually it doesn't
work:

Essentially you suggest to encode a version using:

static int ver(int version1, int version2) {
	return BIT(version1) << 16 | BIT(version2);
}

Now let's take a look at my example again:

> > Now let's say I want to limit an OPP to v0.1, v1.0 and v1.1, but not
> > v0.0. With a single "opp-supported-hw" I think I can only say:
> > 
> > 	opp-supported-hw = <0x3 0x3>;
> > 
> > but that does also include v0.0...
> > I think to exclude that I would need multiple version ranges, e.g.
> > 
> > 	opp-supported-hw = <0x1 0x2>, <0x2 0x3>;
> > 
> > This does not seem to be supported, though.
> > 

So for the versions I mentioned we get:
  - ver(0, 1) = 0x10002
  - ver(1, 0) = 0x20001
  - ver(1, 1) = 0x20002
  - ver(0, 0) = 0x10001 (this one should be excluded)

So to mark v0.1, v1.0 and v1.1 as supported I'd set:
     opp-supported-hw = <(ver(0, 1) | ver(1, 0) | ver(1, 1))>;
i.e. opp-supported-hw = <0x30003>;

Now let's say I'm running v0.0 = 0x10001. This version should not
support the OPP. But actually 0x30003 & 0x10001 = 0x10001 != 0,
so the OPP would consider it as supported.

I tried many different approaches, but you end up being unable to
control subversions independently for most of them.

But as I mentioned I keep getting really confused with these bitmasks,
so maybe I'm wrong :)

Thanks!
Stephan

Re: opp: How to use multiple opp-supported-hw versions properly?

[Viresh Kumar]

On 25-08-20, 10:57, Stephan Gerhold wrote:
> but it doesn't mention anything about the problem I described
> ("conflicting" ranges for one of the "sub-versions").
> 
> I have to admit that I keep getting confused with these bit masks...
> I think this is also one option I considered but actually it doesn't
> work:
> 
> Essentially you suggest to encode a version using:
> 
> static int ver(int version1, int version2) {
> 	return BIT(version1) << 16 | BIT(version2);
> }
> 
> Now let's take a look at my example again:
> 
> So for the versions I mentioned we get:
>   - ver(0, 1) = 0x10002
>   - ver(1, 0) = 0x20001
>   - ver(1, 1) = 0x20002
>   - ver(0, 0) = 0x10001 (this one should be excluded)
> 
> So to mark v0.1, v1.0 and v1.1 as supported I'd set:
>      opp-supported-hw = <(ver(0, 1) | ver(1, 0) | ver(1, 1))>;
> i.e. opp-supported-hw = <0x30003>;
> 
> Now let's say I'm running v0.0 = 0x10001. This version should not
> support the OPP. But actually 0x30003 & 0x10001 = 0x10001 != 0,
> so the OPP would consider it as supported.
> 
> I tried many different approaches, but you end up being unable to
> control subversions independently for most of them.

Okay, the binding doesn't really work well with subversions properly (as I
believed earlier), but because the implementation was simple enough and very
basic we can still work around it in your specific case. I am not sure I want to
touch the bindings at this point of time, unless they aren't workable for
someone.

This should work fine for your case I believe, with two 32 bit words.

static int ver(int version1, int version2)
{
	return BIT(version1 * 8 + version2); //Here 8 is size-of version2
}

-- 
viresh

Re: opp: How to use multiple opp-supported-hw versions properly?

[Stephan Gerhold]

On Tue, Aug 25, 2020 at 03:26:33PM +0530, Viresh Kumar wrote:
> On 25-08-20, 10:57, Stephan Gerhold wrote:
> > but it doesn't mention anything about the problem I described
> > ("conflicting" ranges for one of the "sub-versions").
> > 
> > I have to admit that I keep getting confused with these bit masks...
> > I think this is also one option I considered but actually it doesn't
> > work:
> > 
> > Essentially you suggest to encode a version using:
> > 
> > static int ver(int version1, int version2) {
> > 	return BIT(version1) << 16 | BIT(version2);
> > }
> > 
> > Now let's take a look at my example again:
> > 
> > So for the versions I mentioned we get:
> >   - ver(0, 1) = 0x10002
> >   - ver(1, 0) = 0x20001
> >   - ver(1, 1) = 0x20002
> >   - ver(0, 0) = 0x10001 (this one should be excluded)
> > 
> > So to mark v0.1, v1.0 and v1.1 as supported I'd set:
> >      opp-supported-hw = <(ver(0, 1) | ver(1, 0) | ver(1, 1))>;
> > i.e. opp-supported-hw = <0x30003>;
> > 
> > Now let's say I'm running v0.0 = 0x10001. This version should not
> > support the OPP. But actually 0x30003 & 0x10001 = 0x10001 != 0,
> > so the OPP would consider it as supported.
> > 
> > I tried many different approaches, but you end up being unable to
> > control subversions independently for most of them.
> 
> This should work fine for your case I believe, with two 32 bit words.
> 
> static int ver(int version1, int version2)
> {
> 	return BIT(version1 * 8 + version2); //Here 8 is size-of version2
> }
> 

Actually this does not work properly either, at least if you have
64 possible versions :-)

Let's take version v5.0 for example:

	ver(5, 0) = BIT(5 * 8 + 0) = BIT(40) = <0x00000100 0x00000000>

For each of the two masks the OPP core checks:

	if (!(version & opp_table->supported_hw[count]))
		return false;

The second 0x00000000 will always run into "return false",
because 0 & <something> = 0.

I suppose something like this could work if you need 63 versions only.
For all versions > 32 you would set one particular bit in the second
version, e.g. <0x00000100 0x00000001>, only to make the check happy.

For 64 versions it would work with three 32 bit words, in my case
I could probably ignore one of the versions and fit it into the two
32 bit words with some weird glue code.

But at least in my opinion this is getting to the point where this will
be terribly complicated to understand from looking at the device tree.

> Okay, the binding doesn't really work well with subversions properly (as I
> believed earlier), but because the implementation was simple enough and very
> basic we can still work around it in your specific case. I am not sure I want to
> touch the bindings at this point of time, unless they aren't workable for
> someone.
> 

Note that my original suggestion does not change the behavior of the
bindings for any of the existing uses. It just lets you define multiple
such version ranges for one OPP definition.

We know the number of version masks we want to check
(opp_table->supported_hw_count), then we check the number in the device
tree and if it's a multiple of supported_hw_count we check those ranges
separately.

While it's certainly not ideal to list several version masks there,
I think this would be the most readable and flexible way to handle
the "subversion issue".

Or I could just do it like in tegra20-cpu-opp.dtsi, and specify the OPPs
multiple times with different ranges. It works as-is, it's just quite
a bit of duplication. At least it's quite easy to understand.

What do you think? :)

Thanks!
Stephan

Re: opp: How to use multiple opp-supported-hw versions properly?

[Viresh Kumar]

On 25-08-20, 12:35, Stephan Gerhold wrote:
> Actually this does not work properly either, at least if you have
> 64 possible versions :-)

I must have been very crazy yesterday :)

I think your idea was the only sensible approach and I have sent a patchset with
relevant updates. Please let me know how it looks.

-- 
viresh

Re: opp: How to use multiple opp-supported-hw versions properly?

[Dmitry Osipenko]

25.08.2020 11:16, Viresh Kumar пишет:
> On 25-08-20, 09:44, Stephan Gerhold wrote:
>> Hi Viresh,
>>
>> (an unrelated questions while I investigate the device links ;) )
>>
>> I'm a bit confused about how to use "opp-supported-hw" properly
>> when you have multiple versions defined.
> 
> Documentation/devicetree/bindings/opp/opp.txt gives some explanation which I
> thought should be enough, maybe it isn't :(
> 
> Or maybe I am oversimplifying your problem and I myself have forgotten this
> property and its usage :)
> 
>> In my case I have two version numbers from 0-7, so theoretically up to
>> 64 versions. This does not fit into a single version mask
> 
> I don't think this is correct. The opp-supported-hw property must be thought of
> as a bitfield where each bit represents a version.
> 
> In your case a single 32 bit value should be enough IMO.
> 
> <0x00FF00FF>, here each FF can support upto 8 versions, which is exactly what
> you need. Now if you want to support version 3-3 (where version are numbered
> from 0-7) for an OPP, you set this value to: <0x00080008>
> 
> Does that make sense ?
> 
>> so I added
>> them as separate versions to the OPP table.
>>
>> Now let's say I want to limit an OPP to v0.1, v1.0 and v1.1, but not
>> v0.0. With a single "opp-supported-hw" I think I can only say:
>>
>> 	opp-supported-hw = <0x3 0x3>;
>>
>> but that does also include v0.0...
>> I think to exclude that I would need multiple version ranges, e.g.
>>
>> 	opp-supported-hw = <0x1 0x2>, <0x2 0x3>;
>>
>> This does not seem to be supported, though.
>>
>> I believe a similar situation exists in tegra20-cpu-opp.dtsi:
> 
> Dmitry, can you please explain why you were required to have multiple entries
> for each OPP ?
> 

Like Stephan said correctly, this was done in order to exclude the
unwanted overlaps of the versions.

I wrote a script that generated all the OPP tables and there was no
intention to make output human-friendly because there shouldn't be a
need to change the default OPPs on Tegra ever. It's still possible to
easily specify customized OPPs per board if this will be needed by
anyone because the common_opp.dtsi is included by each board DT
individually and not by the common tegra.dtsi.