Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
From: Jerome Brunet <jbrunet@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>
Cc: Kevin Hilman <khilman@baylibre.com>,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-amlogic@lists.infradead.org
Subject: Re: [PATCH] clk: walk orphan list on clock provider registration
Date: Mon, 02 Dec 2019 19:59:22 +0100
Message-ID: <1j7e3eft9x.fsf@starbuckisacylon.baylibre.com> (raw)
In-Reply-To: <20191202181923.4D26D20717@mail.kernel.org>


On Mon 02 Dec 2019 at 19:19, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2019-11-29 08:16:58)
>> So far, we walked the orphan list every time a new clock was registered
>> in CCF. This was fine since the clocks were only referenced by name.
>> 
>> Now that the clock can be referenced through DT, it is not enough:
>> * Controller A register first a reference clocks from controller B
>>   through DT.
>> * Controller B register all its clocks then register the provider.
>> 
>> Each time controller B registers a new clock, the orphan list is walked
>> but it can't match since the provider is registered yet. When the
>> provider is finally registered, the orphan list is not walked unless
>> another clock is registered afterward.
>> 
>> This can lead to situation where some clocks remain orphaned even if
>> the parent is available.
>> 
>> Walking the orphan list on provider registration solves the problem.
>> 
>> Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>
> Sounds right. Thanks for making the fix!
>
> I suspect there should be a reported-by tag though?

laboriously, yes

>
>>  drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 22 deletions(-)
>> 
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index ef4416721777..917ba37c3b9d 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3249,6 +3249,34 @@ static inline void clk_debug_unregister(struct clk_core *core)
>>  }
>>  #endif
>>  
>> +static void __clk_core_reparent_orphan(void)
>
> Maybe drop the double underscore. clk_core prefix already means "private
> to this file".

I've done it this way just to keep coherent with __clk_core_init(),
which the code was extracted from

In the end, I don't mind so I'll drop it in the v2

>
>> +{
>> +       struct clk_core *orphan;
>> +       struct hlist_node *tmp2;
>> +
>> +       /*
>> +        * walk the list of orphan clocks and reparent any that newly finds a
>> +        * parent.
>> +        */
>> +       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>> +               struct clk_core *parent = __clk_init_parent(orphan);
>> +
>> +               /*
>> +                * We need to use __clk_set_parent_before() and _after() to
>> +                * to properly migrate any prepare/enable count of the orphan
>> +                * clock. This is important for CLK_IS_CRITICAL clocks, which
>> +                * are enabled during init but might not have a parent yet.
>> +                */
>> +               if (parent) {
>> +                       /* update the clk tree topology */
>> +                       __clk_set_parent_before(orphan, parent);
>> +                       __clk_set_parent_after(orphan, parent, NULL);
>> +                       __clk_recalc_accuracies(orphan);
>> +                       __clk_recalc_rates(orphan, 0);
>> +               }
>> +       }
>> +}
>> +
>>  /**
>>   * __clk_core_init - initialize the data structures in a struct clk_core
>>   * @core:      clk_core being initialized
>> @@ -3259,8 +3287,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
>>  static int __clk_core_init(struct clk_core *core)
>>  {
>>         int ret;
>> -       struct clk_core *orphan;
>> -       struct hlist_node *tmp2;
>>         unsigned long rate;
>>  
>>         if (!core)
>> @@ -3416,27 +3442,8 @@ static int __clk_core_init(struct clk_core *core)
>>                 clk_enable_unlock(flags);
>>         }
>>  
>> -       /*
>> -        * walk the list of orphan clocks and reparent any that newly finds a
>> -        * parent.
>> -        */
>> -       hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>> -               struct clk_core *parent = __clk_init_parent(orphan);
>> +       __clk_core_reparent_orphan();
>>  
>> -               /*
>> -                * We need to use __clk_set_parent_before() and _after() to
>> -                * to properly migrate any prepare/enable count of the orphan
>> -                * clock. This is important for CLK_IS_CRITICAL clocks, which
>> -                * are enabled during init but might not have a parent yet.
>> -                */
>> -               if (parent) {
>> -                       /* update the clk tree topology */
>> -                       __clk_set_parent_before(orphan, parent);
>> -                       __clk_set_parent_after(orphan, parent, NULL);
>> -                       __clk_recalc_accuracies(orphan);
>> -                       __clk_recalc_rates(orphan, 0);
>> -               }
>> -       }
>>  
>>         kref_init(&core->ref);
>>  out:
>> @@ -4288,6 +4295,10 @@ int of_clk_add_provider(struct device_node *np,
>>         mutex_unlock(&of_clk_mutex);
>>         pr_debug("Added clock from %pOF\n", np);
>>  
>> +       clk_prepare_lock();
>> +       __clk_core_reparent_orphan();
>> +       clk_prepare_unlock();
>> +
>
> Maybe make a locked version of this function and an unlocked version?
>
>>         ret = of_clk_set_defaults(np, true);
>>         if (ret < 0)
>>                 of_clk_del_provider(np);
>> @@ -4323,6 +4334,10 @@ int of_clk_add_hw_provider(struct device_node *np,
>>         mutex_unlock(&of_clk_mutex);
>>         pr_debug("Added clk_hw provider from %pOF\n", np);
>>  
>> +       clk_prepare_lock();
>> +       __clk_core_reparent_orphan();
>> +       clk_prepare_unlock();
>> +
>
> So we don't duplicate this twice.
>

Sure.

>>         ret = of_clk_set_defaults(np, true);
>>         if (ret < 0)


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

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-29 16:16 Jerome Brunet
2019-12-02 18:19 ` Stephen Boyd
2019-12-02 18:59   ` Jerome Brunet [this message]

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=1j7e3eft9x.fsf@starbuckisacylon.baylibre.com \
    --to=jbrunet@baylibre.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=sboyd@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-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/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-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org
	public-inbox-index linux-amlogic

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


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