[U-Boot] [PATCH 7/9] fsl_esdhc: add support for mx51 processor

[Wolfgang Denk <wd@denx.de>]

Dear Stefano Babic,

In message <1263212760-27272-8-git-send-email-sbabic@denx.de> you wrote:
> The esdhc controller in the mx51 processor is quite
> the same as the one in some powerpc processors
> (MPC83xx, MPC85xx). This patches adapts the driver
> to support the arm mx51.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  drivers/mmc/fsl_esdhc.c |   72 +++++++++++++++++++++++++++++++++++++++-------
>  include/asm-arm/io.h    |   21 +++++++++++++
>  include/fsl_esdhc.h     |    6 +++-
>  include/mmc.h           |    1 +
>  4 files changed, 88 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index c6e9e6e..5dcf6a8 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -37,6 +37,10 @@
>  #include <fdt_support.h>
>  #include <asm/io.h>
>  
> +#ifndef CONFIG_PPC
> +#define out_be32(a,v)	writel(v,a)
> +#define in_be32(a)	__raw_readl(a)
> +#endif

Are you sure these are correct definitions for all architectures?
If so, they should go into a global header file, not here.

> @@ -129,7 +133,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
>  	out_be32(&regs->blkattr, data->blocks << 16 | data->blocksize);
>  
>  	/* Calculate the timeout period for data transactions */
> -	timeout = __ilog2(mmc->tran_speed/10);
> +	timeout = fls(mmc->tran_speed/10) - 1;

What's the reason for this change?

> @@ -236,11 +240,18 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>  
>  void set_sysctl(struct mmc *mmc, uint clock)
>  {
> +#ifdef CONFIG_PPC
>  	int sdhc_clk = gd->sdhc_clk;
> +#else
> +	int sdhc_clk = mxc_get_clock(MXC_IPG_PERCLK);
> +#endif

Can we avoid such #ifdef's here? Why don't we use gd->sdhc_clk
everywhere?

>  	int div, pre_div;
>  	volatile struct fsl_esdhc *regs = mmc->priv;
>  	uint clk;
>  
> +	if (clock < mmc->f_min)
> +		clock = mmc->f_min;

Print warning message ?

> @@ -257,11 +268,14 @@ void set_sysctl(struct mmc *mmc, uint clock)
>  
>  	clk = (pre_div << 8) | (div << 4);
>  
> +	/* On imx the clock must be stopped before changing frequency */
> +	clrbits_be32(&regs->sysctl, SYSCTL_CKEN);

Is this compatible with the other architectures?

>  static void esdhc_set_ios(struct mmc *mmc)
> @@ -278,15 +292,26 @@ static void esdhc_set_ios(struct mmc *mmc)
>  		setbits_be32(&regs->proctl, PROCTL_DTW_4);
>  	else if (mmc->bus_width == 8)
>  		setbits_be32(&regs->proctl, PROCTL_DTW_8);
> +
>  }

Please delete this empty line.

>  static int esdhc_init(struct mmc *mmc)
>  {
>  	struct fsl_esdhc *regs = mmc->priv;
>  	int timeout = 1000;
> +	int ret = 0;
>  
> +#ifdef CONFIG_PPC
>  	/* Enable cache snooping */
>  	out_be32(&regs->scr, 0x00000040);
> +#endif

Why only on PPC?  [I'm asking again because I don;t want to see so
many #ifdef's in such code.]

> +	/* Reset the entire host controller */
> +	out_be32(&regs->sysctl, SYSCTL_RSTA);
> +
> +	/* Wait until the controller is available */
> +	while ((in_be32(&regs->sysctl) & SYSCTL_RSTA) && --timeout)
> +		udelay(1000);

Has this been tested on non-i.MX51 systems as well?

> @@ -299,24 +324,44 @@ static int esdhc_init(struct mmc *mmc)
>  	/* Put the PROCTL reg back to the default */
>  	out_be32(&regs->proctl, PROCTL_INIT);
>  
> -	while (!(in_be32(&regs->prsstat) & PRSSTAT_CINS) && --timeout)
> -		udelay(1000);
> +	/* Set timout to the maximum value */
> +	clrsetbits_be32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
>  
> -	if (timeout <= 0)
> -		return NO_CARD_ERR;
> +	/* Check if there is a callback for detecting the card */
> +	if (!mmc->getcd) {
> +		timeout = 1000;
> +		while (!(in_be32(&regs->prsstat) & PRSSTAT_CINS) && --timeout)
> +			udelay(1000);
>  
> -	return 0;
> +		if (timeout <= 0)
> +			ret = NO_CARD_ERR;
> +	} else {
> +		if (mmc->getcd(mmc))
> +			ret = NO_CARD_ERR;
> +	}
> +
> +	return ret;
>  }

These are a lot of changes - how mu of this has actually been tested
on non-i.MX51 ?

> +int fsl_esdhc_initialize(bd_t *bis, uint32_t esdhc_addr, int (*getcd)(struct mmc *mmc))
>  {
> -	struct fsl_esdhc *regs = (struct fsl_esdhc *)CONFIG_SYS_FSL_ESDHC_ADDR;
> +#ifdef CONFIG_PPC
> +	int sdhc_clk = gd->sdhc_clk;
> +#else
> +	int sdhc_clk = mxc_get_clock(MXC_IPG_PERCLK);
> +#endif

See above.

> +	struct fsl_esdhc *regs;
>  	struct mmc *mmc;
>  	u32 caps;
>  
>  	mmc = malloc(sizeof(struct mmc));
>  
>  	sprintf(mmc->name, "FSL_ESDHC");
> +	if (!esdhc_addr) {
> +		regs = (struct fsl_esdhc *)CONFIG_SYS_FSL_ESDHC_ADDR;
> +	} else {
> +		regs = (struct fsl_esdhc *)esdhc_addr;
> +	}

Argh... Please don't. Use a common way for configuration, please.

> +#ifdef CONFIG_PPC
>  void fdt_fixup_esdhc(void *blob, bd_t *bd)
>  {
>  	const char *compat = "fsl,esdhc";
> @@ -365,3 +414,4 @@ out:
>  	do_fixup_by_compat(blob, compat, "status", status,
>  			   strlen(status) + 1, 1);
>  }
> +#endif

Can we drop this #ifdef ?

> --- a/include/asm-arm/io.h
> +++ b/include/asm-arm/io.h
> @@ -33,6 +33,27 @@ static inline void sync(void)
>  {
>  }
>  
> +/* Clear and set bits in one shot. These macros can be used to clear and
> + * set multiple bits in a register using a single call. These macros can
> + * also be used to set a multiple-bit bit pattern using a mask, by
> + * specifying the mask in the 'clear' parameter and the new bit pattern
> + * in the 'set' parameter.
> + */

Incorrect multiline comment style.

> +#define clrbits(type, addr, clear) \
> +	write##type(__raw_read##type(addr) & ~(clear), (addr))
> +
> +#define setbits(type, addr, set) \
> +	write##type(__raw_read##type(addr) | (set), (addr))
> +
> +#define clrsetbits(type, addr, clear, set) \
> +	write##type((__raw_read##type(addr) & ~(clear)) | (set), (addr))
> +
> +#define clrbits_be32(addr, clear) clrbits(l, addr, clear)
> +#define setbits_be32(addr, set) setbits(l, addr, set)
> +#define clrsetbits_be32(addr, clear, set) clrsetbits(l, addr, clear, set)

Are these macros really generally applicaple to all ARM systems,
including both big and little endian configurations?

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
"There are three principal ways to lose money: wine, women,  and  en-
gineers.  While  the first two are more pleasant, the third is by far
the more certain."                      -- Baron Rothschild, ca. 1800