Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Viresh Kumar <viresh.kumar@linaro.org>]

On 26-07-19, 14:23, Saravana Kannan wrote:
> I was taking a closer look at the OPP framework code to try and do
> what you ask above, but it's kind of a mess. The whole "the same OPP
> table can be used by multiple devices without the opp-shared flag set"
> is effectively breaking "required-opps" at a minimum and probably a
> lot more cases. I don't think I can rewrite my patch the way you want
> it without fixing the existing bugs.
> 
> Let's take this example DT (leaving out the irrelevant part):
> 
> OPP table 1:
>     required-opps = <OPP table 2 entry>;
> 
> OPP table 2:
>     <opp-shared property not set>
> 
> Device A:
>     operating-points-v2 = <&OPP table 1>
> 
> Device B:
>     operating-points-v2 = <&OPP table 2>
> 
> Device C:
>     operating-points-v2 = <&OPP table 2>
> 
> Let's say device B and C add their OPP tables. They both get their own
> "in-memory" copy of the OPP table. They can then enabled/disable
> different OPP entries (rows) and not affect each other's OPP table.
> Which is how it's expected to work.
> 
> Now if device A adds its OPP table 1, the "in-memory"
> required_opp_tables pointer of OPP table 1 can end up pointing to
> either Device A's copy of the OPP table or Device B's copy of the OPP
> table depending on which happens to be added first. This effectively
> random linking of OPP tables is mutually exclusive to the point of
> required-opps.

> Also, at a DT definition level, OPP table 1 pointing to OPP table 2
> when OPP table 2 is used by more than one device doesn't make any
> sense. Which device/genpd is OPP table 1 saying it "requires to
> operate at a certain level"?

I will say that this data is present at least for the present set of
users, i.e. power domains. Just that it isn't present so directly
within the OPP table. But if you look at the device's node, it will
point you to some power domain, etc.

I didn't do anything about it earlier because it required more work
and I thought "required-opps" property is there to get us some data
and it does get that data to us right now. Just that we don't know the
device for which this data is there in the OPP core.

If we do want to get this linking, how should we get it ?

- Using another property in device's DT node, like
  "required-opp-devices" ? I didn't like it earlier because that would
  be like duplicating data we already had for the power domains.

- Using some in-kernel function, using which other drivers can give us
  this data ? Maybe yes. Probably that's the best way of doing it ?

> So I propose that we should consider the OPP table DT configuration
> invalid if one OPP table points to another OPP tables that's NOT
> shared but is ALSO pointed to by multiple devices. Basically the
> example above would be considered an invalid DT configuration. Does
> that sound okay to you? If I make changes to enforce that, will that
> be acceptable?

I don't think that would be the right thing to do as the idea of
sharing the DT OPP tables to avoid duplicate tables in DT was the
correct thing to do and is getting used very much right now as well.

Perhaps we should fix the problem instead.

> If this sounds okay to you, then in the example above, assume Device C
> isn't present. Then when OPP table 1 is added by device A, if OPP
> table 2 hasn't been added already, I can just go ahead and allocate
> OPP table 2. And then when device B tries to add OPP table 2, I can
> just tie device B to OPP table 2 and fill up any of the missing
> pieces.

I can assure you that it would be a big mess. Specially the reference
counting using which we free OPP tables and OPPs right now will come
in your way.

> This sounds better than trying to loop through existing OPP tables and
> seeing if any other table is waiting for the newly added table and
> marking the waiting tables as "linked". Especially because it gets a
> lot more complicated and inefficient when you consider a chain of OPP
> tables and many-to-many linking of OPP tables.

Firstly, this is all going to be initialization code and will not run
after boot normally. And we will probably not have very complex
linking cases as well I believe. Maybe 2-3 levels at the best. And
this will keep code much simpler compared to the above idea.

So here is the thing. I think you should separate out the lazy-linking
thing from this patchset as this is a completely different problem you
are trying to solve and is unnecessarily blocking other patches.

And if you don't want to get into solving the lazy linking thing
yourself, because that is consuming your time unnecessarily, then I
can see if I can put some of my cycles on it.

-- 
viresh