Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <12951175-82c5-e93f-871d-1379e5985b10@solid-run.com>
Date:   Fri, 16 Jun 2023 15:57:35 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH 3/3] arm64: dts: freescale: Add support for LX2162 SoM &
 Clearfog Board
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Cc:     Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Li Yang <leoyang.li@nxp.com>
References: <20230616110610.32173-1-josua@solid-run.com>
 <20230616110610.32173-4-josua@solid-run.com>
 <c25630ca-c791-287f-36a4-ebc0559d6541@linaro.org>
From:   Josua Mayer <josua@solid-run.com>
In-Reply-To: <c25630ca-c791-287f-36a4-ebc0559d6541@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YWpzSDlDaWdlTDZxUzNKSVhvTVNpOFNVZmNLS3kwdVhkWDA2SHZ1amx3eU54?=
 =?utf-8?B?TDQxNkRFWnBtY2lUdDVMVVM5WHI0ZWJwZWw3N2hreG0xNVVoOFd4M3lINnNp?=
 =?utf-8?B?OVNQWlN2VS9pRC8wbkptUUJHdnZTSExtWkRXY1RZZ094bFRod3RhTjQ4QUlR?=
 =?utf-8?B?VG9yMGZLcXpYSGhQRWUrRmFGcUZSb2RKeWRmenRmTVNMS2M0c3JrWmVRQ1pz?=
 =?utf-8?B?Y2RuMWpWcjdpWU9odlIyeW4ycWk0c2ZvSjFabnNkUnZQb0l6QVk0V0pvRVhx?=
 =?utf-8?B?cHNxb0UxbUNsYU9wakIwb3RCZFhLS1NpTDBLR1Z1MTdBY0lJYUl3dDUwbG9P?=
 =?utf-8?B?cVBoTkhVVzRLbVEwUE5vRGtlTWhMYk9SeVFmR0g3bGZrRy9ITEJ4ZVJCSjdY?=
 =?utf-8?B?Zk8xd0c1YmxjTzFLZnBUTHdDbjl6aXpjSmpzSHBxaWI4Q3BXSmFWaTgxYmR5?=
 =?utf-8?B?bXJJVzdHL0tDNmx5OHR3TkhQWDZ5MlhMN3p2SlM3dVZpRy94YlRqYUNSdUox?=
 =?utf-8?B?WTVCcktUVHorZ0IwQ2wxWmh5QjdwUzNYZ3BLbGNMZUJreXlaQ1FmYkkvRll2?=
 =?utf-8?B?V1J1NEFReFJUK1BXUUpOTHB6UHNkeWN0WXFNVXVicWVrRzRhY3FHSFI1My9x?=
 =?utf-8?B?SC9pVURZSFcrOGp0RDkvWUJNUk1ldlM1Z0pVeEtTUUtFcmtJc24reGh3M2JT?=
 =?utf-8?B?bktSa2Jqd2pYNGxkeWxrbm80YWRvb0NPTjFNWWwrOGFGR0l5YStJV3psRTE5?=
 =?utf-8?B?VUs3NEoyNlh4cVF4MUlhMG5JRkxIQ2FlVnhMWE45bEovMFpkTDR0ZThQckpx?=
 =?utf-8?B?Q0tMU2ZDVWkrVW14MkF2RzVHZnhRcXcxRzRMaUVlS1RDZlZnSHFSdTFuK0xL?=
 =?utf-8?B?TFhEL1BQNGRTK29zeE9UNDRQbUp5TWJFUmtqYWh2TmNDdzE0Yk1mcEZvMXZB?=
 =?utf-8?B?ODZKbUJkOGN1dmZZRXVTeGtZdWdkWEg1bjVMcVdhd0duamZzVVFrZFMyVmlF?=
 =?utf-8?B?UkI5TE93b0lJVXRBZTBTUUx0bnU4cVJsTWlZMXJ0RDdQc21VT1BPdXU5RXV3?=
 =?utf-8?B?YWwwQngxakhXMGFZQk5xVG5kR0NRVUNTMGp2emNTTXVCdFA4VXY1SEpVWm8v?=
 =?utf-8?B?bzNUZ0V4VHdkVkgveVEzNTNEemQwNHdEbEM4d0ZHSEI1UzgvVUhoTW5MN3J6?=
 =?utf-8?B?SytjYlNobkxBNmVDSG0yVjBlUm9LTnRiWGtFdDVmVkJzcXNDNGFoWHJDenpB?=
 =?utf-8?B?dXFVS2ljS0V5NTNNaUdQcDJNWDBSeDJkVzFudW9sZ21UVm1vcDlxTHl3Tzdk?=
 =?utf-8?B?Vm9wTU52TlVRRC83d2Q0YnIzVitjdGUzQ1NrZXJjZXlqZFdLSHpwM2pjK2di?=
 =?utf-8?B?bExIeXFBNTE2cGk1d1Nya2ErWnBnQzgzTUNFTHAzT1pGZXNsVXNhRlpMNmhr?=
 =?utf-8?B?bHZ2QWllSDhIV1Q0Y3NHTWhRK21NZEtCYmFjSklYVzg2a0N3alRmaWIrNTFs?=
 =?utf-8?B?Z3hWemNxaVJWWFltcWJ5aTBLaGlWd0phWEtmK2k3bGt4ZW5zSXVoMnZnaVBD?=
 =?utf-8?B?MkVSWmozSVVFKzJ1V3dWVkJHRFJ1eStrRnc4d2hrNExDV1FsOC9ETFRhY3p0?=
 =?utf-8?B?N3JDNDF4bThEMmEwSHBTclZoNmM0R3haQkp3VjMwTVc1QmV2bTZQV1R5RS96?=
 =?utf-8?B?YWJSVUlRa041TGpGTjRNbXB1cmk4NUtBbm5Pb0tYeTVhMWlHbW5DRmpNd1hH?=
 =?utf-8?B?OHN1T0VCSUpkeUtXZGdHNFJoRndZMHFMbm4rRVVzaWZWNzQyZUFtZEZBZmdq?=
 =?utf-8?B?SjlUQTNpcHVxN0hRSCtDUlhHelZtalhyTno1NUpGN3hPY0F6d0tVVHgzM0Rh?=
 =?utf-8?B?VGtYNjNReW03cjBFYTRQVFdRcXNsaHhqaDVSU1R2L1BaVXA0OElscThMMUwx?=
 =?utf-8?B?UWNELzFtZitnemN2bDhIcmdPYzNxblNHa2hpNmp6WWltUzREZmNOM1Q2cjFv?=
 =?utf-8?B?ZEVNQnVNQy9nMHRLQkY1OHJnWHZKSElFQk1ISG5yZUlSU3NsMFZJMk93UHFJ?=
 =?utf-8?B?c25LM1h1bEhoQzB6dHJ5c1E1WEhuV2pLTFJLbzdUM25JM1JUcy90ZytzbmtX?=
 =?utf-8?Q?3Oj4rlBO9cZL+DZbcXq1V9rpk?=
X-OriginatorOrg: solid-run.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 476863d6-3157-45de-54b1-08db6e6944ee
X-MS-Exchange-CrossTenant-AuthSource: AS8PR04MB8963.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2023 12:57:41.1482
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: a4a8aaf3-fd27-4e27-add2-604707ce5b82
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: OTSqhdOJeOCJ+VJKE0ID8rDcZ+FyVhO2GngDt09D0GfMDzIZVV4I1Wdq94qZq0n3QSd+gDuy0ZPBf6OJfwEgnw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB8502
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,
        SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@solid-run.com>
>> +// Copyright 2023 Josua Mayer <josua@solid-run.com>
>> +
>> +&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