[U-Boot] [PATCH V2 7/9] fsl_esdhc: add support for mx51 processor
Andy Fleming
afleming at gmail.com
Tue Jan 19 09:26:36 CET 2010
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 at 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 at 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(®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
More information about the U-Boot
mailing list