Hi Marcel, On Thu, Apr 25, 2019 at 11:47 AM Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx> wrote: > > 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. Ok, will do! > > [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"; > > -}; > > - > > -®_usb_host_vbus { > > - status = "okay"; > > -}; > > - > > -&uart1 { > > - status = "okay"; > > -}; > > - > > -&uart2 { > > - status = "okay"; > > -}; > > - > > -&uart3 { > > - status = "okay"; > > -}; > > - > > -&usbh1 { > > - vbus-supply = <®_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. Will fix > > > + * Copyright 2012 Freescale Semiconductor, Inc. > > + * Copyright 2011 Linaro Ltd. > > I'm not sure whether any such mentioning is really still required. Regarding all your three comments above, this file was just renamed from imx6dl-colibri-eval-v3.dts to imx6dl-colibri-eval-v3.dtsi, that's why I kept almost everything as it's. The question is should I really remove these copyrights and change the licence? > > > + */ > > + > > +/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"; > > +}; > > + > > +®_usb_host_vbus { > > + status = "okay"; > > +}; > > + > > +&uart1 { > > + status = "okay"; > > +}; > > + > > +&uart2 { > > + status = "okay"; > > +}; > > + > > +&uart3 { > > + status = "okay"; > > +}; > > + > > +&usbh1 { > > + vbus-supply = <®_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. This is what we discussed a while ago, absence of this properties caused this issue with non-working UHS-I in the mainline (although it worked perfectly on our Toradex downstream kernels). I'll cross check all supporting UHS-I modes in iMX6DL/S SoC TRM and will add/remove is something is missing/wrong. > > > + 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. Ok, will fix. > > > + 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 = <®_module_3v3>; > > + enable-sdio-wakeup; > > + keep-power-in-suspend; > > bus-width = <4>; > > - no-1-8-v; > > status = "disabled"; > > + cap-sd-highspeed; > > + vmmc-supply = <®_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. Will fix, thanks! > > > }; > > > > /* 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. Will fix. > > > pinctrl_weim_cs0: weimcs0grp { > > fsl,pins = < > > /* nEXT_CS0 */ > > Cheers > > Marcel Thanks for all your comments, I'll send v2 ASAP. -- Best regards - Freundliche Grüsse - Meilleures salutations Senior Development Engineer, Igor Opaniuk Toradex AG Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500 48 00 (main line)
