Re: [PATCH 02/11] dt-bindings: pinctrl: Add RaspberryPi RP1 gpio/pinctrl/pinmux bindings
From: Andrea della Porta <andrea.porta@suse.com>
Date: 2024-08-30 10:22:02
Also in:
linux-arch, linux-clk, linux-devicetree, linux-gpio, linux-pci, lkml, netdev
Hi Krzysztof, On 10:42 Wed 21 Aug , Krzysztof Kozlowski wrote:
On Tue, Aug 20, 2024 at 04:36:04PM +0200, Andrea della Porta wrote:quoted
Add device tree bindings for the gpio/pin/mux controller that is part of the RP1 multi function device, and relative entries in MAINTAINERS file. Signed-off-by: Andrea della Porta <andrea.porta@suse.com> --- .../pinctrl/raspberrypi,rp1-gpio.yaml | 177 +++++++++++++ MAINTAINERS | 2 + include/dt-bindings/misc/rp1.h | 235 ++++++++++++++++++ 3 files changed, 414 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml create mode 100644 include/dt-bindings/misc/rp1.hdiff --git a/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml new file mode 100644 index 000000000000..7011fa258363 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml@@ -0,0 +1,177 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/raspberrypi,rp1-gpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: RaspberryPi RP1 GPIO/Pinconf/Pinmux Controller submodule + +maintainers: + - Andrea della Porta <andrea.porta@suse.com> + +description: + The RP1 chipset is a Multi Function Device containing, among other sub-peripherals, + a gpio/pinconf/mux controller whose 54 pins are grouped into 3 banks. It works also + as an interrupt controller for those gpios. + + Each pin configuration node lists the pin(s) to which it applies, and one or + more of the mux function to select on those pin(s), and their configuration. + The pin configuration and multiplexing supports the generic bindings. + For details on each properties (including the meaning of "pin configuration node"), + you can refer to ./pinctrl-bindings.txt. + +properties: + compatible: + const: raspberrypi,rp1-gpio + + reg: + minItems: 3You can drop minItems.
Ack.
quoted
+ maxItems: 3 + description: One reg specifier for each one of the 3 pin banks. + + '#gpio-cells': + description: The first cell is the pin number and the second cell is used + to specify the flags (see include/dt-bindings/gpio/gpio.h). + const: 2 + + gpio-controller: true + + gpio-ranges: + maxItems: 1 + + gpio-line-names: + maxItems: 54 + + interrupts: + minItems: 3Ditto
Ack.
quoted
+ maxItems: 3 + description: One interrupt specifier for each one of the 3 pin banks. + + '#interrupt-cells': + description: + Specifies the Bank number (as specified in include/dt-bindings/misc/rp1.h) + and Flags (as defined in (include/dt-bindings/interrupt-controller/irq.h). + Possible values for the Bank number are + RP1_INT_IO_BANK0 + RP1_INT_IO_BANK1 + RP1_INT_IO_BANK2 + const: 2 + + interrupt-controller: true + +additionalProperties: + anyOf: + - type: object + additionalProperties: false + allOf: + - $ref: pincfg-node.yaml# + - $ref: pinmux-node.yaml# + + description: + Pin controller client devices use pin configuration subnodes (children + and grandchildren) for desired pin configuration. + Client device subnodes use below standard properties. + + properties: + pins: + description: + A string (or list of strings) adhering to the pattern "gpio[0-5][0-9]" + function: true + bias-disable: true + bias-pull-down: true + bias-pull-up: true + slew-rate: + description: 0 is slow slew rate, 1 is fast slew rate + enum: [ 0, 1 ] + drive-strength: + description: 0 -> 2mA, 1 -> 4mA, 2 -> 8mA, 3 -> 12mA + enum: [ 0, 1, 2, 3 ]No, that's [ 2, 4, 8 and 12 ] Read description of the field - it is in specific units.
Ack. Thanks, this would have been a bug, since the driver was already using the current value instead of the positional.
quoted
+ + - type: object + additionalProperties: + $ref: "#/additionalProperties/anyOf/0" + +allOf: + - $ref: pinctrl.yaml# + +required: + - reg + - compatible + - "#gpio-cells" + - gpio-controller + - interrupts + - "#interrupt-cells" + - interrupt-controller + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/misc/rp1.h> + + rp1 { + #address-cells = <2>; + #size-cells = <2>; + + rp1_gpio: pinctrl@c0400d0000 { + reg = <0xc0 0x400d0000 0x0 0xc000>, + <0xc0 0x400e0000 0x0 0xc000>, + <0xc0 0x400f0000 0x0 0xc000>; + compatible = "raspberrypi,rp1-gpio"; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>, + <RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>, + <RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>; + gpio-line-names = + "ID_SDA", // GPIO0 + "ID_SCL", // GPIO1 + "GPIO2", "GPIO3", "GPIO4", "GPIO5", "GPIO6", + "GPIO7", "GPIO8", "GPIO9", "GPIO10", "GPIO11", + "GPIO12", "GPIO13", "GPIO14", "GPIO15", "GPIO16", + "GPIO17", "GPIO18", "GPIO19", "GPIO20", "GPIO21", + "GPIO22", "GPIO23", "GPIO24", "GPIO25", "GPIO26", + "GPIO27", + "PCIE_RP1_WAKE", // GPIO28 + "FAN_TACH", // GPIO29 + "HOST_SDA", // GPIO30 + "HOST_SCL", // GPIO31 + "ETH_RST_N", // GPIO32 + "", // GPIO33 + "CD0_IO0_MICCLK", // GPIO34 + "CD0_IO0_MICDAT0", // GPIO35 + "RP1_PCIE_CLKREQ_N", // GPIO36 + "", // GPIO37 + "CD0_SDA", // GPIO38 + "CD0_SCL", // GPIO39 + "CD1_SDA", // GPIO40 + "CD1_SCL", // GPIO41 + "USB_VBUS_EN", // GPIO42 + "USB_OC_N", // GPIO43 + "RP1_STAT_LED", // GPIO44 + "FAN_PWM", // GPIO45 + "CD1_IO0_MICCLK", // GPIO46 + "2712_WAKE", // GPIO47 + "CD1_IO1_MICDAT1", // GPIO48 + "EN_MAX_USB_CUR", // GPIO49 + "", // GPIO50 + "", // GPIO51 + "", // GPIO52 + ""; // GPIO53 + + rp1_uart0_14_15: rp1_uart0_14_15 { + pin_txd { + function = "uart0"; + pins = "gpio14"; + bias-disable; + }; + + pin_rxd { + function = "uart0"; + pins = "gpio15"; + bias-pull-up; + }; + }; + }; + };diff --git a/MAINTAINERS b/MAINTAINERS index 6e7db9bce278..c5018232c251 100644 --- a/MAINTAINERS +++ b/MAINTAINERS@@ -19120,7 +19120,9 @@ RASPBERRY PI RP1 PCI DRIVER M: Andrea della Porta <andrea.porta@suse.com> S: Maintained F: Documentation/devicetree/bindings/clock/raspberrypi,rp1-clocks.yaml +F: Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml F: include/dt-bindings/clock/rp1.h +F: include/dt-bindings/misc/rp1.h RC-CORE / LIRC FRAMEWORK M: Sean Young <sean@mess.org>diff --git a/include/dt-bindings/misc/rp1.h b/include/dt-bindings/misc/rp1.hFilename should base on the compatible.
include/dt-bindings/misc/rp1.h is just a shared header currently included from: - arch/arm64/boot/dts/broadcom/rp1.dtso (use RP1_INT_IO_BANKx) - drivers/misc/rp1/rp1-pci.c (use RP1_INT_END) - Documentation/devicetree/bindings/pinctrl/raspberrypi,rp1-gpio.yaml (use RP1_INT_IO_BANKx) and the driver it is logically bound to (misc/rp1-pci.c) has no compatible, since it is a PCI driver. If you consider feasible to drop those interrupt defines in yaml bindings (please, see below), then I can just drop the entire header placing those defines inside rp1-pci.c, otherwise we should come up with a new filename. If this is the case, I kindly ask for a suggestion.
The same applies to clocks bindings.
Ack.
quoted
new file mode 100644 index 000000000000..6dd5e23870c2--- /dev/null +++ b/include/dt-bindings/misc/rp1.h@@ -0,0 +1,235 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/* + * This header provides constants for the PY MFD. + */ + +#ifndef _RP1_H +#define _RP1_HThat's very poor header guard...
Ack.
quoted
+ +/* Address map */ +#define RP1_SYSINFO_BASE 0x000000 +#define RP1_TBMAN_BASE 0x004000Nope, addresses are not bindings. Drop entire header.
I've dropped the address part.
quoted
+#define RP1_SYSCFG_BASE 0x008000 +#define RP1_OTP_BASE 0x00c000 +#define RP1_POWER_BASE 0x010000 +#define RP1_RESETS_BASE 0x014000 +#define RP1_CLOCKS_BANK_DEFAULT_BASE 0x018000 +#define RP1_CLOCKS_BANK_VIDEO_BASE 0x01c000 +#define RP1_PLL_SYS_BASE 0x020000 +#define RP1_PLL_AUDIO_BASE 0x024000 +#define RP1_PLL_VIDEO_BASE 0x028000 +#define RP1_UART0_BASE 0x030000 +#define RP1_UART1_BASE 0x034000 +#define RP1_UART2_BASE 0x038000 +#define RP1_UART3_BASE 0x03c000 +#define RP1_UART4_BASE 0x040000 +#define RP1_UART5_BASE 0x044000 +#define RP1_SPI8_BASE 0x04c000 +#define RP1_SPI0_BASE 0x050000 +#define RP1_SPI1_BASE 0x054000 +#define RP1_SPI2_BASE 0x058000 +#define RP1_SPI3_BASE 0x05c000 +#define RP1_SPI4_BASE 0x060000 +#define RP1_SPI5_BASE 0x064000 +#define RP1_SPI6_BASE 0x068000 +#define RP1_SPI7_BASE 0x06c000 +#define RP1_I2C0_BASE 0x070000 +#define RP1_I2C1_BASE 0x074000 +#define RP1_I2C2_BASE 0x078000 +#define RP1_I2C3_BASE 0x07c000 +#define RP1_I2C4_BASE 0x080000 +#define RP1_I2C5_BASE 0x084000 +#define RP1_I2C6_BASE 0x088000 +#define RP1_AUDIO_IN_BASE 0x090000 +#define RP1_AUDIO_OUT_BASE 0x094000 +#define RP1_PWM0_BASE 0x098000 +#define RP1_PWM1_BASE 0x09c000 +#define RP1_I2S0_BASE 0x0a0000 +#define RP1_I2S1_BASE 0x0a4000 +#define RP1_I2S2_BASE 0x0a8000 +#define RP1_TIMER_BASE 0x0ac000 +#define RP1_SDIO0_APBS_BASE 0x0b0000 +#define RP1_SDIO1_APBS_BASE 0x0b4000 +#define RP1_BUSFABRIC_MONITOR_BASE 0x0c0000 +#define RP1_BUSFABRIC_AXISHIM_BASE 0x0c4000 +#define RP1_ADC_BASE 0x0c8000 +#define RP1_IO_BANK0_BASE 0x0d0000 +#define RP1_IO_BANK1_BASE 0x0d4000 +#define RP1_IO_BANK2_BASE 0x0d8000 +#define RP1_SYS_RIO0_BASE 0x0e0000 +#define RP1_SYS_RIO1_BASE 0x0e4000 +#define RP1_SYS_RIO2_BASE 0x0e8000 +#define RP1_PADS_BANK0_BASE 0x0f0000 +#define RP1_PADS_BANK1_BASE 0x0f4000 +#define RP1_PADS_BANK2_BASE 0x0f8000 +#define RP1_PADS_ETH_BASE 0x0fc000 +#define RP1_ETH_IP_BASE 0x100000 +#define RP1_ETH_CFG_BASE 0x104000 +#define RP1_PCIE_APBS_BASE 0x108000 +#define RP1_MIPI0_CSIDMA_BASE 0x110000 +#define RP1_MIPI0_CSIHOST_BASE 0x114000 +#define RP1_MIPI0_DSIDMA_BASE 0x118000 +#define RP1_MIPI0_DSIHOST_BASE 0x11c000 +#define RP1_MIPI0_MIPICFG_BASE 0x120000 +#define RP1_MIPI0_ISP_BASE 0x124000 +#define RP1_MIPI1_CSIDMA_BASE 0x128000 +#define RP1_MIPI1_CSIHOST_BASE 0x12c000 +#define RP1_MIPI1_DSIDMA_BASE 0x130000 +#define RP1_MIPI1_DSIHOST_BASE 0x134000 +#define RP1_MIPI1_MIPICFG_BASE 0x138000 +#define RP1_MIPI1_ISP_BASE 0x13c000 +#define RP1_VIDEO_OUT_CFG_BASE 0x140000 +#define RP1_VIDEO_OUT_VEC_BASE 0x144000 +#define RP1_VIDEO_OUT_DPI_BASE 0x148000 +#define RP1_XOSC_BASE 0x150000 +#define RP1_WATCHDOG_BASE 0x154000 +#define RP1_DMA_TICK_BASE 0x158000 +#define RP1_SDIO_CLOCKS_BASE 0x15c000 +#define RP1_USBHOST0_APBS_BASE 0x160000 +#define RP1_USBHOST1_APBS_BASE 0x164000 +#define RP1_ROSC0_BASE 0x168000 +#define RP1_ROSC1_BASE 0x16c000 +#define RP1_VBUSCTRL_BASE 0x170000 +#define RP1_TICKS_BASE 0x174000 +#define RP1_PIO_APBS_BASE 0x178000 +#define RP1_SDIO0_AHBLS_BASE 0x180000 +#define RP1_SDIO1_AHBLS_BASE 0x184000 +#define RP1_DMA_BASE 0x188000 +#define RP1_RAM_BASE 0x1c0000 +#define RP1_RAM_SIZE 0x020000 +#define RP1_USBHOST0_AXIS_BASE 0x200000 +#define RP1_USBHOST1_AXIS_BASE 0x300000 +#define RP1_EXAC_BASE 0x400000 + +/* Interrupts */ + +#define RP1_INT_IO_BANK0 0 +#define RP1_INT_IO_BANK1 1Also no, interrupt numbers are not considered bindings. That's too much churn. Otherwise, please point me to driver code using the define (directly! that's the requirement).
As mentioned above, RP1_INT_END is used in rp1-pci.c. To get rid of all those
macroes from dt-binding would mean to hardcode the interrupt number in both
the binding example and in dtso, from this:
interrupts = <RP1_INT_IO_BANK0 IRQ_TYPE_LEVEL_HIGH>,
<RP1_INT_IO_BANK1 IRQ_TYPE_LEVEL_HIGH>,
<RP1_INT_IO_BANK2 IRQ_TYPE_LEVEL_HIGH>;
to this:
interrupts = <0 IRQ_TYPE_LEVEL_HIGH>,
<1 IRQ_TYPE_LEVEL_HIGH>,
<2 IRQ_TYPE_LEVEL_HIGH>;
is this what you are proposing?
Many thanks,
Andrea
Best regards, Krzysztof