Re: [PATCH v3 2/4] arm64: dts: renesas: add support for solidrun rzg2l som and hb-iiot evb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Out-of-bounds `reserved-memory` regions relative to the defined `memory` node.
- [High] Shared edge-triggered interrupt configuration causes lost interrupts.
- [Medium] Invalid USB hub topology and swapped compatible strings under the `ehci1` controller.
- [Medium] Redundant and semantically invalid `gpio-hog` nodes for interrupt inputs.
- [Low] Use of deprecated `gpio` property in fixed regulators.
- [Low] Copy-paste error in multiplexer channel comment.
--

commit 9b7832f1338d358b2387c66c4a4775d9ab744d4b
Author: Josua Mayer <josua@solid-run.com>

arm64: dts: renesas: add support for solidrun rzg2l som and hb-iiot evb

This commit adds support for the SolidRun RZ/G2L SoM on Hummingboard IIoT.
It defines device tree configurations for ethernet, eMMC, DDR, and WiFi/BT,
with overlays for configurable mux states.

Link: https://www.solid-run.com/embedded-industrial-iot/renesas-rz-family/rz-g2l-som/
Link: https://www.solid-run.com/embedded-industrial-iot/renesas-rz-family/hummingboard-rz-series-sbcs/hummingboard-rz-g2l-iot-sbc/

> diff --git a/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-common.dtsi b/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-common.dtsi
> new file mode 100644
> index 0000000000000..fbf2e7c393bf3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-common.dtsi

[ ... ]

> +	/* power for USB-A J27 behind USB Hub Port 3 */
> +	regulator-vbus-2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vbus2";
> +		regulator-always-on;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-min-microvolt = <5000000>;
> +		gpio = <&tca6416_u20 12 GPIO_ACTIVE_HIGH>;

Should this be gpios instead of gpio? Modern device tree schemas require the
plural gpios property for regulator-fixed bindings. The singular gpio property
is deprecated.

> +		enable-active-high;
> +	};
> +
> +	/* power for USB-A J27 behind USB Hub Port 4 */
> +	regulator-vbus-3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vbus3";
> +		regulator-always-on;
> +		regulator-max-microvolt = <5000000>;
> +		regulator-min-microvolt = <5000000>;
> +		gpio = <&tca6416_u20 13 GPIO_ACTIVE_HIGH>;

Should this also be updated to gpios?

[ ... ]

> +	vmmc: regulator-mmc {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vmmc";
> +		regulator-max-microvolt = <3300000>;
> +		regulator-min-microvolt = <3300000>;
> +		startup-delay-us = <250>;
> +		vin-supply = <&v_3_3>;
> +		gpio = <&pinctrl RZG2L_GPIO(4, 1) GPIO_ACTIVE_HIGH>;

Does this regulator also need the plural gpios property?

> +		enable-active-high;
> +	};
> +
> +	/* power for USB-A J5003 */
> +	vbus1: regulator-vbus-1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vbus1";
> +		regulator-max-microvolt = <5000000>;
> +		regulator-min-microvolt = <5000000>;
> +		gpio = <&tca6416_u20 14 GPIO_ACTIVE_HIGH>;

Should this use the gpios property as well?

[ ... ]

> +&ehci1 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	hub_2_0: hub@1 {
> +		compatible = "usb4b4,6502", "usb4b4,6506";

Are these compatible strings swapped with hub_3_0? The string usb4b4,6506
represents a USB 3.0 hub, but this node seems intended for the USB 2.0
portion.

> +		reg = <1>;
> +		peer-hub = <&hub_3_0>;
> +		reset-gpios = <&tca6416_u20 11 GPIO_ACTIVE_LOW>;
> +		vdd2-supply = <&v_3_3>;
> +		vdd-supply = <&v_1_2>;
> +	};
> +
> +	/* this device is not visible because host supports 2.0 only */
> +	hub_3_0: hub@2 {
> +		compatible = "usb4b4,6500", "usb4b4,6504";

The string usb4b4,6504 typically represents the USB 2.0 hub portion.

Also, does placing this USB 3.0 hub under an EHCI controller violate USB
hardware topology rules? EHCI is strictly a USB 2.0 controller.

Will claiming the same reset-gpios block lead to a resource conflict if both
hubs attempt to probe? Since the USB 3.0 portion is physically unconnected
to the SoC, should the hub_3_0 node and peer-hub references be removed?

> +		reg = <2>;
> +		peer-hub = <&hub_2_0>;
> +		reset-gpios = <&tca6416_u20 11 GPIO_ACTIVE_LOW>;
> +		vdd2-supply = <&v_3_3>;
> +		vdd-supply = <&v_1_2>;
> +	};
> +};

[ ... ]

> +		lvds-irq-hog {
> +			gpios = <13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP | GPIO_OPEN_DRAIN)>;

Is the GPIO_OPEN_DRAIN flag valid on an input pin? It is typically an output
driver flag.

Additionally, hogging an interrupt line can logically conflict with its IRQ
usage. Consumer drivers natively request the interrupt which configures the
line as an input. Could this trigger resource request failures?

> +			gpio-hog;
> +			input;
> +			line-name = "lvds-irq";
> +		};

[ ... ]

> +		sensor-irq-hog {
> +			gpios = <7 (GPIO_ACTIVE_LOW | GPIO_PULL_UP | GPIO_OPEN_DRAIN)>;

Does this sensor interrupt also incorrectly use output driver flags on an
input pin, and logically conflict with the IRQ framework?

> +			gpio-hog;
> +			input;
> +			line-name = "sensor-irq";
> +		};
> +
> +		tpm-irq-hog {
> +			gpios = <9 (GPIO_ACTIVE_LOW | GPIO_PULL_UP | GPIO_OPEN_DRAIN)>;

Is this another instance where an interrupt line is redundantly hogged as an
input with invalid flags?

> +			gpio-hog;
> +			input;
> +			line-name = "tpm-irq";
> +		};
> +	};

[ ... ]

> +	light-sensor@44 {
> +		compatible = "isil,isl29023";
> +		reg = <0x44>;
> +		/* IRQ shared between accelerometer, light-sensor and Tamper input (J5007) */
> +		interrupts-extended = <&tca6416_u21 7 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> +	accelerometer@53 {
> +		compatible = "adi,adxl345";
> +		reg = <0x53>;
> +		interrupts-extended = <&tca6416_u21 7 IRQ_TYPE_EDGE_FALLING>;

Will configuring shared physical lines as edge-triggered cause lost interrupts?
If one device pulls the line low and the second device asserts its interrupt
before the first releases it, no falling edge will be generated for the
second device. Should shared physical lines be configured as level-sensitive
instead?

> +		/* IRQ shared between accelerometer, light-sensor and Tamper input (J5007) */
> +		interrupt-names = "INT1";
> +	};

[ ... ]

> +		/* channel 2 routed to mipi-dsi connector (J25) */
> +		i2c_dsi: i2c@2 {
> +			reg = <2>;

[ ... ]

> +		/* channel 2 routed to lvds connector (J24) */
> +		i2c_lvds: i2c@3 {

This isn't a bug, but there's a copy-paste error in the comment. Should it say
channel 3 instead of channel 2?

> +			reg = <3>;

[ ... ]

> diff --git a/arch/arm64/boot/dts/renesas/rzg2l-sr-som.dtsi b/arch/arm64/boot/dts/renesas/rzg2l-sr-som.dtsi
> new file mode 100644
> index 0000000000000..297256299e7a8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/rzg2l-sr-som.dtsi

[ ... ]

> +	reserved-memory {
> +		ranges;
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +
> +		mmp_reserved: linux,multimedia@68000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0x0 0x68000000 0x0 0x8000000>;
> +			reusable;
> +		};
> +
> +		global_cma: linux,cma@58000000 {
> +			compatible = "shared-dma-pool";
> +			reg = <0x0 0x58000000 0x0 0x10000000>;
> +			reusable;
> +			linux,cma-default;
> +		};
> +	};

[ ... ]

> +	memory@40000000 {
> +		reg = <0x0 0x40000000 0x0 0x20000000>;
> +		device_type = "memory";
> +	};

Will these reserved-memory regions cause early boot crashes if the bootloader
fails to dynamically patch the memory node size at runtime?

The memory@40000000 node hardcodes the RAM size to 512MB (0x20000000), which
means the memory map ends at 0x60000000. The global_cma region is placed at
0x58000000 extending to 0x68000000, and mmp_reserved is placed at 0x68000000.
This places them largely or completely outside the explicitly defined 512MB
RAM boundaries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-rzg2-sr-boards-v3-0-f033fc96c906@solid-run.com?part=2