[U-Boot] [PATCH 05/22] mmc: introduces mmc modes.
Jean-Jacques Hiblot
jjhiblot at ti.com
Mon May 15 10:34:42 UTC 2017
Hi Simon,
On 15/05/2017 05:28, Simon Glass wrote:
> On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
>
> Subject: drop the period at the end
>
> Also I think 'mmc: Introduce MMC modes' is better (imperative tense)
>
>> no functionnal changes.
>> In order to add the support for the high speed SD and MMC modes, it is
>> useful to track this information.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
>> ---
>> drivers/mmc/mmc.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
>> include/mmc.h | 34 ++++++++++++++++++++++++++++------
>> 2 files changed, 74 insertions(+), 13 deletions(-)
>>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Also see below
>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 344d760..2e1cb0d 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -149,6 +149,36 @@ void mmc_trace_state(struct mmc *mmc, struct mmc_cmd *cmd)
>> }
>> #endif
>>
>> +const char *mmc_mode_name(enum bus_mode mode)
>> +{
>> + static const char *const names[] = {
>> + [MMC_LEGACY] = "MMC legacy",
>> + [SD_LEGACY] = "SD Legacy",
>> + [MMC_HS] = "MMC High Speed (26MHz)",
>> + [SD_HS] = "SD High Speed (50MHz)",
>> + [UHS_SDR12] = "UHS SDR12 (25MHz)",
>> + [UHS_SDR25] = "UHS SDR25 (50MHz)",
>> + [UHS_SDR50] = "UHS SDR50 (100MHz)",
>> + [UHS_SDR104] = "UHS SDR104 (208MHz)",
>> + [UHS_DDR50] = "UHS DDR50 (50MHz)",
>> + [MMC_HS_52] = "MMC High Speed (52MHz)",
>> + [MMC_DDR_52] = "MMC DDR52 (52MHz)",
>> + [MMC_HS_200] = "HS200 (200MHz)",
> Can we hide this behind a Kconfig so boards can turn it off to reduce
> code size in SPL?
I'll add a MMC_VERBOSE and SPL_MMC_VERBOSE options. And also enable it
if DEBUG is defined
>
>> + };
>> +
>> + if (mode >= MMC_MODES_END)
>> + return "Unknown mode";
>> + else
>> + return names[mode];
>> +}
>> +static int mmc_select_mode(struct mmc *mmc, enum bus_mode mode)
>> +{
>> + mmc->selected_mode = mode;
>> + debug("selecting mode %s (freq : %d MHz)\n", mmc_mode_name(mode),
>> + mmc->tran_speed / 1000000);
>> + return 0;
>> +}
>> +
>> #ifndef CONFIG_DM_MMC_OPS
>> int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>> {
>> @@ -1138,10 +1168,13 @@ static int sd_select_bus_freq_width(struct mmc *mmc)
>> if (err)
>> return err;
>>
>> - if (mmc->card_caps & MMC_MODE_HS)
>> + if (mmc->card_caps & MMC_MODE_HS) {
>> + mmc_select_mode(mmc, SD_HS);
>> mmc->tran_speed = 50000000;
>> - else
>> + } else {
>> + mmc_select_mode(mmc, SD_LEGACY);
>> mmc->tran_speed = 25000000;
>> + }
>>
>> return 0;
>> }
>> @@ -1255,11 +1288,15 @@ static int mmc_select_bus_freq_width(struct mmc *mmc)
>> if (err)
>> return err;
>>
>> - if (mmc->card_caps & MMC_MODE_HS) {
>> - if (mmc->card_caps & MMC_MODE_HS_52MHz)
>> - mmc->tran_speed = 52000000;
>> + if (mmc->card_caps & MMC_MODE_HS_52MHz) {
>> + if (mmc->ddr_mode)
>> + mmc_select_mode(mmc, MMC_DDR_52);
>> else
>> - mmc->tran_speed = 26000000;
>> + mmc_select_mode(mmc, MMC_HS_52);
>> + mmc->tran_speed = 52000000;
>> + } else if (mmc->card_caps & MMC_MODE_HS) {
>> + mmc_select_mode(mmc, MMC_HS);
>> + mmc->tran_speed = 26000000;
>> }
>>
>> return err;
>> @@ -1529,7 +1566,9 @@ static int mmc_startup(struct mmc *mmc)
>> freq = fbase[(cmd.response[0] & 0x7)];
>> mult = multipliers[((cmd.response[0] >> 3) & 0xf)];
>>
>> - mmc->tran_speed = freq * mult;
>> + mmc->legacy_speed = freq * mult;
>> + mmc->tran_speed = mmc->legacy_speed;
>> + mmc_select_mode(mmc, MMC_LEGACY);
>>
>> mmc->dsr_imp = ((cmd.response[1] >> 12) & 0x1);
>> mmc->read_bl_len = 1 << ((cmd.response[1] >> 16) & 0xf);
>> diff --git a/include/mmc.h b/include/mmc.h
>> index 9af6b52..60a43b0 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -52,12 +52,15 @@
>> #define MMC_VERSION_5_0 MAKE_MMC_VERSION(5, 0, 0)
>> #define MMC_VERSION_5_1 MAKE_MMC_VERSION(5, 1, 0)
>>
>> -#define MMC_MODE_HS (1 << 0)
>> -#define MMC_MODE_HS_52MHz (1 << 1)
>> -#define MMC_MODE_4BIT (1 << 2)
>> -#define MMC_MODE_8BIT (1 << 3)
>> -#define MMC_MODE_SPI (1 << 4)
>> -#define MMC_MODE_DDR_52MHz (1 << 5)
>> +#define MMC_CAP(mode) (1 << mode)
>> +#define MMC_MODE_HS (MMC_CAP(MMC_HS) | MMC_CAP(SD_HS))
>> +#define MMC_MODE_HS_52MHz MMC_CAP(MMC_HS_52)
>> +#define MMC_MODE_DDR_52MHz MMC_CAP(MMC_DDR_52)
>> +
>> +#define MMC_MODE_8BIT (1 << 30)
>> +#define MMC_MODE_4BIT (1 << 29)
>> +#define MMC_MODE_SPI (1 << 27)
>> +
>>
>> #define SD_DATA_4BIT 0x00040000
>>
>> @@ -402,6 +405,23 @@ struct sd_ssr {
>> unsigned int erase_offset; /* In milliseconds */
>> };
>>
>> +enum bus_mode {
>> + MMC_LEGACY = 0,
>> + SD_LEGACY = 1,
>> + MMC_HS = 2,
>> + SD_HS = 3,
>> + UHS_SDR12 = 4,
>> + UHS_SDR25 = 5,
>> + UHS_SDR50 = 6,
>> + UHS_SDR104 = 7,
>> + UHS_DDR50 = 8,
>> + MMC_HS_52 = 9,
>> + MMC_DDR_52 = 10,
>> + MMC_HS_200 = 11,
> Do you need to specify the values or would defaults be OK?
I assigned fixed values for debug purpose, I'll remove them.
>
>> + MMC_MODES_END
>> +};
>> +
>> +const char *mmc_mode_name(enum bus_mode mode);
>> /*
>> * With CONFIG_DM_MMC enabled, struct mmc can be accessed from the MMC device
>> * with mmc_get_mmc_dev().
>> @@ -432,6 +452,7 @@ struct mmc {
>> u8 wr_rel_set;
>> char part_config;
>> uint tran_speed;
>> + uint legacy_speed;
> Please add comment. Should this be an enum?
No. The legacy speed is a value in Hz. It's computed from values
provided by the card during the initialization process.
>
>> uint read_bl_len;
>> uint write_bl_len;
>> uint erase_grp_size; /* in 512-byte sectors */
>> @@ -455,6 +476,7 @@ struct mmc {
>> struct udevice *dev; /* Device for this MMC controller */
>> #endif
>> u8 *ext_csd;
>> + enum bus_mode selected_mode;
>> };
>>
>> struct mmc_hwpart_conf {
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
thanks for taking time to review this whole series.
Jean-Jacques
More information about the U-Boot
mailing list