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

[Jason]

On Thu, Sep 15, 2011 at 12:23:52PM -0700, Simon Glass wrote:
> 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?

Sure.  For larger drivers, I'd prefer to see a separate file.  eg:

mvgbe.c
mvgbe-dt.c

with mvgbe.h selecting _init() or _init_fdt() on CONFIG_OF_CONTROL.  Not
sure, gotta think about it more.
 
> > +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.

Yes, it's a stupid/simple driver.  There is definitely room for
optimization.

> >        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.

true.

> > +#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).

That's on the list of words that scare me.  ;-)  Right up there with
perfect, minimized, maximized, secure, and user-friendly.

> However, perhaps we should discuss this separately.

Definitely.

thx,

Jason.