mbox series

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

Message ID 20250424062017.652969-1-uwu@icenowy.me
Headers show
Series pinctrl: starfive: jh7110: support force inputs | expand

Message

Icenowy Zheng April 24, 2025, 6:20 a.m. UTC
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(-)

Comments

Linus Walleij April 24, 2025, 8:51 a.m. UTC | #1
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
Icenowy Zheng April 24, 2025, 9:38 a.m. UTC | #2
在 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
Linus Walleij April 24, 2025, 10:30 a.m. UTC | #3
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
Icenowy Zheng April 25, 2025, 8:43 a.m. UTC | #4
在 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