Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings
From: Krzysztof Kozlowski <hidden>
Date: 2024-02-20 08:09:43
Also in:
linux-gpio, linux-riscv, lkml
On 07/02/2024 03:42, Yuklin Soo wrote:
quoted
quoted
+ 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: falseWhy 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)
I did not talk about subnodes. I asked why the rest of pincfg and pinmux schema properties are not allowed.
quoted
quoted
+#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 47Please explain why do you think these are bindings?The “PAD_GPIO*_*” represent the pin numbers of the PAD_GPIO pins in each domain.
So not bindings.
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)>;So not bindings. Read my question - I did not ask what are these. I asked why these are bindings. Your explanation suggests these are not. Drop. You can always store some defines in DTS headers.
quoted
quoted
+ +/* sys_iomux_east syscon */ +#define SYS_E_VREF_GPIO_E0_SYSCON_REG 0x0fc +#define SYS_E_VREF_GPIO_E0_SYSCON_MASK GENMASK(1,0)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#define SYS_E_VREF_ATB_USB_SYSCON_SHIFT 0Drop all of this, not bindings.All the SYSCON macros will be removed.quoted
quoted
+ +/* 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)quoted
+#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 0Drop all this.All the GMAC and SYSCON macros will be removed.quoted
quoted
+ +/* 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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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)quoted
+#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 0Drop all this.All the SYSCON macros will be removed.quoted
quoted
+ +/* sys_iomux_gmac timing (slew rate) registers */Srsly, "registers", so not bindings.All these will be removed.quoted
quoted
+ +#define GPOUT_LOW 0 +#define GPOUT_HIGH 1That's it. Really. Please do not redefine existing bindings.quoted
+ +#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 /* RMIImode, DVDD 2.5V/3.3V */quoted
+#define GMAC_RGMII_MODE 1 /*RGMII mode, DVDD 1.8V/2.5V */quoted
+ +/* gmac timing syscon value */ +#define GMAC_SLEW_RATE_FAST 0 +#define GMAC_SLEW_RATE_SLOW 1Drop 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
No, why? I think I commented quite strongly about it. Best regards, Krzysztof