> -----Original Message-----
> From: Jean-Jacques Hiblot [mailto:jjhib...@ti.com]
> Sent: 2018年3月6日 22:35
> To: Peng Fan <van.free...@gmail.com>
> Cc: Peng Fan <peng....@nxp.com>; jh80.ch...@samsung.com;
> sba...@denx.de; u-boot@lists.denx.de; Kishon Vijay Abraham I <kis...@ti.com>
> Subject: Re: [U-Boot] [PATCH 1/2] mmc: add HS400 support
> 
> 
> 
> On 06/03/2018 02:46, Peng Fan wrote:
> > 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".
> Good. That makes it easier.
> Can you add something to remove HS400 from the host capabilities if the
> 8-wires configuration is not available ?

In mmc_select_mode_and_width:
"card_caps &= mmc->host_caps;" I think this line has already done that.
When host_caps does not support 8-wires, the card_caps will be set not
Supporting 8-wires and HS400 mode will be selected.

I think I need remove the MMC_MODE_4BIT from HS400 mode in the
mmc_modes_by_pref array.

Thanks,
Peng.

> 
> >
> >>> + 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.
> OK
> >
> > 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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fli
> >>
> sts.denx.de%2Flistinfo%2Fu-boot&data=02%7C01%7Cpeng.fan%40nxp.com%7C
> c
> >>
> 1f326580e3a487c2b4f08d5836f7587%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C
> >>
> 0%7C0%7C636559437115011742&sdata=l%2BuDMMe8ZWIi2uM7hIIVgAIezjm
> stIuZ66
> >> 6dewGYSjc%3D&reserved=0

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

Reply via email to