Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932676AbbGHFo5 (ORCPT ); Wed, 8 Jul 2015 01:44:57 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:60942 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932616AbbGHFoh (ORCPT ); Wed, 8 Jul 2015 01:44:37 -0400 Date: Wed, 8 Jul 2015 07:44:31 +0200 From: Sascha Hauer To: Eddie Huang Cc: Daniel Kurtz , Matthias Brugger , "open list:OPEN FIRMWARE AND..." , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , linux-mediatek@lists.infradead.org, James Liao , mturquette@gmail.com, heiko@sntech.de, sboyd@codeaurora.org Subject: Re: [PATCH] arm64: dts: mt8173: add clock_null Message-ID: <20150708054430.GY18700@pengutronix.de> References: <1434605351-64592-1-git-send-email-eddie.huang@mediatek.com> <20150707130732.GD18561@pengutronix.de> <20150707143621.GW18700@pengutronix.de> <1436323041.27396.23.camel@mtksdaap41> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436323041.27396.23.camel@mtksdaap41> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 07:39:36 up 13 days, 1 min, 42 users, load average: 0.00, 0.05, 0.11 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5482 Lines: 118 On Wed, Jul 08, 2015 at 10:37:21AM +0800, Eddie Huang wrote: > On Tue, 2015-07-07 at 23:10 +0800, Daniel Kurtz wrote: > > On Tue, Jul 7, 2015 at 10:36 PM, Sascha Hauer wrote: > > > On Tue, Jul 07, 2015 at 10:15:29PM +0800, Daniel Kurtz wrote: > > >> On Tue, Jul 7, 2015 at 9:07 PM, Sascha Hauer wrote: > > >> > On Thu, Jun 18, 2015 at 01:29:11PM +0800, Eddie Huang wrote: > > >> >> Add clk_null, which represents clocks that can not / need not > > >> >> controlled by software. > > >> >> There are many clocks' parent set to clk_null. > > >> >> > > >> >> Signed-off-by: James Liao > > >> >> Signed-off-by: Eddie Huang > > >> >> --- > > >> >> Base on 4.1-rc1 > > >> >> > > >> >> Change-Id: I4db9b40d07e28f54f7bae9b676316cbd6a962124 > > >> >> --- > > >> >> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 6 ++++++ > > >> >> 1 file changed, 6 insertions(+) > > >> >> > > >> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > >> >> index 924fdb6..4798f44 100644 > > >> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > >> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi > > >> >> @@ -81,6 +81,12 @@ > > >> >> cpu_on = <0x84000003>; > > >> >> }; > > >> >> > > >> >> + clk_null: clk_null { > > >> >> + compatible = "fixed-clock"; > > >> >> + clock-frequency = <0>; > > >> >> + #clock-cells = <0>; > > >> >> + }; > > >> > > > >> > The discussion around this patch shows that we don't want to have this > > >> > clock in the device tree as it is not a hardware description. > > >> > > > >> > Ok, fine. Eddie, you told us that the rate of the current clk_null children > > >> > is not interesting. What's the motivation to send this patch anyway > > >> > then? Why can't you keep its children on the orphan list where they are > > >> > already now? > > >> > > > >> > Another possibility would be to instantiate the clk_null clock from C > > >> > code rather than from the device tree. This way we wouldn't put any > > >> > wrong descriptions into the device tree and still can implement the > > >> > support for the real parent clocks when we actually need them. > > >> > > >> Some device nodes, like mmc, use a clk_null phandle as one of their clocks: > > >> > > >> mmc1: mmc@11240000 { > > >> compatible = "mediatek,mt8173-mmc", > > >> "mediatek,mt8135-mmc"; > > >> reg = <0 0x11240000 0 0x1000>; > > >> interrupts = ; > > >> clocks = <&pericfg CLK_PERI_MSDC30_1>, > > >> <&clk_null>; > > >> clock-names = "source", "hclk"; > > >> status = "disabled"; > > >> }; > > > > > > This is another case than the one we discussed about. In the case above > > > I motivated using a dummy clock since the clock exists in the system, > > > but is not software controllable. To abstract this from the driver > > > (which needs this clock since it exists) we here have the dummy clock. > > > However, of course I can't prove the clock is indeed not software > > > controllable; that's only the information I have. > > > > I was trying to answer your question "What's the motivation to send > > this patch anyway?". > > The motivation is to send follow on patches that use the clk_null > > phandle. We need to provide some clock as the mmc1's hclk. I do not > > understand why this has to be "clk_null", though. It seems like this > > should be a real clock coming from one of the real clock_controller > > nodes. After all, the mmc driver is going to be enabling/disabling > > this clock for power savings at runtime. What does that even mean for > > clk_null ? > > The original purpose of this patch is to provide a common dummy clock > for both software don't care clock and clock that is not software > controllable.I got comments that device tree should describe hardware > and should put exact clock in device tree. I think this is true. So we > will remove this clock_null patch, and: > > 1. For Mediatek SoC CCF driver, James will clarify clock usage further. > Actually, we still think it's not necessary to describe whole tree that > software don't care, James will deal this in clock driver. I think that aswell since the device tree is not affected in this case. Should we realize later that we indeed need the missing clocks we can still implement them without modifying the device tree. > > 2. For other module that use SW not controllable clock (mmc case > mentioned by Dan), because this is a real clock, we will put a dummy > clock in device tree, like > > clk_mmchclk: dummyhclk { > compatible = "fixed-clock"; > clock-frequency = <0>; > #clock-cells = <0>; > }; > > How about this modification ? I wouldn't name it 'dummy', this will again raise some eyebrows. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/