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

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

[Josua Mayer]

Am 12.05.26 um 04:28 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
> - [High] USB VBUS pinmux nodes are defined but never referenced, breaking USB host power.
> - [High] Edge-triggered interrupt for TCAL6416 GPIO expander leads to lost interrupts.
> - [High] Inverse polarity on `rfkill-m2-b-gnss` disable GPIO will disable the GPS when it should be enabled.
> - [High] Omitted `GPIO_OPEN_DRAIN` flag on I2C mux reset line forces dangerous push-pull drive.
> - [Medium] Missing SDIO interrupt capabilities on WiFi MMC interface degrades performance.
> - [Medium] Pinctrl strict locking prevents documented manual RS485 toggling.
> - [Medium] Contradictory regulator polarity flags in `vmmc` create ambiguous regulator state.
> - [Medium] Unimplemented internal pull-ups on GPIO hogs lead to floating open-drain lines.
> - [Low] Improper append operator `+=` used for multi-blob DTB definitions in Makefile.
> --
>
> commit 264a692a0b295dedac1ff5214c5f24eb1c644647
> 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,
> including definitions for Ethernet, eMMC, DDR, and WiFi/Bluetooth.
> Descriptions for eMMC, microSD, and RS485 are provided as overlays due to
> their dependency on configurable mux states.
>
> [1] https://www.solid-run.com/embedded-industrial-iot/renesas-rz-family/rz-g2l-som/
> [2] 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/Makefile b/arch/arm64/boot/dts/renesas/Makefile
>> index ca45d2857ea7f..37cfefe66a308 100644
>> --- a/arch/arm64/boot/dts/renesas/Makefile
>> +++ b/arch/arm64/boot/dts/renesas/Makefile
>> @@ -169,6 +169,19 @@ dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044c2-smarc-cru-csi-ov5645.dtbo
>>  r9a07g044c2-smarc-cru-csi-ov5645-dtbs := r9a07g044c2-smarc.dtb r9a07g044c2-smarc-cru-csi-ov5645.dtbo
>>  dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044c2-smarc-cru-csi-ov5645.dtb
>>  
>> +dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044l2-hummingboard-iiot.dtb
>> +dtb-$(CONFIG_ARCH_R9A07G044) += rzg2l-sr-som-emmc.dtbo
>> +r9a07g044l2-hummingboard-iiot-emmc-dtbs += r9a07g044l2-hummingboard-iiot.dtb rzg2l-sr-som-emmc.dtbo
> Is it safer to use the exact assignment operator := instead of the append
> operator += for composite device tree blobs? Standard Kbuild convention
> dictates using := for <target>-dtbs to ensure immediate evaluation and avoid
> any risk of build pollution from inherited environment variables.
Queued for v3.
>
> [ ... ]
>> 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..ff2c42bcffe2f
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-common.dtsi
> [ ... ]
>> +	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_LOW>;
>> +		enable-active-high;
>> +	};
> Mixing enable-active-high and GPIO_ACTIVE_LOW creates an ambiguous
> configuration. The fixed regulator driver interprets enable-active-high as a
> physical active-high requirement, but modern gpiolib automatically handles
> the GPIO_ACTIVE_LOW inversion at the descriptor level.
>
> Could this conflicting double-inversion cause the regulator to be driven to
> the wrong physical state and fail to power the eMMC module?

No double inversion because regulator driver ignores gpio flags.

Will align flags for v3, enable-active-high is intentional.

>
> [ ... ]
>> +	rfkill-m2-b-gnss {
>> +		compatible = "rfkill-gpio";
>> +		/* rfkill-gpio inverts internally */
>> +		shutdown-gpios = <&tca6416_u20 10 GPIO_ACTIVE_HIGH>;
>> +		label = "m2-b gnss";
>> +		radio-type = "gps";
>> +	};
> The rfkill-gpio driver asserts shutdown_gpios (drives a logical 1) when the
> radio is turned on. With GPIO_ACTIVE_HIGH, a logical 1 maps to a physical
> HIGH signal. 
>
> Since the pin is an active-low enable (GPS_EN#), driving it physically HIGH
> will disable the GPS. Does this cause the kernel to disable the GPS radio
> when it attempts to enable it? Should the polarity be GPIO_ACTIVE_LOW?

From rfkill-gpio driver:

gpiod_set_value_cansleep(rfkill->shutdown_gpio, !blocked);

this means deassert when blocked, assert when unblocked.

I have not seen an actual M.2 cecllular modem with builtin gps pinout,
so I am not sure what is correct.

Going strictly by board schematics, this is m.2 b-key pin 26, labelled M.2B_GPS_EN#,
indicating it enables gps when low.

So I shall change gpio flag accordingly for v3.

>
> [ ... ]
>> +	tca6416_u21: gpio@21 {
>> +		compatible = "ti,tcal6416";
>> +		reg = <0x21>;
>> +		#interrupt-cells = <2>;
>> +		interrupt-controller;
>> +		#gpio-cells = <2>;
>> +		gpio-controller;
>> +		gpio-line-names = "TCA_SPI_TPM/EXT", "TCA_TPM_RST#",
>> +				  "TCA_I2C_RST", "TCA_RS232_SHTD#",
>> +				  "TCA_LCD_I2C_RST", "TCA_DIG_OUT1",
>> +				  "TCA_bDIG_IN1", "TCA_SENS_INT",
>> +				  "TCA_ALERT#", "TCA_TPM_PIRQ#",
>> +				  "TCA_RTC_INT", "TCA_M.2M_WAKW_ON_LAN",
>> +				  "TCA_M.2M_CLKREQ#", "TCA_LVDS_INT#",
>> +				  "", "TCA_POE_AT";
>> +		interrupts-extended = <&pinctrl RZG2L_GPIO(4, 0) IRQ_TYPE_EDGE_FALLING>;
> The hardware INT pin for the TCAL6416 GPIO expander is an active-low
> level-sensitive output that remains low as long as unread state changes
> exist. 
>
> If this is configured as IRQ_TYPE_EDGE_FALLING, could the kernel miss
> subsequent interrupts if a second GPIO state change occurs before the first
> is cleared, since the line will remain low without generating a new falling
> edge? Should this be IRQ_TYPE_LEVEL_LOW instead?
Level interrupts did not work well in Renesas vendor fork,
and don't work at all in v6.1-rc1.

Interrupts may be lost but this is the best setting for now.

Adding comment for v3.

>
>> +
>> +		lcd-i2c-reset-hog {
>> +			gpios = <4 (GPIO_ACTIVE_LOW|GPIO_PULL_UP|GPIO_OPEN_DRAIN)>;
> The ti,tcal6416 chip is driven by gpio-pca953x, which does not implement the
> .set_config callback for this specific IC because it lacks the PCA_PCAL
> hardware flag. 
>
> Will gpiolib silently ignore the GPIO_PULL_UP flag? For open-drain lines
> like the lcd reset and the TPM interrupt below, could this leave the lines
> floating and lead to spurious interrupts or unstable hardware states if
> there are no external board-level pull-ups?
>
> [ ... ]
>> +	i2c-mux@70 {
>> +		compatible = "nxp,pca9546";
>> +		reg = <0x70>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		/*
>> +		 * This reset is open drain,
>> +		 * but reset core does not support GPIO_OPEN_DRAIN flag.
>> +		 */
>> +		reset-gpios = <&tca6416_u21 2 GPIO_ACTIVE_LOW>;
> The comment mentions that the reset core does not support the
> GPIO_OPEN_DRAIN flag, but the i2c-mux-pca954x driver requests the GPIO using
> devm_gpiod_get_optional(), which fully supports device tree flags.

The driver first tries reset core, and only falls back to direct gpio control.
The reset-core registers with reset-gpios properties by default,
so we must satisfy that and not the i2c device driver.

static int pca954x_get_reset(struct device *dev, struct pca954x *data)
{
    data->reset_cont = devm_reset_control_get_optional_shared(dev, NULL);
    if (IS_ERR(data->reset_cont))
        return dev_err_probe(dev, PTR_ERR(data->reset_cont),
                     "Failed to get reset\n");
    else if (data->reset_cont)
        return 0;

    /*
     * fallback to legacy reset-gpios
     */
    data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
    if (IS_ERR(data->reset_gpio)) {
        return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
                     "Failed to get reset gpio");
    }

    return 0;
}
>
> By omitting the flag, does the GPIO controller get forced to drive the pin
> in push-pull mode? Could actively driving a hard-wired open-drain line high
> cause short circuits or component damage?
Nothing bad happens, the line has a 10k pull-up to same IO voltage as the
tca6416_u21.
>
> [ ... ]
>> diff --git a/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-rs485-a.dtso b/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-rs485-a.dtso
>> new file mode 100644
>> index 0000000000000..886f4d4ecd67f
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-rs485-a.dtso
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +/*
>> + * Copyright 2025 Josua Mayer <josua@solid-run.com>
>> + *
>> + * Overlay for enabling HummingBoard IIoT on-board RS485 Port A on connector J5004.
>> + *
>> + * Because Renesas uart driver does not support rs485,
>> + * users must manually toggle P41_1 between RX & TX.
> The overlay below adds <&scif1_rts_pins> to the pinctrl-0 property of the
> UART controllers. Since the Renesas RZ/G2L pinctrl driver operates in strict
> mode, it will exclusively claim these RTS pins.
>
> Will this prevent userspace from subsequently requesting these pins as GPIOs
> for the documented manual toggling, causing requests to be rejected with
> -EBUSY?
# gpioinfo 
gpiochip0 - 392 lines:
        line 329:      "P41_1"       kernel   input  active-high [used]
# gpioset 0 329=1
[  342.868990] pinctrl-rzg2l 11030000.pinctrl: pin P41_1 already requested by 1004bc00.serial; cannot claim for 11030000.pinctrl:841
[  342.880756] pinctrl-rzg2l 11030000.pinctrl: error -EINVAL: pin-329 (11030000.pinctrl:841)
gpioset: error setting the GPIO line values: Invalid argument
>> + */
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +&scif1 {
>> +	pinctrl-0 = <&scif1_pins>, <&scif1_rts_pins>;
>> +};
I'll drop the scif1_rts_pins reference vor v3.
> [ ... ]
>> 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..1d52530dcf43c
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/renesas/rzg2l-sr-som.dtsi
> [ ... ]
>> +&sdhi1 {
>> +	/* Murata 1MW max rate is 50MHz */
>> +	max-frequency = <50000000>;
>> +	bus-width = <4>;
>> +	mmc-pwrseq = <&sdhi1_pwrseq>;
>> +	non-removable;
>> +	no-1-8-v;
>> +	no-sd;
>> +	pinctrl-0 = <&sdhi1_pins>;
>> +	pinctrl-names = "default";
>> +	vmmc-supply = <&reg_pmic_buck4>;
>> +	/*
>> +	 * Host controller IO voltage is provided from reg_pmic_ldo2,
>> +	 * WiFi module IO voltage from reg_pmic_buck4.
>> +	 * Neither is configurable at run-time so either can be set here.
>> +	 */
>> +	vqmmc-supply = <&reg_pmic_ldo2>;
>> +	status = "okay";
>> +};
> The sdhi1 node is designated for an on-board WiFi module but omits the
> cap-sdio-irq property and lacks a child node defining an out-of-band
> interrupt. 
>
> Will the MMC core fall back to SDIO polling without this capability
> declared,
Yes, this is expected.
> and could this degrade I/O performance and inflate CPU utilization?
We have not previously tested cap-sdio-irq on any SolidRun board,
however it appears to work okay on G2L SoM, v7.1-rc1.
Confirmed interrupt counters going up rapidly during iperf.

Queued for v3.

>
>> +
>> +&usb2_phy0 {
>> +	vbus-supply = <&usb0_vbus_otg>;
>> +	status = "okay";
>> +};
>> +
>> +&usb2_phy1 {
>> +	status = "okay";
>> +};
> The usb0_vbus_pins and usb1_vbus_pins nodes are defined in the pinctrl
> section earlier in this file to configure the USB VBUS enable pins,

They are described in the SoM dtsi:

    usb0_vbus_pins: usb0-vbus {
        pinmux = <RZG2L_PORT_PINMUX(4, 0, 1)>; /* USB0_VBUSEN */
    };

    usb1_vbus_pins: usb1-vbus {
        pinmux = <RZG2L_PORT_PINMUX(42, 0, 1)>; /* USB1_VBUSEN */
    };

They are used as vbus-enable by HummigBoad Ripple,
which was not included in this patch-set for brevity.

The IIOT carrier board uses these pins for different functions:

    tca6416_u21: gpio@21 {
        interrupts-extended = <&pinctrl RZG2L_GPIO(4, 0) IRQ_TYPE_EDGE_FALLING>;
    }

P42_0 is routed to a board-to-board expansion connector for future use.

> but
> neither &usb2_phy0 nor &usb2_phy1 appears to reference them via a pinctrl-0
> property.
>
> Without being multiplexed, will the USB host controllers be able to drive
> VBUS power to connected devices?
There is no issue here, usb power is controlled by other GPIOs.