From: Phil Elwell <phil@raspberrypi.org>
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Stefan Wahren <stefan.wahren@i2se.com>,
Stephen Warren <swarren@wwwdotorg.org>,
Michael Turquette <mturquette@baylibre.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-rpi-kernel <linux-rpi-kernel@lists.infradead.org>,
linux-clk@vger.kernel.org
Subject: Re: CLK_OF_DECLARE advice required
Date: Tue, 6 Jun 2017 09:49:44 +0100 [thread overview]
Message-ID: <4847126c-d2a9-d7f8-a074-92e4e2435fce@raspberrypi.org> (raw)
In-Reply-To: <20170605201314.GI20170@codeaurora.org>
On 05/06/2017 21:13, Stephen Boyd wrote:
> On 06/05, Phil Elwell wrote:
>> That sounds great, but it doesn't match my experience. Let me restate my
>> observations with a bit more detail.
>>
>> In this scenario there three devices in a dependency chain:
>>
>> clock -> fixed-factor->clock -> uart.
>>
>> The Fixed Factor Clock is declared with OF_CLK_DECLARE, while the two platform
>> drivers use normal probe functions.
>>
>> 1) of_clk_init() calls encounter FFC in the list of clocks to initialise and
>> calls parent_ready on the device node.
>>
>> 2) The parent clock has not been initialised, so of_clk_get returns
>> -EPROBE_DEFER.
>>
>> 3) Steps 1 and 2 repeat until no progress is made, at which point the force
>> flag is set for one last iteration. This time the parent_ready check is skipped
>> and the code calls indirectly into _of_fixed_factor_clk_setup().
>>
>> 4) The FFC setup calls of_clk_get_parent_name, which returns a NULL that ends
>> up referred to by the parent_names field of clk_init_data structure indirectly
>> passed to clk_hw_register and clk_register.
>
> That's bad. Does "clock" in this scenario have a
> clock-output-names property so we can find the name of the parent
> of the fixed factor clock? That way we can describe the fixed
> factor to "clock" linkage. Without that, things won't ever work.
No - the clock provider is bcm2835-aux-clk, as declared by bcm283x.dts:
https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/master/arch/arm/boot/dts/bcm283x.dtsi#462
If I patch it to include a "clock-output-names" property with "aux-uart" as the
first entry then the FFC registers correctly, even though the source clock hasn't
been initialised yet.
>> 5) In clk_register, the parent name is copied with kstrdup, which returns NULL
>> for a NULL input. clk_register sees this as an allocation failure and returns
>> -ENOMEM.
>>
>> 6) _of_fixed_factor_clock_setup returns -ENOMEM, bypassing of_clk_add_provider,
>> but the wrapper function registered with CLK_OF_DECLARE has a void return, so
>> the failure is lost.
>
> Yep. We've already failed earlier.
>
>>
>> 7) Back in of_clk_init, which is ignorant of the failure, the OF_POPULATED flag
>> of the FFC node has already been set, preventing the node from subsequently
>> being probed in the usual way.
>>
>> 8) When the downstream uart is probed, devm_clk_get returns -EPROBE_DEFER every
>> time, resulting in a non-functioning UART.
>>
>> Is this behaviour as intended? I can see that the NULL parent name in steps 4
>> and 5 could be handled more gracefully, but the end result would be the same.
>>
>> Where and how is the "orphan" clock concept supposed to help, and what needs to
>> be fixed in this case?
>>
>
> The orphan concept helps here because of_clk_init() eventually
> forces the registration of the fixed factor clock even though the
> fixed factor's parent has not been registered yet. As you've
> determined though, that isn't working properly because the fixed
> factor code is failing to get a name for the parent. Using the
> clock-output-names property would fix that though.
> Also, it may be appropriate to move the fixed factor clock
> registration into the UART driver. It would depend on where the
> fixed factor is situated inside the SoC, but it could be argued
> that if the factor is near or embedded inside the UART hardware
> then the UART driver should register the fixed factor clock as
> well as the UART clock.
I take your point, but I'm trying to use a standard UART and a standard
fixed-factor clock to get non-standard results in what has become a learning
exercise.
Thanks again - this has been very useful.
Phil
prev parent reply other threads:[~2017-06-06 8:49 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-30 12:23 CLK_OF_DECLARE advice required Phil Elwell
2017-05-31 15:27 ` Stephen Warren
2017-05-31 15:58 ` Stefan Wahren
2017-05-31 16:28 ` Phil Elwell
2017-05-31 16:47 ` Stephen Warren
2017-06-01 6:39 ` Stephen Boyd
2017-06-01 8:26 ` Phil Elwell
2017-06-02 22:34 ` Stephen Boyd
2017-06-05 16:24 ` Phil Elwell
2017-06-05 20:13 ` Stephen Boyd
2017-06-06 7:22 ` Geert Uytterhoeven
2017-06-06 20:49 ` Stephen Boyd
2017-06-06 8:49 ` Phil Elwell [this message]
Reply instructions:
You may reply publicly 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=4847126c-d2a9-d7f8-a074-92e4e2435fce@raspberrypi.org \
--to=phil@raspberrypi.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=mturquette@baylibre.com \
--cc=sboyd@codeaurora.org \
--cc=stefan.wahren@i2se.com \
--cc=swarren@wwwdotorg.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).