[U-Boot,v2,1/2] dm: implement a Timer uclass

Implement a Timer uclass to work with lib/time.c.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
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

Hi Thomas,

On 29 September 2015 at 06:39, 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>

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

Hi Simon,

On 09/29/2015 09:57 PM, Simon Glass wrote:
>> +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()

Please allow the additional API be implemented at some later point. It 
might be better to keep the first patch simple as possible,

>> +       /*
>> +        * Get the timer clock rate
>> +        *
>> +        * @dev:        The Timer device
>> +        * @rate:       pointer that returns the timer clock rate
>> +        */
>> +       int (*get_rate)(struct udevice *dev, unsigned long *rate);
>
> in Hz. Also, isn't the clock rate required to be 1000 now?

Sorry for the confusion. It meant the timer input clock frequency. I 
rename it to get_clock() now. Will this be better?

Thanks a lot for your review.

Best regards,
Thomas Chou