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

From: Marcel Ziswiler
Date: Mon May 13 2019 - 06:54:14 EST

Next message: Tobin C. Harding: "Re: [PATCH 2/2] fs: btrfs: Don't leak memory when failing add fsid"
Previous message: Serge Semin: "Re: net: phy: realtek: regression, kernel null pointer dereference"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Igor

On Mon, 2019-05-06 at 11:51 +0000, Igor Opaniuk wrote:
> 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";
> > > -};
> > > -
> > > -&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.
> 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?

Yeah, maybe not on the renames but definitely on new ones.

> > > + */
> > > +
> > > +/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.
>
> 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.

I remember that Stefan also messed with this at one point. However, I
did not closely follow the discussion. For me it just seems wrong
having to add any such in every board's device tree when all we do
support is just whatever the SoC supports.

> > > + 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 = <&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.
> 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.

You are very welcome. And sorry again for me delaying this.

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

Best regards - Mit freundlichen GrÃssen - Meilleures salutations

Marcel Ziswiler
Platform Manager Embedded Linux

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500 48 00
(main line) | Direct: +41 41 500 48 10

Next message: Tobin C. Harding: "Re: [PATCH 2/2] fs: btrfs: Don't leak memory when failing add fsid"
Previous message: Serge Semin: "Re: net: phy: realtek: regression, kernel null pointer dereference"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]