Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Chanwoo Choi <cwchoi00@gmail.com>
To: Saravana Kannan <saravanak@google.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Android Kernel Team <kernel-team@android.com>,
	Linux PM list <linux-pm@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP
Date: Sun, 23 Jun 2019 13:27:56 +0900
Message-ID: <CAGTfZH2jK8s=5d_R7=kbUsPwE6s3fmz2_srZVyr32EU5qcB07Q@mail.gmail.com> (raw)
In-Reply-To: <CAGETcx9Gi24bng_PCqc6=9S584va4hRc4HHZtBLevKHgYGSNDA@mail.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

  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-22  0:34 [PATCH v1 0/3] Add required-opps support to devfreq passive gov Saravana Kannan
2019-06-22  0:34 ` [PATCH v1 1/3] OPP: Allow required-opps even if the device doesn't have power-domains Saravana Kannan
2019-06-22  0:34 ` [PATCH v1 2/3] OPP: Add function to look up required OPP's for a given OPP Saravana Kannan
2019-06-22 11:49   ` Chanwoo Choi
2019-06-22 21:41     ` Saravana Kannan
2019-06-23  4:27       ` Chanwoo Choi [this message]
2019-06-23  6:07         ` Saravana Kannan
2019-06-22  0:34 ` [PATCH v1 3/3] PM / devfreq: Add required OPPs support to passive governor Saravana Kannan
2019-06-22 12:00   ` Chanwoo Choi
2019-06-22 21:45     ` Saravana Kannan
2019-06-24  9:43 ` [PATCH v1 0/3] Add required-opps support to devfreq passive gov Viresh Kumar
2019-06-24 22:17   ` Saravana Kannan
2019-06-25  4:10     ` Viresh Kumar
2019-06-25  5:00       ` Saravana Kannan
2019-06-25  5:22         ` Viresh Kumar
2019-06-25  5:29           ` Saravana Kannan
2019-06-26  6:32             ` Viresh Kumar
2019-06-26 18:10               ` Saravana Kannan
2019-06-28  6:49                 ` Viresh Kumar
2019-06-28 20:26                   ` Saravana Kannan
2019-07-11 23:16                     ` Saravana Kannan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGTfZH2jK8s=5d_R7=kbUsPwE6s3fmz2_srZVyr32EU5qcB07Q@mail.gmail.com' \
    --to=cwchoi00@gmail.com \
    --cc=cw00.choi@samsung.com \
    --cc=kernel-team@android.com \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=myungjoo.ham@samsung.com \
    --cc=nm@ti.com \
    --cc=rjw@rjwysocki.net \
    --cc=saravanak@google.com \
    --cc=sboyd@kernel.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox