linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* dw_apb_timer_of.c: remove parts that were picoxcell-specific
@ 2013-04-26 12:14 Pavel Machek
  2013-04-26 12:41 ` Jamie Iles
  0 siblings, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2013-04-26 12:14 UTC (permalink / raw)
  To: dinguyen, wd, jamie, linux-arm-kernel, olof, arnd, johnstul,
	tglx, kernel list

It seems we made a mistake when creating dw_apb_timer_of.c:
picoxcell sched_clock parts were not related to dw_apb_timer, yet
we moved them to dw_apb_timer_of, and tried to use them on socfpga.
    
This results in system where user/system time is not measured properly,
as demonstrated by
    
    time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
    
    .
    
So this patch:
    
    1) moves picoxcell sched_clock parts back to picoxcell
    
    2) adds support to use dw_apb_timer s as sched_clock. Picoxcell could
    consider switching to these
    
    3) adds missing of_node_put() in dw_apb_timer_init().
    
Tested on socfpga, soc-next (3.9-rc6) based tree.
    
Signed-off-by: Pavel Machek <pavel@denx.de>
Tested-by: Dinh Nguyen <dinguyen@altera.com>

---

I think this is ready to go to soc-next tree. Please apply,
									Pavel


diff --git a/arch/arm/mach-picoxcell/common.c b/arch/arm/mach-picoxcell/common.c
index 70b441a..22759f5 100644
--- a/arch/arm/mach-picoxcell/common.c
+++ b/arch/arm/mach-picoxcell/common.c
@@ -84,11 +84,39 @@ static void picoxcell_wdt_restart(char mode, const char *cmd)
 	}
 }
 
+static const struct of_device_id picochip_rtc_ids[] __initconst = {
+        { .compatible = "picochip,pc3x2-rtc" },
+        { /* Sentinel */ },
+};
+
+static void __iomem *sched_io_base;
+
+static u32 read_sched_clock(void)
+{
+        return __raw_readl(sched_io_base);
+}
+
+static void __init timer_init(void)
+{
+	u32 rate;
+
+	dw_apb_timer_init(0);
+
+	sched_timer = of_find_matching_node(timer, osctimer_ids);
+	if (!sched_timer)
+		panic("No suitable timer for scheduler clock\n");
+
+	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
+	of_node_put(sched_timer);
+
+	setup_sched_clock(read_sched_clock, 32, rate);
+}
+
 DT_MACHINE_START(PICOXCELL, "Picochip picoXcell")
 	.map_io		= picoxcell_map_io,
 	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= irqchip_init,
-	.init_time	= dw_apb_timer_init,
+	.init_time	= timer_init,
 	.init_machine	= picoxcell_init_machine,
 	.dt_compat	= picoxcell_dt_match,
 	.restart	= picoxcell_wdt_restart,
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index 46a0513..3a030e5 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -116,11 +116,17 @@ static const char *altera_dt_match[] = {
 	NULL
 };
 
+static void __init timer_init(void)
+{
+	dw_apb_timer_init(1);
+}
+
+
 DT_MACHINE_START(SOCFPGA, "Altera SOCFPGA")
 	.smp		= smp_ops(socfpga_smp_ops),
 	.map_io		= socfpga_map_io,
 	.init_irq	= socfpga_init_irq,
-	.init_time	= dw_apb_timer_init,
+	.init_time	= timer_init,
 	.init_machine	= socfpga_cyclone5_init,
 	.restart	= socfpga_cyclone5_restart,
 	.dt_compat	= altera_dt_match,
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 8c2a35f..460ac99 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -21,12 +21,6 @@
 #define APBT_MIN_PERIOD			4
 #define APBT_MIN_DELTA_USEC		200
 
-#define APBTMR_N_LOAD_COUNT		0x00
-#define APBTMR_N_CURRENT_VALUE		0x04
-#define APBTMR_N_CONTROL		0x08
-#define APBTMR_N_EOI			0x0c
-#define APBTMR_N_INT_STATUS		0x10
-
 #define APBTMRS_INT_STATUS		0xa0
 #define APBTMRS_EOI			0xa4
 #define APBTMRS_RAW_INT_STATUS		0xa8
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index ab09ed3..ac4ea00 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -57,7 +57,16 @@ static void add_clockevent(struct device_node *event_timer)
 	dw_apb_clockevent_register(ced);
 }
 
-static void add_clocksource(struct device_node *source_timer)
+static void __iomem *sched_io_base;
+
+/* This is actually same as __apbt_read_clocksource(), but with
+   different interface */
+static u32 read_sched_clock_sptimer(void)
+{
+	return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE);
+}
+
+static void add_clocksource(struct device_node *source_timer, int use_as_scheduler)
 {
 	void __iomem *iobase;
 	struct dw_apb_clocksource *cs;
@@ -71,45 +80,33 @@ static void add_clocksource(struct device_node *source_timer)
 
 	dw_apb_clocksource_start(cs);
 	dw_apb_clocksource_register(cs);
-}
 
-static void __iomem *sched_io_base;
-
-static u32 read_sched_clock(void)
-{
-	return __raw_readl(sched_io_base);
+	if (use_as_scheduler) {
+		sched_io_base = iobase;
+		setup_sched_clock(read_sched_clock_sptimer, 32, rate);
+	}
 }
 
-static const struct of_device_id sptimer_ids[] __initconst = {
-	{ .compatible = "picochip,pc3x2-rtc" },
+static const struct of_device_id osctimer_ids[] __initconst = {
+	{ .compatible = "picochip,pc3x2-timer" },
+	{ .compatible = "snps,dw-apb-timer-osc" },
 	{ .compatible = "snps,dw-apb-timer-sp" },
-	{ /* Sentinel */ },
+	{  /* Sentinel */ },
 };
 
-static void init_sched_clock(void)
-{
-	struct device_node *sched_timer;
-	u32 rate;
-
-	sched_timer = of_find_matching_node(NULL, sptimer_ids);
-	if (!sched_timer)
-		panic("No RTC for sched clock to use");
-
-	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
-	of_node_put(sched_timer);
+/*
+   You don't have to use dw_apb_timer for scheduler clock,
+   this should also work fine on arm:
 
-	setup_sched_clock(read_sched_clock, 32, rate);
-}
+  twd_local_timer_of_register();
+  arch_timer_of_register();                 
+  arch_timer_sched_clock_init();
+*/
 
-static const struct of_device_id osctimer_ids[] __initconst = {
-	{ .compatible = "picochip,pc3x2-timer" },
-	{ .compatible = "snps,dw-apb-timer-osc" },
-	{},
-};
 
-void __init dw_apb_timer_init(void)
+void __init dw_apb_timer_init(int use_as_scheduler)
 {
 	struct device_node *event_timer, *source_timer;
 
 	event_timer = of_find_matching_node(NULL, osctimer_ids);
 	if (!event_timer)
@@ -119,9 +117,8 @@ void __init dw_apb_timer_init(void)
 	source_timer = of_find_matching_node(event_timer, osctimer_ids);
 	if (!source_timer)
 		panic("No timer for clocksource");
-	add_clocksource(source_timer);
+	add_clocksource(source_timer, use_as_scheduler);
 
+	of_node_put(event_timer);
 	of_node_put(source_timer);
-
-	init_sched_clock();
 }
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index dd755ce..d503245 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -17,6 +17,12 @@
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
 
+#define APBTMR_N_LOAD_COUNT		0x00
+#define APBTMR_N_CURRENT_VALUE		0x04
+#define APBTMR_N_CONTROL		0x08
+#define APBTMR_N_EOI			0x0c
+#define APBTMR_N_INT_STATUS		0x10
+
 #define APBTMRS_REG_SIZE       0x14
 
 struct dw_apb_timer {
@@ -53,5 +59,5 @@ void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs);
 cycle_t dw_apb_clocksource_read(struct dw_apb_clocksource *dw_cs);
 void dw_apb_clocksource_unregister(struct dw_apb_clocksource *dw_cs);
 
-extern void dw_apb_timer_init(void);
+extern void dw_apb_timer_init(int);
 #endif /* __DW_APB_TIMER_H__ */

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-04-26 12:14 dw_apb_timer_of.c: remove parts that were picoxcell-specific Pavel Machek
@ 2013-04-26 12:41 ` Jamie Iles
  2013-04-26 21:37   ` Pavel Machek
  0 siblings, 1 reply; 15+ messages in thread
From: Jamie Iles @ 2013-04-26 12:41 UTC (permalink / raw)
  To: Pavel Machek
  Cc: dinguyen, wd, jamie, linux-arm-kernel, olof, arnd, johnstul,
	tglx, kernel list

Hi Pavel,

On Fri, Apr 26, 2013 at 02:14:34PM +0200, Pavel Machek wrote:
> diff --git a/arch/arm/mach-picoxcell/common.c b/arch/arm/mach-picoxcell/common.c
> index 70b441a..22759f5 100644
> --- a/arch/arm/mach-picoxcell/common.c
> +++ b/arch/arm/mach-picoxcell/common.c
> @@ -84,11 +84,39 @@ static void picoxcell_wdt_restart(char mode, const char *cmd)
>  	}
>  }
>  
> +static const struct of_device_id picochip_rtc_ids[] __initconst = {
> +        { .compatible = "picochip,pc3x2-rtc" },
> +        { /* Sentinel */ },
> +};
> +
> +static void __iomem *sched_io_base;
> +
> +static u32 read_sched_clock(void)
> +{
> +        return __raw_readl(sched_io_base);
> +}
> +
> +static void __init timer_init(void)
> +{
> +	u32 rate;
> +
> +	dw_apb_timer_init(0);
> +
> +	sched_timer = of_find_matching_node(timer, osctimer_ids);
> +	if (!sched_timer)
> +		panic("No suitable timer for scheduler clock\n");
> +
> +	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
> +	of_node_put(sched_timer);

This doesn't work as osctimer_ids is private to the dw_apb_timer files 
as is timer_get_base_and_rate.  The other timer is unused in picoxcell 
though so dw_apb_timer_init(1), something like the patch below on top of 
yours.

Thanks,

Jamie


diff --git i/arch/arm/mach-picoxcell/common.c w/arch/arm/mach-picoxcell/common.c
index 1484841..a92d203 100644
--- i/arch/arm/mach-picoxcell/common.c
+++ w/arch/arm/mach-picoxcell/common.c
@@ -82,32 +82,9 @@ static void picoxcell_wdt_restart(char mode, const char *cmd)
 	}
 }
 
-static const struct of_device_id picochip_rtc_ids[] __initconst = {
-        { .compatible = "picochip,pc3x2-rtc" },
-        { /* Sentinel */ },
-};
-
-static void __iomem *sched_io_base;
-
-static u32 read_sched_clock(void)
-{
-        return __raw_readl(sched_io_base);
-}
-
 static void __init timer_init(void)
 {
-	u32 rate;
-
-	dw_apb_timer_init(0);
-
-	sched_timer = of_find_matching_node(timer, osctimer_ids);
-	if (!sched_timer)
-		panic("No suitable timer for scheduler clock\n");
-
-	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
-	of_node_put(sched_timer);
-
-	setup_sched_clock(read_sched_clock, 32, rate);
+	dw_apb_timer_init(1);
 }
 
 DT_MACHINE_START(PICOXCELL, "Picochip picoXcell")

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-04-26 12:41 ` Jamie Iles
@ 2013-04-26 21:37   ` Pavel Machek
  2013-04-26 21:43     ` Pavel Machek
  0 siblings, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2013-04-26 21:37 UTC (permalink / raw)
  To: Jamie Iles
  Cc: dinguyen, wd, linux-arm-kernel, olof, arnd, johnstul, tglx, kernel list

Hi!

> On Fri, Apr 26, 2013 at 02:14:34PM +0200, Pavel Machek wrote:
> > diff --git a/arch/arm/mach-picoxcell/common.c b/arch/arm/mach-picoxcell/common.c
> > index 70b441a..22759f5 100644
> > --- a/arch/arm/mach-picoxcell/common.c
> > +++ b/arch/arm/mach-picoxcell/common.c
> > @@ -84,11 +84,39 @@ static void picoxcell_wdt_restart(char mode, const char *cmd)
> >  	}
> >  }
> >  
> > +static const struct of_device_id picochip_rtc_ids[] __initconst = {
> > +        { .compatible = "picochip,pc3x2-rtc" },
> > +        { /* Sentinel */ },
> > +};
> > +
> > +static void __iomem *sched_io_base;
> > +
> > +static u32 read_sched_clock(void)
> > +{
> > +        return __raw_readl(sched_io_base);
> > +}
> > +
> > +static void __init timer_init(void)
> > +{
> > +	u32 rate;
> > +
> > +	dw_apb_timer_init(0);
> > +
> > +	sched_timer = of_find_matching_node(timer, osctimer_ids);
> > +	if (!sched_timer)
> > +		panic("No suitable timer for scheduler clock\n");
> > +
> > +	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
> > +	of_node_put(sched_timer);
> 
> This doesn't work as osctimer_ids is private to the dw_apb_timer files 
> as is timer_get_base_and_rate.  The other timer is unused in picoxcell 
> though so dw_apb_timer_init(1), something like the patch below on top of 
> yours.

That would still not compile due to common.h. ... _but_ it allows
some very nice simplification, with this on top:

I'll post merged-up patch in next mail.

Thanks,
									Pavel

diff --git a/arch/arm/mach-picoxcell/common.c b/arch/arm/mach-picoxcell/common.c
index c027920..70b441a 100644
--- a/arch/arm/mach-picoxcell/common.c
+++ b/arch/arm/mach-picoxcell/common.c
@@ -84,16 +84,11 @@ static void picoxcell_wdt_restart(char mode, const char *cmd)
 	}
 }
 
-static void __init timer_init(void)
-{
-	dw_apb_timer_init(1);
-}
-
 DT_MACHINE_START(PICOXCELL, "Picochip picoXcell")
 	.map_io		= picoxcell_map_io,
 	.nr_irqs	= NR_IRQS_LEGACY,
 	.init_irq	= irqchip_init,
-	.init_time	= timer_init,
+	.init_time	= dw_apb_timer_init,
 	.init_machine	= picoxcell_init_machine,
 	.dt_compat	= picoxcell_dt_match,
 	.restart	= picoxcell_wdt_restart,
diff --git a/arch/arm/mach-picoxcell/common.h b/arch/arm/mach-picoxcell/common.h
index 481b42a..237fb3b 100644
--- a/arch/arm/mach-picoxcell/common.h
+++ b/arch/arm/mach-picoxcell/common.h
@@ -12,6 +12,4 @@
 
 #include <asm/mach/time.h>
 
-extern void dw_apb_timer_init(void);
-
 #endif /* __PICOXCELL_COMMON_H__ */
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index 6efbc12..81b8f1e 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -110,17 +110,11 @@ static const char *altera_dt_match[] = {
 	NULL
 };
 
-static void __init timer_init(void)
-{
-	dw_apb_timer_init(1);
-}
-
-
 DT_MACHINE_START(SOCFPGA, "Altera SOCFPGA")
 	.smp		= smp_ops(socfpga_smp_ops),
 	.map_io		= socfpga_map_io,
 	.init_irq	= socfpga_init_irq,
-	.init_time	= timer_init,
+	.init_time	= dw_apb_timer_init,
 	.init_machine	= socfpga_cyclone5_init,
 	.restart	= socfpga_cyclone5_restart,
 	.dt_compat	= altera_dt_match,
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index 872ce66..0fa3104 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -66,7 +66,7 @@ static u32 read_sched_clock_sptimer(void)
 	return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE);
 }
 
-static void add_clocksource(struct device_node *source_timer, int use_as_scheduler)
+static void add_clocksource(struct device_node *source_timer)
 {
 	void __iomem *iobase;
 	struct dw_apb_clocksource *cs;
@@ -81,10 +81,8 @@ static void add_clocksource(struct device_node *source_timer, int use_as_schedul
 	dw_apb_clocksource_start(cs);
 	dw_apb_clocksource_register(cs);
 
-	if (use_as_scheduler) {
-		sched_io_base = iobase;
-		setup_sched_clock(read_sched_clock_sptimer, 32, rate);
-	}
+	sched_io_base = iobase;
+	setup_sched_clock(read_sched_clock_sptimer, 32, rate);
 }
 
 static const struct of_device_id osctimer_ids[] __initconst = {
@@ -104,7 +102,7 @@ static const struct of_device_id osctimer_ids[] __initconst = {
 */
 
 
-void __init dw_apb_timer_init(int use_as_scheduler)
+void __init dw_apb_timer_init(void)
 {
 	struct device_node *event_timer, *source_timer;
 
@@ -116,7 +114,7 @@ void __init dw_apb_timer_init(int use_as_scheduler)
 	source_timer = of_find_matching_node(event_timer, osctimer_ids);
 	if (!source_timer)
 		panic("No timer for clocksource");
-	add_clocksource(source_timer, use_as_scheduler);
+	add_clocksource(source_timer);
 
 	of_node_put(event_timer);
 	of_node_put(source_timer);
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index d503245..a64c987 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -59,5 +59,5 @@ void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs);
 cycle_t dw_apb_clocksource_read(struct dw_apb_clocksource *dw_cs);
 void dw_apb_clocksource_unregister(struct dw_apb_clocksource *dw_cs);
 
-extern void dw_apb_timer_init(int);
+extern void dw_apb_timer_init(void);
 #endif /* __DW_APB_TIMER_H__ */


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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-04-26 21:37   ` Pavel Machek
@ 2013-04-26 21:43     ` Pavel Machek
  2013-05-06 12:48       ` Pavel Machek
  2013-05-07  9:36       ` Jamie Iles
  0 siblings, 2 replies; 15+ messages in thread
From: Pavel Machek @ 2013-04-26 21:43 UTC (permalink / raw)
  To: Jamie Iles
  Cc: dinguyen, wd, linux-arm-kernel, olof, arnd, johnstul, tglx, kernel list

It seems we made a mistake when creating dw_apb_timer_of.c:
picoxcell sched_clock had parts that were not related to
dw_apb_timer, yet we moved them to dw_apb_timer_of, and tried to
use them on socfpga.

This results in system where user/system time is not measured
properly, as demonstrated by
    
    time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
    
So this patch switches sched_clock to hardware that exists on both
platforms, and adds missing of_node_put() in dw_apb_timer_init().
    
Signed-off-by: Pavel Machek <pavel@denx.de>

diff --git a/arch/arm/mach-picoxcell/common.h b/arch/arm/mach-picoxcell/common.h
index 481b42a..237fb3b 100644
--- a/arch/arm/mach-picoxcell/common.h
+++ b/arch/arm/mach-picoxcell/common.h
@@ -12,6 +12,4 @@
 
 #include <asm/mach/time.h>
 
-extern void dw_apb_timer_init(void);
-
 #endif /* __PICOXCELL_COMMON_H__ */
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 8c2a35f..460ac99 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -21,12 +21,6 @@
 #define APBT_MIN_PERIOD			4
 #define APBT_MIN_DELTA_USEC		200
 
-#define APBTMR_N_LOAD_COUNT		0x00
-#define APBTMR_N_CURRENT_VALUE		0x04
-#define APBTMR_N_CONTROL		0x08
-#define APBTMR_N_EOI			0x0c
-#define APBTMR_N_INT_STATUS		0x10
-
 #define APBTMRS_INT_STATUS		0xa0
 #define APBTMRS_EOI			0xa4
 #define APBTMRS_RAW_INT_STATUS		0xa8
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index ab09ed3..0fa3104 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -57,6 +57,15 @@ static void add_clockevent(struct device_node *event_timer)
 	dw_apb_clockevent_register(ced);
 }
 
+static void __iomem *sched_io_base;
+
+/* This is actually same as __apbt_read_clocksource(), but with
+   different interface */
+static u32 read_sched_clock_sptimer(void)
+{
+	return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE);
+}
+
 static void add_clocksource(struct device_node *source_timer)
 {
 	void __iomem *iobase;
@@ -71,41 +80,27 @@ static void add_clocksource(struct device_node *source_timer)
 
 	dw_apb_clocksource_start(cs);
 	dw_apb_clocksource_register(cs);
-}
 
-static void __iomem *sched_io_base;
-
-static u32 read_sched_clock(void)
-{
-	return __raw_readl(sched_io_base);
+	sched_io_base = iobase;
+	setup_sched_clock(read_sched_clock_sptimer, 32, rate);
 }
 
-static const struct of_device_id sptimer_ids[] __initconst = {
-	{ .compatible = "picochip,pc3x2-rtc" },
+static const struct of_device_id osctimer_ids[] __initconst = {
+	{ .compatible = "picochip,pc3x2-timer" },
+	{ .compatible = "snps,dw-apb-timer-osc" },
 	{ .compatible = "snps,dw-apb-timer-sp" },
-	{ /* Sentinel */ },
+	{  /* Sentinel */ },
 };
 
-static void init_sched_clock(void)
-{
-	struct device_node *sched_timer;
-	u32 rate;
-
-	sched_timer = of_find_matching_node(NULL, sptimer_ids);
-	if (!sched_timer)
-		panic("No RTC for sched clock to use");
+/*
+   You don't have to use dw_apb_timer for scheduler clock,
+   this should also work fine on arm:
 
-	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
-	of_node_put(sched_timer);
+  twd_local_timer_of_register();
+  arch_timer_of_register();                 
+  arch_timer_sched_clock_init();
+*/
 
-	setup_sched_clock(read_sched_clock, 32, rate);
-}
-
-static const struct of_device_id osctimer_ids[] __initconst = {
-	{ .compatible = "picochip,pc3x2-timer" },
-	{ .compatible = "snps,dw-apb-timer-osc" },
-	{},
-};
 
 void __init dw_apb_timer_init(void)
 {
@@ -121,7 +116,6 @@ void __init dw_apb_timer_init(void)
 		panic("No timer for clocksource");
 	add_clocksource(source_timer);
 
+	of_node_put(event_timer);
 	of_node_put(source_timer);
-
-	init_sched_clock();
 }
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index dd755ce..a64c987 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -17,6 +17,12 @@
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
 
+#define APBTMR_N_LOAD_COUNT		0x00
+#define APBTMR_N_CURRENT_VALUE		0x04
+#define APBTMR_N_CONTROL		0x08
+#define APBTMR_N_EOI			0x0c
+#define APBTMR_N_INT_STATUS		0x10
+
 #define APBTMRS_REG_SIZE       0x14
 
 struct dw_apb_timer {


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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-04-26 21:43     ` Pavel Machek
@ 2013-05-06 12:48       ` Pavel Machek
  2013-05-06 13:45         ` Arnd Bergmann
  2013-05-07  9:36       ` Jamie Iles
  1 sibling, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2013-05-06 12:48 UTC (permalink / raw)
  To: Jamie Iles
  Cc: dinguyen, wd, linux-arm-kernel, olof, arnd, johnstul, tglx, kernel list

Hi!

Ping? Previous version was tested by Dinh, and this one is based on
Jamie's updates, so I assume it is acceptable for him, too.

It brings two platforms closer together, and makes time actually work
on socfpga. It would be good to get it applied.

Thanks,
								Pavel

On Fri 2013-04-26 23:43:13, Pavel Machek wrote:
> It seems we made a mistake when creating dw_apb_timer_of.c:
> picoxcell sched_clock had parts that were not related to
> dw_apb_timer, yet we moved them to dw_apb_timer_of, and tried to
> use them on socfpga.
> 
> This results in system where user/system time is not measured
> properly, as demonstrated by
>     
>     time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
>     
> So this patch switches sched_clock to hardware that exists on both
> platforms, and adds missing of_node_put() in dw_apb_timer_init().
>     
> Signed-off-by: Pavel Machek <pavel@denx.de>
> 
> diff --git a/arch/arm/mach-picoxcell/common.h b/arch/arm/mach-picoxcell/common.h
> index 481b42a..237fb3b 100644
> --- a/arch/arm/mach-picoxcell/common.h
> +++ b/arch/arm/mach-picoxcell/common.h
> @@ -12,6 +12,4 @@
>  
>  #include <asm/mach/time.h>
>  
> -extern void dw_apb_timer_init(void);
> -
>  #endif /* __PICOXCELL_COMMON_H__ */
> diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
> index 8c2a35f..460ac99 100644
> --- a/drivers/clocksource/dw_apb_timer.c
> +++ b/drivers/clocksource/dw_apb_timer.c
> @@ -21,12 +21,6 @@
>  #define APBT_MIN_PERIOD			4
>  #define APBT_MIN_DELTA_USEC		200
>  
> -#define APBTMR_N_LOAD_COUNT		0x00
> -#define APBTMR_N_CURRENT_VALUE		0x04
> -#define APBTMR_N_CONTROL		0x08
> -#define APBTMR_N_EOI			0x0c
> -#define APBTMR_N_INT_STATUS		0x10
> -
>  #define APBTMRS_INT_STATUS		0xa0
>  #define APBTMRS_EOI			0xa4
>  #define APBTMRS_RAW_INT_STATUS		0xa8
> diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
> index ab09ed3..0fa3104 100644
> --- a/drivers/clocksource/dw_apb_timer_of.c
> +++ b/drivers/clocksource/dw_apb_timer_of.c
> @@ -57,6 +57,15 @@ static void add_clockevent(struct device_node *event_timer)
>  	dw_apb_clockevent_register(ced);
>  }
>  
> +static void __iomem *sched_io_base;
> +
> +/* This is actually same as __apbt_read_clocksource(), but with
> +   different interface */
> +static u32 read_sched_clock_sptimer(void)
> +{
> +	return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE);
> +}
> +
>  static void add_clocksource(struct device_node *source_timer)
>  {
>  	void __iomem *iobase;
> @@ -71,41 +80,27 @@ static void add_clocksource(struct device_node *source_timer)
>  
>  	dw_apb_clocksource_start(cs);
>  	dw_apb_clocksource_register(cs);
> -}
>  
> -static void __iomem *sched_io_base;
> -
> -static u32 read_sched_clock(void)
> -{
> -	return __raw_readl(sched_io_base);
> +	sched_io_base = iobase;
> +	setup_sched_clock(read_sched_clock_sptimer, 32, rate);
>  }
>  
> -static const struct of_device_id sptimer_ids[] __initconst = {
> -	{ .compatible = "picochip,pc3x2-rtc" },
> +static const struct of_device_id osctimer_ids[] __initconst = {
> +	{ .compatible = "picochip,pc3x2-timer" },
> +	{ .compatible = "snps,dw-apb-timer-osc" },
>  	{ .compatible = "snps,dw-apb-timer-sp" },
> -	{ /* Sentinel */ },
> +	{  /* Sentinel */ },
>  };
>  
> -static void init_sched_clock(void)
> -{
> -	struct device_node *sched_timer;
> -	u32 rate;
> -
> -	sched_timer = of_find_matching_node(NULL, sptimer_ids);
> -	if (!sched_timer)
> -		panic("No RTC for sched clock to use");
> +/*
> +   You don't have to use dw_apb_timer for scheduler clock,
> +   this should also work fine on arm:
>  
> -	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
> -	of_node_put(sched_timer);
> +  twd_local_timer_of_register();
> +  arch_timer_of_register();                 
> +  arch_timer_sched_clock_init();
> +*/
>  
> -	setup_sched_clock(read_sched_clock, 32, rate);
> -}
> -
> -static const struct of_device_id osctimer_ids[] __initconst = {
> -	{ .compatible = "picochip,pc3x2-timer" },
> -	{ .compatible = "snps,dw-apb-timer-osc" },
> -	{},
> -};
>  
>  void __init dw_apb_timer_init(void)
>  {
> @@ -121,7 +116,6 @@ void __init dw_apb_timer_init(void)
>  		panic("No timer for clocksource");
>  	add_clocksource(source_timer);
>  
> +	of_node_put(event_timer);
>  	of_node_put(source_timer);
> -
> -	init_sched_clock();
>  }
> diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
> index dd755ce..a64c987 100644
> --- a/include/linux/dw_apb_timer.h
> +++ b/include/linux/dw_apb_timer.h
> @@ -17,6 +17,12 @@
>  #include <linux/clocksource.h>
>  #include <linux/interrupt.h>
>  
> +#define APBTMR_N_LOAD_COUNT		0x00
> +#define APBTMR_N_CURRENT_VALUE		0x04
> +#define APBTMR_N_CONTROL		0x08
> +#define APBTMR_N_EOI			0x0c
> +#define APBTMR_N_INT_STATUS		0x10
> +
>  #define APBTMRS_REG_SIZE       0x14
>  
>  struct dw_apb_timer {
> 
> 

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-06 12:48       ` Pavel Machek
@ 2013-05-06 13:45         ` Arnd Bergmann
  2013-05-06 15:53           ` Pavel Machek
  0 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2013-05-06 13:45 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jamie Iles, dinguyen, wd, linux-arm-kernel, olof, johnstul, tglx,
	kernel list

On Monday 06 May 2013, Pavel Machek wrote:
> 
> Hi!
> 
> Ping? Previous version was tested by Dinh, and this one is based on
> Jamie's updates, so I assume it is acceptable for him, too.
> 
> It brings two platforms closer together, and makes time actually work
> on socfpga. It would be good to get it applied.

Who should apply it? I guess it needs to go through arm-soc with all the
other clocksource changes already queued up there. Can we have an Ack
from Jamie and from the clocksource maintainers?

	Arnd

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-06 13:45         ` Arnd Bergmann
@ 2013-05-06 15:53           ` Pavel Machek
  2013-05-06 21:24             ` Arnd Bergmann
  0 siblings, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2013-05-06 15:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Jamie Iles, dinguyen, wd, linux-arm-kernel, olof, johnstul, tglx,
	kernel list, john.stultz

On Mon 2013-05-06 15:45:22, Arnd Bergmann wrote:
> On Monday 06 May 2013, Pavel Machek wrote:
> > 
> > Hi!
> > 
> > Ping? Previous version was tested by Dinh, and this one is based on
> > Jamie's updates, so I assume it is acceptable for him, too.
> > 
> > It brings two platforms closer together, and makes time actually work
> > on socfpga. It would be good to get it applied.
> 
> Who should apply it? I guess it needs to go through arm-soc with all the
> other clocksource changes already queued up there. Can we have an Ack
> from Jamie and from the clocksource maintainers?

Well, changes to arch/arm/mach-picoxcell are single line (and that's a
cleanup of duplicate include). 

So I guess it should go through the timekeeping tree...?

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-06 15:53           ` Pavel Machek
@ 2013-05-06 21:24             ` Arnd Bergmann
  2013-05-07 13:57               ` Pavel Machek
  0 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2013-05-06 21:24 UTC (permalink / raw)
  To: Pavel Machek, tglx
  Cc: Jamie Iles, dinguyen, wd, linux-arm-kernel, olof, kernel list,
	john.stultz

On Monday 06 May 2013, Pavel Machek wrote:
> On Mon 2013-05-06 15:45:22, Arnd Bergmann wrote:
> > On Monday 06 May 2013, Pavel Machek wrote:
> > > 
> > > Hi!
> > > 
> > > Ping? Previous version was tested by Dinh, and this one is based on
> > > Jamie's updates, so I assume it is acceptable for him, too.
> > > 
> > > It brings two platforms closer together, and makes time actually work
> > > on socfpga. It would be good to get it applied.
> > 
> > Who should apply it? I guess it needs to go through arm-soc with all the
> > other clocksource changes already queued up there. Can we have an Ack
> > from Jamie and from the clocksource maintainers?
> 
> Well, changes to arch/arm/mach-picoxcell are single line (and that's a
> cleanup of duplicate include). 

The point is that Jamie had comments on the earlier version, so it would
be good to see his Ack on the current one.

> So I guess it should go through the timekeeping tree...?

I would prefer that, but we can also take it through arm-soc, it doesn't
really matter.

	Arnd

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-04-26 21:43     ` Pavel Machek
  2013-05-06 12:48       ` Pavel Machek
@ 2013-05-07  9:36       ` Jamie Iles
  1 sibling, 0 replies; 15+ messages in thread
From: Jamie Iles @ 2013-05-07  9:36 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jamie Iles, dinguyen, wd, linux-arm-kernel, olof, arnd, johnstul,
	tglx, kernel list

Hi Pavel,

Apologies for the delay, but this looks good to me.

On Fri, Apr 26, 2013 at 11:43:14PM +0200, Pavel Machek wrote:
> It seems we made a mistake when creating dw_apb_timer_of.c:
> picoxcell sched_clock had parts that were not related to
> dw_apb_timer, yet we moved them to dw_apb_timer_of, and tried to
> use them on socfpga.
> 
> This results in system where user/system time is not measured
> properly, as demonstrated by
>     
>     time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
>     
> So this patch switches sched_clock to hardware that exists on both
> platforms, and adds missing of_node_put() in dw_apb_timer_init().
>     
> Signed-off-by: Pavel Machek <pavel@denx.de>

Acked-by: Jamie Iles <jamie@jamieiles.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-06 21:24             ` Arnd Bergmann
@ 2013-05-07 13:57               ` Pavel Machek
  2013-05-07 16:39                 ` John Stultz
  0 siblings, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2013-05-07 13:57 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: tglx, Jamie Iles, dinguyen, wd, linux-arm-kernel, olof,
	kernel list, john.stultz

On Mon 2013-05-06 23:24:16, Arnd Bergmann wrote:
> On Monday 06 May 2013, Pavel Machek wrote:
> > On Mon 2013-05-06 15:45:22, Arnd Bergmann wrote:
> > > On Monday 06 May 2013, Pavel Machek wrote:
> > > > 
> > > > Hi!
> > > > 
> > > > Ping? Previous version was tested by Dinh, and this one is based on
> > > > Jamie's updates, so I assume it is acceptable for him, too.
> > > > 
> > > > It brings two platforms closer together, and makes time actually work
> > > > on socfpga. It would be good to get it applied.
> > > 
> > > Who should apply it? I guess it needs to go through arm-soc with all the
> > > other clocksource changes already queued up there. Can we have an Ack
> > > from Jamie and from the clocksource maintainers?
> > 
> > Well, changes to arch/arm/mach-picoxcell are single line (and that's a
> > cleanup of duplicate include). 
> 
> The point is that Jamie had comments on the earlier version, so it would
> be good to see his Ack on the current one.

He sent his Ack this morning.

> > So I guess it should go through the timekeeping tree...?
> 
> I would prefer that, but we can also take it through arm-soc, it doesn't
> really matter.

I see no response from timekeeping people... so if you could take it,
that would be great.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-07 13:57               ` Pavel Machek
@ 2013-05-07 16:39                 ` John Stultz
  2013-05-07 20:11                   ` Pavel Machek
  2013-05-07 20:13                   ` Pavel Machek
  0 siblings, 2 replies; 15+ messages in thread
From: John Stultz @ 2013-05-07 16:39 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Arnd Bergmann, tglx, Jamie Iles, dinguyen, wd, linux-arm-kernel,
	olof, kernel list

On 05/07/2013 06:57 AM, Pavel Machek wrote:
>>> So I guess it should go through the timekeeping tree...?
>> I would prefer that, but we can also take it through arm-soc, it doesn't
>> really matter.
> I see no response from timekeeping people... so if you could take it,
> that would be great.

I don't think the original patch ever made it to my inbox.

thanks
-john



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-07 16:39                 ` John Stultz
@ 2013-05-07 20:11                   ` Pavel Machek
  2013-05-08  0:30                     ` John Stultz
  2013-05-07 20:13                   ` Pavel Machek
  1 sibling, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2013-05-07 20:11 UTC (permalink / raw)
  To: John Stultz
  Cc: Arnd Bergmann, tglx, Jamie Iles, dinguyen, wd, linux-arm-kernel,
	olof, kernel list

Hi!

> >>>So I guess it should go through the timekeeping tree...?
> >>I would prefer that, but we can also take it through arm-soc, it doesn't
> >>really matter.
> >I see no response from timekeeping people... so if you could take it,
> >that would be great.
> 
> I don't think the original patch ever made it to my inbox.

For some reason, I cced johnstul@us.ibm.com. (I guess
get_maintainer.pl? But it seems to behave now.) Anyway, patch is
below...

It would be great if you could apply it... Thanks,
							Pavel

----

It seems we made a mistake when creating dw_apb_timer_of.c:
picoxcell sched_clock had parts that were not related to
dw_apb_timer, yet we moved them to dw_apb_timer_of, and tried to
use them on socfpga.

This results in system where user/system time is not measured
properly, as demonstrated by
    
    time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
    
So this patch switches sched_clock to hardware that exists on both
platforms, and adds missing of_node_put() in dw_apb_timer_init().
    
Signed-off-by: Pavel Machek <pavel@denx.de>
Acked-by: Jamie Iles <jamie@jamieiles.com>

diff --git a/arch/arm/mach-picoxcell/common.h b/arch/arm/mach-picoxcell/common.h
index 481b42a..237fb3b 100644
--- a/arch/arm/mach-picoxcell/common.h
+++ b/arch/arm/mach-picoxcell/common.h
@@ -12,6 +12,4 @@
 
 #include <asm/mach/time.h>
 
-extern void dw_apb_timer_init(void);
-
 #endif /* __PICOXCELL_COMMON_H__ */
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 8c2a35f..460ac99 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -21,12 +21,6 @@
 #define APBT_MIN_PERIOD			4
 #define APBT_MIN_DELTA_USEC		200
 
-#define APBTMR_N_LOAD_COUNT		0x00
-#define APBTMR_N_CURRENT_VALUE		0x04
-#define APBTMR_N_CONTROL		0x08
-#define APBTMR_N_EOI			0x0c
-#define APBTMR_N_INT_STATUS		0x10
-
 #define APBTMRS_INT_STATUS		0xa0
 #define APBTMRS_EOI			0xa4
 #define APBTMRS_RAW_INT_STATUS		0xa8
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index ab09ed3..0fa3104 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -57,6 +57,15 @@ static void add_clockevent(struct device_node *event_timer)
 	dw_apb_clockevent_register(ced);
 }
 
+static void __iomem *sched_io_base;
+
+/* This is actually same as __apbt_read_clocksource(), but with
+   different interface */
+static u32 read_sched_clock_sptimer(void)
+{
+	return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE);
+}
+
 static void add_clocksource(struct device_node *source_timer)
 {
 	void __iomem *iobase;
@@ -71,41 +80,27 @@ static void add_clocksource(struct device_node *source_timer)
 
 	dw_apb_clocksource_start(cs);
 	dw_apb_clocksource_register(cs);
-}
 
-static void __iomem *sched_io_base;
-
-static u32 read_sched_clock(void)
-{
-	return __raw_readl(sched_io_base);
+	sched_io_base = iobase;
+	setup_sched_clock(read_sched_clock_sptimer, 32, rate);
 }
 
-static const struct of_device_id sptimer_ids[] __initconst = {
-	{ .compatible = "picochip,pc3x2-rtc" },
+static const struct of_device_id osctimer_ids[] __initconst = {
+	{ .compatible = "picochip,pc3x2-timer" },
+	{ .compatible = "snps,dw-apb-timer-osc" },
 	{ .compatible = "snps,dw-apb-timer-sp" },
-	{ /* Sentinel */ },
+	{  /* Sentinel */ },
 };
 
-static void init_sched_clock(void)
-{
-	struct device_node *sched_timer;
-	u32 rate;
-
-	sched_timer = of_find_matching_node(NULL, sptimer_ids);
-	if (!sched_timer)
-		panic("No RTC for sched clock to use");
+/*
+   You don't have to use dw_apb_timer for scheduler clock,
+   this should also work fine on arm:
 
-	timer_get_base_and_rate(sched_timer, &sched_io_base, &rate);
-	of_node_put(sched_timer);
+  twd_local_timer_of_register();
+  arch_timer_of_register();                 
+  arch_timer_sched_clock_init();
+*/
 
-	setup_sched_clock(read_sched_clock, 32, rate);
-}
-
-static const struct of_device_id osctimer_ids[] __initconst = {
-	{ .compatible = "picochip,pc3x2-timer" },
-	{ .compatible = "snps,dw-apb-timer-osc" },
-	{},
-};
 
 void __init dw_apb_timer_init(void)
 {
@@ -121,7 +116,6 @@ void __init dw_apb_timer_init(void)
 		panic("No timer for clocksource");
 	add_clocksource(source_timer);
 
+	of_node_put(event_timer);
 	of_node_put(source_timer);
-
-	init_sched_clock();
 }
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index dd755ce..a64c987 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -17,6 +17,12 @@
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
 
+#define APBTMR_N_LOAD_COUNT		0x00
+#define APBTMR_N_CURRENT_VALUE		0x04
+#define APBTMR_N_CONTROL		0x08
+#define APBTMR_N_EOI			0x0c
+#define APBTMR_N_INT_STATUS		0x10
+
 #define APBTMRS_REG_SIZE       0x14
 
 struct dw_apb_timer {


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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-07 16:39                 ` John Stultz
  2013-05-07 20:11                   ` Pavel Machek
@ 2013-05-07 20:13                   ` Pavel Machek
  1 sibling, 0 replies; 15+ messages in thread
From: Pavel Machek @ 2013-05-07 20:13 UTC (permalink / raw)
  To: John Stultz
  Cc: Arnd Bergmann, tglx, Jamie Iles, dinguyen, wd, linux-arm-kernel,
	olof, kernel list

Hi!

> >>>So I guess it should go through the timekeeping tree...?
> >>I would prefer that, but we can also take it through arm-soc, it doesn't
> >>really matter.
> >I see no response from timekeeping people... so if you could take it,
> >that would be great.
> 
> I don't think the original patch ever made it to my inbox.

Aha, so for 3.8, 

pavel@amd:/data/l/linux-n900$ scripts/get_maintainer.pl -f
drivers/clocksource/dw_apb_timer.c
John Stultz <johnstul@us.ibm.com> (supporter:TIMEKEEPING, NTP)
Thomas Gleixner <tglx@linutronix.de> (supporter:TIMEKEEPING, NTP)
linux-kernel@vger.kernel.org (open list)

for 3.9, it works ok.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-07 20:11                   ` Pavel Machek
@ 2013-05-08  0:30                     ` John Stultz
  2013-05-10 12:38                       ` Pavel Machek
  0 siblings, 1 reply; 15+ messages in thread
From: John Stultz @ 2013-05-08  0:30 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Arnd Bergmann, tglx, Jamie Iles, dinguyen, wd, linux-arm-kernel,
	olof, kernel list, jacob.jun.pan

On 05/07/2013 01:11 PM, Pavel Machek wrote:
> It seems we made a mistake when creating dw_apb_timer_of.c:
> picoxcell sched_clock had parts that were not related to
> dw_apb_timer, yet we moved them to dw_apb_timer_of, and tried to
> use them on socfpga.
>
> This results in system where user/system time is not measured
> properly, as demonstrated by
>      
>      time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
>      
> So this patch switches sched_clock to hardware that exists on both
> platforms, and adds missing of_node_put() in dw_apb_timer_init().
>      
> Signed-off-by: Pavel Machek <pavel@denx.de>
> Acked-by: Jamie Iles <jamie@jamieiles.com>

Ok. I'm still not a happy about the general issue of 
sched_clock/clockevent code being in drivers/clocksources (I know, 
everyone is sick of my griping about it :), so reviewing this sucks, but 
at least this patch technically isn't making that issue worse.

Additionally, this is an *ugly* driver in my opinion. Its split between 
arch specific logic in arch/x86/kernel/apb_timer.c, 
arch/arm/mach-picoxcell/common.c, and arch/arm/mach-socfpga/socfpga.c, 
and then arch independent logic in drivers/clocksource/dw_apb_timer.c 
and drivers/clocksource/dw_apb_timer_of.c, but then it seems like much 
of drivers/clocksource/dw_apb_timer_of.c is actually ARM specific.

Are there any plans to clean this up in the future?

Also, next time please run checkpatch.pl to catch trivial issues like 
trailing whitespace. :P

But I've gone ahead and queued this for 3.11.

thanks
-john


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific
  2013-05-08  0:30                     ` John Stultz
@ 2013-05-10 12:38                       ` Pavel Machek
  0 siblings, 0 replies; 15+ messages in thread
From: Pavel Machek @ 2013-05-10 12:38 UTC (permalink / raw)
  To: John Stultz
  Cc: Arnd Bergmann, tglx, Jamie Iles, dinguyen, wd, linux-arm-kernel,
	olof, kernel list, jacob.jun.pan

On Tue 2013-05-07 17:30:52, John Stultz wrote:
> On 05/07/2013 01:11 PM, Pavel Machek wrote:
> >It seems we made a mistake when creating dw_apb_timer_of.c:
> >picoxcell sched_clock had parts that were not related to
> >dw_apb_timer, yet we moved them to dw_apb_timer_of, and tried to
> >use them on socfpga.
> >
> >This results in system where user/system time is not measured
> >properly, as demonstrated by
> >     time dd if=/dev/urandom of=/dev/zero bs=100000 count=100
> >So this patch switches sched_clock to hardware that exists on both
> >platforms, and adds missing of_node_put() in dw_apb_timer_init().
> >Signed-off-by: Pavel Machek <pavel@denx.de>
> >Acked-by: Jamie Iles <jamie@jamieiles.com>
> 
> Ok. I'm still not a happy about the general issue of
> sched_clock/clockevent code being in drivers/clocksources (I know,
> everyone is sick of my griping about it :), so reviewing this sucks,
> but at least this patch technically isn't making that issue worse.

Is there anything concrete you'd like to improve?

Yes, clocksource/clockevent split is confusing, doubly so because it
is same hardware driving both. Would it make sense to create "use this
clocksource for sched_clock()" helper?

> Additionally, this is an *ugly* driver in my opinion. Its split
> between arch specific logic in arch/x86/kernel/apb_timer.c,
> arch/arm/mach-picoxcell/common.c, and
> arch/arm/mach-socfpga/socfpga.c, and 

Can you elaborate? There's very little logic in socfpga.c, just single
line

        .timer          = &dw_apb_timer,

> then arch independent logic in
> drivers/clocksource/dw_apb_timer.c and
> drivers/clocksource/dw_apb_timer_of.c, but then it seems like much
> of drivers/clocksource/dw_apb_timer_of.c is actually ARM specific.

dw_apb_timer_of.c ... it should not be arm specific. Yes, it was
originally separated from picoxcell code, and so far only picoxcell &
socfpga use it, but it should work on other architectures, too..

> Are there any plans to clean this up in the future?

I can try, but I'd need a bit better idea what needs to be done. Would
"use this clocksource for sched_clock()" helper be acceptable?

> But I've gone ahead and queued this for 3.11.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-05-10 12:38 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-26 12:14 dw_apb_timer_of.c: remove parts that were picoxcell-specific Pavel Machek
2013-04-26 12:41 ` Jamie Iles
2013-04-26 21:37   ` Pavel Machek
2013-04-26 21:43     ` Pavel Machek
2013-05-06 12:48       ` Pavel Machek
2013-05-06 13:45         ` Arnd Bergmann
2013-05-06 15:53           ` Pavel Machek
2013-05-06 21:24             ` Arnd Bergmann
2013-05-07 13:57               ` Pavel Machek
2013-05-07 16:39                 ` John Stultz
2013-05-07 20:11                   ` Pavel Machek
2013-05-08  0:30                     ` John Stultz
2013-05-10 12:38                       ` Pavel Machek
2013-05-07 20:13                   ` Pavel Machek
2013-05-07  9:36       ` Jamie Iles

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).