| Message ID | 1443967013-17166-1-git-send-email-thomas@wytron.com.tw |
|---|---|
| State | Superseded |
| Delegated to: | Thomas Chou |
| Headers | show |
Hi Thomas, On 4 October 2015 at 14:56, Thomas Chou <thomas@wytron.com.tw> wrote: > Implement a Timer uclass to work with lib/time.c. > > Signed-off-by: Thomas Chou <thomas@wytron.com.tw> > Reviewed-by: Simon Glass <sjg@chromium.org> Sorry, after a lot of consideration I'd like to retract that :-( Please see below. > --- > v2 > fix coding style. > v3 > add description to Kconfig as Simon suggested. > move timer.c code to lib/time.c. > add dm_timer dev to global data. > remove timer_init(). > change API name get_clock. > v4 > add comment about timer hardware. > > common/board_r.c | 3 +++ > drivers/Kconfig | 2 ++ > drivers/Makefile | 1 + > drivers/timer/Kconfig | 12 +++++++++ > drivers/timer/Makefile | 7 +++++ > drivers/timer/timer-uclass.c | 44 +++++++++++++++++++++++++++++++ > include/asm-generic/global_data.h | 3 +++ > include/dm/uclass-id.h | 1 + > include/timer.h | 52 +++++++++++++++++++++++++++++++++++++ > lib/time.c | 54 +++++++++++++++++++++++++++++++++++++++ > 10 files changed, 179 insertions(+) > create mode 100644 drivers/timer/Kconfig > create mode 100644 drivers/timer/Makefile > create mode 100644 drivers/timer/timer-uclass.c > create mode 100644 include/timer.h Can you please split this into a patch that adds the uclass and one that plumbs it into board_r.c? Also can you add a patch with a sandbox timer driver and a test (in test/dm) for the timer? > > diff --git a/common/board_r.c b/common/board_r.c > index f8c1baa..aaf390e 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -290,6 +290,9 @@ static int initr_dm(void) > /* Save the pre-reloc driver model and start a new one */ > gd->dm_root_f = gd->dm_root; > gd->dm_root = NULL; > +#ifdef CONFIG_DM_TIMER > + gd->dm_timer = NULL; > +#endif > return dm_init_and_scan(false); > } > #endif > diff --git a/drivers/Kconfig b/drivers/Kconfig > index 63c92c5..f9496f7 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -56,6 +56,8 @@ source "drivers/spi/Kconfig" > > source "drivers/thermal/Kconfig" > > +source "drivers/timer/Kconfig" > + > source "drivers/tpm/Kconfig" > > source "drivers/usb/Kconfig" > diff --git a/drivers/Makefile b/drivers/Makefile > index 9d0a595..692da78 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -48,6 +48,7 @@ obj-y += pcmcia/ > obj-y += dfu/ > obj-y += rtc/ > obj-y += sound/ > +obj-y += timer/ > obj-y += tpm/ > obj-y += twserial/ > obj-y += video/ > diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig > new file mode 100644 > index 0000000..54a4c40 > --- /dev/null > +++ b/drivers/timer/Kconfig > @@ -0,0 +1,12 @@ > +menu "Timer Support" > + > +config DM_TIMER > + bool "Enable Driver Model for Timer drivers" > + depends on DM > + help > + Enable driver model for Timer access. It uses the same API as > + lib/time.c. But now implemented by the uclass. The first timer > + will be used. The timer is usually a 32 bits free-running up > + counter. There may be no real tick, and no timer interrupt. > + > +endmenu > diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile > new file mode 100644 > index 0000000..a24179a > --- /dev/null > +++ b/drivers/timer/Makefile > @@ -0,0 +1,7 @@ > +# > +# Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw> > +# > +# SPDX-License-Identifier: GPL-2.0+ > +# > + > +obj-$(CONFIG_DM_TIMER) += timer-uclass.o > diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c > new file mode 100644 > index 0000000..bcf1dde > --- /dev/null > +++ b/drivers/timer/timer-uclass.c > @@ -0,0 +1,44 @@ > +/* > + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <errno.h> > +#include <timer.h> > + > +/* > + * Implement a Timer uclass to work with lib/time.c. The timer is usually > + * a 32 bits free-running up counter. The get_clock() method is used to get > + * the input clock frequency of the timer. The get_count() method is used > + * get the current 32 bits count value. If the hardware is counting down, > + * the value should be inversed inside the method. There may be no real > + * tick, and no timer interrupt. > + */ > + > +int timer_get_count(struct udevice *dev, unsigned long *count) > +{ > + const struct dm_timer_ops *ops = device_get_ops(dev); > + > + if (!ops->get_count) > + return -ENOSYS; > + > + return ops->get_count(dev, count); > +} > + > +int timer_get_clock(struct udevice *dev, unsigned long *freq) Probably timer_get_rate() is better - we yes it has a clock but it is the clock rate that we are returning. Isn't the frequency always the same for a given timer? I think this should be stored in the uclass private data. The timer could do this in its probe function. Then this function can just return that value rather than needing a driver method: unsigned long timer_get_rate(struct udevice *dev) { struct uc_priv *priv = dev_get_uclass_pric(dev); return priv->clock_rate; } > +{ > + const struct dm_timer_ops *ops = device_get_ops(dev); > + > + if (!ops->get_clock) > + return -ENOSYS; > + > + return ops->get_clock(dev, freq); > +} > + > +UCLASS_DRIVER(timer) = { > + .id = UCLASS_TIMER, > + .name = "timer", > +}; > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h > index 2155265..ebecb5f 100644 > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -69,6 +69,9 @@ typedef struct global_data { > struct udevice *dm_root_f; /* Pre-relocation root instance */ > struct list_head uclass_root; /* Head of core tree */ > #endif > +#ifdef CONFIG_DM_TIMER > + struct udevice *dm_timer; /* Timer instance for Driver Model */ > +#endif > > const void *fdt_blob; /* Our device tree, NULL if none */ > void *new_fdt; /* Relocated FDT */ > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > index 1eeec74..aff34a4 100644 > --- a/include/dm/uclass-id.h > +++ b/include/dm/uclass-id.h > @@ -56,6 +56,7 @@ enum uclass_id { > UCLASS_SPI_GENERIC, /* Generic SPI flash target */ > UCLASS_SYSCON, /* System configuration device */ > UCLASS_THERMAL, /* Thermal sensor */ > + UCLASS_TIMER, /* Timer device */ > UCLASS_TPM, /* Trusted Platform Module TIS interface */ > UCLASS_USB, /* USB bus */ > UCLASS_USB_DEV_GENERIC, /* USB generic device */ > diff --git a/include/timer.h b/include/timer.h > new file mode 100644 > index 0000000..c18a195 > --- /dev/null > +++ b/include/timer.h > @@ -0,0 +1,52 @@ > +/* > + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef _DM_TIMER_H_ > +#define _DM_TIMER_H_ > + > +/* > + * Get the current timer count > + * > + * @dev: The Timer device > + * @count: pointer that returns the current timer count > + * @return: 0 if OK, -ve on error > + */ > +int timer_get_count(struct udevice *dev, unsigned long *count); > +/* > + * Get the timer input clock frequency > + * > + * @dev: The Timer device > + * @freq: pointer that returns the timer clock frequency > + * @return: 0 if OK, -ve on error > + */ > +int timer_get_clock(struct udevice *dev, unsigned long *freq); > + > +/* > + * struct dm_timer_ops - Driver model Timer operations > + * > + * The uclass interface is implemented by all Timer devices which use > + * driver model. > + */ > +struct dm_timer_ops { > + /* > + * Get the current timer count > + * > + * @dev: The Timer device > + * @count: pointer that returns the current timer count > + * @return: 0 if OK, -ve on error > + */ > + int (*get_count)(struct udevice *dev, unsigned long *count); > + /* > + * Get the timer input clock frequency > + * > + * @dev: The Timer device > + * @freq: pointer that returns the timer clock frequency > + * @return: 0 if OK, -ve on error > + */ > + int (*get_clock)(struct udevice *dev, unsigned long *freq); > +}; > + > +#endif /* _DM_TIMER_H_ */ > diff --git a/lib/time.c b/lib/time.c > index 477440d..ba063cf 100644 > --- a/lib/time.c > +++ b/lib/time.c > @@ -6,6 +6,9 @@ > */ > > #include <common.h> > +#include <dm.h> > +#include <errno.h> > +#include <timer.h> > #include <watchdog.h> > #include <div64.h> > #include <asm/io.h> > @@ -37,6 +40,57 @@ unsigned long notrace timer_read_counter(void) > extern unsigned long __weak timer_read_counter(void); > #endif > > +#ifdef CONFIG_DM_TIMER > +static int notrace dm_timer_init(void) > +{ > + struct udevice *dev; > + int ret; > + > + if (!gd->dm_timer) { > + ret = uclass_first_device(UCLASS_TIMER, &dev); > + if (ret) > + return ret; > + if (!dev) > + return -ENODEV; > + gd->dm_timer = dev; > + } > + > + return 0; > +} > + > +ulong notrace get_tbclk(void) > +{ > + unsigned long freq; > + int ret; > + > + ret = dm_timer_init(); > + if (ret) > + return ret; > + > + ret = timer_get_clock(gd->dm_timer, &freq); > + if (ret) > + return ret; > + > + return freq; > +} > + > +unsigned long notrace timer_read_counter(void) > +{ > + unsigned long count; > + int ret; > + > + ret = dm_timer_init(); > + if (ret) > + return ret; > + > + ret = timer_get_count(gd->dm_timer, &count); > + if (ret) > + return ret; > + > + return count; > +} > +#endif /* CONFIG_DM_TIMER */ > + > uint64_t __weak notrace get_ticks(void) > { > unsigned long now = timer_read_counter(); > -- > 2.1.4 > Regards, Simon
Hi Simon, On 10/06/2015 10:14 PM, Simon Glass wrote: > Hi Thomas, > > On 4 October 2015 at 14:56, Thomas Chou <thomas@wytron.com.tw> wrote: >> Implement a Timer uclass to work with lib/time.c. >> >> Signed-off-by: Thomas Chou <thomas@wytron.com.tw> >> Reviewed-by: Simon Glass <sjg@chromium.org> > > Sorry, after a lot of consideration I'd like to retract that :-( > > Please see below. No worries. Your nits are welcome. > Also can you add a patch with a sandbox timer driver and a test (in > test/dm) for the timer? I didn't try sandbox before. I will learn sandbox and test. Then submit a patch for a sandbox timer driver. Best regards, Thomas
diff --git a/common/board_r.c b/common/board_r.c index f8c1baa..aaf390e 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -290,6 +290,9 @@ static int initr_dm(void) /* Save the pre-reloc driver model and start a new one */ gd->dm_root_f = gd->dm_root; gd->dm_root = NULL; +#ifdef CONFIG_DM_TIMER + gd->dm_timer = NULL; +#endif return dm_init_and_scan(false); } #endif diff --git a/drivers/Kconfig b/drivers/Kconfig index 63c92c5..f9496f7 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -56,6 +56,8 @@ source "drivers/spi/Kconfig" source "drivers/thermal/Kconfig" +source "drivers/timer/Kconfig" + source "drivers/tpm/Kconfig" source "drivers/usb/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index 9d0a595..692da78 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -48,6 +48,7 @@ obj-y += pcmcia/ obj-y += dfu/ obj-y += rtc/ obj-y += sound/ +obj-y += timer/ obj-y += tpm/ obj-y += twserial/ obj-y += video/ diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig new file mode 100644 index 0000000..54a4c40 --- /dev/null +++ b/drivers/timer/Kconfig @@ -0,0 +1,12 @@ +menu "Timer Support" + +config DM_TIMER + bool "Enable Driver Model for Timer drivers" + depends on DM + help + Enable driver model for Timer access. It uses the same API as + lib/time.c. But now implemented by the uclass. The first timer + will be used. The timer is usually a 32 bits free-running up + counter. There may be no real tick, and no timer interrupt. + +endmenu diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile new file mode 100644 index 0000000..a24179a --- /dev/null +++ b/drivers/timer/Makefile @@ -0,0 +1,7 @@ +# +# Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw> +# +# SPDX-License-Identifier: GPL-2.0+ +# + +obj-$(CONFIG_DM_TIMER) += timer-uclass.o diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c new file mode 100644 index 0000000..bcf1dde --- /dev/null +++ b/drivers/timer/timer-uclass.c @@ -0,0 +1,44 @@ +/* + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <timer.h> + +/* + * Implement a Timer uclass to work with lib/time.c. The timer is usually + * a 32 bits free-running up counter. The get_clock() method is used to get + * the input clock frequency of the timer. The get_count() method is used + * get the current 32 bits count value. If the hardware is counting down, + * the value should be inversed inside the method. There may be no real + * tick, and no timer interrupt. + */ + +int timer_get_count(struct udevice *dev, unsigned long *count) +{ + const struct dm_timer_ops *ops = device_get_ops(dev); + + if (!ops->get_count) + return -ENOSYS; + + return ops->get_count(dev, count); +} + +int timer_get_clock(struct udevice *dev, unsigned long *freq) +{ + const struct dm_timer_ops *ops = device_get_ops(dev); + + if (!ops->get_clock) + return -ENOSYS; + + return ops->get_clock(dev, freq); +} + +UCLASS_DRIVER(timer) = { + .id = UCLASS_TIMER, + .name = "timer", +}; diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 2155265..ebecb5f 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -69,6 +69,9 @@ typedef struct global_data { struct udevice *dm_root_f; /* Pre-relocation root instance */ struct list_head uclass_root; /* Head of core tree */ #endif +#ifdef CONFIG_DM_TIMER + struct udevice *dm_timer; /* Timer instance for Driver Model */ +#endif const void *fdt_blob; /* Our device tree, NULL if none */ void *new_fdt; /* Relocated FDT */ diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 1eeec74..aff34a4 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -56,6 +56,7 @@ enum uclass_id { UCLASS_SPI_GENERIC, /* Generic SPI flash target */ UCLASS_SYSCON, /* System configuration device */ UCLASS_THERMAL, /* Thermal sensor */ + UCLASS_TIMER, /* Timer device */ UCLASS_TPM, /* Trusted Platform Module TIS interface */ UCLASS_USB, /* USB bus */ UCLASS_USB_DEV_GENERIC, /* USB generic device */ diff --git a/include/timer.h b/include/timer.h new file mode 100644 index 0000000..c18a195 --- /dev/null +++ b/include/timer.h @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _DM_TIMER_H_ +#define _DM_TIMER_H_ + +/* + * Get the current timer count + * + * @dev: The Timer device + * @count: pointer that returns the current timer count + * @return: 0 if OK, -ve on error + */ +int timer_get_count(struct udevice *dev, unsigned long *count); +/* + * Get the timer input clock frequency + * + * @dev: The Timer device + * @freq: pointer that returns the timer clock frequency + * @return: 0 if OK, -ve on error + */ +int timer_get_clock(struct udevice *dev, unsigned long *freq); + +/* + * struct dm_timer_ops - Driver model Timer operations + * + * The uclass interface is implemented by all Timer devices which use + * driver model. + */ +struct dm_timer_ops { + /* + * Get the current timer count + * + * @dev: The Timer device + * @count: pointer that returns the current timer count + * @return: 0 if OK, -ve on error + */ + int (*get_count)(struct udevice *dev, unsigned long *count); + /* + * Get the timer input clock frequency + * + * @dev: The Timer device + * @freq: pointer that returns the timer clock frequency + * @return: 0 if OK, -ve on error + */ + int (*get_clock)(struct udevice *dev, unsigned long *freq); +}; + +#endif /* _DM_TIMER_H_ */ diff --git a/lib/time.c b/lib/time.c index 477440d..ba063cf 100644 --- a/lib/time.c +++ b/lib/time.c @@ -6,6 +6,9 @@ */ #include <common.h> +#include <dm.h> +#include <errno.h> +#include <timer.h> #include <watchdog.h> #include <div64.h> #include <asm/io.h> @@ -37,6 +40,57 @@ unsigned long notrace timer_read_counter(void) extern unsigned long __weak timer_read_counter(void); #endif +#ifdef CONFIG_DM_TIMER +static int notrace dm_timer_init(void) +{ + struct udevice *dev; + int ret; + + if (!gd->dm_timer) { + ret = uclass_first_device(UCLASS_TIMER, &dev); + if (ret) + return ret; + if (!dev) + return -ENODEV; + gd->dm_timer = dev; + } + + return 0; +} + +ulong notrace get_tbclk(void) +{ + unsigned long freq; + int ret; + + ret = dm_timer_init(); + if (ret) + return ret; + + ret = timer_get_clock(gd->dm_timer, &freq); + if (ret) + return ret; + + return freq; +} + +unsigned long notrace timer_read_counter(void) +{ + unsigned long count; + int ret; + + ret = dm_timer_init(); + if (ret) + return ret; + + ret = timer_get_count(gd->dm_timer, &count); + if (ret) + return ret; + + return count; +} +#endif /* CONFIG_DM_TIMER */ + uint64_t __weak notrace get_ticks(void) { unsigned long now = timer_read_counter();