Hi Simon,

On 15/05/2017 05:28, Simon Glass wrote:
On 12 May 2017 at 12:16, Jean-Jacques Hiblot <jjhib...@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 <jjhib...@ti.com>
---
  drivers/mmc/mmc.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
  include/mmc.h     | 34 ++++++++++++++++++++++++++++------
  2 files changed, 74 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass <s...@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


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

Reply via email to