Message-ID: <20170605201314.GI20170@codeaurora.org>
Date:   Mon, 5 Jun 2017 13:13:14 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Phil Elwell <phil@...pberrypi.org>
Cc:     Stefan Wahren <stefan.wahren@...e.com>,
        Stephen Warren <swarren@...dotorg.org>,
        Michael Turquette <mturquette@...libre.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-rpi-kernel <linux-rpi-kernel@...ts.infradead.org>,
        linux-clk@...r.kernel.org
Subject: Re: CLK_OF_DECLARE advice required

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.

> 
> 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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists