From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= <benoit.thebaudeau@advansee.com>
Date: Sun, 5 Aug 2012 16:14:17 +0200 (CEST)
Subject: [U-Boot] [PATCH] rtc: imxdi: Initial support
In-Reply-To: <501E1D19.5040906@denx.de>
Message-ID: <1483827613.1452102.1344176057635.JavaMail.root@advansee.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Sun, Aug 5, 2012 at 09:13:29 AM, Stefano Babic wrote:
> On 20/07/2012 16:24, Beno?t Th?baudeau wrote:
> > Add support for Freescale's i.MX DryIce RTC, present on i.MX25.
> > 
> > Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> 
> Hi Benoit,
> 
> 
> just a few comments.
> 
> > +
> > +/*
> > + * Date & Time support for Freescale i.MX DryIce RTC
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <rtc.h>
> > +
> > +#if defined(CONFIG_CMD_DATE)
> > +
> > +#include <asm/io.h>
> > +#include <asm/arch/imx-regs.h>
> > +
> > +/* DryIce Register Definitions */
> > +
> > +#define DTCMR     0x00           /* Time Counter MSB Reg */
> > +#define DTCLR     0x04           /* Time Counter LSB Reg */
> > +
> > +#define DCAMR     0x08           /* Clock Alarm MSB Reg */
> > +#define DCALR     0x0c           /* Clock Alarm LSB Reg */
> 
> U-Boot as rule does not allow usage of start address + offset to
> access
> internal registers. Instead of that, please define a proper C
> structure
> with the register's layout. This should be fixed globally for all
> read /
> write accesses.

OK, I'll do that.

> > +/*
> > + * This function attempts to clear the dryice write-error flag.
> > + *
> > + * A dryice write error is similar to a bus fault and should not
> > occur in
> > + * normal operation.  Clearing the flag requires another write, so
> > the root
> > + * cause of the problem may need to be fixed before the flag can
> > be cleared.
> > + */
> > +static void clear_write_error(void)
> > +{
> > +	int cnt;
> > +
> > +	puts("### Warning: RTC - Register write error!\n");
> > +
> > +	/* clear the write error flag */
> > +	__raw_writel(DSR_WEF, IMX_DRYICE_BASE + DSR);
> 
> Ditto
> 
> > +int rtc_get(struct rtc_time *tmp)
> > +{
> > +	unsigned long now;
> > +	int rc;
> > +
> > +	rc = di_init();
> 
> Why is di_init always called ? Should be not checked if it is was
> already initialized and the function should be called only once ?

I think this can be done. I'll check that this does not cause any regression.
I think only the reset function should have to call di_init unconditionally.

Best regards,
Beno?t