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

[Patrick DELAUNAY]

Hi Alex,

> From: Alex G. <mr.nuke.me at gmail.com>
> Sent: jeudi 10 septembre 2020 22:10
> 
> 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.

Thanks;
 
> 
> > 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.

Thanks;
 
> 
> [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.

Yes it is a device tree issue when the mode is not supported by board / SOC.

But high mode is supported by the STM32MP15x SOC but only if additional
Operation are done (IO configuration to support higher speed) 

It is not supported in U-Boot driver (yet ?) but it is supported by Linux driver...

> 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.
> 

Yes  we should have issue only if UHS defconfig was activated
(CONFIG_MMC_UHS_SUPPORT, CONFIG_MMC_HS*_SUPPORT)....

And it is not the case today in stm32*defconfig
And my proposal is protection (over).

> > 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.

- card_caps = CARD capacity

For you
- host_caps is SOC capacity (read from DT)

For me 
- host_caps is SOC capacity (read from DT) AND driver capacity

=> then in mmc u-class, the real capacity is  host_caps | card_caps

I already see this kind of limitation in one driver 
omap_hsmmc.c:1939:		cfg->host_caps &= ~fixups->unsupported_caps;

But I agree it is today an over protection on host_caps and it can be confusing
so you can forget this point....

> Alex

Regards

Patrick