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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list] 
Message-ID:
 <ZQ0PR01MB130288F0EC3C17147A46E4CDF622A@ZQ0PR01MB1302.CHNPR01.prod.partner.outlook.cn>
Date: Tue, 5 Mar 2024 12:00:45 +0000
From: Yuklin Soo <yuklin.soo@...rfivetech.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, 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



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Tuesday, February 20, 2024 4:10 PM
> 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-kernel@...r.kernel.org;
> devicetree@...r.kernel.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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ