[PATCH 2/2] arm64: dts: imx8mm: Add i.MX8M Mini Toradex Verdin based Menlo board

[Marek Vasut]

On 4/8/22 08:46, Francesco Dolcini wrote:

Hi,

>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-mx8menlo.dts
>> @@ -0,0 +1,331 @@
>> +// SPDX-License-Identifier: GPL-2.0+ OR MIT
>> +/*
>> + * Copyright 2021-2022 Marek Vasut <marex at denx.de>
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "imx8mm-verdin.dtsi"
>> +
>> +/ {
>> +	model = "MENLO MX8MM EMBEDDED DEVICE";
>> +	compatible = "menlo,mx8menlo",
>> +		     "toradex,verdin-imx8mm",
>> +		     "fsl,imx8mm";
>> +
>> +	/delete-node/ gpio-keys;
> would it be better if we had a label in the imx8mm-verdin.dtsi and we
> could just set status=disabled here?

It would be better if there was Verdin SoM dtsi and Verdin carrier board 
dts which includes the SoM dtsi. Right now, it seems these two things 
are conflated a bit.

There are no GPIO buttons on the Verdin SoM, there are some on the 
Dahlia carrier board I think.

>> +	leds {
>> +		compatible = "gpio-leds";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_led>;
>> +
>> +		user1 {
>> +			label = "TestLed601";
>> +			gpios = <&gpio4 18 GPIO_ACTIVE_HIGH>;
>> +			linux,default-trigger = "mmc0";
>> +		};
>> +
>> +		user2 {
>> +			label = "TestLed602";
>> +			gpios = <&gpio4 10 GPIO_ACTIVE_HIGH>;
>> +			linux,default-trigger = "heartbeat";
>> +		};
>> +	};
>> +
>> +	beeper {
>> +		compatible = "gpio-beeper";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_beeper>;
>> +		gpios = <&gpio5 3 GPIO_ACTIVE_HIGH>;
>> +	};
>> +};
>> +
>> +&ecspi1 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>> +	cs-gpios = <&gpio5 9 GPIO_ACTIVE_LOW>;
>> +	status = "okay";
>> +
>> +	/* CAN controller on the baseboard */
>> +	canfd: can at 0 {
>> +		compatible = "microchip,mcp2518fd";
>> +		clocks = <&clk20m>;
> You are using the node defined in the verdin.dtsi here, while I guess
> this is a separate oscillator part of the carrier board.
> 
> Should you define a new clock? My concern is that we had discussion to
> change the SoM to move from 20 to 40 MHz, and we would remove this entry
> from the dtsi if we would do such a change.

Is the 20 MHz oscillator on your SoM or on the carrier board ?

[...]

>> +&gpio1 {
>> +	gpio-line-names =
>> +		"", "", "", "",
>> +		"", "", "", "",
>> +		"", "", "", "",
>> +		"", "", "", "",
>> +		"", "", "", "",
>> +		"", "", "", "",
>> +		"", "", "", "",
>> +		"", "", "", "";
>> +};
> 
> It does not look an elegant solution to me to clean-up the
> gpio-line-names, but I guess is the only option you have ...

Right, I need pin names which are compatible with the previous 
generation hardware.

[...]

>> +&hwmon {
>> +	status = "okay";
>> +};
> you delete this node in a few lines, why setting the status?
> (`hwmon: hwmon at 40`)

This one is superfluous indeed.

>> +&i2c1 {
>> +	/* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
>> +	clock-frequency = <100000>;
> should this and the related changes in the other i2c nodes done in the
> verdin.dtsi? Marcel? (errata is here:
> https://www.nxp.com/docs/en/errata/IMX8MDQLQ_1N14W.pdf)

More like the imx8m{m,n,p,q}.dtsi should set this to 320000 because that 
is nicely divided down and well within tolerance, and then boards should 
be able to override it to whatever frequency they want.

Maybe this should even be limited in the imx i2c controller driver itself?

>> +};
>> +
>> +&i2c2 {
>> +	/* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
>> +	clock-frequency = <100000>;
>> +};
>> +
>> +&i2c3 {
>> +	/* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
>> +	clock-frequency = <100000>;
>> +	status = "okay";
>> +};
>> +
>> +&i2c4 {
>> +	/* IMX8MM ERRATA e7805 -- I2C is limited to 384 kHz due to SoC bug */
>> +	clock-frequency = <100000>;
>> +	/delete-node/ bridge at 2c;
>> +	/delete-node/ hwmon at 40;
>> +	/delete-node/ hdmi at 48;
>> +	/delete-node/ touch at 4a;
>> +	/delete-node/ hwmontemp at 4f;
>> +	/delete-node/ eeprom at 50;
>> +	/delete-node/ eeprom at 57;
> All of those are disabled in the dtsi, is it really worth deleting the
> nodes?

They are not present on the hardware, why should they be described in 
the DT ? Hence the /delete-node/ .

That reminds me -- why is there lt8912 described in verdin.dtsi when 
this lt8912 chip is not on the SoM, not even on the carrier board, but 
on an optional carrier board expansion card ?

[...]