[U-Boot] [PATCH 3/4] Armada100: SD/MMC support for Marvell gplugD

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <1310106168-17166-3-git-send-email-ajay.bhargav at einfochips.com> you wrote:
> This patch provide support for MMC on GuruPlug-Display in uboot.

> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index a8fe17a..7f88031 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -38,6 +38,7 @@ COBJS-$(CONFIG_OMAP3_MMC) += omap3_mmc.o
>  COBJS-$(CONFIG_OMAP_HSMMC) += omap_hsmmc.o
>  COBJS-$(CONFIG_PXA_MMC) += pxa_mmc.o
>  COBJS-$(CONFIG_S5P_MMC) += s5p_mmc.o
> +COBJS-$(CONFIG_PXASDH) += pxa_sdh.o

Please keep list sorted.

> --- /dev/null
> +++ b/drivers/mmc/pxa_sdh.c
> @@ -0,0 +1,677 @@
> +/**************************************************************************
> + *
> + * Copyright (c) 2009, 2010 Marvell International Ltd.

Incorrect multi-line comment style. etc. etc.

...
> +#define CLKRT_OFF			(~0)
> +#define GET_REG(host, reg)		readw(host->regbase + reg)
> +#define SET_REG(host, val, reg)		writew(val, host->regbase + reg)
> +#define DATA_DIRECTION_READ(data)	(data->flags & MMC_DATA_READ)
> +#define SET_REG_BIT(host, bit_mask, reg) \
> +		SET_REG(host, \
> +			GET_REG(host, reg) | (bit_mask), reg)
> +#define CLEAR_REG_BIT(host, bit_mask, reg) \
> +		SET_REG(host, \
> +			GET_REG(host, reg) & ~(bit_mask), reg)
> +#define SET_REG_BITS(host, bits_pos, bits_mask, val, reg) \
> +		{SET_REG(host, \
> +			GET_REG(host, reg) & ~(bits_mask << bits_pos), reg); \
> +		SET_REG(host, \
> +			GET_REG(host, reg) | (val << bits_pos), reg); }
> +#define GET_REG_BITS(host, bit_pos, bits_mask, reg) \
> +		((GET_REG(host, reg) >> bit_pos) & bits_mask)

NAK.  Please don't invent your own accessors, instead use the existing
ones.  Also, make sure to use proper C structs instead of base address
+ offset notation.


> +static void pxa_sdh_finish_request(struct pxa_sdh_host *host)
> +{
> +
> +#ifdef CONFIG_MMC_DEBUG

Drop that blank line above (and all similar ones).

...
> +		do {
> +			ret = pxa_sdh_process_irq(host, (DMA_INT | XFER_COMP));
> +			/* If error or xfer completed (in which case
> +			 * bytes_xfered is reset to 0) break */

Incorrect multiline-comment ...

> +static int pxa_sdh_cmd_done(struct pxa_sdh_host *host)
> +{
> +	struct mmc_cmd *cmd = host->cmd;
> +	u32 resp[8];
> +
> +	BUG_ON(!cmd);
> +
> +	/* get cmd response */
> +	resp[0] = GET_REG(host, SD_RESP_0);
> +	resp[1] = GET_REG(host, SD_RESP_1);
> +	resp[2] = GET_REG(host, SD_RESP_2);
> +	resp[3] = GET_REG(host, SD_RESP_3);
> +	resp[4] = GET_REG(host, SD_RESP_4);
> +	resp[5] = GET_REG(host, SD_RESP_5);
> +	resp[6] = GET_REG(host, SD_RESP_6);
> +	resp[7] = readb(host->regbase + SD_RESP_7);
> +

NAK - see above, please use proper I/O accessors.

...
> +		char *src = (char *) data->src + host->bytes_xfered;
> +		for (i = 0; i < blk_size; i += sizeof(u32))
> +			writel(*(u32 *)(src +  i),
> +				host->regbase + SD_BUF_DPORT_0);

Braces needed for multiline statement.

> +	if (host->bytes_xfered < host->data_len) {
> +		host_dma_bdry_size = 0x1000 << (((u16) GET_REG(host,
> +				SD_BLOCK_SIZE)) >> HOST_DMA_BDRY_OFFSET);
> +		if (host_dma_bdry_size < data->blocksize)
> +			host->bytes_xfered = host->bytes_xfered +
> +							host_dma_bdry_size;

Braces needed for multiline statement.

> +		else
> +			host->bytes_xfered = host->bytes_xfered +
> +							data->blocksize;

Braces needed for multiline statement. Please fix globally.


> +	switch (host->port_num) {
> +	case 0:
> +		/* Setup the MMC/SD(1) Host Controller Clock */
> +		writel(0x19, 0xd4282854);
> +		udelay(10);
> +		writel(0x1B, 0xd4282854);
> +		break;
> +	case 1:
> +		/* Setup the MMC/SD(2) Host Controller Clock */
> +		writel(0x10, 0xd4282858);
> +		udelay(10);
> +		writel(0x12, 0xd4282858);
> +		break;
> +	case 2:
> +		/* Setup the MMC/SD(3) Host Controller Clock */
> +		writel(0x19, 0xd42828e0);
> +		udelay(10);
> +		writel(0x1b, 0xd42828e0);
> +		break;
> +	case 3:
> +		/* Setup the MMC/SD(4) Host Controller Clock */
> +		writel(0x10, 0xd42828e4);
> +		udelay(10);
> +		writel(0x12, 0xd42828e4);
> +		break;

Please provide symbolic constants / definitions for these (and
similar) magic numbers.

...
> --- /dev/null
> +++ b/drivers/mmc/pxa_sdh.h
> @@ -0,0 +1,241 @@
...
> +/* register definitions of PXA SD Host Controller*/
> +#define SD_SYS_ADDR_LOW		0x0000	/* DMA System Address Low */
> +#define SD_SYS_ADDR_HIGH	0x0002	/* DMA System Address High */
> +#define SD_BLOCK_SIZE		0x0004	/* Block Size*/
...

NAK.   Please use a C struct instead.

...
> +#define HOST_DMA_BDRY_MASK	((u16)0x7)
> +#define BLOCK_SIZE_OFFSET	0
> +#define BLOCK_SIZE_MASK		((u16)0x0fff)
> +#define BLOCK_SIZE_MAX		((u16)0x0800)

Please drop all these casts (and get rid of the parens).

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
I must follow the people.  Am I not their leader? - Benjamin Disraeli