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

Andy Fleming
2010-01-19T08:26:36Z

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