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

[Chin Liang See <clsee@altera.com>]

Dear Jaehoon,

On Thu, 2013-12-26 at 14:05 +0900, Jaehoon Chung wrote:
> Hi, Chin.
> 
> On 12/19/2013 02:16 AM, Chin Liang See wrote:
> > 
> > +
> > +#define CLKMGR_PERPLLGRP_EN_REG		(SOCFPGA_CLKMGR_ADDRESS + 0xA0)
> > +#define CLKMGR_SDMMC_CLK_ENABLE		(1 << 8)
> > +#define SYSMGR_SDMMCGRP_CTRL_REG	(SOCFPGA_SYSMGR_ADDRESS + 0x108)
> Where is SOCFPGA_CLKMGR_ADDRESS defined?

This is located at platform specific declaration file. 

> 
> > +#define SYSMGR_SDMMC_CTRL_GET_DRVSEL(x)	(((x) >> 0) & 0x7)
> ((x) & 0x7) is more readable?
> > +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)	\
> > +	((((drvsel) << 0) & 0x7) | (((smplsel) << 3) & 0x38))
> > +
> > +static char *SOCFPGA_NAME = "SOCFPGA DWMMC";
> > +
> > +static void socfpga_dwmci_clksel(struct dwmci_host *host)
> > +{
> > +	unsigned int en;
> > +	unsigned int drvsel;
> > +	unsigned int smplsel;
> > +
> > +	/* Disable SDMMC clock. */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en &= ~CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> > +
> > +	/* Configures drv_sel and smpl_sel */
> > +	drvsel = 3;
> > +	smplsel = 0;
> Is this value static? then why is value assigned drvsel and smpsel at here?
> I didn't know that SOCFPGA is only used with drvsel = 3, smplsel = 0.
> But if you need to change this value for other SoC version in future, I think that hard coding is not good.

Good suggestion as I put them as macro now for v3

> 
> > +
> > +	debug("%s: drvsel %d smplsel %d\n", __FUNCTION__, drvsel, smplsel);
> > +	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
> > +		SYSMGR_SDMMCGRP_CTRL_REG);
> > +
> > +	debug("%s: SYSMGR_SDMMCGRP_CTRL_REG = 0x%x\n", __FUNCTION__,
> > +		readl(SYSMGR_SDMMCGRP_CTRL_REG));
> > +	/* Enable SDMMC clock */
> > +	en = readl(CLKMGR_PERPLLGRP_EN_REG);
> > +	en |= CLKMGR_SDMMC_CLK_ENABLE;
> > +	writel(en, CLKMGR_PERPLLGRP_EN_REG);
> > +}
> > +
> > +int socfpga_dwmmc_init(u32 regbase, int bus_width, int index)
> > +{
> > +	struct dwmci_host *host = NULL;
> > +	host = calloc(sizeof(struct dwmci_host), 1);
> > +	if (!host) {
> > +		printf("dwmci_host calloc fail!\n");
> > +		return 1;
> > +	}
> > +
> > +	host->name = SOCFPGA_NAME;
> > +	host->ioaddr = (void *)regbase;
> > +	host->buswidth = bus_width;
> > +	host->clksel = socfpga_dwmci_clksel;
> > +	host->dev_index = index;
> > +	/* fixed clock divide by 4 which due to the SDMMC wrapper */
> > +	host->bus_hz = CONFIG_DWMMC_BUS_HZ;
> I didn't want to use the CONFIG_DWMMC_BUS_HZ.

Yup, this is SOCFPGA specific and I am using CONFIG_SOCFPGA_DWMMC_BUS_HZ
for v3


Thanks

Chin Liang

> 
> > +	host->fifoth_val = MSIZE(0x2) |
> > +		RX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2 - 1) |
> > +		TX_WMARK(CONFIG_DWMMC_FIFO_DEPTH / 2);
> > +
> > +	add_dwmci(host, host->bus_hz, 400000);
> add_dwmci() has the return value.
> 
> Best Regards,
> Jaehoon Chung
> > +
> > +	return 0;
> > +}
> > +
> > 
> 
>