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

From: Yuklin Soo
Date: Tue Mar 05 2024 - 07:01:40 EST

Next message: Frank Binns: "Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3"
Previous message: Sunil Khatri: "[PATCH v2] drm/amdgpu: add ring timeout information in devcoredump"
Next in thread: Krzysztof Kozlowski: "Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Tuesday, February 20, 2024 4:10 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 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.

Initially, I wanted to allow all properties in the pincfg and pinmux schema. I misunderstood the meaning of “additionalProperties: false”
and I thought it means all additional properties outside the pincfg and pinmux schema are excluded. The “additionalProperties” will be
set to “true” to include the rest of the properties in pincfg and pinmux schema and not to be restricted to only the properties defined in
the documents. This will be fixed in the next version.

>
>
> >>> +#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.

All the “PAD_GPIO*_*” and “PAD_RGPIO*” macros are used by the pinctrl driver and the JH8100 device tree. Dropping all these macros
will cause the delete of the DT-binding header file “include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h”. These macros will be moved to
the DTS header file “arch/riscv/boot/dts/starfive/jh8100-pinfunc.h” and the driver header file “drivers/pinctrl/starfive/pinctrl-starfive-jh8100.h”
and appear as duplicated in both places. Is that okay? Please advise.

>
> >
> >>
> >>> +
> >>> +/* 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.

All above macros are used by the pinctrl driver and the JH8100 device tree. Dropping these macros together with the PAD_GPIO* macros
will cause the delete of the DT-binding header file “include/dt-bindings/pinctrl/starfive,jh8100-pinctrl.h”. These macros will be moved to
the DTS header file “arch/riscv/boot/dts/starfive/jh8100-pinfunc.h” and the driver header file “drivers/pinctrl/starfive/pinctrl-starfive-jh8100.h”
and appear as duplicated in both places. Is that okay? Please advise.

>
> Best regards,
> Krzysztof

Next message: Frank Binns: "Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3"
Previous message: Sunil Khatri: "[PATCH v2] drm/amdgpu: add ring timeout information in devcoredump"
Next in thread: Krzysztof Kozlowski: "Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl bindings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]