From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752525AbcGLIsZ (ORCPT ); Tue, 12 Jul 2016 04:48:25 -0400 Received: from mout.kundenserver.de ([212.227.17.10]:55826 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751611AbcGLIsV (ORCPT ); Tue, 12 Jul 2016 04:48:21 -0400 From: Arnd Bergmann To: Michael Turquette Cc: John Stultz , Olof Johansson , lkml , "arm@kernel.org" , Stephen Boyd , Rob Herring , Pawel Moll , Wei Xu , Guodong Xu , Zhangfei Gao Subject: Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Date: Tue, 12 Jul 2016 10:51:36 +0200 Message-ID: <5612350.vuApcMIIUI@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <146827441381.73491.4865692343236492728@resonance> References: <1467247725-3665-1-git-send-email-john.stultz@linaro.org> <8428871.BtryaqAxHS@wuerfel> <146827441381.73491.4865692343236492728@resonance> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:7FIdDL/lIfDn70lLn4HlFMqevoQalAy7Tgn3p9DNCX6XzAW0BJT cW0IQUq4JSN/clr3STTzlh4Dhcj8jBUXIFbpXIfNHPaf/aofhPGMht7aSIAhV3MKL6f1sbh AEh1Te9ceH0Bp5v4VqA9rZFzvEhmAt7D9HMuig+Wzl2FWo8Hg4GVKCo0uPVPp57NXqQPbNw cssyh92e4eKGxNBdHyYCQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:vywBROIqXE8=:VZAFIKuTkP7YjavJF8J+9J +COOkLVoASwE009gLftPzKIxNGaqdoDUWv15a1i/ioGvGR+QZNYvV/IH9YupmKZaXE/ONaxtU muYZxiPUkLl+P3SU15v3bWs+SIkZUyOhU7RtDFpFG3u7XigbdBWBpxxeQmjbGwsVW6Pl6Bmcm LaZHuBIkJYPs9kJ+zfYRREVESMZbYDFl6AcYc9rVrBd2JLwH92hOO0lB0nNI5PNHIEpuWYzwN D7LjHL2GZSRJCIR39519zlLW/jCu+/jltSU+QOWkK1p3SUKR0ULm/GS2xwNsCEp2CPAMAm5Dq 46X+oeaTLAw7D+lEnjzQmn8vw0UmYH0htd79fTLRnAxYJOXLfXn0tFfm8RlcwspG957n1uhPY AEoh2DLiTv9i9Z1Um7lZnF0foo7zV/ETCo2QjtRegnU8vK6jXKPOa3adAfQYjHS5LNkGBMXM1 I5Eej8vaOrHMIXxBcaRB+5GMQsU6EZ+UXvQpSztNrCmM3aO5jzuoZxyTraEuY5RUeP+euZQNt f/OMlXIDZH7Cofu0zdUkKTzDocBfkUDh86rMZr2L5iahOzdRVlnJI2dKs3v28htogyqn/CYXb 3aeT/sLQbbfCv19Zee9S6bQv0CIHSj/XMZr3RqMMl8LFgT4XUM6Br+TIztpclw4ShhG2AyTdA XSN8asVDIaLp1yZxSFAeLbooAP2Lujj76i6uAVdqYVb69YutMK83/oSHiFdRy8/aKkB9HO3uA +VGAp9uONm+X+r3Q Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, July 11, 2016 3:00:13 PM CEST Michael Turquette wrote: > Quoting Arnd Bergmann (2016-07-11 13:21:17) > > On Thursday, July 7, 2016 7:10:30 PM CEST Michael Turquette wrote: > > > Quoting Arnd Bergmann (2016-07-07 01:13:58) > > > > On Wednesday, July 6, 2016 5:19:53 PM CEST Michael Turquette wrote: > > > > > On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann wrote: > > > > > > On Wednesday, July 6, 2016 12:20:15 AM CEST John Stultz wrote: > > > > > > > On Wed, Jul 6, 2016 at 12:04 AM, Olof Johansson wrote: > > > > > > > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz wrote: > > > > > > > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson wrote: > > > > > > > >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: > > > > > > The question this raises is why that clock was missed the first time > > > > > > around. I'd suggest whoever owns the clock driver can go through the > > > > > > documentation again and look for others that may have been missed, > > > > > > then send a patch to the driver to add *all* the missing ones for the > > > > > > merge window, and one release later we add the driver depending on > > > > > > previously unknown clocks. > > > > > > > > > > Well, I'm kicking the ant pile on this one, but sometimes the above > > > > > suggestion is not possible. I'm currently hacking on a platform with > > > > > very limited docs, so I cannot understand the whole clock tree, nor > > > > > how all peripherals are wired up to it. > > > > > > > > That's clearly not the case here though: the hi6220 clk driver > > > > was contributed by hisilicon engineers that have all the documentation. > > > > > > > > > Further complicating matters is that fact that any headers in the DT > > > > > include chroot constitute an unbreakable ABI that shall stand for > > > > > 1,000 years at least, so I'm very remiss to dump a bunch of constant > > > > > values in there with names that might need to change at a later date. > > > > > > > > Can you give an example why they might need to change? > > > > > > Yes, the AmLogic GXBB clk driver that I just merged is a great example. > > > The names of the clock signals enumerated in the header will surely > > > change as our understanding of the hardware changes. These names are > > > part of the ABI and yet we cannot possibly get them right on the first > > > pass. > > > > I see. Actually the names are not part of the ABI, just the numeric > > values are. Changing the names is of course still a pain, but it's > > not nearly as bad as adding further constants, which requires coordinating > > between the dts files and the driver. > > Hmm, that's interesting. How do you feel about updates to Linux kernel > clk driver + some consumer driver + dts, after we find out that some > name is horribly wrong and needs to change? That would be a bugfix and not part of the ABI, so I don't think that's a problem. > What about the case where the number needs to change? E.g. we find out > later on that there is a post divider downstream that we did not know > about, and we want the clk consumer to reference that post-divider > instead of the parent of the post-divider? I would never change the number that a specific name refers to. If you have to rename a clk, I'd do it like this - #define OLD_CLK_NAME 41 + #define OLD_CLK_NAME 41 /* incorrect, don't use */ + #define NEW_CLK_NAME 42 and in the example here, the DT entry for the device should be changed to point ot the other clk. > Would we be forced to play a tricky game of keeping the number the same, > but just adding a new number for the post-divider and updating the > driver & dts? It depends on how the clk driver is structured of course. In the ideal case, the clk driver already knows about all the dividers and doesn't need the macros in the first place, so the update is just in the dts, but this is probably not the case you are talking about in. Generally speaking, I'd say the clk driver should have little or no knowledge of how the clks are being used by drivers, that would be a layering violation. A similar problem is the patch below that was just added: what in the world does the clk driver care about the settings that the bootloader sets? If something comes from the bootloader, the driver should get it from the DT rather than hardcode it. Arnd commit c6e80ace83a90a410d09de0727ff9b151de6291a Author: Xinliang Liu Date: Wed Jun 29 16:45:54 2016 +0800 clk: hi6220: Change syspll and media_syspll clk to 1.19GHz In the bootloader of HiKey/96boards, syspll and media_syspll clk was initialized to 1.19GHz. So, here changes it in kernel accordingly. 1.19GHz was chosen over 1.2GHz because at 1.19GHz we get more precise HDMI pixel clock (1.19G/16 = 74.4MHz) for 1280x720p@60Hz HDMI (74.25MHz required by standards). Closer pixel clock means better compatibility to HDMI monitors. Signed-off-by: Guodong Xu Signed-off-by: Xinliang Liu Signed-off-by: Michael Turquette Link: lkml.kernel.org/r/1467189955-21694-1-git-send-email-guodong.xu@linaro.org diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c index 76de9a762a86..fe364e63f8de 100644 --- a/drivers/clk/hisilicon/clk-hi6220.c +++ b/drivers/clk/hisilicon/clk-hi6220.c @@ -34,8 +34,8 @@ static struct hisi_fixed_rate_clock hi6220_fixed_rate_clks[] __initdata = { { HI6220_PLL_BBP, "bbppll0", NULL, 0, 245760000, }, { HI6220_PLL_GPU, "gpupll", NULL, 0, 1000000000,}, { HI6220_PLL1_DDR, "ddrpll1", NULL, 0, 1066000000,}, - { HI6220_PLL_SYS, "syspll", NULL, 0, 1200000000,}, - { HI6220_PLL_SYS_MEDIA, "media_syspll", NULL, 0, 1200000000,}, + { HI6220_PLL_SYS, "syspll", NULL, 0, 1190400000,}, + { HI6220_PLL_SYS_MEDIA, "media_syspll", NULL, 0, 1190400000,}, { HI6220_DDR_SRC, "ddr_sel_src", NULL, 0, 1200000000,}, { HI6220_PLL_MEDIA, "media_pll", NULL, 0, 1440000000,}, { HI6220_PLL_DDR, "ddrpll0", NULL, 0, 1600000000,},