Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 6 Apr 2022 12:27:48 +0200 | Subject | Re: [PATCH v15 4/5] clocksource/drivers: Add a goldfish-timer clocksource | From | 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@vivier.eu> >> --- >> 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
| ![\](/images/icornerr.gif) |