lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <22af22b7-57e5-4d81-824e-4b72ee8bdba2@gmx.net>
Date: Tue, 8 Oct 2024 17:53:07 +0200
From: Stefan Wahren <wahrenst@....net>
To: Lukasz Majewski <lukma@...x.de>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] dts: nxp: mxs: Add descriptions for imx287 based
 btt3-[012] devices

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@...x.de>
>>>
>>> ---
>>> 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@...x.de
>>> + */
>>> +
>>> +/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@...x.de
>>> + */
>>> +
>>> +/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@...x.de
>>> + */
>>> +
>>> +/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@...x.de
>>> + */
>>> +/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.
>
>>> +
>>> +	memory@...00000 {
>>> +		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?
>
>>> +	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

Regards

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ