[U-Boot] [RFC PATCH 3/4 v1] mvrtc: add fdt support.

[Simon Glass]

Hi Jason,

On Thu, Sep 15, 2011 at 6:54 AM, Jason Cooper <u-boot at lakedaemon.net> wrote:
>
> Signed-off-by: Jason Cooper <u-boot at lakedaemon.net>
> ---
>  common/fdt_decode.c  |    1 +
>  drivers/rtc/mvrtc.c  |   62 +++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/rtc/mvrtc.h  |    7 +++++
>  include/fdt_decode.h |    1 +
>  4 files changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/common/fdt_decode.c b/common/fdt_decode.c
> index 0f13089..1a0dcf4 100644
> --- a/common/fdt_decode.c
> +++ b/common/fdt_decode.c
> @@ -34,6 +34,7 @@
>  */
>  #define COMPAT(id, name) name
>  const char *compat_names[COMPAT_COUNT] = {
> +       COMPAT(MARVELL_KIRKWOOD_RTC, "marvell,kirkwood-rtc"),
>  };
>
>  /**
> diff --git a/drivers/rtc/mvrtc.c b/drivers/rtc/mvrtc.c
> index ccc573a..ce2dc3d 100644
> --- a/drivers/rtc/mvrtc.c
> +++ b/drivers/rtc/mvrtc.c
> @@ -28,18 +28,62 @@
>  #include <common.h>
>  #include <command.h>
>  #include <rtc.h>
> +#ifdef CONFIG_OF_CONTROL
> +#include <fdt_decode.h>
> +#endif
>  #include "mvrtc.h"
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  /* This RTC does not support century, so we assume 20 */
>  #define CENTURY 20
>
> +#ifndef CONFIG_OF_CONTROL

Perhaps #ifdef and put the fdt code first?

> +struct mvrtc_registers *mvrtc_get_config(void) {
> +       return (struct mvrtc_registers *)KW_RTC_BASE;
> +}
> +
> +#else
> +int fdt_decode_rtc(const void *blob, int node, struct fdt_rtc *config)
> +{
> +       config->reg = get_addr(blob, node, "reg");
> +       config->enabled = get_is_enabled(blob, node, 0);
> +
> +       return 0;
> +}
> +
> +struct mvrtc_registers *mvrtc_get_config(void) {
> +       const void     *blob = gd->blob;
> +       struct fdt_rtc config;
> +       int            node;
> +       int            index=0;
> +
> +       node = fdt_decode_next_alias(blob, "rtc",
> +                                    COMPAT_MARVELL_KIRKWOOD_RTC, &index);
> +
> +       if (node < 0)
> +               return NULL;
> +
> +       if (fdt_decode_rtc(blob, node, &config))
> +               return NULL;
> +
> +       return config.enabled ? (struct mvrtc_registers *)config.reg : NULL;
> +}
> +#endif
> +
>  int rtc_get(struct rtc_time *t)
>  {
>        u32 time;
>        u32 date;
>        struct mvrtc_registers *mvrtc_regs;
>
> -       mvrtc_regs = (struct mvrtc_registers *)KW_RTC_BASE;
> +       mvrtc_regs = mvrtc_get_config();
> +#ifdef CONFIG_OF_CONTROL
> +       if (mvrtc_regs == NULL) {
> +               printf("Error decoding fdt for mvrtc.\n");
> +               return -1;
> +       }
> +#endif
>
>        /* read the time register */
>        time = readl(&mvrtc_regs->time);
> @@ -79,7 +123,13 @@ int rtc_set(struct rtc_time *t)
>        u32 date = 0;
>        struct mvrtc_registers *mvrtc_regs;
>
> -       mvrtc_regs = (struct mvrtc_registers *)KW_RTC_BASE;
> +       mvrtc_regs = mvrtc_get_config();
> +#ifdef CONFIG_OF_CONTROL
> +       if (mvrtc_regs == NULL) {
> +               printf("Error decoding fdt for mvrtc.\n");
> +               return -1;
> +       }
> +#endif
>
>        /* check that this code isn't 80+ years old ;-) */
>        if ((t->tm_year / 100) != CENTURY)
> @@ -111,7 +161,13 @@ void rtc_reset(void)
>        u32 sec;
>        struct mvrtc_registers *mvrtc_regs;
>
> -       mvrtc_regs = (struct mvrtc_registers *)KW_RTC_BASE;
> +       mvrtc_regs = mvrtc_get_config();
> +#ifdef CONFIG_OF_CONTROL
> +       if (mvrtc_regs == NULL) {
> +               printf("Error decoding fdt for mvrtc.\n");
> +               return;
> +       }
> +#endif
>
>        /* no init routine for this RTC needed, just check that it's working */

Hmm I think it would be better to decode the fdt once in init, and
store it in your structure. It seems like you are doing it each time
the driver is called.

>        time = readl(&mvrtc_regs->time);
> diff --git a/drivers/rtc/mvrtc.h b/drivers/rtc/mvrtc.h
> index b9d5c6f..56b09f2 100644
> --- a/drivers/rtc/mvrtc.h
> +++ b/drivers/rtc/mvrtc.h
> @@ -37,6 +37,13 @@ struct mvrtc_registers {
>        u32 date;
>  };
>
> +#ifdef CONFIG_OF_CONTROL
> +struct fdt_rtc {
> +       addr_t reg;  /* address of the registers */
> +       int enabled; /* 1 if enabled, 0 if disabled */
> +};

It seems to be that this structure should generally only be needed in
the C file, so should perhaps go there.

> +#endif
> +
>  /* time register */
>  #define MVRTC_SEC_SFT          0
>  #define MVRTC_SEC_MSK          0x7f
> diff --git a/include/fdt_decode.h b/include/fdt_decode.h
> index 4264e3b..f236643 100644
> --- a/include/fdt_decode.h
> +++ b/include/fdt_decode.h
> @@ -54,6 +54,7 @@ struct fdt_memory {
>  */
>  enum fdt_compat_id {
>        COMPAT_UNKNOWN,
> +       COMPAT_MARVELL_KIRKWOOD_RTC,

My cunning plan is that this can be some sort of driver ID and could
serve as a lead in to a unified device model (it provides a simple
enumeration of available drivers). However, perhaps we should discuss
this separately.

Regards,
Simon

>        COMPAT_COUNT,
>  };
>
> --
> 1.7.0.4
>
>