Dear Stefano Babic,
In message <1263212760-27272-8-git-send-email-sba...@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 <sba...@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(®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;
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(®s->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(®s->proctl, PROCTL_DTW_4);
> else if (mmc->bus_width == 8)
> setbits_be32(®s->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(®s->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(®s->sysctl, SYSCTL_RSTA);
> +
> + /* Wait until the controller is available */
> + while ((in_be32(®s->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(®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;
> }
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
"There are three principal ways to lose money: wine, women, and en-
gineers. While the first two are more pleasant, the third is by far
the more certain." -- Baron Rothschild, ca. 1800
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
