| Message ID | 6d40c2a90839e1f8dae389a562ea20ed2bd15083.1316092940.git.u-boot@lakedaemon.net |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Jason, On Thu, Sep 15, 2011 at 6:54 AM, Jason Cooper <u-boot@lakedaemon.net> wrote: > > Signed-off-by: Jason Cooper <u-boot@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 > >
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@lakedaemon.net> wrote: > > > > Signed-off-by: Jason Cooper <u-boot@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.
Dear Jason Cooper, In message <6d40c2a90839e1f8dae389a562ea20ed2bd15083.1316092940.git.u-boot@lakedaemon.net> you wrote: > > Signed-off-by: Jason Cooper <u-boot@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(-) Checkpatch says: total: 3 errors, 0 warnings, 118 lines checked Please clean up and resubmit. Thanks. Best regards, Wolfgang Denk
Hi Wolfgang, On Thu, Oct 6, 2011 at 2:31 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Jason Cooper, > > In message <6d40c2a90839e1f8dae389a562ea20ed2bd15083.1316092940.git.u-boot@lakedaemon.net> you wrote: >> >> Signed-off-by: Jason Cooper <u-boot@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(-) > FYI I plan to do a new version of the base fdt patch in the next few days which addresses comments, etc. Regards, Simon > > Checkpatch says: > > total: 3 errors, 0 warnings, 118 lines checked > > Please clean up and resubmit. Thanks. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > The Buddha, the Godhead, resides quite as comfortably in the circuits > of a digital computer or the gears of a cycle transmission as he does > at the top of a mountain or in the petals of a flower. > - R. Pirsig, "Zen and the Art of Motorcycle Maintenance" >
Hi Jason, On Thu, Oct 6, 2011 at 2:42 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Wolfgang, > > On Thu, Oct 6, 2011 at 2:31 PM, Wolfgang Denk <wd@denx.de> wrote: >> Dear Jason Cooper, >> >> In message <6d40c2a90839e1f8dae389a562ea20ed2bd15083.1316092940.git.u-boot@lakedaemon.net> you wrote: >>> >>> Signed-off-by: Jason Cooper <u-boot@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(-) >> > > FYI I plan to do a new version of the base fdt patch in the next few > days which addresses comments, etc. > I have done this now - if you have time please rebase and try again. Regards, Simon >> >> Checkpatch says: >> >> total: 3 errors, 0 warnings, 118 lines checked >> >> Please clean up and resubmit. Thanks. >> >> Best regards, >> >> Wolfgang Denk >> >> -- >> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de >> The Buddha, the Godhead, resides quite as comfortably in the circuits >> of a digital computer or the gears of a cycle transmission as he does >> at the top of a mountain or in the petals of a flower. >> - R. Pirsig, "Zen and the Art of Motorcycle Maintenance" >> >
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 +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 */ 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 */ +}; +#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, COMPAT_COUNT, };
Signed-off-by: Jason Cooper <u-boot@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(-)