[U-Boot] [PATCH] PXA: New MMC driver

[Wolfgang Denk]

Dear Marek Vasut,

In message <1281037915-3224-1-git-send-email-marek.vasut at gmail.com> you wrote:
> The new driver is a complete rewrite. It uses the MMC framework and should
> support both pxa2xx and pxa3xx.

Thanks.

> +/* PXAMMC Generic default config for various CPUs */
> +#if defined(CONFIG_PXA250)
> +	#define	PXAMMC_FIFO_SIZE	1
> +	#define	PXAMMC_MIN_SPEED	312500
> +	#define	PXAMMC_MAX_SPEED	20000000
> +	#define	PXAMMC_HOST_CAPS	(0)
> +#elif defined(CONFIG_PXA27X)
> +	#define	PXAMMC_CRC_SKIP
> +	#define	PXAMMC_FIFO_SIZE	32
> +	#define	PXAMMC_MIN_SPEED	304000
> +	#define	PXAMMC_MAX_SPEED	19500000
> +	#define	PXAMMC_HOST_CAPS	(MMC_MODE_4BIT)
> +#elif defined(CONFIG_CPU_MONAHANS)
> +	#define	PXAMMC_FIFO_SIZE	32
> +	#define	PXAMMC_MIN_SPEED	304000
> +	#define	PXAMMC_MAX_SPEED	26000000
> +	#define	PXAMMC_HOST_CAPS	(MMC_MODE_4BIT | MMC_MODE_HS)

Please put the '#' always in column 1.

> +#else
> +#error	"Unknown CPU for PXAMMC"
> +#endif
> +
>  #endif /* __MMC_PXA_P_H__ */
> diff --git a/drivers/mmc/pxa_mmc_gen.c b/drivers/mmc/pxa_mmc_gen.c
> new file mode 100644
> index 0000000..b50ded6
> --- /dev/null
> +++ b/drivers/mmc/pxa_mmc_gen.c
> @@ -0,0 +1,333 @@
> +/*
> + * Copyright (C) 2010 Marek Vasut <marek.vasut at gmail.com>
> + *
> + * Loosely based on the old code and Linux's PXA MMC driver
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <malloc.h>
> +
> +#include <mmc.h>
> +#include <asm/errno.h>
> +#include <asm/arch/hardware.h>
> +
> +#include "pxa_mmc.h"
> +
> +#define	PXA_MMC_TIMEOUT	100

Please add a comment about the unit (10 us?)

> +static int pxa_mmc_stop_clock(void)
> +{
> +	int timeout = PXA_MMC_TIMEOUT;
> +
> +	/* If the clock aren't running, exit */

"clock isn't" or "clocks aren't" ?

> +	while (!(MMC_STAT & MMC_STAT_CLK_EN) && --timeout)
> +		udelay(10);
> +
> +	/* The clock didn't start, we have a problem */
> +	if (!timeout)
> +		return -ETIMEDOUT;

This block repeats. Make it a (inline?) function (with the mask as
param)?  Also include the "int timeout = PXA_MMC_TIMEOUT;" part.

> +	/* Wait for the transmission-done interrupt */
> +	while (!(MMC_STAT & MMC_STAT_DATA_TRAN_DONE) && --timeout)
> +		udelay(10);
> +
> +	if (!timeout)
> +		return -ETIMEDOUT;

Third time the same code.

> +	/* Wait for the transmission-done interrupt */
> +	while (!(MMC_STAT & MMC_STAT_DATA_TRAN_DONE) && --timeout)
> +		udelay(10);
> +
> +	if (!timeout)
> +		return -ETIMEDOUT;

# 4.

> +	/* Wait until the data are really written to the card */
> +	timeout = PXA_MMC_TIMEOUT;
> +	while (!(MMC_STAT & MMC_STAT_PRG_DONE) && --timeout)
> +		udelay(10);
> +
> +	if (!timeout)
> +		return -ETIMEDOUT;

# 5.

> +	/* Wait until the command completes */
> +	while (!(MMC_STAT & MMC_STAT_END_CMD_RES) && --timeout)
> +		udelay(10);
> +
> +	/* If the command doesn't complete, die */
> +	if (!timeout)
> +		return -ETIMEDOUT;

# 6.

> +int pxa_mmc_init(bd_t *bis)
> +{
> +	struct mmc *mmc = NULL;

Drop the "= NULL" - it makes no sense given the next line is this:

> +	mmc = malloc(sizeof(struct mmc));



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: wd at denx.de
The Empire didn't encourage its subjects to go far away, in case they
saw things that might disturb them. For the same reason it had  built
a  wall  around  the  entire country, patrolled by the Heavenly Guard
whose main function was to tread heavily on the fingers of any  inha-
bitants who felt they might like to step outside for five minutes for
a breath of fresh air.                      - Terry Pratchett, _Mort_