Re: [PATCH 3/3] arm64: dts: freescale: Add support for LX2162 SoM & Clearfog Board

[Josua Mayer <josua@xxxxxxxxxxxxx>]

Hi Krzysztof,

Thank you for the comments!
Before sending a v2, I will:
- move all "reg" properties just after "compatible"
- give the phy nodes generic names
- remove dead code / review spi-nor-flash node

Am 16.06.23 um 14:39 schrieb Krzysztof Kozlowski:
> On 16/06/2023 13:06, Josua Mayer wrote:
> > Add support for the SolidRun LX2162A System on Module (SoM), and the
> > Clearfog evaluation board.
> >
> > The SoM has few software-controllable features:
> > - AR8035 Ethernet PHY
> > - eMMC
> > - SPI Flash
> > - fan controller
> > - various eeproms
> Thank you for your patch. There is something to discuss/improve.
>
> > +
> > +&i2c2 {
> > +	/* retimer: ds250df410@18 */
> > +
> > +	i2c-switch@70 {
> > +		compatible = "nxp,pca9546";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		reg = <0x70>;
> reg is usually just after compatible.
>
> ...
>
> > +
> > +&emdio1 {
> > +	/*
> > +	 * SoM has a phy at address 1 connected to SoC Ethernet Controller 1.
> > +	 * It competes for WRIOP MAC17, and no connector has been wired.
> > +	 */
> > +	/delete-node/ ethernet-phy@1;
> > +
> > +	ethernet_phy0: mv88e2580@8 {
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> > +		reg = <8>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy1: mv88e2580@9 {
> > +		reg = <9>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy2: mv88e2580@a {
> > +		reg = <10>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy3: mv88e2580@b {
> > +		reg = <11>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy4: mv88e2580@c {
> > +		reg = <12>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy5: mv88e2580@d {
> > +		reg = <13>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy6: mv88e2580@e {
> > +		reg = <14>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +
> > +	ethernet_phy7: mv88e2580@f {
> > +		reg = <15>;
> > +		compatible = "ethernet-phy-ieee802.3-c45";
> > +		max-speed = <1000>;
> > +	};
> > +};
> > +
> > +&esdhc0 {
> > +	status = "okay";
> > +	sd-uhs-sdr104;
> > +	sd-uhs-sdr50;
> > +	sd-uhs-sdr25;
> > +	sd-uhs-sdr12;
> > +};
> > +
> > +&pcie3 {
> > +	status = "disabled";
> > +};
> > +
> > +&pcie4 {
> > +	status = "disabled";
> > +};
> > +
> > +&pcs_mdio3 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio4 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio5 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio6 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio11 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio12 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio13 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio14 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio15 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio16 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio17 {
> > +	status = "okay";
> > +};
> > +
> > +&pcs_mdio18 {
> > +	status = "okay";
> > +};
> > +
> > +&serdes_1 {
> > +	status = "okay";
> > +};
> > +
> > +&serdes_2 {
> > +	status = "okay";
> > +};
> > +
> > +&uart0 {
> > +	status = "okay";
> > +};
> > +
> > +&usb0 {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi
> > new file mode 100644
> > index 000000000000..49e03b6600d6
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi
> > @@ -0,0 +1,78 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +//
> > +// Device Tree file for LX2162A-SOM
> > +//
> > +// Copyright 2021 Rabeeh Khoury <rabeeh@xxxxxxxxxxxxx>
> > +// Copyright 2023 Josua Mayer <josua@xxxxxxxxxxxxx>
> > +
> > +&crypto {
> > +	status = "okay";
> > +};
> > +
> > +&dpmac17 {
> > +	phy-handle = <&ethernet_phy0>;
> > +	phy-connection-type = "rgmii-id";
> > +};
> > +
> > +&emdio1 {
> > +	status = "okay";
> > +
> > +	ethernet_phy0: ethernet-phy@1 {
> > +		reg = <1>;
> > +	};
> > +};
> > +
> > +&esdhc1 {
> > +	status = "okay";
> > +	bus-width = <8>;
> > +	mmc-hs200-1_8v;
> > +	mmc-hs400-1_8v;
> > +};
> > +
> > +&fspi {
> > +	status = "okay";
> > +
> > +	flash@0 {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "m25p80", "jedec,spi-nor";
> > +		m25p,fast-read;
> > +		spi-max-frequency = <50000000>;
> > +		reg = <0>;
> Fix the order of properties. compatible is always first, then the reg.
>
> > +		/* The following setting enables 1-1-8 (CMD-ADDR-DATA) mode */
> > +		spi-rx-bus-width = <8>;
> > +		// spi-tx-bus-width = <8>;
> Don't add dead code without appropriate comment why it is dead.
>
> > +	};
> > +};
> > +
> > +&i2c0 {
> > +	status = "okay";
> > +
> > +	fan-controller@18 {
> > +		compatible = "ti,amc6821";
> > +		reg = <0x18>;
> > +		cooling-min-state = <0>;
> > +		cooling-max-state = <9>;
> > +		#cooling-cells = <2>;
> > +	};
> > +
> > +	ddr_spd: eeprom@51 {
> > +		reg = <0x57>;
> > +		compatible = "st,24c02", "atmel,24c02";
> Ditto
>
> > +		read-only;
> > +	};
> > +
> > +	config_eeprom: eeprom@57 {
> > +		reg = <0x57>;
> > +		compatible = "st,24c02", "atmel,24c02";
> Ditto
>
> > +	};
> > +};
> > +
> > +&i2c4 {
> > +	status = "okay";
> > +
> > +	variable_eeprom: eeprom@54 {
> > +		reg = <0x54>;
> > +		compatible = "st,24c2048", "atmel,24c2048";
> ditto
>
> > +	};
> > +};
> Best regards,
> Krzysztof
sincerely
Josua Mayer