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

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