Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Monday, July 18, 2022 8:01 AM > To: Shenwei Wang <shenwei.wang@xxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; > Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team > <kernel@xxxxxxxxxxxxxx>; Peng Fan <peng.fan@xxxxxxx> > Cc: devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: [EXT] Re: [PATCH RESEND v3 3/3] arm64: dts: imx: add imx8dxl support > > Caution: EXT Email > > On 16/07/2022 01:12, Shenwei Wang wrote: > > i.MX8DXL is a device targeting the automotive and industrial market > > segments. The chip is designed to achieve both high performance and > > low power consumption. It has a dual (2x) Cortex-A35 processor. > > > > This patch adds the imx8dxl soc and EVK board support. > > I saw this patch and I was already commenting it: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern > el.org%2Fall%2F20220404134609.2676793-2- > abel.vesa%40nxp.com%2F&data=05%7C01%7Cshenwei.wang%40nxp.com > %7C235450e576d44c030c1e08da68bd88de%7C686ea1d3bc2b4c6fa92cd99c5c3 > 01635%7C0%7C0%7C637937460494602259%7CUnknown%7CTWFpbGZsb3d8ey > JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C > 3000%7C%7C%7C&sdata=ZiDgTTZbcifNMBjDHTCdMKC1hgmf7BGzuvCJe%2 > FyagzQ%3D&reserved=0 > > Doing things twice, reviewing twice is waste of time. I actually spotted this > duplication after I perform a review, but this patch should be abandoned and > Abel's patches should rather go. I am not sure if Abel is still working on this task. The goal is to get the imx8dxl supported by upstreaming kernel. As both patches were picked up from the company internal repo and modified for upstreaming requirements, I don't mind whose patches to get included. Please let me know which one is easy for you to go ahead, and I can continue with Abel's patch if needed. > > > > > (...) > > > + > > +/ { > > + model = "Freescale i.MX8DXL EVK"; > > + compatible = "fsl,imx8dxl-evk", "fsl,imx8dxl"; > > + > > + chosen { > > + stdout-path = &lpuart0; > > + }; > > + > > + memory@80000000 { > > + device_type = "memory"; > > + reg = <0x00000000 0x80000000 0 0x40000000>; > > + }; > > + > > + reserved-memory { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > > + > > + /* > > + *Memory reserved for optee usage. Please do not use. > > + *This will be automaticky added to dtb if OP-TEE is installed. > > + *optee@96000000 { > > + * reg = <0 0x96000000 0 0x2000000>; > > + * no-map; > > + *}; > > + */ > > Now it is not a proper Linux coding style comment. See Coding Style. Let me fix it in next version. > > > + > > + /* global autoconfigured region for contiguous allocations */ > > + linux,cma { > > + compatible = "shared-dma-pool"; > > + reusable; > > + size = <0 0x14000000>; > > + alloc-ranges = <0 0x98000000 0 0x14000000>; > > + linux,cma-default; > > + }; > > + }; > > + > > + mux3_en: regulator-0 { > > + compatible = "regulator-fixed"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + regulator-name = "mux3_en"; > > + gpio = <&pca6416_2 8 GPIO_ACTIVE_LOW>; > > + regulator-always-on; > > + }; > > + > > + reg_fec1_sel: regulator-1 { > > + compatible = "regulator-fixed"; > > + regulator-name = "fec1_supply"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + gpio = <&pca6416_1 11 GPIO_ACTIVE_HIGH>; > > + regulator-always-on; > > + status = "disabled"; > > + }; > > + > > + reg_fec1_io: regulator-2 { > > + compatible = "regulator-fixed"; > > + regulator-name = "fec1_io_supply"; > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + gpio = <&max7322 0 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + regulator-always-on; > > + status = "disabled"; > > + }; > > + > > + reg_usdhc2_vmmc: regulator-3 { > > + compatible = "regulator-fixed"; > > + regulator-name = "SD1_SPWR"; > > + regulator-min-microvolt = <3000000>; > > + regulator-max-microvolt = <3000000>; > > + gpio = <&lsio_gpio4 30 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + off-on-delay-us = <3480>; > > + }; > > +}; > > + > > +&eqos { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_eqos>; > > + phy-mode = "rgmii-id"; > > + phy-handle = <ðphy0>; > > + nvmem-cells = <&fec_mac1>; > > + nvmem-cell-names = "mac-address"; > > + snps,reset-gpios = <&pca6416_1 2 GPIO_ACTIVE_LOW>; > > + snps,reset-delays-us = <10 20 200000>; > > + status = "okay"; > > + > > + mdio { > > + compatible = "snps,dwmac-mdio"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ethphy0: ethernet-phy@0 { > > + compatible = "ethernet-phy-ieee802.3-c22"; > > + reg = <0>; > > + eee-broken-1000t; > > + qca,disable-smarteee; > > + vddio-supply = <&vddio0>; > > + > > + vddio0: vddio-regulator { > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + }; > > + }; > > + }; > > +}; > > + > > +&fec1 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_fec1>; > > + phy-mode = "rgmii-txid"; > > + phy-handle = <ðphy1>; > > + fsl,magic-packet; > > + rx-internal-delay-ps = <2000>; > > + nvmem-cells = <&fec_mac0>; > > + nvmem-cell-names = "mac-address"; > > + phy-reset-gpios = <&pca6416_1 0 GPIO_ACTIVE_LOW>; > > + phy-reset-duration = <10>; > > + phy-reset-post-delay = <150>; > > + status = "disabled"; > > + > > + mdio { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ethphy1: ethernet-phy@1 { > > + compatible = "ethernet-phy-ieee802.3-c22"; > > + reg = <1>; > > + qca,disable-smarteee; > > + vddio-supply = <&vddio1>; > > + > > + vddio1: vddio-regulator { > > + regulator-min-microvolt = <1800000>; > > + regulator-max-microvolt = <1800000>; > > + }; > > + }; > > + }; > > +}; > > + > > +&flexspi0 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_flexspi0>; > > + nxp,fspi-dll-slvdly = <4>; > > + status = "okay"; > > + > > + mt35xu512aba0: flash@0 { > > + compatible = "jedec,spi-nor"; > > + reg = <0>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + spi-max-frequency = <133000000>; > > + spi-tx-bus-width = <8>; > > + spi-rx-bus-width = <8>; > > + }; > > +}; > > + > > +&i2c2 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + clock-frequency = <100000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c2>; > > + status = "okay"; > > + > > + pca6416_1: gpio@20 { > > + compatible = "ti,tca6416"; > > + reg = <0x20>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > + > > + pca6416_2: gpio@21 { > > + compatible = "ti,tca6416"; > > + reg = <0x21>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > + > > + pca9548_1: gpio@70 { > > + compatible = "nxp,pca9548"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x70>; > > + > > + i2c@0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x0>; > > + > > + max7322: gpio@68 { > > + compatible = "maxim,max7322"; > > + reg = <0x68>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + status = "disabled"; > > + }; > > + }; > > + > > + i2c@4 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x4>; > > + }; > > + > > + i2c@5 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x5>; > > + }; > > + > > + i2c@6 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x6>; > > + }; > > + }; > > +}; > > + > > +&i2c3 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + clock-frequency = <100000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c3>; > > + status = "okay"; > > + > > + pca6416_3: gpio@20 { > > + compatible = "ti,tca6416"; > > + reg = <0x20>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-parent = <&lsio_gpio2>; > > + interrupts = <5 IRQ_TYPE_EDGE_RISING>; > > + }; > > + > > + pca9548_2: gpio@70 { > > + compatible = "nxp,pca9548"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x70>; > > + > > + i2c@0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x0>; > > + }; > > + > > + i2c@1 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x1>; > > + }; > > + > > + i2c@2 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x2>; > > + }; > > + > > + i2c@3 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x3>; > > + }; > > + > > + i2c@4 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <0x4>; > > + }; > > + }; > > +}; > > + > > +&lpuart0 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_lpuart0>; > > + status = "okay"; > > +}; > > + > > +&lsio_gpio4 { > > + status = "okay"; > > +}; > > + > > +&lsio_gpio5 { > > + status = "okay"; > > +}; > > + > > +&thermal_zones { > > + pmic-thermal0 { > > + polling-delay-passive = <250>; > > + polling-delay = <2000>; > > + thermal-sensors = <&tsens IMX_SC_R_PMIC_0>; > > + > > + trips { > > + pmic_alert0: trip0 { > > + temperature = <110000>; > > + hysteresis = <2000>; > > + type = "passive"; > > + }; > > + pmic_crit0: trip1 { > > + temperature = <125000>; > > + hysteresis = <2000>; > > + type = "critical"; > > + }; > > + }; > > + > > + cooling-maps { > > + map0 { > > + trip = <&pmic_alert0>; > > + cooling-device = > > + <&A35_0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > > + <&A35_1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > > + }; > > + }; > > + }; > > +}; > > + > > +&usdhc1 { > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > > + pinctrl-0 = <&pinctrl_usdhc1>; > > + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; > > + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; > > + bus-width = <8>; > > + no-sd; > > + no-sdio; > > + non-removable; > > + status = "okay"; > > +}; > > + > > +&usdhc2 { > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > > + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > > + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; > > + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; > > + bus-width = <4>; > > + vmmc-supply = <®_usdhc2_vmmc>; > > + cd-gpios = <&lsio_gpio5 1 GPIO_ACTIVE_LOW>; > > + wp-gpios = <&lsio_gpio5 0 GPIO_ACTIVE_HIGH>; > > + max-frequency = <100000000>; > > + status = "okay"; > > +}; > > + > > +&iomuxc { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_hog>; > > + > > + pinctrl_hog: hoggrp { > > + fsl,pins = < > > + IMX8DXL_COMP_CTL_GPIO_1V8_3V3_GPIORHB_PAD > 0x000514a0 > > + IMX8DXL_COMP_CTL_GPIO_1V8_3V3_GPIORHK_PAD > 0x000014a0 > > + IMX8DXL_SPI3_CS0_ADMA_ACM_MCLK_OUT1 > 0x0600004c > > + IMX8DXL_SNVS_TAMPER_OUT1_LSIO_GPIO2_IO05_IN > 0x0600004c > > + >; > > + }; > > + > > + pinctrl_usbotg1: otg1 { > > All pinctrl groups must end with "grp". Just open the bindings for several iMX pin > controllers. > > I pointed it out and you still did not fix it. Sorry, I misunderstood that. Will get it fixed in next version. > > > > + fsl,pins = < > > + IMX8DXL_USB_SS3_TC0_CONN_USB_OTG1_PWR > 0x00000021 > > + >; > > + }; > > + > > + pinctrl_usbotg2: otg2 { > > + fsl,pins = < > > + IMX8DXL_USB_SS3_TC1_CONN_USB_OTG2_PWR > 0x00000021 > > + >; > > + }; > > (...) > > > diff --git a/arch/arm64/boot/dts/freescale/imx8dxl.dtsi > > b/arch/arm64/boot/dts/freescale/imx8dxl.dtsi > > new file mode 100644 > > index 0000000000000..2de56dfa16d6b > > --- /dev/null > > +++ b/arch/arm64/boot/dts/freescale/imx8dxl.dtsi > > @@ -0,0 +1,245 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright 2019~2020 NXP > > + */ > > + > > +#include <dt-bindings/clock/imx8-clock.h> #include > > +<dt-bindings/firmware/imx/rsrc.h> > > +#include <dt-bindings/gpio/gpio.h> > > +#include <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/input/input.h> > > +#include <dt-bindings/pinctrl/pads-imx8dxl.h> > > +#include <dt-bindings/thermal/thermal.h> > > + > > +/ { > > + interrupt-parent = <&gic>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + aliases { > > + ethernet0 = &fec1; > > + ethernet1 = &eqos; > > + gpio0 = &lsio_gpio0; > > + gpio1 = &lsio_gpio1; > > + gpio2 = &lsio_gpio2; > > + gpio3 = &lsio_gpio3; > > + gpio4 = &lsio_gpio4; > > + gpio5 = &lsio_gpio5; > > + gpio6 = &lsio_gpio6; > > + gpio7 = &lsio_gpio7; > > + i2c2 = &i2c2; > > + i2c3 = &i2c3; > > Still not a SoC aliases but rather board. See my comments in another thread. > > > + mmc0 = &usdhc1; > > + mmc1 = &usdhc2; > > + mu1 = &lsio_mu1; > > + serial0 = &lpuart0; > > + serial1 = &lpuart1; > > + serial2 = &lpuart2; > > + serial3 = &lpuart3; > > + }; > > (...) > > > + > > + xtal32k: clock-xtal32k { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <32768>; > > + clock-output-names = "xtal_32KHz"; > > + }; > > + > > + xtal24m: clock-xtal24m { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <24000000>; > > > Didn't you ignore (again) comments? The SoC requires two Crystal clock inputs, one is 24MHz and the other is 32KHz. The board design doesn't have an option to change the values. That's why we keep the nodes here. Thanks, Shenwei > > So let me again clarify - either you implement the comment or you keep > discussion going. > > You keep not implementing the comments and you decided not discussing them, > so it's still a NAK. > > > Best regards, > Krzysztof
