| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2236948f-bd68-4476-bc2a-814c46c99334@linaro.org> Date: Tue, 20 Feb 2024 09:09:39 +0100 From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> To: Yuklin Soo <yuklin.soo@...rfivetech.com>, Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Hal Feng <hal.feng@...rfivetech.com>, Leyfoon Tan <leyfoon.tan@...rfivetech.com>, Jianlong Huang <jianlong.huang@...rfivetech.com>, Emil Renner Berthing <kernel@...il.dk>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Drew Fustini <drew@...gleboard.org> Cc: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>, Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu> 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. >>> +#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
Powered by blists - more mailing lists