From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chin Liang See <clsee@altera.com>
Date: Mon, 30 Dec 2013 18:25:34 -0600
Subject: [U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver
 support for SOCFPGA
In-Reply-To: <20131226083810.C08C03824CD@gemini.denx.de>
References: <1387386987-3581-1-git-send-email-clsee@altera.com>
	<20131226083810.C08C03824CD@gemini.denx.de>
Message-ID: <1388449534.6500.11.camel@clsee-VirtualBox.altera.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

Dear Wolfgang,

On Thu, 2013-12-26 at 09:38 +0100, ZY - wd wrote:
> Dear Chin Liang See,
> 
> PLease fix your address list.  There is no such address as
> "Andy at denx.de".

Oh... I presume you are referring to Andy Fleming. I am getting delivery
failure to afleming at freescale.com and removing it from CC list.

> 
> In message <1387386987-3581-1-git-send-email-clsee@altera.com> you wrote:
> > To add the DesignWare MMC driver support for Altera SOCFPGA. It
> > required information such as clocks and bus width from platform
> > specific files (SOCFPGA handoff files)
> ...
> > +#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
> ...
> > +#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)
> 
> This looks as if you were trying to access device rtegisters through a
> base address plus offset notation?
> 
> We do not allow this in U-Boot, as the compiler then has no chance to
> check if you are using the correct data types, i. e. it cannot warn
> you for example when you access a 32 bit register for a 16 bit data
> type.
> 

Noted and I believe I miss out this. I will be using structure for v3

> Please use C structs and proper I/O accessors instead.
> 
> > +	/* Disable SDMMC clock. */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> 
> Please fix such code.  Use proper I/O accessors.  This could (and
> should) be written as:
> 
> 	clrbits_le32(clkmgr->perpllgrp_en, CLKMGR_SDMMC_CLK_ENABLE);
> 
> [or similar struct member name].
> 
> > +	/* Enable SDMMC clock */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en |= CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> 
> Ditto here, etc.

Noted.

Thanks

Chin Liang

> 
> Please check all your device accesses.
> 
> Best regards,
> 
> Wolfgang Denk
>