| Message ID | 1494613000-8156-6-git-send-email-jjhiblot@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@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@ti.com> > --- > drivers/mmc/mmc.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++------- > include/mmc.h | 34 ++++++++++++++++++++++++++++------ > 2 files changed, 74 insertions(+), 13 deletions(-) > Reviewed-by: Simon Glass <sjg@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? > + }; > + > + 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? > + 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? > 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
Hi Simon, On 15/05/2017 05:28, Simon Glass wrote: > On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhiblot@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@ti.com> >> --- >> drivers/mmc/mmc.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++------- >> include/mmc.h | 34 ++++++++++++++++++++++++++++------ >> 2 files changed, 74 insertions(+), 13 deletions(-) >> > Reviewed-by: Simon Glass <sjg@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
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)", + }; + + 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, + 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; 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 {
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@ti.com> --- drivers/mmc/mmc.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++------- include/mmc.h | 34 ++++++++++++++++++++++++++++------ 2 files changed, 74 insertions(+), 13 deletions(-)