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

Wolfgang Denk
2013-12-26T08:38:10Z

Dear Chin Liang See,

PLease fix your address list. There is no such address as
"Andy at denx.de".

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.

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.

Please check all your device accesses.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
That was the thing about deserts. They had their own gravity. They
sucked you into the centre. - Terry Pratchett, _Small Gods_