[U-Boot] [PATCH 1/4] mmc: Fix handling of bus widths and DDR card capabilities

[Pantelis Antoniou]

Hi Andrew,

Looks good. I don’t have a DDR capable board to test, but at first glance
it’s fine.

> On Dec 1, 2014, at 14:59 , Andrew Gabbasov <andrew_gabbasov at mentor.com> wrote:
> 
> If the MMC_MODE_DDR_52MHz flag is set in card capabilities bitmask,
> it is never cleared, even if switching to DDR mode fails, and if
> the controller driver uses this flag to check the DDR mode, it can
> take incorrect actions.
> 
> Also, DDR related checks in mmc_startup() incorrectly handle the case
> when the host controller does not support some bus widths (e.g. can't
> support 8 bits), since the host_caps is checked for DDR bit, but not
> bus width bits.
> 
> This fix clearly separates using of card_caps bitmask, having there
> the flags for the capabilities, that the card can support, and actual
> operation mode, described outside of card_caps (i.e. bus_width and
> ddr_mode fields in mmc structure). Separate host controller drivers
> may need to be updated to use the actual flags. Respectively,
> the capabilities checks in mmc_startup are made more correct and clear.
> 
> Also, some clean up is made with errors handling and code syntax layout.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov at mentor.com>
> ---
> common/cmd_mmc.c  |  3 ++-
> drivers/mmc/mmc.c | 52 +++++++++++++++++++++++++++++++---------------------
> include/mmc.h     |  1 +
> 3 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c
> index 4286e26..96478e4 100644
> --- a/common/cmd_mmc.c
> +++ b/common/cmd_mmc.c
> @@ -90,7 +90,8 @@ static void print_mmcinfo(struct mmc *mmc)
> 	puts("Capacity: ");
> 	print_size(mmc->capacity, "\n");
> 
> -	printf("Bus Width: %d-bit\n", mmc->bus_width);
> +	printf("Bus Width: %d-bit%s\n", mmc->bus_width,
> +			mmc->ddr_mode ? " DDR" : "");
> }
> static struct mmc *init_mmc_device(int dev, bool force_init)
> {
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 44a4feb..4603a81 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -159,7 +159,7 @@ int mmc_set_blocklen(struct mmc *mmc, int len)
> {
> 	struct mmc_cmd cmd;
> 
> -	if (mmc->card_caps & MMC_MODE_DDR_52MHz)
> +	if (mmc->ddr_mode)
> 		return 0;
> 
> 	cmd.cmdidx = MMC_CMD_SET_BLOCKLEN;
> @@ -486,7 +486,7 @@ static int mmc_change_freq(struct mmc *mmc)
> 	char cardtype;
> 	int err;
> 
> -	mmc->card_caps = 0;
> +	mmc->card_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
> 
> 	if (mmc_host_is_spi(mmc))
> 		return 0;
> @@ -1103,8 +1103,10 @@ static int mmc_startup(struct mmc *mmc)
> 
> 		/* An array to map CSD bus widths to host cap bits */
> 		static unsigned ext_to_hostcaps[] = {
> -			[EXT_CSD_DDR_BUS_WIDTH_4] = MMC_MODE_DDR_52MHz,
> -			[EXT_CSD_DDR_BUS_WIDTH_8] = MMC_MODE_DDR_52MHz,
> +			[EXT_CSD_DDR_BUS_WIDTH_4] =
> +				MMC_MODE_DDR_52MHz | MMC_MODE_4BIT,
> +			[EXT_CSD_DDR_BUS_WIDTH_8] =
> +				MMC_MODE_DDR_52MHz | MMC_MODE_8BIT,
> 			[EXT_CSD_BUS_WIDTH_4] = MMC_MODE_4BIT,
> 			[EXT_CSD_BUS_WIDTH_8] = MMC_MODE_8BIT,
> 		};
> @@ -1116,13 +1118,13 @@ static int mmc_startup(struct mmc *mmc)
> 
> 		for (idx=0; idx < ARRAY_SIZE(ext_csd_bits); idx++) {
> 			unsigned int extw = ext_csd_bits[idx];
> +			unsigned int caps = ext_to_hostcaps[extw];
> 
> 			/*
> -			 * Check to make sure the controller supports
> -			 * this bus width, if it's more than 1
> +			 * Check to make sure the card and controller support
> +			 * these capabilities
> 			 */
> -			if (extw != EXT_CSD_BUS_WIDTH_1 &&
> -					!(mmc->cfg->host_caps & ext_to_hostcaps[extw]))
> +			if ((mmc->card_caps & caps) != caps)
> 				continue;
> 
> 			err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
> @@ -1131,26 +1133,33 @@ static int mmc_startup(struct mmc *mmc)
> 			if (err)
> 				continue;
> 
> +			mmc->ddr_mode = (caps & MMC_MODE_DDR_52MHz) ? 1 : 0;
> 			mmc_set_bus_width(mmc, widths[idx]);
> 
> 			err = mmc_send_ext_csd(mmc, test_csd);
> +
> +			if (err)
> +				continue;
> +
> 			/* Only compare read only fields */
> -			if (!err && ext_csd[EXT_CSD_PARTITIONING_SUPPORT] \
> -				    == test_csd[EXT_CSD_PARTITIONING_SUPPORT]
> -				 && ext_csd[EXT_CSD_HC_WP_GRP_SIZE] \
> -				    == test_csd[EXT_CSD_HC_WP_GRP_SIZE] \
> -				 && ext_csd[EXT_CSD_REV] \
> -				    == test_csd[EXT_CSD_REV]
> -				 && ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] \
> -				    == test_csd[EXT_CSD_HC_ERASE_GRP_SIZE]
> -				 && memcmp(&ext_csd[EXT_CSD_SEC_CNT], \
> -					&test_csd[EXT_CSD_SEC_CNT], 4) == 0) {
> -
> -				mmc->card_caps |= ext_to_hostcaps[extw];
> +			if (ext_csd[EXT_CSD_PARTITIONING_SUPPORT]
> +				== test_csd[EXT_CSD_PARTITIONING_SUPPORT] &&
> +			    ext_csd[EXT_CSD_HC_WP_GRP_SIZE]
> +				== test_csd[EXT_CSD_HC_WP_GRP_SIZE] &&
> +			    ext_csd[EXT_CSD_REV]
> +				== test_csd[EXT_CSD_REV] &&
> +			    ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE]
> +				== test_csd[EXT_CSD_HC_ERASE_GRP_SIZE] &&
> +			    memcmp(&ext_csd[EXT_CSD_SEC_CNT],
> +				   &test_csd[EXT_CSD_SEC_CNT], 4) == 0)
> 				break;
> -			}
> +			else
> +				err = SWITCH_ERR;
> 		}
> 
> +		if (err)
> +			return err;
> +
> 		if (mmc->card_caps & MMC_MODE_HS) {
> 			if (mmc->card_caps & MMC_MODE_HS_52MHz)
> 				mmc->tran_speed = 52000000;
> @@ -1299,6 +1308,7 @@ int mmc_start_init(struct mmc *mmc)
> 	if (err)
> 		return err;
> 
> +	mmc->ddr_mode = 0;
> 	mmc_set_bus_width(mmc, 1);
> 	mmc_set_clock(mmc, 1);
> 
> diff --git a/include/mmc.h b/include/mmc.h
> index d74a190..9c6d792 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -314,6 +314,7 @@ struct mmc {
> 	char init_in_progress;	/* 1 if we have done mmc_start_init() */
> 	char preinit;		/* start init as early as possible */
> 	uint op_cond_response;	/* the response byte from the last op_cond */
> +	int ddr_mode;
> };
> 
> int mmc_register(struct mmc *mmc);
> -- 
> 2.1.0

Applied, thanks.

— Pantelis