Re: CLK_OF_DECLARE advice required

[Phil Elwell]

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