Re: [PATCH v6 2/2] dts: nxp: mxs: Add descriptions for imx287 based btt3-[012] devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Stefan,

> Hi Lukasz,
> 
> Am 08.10.24 um 13:41 schrieb Lukasz Majewski:
> > Hi Stefan,
> >  
> >> Hi Lukasz,
> >>
> >> please adjust the subject of your patch accordingly to the
> >> subsystem.
> >>
> >> Suggestion
> >>
> >> ARM: dts: mxs: Add descriptions for imx287 based btt3-[012] devices
> >>
> >> Am 12.09.24 um 14:48 schrieb Lukasz Majewski:  
> >>> The btt3 device' HW revisions from 0 to 2 use imx287 SoC and are
> >>> to some extend similar to already upstreamed XEA devices, hence
> >>> are using common imx28-lwe.dtsi file.
> >>>
> >>> New, imx28-btt3.dtsi has been added to embrace common DTS
> >>> properties for different HW revisions for this device.
> >>>
> >>> As a result - changes introduced in imx28-btt3-[012].dts are
> >>> minimal.
> >>>
> >>> Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
> >>>
> >>> ---
> >>> Changes for v2:
> >>> - Rename dts file from btt3-[012] to imx28-btt3-[012] to match
> >>> current linux kernel naming convention
> >>> - Remove 'wlf,wm8974' from compatible for codec@1a
> >>>
> >>> Changes for v3:
> >>> - Keep alphabethical order for Makefile entries
> >>>
> >>> Changes for v4:
> >>> - Change compatible for btt3 board (to 'lwn,imx28-btt3')
> >>>
> >>> Changes for v5:
> >>> - Combine patch, which adds btt3-[012] with one adding board entry
> >>> to fsl.yaml
> >>>
> >>> Changes for v6:
> >>> - Make the patch series for adding entry in fsl.yaml and btt3
> >>> ---
> >>>    arch/arm/boot/dts/nxp/mxs/Makefile         |   3 +
> >>>    arch/arm/boot/dts/nxp/mxs/imx28-btt3-0.dts |  12 +
> >>>    arch/arm/boot/dts/nxp/mxs/imx28-btt3-1.dts |   8 +
> >>>    arch/arm/boot/dts/nxp/mxs/imx28-btt3-2.dts |  12 +
> >>>    arch/arm/boot/dts/nxp/mxs/imx28-btt3.dtsi  | 320
> >>> +++++++++++++++++++++ 5 files changed, 355 insertions(+)
> >>>    create mode 100644 arch/arm/boot/dts/nxp/mxs/imx28-btt3-0.dts
> >>>    create mode 100644 arch/arm/boot/dts/nxp/mxs/imx28-btt3-1.dts
> >>>    create mode 100644 arch/arm/boot/dts/nxp/mxs/imx28-btt3-2.dts
> >>>    create mode 100644 arch/arm/boot/dts/nxp/mxs/imx28-btt3.dtsi
> >>>
> >>> diff --git a/arch/arm/boot/dts/nxp/mxs/Makefile
> >>> b/arch/arm/boot/dts/nxp/mxs/Makefile index
> >>> a430d04f9c69..96dd31ea19ba 100644 ---
> >>> a/arch/arm/boot/dts/nxp/mxs/Makefile +++
> >>> b/arch/arm/boot/dts/nxp/mxs/Makefile @@ -8,6 +8,9 @@
> >>> dtb-$(CONFIG_ARCH_MXS) += \ imx28-apf28.dtb \
> >>>    	imx28-apf28dev.dtb \
> >>>    	imx28-apx4devkit.dtb \
> >>> +	imx28-btt3-0.dtb \
> >>> +	imx28-btt3-1.dtb \
> >>> +	imx28-btt3-2.dtb \
> >>>    	imx28-cfa10036.dtb \
> >>>    	imx28-cfa10037.dtb \
> >>>    	imx28-cfa10049.dtb \
> >>> diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-btt3-0.dts
> >>> b/arch/arm/boot/dts/nxp/mxs/imx28-btt3-0.dts new file mode 100644
> >>> index 000000000000..6ac46e4b21bb
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/nxp/mxs/imx28-btt3-0.dts
> >>> @@ -0,0 +1,12 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> >>> +/*
> >>> + * Copyright 2024
> >>> + * Lukasz Majewski, DENX Software Engineering, lukma@xxxxxxx
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +#include "imx28-btt3.dtsi"
> >>> +
> >>> +&hog_pins_rev {
> >>> +	fsl,pull-up = <MXS_PULL_ENABLE>;
> >>> +};
> >>> diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-btt3-1.dts
> >>> b/arch/arm/boot/dts/nxp/mxs/imx28-btt3-1.dts new file mode 100644
> >>> index 000000000000..213fe931c58b
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/nxp/mxs/imx28-btt3-1.dts
> >>> @@ -0,0 +1,8 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> >>> +/*
> >>> + * Copyright 2024
> >>> + * Lukasz Majewski, DENX Software Engineering, lukma@xxxxxxx
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +#include "imx28-btt3.dtsi"
> >>> diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-btt3-2.dts
> >>> b/arch/arm/boot/dts/nxp/mxs/imx28-btt3-2.dts new file mode 100644
> >>> index 000000000000..c787c2d03463
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/nxp/mxs/imx28-btt3-2.dts
> >>> @@ -0,0 +1,12 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> >>> +/*
> >>> + * Copyright 2024
> >>> + * Lukasz Majewski, DENX Software Engineering, lukma@xxxxxxx
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +#include "imx28-btt3.dtsi"
> >>> +
> >>> +&lcdif {
> >>> +	display = <&display_te_b>;  
> >> The reason why you don't move the second display into this file is
> >> because you expect a new hardware revision in the future?  
> > Yes, exactly. This is long-standing device.
> >  
> >>> +};
> >>> diff --git a/arch/arm/boot/dts/nxp/mxs/imx28-btt3.dtsi
> >>> b/arch/arm/boot/dts/nxp/mxs/imx28-btt3.dtsi new file mode 100644
> >>> index 000000000000..94a21ea8d5d2
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/nxp/mxs/imx28-btt3.dtsi
> >>> @@ -0,0 +1,320 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> >>> +/*
> >>> + * Copyright 2024
> >>> + * Lukasz Majewski, DENX Software Engineering, lukma@xxxxxxx
> >>> + */
> >>> +/dts-v1/;
> >>> +#include "imx28-lwe.dtsi"
> >>> +
> >>> +/ {
> >>> +	model = "BTT3";
> >>> +
> >>> +	compatible = "lwn,imx28-btt3", "fsl,imx28";
> >>> +
> >>> +	chosen {
> >>> +	       bootargs = "root=/dev/mmcblk0p2 rootfstype=ext4 ro
> >>> rootwait console=ttyAMA0,115200 panic=1 quiet";
> >>> +	};  
> >> It's a little bit unusual to place so many Linux specific stuff
> >> into the device tree bootargs.  
> > I do keep the bootargs from first version of the device/DTS to avoid
> > any "unexpected" regressions.  
> Not strong opinion about this this, but it sounds like your customer
> use a limited bootloader like imx-bootlets and the whole cmdline is
> configured via DT.

We do use u-boot from the very beginning...

> >  
> >>> +
> >>> +	memory@40000000 {
> >>> +		reg = <0x40000000 0x10000000>;
> >>> +		device_type = "memory";
> >>> +	};
> >>> +
> >>> +	poweroff {
> >>> +		compatible = "gpio-poweroff";
> >>> +		gpios = <&gpio0 24 0>;  
> >> Please use the GPIO polarity defines.  
> > Ok.
> >  
> >>> +	};
> >>> +
> >>> +	sound {
> >>> +		compatible = "simple-audio-card";
> >>> +		simple-audio-card,name = "BTTC Audio";
> >>> +		simple-audio-card,widgets = "Speaker", "BTTC
> >>> Speaker";
> >>> +		simple-audio-card,routing = "BTTC Speaker",
> >>> "SPKOUTN", "BTTC Speaker", "SPKOUTP";
> >>> +		simple-audio-card,dai-link@0 {
> >>> +			format = "left_j";
> >>> +			bitclock-master = <&dai0_master>;
> >>> +			frame-master = <&dai0_master>;
> >>> +			mclk-fs = <256>;
> >>> +			dai0_master: cpu {
> >>> +				sound-dai = <&saif0>;
> >>> +			};
> >>> +			codec {
> >>> +				sound-dai = <&wm89xx>;
> >>> +				clocks = <&saif0>;
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +
> >>> +	wifi_pwrseq: sdio-pwrseq {
> >>> +		compatible = "mmc-pwrseq-simple";
> >>> +		pinctrl-names = "default";
> >>> +		pinctrl-0 = <&wifi_en_pin_bttc>;
> >>> +		reset-gpios = <&gpio0 27 GPIO_ACTIVE_LOW>;
> >>> +		/* W1-163 needs 60us for WL_EN to be low and */
> >>> +		/* 150ms after high before downloading FW is
> >>> possible */
> >>> +		post-power-on-delay-ms = <200>;
> >>> +		power-off-delay-us = <100>;
> >>> +	};
> >>> +};
> >>> +
> >>> +&auart0 {
> >>> +	pinctrl-names = "default";
> >>> +	pinctrl-0 = <&auart0_2pins_a>;
> >>> +	status = "okay";
> >>> +};
> >>> +
> >>> +&auart3 {
> >>> +	pinctrl-names = "default";
> >>> +	pinctrl-0 = <&auart3_pins_a>;
> >>> +	uart-has-rtscts;
> >>> +	status = "okay";
> >>> +};
> >>> +
> >>> +&i2c0 {
> >>> +	wm89xx: codec@1a {
> >>> +		compatible = "wlf,wm8940";
> >>> +		reg = <0x1a>;
> >>> +		#sound-dai-cells = <0>;
> >>> +	};
> >>> +};
> >>> +
> >>> +&lcdif {
> >>> +	pinctrl-names = "default";
> >>> +	pinctrl-0 = <&lcdif_24bit_pins_a>,
> >>> <&lcdif_sync_pins_bttc>,
> >>> +		    <&lcdif_reset_pins_bttc>;
> >>> +	lcd-supply = <&reg_3v3>;
> >>> +	display = <&display0>;
> >>> +	status = "okay";
> >>> +	display0: display0 {
> >>> +		bits-per-pixel = <32>;
> >>> +		bus-width = <24>;
> >>> +		display-timings {
> >>> +			native-mode = <&timing0>;
> >>> +			timing0: timing0 {
> >>> +				clock-frequency = <6500000>;
> >>> +				hactive = <320>;
> >>> +				vactive = <240>;
> >>> +				hfront-porch = <20>;
> >>> +				hback-porch = <38>;
> >>> +				hsync-len = <30>;
> >>> +				vfront-porch = <4>;
> >>> +				vback-porch = <14>;
> >>> +				vsync-len = <4>;
> >>> +				hsync-active = <0>;
> >>> +				vsync-active = <0>;
> >>> +				de-active = <0>;
> >>> +				pixelclk-active = <1>;
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +	display_te_b: display1 {
> >>> +		bits-per-pixel = <32>;
> >>> +		bus-width = <24>;
> >>> +		display-timings {
> >>> +			native-mode = <&timing0>;
> >>> +			timing_te_b: timing0 {
> >>> +				clock-frequency = <6500000>;
> >>> +				hactive = <320>;
> >>> +				vactive = <240>;
> >>> +				hfront-porch = <20>;
> >>> +				hback-porch = <68>;
> >>> +				hsync-len = <30>;
> >>> +				vfront-porch = <4>;
> >>> +				vback-porch = <14>;
> >>> +				vsync-len = <4>;
> >>> +				hsync-active = <0>;
> >>> +				vsync-active = <0>;
> >>> +				de-active = <1>;
> >>> +				pixelclk-active = <1>;
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +
> >>> +};
> >>> +
> >>> +&mac0 {
> >>> +	clocks = <&clks 57>, <&clks 57>, <&clks 64>;
> >>> +	clock-names = "ipg", "ahb", "enet_out";
> >>> +	phy-handle = <&mac0_phy>;
> >>> +	phy-mode = "rmii";
> >>> +	phy-supply = <&reg_3v3>;
> >>> +	local-mac-address = [ 00 11 B8 00 BF 8A ];  
> >> Is this replaced dynamically by the bootloader? Otherwise this
> >> suggests all boards use the same MAC address.  
> > Yes, this is replaced during production. In fact it could be 00 00
> > 00 00 00 00 as well.
> >
> > The IP address assigned here allows the device to be recognizable on
> > the network even when the full "flashing" is not successful.  
> Could you please add a short comment about this?

Ok.

> >  
> >>> +	status = "okay";
> >>> +
> >>> +	mdio {
> >>> +		#address-cells = <1>;
> >>> +		#size-cells = <0>;
> >>> +
> >>> +		mac0_phy: ethernet-phy@0 {
> >>> +			/* LAN8720Ai - PHY ID */
> >>> +			compatible =
> >>> "ethernet-phy-id0007.c0f0","ethernet-phy-ieee802.3-c22";
> >>> +			reg = <0>;
> >>> +			smsc,disable-energy-detect;
> >>> +			max-speed = <100>;
> >>> +
> >>> +			reset-gpios = <&gpio4 12
> >>> GPIO_ACTIVE_LOW>; /* GPIO4_12 */  
> >> I think the comment only repeat what is already defined here.  
> > Yes - I will remove it.
> >  
> >>> +			reset-assert-us = <1000>;
> >>> +			reset-deassert-us = <1000>;
> >>> +		};
> >>> +	};
> >>> +};
> >>> +
> >>> +&pinctrl {
> >>> +	pinctrl-names = "default";
> >>> +	pinctrl-0 = <&hog_pins_a>, <&hog_pins_rev>;
> >>> +
> >>> +	hog_pins_a: hog@0 {
> >>> +		reg = <0>;
> >>> +		fsl,pinmux-ids = <
> >>> +			MX28_PAD_GPMI_RDY2__GPIO_0_22
> >>> +			MX28_PAD_GPMI_RDY3__GPIO_0_23
> >>> +			MX28_PAD_GPMI_RDN__GPIO_0_24
> >>> +			MX28_PAD_LCD_VSYNC__GPIO_1_28
> >>> +			MX28_PAD_SSP2_SS1__GPIO_2_20
> >>> +			MX28_PAD_SSP2_SS2__GPIO_2_21
> >>> +			MX28_PAD_AUART2_CTS__GPIO_3_10
> >>> +			MX28_PAD_AUART2_RTS__GPIO_3_11
> >>> +			MX28_PAD_GPMI_WRN__GPIO_0_25
> >>> +			MX28_PAD_ENET0_RXD2__GPIO_4_9
> >>> +			MX28_PAD_ENET0_TXD2__GPIO_4_11
> >>> +		>;
> >>> +		fsl,drive-strength = <MXS_DRIVE_4mA>;
> >>> +		fsl,voltage = <MXS_VOLTAGE_HIGH>;
> >>> +		fsl,pull-up = <MXS_PULL_DISABLE>;
> >>> +	};
> >>> +
> >>> +	hog_pins_rev: hog@1 {
> >>> +		reg = <1>;
> >>> +		fsl,pinmux-ids = <
> >>> +			MX28_PAD_ENET0_RXD3__GPIO_4_10
> >>> +			MX28_PAD_ENET0_TX_CLK__GPIO_4_5
> >>> +			MX28_PAD_ENET0_COL__GPIO_4_14
> >>> +			MX28_PAD_ENET0_CRS__GPIO_4_15
> >>> +		>;
> >>> +		fsl,drive-strength = <MXS_DRIVE_4mA>;
> >>> +		fsl,voltage = <MXS_VOLTAGE_HIGH>;
> >>> +		fsl,pull-up = <MXS_PULL_DISABLE>;
> >>> +	};
> >>> +
> >>> +	keypad_pins_bttc: keypad-bttc@0 {
> >>> +		reg = <0>;
> >>> +		fsl,pinmux-ids = <
> >>> +			MX28_PAD_GPMI_D00__GPIO_0_0
> >>> +			MX28_PAD_AUART0_CTS__GPIO_3_2
> >>> +			MX28_PAD_AUART0_RTS__GPIO_3_3
> >>> +			MX28_PAD_GPMI_D03__GPIO_0_3
> >>> +			MX28_PAD_GPMI_D04__GPIO_0_4
> >>> +			MX28_PAD_GPMI_D05__GPIO_0_5
> >>> +			MX28_PAD_GPMI_D06__GPIO_0_6
> >>> +			MX28_PAD_GPMI_D07__GPIO_0_7
> >>> +			MX28_PAD_GPMI_CE1N__GPIO_0_17
> >>> +			MX28_PAD_GPMI_CE2N__GPIO_0_18
> >>> +			MX28_PAD_GPMI_CE3N__GPIO_0_19
> >>> +			MX28_PAD_GPMI_RDY0__GPIO_0_20
> >>> +		>;
> >>> +		fsl,drive-strength = <MXS_DRIVE_4mA>;
> >>> +		fsl,voltage = <MXS_VOLTAGE_HIGH>;
> >>> +		fsl,pull-up = <MXS_PULL_DISABLE>;
> >>> +	};
> >>> +
> >>> +	lcdif_sync_pins_bttc: lcdif-bttc@0 {
> >>> +		reg = <0>;
> >>> +		fsl,pinmux-ids = <
> >>> +			MX28_PAD_LCD_DOTCLK__LCD_DOTCLK
> >>> +			MX28_PAD_LCD_ENABLE__LCD_ENABLE
> >>> +			MX28_PAD_LCD_HSYNC__LCD_HSYNC
> >>> +			MX28_PAD_LCD_RD_E__LCD_VSYNC
> >>> +		>;
> >>> +		fsl,drive-strength = <MXS_DRIVE_4mA>;
> >>> +		fsl,voltage = <MXS_VOLTAGE_HIGH>;
> >>> +		fsl,pull-up = <MXS_PULL_DISABLE>;
> >>> +	};
> >>> +
> >>> +	lcdif_reset_pins_bttc: lcdif-bttc@1 {
> >>> +		reg = <1>;
> >>> +		fsl,pinmux-ids = <
> >>> +			MX28_PAD_LCD_RESET__GPIO_3_30
> >>> +		>;
> >>> +		fsl,drive-strength = <MXS_DRIVE_4mA>;
> >>> +		fsl,voltage = <MXS_VOLTAGE_HIGH>;
> >>> +		fsl,pull-up = <MXS_PULL_ENABLE>;
> >>> +	};
> >>> +
> >>> +	ssp1_sdio_pins_a: ssp1-sdio@0 {
> >>> +		reg = <0>;
> >>> +		fsl,pinmux-ids = <
> >>> +			MX28_PAD_SSP1_DATA0__SSP1_D0
> >>> +			MX28_PAD_GPMI_D01__SSP1_D1
> >>> +			MX28_PAD_GPMI_D02__SSP1_D2
> >>> +			MX28_PAD_SSP1_DATA3__SSP1_D3
> >>> +			MX28_PAD_SSP1_CMD__SSP1_CMD
> >>> +			MX28_PAD_SSP1_SCK__SSP1_SCK
> >>> +		>;
> >>> +		fsl,drive-strength = <MXS_DRIVE_8mA>;
> >>> +		fsl,voltage = <MXS_VOLTAGE_HIGH>;
> >>> +		fsl,pull-up = <MXS_PULL_ENABLE>;
> >>> +	};
> >>> +
> >>> +	wifi_en_pin_bttc: wifi_en_pin@0 {  
> >> This should trigger a schema warning. The node name should use
> >> dashes instead of underscore.  
> > IIRC - there was no schema warning for it.  
> Nevertheless please use dashes like on the other nodes

Ok.

> 
> Regards




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgpVklPws84Dn.pgp
Description: OpenPGP digital signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux