[1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller

Commit Message

Lorenzo Bianconi Aug. 11, 2024, 4:12 p.m. UTC

Introduce device-tree binding documentation for Airoha EN7581 pinctrl
controller.

Signed-off-by: Lorenzo Bianconi <[e-mail]>
---
 .../pinctrl/airoha,en7581-pinctrl.yaml        | 467 ++++++++++++++++++
 1 file changed, 467 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml

Comments

Krzysztof Kozlowski Aug. 12, 2024, 6:48 a.m. UTC | #1

On 11/08/2024 18:12, Lorenzo Bianconi wrote:
> Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> controller.
> 
> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> ---
>  .../pinctrl/airoha,en7581-pinctrl.yaml        | 467 ++++++++++++++++++
>  1 file changed, 467 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
> new file mode 100644
> index 000000000000..b1f980613864
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
> @@ -0,0 +1,467 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/airoha,en7581-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Airoha EN7581 Pin Controller
> +
> +maintainers:
> +  - Lorenzo Bianconi <[e-mail]>
> +
> +description:
> +  The Airoha's EN7581 Pin controller is used to control SoC pins.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - airoha,en7581-pinctrl
> +
> +  reg:
> +    items:
> +      - description: IOMUX base address
> +      - description: LED IOMUX base address
> +      - description: GPIO flash mode base address
> +      - description: GPIO flash mode extended base address
> +      - description: IO pin configuration base address
> +      - description: PCIE reset open-drain base address
> +      - description: GPIO bank0 data register base address
> +      - description: GPIO bank1 data register base address
> +      - description: GPIO bank0 first control register base address
> +      - description: GPIO bank0 second control register base address
> +      - description: GPIO bank1 first control register base address
> +      - description: GPIO bank1 second control register base address
> +      - description: GPIO bank0 output enable register base address
> +      - description: GPIO bank1 output enable register base address
> +      - description: GPIO bank0 irq status register base address
> +      - description: GPIO bank1 irq status register base address
> +      - description: GPIO bank0 irq level first control register base address
> +      - description: GPIO bank0 irq level second control register base address
> +      - description: GPIO bank1 irq level first control register base address
> +      - description: GPIO bank1 irq level second control register base address
> +      - description: GPIO bank0 irq edge first control register base address
> +      - description: GPIO bank0 irq edge second control register base address
> +      - description: GPIO bank1 irq edge first control register base address
> +      - description: GPIO bank1 irq edge second control register base address
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +    description:
> +      Number of cells in GPIO specifier. Since the generic GPIO binding is
> +      used, the amount of cells must be specified as 2. See the below mentioned
> +      gpio binding representation for description of particular cells.
> +
> +  gpio-ranges:
> +    maxItems: 1
> +    description:
> +      GPIO valid number range.
> +
> +  interrupt-controller: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +allOf:
> +  - $ref: pinctrl.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +patternProperties:

Keep it after properties: block.

> +  '-pins$':
> +    type: object
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '^.*mux.*$':
> +        type: object
> +        additionalProperties: false
> +        description: |

Do not need '|' unless you need to preserve formatting.

> +          pinmux configuration nodes.
> +
> +        $ref: /schemas/pinctrl/pinmux-node.yaml
> +        properties:
> +          function:
> +            description:
> +              A string containing the name of the function to mux to the group.
> +            enum: [pon, tod_1pps, sipo, mdio, uart, i2c, jtag, pcm, spi,
> +                   pcm_spi, i2s, emmc, pnand, pcie_reset, pwm, phy1_led0,
> +                   phy2_led0, phy3_led0, phy4_led0, phy1_led1, phy2_led1,
> +                   phy3_led1, phy4_led1]
> +          groups:

minItems: 1
maxItems: 32 (or whatever is sensible)

> +            description:
> +              An array of strings. Each string contains the name of a group.
> +        required:
> +          - function
> +          - groups
> +
> +        allOf:
> +          - if:
> +              properties:
> +                function:
> +                  const: pon
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pon]
> +          - if:
> +              properties:
> +                function:
> +                  const: tod_1pps
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pon_tod_1pps, gsw_tod_1pps]
> +          - if:
> +              properties:
> +                function:
> +                  const: sipo
> +            then:
> +              properties:
> +                groups:
> +                  enum: [sipo, sipo_rclk]
> +          - if:
> +              properties:
> +                function:
> +                  const: mdio
> +            then:
> +              properties:
> +                groups:
> +                  enum: [mdio]
> +          - if:
> +              properties:
> +                function:
> +                  const: uart
> +            then:
> +              properties:
> +                groups:
> +                  items:
> +                    enum: [uart2, uart2_cts_rts, hsuart, hsuart_cts_rts, uart4,
> +                           uart5]
> +                  maxItems: 2
> +          - if:
> +              properties:
> +                function:
> +                  const: i2c
> +            then:
> +              properties:
> +                groups:
> +                  enum: [i2c1]
> +          - if:
> +              properties:
> +                function:
> +                  const: jtag
> +            then:
> +              properties:
> +                groups:
> +                  enum: [jtag_udi, jtag_dfd]
> +          - if:
> +              properties:
> +                function:
> +                  const: pcm
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pcm1, pcm2]
> +          - if:
> +              properties:
> +                function:
> +                  const: spi
> +            then:
> +              properties:
> +                groups:
> +                  items:
> +                    enum: [spi_quad, spi_cs1]
> +                  maxItems: 2
> +          - if:
> +              properties:
> +                function:
> +                  const: pcm_spi
> +            then:
> +              properties:
> +                groups:
> +                  items:
> +                    enum: [pcm_spi, pcm_spi_int, pcm_spi_rst, pcm_spi_cs1,
> +                           pcm_spi_cs2_p156, pcm_spi_cs2_p128, pcm_spi_cs3,
> +                           pcm_spi_cs4]
> +                  maxItems: 7
> +          - if:
> +              properties:
> +                function:
> +                  const: i2c
> +            then:
> +              properties:
> +                groups:
> +                  enum: [i2s]
> +          - if:
> +              properties:
> +                function:
> +                  const: emmc
> +            then:
> +              properties:
> +                groups:
> +                  enum: [emmc]
> +          - if:
> +              properties:
> +                function:
> +                  const: pnand
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pnand]
> +          - if:
> +              properties:
> +                function:
> +                  const: pcie_reset
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pcie_reset0, pcie_reset1, pcie_reset2]
> +          - if:
> +              properties:
> +                function:
> +                  const: pwm
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio0, 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, gpio28, gpio29, gpio30, gpio31,
> +                         gpio36, gpio37, gpio38, gpio39, gpio40, gpio41,
> +                         gpio42, gpio43, gpio44, gpio45, gpio46, gpio47]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy1_led0
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio33, gpio34, gpio35, gpio42]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy2_led0
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio33, gpio34, gpio35, gpio42]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy3_led0
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio33, gpio34, gpio35, gpio42]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy4_led0
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio33, gpio34, gpio35, gpio42]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy1_led1
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio43, gpio44, gpio45, gpio46]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy2_led1
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio43, gpio44, gpio45, gpio46]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy3_led1
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio43, gpio44, gpio45, gpio46]
> +          - if:
> +              properties:
> +                function:
> +                  const: phy4_led1
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio43, gpio44, gpio45, gpio46]
> +
> +      '^.*conf.*$':
> +        type: object
> +        additionalProperties: false
> +        description:
> +          pinconf configuration nodes.
> +        $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> +        properties:
> +          pins:
> +            description:
> +              An array of strings. Each string contains the name of a pin.
> +            items:
> +              enum: [uart1_txd, uart1_rxd, i2c_scl, i2c_sda, spi_cs0, spi_clk,
> +                     spi_mosi, spi_miso, gpio0, 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, gpio28, gpio29, gpio30, gpio31, gpio32, gpio33,
> +                     gpio34, gpio35, gpio36, gpio37, gpio38, gpio39, gpio40,
> +                     gpio41, gpio42, gpio43, gpio44, gpio45, gpio46,
> +                     pcie_reset0, pcie_reset1, pcie_reset2]

minItems

> +            maxItems: 58
> +
> +          bias-disable: true
> +
> +          bias-pull-up: true
> +
> +          bias-pull-down: true
> +
> +          input-enable: true
> +
> +          output-enable: true
> +
> +          output-low: true
> +
> +          output-high: true
> +
> +          drive-open-drain: true

Drop blank lines between these.
> +
> +          drive-strength:

What are the units? Shouldn't this be drive-strength-microamp?

> +            enum: [2, 4, 6, 8]
> +
> +        required:
> +          - pins
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      pio: pinctrl@1fa20214 {
> +        compatible = "airoha,en7581-pinctrl";
> +        reg = <0x0 0x1fa20214 0x0 0x30>,
> +              <0x0 0x1fa2027c 0x0 0x8>,
> +              <0x0 0x1fbf0234 0x0 0x4>,
> +              <0x0 0x1fbf0268 0x0 0x4>,
> +              <0x0 0x1fa2001c 0x0 0x50>,
> +              <0x0 0x1fa2018c 0x0 0x4>,
> +              <0x0 0x1fbf0204 0x0 0x4>,
> +              <0x0 0x1fbf0270 0x0 0x4>,
> +              <0x0 0x1fbf0200 0x0 0x4>,
> +              <0x0 0x1fbf0220 0x0 0x4>,
> +              <0x0 0x1fbf0260 0x0 0x4>,
> +              <0x0 0x1fbf0264 0x0 0x4>,
> +              <0x0 0x1fbf0214 0x0 0x4>,
> +              <0x0 0x1fbf0278 0x0 0x4>,
> +              <0x0 0x1fbf0208 0x0 0x4>,
> +              <0x0 0x1fbf027c 0x0 0x4>,
> +              <0x0 0x1fbf0210 0x0 0x4>,
> +              <0x0 0x1fbf028c 0x0 0x4>,
> +              <0x0 0x1fbf0290 0x0 0x4>,
> +              <0x0 0x1fbf0294 0x0 0x4>,
> +              <0x0 0x1fbf020c 0x0 0x4>,
> +              <0x0 0x1fbf0280 0x0 0x4>,
> +              <0x0 0x1fbf0284 0x0 0x4>,
> +              <0x0 0x1fbf0288 0x0 0x4>;

Why are you mapping individual registers? At least half of these are
continuous.

> +
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&pio 0 13 47>;
> +
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupt-parent = <&gic>;
> +        interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        pcie1-rst-pins {
> +          conf {
> +            pins = "pcie_reset1";
> +            drive-open-drain = <1>;
> +          };
> +        };
> +
> +        pwm-pins {
> +          mux {
> +            function = "pwm";
> +            groups = "gpio18";
> +          };
> +        };
> +
> +        spi-pins {
> +          mux {
> +            function = "spi";
> +            groups = "spi_quad", "spi_cs1";
> +          };
> +        };
> +
> +        uuart2-pins {
> +          mux {
> +            function = "uart";
> +            groups = "uart2", "uart2_cts_rts";
> +          };
> +        };
> +
> +        uar5-pins {
> +          mux {
> +            function = "uart";
> +            groups = "uart5";
> +          };
> +        };
> +
> +        mmc-pins {
> +          mux {
> +            function = "emmc";
> +            groups = "emmc";
> +          };
> +        };
> +
> +        mdio-pins {
> +          mux {
> +            function = "mdio";
> +            groups = "mdio";
> +          };
> +
> +          conf {
> +            pins = "gpio2";

What is the point of having both groups and pins?

> +            output-enable;
> +          };
> +        };
> +
> +        gswp1-led0-pins {
> +          mux {
> +            function = "phy1_led0";
> +            groups = "gpio33";
> +          };
> +        };
> +
> +        gswp2-led1-pins {
> +          mux {
> +            function = "phy2_led1";
> +            groups = "gpio44";

That's not a group but pin name.

Best regards,
Krzysztof

Benjamin Larsson Aug. 13, 2024, 8:06 a.m. UTC | #2

On 2024-08-12 08:48, Krzysztof Kozlowski wrote:
>> +      pio: pinctrl@1fa20214 {
>> +        compatible = "airoha,en7581-pinctrl";
>> +        reg = <0x0 0x1fa20214 0x0 0x30>,
>> +              <0x0 0x1fa2027c 0x0 0x8>,
>> +              <0x0 0x1fbf0234 0x0 0x4>,
>> +              <0x0 0x1fbf0268 0x0 0x4>,
>> +              <0x0 0x1fa2001c 0x0 0x50>,
>> +              <0x0 0x1fa2018c 0x0 0x4>,
>> +              <0x0 0x1fbf0204 0x0 0x4>,
>> +              <0x0 0x1fbf0270 0x0 0x4>,
>> +              <0x0 0x1fbf0200 0x0 0x4>,
>> +              <0x0 0x1fbf0220 0x0 0x4>,
>> +              <0x0 0x1fbf0260 0x0 0x4>,
>> +              <0x0 0x1fbf0264 0x0 0x4>,
>> +              <0x0 0x1fbf0214 0x0 0x4>,
>> +              <0x0 0x1fbf0278 0x0 0x4>,
>> +              <0x0 0x1fbf0208 0x0 0x4>,
>> +              <0x0 0x1fbf027c 0x0 0x4>,
>> +              <0x0 0x1fbf0210 0x0 0x4>,
>> +              <0x0 0x1fbf028c 0x0 0x4>,
>> +              <0x0 0x1fbf0290 0x0 0x4>,
>> +              <0x0 0x1fbf0294 0x0 0x4>,
>> +              <0x0 0x1fbf020c 0x0 0x4>,
>> +              <0x0 0x1fbf0280 0x0 0x4>,
>> +              <0x0 0x1fbf0284 0x0 0x4>,
>> +              <0x0 0x1fbf0288 0x0 0x4>;
> Why are you mapping individual registers? At least half of these are
> continuous.

Hi, this is by design because of the register placement in the gpio 
block and the fact that the pwm functionality is intermixed in there 
also. As example the following registers are all GPIOCTRL:

<0x0 0x1fbf0200 0x0 0x4>,
<0x0 0x1fbf0220 0x0 0x4>,
<0x0 0x1fbf0260 0x0 0x4>,
<0x0 0x1fbf0264 0x0 0x4>,

To simplify the driver code logic the complexity is moved to the dts 
because of that.

MvH

Benjamin Larsson

Rob Herring (Arm) Aug. 16, 2024, 10:52 p.m. UTC | #3

On Tue, Aug 13, 2024 at 10:06:41AM +0200, Benjamin Larsson wrote:
> On 2024-08-12 08:48, Krzysztof Kozlowski wrote:
> > > +      pio: pinctrl@1fa20214 {
> > > +        compatible = "airoha,en7581-pinctrl";
> > > +        reg = <0x0 0x1fa20214 0x0 0x30>,
> > > +              <0x0 0x1fa2027c 0x0 0x8>,
> > > +              <0x0 0x1fbf0234 0x0 0x4>,
> > > +              <0x0 0x1fbf0268 0x0 0x4>,
> > > +              <0x0 0x1fa2001c 0x0 0x50>,
> > > +              <0x0 0x1fa2018c 0x0 0x4>,
> > > +              <0x0 0x1fbf0204 0x0 0x4>,
> > > +              <0x0 0x1fbf0270 0x0 0x4>,
> > > +              <0x0 0x1fbf0200 0x0 0x4>,
> > > +              <0x0 0x1fbf0220 0x0 0x4>,
> > > +              <0x0 0x1fbf0260 0x0 0x4>,
> > > +              <0x0 0x1fbf0264 0x0 0x4>,
> > > +              <0x0 0x1fbf0214 0x0 0x4>,
> > > +              <0x0 0x1fbf0278 0x0 0x4>,
> > > +              <0x0 0x1fbf0208 0x0 0x4>,
> > > +              <0x0 0x1fbf027c 0x0 0x4>,
> > > +              <0x0 0x1fbf0210 0x0 0x4>,
> > > +              <0x0 0x1fbf028c 0x0 0x4>,
> > > +              <0x0 0x1fbf0290 0x0 0x4>,
> > > +              <0x0 0x1fbf0294 0x0 0x4>,
> > > +              <0x0 0x1fbf020c 0x0 0x4>,
> > > +              <0x0 0x1fbf0280 0x0 0x4>,
> > > +              <0x0 0x1fbf0284 0x0 0x4>,
> > > +              <0x0 0x1fbf0288 0x0 0x4>;
> > Why are you mapping individual registers? At least half of these are
> > continuous.
> 
> Hi, this is by design because of the register placement in the gpio block
> and the fact that the pwm functionality is intermixed in there also. As
> example the following registers are all GPIOCTRL:
> 
> <0x0 0x1fbf0200 0x0 0x4>,
> <0x0 0x1fbf0220 0x0 0x4>,
> <0x0 0x1fbf0260 0x0 0x4>,
> <0x0 0x1fbf0264 0x0 0x4>,
> 
> To simplify the driver code logic the complexity is moved to the dts because
> of that.

DT to OS is an ABI. Don't put the complexity there. The driver is easy 
to change.

Lot's of h/w blocks are just bit soup. This is not special. If a few 
regions is helpful, then that would be fine.

Rob

Benjamin Larsson Aug. 17, 2024, 8:46 p.m. UTC | #4

On 17/08/2024 00:52, Rob Herring wrote:
>> Hi, this is by design because of the register placement in the gpio block
>> and the fact that the pwm functionality is intermixed in there also. As
>> example the following registers are all GPIOCTRL:
>>
>> <0x0 0x1fbf0200 0x0 0x4>,
>> <0x0 0x1fbf0220 0x0 0x4>,
>> <0x0 0x1fbf0260 0x0 0x4>,
>> <0x0 0x1fbf0264 0x0 0x4>,
>>
>> To simplify the driver code logic the complexity is moved to the dts because
>> of that.
> DT to OS is an ABI. Don't put the complexity there. The driver is easy
> to change.
>
> Lot's of h/w blocks are just bit soup. This is not special. If a few
> regions is helpful, then that would be fine.
>
> Rob

Hi, the pwm functionality is to blame.

The following is the logic that populates the direction registers 
(GPIOCTRL).

     for (i = 0; i < ARRAY_SIZE(pinctrl->gpiochip.dir); i++) {
         ptr = devm_platform_ioremap_resource(pdev, index++);
         if (IS_ERR(ptr))
             return dev_err_probe(dev, PTR_ERR(ptr),
                          "failed to map gpio dir regs\n");

         pinctrl->gpiochip.dir[i] = ptr;
     }


As example in between 0x1fbf0200, 0x1fbf0220 and 0x1fbf0260 we have pwm 
related registers.

The gpio block could if I count it correctly be split into 8+ regions. 
The dts list contain 18 rows related to the gpio block. So the savings 
would be ca 10 rows but a register mapping list in the driver would be 
needed instead.

Is that savings worth the addition of a register lookup table ?

MvH

Benjamin Larsson

Andrew Lunn Aug. 17, 2024, 9:39 p.m. UTC | #5

On Sat, Aug 17, 2024 at 10:46:10PM +0200, Benjamin Larsson wrote:
> On 17/08/2024 00:52, Rob Herring wrote:
> > > Hi, this is by design because of the register placement in the gpio block
> > > and the fact that the pwm functionality is intermixed in there also. As
> > > example the following registers are all GPIOCTRL:
> > > 
> > > <0x0 0x1fbf0200 0x0 0x4>,
> > > <0x0 0x1fbf0220 0x0 0x4>,
> > > <0x0 0x1fbf0260 0x0 0x4>,
> > > <0x0 0x1fbf0264 0x0 0x4>,
> > > 
> > > To simplify the driver code logic the complexity is moved to the dts because
> > > of that.
> > DT to OS is an ABI. Don't put the complexity there. The driver is easy
> > to change.
> > 
> > Lot's of h/w blocks are just bit soup. This is not special. If a few
> > regions is helpful, then that would be fine.
> > 
> > Rob
> 
> Hi, the pwm functionality is to blame.
> 
> The following is the logic that populates the direction registers
> (GPIOCTRL).
> 
>     for (i = 0; i < ARRAY_SIZE(pinctrl->gpiochip.dir); i++) {
>         ptr = devm_platform_ioremap_resource(pdev, index++);
>         if (IS_ERR(ptr))
>             return dev_err_probe(dev, PTR_ERR(ptr),
>                          "failed to map gpio dir regs\n");
> 
>         pinctrl->gpiochip.dir[i] = ptr;
>     }
> 
> 
> As example in between 0x1fbf0200, 0x1fbf0220 and 0x1fbf0260 we have pwm
> related registers.
> 
> The gpio block could if I count it correctly be split into 8+ regions. The
> dts list contain 18 rows related to the gpio block. So the savings would be
> ca 10 rows but a register mapping list in the driver would be needed
> instead.

How messy are the GPIO and PWM registers? Are there N blocks of
independent GPIO registers? and M blocks of independent PWM registers?
By that, does one block of GPIO registers contain all you need for one
GPIO controller? One block of PWM registers give you all you need for
one PWM controller? Or are the registers for one GPIO controller
scattered all over the place?

Could you point at a public datasheet?

      Andrew

Benjamin Larsson Aug. 18, 2024, 12:48 p.m. UTC | #6

On 17/08/2024 23:39, Andrew Lunn wrote:
> How messy are the GPIO and PWM registers? Are there N blocks of
> independent GPIO registers? and M blocks of independent PWM registers?
> By that, does one block of GPIO registers contain all you need for one
> GPIO controller? One block of PWM registers give you all you need for
> one PWM controller? Or are the registers for one GPIO controller
> scattered all over the place?
>
> Could you point at a public datasheet?
>
>        Andrew
>
Hi, per my understanding there is no public datasheet/register reference 
manual.

But here is the division of regions of the registers in the gpio block 
and how it is currently divided between the drivers (according to my 
current understanding).

1FBF0200, gpio/pinctrl
1FBF0204, gpio/pinctrl
1FBF0208, gpio/pinctrl
1FBF020C, gpio/pinctrl
1FBF0210, gpio/pinctrl
1FBF0214, gpio/pinctrl
1FBF0218, unclaimed
1FBF021C, pwm
1FBF0220, gpio/pinctrl
1FBF0224, pwm
1FBF0228, pwm
1FBF022C, pwm
1FBF0230, pwm
1FBF0234, pwm
1FBF0238, unclaimed
1FBF023C, pwm
1FBF0240, pwm
1FBF0244, pwm
1FBF0248, pwm
1FBF024C, pwm
1FBF0250, pwm
1FBF0254, pwm
1FBF0258, pwm
1FBF025C, pwm
1FBF0260, gpio/pinctrl
1FBF0264, gpio/pinctrl
1FBF0268, gpio/pinctrl
1FBF0270, gpio/pinctrl
1FBF0278, gpio/pinctrl
1FBF027C, gpio/pinctrl
1FBF0280, gpio/pinctrl
1FBF0284, gpio/pinctrl
1FBF0288, gpio/pinctrl
1FBF028C, gpio/pinctrl
1FBF0290, gpio/pinctrl
1FBF0294, gpio/pinctrl
1FBF0298, pwm
1FBF029C, pwm
1FBF02A0, unclaimed
1FBF02A4, unclaimed
1FBF02A8, unclaimed
1FBF02AC, unclaimed
1FBF02B0, unclaimed
1FBF02B4, unclaimed
1FBF02B8, unclaimed
1FBF02BC, pwm (but currently unclaimed)

The gpio functions are split in 2x32bit register banks. The pin-io and 
interrupt support for these are split amongst the regions.

MvH

Benjamin Larsson

Lorenzo Bianconi Aug. 18, 2024, 2:20 p.m. UTC | #7

[...]
> > +allOf:
> > +  - $ref: pinctrl.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - gpio-controller
> > +  - "#gpio-cells"
> > +
> > +patternProperties:
> 
> Keep it after properties: block.

ack, I will fix it in v2

> 
> > +  '-pins$':
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    patternProperties:
> > +      '^.*mux.*$':
> > +        type: object
> > +        additionalProperties: false
> > +        description: |
> 
> Do not need '|' unless you need to preserve formatting.

ack, I will fix it in v2

> 
> > +          pinmux configuration nodes.
> > +
> > +        $ref: /schemas/pinctrl/pinmux-node.yaml
> > +        properties:
> > +          function:
> > +            description:
> > +              A string containing the name of the function to mux to the group.
> > +            enum: [pon, tod_1pps, sipo, mdio, uart, i2c, jtag, pcm, spi,
> > +                   pcm_spi, i2s, emmc, pnand, pcie_reset, pwm, phy1_led0,
> > +                   phy2_led0, phy3_led0, phy4_led0, phy1_led1, phy2_led1,
> > +                   phy3_led1, phy4_led1]
> > +          groups:
> 
> minItems: 1
> maxItems: 32 (or whatever is sensible)

I was assuming the default values are minIems = maxItems = 1 and we can
overwrite maxItems if required (e.g. for "uart" or for "pcm_spi" blocks).
Am I missing something?

> 
> > +            description:
> > +              An array of strings. Each string contains the name of a group.
> > +        required:
> > +          - function
> > +          - groups
> > +
> > +        allOf:
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: pon
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [pon]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: tod_1pps
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [pon_tod_1pps, gsw_tod_1pps]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: sipo
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [sipo, sipo_rclk]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: mdio
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [mdio]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: uart
> > +            then:
> > +              properties:
> > +                groups:
> > +                  items:
> > +                    enum: [uart2, uart2_cts_rts, hsuart, hsuart_cts_rts, uart4,
> > +                           uart5]
> > +                  maxItems: 2
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: i2c
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [i2c1]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: jtag
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [jtag_udi, jtag_dfd]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: pcm
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [pcm1, pcm2]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: spi
> > +            then:
> > +              properties:
> > +                groups:
> > +                  items:
> > +                    enum: [spi_quad, spi_cs1]
> > +                  maxItems: 2
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: pcm_spi
> > +            then:
> > +              properties:
> > +                groups:
> > +                  items:
> > +                    enum: [pcm_spi, pcm_spi_int, pcm_spi_rst, pcm_spi_cs1,
> > +                           pcm_spi_cs2_p156, pcm_spi_cs2_p128, pcm_spi_cs3,
> > +                           pcm_spi_cs4]
> > +                  maxItems: 7
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: i2c
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [i2s]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: emmc
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [emmc]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: pnand
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [pnand]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: pcie_reset
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [pcie_reset0, pcie_reset1, pcie_reset2]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: pwm
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio0, 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, gpio28, gpio29, gpio30, gpio31,
> > +                         gpio36, gpio37, gpio38, gpio39, gpio40, gpio41,
> > +                         gpio42, gpio43, gpio44, gpio45, gpio46, gpio47]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy1_led0
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio33, gpio34, gpio35, gpio42]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy2_led0
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio33, gpio34, gpio35, gpio42]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy3_led0
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio33, gpio34, gpio35, gpio42]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy4_led0
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio33, gpio34, gpio35, gpio42]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy1_led1
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio43, gpio44, gpio45, gpio46]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy2_led1
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio43, gpio44, gpio45, gpio46]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy3_led1
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio43, gpio44, gpio45, gpio46]
> > +          - if:
> > +              properties:
> > +                function:
> > +                  const: phy4_led1
> > +            then:
> > +              properties:
> > +                groups:
> > +                  enum: [gpio43, gpio44, gpio45, gpio46]
> > +
> > +      '^.*conf.*$':
> > +        type: object
> > +        additionalProperties: false
> > +        description:
> > +          pinconf configuration nodes.
> > +        $ref: /schemas/pinctrl/pincfg-node.yaml
> > +
> > +        properties:
> > +          pins:
> > +            description:
> > +              An array of strings. Each string contains the name of a pin.
> > +            items:
> > +              enum: [uart1_txd, uart1_rxd, i2c_scl, i2c_sda, spi_cs0, spi_clk,
> > +                     spi_mosi, spi_miso, gpio0, 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, gpio28, gpio29, gpio30, gpio31, gpio32, gpio33,
> > +                     gpio34, gpio35, gpio36, gpio37, gpio38, gpio39, gpio40,
> > +                     gpio41, gpio42, gpio43, gpio44, gpio45, gpio46,
> > +                     pcie_reset0, pcie_reset1, pcie_reset2]
> 
> minItems

ack, I will fix it in v2.

> 
> > +            maxItems: 58
> > +
> > +          bias-disable: true
> > +
> > +          bias-pull-up: true
> > +
> > +          bias-pull-down: true
> > +
> > +          input-enable: true
> > +
> > +          output-enable: true
> > +
> > +          output-low: true
> > +
> > +          output-high: true
> > +
> > +          drive-open-drain: true
> 
> Drop blank lines between these.

ack, I will fix it in v2.

> > +
> > +          drive-strength:
> 
> What are the units? Shouldn't this be drive-strength-microamp?

nope, it is in mA. I will add a description to specify it.

> 
> > +            enum: [2, 4, 6, 8]
> > +
> > +        required:
> > +          - pins
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      pio: pinctrl@1fa20214 {
> > +        compatible = "airoha,en7581-pinctrl";
> > +        reg = <0x0 0x1fa20214 0x0 0x30>,
> > +              <0x0 0x1fa2027c 0x0 0x8>,
> > +              <0x0 0x1fbf0234 0x0 0x4>,
> > +              <0x0 0x1fbf0268 0x0 0x4>,
> > +              <0x0 0x1fa2001c 0x0 0x50>,
> > +              <0x0 0x1fa2018c 0x0 0x4>,
> > +              <0x0 0x1fbf0204 0x0 0x4>,
> > +              <0x0 0x1fbf0270 0x0 0x4>,
> > +              <0x0 0x1fbf0200 0x0 0x4>,
> > +              <0x0 0x1fbf0220 0x0 0x4>,
> > +              <0x0 0x1fbf0260 0x0 0x4>,
> > +              <0x0 0x1fbf0264 0x0 0x4>,
> > +              <0x0 0x1fbf0214 0x0 0x4>,
> > +              <0x0 0x1fbf0278 0x0 0x4>,
> > +              <0x0 0x1fbf0208 0x0 0x4>,
> > +              <0x0 0x1fbf027c 0x0 0x4>,
> > +              <0x0 0x1fbf0210 0x0 0x4>,
> > +              <0x0 0x1fbf028c 0x0 0x4>,
> > +              <0x0 0x1fbf0290 0x0 0x4>,
> > +              <0x0 0x1fbf0294 0x0 0x4>,
> > +              <0x0 0x1fbf020c 0x0 0x4>,
> > +              <0x0 0x1fbf0280 0x0 0x4>,
> > +              <0x0 0x1fbf0284 0x0 0x4>,
> > +              <0x0 0x1fbf0288 0x0 0x4>;
> 
> Why are you mapping individual registers? At least half of these are
> continuous.
> 
> > +
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        gpio-ranges = <&pio 0 13 47>;
> > +
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +        interrupt-parent = <&gic>;
> > +        interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +        pcie1-rst-pins {
> > +          conf {
> > +            pins = "pcie_reset1";
> > +            drive-open-drain = <1>;
> > +          };
> > +        };
> > +
> > +        pwm-pins {
> > +          mux {
> > +            function = "pwm";
> > +            groups = "gpio18";
> > +          };
> > +        };
> > +
> > +        spi-pins {
> > +          mux {
> > +            function = "spi";
> > +            groups = "spi_quad", "spi_cs1";
> > +          };
> > +        };
> > +
> > +        uuart2-pins {
> > +          mux {
> > +            function = "uart";
> > +            groups = "uart2", "uart2_cts_rts";
> > +          };
> > +        };
> > +
> > +        uar5-pins {
> > +          mux {
> > +            function = "uart";
> > +            groups = "uart5";
> > +          };
> > +        };
> > +
> > +        mmc-pins {
> > +          mux {
> > +            function = "emmc";
> > +            groups = "emmc";
> > +          };
> > +        };
> > +
> > +        mdio-pins {
> > +          mux {
> > +            function = "mdio";
> > +            groups = "mdio";
> > +          };
> > +
> > +          conf {
> > +            pins = "gpio2";
> 
> What is the point of having both groups and pins?

"pins" is specific for "conf" block and it is used to specify the pins where
we need to apply the configuration while "group" is just used in the "mux"
block. Am I missing something?

> 
> > +            output-enable;
> > +          };
> > +        };
> > +
> > +        gswp1-led0-pins {
> > +          mux {
> > +            function = "phy1_led0";
> > +            groups = "gpio33";
> > +          };
> > +        };
> > +
> > +        gswp2-led1-pins {
> > +          mux {
> > +            function = "phy2_led1";
> > +            groups = "gpio44";
> 
> That's not a group but pin name.

Looking at the patch 2/2, "gpio44" is actually a group used to specify a single
pin group.

Regards,
Lorenzo

> 
> Best regards,
> Krzysztof
>

Lorenzo Bianconi Aug. 18, 2024, 2:22 p.m. UTC | #8

> On Tue, Aug 13, 2024 at 10:06:41AM +0200, Benjamin Larsson wrote:
> > On 2024-08-12 08:48, Krzysztof Kozlowski wrote:
> > > > +      pio: pinctrl@1fa20214 {
> > > > +        compatible = "airoha,en7581-pinctrl";
> > > > +        reg = <0x0 0x1fa20214 0x0 0x30>,
> > > > +              <0x0 0x1fa2027c 0x0 0x8>,
> > > > +              <0x0 0x1fbf0234 0x0 0x4>,
> > > > +              <0x0 0x1fbf0268 0x0 0x4>,
> > > > +              <0x0 0x1fa2001c 0x0 0x50>,
> > > > +              <0x0 0x1fa2018c 0x0 0x4>,
> > > > +              <0x0 0x1fbf0204 0x0 0x4>,
> > > > +              <0x0 0x1fbf0270 0x0 0x4>,
> > > > +              <0x0 0x1fbf0200 0x0 0x4>,
> > > > +              <0x0 0x1fbf0220 0x0 0x4>,
> > > > +              <0x0 0x1fbf0260 0x0 0x4>,
> > > > +              <0x0 0x1fbf0264 0x0 0x4>,
> > > > +              <0x0 0x1fbf0214 0x0 0x4>,
> > > > +              <0x0 0x1fbf0278 0x0 0x4>,
> > > > +              <0x0 0x1fbf0208 0x0 0x4>,
> > > > +              <0x0 0x1fbf027c 0x0 0x4>,
> > > > +              <0x0 0x1fbf0210 0x0 0x4>,
> > > > +              <0x0 0x1fbf028c 0x0 0x4>,
> > > > +              <0x0 0x1fbf0290 0x0 0x4>,
> > > > +              <0x0 0x1fbf0294 0x0 0x4>,
> > > > +              <0x0 0x1fbf020c 0x0 0x4>,
> > > > +              <0x0 0x1fbf0280 0x0 0x4>,
> > > > +              <0x0 0x1fbf0284 0x0 0x4>,
> > > > +              <0x0 0x1fbf0288 0x0 0x4>;
> > > Why are you mapping individual registers? At least half of these are
> > > continuous.
> > 
> > Hi, this is by design because of the register placement in the gpio block
> > and the fact that the pwm functionality is intermixed in there also. As
> > example the following registers are all GPIOCTRL:
> > 
> > <0x0 0x1fbf0200 0x0 0x4>,
> > <0x0 0x1fbf0220 0x0 0x4>,
> > <0x0 0x1fbf0260 0x0 0x4>,
> > <0x0 0x1fbf0264 0x0 0x4>,
> > 
> > To simplify the driver code logic the complexity is moved to the dts because
> > of that.
> 
> DT to OS is an ABI. Don't put the complexity there. The driver is easy 
> to change.
> 
> Lot's of h/w blocks are just bit soup. This is not special. If a few 
> regions is helpful, then that would be fine.

ack, I guess we can try to move the complexity in the driver, at least for
gpio-irq controllers, merging regs whenever possible. I will work on it.

Regards,
Lorenzo

> 
> Rob

Christian Marangi Aug. 18, 2024, 3:42 p.m. UTC | #9

On Sun, Aug 18, 2024 at 06:02:28PM +0200, Andrew Lunn wrote:
> On Sun, Aug 18, 2024 at 02:48:05PM +0200, Benjamin Larsson wrote:
> > On 17/08/2024 23:39, Andrew Lunn wrote:
> > > How messy are the GPIO and PWM registers? Are there N blocks of
> > > independent GPIO registers? and M blocks of independent PWM registers?
> > > By that, does one block of GPIO registers contain all you need for one
> > > GPIO controller? One block of PWM registers give you all you need for
> > > one PWM controller? Or are the registers for one GPIO controller
> > > scattered all over the place?
> > > 
> > > Could you point at a public datasheet?
> > > 
> > >        Andrew
> > > 
> > Hi, per my understanding there is no public datasheet/register reference
> > manual.
> > 
> > But here is the division of regions of the registers in the gpio block and
> > how it is currently divided between the drivers (according to my current
> > understanding).
> > 
> > 1FBF0200, gpio/pinctrl
> > 1FBF0204, gpio/pinctrl
> > 1FBF0208, gpio/pinctrl
> > 1FBF020C, gpio/pinctrl
> > 1FBF0210, gpio/pinctrl
> > 1FBF0214, gpio/pinctrl
> 
> A typical SoC has multiple instances of a GPIO controller. Each GPIO
> controller typically has 4 or 5 registers: In, Out, Direction,
> Interrupt Enable, Interrupt Status. If these 4 or 5 registers are
> contiguous, you could have one DT node per controller, rather than one
> node for all GPIO controllers.
> 
> If the hardware designer has really messed up and fully interleaved
> GPIO and PWM, it might be better to have an MFD. The MFD node has a
> single reg covering the entire range. The MFD would then map the whole
> range, and provide accessors to the child devices. Hard code the
> knowledge of what registers are where. Given how badly the hardware is
> designed, it is unlikely it will get reused in the future, so there is
> no point putting lots of stuff into DT. Hard code it.
>

Problem is that the MFD will also affect other stuff like watchdog...
thermal sensor/monitor, clocks... They really messed and put in that
range all kind of stuff so we would end up in a very big mapped range
and lots of child for the MFD.

Andrew Lunn Aug. 18, 2024, 4:02 p.m. UTC | #10

On Sun, Aug 18, 2024 at 02:48:05PM +0200, Benjamin Larsson wrote:
> On 17/08/2024 23:39, Andrew Lunn wrote:
> > How messy are the GPIO and PWM registers? Are there N blocks of
> > independent GPIO registers? and M blocks of independent PWM registers?
> > By that, does one block of GPIO registers contain all you need for one
> > GPIO controller? One block of PWM registers give you all you need for
> > one PWM controller? Or are the registers for one GPIO controller
> > scattered all over the place?
> > 
> > Could you point at a public datasheet?
> > 
> >        Andrew
> > 
> Hi, per my understanding there is no public datasheet/register reference
> manual.
> 
> But here is the division of regions of the registers in the gpio block and
> how it is currently divided between the drivers (according to my current
> understanding).
> 
> 1FBF0200, gpio/pinctrl
> 1FBF0204, gpio/pinctrl
> 1FBF0208, gpio/pinctrl
> 1FBF020C, gpio/pinctrl
> 1FBF0210, gpio/pinctrl
> 1FBF0214, gpio/pinctrl

A typical SoC has multiple instances of a GPIO controller. Each GPIO
controller typically has 4 or 5 registers: In, Out, Direction,
Interrupt Enable, Interrupt Status. If these 4 or 5 registers are
contiguous, you could have one DT node per controller, rather than one
node for all GPIO controllers.

If the hardware designer has really messed up and fully interleaved
GPIO and PWM, it might be better to have an MFD. The MFD node has a
single reg covering the entire range. The MFD would then map the whole
range, and provide accessors to the child devices. Hard code the
knowledge of what registers are where. Given how badly the hardware is
designed, it is unlikely it will get reused in the future, so there is
no point putting lots of stuff into DT. Hard code it.

	Andrew

Lorenzo Bianconi Aug. 18, 2024, 4:15 p.m. UTC | #11

On Aug 18, Andrew Lunn wrote:
> On Sun, Aug 18, 2024 at 02:48:05PM +0200, Benjamin Larsson wrote:
> > On 17/08/2024 23:39, Andrew Lunn wrote:
> > > How messy are the GPIO and PWM registers? Are there N blocks of
> > > independent GPIO registers? and M blocks of independent PWM registers?
> > > By that, does one block of GPIO registers contain all you need for one
> > > GPIO controller? One block of PWM registers give you all you need for
> > > one PWM controller? Or are the registers for one GPIO controller
> > > scattered all over the place?
> > > 
> > > Could you point at a public datasheet?
> > > 
> > >        Andrew
> > > 
> > Hi, per my understanding there is no public datasheet/register reference
> > manual.
> > 
> > But here is the division of regions of the registers in the gpio block and
> > how it is currently divided between the drivers (according to my current
> > understanding).
> > 
> > 1FBF0200, gpio/pinctrl
> > 1FBF0204, gpio/pinctrl
> > 1FBF0208, gpio/pinctrl
> > 1FBF020C, gpio/pinctrl
> > 1FBF0210, gpio/pinctrl
> > 1FBF0214, gpio/pinctrl
> 
> A typical SoC has multiple instances of a GPIO controller. Each GPIO
> controller typically has 4 or 5 registers: In, Out, Direction,
> Interrupt Enable, Interrupt Status. If these 4 or 5 registers are
> contiguous, you could have one DT node per controller, rather than one
> node for all GPIO controllers.

it is the same for en7581 pinctrl too. I think we can squash most of the
gpio/irq registers into "bigger" io-regions (just keeping a couple of holes
for pwm and leds). It is just a matter of moving the logic from the dts to
the driver. I am currently working on it. I will post v2 soon.

> 
> If the hardware designer has really messed up and fully interleaved
> GPIO and PWM, it might be better to have an MFD. The MFD node has a
> single reg covering the entire range. The MFD would then map the whole
> range, and provide accessors to the child devices. Hard code the
> knowledge of what registers are where. Given how badly the hardware is
> designed, it is unlikely it will get reused in the future, so there is
> no point putting lots of stuff into DT. Hard code it.

I am not sure it is possible/feasible to implement a MFD device here since
the mapped region is huge and sparse.

Regards,
Lorenzo

> 
> 	Andrew

Benjamin Larsson Aug. 18, 2024, 4:59 p.m. UTC | #12

On 18/08/2024 18:02, Andrew Lunn wrote:
>>
>> Hi, per my understanding there is no public datasheet/register reference
>> manual.
>>
>> But here is the division of regions of the registers in the gpio block and
>> how it is currently divided between the drivers (according to my current
>> understanding).
>>
>> 1FBF0200, gpio/pinctrl
>> 1FBF0204, gpio/pinctrl
>> 1FBF0208, gpio/pinctrl
>> 1FBF020C, gpio/pinctrl
>> 1FBF0210, gpio/pinctrl
>> 1FBF0214, gpio/pinctrl
> A typical SoC has multiple instances of a GPIO controller. Each GPIO
> controller typically has 4 or 5 registers: In, Out, Direction,
> Interrupt Enable, Interrupt Status. If these 4 or 5 registers are
> contiguous, you could have one DT node per controller, rather than one
> node for all GPIO controllers.
>
> If the hardware designer has really messed up and fully interleaved
> GPIO and PWM, it might be better to have an MFD. The MFD node has a
> single reg covering the entire range. The MFD would then map the whole
> range, and provide accessors to the child devices. Hard code the
> knowledge of what registers are where. Given how badly the hardware is
> designed, it is unlikely it will get reused in the future, so there is
> no point putting lots of stuff into DT. Hard code it.
>
> 	Andrew

Hi, the pwm driver could be re-used on the EN7523 SoC. Future Airoha 
SoCs will most likely have the same ip-block also.

MvH

Benjamin Larsson

Andrew Lunn Aug. 18, 2024, 5:35 p.m. UTC | #13

> Hi, the pwm driver could be re-used on the EN7523 SoC. Future Airoha SoCs
> will most likely have the same ip-block also.

A lot will depend on if they sort out the mess they made with the
registers. Given how simple these drivers are, it is sometimes better
to just write a new driver once the registers are in sane blocks,
rather than all interleaved.

       Andrew

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
new file mode 100644
index 000000000000..b1f980613864
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
@@ -0,0 +1,467 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/airoha,en7581-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Pin Controller
+
+maintainers:
+  - Lorenzo Bianconi <[e-mail]>
+
+description:
+  The Airoha's EN7581 Pin controller is used to control SoC pins.
+
+properties:
+  compatible:
+    enum:
+      - airoha,en7581-pinctrl
+
+  reg:
+    items:
+      - description: IOMUX base address
+      - description: LED IOMUX base address
+      - description: GPIO flash mode base address
+      - description: GPIO flash mode extended base address
+      - description: IO pin configuration base address
+      - description: PCIE reset open-drain base address
+      - description: GPIO bank0 data register base address
+      - description: GPIO bank1 data register base address
+      - description: GPIO bank0 first control register base address
+      - description: GPIO bank0 second control register base address
+      - description: GPIO bank1 first control register base address
+      - description: GPIO bank1 second control register base address
+      - description: GPIO bank0 output enable register base address
+      - description: GPIO bank1 output enable register base address
+      - description: GPIO bank0 irq status register base address
+      - description: GPIO bank1 irq status register base address
+      - description: GPIO bank0 irq level first control register base address
+      - description: GPIO bank0 irq level second control register base address
+      - description: GPIO bank1 irq level first control register base address
+      - description: GPIO bank1 irq level second control register base address
+      - description: GPIO bank0 irq edge first control register base address
+      - description: GPIO bank0 irq edge second control register base address
+      - description: GPIO bank1 irq edge first control register base address
+      - description: GPIO bank1 irq edge second control register base address
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+    description:
+      Number of cells in GPIO specifier. Since the generic GPIO binding is
+      used, the amount of cells must be specified as 2. See the below mentioned
+      gpio binding representation for description of particular cells.
+
+  gpio-ranges:
+    maxItems: 1
+    description:
+      GPIO valid number range.
+
+  interrupt-controller: true
+
+  interrupts:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 2
+
+allOf:
+  - $ref: pinctrl.yaml#
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      '^.*mux.*$':
+        type: object
+        additionalProperties: false
+        description: |
+          pinmux configuration nodes.
+
+        $ref: /schemas/pinctrl/pinmux-node.yaml
+        properties:
+          function:
+            description:
+              A string containing the name of the function to mux to the group.
+            enum: [pon, tod_1pps, sipo, mdio, uart, i2c, jtag, pcm, spi,
+                   pcm_spi, i2s, emmc, pnand, pcie_reset, pwm, phy1_led0,
+                   phy2_led0, phy3_led0, phy4_led0, phy1_led1, phy2_led1,
+                   phy3_led1, phy4_led1]
+          groups:
+            description:
+              An array of strings. Each string contains the name of a group.
+        required:
+          - function
+          - groups
+
+        allOf:
+          - if:
+              properties:
+                function:
+                  const: pon
+            then:
+              properties:
+                groups:
+                  enum: [pon]
+          - if:
+              properties:
+                function:
+                  const: tod_1pps
+            then:
+              properties:
+                groups:
+                  enum: [pon_tod_1pps, gsw_tod_1pps]
+          - if:
+              properties:
+                function:
+                  const: sipo
+            then:
+              properties:
+                groups:
+                  enum: [sipo, sipo_rclk]
+          - if:
+              properties:
+                function:
+                  const: mdio
+            then:
+              properties:
+                groups:
+                  enum: [mdio]
+          - if:
+              properties:
+                function:
+                  const: uart
+            then:
+              properties:
+                groups:
+                  items:
+                    enum: [uart2, uart2_cts_rts, hsuart, hsuart_cts_rts, uart4,
+                           uart5]
+                  maxItems: 2
+          - if:
+              properties:
+                function:
+                  const: i2c
+            then:
+              properties:
+                groups:
+                  enum: [i2c1]
+          - if:
+              properties:
+                function:
+                  const: jtag
+            then:
+              properties:
+                groups:
+                  enum: [jtag_udi, jtag_dfd]
+          - if:
+              properties:
+                function:
+                  const: pcm
+            then:
+              properties:
+                groups:
+                  enum: [pcm1, pcm2]
+          - if:
+              properties:
+                function:
+                  const: spi
+            then:
+              properties:
+                groups:
+                  items:
+                    enum: [spi_quad, spi_cs1]
+                  maxItems: 2
+          - if:
+              properties:
+                function:
+                  const: pcm_spi
+            then:
+              properties:
+                groups:
+                  items:
+                    enum: [pcm_spi, pcm_spi_int, pcm_spi_rst, pcm_spi_cs1,
+                           pcm_spi_cs2_p156, pcm_spi_cs2_p128, pcm_spi_cs3,
+                           pcm_spi_cs4]
+                  maxItems: 7
+          - if:
+              properties:
+                function:
+                  const: i2c
+            then:
+              properties:
+                groups:
+                  enum: [i2s]
+          - if:
+              properties:
+                function:
+                  const: emmc
+            then:
+              properties:
+                groups:
+                  enum: [emmc]
+          - if:
+              properties:
+                function:
+                  const: pnand
+            then:
+              properties:
+                groups:
+                  enum: [pnand]
+          - if:
+              properties:
+                function:
+                  const: pcie_reset
+            then:
+              properties:
+                groups:
+                  enum: [pcie_reset0, pcie_reset1, pcie_reset2]
+          - if:
+              properties:
+                function:
+                  const: pwm
+            then:
+              properties:
+                groups:
+                  enum: [gpio0, 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, gpio28, gpio29, gpio30, gpio31,
+                         gpio36, gpio37, gpio38, gpio39, gpio40, gpio41,
+                         gpio42, gpio43, gpio44, gpio45, gpio46, gpio47]
+          - if:
+              properties:
+                function:
+                  const: phy1_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy2_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy3_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy4_led0
+            then:
+              properties:
+                groups:
+                  enum: [gpio33, gpio34, gpio35, gpio42]
+          - if:
+              properties:
+                function:
+                  const: phy1_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+          - if:
+              properties:
+                function:
+                  const: phy2_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+          - if:
+              properties:
+                function:
+                  const: phy3_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+          - if:
+              properties:
+                function:
+                  const: phy4_led1
+            then:
+              properties:
+                groups:
+                  enum: [gpio43, gpio44, gpio45, gpio46]
+
+      '^.*conf.*$':
+        type: object
+        additionalProperties: false
+        description:
+          pinconf configuration nodes.
+        $ref: /schemas/pinctrl/pincfg-node.yaml
+
+        properties:
+          pins:
+            description:
+              An array of strings. Each string contains the name of a pin.
+            items:
+              enum: [uart1_txd, uart1_rxd, i2c_scl, i2c_sda, spi_cs0, spi_clk,
+                     spi_mosi, spi_miso, gpio0, 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, gpio28, gpio29, gpio30, gpio31, gpio32, gpio33,
+                     gpio34, gpio35, gpio36, gpio37, gpio38, gpio39, gpio40,
+                     gpio41, gpio42, gpio43, gpio44, gpio45, gpio46,
+                     pcie_reset0, pcie_reset1, pcie_reset2]
+            maxItems: 58
+
+          bias-disable: true
+
+          bias-pull-up: true
+
+          bias-pull-down: true
+
+          input-enable: true
+
+          output-enable: true
+
+          output-low: true
+
+          output-high: true
+
+          drive-open-drain: true
+
+          drive-strength:
+            enum: [2, 4, 6, 8]
+
+        required:
+          - pins
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pio: pinctrl@1fa20214 {
+        compatible = "airoha,en7581-pinctrl";
+        reg = <0x0 0x1fa20214 0x0 0x30>,
+              <0x0 0x1fa2027c 0x0 0x8>,
+              <0x0 0x1fbf0234 0x0 0x4>,
+              <0x0 0x1fbf0268 0x0 0x4>,
+              <0x0 0x1fa2001c 0x0 0x50>,
+              <0x0 0x1fa2018c 0x0 0x4>,
+              <0x0 0x1fbf0204 0x0 0x4>,
+              <0x0 0x1fbf0270 0x0 0x4>,
+              <0x0 0x1fbf0200 0x0 0x4>,
+              <0x0 0x1fbf0220 0x0 0x4>,
+              <0x0 0x1fbf0260 0x0 0x4>,
+              <0x0 0x1fbf0264 0x0 0x4>,
+              <0x0 0x1fbf0214 0x0 0x4>,
+              <0x0 0x1fbf0278 0x0 0x4>,
+              <0x0 0x1fbf0208 0x0 0x4>,
+              <0x0 0x1fbf027c 0x0 0x4>,
+              <0x0 0x1fbf0210 0x0 0x4>,
+              <0x0 0x1fbf028c 0x0 0x4>,
+              <0x0 0x1fbf0290 0x0 0x4>,
+              <0x0 0x1fbf0294 0x0 0x4>,
+              <0x0 0x1fbf020c 0x0 0x4>,
+              <0x0 0x1fbf0280 0x0 0x4>,
+              <0x0 0x1fbf0284 0x0 0x4>,
+              <0x0 0x1fbf0288 0x0 0x4>;
+
+        gpio-controller;
+        #gpio-cells = <2>;
+        gpio-ranges = <&pio 0 13 47>;
+
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&gic>;
+        interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
+
+        pcie1-rst-pins {
+          conf {
+            pins = "pcie_reset1";
+            drive-open-drain = <1>;
+          };
+        };
+
+        pwm-pins {
+          mux {
+            function = "pwm";
+            groups = "gpio18";
+          };
+        };
+
+        spi-pins {
+          mux {
+            function = "spi";
+            groups = "spi_quad", "spi_cs1";
+          };
+        };
+
+        uuart2-pins {
+          mux {
+            function = "uart";
+            groups = "uart2", "uart2_cts_rts";
+          };
+        };
+
+        uar5-pins {
+          mux {
+            function = "uart";
+            groups = "uart5";
+          };
+        };
+
+        mmc-pins {
+          mux {
+            function = "emmc";
+            groups = "emmc";
+          };
+        };
+
+        mdio-pins {
+          mux {
+            function = "mdio";
+            groups = "mdio";
+          };
+
+          conf {
+            pins = "gpio2";
+            output-enable;
+          };
+        };
+
+        gswp1-led0-pins {
+          mux {
+            function = "phy1_led0";
+            groups = "gpio33";
+          };
+        };
+
+        gswp2-led1-pins {
+          mux {
+            function = "phy2_led1";
+            groups = "gpio44";
+          };
+        };
+      };
+    };