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

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

Am 13.05.26 um 01:05 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] USB OTG1 port is configured as peripheral but breaks out to a Type-A host connector.
> - [High] Inconsistent and incorrect pad configurations for I2C2 and I2C3 GPIO recovery pins.
> - [Medium] The 'reset-duration-us' property in the 'usb-device@1' node is undocumented, unparsed by any kernel driver, and will fail schema validation.
> - [Low] Mismatch in the number of 100Base-T1 add-on ports between commit message and device tree.
> --
>
> commit f382c89d7dfc4938b73c73c681a3544f6363435e
> Author: Josua Mayer <josua@solid-run.com>
>
> arm64: dts: imx8dxl: Add SolidRun SoM and HummingBoard
>
> This commit adds support for the SolidRun i.MX8DXL System-on-Module and its
> HummingBoard Telematics carrier board. It configures various hardware
> interfaces including ethernet, CAN, USB, a cellular modem, and I2C sensors.
>
>> The multi-interface add-on board connector provides additional
>> interfaces (4x 100Base-T1, 2x SGMII, USB 2.0 shared with the cellular
>> modem, CAN, MDIO, SPI, UART, PCIe, I2C, and GPIO).
> This isn't a bug, but there is a mismatch between the commit message and the
> device tree. The message mentions 4x 100Base-T1 interfaces, but the code
> defines 5 disabled ports (port@5 through port@9) and 5 corresponding PHYs
> for the add-on connector. Could the commit message be updated to match the
> implemented ports?
Queued for v5.
>
>> 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..7e822cbd7a528
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/freescale/imx8dxl-hummingboard-telematics.dts
> [ ... ]
>
>> +&usbotg1 {
>> +	vbus-supply = <&vbus1>;
>> +};
> Since the base SoM device tree sets dr_mode to peripheral, and the carrier
> board does not override it to host or otg, will this controller remain in
> peripheral mode?
There is no automatic switching due to lack of ID pin on connector.
>
> Because this port breaks out to a USB Type-A connector on the evaluation kit,
> leaving it as a peripheral means it will not drive the VBUS regulator to
> power attached devices.
That is okay.
> Should this be explicitly set to host mode here on
> the carrier board?
For the development kit / vendor BSP we had to rely on peripheral mode during
programming of Bootloader and OS to eMMC.

It can be argued that after production once bootloader is programmed to eMMC,
users do not need to use peripheral mode and can deploy OS by USB or Network.

I will change it to host in v5.

>
> [ ... ]
>
>> +/* cellular modem */
>> +&usbotg2 {
> [ ... ]
>
>> +	usb-device@1 {
>> +		compatible = "usb2c7c,125";
>> +		reg = <1>;
>> +		reset-duration-us = <150000>;
> Is this reset-duration-us property supported by any upstream driver or
> documented in the device tree bindings?
>
> Because it appears to be unparsed by any kernel driver, will its inclusion
> cause schema validation warnings during the binding check?

This passes make dtbs_check.

However since it is undocumented I will drop the delay for v5.
It can be re-added with driver support if it turns out to be required at a later point.

>
> [ ... ]
>
>> 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
> [ ... ]
>
>> +	i2c2_gpio_pins: pinctrl-i2c2-gpio-grp {
>> +		fsl,pins = <
>> +			/* io with pull-up and weak drive */
>> +			IMX8DXL_SPI1_SCK_LSIO_GPIO3_IO00		0x00000021
>> +			/* io with pull-up, weak drive, open-drain */
>> +			IMX8DXL_SPI1_SDO_LSIO_GPIO3_IO01		0x02000021
>> +		>;
>> +	};
> Do these GPIO values configure the pads correctly for I2C bus recovery?
>
> The comments mention io with pull-up, but 0x00000021 and 0x02000021 both
> lack the pull-up enable bits present in the active I2C configuration.
In i.MX8DXL Bits 6-5 are pull-up/down. I.e. 0x20 means pull-up.

Bit 0 means drive strength where 1 = weak.

Bits 26-25 are for open-drain, i.e. 0x2000000 = open drain IO

So pinconf bits match comments.

>
> Additionally, does using 0x00000021 for SDA leave the open-drain bit unset?
> Both lines generally must be open-drain during I2C bus recovery. If SDA is
> actively driven high via push-pull while a stuck slave device pulls the line
> low, can this cause an electrical short circuit on the bus?
i2c node specifies OPEN_DRAIN bit, which means gpio driver will avoid
push-pull keeping the pin as input without relying on pinconf:

&i2c2 {
    scl-gpios = <&lsio_gpio3 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
    sda-gpios = <&lsio_gpio3 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
};

>
> [ ... ]
>
>> +	i2c3_gpio_pins: pinctrl-i2c3-gpio-grp {
>> +		fsl,pins = <
>> +			/* io with pull-up and weak drive */
>> +			IMX8DXL_SPI1_CS0_LSIO_GPIO3_IO03		0x00000021
>> +			/* io with pull-up, weak drive, open-drain */
>> +			IMX8DXL_SPI1_SDI_LSIO_GPIO3_IO02		0x02000021
>> +		>;
>> +	};
> Does the same configuration issue apply to these I2C3 recovery pins as well?
These are correct, too.