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