RE: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings

[Yuklin Soo <yuklin.soo@xxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: Thursday, December 21, 2023 9:57 PM
> To: Yuklin Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>; Hal Feng
> <hal.feng@xxxxxxxxxxxxxxxx>; Leyfoon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>;
> Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>; Emil Renner Berthing
> <kernel@xxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> Drew Fustini <drew@xxxxxxxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> riscv@xxxxxxxxxxxxxxxxxxx; Paul Walmsley <paul.walmsley@xxxxxxxxxx>; Palmer
> Dabbelt <palmer@xxxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx>
> Subject: Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl
> bindings
> 
> Hi Alex,
> 
> thanks for your patch!
> 
> On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> wrote:
> 
> > Add dt-binding documentation and header file for JH8100 pinctrl
> > driver.
> >
> > Signed-off-by: Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>
> (...)
> > +title: StarFive JH8100 AON Pin Controller
> 
> If AON means "always-on" then spell that out in the title, the world has too
> many opaque TLAs.
> 
> title: StarFive JH8100 AON (always-on) Pin Controller

This title will be updated as shown above to clarify the meaning of acronym AON.

> 
> (...)
> > +        properties:
> > +          pinmux:
> > +            description: |
> > +              The list of GPIOs and their mux settings or function select.
> > +              The GPIOMUX and PINMUX macros are used to configure the
> > +              I/O multiplexing and function selection respectively.
> > +
> > +          bias-disable: true
> > +
> > +          bias-pull-up:
> > +            type: boolean
> > +
> > +          bias-pull-down:
> > +            type: boolean
> > +
> > +          drive-strength:
> > +            enum: [ 2, 4, 8, 12 ]
> 
> Milliamperes? Then spell that out in a description:

Yes, the unit is milliamperes, “drive-strength:” will be changed to “drive-strength: Drive strength in mA”

> 
> > +  Voltage regulator supply the actual voltage to the GPIO bank while
> > + the syscon register  configuration in bit [1:0] indicates the current voltage
> setting.
> > +
> > +  +------+------+-------------------+
> > +  | Bit1 | Bit0 | Reference Voltage |
> > + +------+------+-------------------+
> > +  |  0   |  0   |     3.3 V         |
> > +  +------+------+-------------------+
> > +  |  1   |  x   |     1.8 V         |
> > +  +------+------+-------------------+
> > +
> > +  To increase the device voltage, set bit [1:0] to the new operating
> > + state first before  raising the actual voltage to the higher operating point.
> > +
> > +  To decrease the device voltage, hold bit [1:0] to the current
> > + operating state until  the actual voltage has stabilized at the
> > + lower operating point before changing the  setting.
> > +
> > +  Alternatively, a device voltage change can always be initiated by
> > + first setting syscon  register bit [1:0] = 0, the safe 3.3V startup
> > + condition, before changing the device  voltage. Then once the actual
> > + voltage is changed and has stabilized at the new operating  point, bit [1:0]
> can be reset as appropriate.
> 
> Actually: where is this information even used?

Each pin controller in JH8100 SoC has a set of regmap-based syscon registers to serve different purposes in respective pinctrl domain.
In AON (always-on) pinctrl, there are: 
-	syscon registers to indicate the I/O voltage level of eMMC, SD0, RGPIOs, and XSPI.
-	syscon registers to control the GMAC settings.

The former will be used in the SD/eMMC device driver to indicate the change in voltage supply during voltage switching in the initialization process. The syscon register configuration provides information to indicate the device I/O voltage level. The device driver must make sure these syscon registers are configured properly. In case if setting syscon register to indicate device I/O voltage as 1.8V, but the actual voltage supply is 3.3V. The pads used for the device I/O interface will get damaged.

> 
> > +          slew-rate:
> > +            maximum: 1
> 
> Milliseconds? Write unit in description please.

The slew-rate is a single bit in the IO Pad configuration register. Bit value 0 means slow and 1 means fast.

          slew-rate:
            maximum: 1

will be changed to 

      slew-rate:
        enum: [ 0,  1 ]
        default: 0
        description: |
            0: slow (half frequency)
            1: fast

Is that okay?

> 
> Yours,
> Linus Walleij