Hi Alexandru,
Sent: mercredi 9 septembre 2020 23:54 To: uboot-stm32@st-md-mailman.stormreply.com Cc: Alexandru Gagniuc mr.nuke.me@gmail.com; Patrick DELAUNAY patrick.delaunay@st.com; Patrice CHOTARD patrice.chotard@st.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de Subject: [PATCH] mmc: stm32_sdmmc2: Use mmc_of_parse() to read host capabilities Importance: High
mmc_of_parse() can populate the 'f_max' and 'host_caps' fields of struct mmc_config from devicetree. The same logic is duplicated in stm32_sdmmc2_probe(). Use mmc_of_parse(), which is more generic.
Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com
Thanks for the patch, I miss the addition of this new API.
drivers/mmc/stm32_sdmmc2.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/stm32_sdmmc2.c b/drivers/mmc/stm32_sdmmc2.c index 6d50356217..77871d5afc 100644 --- a/drivers/mmc/stm32_sdmmc2.c +++ b/drivers/mmc/stm32_sdmmc2.c @@ -676,27 +676,13 @@ static int stm32_sdmmc2_probe(struct udevice *dev) GPIOD_IS_IN);
cfg->f_min = 400000;
- cfg->f_max = dev_read_u32_default(dev, "max-frequency", 52000000); cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34 |
MMC_VDD_165_195; cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT; cfg->name = "STM32 SD/MMC";
cfg->host_caps = 0;
- if (cfg->f_max > 25000000)
cfg->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;- switch (dev_read_u32_default(dev, "bus-width", 1)) {
- case 8:
cfg->host_caps |= MMC_MODE_8BIT;/* fall through */- case 4:
cfg->host_caps |= MMC_MODE_4BIT;break;- case 1:
break;- default:
pr_err("invalid \"bus-width\" property, force to 1\n");- }
- cfg->f_max = 52000000;
- mmc_of_parse(dev, cfg);
Result of mmc_of_parse is not tested ?
I proposed:
+ ret = mmc_of_parse(dev, cfg); + if (ret) + return ret;
upriv->mmc = &plat->mmc;
-- 2.25.4
I test this patch with a trace in stm32_sdmmc2_probe on STM32MP157C-EV1
Before the patch : cfg->host_caps = 0x2000000e (SD card) cfg->host_caps = 0x6000000e (eMMC)
After the patch: cfg->host_caps = 0x300001e6 (SD card) cfg->host_caps = 0x70004006 (eMMC)
I see 2 problem here :
1/ MMC_MODE_HS_52MHz = MMC_CAP(MMC_HS_52) is removed and the eMMC on EV1 use only at 26MHz instead 52MHz before the patch
Mode: MMC High Speed (52MHz) => Mode: MMC High Speed (26MHz)
The "cap-mmc-highspeed" is used in Linux for MMC HS at 26MHz or 52MHz
./Documentation/devicetree/bindings/mmc/mmc-controller.yaml:122: cap-mmc-highspeed: $ref: /schemas/types.yaml#/definitions/flag description: MMC high-speed timing is supported.
And in Linux mmc/core/mmc.c static void mmc_select_card_type(struct mmc_card *card) { .....
if (caps & MMC_CAP_MMC_HIGHSPEED && card_type & EXT_CSD_CARD_TYPE_HS_26) { hs_max_dtr = MMC_HIGH_26_MAX_DTR; avail_type |= EXT_CSD_CARD_TYPE_HS_26; }
if (caps & MMC_CAP_MMC_HIGHSPEED && card_type & EXT_CSD_CARD_TYPE_HS_52) { hs_max_dtr = MMC_HIGH_52_MAX_DTR; avail_type |= EXT_CSD_CARD_TYPE_HS_52; } ....
include/linux/mmc/mmc.h:347: #define EXT_CSD_CARD_TYPE_HS_26 (1<<0) /* Card can run at 26MHz */ #define EXT_CSD_CARD_TYPE_HS_52 (1<<1) /* Card can run at 52MHz */
I think that it is a issue U-Boot, in mmc uclass in mmc_of_parse():
if (dev_read_bool(dev, "cap-mmc-highspeed")) - cfg->host_caps |= MMC_CAP(MMC_HS); + cfg->host_caps |= MMC_CAP(MMC_HS) | MMC_CAP(MMC_HS_52);
In U-Boot this capability is splitted in 2 bits = one for 26MHz one for 52MHz And for card side it is managed on card side by mmc_get_capabilities()
include/mmc.h:254: #define EXT_CSD_CARD_TYPE_26 (1 << 0) /* Card can run at 26MHz */ #define EXT_CSD_CARD_TYPE_52 (1 << 1) /* Card can run at 52MHz */
A other solution is to only impact the sdmmc driver..... and to activate 52MHZ mode support is frequency is >= 26MHz
cfg->f_max = 52000000; ret = mmc_of_parse(dev, cfg); if (ret) return ret;
if ((cfg->host_caps & MMC_HS) && cfg->f_max > 26000000) cfg->host_caps |= MMC_HS_52;
but I prefer the previous generic solution in u-class.
2) some host caps can be added in device tree (they are supported by SOC and by Linux driver) but they are not supported by U-Boot driver, for example:
#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_HS400_ES MMC_CAP(MMC_HS_400_ES)
MMC_CAP(UHS_SDR12) MMC_CAP(UHS_SDR25) MMC_CAP(UHS_SDR50) MMC_CAP(UHS_SDR104) MMC_CAP(UHS_DDR50)
I afraid (I don't sure) that this added caps change the mmc core behavior in U-Boot = U-Boot try to select the high speed mode even if not supported by driver....
The host_caps bitfield should be limited by the supported features in the driver (or remove the unsupported features)
#define SDMMC_SUPPORTED_CAPS (MMC_MODE_1BIT | MMC_MODE_4BIT | MMC_MODE_8BIT | MMC_CAP_CD_ACTIVE_HIGH | MMC_CAP_NEEDS_POLL | MMC_CAP_NONREMOVABLE | MMC_MODE_HS | MMC_MODE_HS_52MHz) or #define SDMMC_UNSUPPORTED_CAPS (MMC_MODE_DDR_52MHz | MMC_MODE_HS200 | MMC_MODE_HS400 | MMC_MODE_HS400_ES | UHS_CAPS)
cfg->f_max = 52000000; ret = mmc_of_parse(dev, cfg); if (ret) return ret;
cfg->host_caps &= SDMMC_SUPPORTED_CAPS; or cfg->host_caps |= ~SDMMC_UNSUPPORTED_CAPS;
Best Regards
Patrick