Skip to contentSkip to conversation

riscv: dts: eswin: eic7700: add pinctrl support

PATCH Yulin Lu 1 participant 1 message Jul 28 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Yulin Lu

author
#1 lore raw

Hi, conor, robh

Quoted 45 lines

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Board-specific GPIO pin configurations are defined but never referenced via pinctrl-0, rendering them inert.
- [Medium] Peripheral nodes do not reference their newly added pinctrl configurations.

Pre-existing issues:
- [High] The SoC device tree lacks gpio-ranges properties linking the DW APB GPIO controllers to the newly introduced pinctrl controller, bypassing GPIO muxing logic. --

commit 92abe836b11b6b811731ac872e0c01dccbccefd0
Author: Yulin Lu <[email protected]>

riscv: dts: eswin: eic7700: add pinctrl support

This commit adds pinctrl nodes and related pin configurations for the EIC7700 SoC. It also provides the specific pin states for the HiFive Premier P550 board.

Quoted 4 lines
arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts +0 -0
index 1266762dfcd8c..743dedbdb081a 100644

[ ... ]

arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts &clock_24m { · line 46

Quoted 13 lines
arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts +9 -0
@@ -25,6 +35,105 @@ &clock_24m {
 	clock-output-names = "clock_24m";
 };
 
+&gpio0_pins {
+	bias-disable;
+	input-enable;
+};
+
+&gpio5_pins {
+	bias-disable;
+	input-enable;
+};

[Severity: Medium]
Will these board-specific GPIO pin configurations ever be applied?

The board DTS overrides properties like bias-pull-up for various GPIO nodes (&gpio0_pins, &gpio14_pins, etc.), but these pin configurations are never referenced by any pinctrl-0 property in a consumer device or as a pinctrl hog.

Quoted 6 lines

Without being referenced via pinctrl-names = "default"; and pinctrl-0 = <...>; within the &pinctrl node, the pinctrl subsystem will parse them as templates but never commit the hardware configurations.

This could leave the hardware lacking required electrical configurations.

In the next version, the unreferenced electrical property node configurations will be removed.

Quoted 12 lines

[ ... ]

arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts

Quoted 7 lines

+&pinctrl {

arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts +3 -0
+	vrgmii-supply = <&vcc_1v8>;
+};
+
 &uart0 {
 	status = "okay";
 };

[Severity: Medium]
Should this peripheral node reference its newly added pinctrl configurations?

Quoted 9 lines

The eic7700-pinctrl.dtsi file defines extensive pinmux configurations, such as uart0_pins and i2c0_pins, and the board DTS enables these peripherals here.

However, the board DTS fails to assign these pinmux configurations to the peripherals using pinctrl-names = "default"; and pinctrl-0 = <&uart0_pins>;.

As a result, peripheral pin multiplexing is not applied by the kernel, relying entirely on the bootloader's initial state.

In the next version, references to i2c0_pins, i2c1_pins, uart0_pins, and uart2_pins will be added via pinctrl-0 properties.

Quoted 30 lines
Quoted 4 lines
arch/riscv/boot/dts/eswin/eic7700.dtsi +0 -0
index af9e40d8739ad..a0fb82f4adca9 100644

[ ... ]

arch/riscv/boot/dts/eswin/eic7700.dtsi gpioD: gpio-port@3 { · line 356

Quoted 9 lines
arch/riscv/boot/dts/eswin/eic7700.dtsi +5 -0
@@ -349,6 +349,11 @@ gpioD: gpio-port@3 {
 			};
 		};
 
+		pinctrl: pinctrl@51600080 {
+			compatible = "eswin,eic7700-pinctrl";
+			reg = <0x0 0x51600080 0x0 0x1fff80>;
+		};
+

[Severity: High]
This isn't a bug introduced by this patch, but does the SoC device tree lack gpio-ranges properties linking the DW APB GPIO controllers to the newly introduced pinctrl controller?

The eic7700.dtsi defines GPIO controllers using the snps,dw-apb-gpio driver:

			gpioA: gpio-port@0 {
				compatible = "snps,dw-apb-gpio-port";

This driver unconditionally sets .request = gpiochip_generic_request.

When a GPIO is requested, gpiochip_generic_request checks if the gc->gpiodev->pin_ranges list is empty.

Quoted 9 lines

Because eic7700.dtsi fails to define gpio-ranges mapping the gpio-port subnodes to the pinctrl controller, the pin_ranges list is empty. As a result, gpiochip_generic_request() returns 0 early and never calls pinctrl_gpio_request().

This bypasses the pinctrl driver's hardware muxing logic entirely, meaning GPIO hardware muxing will not occur when a GPIO is requested, leaving the pins in their default or peripheral state.

&gmac0 {
	...
	pinctrl-names = "default";
	pinctrl-0 = <&gpio106_pins>;
	...
	status = "okay";
};
&gmac0_mdio {
	gmac0_phy0: ethernet-phy@0 {
		...
		reset-gpios = <&gpioD 10 GPIO_ACTIVE_LOW>;
		...
	};
};
gpioD: gpio-port@3 {
	...
	gpio-ranges = <&pinctrl 0 143 16>;
};

The DTS configuration is as shown above.
When "pinctrl-0", "reset-gpios", and "gpio-ranges" are present at the
same time, the GPIO module and the Ethernet module both attempt to
acquire the same GPIO, causing the kernel to report an error:
"pin gpio106 already requested by 50400000.ethernet; cannot claim for 51600000.gpio:618."

For the EIC7700 SoC, a pin can only be used as GPIO or as another function at the same time. Therefore, the ".strict" in pinctrl-eic7700.c is set to "true".

"pinctrl-names" and "pinctrl-0" are necessary, so I think that adding "gpio-ranges" to the GPIO child nodes on the EIC7700 SoC is not supported. If the "gpio-ranges" property is not added to the GPIO child nodes, is that acceptable?

Quoted 2 lines
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

Thanks,
Yulin Lu