| Message ID | 1447412292-841-1-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 11/13/2015 01:20 PM, Marc Gonzalez wrote: > On 13/11/2015 11:58, Daniel Lezcano wrote: > >> The current code to initialize, register and read the clocksource is >> already factored out in mmio.c via the clocksource_mmio_init function. >> >> Factor out the code with the clocksource_mmio_init function. > > The reason I didn't like clocksource_mmio_init() is because it exports > 4 generic accessors. > > I guess this function makes more sense when all platforms are using it, > in an ARCH_MULTIPLATFORM kernel. (Also the accessors are probably quite > small, so the waste is probably minimal.) Right. > In my opinion, defining struct clocksource_mmio with reg "outside" > struct clocksource leads to less efficient(1) and less clear(2) code. > 1) because of the padding from ____cacheline_aligned > 2) because of the container_of() gymnastics I am not sure that would impact the cacheline. > I tried discussing this in March, but it didn't go anywhere. > Lemme brush up the patch. > > Should the reg field be considered "hot-path data"? Yes. > One problem with my patch: if some ports define CLKSRC_MMIO but > have lots of static struct clocksource, the extra reg field might > waste memory / worsen cache locality? Yes. But the current situation is we have the base address always defined in different drivers, so that won't change the situation too much. The clocksource and the clock_event_device have some commons fields. I am wondering if we can create a common structure for both containing those fields and use them, as the network stack does with the routes, we should have a common structure to deal with, without using the container of. For example: struct clockcommon { u32 mult; u32 shift; int rating; void __iomem *base; char *name; int irq; }; struct clocksource { struct clockcommon common; /* MUST be the first field */ cycle_t (*read)(struct clocksource *cs); cycle_t mask; ... }; struct clockevent { struct clockcommon common; /* MUST be the first field */ ktime_t next_event; ... }; int clocksource_init(struct clockcommon *clock) { struct clocksource *clksrc = (struct clocksource *)clock; } int clockevent_init(struct clockcommon *clock) { struct clockevent *clkevt = (struct clockevent *)clock; } Hence we have the base address for both and we can remove the base@ from all the drivers. The good thing with the mmio is, as you mentioned, instead of having multiple clocksource structure defined, we have just one allocated at init time. The rest falls in the __init section and unloaded. This approach is valid for the multiplatform and I believe all SoC will migrate to it. > Also, maybe the fields should be copied in ascending order? > > Regards. > > > > diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c > index 1593ade2a815..aba5f24ba346 100644 > --- a/drivers/clocksource/mmio.c > +++ b/drivers/clocksource/mmio.c > @@ -10,34 +10,24 @@ > #include <linux/init.h> > #include <linux/slab.h> > > -struct clocksource_mmio { > - void __iomem *reg; > - struct clocksource clksrc; > -}; > - > -static inline struct clocksource_mmio *to_mmio_clksrc(struct clocksource *c) > -{ > - return container_of(c, struct clocksource_mmio, clksrc); > -} > - > cycle_t clocksource_mmio_readl_up(struct clocksource *c) > { > - return (cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg); > + return (cycle_t)readl_relaxed(c->reg); > } > > cycle_t clocksource_mmio_readl_down(struct clocksource *c) > { > - return ~(cycle_t)readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask; > + return ~(cycle_t)readl_relaxed(c->reg) & c->mask; > } > > cycle_t clocksource_mmio_readw_up(struct clocksource *c) > { > - return (cycle_t)readw_relaxed(to_mmio_clksrc(c)->reg); > + return (cycle_t)readw_relaxed(c->reg); > } > > cycle_t clocksource_mmio_readw_down(struct clocksource *c) > { > - return ~(cycle_t)readw_relaxed(to_mmio_clksrc(c)->reg) & c->mask; > + return ~(cycle_t)readw_relaxed(c->reg) & c->mask; > } > > /** > @@ -53,21 +43,21 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name, > unsigned long hz, int rating, unsigned bits, > cycle_t (*read)(struct clocksource *)) > { > - struct clocksource_mmio *cs; > + struct clocksource *cs; > > if (bits > 32 || bits < 16) > return -EINVAL; > > - cs = kzalloc(sizeof(struct clocksource_mmio), GFP_KERNEL); > + cs = kzalloc(sizeof *cs, GFP_KERNEL); > if (!cs) > return -ENOMEM; > > cs->reg = base; > - cs->clksrc.name = name; > - cs->clksrc.rating = rating; > - cs->clksrc.read = read; > - cs->clksrc.mask = CLOCKSOURCE_MASK(bits); > - cs->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS; > + cs->name = name; > + cs->rating = rating; > + cs->read = read; > + cs->mask = CLOCKSOURCE_MASK(bits); > + cs->flags = CLOCK_SOURCE_IS_CONTINUOUS; > > - return clocksource_register_hz(&cs->clksrc, hz); > + return clocksource_register_hz(cs, hz); > } > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index 278dd279a7a8..03807ca0d54e 100644 > --- a/include/linux/clocksource.h > +++ b/include/linux/clocksource.h > @@ -74,6 +74,9 @@ struct clocksource { > u32 shift; > u64 max_idle_ns; > u32 maxadj; > +#ifdef CONFIG_CLKSRC_MMIO > + void __iomem *reg; > +#endif > #ifdef CONFIG_ARCH_CLOCKSOURCE_DATA > struct arch_clocksource_data archdata; > #endif > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 11/13/2015 02:33 PM, Marc Gonzalez wrote: > On 13/11/2015 11:58, Daniel Lezcano wrote: > >> The current code to initialize, register and read the clocksource is >> already factored out in mmio.c via the clocksource_mmio_init function. >> >> Factor out the code with the clocksource_mmio_init function. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/clocksource/tango_xtal.c | 18 +++--------------- >> 1 file changed, 3 insertions(+), 15 deletions(-) > > Hello Daniel, > > I think you also need: > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 63c878d4d9e3..92bf3fc3e646 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -278,6 +278,7 @@ config CLKSRC_MIPS_GIC > config CLKSRC_TANGO_XTAL > bool > select CLKSRC_OF > + select CLKSRC_MMIO > > config CLKSRC_PXA > def_bool y if ARCH_PXA || ARCH_SA1100 > > > Otherwise, we'll get "undefined reference" link errors. Ok, thanks for spotting that. It did not happen because the dependency was set by another driver (using COMPILE_TEST). Will fix it. -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 11/13/2015 01:20 PM, Marc Gonzalez wrote: > On 13/11/2015 11:58, Daniel Lezcano wrote: > >> The current code to initialize, register and read the clocksource is >> already factored out in mmio.c via the clocksource_mmio_init function. >> >> Factor out the code with the clocksource_mmio_init function. > > The reason I didn't like clocksource_mmio_init() is because it exports > 4 generic accessors. > > I guess this function makes more sense when all platforms are using it, > in an ARCH_MULTIPLATFORM kernel. (Also the accessors are probably quite > small, so the waste is probably minimal.) Hi Marc, it is not clear for me if you agree with this patch or not. Can you clarify ? Thanks -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 11/17/2015 01:48 PM, Marc Gonzalez wrote: > On 17/11/2015 13:22, Daniel Lezcano wrote: >> On 11/13/2015 01:20 PM, Marc Gonzalez wrote: >>> On 13/11/2015 11:58, Daniel Lezcano wrote: >>> >>>> The current code to initialize, register and read the clocksource is >>>> already factored out in mmio.c via the clocksource_mmio_init function. >>>> >>>> Factor out the code with the clocksource_mmio_init function. >>> >>> The reason I didn't like clocksource_mmio_init() is because it exports >>> 4 generic accessors. >>> >>> I guess this function makes more sense when all platforms are using it, >>> in an ARCH_MULTIPLATFORM kernel. (Also the accessors are probably quite >>> small, so the waste is probably minimal.) >> >> Hi Marc, >> >> it is not clear for me if you agree with this patch or not. Can you >> clarify ? > > [ Adding rmk as the original mmio.c author ] Good idea. Russell, the patch is https://lkml.org/lkml/2015/11/13/261 > Hello Daniel, > > It's hard to give a straight answer. From my limited perspective (building > a kernel for Sigma boards only) I feel that mmio.c brings nothing. (As you > mentioned, it would be interesting to measure whether having reg_base in > the ctx, rather than as a global is better or worse for perf, though the > actual difference may be lost in the noise.) > > On the other hand, I can see how a different perspective, such as yours, > may see benefits in having all drivers use the same APIs; and there may > be other savings for ARCH_MULTIPLATFORM builds with lots of platforms. IMO, if all the drivers are using the same code, then optimizing it will benefit to all drivers at the same time. > I guess if you think it is a good patch, I will defer to your experience. Ok, I will wait for Russell comments then. Thanks! -- Daniel > I just wish Thomas would take a look at my mmio patch. I will make an > official submission, so that it can be properly shot down :-) -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c index d297b30..e756eea 100644 --- a/drivers/clocksource/tango_xtal.c +++ b/drivers/clocksource/tango_xtal.c @@ -19,19 +19,6 @@ static u64 notrace read_sched_clock(void) return read_xtal_counter(); } -static cycle_t read_clocksource(struct clocksource *cs) -{ - return read_xtal_counter(); -} - -static struct clocksource tango_xtal = { - .name = "tango-xtal", - .rating = 350, - .read = read_clocksource, - .mask = CLOCKSOURCE_MASK(32), - .flags = CLOCK_SOURCE_IS_CONTINUOUS, -}; - static void __init tango_clocksource_init(struct device_node *np) { struct clk *clk; @@ -53,8 +40,9 @@ static void __init tango_clocksource_init(struct device_node *np) delay_timer.freq = xtal_freq; delay_timer.read_current_timer = read_xtal_counter; - ret = clocksource_register_hz(&tango_xtal, xtal_freq); - if (ret != 0) { + ret = clocksource_mmio_init(xtal_in_cnt, "tango-xtal", xtal_freq, 350, + CLOCKSOURCE_MASK(32), clocksource_mmio_readl_up); + if (!ret) { pr_err("%s: registration failed\n", np->full_name); return; }
The current code to initialize, register and read the clocksource is already factored out in mmio.c via the clocksource_mmio_init function. Factor out the code with the clocksource_mmio_init function. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/clocksource/tango_xtal.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/