Thread (27 messages) 27 messages, 5 authors, 2024-03-27

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: 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)
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				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.
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		0
Drop 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		0
Drop 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		0
Drop 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				1
That'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	/* RMII
mode,  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			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
No, why?

I think I commented quite strongly about it.

Best regards,
Krzysztof
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help