[U-Boot] [PATCH v2] socfpga/dwmmc: Adding DesignWare MMC driver support for SOCFPGA

Chin Liang See
2013-12-31T00:25:34Z

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
>