| Message ID | 20240117074911.7425-3-othacehe@gnu.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Phytec i.MX93 Segin support | expand |
Hi Mathieu, first of all, nice to see an attempt to upstream the board. A bit fast though as upstreaming was planned after the official PHYTEC release. However, there is quite a lot of differences in respect to the downstream PHYTEC kernel tree. I am sure PHYTEC wants to sync here to avoid general confusion. CC-ing: upstream@list.phytec.de So lets start: - the board should be named "imx93-phyboard-segin" instead of "imx93-phycore-segin". PHYTEC naming convention: phyCORE -> the SoM phyBOARD -> the base board (with the SoM) On 17. 01. 24 08:49, Mathieu Othacehe wrote: > Add DTSI for Phytec i.MX93 System on Module and DTS for Phytec > i.MX93 on Segin evaluation board. > > This version comes with: > - 1GB LPDDR4 RAM > - external SD > - debug UART > - 1x 100Mbit Ethernet Maybe you sync this commit msg + title with downstream commit like so: > arm64: dts: imx93: Add phyBOARD-Segin-i.MX93 support > > Add basic support for phyBOARD-Segin-i.MX93. > Main features are: > * Ethernet > * SD-Card > * UART > plus add eMMC to the list if you decided to enable it? > > Signed-off-by: Mathieu Othacehe <othacehe@gnu.org> > --- > arch/arm64/boot/dts/freescale/Makefile | 1 + > .../dts/freescale/imx93-phycore-segin.dts | 93 +++++++++++++++++++ > .../boot/dts/freescale/imx93-phycore-som.dtsi | 64 +++++++++++++ > 3 files changed, 158 insertions(+) > create mode 100644 arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts > create mode 100644 arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile > index 2e027675d7bb..f078d6ef75f7 100644 > --- a/arch/arm64/boot/dts/freescale/Makefile > +++ b/arch/arm64/boot/dts/freescale/Makefile > @@ -201,6 +201,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8qxp-colibri-iris-v2.dtb > dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb > dtb-$(CONFIG_ARCH_MXC) += imx8ulp-evk.dtb > dtb-$(CONFIG_ARCH_MXC) += imx93-11x11-evk.dtb > +dtb-$(CONFIG_ARCH_MXC) += imx93-phycore-segin.dtb Like I said, this has to be renamed to imx93-phyboard-segin.dts as the official kit name is "phyBOARD-Segin-i.MX93". > dtb-$(CONFIG_ARCH_MXC) += imx93-tqma9352-mba93xxca.dtb > dtb-$(CONFIG_ARCH_MXC) += imx93-tqma9352-mba93xxla.dtb > > diff --git a/arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts b/arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts > new file mode 100644 > index 000000000000..748b779a9dc1 > --- /dev/null > +++ b/arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts> @@ -0,0 +1,93 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Copyright (C) 2023 PHYTEC Messtechnik GmbH > + * Christoph Stoidner <c.stoidner@phytec.de> > + * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com> > + * > + */ > +/dts-v1/; > + > +#include "imx93-phycore-som.dtsi" > + > +/{ > + model = "PHYTEC phyBOARD-Segin-i.MX93"; > + compatible = "phytec,imx93-phycore-segin", "phytec,imx93-phycore-som", > + "fsl,imx93"; > + > + chosen { > + stdout-path = &lpuart1; > + }; > + > + reg_usdhc2_vmmc: regulator-usdhc2 { > + compatible = "regulator-fixed"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>; > + regulator-name = "VSD_3V3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + gpio = <&gpio3 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; Order properties here alphabetically like in the downstream kernel. Comment applies for the entire patch. > + }; > +}; > + > +/* Console */ > +&lpuart1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_uart1>; > + status = "okay"; > +}; > + > +/* SD-Card */ > +&usdhc2 { > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > + pinctrl-1 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > + pinctrl-2 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > + cd-gpios = <&gpio3 00 GPIO_ACTIVE_LOW>; > + vmmc-supply = <®_usdhc2_vmmc>; > + bus-width = <4>; > + status = "okay"; > + no-sdio; > + no-mmc; > +}; > + Please consider adding pinctrl_100mhz and pinctrl_200mhz from the downstream kernel which were determined by HW measurements at those operating frequencies. > +/* Watchdog */ > +&wdog3 { > + status = "okay"; > +}; > + > +&iomuxc { > + pinctrl-names = "default"; > + status = "okay"; Remove this pinctrl + status left-over? > + > + pinctrl_uart1: uart1grp { > + fsl,pins = < > + MX93_PAD_UART1_RXD__LPUART1_RX 0x31e > + MX93_PAD_UART1_TXD__LPUART1_TX 0x31e Please consider pinctrl settings from the down-stream kernel. They differ. > + >; > + }; > + > + pinctrl_reg_usdhc2_vmmc: regusdhc2vmmcgrp { > + fsl,pins = < > + MX93_PAD_SD2_RESET_B__GPIO3_IO07 0x31e > + >; > + }; > + > + pinctrl_usdhc2_gpio: usdhc2gpiogrp { > + fsl,pins = < > + MX93_PAD_SD2_CD_B__GPIO3_IO00 0x31e > + >; > + }; > + > + pinctrl_usdhc2: usdhc2grp { > + fsl,pins = < > + MX93_PAD_SD2_CLK__USDHC2_CLK 0x178e > + MX93_PAD_SD2_CMD__USDHC2_CMD 0x139e > + MX93_PAD_SD2_DATA0__USDHC2_DATA0 0x138e > + MX93_PAD_SD2_DATA1__USDHC2_DATA1 0x138e > + MX93_PAD_SD2_DATA2__USDHC2_DATA2 0x138e > + MX93_PAD_SD2_DATA3__USDHC2_DATA3 0x139e > + MX93_PAD_SD2_VSELECT__USDHC2_VSELECT 0x51e > + >; > + }; > +}; > diff --git a/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi b/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi > new file mode 100644 > index 000000000000..4edff4a50b2b > --- /dev/null > +++ b/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Copyright (C) 2023 PHYTEC Messtechnik GmbH > + * Christoph Stoidner <c.stoidner@phytec.de> > + * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com> > + * > + */ > +/dts-v1/; > + > +#include "imx93.dtsi" > + > +/{ > + model = "PHYTEC phyCORE-i.MX93"; > + compatible = "phytec,imx93-phycore-som", "fsl,imx93"; > + > + reserved-memory { > + ranges; > + #address-cells = <2>; > + #size-cells = <2>; > + > + linux,cma { > + compatible = "shared-dma-pool"; > + reusable; > + alloc-ranges = <0 0x80000000 0 0x40000000>; > + size = <0 0x10000000>; > + linux,cma-default; > + }; > + > + ele_reserved: ele-reserved@a4120000 { > + compatible = "shared-dma-pool"; > + reg = <0 0xa4120000 0 0x100000>; > + no-map; > + }; Remove ele_reserved if you are not using it anywhere. This only applies to vendor kernel at the moment. > + }; > +}; > + > +/* eMMC */ > +&usdhc1 { > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > + pinctrl-0 = <&pinctrl_usdhc1>; > + pinctrl-1 = <&pinctrl_usdhc1>; > + pinctrl-2 = <&pinctrl_usdhc1>; > + bus-width = <8>; > + non-removable; > + status = "okay"; Currently only default DDR50 is supported. So no need for 100mhz + 200mhz pinctrls. > +}; > + > +&iomuxc { > + pinctrl_usdhc1: usdhc1grp { > + fsl,pins = < > + MX93_PAD_SD1_CLK__USDHC1_CLK 0x15fe > + MX93_PAD_SD1_CMD__USDHC1_CMD 0x13fe > + MX93_PAD_SD1_DATA0__USDHC1_DATA0 0x13fe > + MX93_PAD_SD1_DATA1__USDHC1_DATA1 0x13fe > + MX93_PAD_SD1_DATA2__USDHC1_DATA2 0x13fe > + MX93_PAD_SD1_DATA3__USDHC1_DATA3 0x13fe > + MX93_PAD_SD1_DATA4__USDHC1_DATA4 0x13fe > + MX93_PAD_SD1_DATA5__USDHC1_DATA5 0x13fe > + MX93_PAD_SD1_DATA6__USDHC1_DATA6 0x13fe > + MX93_PAD_SD1_DATA7__USDHC1_DATA7 0x13fe > + MX93_PAD_SD1_STROBE__USDHC1_STROBE 0x15fe > + >; > + }; > +}; BR, Primoz
On Thu, Jan 18, 2024 at 10:58:36AM +0100, Primoz Fiser wrote: > > + reg_usdhc2_vmmc: regulator-usdhc2 { > > + compatible = "regulator-fixed"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>; > > + regulator-name = "VSD_3V3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + gpio = <&gpio3 7 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > Order properties here alphabetically like in the downstream kernel. > > Comment applies for the entire patch. Please do not order properties alphabetically. Instead, please read the new documentation on property ordering that makes explicit what has just been convention until now: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst?h=for-next&id=83a368a3fc8ae8538bccb713dc0cae9eacc04790#n112 Cheers, Conor.
Hey, > Please do not order properties alphabetically. Instead, please read > the new documentation on property ordering that makes explicit what > has just been convention until now: > https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst?h=for-next&id=83a368a3fc8ae8538bccb713dc0cae9eacc04790#n112 Thanks for the link. I have a question though. Regarding that section: --8<---------------cut here---------------start------------->8--- /* SD-Card */ &usdhc2 { pinctrl-names = "default", "state_100mhz", "state_200mhz"; pinctrl-0 = <&pinctrl_usdhc2_default>, <&pinctrl_usdhc2_cd>; pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_cd>; pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_cd>; bus-width = <4>; cd-gpios = <&gpio3 00 GPIO_ACTIVE_LOW>; no-sdio; no-mmc; vmmc-supply = <®_usdhc2_vmmc>; status = "okay"; }; --8<---------------cut here---------------end--------------->8--- The documentation states: --8<---------------cut here---------------start------------->8--- Order of Properties in Device Node ---------------------------------- The following order of properties in device nodes is preferred: 1. "compatible" 2. "reg" 3. "ranges" 4. Standard/common properties (defined by common bindings, e.g. without vendor-prefixes) 5. Vendor-specific properties 6. "status" (if applicable) 7. Child nodes, where each node is preceded with a blank line --8<---------------cut here---------------end--------------->8--- All of the properties in my example are falling into the "4" category I guess, except for "status" that should come last. Now, how am I supposed to order those properties? I had a look to other IMX device trees and it is hard to establish a pattern. Pinctrl first, then alphabetical order? Anything else? Thanks for the guidance, Mathieu
On Thu, Jan 18, 2024 at 02:43:07PM +0100, Mathieu Othacehe wrote: > > Hey, > > > Please do not order properties alphabetically. Instead, please read > > the new documentation on property ordering that makes explicit what > > has just been convention until now: > > https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst?h=for-next&id=83a368a3fc8ae8538bccb713dc0cae9eacc04790#n112 > > Thanks for the link. > > I have a question though. Regarding that section: > > --8<---------------cut here---------------start------------->8--- > /* SD-Card */ > &usdhc2 { > pinctrl-names = "default", "state_100mhz", "state_200mhz"; > pinctrl-0 = <&pinctrl_usdhc2_default>, <&pinctrl_usdhc2_cd>; > pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_cd>; > pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_cd>; > bus-width = <4>; > cd-gpios = <&gpio3 00 GPIO_ACTIVE_LOW>; > no-sdio; > no-mmc; > vmmc-supply = <®_usdhc2_vmmc>; > status = "okay"; > }; > --8<---------------cut here---------------end--------------->8--- > > The documentation states: > > --8<---------------cut here---------------start------------->8--- > Order of Properties in Device Node > ---------------------------------- > > The following order of properties in device nodes is preferred: > > 1. "compatible" > 2. "reg" > 3. "ranges" > 4. Standard/common properties (defined by common bindings, e.g. without > vendor-prefixes) > 5. Vendor-specific properties > 6. "status" (if applicable) > 7. Child nodes, where each node is preceded with a blank line > --8<---------------cut here---------------end--------------->8--- > > All of the properties in my example are falling into the "4" category I > guess, except for "status" that should come last. Now, how am I supposed > to order those properties? I had a look to other IMX device trees and it > is hard to establish a pattern. Pinctrl first, then alphabetical order? > Anything else? If that is the established order for imx devicetrees, then yeah, I would follow that ordering. From my own quick check of recently added boards, that is the way things seem to be. Cheers, Conor.
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile index 2e027675d7bb..f078d6ef75f7 100644 --- a/arch/arm64/boot/dts/freescale/Makefile +++ b/arch/arm64/boot/dts/freescale/Makefile @@ -201,6 +201,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8qxp-colibri-iris-v2.dtb dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb dtb-$(CONFIG_ARCH_MXC) += imx8ulp-evk.dtb dtb-$(CONFIG_ARCH_MXC) += imx93-11x11-evk.dtb +dtb-$(CONFIG_ARCH_MXC) += imx93-phycore-segin.dtb dtb-$(CONFIG_ARCH_MXC) += imx93-tqma9352-mba93xxca.dtb dtb-$(CONFIG_ARCH_MXC) += imx93-tqma9352-mba93xxla.dtb diff --git a/arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts b/arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts new file mode 100644 index 000000000000..748b779a9dc1 --- /dev/null +++ b/arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (C) 2023 PHYTEC Messtechnik GmbH + * Christoph Stoidner <c.stoidner@phytec.de> + * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com> + * + */ +/dts-v1/; + +#include "imx93-phycore-som.dtsi" + +/{ + model = "PHYTEC phyBOARD-Segin-i.MX93"; + compatible = "phytec,imx93-phycore-segin", "phytec,imx93-phycore-som", + "fsl,imx93"; + + chosen { + stdout-path = &lpuart1; + }; + + reg_usdhc2_vmmc: regulator-usdhc2 { + compatible = "regulator-fixed"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>; + regulator-name = "VSD_3V3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + gpio = <&gpio3 7 GPIO_ACTIVE_HIGH>; + enable-active-high; + }; +}; + +/* Console */ +&lpuart1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart1>; + status = "okay"; +}; + +/* SD-Card */ +&usdhc2 { + pinctrl-names = "default", "state_100mhz", "state_200mhz"; + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; + pinctrl-1 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; + pinctrl-2 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; + cd-gpios = <&gpio3 00 GPIO_ACTIVE_LOW>; + vmmc-supply = <®_usdhc2_vmmc>; + bus-width = <4>; + status = "okay"; + no-sdio; + no-mmc; +}; + +/* Watchdog */ +&wdog3 { + status = "okay"; +}; + +&iomuxc { + pinctrl-names = "default"; + status = "okay"; + + pinctrl_uart1: uart1grp { + fsl,pins = < + MX93_PAD_UART1_RXD__LPUART1_RX 0x31e + MX93_PAD_UART1_TXD__LPUART1_TX 0x31e + >; + }; + + pinctrl_reg_usdhc2_vmmc: regusdhc2vmmcgrp { + fsl,pins = < + MX93_PAD_SD2_RESET_B__GPIO3_IO07 0x31e + >; + }; + + pinctrl_usdhc2_gpio: usdhc2gpiogrp { + fsl,pins = < + MX93_PAD_SD2_CD_B__GPIO3_IO00 0x31e + >; + }; + + pinctrl_usdhc2: usdhc2grp { + fsl,pins = < + MX93_PAD_SD2_CLK__USDHC2_CLK 0x178e + MX93_PAD_SD2_CMD__USDHC2_CMD 0x139e + MX93_PAD_SD2_DATA0__USDHC2_DATA0 0x138e + MX93_PAD_SD2_DATA1__USDHC2_DATA1 0x138e + MX93_PAD_SD2_DATA2__USDHC2_DATA2 0x138e + MX93_PAD_SD2_DATA3__USDHC2_DATA3 0x139e + MX93_PAD_SD2_VSELECT__USDHC2_VSELECT 0x51e + >; + }; +}; diff --git a/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi b/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi new file mode 100644 index 000000000000..4edff4a50b2b --- /dev/null +++ b/arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (C) 2023 PHYTEC Messtechnik GmbH + * Christoph Stoidner <c.stoidner@phytec.de> + * Copyright (C) 2024 Mathieu Othacehe <m.othacehe@gmail.com> + * + */ +/dts-v1/; + +#include "imx93.dtsi" + +/{ + model = "PHYTEC phyCORE-i.MX93"; + compatible = "phytec,imx93-phycore-som", "fsl,imx93"; + + reserved-memory { + ranges; + #address-cells = <2>; + #size-cells = <2>; + + linux,cma { + compatible = "shared-dma-pool"; + reusable; + alloc-ranges = <0 0x80000000 0 0x40000000>; + size = <0 0x10000000>; + linux,cma-default; + }; + + ele_reserved: ele-reserved@a4120000 { + compatible = "shared-dma-pool"; + reg = <0 0xa4120000 0 0x100000>; + no-map; + }; + }; +}; + +/* eMMC */ +&usdhc1 { + pinctrl-names = "default", "state_100mhz", "state_200mhz"; + pinctrl-0 = <&pinctrl_usdhc1>; + pinctrl-1 = <&pinctrl_usdhc1>; + pinctrl-2 = <&pinctrl_usdhc1>; + bus-width = <8>; + non-removable; + status = "okay"; +}; + +&iomuxc { + pinctrl_usdhc1: usdhc1grp { + fsl,pins = < + MX93_PAD_SD1_CLK__USDHC1_CLK 0x15fe + MX93_PAD_SD1_CMD__USDHC1_CMD 0x13fe + MX93_PAD_SD1_DATA0__USDHC1_DATA0 0x13fe + MX93_PAD_SD1_DATA1__USDHC1_DATA1 0x13fe + MX93_PAD_SD1_DATA2__USDHC1_DATA2 0x13fe + MX93_PAD_SD1_DATA3__USDHC1_DATA3 0x13fe + MX93_PAD_SD1_DATA4__USDHC1_DATA4 0x13fe + MX93_PAD_SD1_DATA5__USDHC1_DATA5 0x13fe + MX93_PAD_SD1_DATA6__USDHC1_DATA6 0x13fe + MX93_PAD_SD1_DATA7__USDHC1_DATA7 0x13fe + MX93_PAD_SD1_STROBE__USDHC1_STROBE 0x15fe + >; + }; +};
Add DTSI for Phytec i.MX93 System on Module and DTS for Phytec i.MX93 on Segin evaluation board. This version comes with: - 1GB LPDDR4 RAM - external SD - debug UART - 1x 100Mbit Ethernet Signed-off-by: Mathieu Othacehe <othacehe@gnu.org> --- arch/arm64/boot/dts/freescale/Makefile | 1 + .../dts/freescale/imx93-phycore-segin.dts | 93 +++++++++++++++++++ .../boot/dts/freescale/imx93-phycore-som.dtsi | 64 +++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 arch/arm64/boot/dts/freescale/imx93-phycore-segin.dts create mode 100644 arch/arm64/boot/dts/freescale/imx93-phycore-som.dtsi