Message ID | 1635324926-22319-4-git-send-email-wells.lu@sunplus.com |
---|---|
State | New |
Headers | show |
Series | Add pin control driver for Sunplus SP7021 SoC | expand |
Hi Wells Lu, thanks for your patch! On Wed, Oct 27, 2021 at 10:55 AM Wells Lu <wellslutw@gmail.com> wrote: > + properties: > + pins: > + description: | > + Define pins which are used by pinctrl node's client device. > + > + It consists of one or more integers which represents the config > + setting for corresponding pin. Please use macro SPPCTL_IOPAD to > + define the integers for pins. > + > + The first argument of the macro is pin number, the second is pin > + type, the third is type of GPIO, the last is default output state > + of GPIO. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + > + function: > + description: | > + Define pin-function which is used by pinctrl node's client device. > + The name should be one of string in the following enumeration. > + $ref: "/schemas/types.yaml#/definitions/string" > + enum: [ SPI_FLASH, SPI_FLASH_4BIT, SPI_NAND, CARD0_EMMC, SD_CARD, > + UA0, FPGA_IFX, HDMI_TX, LCDIF, USB0_OTG, USB1_OTG ] > + > + groups: > + description: | > + Define pin-group in a specified pin-function. > + The name should be one of string in the following enumeration. > + $ref: "/schemas/types.yaml#/definitions/string" > + enum: [ SPI_FLASH1, SPI_FLASH2, SPI_FLASH_4BIT1, SPI_FLASH_4BIT2, > + SPI_NAND, CARD0_EMMC, SD_CARD, UA0, FPGA_IFX, HDMI_TX1, > + HDMI_TX2, HDMI_TX3, LCDIF, USB0_OTG, USB1_OTG ] Is it possible to use Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml for this like other drivers do? > + zero_func: > + description: | > + Disabled pins which are not used by pinctrl node's client device. > + $ref: /schemas/types.yaml#/definitions/uint32-array I have never seen this before. Can't you just use pin control hogs for this so the pin controller just take care of these pins? > + allOf: > + - if: > + properties: > + function: > + enum: > + - SPI_FLASH > + then: > + properties: > + groups: > + enum: > + - SPI_FLASH1 > + - SPI_FLASH2 > + - if: > + properties: > + function: > + enum: > + - SPI_FLASH_4BIT > + then: > + properties: > + groups: > + enum: > + - SPI_FLASH_4BIT1 > + - SPI_FLASH_4BIT2 > + - if: > + properties: > + function: > + enum: > + - SPI_NAND > + then: > + properties: > + groups: > + enum: > + - SPI_NAND > + - if: > + properties: > + function: > + enum: > + - CARD0_EMMC > + then: > + properties: > + groups: > + enum: > + - CARD0_EMMC > + - if: > + properties: > + function: > + enum: > + - SD_CARD > + then: > + properties: > + groups: > + enum: > + - SD_CARD > + - if: > + properties: > + function: > + enum: > + - UA0 > + then: > + properties: > + groups: > + enum: > + - UA0 > + - if: > + properties: > + function: > + enum: > + - FPGA_IFX > + then: > + properties: > + groups: > + enum: > + - FPGA_IFX > + - if: > + properties: > + function: > + enum: > + - HDMI_TX > + then: > + properties: > + groups: > + enum: > + - HDMI_TX1 > + - HDMI_TX2 > + - HDMI_TX3 > + - if: > + properties: > + function: > + enum: > + - LCDIF > + then: > + properties: > + groups: > + enum: > + - LCDIF > + - if: > + properties: > + function: > + enum: > + - USB0_OTG > + then: > + properties: > + groups: > + enum: > + - USB0_OTG > + - if: > + properties: > + function: > + enum: > + - USB1_OTG > + then: > + properties: > + groups: > + enum: > + - USB1_OTG This looks complex to me, I need feedback from bindings people on this. > + pins_uart0: pins_uart0 { > + function = "UA0"; > + groups = "UA0"; > + }; > + > + pins_uart1: pins_uart1 { > + pins = < > + SPPCTL_IOPAD(11,SPPCTL_PCTL_G_PMUX,MUXF_UA1_TX,0) > + SPPCTL_IOPAD(10,SPPCTL_PCTL_G_PMUX,MUXF_UA1_RX,0) > + SPPCTL_IOPAD(7,SPPCTL_PCTL_G_GPIO,0,SPPCTL_PCTL_L_OUT) > + >; > + }; This first looks like two ways to do the same thing? UART0 uses strings for group + function and uart1 control individual pins. Is it possible to just do it one way? I think the pins = <...> scheme includes also multiplexing settings and then it should be named pinmux = <...>: Please read Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml closely. Yours, Linus Walleij
Dear Linus! I am the person who wrote this driver. Let me answer to your questions... -----Original Message----- >> From: Linus Walleij <linus.walleij@linaro.org> >> Sent: Tuesday, November 9, 2021 12:00 PM >> To: Wells Lu <wellslutw@gmail.com> >> Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; >> robh+dt@kernel.org; devicetree@vger.kernel.org; qinjian@cqplus1.com; >> dvorkin@tibbo.com; Wells Lu 呂芳騰 <wells.lu@sunplus.com> >> Subject: Re: [PATCH 3/3] devicetree: bindings: pinctrl: Add bindings doc for >> Sunplus SP7021. >> >> >>> + zero_func: >>> + description: | >>> + Disabled pins which are not used by pinctrl node's client >> device. >>> + $ref: /schemas/types.yaml#/definitions/uint32-array >> I have never seen this before. Can't you just use pin control hogs for this so the >> pin controller just take care of these pins? zero_func is required. The bootloader may have different device tree (I am using general sp7021 DTS in my u-boot setup, for example), while the kernel DTS may be changed between boots and specifies it more precisely - it is configured by user. So u-boot DTB and kernel DTB may be different -> result is that some pins may be muxed wrongly after u-boot starts kernel. Or even in pre-u-boot stage (we have the bootloader that starts u-boot, called XBoot). This XBoot also do some muxing. So we need this feature to get rid of possible unneded muxes done before kernel has been started. There is the "group of pins" functions and individual pins that may intersect. You may have "group of pins", say, emmc preconfigured before kernel started (in general DTS for u-boot) and you may want to have the pin from emmc group to be muxed as, say, SD card detect. You mux it in kernel DTS as GPIO, it will be in correct GPIO state, configured correctly, but while emmc group is enabled (nobody disabled it in kernel DTS!) the pin will belong to emmc function (preset group) and will not be functional. I invented zero_func while has been debugging the problem like "why my Eth is not working when all pins are configured correctly and muxed to Eth". I spend some time to find that the pin I muxed to Eth has been muxed to SPI_FLASH GROUP in very early stage (in ROM boot). And I have no way to cleanup this mux group easily. zero_func is the way to easily guarantee that you will successfully and correctly mux some pins / functions on kernel load even if somebody muxed other pins to this functions before kernel. If I'd implement "automatic" mux cleanup before muxing some pin, the code would be more complex. I would like to keep code as simple as I can and give better control to user. >> >>> + allOf: >>> + - if: >>> + properties: >>> + function: >>> + enum: >>> + - SPI_FLASH >>> + then: >>> + properties: >>> + groups: >>> + enum: >>> + - SPI_FLASH1 >>> + - SPI_FLASH2 >>> + - if: >>> + properties: >>> + function: >>> + enum: >>> + - SPI_FLASH_4BIT >>> + then: >>> + properties: >>> + groups: >>> + enum: >>> + - SPI_FLASH_4BIT1 >>> + - SPI_FLASH_4BIT2 >>> + - if: >>> + properties: >>> + function: >>> + enum: >>> + - HDMI_TX >>> + then: >>> + properties: >>> + groups: >>> + enum: >>> + - HDMI_TX1 >>> + - HDMI_TX2 >>> + - HDMI_TX3 >>> + - if: >>> + properties: >>> + function: >>> + enum: >>> + - LCDIF >>> + then: >>> + properties: >>> + groups: >>> + enum: >>> + - LCDIF >>> >>> This looks complex to me, I need feedback from bindings people on this. sp7021 supports two types of muxes: 1) group muxing (1-N sets of predefined pins for some function) 2) individual pin muxing Some functions may be muxed only in group, like SPI_FLASH or HDMI. That's why we have pins = <...>; and function = <funcname>; group = <funcsubname-group>; second case could be cuted to function = <funcsubname-group> only; But I think, the syntax of a pair {function,group} fits SoC logic better. Especially if customer is reading possible muxes table for the chip. >>> >>> + pins_uart0: pins_uart0 { >>> + function = "UA0"; >>> + groups = "UA0"; >>> + }; >>> + >>> + pins_uart1: pins_uart1 { >>> + pins = < >>> + >> SPPCTL_IOPAD(11,SPPCTL_PCTL_G_PMUX,MUXF_UA1_TX,0) >>> + >> SPPCTL_IOPAD(10,SPPCTL_PCTL_G_PMUX,MUXF_UA1_RX,0) >>> + >> SPPCTL_IOPAD(7,SPPCTL_PCTL_G_GPIO,0,SPPCTL_PCTL_L_OUT) >>> + >; >>> + }; >> This first looks like two ways to do the same thing? >> UART0 uses strings for group + function and uart1 control individual pins. >> >> Is it possible to just do it one way? >> >> I think the pins = <...> scheme includes also multiplexing settings and then it >> should be named pinmux = <...>: No. Sorry. It is two different way of supported two different types of muxing, described above. >> >> Please read >> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml >> closely. >> >> Yours, >> Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml new file mode 100644 index 0000000..7cfa0ce --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml @@ -0,0 +1,277 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) Sunplus Co., Ltd. 2021 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/sunplus,sp7021-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sunplus SP7021 Pin Controller Device Tree Bindings + +maintainers: + - Dvorkin Dmitry <dvorkin@tibbo.com> + - Wells Lu <wells.lu@sunplus.com> + +description: | + The Sunplus SP7021 pin controller is used to control SoC pins. Please + refer to pinctrl-bindings.txt in this directory for details of the common + pinctrl bindings used by client devices. + + Refer to https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/pages/ + 1443495991/How+to+setup+pins+of+SP7021+in+device-tree+source + + The device node of pin controller of Sunplus SP7021 has following + properties. + +properties: + compatible: + const: sunplus,sp7021-pctl + + gpio-controller: true + + '#gpio-cells': + const: 2 + + reg: + items: + - description: Base address and length of the MOON2 registers. + - description: Base address and length of the GPIOXT registers. + - description: Base address and length of the GPIOXT2 registers. + - description: Base address and length of the FIRST registers. + - description: Base address and length of the MOON1 registers. + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + +patternProperties: + '^.*$': + if: + type: object + then: + description: | + A pinctrl node should contain at least one subnodes representing the + pins or function-pins group available on the machine. Each subnode + will list the pins it needs, and how they should be configured. + + Pinctrl node's client devices use subnodes for desired pin + configuration. Client device subnodes use below standard properties. + + properties: + pins: + description: | + Define pins which are used by pinctrl node's client device. + + It consists of one or more integers which represents the config + setting for corresponding pin. Please use macro SPPCTL_IOPAD to + define the integers for pins. + + The first argument of the macro is pin number, the second is pin + type, the third is type of GPIO, the last is default output state + of GPIO. + $ref: /schemas/types.yaml#/definitions/uint32-array + + function: + description: | + Define pin-function which is used by pinctrl node's client device. + The name should be one of string in the following enumeration. + $ref: "/schemas/types.yaml#/definitions/string" + enum: [ SPI_FLASH, SPI_FLASH_4BIT, SPI_NAND, CARD0_EMMC, SD_CARD, + UA0, FPGA_IFX, HDMI_TX, LCDIF, USB0_OTG, USB1_OTG ] + + groups: + description: | + Define pin-group in a specified pin-function. + The name should be one of string in the following enumeration. + $ref: "/schemas/types.yaml#/definitions/string" + enum: [ SPI_FLASH1, SPI_FLASH2, SPI_FLASH_4BIT1, SPI_FLASH_4BIT2, + SPI_NAND, CARD0_EMMC, SD_CARD, UA0, FPGA_IFX, HDMI_TX1, + HDMI_TX2, HDMI_TX3, LCDIF, USB0_OTG, USB1_OTG ] + + zero_func: + description: | + Disabled pins which are not used by pinctrl node's client device. + $ref: /schemas/types.yaml#/definitions/uint32-array + + additionalProperties: false + + allOf: + - if: + properties: + function: + enum: + - SPI_FLASH + then: + properties: + groups: + enum: + - SPI_FLASH1 + - SPI_FLASH2 + - if: + properties: + function: + enum: + - SPI_FLASH_4BIT + then: + properties: + groups: + enum: + - SPI_FLASH_4BIT1 + - SPI_FLASH_4BIT2 + - if: + properties: + function: + enum: + - SPI_NAND + then: + properties: + groups: + enum: + - SPI_NAND + - if: + properties: + function: + enum: + - CARD0_EMMC + then: + properties: + groups: + enum: + - CARD0_EMMC + - if: + properties: + function: + enum: + - SD_CARD + then: + properties: + groups: + enum: + - SD_CARD + - if: + properties: + function: + enum: + - UA0 + then: + properties: + groups: + enum: + - UA0 + - if: + properties: + function: + enum: + - FPGA_IFX + then: + properties: + groups: + enum: + - FPGA_IFX + - if: + properties: + function: + enum: + - HDMI_TX + then: + properties: + groups: + enum: + - HDMI_TX1 + - HDMI_TX2 + - HDMI_TX3 + - if: + properties: + function: + enum: + - LCDIF + then: + properties: + groups: + enum: + - LCDIF + - if: + properties: + function: + enum: + - USB0_OTG + then: + properties: + groups: + enum: + - USB0_OTG + - if: + properties: + function: + enum: + - USB1_OTG + then: + properties: + groups: + enum: + - USB1_OTG + +required: + - compatible + - reg + - "#gpio-cells" + - gpio-controller + - clocks + - resets + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/sp-sp7021.h> + #include <dt-bindings/reset/sp-sp7021.h> + #include <dt-bindings/pinctrl/sppctl-sp7021.h> + + pctl: pctl@9C000100 { + compatible = "sunplus,sp7021-pctl"; + reg = <0x9C000100 0x100>, <0x9C000300 0x80>, <0x9C000380 0x80>, + <0x9C0032e4 0x1C>, <0x9C000080 0x20>; + gpio-controller; + #gpio-cells = <2>; + clocks = <&clkc GPIO>; + resets = <&rstc RST_GPIO>; + + pins_uart0: pins_uart0 { + function = "UA0"; + groups = "UA0"; + }; + + pins_uart1: pins_uart1 { + pins = < + SPPCTL_IOPAD(11,SPPCTL_PCTL_G_PMUX,MUXF_UA1_TX,0) + SPPCTL_IOPAD(10,SPPCTL_PCTL_G_PMUX,MUXF_UA1_RX,0) + SPPCTL_IOPAD(7,SPPCTL_PCTL_G_GPIO,0,SPPCTL_PCTL_L_OUT) + >; + }; + + emmc_mux: emmc_mux { + function = "CARD0_EMMC"; + groups = "CARD0_EMMC"; + }; + + mmc1_mux: mmc1_mux { + function = "SD_CARD"; + groups = "SD_CARD"; + pins = < SPPCTL_IOPAD(91,SPPCTL_PCTL_G_GPIO,0,0) >; + }; + + hdmi_A_tx1: hdmi_A_tx1_pins { + function = "HDMI_TX"; + groups = "HDMI_TX1"; + }; + hdmi_A_tx2: hdmi_A_tx2_pins { + function = "HDMI_TX"; + groups = "HDMI_TX2"; + }; + hdmi_A_tx3: hdmi_A_tx3_pins { + function = "HDMI_TX"; + groups = "HDMI_TX3"; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index 9cae8e7..fe3f359 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14872,6 +14872,7 @@ M: Wells Lu <wells.lu@sunplus.com> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) S: Maintained W: https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview +F: Documentation/devicetree/bindings/pinctrl/sunplus,* F: drivers/pinctrl/sunplus/ F: include/dt-bindings/pinctrl/sppctl*
Add bindings documentation for Sunplus SP7021. Signed-off-by: Wells Lu <wells.lu@sunplus.com> --- .../bindings/pinctrl/sunplus,sp7021-pinctrl.yaml | 277 +++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 278 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/sunplus,sp7021-pinctrl.yaml