* [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 @ 2016-06-30 0:48 John Stultz 2016-06-30 0:48 ` [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 John Stultz ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: John Stultz @ 2016-06-30 0:48 UTC (permalink / raw) To: lkml, arm Cc: John Stultz, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao This patchset enables the pl031 RTC on the Hi6220 SoC. I'd like to submit it to be merged. Wei has acked the second patch (modulo a whitespace fix which I've included in this v3), so it seems like both could go through the clk tree. But Wei also seemed open to pulling in a clk tree branch as it goes through arm-soc. Michael/Stephen: If there's no other objections, could you queue the first patch and make it avilable via the branch for Wei, or just take both patches? thanks -john Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Wei Xu <xuwei5@hisilicon.com> Cc: Guodong Xu <guodong.xu@linaro.org> Cc: Zhangfei Gao <zhangfei.gao@linaro.org> Zhangfei Gao (2): clk: hi6220: Add RTC clock for pl031 arm64: dts: hi6220: Add pl031 RTC support arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 16 ++++++++++++++++ drivers/clk/hisilicon/clk-hi6220.c | 2 ++ include/dt-bindings/clock/hi6220-clock.h | 5 +++-- 3 files changed, 21 insertions(+), 2 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 2016-06-30 0:48 [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz @ 2016-06-30 0:48 ` John Stultz 2016-06-30 19:15 ` Stephen Boyd 2016-06-30 0:48 ` [PATCH 2/2 v3] arm64: dts: hi6220: Add pl031 RTC support John Stultz ` (2 subsequent siblings) 3 siblings, 1 reply; 23+ messages in thread From: John Stultz @ 2016-06-30 0:48 UTC (permalink / raw) To: lkml, arm Cc: Zhangfei Gao, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, John Stultz From: Zhangfei Gao <zhangfei.gao@linaro.org> Adds clk support for the pl031 RTC on hi6220 Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Wei Xu <xuwei5@hisilicon.com> Cc: Guodong Xu <guodong.xu@linaro.org> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> [jstultz: Forward ported, tweaked commit description] Signed-off-by: John Stultz <john.stultz@linaro.org> --- v3: No changes from v1, version noted just to be consistent with the other patch in this patchset. drivers/clk/hisilicon/clk-hi6220.c | 2 ++ include/dt-bindings/clock/hi6220-clock.h | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/clk/hisilicon/clk-hi6220.c b/drivers/clk/hisilicon/clk-hi6220.c index f02cb41..76de9a7 100644 --- a/drivers/clk/hisilicon/clk-hi6220.c +++ b/drivers/clk/hisilicon/clk-hi6220.c @@ -68,6 +68,8 @@ static struct hisi_gate_clock hi6220_separated_gate_clks_ao[] __initdata = { { HI6220_TIMER7_PCLK, "timer7_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 22, 0, }, { HI6220_TIMER8_PCLK, "timer8_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 23, 0, }, { HI6220_UART0_PCLK, "uart0_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 24, 0, }, + { HI6220_RTC0_PCLK, "rtc0_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 25, 0, }, + { HI6220_RTC1_PCLK, "rtc1_pclk", "clk_tcxo", CLK_SET_RATE_PARENT|CLK_IGNORE_UNUSED, 0x630, 26, 0, }, }; static void __init hi6220_clk_ao_init(struct device_node *np) diff --git a/include/dt-bindings/clock/hi6220-clock.h b/include/dt-bindings/clock/hi6220-clock.h index 70ee383..6b03c84 100644 --- a/include/dt-bindings/clock/hi6220-clock.h +++ b/include/dt-bindings/clock/hi6220-clock.h @@ -55,8 +55,9 @@ #define HI6220_TIMER7_PCLK 34 #define HI6220_TIMER8_PCLK 35 #define HI6220_UART0_PCLK 36 - -#define HI6220_AO_NR_CLKS 37 +#define HI6220_RTC0_PCLK 37 +#define HI6220_RTC1_PCLK 38 +#define HI6220_AO_NR_CLKS 39 /* clk in Hi6220 systrl */ /* gate clock */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 2016-06-30 0:48 ` [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 John Stultz @ 2016-06-30 19:15 ` Stephen Boyd 2016-06-30 19:23 ` John Stultz 0 siblings, 1 reply; 23+ messages in thread From: Stephen Boyd @ 2016-06-30 19:15 UTC (permalink / raw) To: John Stultz Cc: lkml, arm, Zhangfei Gao, Michael Turquette, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu On 06/29, John Stultz wrote: > From: Zhangfei Gao <zhangfei.gao@linaro.org> > > Adds clk support for the pl031 RTC on hi6220 > > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Wei Xu <xuwei5@hisilicon.com> > Cc: Guodong Xu <guodong.xu@linaro.org> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > [jstultz: Forward ported, tweaked commit description] > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- Applied to clk-hi6220-rtc and merged into clk-next. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 2016-06-30 19:15 ` Stephen Boyd @ 2016-06-30 19:23 ` John Stultz 0 siblings, 0 replies; 23+ messages in thread From: John Stultz @ 2016-06-30 19:23 UTC (permalink / raw) To: Stephen Boyd Cc: lkml, arm, Zhangfei Gao, Michael Turquette, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu On Thu, Jun 30, 2016 at 12:15 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 06/29, John Stultz wrote: >> From: Zhangfei Gao <zhangfei.gao@linaro.org> >> >> Adds clk support for the pl031 RTC on hi6220 >> >> Cc: Michael Turquette <mturquette@baylibre.com> >> Cc: Stephen Boyd <sboyd@codeaurora.org> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Pawel Moll <pawel.moll@arm.com> >> Cc: Wei Xu <xuwei5@hisilicon.com> >> Cc: Guodong Xu <guodong.xu@linaro.org> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> [jstultz: Forward ported, tweaked commit description] >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> --- > > Applied to clk-hi6220-rtc and merged into clk-next. Great! Thanks! Wei: Could you merge git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-hi6220-rtc into your tree, then apply patch 2/2? thanks -john ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/2 v3] arm64: dts: hi6220: Add pl031 RTC support 2016-06-30 0:48 [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz 2016-06-30 0:48 ` [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 John Stultz @ 2016-06-30 0:48 ` John Stultz 2016-06-30 15:12 ` [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Wei Xu 2016-07-06 5:22 ` Olof Johansson 3 siblings, 0 replies; 23+ messages in thread From: John Stultz @ 2016-06-30 0:48 UTC (permalink / raw) To: lkml, arm Cc: Zhangfei Gao, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, John Stultz From: Zhangfei Gao <zhangfei.gao@linaro.org> Add pl031 rtc0 and rtc1 support to hi6220 dtsi Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Wei Xu <xuwei5@hisilicon.com> Cc: Guodong Xu <guodong.xu@linaro.org> Acked-by: Wei Xu <xuwei5@hisilicon.com> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> [jstultz: Forward ported and tweaked commit description, added rtc1 entry as suggested by Guodong] Signed-off-by: John Stultz <john.stultz@linaro.org> --- v2: Add rtc1 entry as suggested by Guodong v3: Whitespace fixup requested by Wei arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi index 189d215..758fd22 100644 --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi @@ -336,6 +336,22 @@ clock-names = "timer1", "timer2", "apb_pclk"; }; + rtc0: rtc@f8003000 { + compatible = "arm,pl031", "arm,primecell"; + reg = <0x0 0xf8003000 0x0 0x1000>; + interrupts = <0 12 4>; + clocks = <&ao_ctrl HI6220_RTC0_PCLK>; + clock-names = "apb_pclk"; + }; + + rtc1: rtc@f8004000 { + compatible = "arm,pl031", "arm,primecell"; + reg = <0x0 0xf8004000 0x0 0x1000>; + interrupts = <0 8 4>; + clocks = <&ao_ctrl HI6220_RTC1_PCLK>; + clock-names = "apb_pclk"; + }; + pmx0: pinmux@f7010000 { compatible = "pinctrl-single"; reg = <0x0 0xf7010000 0x0 0x27c>; -- 1.9.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-06-30 0:48 [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz 2016-06-30 0:48 ` [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 John Stultz 2016-06-30 0:48 ` [PATCH 2/2 v3] arm64: dts: hi6220: Add pl031 RTC support John Stultz @ 2016-06-30 15:12 ` Wei Xu 2016-07-06 5:22 ` Olof Johansson 3 siblings, 0 replies; 23+ messages in thread From: Wei Xu @ 2016-06-30 15:12 UTC (permalink / raw) To: John Stultz, lkml, arm Cc: Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Guodong Xu, Zhangfei Gao Hi John, On 30/06/2016 01:48, John Stultz wrote: > This patchset enables the pl031 RTC on the Hi6220 SoC. > > I'd like to submit it to be merged. > > Wei has acked the second patch (modulo a whitespace fix which > I've included in this v3), so it seems like both could go > through the clk tree. > > But Wei also seemed open to pulling in a clk tree branch > as it goes through arm-soc. Thanks for your patch :) Both are fine to me. Best Regards, Wei > > Michael/Stephen: If there's no other objections, could you > queue the first patch and make it avilable via the branch for > Wei, or just take both patches? > > thanks > -john > > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Wei Xu <xuwei5@hisilicon.com> > Cc: Guodong Xu <guodong.xu@linaro.org> > Cc: Zhangfei Gao <zhangfei.gao@linaro.org> > > > Zhangfei Gao (2): > clk: hi6220: Add RTC clock for pl031 > arm64: dts: hi6220: Add pl031 RTC support > > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 16 ++++++++++++++++ > drivers/clk/hisilicon/clk-hi6220.c | 2 ++ > include/dt-bindings/clock/hi6220-clock.h | 5 +++-- > 3 files changed, 21 insertions(+), 2 deletions(-) > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-06-30 0:48 [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz ` (2 preceding siblings ...) 2016-06-30 15:12 ` [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Wei Xu @ 2016-07-06 5:22 ` Olof Johansson 2016-07-06 6:55 ` John Stultz 3 siblings, 1 reply; 23+ messages in thread From: Olof Johansson @ 2016-07-06 5:22 UTC (permalink / raw) To: John Stultz Cc: lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: > This patchset enables the pl031 RTC on the Hi6220 SoC. > > I'd like to submit it to be merged. > > Wei has acked the second patch (modulo a whitespace fix which > I've included in this v3), so it seems like both could go > through the clk tree. > > But Wei also seemed open to pulling in a clk tree branch > as it goes through arm-soc. > > Michael/Stephen: If there's no other objections, could you > queue the first patch and make it avilable via the branch for > Wei, or just take both patches? I happen to dread these kind of patchsets these days. There's added dependencies across trees just because a defined name for the clock number is added to a header file. I much prefer to use numerical clocks for one release, and then once everything is in, switch over to the defines in the DTS. That way there are no dependencies, no need to setup a shared branch for a simple 3-line patch, etc. So, mind respinning the DTS piece? Thanks! -Olof ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 5:22 ` Olof Johansson @ 2016-07-06 6:55 ` John Stultz 2016-07-06 7:04 ` Olof Johansson 0 siblings, 1 reply; 23+ messages in thread From: John Stultz @ 2016-07-06 6:55 UTC (permalink / raw) To: Olof Johansson Cc: lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: > On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >> This patchset enables the pl031 RTC on the Hi6220 SoC. >> >> I'd like to submit it to be merged. >> >> Wei has acked the second patch (modulo a whitespace fix which >> I've included in this v3), so it seems like both could go >> through the clk tree. >> >> But Wei also seemed open to pulling in a clk tree branch >> as it goes through arm-soc. >> >> Michael/Stephen: If there's no other objections, could you >> queue the first patch and make it avilable via the branch for >> Wei, or just take both patches? > > I happen to dread these kind of patchsets these days. There's added > dependencies across trees just because a defined name for the clock > number is added to a header file. > > I much prefer to use numerical clocks for one release, and then once > everything is in, switch over to the defines in the DTS. > > That way there are no dependencies, no need to setup a shared branch > for a simple 3-line patch, etc. > > So, mind respinning the DTS piece? Huh.. But trying to boot w/ the numerical clock in the DTS, without the clk change results in lots of noise: [ 116.491458] of_clk_src_onecell_get: invalid clock index 37 [ 116.511627] of_clk_src_onecell_get: invalid clock index 38 Is that acceptable? thanks -john ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 6:55 ` John Stultz @ 2016-07-06 7:04 ` Olof Johansson 2016-07-06 7:20 ` John Stultz 0 siblings, 1 reply; 23+ messages in thread From: Olof Johansson @ 2016-07-06 7:04 UTC (permalink / raw) To: John Stultz Cc: lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: >> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >>> This patchset enables the pl031 RTC on the Hi6220 SoC. >>> >>> I'd like to submit it to be merged. >>> >>> Wei has acked the second patch (modulo a whitespace fix which >>> I've included in this v3), so it seems like both could go >>> through the clk tree. >>> >>> But Wei also seemed open to pulling in a clk tree branch >>> as it goes through arm-soc. >>> >>> Michael/Stephen: If there's no other objections, could you >>> queue the first patch and make it avilable via the branch for >>> Wei, or just take both patches? >> >> I happen to dread these kind of patchsets these days. There's added >> dependencies across trees just because a defined name for the clock >> number is added to a header file. >> >> I much prefer to use numerical clocks for one release, and then once >> everything is in, switch over to the defines in the DTS. >> >> That way there are no dependencies, no need to setup a shared branch >> for a simple 3-line patch, etc. >> >> So, mind respinning the DTS piece? > > Huh.. Sorry if it appeared random, I've complained about it for a while to submaintainers. :) > But trying to boot w/ the numerical clock in the DTS, without the clk > change results in lots of noise: > [ 116.491458] of_clk_src_onecell_get: invalid clock index 37 > [ 116.511627] of_clk_src_onecell_get: invalid clock index 38 > > Is that acceptable? Grmbl. Is it a lot of those? That's definitely not ideal either. If it's one or two during probe (since clk_gets should ideally fail at probe time) then I'd be less worried. -Olof ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 7:04 ` Olof Johansson @ 2016-07-06 7:20 ` John Stultz 2016-07-06 7:38 ` Arnd Bergmann 0 siblings, 1 reply; 23+ messages in thread From: John Stultz @ 2016-07-06 7:20 UTC (permalink / raw) To: Olof Johansson Cc: lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Wed, Jul 6, 2016 at 12:04 AM, Olof Johansson <olof@lixom.net> wrote: > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >>>> This patchset enables the pl031 RTC on the Hi6220 SoC. >>>> >>>> I'd like to submit it to be merged. >>>> >>>> Wei has acked the second patch (modulo a whitespace fix which >>>> I've included in this v3), so it seems like both could go >>>> through the clk tree. >>>> >>>> But Wei also seemed open to pulling in a clk tree branch >>>> as it goes through arm-soc. >>>> >>>> Michael/Stephen: If there's no other objections, could you >>>> queue the first patch and make it avilable via the branch for >>>> Wei, or just take both patches? >>> >>> I happen to dread these kind of patchsets these days. There's added >>> dependencies across trees just because a defined name for the clock >>> number is added to a header file. >>> >>> I much prefer to use numerical clocks for one release, and then once >>> everything is in, switch over to the defines in the DTS. >>> >>> That way there are no dependencies, no need to setup a shared branch >>> for a simple 3-line patch, etc. >>> >>> So, mind respinning the DTS piece? >> >> Huh.. > > Sorry if it appeared random, I've complained about it for a while to > submaintainers. :) No.. I get it, the cross-maintainer shared branch is complex enough to want to avoid. I figured it would be easier to just take a maintainer acked patch in via the clk tree, but its not my tree, so I'll leave it to you maintainers to resolve. >> But trying to boot w/ the numerical clock in the DTS, without the clk >> change results in lots of noise: >> [ 116.491458] of_clk_src_onecell_get: invalid clock index 37 >> [ 116.511627] of_clk_src_onecell_get: invalid clock index 38 >> >> Is that acceptable? > > Grmbl. Is it a lot of those? That's definitely not ideal either. If > it's one or two during probe (since clk_gets should ideally fail at > probe time) then I'd be less worried. Its a fair amount of noise, and seems to go beyond probe time. I'm not sure why the probe didn't fail, but its getting late so I'll have to look into it tomorrow. thanks -john ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 7:20 ` John Stultz @ 2016-07-06 7:38 ` Arnd Bergmann 2016-07-06 8:13 ` Wei Xu ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Arnd Bergmann @ 2016-07-06 7:38 UTC (permalink / raw) To: John Stultz Cc: Olof Johansson, lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Wednesday, July 6, 2016 12:20:15 AM CEST John Stultz wrote: > On Wed, Jul 6, 2016 at 12:04 AM, Olof Johansson <olof@lixom.net> wrote: > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: > >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: > >>>> This patchset enables the pl031 RTC on the Hi6220 SoC. > >>>> > >>>> I'd like to submit it to be merged. > >>>> > >>>> Wei has acked the second patch (modulo a whitespace fix which > >>>> I've included in this v3), so it seems like both could go > >>>> through the clk tree. > >>>> > >>>> But Wei also seemed open to pulling in a clk tree branch > >>>> as it goes through arm-soc. > >>>> > >>>> Michael/Stephen: If there's no other objections, could you > >>>> queue the first patch and make it avilable via the branch for > >>>> Wei, or just take both patches? > >>> > >>> I happen to dread these kind of patchsets these days. There's added > >>> dependencies across trees just because a defined name for the clock > >>> number is added to a header file. > >>> > >>> I much prefer to use numerical clocks for one release, and then once > >>> everything is in, switch over to the defines in the DTS. > >>> > >>> That way there are no dependencies, no need to setup a shared branch > >>> for a simple 3-line patch, etc. > >>> > >>> So, mind respinning the DTS piece? > >> > >> Huh.. > > > > Sorry if it appeared random, I've complained about it for a while to > > submaintainers. > > No.. I get it, the cross-maintainer shared branch is complex enough to > want to avoid. I figured it would be easier to just take a maintainer > acked patch in via the clk tree, but its not my tree, so I'll leave it > to you maintainers to resolve. 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. Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 7:38 ` Arnd Bergmann @ 2016-07-06 8:13 ` Wei Xu 2016-07-12 1:03 ` John Stultz 2016-07-07 0:19 ` Michael Turquette 2016-07-07 0:58 ` John Stultz 2 siblings, 1 reply; 23+ messages in thread From: Wei Xu @ 2016-07-06 8:13 UTC (permalink / raw) To: Arnd Bergmann, John Stultz Cc: Olof Johansson, lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Guodong Xu, Zhangfei Gao Hi Arnd, Olof, On 06/07/2016 08:38, 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 <olof@lixom.net> wrote: >>> On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: >>>> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: >>>>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >>>>>> This patchset enables the pl031 RTC on the Hi6220 SoC. >>>>>> >>>>>> I'd like to submit it to be merged. >>>>>> >>>>>> Wei has acked the second patch (modulo a whitespace fix which >>>>>> I've included in this v3), so it seems like both could go >>>>>> through the clk tree. >>>>>> >>>>>> But Wei also seemed open to pulling in a clk tree branch >>>>>> as it goes through arm-soc. >>>>>> >>>>>> Michael/Stephen: If there's no other objections, could you >>>>>> queue the first patch and make it avilable via the branch for >>>>>> Wei, or just take both patches? >>>>> >>>>> I happen to dread these kind of patchsets these days. There's added >>>>> dependencies across trees just because a defined name for the clock >>>>> number is added to a header file. >>>>> >>>>> I much prefer to use numerical clocks for one release, and then once >>>>> everything is in, switch over to the defines in the DTS. >>>>> >>>>> That way there are no dependencies, no need to setup a shared branch >>>>> for a simple 3-line patch, etc. >>>>> >>>>> So, mind respinning the DTS piece? >>>> >>>> Huh.. >>> >>> Sorry if it appeared random, I've complained about it for a while to >>> submaintainers. >> >> No.. I get it, the cross-maintainer shared branch is complex enough to >> want to avoid. I figured it would be easier to just take a maintainer >> acked patch in via the clk tree, but its not my tree, so I'll leave it >> to you maintainers to resolve. > > 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. I have picked this patch based on the clk-hi6220-rtc which is based on 4.7-rc1 and am planning to send out the pull request which will distinguish the clk commits and dts commits. So should I continue to send out the pull request? Thanks! Best Regards, Wei Xu > > Arnd > > . > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 8:13 ` Wei Xu @ 2016-07-12 1:03 ` John Stultz 0 siblings, 0 replies; 23+ messages in thread From: John Stultz @ 2016-07-12 1:03 UTC (permalink / raw) To: Wei Xu Cc: Arnd Bergmann, Olof Johansson, lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Guodong Xu, Zhangfei Gao On Wed, Jul 6, 2016 at 1:13 AM, Wei Xu <xuwei5@hisilicon.com> wrote: > Hi Arnd, Olof, > > On 06/07/2016 08:38, 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 <olof@lixom.net> wrote: >>>> On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: >>>>> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: >>>>>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >>>>>>> This patchset enables the pl031 RTC on the Hi6220 SoC. >>>>>>> >>>>>>> I'd like to submit it to be merged. >>>>>>> >>>>>>> Wei has acked the second patch (modulo a whitespace fix which >>>>>>> I've included in this v3), so it seems like both could go >>>>>>> through the clk tree. >>>>>>> >>>>>>> But Wei also seemed open to pulling in a clk tree branch >>>>>>> as it goes through arm-soc. >>>>>>> >>>>>>> Michael/Stephen: If there's no other objections, could you >>>>>>> queue the first patch and make it avilable via the branch for >>>>>>> Wei, or just take both patches? >>>>>> >>>>>> I happen to dread these kind of patchsets these days. There's added >>>>>> dependencies across trees just because a defined name for the clock >>>>>> number is added to a header file. >>>>>> >>>>>> I much prefer to use numerical clocks for one release, and then once >>>>>> everything is in, switch over to the defines in the DTS. >>>>>> >>>>>> That way there are no dependencies, no need to setup a shared branch >>>>>> for a simple 3-line patch, etc. >>>>>> >>>>>> So, mind respinning the DTS piece? >>>>> >>>>> Huh.. >>>> >>>> Sorry if it appeared random, I've complained about it for a while to >>>> submaintainers. >>> >>> No.. I get it, the cross-maintainer shared branch is complex enough to >>> want to avoid. I figured it would be easier to just take a maintainer >>> acked patch in via the clk tree, but its not my tree, so I'll leave it >>> to you maintainers to resolve. >> >> 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. > > I have picked this patch based on the clk-hi6220-rtc which is based on > 4.7-rc1 and am planning to send out the pull request which will distinguish > the clk commits and dts commits. > > So should I continue to send out the pull request? Hey Wei, I chatted with Arnd on IRC and from that discussion it sounded like the pull request including the single patch clk branch will be the way to go on this one. Arnd: Let me know if I'm miss-characterizing this. I just wanted to make sure there wasn't further confusion what the next steps were. thanks -john ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 7:38 ` Arnd Bergmann 2016-07-06 8:13 ` Wei Xu @ 2016-07-07 0:19 ` Michael Turquette 2016-07-07 8:13 ` Arnd Bergmann 2016-07-07 0:58 ` John Stultz 2 siblings, 1 reply; 23+ messages in thread From: Michael Turquette @ 2016-07-07 0:19 UTC (permalink / raw) To: Arnd Bergmann Cc: John Stultz, Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao Hi! On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> 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 <olof@lixom.net> wrote: > > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: > > >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: > > >>>> This patchset enables the pl031 RTC on the Hi6220 SoC. > > >>>> > > >>>> I'd like to submit it to be merged. > > >>>> > > >>>> Wei has acked the second patch (modulo a whitespace fix which > > >>>> I've included in this v3), so it seems like both could go > > >>>> through the clk tree. > > >>>> > > >>>> But Wei also seemed open to pulling in a clk tree branch > > >>>> as it goes through arm-soc. > > >>>> > > >>>> Michael/Stephen: If there's no other objections, could you > > >>>> queue the first patch and make it avilable via the branch for > > >>>> Wei, or just take both patches? > > >>> > > >>> I happen to dread these kind of patchsets these days. There's added > > >>> dependencies across trees just because a defined name for the clock > > >>> number is added to a header file. > > >>> > > >>> I much prefer to use numerical clocks for one release, and then once > > >>> everything is in, switch over to the defines in the DTS. > > >>> > > >>> That way there are no dependencies, no need to setup a shared branch > > >>> for a simple 3-line patch, etc. > > >>> > > >>> So, mind respinning the DTS piece? > > >> > > >> Huh.. > > > > > > Sorry if it appeared random, I've complained about it for a while to > > > submaintainers. > > > > No.. I get it, the cross-maintainer shared branch is complex enough to > > want to avoid. I figured it would be easier to just take a maintainer > > acked patch in via the clk tree, but its not my tree, so I'll leave it > > to you maintainers to resolve. > > 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. 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. Thoughts? Regards, Mike > > Arnd -- Michael Turquette CEO BayLibre - At the Heart of Embedded Linux http://baylibre.com/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-07 0:19 ` Michael Turquette @ 2016-07-07 8:13 ` Arnd Bergmann [not found] ` <146794382979.73491.3322475351079454720@resonance> 0 siblings, 1 reply; 23+ messages in thread From: Arnd Bergmann @ 2016-07-07 8:13 UTC (permalink / raw) To: Michael Turquette Cc: John Stultz, Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Wednesday, July 6, 2016 5:19:53 PM CEST Michael Turquette wrote: > On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> 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 <olof@lixom.net> wrote: > > > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > > > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: > > > >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: > > > >>>> This patchset enables the pl031 RTC on the Hi6220 SoC. > > > >>>> > > > >>>> I'd like to submit it to be merged. > > > >>>> > > > >>>> Wei has acked the second patch (modulo a whitespace fix which > > > >>>> I've included in this v3), so it seems like both could go > > > >>>> through the clk tree. > > > >>>> > > > >>>> But Wei also seemed open to pulling in a clk tree branch > > > >>>> as it goes through arm-soc. > > > >>>> > > > >>>> Michael/Stephen: If there's no other objections, could you > > > >>>> queue the first patch and make it avilable via the branch for > > > >>>> Wei, or just take both patches? > > > >>> > > > >>> I happen to dread these kind of patchsets these days. There's added > > > >>> dependencies across trees just because a defined name for the clock > > > >>> number is added to a header file. > > > >>> > > > >>> I much prefer to use numerical clocks for one release, and then once > > > >>> everything is in, switch over to the defines in the DTS. > > > >>> > > > >>> That way there are no dependencies, no need to setup a shared branch > > > >>> for a simple 3-line patch, etc. > > > >>> > > > >>> So, mind respinning the DTS piece? > > > >> > > > >> Huh.. > > > > > > > > Sorry if it appeared random, I've complained about it for a while to > > > > submaintainers. > > > > > > No.. I get it, the cross-maintainer shared branch is complex enough to > > > want to avoid. I figured it would be easier to just take a maintainer > > > acked patch in via the clk tree, but its not my tree, so I'll leave it > > > to you maintainers to resolve. > > > > 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? Usually the hardware doesn't change. There is also a risk of having a driver binding that makes sense for the first 10 clocks that get added, and then it later turns out that the chip actually has hundreds of clocks that could really use a completely different binding to allow a simpler driver. Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <146794382979.73491.3322475351079454720@resonance>]
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 [not found] ` <146794382979.73491.3322475351079454720@resonance> @ 2016-07-11 20:21 ` Arnd Bergmann [not found] ` <146827441381.73491.4865692343236492728@resonance> 0 siblings, 1 reply; 23+ messages in thread From: Arnd Bergmann @ 2016-07-11 20:21 UTC (permalink / raw) To: Michael Turquette Cc: John Stultz, Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao 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 <arnd@arndb.de> 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 <olof@lixom.net> wrote: > > > > > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > > > > > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> 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. > > Usually the > > hardware doesn't change. There is also a risk of having a driver > > It's not about the hardware changing, but our incomplete understanding > of the hardware will certainly change. > > > binding that makes sense for the first 10 clocks that get added, > > and then it later turns out that the chip actually has hundreds > > of clocks that could really use a completely different binding > > to allow a simpler driver. > > Can you give an example of this scenario? My impression here was that the include/dt-bindings/clock/hi6220-clock.h was such a case: it started out with the clocks that were initially needed, and then the RTC driver was added and it happened to need additional clocks. My conclusion from this was that the initial set of clocks did not even cover the essential devices and that there would be further additions required once we add more drivers. However, as Guodong Xu said, this was just a mistake and we won't need to make any other changes to this file in the future, so my rant was probably misplaced here. A better example might be include/dt-bindings/clock/exynos5410.h: In this case, we have already had four separate patches adding additional clocks after the driver was first written (by folks that have direct access to the documentation and to the hardware developers), and they even left space for the new numbers in the initial file, so it's not like they didn't know there was something else. Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <146827441381.73491.4865692343236492728@resonance>]
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 [not found] ` <146827441381.73491.4865692343236492728@resonance> @ 2016-07-12 8:51 ` Arnd Bergmann [not found] ` <146834751937.73491.12265160509757545340@resonance> 0 siblings, 1 reply; 23+ messages in thread From: Arnd Bergmann @ 2016-07-12 8:51 UTC (permalink / raw) To: Michael Turquette Cc: John Stultz, Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao 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 <arnd@arndb.de> 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 <olof@lixom.net> wrote: > > > > > > > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > > > > > > > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> 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 <xinliang.liu@linaro.org> 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 <guodong.xu@linaro.org> Signed-off-by: Xinliang Liu <xinliang.liu@linaro.org> Signed-off-by: Michael Turquette <mturquette@baylibre.com> 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,}, ^ permalink raw reply related [flat|nested] 23+ messages in thread
[parent not found: <146834751937.73491.12265160509757545340@resonance>]
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 [not found] ` <146834751937.73491.12265160509757545340@resonance> @ 2016-07-14 14:30 ` Arnd Bergmann 0 siblings, 0 replies; 23+ messages in thread From: Arnd Bergmann @ 2016-07-14 14:30 UTC (permalink / raw) To: Michael Turquette Cc: John Stultz, Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Tuesday, July 12, 2016 11:18:39 AM CEST Michael Turquette wrote: > Quoting Arnd Bergmann (2016-07-12 01:51:36) > > 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: > > 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. > > Clock drivers are hard. Strictly speaking, any clock that is consumed > and controlled by a clock consumer driver in Linux should *not* need the > kind of hack you see below in the clock consumer driver. The best fix is > for a display driver to call clk_get() and clk_set_rate() on the clock. Makes sense. > However there are two examples of where this doesn't work: > > 1) There is no consumer driver (e.g. DDR) > 2) Support is coming for the consumer driver Some Day(tm), but we want > things to work for now. I have some vague memory that we talked about initializing clocks from values in DT at some point, which would avoid the need for hardcoding them in the driver. Am I misremembering that, or is it something that just never happened? > The hi6220 clk provider driver was already setting the PLLs, and the > patch below merely tweaks the chosen rate. That's why I accepted it. > Clearly it would be better for the PLL rate to set by the display > driver. Right, my comment wasn't about the fact that you merged this patch on top, but rather about the fact that the driver started out by hardcoding clockrates that are assumed to match whatever the boot loader sets. Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-06 7:38 ` Arnd Bergmann 2016-07-06 8:13 ` Wei Xu 2016-07-07 0:19 ` Michael Turquette @ 2016-07-07 0:58 ` John Stultz 2016-07-07 8:22 ` Arnd Bergmann 2016-07-11 1:30 ` Guodong Xu 2 siblings, 2 replies; 23+ messages in thread From: John Stultz @ 2016-07-07 0:58 UTC (permalink / raw) To: Arnd Bergmann Cc: Olof Johansson, lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> 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 <olof@lixom.net> wrote: >> > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: >> >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: >> >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >> >>>> This patchset enables the pl031 RTC on the Hi6220 SoC. >> >>>> >> >>>> I'd like to submit it to be merged. >> >>>> >> >>>> Wei has acked the second patch (modulo a whitespace fix which >> >>>> I've included in this v3), so it seems like both could go >> >>>> through the clk tree. >> >>>> >> >>>> But Wei also seemed open to pulling in a clk tree branch >> >>>> as it goes through arm-soc. >> >>>> >> >>>> Michael/Stephen: If there's no other objections, could you >> >>>> queue the first patch and make it avilable via the branch for >> >>>> Wei, or just take both patches? >> >>> >> >>> I happen to dread these kind of patchsets these days. There's added >> >>> dependencies across trees just because a defined name for the clock >> >>> number is added to a header file. >> >>> >> >>> I much prefer to use numerical clocks for one release, and then once >> >>> everything is in, switch over to the defines in the DTS. >> >>> >> >>> That way there are no dependencies, no need to setup a shared branch >> >>> for a simple 3-line patch, etc. >> >>> >> >>> So, mind respinning the DTS piece? >> >> >> >> Huh.. >> > >> > Sorry if it appeared random, I've complained about it for a while to >> > submaintainers. >> >> No.. I get it, the cross-maintainer shared branch is complex enough to >> want to avoid. I figured it would be easier to just take a maintainer >> acked patch in via the clk tree, but its not my tree, so I'll leave it >> to you maintainers to resolve. > > 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. Though this seemingly goes against the otherwise widely recommended approach of breaking up patches into small obvious chunks. And personally, and I don't mean to criticize, but the suggestions here (use numerical values, then later rename to macros; add everything in one go, then make dts changes a release later) all seem like non-optimal workarounds for the fact that adding almost any functionality requires cross subsystem-maintainer negotiations (or two release steps to get one bit of functionality merged). It seems like it might even just be clearer to make the two-release-steps method the widely broadcast rule (ie: no dependencies on in-flight patches for dts changes), so this doesn't confuse/dismay new developers. Anyway... In this case, I don't have the clk documentation, so I'll ping Zhangfei to check if there is any other clock values that should be added in the future, but at least for HiKey, while there are still a few clk patches remaining in the tree, I don't have any more additions to the clk list. thanks -john ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-07 0:58 ` John Stultz @ 2016-07-07 8:22 ` Arnd Bergmann 2016-07-08 2:21 ` Michael Turquette 2016-07-11 1:30 ` Guodong Xu 1 sibling, 1 reply; 23+ messages in thread From: Arnd Bergmann @ 2016-07-07 8:22 UTC (permalink / raw) To: John Stultz Cc: Olof Johansson, lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Wednesday, July 6, 2016 5:58:14 PM CEST John Stultz wrote: > On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> 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 <olof@lixom.net> wrote: > >> > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > > Though this seemingly goes against the otherwise widely recommended > approach of breaking up patches into small obvious chunks. > > And personally, and I don't mean to criticize, but the suggestions > here (use numerical values, then later rename to macros; add > everything in one go, then make dts changes a release later) all seem > like non-optimal workarounds for the fact that adding almost any > functionality requires cross subsystem-maintainer negotiations (or two > release steps to get one bit of functionality merged). > > It seems like it might even just be clearer to make the > two-release-steps method the widely broadcast rule (ie: no > dependencies on in-flight patches for dts changes), so this doesn't > confuse/dismay new developers. > > Anyway... In this case, I don't have the clk documentation, so I'll > ping Zhangfei to check if there is any other clock values that should > be added in the future, but at least for HiKey, while there are still > a few clk patches remaining in the tree, I don't have any more > additions to the clk list. I think the main underlying problem is hardware that is so badly structured that there is no way to describe it other than to enumerate each output in a header file and have a separate handler in the driver for it. We typically have it easier for other subsystems like irqchip or gpio where nobody would consider writing a driver that can only handle the I/O lines that are used on their board with a minimal set of drivers, but for some reason it seems acceptable to do it for clock controllers just because they are harder to describe. For the common case where the driver developer actually has a description of the clock controller hardware in a manual, I see no reason not to implement the complete driver right away. Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-07 8:22 ` Arnd Bergmann @ 2016-07-08 2:21 ` Michael Turquette 2016-07-15 8:06 ` Arnd Bergmann 0 siblings, 1 reply; 23+ messages in thread From: Michael Turquette @ 2016-07-08 2:21 UTC (permalink / raw) To: Arnd Bergmann, John Stultz Cc: Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao Quoting Arnd Bergmann (2016-07-07 01:22:30) > On Wednesday, July 6, 2016 5:58:14 PM CEST John Stultz wrote: > > On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> 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 <olof@lixom.net> wrote: > > >> > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: > > > > Though this seemingly goes against the otherwise widely recommended > > approach of breaking up patches into small obvious chunks. > > > > And personally, and I don't mean to criticize, but the suggestions > > here (use numerical values, then later rename to macros; add > > everything in one go, then make dts changes a release later) all seem > > like non-optimal workarounds for the fact that adding almost any > > functionality requires cross subsystem-maintainer negotiations (or two > > release steps to get one bit of functionality merged). > > > > It seems like it might even just be clearer to make the > > two-release-steps method the widely broadcast rule (ie: no > > dependencies on in-flight patches for dts changes), so this doesn't > > confuse/dismay new developers. > > > > Anyway... In this case, I don't have the clk documentation, so I'll > > ping Zhangfei to check if there is any other clock values that should > > be added in the future, but at least for HiKey, while there are still > > a few clk patches remaining in the tree, I don't have any more > > additions to the clk list. > > I think the main underlying problem is hardware that is so badly > structured that there is no way to describe it other than to enumerate > each output in a header file and have a separate handler in the driver > for it. > > We typically have it easier for other subsystems like irqchip or gpio > where nobody would consider writing a driver that can only handle > the I/O lines that are used on their board with a minimal set of > drivers, but for some reason it seems acceptable to do it for clock > controllers just because they are harder to describe. gpio and irqchip are interesting analogues. It makes pretty good sense to expose all of those lines via DT, since those are resources that consumer drivers may be interested in. But is the same true for clock signals? Clearly drivers will care about their input clocks, which are often leaf gates. But the mess and tangle of "root" and "branch" clocks above that? Why expose it to DT if we don't need to? These are resources that are often internal to the clock controller IP block. In an ideal world we would never need to provide a way for clock consumer drivers to get at these root and branch clocks, just the peripheral leaf clocks. As an example of this, ccf has tried to be smart about propagating rate requests up the chain of parents since it was originally merged, and that directly has led to lots of consolidation around the cpufreq-dt.c driver, where a single leaf clock can ultimately affect a PLL or post-divider without the cpufreq driver needing to know the details of the clock hierarchy internal to the clock controller IP block. (in reality we do need to expose root and branch clocks for drivers some times, but I disagree that we should expose every single clock signal just because it is there) > > For the common case where the driver developer actually has a > description of the clock controller hardware in a manual, I see > no reason not to implement the complete driver right away. That case is less common than you might think. Regards, Mike > > Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-08 2:21 ` Michael Turquette @ 2016-07-15 8:06 ` Arnd Bergmann 0 siblings, 0 replies; 23+ messages in thread From: Arnd Bergmann @ 2016-07-15 8:06 UTC (permalink / raw) To: Michael Turquette Cc: John Stultz, Olof Johansson, lkml, arm, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Guodong Xu, Zhangfei Gao On Thursday, July 7, 2016 7:21:51 PM CEST Michael Turquette wrote: > Quoting Arnd Bergmann (2016-07-07 01:22:30) > > On Wednesday, July 6, 2016 5:58:14 PM CEST John Stultz wrote: > > > > We typically have it easier for other subsystems like irqchip or gpio > > where nobody would consider writing a driver that can only handle > > the I/O lines that are used on their board with a minimal set of > > drivers, but for some reason it seems acceptable to do it for clock > > controllers just because they are harder to describe. > > gpio and irqchip are interesting analogues. It makes pretty good sense > to expose all of those lines via DT, since those are resources that > consumer drivers may be interested in. But is the same true for clock > signals? > > Clearly drivers will care about their input clocks, which are often leaf > gates. But the mess and tangle of "root" and "branch" clocks above that? > Why expose it to DT if we don't need to? These are resources that are > often internal to the clock controller IP block. In an ideal world we > would never need to provide a way for clock consumer drivers to get at > these root and branch clocks, just the peripheral leaf clocks. > > As an example of this, ccf has tried to be smart about propagating rate > requests up the chain of parents since it was originally merged, and > that directly has led to lots of consolidation around the cpufreq-dt.c > driver, where a single leaf clock can ultimately affect a PLL or > post-divider without the cpufreq driver needing to know the details of > the clock hierarchy internal to the clock controller IP block. > > (in reality we do need to expose root and branch clocks for drivers some > times, but I disagree that we should expose every single clock signal > just because it is there) (sorry for coming back to this late) I still don't fully understand how we ended up with the missing clk in the specific example here, but it seems to me that what was missing here is indeed a leaf clock, not one of the clocks above it. This is a simple gate that is controlled by a bit in the same register as a number of other clocks, so if I understand you right, it should have been there even if we don't want to expose the internal clocks, correct? Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 2016-07-07 0:58 ` John Stultz 2016-07-07 8:22 ` Arnd Bergmann @ 2016-07-11 1:30 ` Guodong Xu 1 sibling, 0 replies; 23+ messages in thread From: Guodong Xu @ 2016-07-11 1:30 UTC (permalink / raw) To: John Stultz Cc: Arnd Bergmann, Olof Johansson, lkml, arm, Michael Turquette, Stephen Boyd, Rob Herring, Pawel Moll, Wei Xu, Zhangfei Gao On 7 July 2016 at 08:58, John Stultz <john.stultz@linaro.org> wrote: > On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@arndb.de> 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 <olof@lixom.net> wrote: >>> > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@linaro.org> wrote: >>> >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@lixom.net> wrote: >>> >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote: >>> >>>> This patchset enables the pl031 RTC on the Hi6220 SoC. >>> >>>> >>> >>>> I'd like to submit it to be merged. >>> >>>> >>> >>>> Wei has acked the second patch (modulo a whitespace fix which >>> >>>> I've included in this v3), so it seems like both could go >>> >>>> through the clk tree. >>> >>>> >>> >>>> But Wei also seemed open to pulling in a clk tree branch >>> >>>> as it goes through arm-soc. >>> >>>> >>> >>>> Michael/Stephen: If there's no other objections, could you >>> >>>> queue the first patch and make it avilable via the branch for >>> >>>> Wei, or just take both patches? >>> >>> >>> >>> I happen to dread these kind of patchsets these days. There's added >>> >>> dependencies across trees just because a defined name for the clock >>> >>> number is added to a header file. >>> >>> >>> >>> I much prefer to use numerical clocks for one release, and then once >>> >>> everything is in, switch over to the defines in the DTS. >>> >>> >>> >>> That way there are no dependencies, no need to setup a shared branch >>> >>> for a simple 3-line patch, etc. >>> >>> >>> >>> So, mind respinning the DTS piece? >>> >> >>> >> Huh.. >>> > >>> > Sorry if it appeared random, I've complained about it for a while to >>> > submaintainers. >>> >>> No.. I get it, the cross-maintainer shared branch is complex enough to >>> want to avoid. I figured it would be easier to just take a maintainer >>> acked patch in via the clk tree, but its not my tree, so I'll leave it >>> to you maintainers to resolve. >> >> 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. > > Though this seemingly goes against the otherwise widely recommended > approach of breaking up patches into small obvious chunks. > > And personally, and I don't mean to criticize, but the suggestions > here (use numerical values, then later rename to macros; add > everything in one go, then make dts changes a release later) all seem > like non-optimal workarounds for the fact that adding almost any > functionality requires cross subsystem-maintainer negotiations (or two > release steps to get one bit of functionality merged). > > It seems like it might even just be clearer to make the > two-release-steps method the widely broadcast rule (ie: no > dependencies on in-flight patches for dts changes), so this doesn't > confuse/dismay new developers. > > Anyway... In this case, I don't have the clk documentation, so I'll > ping Zhangfei to check if there is any other clock values that should Arnd, John After checking documentation, I didn't see other clock values that need to be added. -Guodong > be added in the future, but at least for HiKey, while there are still > a few clk patches remaining in the tree, I don't have any more > additions to the clk list. > > thanks > -john ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-07-15 8:07 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-30 0:48 [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz 2016-06-30 0:48 ` [PATCH 1/2 v3] clk: hi6220: Add RTC clock for pl031 John Stultz 2016-06-30 19:15 ` Stephen Boyd 2016-06-30 19:23 ` John Stultz 2016-06-30 0:48 ` [PATCH 2/2 v3] arm64: dts: hi6220: Add pl031 RTC support John Stultz 2016-06-30 15:12 ` [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Wei Xu 2016-07-06 5:22 ` Olof Johansson 2016-07-06 6:55 ` John Stultz 2016-07-06 7:04 ` Olof Johansson 2016-07-06 7:20 ` John Stultz 2016-07-06 7:38 ` Arnd Bergmann 2016-07-06 8:13 ` Wei Xu 2016-07-12 1:03 ` John Stultz 2016-07-07 0:19 ` Michael Turquette 2016-07-07 8:13 ` Arnd Bergmann [not found] ` <146794382979.73491.3322475351079454720@resonance> 2016-07-11 20:21 ` Arnd Bergmann [not found] ` <146827441381.73491.4865692343236492728@resonance> 2016-07-12 8:51 ` Arnd Bergmann [not found] ` <146834751937.73491.12265160509757545340@resonance> 2016-07-14 14:30 ` Arnd Bergmann 2016-07-07 0:58 ` John Stultz 2016-07-07 8:22 ` Arnd Bergmann 2016-07-08 2:21 ` Michael Turquette 2016-07-15 8:06 ` Arnd Bergmann 2016-07-11 1:30 ` Guodong Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.