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-b...@lakedaemon.net> wrote:
> >
> > Signed-off-by: Jason Cooper <u-b...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
