From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Sat, 12 Nov 2011 17:35:46 +0100
Subject: [U-Boot] [PATCH 5/6] i.mx: fsl_esdhc: add the i.mx6q support
In-Reply-To: <1321094190-8108-6-git-send-email-jason.hui@linaro.org>
References: <1321094190-8108-1-git-send-email-jason.hui@linaro.org>
	<1321094190-8108-6-git-send-email-jason.hui@linaro.org>
Message-ID: <201111121735.46668.marek.vasut@gmail.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

> The mmc host controller on the i.mx6q is called usdhc which
> is redesigned based on the freescale esdhc controller.
> 
> The usdhc controller is almost compatible with esdhc except
> it adds one misc control register from user using experience.
> 
> Signed-off-by: Jason Liu <jason.hui@linaro.org>
> ---
>  drivers/mmc/fsl_esdhc.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index ec953f0..cd17ef2 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -58,7 +58,12 @@ struct fsl_esdhc {
>  	uint	autoc12err;
>  	uint	hostcapblt;
>  	uint	wml;
> -	char	reserved1[8];
> +#if defined(CONFIG_FSL_USDHC)
> +	uint    mixctrl;
> +	char    reserved1[4];
> +#else
> +	char    reserved1[8];
> +#endif

Hi Jason,

can't we just drop this ifdef ?

>  	uint	fevt;
>  	char	reserved2[168];
>  	uint	hostver;
> @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> struct mmc_data *data)
> 
>  	/* Send the command */
>  	esdhc_write32(&regs->cmdarg, cmd->cmdarg);
> +#if defined(CONFIG_FSL_USDHC)
> +	esdhc_write32(&regs->mixctrl, xfertyp & 0xFFFF);
> +#endif
>  	esdhc_write32(&regs->xfertyp, xfertyp);

Why is this duplicated? This seems like a huge user-experience nonsense to me :)

If you write to xfertyp register, you still have to write the same thing to 
mixctrl? Or if you do it vice versa, won't it work ? Why did you add the 
register?

> 
>  	/* Wait for the command to complete */
> @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct
> fsl_esdhc_cfg *cfg)
> 
>  	mmc = malloc(sizeof(struct mmc));
> 
> +#if defined(CONFIG_FSL_USDHC)
> +	sprintf(mmc->name, "FSL_USDHC");
> +#else
>  	sprintf(mmc->name, "FSL_ESDHC");
> +#endif

Why not just rename it to FSL_SDHC and be done with it ?

>  	regs = (struct fsl_esdhc *)cfg->esdhc_base;
> 
>  	/* First reset the eSDHC controller */