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

Yuklin Soo yuklin.soo at starfivetech.com
Tue Feb 6 18:42:47 PST 2024



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
> Sent: Thursday, December 21, 2023 11:50 PM
> To: Yuklin Soo <yuklin.soo at starfivetech.com>; Linus Walleij
> <linus.walleij at linaro.org>; Bartosz Golaszewski
> <bartosz.golaszewski at linaro.org>; Hal Feng <hal.feng at starfivetech.com>;
> Leyfoon Tan <leyfoon.tan at starfivetech.com>; Jianlong Huang
> <jianlong.huang at starfivetech.com>; Emil Renner Berthing
> <kernel at esmil.dk>; Rob Herring <robh at kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt at linaro.org>; Conor Dooley <conor+dt at kernel.org>;
> Drew Fustini <drew at beagleboard.org>
> Cc: linux-gpio at vger.kernel.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org; linux-riscv at lists.infradead.org; Paul Walmsley
> <paul.walmsley at sifive.com>; Palmer Dabbelt <palmer at dabbelt.com>;
> Albert Ou <aou at eecs.berkeley.edu>
> Subject: Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl
> bindings
> 
> On 21/12/2023 09:36, Alex Soo wrote:
> > Add dt-binding documentation and header file for JH8100 pinctrl
> > driver.
> >
> > Signed-off-by: Alex Soo <yuklin.soo at starfivetech.com>
> > Reviewed-by: Ley Foon Tan <leyfoon.tan at starfivetech.com>
> > ---
> 
> 
> A nit, subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.
> 
> ...
> > +  i
> 
> nterrupt-controller: true
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +patternProperties:
> > +  '-[0-9]+$':
> 
> This is a bit too wide pattern. Consider some suffix like -grp or -group. Look at
> other bindings how they do it.

The regular expression "-[0-9]+$" will be changed to "-grp$" to standardize client node names to end with suffix "-grp" instead of "-<numerical _number>".
In device tree source, for instance, i2c0 node name will be changed to “i2c0-grp”.

> 
> > +    type: object
> > +    additionalProperties: false
> > +    patternProperties:
> > +      '-pins$':
> > +        type: object
> > +        description: |
> > +          A pinctrl node should contain at least one subnode representing the
> > +          pinctrl groups available in the domain. Each subnode will list the
> > +          pins it needs, and how they should be configured, with regard to
> > +          muxer configuration, bias, input enable/disable, input schmitt
> > +          trigger enable/disable, slew-rate and drive strength.
> > +        allOf:
> > +          - $ref: /schemas/pinctrl/pincfg-node.yaml
> > +          - $ref: /schemas/pinctrl/pinmux-node.yaml
> > +        additionalProperties: false
> 
> Why the rest of the properties is not applicable?

The regex “-pins$” make sure all client subnode names end with suffix “-pins” (e.g, i2c0-scl-pins, i2c-sda-pins)
All properties inside these subnodes are defined in ‘pincfg-node.yaml’ and ‘pinmux-mode.yaml’. No other properties are used beside that.

        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;
                };
        };
> 
> > +
> > +        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 ]
> > +
> > +          input-enable: true
> > +
> > +          input-disable: true
> > +
> > +          input-schmitt-enable: true
> > +
> > +          input-schmitt-disable: true
> > +
> > +          slew-rate:
> > +            maximum: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - resets
> > +  - interrupts
> > +  - interrupt-controller
> > +  - gpio-controller
> > +  - '#gpio-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/starfive,jh8100.h>
> > +    #include <dt-bindings/reset/starfive-jh8100.h>
> > +    #include <dt-bindings/pinctrl/starfive,jh8100-pinctrl.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        pinctrl_aon: pinctrl at 1f300000 {
> > +                compatible = "starfive,jh8100-aon-pinctrl",
> > +                             "syscon", "simple-mfd";
> 
> You did not even bother to test it before sending.
> 
> > +                reg = <0x0 0x1f300000 0x0 0x10000>;
> > +                resets = <&aoncrg 0>;
> 
> Use 4 spaces for example indentation.

Now 4 spaces are used for indentation.

> 
> > +                interrupts = <160>;
> > +                interrupt-controller;
> > +                gpio-controller;
> > +                #gpio-cells = <2>;
> 
> 
> Incomplete example.

Only the “pinctrl_gmac” does not provide any pinmuxing capability.
The example in “pinctrl_aon”, “pinctrl_east”, and “pinctrl_west” will be updated with the client driver pinmuxing.

examples:
  - |
    soc {
        #address-cells = <2>;
        #size-cells = <2>;

        pinctrl_aon: pinctrl at 1f300000 {
            compatible = "starfive,jh8100-aon-pinctrl", "syscon", "simple-mfd";
            reg = <0x0 0x1f300000 0x0 0x10000>;
            resets = <&aoncrg 0>;
            interrupts = <160>;
            interrupt-controller;
            gpio-controller;
            #gpio-cells = <2>;

            i2c7_pins: i2c7-grp {
                i2c7-scl-pins {
                    pinmux = <0x23265409>;
                    bias-pull-up;
                    input-enable;
                };

                i2c7-sda-pins {
                    pinmux = <0x2427580a>;
                    bias-pull-up;
                    input-enable;
                };
            };
        };
    };

> 
> I'll stop review, except one more comment, this was not tested and does not
> work. Reviewing code which was never tested is a waste of reviewer's time.
> 
> 
> ...
> 
> > diff --git a/include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h
> > b/include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h
> > new file mode 100644
> > index 000000000000..c57b7ece37a2
> > --- /dev/null
> > +++ b/include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h
> > @@ -0,0 +1,303 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/*
> > + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> > + * Author: Alex Soo <yuklin.soo at starfivetech.com>
> > + *
> > + */
> > +
> > +#ifndef __DT_BINDINGS_PINCTRL_STARFIVE_JH8100_H__
> > +#define __DT_BINDINGS_PINCTRL_STARFIVE_JH8100_H__
> > +
> > +/* sys_iomux_west pins */
> > +#define PAD_GPIO0_W				0
> > +#define PAD_GPIO1_W				1
> > +#define PAD_GPIO2_W				2
> > +#define PAD_GPIO3_W				3
> > +#define PAD_GPIO4_W				4
> > +#define PAD_GPIO5_W				5
> > +#define PAD_GPIO6_W				6
> > +#define PAD_GPIO7_W				7
> > +#define PAD_GPIO8_W				8
> > +#define PAD_GPIO9_W				9
> > +#define PAD_GPIO10_W				10
> > +#define PAD_GPIO11_W				11
> > +#define PAD_GPIO12_W				12
> > +#define PAD_GPIO13_W				13
> > +#define PAD_GPIO14_W				14
> > +#define PAD_GPIO15_W				15
> > +
> > +/* sys_iomux_west syscon */
> > +#define SYS_W_VREF_GPIO_W_SYSCON_REG		0x6c
> > +#define SYS_W_VREF_GPIO_W_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_W_VREF_GPIO_W_SYSCON_SHIFT		0
> > +#define SYS_W_VREF_ATB_ISP_VOUT_SYSCON_REG	0x70
> > +#define SYS_W_VREF_ATB_ISP_VOUT_SYSCON_MASK	GENMASK(1,
> 0)
> > +#define SYS_W_VREF_ATB_ISP_VOUT_SYSCON_SHIFT	0
> 
> None of these are bindings, drop.

All the SYSCON macros will be removed.

> 
> > +
> > +/* sys_iomux_east pins */
> > +#define PAD_GPIO0_E				0
> > +#define PAD_GPIO1_E				1
> > +#define PAD_GPIO2_E				2
> > +#define PAD_GPIO3_E				3
> > +#define PAD_GPIO4_E				4
> > +#define PAD_GPIO5_E				5
> > +#define PAD_GPIO6_E				6
> > +#define PAD_GPIO7_E				7
> > +#define PAD_GPIO8_E				8
> > +#define PAD_GPIO9_E				9
> > +#define PAD_GPIO10_E				10
> > +#define PAD_GPIO11_E				11
> > +#define PAD_GPIO12_E				12
> > +#define PAD_GPIO13_E				13
> > +#define PAD_GPIO14_E				14
> > +#define PAD_GPIO15_E				15
> > +#define PAD_GPIO16_E				16
> > +#define PAD_GPIO17_E				17
> > +#define PAD_GPIO18_E				18
> > +#define PAD_GPIO19_E				19
> > +#define PAD_GPIO20_E				20
> > +#define PAD_GPIO21_E				21
> > +#define PAD_GPIO22_E				22
> > +#define PAD_GPIO23_E				23
> > +#define PAD_GPIO24_E				24
> > +#define PAD_GPIO25_E				25
> > +#define PAD_GPIO26_E				26
> > +#define PAD_GPIO27_E				27
> > +#define PAD_GPIO28_E				28
> > +#define PAD_GPIO29_E				29
> > +#define PAD_GPIO30_E				30
> > +#define PAD_GPIO31_E				31
> > +#define PAD_GPIO32_E				32
> > +#define PAD_GPIO33_E				33
> > +#define PAD_GPIO34_E				34
> > +#define PAD_GPIO35_E				35
> > +#define PAD_GPIO36_E				36
> > +#define PAD_GPIO37_E				37
> > +#define PAD_GPIO38_E				38
> > +#define PAD_GPIO39_E				39
> > +#define PAD_GPIO40_E				40
> > +#define PAD_GPIO41_E				41
> > +#define PAD_GPIO42_E				42
> > +#define PAD_GPIO43_E				43
> > +#define PAD_GPIO44_E				44
> > +#define PAD_GPIO45_E				45
> > +#define PAD_GPIO46_E				46
> > +#define PAD_GPIO47_E				47
> 
> Please explain why do you think these are bindings?

The “PAD_GPIO*_*” represent the pin numbers of the PAD_GPIO pins in each domain.
It is part of the pinmux value. The pinmux value is generated by macro GPIOMUX as follow:

pinmux = <GPIOMUX(Pin_Number, Output_Signal_Index,
                   Output_Enable_Signal_Index,
                   Input_Signal_Index)>;

Use I2C0 as example,
pinmux = <GPIOMUX(PAD_GPIO9_E, GPOUT_SYS_I2C0_CLK,
                    GPOEN_SYS_I2C0_CLK,
                    GPI_SYS_I2C0_CLK)>;

> 
> > +
> > +/* sys_iomux_east syscon */
> > +#define SYS_E_VREF_GPIO_E0_SYSCON_REG		0x0fc
> > +#define SYS_E_VREF_GPIO_E0_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_E_VREF_GPIO_E0_SYSCON_SHIFT		0
> > +#define SYS_E_VREF_GPIO_E1_SYSCON_REG		0x100
> > +#define SYS_E_VREF_GPIO_E1_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_E_VREF_GPIO_E1_SYSCON_SHIFT		0
> > +#define SYS_E_VREF_GPIO_E2_SYSCON_REG		0x104
> > +#define SYS_E_VREF_GPIO_E2_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_E_VREF_GPIO_E2_SYSCON_SHIFT		0
> > +#define SYS_E_VREF_GPIO_E3_SYSCON_REG		0x108
> > +#define SYS_E_VREF_GPIO_E3_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_E_VREF_GPIO_E3_SYSCON_SHIFT		0
> > +#define SYS_E_VREF_ATB_STG1_SYSCON_REG		0x10c
> > +#define SYS_E_VREF_ATB_STG1_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_E_VREF_ATB_STG1_SYSCON_SHIFT	0
> > +#define SYS_E_VREF_ATB_USB_SYSCON_REG		0x110
> > +#define SYS_E_VREF_ATB_USB_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_E_VREF_ATB_USB_SYSCON_SHIFT		0
> 
> Drop all of this, not bindings.

All the SYSCON macros will be removed.

> 
> > +
> > +/* sys_iomux_gmac pins */
> > +#define PAD_GMAC1_MDC				0
> > +#define PAD_GMAC1_MDIO				1
> > +#define PAD_GMAC1_RXD0				2
> > +#define PAD_GMAC1_RXD1				3
> > +#define PAD_GMAC1_RXD2				4
> > +#define PAD_GMAC1_RXD3				5
> > +#define PAD_GMAC1_RXDV				6
> > +#define PAD_GMAC1_RXC				7
> > +#define PAD_GMAC1_TXD0				8
> > +#define PAD_GMAC1_TXD1				9
> > +#define PAD_GMAC1_TXD2				10
> > +#define PAD_GMAC1_TXD3				11
> > +#define PAD_GMAC1_TXEN				12
> > +#define PAD_GMAC1_TXC				13
> > +
> > +/* sys_iomux_gmac vref syscon registers */
> > +#define SYS_G_VREF_GMAC1_SYSCON_REG		0x08
> > +#define SYS_G_VREF_GMAC1_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_VREF_GMAC1_SYSCON_SHIFT		0
> > +#define SYS_G_VREF_SDIO1_SYSCON_REG		0x0c
> > +#define SYS_G_VREF_SDIO1_SYSCON_MASK		GENMASK(1, 0)
> > +#define SYS_G_VREF_SDIO1_SYSCON_SHIFT		0
> 
> Drop all this.

All the GMAC and SYSCON macros will be removed.

> 
> > +
> > +/* sys_iomux_gmac interface (rmii/rgmii) syscon registers */
> > +#define SYS_G_GMAC1_MDC_SYSCON_REG		0x10
> > +#define SYS_G_GMAC1_MDC_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_MDC_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_MDIO_SYSCON_REG		0x14
> > +#define SYS_G_GMAC1_MDIO_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_MDIO_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_RXD0_SYSCON_REG		0x18
> > +#define SYS_G_GMAC1_RXD0_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_RXD0_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_RXD1_SYSCON_REG		0x1c
> > +#define SYS_G_GMAC1_RXD1_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_RXD1_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_RXD2_SYSCON_REG		0x20
> > +#define SYS_G_GMAC1_RXD2_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_RXD2_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_RXD3_SYSCON_REG		0x24
> > +#define SYS_G_GMAC1_RXD3_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_RXD3_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_RXDV_SYSCON_REG		0x28
> > +#define SYS_G_GMAC1_RXDV_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_RXDV_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_RXC_SYSCON_REG		0x2c
> > +#define SYS_G_GMAC1_RXC_SYSCON_MASK		GENMASK(1, 0)
> > +#define SYS_G_GMAC1_RXC_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_TXD0_SYSCON_REG		0x30
> > +#define SYS_G_GMAC1_TXD0_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_TXD0_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_TXD1_SYSCON_REG		0x34
> > +#define SYS_G_GMAC1_TXD1_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_TXD1_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_TXD2_SYSCON_REG		0x38
> > +#define SYS_G_GMAC1_TXD2_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_TXD2_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_TXD3_SYSCON_REG		0x3c
> > +#define SYS_G_GMAC1_TXD3_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_TXD3_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_TXEN_SYSCON_REG		0x40
> > +#define SYS_G_GMAC1_TXEN_SYSCON_MASK		GENMASK(1,
> 0)
> > +#define SYS_G_GMAC1_TXEN_SYSCON_SHIFT		0
> > +#define SYS_G_GMAC1_TXC_SYSCON_REG		0x44
> > +#define SYS_G_GMAC1_TXC_SYSCON_MASK		GENMASK(1, 0)
> > +#define SYS_G_GMAC1_TXC_SYSCON_SHIFT		0
> 
> Drop all this.

All the SYSCON macros will be removed.

> 
> 
> > +
> > +/* sys_iomux_gmac timing (slew rate) registers */
> 
> Srsly, "registers", so not bindings.

All these will be removed.

> 
> 
> > +
> > +#define GPOUT_LOW				0
> > +#define GPOUT_HIGH				1
> 
> That's it. Really. Please do not redefine existing bindings.
> 
> > +
> > +#define GPOEN_ENABLE				0
> > +#define GPOEN_DISABLE				1
> > +
> > +#define GPI_NONE				255
> > +
> > +/* vref syscon value  */
> > +#define PAD_VREF_SYSCON_IO_3V3			0
> > +#define PAD_VREF_SYSCON_IO_1V8			2
> > +
> > +/* gmac interface (rmii/rgmii) syscon value */
> > +#define GMAC_RMII_MODE				0	/* RMII
> mode,  DVDD 2.5V/3.3V */
> > +#define GMAC_RGMII_MODE				1	/*
> RGMII mode, DVDD 1.8V/2.5V */
> > +
> > +/* gmac timing syscon value */
> > +#define GMAC_SLEW_RATE_FAST			0
> > +#define GMAC_SLEW_RATE_SLOW			1
> 
> Drop all above.

All SYSCON macros will be dropped.
However, the following will be kept in the header file,
#define GPOUT_LOW                               0
#define GPOUT_HIGH                              1

#define GPOEN_ENABLE                            0
#define GPOEN_DISABLE                           1

#define GPI_NONE                                255

> 
> > +
> > +#endif
> 
> Best regards,
> Krzysztof



More information about the linux-riscv mailing list