From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Babic <sbabic@denx.de>
Date: Sun, 05 Aug 2012 09:13:29 +0200
Subject: [U-Boot] [PATCH] rtc: imxdi: Initial support
In-Reply-To: <194408868.336913.1342794278862.JavaMail.root@advansee.com>
References: <194408868.336913.1342794278862.JavaMail.root@advansee.com>
Message-ID: <501E1D19.5040906@denx.de>
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 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.

> +/*
> + * 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 ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================