[2/2] ARM: dts: imx8mm-beacon: add UHS and HS400/HS400ES properties

i.MX8M Mini can provide support for high speed grades in the
usdhc controllers.

On the imx8mm-beacon-kit, the sdhc2 hosts a microSD card, and
sdhc3 hosts an eMMC capable of HS400ES.

In order to toggle this mode in SPL, the reg_usdhc2_vmmc must
be available in SPL. Add all these flags to
imx8mm-beacon-kit-u-boot.dtsi

Suggested-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com>
Signed-off-by: Adam Ford <aford173@gmail.com>

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?

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/50b1a69cee0dc06d0c713a5a978998f2b4a9cb31

adam

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.

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

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-aford173@gmail.com/

Hi Adam,

On Wed, Dec 30, 2020 at 2:44 PM Adam Ford <aford173@gmail.com> wrote:

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

Thanks for investigating and providing a patch. Appreciated!