Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1434132 > unrolled thread

[PATCH 0/2 v3] Add pl031 RTC support for Hi6220

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz <john.stultz@linaro.org> - 2016-06-30 03:00 +0200
    [PATCH 2/2 v3] arm64: dts: hi6220: Add pl031 RTC support John Stultz <john.stultz@linaro.org> - 2016-06-30 03:00 +0200
    Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Wei Xu <xuwei5@hisilicon.com> - 2016-06-30 17:20 +0200
    Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Olof Johansson <olof@lixom.net> - 2016-07-06 08:10 +0200
      Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz <john.stultz@linaro.org> - 2016-07-06 09:00 +0200
        Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Olof Johansson <olof@lixom.net> - 2016-07-06 09:10 +0200
          Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz <john.stultz@linaro.org> - 2016-07-06 09:30 +0200
            Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Arnd Bergmann <arnd@arndb.de> - 2016-07-06 09:40 +0200
              Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Wei Xu <xuwei5@hisilicon.com> - 2016-07-06 10:20 +0200
              Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Michael Turquette <mturquette@baylibre.com> - 2016-07-07 02:30 +0200
                Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Arnd Bergmann <arnd@arndb.de> - 2016-07-07 10:20 +0200
              Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 John Stultz <john.stultz@linaro.org> - 2016-07-07 03:00 +0200
                Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Arnd Bergmann <arnd@arndb.de> - 2016-07-07 10:20 +0200
                  Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220 Michael Turquette <mturquette@baylibre.com> - 2016-07-08 04:30 +0200

#1434132 — [PATCH 0/2 v3] Add pl031 RTC support for Hi6220

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

[toc] | [next] | [standalone]

#1434133 — [PATCH 2/2 v3] arm64: dts: hi6220: Add pl031 RTC support

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

[toc] | [prev] | [next] | [standalone]

#1434655

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(-)
> 

[toc] | [prev] | [next] | [standalone]

#1437438

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

[toc] | [prev] | [next] | [standalone]

#1437499

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

[toc] | [prev] | [next] | [standalone]

#1437502

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

[toc] | [prev] | [next] | [standalone]

#1437508

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

[toc] | [prev] | [next] | [standalone]

#1437511

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

[toc] | [prev] | [next] | [standalone]

#1437536

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

[toc] | [prev] | [next] | [standalone]

#1438025

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/

[toc] | [prev] | [next] | [standalone]

#1438311

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

[toc] | [prev] | [next] | [standalone]

#1438041

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

[toc] | [prev] | [next] | [standalone]

#1438323

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

[toc] | [prev] | [next] | [standalone]

#1439072

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web