Re: [PATCH v15 4/5] clocksource/drivers: Add a goldfish-timer clocksource

[Daniel Lezcano]

Hi Laurent,


On 01/04/2022 12:20, Laurent Vivier wrote:
> Daniel ?
>
> Thanks,
> Laurent
>
> Le 10/03/2022 à 10:00, Laurent Vivier a écrit :
> > Add a clocksource based on the goldfish-rtc device.
> >
> > Move the timer register definition to <clocksource/timer-goldfish.h>
> >
> > This kernel implementation is based on the QEMU upstream implementation:
> >
> >     https://git.qemu.org/?p=qemu.git;a=blob_plain;f=hw/rtc/goldfish_rtc.c
> >
> > goldfish-timer is a high-precision signed 64-bit nanosecond timer.
> > It is part of the 'goldfish' virtual hardware platform used to run
> > some emulated Android systems under QEMU.
> > This timer only supports oneshot event.
> >
> > Signed-off-by: Laurent Vivier <laurent@xxxxxxxxx>
> > ---
> >   drivers/clocksource/Kconfig          |   7 ++
> >   drivers/clocksource/Makefile         |   1 +
> >   drivers/clocksource/timer-goldfish.c | 153 +++++++++++++++++++++++++++
> >   drivers/rtc/rtc-goldfish.c           |  13 +--
> >   include/clocksource/timer-goldfish.h |  31 ++++++
> >   5 files changed, 193 insertions(+), 12 deletions(-)
> >   create mode 100644 drivers/clocksource/timer-goldfish.c
> >   create mode 100644 include/clocksource/timer-goldfish.h
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index cfb8ea0df3b1..94f00374cebb 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -721,4 +721,11 @@ config MICROCHIP_PIT64B
> >         modes and high resolution. It is used as a clocksource
> >         and a clockevent.
> > +config GOLDFISH_TIMER
> > +    bool "Clocksource using goldfish-rtc"
> > +    depends on M68K || COMPILE_TEST
> > +    depends on RTC_DRV_GOLDFISH
> > +    help
> > +      Support for the timer/counter of goldfish-rtc
> > +
> >   endmenu
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index fa5f624eadb6..12f5d7e8cc2d 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -89,3 +89,4 @@ obj-$(CONFIG_GX6605S_TIMER)        += timer-gx6605s.o
> >   obj-$(CONFIG_HYPERV_TIMER)        += hyperv_timer.o
> >   obj-$(CONFIG_MICROCHIP_PIT64B)        += timer-microchip-pit64b.o
> >   obj-$(CONFIG_MSC313E_TIMER)        += timer-msc313e.o
> > +obj-$(CONFIG_GOLDFISH_TIMER)        += timer-goldfish.o
> > diff --git a/drivers/clocksource/timer-goldfish.c 
> > b/drivers/clocksource/timer-goldfish.c
> > new file mode 100644
> > index 000000000000..0512d5eabc82
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-goldfish.c
> > @@ -0,0 +1,153 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/ioport.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/goldfish.h>

Not related to this patch: would it make sense to move it to linux/soc/... ?


> > +#include <clocksource/timer-goldfish.h>
> > +
> > +struct goldfish_timer {
> > +    struct clocksource cs;
> > +    struct clock_event_device ced;
> > +    struct resource res;
> > +    void __iomem *base;
> > +};
> > +
> > +static struct goldfish_timer *ced_to_gf(struct clock_event_device *ced)
> > +{
> > +    return container_of(ced, struct goldfish_timer, ced);
> > +}
> > +
> > +static struct goldfish_timer *cs_to_gf(struct clocksource *cs)
> > +{
> > +    return container_of(cs, struct goldfish_timer, cs);
> > +}
> > +
> > +static u64 goldfish_timer_read(struct clocksource *cs)
> > +{
> > +    struct goldfish_timer *timerdrv = cs_to_gf(cs);
> > +    void __iomem *base = timerdrv->base;
> > +    u32 time_low, time_high;
> > +    u64 ticks;
> > +
> > +    /*
> > +     * time_low: get low bits of current time and update time_high
> > +     * time_high: get high bits of time at last time_low read
> > +     */
> > +    time_low = gf_ioread32(base + TIMER_TIME_LOW);
> > +    time_high = gf_ioread32(base + TIMER_TIME_HIGH);

There is a risk here to have the counter rolling over between low and 
high reading, no ?

If it is the case, you may consider using 32bits instead of 64bits, 
otherwise handle the counter wrapping around.

> > +    ticks = ((u64)time_high << 32) | time_low;
> > +
> > +    return ticks;
> > +}
> > +

[ ... ]

> > +/*
> > + * TIMER_TIME_LOW     get low bits of current time and update 
> > TIMER_TIME_HIGH
> > + * TIMER_TIME_HIGH     get high bits of time at last TIMER_TIME_LOW read
> > + * TIMER_ALARM_LOW     set low bits of alarm and activate it
> > + * TIMER_ALARM_HIGH     set high bits of next alarm
> > + * TIMER_IRQ_ENABLED     enable alarm interrupt
> > + * TIMER_CLEAR_ALARM     disarm an existin alarm

typo: s/existin/existing/

> > + * TIMER_ALARM_STATUS     alarm status (running or not)
> > + * TIMER_CLEAR_INTERRUPT clear interrupt
> > + */
> > +#define TIMER_TIME_LOW        0x00
> > +#define TIMER_TIME_HIGH        0x04
> > +#define TIMER_ALARM_LOW        0x08
> > +#define TIMER_ALARM_HIGH    0x0c
> > +#define TIMER_IRQ_ENABLED    0x10
> > +#define TIMER_CLEAR_ALARM    0x14
> > +#define TIMER_ALARM_STATUS    0x18
> > +#define TIMER_CLEAR_INTERRUPT    0x1c
> > +
> > +extern int goldfish_timer_init(int irq, void __iomem *base);
> > +
> > +#endif /* _CLOCKSOURCE_TIMER_GOLDFISH_H */





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