Re: [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP

[Saravana Kannan <saravanak@google.com>]

On Sat, Jun 22, 2019 at 9:28 PM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> Hi,
>
> 2019년 6월 23일 (일) 오전 6:42, Saravana Kannan <saravanak@google.com>님이 작성:
> >
> > On Sat, Jun 22, 2019 at 4:50 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > Absolutely, I like this approach. I think that it is necessary to make
> > > the connection
> > > between frequencies of devices.
> >
> > Happy to hear that.
> >
> > > But, I have a question on below.
> > >
> > > 2019년 6월 22일 (토) 오전 9:35, Saravana Kannan <saravanak@google.com>님이 작성:
> > > >
> > > > Add a function that allows looking up required OPPs given a source OPP
> > > > table, destination OPP table and the source OPP.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/opp/core.c     | 54 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/pm_opp.h | 11 +++++++++
> > > >  2 files changed, 65 insertions(+)
> > > >
> > > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > > index 74c7bdc6f463..4f7870bffbf8 100644
> > > > --- a/drivers/opp/core.c
> > > > +++ b/drivers/opp/core.c
> > > > @@ -1830,6 +1830,60 @@ void dev_pm_opp_put_genpd_virt_dev(struct opp_table *opp_table,
> > > >                 dev_err(virt_dev, "Failed to find required device entry\n");
> > > >  }
> > > >
> > > > +/**
> > > > + * dev_pm_opp_xlate_opp() - Find required OPP for src_table OPP.
> > > > + * @src_table: OPP table which has dst_table as one of its required OPP table.
> > > > + * @dst_table: Required OPP table of the src_table.
> > > > + * @pstate: OPP of the src_table.
> > > > + *
> > > > + * This function returns the OPP (present in @dst_table) pointed out by the
> > > > + * "required-opps" property of the OPP (present in @src_table).
> > > > + *
> > > > + * The callers are required to call dev_pm_opp_put() for the returned OPP after
> > > > + * use.
> > > > + *
> > > > + * Return: destination table OPP on success, otherwise NULL on errors.
> > > > + */
> > > > +struct dev_pm_opp *dev_pm_opp_xlate_opp(struct opp_table *src_table,
> > > > +                                       struct opp_table *dst_table,
> > > > +                                       struct dev_pm_opp *src_opp)
> > > > +{
> > > > +       struct dev_pm_opp *opp, *dest_opp = NULL;
> > > > +       int i;
> > > > +
> > > > +       if (!src_table || !dst_table || !src_opp)
> > > > +               return NULL;
> > > > +
> > > > +       for (i = 0; i < src_table->required_opp_count; i++) {
> > > > +               if (src_table->required_opp_tables[i]->np == dst_table->np)
> > > > +                       break;
> > > > +       }
> > > > +
> > > > +       if (unlikely(i == src_table->required_opp_count)) {
> > > > +               pr_err("%s: Couldn't find matching OPP table (%p: %p)\n",
> > > > +                      __func__, src_table, dst_table);
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       mutex_lock(&src_table->lock);
> > > > +
> > > > +       list_for_each_entry(opp, &src_table->opp_list, node) {
> > > > +               if (opp == src_opp) {
> > > > +                       dest_opp = opp->required_opps[i];
> > >
> > > Correct me if I am wrong. This patch assume that 'i' index is same on between
> > > [1] and [2]. But in order to guarantee this assumption, all OPP entries
> > > in the same opp_table have to have the same number of 'required-opps' properties
> > > and keep the sequence among 'required-opps' entries.
> > >
> > > [1] src_table->required_opp_tables[i]->np
> > > [2] opp->required_opps[I];
> > >
> > > For example, three OPP entries in the 'parent_bus_opp'
> > > have the different sequence of 'required-opps' and the different
> > > number of 'required-opps'. Is it no problem?
> > >
> > > parent_bus_opp: opp_table {
> > >     compatible = "operating-points-v2";
> > >
> > >     opp2 {
> > >         opp-hz = /bits/ 64 <200000>;
> > >         required-opps = <&child_bus_a_opp2>, <&child_bus_b_opp2>,
> > > <&child_bus_c_opp2>;
> > >     };
> > >
> > >     opp1 {
> > >         opp-hz = /bits/ 64 <200000>;
> > >         // change the sequence between child_bus_b_opp2  and child_bus_c_opp2
> > >         required-opps = <&child_bus_a_opp2>, <&child_bus_c_opp2>,
> > > <&child_bus_b_opp2>
> > >     };
> > >
> > >     opp0 {
> > >         opp-hz = /bits/ 64 <200000>;
> > >         // missing 'child_bus_a_opp2'
> > >         required-opps = <&child_bus_c_opp2>, <&child_bus_b_opp2>
> > >     };
> > >
> > > }
> > >
> >
> > I get your question. If I'm not mistaken the OPP framework DT parsing
> > code makes the assumption that the required-opps list has the phandles
> > in the same order for each "row" in the OPP table. It actually only
> > looks at the first OPP entry to figure out the list of required OPP
> > tables.
>
> Thanks for description. It is the limitation of 'required-opps' until now.
>
> >
> > Technically one can write code to deal with random order of the
> > required-opp list, but doesn't seem like that's worth it because
> > there's no need to have that order all mixed up in DT. And even if
> > someone wants to add support for that, I don't think improving the DT
> > parsing to handle random order would be part of this patch series.
>
> I understand the existing ' required-opps' only consider the same sequence
> of entries which are included in the same OPP table.
>
> One more thing, 'required-opps' properties doesn't support
> the other OPP enters of the different OPP table. Is it right of 'required-opps'?

Not sure I fully understand the question.

> Except for the random order, just each OPP might will requires
> the different 'required-opps' of different OPP table. Even if it is
> not related to random order, I think that this approach cannot
> support them.
>
> For example as following:
> - opp2 used the OPP entries of 'child_bus_A' and 'child_bus_B' opp-table.
> - opp1 used the OPP entries of 'child_bus_C' and 'child_bus_D' opp-table.
>
> parent_bus_opp: opp_table {
>     compatible = "operating-points-v2";
>
>      opp2 {
>          opp-hz = /bits/ 64 <200000>;

I'm guessing this is a typo and let's assume you meant to sat 400000

>          required-opps = <&child_bus_A_opp2>, <&child_bus_B_opp2>;
>     };
>
>    opp1 {
>          opp-hz = /bits/ 64 <200000>;
>          required-opps = <&child_bus_C_opp0>, <&child_bus_D_opp0>;
>     };
> };

Is this a real use case? If it is, in reality parent_bus_opp_table
always has requirements on all 4 children bus, just that opp1 is okay
with the lowest frequency for some of the children?

So, in this example, you just need to always list all 4 child OPPs for
each parent OPP. And some of the children OPP values might not change
when going from one parent OPP to another.

-Saravana