| Message ID | 20201223110343.126638-5-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini | expand |
On Thu, 24 Dec 2020 at 11:08, Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Thu, Dec 24, 2020 at 2:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On Wed, 23 Dec 2020 at 13:07, Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > On Wed, Dec 23, 2020 at 5:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > > > On Wed, Dec 23, 2020 at 04:33:41PM +0530, Jagan Teki wrote: > > > > > Engicam C.TOUCH 2.0 is an EDIMM compliant general purpose Carrier > > > > > board. > > > > > > > > > > Genaral features: > > > > > - Ethernet 10/100 > > > > > - Wifi/BT > > > > > - USB Type A/OTG > > > > > - Audio Out > > > > > - CAN > > > > > - LVDS panel connector > > > > > > > > > > i.Core MX8M Mini is an EDIMM SoM based on NXP i.MX8M Mini from Engicam. > > > > > > > > > > i.Core MX8M Mini needs to mount on top of this Carrier board for > > > > > creating complete i.Core MX8M Mini C.TOUCH 2.0 board. > > > > > > > > > > Add support for it. > > > > > > > > > > Signed-off-by: Matteo Lisi <matteo.lisi@engicam.com> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > --- > > > > > Changes for v3: > > > > > - don't maintain common nodes and include it, if no feature diff > > > > > Changes for v2: > > > > > - enabled fec1 node > > > > > - updated commit message > > > > > - dropped engicam from filename since it aligned with imx6 engicam > > > > > dts files naming conventions. > > > > > - add i2c nodes > > > > > - fixed v1 comments > > > > > > > > > > arch/arm64/boot/dts/freescale/Makefile | 1 + > > > > > .../dts/freescale/imx8mm-engicam-ctouch2.dtsi | 82 +++++++++++++++++++ > > > > > .../freescale/imx8mm-icore-mx8mm-ctouch2.dts | 21 +++++ > > > > > 3 files changed, 104 insertions(+) > > > > > create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi > > > > > > > > You split some common part to ctouch2.dtsi so it can be reused in > > > > multiple places. I saw so far only one usage, where are the others? > > > > > > To be clear, ctouch2.dtsi not mean for common it is C.TOUCH2 carrier > > > board dtsi. The other carrier is C.TOUCH2 10.1" Open Frame(display), > > > since DSI is not yet mainlined, I didn't add this yet. > > > > If I understand correctly: it is a DTSI which is included only by one > > DTS... and DTS does not have any other nodes. This as well is not the > > This is not mandatory as per my understanding, including exiting DTS > topologies in Mainline. > > There are several places where more than one dtsi has been included, > Simple example of imx8mm tree is It's not the problem of including more than one DTSI. It's the problem of creating fake DTS or DTSI files whose purpose is only to include others. Keep it simple. Don't create unnecessary files. "Entities should not be multiplied without necessity." > > arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dts Which was wrong as well. Don't create unnecessary files. > > /dts-v1/; > > #include "imx8mm.dtsi" > #include "imx8mm-beacon-som.dtsi" > #include "imx8mm-beacon-baseboard.dtsi" > > (SoC dtsi, SoM dtsi, Carrier board dtsi) > > > design which makes any sense. We do not create empty DTS files which > > only include one more DTSI. The contents of > > imx8mm-engicam-ctouch2.dtsi should be directly in > > imx8mm-icore-mx8mm-ctouch2.dts. That's the same problem as with v1 - > > you overcomplicate simple stuff. It really looks like you ignored the > > comments from v1 in multiple places. > > As explained above, the design is pretty much the same as the existing SoM's. > > imx8mm-engicam-ctouch2.dtsi is not just a dtsi file where nodes are > enabled. It has nodes enabled for Carrier board, so keeping nodes > separately will The files represent real devices or their components. So you have a SOM - a DTSI file. You have a carrier board - a DTS file. That's simple design which is mostly followed, unless something over complicated passes the review. > 1. More verbose for which IP's are available in the carrier board No difference when carrier DTSI is the DTS. Exactly the same. > 2. Easy to extend if someone can create another SoM with a similar Carrier. Not really, if they include carrier DTSI they need to override a lot. So usually (including practice - I did it) they *copy* the carrier to create their own design. > > Ie is the whole idea to keep carrier board dtsi and includes them in dts. > > As I suggest, if you can look into px30 you can understand more easily. NAK from my side. I explained my reasoning. You created a fake, empty DTSI which included only other DTSI. After review, you agreed to fix it. However you still create a fake DTS which includes only a DTSI. "Entities should not be multiplied without necessity." Best regards, Krzysztof
On Thu, Dec 24, 2020 at 3:51 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Thu, 24 Dec 2020 at 11:08, Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > On Thu, Dec 24, 2020 at 2:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > On Wed, 23 Dec 2020 at 13:07, Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > > > On Wed, Dec 23, 2020 at 5:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > > > > > On Wed, Dec 23, 2020 at 04:33:41PM +0530, Jagan Teki wrote: > > > > > > Engicam C.TOUCH 2.0 is an EDIMM compliant general purpose Carrier > > > > > > board. > > > > > > > > > > > > Genaral features: > > > > > > - Ethernet 10/100 > > > > > > - Wifi/BT > > > > > > - USB Type A/OTG > > > > > > - Audio Out > > > > > > - CAN > > > > > > - LVDS panel connector > > > > > > > > > > > > i.Core MX8M Mini is an EDIMM SoM based on NXP i.MX8M Mini from Engicam. > > > > > > > > > > > > i.Core MX8M Mini needs to mount on top of this Carrier board for > > > > > > creating complete i.Core MX8M Mini C.TOUCH 2.0 board. > > > > > > > > > > > > Add support for it. > > > > > > > > > > > > Signed-off-by: Matteo Lisi <matteo.lisi@engicam.com> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > > --- > > > > > > Changes for v3: > > > > > > - don't maintain common nodes and include it, if no feature diff > > > > > > Changes for v2: > > > > > > - enabled fec1 node > > > > > > - updated commit message > > > > > > - dropped engicam from filename since it aligned with imx6 engicam > > > > > > dts files naming conventions. > > > > > > - add i2c nodes > > > > > > - fixed v1 comments > > > > > > > > > > > > arch/arm64/boot/dts/freescale/Makefile | 1 + > > > > > > .../dts/freescale/imx8mm-engicam-ctouch2.dtsi | 82 +++++++++++++++++++ > > > > > > .../freescale/imx8mm-icore-mx8mm-ctouch2.dts | 21 +++++ > > > > > > 3 files changed, 104 insertions(+) > > > > > > create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi > > > > > > > > > > You split some common part to ctouch2.dtsi so it can be reused in > > > > > multiple places. I saw so far only one usage, where are the others? > > > > > > > > To be clear, ctouch2.dtsi not mean for common it is C.TOUCH2 carrier > > > > board dtsi. The other carrier is C.TOUCH2 10.1" Open Frame(display), > > > > since DSI is not yet mainlined, I didn't add this yet. > > > > > > If I understand correctly: it is a DTSI which is included only by one > > > DTS... and DTS does not have any other nodes. This as well is not the > > > > This is not mandatory as per my understanding, including exiting DTS > > topologies in Mainline. > > > > There are several places where more than one dtsi has been included, > > Simple example of imx8mm tree is > > It's not the problem of including more than one DTSI. It's the problem > of creating fake DTS or DTSI files whose purpose is only to include > others. Keep it simple. Don't create unnecessary files. "Entities > should not be multiplied without necessity." > > > > > arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dts > > Which was wrong as well. Don't create unnecessary files. > > > > > /dts-v1/; > > > > #include "imx8mm.dtsi" > > #include "imx8mm-beacon-som.dtsi" > > #include "imx8mm-beacon-baseboard.dtsi" > > > > (SoC dtsi, SoM dtsi, Carrier board dtsi) > > > > > design which makes any sense. We do not create empty DTS files which > > > only include one more DTSI. The contents of > > > imx8mm-engicam-ctouch2.dtsi should be directly in > > > imx8mm-icore-mx8mm-ctouch2.dts. That's the same problem as with v1 - > > > you overcomplicate simple stuff. It really looks like you ignored the > > > comments from v1 in multiple places. > > > > As explained above, the design is pretty much the same as the existing SoM's. > > > > imx8mm-engicam-ctouch2.dtsi is not just a dtsi file where nodes are > > enabled. It has nodes enabled for Carrier board, so keeping nodes > > separately will > > The files represent real devices or their components. So you have a > SOM - a DTSI file. You have a carrier board - a DTS file. That's > simple design which is mostly followed, unless something over > complicated passes the review. > > > 1. More verbose for which IP's are available in the carrier board > > No difference when carrier DTSI is the DTS. Exactly the same. > > > 2. Easy to extend if someone can create another SoM with a similar Carrier. > > Not really, if they include carrier DTSI they need to override a lot. > So usually (including practice - I did it) they *copy* the carrier to > create their own design. But what if the new board has slite change to use exiting carrier like what ctouch2 10" OF. Can we add ctouch2 dtsi as a separate file for this case? > > > > > Ie is the whole idea to keep carrier board dtsi and includes them in dts. > > > > As I suggest, if you can look into px30 you can understand more easily. > > NAK from my side. I explained my reasoning. You created a fake, empty > DTSI which included only other DTSI. After review, you agreed to fix > it. However you still create a fake DTS which includes only a DTSI. Not sure. I have updated the series according to comments by dropping -common.dtsi ie what I was thought of "empty DTSI inclusion" you have pointed at previous versions. Jagan.
On Mon, 28 Dec 2020 at 09:21, Jagan Teki <jagan@amarulasolutions.com> wrote: > > > #include "imx8mm.dtsi" > > > #include "imx8mm-beacon-som.dtsi" > > > #include "imx8mm-beacon-baseboard.dtsi" > > > > > > (SoC dtsi, SoM dtsi, Carrier board dtsi) > > > > > > > design which makes any sense. We do not create empty DTS files which > > > > only include one more DTSI. The contents of > > > > imx8mm-engicam-ctouch2.dtsi should be directly in > > > > imx8mm-icore-mx8mm-ctouch2.dts. That's the same problem as with v1 - > > > > you overcomplicate simple stuff. It really looks like you ignored the > > > > comments from v1 in multiple places. > > > > > > As explained above, the design is pretty much the same as the existing SoM's. > > > > > > imx8mm-engicam-ctouch2.dtsi is not just a dtsi file where nodes are > > > enabled. It has nodes enabled for Carrier board, so keeping nodes > > > separately will > > > > The files represent real devices or their components. So you have a > > SOM - a DTSI file. You have a carrier board - a DTS file. That's > > simple design which is mostly followed, unless something over > > complicated passes the review. > > > > > 1. More verbose for which IP's are available in the carrier board > > > > No difference when carrier DTSI is the DTS. Exactly the same. > > > > > 2. Easy to extend if someone can create another SoM with a similar Carrier. > > > > Not really, if they include carrier DTSI they need to override a lot. > > So usually (including practice - I did it) they *copy* the carrier to > > create their own design. > > But what if the new board has slite change to use exiting carrier like > what ctouch2 10" OF. Can we add ctouch2 dtsi as a separate file for > this case? If you submit another DTS using the imx8mm-engicam-ctouch2.dtsi - with its own differences of course (not copying other DTS...) - then having a DTSI makes sense. In current form, still NAK for all the reasons I explained more than once. Best regards, Krzysztof
On Mon, Dec 28, 2020 at 2:04 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Mon, 28 Dec 2020 at 09:21, Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > #include "imx8mm.dtsi" > > > > #include "imx8mm-beacon-som.dtsi" > > > > #include "imx8mm-beacon-baseboard.dtsi" > > > > > > > > (SoC dtsi, SoM dtsi, Carrier board dtsi) > > > > > > > > > design which makes any sense. We do not create empty DTS files which > > > > > only include one more DTSI. The contents of > > > > > imx8mm-engicam-ctouch2.dtsi should be directly in > > > > > imx8mm-icore-mx8mm-ctouch2.dts. That's the same problem as with v1 - > > > > > you overcomplicate simple stuff. It really looks like you ignored the > > > > > comments from v1 in multiple places. > > > > > > > > As explained above, the design is pretty much the same as the existing SoM's. > > > > > > > > imx8mm-engicam-ctouch2.dtsi is not just a dtsi file where nodes are > > > > enabled. It has nodes enabled for Carrier board, so keeping nodes > > > > separately will > > > > > > The files represent real devices or their components. So you have a > > > SOM - a DTSI file. You have a carrier board - a DTS file. That's > > > simple design which is mostly followed, unless something over > > > complicated passes the review. > > > > > > > 1. More verbose for which IP's are available in the carrier board > > > > > > No difference when carrier DTSI is the DTS. Exactly the same. > > > > > > > 2. Easy to extend if someone can create another SoM with a similar Carrier. > > > > > > Not really, if they include carrier DTSI they need to override a lot. > > > So usually (including practice - I did it) they *copy* the carrier to > > > create their own design. > > > > But what if the new board has slite change to use exiting carrier like > > what ctouch2 10" OF. Can we add ctouch2 dtsi as a separate file for > > this case? > > If you submit another DTS using the imx8mm-engicam-ctouch2.dtsi - with > its own differences of course (not copying other DTS...) - then having > a DTSI makes sense. In current form, still NAK for all the reasons I > explained more than once. Okay, thanks for the review. Jagan.
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile index 6f0777ee6cd6..8d49a2c74604 100644 --- a/arch/arm64/boot/dts/freescale/Makefile +++ b/arch/arm64/boot/dts/freescale/Makefile @@ -32,6 +32,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2162a-qds.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mm-beacon-kit.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mm-ddr4-evk.dtb +dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-ctouch2.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mm-kontron-n801x-s.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mm-var-som-symphony.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mn-evk.dtb diff --git a/arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi new file mode 100644 index 000000000000..f7870efd9dab --- /dev/null +++ b/arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (c) 2020 Engicam srl + * Copyright (c) 2020 Amarula Solutions(India) + */ + +&fec1 { + status = "okay"; +}; + +&i2c2 { + clock-frequency = <400000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c2>; + status = "okay"; +}; + +&i2c4 { + clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c4>; + status = "okay"; +}; + +&iomuxc { + pinctrl_i2c2: i2c2grp { + fsl,pins = < + MX8MM_IOMUXC_I2C2_SCL_I2C2_SCL 0x400001c3 + MX8MM_IOMUXC_I2C2_SDA_I2C2_SDA 0x400001c3 + >; + }; + + pinctrl_i2c4: i2c4grp { + fsl,pins = < + MX8MM_IOMUXC_I2C4_SCL_I2C4_SCL 0x400001c3 + MX8MM_IOMUXC_I2C4_SDA_I2C4_SDA 0x400001c3 + >; + }; + + pinctrl_uart2: uart2grp { + fsl,pins = < + MX8MM_IOMUXC_UART2_RXD_UART2_DCE_RX 0x140 + MX8MM_IOMUXC_UART2_TXD_UART2_DCE_TX 0x140 + >; + }; + + pinctrl_usdhc1_gpio: usdhc1gpiogrp { + fsl,pins = < + MX8MM_IOMUXC_GPIO1_IO06_GPIO1_IO6 0x41 + >; + }; + + pinctrl_usdhc1: usdhc1grp { + fsl,pins = < + MX8MM_IOMUXC_SD1_CLK_USDHC1_CLK 0x190 + MX8MM_IOMUXC_SD1_CMD_USDHC1_CMD 0x1d0 + MX8MM_IOMUXC_SD1_DATA0_USDHC1_DATA0 0x1d0 + MX8MM_IOMUXC_SD1_DATA1_USDHC1_DATA1 0x1d0 + MX8MM_IOMUXC_SD1_DATA2_USDHC1_DATA2 0x1d0 + MX8MM_IOMUXC_SD1_DATA3_USDHC1_DATA3 0x1d0 + >; + }; +}; + +&uart2 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart2>; + status = "okay"; +}; + +/* SD */ +&usdhc1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_usdhc1_gpio>; + cd-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>; + max-frequency = <50000000>; + bus-width = <4>; + no-1-8-v; + pm-ignore-notify; + keep-power-in-suspend; + status = "okay"; +}; diff --git a/arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts b/arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts new file mode 100644 index 000000000000..8eb01b1f882a --- /dev/null +++ b/arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (c) 2019 NXP + * Copyright (c) 2019 Engicam srl + * Copyright (c) 2020 Amarula Solutions(India) + */ + +/dts-v1/; +#include "imx8mm.dtsi" +#include "imx8mm-engicam-ctouch2.dtsi" +#include "imx8mm-icore-mx8mm.dtsi" + +/ { + model = "Engicam i.Core MX8M Mini C.TOUCH 2.0"; + compatible = "engicam,icore-mx8mm-ctouch2", "engicam,icore-mx8mm", + "fsl,imx8mm"; + + chosen { + stdout-path = &uart2; + }; +};