> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Thursday, December 21, 2023 11:50 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 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@xxxxxxxxxxxxxxxx> > > Reviewed-by: Ley Foon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx> > > --- > > > 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@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@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@xxxxxxxxxxxxxxxx> > > + * > > + */ > > + > > +#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