Re: [PATCH v2 2/4] arm64: dts: renesas: add support for solidrun rzg2l som and hb-iiot evb
Josua Mayer <[email protected]> Tue, 12 May 2026 12:50:19 +0000
| Newsgroups | dev.linux.lists.sashiko,org.kernel.vger.linux-devicetree |
|---|---|
| Message-ID | <[email protected]> |
Am 12.05.26 um 04:28 schrieb [email protected]: > 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 <[email protected]> > > 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 <[email protected]> >> + * >> + * 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 = <®_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 = <®_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.