Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings
From: Krzysztof Kozlowski
Date: Tue Feb 20 2024 - 03:09:51 EST
On 07/02/2024 03:42, Yuklin Soo wrote:
>>
>>> + 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.
>>> +#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.
>
>>
>>> +
>>> +/* 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
No, why?
I think I commented quite strongly about it.
Best regards,
Krzysztof