Re: [PATCH v5 2/2] arm64: dts: imx8dxl: Add SolidRun SoM and HummingBoard

[Josua Mayer <josua@solid-run.com>]

Am 14.05.26 um 12:35 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The Quectel cellular modem (`usb-device@1`) will never be powered on because its DT node is unhandled.
> - [High] The `ethernet1` alias is explicitly assigned but then immediately deleted within the same node block, breaking MAC address assignment.
> - [Medium] The `usdhc2` host controller is enabled and marked `non-removable` for omitted hardware.
> --
>
> commit a7ba7e5c27b439ec498400fe760e61ca8b0b035c
> Author: Josua Mayer <josua@solid-run.com>
>
> arm64: dts: imx8dxl: Add SolidRun SoM and HummingBoard
>
> Add support for the SolidRun i.MX8DXL System-on-Module (revision 2.1)
> and its corresponding evaluation carrier board, the HummingBoard
> Telematics (revision 2.0).
>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8dxl-hummingboard-telematics.dts b/arch/arm64/boot/dts/freescale/imx8dxl-hummingboard-telematics.dts
>> new file mode 100644
>> index 0000000000000..5f42c4f2e3be9
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8dxl-hummingboard-telematics.dts
>> @@ -0,0 +1,536 @@
> [ ... ]
>> +	aliases {
>> +		/* override ethernet aliases from imx8dxl.dtsi */
>> +		ethernet0 = &eqos;
>> +		ethernet1 = &switch_port1;
>> +		ethernet2 = &switch_port2;
>> +		ethernet3 = &switch_port3;
>> +		ethernet4 = &switch_port4;
>> +		ethernet5 = &switch_port5;
>> +		ethernet6 = &switch_port6;
>> +		ethernet7 = &switch_port7;
>> +		ethernet8 = &switch_port8;
>> +		ethernet9 = &switch_port9;
>> +		ethernet10 = &switch_port10;
>> +		/delete-property/ ethernet1;
Leftover from previous revision before switch ports had aliases.
Dropping in v6.
> Will deleting this alias break MAC address assignment? 
>
> Because the Device Tree Compiler evaluates properties sequentially within the
> same node block, this removes the ethernet1 alias from the final compiled
> device tree.
>
> Since bootloaders typically iterate over ethernet%d aliases starting from 0,
> won't breaking the sequence cause the iteration to abort prematurely and
> leave ports 2 through 10 without MAC addresses?
>
> [ ... ]
>> +/* cellular modem */
>> +&usbotg2 {
> [ ... ]
>> +	usb-device@1 {
>> +		compatible = "usb2c7c,125";
>> +		reg = <1>;
>> +		reset-gpios = <&lsio_gpio1 11 GPIO_ACTIVE_LOW>;
>> +		vbus-supply = <&v_3_3>;
>> +		vdd-supply = <&modem_vbat>;
>> +	};
>> +};
> Will this Quectel cellular modem ever be powered on? 

Yes. By the bluetooth driver because of the name "vbat" :(

root@58581b109b8e:~# cat /sys/class/regulator/regulator.8/uevent
OF_NAME=regulator-modem-vbat
OF_FULLNAME=/regulator-modem-vbat
OF_COMPATIBLE_0=regulator-fixed
OF_COMPATIBLE_N=1
root@58581b109b8e:~# cat /sys/class/regulator/regulator.8/state 
enabled
root@58581b109b8e:~# cat /sys/class/regulator/regulator.8/consumer\:serial\:serial0-0/consumer/uevent 
DRIVER=hci_uart_bcm
OF_NAME=bluetooth
OF_FULLNAME=/bus@5a000000/serial@5a070000/bluetooth
OF_COMPATIBLE_0=brcm,bcm4345c5
OF_COMPATIBLE_N=1
MODALIAS=of:NbluetoothT(null)Cbrcm,bcm4345c5

I will change this for v6, however getting the modem to actually power on
is not a priority while writing hardware description.

>
> Since generic USB devices are dynamically enumerated by the host, the kernel
> relies exclusively on the onboard_usb_dev platform driver to power them up
> before enumeration. 
>
> Since usb2c7c,125 is not in the onboard_dev_match table and there is no
> generic power sequence fallback in the USB core, won't this node be ignored?
> This would leave modem_vbat disabled, the reset pin unhandled, and the modem
> physically unpowered and invisible to the USB host.
>
>> diff --git a/arch/arm64/boot/dts/freescale/imx8dxl-sr-som.dtsi b/arch/arm64/boot/dts/freescale/imx8dxl-sr-som.dtsi
>> new file mode 100644
>> index 0000000000000..93a0eb4d7f770
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8dxl-sr-som.dtsi
>> @@ -0,0 +1,458 @@
> [ ... ]
>> +/* DSRC Radio */
>> +&usdhc2 {
>> +	bus-width = <4>;
>> +	keep-power-in-suspend;
>> +	max-frequency = <40000000>;
>> +	non-removable;
> Does this cause boot delays and MMC enumeration timeouts?

No.

[   12.853689] mmc2: SDHCI controller on 5b030000.mmc [5b030000.mmc] using ADMA
[   13.038719] mmc2: new high speed SDIO card at address 0001

>
> The commit message notes the V2X DSRC Radio peripheral device node is
> intentionally omitted due to a lack of upstream bindings.
>
> Because the controller expects a permanently attached card and is marked as
> non-removable, won't the MMC core attempt to initialize it during boot and
> fail since the peripheral power sequence and resets are not driven by any
> node?
>
> Should the host controller be marked with status = "disabled" until the
> peripheral is actually supported and present in the DT?
No. The card is detected, just no driver binds to it beyond sdhi.
>
>> +	no-sd;
>> +	pinctrl-0 = <&usdhc2_pins>, <&dsrc_pins>;
>> +	pinctrl-names = "default";
>> +	vmmc-supply = <&v_3_3>;
>> +	vqmmc-supply = <&v_1_8>;
>> +	status = "okay";
>> +};