On Wed, Dec 30, 2020 at 8:22 AM Fabio Estevam festevam@gmail.com wrote:

Hi Adam,

On Wed, Dec 30, 2020 at 11:14 AM Adam Ford aford173@gmail.com wrote:

...

&usdhc2 { u-boot,dm-spl;

•   sd-uhs-sdr104;
  

•   sd-uhs-ddr50;
  

Shouldn't these properties be part of the main dtsi instead of adding them to the u-boot.dtsi one?

Linux doesn't appear to need it, and no 64-bit imx boards in Linux appear to use them. Some layerscape boards appear to use these flags. If it's not necessary for Linux, but it is necessary for U-Boot, it seems like the -u-boot.dtsi file is the right place to put it. It's consistent with that was done for the imx8mm-evk-u-boot.dtsi and others [1]

[1] - https://gitlab.denx.de/u-boot/u-boot/-/commit/50b1a69cee0dc06d0c713a5a978998...

adam

On Wed, Dec 30, 2020 at 9:24 AM Fabio Estevam festevam@gmail.com wrote:

On Wed, Dec 30, 2020 at 11:45 AM Adam Ford aford173@gmail.com wrote:

...

Linux doesn't appear to need it, and no 64-bit imx boards in Linux appear to use them. Some layerscape boards appear to use these flags. If it's not necessary for Linux, but it is necessary for U-Boot, it seems like the -u-boot.dtsi file is the right place to put it. It's consistent with that was done for the imx8mm-evk-u-boot.dtsi and others [1]

IMHO the device trees should be the same for Linux and U-Boot as much as we can.

Of course, it is OK to add custom U-Boot properties, such as u-boot,dm-spl inside -u-boot.dtsi file, but adding MMC related properties only in U-Boot dtsi does not seem correct.

It would be nice if someone could investigate what is missing in the U-Boot sdhc driver to handle these modes by default.

U-Boot has a structure to define the capabilities of the usdhc controller.

static struct esdhc_soc_data usdhc_imx8qm_data = { .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200 | ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES, };

This is referenced from a variety of compatible flags, including the imx8mm.

It looks like the code is missing the checks for all of them except ESDHC_FLAG_USDHC and ESDHC_FLAG_STD_TUNING. The flags get copied from data->flags to priv->flags, but we check the flags and convert them to host capabilities. I think these flags need to somehow get checked and then converted to cfg->host_caps. It gets more complicated, because the flags would need to get cleared if the pinmux doesn't have 100MHz and 200MHz options available to it.

By comparison, the Linux code appears to handle the various flags.

In the meantime, would you be opposed to this patch since other imx8m kits have the equivalent patch already applied? They could be removed if/when the esdhc driver gets updated.

adam

On Wed, Dec 30, 2020 at 10:34 AM Fabio Estevam festevam@gmail.com wrote:

On Wed, Dec 30, 2020 at 1:29 PM Adam Ford aford173@gmail.com wrote:

...

In the meantime, would you be opposed to this patch since other imx8m kits have the equivalent patch already applied? They could be removed if/when the esdhc driver gets updated.

I am not opposed to this patch, but would be nice if the U-Boot esdhc driver could be changed to behave like the kernel one.

I just sent an RFC [1] for change that uses the esdhc_soc_data flags to determine whether or not the function/feature is present in the controller and checks to see if the function is enabled from Kconfig.

I was able to remove the u-boot.dtsi changes that I previously pushed, and I was able to see that the eMMC device is running at HS400_ES (200MHz)

u-boot=> mmc dev 2 switch to partitions #0, OK mmc2(part 0) is current device u-boot=> mmc info Device: FSL_SDHC Manufacturer ID: 45 OEM: 100 Name: DG403 Bus Speed: 200000000 Mode: HS400ES (200MHz) Rd Block Len: 512 MMC version 5.1 High Capacity: Yes Capacity: 29.1 GiB Bus Width: 8-bit DDR Erase Group Size: 512 KiB HC WP Group Size: 8 MiB User Capacity: 29.1 GiB WRREL Boot Capacity: 4 MiB ENH RPMB Capacity: 4 MiB ENH Boot area 0 is not write protected Boot area 1 is not write protected u-boot=>

[1] - https://patchwork.ozlabs.org/project/uboot/patch/20201230173907.2891555-1-af...