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

Jason
2011-09-15T20:01:31Z

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.