| Message ID | 20130507201126.GA8169@amd.pavel.ucw.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 {