Hi Alexandru,
> Sent: mercredi 9 septembre 2020 23:54
> To: uboot-st...@st-md-mailman.stormreply.com
> Cc: Alexandru Gagniuc <mr.nuke...@gmail.com>; Patrick DELAUNAY
> <patrick.delau...@st.com>; Patrice CHOTARD <patrice.chot...@st.com>; Peng
> Fan <peng....@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...@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
