[PATCH v2 0/3] pinctrl: starfive: jh7110: support force inputs

[PATCH v2 0/3] pinctrl: starfive: jh7110: support force inputs

[Icenowy Zheng]

The input signals inside the JH7110 SoC (to be routed by the pin
controller) could be routed to GPIOs and internal fixed low/high levels.
As the total GPIO count of JH7110 is not very high, it's sometime
feasible to omit some hardwiring outside the SoC and do them in the pin
controller. One such example is the USB overcurrent_n signal, which
defaults to low at SoC reset, needs to be high for the USB controller to
correctly work (the _n means low indicates overcurrent situation) and
gets omitted on the Pine64 Star64 board.

Add the support for hardwiring GPI signals inside the JH7110 pin
controllers, via two virtual "pins" which mean fixed low/high.

Changes in v2:
- Use virtual pins instead of special properties.
- No longer RFC.

Icenowy Zheng (3):
  dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins
  pinctrl: starfive: jh7110: add support for PAD_INTERNAL_* for GPI
  riscv: dts: starfive: jh7110-pine64-star64: force no USB overcurrent

 .../dts/starfive/jh7110-pine64-star64.dts     |  7 ++++
 .../starfive/pinctrl-starfive-jh7110.c        | 41 +++++++++++++++----
 .../pinctrl/starfive,jh7110-pinctrl.h         |  4 ++
 3 files changed, 45 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Icenowy Zheng]

The JH7110 SoC could support internal GPI signals to be routed to not
external GPIO but internal low/high levels.

Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two virtual
"pads" to represent internal GPI sources with fixed low/high levels.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
index 3865f01396395..3cca874b2bef7 100644
--- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
+++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
@@ -126,6 +126,10 @@
 #define	PAD_GMAC0_TXEN		18
 #define	PAD_GMAC0_TXC		19
 
+/* virtual pins for forcing GPI */
+#define PAD_INTERNAL_LOW	254
+#define PAD_INTERNAL_HIGH	255
+
 #define GPOUT_LOW		0
 #define GPOUT_HIGH		1
 
-- 
2.49.0

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[E Shattow]

On 4/23/25 23:20, Icenowy Zheng wrote:
> The JH7110 SoC could support internal GPI signals to be routed to not
> external GPIO but internal low/high levels.
> 
> Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two virtual
> "pads" to represent internal GPI sources with fixed low/high levels.
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> index 3865f01396395..3cca874b2bef7 100644
> --- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> +++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> @@ -126,6 +126,10 @@
>  #define	PAD_GMAC0_TXEN		18
>  #define	PAD_GMAC0_TXC		19
>  
> +/* virtual pins for forcing GPI */
> +#define PAD_INTERNAL_LOW	254
> +#define PAD_INTERNAL_HIGH	255
> +
>  #define GPOUT_LOW		0
>  #define GPOUT_HIGH		1
>  

Asking about the choice of 255 and 254 values for virtual high/low pins,
here. There's not much result when grep Linux source for 'virtual pin'
to compare with. Are these the best values for this approach?

What happens when devicetree has in it to route PAD_INTERNAL_LOW to
PAD_INTERNAL_HIGH and other unlikely combinations?  Or a devicetree blob
with this computed value is paired to Linux kernel that does not have
the code to handle these virtual pins, for compatibility concern?

Do we know yet if JH8100 will share some of this design?

-E

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Icenowy Zheng]

在 2025-04-24星期四的 01:15 -0700，E Shattow写道：
> On 4/23/25 23:20, Icenowy Zheng wrote:
> > The JH7110 SoC could support internal GPI signals to be routed to
> > not
> > external GPIO but internal low/high levels.
> > 
> > Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
> > virtual
> > "pads" to represent internal GPI sources with fixed low/high
> > levels.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >  include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > index 3865f01396395..3cca874b2bef7 100644
> > --- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > +++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > @@ -126,6 +126,10 @@
> >  #define        PAD_GMAC0_TXEN          18
> >  #define        PAD_GMAC0_TXC           19
> >  
> > +/* virtual pins for forcing GPI */
> > +#define PAD_INTERNAL_LOW       254
> > +#define PAD_INTERNAL_HIGH      255
> > +
> >  #define GPOUT_LOW              0
> >  #define GPOUT_HIGH             1
> >  
> 
> Asking about the choice of 255 and 254 values for virtual high/low
> pins,
> here. There's not much result when grep Linux source for 'virtual
> pin'
> to compare with. Are these the best values for this approach?

These two values are picked because the following reasons:

- The pin field has 8 bits (see the comments of jh7110_pinmux_din() in
pinctrl-starfive-jh7110.c)
- We are already using values 0 and 1 for GPIO0/GPIO1

If we're designing from scratch, it's possible to have another practice
by using 0 and 1 for internal low/high and 2 for gpio0 so on.

> 
> What happens when devicetree has in it to route PAD_INTERNAL_LOW to
> PAD_INTERNAL_HIGH and other unlikely combinations?  Or a devicetree
> blob
> with this computed value is paired to Linux kernel that does not have
> the code to handle these virtual pins, for compatibility concern?

I think it's not supported for newer DTs to be compatible with old
kernels, but I analyzed the code, a read-out-of-bound could happen in
jh7110_set_function() in pinctrl-starfive-jh7110-sys.c . Well this is
unfortunate, but we can do few things to fix old kernels -- we can fix
the problem in newer kernels.

And even picking other values cannot make things better...

> 
> Do we know yet if JH8100 will share some of this design?

We don't know yet whether JH8100 can exist.

> 
> -E

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Linus Walleij]

On Thu, Apr 24, 2025 at 8:20 AM Icenowy Zheng <uwu@icenowy.me> wrote:

> The JH7110 SoC could support internal GPI signals to be routed to not
> external GPIO but internal low/high levels.
>
> Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two virtual
> "pads" to represent internal GPI sources with fixed low/high levels.
>
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>

As per my other reply in the previous post, I think this should be
handled internal in the kernel instead using a tighter integration between
the GPIO and pin control parts of the driver and utilizing the
gpio-specific struct pinmux_ops callbacks.

This solution looks like software configuration disguised as hardware
configuration.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Icenowy Zheng]

在 2025-04-24星期四的 10:51 +0200，Linus Walleij写道：
> On Thu, Apr 24, 2025 at 8:20 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> 
> > The JH7110 SoC could support internal GPI signals to be routed to
> > not
> > external GPIO but internal low/high levels.
> > 
> > Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
> > virtual
> > "pads" to represent internal GPI sources with fixed low/high
> > levels.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> 
> As per my other reply in the previous post, I think this should be
> handled internal in the kernel instead using a tighter integration
> between
> the GPIO and pin control parts of the driver and utilizing the
> gpio-specific struct pinmux_ops callbacks.

Well I cannot understand this -- these signals are not GPIOs, totally
not related to the GPIO subsystem (because they're only pinmux, not
related to GPIOs). This is described in my previous mail.

The pin mux of JH7110 strictly route its inputs to its outputs. For
signals from other SoC blocks (to external pins), the registers define
how OUT/OEn of IO buffers *are driven by* the signals; however for
signals to other SoC blocks (from external pins), the registers define
how IN of IO buffers *drive* the signals. (This follows the generic
signal-driving rule that one signal can drive multiple signals but
cannot be multi-driven).

In addition the situation I am trying to handle here is an addition to
the latter part of the previous paragraph -- in addition to 64 inputs
corresponding to 64 GPIOs, two extra inputs, one always 0 and another
always 1 are available to the pin controller for driving other SoC
blocks' input (as output of pin controller).

In fact this is why there is ` + 2` when calculating ival in
jh7110_set_gpiomux() -- the first two possible values are for always 0
and always 1, 3 represents the IN of GPIO0, etc.

> 
> This solution looks like software configuration disguised as hardware
> configuration.

Well this solution handles these internal wires in the same way as
signals from external GPIOs, excepting specifying special GPIO numbers.
If you are against the principle, maybe the current already-included
GPIOMUX system of the StarFive pinctrl is to be blamed instead of my
small extension to it.

I must admit that the current GPIOMUX system isn't a faithful
representation of the hardware because it's a pad-centric setup instead
of a register-field-centric one, which isn't very natural for input
signals. However configurating the mux in such a way is against people
reading, and we're not able to break the system because it's already
there.

Well in the situation that one GPIO used as input drives multiple
internal signals the pinmux looks a little confusing too, e.g. the I2S
clock situation I mentioned in my reply in the previous revision of the
patchset.

> 
> Yours,
> Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Linus Walleij]

On Thu, Apr 24, 2025 at 11:38 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> 在 2025-04-24星期四的 10:51 +0200，Linus Walleij写道：
> > On Thu, Apr 24, 2025 at 8:20 AM Icenowy Zheng <uwu@icenowy.me> wrote:
> >
> > > The JH7110 SoC could support internal GPI signals to be routed to
> > > not
> > > external GPIO but internal low/high levels.
> > >
> > > Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
> > > virtual
> > > "pads" to represent internal GPI sources with fixed low/high
> > > levels.
> > >
> > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> >
> > As per my other reply in the previous post, I think this should be
> > handled internal in the kernel instead using a tighter integration
> > between
> > the GPIO and pin control parts of the driver and utilizing the
> > gpio-specific struct pinmux_ops callbacks.
>
> Well I cannot understand this -- these signals are not GPIOs, totally
> not related to the GPIO subsystem (because they're only pinmux, not
> related to GPIOs). This is described in my previous mail.

OK sorry if I'm a bit dumb at times :(

I guess I was falling into the common confusion of "something named
general purpose" such as your GPI and GPO registers, is also
related to GPIO which it is not, at least not always.

> The pin mux of JH7110 strictly route its inputs to its outputs. For
> signals from other SoC blocks (to external pins), the registers define
> how OUT/OEn of IO buffers *are driven by* the signals; however for
> signals to other SoC blocks (from external pins), the registers define
> how IN of IO buffers *drive* the signals. (This follows the generic
> signal-driving rule that one signal can drive multiple signals but
> cannot be multi-driven).
>
> In addition the situation I am trying to handle here is an addition to
> the latter part of the previous paragraph -- in addition to 64 inputs
> corresponding to 64 GPIOs, two extra inputs, one always 0 and another
> always 1 are available to the pin controller for driving other SoC
> blocks' input (as output of pin controller).

OK ... maybe I get it now.

> > This solution looks like software configuration disguised as hardware
> > configuration.
>
> Well this solution handles these internal wires in the same way as
> signals from external GPIOs, excepting specifying special GPIO numbers.
> If you are against the principle, maybe the current already-included
> GPIOMUX system of the StarFive pinctrl is to be blamed instead of my
> small extension to it.
>
> I must admit that the current GPIOMUX system isn't a faithful
> representation of the hardware because it's a pad-centric setup instead
> of a register-field-centric one, which isn't very natural for input
> signals. However configurating the mux in such a way is against people
> reading, and we're not able to break the system because it's already
> there.
>
> Well in the situation that one GPIO used as input drives multiple
> internal signals the pinmux looks a little confusing too, e.g. the I2S
> clock situation I mentioned in my reply in the previous revision of the
> patchset.

I guess what rubs me the wrong way is why the external users
(devices, device drivers or even pin hogs) cannot trigger the chain of
events leading to this configuration, instead of different "magic"
configurations that are just set up in the pin controller itself.

But if you are positively convinced that there is no other way,
I guess I have to live with it.

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Icenowy Zheng]

在 2025-04-24星期四的 12:30 +0200，Linus Walleij写道：
> On Thu, Apr 24, 2025 at 11:38 AM Icenowy Zheng <uwu@icenowy.me>
> wrote:
> > 在 2025-04-24星期四的 10:51 +0200，Linus Walleij写道：
> > > On Thu, Apr 24, 2025 at 8:20 AM Icenowy Zheng <uwu@icenowy.me>
> > > wrote:
> > > 
> > > > The JH7110 SoC could support internal GPI signals to be routed
> > > > to
> > > > not
> > > > external GPIO but internal low/high levels.
> > > > 
> > > > Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
> > > > virtual
> > > > "pads" to represent internal GPI sources with fixed low/high
> > > > levels.
> > > > 
> > > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > 
> > > As per my other reply in the previous post, I think this should
> > > be
> > > handled internal in the kernel instead using a tighter
> > > integration
> > > between
> > > the GPIO and pin control parts of the driver and utilizing the
> > > gpio-specific struct pinmux_ops callbacks.
> > 
> > Well I cannot understand this -- these signals are not GPIOs,
> > totally
> > not related to the GPIO subsystem (because they're only pinmux, not
> > related to GPIOs). This is described in my previous mail.
> 
> OK sorry if I'm a bit dumb at times :(
> 
> I guess I was falling into the common confusion of "something named
> general purpose" such as your GPI and GPO registers, is also
> related to GPIO which it is not, at least not always.

Ah, sorry, these GPI/GPO names are directly taken from the StarFive TRM
from the register field names. They CAN be routed to the external
GPIOs, but is not required so.

> 
> > The pin mux of JH7110 strictly route its inputs to its outputs. For
> > signals from other SoC blocks (to external pins), the registers
> > define
> > how OUT/OEn of IO buffers *are driven by* the signals; however for
> > signals to other SoC blocks (from external pins), the registers
> > define
> > how IN of IO buffers *drive* the signals. (This follows the generic
> > signal-driving rule that one signal can drive multiple signals but
> > cannot be multi-driven).
> > 
> > In addition the situation I am trying to handle here is an addition
> > to
> > the latter part of the previous paragraph -- in addition to 64
> > inputs
> > corresponding to 64 GPIOs, two extra inputs, one always 0 and
> > another
> > always 1 are available to the pin controller for driving other SoC
> > blocks' input (as output of pin controller).
> 
> OK ... maybe I get it now.
> 
> > > This solution looks like software configuration disguised as
> > > hardware
> > > configuration.
> > 
> > Well this solution handles these internal wires in the same way as
> > signals from external GPIOs, excepting specifying special GPIO
> > numbers.
> > If you are against the principle, maybe the current already-
> > included
> > GPIOMUX system of the StarFive pinctrl is to be blamed instead of
> > my
> > small extension to it.
> > 
> > I must admit that the current GPIOMUX system isn't a faithful
> > representation of the hardware because it's a pad-centric setup
> > instead
> > of a register-field-centric one, which isn't very natural for input
> > signals. However configurating the mux in such a way is against
> > people
> > reading, and we're not able to break the system because it's
> > already
> > there.
> > 
> > Well in the situation that one GPIO used as input drives multiple
> > internal signals the pinmux looks a little confusing too, e.g. the
> > I2S
> > clock situation I mentioned in my reply in the previous revision of
> > the
> > patchset.
> 
> I guess what rubs me the wrong way is why the external users
> (devices, device drivers or even pin hogs) cannot trigger the chain
> of
> events leading to this configuration, instead of different "magic"
> configurations that are just set up in the pin controller itself.

Well I am just extending what's already in use...

Currently it's already supported to route GPIOs to GPI signals, I added
the support to route fixed level sources to them, in a similar way.

If any external users ever have the need of banging the internal
signals instead of tying it fixedly, maybe switching between different
pinctrl configuration sets is enough? (Because this kind of operation
could never be as high speed enough as real hardware pins)

> 
> But if you are positively convinced that there is no other way,
> I guess I have to live with it.
> 
> Yours,
> Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Linus Walleij]

On Thu, Apr 24, 2025 at 2:26 PM Icenowy Zheng <uwu@icenowy.me> wrote:
[Me]
> > I guess what rubs me the wrong way is why the external users
> > (devices, device drivers or even pin hogs) cannot trigger the chain
> > of
> > events leading to this configuration, instead of different "magic"
> > configurations that are just set up in the pin controller itself.
>
> Well I am just extending what's already in use...
>
> Currently it's already supported to route GPIOs to GPI signals, I added
> the support to route fixed level sources to them, in a similar way.
>
> If any external users ever have the need of banging the internal
> signals instead of tying it fixedly, maybe switching between different
> pinctrl configuration sets is enough? (Because this kind of operation
> could never be as high speed enough as real hardware pins)

What I am thinking is that one of the following must be true:

1. The internal pads are always set up the same way for this SoC
  in which case they should be just hardcoded instead, or at
  least just implied from the compatible string of the pin controller.

2. The internal pads are routed differently depending on different
  use cases, in which case they need to be set up or implied
  from configuration in other DT nodes describing this use.

I guess this binding if for (2)?

Yours,
Linus Walleij

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Krzysztof Kozlowski]

On Thu, Apr 24, 2025 at 02:20:15PM GMT, Icenowy Zheng wrote:
> The JH7110 SoC could support internal GPI signals to be routed to not
> external GPIO but internal low/high levels.
> 
> Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two virtual
> "pads" to represent internal GPI sources with fixed low/high levels.
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> index 3865f01396395..3cca874b2bef7 100644
> --- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> +++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> @@ -126,6 +126,10 @@
>  #define	PAD_GMAC0_TXEN		18
>  #define	PAD_GMAC0_TXC		19
>  
> +/* virtual pins for forcing GPI */
> +#define PAD_INTERNAL_LOW	254
> +#define PAD_INTERNAL_HIGH	255

Why this cannot be 20 and 21? These are not values for registers, but
abstract numbers.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Icenowy Zheng]

在 2025-04-28星期一的 09:20 +0200，Krzysztof Kozlowski写道：
> On Thu, Apr 24, 2025 at 02:20:15PM GMT, Icenowy Zheng wrote:
> > The JH7110 SoC could support internal GPI signals to be routed to
> > not
> > external GPIO but internal low/high levels.
> > 
> > Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
> > virtual
> > "pads" to represent internal GPI sources with fixed low/high
> > levels.
> > 
> > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > ---
> >  include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > index 3865f01396395..3cca874b2bef7 100644
> > --- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > +++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > @@ -126,6 +126,10 @@
> >  #define        PAD_GMAC0_TXEN          18
> >  #define        PAD_GMAC0_TXC           19
> >  
> > +/* virtual pins for forcing GPI */
> > +#define PAD_INTERNAL_LOW       254
> > +#define PAD_INTERNAL_HIGH      255
> 
> Why this cannot be 20 and 21? These are not values for registers, but
> abstract numbers.

The number must not collide with SYS GPIO pads too.

Using 95 and 96 is okay, but dirty.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Krzysztof Kozlowski]

On 28/04/2025 10:40, Icenowy Zheng wrote:
> 在 2025-04-28星期一的 09:20 +0200，Krzysztof Kozlowski写道：
>> On Thu, Apr 24, 2025 at 02:20:15PM GMT, Icenowy Zheng wrote:
>>> The JH7110 SoC could support internal GPI signals to be routed to
>>> not
>>> external GPIO but internal low/high levels.
>>>
>>> Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
>>> virtual
>>> "pads" to represent internal GPI sources with fixed low/high
>>> levels.
>>>
>>> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
>>> ---
>>>  include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>>> b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>>> index 3865f01396395..3cca874b2bef7 100644
>>> --- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>>> +++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>>> @@ -126,6 +126,10 @@
>>>  #define        PAD_GMAC0_TXEN          18
>>>  #define        PAD_GMAC0_TXC           19
>>>  
>>> +/* virtual pins for forcing GPI */
>>> +#define PAD_INTERNAL_LOW       254
>>> +#define PAD_INTERNAL_HIGH      255
>>
>> Why this cannot be 20 and 21? These are not values for registers, but
>> abstract numbers.
> 
> The number must not collide with SYS GPIO pads too.

There are no SYS GPIO pads here. Do you understand that this is not
value for registers?

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Icenowy Zheng]

在 2025-04-29星期二的 09:31 +0200，Krzysztof Kozlowski写道：
> On 28/04/2025 10:40, Icenowy Zheng wrote:
> > 在 2025-04-28星期一的 09:20 +0200，Krzysztof Kozlowski写道：
> > > On Thu, Apr 24, 2025 at 02:20:15PM GMT, Icenowy Zheng wrote:
> > > > The JH7110 SoC could support internal GPI signals to be routed
> > > > to
> > > > not
> > > > external GPIO but internal low/high levels.
> > > > 
> > > > Add two macros, PAD_INTERNAL_LOW and PAD_INTERNAL_HIGH, as two
> > > > virtual
> > > > "pads" to represent internal GPI sources with fixed low/high
> > > > levels.
> > > > 
> > > > Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> > > > ---
> > > >  include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/include/dt-bindings/pinctrl/starfive,jh7110-
> > > > pinctrl.h
> > > > b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > > > index 3865f01396395..3cca874b2bef7 100644
> > > > --- a/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > > > +++ b/include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
> > > > @@ -126,6 +126,10 @@
> > > >  #define        PAD_GMAC0_TXEN          18
> > > >  #define        PAD_GMAC0_TXC           19
> > > >  
> > > > +/* virtual pins for forcing GPI */
> > > > +#define PAD_INTERNAL_LOW       254
> > > > +#define PAD_INTERNAL_HIGH      255
> > > 
> > > Why this cannot be 20 and 21? These are not values for registers,
> > > but
> > > abstract numbers.
> > 
> > The number must not collide with SYS GPIO pads too.
> 
> There are no SYS GPIO pads here. Do you understand that this is not
> value for registers?

Yes I understand.

The situation is that JH7110 has two similar pin mux controllers, one
SYSGPIO and one AONGPIO. Despite I listed the values after the AONGPIO
pad list, these values should apply to SYSGPIO too (unless you want to
let them have different values for these two pinmux controllers), which
is the part with comment "sys_iomux pins".

> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/3] dt-bindings: pinctrl: starfive,jh7110: add PAD_INTERNAL_* virtual pins

[Krzysztof Kozlowski]

On Tue, Apr 29, 2025 at 05:00:04PM GMT, Icenowy Zheng wrote:
> > > > > +/* virtual pins for forcing GPI */
> > > > > +#define PAD_INTERNAL_LOW       254
> > > > > +#define PAD_INTERNAL_HIGH      255
> > > > 
> > > > Why this cannot be 20 and 21? These are not values for registers,
> > > > but
> > > > abstract numbers.
> > > 
> > > The number must not collide with SYS GPIO pads too.
> > 
> > There are no SYS GPIO pads here. Do you understand that this is not
> > value for registers?
> 
> Yes I understand.
> 
> The situation is that JH7110 has two similar pin mux controllers, one
> SYSGPIO and one AONGPIO. Despite I listed the values after the AONGPIO
> pad list, these values should apply to SYSGPIO too (unless you want to
> let them have different values for these two pinmux controllers), which
> is the part with comment "sys_iomux pins".

It is fine for me in such case.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 2/3] pinctrl: starfive: jh7110: add support for PAD_INTERNAL_* for GPI

[Icenowy Zheng]

The JH7110 SoC's both pin controller support routing GPI signals to
internal fixed low/high level.

As we allocated two special "pin" numbers for these situations
(PAD_INTERNAL_{LOW,HIGH}), add special handling code for these "pins".
The DOEn/DOUT/FUNCTION fields are ignored and the internal input signal
specified by the DIN field is routed to fixed low/high level.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 .../starfive/pinctrl-starfive-jh7110.c        | 41 +++++++++++++++----
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
index 1d0d6c224c104..fb18c7974ec86 100644
--- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
+++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
@@ -291,6 +291,24 @@ void jh7110_set_gpiomux(struct jh7110_pinctrl *sfp, unsigned int pin,
 }
 EXPORT_SYMBOL_GPL(jh7110_set_gpiomux);
 
+static void jh7110_set_gpi(struct jh7110_pinctrl *sfp, u32 gpi, u32 val)
+{
+	u32 offset, shift;
+	u32 reg_val;
+	const struct jh7110_pinctrl_soc_info *info = sfp->info;
+
+	offset = 4 * (gpi / 4);
+	shift  = 8 * (gpi % 4);
+
+	reg_val = readl_relaxed(sfp->base +
+				info->gpi_reg_base + offset);
+	reg_val &= info->gpi_mask << shift;
+	reg_val |= (val & info->gpi_mask) << shift;
+
+	writel_relaxed(reg_val, sfp->base +
+				info->gpi_reg_base + offset);
+}
+
 static int jh7110_set_mux(struct pinctrl_dev *pctldev,
 			  unsigned int fsel, unsigned int gsel)
 {
@@ -307,14 +325,23 @@ static int jh7110_set_mux(struct pinctrl_dev *pctldev,
 	pinmux = group->data;
 	for (i = 0; i < group->grp.npins; i++) {
 		u32 v = pinmux[i];
+		u32 pin = jh7110_pinmux_pin(v);
 
-		if (info->jh7110_set_one_pin_mux)
-			info->jh7110_set_one_pin_mux(sfp,
-					jh7110_pinmux_pin(v),
-					jh7110_pinmux_din(v),
-					jh7110_pinmux_dout(v),
-					jh7110_pinmux_doen(v),
-					jh7110_pinmux_function(v));
+		switch (pin) {
+		case PAD_INTERNAL_LOW:
+		case PAD_INTERNAL_HIGH:
+			jh7110_set_gpi(sfp, jh7110_pinmux_din(v),
+				       pin == PAD_INTERNAL_HIGH);
+			break;
+		default:
+			if (info->jh7110_set_one_pin_mux)
+				info->jh7110_set_one_pin_mux(sfp,
+						jh7110_pinmux_pin(v),
+						jh7110_pinmux_din(v),
+						jh7110_pinmux_dout(v),
+						jh7110_pinmux_doen(v),
+						jh7110_pinmux_function(v));
+		}
 	}
 
 	return 0;
-- 
2.49.0

Re: [PATCH v2 2/3] pinctrl: starfive: jh7110: add support for PAD_INTERNAL_* for GPI

[E Shattow]

On 4/23/25 23:20, Icenowy Zheng wrote:
> The JH7110 SoC's both pin controller support routing GPI signals to
> internal fixed low/high level.
> 
> As we allocated two special "pin" numbers for these situations
> (PAD_INTERNAL_{LOW,HIGH}), add special handling code for these "pins".
> The DOEn/DOUT/FUNCTION fields are ignored and the internal input signal
> specified by the DIN field is routed to fixed low/high level.
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  .../starfive/pinctrl-starfive-jh7110.c        | 41 +++++++++++++++----
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> index 1d0d6c224c104..fb18c7974ec86 100644
> --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> @@ -291,6 +291,24 @@ void jh7110_set_gpiomux(struct jh7110_pinctrl *sfp, unsigned int pin,
>  }
>  EXPORT_SYMBOL_GPL(jh7110_set_gpiomux);
>  

> +static void jh7110_set_gpi(struct jh7110_pinctrl *sfp, u32 gpi, u32 val)
> +{
> +	u32 offset, shift;
> +	u32 reg_val;
> +	const struct jh7110_pinctrl_soc_info *info = sfp->info;
> +
> +	offset = 4 * (gpi / 4);
> +	shift  = 8 * (gpi % 4);
> +
> +	reg_val = readl_relaxed(sfp->base +
> +				info->gpi_reg_base + offset);
> +	reg_val &= info->gpi_mask << shift;
> +	reg_val |= (val & info->gpi_mask) << shift;
> +
> +	writel_relaxed(reg_val, sfp->base +
> +				info->gpi_reg_base + offset);
> +}
> +

Are there some bit ops masking macros common to Linux that should be
used here?

>  static int jh7110_set_mux(struct pinctrl_dev *pctldev,
>  			  unsigned int fsel, unsigned int gsel)
>  {
> @@ -307,14 +325,23 @@ static int jh7110_set_mux(struct pinctrl_dev *pctldev,
>  	pinmux = group->data;
>  	for (i = 0; i < group->grp.npins; i++) {
>  		u32 v = pinmux[i];
> +		u32 pin = jh7110_pinmux_pin(v);
>  
> -		if (info->jh7110_set_one_pin_mux)
> -			info->jh7110_set_one_pin_mux(sfp,
> -					jh7110_pinmux_pin(v),
> -					jh7110_pinmux_din(v),
> -					jh7110_pinmux_dout(v),
> -					jh7110_pinmux_doen(v),
> -					jh7110_pinmux_function(v));

I cannot think of any reason why you would need to do it this way:

> +		switch (pin) {
> +		case PAD_INTERNAL_LOW:
> +		case PAD_INTERNAL_HIGH:
> +			jh7110_set_gpi(sfp, jh7110_pinmux_din(v),
> +				       pin == PAD_INTERNAL_HIGH);
> +			break;

Please, just more readable and let compiler do its job to optimize:

		switch (pin) {
		case PAD_INTERNAL_LOW:
			jh7110_set_gpi(sfp, jh7110_pinmux_din(v), 0);
			break;

		case PAD_INTERNAL_HIGH:
			jh7110_set_gpi(sfp, jh7110_pinmux_din(v), 1);
			break;

> +		default:
> +			if (info->jh7110_set_one_pin_mux)
> +				info->jh7110_set_one_pin_mux(sfp,
> +						jh7110_pinmux_pin(v),
> +						jh7110_pinmux_din(v),
> +						jh7110_pinmux_dout(v),
> +						jh7110_pinmux_doen(v),
> +						jh7110_pinmux_function(v));
> +		}
>  	}
>  
>  	return 0;

Thank you for your work on this series!  -E

Re: [PATCH v2 2/3] pinctrl: starfive: jh7110: add support for PAD_INTERNAL_* for GPI

[Icenowy Zheng]

在 2025-04-24星期四的 14:20 +0800，Icenowy Zheng写道：
> The JH7110 SoC's both pin controller support routing GPI signals to
> internal fixed low/high level.
> 
> As we allocated two special "pin" numbers for these situations
> (PAD_INTERNAL_{LOW,HIGH}), add special handling code for these
> "pins".
> The DOEn/DOUT/FUNCTION fields are ignored and the internal input
> signal
> specified by the DIN field is routed to fixed low/high level.

Oops today I found that this patchset has some problem -- the GPIOMUX
macro masks bits 7 and 8 in GPIO number, and it maps
GPI_SYS_USB_OVERCURRENT to GPIO63 instead of PAD_INTERNAL_HIGH instead.

When I fixed this, I got

```
[    9.865841] starfive-jh7110-sys-pinctrl 13040000.pinctrl: pin 255 is
not registered so it cannot be requested
[    9.875814] starfive-jh7110-sys-pinctrl 13040000.pinctrl: error -
EINVAL: pin-255 (soc:usb@10100000)
[    9.884882] starfive-jh7110-sys-pinctrl 13040000.pinctrl: error -
EINVAL: could not request pin 255 (non-existing) from group usb0-
0.overcurrent-pins on device 13040000.pinctrl
```

Weird problem, how could I solve this?

> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  .../starfive/pinctrl-starfive-jh7110.c        | 41 +++++++++++++++--
> --
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> index 1d0d6c224c104..fb18c7974ec86 100644
> --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> @@ -291,6 +291,24 @@ void jh7110_set_gpiomux(struct jh7110_pinctrl
> *sfp, unsigned int pin,
>  }
>  EXPORT_SYMBOL_GPL(jh7110_set_gpiomux);
>  
> +static void jh7110_set_gpi(struct jh7110_pinctrl *sfp, u32 gpi, u32
> val)
> +{
> +       u32 offset, shift;
> +       u32 reg_val;
> +       const struct jh7110_pinctrl_soc_info *info = sfp->info;
> +
> +       offset = 4 * (gpi / 4);
> +       shift  = 8 * (gpi % 4);
> +
> +       reg_val = readl_relaxed(sfp->base +
> +                               info->gpi_reg_base + offset);
> +       reg_val &= info->gpi_mask << shift;
> +       reg_val |= (val & info->gpi_mask) << shift;
> +
> +       writel_relaxed(reg_val, sfp->base +
> +                               info->gpi_reg_base + offset);
> +}
> +
>  static int jh7110_set_mux(struct pinctrl_dev *pctldev,
>                           unsigned int fsel, unsigned int gsel)
>  {
> @@ -307,14 +325,23 @@ static int jh7110_set_mux(struct pinctrl_dev
> *pctldev,
>         pinmux = group->data;
>         for (i = 0; i < group->grp.npins; i++) {
>                 u32 v = pinmux[i];
> +               u32 pin = jh7110_pinmux_pin(v);
>  
> -               if (info->jh7110_set_one_pin_mux)
> -                       info->jh7110_set_one_pin_mux(sfp,
> -                                       jh7110_pinmux_pin(v),
> -                                       jh7110_pinmux_din(v),
> -                                       jh7110_pinmux_dout(v),
> -                                       jh7110_pinmux_doen(v),
> -                                       jh7110_pinmux_function(v));
> +               switch (pin) {
> +               case PAD_INTERNAL_LOW:
> +               case PAD_INTERNAL_HIGH:
> +                       jh7110_set_gpi(sfp, jh7110_pinmux_din(v),
> +                                      pin == PAD_INTERNAL_HIGH);
> +                       break;
> +               default:
> +                       if (info->jh7110_set_one_pin_mux)
> +                               info->jh7110_set_one_pin_mux(sfp,
> +                                               jh7110_pinmux_pin(v),
> +                                               jh7110_pinmux_din(v),
> +                                               jh7110_pinmux_dout(v)
> ,
> +                                               jh7110_pinmux_doen(v)
> ,
> +                                               jh7110_pinmux_functio
> n(v));
> +               }
>         }
>  
>         return 0;

[PATCH v2 3/3] riscv: dts: starfive: jh7110-pine64-star64: force no USB overcurrent

[Icenowy Zheng]

The Star64 board has no GPIOs to indicate USB overcurrent, however the
USB controller would stop to work if the overcurrent_n signal it gets is
low (which means overcurrent situations happening because of the _n).

Use the pin controller to force the overcurrent_n signal to be high in
order to ensure stable behavior of the USB controller.

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
This patch depends on [1] for including the necessary header file.

[1] https://lore.kernel.org/linux-riscv/20250424060605.638678-1-uwu@icenowy.me/T/#u

 arch/riscv/boot/dts/starfive/jh7110-pine64-star64.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-pine64-star64.dts b/arch/riscv/boot/dts/starfive/jh7110-pine64-star64.dts
index 31e825be2065a..4fb522d127e21 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-pine64-star64.dts
+++ b/arch/riscv/boot/dts/starfive/jh7110-pine64-star64.dts
@@ -91,6 +91,13 @@ GPOEN_ENABLE,
 			input-schmitt-disable;
 			slew-rate = <0>;
 		};
+
+		overcurrent-pins {
+			pinmux = <GPIOMUX(PAD_INTERNAL_HIGH,
+					  GPOUT_LOW,
+					  GPOEN_DISABLE,
+					  GPI_SYS_USB_OVERCURRENT)>;
+		};
 	};
 };
 
-- 
2.49.0

Re: [PATCH v2 3/3] riscv: dts: starfive: jh7110-pine64-star64: force no USB overcurrent

[kernel test robot]

Hi Icenowy,

kernel test robot noticed the following build errors:

[auto build test ERROR on linusw-pinctrl/devel]
[also build test ERROR on linusw-pinctrl/for-next robh/for-next linus/master v6.15-rc3 next-20250424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Icenowy-Zheng/dt-bindings-pinctrl-starfive-jh7110-add-PAD_INTERNAL_-virtual-pins/20250424-142640
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/20250424062154.655128-1-uwu%40icenowy.me
patch subject: [PATCH v2 3/3] riscv: dts: starfive: jh7110-pine64-star64: force no USB overcurrent
config: riscv-randconfig-001-20250425 (https://download.01.org/0day-ci/archive/20250425/202504251758.YghAqPPR-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250425/202504251758.YghAqPPR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504251758.YghAqPPR-lkp@intel.com/

All errors (new ones prefixed by >>):

>> Error: arch/riscv/boot/dts/starfive/jh7110-pine64-star64.dts:96.87-88 syntax error
   FATAL ERROR: Unable to parse input tree

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki