2013-04-26 12:14:38

by Pavel Machek

[permalink] [raw]
Subject: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]>
Tested-by: Dinh Nguyen <[email protected]>

---

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


2013-04-26 12:41:35

by Jamie Iles

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-04-26 21:37:28

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-04-26 21:43:17

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]>

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

2013-05-06 12:48:18

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]>
>
> 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

2013-05-06 13:45:30

by Arnd Bergmann

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-05-06 15:53:10

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-05-06 21:24:52

by Arnd Bergmann

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-05-07 09:41:37

by Jamie Iles

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]>

Acked-by: Jamie Iles <[email protected]>

2013-05-07 13:58:00

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-05-07 16:40:03

by John Stultz

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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

2013-05-07 20:11:36

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 [email protected]. (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 <[email protected]>
Acked-by: Jamie Iles <[email protected]>

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

2013-05-07 20:13:46

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]> (supporter:TIMEKEEPING, NTP)
Thomas Gleixner <[email protected]> (supporter:TIMEKEEPING, NTP)
[email protected] (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

2013-05-08 00:30:57

by John Stultz

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]>
> Acked-by: Jamie Iles <[email protected]>

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

2013-05-10 12:38:27

by Pavel Machek

[permalink] [raw]
Subject: Re: dw_apb_timer_of.c: remove parts that were picoxcell-specific

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 <[email protected]>
> >Acked-by: Jamie Iles <[email protected]>
>
> 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