[PATCH] mmc: stm32_sdmmc2: Use mmc_of_parse() to read host capabilities

[Patrick DELAUNAY]

Hi Alexandru,

> Sent: mercredi 9 septembre 2020 23:54
> To: uboot-stm32 at st-md-mailman.stormreply.com
> Cc: Alexandru Gagniuc <mr.nuke.me at gmail.com>; Patrick DELAUNAY
> <patrick.delaunay at st.com>; Patrice CHOTARD <patrice.chotard at st.com>; Peng
> Fan <peng.fan at nxp.com>; u-boot at 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 at 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