linux-kernel - Re: [PATCH v4 2/2] arm64: dts: imx: Add imx8mp-iota2-lumpy board

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] [day] [month] [year] [list]
Message-ID: <20240926075024.777bdooiqd5myv3v@pengutronix.de>
Date: Thu, 26 Sep 2024 09:50:24 +0200
From: Marco Felsch <m.felsch@...gutronix.de>
To: Frank Li <Frank.li@....com>
Cc: Michal Vokáč <michal.vokac@...ft.com>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Fabio Estevam <festevam@...il.com>,
	Michael Walle <mwalle@...nel.org>, devicetree@...r.kernel.org,
	Alexander Stein <alexander.stein@...tq-group.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Mathieu Othacehe <m.othacehe@...il.com>,
	Hugo Villeneuve <hvilleneuve@...onoff.com>,
	linux-kernel@...r.kernel.org,
	Hiago De Franco <hiago.franco@...adex.com>,
	Herburger <gregor.herburger@...tq-group.com>,
	Petr Benes <petr.benes@...ft.com>, linux-usb@...r.kernel.org,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Joao Paulo Goncalves <joao.goncalves@...adex.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, imx@...ts.linux.dev,
	Shawn Guo <shawnguo@...nel.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 2/2] arm64: dts: imx: Add imx8mp-iota2-lumpy board

On 24-09-25, Frank Li wrote:
> On Wed, Sep 25, 2024 at 01:30:31PM +0200, Michal Vokáč wrote:
> > On 24. 09. 24 19:37, Marco Felsch wrote:
> > > Hi Frank,
> > >
> > > On 24-09-24, Frank Li wrote:
> > > > On Tue, Sep 24, 2024 at 12:39:41PM +0200, Michal Vokáč wrote:
> > > > > The IOTA2 Lumpy board is based on the i.MX8MPlus EVK.
> > > > >
> > > > > Basic features are:
> > > > > - 4GB LPDDR4
> > > > > - 64GB eMMC
> > > > > - 2x 1GB Ethernet
> > > > > - USB 3.0 Type-C dual role port, without power delivery
> > > > > - USB 3.0 Type-A host port
> > > > > - RGB LED - PWM driven
> > > > > - speaker - PWM driven
> > > > > - RTC with super capacitor backup
> > > > >
> > > > > Signed-off-by: Michal Vokáč <michal.vokac@...ft.com>
> > > > > ---
> > > > > v4:
> > > > > - Moved the iomuxc node to the end of the file.
> > > > > - Moved the bus-width and non-removeable properties below
> > > > >    the pinctrl-* properties in &usdhc3 node.
> > > > > - Moved the fsl,ext-reset-output below the pinctrl-* properties
> > > > >    in &wdog1 node.
> > > > > v3:
> > > > > - Dropped pinctrl-names property from &usb_dwc3_1 node.
> > > > > v2:
> > > > > - Dropped unused property from pwm4 node.
> > > > > - Sorted all nodes and properties using dt-format tool from Frank.
> > > > >
> > > > >   arch/arm64/boot/dts/freescale/Makefile        |   1 +
> > > > >   .../boot/dts/freescale/imx8mp-iota2-lumpy.dts | 423 ++++++++++++++++++
> > > >
> > > > Suggest use https://github.com/lznuaa/dt-format
> > > > sort node. any issue, let me know.
> > >
> > > Thanks for the link :) would be nice to have this script to be part of
> > > the kernel.
> 
> It depend on how much people like and use it.

I don't see any reason why the kernel shouldn't have such a script, it
makes the life easier for all of us (incl. the dt-maintainers). By that
I mean the idea of having such a script since I actually didn't looked
into your code.

> >> The script follows the rules in [1] I'm just used to have
> > > common properties like pinctrl-* in front of the device specific
> > > properties e.g. "enable-active-high". But this rule is not part of [1]
> > > so I can't blame the script.
> 
> I just write it. Not 100% align order-of-properties-in-device-node yet.
> Some propertiy need special treated. Thank you provide the feedback.
> 
> I push change, enable-active-high and gpio will after regulator*.

:) Thank you!

Regards,
  Marco

> 
> Frank
> 
> > >
> > > Regards,
> > >    Marco
> > >
> > > [1] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node
> >
> > Thank you for the review Frank & Marco.
> > I quickly went through the file again and found another few properties
> > that could be better ordered according to the kernel documentation [1].
> >
> > > > >   2 files changed, 424 insertions(+)
> > > > >   create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-iota2-lumpy.dts
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> > > > > index 9d3df8b218a2..aa26a50b7bb4 100644
> > > > > --- a/arch/arm64/boot/dts/freescale/Makefile
> > > > > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > > > > @@ -171,6 +171,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mp-dhcom-pdk2.dtb
> > > > >   dtb-$(CONFIG_ARCH_MXC) += imx8mp-dhcom-pdk3.dtb
> > > > >   dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
> > > > >   dtb-$(CONFIG_ARCH_MXC) += imx8mp-icore-mx8mp-edimm2.2.dtb
> > > > > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-iota2-lumpy.dtb
> > > > >   dtb-$(CONFIG_ARCH_MXC) += imx8mp-msc-sm2s-ep1.dtb
> > > > >   dtb-$(CONFIG_ARCH_MXC) += imx8mp-navqp.dtb
> > > > >   dtb-$(CONFIG_ARCH_MXC) += imx8mp-phyboard-pollux-rdk.dtb
> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-iota2-lumpy.dts b/arch/arm64/boot/dts/freescale/imx8mp-iota2-lumpy.dts
> > > > > new file mode 100644
> > > > > index 000000000000..9eb58e818dc7
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp-iota2-lumpy.dts
> > > > > @@ -0,0 +1,423 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > +/*
> > > > > + * Copyright 2023 Y Soft
> > > > > + */
> > > > > +
> > > > > +/dts-v1/;
> > > > > +
> > > > > +#include "imx8mp.dtsi"
> > > > > +
> > > > > +/ {
> > > > > +	compatible = "ysoft,imx8mp-iota2-lumpy", "fsl,imx8mp";
> > > > > +	model = "Y Soft i.MX8MPlus IOTA2 Lumpy board";
> > > > > +
> > > > > +	beeper {
> > > > > +		compatible = "pwm-beeper";
> > > > > +		pwms = <&pwm4 0 500000 0>;
> > > > > +	};
> > > > > +
> > > > > +	chosen {
> > > > > +		stdout-path = &uart2;
> > > > > +	};
> > > > > +
> > > > > +	gpio_keys: gpio-keys {
> > > > > +		compatible = "gpio-keys";
> > > > > +		pinctrl-0 = <&pinctrl_gpio_keys>;
> > > > > +		pinctrl-names = "default";
> > > > > +
> > > > > +		button-reset {
> > > > > +			gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
> > > > > +			label = "Factory RESET";
> > > > > +			linux,code = <BTN_0>;
> > > > > +		};
> > > > > +	};
> > > > > +
> > > > > +	reg_usb_host: regulator-usb-host {
> > > > > +		compatible = "regulator-fixed";
> > > > > +		enable-active-high;
> > > > > +		gpio = <&gpio1 14 GPIO_ACTIVE_HIGH>;
> >
> > The enable-active-high and gpio should go bellow regulator-*.
> >
> > > > > +		pinctrl-0 = <&pinctrl_usb_host_vbus>;
> > > > > +		pinctrl-names = "default";
> > > > > +		regulator-max-microvolt = <5000000>;
> > > > > +		regulator-min-microvolt = <5000000>;
> > > > > +		regulator-name = "usb-host";
> > > > > +	};
> > > > > +
> > > > > +	memory@...00000 {
> > > > > +		reg = <0x0 0x40000000 0 0x80000000>,
> > > > > +		      <0x1 0x00000000 0 0x80000000>;
> > > > > +		device_type = "memory";
> > > > > +	};
> > > > > +};
> > > > > +
> > > > > +&A53_0 {
> > > > > +	cpu-supply = <&reg_arm>;
> > > > > +};
> > > > > +
> > > > > +&A53_1 {
> > > > > +	cpu-supply = <&reg_arm>;
> > > > > +};
> > > > > +
> > > > > +&A53_2 {
> > > > > +	cpu-supply = <&reg_arm>;
> > > > > +};
> > > > > +
> > > > > +&A53_3 {
> > > > > +	cpu-supply = <&reg_arm>;
> > > > > +};
> > > > > +
> > > > > +&eqos {
> > > > > +	phy-handle = <&ethphy0>;
> > > > > +	phy-mode = "rgmii-id";
> > > > > +	pinctrl-0 = <&pinctrl_eqos>;
> > > > > +	pinctrl-names = "default";
> > > > > +	status = "okay";
> > > > > +
> > > > > +	mdio {
> > > > > +		compatible = "snps,dwmac-mdio";
> > > > > +		#address-cells = <1>;
> > > > > +		#size-cells = <0>;
> > > > > +
> > > > > +		ethphy0: ethernet-phy@0 {
> > > > > +			reg = <0>;
> > > > > +			interrupts = <21 IRQ_TYPE_LEVEL_LOW>;
> > > > > +			interrupt-parent = <&gpio3>;
> > > > > +			micrel,led-mode = <0>;
> >
> > The micrel,* is a vendor specific property. It should go bellow the reset-*.
> >
> > > > > +			pinctrl-0 = <&pinctrl_ethphy0>;
> > > > > +			pinctrl-names = "default";
> > > > > +			reset-assert-us = <1000>;
> > > > > +			reset-deassert-us = <1000>;
> > > > > +			reset-gpios = <&gpio3 22 GPIO_ACTIVE_LOW>;
> > > > > +		};
> > > > > +	};
> > > > > +};
> > > > > +
> > > > > +&fec {
> > > > > +	fsl,magic-packet;
> > > > > +	phy-handle = <&ethphy1>;
> > > > > +	phy-mode = "rgmii-id";
> > > > > +	pinctrl-0 = <&pinctrl_fec>;
> > > > > +	pinctrl-names = "default";
> > > > > +	status = "okay";
> > > > > +
> > > > > +	mdio {
> > > > > +		#address-cells = <1>;
> > > > > +		#size-cells = <0>;
> > > > > +
> > > > > +		ethphy1: ethernet-phy@0 {
> > > > > +			reg = <0>;
> > > > > +			interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
> > > > > +			interrupt-parent = <&gpio3>;
> > > > > +			micrel,led-mode = <0>;
> >
> > Same as above, micrel,* should go bellow common properties.
> > I will send a v5 with these fixed.
> >
> > Michal
> >
> > > > > +			pinctrl-0 = <&pinctrl_ethphy1>;
> > > > > +			pinctrl-names = "default";
> > > > > +			reset-assert-us = <1000>;
> > > > > +			reset-deassert-us = <1000>;
> > > > > +			reset-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>;
> > > > > +		};
> > > > > +	};
> > > > > +};
> > > > > +
>