Hi, 
On Mon, Mar 05, 2018 at 05:29:08PM +0100, Jean-Jacques Hiblot wrote:
>Hi Peng,
>
>I'm glad you are adding HS400 support. Thanks.
>
>
>On 05/03/2018 10:11, Peng Fan wrote:
>>Add HS400 support.
>>Selecting HS400 needs first select HS199 according to spec, so use
>>a dedicated function for HS400.
>>Add HS400 related macros.
>>Remove the restriction of only using the low 6 bits of
>>EXT_CSD_CARD_TYPE, using all the 8 bits.
>>
>>Signed-off-by: Peng Fan <peng....@nxp.com>
>>Cc: Jaehoon Chung <jh80.ch...@samsung.com>
>>Cc: Jean-Jacques Hiblot <jjhib...@ti.com>
>>Cc: Stefano Babic <sba...@denx.de>
>>Cc: Simon Glass <s...@chromium.org>
>>Cc: Kishon Vijay Abraham I <kis...@ti.com>
>>Cc: Bin Meng <bmeng...@gmail.com>
>>---
>>  drivers/mmc/Kconfig |   7 +++
>>  drivers/mmc/mmc.c   | 133 
>> ++++++++++++++++++++++++++++++++++++++++++----------
>>  include/mmc.h       |  12 +++++
>>  3 files changed, 127 insertions(+), 25 deletions(-)
>>
>>diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>>index 5f67e336db..e9be18b333 100644
>>--- a/drivers/mmc/Kconfig
>>+++ b/drivers/mmc/Kconfig
>>@@ -104,6 +104,13 @@ config SPL_MMC_UHS_SUPPORT
>>        cards. The IO voltage must be switchable from 3.3v to 1.8v. The bus
>>        frequency can go up to 208MHz (SDR104)
>>+config MMC_HS400_SUPPORT
>>+     bool "enable HS400 support"
>>+     select MMC_HS200_SUPPORT
>I'd use "depends on" instead of select or maybe use the same option
>for both HS200 and HS400
>>+     help
>>+       The HS400 mode is support by some eMMC. The bus frequency is up to
>>+       200MHz. This mode requires tuning the IO.
>>+
>>  config MMC_HS200_SUPPORT
>>      bool "enable HS200 support"
>>      help
>>diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>index 92ea78b8af..eef229c8b4 100644
>>--- a/drivers/mmc/mmc.c
>>+++ b/drivers/mmc/mmc.c
>>@@ -169,6 +169,7 @@ const char *mmc_mode_name(enum bus_mode mode)
>>            [MMC_HS_52]       = "MMC High Speed (52MHz)",
>>            [MMC_DDR_52]      = "MMC DDR52 (52MHz)",
>>            [MMC_HS_200]      = "HS200 (200MHz)",
>>+           [MMC_HS_400]      = "HS400 (200MHz)",
>>      };
>>      if (mode >= MMC_MODES_END)
>>@@ -193,6 +194,7 @@ static uint mmc_mode2freq(struct mmc *mmc, enum bus_mode 
>>mode)
>>            [UHS_DDR50]       = 50000000,
>>            [UHS_SDR104]      = 208000000,
>>            [MMC_HS_200]      = 200000000,
>>+           [MMC_HS_400]      = 200000000,
>>      };
>>      if (mode == MMC_LEGACY)
>>@@ -790,6 +792,11 @@ static int mmc_set_card_speed(struct mmc *mmc, enum 
>>bus_mode mode)
>>      case MMC_HS_200:
>>              speed_bits = EXT_CSD_TIMING_HS200;
>>              break;
>>+#endif
>>+#if CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
>>+     case MMC_HS_400:
>>+             speed_bits = EXT_CSD_TIMING_HS400;
>>+             break;
>>  #endif
>>      case MMC_LEGACY:
>>              speed_bits = EXT_CSD_TIMING_LEGACY;
>>@@ -837,7 +844,7 @@ static int mmc_get_capabilities(struct mmc *mmc)
>>      mmc->card_caps |= MMC_MODE_4BIT | MMC_MODE_8BIT;
>>-     cardtype = ext_csd[EXT_CSD_CARD_TYPE] & 0x3f;
>>+     cardtype = ext_csd[EXT_CSD_CARD_TYPE];
>>      mmc->cardtype = cardtype;
>>  #if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)
>>@@ -845,6 +852,12 @@ static int mmc_get_capabilities(struct mmc *mmc)
>>                      EXT_CSD_CARD_TYPE_HS200_1_8V)) {
>>              mmc->card_caps |= MMC_MODE_HS200;
>>      }
>>+#endif
>>+#if CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
>>+     if (cardtype & (EXT_CSD_CARD_TYPE_HS400_1_2V |
>>+                     EXT_CSD_CARD_TYPE_HS400_1_8V)) {
>>+             mmc->card_caps |= MMC_MODE_HS400;
>>+     }
>>  #endif
>>      if (cardtype & EXT_CSD_CARD_TYPE_52) {
>>              if (cardtype & EXT_CSD_CARD_TYPE_DDR_52)
>>@@ -1748,6 +1761,12 @@ static int mmc_set_lowest_voltage(struct mmc *mmc, 
>>enum bus_mode mode,
>>      u32 card_mask = 0;
>>      switch (mode) {
>>+     case MMC_HS_400:
>>+             if (mmc->cardtype & EXT_CSD_CARD_TYPE_HS400_1_8V)
>>+                     card_mask |= MMC_SIGNAL_VOLTAGE_180;
>>+             if (mmc->cardtype & EXT_CSD_CARD_TYPE_HS400_1_2V)
>>+                     card_mask |= MMC_SIGNAL_VOLTAGE_120;
>>+             break;
>>      case MMC_HS_200:
>>              if (mmc->cardtype & EXT_CSD_CARD_TYPE_HS200_1_8V)
>>                      card_mask |= MMC_SIGNAL_VOLTAGE_180;
>>@@ -1787,6 +1806,13 @@ static inline int mmc_set_lowest_voltage(struct mmc 
>>*mmc, enum bus_mode mode,
>>  #endif
>>  static const struct mode_width_tuning mmc_modes_by_pref[] = {
>>+#if CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
>>+     {
>>+             .mode = MMC_HS_400,
>>+             .widths = MMC_MODE_8BIT | MMC_MODE_4BIT,
>>+             .tuning = MMC_CMD_SEND_TUNING_BLOCK_HS200
>>+     },
>>+#endif
>>  #if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)
>>      {
>>              .mode = MMC_HS_200,
>>@@ -1830,6 +1856,54 @@ static const struct ext_csd_bus_width {
>>      {MMC_MODE_1BIT, false, EXT_CSD_BUS_WIDTH_1},
>>  };
>>+#if CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
>>+static int mmc_select_hs400(struct mmc *mmc)
>>+{
>>+     int err;
>>+
>>+     /* Set timing to HS200 for tuning */
>>+     err = mmc_set_card_speed(mmc, MMC_HS_200);
>>+     if (err)
>>+             return err;
>>+
>>+     /* configure the bus mode (host) */
>>+     mmc_select_mode(mmc, MMC_HS_200);
>>+     mmc_set_clock(mmc, mmc->tran_speed, false);
>>+
>>+     /* execute tuning if needed */
>>+     err = mmc_execute_tuning(mmc, MMC_CMD_SEND_TUNING_BLOCK_HS200);
>>+     if (err) {
>>+             debug("tuning failed\n");
>>+             return err;
>>+     }
>>+
>>+     /* Set back to HS */
>>+     mmc_set_card_speed(mmc, MMC_HS);
>>+     mmc_set_clock(mmc, mmc_mode2freq(mmc, MMC_HS), false);
>>+
>>+     err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
>>+                      EXT_CSD_BUS_WIDTH_8 | EXT_CSD_DDR_FLAG);
>What happens if only 4 wires are used. It is a legit mode, isn't it ?

According to spec, HS400 only supports 8bits mode, see "5.3.6 HS400 Bus Speed 
Mode".

>>+     if (err)
>>+             return err;
>>+
>>+     err = mmc_set_card_speed(mmc, MMC_HS_400);
>>+     if (err)
>>+             return err;
>>+
>>+     mmc_select_mode(mmc, MMC_HS_400);
>>+     err = mmc_set_clock(mmc, mmc->tran_speed, false);
>>+     if (err)
>>+             return err;
>>+
>>+     return 0;
>>+}
>>+#else
>>+static int mmc_select_hs400(struct mmc *mmc)
>>+{
>>+     return -ENOTSUPP;
>>+}
>>+#endif
>>+
>>  #define for_each_supported_width(caps, ddr, ecbv) \
>>      for (ecbv = ext_csd_bus_width;\
>>          ecbv < ext_csd_bus_width + ARRAY_SIZE(ext_csd_bus_width);\
>>@@ -1883,37 +1957,46 @@ static int mmc_select_mode_and_width(struct mmc *mmc, 
>>uint card_caps)
>>                              goto error;
>>                      mmc_set_bus_width(mmc, bus_width(ecbw->cap));
>>-                     /* configure the bus speed (card) */
>>-                     err = mmc_set_card_speed(mmc, mwt->mode);
>>-                     if (err)
>>-                             goto error;
>>-
>>-                     /*
>>-                      * configure the bus width AND the ddr mode (card)
>>-                      * The host side will be taken care of in the next step
>>-                      */
>>-                     if (ecbw->ext_csd_bits & EXT_CSD_DDR_FLAG) {
>>-                             err = mmc_switch(mmc, EXT_CSD_CMD_SET_NORMAL,
>>-                                              EXT_CSD_BUS_WIDTH,
>>-                                              ecbw->ext_csd_bits);
>>+                     if (mwt->mode == MMC_HS_400) {
>>+                             err = mmc_select_hs400(mmc);
>>+                             if (err)
>>+                                     goto error;
>>+                     } else {
>>+                             /* configure the bus speed (card) */
>>+                             err = mmc_set_card_speed(mmc, mwt->mode);
>Instead of having?? a separate mmc_select_hs400() is not possible to
>leverage the existing code ?

hs400 requires first runs into HS200 mode, after tuning finished,
need switch back to HS mode to configure bus. It's not that straightforward
to use the existing code. Use a dedicated mmc_select_hs400 will make it
clearer compared with adding more logic in the existing flow.

Thanks,
Peng.

>
>JJ
>
>>                              if (err)
>>                                      goto error;
>>-                     }
>>-                     /* configure the bus mode (host) */
>>-                     mmc_select_mode(mmc, mwt->mode);
>>-                     mmc_set_clock(mmc, mmc->tran_speed, false);
>>+                             /*
>>+                              * configure the bus width AND the ddr mode
>>+                              * (card). The host side will be taken care
>>+                              * of in the next step
>>+                              */
>>+                             if (ecbw->ext_csd_bits & EXT_CSD_DDR_FLAG) {
>>+                                     err = mmc_switch(mmc,
>>+                                                      EXT_CSD_CMD_SET_NORMAL,
>>+                                                      EXT_CSD_BUS_WIDTH,
>>+                                                      ecbw->ext_csd_bits);
>>+                                     if (err)
>>+                                             goto error;
>>+                             }
>>+
>>+                             /* configure the bus mode (host) */
>>+                             mmc_select_mode(mmc, mwt->mode);
>>+                             mmc_set_clock(mmc, mmc->tran_speed, false);
>>  #ifdef MMC_SUPPORTS_TUNING
>>-                     /* execute tuning if needed */
>>-                     if (mwt->tuning) {
>>-                             err = mmc_execute_tuning(mmc, mwt->tuning);
>>-                             if (err) {
>>-                                     pr_debug("tuning failed\n");
>>-                                     goto error;
>>+                             /* execute tuning if needed */
>>+                             if (mwt->tuning) {
>>+                                     err = mmc_execute_tuning(mmc,
>>+                                                              mwt->tuning);
>>+                                     if (err) {
>>+                                             pr_debug("tuning failed\n");
>>+                                             goto error;
>>+                                     }
>>                              }
>>-                     }
>>  #endif
>>+                     }
>>                      /* do a transfer to check the configuration */
>>                      err = mmc_read_and_compare_ext_csd(mmc);
>>diff --git a/include/mmc.h b/include/mmc.h
>>index 86f885b504..8c01c6a530 100644
>>--- a/include/mmc.h
>>+++ b/include/mmc.h
>>@@ -65,6 +65,7 @@
>>  #define MMC_MODE_HS_52MHz   MMC_CAP(MMC_HS_52)
>>  #define MMC_MODE_DDR_52MHz  MMC_CAP(MMC_DDR_52)
>>  #define MMC_MODE_HS200              MMC_CAP(MMC_HS_200)
>>+#define MMC_MODE_HS400               MMC_CAP(MMC_HS_400)
>>  #define MMC_MODE_8BIT               BIT(30)
>>  #define MMC_MODE_4BIT               BIT(29)
>>@@ -250,6 +251,11 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx)
>>  #define EXT_CSD_CARD_TYPE_HS200             (EXT_CSD_CARD_TYPE_HS200_1_8V | 
>> \
>>                                       EXT_CSD_CARD_TYPE_HS200_1_2V)
>>+#define EXT_CSD_CARD_TYPE_HS400_1_8V BIT(6)
>>+#define EXT_CSD_CARD_TYPE_HS400_1_2V BIT(7)
>>+#define EXT_CSD_CARD_TYPE_HS400              (EXT_CSD_CARD_TYPE_HS400_1_8V | 
>>\
>>+                                      EXT_CSD_CARD_TYPE_HS400_1_2V)
>>+
>>  #define EXT_CSD_BUS_WIDTH_1 0       /* Card is in 1 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_4 1       /* Card is in 4 bit mode */
>>  #define EXT_CSD_BUS_WIDTH_8 2       /* Card is in 8 bit mode */
>>@@ -260,6 +266,7 @@ static inline bool mmc_is_tuning_cmd(uint cmdidx)
>>  #define EXT_CSD_TIMING_LEGACY       0       /* no high speed */
>>  #define EXT_CSD_TIMING_HS   1       /* HS */
>>  #define EXT_CSD_TIMING_HS200        2       /* HS200 */
>>+#define EXT_CSD_TIMING_HS400 3       /* HS400 */
>>  #define EXT_CSD_BOOT_ACK_ENABLE                     (1 << 6)
>>  #define EXT_CSD_BOOT_PARTITION_ENABLE               (1 << 3)
>>@@ -520,6 +527,7 @@ enum bus_mode {
>>      UHS_DDR50,
>>      UHS_SDR104,
>>      MMC_HS_200,
>>+     MMC_HS_400,
>>      MMC_MODES_END
>>  };
>>@@ -533,6 +541,10 @@ static inline bool mmc_is_mode_ddr(enum bus_mode mode)
>>  #if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT)
>>      else if (mode == UHS_DDR50)
>>              return true;
>>+#endif
>>+#if CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
>>+     else if (mode == MMC_HS_400)
>>+             return true;
>>  #endif
>>      else
>>              return false;
>
>_______________________________________________
>U-Boot mailing list
>U-Boot@lists.denx.de
>https://lists.denx.de/listinfo/u-boot

-- 
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to