On Sat, Jun 22, 2019 at 9:28 PM Chanwoo Choi <cwcho...@gmail.com> wrote: > > Hi, > > 2019년 6월 23일 (일) 오전 6:42, Saravana Kannan <sarava...@google.com>님이 작성: > > > > On Sat, Jun 22, 2019 at 4:50 AM Chanwoo Choi <cwcho...@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 <sarava...@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 <sarava...@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