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

Groups > linux.kernel > #1370276 > unrolled thread

Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Linus Walleij <linus.walleij@linaro.org> - 2016-04-03 21:30 +0200
    Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Leo Yan <leo.yan@linaro.org> - 2016-04-04 03:50 +0200
      Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Linus Walleij <linus.walleij@linaro.org> - 2016-04-04 13:30 +0200
        Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Leo Yan <leo.yan@linaro.org> - 2016-04-04 15:50 +0200
          Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Linus Walleij <linus.walleij@linaro.org> - 2016-04-04 16:00 +0200
            Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Leo Yan <leo.yan@linaro.org> - 2016-04-04 16:10 +0200
    Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220 Rob Herring <robh@kernel.org> - 2016-04-04 07:20 +0200

#1370276 — Re: [PATCH v2 02/16] arm64: dts: add sp804 timer node for Hi6220

On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu <guodong.xu@linaro.org> wrote:

> From: Leo Yan <leo.yan@linaro.org>
>
> Add sp804 timer for hi6220, so it can be used as broadcast timer.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index ad1f1eb..82c4756 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -209,5 +209,14 @@
>                         clock-names = "uartclk", "apb_pclk";
>                         status = "disabled";
>                 };
> +
> +               dual_timer0: dual_timer@f8008000 {
> +                       compatible = "arm,sp804", "arm,primecell";
> +                       reg = <0x0 0xf8008000 0x0 0x1000>;
> +                       interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> +                                    <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ao_ctrl 27>;
> +                       clock-names = "apb_pclk";

How can this work? You only give the apb_pclk for clocking the
bus to the timer.

Most platforms using this driver has something like this:

                timer01: timer@10104000 {
                        compatible = "arm,sp804", "arm,primecell";
                        reg = <0x10104000 0x1000>;
                        interrupt-parent = <&intc_dc1176>;
                        interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>, <0 9
IRQ_TYPE_LEVEL_HIGH>;
                        clocks = <&timclk>, <&timclk>, <&pclk>;
                        clock-names = "timer1", "timer2", "apb_pclk";
                };

It then reads the two clocks in the beginning of clocks() to
determine the frequency of each timer.

By chance the code in the driver will allow just one clock and
will then assume that both the bus to the timer and the timer
itself is clocked from the same clock. But I highly doubt that this
is the case.

Please verify what clocks actually goes into this timer, it should
nominally be three of them.

Yours,
Linus Walleij

[toc] | [next] | [standalone]

#1370325

Hi Linus,

Thanks for review.

On Sun, Apr 03, 2016 at 09:23:42PM +0200, Linus Walleij wrote:
> On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> 
> > From: Leo Yan <leo.yan@linaro.org>
> >
> > Add sp804 timer for hi6220, so it can be used as broadcast timer.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > index ad1f1eb..82c4756 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > @@ -209,5 +209,14 @@
> >                         clock-names = "uartclk", "apb_pclk";
> >                         status = "disabled";
> >                 };
> > +
> > +               dual_timer0: dual_timer@f8008000 {
> > +                       compatible = "arm,sp804", "arm,primecell";
> > +                       reg = <0x0 0xf8008000 0x0 0x1000>;
> > +                       interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> > +                                    <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&ao_ctrl 27>;
> > +                       clock-names = "apb_pclk";
> 
> How can this work? You only give the apb_pclk for clocking the
> bus to the timer.
> 
> Most platforms using this driver has something like this:
> 
>                 timer01: timer@10104000 {
>                         compatible = "arm,sp804", "arm,primecell";
>                         reg = <0x10104000 0x1000>;
>                         interrupt-parent = <&intc_dc1176>;
>                         interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>, <0 9
> IRQ_TYPE_LEVEL_HIGH>;
>                         clocks = <&timclk>, <&timclk>, <&pclk>;
>                         clock-names = "timer1", "timer2", "apb_pclk";
>                 };
> 
> It then reads the two clocks in the beginning of clocks() to
> determine the frequency of each timer.
> 
> By chance the code in the driver will allow just one clock and
> will then assume that both the bus to the timer and the timer
> itself is clocked from the same clock. But I highly doubt that this
> is the case.

This patch has been sent out for review previously [1]; So I refered
other platforms and changed to only enable apb bus clock due I have
not found timer enabling bits in Hi6220's spec.

> Please verify what clocks actually goes into this timer, it should
> nominally be three of them.

Yes, it can work well even I don't enable any timer clock. I will
check furthermore in spec to see if can bind with formal way as your
suggestion.

[1] http://archive.arm.linux.org.uk/lurker/message/20151009.131015.687ed525.en.html

Thanks,
Leo Yan

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

#1370558

On Mon, Apr 4, 2016 at 3:43 AM, Leo Yan <leo.yan@linaro.org> wrote:
> On Sun, Apr 03, 2016 at 09:23:42PM +0200, Linus Walleij wrote:
>> On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu <guodong.xu@linaro.org> wrote:

>> By chance the code in the driver will allow just one clock and
>> will then assume that both the bus to the timer and the timer
>> itself is clocked from the same clock. But I highly doubt that this
>> is the case.
>
> This patch has been sent out for review previously [1]; So I refered
> other platforms and changed to only enable apb bus clock due I have
> not found timer enabling bits in Hi6220's spec.

This is not about enabling/disabling the clock(s) to the timer.
It doesn't matter if these clocks are always on.

It is about determining the *frequency* of the timers.

It is vital that the timer driver get the right frequency of the clock
to the block from the clock implementation, and I do not think
it is the same as the "apb_pclk".

The thing is that of course "any frequency" will
work but what you will notice is that the timer runs very
weirdly compared to wall-clock time unless the right clock
yielding the right frequency has been specified here.

Yours,
Linus Walleij

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

#1370630

On Mon, Apr 04, 2016 at 01:21:00PM +0200, Linus Walleij wrote:
> On Mon, Apr 4, 2016 at 3:43 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > On Sun, Apr 03, 2016 at 09:23:42PM +0200, Linus Walleij wrote:
> >> On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> 
> >> By chance the code in the driver will allow just one clock and
> >> will then assume that both the bus to the timer and the timer
> >> itself is clocked from the same clock. But I highly doubt that this
> >> is the case.
> >
> > This patch has been sent out for review previously [1]; So I refered
> > other platforms and changed to only enable apb bus clock due I have
> > not found timer enabling bits in Hi6220's spec.
> 
> This is not about enabling/disabling the clock(s) to the timer.
> It doesn't matter if these clocks are always on.
> 
> It is about determining the *frequency* of the timers.
> 
> It is vital that the timer driver get the right frequency of the clock
> to the block from the clock implementation, and I do not think
> it is the same as the "apb_pclk".

Thanks for reminding. Fortunately, apb_pclk and timer's clock are
same, all of them's rate are 19.2MHz.

> The thing is that of course "any frequency" will
> work but what you will notice is that the timer runs very
> weirdly compared to wall-clock time unless the right clock
> yielding the right frequency has been specified here.

Compared sp804 timer counter with wall-clock (which is using ARM's
arch timer), and confirmed that sp804 timer's counter register is
decreasing with rate 19.2MHz.

Also have checked Hi6220's spec, there have no timer's dediated clock
enabling bits. This is the reason before I only registered one clock.
So according to you and Rob's comments, how about change as below?

		dual_timer0: timer@f8008000 {
			compatible = "arm,sp804", "arm,primecell";
			reg = <0x0 0xf8008000 0x0 0x1000>;
			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
				     <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
			clocks = <&ao_ctrl HI6220_TIMER0_PCLK>,
				 <&ao_ctrl HI6220_TIMER0_PCLK>,
				 <&ao_ctrl HI6220_TIMER0_PCLK>;
			clock-names = "apb_pclk", "apb_pclk", "apb_pclk";
		};

Thanks,
Leo Yan

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

#1370633

On Mon, Apr 4, 2016 at 3:42 PM, Leo Yan <leo.yan@linaro.org> wrote:
> On Mon, Apr 04, 2016 at 01:21:00PM +0200, Linus Walleij wrote:

>> This is not about enabling/disabling the clock(s) to the timer.
>> It doesn't matter if these clocks are always on.
>>
>> It is about determining the *frequency* of the timers.
>>
>> It is vital that the timer driver get the right frequency of the clock
>> to the block from the clock implementation, and I do not think
>> it is the same as the "apb_pclk".
>
> Thanks for reminding. Fortunately, apb_pclk and timer's clock are
> same, all of them's rate are 19.2MHz.

OK I was wrong, worrying about nothing.

>> The thing is that of course "any frequency" will
>> work but what you will notice is that the timer runs very
>> weirdly compared to wall-clock time unless the right clock
>> yielding the right frequency has been specified here.
>
> Compared sp804 timer counter with wall-clock (which is using ARM's
> arch timer), and confirmed that sp804 timer's counter register is
> decreasing with rate 19.2MHz.

Thanks, awesome.

> Also have checked Hi6220's spec, there have no timer's dediated clock
> enabling bits. This is the reason before I only registered one clock.
> So according to you and Rob's comments, how about change as below?
>
>                 dual_timer0: timer@f8008000 {
>                         compatible = "arm,sp804", "arm,primecell";
>                         reg = <0x0 0xf8008000 0x0 0x1000>;
>                         interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
>                                      <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
>                         clocks = <&ao_ctrl HI6220_TIMER0_PCLK>,
>                                  <&ao_ctrl HI6220_TIMER0_PCLK>,
>                                  <&ao_ctrl HI6220_TIMER0_PCLK>;
>                         clock-names = "apb_pclk", "apb_pclk", "apb_pclk";

This works for me, though I think only the last name
matters so I would name the first two "timer1" and "timer2".

Yours,
Linus Walleij

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

#1370639

On Mon, Apr 04, 2016 at 03:53:47PM +0200, Linus Walleij wrote:
> On Mon, Apr 4, 2016 at 3:42 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > On Mon, Apr 04, 2016 at 01:21:00PM +0200, Linus Walleij wrote:

[...]

> > Also have checked Hi6220's spec, there have no timer's dediated clock
> > enabling bits. This is the reason before I only registered one clock.
> > So according to you and Rob's comments, how about change as below?
> >
> >                 dual_timer0: timer@f8008000 {
> >                         compatible = "arm,sp804", "arm,primecell";
> >                         reg = <0x0 0xf8008000 0x0 0x1000>;
> >                         interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> >                                      <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> >                         clocks = <&ao_ctrl HI6220_TIMER0_PCLK>,
> >                                  <&ao_ctrl HI6220_TIMER0_PCLK>,
> >                                  <&ao_ctrl HI6220_TIMER0_PCLK>;
> >                         clock-names = "apb_pclk", "apb_pclk", "apb_pclk";
> 
> This works for me, though I think only the last name
> matters so I would name the first two "timer1" and "timer2".

Will fix to "timer1" and "timer2" and sent out new patch.

Thanks,
Leo Yan

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

#1370347

On Sun, Apr 03, 2016 at 09:23:42PM +0200, Linus Walleij wrote:
> On Sat, Apr 2, 2016 at 11:29 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> 
> > From: Leo Yan <leo.yan@linaro.org>
> >
> > Add sp804 timer for hi6220, so it can be used as broadcast timer.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > index ad1f1eb..82c4756 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > @@ -209,5 +209,14 @@
> >                         clock-names = "uartclk", "apb_pclk";
> >                         status = "disabled";
> >                 };
> > +
> > +               dual_timer0: dual_timer@f8008000 {

timer@...

> > +                       compatible = "arm,sp804", "arm,primecell";
> > +                       reg = <0x0 0xf8008000 0x0 0x1000>;
> > +                       interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> > +                                    <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&ao_ctrl 27>;
> > +                       clock-names = "apb_pclk";
> 
> How can this work? You only give the apb_pclk for clocking the
> bus to the timer.
> 
> Most platforms using this driver has something like this:
> 
>                 timer01: timer@10104000 {
>                         compatible = "arm,sp804", "arm,primecell";
>                         reg = <0x10104000 0x1000>;
>                         interrupt-parent = <&intc_dc1176>;
>                         interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>, <0 9
> IRQ_TYPE_LEVEL_HIGH>;
>                         clocks = <&timclk>, <&timclk>, <&pclk>;
>                         clock-names = "timer1", "timer2", "apb_pclk";
>                 };
> 
> It then reads the two clocks in the beginning of clocks() to
> determine the frequency of each timer.
> 
> By chance the code in the driver will allow just one clock and
> will then assume that both the bus to the timer and the timer
> itself is clocked from the same clock. But I highly doubt that this
> is the case.

I believe 1 clock was allowed for early bindings, but yes it should be 
3.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web