| 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
| ||
|
Message-ID: <bf2c81e1-4e97-cfa2-326f-0a6125b2cff9@denx.de>
Date: Mon, 28 Oct 2024 11:41:07 +0100
From: Heiko Schocher <hs@...x.de>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org
Cc: Conor Dooley <conor+dt@...nel.org>, Fabio Estevam <festevam@...il.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Rob Herring <robh@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
Shawn Guo <shawnguo@...nel.org>, devicetree@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v1 2/2] arm64: dts: imx8mp: add aristainetos3 board
support
Hello Krzysztof,
On 28.10.24 11:24, Krzysztof Kozlowski wrote:
> On 28/10/2024 09:23, Heiko Schocher wrote:
>> Add support for the i.MX8MP based aristainetos3 boards from ABB.
>>
>> The board uses a ABB specific SoM from ADLink, based on NXP
>> i.MX8MP SoC. The SoM is used on 3 different carrier boards,
>> with small differences, which are all catched up in
>> devicetree overlays. The kernel image, the basic dtb
>> and all dtbos are collected in a fitimage. As bootloader
>> is used U-Boot which detects in his SPL stage the carrier
>> board by probing some i2c devices. When the correct
>> carrier is probed, the SPL applies all needed dtbos to
>> the dtb with which U-Boot gets loaded. Same principle
>> later before linux image boot, U-Boot applies the dtbos
>> needed for the carrier board before booting Linux.
>>
>> Signed-off-by: Heiko Schocher <hs@...x.de>
>> ---
>> checkpatch dropped the following warnings:
>> arch/arm64/boot/dts/freescale/imx8mp-aristainetos3a-som-v1.dtsi:248: warning: DT compatible string "ethernet-phy-id2000.a231" appears un-documented -- check ./Documentation/devicetree/bindings/
>>
>> ignored, as this compatible string is usedin other dts too, for example in
>>
>> arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
>>
>> arch/arm64/boot/dts/freescale/Makefile | 5 +
>> .../imx8mp-aristainetos3-adpismarc.dtsi | 64 +
>> .../imx8mp-aristainetos3-adpismarc.dtso | 14 +
>> .../imx8mp-aristainetos3-helios-lvds.dtsi | 89 ++
>> .../imx8mp-aristainetos3-helios-lvds.dtso | 13 +
>> .../imx8mp-aristainetos3-helios.dtsi | 103 ++
>> .../imx8mp-aristainetos3-helios.dtso | 13 +
>> .../imx8mp-aristainetos3-proton2s.dtsi | 176 +++
>> .../imx8mp-aristainetos3-proton2s.dtso | 13 +
>> .../imx8mp-aristainetos3a-som-v1.dts | 18 +
>> .../imx8mp-aristainetos3a-som-v1.dtsi | 1210 +++++++++++++++++
>> 11 files changed, 1718 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtso
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtso
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios.dtso
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-proton2s.dtsi
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-proton2s.dtso
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3a-som-v1.dts
>> create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-aristainetos3a-som-v1.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
>> index 9d3df8b218a2..7c3586509b8b 100644
>> --- a/arch/arm64/boot/dts/freescale/Makefile
>> +++ b/arch/arm64/boot/dts/freescale/Makefile
>> @@ -163,6 +163,11 @@ imx8mn-tqma8mqnl-mba8mx-usbotg-dtbs += imx8mn-tqma8mqnl-mba8mx.dtb imx8mn-tqma8m
>> dtb-$(CONFIG_ARCH_MXC) += imx8mn-tqma8mqnl-mba8mx-lvds-tm070jvhg33.dtb
>> dtb-$(CONFIG_ARCH_MXC) += imx8mn-tqma8mqnl-mba8mx-usbotg.dtb
>>
>> +dtb-$(CONFIG_ARCH_MXC) += imx8mp-aristainetos3a-som-v1.dtb \
>> + imx8mp-aristainetos3-adpismarc.dtbo \
>> + imx8mp-aristainetos3-proton2s.dtbo \
>> + imx8mp-aristainetos3-helios.dtbo \
>> + imx8mp-aristainetos3-helios-lvds.dtbo
>> dtb-$(CONFIG_ARCH_MXC) += imx8mp-beacon-kit.dtb
>> dtb-$(CONFIG_ARCH_MXC) += imx8mp-data-modul-edm-sbc.dtb
>> dtb-$(CONFIG_ARCH_MXC) += imx8mp-debix-model-a.dtb
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtsi
>> new file mode 100644
>> index 000000000000..cc0cddaa33ea
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtsi
>> @@ -0,0 +1,64 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Heiko Schocher <hs@...x.de>
>> + */
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +&ecspi1 {
>> + spidev0: spi@0 {
>> + reg = <0>;
>> + compatible = "rohm,dh2228fv";
>
> Hm? I have some doubts, what device is here?
$ grep -lr dh2228fv drivers/
drivers/spi/spidev.c
Customer uses an userspace implementation...
>
>> + spi-max-frequency = <500000>;
>> + };
>> +};
>> +
>> +&ecspi2 {
>> + spidev1: spi@0 {
>> + reg = <0>;
>> + compatible = "rohm,dh2228fv";
>> + spi-max-frequency = <500000>;
>> + };
>> +};
>> +
>> +&i2c2 {
>> + /* SX1509(2) U1001@IPi SMARC Plus */
>> + gpio8: i2c2_gpioext0@3e {
>
> Uh, no, please never send us downstream code.
>
> Please follow DTS coding style in all upstream submissions.
driver is in here:
$ grep -lr probe-reset drivers/pinctrl/
drivers/pinctrl/pinctrl-sx150x.c
> Also:
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Sorry for the nodenames...
And yes, I remove the comments.
>> + /* GPIO Expander 2 Mapping :
>> + * - 0: E_GPIO1_0 <=> IPi SMARC Plus CN101_PIN29: E_GPIO1_0
>> + * - 1: E_GPIO1_1 <=> IPi SMARC Plus CN101_PIN31: E_GPIO1_1
>> + * - 2: E_GPIO1_2 <=> IPi SMARC Plus CN101_PIN32: E_GPIO1_2
>> + * - 3: E_GPIO1_3 <=> IPi SMARC Plus CN101_PIN33: E_GPIO1_3
>> + * - 4: E_GPIO1_4 <=> IPi SMARC Plus CN101_PIN35: E_GPIO1_4
>> + * - 5: E_GPIO1_5 <=> IPi SMARC Plus CN101_PIN36: E_GPIO1_5
>> + * - 6: E_GPIO1_6 <=> IPi SMARC Plus CN101_PIN37: E_GPIO1_6
>> + * - 7: E_GPIO1_7 <=> IPi SMARC Plus CN101_PIN38: E_GPIO1_7
>> + * - 8: E_GPIO2_8 <=> IPi SMARC Plus CN101_PIN40: E_GPIO2_8
>> + * - 9: TP1002 <=> IPi SMARC Plus TP1002 (won't use)
>> + * - 10: TP1003 <=> IPi SMARC Plus TP1003 (won't use)
>> + * - 11: TP1004 <=> IPi SMARC Plus TP1004 (won't use)
>> + * - 12: TP1005 <=> IPi SMARC Plus TP1005 (won't use)
>> + * - 13: TP1006 <=> IPi SMARC Plus TP1006 (won't use)
>> + * - 14: TP1007 <=> IPi SMARC Plus TP1007 (won't use)
>> + * - 15: TP1008 <=> IPi SMARC Plus TP1008 (won't use)
>> + * - 16: OSCIO <=> IPi SMARC Plus TP1001 (won't use)
>> + */
>> + #gpio-cells = <2>;
>> + #interrupt-cells = <2>;
>> + compatible = "semtech,sx1509q";
>> + reg = <0x3e>;
>> +
>> + semtech,probe-reset;
>> + gpio-controller;
>> + interrupt-controller;
>> +
>> + interrupt-parent = <&gpio6>;
>> + interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
>> + };
>> +
>
> Drop
>
>> +};
>> +
>> +&flexcan1 {
>> + status = "okay";
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtso b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtso
>> new file mode 100644
>> index 000000000000..5a9adccbf7cf
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-adpismarc.dtso
>> @@ -0,0 +1,14 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Heiko Schocher <hs@...x.de>
>> + */
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +#include "imx8mp-aristainetos3-adpismarc.dtsi"
>> +
>> +&{/} {
>> + model = "Aristainetos3 ADLink PI SMARC carrier";
>> + compatible = "abb,aristainetos3-adpismarc", "imx8mp-aristianetos3",
>> + "abb,aristianetos3-som", "fsl,imx8mp";
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
Thanks for the hint! I have to fix my scripts...
> And why this is DTSO, I have no clue...
>
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtsi
>> new file mode 100644
>> index 000000000000..55aabd6fc1f7
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtsi
>> @@ -0,0 +1,89 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Heiko Schocher <hs@...x.de>
>> + */
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/pwm/pwm.h>
>> +
>> +&{/} {
>> + panel: panel {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_lcd0_vdd_en>;
>> + compatible = "lg,lb070wv8";
>> + backlight = <&lvds_backlight>;
>> + enable-gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>;
>> +
>> + port {
>> + panel_in: endpoint {
>> + remote-endpoint = <&ldb_lvds_ch0>;
>> + };
>> + };
>> + };
>> +};
>> +
>> +&gpio3 {
>> + mipi_lvds_select {
>
> No, read coding style.
>
>
>> + gpio-hog;
>> + gpios = <23 GPIO_ACTIVE_HIGH>;
>> + output-low;
>> + line-name = "mipi_lvds_select";
>> + };
>> +};
>> +
>> +&hdmi_blk_ctrl {
>> + status = "disabled";
>> +};
>> +
>> +&hdmi_pvi {
>> + status = "disabled";
>> +};
>> +
>> +&hdmi_tx {
>> + status = "disabled";
>> +};
>> +
>> +&hdmi_tx_phy {
>> + status = "disabled";
>> +};
>> +
>> +&irqsteer_hdmi {
>> + status = "disabled";
>> +};
>> +
>> +&ldb_lvds_ch0 {
>> + fsl,data-mapping = "jeida";
>> + fsl,data-width = <24>;
>> + remote-endpoint = <&panel_in>;
>> +};
>> +
>> +&lcdif1 {
>> + status = "disabled";
>> +};
>> +
>> +&lcdif2 {
>> + status = "okay";
>> +};
>> +
>> +&lcdif3 {
>> + status = "disabled";
>> +};
>> +
>> +&lvds_backlight {
>> + status = "okay";
>> +};
>> +
>> +&lvds_bridge {
>> + status = "okay";
>> +};
>> +
>> +&media_blk_ctrl {
>> + /*
>> + * The internal divider will always divide the output LVDS clock by 7
>> + * so our display needs 33246000 Hz, so set VIDEO_PLL1 to
>> + * 33246000 * 7 = 232722000 Hz
>> + */
>> + assigned-clock-rates = <500000000>, <200000000>, <0>, <0>, <232722000>;
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtso b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtso
>> new file mode 100644
>> index 000000000000..06d1883b962a
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios-lvds.dtso
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Heiko Schocher <hs@...x.de>
>> + */
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +#include "imx8mp-aristainetos3-helios-lvds.dtsi"
>> +
>> +&{/} {
>> + model = "Aristainetos3 helios LVDS carrier";
>> + compatible = "abb,aristainetos3-helios-lvds", "abb,aristainetos3-helios", "abb,aristianetos3-som", "fsl,imx8mp";
>
> Read not only DTS coding style, but also kernel coding style. Lines are
> supposed to be wrapped according to kernel coding style.
Yes, indeed, I have to fix my scripts...
>> +};
>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios.dtsi
>> new file mode 100644
>> index 000000000000..b4b1cb3b0cb3
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-aristainetos3-helios.dtsi
>> @@ -0,0 +1,103 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Heiko Schocher <hs@...x.de>
>> + */
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +
>> +&{/} {
>> + helios_gpio_leds {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
>
>> + compatible = "gpio-leds";
>> +
>> + helios_blue {
>
> So this was absolutely never tested.
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
>
>> + label = "helios:blue";
>
> Use function and color instead.
>
> I finished review here. Rest of the code does not look good, really. You
> have so many, really so many, trivial issues which tools point out, that
> using humans for such review is just waste of our time.
Sorry for that, will call the tools...
bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@...x.de
Powered by blists - more mailing lists