Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751413AbdFFItv (ORCPT ); Tue, 6 Jun 2017 04:49:51 -0400 Received: from mx08-00252a01.pphosted.com ([91.207.212.211]:42444 "EHLO mx08-00252a01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751274AbdFFItt (ORCPT ); Tue, 6 Jun 2017 04:49:49 -0400 From: Phil Elwell Subject: Re: CLK_OF_DECLARE advice required To: Stephen Boyd Cc: Stefan Wahren , Stephen Warren , Michael Turquette , "linux-kernel@vger.kernel.org" , linux-rpi-kernel , linux-clk@vger.kernel.org References: <8b65e551-e6dd-cf5c-1b22-e1f1a5996d73@wwwdotorg.org> <0794f430-9761-c855-9a89-13d9871c5831@i2se.com> <6765be64-9cf6-4663-4182-5b63f27bfb93@raspberrypi.org> <20170601063937.GN20170@codeaurora.org> <215eea3c-febe-5aa4-9dbb-e1f170bc0b0d@raspberrypi.org> <20170602223432.GU20170@codeaurora.org> <35742698-9f04-fa57-60b0-eecb3d790e03@raspberrypi.org> <20170605201314.GI20170@codeaurora.org> Message-ID: <4847126c-d2a9-d7f8-a074-92e4e2435fce@raspberrypi.org> Date: Tue, 6 Jun 2017 09:49:44 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170605201314.GI20170@codeaurora.org> Content-Type: text/plain; charset=windows-1252 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-06_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_spam_notspam policy=outbound_spam score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1706060162 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3677 Lines: 85 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