[v3,3/5] OPP: Improve require-opps linking
diff mbox series

Message ID 20190717222340.137578-4-saravanak@google.com
State Superseded, archived
Headers show
Series
  • Add required-opps support to devfreq passive gov
Related show

Commit Message

Saravana Kannan July 17, 2019, 10:23 p.m. UTC
Currently, the linking of required-opps fails silently if the
destination OPP table hasn't been added before the source OPP table is
added. This puts an unnecessary requirement that the destination table
be added before the source table is added.

In reality, the destination table is needed only when we try to
translate from source OPP to destination OPP. So, instead of
completely failing, retry linking the tables when the translation is
attempted.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/opp/core.c | 32 +++++++++++-----
 drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
 drivers/opp/opp.h  |  5 +++
 3 files changed, 71 insertions(+), 57 deletions(-)

Comments

Viresh Kumar July 23, 2019, 10:28 a.m. UTC | #1
$subject doesn't have correct property name.

On 17-07-19, 15:23, Saravana Kannan wrote:
> Currently, the linking of required-opps fails silently if the
> destination OPP table hasn't been added before the source OPP table is
> added. This puts an unnecessary requirement that the destination table
> be added before the source table is added.
> 
> In reality, the destination table is needed only when we try to
> translate from source OPP to destination OPP. So, instead of
> completely failing, retry linking the tables when the translation is
> attempted.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/opp/core.c | 32 +++++++++++-----
>  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
>  drivers/opp/opp.h  |  5 +++
>  3 files changed, 71 insertions(+), 57 deletions(-)

Here is the general feedback and requirements I have:

- We shouldn't do it from _set_required_opps() but way earlier, it
  shouldn't affect the fast path (where we change the frequency).

- Programming required-opps for half of the properties isn't correct,
  i.e. in case only few of the required-opps are parsed until now. So
  setting of rate shouldn't even start unless the OPP table is fully
  initialized with all required-opps in it.
Saravana Kannan July 23, 2019, 8:36 p.m. UTC | #2
Resending again due to accidental HTML (minor rewording/typo fixes too).

On Tue, Jul 23, 2019 at 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> $subject doesn't have correct property name.
>
> On 17-07-19, 15:23, Saravana Kannan wrote:
> > Currently, the linking of required-opps fails silently if the
> > destination OPP table hasn't been added before the source OPP table is
> > added. This puts an unnecessary requirement that the destination table
> > be added before the source table is added.
> >
> > In reality, the destination table is needed only when we try to
> > translate from source OPP to destination OPP. So, instead of
> > completely failing, retry linking the tables when the translation is
> > attempted.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/opp/core.c | 32 +++++++++++-----
> >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> >  drivers/opp/opp.h  |  5 +++
> >  3 files changed, 71 insertions(+), 57 deletions(-)
>
> Here is the general feedback and requirements I have:
>
> - We shouldn't do it from _set_required_opps() but way earlier, it
>   shouldn't affect the fast path (where we change the frequency).

I don't think there's any other intermediate OPP API call where we can
try to lazily link the tables. Also if the tables are already fully
linked, I don't think this is really going to slow down the fast path
in anyway (because it's just a few NULL checks).

If you have other ideas, I'm willing to look at it. But as it is right
now seems the best to me.

> - Programming required-opps for half of the properties isn't correct,
>   i.e. in case only few of the required-opps are parsed until now. So
>   setting of rate shouldn't even start unless the OPP table is fully
>   initialized with all required-opps in it.

Ok, I can fix this.

-Saravana
Viresh Kumar July 25, 2019, 3:07 a.m. UTC | #3
On 23-07-19, 07:47, Saravana Kannan wrote:
> On Tue, Jul 23, 2019, 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > $subject doesn't have correct property name.
> >
> > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > Currently, the linking of required-opps fails silently if the
> > > destination OPP table hasn't been added before the source OPP table is
> > > added. This puts an unnecessary requirement that the destination table
> > > be added before the source table is added.
> > >
> > > In reality, the destination table is needed only when we try to
> > > translate from source OPP to destination OPP. So, instead of
> > > completely failing, retry linking the tables when the translation is
> > > attempted.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  drivers/opp/core.c | 32 +++++++++++-----
> > >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> > >  drivers/opp/opp.h  |  5 +++
> > >  3 files changed, 71 insertions(+), 57 deletions(-)
> >
> > Here is the general feedback and requirements I have:
> >
> > - We shouldn't do it from _set_required_opps() but way earlier, it
> >   shouldn't affect the fast path (where we change the frequency).
> >
> 
> I don't think there's any other intermediate OPP call where we can try to
> link the tables. Also if there tables are already fully linked, this is
> just checking for not NULL for a few elements in the array.

We should be doing this whenever a new OPP table is created, and see
if someone else was waiting for this OPP table to come alive. Also we
must make sure that we do this linking only if the new OPP table has
its own required-opps links fixed, otherwise delay further.

> I don't think
> this is really going to allow down the fast path in anyway.

> If you have other ideas, I'm willing to look at it. But as it is right now
> seems the best to me.
> 
Even then I don't want to add these checks to those places. For the
opp-set-rate routine, add another flag to the OPP table which
indicates if we are ready to do dvfs or not and mark it true only
after the required-opps are all set.
Saravana Kannan July 25, 2019, 4:09 a.m. UTC | #4
On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 07:47, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019, 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > $subject doesn't have correct property name.
> > >
> > > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > > Currently, the linking of required-opps fails silently if the
> > > > destination OPP table hasn't been added before the source OPP table is
> > > > added. This puts an unnecessary requirement that the destination table
> > > > be added before the source table is added.
> > > >
> > > > In reality, the destination table is needed only when we try to
> > > > translate from source OPP to destination OPP. So, instead of
> > > > completely failing, retry linking the tables when the translation is
> > > > attempted.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/opp/core.c | 32 +++++++++++-----
> > > >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> > > >  drivers/opp/opp.h  |  5 +++
> > > >  3 files changed, 71 insertions(+), 57 deletions(-)
> > >
> > > Here is the general feedback and requirements I have:
> > >
> > > - We shouldn't do it from _set_required_opps() but way earlier, it
> > >   shouldn't affect the fast path (where we change the frequency).
> > >
> >
> > I don't think there's any other intermediate OPP call where we can try to
> > link the tables. Also if there tables are already fully linked, this is
> > just checking for not NULL for a few elements in the array.
>
> We should be doing this whenever a new OPP table is created, and see
> if someone else was waiting for this OPP table to come alive.

Searching the global OPP table list seems a ton more wasteful than
doing the lazy linking. I'd rather not do this.

> Also we
> must make sure that we do this linking only if the new OPP table has
> its own required-opps links fixed, otherwise delay further.

This can be done. Although even without doing that, this patch is
making things better by not failing silently like it does today? Can I
do this later as a separate patch set series?

> > I don't think
> > this is really going to allow down the fast path in anyway.
>
> > If you have other ideas, I'm willing to look at it. But as it is right now
> > seems the best to me.
> >
> Even then I don't want to add these checks to those places. For the
> opp-set-rate routine, add another flag to the OPP table which
> indicates if we are ready to do dvfs or not and mark it true only
> after the required-opps are all set.

Honestly, this seems like extra memory and micro optimization without
any data to back it. Show me data that checking all these table
pointers is noticeably slower than what I'm doing. What's the max
"required tables count" you've seen in upstream so far?

I'd even argue that doing it the way I do might actually reduce the
cache misses/warm the cache because those pointers are going to be
searched/used right after anyway.

-Saravana
Viresh Kumar July 25, 2019, 5:17 a.m. UTC | #5
On 24-07-19, 21:09, Saravana Kannan wrote:
> On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > We should be doing this whenever a new OPP table is created, and see
> > if someone else was waiting for this OPP table to come alive.
> 
> Searching the global OPP table list seems a ton more wasteful than
> doing the lazy linking. I'd rather not do this.

We can see how best to optimize that, but it will be done only once
while a new OPP table is created and putting stress there is the right
thing to do IMO. And doing anything like that in a place like
opp-set-rate is the worst one. It will be a bad choice by design if
you ask me and so I am very much against that.

> > Also we
> > must make sure that we do this linking only if the new OPP table has
> > its own required-opps links fixed, otherwise delay further.
> 
> This can be done. Although even without doing that, this patch is
> making things better by not failing silently like it does today? Can I
> do this later as a separate patch set series?

I would like this to get fixed now in a proper way, there is no hurry
for a quick fix currently. No band-aids please.

> > Even then I don't want to add these checks to those places. For the
> > opp-set-rate routine, add another flag to the OPP table which
> > indicates if we are ready to do dvfs or not and mark it true only
> > after the required-opps are all set.
> 
> Honestly, this seems like extra memory and micro optimization without
> any data to back it.

Again, opp-set-rate isn't supposed to do something like this. It
shouldn't handle initializations of things, that is broken design.

> Show me data that checking all these table
> pointers is noticeably slower than what I'm doing. What's the max
> "required tables count" you've seen in upstream so far?

Running anything extra (specially some initialization stuff) in
opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
is my responsibility to not allow such things to happen.

> I'd even argue that doing it the way I do might actually reduce the
> cache misses/warm the cache because those pointers are going to be
> searched/used right after anyway.

So you want to make the cache hot with data, by running some code at a
place where it is not required to be run really, and the fact that
most of the data cached may not get used anyway ? And that is an
improvement somehow ?
Saravana Kannan July 26, 2019, 1:52 a.m. UTC | #6
On Wed, Jul 24, 2019 at 10:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 24-07-19, 21:09, Saravana Kannan wrote:
> > On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > We should be doing this whenever a new OPP table is created, and see
> > > if someone else was waiting for this OPP table to come alive.
> >
> > Searching the global OPP table list seems a ton more wasteful than
> > doing the lazy linking. I'd rather not do this.
>
> We can see how best to optimize that, but it will be done only once
> while a new OPP table is created and putting stress there is the right
> thing to do IMO. And doing anything like that in a place like
> opp-set-rate is the worst one. It will be a bad choice by design if
> you ask me and so I am very much against that.
>
> > > Also we
> > > must make sure that we do this linking only if the new OPP table has
> > > its own required-opps links fixed, otherwise delay further.
> >
> > This can be done. Although even without doing that, this patch is
> > making things better by not failing silently like it does today? Can I
> > do this later as a separate patch set series?
>
> I would like this to get fixed now in a proper way, there is no hurry
> for a quick fix currently. No band-aids please.
>
> > > Even then I don't want to add these checks to those places. For the
> > > opp-set-rate routine, add another flag to the OPP table which
> > > indicates if we are ready to do dvfs or not and mark it true only
> > > after the required-opps are all set.
> >
> > Honestly, this seems like extra memory and micro optimization without
> > any data to back it.
>
> Again, opp-set-rate isn't supposed to do something like this. It
> shouldn't handle initializations of things, that is broken design.
>
> > Show me data that checking all these table
> > pointers is noticeably slower than what I'm doing. What's the max
> > "required tables count" you've seen in upstream so far?
>
> Running anything extra (specially some initialization stuff) in
> opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
> is my responsibility to not allow such things to happen.

Doing operations lazily right before they are needed isn't something
new in the kernel. It's done all over the place (VFP save/restore?).
It's not worth arguing though -- so I'll agree to disagree but follow
the Maintainer's preference.

> > I'd even argue that doing it the way I do might actually reduce the
> > cache misses/warm the cache because those pointers are going to be
> > searched/used right after anyway.
>
> So you want to make the cache hot with data, by running some code at a
> place where it is not required to be run really, and the fact that
> most of the data cached may not get used anyway ? And that is an
> improvement somehow ?

My point is that both of us are hypothesizing and for some
micro-optimization like this, data is needed.

-Saravana
Saravana Kannan July 26, 2019, 9:23 p.m. UTC | #7
On Thu, Jul 25, 2019 at 6:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jul 24, 2019 at 10:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 24-07-19, 21:09, Saravana Kannan wrote:
> > > On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > We should be doing this whenever a new OPP table is created, and see
> > > > if someone else was waiting for this OPP table to come alive.
> > >
> > > Searching the global OPP table list seems a ton more wasteful than
> > > doing the lazy linking. I'd rather not do this.
> >
> > We can see how best to optimize that, but it will be done only once
> > while a new OPP table is created and putting stress there is the right
> > thing to do IMO. And doing anything like that in a place like
> > opp-set-rate is the worst one. It will be a bad choice by design if
> > you ask me and so I am very much against that.
> >
> > > > Also we
> > > > must make sure that we do this linking only if the new OPP table has
> > > > its own required-opps links fixed, otherwise delay further.
> > >
> > > This can be done. Although even without doing that, this patch is
> > > making things better by not failing silently like it does today? Can I
> > > do this later as a separate patch set series?
> >
> > I would like this to get fixed now in a proper way, there is no hurry
> > for a quick fix currently. No band-aids please.
> >
> > > > Even then I don't want to add these checks to those places. For the
> > > > opp-set-rate routine, add another flag to the OPP table which
> > > > indicates if we are ready to do dvfs or not and mark it true only
> > > > after the required-opps are all set.
> > >
> > > Honestly, this seems like extra memory and micro optimization without
> > > any data to back it.
> >
> > Again, opp-set-rate isn't supposed to do something like this. It
> > shouldn't handle initializations of things, that is broken design.
> >
> > > Show me data that checking all these table
> > > pointers is noticeably slower than what I'm doing. What's the max
> > > "required tables count" you've seen in upstream so far?
> >
> > Running anything extra (specially some initialization stuff) in
> > opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
> > is my responsibility to not allow such things to happen.
>
> Doing operations lazily right before they are needed isn't something
> new in the kernel. It's done all over the place (VFP save/restore?).
> It's not worth arguing though -- so I'll agree to disagree but follow
> the Maintainer's preference.
>

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"?

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?

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.

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.

-Saravana
Viresh Kumar July 29, 2019, 7:26 a.m. UTC | #8
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.
Hsin-Yi Wang July 30, 2019, 11:02 p.m. UTC | #9
On Wed, Jul 17, 2019 at 10:23 PM Saravana Kannan <saravanak@google.com> wrote:

> -free_required_tables:
> -       _opp_table_free_required_tables(opp_table);
> -put_np:
> -       of_node_put(np);
> +       for (i = 0; i < src->required_opp_count; i++) {
> +               if (src->required_opp_tables[i])
> +                       continue;
> +
> +               req_np = of_parse_required_opp(src_opp->np, i);
> +               if (!req_np)
> +                       continue;
> +
> +               req_table = _find_table_of_opp_np(req_np);
Not yet tested in v4, but in v3:
In _find_table_of_opp_np(), there's a lockdep check:
lockdep_assert_held(&opp_table_lock);
which would lead to lockdep warnings.

Call trace:
_find_table_of_opp_np
_of_lazy_link_required_tables
dev_pm_opp_xlate_opp
Saravana Kannan July 30, 2019, 11:05 p.m. UTC | #10
On Tue, Jul 30, 2019 at 4:03 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Wed, Jul 17, 2019 at 10:23 PM Saravana Kannan <saravanak@google.com> wrote:
>
> > -free_required_tables:
> > -       _opp_table_free_required_tables(opp_table);
> > -put_np:
> > -       of_node_put(np);
> > +       for (i = 0; i < src->required_opp_count; i++) {
> > +               if (src->required_opp_tables[i])
> > +                       continue;
> > +
> > +               req_np = of_parse_required_opp(src_opp->np, i);
> > +               if (!req_np)
> > +                       continue;
> > +
> > +               req_table = _find_table_of_opp_np(req_np);
> Not yet tested in v4, but in v3:
> In _find_table_of_opp_np(), there's a lockdep check:
> lockdep_assert_held(&opp_table_lock);
> which would lead to lockdep warnings.
>
> Call trace:
> _find_table_of_opp_np
> _of_lazy_link_required_tables
> dev_pm_opp_xlate_opp

Thanks for testing. I'll keep this in mind based on where the rest of
the discussion goes.

-Saravana

Patch
diff mbox series

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 72c055a3f6b7..cafe6ec05d6c 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -706,8 +706,11 @@  static int _set_required_opps(struct device *dev,
 	if (!required_opp_tables)
 		return 0;
 
+	_of_lazy_link_required_tables(opp_table);
+
 	/* Single genpd case */
-	if (!genpd_virt_devs && required_opp_tables[0]->is_genpd) {
+	if (!genpd_virt_devs && required_opp_tables[0]
+			     && required_opp_tables[0]->is_genpd) {
 		pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
 		if (ret) {
@@ -726,11 +729,16 @@  static int _set_required_opps(struct device *dev,
 	mutex_lock(&opp_table->genpd_virt_dev_lock);
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
-		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
-
 		if (!genpd_virt_devs[i])
 			continue;
 
+		if (!opp->required_opps[i]) {
+			ret = -ENODEV;
+			break;
+		}
+
+		pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
+
 		ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i], pstate);
 		if (ret) {
 			dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
@@ -1907,8 +1915,11 @@  struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
 	if (!src_table || !dst_table || !src_opp)
 		return NULL;
 
+	_of_lazy_link_required_tables(src_table);
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
-		if (src_table->required_opp_tables[i]->np == dst_table->np)
+		if (src_table->required_opp_tables[i]
+		    && src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
 	}
 
@@ -1971,6 +1982,8 @@  int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 	if (!src_table->required_opp_count)
 		return pstate;
 
+	_of_lazy_link_required_tables(src_table);
+
 	for (i = 0; i < src_table->required_opp_count; i++) {
 		if (src_table->required_opp_tables[i]->np == dst_table->np)
 			break;
@@ -1986,15 +1999,16 @@  int dev_pm_opp_xlate_performance_state(struct opp_table *src_table,
 
 	list_for_each_entry(opp, &src_table->opp_list, node) {
 		if (opp->pstate == pstate) {
-			dest_pstate = opp->required_opps[i]->pstate;
-			goto unlock;
+			if (opp->required_opps[i])
+				dest_pstate = opp->required_opps[i]->pstate;
+			break;
 		}
 	}
 
-	pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__, src_table,
-	       dst_table);
+	if (dest_pstate < 0)
+		pr_err("%s: Couldn't find matching OPP (%p: %p)\n", __func__,
+		       src_table, dst_table);
 
-unlock:
 	mutex_unlock(&src_table->lock);
 
 	return dest_pstate;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ff88eaf66b56..4e527245fd59 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -145,7 +145,7 @@  static void _opp_table_free_required_tables(struct opp_table *opp_table)
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (IS_ERR_OR_NULL(required_opp_tables[i]))
-			break;
+			continue;
 
 		dev_pm_opp_put_opp_table(required_opp_tables[i]);
 	}
@@ -165,8 +165,8 @@  static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 					     struct device_node *opp_np)
 {
 	struct opp_table **required_opp_tables;
-	struct device_node *required_np, *np;
-	int count, i;
+	struct device_node *np;
+	int count;
 
 	/* Traversing the first OPP node is all we need */
 	np = of_get_next_available_child(opp_np, NULL);
@@ -176,35 +176,57 @@  static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	}
 
 	count = of_count_phandle_with_args(np, "required-opps", NULL);
+	of_node_put(np);
 	if (!count)
-		goto put_np;
+		return;
 
 	required_opp_tables = kcalloc(count, sizeof(*required_opp_tables),
 				      GFP_KERNEL);
 	if (!required_opp_tables)
-		goto put_np;
+		return;
 
 	opp_table->required_opp_tables = required_opp_tables;
 	opp_table->required_opp_count = count;
+}
 
-	for (i = 0; i < count; i++) {
-		required_np = of_parse_required_opp(np, i);
-		if (!required_np)
-			goto free_required_tables;
+void _of_lazy_link_required_tables(struct opp_table *src)
+{
+	struct dev_pm_opp *src_opp, *tmp_opp;
+	struct opp_table *req_table;
+	struct device_node *req_np;
+	int i;
 
-		required_opp_tables[i] = _find_table_of_opp_np(required_np);
-		of_node_put(required_np);
+	mutex_lock(&src->lock);
 
-		if (IS_ERR(required_opp_tables[i]))
-			goto free_required_tables;
-	}
+	if (list_empty(&src->opp_list))
+		goto out;
 
-	goto put_np;
+	src_opp = list_first_entry(&src->opp_list, struct dev_pm_opp, node);
 
-free_required_tables:
-	_opp_table_free_required_tables(opp_table);
-put_np:
-	of_node_put(np);
+	for (i = 0; i < src->required_opp_count; i++) {
+		if (src->required_opp_tables[i])
+			continue;
+
+		req_np = of_parse_required_opp(src_opp->np, i);
+		if (!req_np)
+			continue;
+
+		req_table = _find_table_of_opp_np(req_np);
+		of_node_put(req_np);
+		if (!req_table)
+			continue;
+
+		src->required_opp_tables[i] = req_table;
+		list_for_each_entry(tmp_opp, &src->opp_list, node) {
+			req_np = of_parse_required_opp(tmp_opp->np, i);
+			tmp_opp->required_opps[i] = _find_opp_of_np(req_table,
+								    req_np);
+			of_node_put(req_np);
+		}
+	}
+
+out:
+	mutex_unlock(&src->lock);
 }
 
 void _of_init_opp_table(struct opp_table *opp_table, struct device *dev,
@@ -267,7 +289,7 @@  void _of_opp_free_required_opps(struct opp_table *opp_table,
 
 	for (i = 0; i < opp_table->required_opp_count; i++) {
 		if (!required_opps[i])
-			break;
+			continue;
 
 		/* Put the reference back */
 		dev_pm_opp_put(required_opps[i]);
@@ -282,9 +304,7 @@  static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 				       struct dev_pm_opp *opp)
 {
 	struct dev_pm_opp **required_opps;
-	struct opp_table *required_table;
-	struct device_node *np;
-	int i, ret, count = opp_table->required_opp_count;
+	int count = opp_table->required_opp_count;
 
 	if (!count)
 		return 0;
@@ -295,32 +315,7 @@  static int _of_opp_alloc_required_opps(struct opp_table *opp_table,
 
 	opp->required_opps = required_opps;
 
-	for (i = 0; i < count; i++) {
-		required_table = opp_table->required_opp_tables[i];
-
-		np = of_parse_required_opp(opp->np, i);
-		if (unlikely(!np)) {
-			ret = -ENODEV;
-			goto free_required_opps;
-		}
-
-		required_opps[i] = _find_opp_of_np(required_table, np);
-		of_node_put(np);
-
-		if (!required_opps[i]) {
-			pr_err("%s: Unable to find required OPP node: %pOF (%d)\n",
-			       __func__, opp->np, i);
-			ret = -ENODEV;
-			goto free_required_opps;
-		}
-	}
-
 	return 0;
-
-free_required_opps:
-	_of_opp_free_required_opps(opp_table, opp);
-
-	return ret;
 }
 
 static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 01a500e2c40a..5b074eb7da07 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -225,12 +225,17 @@  void _of_clear_opp_table(struct opp_table *opp_table);
 struct opp_table *_managed_opp(struct device *dev, int index);
 void _of_opp_free_required_opps(struct opp_table *opp_table,
 				struct dev_pm_opp *opp);
+void _of_lazy_link_required_tables(struct opp_table *src);
 #else
 static inline void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, int index) {}
 static inline void _of_clear_opp_table(struct opp_table *opp_table) {}
 static inline struct opp_table *_managed_opp(struct device *dev, int index) { return NULL; }
 static inline void _of_opp_free_required_opps(struct opp_table *opp_table,
 					      struct dev_pm_opp *opp) {}
+void bool _of_lazy_link_required_tables(struct opp_table *src)
+{
+	return false;
+}
 #endif
 
 #ifdef CONFIG_DEBUG_FS