Hello Adam, > -----Original Message----- > From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of ZHIZHIKIN Andrey > Sent: Saturday, December 5, 2020 4:09 PM > To: Adam Ford <aford...@gmail.com> > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic > <sba...@denx.de>; Ye Li <ye...@nxp.com> > Subject: RE: [PATCH 2/2] ARM: dts: imx8m: add UHS or HS400/HS400ES properties > > Hello Adam, > > > -----Original Message----- > > From: Adam Ford <aford...@gmail.com> > > Sent: Saturday, December 5, 2020 3:31 PM > > To: ZHIZHIKIN Andrey <andrey.zhizhi...@leica-geosystems.com> > > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic > > <sba...@denx.de>; Ye Li <ye...@nxp.com> > > Subject: Re: [PATCH 2/2] ARM: dts: imx8m: add UHS or HS400/HS400ES > > properties > > > > > > On Sat, Dec 5, 2020 at 8:21 AM ZHIZHIKIN Andrey > > <andrey.zhizhikin@leica- geosystems.com> wrote: > > > > > > Hello Adam, > > > > > > > -----Original Message----- > > > > From: Adam Ford <aford...@gmail.com> > > > > Sent: Saturday, December 5, 2020 1:43 AM > > > > To: ZHIZHIKIN Andrey <andrey.zhizhi...@leica-geosystems.com> > > > > Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Stefano Babic > > > > <sba...@denx.de>; Ye Li <ye...@nxp.com> > > > > Subject: Re: [PATCH 2/2] ARM: dts: imx8m: add UHS or HS400/HS400ES > > > > properties > > > > > > > > On Tue, Dec 1, 2020 at 7:32 AM Andrey Zhizhikin > > > > <andrey.zhizhikin@leica- geosystems.com> wrote: > > > > > > > > > > i.MX8M series provide support for high speed grades in their > > > > > usdhc controllers, which has eMMC and SDHC connected to them. > > > > > > > > > > Enable this support across the entire i.MX8M family by providing > > > > > quirks to usdhc controllers designated by storage media connected to > them. > > > > > > > > > > Signed-off-by: Andrey Zhizhikin > > > > > <andrey.zhizhi...@leica-geosystems.com> > > > > > Cc: Stefano Babic <sba...@denx.de> > > > > > Cc: Ye Li <ye...@nxp.com> > > > > > --- > > > > > arch/arm/dts/fsl-imx8qm-mek-u-boot.dtsi | 3 +++ > > > > > arch/arm/dts/fsl-imx8qxp-mek-u-boot.dtsi | 3 +++ > > > > > arch/arm/dts/imx8mm-evk-u-boot.dtsi | 4 ++++ > > > > > arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi | 4 ++++ > > > > > arch/arm/dts/imx8mp-evk-u-boot.dtsi | 4 ++++ > > > > > arch/arm/dts/imx8mq-evk.dts | 3 +++ > > > > > 6 files changed, 21 insertions(+) > > > > > > > > > > diff --git a/arch/arm/dts/fsl-imx8qm-mek-u-boot.dtsi > > > > > b/arch/arm/dts/fsl-imx8qm-mek-u-boot.dtsi > > > > > index 80d6475b7c..2f86fcce3e 100644 > > > > > --- a/arch/arm/dts/fsl-imx8qm-mek-u-boot.dtsi > > > > > +++ b/arch/arm/dts/fsl-imx8qm-mek-u-boot.dtsi > > > > > @@ -118,8 +118,11 @@ > > > > > > > > > > &usdhc1 { > > > > > u-boot,dm-spl; > > > > > + u-boot,mmc-hs400-1_8v; > > > > > }; > > > > > > > > > > > > > I don't think the "u-boot," prefix is needed. Looking at other > > > > boards' device trees, they don't seem to have this. > > > > > > Correct, I've already realized that and currently working on V3 of the > > > series: > > > > > > > > > > Awesome! > > > > > Thanks a lot for testing it though! > > > > > > > I tried it on the beacon imx8mm-beacon-kit, and it didn't work > > > > until I removed the "u-boot," > > > > > > I believe you had it tested with your patches on top, since it would > > > also require additional config options to be set, which you > > > introduced for > > imx8mm-beacon-kit. > > > > > I wouldn't have even thought about it until I saw your patch, so > > thanks for bringing it to my attention. :-) > > > > > As I see it now - those changes might be useful and applicable to > > > all > > > imx8m* derivatives and I'm thinking to move those binding to base > > > dtsi files > > then. > > > > > > Would there be any objections here? > > > > None from me. > > Great! > > > > > If I understand correctly, we could add a imx8mm-u-boot-dtsi with > > these U-Boot additions, and all imx8mm boards would include them > > instead of modifying each board's u-boot-dtsi. > > I would make this change in the V3 then.
This unfortunately would not work, since build system does not automatically pick up '-uboot.dtsi' if it does not match to the entry in Makefile. Only those DTS file names, which are explicitly present in the Makefile are searched for corresponding '-u-boot.dtsi' to be auto-included. I would have to abandon this idea and go ahead with enabling those bindings only in '-u-boot.dtsi' files which have corresponding .dts files in the tree. That would mean for all boards requiring to have high speed modes enabled - corresponding bindings should be introduced separately in their respective device trees. > > > > > My only concern there is whether or not enabling hs400, hs200 or > > sdh104 on boards that don't have devices compatible with such flags. > > I would expect them to autonegotiate, but I have no way to test it. > > According to the specification, the speed mode should be auto-negotiated so it > should be safe to include those binding across. > > > > > In theory, we could also make those Kconfig options 'imply' HS400, HS200, > > etc. > > automatically in the Kconfig which would eliminate the need to modify > > each defconfig file. Implying that option would allow users to remove > > them in their respective defconfig files if they don't want the feature. > > Looking at the way how high speed modes are enabled in U-Boot, it does require > that both config options and DT bindings are to be turned on, with bindings > providing a fine-grained control over which modes are supported. > > I might look into this part further on, but would rather leave it outside of > this > series. > > > > > adam > > > > > > > > > > > > > With that removed, I can get the following: > > > > > > > > u-boot=> mmc info > > > > Device: FSL_SDHC > > > > Manufacturer ID: 27 > > > > OEM: 5048 > > > > Name: SD32G > > > > Bus Speed: 200000000 > > > > Mode: UHS SDR104 (208MHz) > > > > Rd Block Len: 512 > > > > SD version 3.0 > > > > High Capacity: Yes > > > > Capacity: 29 GiB > > > > Bus Width: 4-bit > > > > Erase Group Size: 512 Bytes > > > > u-boot=> > > > > > > > > and > > > > > > > > u-boot=> mmc info > > > > Device: FSL_SDHC > > > > Manufacturer ID: 45 > > > > OEM: 100 > > > > Name: DG403 > > > > Bus Speed: 200000000 > > > > Mode: HS400 (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=> > > > > > > > > adam > > > > > > > > > &usdhc2 { > > > > > u-boot,dm-spl; > > > > > + u-boot,sd-uhs-sdr104; > > > > > + u-boot,sd-uhs-ddr50; > > > > > }; > > > > > diff --git a/arch/arm/dts/fsl-imx8qxp-mek-u-boot.dtsi > > > > > b/arch/arm/dts/fsl-imx8qxp-mek-u-boot.dtsi > > > > > index 771ab635f1..f4332edac5 100644 > > > > > --- a/arch/arm/dts/fsl-imx8qxp-mek-u-boot.dtsi > > > > > +++ b/arch/arm/dts/fsl-imx8qxp-mek-u-boot.dtsi > > > > > @@ -118,8 +118,11 @@ > > > > > > > > > > &usdhc1 { > > > > > u-boot,dm-spl; > > > > > + u-boot,mmc-hs400-1_8v; > > > > > }; > > > > > > > > > > &usdhc2 { > > > > > u-boot,dm-spl; > > > > > + u-boot,sd-uhs-sdr104; > > > > > + u-boot,sd-uhs-ddr50; > > > > > }; > > > > > diff --git a/arch/arm/dts/imx8mm-evk-u-boot.dtsi > > > > > b/arch/arm/dts/imx8mm-evk-u-boot.dtsi > > > > > index 9f77d3c6ff..67666a08ec 100644 > > > > > --- a/arch/arm/dts/imx8mm-evk-u-boot.dtsi > > > > > +++ b/arch/arm/dts/imx8mm-evk-u-boot.dtsi > > > > > @@ -100,10 +100,14 @@ > > > > > > > > > > &usdhc2 { > > > > > u-boot,dm-spl; > > > > > + u-boot,sd-uhs-sdr104; > > > > > + u-boot,sd-uhs-ddr50; > > > > > }; > > > > > > > > > > &usdhc3 { > > > > > u-boot,dm-spl; > > > > > + u-boot,mmc-hs400-1_8v; > > > > > + u-boot,mmc-hs400-enhanced-strobe; > > > > > }; > > > > > > > > > > &i2c1 { > > > > > diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > > > > > b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > > > > > index 98b0b9891b..e03e635213 100644 > > > > > --- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > > > > > +++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi > > > > > @@ -97,10 +97,14 @@ > > > > > > > > > > &usdhc2 { > > > > > u-boot,dm-spl; > > > > > + u-boot,sd-uhs-sdr104; > > > > > + u-boot,sd-uhs-ddr50; > > > > > }; > > > > > > > > > > &usdhc3 { > > > > > u-boot,dm-spl; > > > > > + u-boot,mmc-hs400-1_8v; > > > > > + u-boot,mmc-hs400-enhanced-strobe; > > > > > }; > > > > > > > > > > &wdog1 { > > > > > diff --git a/arch/arm/dts/imx8mp-evk-u-boot.dtsi > > > > > b/arch/arm/dts/imx8mp-evk-u-boot.dtsi > > > > > index 2452e9175c..0776b24a6e 100644 > > > > > --- a/arch/arm/dts/imx8mp-evk-u-boot.dtsi > > > > > +++ b/arch/arm/dts/imx8mp-evk-u-boot.dtsi > > > > > @@ -126,10 +126,14 @@ > > > > > > > > > > &usdhc2 { > > > > > u-boot,dm-spl; > > > > > + u-boot,sd-uhs-sdr104; > > > > > + u-boot,sd-uhs-ddr50; > > > > > }; > > > > > > > > > > &usdhc3 { > > > > > u-boot,dm-spl; > > > > > + u-boot,mmc-hs400-1_8v; > > > > > + u-boot,mmc-hs400-enhanced-strobe; > > > > > }; > > > > > > > > > > &wdog1 { > > > > > diff --git a/arch/arm/dts/imx8mq-evk.dts > > > > > b/arch/arm/dts/imx8mq-evk.dts index 9663683f69..985e7e7f8b > > > > > 100644 > > > > > --- a/arch/arm/dts/imx8mq-evk.dts > > > > > +++ b/arch/arm/dts/imx8mq-evk.dts > > > > > @@ -291,6 +291,7 @@ > > > > > non-removable; > > > > > no-sd; > > > > > no-sdio; > > > > > + u-boot,mmc-hs400-1_8v; > > > > > status = "okay"; > > > > > }; > > > > > > > > > > @@ -301,6 +302,8 @@ > > > > > pinctrl-2 = <&pinctrl_usdhc2_200mhz>; > > > > > cd-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>; > > > > > vmmc-supply = <®_usdhc2_vmmc>; > > > > > + u-boot,sd-uhs-sdr104; > > > > > + u-boot,sd-uhs-ddr50; > > > > > status = "okay"; > > > > > }; > > > > > > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > -- Andrey > > -- andrey -- andrey