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_gabba...@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_gabba...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
