[PATCH 42/48] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Alexandre Belloni
alexandre.belloni at free-electrons.com
Tue May 16 04:59:16 PDT 2017
Hi Daniel,
Almost one year later, I'm back on that topic.
On 24/06/2016 at 12:07:01 +0200, Daniel Lezcano wrote:
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 47352d25c15e..ff7f4022c749 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -258,6 +258,19 @@ config ATMEL_ST
> > select CLKSRC_OF
> > select MFD_SYSCON
> >
> > +config ATMEL_ARM_TCB_CLKSRC
> > + bool "TC Block Clocksource"
>
> The Kconfig options are set now with the COMPILE_TEST option in order to
> increase the compilation test coverage.
>
> Please, add bool "TC Block Clocksource" if COMPILE_TEST, ...
>
> > + select REGMAP_MMIO
> > + depends on GENERIC_CLOCKEVENTS
> > + depends on SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5
> > + default SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5
>
> ... remove these dependencies and let the SoC's Kconfig to select the timer
> like the other timers are.
>
The main issue with what you suggest is that it removes the possibility
to not compile the driver. This may be interesting for people using the
PIT as the clocksource/clockevent and the TCBs for something else.
> > +static cycle_t tc_get_cycles(struct clocksource *cs)
> > +{
> > + unsigned long flags;
> > + u32 lower, upper, tmp;
> > +
> > + raw_local_irq_save(flags);
> > + do {
> > + regmap_read(tc.regmap, ATMEL_TC_CV(1), &upper);
> > + regmap_read(tc.regmap, ATMEL_TC_CV(0), &lower);
> > + regmap_read(tc.regmap, ATMEL_TC_CV(1), &tmp);
> > + } while (upper != tmp);
> > +
> > + raw_local_irq_restore(flags);
>
> Why is this lock needed ?
>
I've taken that from the old driver, I'm not sure this is needed. Maybe
to lower the chance to have upper != tmp.
> > + return (upper << 16) | lower;
> > +}
> > +
> > +static int tcb_clkevt_oneshot(struct clock_event_device *dev)
> > +{
> > + int ret;
> > +
> > + if (tc.irq_requested)
> > + return 0;
> > +
> > + ret = request_irq(tc.irq, tc_clkevt_irq, IRQF_TIMER | IRQF_SHARED,
> > + "tcb_clkevt", &tc);
> > + if (!ret)
> > + tc.irq_requested = true;
>
> The legacy driver checks clockevent_state_detached() and disables the clock.
>
This feature is different from the legacy driver. Here, the driver is
using a single TCB channel for both clocksource and clockevent while the
legacy driver always uses at least two channels.
> Why is 'irq_requested' and request_irq/free_irq cleaner ?
>
> Isn't there a configuration with the TCB register to disable the clockevent
> only ?
>
I'll try something cleaner anyway.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
More information about the linux-arm-kernel
mailing list