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

[Chanwoo Choi <cwchoi00@gmail.com>]

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

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>;
         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>;
    };
};

-- 
Best Regards,
Chanwoo Choi