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

Wolfgang has already covered most of this, but I have a few other comments (plus a couple of redundant ones)

On Wed, Jan 13, 2010 at 3:50 AM, Stefano Babic sbabic@denx.de 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..d9574d0 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

I know why you did this, but I really think it's a bad idea to "trick" the driver into doing the right thing. It's more painful, but we need to change all of the out_be32/in_be32 commands into something generic, and then add support for big and little endian accesses. This is just a hack. :(

DECLARE_GLOBAL_DATA_PTR;

@@ -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;

timeout -= 13;

if (timeout > 14) @@ -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

Let's try to use #ifdefs sparingly, and definitely have them trigger on constants that are directly related to the difference. This isn't a PPC/ARM problem. I see you've already agreed to use gd->sdhc_clk, though, so that's good.

int div, pre_div;        volatile struct fsl_esdhc *regs = mmc->priv;        uint clk;

• if (clock < mmc->f_min)
• clock = mmc->f_min;

if (sdhc_clk / 16 > clock) {                for (pre_div = 2; pre_div < 256; pre_div *= 2)                        if ((sdhc_clk / pre_div) <= (clock * 16)) @@ -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);

clrsetbits_be32(&regs->sysctl, SYSCTL_CLOCK_MASK, clk);

udelay(10000);

• setbits_be32(&regs->sysctl, SYSCTL_PEREN);

• setbits_be32(&regs->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);

}

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);

}

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

You have two choices, here. Either create a new CONFIG_SYS option that declares the existence of this register for platforms that have it, OR (my preference) design a configuration mechanism which allows board/cpu code to declare such things. Ideally, the driver could detect this based on a version register, but I'm well aware that FSL's hardware designers frequently forget to increment their version numbers for such "small" changes. What is certain is that CONFIG_PPC is wrong.

• /* 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);

out_be32(&regs->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN);

@@ -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;

}

-static int esdhc_initialize(bd_t *bis) +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

• struct fsl_esdhc *regs;

struct mmc *mmc;        u32 caps;

mmc = malloc(sizeof(struct mmc));

sprintf(mmc->name, "FSL_ESDHC");

• regs = (struct fsl_esdhc *)CONFIG_SYS_FSL_ESDHC_ADDR;
• if (esdhc_addr)
• regs = (struct fsl_esdhc *)esdhc_addr;

I agree with Wolfgang, here. Also, it's a bit iffy to set it, then optionally overwrite it.

mmc->priv = regs;        mmc->send_cmd = esdhc_send_cmd;        mmc->set_ios = esdhc_set_ios; @@ -337,7 +382,10 @@ static int esdhc_initialize(bd_t *bis)                mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;

mmc->f_min = 400000;

• mmc->f_max = MIN(gd->sdhc_clk, 50000000);

• mmc->f_max = MIN(sdhc_clk, 50000000);
• if (getcd)
• mmc->getcd = getcd;

I don't fundamentally object to this change, but I have more comments on it later.

mmc_register(mmc);

@@ -346,9 +394,10 @@ static int esdhc_initialize(bd_t *bis)

int fsl_esdhc_mmc_init(bd_t *bis)  {

• return esdhc_initialize(bis);

• return fsl_esdhc_initialize(bis, 0, NULL);

}

+#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

Use the OF config option, here.

diff --git a/include/asm-arm/io.h b/include/asm-arm/io.h index fec3a7e..fb104aa 100644 --- 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.
• */

+#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)

This should be part of a completely different patch. Also, I'm positive that it's completely wrong. setbits_be32 is big endian, writel is little endian.

/*  * Given a physical address and a length, return a virtual address  * that can be used to access the memory range with the caching diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc.h index 89b8304..605e065 100644 --- a/include/fsl_esdhc.h +++ b/include/fsl_esdhc.h @@ -32,7 +32,9 @@  #define SYSCTL                 0x0002e02c  #define SYSCTL_INITA           0x08000000  #define SYSCTL_TIMEOUT_MASK    0x000f0000 -#define SYSCTL_CLOCK_MASK      0x00000fff +#define SYSCTL_CLOCK_MASK      0x0000ffff +#define SYSCTL_RSTA            0x01000000 +#define SYSCTL_CKEN            0x00000008  #define SYSCTL_PEREN           0x00000004  #define SYSCTL_HCKEN           0x00000002  #define SYSCTL_IPGEN           0x00000001 @@ -144,6 +146,8 @@

#ifdef CONFIG_FSL_ESDHC  int fsl_esdhc_mmc_init(bd_t *bis); +int fsl_esdhc_initialize(bd_t *bis, uint32_t esdhc_addr,

• int (*getcd)(struct mmc *mmc));

Hmmm....this doesn't scale well. Rather than pass in an address and a function pointer, create a structure with that information, and then pass *that* in. That way, when we discover we want some other information/functions, we can add them without having to modify the API.

Also, I'm not entirely sold on the idea that getcd should be a function pointer. It might be more appropriate as a weakly defined function that a board can choose to implement.

void fdt_fixup_esdhc(void *blob, bd_t *bd);  #else  static inline int fsl_esdhc_mmc_init(bd_t *bis) { return -ENOSYS; } diff --git a/include/mmc.h b/include/mmc.h index 2dc69ab..bc29953 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -264,6 +264,7 @@ struct mmc {                        struct mmc_cmd *cmd, struct mmc_data *data);        void (*set_ios)(struct mmc *mmc);        int (*init)(struct mmc *mmc);

• int (*getcd)(struct mmc *mmc);

};

int mmc_register(struct mmc *mmc);

I think getcd needs more discussion, but even if it doesn't, this clearly belongs in a separate patch. You are modifying the U-Boot MMC API, here, not just the fsl_esdhc driver.

Andy