Hi Thomas,
On 29 September 2015 at 06:39, Thomas Chou <tho...@wytron.com.tw> wrote:
> Implement a Timer uclass to work with lib/time.c.
>
> Signed-off-by: Thomas Chou <tho...@wytron.com.tw>
Thanks for doing this work! I have some comments below.
> ---
> v2
> fix coding style.
>
> common/board_r.c | 5 +++--
> drivers/Kconfig | 2 ++
> drivers/Makefile | 1 +
> drivers/timer/Kconfig | 9 ++++++++
> drivers/timer/Makefile | 7 ++++++
> drivers/timer/timer-uclass.c | 35 ++++++++++++++++++++++++++++++
> drivers/timer/timer.c | 51
> ++++++++++++++++++++++++++++++++++++++++++++
> include/dm/uclass-id.h | 1 +
> include/timer.h | 36 +++++++++++++++++++++++++++++++
> 9 files changed, 145 insertions(+), 2 deletions(-)
> create mode 100644 drivers/timer/Kconfig
> create mode 100644 drivers/timer/Makefile
> create mode 100644 drivers/timer/timer-uclass.c
> create mode 100644 drivers/timer/timer.c
> create mode 100644 include/timer.h
>
> diff --git a/common/board_r.c b/common/board_r.c
> index f8c1baa..64b0577 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -828,8 +828,9 @@ init_fnc_t init_sequence_r[] = {
> #if defined(CONFIG_ARM) || defined(CONFIG_AVR32)
> initr_enable_interrupts,
> #endif
> -#if defined(CONFIG_X86) || defined(CONFIG_MICROBLAZE) ||
> defined(CONFIG_AVR32) \
> - || defined(CONFIG_M68K)
> +#if defined(CONFIG_X86) || defined(CONFIG_MICROBLAZE) || \
> + defined(CONFIG_AVR32) || defined(CONFIG_M68K) || \
> + defined(CONFIG_DM_TIMER)
> timer_init, /* initialize timer */
> #endif
> #if defined(CONFIG_STATUS_LED) && defined(STATUS_LED_BOOT)
> 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..97014f3
> --- /dev/null
> +++ b/drivers/timer/Kconfig
> @@ -0,0 +1,9 @@
> +menu "Timer Support"
> +
> +config DM_TIMER
> + bool "Enable Driver Model for Timer drivers"
> + depends on DM
> + help
> + Enable driver model for Timer access.
Can you expand this a little bit?
- Uses the same API
- But now implemented by the uclass
- The first timer is used
> +
> +endmenu
> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> new file mode 100644
> index 0000000..58acd7c
> --- /dev/null
> +++ b/drivers/timer/Makefile
> @@ -0,0 +1,7 @@
> +#
> +# Copyright (C) 2015 Thomas Chou <tho...@wytron.com.tw>
> +#
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +
> +obj-$(CONFIG_DM_TIMER) += timer-uclass.o timer.o
> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
> new file mode 100644
> index 0000000..1f088bf
> --- /dev/null
> +++ b/drivers/timer/timer-uclass.c
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2015 Thomas Chou <tho...@wytron.com.tw>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <timer.h>
> +
> +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_rate(struct udevice *dev, unsigned long *rate)
> +{
> + const struct dm_timer_ops *ops = device_get_ops(dev);
> +
> + if (!ops->get_rate)
> + return -ENOSYS;
> +
> + return ops->get_rate(dev, rate);
> +}
> +
> +UCLASS_DRIVER(timer) = {
> + .id = UCLASS_TIMER,
> + .name = "timer",
> +};
> diff --git a/drivers/timer/timer.c b/drivers/timer/timer.c
> new file mode 100644
> index 0000000..766eabf
> --- /dev/null
> +++ b/drivers/timer/timer.c
I think these functions should go in lib/time.c.
At some point we should look at implementing udelay() also.
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (C) 2015 Thomas Chou <tho...@wytron.com.tw>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <timer.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +unsigned long notrace get_tbclk(void)
> +{
> + struct udevice *dev;
> + unsigned long rate;
> +
> + uclass_first_device(UCLASS_TIMER, &dev);
> + if (!dev)
> + return -ENODEV;
> +
> + timer_get_rate(dev, &rate);
> +
> + return rate;
> +}
> +
> +unsigned long notrace timer_read_counter(void)
> +{
> + struct udevice *dev;
> + unsigned long count;
> +
> + uclass_first_device(UCLASS_TIMER, &dev);
> + if (!dev)
> + return -ENODEV;
I suggest saving this device in global_data. This might get called a lot.
> +
> + timer_get_count(dev, &count);
> +
> + return count;
> +}
> +
> +int timer_init(void)
I suspect this function is not needed. We should try to avoid this
sort of thing with driver model - devices should be able to be inited
when needed. Granted the timer is a very basic device, but it should
follow the same model if possible.
> +{
> + struct udevice *dev;
> +
> + uclass_first_device(UCLASS_TIMER, &dev);
> + if (!dev)
> + return -ENODEV;
> +
> + return 0;
> +}
> 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..5d71612
> --- /dev/null
> +++ b/include/timer.h
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (C) 2015 Thomas Chou <tho...@wytron.com.tw>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef _DM_TIMER_H_
> +#define _DM_TIMER_H_
> +
> +int timer_get_count(struct udevice *dev, unsigned long *count);
We should also handle microsecond time in this API. How about:
timer_get_ms()
timer_get_us()
> +int timer_get_rate(struct udevice *dev, unsigned long *rate);
Function comments.
> +
> +/*
> + * 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 currnet timer count
current
Also don't forget @return
> + */
> + int (*get_count)(struct udevice *dev, unsigned long *count);
> + /*
> + * Get the timer clock rate
> + *
> + * @dev: The Timer device
> + * @rate: pointer that returns the timer clock rate
in Hz. Also, isn't the clock rate required to be 1000 now?
> + */
> + int (*get_rate)(struct udevice *dev, unsigned long *rate);
> +};
> +
> +#endif /* _DM_TIMER_H_ */
> --
> 2.1.4
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
