On 5/31/21 4:42 PM, Hsin-Yi Wang wrote:

On Mon, May 31, 2021 at 3:37 PM Chanwoo Choi <cw00.choi@samsung.com <mailto:cw00.choi@samsung.com>> wrote:

    Hi,

    On 5/31/21 12:22 PM, andrew-sh.cheng wrote:
    > On Wed, 2021-05-26 at 12:08 +0900, Chanwoo Choi wrote:
    >> Hi,
    >> On 5/26/21 11:22 AM, andrew-sh.cheng wrote:
    >>> On Thu, 2021-04-08 at 11:47 +0900, Chanwoo Choi wrote:
    >>>> On 4/1/21 9:16 AM, Chanwoo Choi wrote:
    >>>>> On 3/31/21 10:03 PM, andrew-sh.cheng wrote:
    >>>>>> On Wed, 2021-03-31 at 17:35 +0900, Chanwoo Choi wrote:
    >>>>>>> On 3/31/21 5:27 PM, Chanwoo Choi wrote:
    >>>>>>>> Hi,
    >>>>>>>>
    >>>>>>>> On 3/31/21 5:03 PM, andrew-sh.cheng wrote:
    >>>>>>>>> On Thu, 2021-03-25 at 17:14 +0900, Chanwoo Choi wrote:
    >>>>>>>>>> Hi,
    >>>>>>>>>>
    >>>>>>>>>> You are missing to add these patches to linux-pm mailing list.
    >>>>>>>>>> Need to send them to linu-pm ML.
    >>>>>>>>>>
    >>>>>>>>>> Also, before received this series, I tried to clean-up these patches
    >>>>>>>>>> on testing branch[1]. So that I add my comment with my clean-up case.
    >>>>>>>>>> [1] https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing-passive-gov__;!!CTRNKA9wMg0ARbw!zIrzeDp9vPnm1_SDzVPuzqdHn3zWie9DnfBXaA-j9-CSrVc6aR9_rJQQiw81_CgAPh9XRRs$
    >>>>>>>>>>
    >>>>>>>>>> And 'Saravana Kannan <skannan@codeaurora.org <mailto:skannan@codeaurora.org>>' is wrong email address.
    >>>>>>>>>> Please update the email or drop this email.
    >>>>>>>>>
    >>>>>>>>> Hi Chanwoo,
    >>>>>>>>>
    >>>>>>>>> Thank you for the advices.
    >>>>>>>>> I will resend patch v9 (add to linux-pm ML), remove this patch, and note
    >>>>>>>>> that my patch set base on
    >>>>>>>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing-passive-gov__;!!CTRNKA9wMg0ARbw!yUlsuxrL5PcbF7o6A9DlCfvoA6w8V8VXKjYIybYyiJg3D0HM-lI2xRuxLUV6b3UJ8WFhg_g$
    >>>>>>>>
    >>>>>>>> I has not yet test this patch[1] on devfreq-testing-passive-gov branch.
    >>>>>>>> So that if possible, I'd like you to test your patches with this patch[1]
    >>>>>>>> and then if there is no problem, could you send the next patches with patch[1]?
    >>>>>>>>
    >>>>>>>> [1]https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing-passive-gov&id=39c80d11a8f42dd63ecea1e0df595a0ceb83b454__;!!CTRNKA9wMg0ARbw!yUlsuxrL5PcbF7o6A9DlCfvoA6w8V8VXKjYIybYyiJg3D0HM-lI2xRuxLUV6b3UJR2cQqZs$
    >>>>>>>
    >>>>>>>
    >>>>>>> Sorry for the confusion. I make the devfreq-testing-passive-gov[1]
    >>>>>>> branch based on latest devfreq-next branch.
    >>>>>>> [1] https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing-passive-gov__;!!CTRNKA9wMg0ARbw!yUlsuxrL5PcbF7o6A9DlCfvoA6w8V8VXKjYIybYyiJg3D0HM-lI2xRuxLUV6b3UJ8WFhg_g$
    >>>>>>>
    >>>>>>> First of all, if possible, I want to test them[1] with your patches in this series.
    >>>>>>> And then if there are no any problem, please let me know. After confirmed from you,
    >>>>>>> I'll send the patches of devfreq-testing-passive-gov[1] branch.
    >>>>>>> How about that?
    >>>>>>>
    >>>>>> Hi Chanwoo~
    >>>>>>
    >>>>>> We will use this on Google Chrome project.
    >>>>>> Google Hsin-Yi has test your patch + my patch set v8 [2~8]
    >>>>>>
    >>>>>>     make sure cci devfreqs runs with cpufreq.
    >>>>>>     suspend resume
    >>>>>>     speedometer2 benchmark
    >>>>>> It is okay.
    >>>>>>
    >>>>>> Please send the patches of devfreq-testing-passive-gov[1] branch.
    >>>>>>
    >>>>>> I will send patch v9 base on yours latter.
    >>>>>
    >>>>> Thanks for your test. I'll send the patches today.
    >>>>
    >>>> I'm sorry for delay because when I tested the patches
    >>>> for devfreq parent type on Odroid-xu3, there are some problem
    >>>> related to lazy linking of OPP. So I'm trying to analyze them.
    >>>> Unfortunately, we need to postpone these patches to next linux
    >>>> version.
    >>>>
    >>> Hi Chanwoo Choi~
    >>>
    >>> It is said that you are busy on another task recently.
    >>> May I know your plan on this patch?
    >>> Thank you.
    >>
    >> Sorry for late work. I have a question.
    >> When I tested exynos-bus.c with adding the 'required-opp' property
    >> on odroid-xu3 board. I got some fail about
    >>
    >> When calling _set_required_opps(), always _set_required_opp() returns
    >> -EBUSY error because of following lazy linking case[1].
    >>
    >> [1] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/opp/core.c*L896__;Iw!!CTRNKA9wMg0ARbw!3eNxwDZRy-Ev5BHGxT-BxCz4qrNy0NZohQuBGW36krkwOkl_WX8yBmxlqSk9hxp_kxspMJI$
    >>
    >> /* required-opps not fully initialized yet */
    >> if (lazy_linking_pending(opp_table))
    >>      return -EBUSY; 
    >>
    >>
    >> For calling dev_pm_opp_of_add_table(), lazy_link_required_opp_table() function
    >> will be called. But, there is constraint[2]. If is_genpd of opp_table is false,
    >> driver/opp/of.c cannot resolve the lazy linking issue.
    >>
    >> [2]  https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.13-rc3/source/drivers/opp/of.c*L386__;Iw!!CTRNKA9wMg0ARbw!3eNxwDZRy-Ev5BHGxT-BxCz4qrNy0NZohQuBGW36krkwOkl_WX8yBmxlqSk9hxp_QFUVY9E$
    >>
    >> /* Link required OPPs for all OPPs of the newly added OPP table */
    >> static void lazy_link_required_opp_table(struct opp_table *new_table)
    >> {
    >>      struct opp_table *opp_table, *temp, **required_opp_tables;
    >>      struct device_node *required_np, *opp_np, *required_table_np;
    >>      struct dev_pm_opp *opp;
    >>      int i, ret;
    >>
    >>      /*
    >>       * We only support genpd's OPPs in the "required-opps" for now,
    >>       * as we don't know much about other cases.
    >>       */
    >>      if (!new_table->is_genpd)
    >>              return;
    >>
    >> Even if this case, there are no problem on your test case?
    >>
    >
    > Hi Chanwoo~
    > Sorry for late reply.
    > Yes, we meet similar issue.
    > Google member Hsin-Yi had helped deal with this issue on Chrome project.
    >
    > Patch segment:
    > @ /drivers/opp/of.c
    >
    > /* Link required OPPs for all OPPs of the newly added OPP table */
    > static void lazy_link_required_opp_table(struct opp_table *new_table)
    > {
    >       struct opp_table *opp_table, *temp, **required_opp_tables;
    >       struct device_node *required_np, *opp_np, *required_table_np;
    >       struct dev_pm_opp *opp;
    >       int i, ret;
    >
    > +     /*
    > +      * We only support genpd's OPPs in the "required-opps" for now,
    > +      * as we don't know much about other cases.
    > +      */
    > +     if (!new_table->is_genpd)
    > +             return;
    >
    >
    > Hsin-Yi replied this issue in the discussion list in the original lazy
    > link thread:
    > https://patchwork.kernel.org/project/linux-pm/patch/20190717222340.137578-4-saravanak@google.com/#23932203
    >
    > Loop Hsin-YI here.
    > You can discuss with her if needing more detail.
    >
    > Thank you both.
    >

    Thanks. First of all, we need to resolve and discuss this issue.


Hi Chanwoo, 

We think removing the genpd check is sufficient for our use case since we only use the lazy link for opp table translation.

Hi Hsin-Yi,

IMHO, I think 'is_genpd' checking should be removed for devices except for genpd
like as following:

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index c582a9ca397b..b54d3a985515 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c

@@ -201,17 +201,6 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
                        lazy = true;
                        continue;
                }
-
-               /*
-                * We only support genpd's OPPs in the "required-opps" for now,
-                * as we don't know how much about other cases. Error out if the
-                * required OPP doesn't belong to a genpd.
-                */
-               if (!required_opp_tables[i]->is_genpd) {
-                       dev_err(dev, "required-opp doesn't belong to genpd: %pOF\n",
-                               required_np);
-                       goto free_required_tables;
-               }
        }
 
        /* Let's do the linking later on */

@@ -379,13 +368,6 @@ static void lazy_link_required_opp_table(struct opp_table *new_table)
        struct dev_pm_opp *opp;
        int i, ret;
 
-       /*
-        * We only support genpd's OPPs in the "required-opps" for now,
-        * as we don't know much about other cases.
-        */
-       if (!new_table->is_genpd)
-               return;
-
        mutex_lock(&opp_table_lock);
 
        list_for_each_entry_safe(opp_table, temp, &lazy_opp_tables, lazy) {

@@ -874,7 +856,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
                return ERR_PTR(-ENOMEM);
 
        ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
-       if (ret < 0 && !opp_table->is_genpd) {
+       if (ret < 0) {
                dev_err(dev, "%s: opp key field not found\n", __func__);
                goto free_opp;
        }

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek