[U-Boot,v2,06/10] timer: Add MPC83xx timer driver

Add a timer driver for the MPC83xx architecture.

Signed-off-by: Mario Six <mario.six@gdsys.cc>

---

v1 -> v2:
* Removed now-superfluous comments
* Removed usage of uclass_{first,next}_device_compat
* Switched to usage of new board uclass (instead of devinfo)

---
 arch/powerpc/cpu/mpc83xx/cpu.c |   4 +-
 arch/powerpc/lib/Makefile      |   4 ++
 arch/powerpc/lib/interrupts.c  |   5 +-
 drivers/timer/Kconfig          |   7 ++
 drivers/timer/Makefile         |   1 +
 drivers/timer/mpc83xx_timer.c  | 160 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 178 insertions(+), 3 deletions(-)
 create mode 100644 drivers/timer/mpc83xx_timer.c

--
2.16.1

Hi Mario,

On 27 April 2018 at 06:52, Mario Six <mario.six@gdsys.cc> wrote:
> Add a timer driver for the MPC83xx architecture.
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>
> ---
>
> v1 -> v2:
> * Removed now-superfluous comments
> * Removed usage of uclass_{first,next}_device_compat
> * Switched to usage of new board uclass (instead of devinfo)
>
> ---
>  arch/powerpc/cpu/mpc83xx/cpu.c |   4 +-
>  arch/powerpc/lib/Makefile      |   4 ++
>  arch/powerpc/lib/interrupts.c  |   5 +-
>  drivers/timer/Kconfig          |   7 ++
>  drivers/timer/Makefile         |   1 +
>  drivers/timer/mpc83xx_timer.c  | 160 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 178 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/timer/mpc83xx_timer.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below.

>
> diff --git a/arch/powerpc/cpu/mpc83xx/cpu.c b/arch/powerpc/cpu/mpc83xx/cpu.c
> index 9a5c1b7d55..376483eda7 100644
> --- a/arch/powerpc/cpu/mpc83xx/cpu.c
> +++ b/arch/powerpc/cpu/mpc83xx/cpu.c
> @@ -176,12 +176,12 @@ do_reset (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  /*
>   * Get timebase clock frequency (like cpu_clk in Hz)
>   */
> -
> +#ifndef CONFIG_TIMER
>  unsigned long get_tbclk(void)
>  {
>         return (gd->bus_clk + 3L) / 4L;
>  }
> -
> +#endif
>
>  #if defined(CONFIG_WATCHDOG)
>  void watchdog_reset (void)
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index 9a3043abf8..537693e3bc 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -19,13 +19,17 @@ endif
>
>  ifdef MINIMAL
>  obj-y += cache.o time.o
> +ifndef CONFIG_TIMER
>  obj-y += ticks.o
> +endif
>  else
>
>  obj-y  += ppcstring.o
>
>  obj-y  += ppccache.o
> +ifndef CONFIG_TIMER
>  obj-y  += ticks.o
> +endif
>  obj-y  += reloc.o
>
>  obj-$(CONFIG_BAT_RW) += bat_rw.o
> diff --git a/arch/powerpc/lib/interrupts.c b/arch/powerpc/lib/interrupts.c
> index e8784aa16e..56a697d28c 100644
> --- a/arch/powerpc/lib/interrupts.c
> +++ b/arch/powerpc/lib/interrupts.c
> @@ -15,6 +15,7 @@
>  #include <status_led.h>
>  #endif
>
> +#ifndef CONFIG_MPC83XX_TIMER
>  #ifdef CONFIG_SHOW_ACTIVITY
>  void board_show_activity (ulong) __attribute__((weak, alias("__board_show_activity")));
>
> @@ -45,7 +46,7 @@ static __inline__ void set_dec (unsigned long val)
>         if (val)
>                 asm volatile ("mtdec %0"::"r" (val));
>  }
> -
> +#endif /* !CONFIG_MPC83XX_TIMER */
>
>  void enable_interrupts (void)
>  {
> @@ -61,6 +62,7 @@ int disable_interrupts (void)
>         return ((msr & MSR_EE) != 0);
>  }
>
> +#ifndef CONFIG_MPC83XX_TIMER
>  int interrupt_init (void)
>  {
>         /* call cpu specific function from $(CPU)/interrupts.c */
> @@ -103,3 +105,4 @@ ulong get_timer (ulong base)
>  {
>         return (timestamp - base);
>  }
> +#endif /* !CONFIG_MPC83XX_TIMER */
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index 2c96896726..1b78ce784a 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -126,4 +126,11 @@ config STM32_TIMER
>           Select this to enable support for the timer found on
>           STM32 devices.
>
> +config MPC83XX_TIMER
> +        bool "MPC83xx timer support"
> +       depends on TIMER
> +       help
> +         Select this to enable support for the timer found on
> +         devices based on the MPC83xx family of SoCs.
> +
>  endmenu
> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> index a6e7832154..d35d235195 100644
> --- a/drivers/timer/Makefile
> +++ b/drivers/timer/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += atcpit100_timer.o
>  obj-$(CONFIG_ROCKCHIP_TIMER) += rockchip_timer.o
>  obj-$(CONFIG_ATMEL_PIT_TIMER) += atmel_pit_timer.o
>  obj-$(CONFIG_STM32_TIMER)      += stm32_timer.o
> +obj-$(CONFIG_MPC83XX_TIMER) += mpc83xx_timer.o
> diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
> new file mode 100644
> index 0000000000..c3dd306c5b
> --- /dev/null
> +++ b/drivers/timer/mpc83xx_timer.c
> @@ -0,0 +1,160 @@
> +/*
> + * (C) Copyright 2018
> + * Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <clk.h>
> +#include <timer.h>
> +#include <watchdog.h>
> +#include <board.h>
> +
> +struct mpc83xx_timer_priv {
> +};
> +
> +static uint decrementer_count; /* count value for 1e6/HZ microseconds */

Have a static var in BSS means it cannot be used before relocation, or
in SPL, right?

> +
> +static inline unsigned long get_dec(void)
> +{
> +       unsigned long val;
> +
> +       asm volatile ("mfdec %0":"=r" (val):);
> +
> +       return val;
> +}
> +
> +static inline void set_dec(unsigned long val)
> +{
> +       if (val)
> +               asm volatile ("mtdec %0"::"r" (val));
> +}
> +
> +/* TODO(mario.six@gdsys.cc): This should really be done by timer_init, and the
> + * interrupt init should go into a interrupt driver.
> + */
> +int interrupt_init(void)
> +{
> +       immap_t *immr = (immap_t *)CONFIG_SYS_IMMR;
> +       struct udevice *csb;
> +       struct udevice *board;
> +       struct clk clock;
> +
> +       if (get_board(&board))

debug() ?

> +               return -ENOENT;
> +
> +       uclass_get_device_by_phandle(UCLASS_SIMPLE_BUS, board, "csb", &csb);
> +       clk_get_by_index(csb, 0, &clock);

error checking

> +
> +       decrementer_count = (clk_get_rate(&clock) / 4) / CONFIG_SYS_HZ;
> +
> +       /* Enable e300 time base */
> +

Drop blank line

> +       setbits_be32(&immr->sysconf.spcr, 0x00400000);
> +
> +       set_dec(decrementer_count);
> +
> +       set_msr(get_msr() | MSR_EE);
> +
> +       return 0;
> +}
> +
> +static volatile ulong timestamp = 0;
> +
> +void timer_interrupt(struct pt_regs *regs)
> +{
> +       /* Restore Decrementer Count */
> +       set_dec(decrementer_count);
> +
> +       timestamp++;
> +
> +#if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG)
> +       if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
> +               WATCHDOG_RESET();
> +#endif    /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */
> +
> +#ifdef CONFIG_LED_STATUS
> +       status_led_tick(timestamp);
> +#endif /* CONFIG_LED_STATUS */
> +
> +#ifdef CONFIG_SHOW_ACTIVITY
> +       board_show_activity(timestamp);
> +#endif /* CONFIG_SHOW_ACTIVITY */
> +}
> +
> +ulong get_timer(ulong base)
> +{
> +       return (timestamp - base);
> +}
> +
> +static inline u32 mftbu(void)
> +{
> +       u32 rval;
> +
> +       asm volatile("mftbu %0" : "=r" (rval));
> +       return rval;
> +}
> +
> +static inline u32 mftb(void)
> +{
> +       u32 rval;
> +
> +       asm volatile("mftb %0" : "=r" (rval));
> +       return rval;
> +}
> +
> +void wait_ticks(ulong ticks)
> +{
> +       ulong end = get_ticks() + ticks;
> +
> +       while (end > get_ticks())
> +               WATCHDOG_RESET();
> +}
> +
> +static int mpc83xx_timer_get_count(struct udevice *dev, u64 *count)
> +{
> +       u32 tbu, tbl;
> +
> +       do {
> +               tbu = mftbu();
> +               tbl = mftb();
> +       } while (tbu != mftbu());
> +
> +       *count = (tbu * 0x10000ULL) + tbl;
> +
> +       return 0;
> +}
> +
> +static int mpc83xx_timer_probe(struct udevice *dev)
> +{
> +       struct timer_dev_priv *uc_priv = dev->uclass_priv;
> +       struct clk clock;
> +
> +       interrupt_init();
> +
> +       clk_get_by_index(dev, 0, &clock);
> +
> +       uc_priv->clock_rate = (clk_get_rate(&clock) + 3L) / 4L;
> +
> +       return 0;
> +}
> +
> +static const struct timer_ops mpc83xx_timer_ops = {
> +       .get_count = mpc83xx_timer_get_count,
> +};
> +
> +static const struct udevice_id mpc83xx_timer_ids[] = {
> +       { .compatible = "fsl,mpc83xx-timer" },
> +       { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(mpc83xx_timer) = {
> +       .name   = "mpc83xx_timer",
> +       .id     = UCLASS_TIMER,
> +       .of_match = mpc83xx_timer_ids,
> +       .probe = mpc83xx_timer_probe,
> +       .ops    = &mpc83xx_timer_ops,
> +       .flags = DM_FLAG_PRE_RELOC,
> +};
> --
> 2.16.1
>

Regards,
Simon

Hi Simon,

On Tue, May 1, 2018 at 1:13 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 27 April 2018 at 06:52, Mario Six <mario.six@gdsys.cc> wrote:
>> Add a timer driver for the MPC83xx architecture.
>>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>>
>> ---
>>
>> v1 -> v2:
>> * Removed now-superfluous comments
>> * Removed usage of uclass_{first,next}_device_compat
>> * Switched to usage of new board uclass (instead of devinfo)
>>
>> ---
>>  arch/powerpc/cpu/mpc83xx/cpu.c |   4 +-
>>  arch/powerpc/lib/Makefile      |   4 ++
>>  arch/powerpc/lib/interrupts.c  |   5 +-
>>  drivers/timer/Kconfig          |   7 ++
>>  drivers/timer/Makefile         |   1 +
>>  drivers/timer/mpc83xx_timer.c  | 160 +++++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 178 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/timer/mpc83xx_timer.c
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Please see below.
>
>>
>> diff --git a/arch/powerpc/cpu/mpc83xx/cpu.c b/arch/powerpc/cpu/mpc83xx/cpu.c
>> index 9a5c1b7d55..376483eda7 100644
>> --- a/arch/powerpc/cpu/mpc83xx/cpu.c
>> +++ b/arch/powerpc/cpu/mpc83xx/cpu.c
>> @@ -176,12 +176,12 @@ do_reset (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>>  /*
>>   * Get timebase clock frequency (like cpu_clk in Hz)
>>   */
>> -
>> +#ifndef CONFIG_TIMER
>>  unsigned long get_tbclk(void)
>>  {
>>         return (gd->bus_clk + 3L) / 4L;
>>  }
>> -
>> +#endif
>>
>>  #if defined(CONFIG_WATCHDOG)
>>  void watchdog_reset (void)
>> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
>> index 9a3043abf8..537693e3bc 100644
>> --- a/arch/powerpc/lib/Makefile
>> +++ b/arch/powerpc/lib/Makefile
>> @@ -19,13 +19,17 @@ endif
>>
>>  ifdef MINIMAL
>>  obj-y += cache.o time.o
>> +ifndef CONFIG_TIMER
>>  obj-y += ticks.o
>> +endif
>>  else
>>
>>  obj-y  += ppcstring.o
>>
>>  obj-y  += ppccache.o
>> +ifndef CONFIG_TIMER
>>  obj-y  += ticks.o
>> +endif
>>  obj-y  += reloc.o
>>
>>  obj-$(CONFIG_BAT_RW) += bat_rw.o
>> diff --git a/arch/powerpc/lib/interrupts.c b/arch/powerpc/lib/interrupts.c
>> index e8784aa16e..56a697d28c 100644
>> --- a/arch/powerpc/lib/interrupts.c
>> +++ b/arch/powerpc/lib/interrupts.c
>> @@ -15,6 +15,7 @@
>>  #include <status_led.h>
>>  #endif
>>
>> +#ifndef CONFIG_MPC83XX_TIMER
>>  #ifdef CONFIG_SHOW_ACTIVITY
>>  void board_show_activity (ulong) __attribute__((weak, alias("__board_show_activity")));
>>
>> @@ -45,7 +46,7 @@ static __inline__ void set_dec (unsigned long val)
>>         if (val)
>>                 asm volatile ("mtdec %0"::"r" (val));
>>  }
>> -
>> +#endif /* !CONFIG_MPC83XX_TIMER */
>>
>>  void enable_interrupts (void)
>>  {
>> @@ -61,6 +62,7 @@ int disable_interrupts (void)
>>         return ((msr & MSR_EE) != 0);
>>  }
>>
>> +#ifndef CONFIG_MPC83XX_TIMER
>>  int interrupt_init (void)
>>  {
>>         /* call cpu specific function from $(CPU)/interrupts.c */
>> @@ -103,3 +105,4 @@ ulong get_timer (ulong base)
>>  {
>>         return (timestamp - base);
>>  }
>> +#endif /* !CONFIG_MPC83XX_TIMER */
>> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
>> index 2c96896726..1b78ce784a 100644
>> --- a/drivers/timer/Kconfig
>> +++ b/drivers/timer/Kconfig
>> @@ -126,4 +126,11 @@ config STM32_TIMER
>>           Select this to enable support for the timer found on
>>           STM32 devices.
>>
>> +config MPC83XX_TIMER
>> +        bool "MPC83xx timer support"
>> +       depends on TIMER
>> +       help
>> +         Select this to enable support for the timer found on
>> +         devices based on the MPC83xx family of SoCs.
>> +
>>  endmenu
>> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
>> index a6e7832154..d35d235195 100644
>> --- a/drivers/timer/Makefile
>> +++ b/drivers/timer/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += atcpit100_timer.o
>>  obj-$(CONFIG_ROCKCHIP_TIMER) += rockchip_timer.o
>>  obj-$(CONFIG_ATMEL_PIT_TIMER) += atmel_pit_timer.o
>>  obj-$(CONFIG_STM32_TIMER)      += stm32_timer.o
>> +obj-$(CONFIG_MPC83XX_TIMER) += mpc83xx_timer.o
>> diff --git a/drivers/timer/mpc83xx_timer.c b/drivers/timer/mpc83xx_timer.c
>> new file mode 100644
>> index 0000000000..c3dd306c5b
>> --- /dev/null
>> +++ b/drivers/timer/mpc83xx_timer.c
>> @@ -0,0 +1,160 @@
>> +/*
>> + * (C) Copyright 2018
>> + * Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <clk.h>
>> +#include <timer.h>
>> +#include <watchdog.h>
>> +#include <board.h>
>> +
>> +struct mpc83xx_timer_priv {
>> +};
>> +
>> +static uint decrementer_count; /* count value for 1e6/HZ microseconds */
>
> Have a static var in BSS means it cannot be used before relocation, or
> in SPL, right?
>

True; same with the timestamp variable a bit below. I copied that part verbatim
from arch/powerpc/lib/interrupts.c, so that's a general problem with the
powerpc arch. I see that x86 keeps, e.g. the clock rate in the global data. I
think this might be a way around this (two ulongs will hopefully still fit in
the GD).

>> +
>> +static inline unsigned long get_dec(void)
>> +{
>> +       unsigned long val;
>> +
>> +       asm volatile ("mfdec %0":"=r" (val):);
>> +
>> +       return val;
>> +}
>> +
>> +static inline void set_dec(unsigned long val)
>> +{
>> +       if (val)
>> +               asm volatile ("mtdec %0"::"r" (val));
>> +}
>> +
>> +/* TODO(mario.six@gdsys.cc): This should really be done by timer_init, and the
>> + * interrupt init should go into a interrupt driver.
>> + */
>> +int interrupt_init(void)
>> +{
>> +       immap_t *immr = (immap_t *)CONFIG_SYS_IMMR;
>> +       struct udevice *csb;
>> +       struct udevice *board;
>> +       struct clk clock;
>> +
>> +       if (get_board(&board))
>
> debug() ?
>

Indeed; will be fixed in v3.

>> +               return -ENOENT;
>> +
>> +       uclass_get_device_by_phandle(UCLASS_SIMPLE_BUS, board, "csb", &csb);
>> +       clk_get_by_index(csb, 0, &clock);
>
> error checking
>

Will be fixed in v3.

>> +
>> +       decrementer_count = (clk_get_rate(&clock) / 4) / CONFIG_SYS_HZ;
>> +
>> +       /* Enable e300 time base */
>> +
>
> Drop blank line
>

Will be fixed in v3.

>> +       setbits_be32(&immr->sysconf.spcr, 0x00400000);
>> +
>> +       set_dec(decrementer_count);
>> +
>> +       set_msr(get_msr() | MSR_EE);
>> +
>> +       return 0;
>> +}
>> +
>> +static volatile ulong timestamp = 0;
>> +
>> +void timer_interrupt(struct pt_regs *regs)
>> +{
>> +       /* Restore Decrementer Count */
>> +       set_dec(decrementer_count);
>> +
>> +       timestamp++;
>> +
>> +#if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG)
>> +       if ((timestamp % (CONFIG_SYS_WATCHDOG_FREQ)) == 0)
>> +               WATCHDOG_RESET();
>> +#endif    /* CONFIG_WATCHDOG || CONFIG_HW_WATCHDOG */
>> +
>> +#ifdef CONFIG_LED_STATUS
>> +       status_led_tick(timestamp);
>> +#endif /* CONFIG_LED_STATUS */
>> +
>> +#ifdef CONFIG_SHOW_ACTIVITY
>> +       board_show_activity(timestamp);
>> +#endif /* CONFIG_SHOW_ACTIVITY */
>> +}
>> +
>> +ulong get_timer(ulong base)
>> +{
>> +       return (timestamp - base);
>> +}
>> +
>> +static inline u32 mftbu(void)
>> +{
>> +       u32 rval;
>> +
>> +       asm volatile("mftbu %0" : "=r" (rval));
>> +       return rval;
>> +}
>> +
>> +static inline u32 mftb(void)
>> +{
>> +       u32 rval;
>> +
>> +       asm volatile("mftb %0" : "=r" (rval));
>> +       return rval;
>> +}
>> +
>> +void wait_ticks(ulong ticks)
>> +{
>> +       ulong end = get_ticks() + ticks;
>> +
>> +       while (end > get_ticks())
>> +               WATCHDOG_RESET();
>> +}
>> +
>> +static int mpc83xx_timer_get_count(struct udevice *dev, u64 *count)
>> +{
>> +       u32 tbu, tbl;
>> +
>> +       do {
>> +               tbu = mftbu();
>> +               tbl = mftb();
>> +       } while (tbu != mftbu());
>> +
>> +       *count = (tbu * 0x10000ULL) + tbl;
>> +
>> +       return 0;
>> +}
>> +
>> +static int mpc83xx_timer_probe(struct udevice *dev)
>> +{
>> +       struct timer_dev_priv *uc_priv = dev->uclass_priv;
>> +       struct clk clock;
>> +
>> +       interrupt_init();
>> +
>> +       clk_get_by_index(dev, 0, &clock);
>> +
>> +       uc_priv->clock_rate = (clk_get_rate(&clock) + 3L) / 4L;
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct timer_ops mpc83xx_timer_ops = {
>> +       .get_count = mpc83xx_timer_get_count,
>> +};
>> +
>> +static const struct udevice_id mpc83xx_timer_ids[] = {
>> +       { .compatible = "fsl,mpc83xx-timer" },
>> +       { /* sentinel */ }
>> +};
>> +
>> +U_BOOT_DRIVER(mpc83xx_timer) = {
>> +       .name   = "mpc83xx_timer",
>> +       .id     = UCLASS_TIMER,
>> +       .of_match = mpc83xx_timer_ids,
>> +       .probe = mpc83xx_timer_probe,
>> +       .ops    = &mpc83xx_timer_ops,
>> +       .flags = DM_FLAG_PRE_RELOC,
>> +};
>> --
>> 2.16.1
>>
>
> Regards,
> Simon

Best regards,
Mario