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

[Stephan Gerhold <stephan@gerhold.net>]

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