Linux-RISC-V Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: Re: [PATCH 3/7] riscv: dts: eswin: eic7700: add pinctrl support
  2026-06-15 16:33   ` Conor Dooley
@ 2026-06-26  6:01     ` Yulin Lu
  2026-06-26  7:05       ` Conor Dooley
  0 siblings, 1 reply; 3+ messages in thread
From: Yulin Lu @ 2026-06-26  6:01 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Pinkesh Vaghela, Lee Jones, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Alexandre Ghiti, devicetree, linux-kernel, linux-riscv, Min Lin,
	Samuel Holland, Darshan Prajapati, Pritesh Patel

Hi, Conor. Thanks for your review.

> On Mon, Jun 15, 2026 at 05:50:12PM +0530, Pinkesh Vaghela wrote:
> > From: Yulin Lu <luyulin@eswincomputing.com>
> > 
> > Add pinctrl node and related pin configuration for EIC7700 SoC
> > 
> > Co-developed-by: Pritesh Patel <pritesh.patel@einfochips.com>
> > Signed-off-by: Pritesh Patel <pritesh.patel@einfochips.com>
> > Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> > Signed-off-by: Pinkesh Vaghela <pinkesh.vaghela@einfochips.com>
> > ---
> >  .../dts/eswin/eic7700-hifive-premier-p550.dts | 109 +++
> >  .../riscv/boot/dts/eswin/eic7700-pinctrl.dtsi | 888 ++++++++++++++++++
> >  arch/riscv/boot/dts/eswin/eic7700.dtsi        |   5 +
> >  3 files changed, 1002 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > 
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > index 1fb92f0e7c55..e7bb96e14958 100644
> > --- a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > +++ b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > @@ -6,6 +6,7 @@
> >  /dts-v1/;
> >  
> >  #include "eic7700.dtsi"
> > +#include "eic7700-pinctrl.dtsi"
> >  

...

> > +&gpio79_pins {
> > +	bias-disable;
> > +	input-disable;
> > +};
> > +
> > +&gpio80_pins {
> > +	bias-pull-up;
> > +	input-disable;
> > +};
> > +
> > +&gpio82_pins {
> > +	bias-pull-up;
> > +	input-disable;
> > +};
> > +
> > +&gpio84_pins {
> > +	bias-disable;
> > +	input-disable;
> > +};
> > +
> > +&gpio85_pins {
> > +	bias-pull-up;
> > +	input-disable;
> > +};
> > +
> > +&gpio94_pins {
> > +	bias-disable;
> > +	input-disable;
> > +};
> > +
> > +&gpio106_pins {
> > +	bias-disable;
> > +	input-disable;
> > +};
> > +
> > +&gpio111_pins {
> > +	bias-disable;
> > +	input-disable;
> > +};
> > +
> > +&pinctrl {
> > +	vrgmii-supply = <&vcc_1v8>;
> > +};
> > +
> >  &uart0 {
> >  	status = "okay";
> >  };
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > new file mode 100644
> > index 000000000000..7293df146aa7
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > @@ -0,0 +1,888 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright (c) 2025 Beijing ESWIN Computing Technology Co., Ltd.
> > + *
> > + * ESWIN's EIC7700 SoC pin-mux and pin-config options are listed as
> > + * device tree nodes in this file.
> > + *
> > + * Authors: Yulin Lu <luyulin@eswincomputing.com>
> > + */
> > +
> 
> I don't really understand the groups here. I think you should make more
> effort to put more pins in each group.
> 
> > +		gpio1_pins: gpio1-pins {
> > +			pins = "jtag0_tck";
> > +			function = "gpio";
> > +		};
> > +
> > +		gpio2_pins: gpio2-pins {
> > +			pins = "jtag0_tms";
> > +			function = "gpio";
> > +		};
> > +
> > +		gpio3_pins: gpio3-pins {
> > +			pins = "jtag0_tdi";
> > +			function = "gpio";
> > +		};
> > +
> > +		gpio4_pins: gpio4-pins {
> > +			pins = "jtag0_tdo";
> > +			function = "gpio";
> > +		};
> 
> Like these 4 for example, why not group these?

The 'group' is used to correspond to the '-grp' tag in the YAML file and
has no practical significance.
Different board designs have different requirements for pin multiplexing.
Therefore, eic7700-pinctrl.dtsi only provides pins for the board-level DTS.
Pins are combined and used in the board-level DTS via pinctrl-0 property.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: Re: [PATCH 3/7] riscv: dts: eswin: eic7700: add pinctrl support
       [not found] <20260615123315.ABB4A1F00A3A@smtp.kernel.org>
@ 2026-06-26  6:42 ` Yulin Lu
  0 siblings, 0 replies; 3+ messages in thread
From: Yulin Lu @ 2026-06-26  6:42 UTC (permalink / raw)
  To: Pinkesh Vaghela, Lee Jones, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Alexandre Ghiti, devicetree, linux-kernel, linux-riscv, Min Lin
  Cc: Yulin Lu, Samuel Holland, Darshan Prajapati, Pritesh Patel

Hi, sashiko.

> [Severity: Low]
> Should this node be named using the generic regulator-* pattern (such as
> regulator-1v8) to avoid dtbs_check warnings for deprecated names?
> There is also an extra leading space on the properties within this node
> that deviates from standard spacing.

I will rename this node to "regulator-1v8".

> > +  regulator-name = "vcc1v8";
> > +  regulator-always-on;
> > +  regulator-boot-on;
> > +  regulator-min-microvolt = <1800000>;
> > +  regulator-max-microvolt = <1800000>;
> > +  };
> >  };
> [ ... ]
> > @@ -25,6 +35,105 @@ &xtal {
>   clock-output-names = "xtal24m";
> >  };
> >  
> > +&gpio0_pins {
> > + bias-disable;
> > + input-enable;
> > +};
> [Severity: High]
> Will these pin configuration overrides be silently ignored by the pinctrl
> core during boot?
> Since these pin definitions are not referenced by a pinctrl-0 property in
> an active device node, and are not configured as pinctrl hogs in the provider,
> the intended hardware states for these pins will not be programmed.

I don't think this is an issue. These pin configurations won't be ignored by the kernel during boot.
Some pins have input-enable, pull-down, or pull-up settings that differ from the chip defaults on
certain boards, these configurations are added these configurations in the board-level DTS.

> [ ... ]
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > new file mode 100644
> > index 0000000000000..7293df146aa7b
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > @@ -0,0 +1,888 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> [ ... ]
> > + hdmi_pins: hdmi-pins {
> > + pins = "hdmi_scl", "hdmi_sda", "hdmi_cec";
> > + function = "hdmi";
> > + };
> > +
> > + rgmii0_pins: rgmii0-pins {
> > + pins = "rgmii0_clk_125", "rgmii0_txen", "rgmii0_txclk", "rgmii0_txd0",
> > +        "rgmii0_txd1", "rgmii0_txd2", "rgmii0_txd3", "rgmii0_rxclk",
> > +        "rgmii0_rxdv", "rgmii0_rxd0", "rgmii0_rxd1", "rgmii0_rxd2",
> > +        "rgmii0_rxd3", "rgmii0_mdc", "rgmii0_mdio", "rgmii0_intb";
> > + function = "hdmi";
> > + };
> [Severity: High]
> Is the hdmi function correct for this network pin group?
> It appears that rgmii0, rgmii1, and the i2s pin groups that follow all use
> the hdmi function. This will prevent the pin controller from routing these
> pins properly, likely leading to driver probe failures when peripherals
> attempt to request them.

I will fix the function of rgmii0_pins and rgmii1_pins to rgmii, and the function of i2s0_pins,
i2s1_pins, and i2s2_pins to i2s.

> [ ... ]
> > diff --git a/arch/riscv/boot/dts/eswin/eic7700.dtsi b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > index a7ebb11159580..8798c50f7584d 100644
> > --- a/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > +++ b/arch/riscv/boot/dts/eswin/eic7700.dtsi
> > @@ -349,6 +349,11 @@ gpioD: gpio-port@3 {
> >   };
> >   };
> >  
> > + pinctrl: pinctrl@51600080 {
> > + compatible = "eswin,eic7700-pinctrl";
> > + reg = <0x0 0x51600080 0x0 0x1fff80>;
> > + };
> > +
> [Severity: High]
> Should the existing gpio controller nodes (gpioA, gpioB, gpioC, and gpioD)
> be updated with a gpio-ranges property mapping to this new pinctrl provider?
> Without gpio-ranges, the GPIO subsystem cannot translate GPIO offsets into
> pinctrl pins. The pinctrl core's .gpio_request_enable callback will not be
> invoked when a software component requests a GPIO dynamically, preventing
> the multiplexer from automatically switching the pin to GPIO mode.
> >   clk: clock-controller@51828000 {
> >   compatible = "eswin,eic7700-clock";

I think add gpio-ranges is unnecessary, the driver does not implement
the .gpio_request_enable callback.

Regards,
Yulin Lu


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: Re: [PATCH 3/7] riscv: dts: eswin: eic7700: add pinctrl support
  2026-06-26  6:01     ` Yulin Lu
@ 2026-06-26  7:05       ` Conor Dooley
  0 siblings, 0 replies; 3+ messages in thread
From: Conor Dooley @ 2026-06-26  7:05 UTC (permalink / raw)
  To: Yulin Lu
  Cc: Conor Dooley, Pinkesh Vaghela, Lee Jones, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, Alexandre Ghiti, devicetree, linux-kernel, linux-riscv,
	Min Lin, Samuel Holland, Darshan Prajapati, Pritesh Patel


[-- Attachment #1.1: Type: text/plain, Size: 4480 bytes --]

On Fri, Jun 26, 2026 at 02:01:42PM +0800, Yulin Lu wrote:
> Hi, Conor. Thanks for your review.
> 
> > On Mon, Jun 15, 2026 at 05:50:12PM +0530, Pinkesh Vaghela wrote:
> > > From: Yulin Lu <luyulin@eswincomputing.com>
> > > 
> > > Add pinctrl node and related pin configuration for EIC7700 SoC
> > > 
> > > Co-developed-by: Pritesh Patel <pritesh.patel@einfochips.com>
> > > Signed-off-by: Pritesh Patel <pritesh.patel@einfochips.com>
> > > Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> > > Signed-off-by: Pinkesh Vaghela <pinkesh.vaghela@einfochips.com>
> > > ---
> > >  .../dts/eswin/eic7700-hifive-premier-p550.dts | 109 +++
> > >  .../riscv/boot/dts/eswin/eic7700-pinctrl.dtsi | 888 ++++++++++++++++++
> > >  arch/riscv/boot/dts/eswin/eic7700.dtsi        |   5 +
> > >  3 files changed, 1002 insertions(+)
> > >  create mode 100644 arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > > 
> > > diff --git a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > > index 1fb92f0e7c55..e7bb96e14958 100644
> > > --- a/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > > +++ b/arch/riscv/boot/dts/eswin/eic7700-hifive-premier-p550.dts
> > > @@ -6,6 +6,7 @@
> > >  /dts-v1/;
> > >  
> > >  #include "eic7700.dtsi"
> > > +#include "eic7700-pinctrl.dtsi"
> > >  
> 
> ...
> 
> > > +&gpio79_pins {
> > > +	bias-disable;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio80_pins {
> > > +	bias-pull-up;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio82_pins {
> > > +	bias-pull-up;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio84_pins {
> > > +	bias-disable;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio85_pins {
> > > +	bias-pull-up;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio94_pins {
> > > +	bias-disable;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio106_pins {
> > > +	bias-disable;
> > > +	input-disable;
> > > +};
> > > +
> > > +&gpio111_pins {
> > > +	bias-disable;
> > > +	input-disable;
> > > +};
> > > +
> > > +&pinctrl {
> > > +	vrgmii-supply = <&vcc_1v8>;
> > > +};
> > > +
> > >  &uart0 {
> > >  	status = "okay";
> > >  };
> > > diff --git a/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > > new file mode 100644
> > > index 000000000000..7293df146aa7
> > > --- /dev/null
> > > +++ b/arch/riscv/boot/dts/eswin/eic7700-pinctrl.dtsi
> > > @@ -0,0 +1,888 @@
> > > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > > +/*
> > > + * Copyright (c) 2025 Beijing ESWIN Computing Technology Co., Ltd.
> > > + *
> > > + * ESWIN's EIC7700 SoC pin-mux and pin-config options are listed as
> > > + * device tree nodes in this file.
> > > + *
> > > + * Authors: Yulin Lu <luyulin@eswincomputing.com>
> > > + */
> > > +
> > 
> > I don't really understand the groups here. I think you should make more
> > effort to put more pins in each group.
> > 
> > > +		gpio1_pins: gpio1-pins {
> > > +			pins = "jtag0_tck";
> > > +			function = "gpio";
> > > +		};
> > > +
> > > +		gpio2_pins: gpio2-pins {
> > > +			pins = "jtag0_tms";
> > > +			function = "gpio";
> > > +		};
> > > +
> > > +		gpio3_pins: gpio3-pins {
> > > +			pins = "jtag0_tdi";
> > > +			function = "gpio";
> > > +		};
> > > +
> > > +		gpio4_pins: gpio4-pins {
> > > +			pins = "jtag0_tdo";
> > > +			function = "gpio";
> > > +		};
> > 
> > Like these 4 for example, why not group these?
> 
> The 'group' is used to correspond to the '-grp' tag in the YAML file and
> has no practical significance.
> Different board designs have different requirements for pin multiplexing.
> Therefore, eic7700-pinctrl.dtsi only provides pins for the board-level DTS.
> Pins are combined and used in the board-level DTS via pinctrl-0 property.

These 4 pins in the driver are represented as:
	EIC7700_PIN(14,  "jtag0_tck",		[0] = F_JTAG, [1] = F_SPI, [2] = F_GPIO),
	EIC7700_PIN(15,  "jtag0_tms",		[0] = F_JTAG, [1] = F_SPI, [2] = F_GPIO),
	EIC7700_PIN(16,  "jtag0_tdi",		[0] = F_JTAG, [1] = F_SPI, [2] = F_GPIO),
	EIC7700_PIN(17,  "jtag0_tdo",		[0] = F_JTAG, [1] = F_SPI, [2] = F_GPIO),

	EIC7700_PIN(18,  "gpio5",		[0] = F_GPIO, [1] = F_SPI),

There is no reason to ever partially use these as GPIO. Either they will
be all jtag, all spi or all gpio. pin 18 on the other than makes sense to have
in a dedicated group.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-06-26  7:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260615123315.ABB4A1F00A3A@smtp.kernel.org>
2026-06-26  6:42 ` Re: [PATCH 3/7] riscv: dts: eswin: eic7700: add pinctrl support Yulin Lu
2026-06-15 12:20 [PATCH 0/7] riscv: eswin: eic7700: Add support for clocks, resets, pinctrl, HSP power domain, I2C and watchdog Pinkesh Vaghela
2026-06-15 12:20 ` [PATCH 3/7] riscv: dts: eswin: eic7700: add pinctrl support Pinkesh Vaghela
2026-06-15 16:33   ` Conor Dooley
2026-06-26  6:01     ` Yulin Lu
2026-06-26  7:05       ` Conor Dooley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox