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

[Alex G.]

On 9/10/20 11:04 AM, Patrick DELAUNAY wrote:
> Hi Alexandru,

Hi

[snip]

>> +	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;

You're right. I'll get that updated.


> I test this patch with a trace in stm32_sdmmc2_probe on STM32MP157C-EV1
[snip]
> 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()

I agree. I am preparing a patch to address this.


[snip]
> 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)
> 
> 
> 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....

Two issues here. The first is when devicetree enables an unsupported 
mode, say "mmc-hs400-1_2v". That's a devicetree bug, and not something 
we should fix in the code. Hypothetical exam: DT says
	bus-width = <8>;
but only four lines are routed on the board.

The second issue is what happens when somebody plugs in a UHS SD card? 
UHS support is not enabled by default in the stm32mp1 defconfigs, so the 
mmc core won't try to run it at UHS.

Now if somebody were to enable UHS manually:
	CONFIG_MMC_IO_VOLTAGE=y
	CONFIG_MMC_UHS_SUPPORT=y
Then yes, the UHS switch will be attempted, fail, and the card will not 
be detected.

To work around that, one could implement a .wait_dat0() that returns an 
error other than -ENOSYS. This would cause mmc_switch_voltage() to fail, 
making the mmc core to leave the card at default speeds.

My argument is that it takes conscious effort to break things in the 
first place, so it's not a situation we should worry about.


> The host_caps bitfield should be limited by the supported features in the driver  (or remove the unsupported features)
[snip]
> 	cfg->host_caps &= SDMMC_SUPPORTED_CAPS;
> or
> 	cfg->host_caps |= ~SDMMC_UNSUPPORTED_CAPS;

I think this sort of playing with the flags will cause more confusion. 
People would expect this to come from DT. For example, somebody sets 
"sd-uhs-ddr52" in devicetree. It's more confusing to check host_caps, 
and find that MMC_MODE_DDR_52MHz is not set than it is to see the driver 
trying to place the card in DDR52 and failing.

Alex