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 <[email protected]> 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 <[email protected]> > --- > 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(®s->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(®s->sysctl, SYSCTL_CKEN); > + > clrsetbits_be32(®s->sysctl, SYSCTL_CLOCK_MASK, clk); > > udelay(10000); > > - setbits_be32(®s->sysctl, SYSCTL_PEREN); > + setbits_be32(®s->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(®s->proctl, PROCTL_DTW_4); > else if (mmc->bus_width == 8) > setbits_be32(®s->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(®s->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(®s->sysctl, SYSCTL_RSTA); > + > + /* Wait until the controller is available */ > + while ((in_be32(®s->sysctl) & SYSCTL_RSTA) && --timeout) > + udelay(1000); > > out_be32(®s->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(®s->proctl, PROCTL_INIT); > > - while (!(in_be32(®s->prsstat) & PRSSTAT_CINS) && --timeout) > - udelay(1000); > + /* Set timout to the maximum value */ > + clrsetbits_be32(®s->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(®s->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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
