RE: [RFC PATCH 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree nodes

[Yuklin Soo <yuklin.soo@xxxxxxxxxxxxxxxx>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Thursday, December 21, 2023 11:53 PM
> To: Yuklin Soo <yuklin.soo@xxxxxxxxxxxxxxxx>; Linus Walleij
> <linus.walleij@xxxxxxxxxx>; 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>
> Cc: 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 6/6] riscv: dts: starfive: jh8100: add pinctrl device tree
> nodes
> 
> On 21/12/2023 09:36, Alex Soo wrote:
> > Add pinctrl_east/pinctrl_west/pinctrl_gmac/pinctrl_aon device tree
> > nodes for JH8100 SoC.
> >
> > Signed-off-by: Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>
> 
> I have some doubts about it...
> 
> > ---
> >  arch/riscv/boot/dts/starfive/jh8100-evb.dts   |   5 +
> >  arch/riscv/boot/dts/starfive/jh8100-pinfunc.h | 418 ++++++++++++++++++
> >  arch/riscv/boot/dts/starfive/jh8100.dtsi      |  44 ++
> >  3 files changed, 467 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/starfive/jh8100-pinfunc.h
> >
> > diff --git a/arch/riscv/boot/dts/starfive/jh8100-evb.dts
> > b/arch/riscv/boot/dts/starfive/jh8100-evb.dts
> > index c16bc25d8988..8634e41984f8 100644
> > --- a/arch/riscv/boot/dts/starfive/jh8100-evb.dts
> > +++ b/arch/riscv/boot/dts/starfive/jh8100-evb.dts
> > @@ -26,3 +26,8 @@ memory@40000000 {
> >  &uart0 {
> >  	status = "okay";
> >  };
> > +
> > +&pinctrl_aon {
> 
> Wrong order. Nodes do not go to the end.

Could you please explain what is meant by “Nodes do not go to the end.”? How it causes wrong order?

> 
> > +	wakeup-gpios = <&pinctrl_aon PAD_RGPIO2 GPIO_ACTIVE_HIGH>;
> > +	wakeup-source;
> 
> None of these were tested.
> 
> It does not look like you tested the DTS against bindings. Please run `make
> dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst
> or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-
> sources-with-the-devicetree-schema/
> for instructions).

The “pinctrl_aon” document has been updated:

  wakeup-gpios:
    maxItems: 1
    description: GPIO pin to be used for waking up the system from sleep mode.

  wakeup-source:
    maxItems: 1
    description: to indicate pinctrl has wakeup capability.

Running “make dt_binding_check” and “make dtbs_check” tests are successful.
Will send out in next version.

> 
> > +};
> > diff --git a/arch/riscv/boot/dts/starfive/jh8100-pinfunc.h
> > b/arch/riscv/boot/dts/starfive/jh8100-pinfunc.h
> > new file mode 100644
> > index 000000000000..3fb16ef62d90
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/starfive/jh8100-pinfunc.h
> > @@ -0,0 +1,418 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/*
> > + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> > + * Author: Alex Soo <yuklin.soo@xxxxxxxxxxxxxxxx>
> > + *
> > + */
> > +
> > +#ifndef __JH8100_PINFUNC_H__
> > +#define __JH8100_PINFUNC_H__
> > +
> > +/*
> > + * mux bits:
> > + *  | 31 - 24 | 23 - 16 | 15 - 10 |  9 - 8   |  7 - 0  |
> > + *  |  din    |  dout   |  doen   | function | gpio nr |
> > + *
> > + * dout:     output signal
> > + * doen:     output enable signal
> > + * din:      optional input signal, 0xff = none
> > + * function:
> > + * gpio nr:  gpio number, 0 - 63
> > + */
> > +#define GPIOMUX(n, dout, doen, din) ( \
> > +		(((din)  & 0xff) << 24) | \
> > +		(((dout) & 0xff) << 16) | \
> > +		(((doen) & 0x3f) << 10) | \
> > +		((n) & 0x3f))
> > +
> > +#define PINMUX(n, func) ((1 << 10) | (((func) & 0x3) << 8) | ((n) &
> > +0xff))
> > +
> > +/* sys_iomux_east dout */
> > +#define GPOUT_LOW				0
> > +#define GPOUT_HIGH				1
> 
> Where are these used?

These macros are used to set the logic level of a pin to 0 or 1 respectively:

        can0_pins: can0-grp {
                can0_rx-pins {
                        pinmux = <GPIOMUX(PAD_GPIO28_E, GPOUT_LOW,
                                GPOEN_SYS_DISABLE, GPI_SYS_CAN0_RXD)>;
                        bias-pull-up;
                        input-enable;
                };

However,  " GPOUT_LOW" and " GPOUT_LOW" will be removed from
"arch/riscv/boot/dts/starfive/jh8100-pinfunc.h", and kept in
" include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h".

> 
> > +#define GPOUT_SYS_CAN0_STBY			2
> > +#define GPOUT_SYS_CAN0_TST_NEXT_BIT		3
> > +#define GPOUT_SYS_CAN0_TST_SAMPLE_POINT		4
> > +#define GPOUT_SYS_CAN0_TXD			5
> > +#define GPOUT_SYS_I2C0_CLK			6
> > +#define GPOUT_SYS_I2C0_DATA			7
> > +#define GPOUT_SYS_I2S0_STEREO_RSCKO		8
> 
> You add here bunch of constants not used anywhere. No single example of
> their usage, not a one.

These constants are indexes of output signals, and it is part of the pinmux value
to be written to an iomux register to configure which output signal will go through
which GPIO_PAD.

For i2c-0,  

#define GPOUT_SYS_I2C0_CLK			6
#define GPOUT_SYS_I2C0_DATA			7

Output signal "GPOUT_SYS_I2C0_CLK" goes through PAD_GPIO9_E.
Output signal "GPOUT_SYS_I2C0_DATA" goes through PAD_GPIO10_E.

        i2c0_pins: i2c0-grp {
                i2c0-scl-pins {
                        pinmux = <GPIOMUX(PAD_GPIO9_E, GPOUT_SYS_I2C0_CLK,
                                  GPOEN_SYS_I2C0_CLK,
                                  GPI_SYS_I2C0_CLK)>;
                        bias-pull-up;
                        input-enable;
                };

                i2c0-sda-pins {
                        pinmux = <GPIOMUX(PAD_GPIO10_E, GPOUT_SYS_I2C0_DATA,
                                  GPOEN_SYS_I2C0_DATA,
                                  GPI_SYS_I2C0_DATA)>;
                        bias-pull-up;
                        input-enable;
                };
        };

Will add the above to device tree for next version.

> 
> Best regards,
> Krzysztof