[PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[dinguyen-EIB2kfCEclfQT0dZR+AlfA]

From: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>

Hi Arnd/Olof,

Because of the following patch series that is currently in arm-soc/for-next:

10021488997317d1121505a7ac659124c058efed clocksource: dw_apb_timer_of: use clocksource_of_init
1b4eca0f634be2a99f2baa6c29dfd183590ead3f clocksource: dw_apb_timer_of: select DW_APB_TIMER
a8b447f2bbbba737ff4478f498d7f83c75a9461b clocksource: dw_apb_timer_of: add clock-handling
a1198f83407ae3421f3f58355a0f296d5ea6249c clocksource: dw_apb_timer_of: enable the use the clocksource as sched clock

there will be a merge conflict with:

55a68c23e0a675b2b8ac2656fd6edbf98b78e4c6 dw_apb_timer_of.c: Remove parts that were picoxcell-specific

that is currently in John Stultz's tree fortglx/3.11/time.

The following 2 patches will eliminate the need for the patch in John
Stultz's tree. If there is to be merge of the 2 trees, then the
patch:

dw_apb_timer_of.c: Remove parts that were picoxcell-specific

can be removed from John's tree to avoid a merge-conflict.

Based on arm-soc/for-next:

PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings to just
"dw-apb-timer"
PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()

Thanks,
Dinh

Dinh Nguyen (2):
  ARM: dts: Change dw-apb-timer-osc and dw-apb-timer-sp to just
    dw-apb-timer
  clocksource: dw_apb_timer_of: Fix read_sched_clock

 Documentation/devicetree/bindings/rtc/dw-apb.txt |   21 +++------------------
 arch/arm/boot/dts/socfpga.dtsi                   |   12 ++++++++----
 drivers/clocksource/dw_apb_timer_of.c            |    7 +++----
 3 files changed, 14 insertions(+), 26 deletions(-)


CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
CC: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
Cc: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Pavel Machek <pavel-ynQEQJNshbs@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

-- 
1.7.9.5

[PATCH 1/2] ARM: dts: Change dw-apb-timer-osc and dw-apb-timer-sp to just dw-apb-timer

[dinguyen-EIB2kfCEclfQT0dZR+AlfA]

From: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>

"dw-apb-timer-osc" and "dw-apb-timer-sp" are the same implementation of the
DW APB timer, just fed by different clocks.

Signed-off-by: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
CC: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
CC: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
Cc: John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Pavel Machek <pavel-ynQEQJNshbs@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
---
 Documentation/devicetree/bindings/rtc/dw-apb.txt |   21 +++------------------
 arch/arm/boot/dts/socfpga.dtsi                   |   12 ++++++++----
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/dw-apb.txt b/Documentation/devicetree/bindings/rtc/dw-apb.txt
index eb2327b..0a1020e 100644
--- a/Documentation/devicetree/bindings/rtc/dw-apb.txt
+++ b/Documentation/devicetree/bindings/rtc/dw-apb.txt
@@ -1,7 +1,7 @@
 * Designware APB timer
 
 Required properties:
-- compatible: "snps,dw-apb-timer-sp" or "snps,dw-apb-timer-osc"
+- compatible: "snps,dw-apb-timer"
 - reg: physical base address of the controller and length of memory mapped
   region.
 - interrupts: IRQ line for the timer.
@@ -20,23 +20,8 @@ systems may use one.
 
 
 Example:
-
-		timer1: timer@ffc09000 {
-				compatible = "snps,dw-apb-timer-sp";
-				interrupts = <0 168 4>;
-				clock-frequency = <200000000>;
-				reg = <0xffc09000 0x1000>;
-			};
-
-		timer2: timer@ffd00000 {
-				compatible = "snps,dw-apb-timer-osc";
-				interrupts = <0 169 4>;
-				clock-frequency = <200000000>;
-				reg = <0xffd00000 0x1000>;
-			};
-
-		timer3: timer@ffe00000 {
-				compatible = "snps,dw-apb-timer-osc";
+		timer1: timer@ffe00000 {
+				compatible = "snps,dw-apb-timer";
 				interrupts = <0 170 4>;
 				reg = <0xffe00000 0x1000>;
 				clocks = <&timer_clk>, <&timer_pclk>;
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index bee62a2..ede33ae 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -476,27 +476,31 @@
 		};
 
 		timer0: timer0@ffc08000 {
-			compatible = "snps,dw-apb-timer-sp";
+			compatible = "snps,dw-apb-timer";
 			interrupts = <0 167 4>;
 			reg = <0xffc08000 0x1000>;
+			clocks = <&osc>;
 		};
 
 		timer1: timer1@ffc09000 {
-			compatible = "snps,dw-apb-timer-sp";
+			compatible = "snps,dw-apb-timer";
 			interrupts = <0 168 4>;
 			reg = <0xffc09000 0x1000>;
+			clocks = <&osc>;
 		};
 
 		timer2: timer2@ffd00000 {
-			compatible = "snps,dw-apb-timer-osc";
+			compatible = "snps,dw-apb-timer";
 			interrupts = <0 169 4>;
 			reg = <0xffd00000 0x1000>;
+			clocks = <&l4_sp_clk>;
 		};
 
 		timer3: timer3@ffd01000 {
-			compatible = "snps,dw-apb-timer-osc";
+			compatible = "snps,dw-apb-timer";
 			interrupts = <0 170 4>;
 			reg = <0xffd01000 0x1000>;
+			clocks = <&l4_sp_clk>;
 		};
 
 		uart0: serial0@ffc02000 {
-- 
1.7.9.5

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[John Stultz]

On 06/17/2013 05:08 PM, dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org wrote:
> From: Dinh Nguyen <dinguyen-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>
>
> Hi Arnd/Olof,
>
> Because of the following patch series that is currently in arm-soc/for-next:
>
> 10021488997317d1121505a7ac659124c058efed clocksource: dw_apb_timer_of: use clocksource_of_init
> 1b4eca0f634be2a99f2baa6c29dfd183590ead3f clocksource: dw_apb_timer_of: select DW_APB_TIMER
> a8b447f2bbbba737ff4478f498d7f83c75a9461b clocksource: dw_apb_timer_of: add clock-handling
> a1198f83407ae3421f3f58355a0f296d5ea6249c clocksource: dw_apb_timer_of: enable the use the clocksource as sched clock
>
> there will be a merge conflict with:
>
> 55a68c23e0a675b2b8ac2656fd6edbf98b78e4c6 dw_apb_timer_of.c: Remove parts that were picoxcell-specific
>
> that is currently in John Stultz's tree fortglx/3.11/time.

:( That one is also in Thomas' tip/timers/core already.


> The following 2 patches will eliminate the need for the patch in John
> Stultz's tree. If there is to be merge of the 2 trees, then the
> patch:
>
> dw_apb_timer_of.c: Remove parts that were picoxcell-specific
>
> can be removed from John's tree to avoid a merge-conflict.
>
> Based on arm-soc/for-next:
>
> PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings to just
> "dw-apb-timer"
> PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()

Pavel/Jamie: Can you take a look at these too and make sure these cover what you were doing.


So Dinh, just to get this right, you're wanting me to revert  "Remove parts that were picoxcell-specific" and apply your two changes to my tree?

The other 4 patches above are then fine to go in via arm-soc? Or do I need to merge those in too?

thanks
-john

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[Dinh Nguyen]

Hi John,

On 06/17/2013 07:38 PM, John Stultz wrote:
> On 06/17/2013 05:08 PM, dinguyen@altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> Hi Arnd/Olof,
>>
>> Because of the following patch series that is currently in
>> arm-soc/for-next:
>>
>> 10021488997317d1121505a7ac659124c058efed clocksource: dw_apb_timer_of:
>> use clocksource_of_init
>> 1b4eca0f634be2a99f2baa6c29dfd183590ead3f clocksource: dw_apb_timer_of:
>> select DW_APB_TIMER
>> a8b447f2bbbba737ff4478f498d7f83c75a9461b clocksource: dw_apb_timer_of:
>> add clock-handling
>> a1198f83407ae3421f3f58355a0f296d5ea6249c clocksource: dw_apb_timer_of:
>> enable the use the clocksource as sched clock
>>
>> there will be a merge conflict with:
>>
>> 55a68c23e0a675b2b8ac2656fd6edbf98b78e4c6 dw_apb_timer_of.c: Remove
>> parts that were picoxcell-specific
>>
>> that is currently in John Stultz's tree fortglx/3.11/time.
> 
> :( That one is also in Thomas' tip/timers/core already.
> 
> 
>> The following 2 patches will eliminate the need for the patch in John
>> Stultz's tree. If there is to be merge of the 2 trees, then the
>> patch:
>>
>> dw_apb_timer_of.c: Remove parts that were picoxcell-specific
>>
>> can be removed from John's tree to avoid a merge-conflict.
>>
>> Based on arm-soc/for-next:
>>
>> PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings
>> to just
>> "dw-apb-timer"
>> PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
> 
> Pavel/Jamie: Can you take a look at these too and make sure these cover
> what you were doing.
> 
> 
> So Dinh, just to get this right, you're wanting me to revert  "Remove
> parts that were picoxcell-specific" and apply your two changes to my tree?

Yes, revert the "Remove parts that were picoxcell-specific". My 2
patches were not based on your tree, so I don't think they can be
applied there. It was based on the arm-soc with the 4 patches in it.

> 
> The other 4 patches above are then fine to go in via arm-soc? Or do I
> need to merge those in too?

The 4 patches are fine in arm-soc. But I'll let Arnd, Olof and yourself
decide on what's best. I just know that Pavel's patch will conflict with
the 4 that are in arm-soc.

Dinh
> 
> thanks
> -john
> 

Re: [PATCH 1/2] ARM: dts: Change dw-apb-timer-osc and dw-apb-timer-sp to just dw-apb-timer

[Arnd Bergmann]

On Tuesday 18 June 2013, dinguyen@altera.com wrote:
> @@ -476,27 +476,31 @@
>                 };
>  
>                 timer0: timer0@ffc08000 {
> -                       compatible = "snps,dw-apb-timer-sp";
> +                       compatible = "snps,dw-apb-timer";
>                         interrupts = <0 167 4>;
>                         reg = <0xffc08000 0x1000>;
> +                       clocks = <&osc>;
>                 };
>  
>                 timer1: timer1@ffc09000 {
> -                       compatible = "snps,dw-apb-timer-sp";
> +                       compatible = "snps,dw-apb-timer";
>                         interrupts = <0 168 4>;
>                         reg = <0xffc09000 0x1000>;
> +                       clocks = <&osc>;
>                 };

I think it would make sense to fix the device name as well, they should
all be named "timer", not "timer0", so I would add


-                 timer0: timer0@ffc08000 {
+                 timer0: timer@ffc08000 {

for all the timers in the dts file here.

	Arnd

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[Pavel Machek]

Hi!

> >The following 2 patches will eliminate the need for the patch in John
> >Stultz's tree. If there is to be merge of the 2 trees, then the
> >patch:
> >
> >dw_apb_timer_of.c: Remove parts that were picoxcell-specific
> >
> >can be removed from John's tree to avoid a merge-conflict.
> >
> >Based on arm-soc/for-next:
> >
> >PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings to just
> >"dw-apb-timer"
> >PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
> 
> Pavel/Jamie: Can you take a look at these too and make sure these cover what you were doing.

[It seems like Heiko Stübner was not aware of patches in the clock
tree, so did pretty much equivalent patch.]

Dinh's changes look good to me, but

[PATCH v2 4/4] clocksource: dw_apb_timer_of: use clocksource_of_init

does not exactly look nice: (I'm sorry I did not see original series,
before it was merged to -soc.). The function counts number of times it
was called, and behaves differently in each case. It is not very
traditional kernel code at the very least.

+static int num_called;
+static void __init dw_apb_timer_init(struct device_node *timer)
 {
-       struct device_node *event_timer, *source_timer;
-
-       event_timer = of_find_matching_node(NULL, osctimer_ids);
-       if (!event_timer)
-               panic("No timer for clockevent");
-       add_clockevent(event_timer);
-       
-       source_timer = of_find_matching_node(event_timer,
osctimer_ids);
-       if (!source_timer)
-               panic("No timer for clocksource");
-       add_clocksource(source_timer);
-
-       of_node_put(source_timer);
+       switch (num_called) {
+       case 0:
+               pr_debug("%s: found clockevent timer\n", __func__);
+               add_clockevent(timer);
+               of_node_put(timer);
+               break;
+       case 1:
+               pr_debug("%s: found clocksource timer\n", __func__);
+               add_clocksource(timer);
+               of_node_put(timer);
+               init_sched_clock();
+               break;
+       default:
+               break;
+       }

-       init_sched_clock();
+       num_called++;
 }

Heiko, can you take a look at John Stultz tree? We modified this area
already... I understand you only have one timer on your silicon?

Would perhaps parameter on dw_apb_timer_init telling it what to do be
better solution? I don't like the "num_called" variable too much...

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[Heiko Stübner]

Hi Pavel,

Am Dienstag, 18. Juni 2013, 17:02:44 schrieb Pavel Machek:
> Hi!
> 
> > >The following 2 patches will eliminate the need for the patch in John
> > >Stultz's tree. If there is to be merge of the 2 trees, then the
> > >patch:
> > >
> > >dw_apb_timer_of.c: Remove parts that were picoxcell-specific
> > >
> > >can be removed from John's tree to avoid a merge-conflict.
> > >
> > >Based on arm-soc/for-next:
> > >
> > >PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings to
> > >just "dw-apb-timer"
> > >PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
> > 
> > Pavel/Jamie: Can you take a look at these too and make sure these cover
> > what you were doing.
> 
> [It seems like Heiko Stübner was not aware of patches in the clock
> tree, so did pretty much equivalent patch.]

Correct ... I was going after what was in linux-next and the tip.git [which I 
also only saw recently at all] does not seem to be part of it. 

> Dinh's changes look good to me, but
> 
> [PATCH v2 4/4] clocksource: dw_apb_timer_of: use clocksource_of_init
> 
> does not exactly look nice: (I'm sorry I did not see original series,
> before it was merged to -soc.). The function counts number of times it
> was called, and behaves differently in each case. It is not very
> traditional kernel code at the very least.
> 
> +static int num_called;
> +static void __init dw_apb_timer_init(struct device_node *timer)
>  {
> -       struct device_node *event_timer, *source_timer;
> -
> -       event_timer = of_find_matching_node(NULL, osctimer_ids);
> -       if (!event_timer)
> -               panic("No timer for clockevent");
> -       add_clockevent(event_timer);
> -
> -       source_timer = of_find_matching_node(event_timer,
> osctimer_ids);
> -       if (!source_timer)
> -               panic("No timer for clocksource");
> -       add_clocksource(source_timer);
> -
> -       of_node_put(source_timer);
> +       switch (num_called) {
> +       case 0:
> +               pr_debug("%s: found clockevent timer\n", __func__);
> +               add_clockevent(timer);
> +               of_node_put(timer);
> +               break;
> +       case 1:
> +               pr_debug("%s: found clocksource timer\n", __func__);
> +               add_clocksource(timer);
> +               of_node_put(timer);
> +               init_sched_clock();
> +               break;
> +       default:
> +               break;
> +       }
> 
> -       init_sched_clock();
> +       num_called++;
>  }
> 
> Heiko, can you take a look at John Stultz tree? We modified this area
> already... I understand you only have one timer on your silicon?

nope, my silicon has actually three timers of this type (all of them of the 
"snps,dw-apb-timer-osc" type ... which did change it seems).

But the clocksource also needs to provide the sched_clock on it.

Due to the multiple matching I came up with the numbering, because the of-
clocksource must match the timer ips multiple times and needs to use one as 
clockevent and one as clocksource.


> Would perhaps parameter on dw_apb_timer_init telling it what to do be
> better solution? I don't like the "num_called" variable too much...

The problem I see is, how do you want to distinguish between the timer used as 
clockevent and the one used as clocksource. The ip blocks are the same, so the 
dt binding must also be the same, as it only describes the hardware.

And the
CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer-osc", dw_apb_timer_init);
of course also matches against all the timer nodes in the dt.


Heiko

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[Heiko Stübner]

Am Dienstag, 18. Juni 2013, 17:38:35 schrieb Heiko Stübner:
> Hi Pavel,
> 
> Am Dienstag, 18. Juni 2013, 17:02:44 schrieb Pavel Machek:
> > Hi!
> > 
> > > >The following 2 patches will eliminate the need for the patch in John
> > > >Stultz's tree. If there is to be merge of the 2 trees, then the
> > > >patch:
> > > >
> > > >dw_apb_timer_of.c: Remove parts that were picoxcell-specific
> > > >
> > > >can be removed from John's tree to avoid a merge-conflict.
> > > >
> > > >Based on arm-soc/for-next:
> > > >
> > > >PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings
> > > >to just "dw-apb-timer"
> > > >PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
> > > 
> > > Pavel/Jamie: Can you take a look at these too and make sure these cover
> > > what you were doing.
> > 
> > [It seems like Heiko Stübner was not aware of patches in the clock
> > tree, so did pretty much equivalent patch.]
> 
> Correct ... I was going after what was in linux-next and the tip.git [which
> I also only saw recently at all] does not seem to be part of it.
> 
> > Dinh's changes look good to me, but
> > 
> > [PATCH v2 4/4] clocksource: dw_apb_timer_of: use clocksource_of_init
> > 
> > does not exactly look nice: (I'm sorry I did not see original series,
> > before it was merged to -soc.). The function counts number of times it
> > was called, and behaves differently in each case. It is not very
> > traditional kernel code at the very least.
> > 
> > +static int num_called;
> > +static void __init dw_apb_timer_init(struct device_node *timer)
> > 
> >  {
> > 
> > -       struct device_node *event_timer, *source_timer;
> > -
> > -       event_timer = of_find_matching_node(NULL, osctimer_ids);
> > -       if (!event_timer)
> > -               panic("No timer for clockevent");
> > -       add_clockevent(event_timer);
> > -
> > -       source_timer = of_find_matching_node(event_timer,
> > osctimer_ids);
> > -       if (!source_timer)
> > -               panic("No timer for clocksource");
> > -       add_clocksource(source_timer);
> > -
> > -       of_node_put(source_timer);
> > +       switch (num_called) {
> > +       case 0:
> > +               pr_debug("%s: found clockevent timer\n", __func__);
> > +               add_clockevent(timer);
> > +               of_node_put(timer);
> > +               break;
> > +       case 1:
> > +               pr_debug("%s: found clocksource timer\n", __func__);
> > +               add_clocksource(timer);
> > +               of_node_put(timer);
> > +               init_sched_clock();
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > 
> > -       init_sched_clock();
> > +       num_called++;
> > 
> >  }
> > 
> > Heiko, can you take a look at John Stultz tree? We modified this area
> > already... I understand you only have one timer on your silicon?

also it seems like not being able to use the apb_timer as sched_clock will 
hurt my platform too.

I've tried to use the arm_arch_timer, but when the arch_timer_get_cntfrq() 
function gets called, I only get an "undefined instruction" Oops for the 
executed asm in there.

As there is no manual available for the SoC, I can only guess that it doesn't 
contain such a component. This is fueled additionally by the PPI part of the 
gic only having 3 interrupt sources [there is small excerpt of the soc-manual 
floating around that contains this information], with one already being the 
twd interrupt, while the arm_arch_timer seems to require 4 itself.

Therefore it would cool, if we could keep the sched_clock functionality 
(provided by the clocksource timer) around somehow.


Heiko



> nope, my silicon has actually three timers of this type (all of them of the
> "snps,dw-apb-timer-osc" type ... which did change it seems).
> 
> But the clocksource also needs to provide the sched_clock on it.
> 
> Due to the multiple matching I came up with the numbering, because the of-
> clocksource must match the timer ips multiple times and needs to use one as
> clockevent and one as clocksource.
> 
> > Would perhaps parameter on dw_apb_timer_init telling it what to do be
> > better solution? I don't like the "num_called" variable too much...
> 
> The problem I see is, how do you want to distinguish between the timer used
> as clockevent and the one used as clocksource. The ip blocks are the same,
> so the dt binding must also be the same, as it only describes the
> hardware.
> 
> And the
> CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer-osc",
> dw_apb_timer_init); of course also matches against all the timer nodes in
> the dt.

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[John Stultz]

On 06/18/2013 11:28 AM, Heiko Stübner wrote:
> Am Dienstag, 18. Juni 2013, 17:38:35 schrieb Heiko Stübner:
>> Hi Pavel,
>>
>> Am Dienstag, 18. Juni 2013, 17:02:44 schrieb Pavel Machek:
>>> Hi!
>>>
>>>>> The following 2 patches will eliminate the need for the patch in John
>>>>> Stultz's tree. If there is to be merge of the 2 trees, then the
>>>>> patch:
>>>>>
>>>>> dw_apb_timer_of.c: Remove parts that were picoxcell-specific
>>>>>
>>>>> can be removed from John's tree to avoid a merge-conflict.
>>>>>
>>>>> Based on arm-soc/for-next:
>>>>>
>>>>> PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings
>>>>> to just "dw-apb-timer"
>>>>> PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
>>>> Pavel/Jamie: Can you take a look at these too and make sure these cover
>>>> what you were doing.
>>> [It seems like Heiko Stübner was not aware of patches in the clock
>>> tree, so did pretty much equivalent patch.]
>> Correct ... I was going after what was in linux-next and the tip.git [which
>> I also only saw recently at all] does not seem to be part of it.
>>
>>> Dinh's changes look good to me, but
>>>
>>> [PATCH v2 4/4] clocksource: dw_apb_timer_of: use clocksource_of_init
>>>
>>> does not exactly look nice: (I'm sorry I did not see original series,
>>> before it was merged to -soc.). The function counts number of times it
>>> was called, and behaves differently in each case. It is not very
>>> traditional kernel code at the very least.
>>>
>>> +static int num_called;
>>> +static void __init dw_apb_timer_init(struct device_node *timer)
>>>
>>>   {
>>>
>>> -       struct device_node *event_timer, *source_timer;
>>> -
>>> -       event_timer = of_find_matching_node(NULL, osctimer_ids);
>>> -       if (!event_timer)
>>> -               panic("No timer for clockevent");
>>> -       add_clockevent(event_timer);
>>> -
>>> -       source_timer = of_find_matching_node(event_timer,
>>> osctimer_ids);
>>> -       if (!source_timer)
>>> -               panic("No timer for clocksource");
>>> -       add_clocksource(source_timer);
>>> -
>>> -       of_node_put(source_timer);
>>> +       switch (num_called) {
>>> +       case 0:
>>> +               pr_debug("%s: found clockevent timer\n", __func__);
>>> +               add_clockevent(timer);
>>> +               of_node_put(timer);
>>> +               break;
>>> +       case 1:
>>> +               pr_debug("%s: found clocksource timer\n", __func__);
>>> +               add_clocksource(timer);
>>> +               of_node_put(timer);
>>> +               init_sched_clock();
>>> +               break;
>>> +       default:
>>> +               break;
>>> +       }
>>>
>>> -       init_sched_clock();
>>> +       num_called++;
>>>
>>>   }
>>>
>>> Heiko, can you take a look at John Stultz tree? We modified this area
>>> already... I understand you only have one timer on your silicon?
> also it seems like not being able to use the apb_timer as sched_clock will
> hurt my platform too.
>
> I've tried to use the arm_arch_timer, but when the arch_timer_get_cntfrq()
> function gets called, I only get an "undefined instruction" Oops for the
> executed asm in there.
>
> As there is no manual available for the SoC, I can only guess that it doesn't
> contain such a component. This is fueled additionally by the PPI part of the
> gic only having 3 interrupt sources [there is small excerpt of the soc-manual
> floating around that contains this information], with one already being the
> twd interrupt, while the arm_arch_timer seems to require 4 itself.
>
> Therefore it would cool, if we could keep the sched_clock functionality
> (provided by the clocksource timer) around somehow.

So whats the plan now?

I'm feeling likely to revert "dw_apb_timer_of.c: Remove parts that were 
picoxcell-specific" in my tree and leave the rest of the wreckage to the 
arm-soc folks to sort out (either getting the fix in or reverting the 
lot and trying again for 3.12), unless we get some agreed upon path 
forward sorted quickly here.

thanks
-john

Re: [PATCH 0/2] Fix potential merge conflict for dw_apb_timer_of

[Olof Johansson]

On Tue, Jun 18, 2013 at 11:40 AM, John Stultz <john.stultz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 06/18/2013 11:28 AM, Heiko Stübner wrote:
>>
>> Am Dienstag, 18. Juni 2013, 17:38:35 schrieb Heiko Stübner:
>>>
>>> Hi Pavel,
>>>
>>> Am Dienstag, 18. Juni 2013, 17:02:44 schrieb Pavel Machek:
>>>>
>>>> Hi!
>>>>
>>>>>> The following 2 patches will eliminate the need for the patch in John
>>>>>> Stultz's tree. If there is to be merge of the 2 trees, then the
>>>>>> patch:
>>>>>>
>>>>>> dw_apb_timer_of.c: Remove parts that were picoxcell-specific
>>>>>>
>>>>>> can be removed from John's tree to avoid a merge-conflict.
>>>>>>
>>>>>> Based on arm-soc/for-next:
>>>>>>
>>>>>> PATCH[1/2] - Rename "dw-apb-timer-osc" and "dw-apb-timer-sp" bindings
>>>>>> to just "dw-apb-timer"
>>>>>> PATCH[2/2] - Fix user/system reporting by fixing read_sched_clock()
>>>>>
>>>>> Pavel/Jamie: Can you take a look at these too and make sure these cover
>>>>> what you were doing.
>>>>
>>>> [It seems like Heiko Stübner was not aware of patches in the clock
>>>> tree, so did pretty much equivalent patch.]
>>>
>>> Correct ... I was going after what was in linux-next and the tip.git
>>> [which
>>> I also only saw recently at all] does not seem to be part of it.
>>>
>>>> Dinh's changes look good to me, but
>>>>
>>>> [PATCH v2 4/4] clocksource: dw_apb_timer_of: use clocksource_of_init
>>>>
>>>> does not exactly look nice: (I'm sorry I did not see original series,
>>>> before it was merged to -soc.). The function counts number of times it
>>>> was called, and behaves differently in each case. It is not very
>>>> traditional kernel code at the very least.
>>>>
>>>> +static int num_called;
>>>> +static void __init dw_apb_timer_init(struct device_node *timer)
>>>>
>>>>   {
>>>>
>>>> -       struct device_node *event_timer, *source_timer;
>>>> -
>>>> -       event_timer = of_find_matching_node(NULL, osctimer_ids);
>>>> -       if (!event_timer)
>>>> -               panic("No timer for clockevent");
>>>> -       add_clockevent(event_timer);
>>>> -
>>>> -       source_timer = of_find_matching_node(event_timer,
>>>> osctimer_ids);
>>>> -       if (!source_timer)
>>>> -               panic("No timer for clocksource");
>>>> -       add_clocksource(source_timer);
>>>> -
>>>> -       of_node_put(source_timer);
>>>> +       switch (num_called) {
>>>> +       case 0:
>>>> +               pr_debug("%s: found clockevent timer\n", __func__);
>>>> +               add_clockevent(timer);
>>>> +               of_node_put(timer);
>>>> +               break;
>>>> +       case 1:
>>>> +               pr_debug("%s: found clocksource timer\n", __func__);
>>>> +               add_clocksource(timer);
>>>> +               of_node_put(timer);
>>>> +               init_sched_clock();
>>>> +               break;
>>>> +       default:
>>>> +               break;
>>>> +       }
>>>>
>>>> -       init_sched_clock();
>>>> +       num_called++;
>>>>
>>>>   }
>>>>
>>>> Heiko, can you take a look at John Stultz tree? We modified this area
>>>> already... I understand you only have one timer on your silicon?
>>
>> also it seems like not being able to use the apb_timer as sched_clock will
>> hurt my platform too.
>>
>> I've tried to use the arm_arch_timer, but when the arch_timer_get_cntfrq()
>> function gets called, I only get an "undefined instruction" Oops for the
>> executed asm in there.
>>
>> As there is no manual available for the SoC, I can only guess that it
>> doesn't
>> contain such a component. This is fueled additionally by the PPI part of
>> the
>> gic only having 3 interrupt sources [there is small excerpt of the
>> soc-manual
>> floating around that contains this information], with one already being
>> the
>> twd interrupt, while the arm_arch_timer seems to require 4 itself.
>>
>> Therefore it would cool, if we could keep the sched_clock functionality
>> (provided by the clocksource timer) around somehow.
>
>
> So whats the plan now?
>
> I'm feeling likely to revert "dw_apb_timer_of.c: Remove parts that were
> picoxcell-specific" in my tree and leave the rest of the wreckage to the
> arm-soc folks to sort out (either getting the fix in or reverting the lot
> and trying again for 3.12), unless we get some agreed upon path forward
> sorted quickly here.

It was bad timing that linux-next was down for a week right around the
time of all of this, but not much we can do about in hindsight.

I'd be OK with reverting on your side and revisiting for 3.12. Arnd is
doing arm-soc merges for a bit so it's up to him if he wants to try
merging it without conflicts on our side.


-Olof