Re: [PATCH v1 1/1] ARM: dts: colibri: introduce dts with UHS-I support enabled

[Marcel Ziswiler]

Hi Igor

Sorry, for my late reply but this one got stuck in my private email.

On Thu, 2019-04-04 at 11:19 +0200, Igor Opaniuk wrote:
> Introduce DTS for Colibri iMX6DL with proper configuration for VGEN3,
> which allows that rail to be automatically switched to 1.8 volts for
> proper UHS-I operation mode.

In general, this looks very good. However, thinking some more about the
whole thing I see two issues: the UHS-I feature was not present on
V1.0x Colibri iMX6 modules but got only added later as part of the
V1.1x re-design [1]. Maybe it would therefore make more sense to name
it accordingly e.g. imx6dl-colibri-v1.1.dtsi et. al. similar to what I
did for Apalis T30 V1.1x [2]. Whether or not to keep the uhs postfix is
a good question but me personally I would just drop it. Another issue
are the 3.3 volt pull-ups on all our Colibri carrier boards. While
those seem not to cause any issues with most any SD resp. micro SD card
we do know that SDIO devices are quite sensitive in that regard and may
fail in various ways unless they get removed. I would at least add a
note indicating that the pull-ups on our carrier boards may still need
removing for fully compliant operation.

[1] 
https://developer.toradex.com/products/colibri-imx6#revision-history
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=b57d6b996ebe25e7f1e92de0abc7a2da42005454

> Signed-off-by: Igor Opaniuk <igor.opaniuk@xxxxxxxxxxx>
> ---
>  arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts  | 245 +---------------
> --
>  arch/arm/boot/dts/imx6dl-colibri-eval-v3.dtsi | 213 +++++++++++++++
>  .../boot/dts/imx6dl-colibri-uhs-eval-v3.dts   |  29 +++
>  arch/arm/boot/dts/imx6qdl-colibri.dtsi        |  36 ++-
>  4 files changed, 278 insertions(+), 245 deletions(-)
>  create mode 100644 arch/arm/boot/dts/imx6dl-colibri-eval-v3.dtsi
>  create mode 100644 arch/arm/boot/dts/imx6dl-colibri-uhs-eval-v3.dts
> 
> diff --git a/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> b/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> index 9a5d6c94cca4..14d7f359d8d6 100644
> --- a/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> +++ b/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> @@ -1,258 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0 OR X11
>  /*
> - * Copyright 2014-2016 Toradex AG
> - * Copyright 2012 Freescale Semiconductor, Inc.
> - * Copyright 2011 Linaro Ltd.
> - *
> - * This file is dual-licensed: you can use it either under the terms
> - * of the GPL or the X11 license, at your option. Note that this
> dual
> - * licensing only applies to this file, and not this project as a
> - * whole.
> - *
> - *  a) This file is free software; you can redistribute it and/or
> - *     modify it under the terms of the GNU General Public License
> - *     version 2 as published by the Free Software Foundation.
> - *
> - *     This file is distributed in the hope that it will be useful,
> - *     but WITHOUT ANY WARRANTY; without even the implied warranty
> of
> - *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *     GNU General Public License for more details.
> - *
> - * Or, alternatively,
> - *
> - *  b) Permission is hereby granted, free of charge, to any person
> - *     obtaining a copy of this software and associated
> documentation
> - *     files (the "Software"), to deal in the Software without
> - *     restriction, including without limitation the rights to use,
> - *     copy, modify, merge, publish, distribute, sublicense, and/or
> - *     sell copies of the Software, and to permit persons to whom
> the
> - *     Software is furnished to do so, subject to the following
> - *     conditions:
> - *
> - *     The above copyright notice and this permission notice shall
> be
> - *     included in all copies or substantial portions of the
> Software.
> - *
> - *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> - *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
> WARRANTIES
> - *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> - *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> - *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> - *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> - *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> - *     OTHER DEALINGS IN THE SOFTWARE.
> + * Copyright 2019 Toradex AG
>   */
>  
>  /dts-v1/;
>  
> -#include <dt-bindings/input/input.h>
> -#include <dt-bindings/interrupt-controller/irq.h>
> -#include "imx6dl.dtsi"
> -#include "imx6qdl-colibri.dtsi"
> +#include "imx6dl-colibri-eval-v3.dtsi"
>  
>  / {
>  	model = "Toradex Colibri iMX6DL/S on Colibri Evaluation Board
> V3";
>  	compatible = "toradex,colibri_imx6dl-eval-v3",
> "toradex,colibri_imx6dl",
>  		     "fsl,imx6dl";
> -
> -	/* Will be filled by the bootloader */
> -	memory@10000000 {
> -		device_type = "memory";
> -		reg = <0x10000000 0>;
> -	};
> -
> -	aliases {
> -		i2c0 = &i2c2;
> -		i2c1 = &i2c3;
> -	};
> -
> -	aliases {
> -		rtc0 = &rtc_i2c;
> -		rtc1 = &snvs_rtc;
> -	};
> -
> -	chosen {
> -		stdout-path = "serial0:115200n8";
> -	};
> -
> -	/* Fixed crystal dedicated to mcp251x */
> -	clk16m: clock-16m {
> -		compatible = "fixed-clock";
> -		#clock-cells = <0>;
> -		clock-frequency = <16000000>;
> -		clock-output-names = "clk16m";
> -	};
> -
> -	gpio-keys {
> -		compatible = "gpio-keys";
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&pinctrl_gpio_keys>;
> -
> -		wakeup {
> -			label = "Wake-Up";
> -			gpios = <&gpio2 22 GPIO_ACTIVE_HIGH>; /* SODIMM
> 45 */
> -			linux,code = <KEY_WAKEUP>;
> -			debounce-interval = <10>;
> -			wakeup-source;
> -		};
> -	};
> -
> -	lcd_display: disp0 {
> -		compatible = "fsl,imx-parallel-display";
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -		interface-pix-fmt = "bgr666";
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&pinctrl_ipu1_lcdif>;
> -		status = "okay";
> -
> -		port@0 {
> -			reg = <0>;
> -
> -			lcd_display_in: endpoint {
> -				remote-endpoint = <&ipu1_di0_disp0>;
> -			};
> -		};
> -
> -		port@1 {
> -			reg = <1>;
> -
> -			lcd_display_out: endpoint {
> -				remote-endpoint = <&lcd_panel_in>;
> -			};
> -		};
> -	};
> -
> -	panel: panel {
> -		/*
> -		 * edt,et057090dhu: EDT 5.7" LCD TFT
> -		 * edt,et070080dh6: EDT 7.0" LCD TFT
> -		 */
> -		compatible = "edt,et057090dhu";
> -		backlight = <&backlight>;
> -
> -		port {
> -			lcd_panel_in: endpoint {
> -				remote-endpoint = <&lcd_display_out>;
> -			};
> -		};
> -	};
> -};
> -
> -&backlight {
> -	brightness-levels = <0 127 191 223 239 247 251 255>;
> -	default-brightness-level = <1>;
> -	status = "okay";
> -};
> -
> -/* Colibri SSP */
> -&ecspi4 {
> -	status = "okay";
> -
> -	mcp251x0: mcp251x@0 {
> -		compatible = "microchip,mcp2515";
> -		reg = <0>;
> -		clocks = <&clk16m>;
> -		interrupt-parent = <&gpio3>;
> -		interrupts = <27 0x2>;
> -		spi-max-frequency = <10000000>;
> -		status = "okay";
> -	};
> -};
> -
> -&hdmi {
> -	status = "okay";
> -};
> -
> -/*
> - * Colibri I2C: I2C3_SDA/SCL on SODIMM 194/196 (e.g. RTC on carrier
> board)
> - */
> -&i2c3 {
> -	status = "okay";
> -
> -	/* M41T0M6 real time clock on carrier board */
> -	rtc_i2c: rtc@68 {
> -		compatible = "st,m41t0";
> -		reg = <0x68>;
> -	};
> -};
> -
> -&ipu1_di0_disp0 {
> -	remote-endpoint = <&lcd_display_in>;
> -};
> -
> -&pwm1 {
> -	status = "okay";
> -};
> -
> -&pwm2 {
> -	status = "okay";
> -};
> -
> -&pwm3 {
> -	status = "okay";
> -};
> -
> -&pwm4 {
> -	status = "okay";
> -};
> -
> -&reg_usb_host_vbus {
> -	status = "okay";
> -};
> -
> -&uart1 {
> -	status = "okay";
> -};
> -
> -&uart2 {
> -	status = "okay";
> -};
> -
> -&uart3 {
> -	status = "okay";
> -};
> -
> -&usbh1 {
> -	vbus-supply = <&reg_usb_host_vbus>;
> -	status = "okay";
> -};
> -
> -&usbotg {
> -	status = "okay";
>  };
>  
>  /* Colibri MMC */
>  &usdhc1 {
>  	status = "okay";
>  };
> -
> -&weim {
> -	status = "okay";
> -
> -	/* weim memory map: 32MB on CS0, CS1, CS2 and CS3 */
> -	ranges = <0 0 0x08000000 0x02000000
> -		  1 0 0x0a000000 0x02000000
> -		  2 0 0x0c000000 0x02000000
> -		  3 0 0x0e000000 0x02000000>;
> -
> -	/* SRAM on Colibri nEXT_CS0 */
> -	sram@0,0 {
> -		compatible = "cypress,cy7c1019dv33-10zsxi, mtd-ram";
> -		reg = <0 0 0x00010000>;
> -		#address-cells = <1>;
> -		#size-cells = <1>;
> -		bank-width = <2>;
> -		fsl,weim-cs-timing = <0x00010081 0x00000000 0x04000000
> -				      0x00000000 0x04000040
> 0x00000000>;
> -	};
> -
> -	/* SRAM on Colibri nEXT_CS1 */
> -	sram@1,0 {
> -		compatible = "cypress,cy7c1019dv33-10zsxi, mtd-ram";
> -		reg = <1 0 0x00010000>;
> -		#address-cells = <1>;
> -		#size-cells = <1>;
> -		bank-width = <2>;
> -		fsl,weim-cs-timing = <0x00010081 0x00000000 0x04000000
> -				      0x00000000 0x04000040
> 0x00000000>;
> -	};
> -};
> diff --git a/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dtsi
> b/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dtsi
> new file mode 100644
> index 000000000000..3e73fcaca057
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dtsi
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0 OR X11

Any newly introduced device tree file should directly be licensed as
GPL-2.0+ OR X11 which is where we like to move all device trees to.

> +/*
> + * Copyright 2014-2016 Toradex AG

The copyright period should be updated.

> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.

I'm not sure whether any such mentioning is really still required.

> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include "imx6dl.dtsi"
> +#include "imx6qdl-colibri.dtsi"
> +
> +/ {
> +	/* Will be filled by the bootloader */
> +	memory@10000000 {
> +		device_type = "memory";
> +		reg = <0x10000000 0>;
> +	};
> +
> +	aliases {
> +		i2c0 = &i2c2;
> +		i2c1 = &i2c3;
> +	};
> +
> +	aliases {
> +		rtc0 = &rtc_i2c;
> +		rtc1 = &snvs_rtc;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	/* Fixed crystal dedicated to mcp251x */
> +	clk16m: clock-16m {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <16000000>;
> +		clock-output-names = "clk16m";
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_keys>;
> +
> +		wakeup {
> +			label = "Wake-Up";
> +			gpios = <&gpio2 22 GPIO_ACTIVE_HIGH>; /* SODIMM
> 45 */
> +			linux,code = <KEY_WAKEUP>;
> +			debounce-interval = <10>;
> +			wakeup-source;
> +		};
> +	};
> +
> +	lcd_display: disp0 {
> +		compatible = "fsl,imx-parallel-display";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		interface-pix-fmt = "bgr666";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ipu1_lcdif>;
> +		status = "okay";
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			lcd_display_in: endpoint {
> +				remote-endpoint = <&ipu1_di0_disp0>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			lcd_display_out: endpoint {
> +				remote-endpoint = <&lcd_panel_in>;
> +			};
> +		};
> +	};
> +
> +	panel: panel {
> +		/*
> +		 * edt,et057090dhu: EDT 5.7" LCD TFT
> +		 * edt,et070080dh6: EDT 7.0" LCD TFT
> +		 */
> +		compatible = "edt,et057090dhu";
> +		backlight = <&backlight>;
> +
> +		port {
> +			lcd_panel_in: endpoint {
> +				remote-endpoint = <&lcd_display_out>;
> +			};
> +		};
> +	};
> +};
> +
> +&backlight {
> +	brightness-levels = <0 127 191 223 239 247 251 255>;
> +	default-brightness-level = <1>;
> +	status = "okay";
> +};
> +
> +/* Colibri SSP */
> +&ecspi4 {
> +	status = "okay";
> +
> +	mcp251x0: mcp251x@0 {
> +		compatible = "microchip,mcp2515";
> +		reg = <0>;
> +		clocks = <&clk16m>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <27 0x2>;
> +		spi-max-frequency = <10000000>;
> +		status = "okay";
> +	};
> +};
> +
> +&hdmi {
> +	status = "okay";
> +};
> +
> +/*
> + * Colibri I2C: I2C3_SDA/SCL on SODIMM 194/196 (e.g. RTC on carrier
> board)
> + */
> +&i2c3 {
> +	status = "okay";
> +
> +	/* M41T0M6 real time clock on carrier board */
> +	rtc_i2c: rtc@68 {
> +		compatible = "st,m41t0";
> +		reg = <0x68>;
> +	};
> +};
> +
> +&ipu1_di0_disp0 {
> +	remote-endpoint = <&lcd_display_in>;
> +};
> +
> +&pwm1 {
> +	status = "okay";
> +};
> +
> +&pwm2 {
> +	status = "okay";
> +};
> +
> +&pwm3 {
> +	status = "okay";
> +};
> +
> +&pwm4 {
> +	status = "okay";
> +};
> +
> +&reg_usb_host_vbus {
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	status = "okay";
> +};
> +
> +&uart2 {
> +	status = "okay";
> +};
> +
> +&uart3 {
> +	status = "okay";
> +};
> +
> +&usbh1 {
> +	vbus-supply = <&reg_usb_host_vbus>;
> +	status = "okay";
> +};
> +
> +&usbotg {
> +	status = "okay";
> +};
> +
> +&weim {
> +	status = "okay";
> +
> +	/* weim memory map: 32MB on CS0, CS1, CS2 and CS3 */
> +	ranges = <0 0 0x08000000 0x02000000
> +		  1 0 0x0a000000 0x02000000
> +		  2 0 0x0c000000 0x02000000
> +		  3 0 0x0e000000 0x02000000>;
> +
> +	/* SRAM on Colibri nEXT_CS0 */
> +	sram@0,0 {
> +		compatible = "cypress,cy7c1019dv33-10zsxi, mtd-ram";
> +		reg = <0 0 0x00010000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		bank-width = <2>;
> +		fsl,weim-cs-timing = <0x00010081 0x00000000 0x04000000
> +				      0x00000000 0x04000040
> 0x00000000>;
> +	};
> +
> +	/* SRAM on Colibri nEXT_CS1 */
> +	sram@1,0 {
> +		compatible = "cypress,cy7c1019dv33-10zsxi, mtd-ram";
> +		reg = <1 0 0x00010000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		bank-width = <2>;
> +		fsl,weim-cs-timing = <0x00010081 0x00000000 0x04000000
> +				      0x00000000 0x04000040
> 0x00000000>;
> +	};
> +};
> diff --git a/arch/arm/boot/dts/imx6dl-colibri-uhs-eval-v3.dts
> b/arch/arm/boot/dts/imx6dl-colibri-uhs-eval-v3.dts
> new file mode 100644
> index 000000000000..9a18b5c70752
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6dl-colibri-uhs-eval-v3.dts
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0 OR X11

Ditto.

> +/*
> + * Copyright 2019 Toradex AG

Perfect.

> + */
> +
> +/dts-v1/;
> +
> +#include "imx6dl-colibri-eval-v3.dtsi"
> +
> +/ {
> +	model = "Toradex Colibri iMX6DL/S on Colibri Ev. Board V3 with
> UHS-I";
> +	compatible = "toradex,colibri_imx6dl-eval-v3",
> "toradex,colibri_imx6dl",
> +		     "fsl,imx6dl";
> +};
> +
> +/* Colibri MMC with UHS-I support*/
> +&usdhc1 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc1 &pinctrl_mmc_cd>;
> +	pinctrl-1 = <&pinctrl_usdhc1_100mhz &pinctrl_mmc_cd>;
> +	pinctrl-2 = <&pinctrl_usdhc1_200mhz &pinctrl_mmc_cd>;
> +	vqmmc-supply = <&vgen3_reg>;
> +	sd-uhs-sdr12;
> +	sd-uhs-sdr25;
> +	sd-uhs-sdr50;
> +	sd-uhs-sdr104;

I'm not quite sure whether this is the right approach. As in theory we
should now just support whatever the i.MX 6DL/S SoC supports and
nothing else special really. At least on the Tegras this was
sufficient. Maybe somebody with more experience concerning this on i.MX
6 may comment.

> +	status = "okay";
> +	/delete-property/no-1-8-v;
> +};
> diff --git a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> index 1beac22266ed..8eed89634a45 100644
> --- a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
> @@ -215,7 +215,12 @@
>  				regulator-always-on;
>  			};
>  
> -			/* vgen3: unused */

I would add a comment mentioning the name of this rail as used in the
hardware schematics: +V3.3_1.8_SD1 coming off VGEN3 (OK, that part may
be omitted as that is where we are here) and supplying the i.MX 6
NVCC_SD1.

> +			vgen3_reg: vgen3 {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-boot-on;

This is probably appropriate resp. as a matter of fact our PMICs should
even get fused to have VGEN3 default to 3.3 volts being enabled.

> +				regulator-always-on;

This one may prevent any kind of power saving. Looking at the
schematics the regular Colibri MMC card detect pin is indeed on a
different always-on rail so this one should really be disengageable
without losing any card detection resp. wake-up capabilities.

> +			};
>  
>  			vgen4_reg: vgen4 {
>  				regulator-min-microvolt = <1800000>;
> @@ -385,12 +390,15 @@
>  &usdhc1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_usdhc1 &pinctrl_mmc_cd>;
> +	no-1-8-v;
>  	cd-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; /* MMCD */
>  	disable-wp;
> -	vqmmc-supply = <&reg_module_3v3>;
> +	enable-sdio-wakeup;
> +	keep-power-in-suspend;
>  	bus-width = <4>;
> -	no-1-8-v;
>  	status = "disabled";
> +	cap-sd-highspeed;
> +	vmmc-supply = <&reg_module_3v3>;

I would keep an alphabetical order of device tree properties maybe with
the exception of keeping pinctrl on top and status on the bottom as
generally done.

>  };
>  
>  /* eMMC */
> @@ -698,6 +706,28 @@
>  		>;
>  	};
>  
> +	pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
> +		fsl,pins = <
> +			MX6QDL_PAD_SD1_CMD__SD1_CMD    0x170b1
> +			MX6QDL_PAD_SD1_CLK__SD1_CLK    0x100b1
> +			MX6QDL_PAD_SD1_DAT0__SD1_DATA0 0x170b1
> +			MX6QDL_PAD_SD1_DAT1__SD1_DATA1 0x170b1
> +			MX6QDL_PAD_SD1_DAT2__SD1_DATA2 0x170b1
> +			MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x170b1
> +		>;
> +	};
> +
> +	pinctrl_usdhc1_200mhz: usdhc1grp200mhz {
> +		fsl,pins = <
> +			MX6QDL_PAD_SD1_CMD__SD1_CMD    0x170f1
> +			MX6QDL_PAD_SD1_CLK__SD1_CLK    0x100f1
> +			MX6QDL_PAD_SD1_DAT0__SD1_DATA0 0x170f1
> +			MX6QDL_PAD_SD1_DAT1__SD1_DATA1 0x170f1
> +			MX6QDL_PAD_SD1_DAT2__SD1_DATA2 0x170f1
> +			MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x170f1
> +		>;
> +	};
> +

I would really add those directly underneath pinctrl_usdhc1 rather than
pinctrl_usdhc3.

>  	pinctrl_weim_cs0: weimcs0grp {
>  		fsl,pins = <
>  			/* nEXT_CS0 */

Cheers

Marcel