diff mbox series

[v6,04/15] dt-bindings: add BCM6328 pincontroller binding documentation

Message ID 20210310125504.31886-5-noltari@gmail.com (mailing list archive)
State New, archived
Headers show
Series pinctrl: add BCM63XX pincontrol support | expand

Commit Message

Álvaro Fernández Rojas March 10, 2021, 12:54 p.m. UTC
Add binding documentation for the pincontrol core found in BCM6328 SoCs.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v6: add changes suggested by Rob Herring
 v5: change Documentation to dt-bindings in commit title
 v4: no changes
 v3: add new gpio node
 v2: remove interrupts

 .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
 1 file changed, 174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml

Comments

Rob Herring March 10, 2021, 5:45 p.m. UTC | #1
On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
>
> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>
> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  v6: add changes suggested by Rob Herring
>  v5: change Documentation to dt-bindings in commit title
>  v4: no changes
>  v3: add new gpio node
>  v2: remove interrupts
>
>  .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> new file mode 100644
> index 000000000000..471f6efa1754
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom BCM6328 pin controller
> +
> +maintainers:
> +  - Álvaro Fernández Rojas <noltari@gmail.com>
> +  - Jonas Gorski <jonas.gorski@gmail.com>
> +
> +description: |+
> +  The pin controller node should be the child of a syscon node.
> +
> +  Refer to the the bindings described in
> +  Documentation/devicetree/bindings/mfd/syscon.yaml
> +
> +properties:
> +  compatible:
> +    const: brcm,bcm6328-pinctrl
> +
> +  gpio:
> +    type: object
> +    properties:
> +      compatible:
> +        const: brcm,bcm6328-gpio
> +
> +      data:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: |
> +          Offset in the register map for the data register (in bytes).
> +
> +      dirout:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: |
> +          Offset in the register map for the dirout register (in bytes).
> +
> +      gpio-controller: true
> +
> +      "#gpio-cells":
> +        const: 2
> +
> +      gpio-ranges:
> +        maxItems: 1
> +
> +    required:
> +      - gpio-controller
> +      - gpio-ranges
> +      - '#gpio-cells'
> +
> +    additionalProperties: false
> +
> +patternProperties:
> +  '^.*-pins$':
> +    if:
> +      type: object
> +    then:
> +      properties:
> +        function:
> +          $ref: "pinmux-node.yaml#/properties/function"
> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
> +
> +        pins:
> +          $ref: "pinmux-node.yaml#/properties/pins"
> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
> +                  usb_port1 ]
> +
> +required:
> +  - compatible
> +  - gpio
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio_cntl@10000080 {
> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";

You just added "brcm,bcm6328-gpio-controller", it would need to be documented.

> +      reg = <0x10000080 0x80>;
> +
> +      pinctrl: pinctrl {
> +        compatible = "brcm,bcm6328-pinctrl";
> +
> +        gpio {
> +          compatible = "brcm,bcm6328-gpio";

I'm still trying to understand why you need 3 levels of nodes here?
The gpio controller contains a pin controller plus other undefined
functions (because of 'syscon') and the pin controller contains a gpio
controller?

I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
should be a single node.

> +          data = <0xc>;
> +          dirout = <0x4>;

This looks similar to the brcm,bcm6345-gpio.txt binding which then
uses the gpio-mmio driver. Defining addresses with 'reg' is much
preferred over custom properties. That binding also captures the bank
size.

Rob
Álvaro Fernández Rojas March 10, 2021, 6:03 p.m. UTC | #2
Hi Rob,

> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
> 
> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>> 
>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>> 
>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>> v6: add changes suggested by Rob Herring
>> v5: change Documentation to dt-bindings in commit title
>> v4: no changes
>> v3: add new gpio node
>> v2: remove interrupts
>> 
>> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
>> 1 file changed, 174 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..471f6efa1754
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>> @@ -0,0 +1,174 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Broadcom BCM6328 pin controller
>> +
>> +maintainers:
>> +  - Álvaro Fernández Rojas <noltari@gmail.com>
>> +  - Jonas Gorski <jonas.gorski@gmail.com>
>> +
>> +description: |+
>> +  The pin controller node should be the child of a syscon node.
>> +
>> +  Refer to the the bindings described in
>> +  Documentation/devicetree/bindings/mfd/syscon.yaml
>> +
>> +properties:
>> +  compatible:
>> +    const: brcm,bcm6328-pinctrl
>> +
>> +  gpio:
>> +    type: object
>> +    properties:
>> +      compatible:
>> +        const: brcm,bcm6328-gpio
>> +
>> +      data:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: |
>> +          Offset in the register map for the data register (in bytes).
>> +
>> +      dirout:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: |
>> +          Offset in the register map for the dirout register (in bytes).
>> +
>> +      gpio-controller: true
>> +
>> +      "#gpio-cells":
>> +        const: 2
>> +
>> +      gpio-ranges:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - gpio-controller
>> +      - gpio-ranges
>> +      - '#gpio-cells'
>> +
>> +    additionalProperties: false
>> +
>> +patternProperties:
>> +  '^.*-pins$':
>> +    if:
>> +      type: object
>> +    then:
>> +      properties:
>> +        function:
>> +          $ref: "pinmux-node.yaml#/properties/function"
>> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
>> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
>> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
>> +
>> +        pins:
>> +          $ref: "pinmux-node.yaml#/properties/pins"
>> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
>> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
>> +                  usb_port1 ]
>> +
>> +required:
>> +  - compatible
>> +  - gpio
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    gpio_cntl@10000080 {
>> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
> 
> You just added "brcm,bcm6328-gpio-controller", it would need to be documented.

I just added that because you requested me to do it ¯\_(ツ)_/¯
What should I do to document it?
I still don’t get most of this .yaml stuff...

> 
>> +      reg = <0x10000080 0x80>;
>> +
>> +      pinctrl: pinctrl {
>> +        compatible = "brcm,bcm6328-pinctrl";
>> +
>> +        gpio {
>> +          compatible = "brcm,bcm6328-gpio";
> 
> I'm still trying to understand why you need 3 levels of nodes here?
> The gpio controller contains a pin controller plus other undefined
> functions (because of 'syscon') and the pin controller contains a gpio
> controller?

In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…

> 
> I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
> should be a single node.

I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?
Something like:
syscon {
	pinctrl: pinctrl {
		compatible …

		gpio-controller;
		gpio-ranges = <&pinctrl 0 0 32>;
		#gpio-cells = <2>;

		…
	};
};

> 
>> +          data = <0xc>;
>> +          dirout = <0x4>;
> 
> This looks similar to the brcm,bcm6345-gpio.txt binding which then
> uses the gpio-mmio driver. Defining addresses with 'reg' is much
> preferred over custom properties. That binding also captures the bank
> size.

It’s similar, but Linus requested to use gpio regmap because we had a large amount of registers, so we’re not using it.

> 
> Rob

Best regards,
Álvaro.
Rob Herring March 10, 2021, 6:45 p.m. UTC | #3
On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
>
> Hi Rob,
>
> > El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
> >
> > On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
> > <noltari@gmail.com> wrote:
> >>
> >> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
> >>
> >> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> >> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >> ---
> >> v6: add changes suggested by Rob Herring
> >> v5: change Documentation to dt-bindings in commit title
> >> v4: no changes
> >> v3: add new gpio node
> >> v2: remove interrupts
> >>
> >> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
> >> 1 file changed, 174 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >> new file mode 100644
> >> index 000000000000..471f6efa1754
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >> @@ -0,0 +1,174 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Broadcom BCM6328 pin controller
> >> +
> >> +maintainers:
> >> +  - Álvaro Fernández Rojas <noltari@gmail.com>
> >> +  - Jonas Gorski <jonas.gorski@gmail.com>
> >> +
> >> +description: |+
> >> +  The pin controller node should be the child of a syscon node.
> >> +
> >> +  Refer to the the bindings described in
> >> +  Documentation/devicetree/bindings/mfd/syscon.yaml
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: brcm,bcm6328-pinctrl
> >> +
> >> +  gpio:
> >> +    type: object
> >> +    properties:
> >> +      compatible:
> >> +        const: brcm,bcm6328-gpio
> >> +
> >> +      data:
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        description: |
> >> +          Offset in the register map for the data register (in bytes).
> >> +
> >> +      dirout:
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        description: |
> >> +          Offset in the register map for the dirout register (in bytes).
> >> +
> >> +      gpio-controller: true
> >> +
> >> +      "#gpio-cells":
> >> +        const: 2
> >> +
> >> +      gpio-ranges:
> >> +        maxItems: 1
> >> +
> >> +    required:
> >> +      - gpio-controller
> >> +      - gpio-ranges
> >> +      - '#gpio-cells'
> >> +
> >> +    additionalProperties: false
> >> +
> >> +patternProperties:
> >> +  '^.*-pins$':
> >> +    if:
> >> +      type: object
> >> +    then:
> >> +      properties:
> >> +        function:
> >> +          $ref: "pinmux-node.yaml#/properties/function"
> >> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
> >> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
> >> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
> >> +
> >> +        pins:
> >> +          $ref: "pinmux-node.yaml#/properties/pins"
> >> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
> >> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
> >> +                  usb_port1 ]
> >> +
> >> +required:
> >> +  - compatible
> >> +  - gpio
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    gpio_cntl@10000080 {
> >> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
> >
> > You just added "brcm,bcm6328-gpio-controller", it would need to be documented.
>
> I just added that because you requested me to do it ¯\_(ツ)_/¯

I said 'syscon' by itself was not allowed, then asked about the multiple levels.

> What should I do to document it?
> I still don’t get most of this .yaml stuff...
>
> >
> >> +      reg = <0x10000080 0x80>;
> >> +
> >> +      pinctrl: pinctrl {
> >> +        compatible = "brcm,bcm6328-pinctrl";
> >> +
> >> +        gpio {
> >> +          compatible = "brcm,bcm6328-gpio";
> >
> > I'm still trying to understand why you need 3 levels of nodes here?
> > The gpio controller contains a pin controller plus other undefined
> > functions (because of 'syscon') and the pin controller contains a gpio
> > controller?
>
> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…
>
> >
> > I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
> > should be a single node.
>
> I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?

It wouldn't be. I wasn't saying the pinctrl and gpio controller are
the same node. My suggestion was combining syscon and pinctrl.

> Something like:
> syscon {

Again with the syscon. If pinctrl and GPIO are the only functions
within this h/w block, then this is not a syscon. You are just abusing
that having 'syscon' compatible means you get a regmap created
automagically for you. Nothing here looks like a 'system controller'
to me. A 'system controller' is a random collection of register bits
with functions that don't fit anywhere else.

>         pinctrl: pinctrl {
>                 compatible …
>
>                 gpio-controller;
>                 gpio-ranges = <&pinctrl 0 0 32>;
>                 #gpio-cells = <2>;

I was assuming you have multiple GPIO controllers within 1 pinctlr?
The pinctrl and gpio could be a single node like above if there's only
1 GPIO controller. But I'm still somewhat guessing what the h/w looks
like because I have to go searching thru the driver to decipher.
Please describe the h/w in the binding.

If there's more than 1 GPIO controller, then I'd imagine you have
something like this:

pinctrl {
  ...
  reg = <base 0x80>;
  ranges = <0 base 0x80;
  gpio@4 {
    reg = <4 4>, <c 4>;
    reg-names = "dirout", "dat";
  };
  gpio@? {};

  foo-pins {};
};

>
>                 …
>         };
> };
>
> >
> >> +          data = <0xc>;
> >> +          dirout = <0x4>;
> >
> > This looks similar to the brcm,bcm6345-gpio.txt binding which then
> > uses the gpio-mmio driver. Defining addresses with 'reg' is much
> > preferred over custom properties. That binding also captures the bank
> > size.
>
> It’s similar, but Linus requested to use gpio regmap because we had a large amount of registers, so we’re not using it.

Looks like you have 2 registers to me.

Rob
Álvaro Fernández Rojas March 10, 2021, 7:10 p.m. UTC | #4
Hi Rob,

> El 10 mar 2021, a las 19:45, Rob Herring <robh+dt@kernel.org> escribió:
> 
> On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>> 
>> Hi Rob,
>> 
>>> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
>>> 
>>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
>>> <noltari@gmail.com> wrote:
>>>> 
>>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>>>> 
>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>> ---
>>>> v6: add changes suggested by Rob Herring
>>>> v5: change Documentation to dt-bindings in commit title
>>>> v4: no changes
>>>> v3: add new gpio node
>>>> v2: remove interrupts
>>>> 
>>>> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
>>>> 1 file changed, 174 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>> new file mode 100644
>>>> index 000000000000..471f6efa1754
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>> @@ -0,0 +1,174 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Broadcom BCM6328 pin controller
>>>> +
>>>> +maintainers:
>>>> +  - Álvaro Fernández Rojas <noltari@gmail.com>
>>>> +  - Jonas Gorski <jonas.gorski@gmail.com>
>>>> +
>>>> +description: |+
>>>> +  The pin controller node should be the child of a syscon node.
>>>> +
>>>> +  Refer to the the bindings described in
>>>> +  Documentation/devicetree/bindings/mfd/syscon.yaml
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: brcm,bcm6328-pinctrl
>>>> +
>>>> +  gpio:
>>>> +    type: object
>>>> +    properties:
>>>> +      compatible:
>>>> +        const: brcm,bcm6328-gpio
>>>> +
>>>> +      data:
>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>> +        description: |
>>>> +          Offset in the register map for the data register (in bytes).
>>>> +
>>>> +      dirout:
>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>> +        description: |
>>>> +          Offset in the register map for the dirout register (in bytes).
>>>> +
>>>> +      gpio-controller: true
>>>> +
>>>> +      "#gpio-cells":
>>>> +        const: 2
>>>> +
>>>> +      gpio-ranges:
>>>> +        maxItems: 1
>>>> +
>>>> +    required:
>>>> +      - gpio-controller
>>>> +      - gpio-ranges
>>>> +      - '#gpio-cells'
>>>> +
>>>> +    additionalProperties: false
>>>> +
>>>> +patternProperties:
>>>> +  '^.*-pins$':
>>>> +    if:
>>>> +      type: object
>>>> +    then:
>>>> +      properties:
>>>> +        function:
>>>> +          $ref: "pinmux-node.yaml#/properties/function"
>>>> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
>>>> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
>>>> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
>>>> +
>>>> +        pins:
>>>> +          $ref: "pinmux-node.yaml#/properties/pins"
>>>> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
>>>> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
>>>> +                  usb_port1 ]
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - gpio
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    gpio_cntl@10000080 {
>>>> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
>>> 
>>> You just added "brcm,bcm6328-gpio-controller", it would need to be documented.
>> 
>> I just added that because you requested me to do it ¯\_(ツ)_/¯
> 
> I said 'syscon' by itself was not allowed, then asked about the multiple levels.

Why not?
What if you have several controllers inside a syscon?
The root should also have “something" in it?

> 
>> What should I do to document it?
>> I still don’t get most of this .yaml stuff...
>> 
>>> 
>>>> +      reg = <0x10000080 0x80>;
>>>> +
>>>> +      pinctrl: pinctrl {
>>>> +        compatible = "brcm,bcm6328-pinctrl";
>>>> +
>>>> +        gpio {
>>>> +          compatible = "brcm,bcm6328-gpio";
>>> 
>>> I'm still trying to understand why you need 3 levels of nodes here?
>>> The gpio controller contains a pin controller plus other undefined
>>> functions (because of 'syscon') and the pin controller contains a gpio
>>> controller?
>> 
>> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
>> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…
>> 
>>> 
>>> I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
>>> should be a single node.
>> 
>> I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?
> 
> It wouldn't be. I wasn't saying the pinctrl and gpio controller are
> the same node. My suggestion was combining syscon and pinctrl.

But that wouldn’t be correct if there were more “things” inside the syscon, right?

> 
>> Something like:
>> syscon {
> 
> Again with the syscon. If pinctrl and GPIO are the only functions
> within this h/w block, then this is not a syscon. You are just abusing
> that having 'syscon' compatible means you get a regmap created
> automagically for you. Nothing here looks like a 'system controller'
> to me. A 'system controller' is a random collection of register bits
> with functions that don't fit anywhere else.

pinctrl and GPIO aren’t the only functions within this HW block.
Maybe I didn’t document/code it properly, but I’m sure I’m not abusing what a system controller is.
Please, take a look at http://www.datashed.science/misc/bcm/gpl/broadcom-sdk-416L05/shared/opensource/include/bcm963xx/6328_map_part.h:
typedef struct GpioControl {
    uint32      GPIODirHi;                  /* 0 */
    uint32      GPIODir;                    /* 4 */
    uint32      GPIOioHi;                   /* 8 */
    uint32      GPIOio;                     /* C */
    uint32      unused0;                    /* 10 */
    uint32      SpiSlaveCfg;                /* 14 */
    uint32      GPIOMode;                   /* 18 */
    uint64      PinMuxSel;                  /* 1C */
    uint32      PinMuxSelOther;             /* 24 */
    uint32      TestControl;                /* 28 */
    uint32      unused2;                    /* 2C */
    uint32      RoboSWLEDControl;           /* 30 */
    uint32      RoboSWLEDLSR;               /* 34 */
    uint32      unused3;                    /* 38 */
    uint32      RoboswEphyCtrl;             /* 3C */
    uint32      RoboswSwitchCtrl;           /* 40 */
    uint32      RegFileTmCtl;               /* 44 */
    uint32      RingOscCtrl0;               /* 48 */
    uint32      RingOscCtrl1;               /* 4C */
    uint32      unused4[6];                 /* 50 - 64 */
    uint32      DieRevID;                   /* 68 */
    uint32      unused5;                    /* 6c */
    uint32      DiagSelControl;             /* 70 */
    uint32      DiagReadBack;               /* 74 */
    uint32      DiagReadBackHi;             /* 78 */
    uint32      DiagMiscControl;            /* 7c */
} GpioControl;

So we’re using GPIODirHi, GPIODir, GPIOioHi and GPIOio registers for GPIO regmap driver.
And we’re using GPIOMode, PinMuxSel (u64 -> x2 u32), PinMuxSelOther for pinctrl driver.
And this is for BCM6328, but some of the other SoCs are even more scattered.

> 
>>        pinctrl: pinctrl {
>>                compatible …
>> 
>>                gpio-controller;
>>                gpio-ranges = <&pinctrl 0 0 32>;
>>                #gpio-cells = <2>;
> 
> I was assuming you have multiple GPIO controllers within 1 pinctlr?
> The pinctrl and gpio could be a single node like above if there's only
> 1 GPIO controller. But I'm still somewhat guessing what the h/w looks
> like because I have to go searching thru the driver to decipher.
> Please describe the h/w in the binding.

GPIO dirout and data rely on 2x u32 registers or a single u64 register.
This is can be either be implemented as a single GPIO controller, or as 2 separate GPIO controllers.
However, since I’m overriding reg_mask_xlate with bcm63xx_reg_mask_xlate I can register it as a single GPIO controller, which makes more sense to me.

> 
> If there's more than 1 GPIO controller, then I'd imagine you have
> something like this:
> 
> pinctrl {
>  ...
>  reg = <base 0x80>;
>  ranges = <0 base 0x80;
>  gpio@4 {
>    reg = <4 4>, <c 4>;
>    reg-names = "dirout", "dat";
>  };
>  gpio@? {};
> 
>  foo-pins {};
> };
> 
>> 
>>                …
>>        };
>> };
>> 
>>> 
>>>> +          data = <0xc>;
>>>> +          dirout = <0x4>;
>>> 
>>> This looks similar to the brcm,bcm6345-gpio.txt binding which then
>>> uses the gpio-mmio driver. Defining addresses with 'reg' is much
>>> preferred over custom properties. That binding also captures the bank
>>> size.
>> 
>> It’s similar, but Linus requested to use gpio regmap because we had a large amount of registers, so we’re not using it.
> 
> Looks like you have 2 registers to me.

For the GPIO controller there are 4 registers (data high and low, and dirout high and low).
For the pinctrl there are 3 registers (Pinmux high, low and other).

> 
> Rob

Best regards,
Álvaro.
Rob Herring March 10, 2021, 8:52 p.m. UTC | #5
On Wed, Mar 10, 2021 at 12:10 PM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
>
> Hi Rob,
>
> > El 10 mar 2021, a las 19:45, Rob Herring <robh+dt@kernel.org> escribió:
> >
> > On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
> > <noltari@gmail.com> wrote:
> >>
> >> Hi Rob,
> >>
> >>> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
> >>>
> >>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
> >>> <noltari@gmail.com> wrote:
> >>>>
> >>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
> >>>>
> >>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> >>>> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >>>> ---
> >>>> v6: add changes suggested by Rob Herring
> >>>> v5: change Documentation to dt-bindings in commit title
> >>>> v4: no changes
> >>>> v3: add new gpio node
> >>>> v2: remove interrupts
> >>>>
> >>>> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
> >>>> 1 file changed, 174 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..471f6efa1754
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>>> @@ -0,0 +1,174 @@
> >>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Broadcom BCM6328 pin controller
> >>>> +
> >>>> +maintainers:
> >>>> +  - Álvaro Fernández Rojas <noltari@gmail.com>
> >>>> +  - Jonas Gorski <jonas.gorski@gmail.com>
> >>>> +
> >>>> +description: |+
> >>>> +  The pin controller node should be the child of a syscon node.
> >>>> +
> >>>> +  Refer to the the bindings described in
> >>>> +  Documentation/devicetree/bindings/mfd/syscon.yaml
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    const: brcm,bcm6328-pinctrl
> >>>> +
> >>>> +  gpio:
> >>>> +    type: object
> >>>> +    properties:
> >>>> +      compatible:
> >>>> +        const: brcm,bcm6328-gpio
> >>>> +
> >>>> +      data:
> >>>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +        description: |
> >>>> +          Offset in the register map for the data register (in bytes).
> >>>> +
> >>>> +      dirout:
> >>>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +        description: |
> >>>> +          Offset in the register map for the dirout register (in bytes).
> >>>> +
> >>>> +      gpio-controller: true
> >>>> +
> >>>> +      "#gpio-cells":
> >>>> +        const: 2
> >>>> +
> >>>> +      gpio-ranges:
> >>>> +        maxItems: 1
> >>>> +
> >>>> +    required:
> >>>> +      - gpio-controller
> >>>> +      - gpio-ranges
> >>>> +      - '#gpio-cells'
> >>>> +
> >>>> +    additionalProperties: false
> >>>> +
> >>>> +patternProperties:
> >>>> +  '^.*-pins$':
> >>>> +    if:
> >>>> +      type: object
> >>>> +    then:
> >>>> +      properties:
> >>>> +        function:
> >>>> +          $ref: "pinmux-node.yaml#/properties/function"
> >>>> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
> >>>> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
> >>>> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
> >>>> +
> >>>> +        pins:
> >>>> +          $ref: "pinmux-node.yaml#/properties/pins"
> >>>> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
> >>>> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
> >>>> +                  usb_port1 ]
> >>>> +
> >>>> +required:
> >>>> +  - compatible
> >>>> +  - gpio
> >>>> +
> >>>> +additionalProperties: false
> >>>> +
> >>>> +examples:
> >>>> +  - |
> >>>> +    gpio_cntl@10000080 {
> >>>> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
> >>>
> >>> You just added "brcm,bcm6328-gpio-controller", it would need to be documented.
> >>
> >> I just added that because you requested me to do it ¯\_(ツ)_/¯
> >
> > I said 'syscon' by itself was not allowed, then asked about the multiple levels.
>
> Why not?

Because 'syscon' alone doesn't mean anything and doesn't describe what
registers it contains. You need something that says this is the XYZ
block in the ABC SoC.

> What if you have several controllers inside a syscon?

You either just add properties (e.g. just add #clock-cells and it's a
clock provider) or you have child nodes. Which one you do generally
depends on if the providers have DT resources themselves.

> The root should also have “something" in it?
>
> >
> >> What should I do to document it?
> >> I still don’t get most of this .yaml stuff...
> >>
> >>>
> >>>> +      reg = <0x10000080 0x80>;
> >>>> +
> >>>> +      pinctrl: pinctrl {
> >>>> +        compatible = "brcm,bcm6328-pinctrl";
> >>>> +
> >>>> +        gpio {
> >>>> +          compatible = "brcm,bcm6328-gpio";
> >>>
> >>> I'm still trying to understand why you need 3 levels of nodes here?
> >>> The gpio controller contains a pin controller plus other undefined
> >>> functions (because of 'syscon') and the pin controller contains a gpio
> >>> controller?
> >>
> >> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
> >> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…
> >>
> >>>
> >>> I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
> >>> should be a single node.
> >>
> >> I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?
> >
> > It wouldn't be. I wasn't saying the pinctrl and gpio controller are
> > the same node. My suggestion was combining syscon and pinctrl.
>
> But that wouldn’t be correct if there were more “things” inside the syscon, right?

Right.

> >> Something like:
> >> syscon {
> >
> > Again with the syscon. If pinctrl and GPIO are the only functions
> > within this h/w block, then this is not a syscon. You are just abusing
> > that having 'syscon' compatible means you get a regmap created
> > automagically for you. Nothing here looks like a 'system controller'
> > to me. A 'system controller' is a random collection of register bits
> > with functions that don't fit anywhere else.
>
> pinctrl and GPIO aren’t the only functions within this HW block.
> Maybe I didn’t document/code it properly, but I’m sure I’m not abusing what a system controller is.

Okay, that's the detail missing from this patch.

> Please, take a look at http://www.datashed.science/misc/bcm/gpl/broadcom-sdk-416L05/shared/opensource/include/bcm963xx/6328_map_part.h:
> typedef struct GpioControl {
>     uint32      GPIODirHi;                  /* 0 */
>     uint32      GPIODir;                    /* 4 */
>     uint32      GPIOioHi;                   /* 8 */
>     uint32      GPIOio;                     /* C */
>     uint32      unused0;                    /* 10 */
>     uint32      SpiSlaveCfg;                /* 14 */
>     uint32      GPIOMode;                   /* 18 */
>     uint64      PinMuxSel;                  /* 1C */
>     uint32      PinMuxSelOther;             /* 24 */
>     uint32      TestControl;                /* 28 */
>     uint32      unused2;                    /* 2C */
>     uint32      RoboSWLEDControl;           /* 30 */
>     uint32      RoboSWLEDLSR;               /* 34 */
>     uint32      unused3;                    /* 38 */
>     uint32      RoboswEphyCtrl;             /* 3C */
>     uint32      RoboswSwitchCtrl;           /* 40 */
>     uint32      RegFileTmCtl;               /* 44 */
>     uint32      RingOscCtrl0;               /* 48 */
>     uint32      RingOscCtrl1;               /* 4C */
>     uint32      unused4[6];                 /* 50 - 64 */
>     uint32      DieRevID;                   /* 68 */
>     uint32      unused5;                    /* 6c */
>     uint32      DiagSelControl;             /* 70 */
>     uint32      DiagReadBack;               /* 74 */
>     uint32      DiagReadBackHi;             /* 78 */
>     uint32      DiagMiscControl;            /* 7c */
> } GpioControl;
>
> So we’re using GPIODirHi, GPIODir, GPIOioHi and GPIOio registers for GPIO regmap driver.
> And we’re using GPIOMode, PinMuxSel (u64 -> x2 u32), PinMuxSelOther for pinctrl driver.
> And this is for BCM6328, but some of the other SoCs are even more scattered.

So based on this I'd do something like this:

syscon {
  reg = <base 0x80>;
  ranges = <0 base 0x80>;
  pinctrl@18 {
    reg = <0x18 0x10>;
    foo-pins {};
  gpio@0 {
    reg = <0x0 0x10>;
  };
};

If things are more scattered within gpio or pinctrl, then maybe you
need multiple reg entries. Whether the OS uses 'reg' and mmio or a
regmap from the syscon is up to you. That's independent of the
binding.

> >>        pinctrl: pinctrl {
> >>                compatible …
> >>
> >>                gpio-controller;
> >>                gpio-ranges = <&pinctrl 0 0 32>;
> >>                #gpio-cells = <2>;
> >
> > I was assuming you have multiple GPIO controllers within 1 pinctlr?
> > The pinctrl and gpio could be a single node like above if there's only
> > 1 GPIO controller. But I'm still somewhat guessing what the h/w looks
> > like because I have to go searching thru the driver to decipher.
> > Please describe the h/w in the binding.
>
> GPIO dirout and data rely on 2x u32 registers or a single u64 register.
> This is can be either be implemented as a single GPIO controller, or as 2 separate GPIO controllers.
> However, since I’m overriding reg_mask_xlate with bcm63xx_reg_mask_xlate I can register it as a single GPIO controller, which makes more sense to me.

I think 1 makes more sense.

Rob
Linus Walleij March 11, 2021, 1:22 a.m. UTC | #6
On Wed, Mar 10, 2021 at 7:03 PM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> > El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:

> >> +      reg = <0x10000080 0x80>;
> >> +
> >> +      pinctrl: pinctrl {
> >> +        compatible = "brcm,bcm6328-pinctrl";
> >> +
> >> +        gpio {
> >> +          compatible = "brcm,bcm6328-gpio";
> >
> > I'm still trying to understand why you need 3 levels of nodes here?
> > The gpio controller contains a pin controller plus other undefined
> > functions (because of 'syscon') and the pin controller contains a gpio
> > controller?
>
> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…

I think the crucial point is that these hardwares (all patches in the series)
is very similar and some contain several GPIO blocks rather than just
one.

The structure needs to be seen as a common pattern. We are doing this
because it gives a common structure to all BCM SoCs in this
family, which is nice.

Yours,
Linus Walleij
Álvaro Fernández Rojas March 11, 2021, 5:09 p.m. UTC | #7
Hi Rob,

El 10/03/2021 a las 21:52, Rob Herring escribió:
> On Wed, Mar 10, 2021 at 12:10 PM Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>>
>> Hi Rob,
>>
>>> El 10 mar 2021, a las 19:45, Rob Herring <robh+dt@kernel.org> escribió:
>>>
>>> On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
>>> <noltari@gmail.com> wrote:
>>>>
>>>> Hi Rob,
>>>>
>>>>> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
>>>>>
>>>>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
>>>>> <noltari@gmail.com> wrote:
>>>>>>
>>>>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>>>>>>
>>>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>>>> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>>>> ---
>>>>>> v6: add changes suggested by Rob Herring
>>>>>> v5: change Documentation to dt-bindings in commit title
>>>>>> v4: no changes
>>>>>> v3: add new gpio node
>>>>>> v2: remove interrupts
>>>>>>
>>>>>> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
>>>>>> 1 file changed, 174 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..471f6efa1754
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>>>> @@ -0,0 +1,174 @@
>>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Broadcom BCM6328 pin controller
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Álvaro Fernández Rojas <noltari@gmail.com>
>>>>>> +  - Jonas Gorski <jonas.gorski@gmail.com>
>>>>>> +
>>>>>> +description: |+
>>>>>> +  The pin controller node should be the child of a syscon node.
>>>>>> +
>>>>>> +  Refer to the the bindings described in
>>>>>> +  Documentation/devicetree/bindings/mfd/syscon.yaml
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: brcm,bcm6328-pinctrl
>>>>>> +
>>>>>> +  gpio:
>>>>>> +    type: object
>>>>>> +    properties:
>>>>>> +      compatible:
>>>>>> +        const: brcm,bcm6328-gpio
>>>>>> +
>>>>>> +      data:
>>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +        description: |
>>>>>> +          Offset in the register map for the data register (in bytes).
>>>>>> +
>>>>>> +      dirout:
>>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +        description: |
>>>>>> +          Offset in the register map for the dirout register (in bytes).
>>>>>> +
>>>>>> +      gpio-controller: true
>>>>>> +
>>>>>> +      "#gpio-cells":
>>>>>> +        const: 2
>>>>>> +
>>>>>> +      gpio-ranges:
>>>>>> +        maxItems: 1
>>>>>> +
>>>>>> +    required:
>>>>>> +      - gpio-controller
>>>>>> +      - gpio-ranges
>>>>>> +      - '#gpio-cells'
>>>>>> +
>>>>>> +    additionalProperties: false
>>>>>> +
>>>>>> +patternProperties:
>>>>>> +  '^.*-pins$':
>>>>>> +    if:
>>>>>> +      type: object
>>>>>> +    then:
>>>>>> +      properties:
>>>>>> +        function:
>>>>>> +          $ref: "pinmux-node.yaml#/properties/function"
>>>>>> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
>>>>>> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
>>>>>> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
>>>>>> +
>>>>>> +        pins:
>>>>>> +          $ref: "pinmux-node.yaml#/properties/pins"
>>>>>> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
>>>>>> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
>>>>>> +                  usb_port1 ]
>>>>>> +
>>>>>> +required:
>>>>>> +  - compatible
>>>>>> +  - gpio
>>>>>> +
>>>>>> +additionalProperties: false
>>>>>> +
>>>>>> +examples:
>>>>>> +  - |
>>>>>> +    gpio_cntl@10000080 {
>>>>>> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
>>>>>
>>>>> You just added "brcm,bcm6328-gpio-controller", it would need to be documented.
>>>>
>>>> I just added that because you requested me to do it ¯\_(ツ)_/¯
>>>
>>> I said 'syscon' by itself was not allowed, then asked about the multiple levels.
>>
>> Why not?
> 
> Because 'syscon' alone doesn't mean anything and doesn't describe what
> registers it contains. You need something that says this is the XYZ
> block in the ABC SoC.
> 
>> What if you have several controllers inside a syscon?
> 
> You either just add properties (e.g. just add #clock-cells and it's a
> clock provider) or you have child nodes. Which one you do generally
> depends on if the providers have DT resources themselves.
> 
>> The root should also have “something" in it?
>>
>>>
>>>> What should I do to document it?

You didn't answer my question about adding documentation...

An example driver which adds a custom compatible string and doesn't 
document it:
https://github.com/torvalds/linux/blob/a74e6a014c9d4d4161061f770c9b4f98372ac778/Documentation/devicetree/bindings/clock/sprd%2Csc9863a-clk.yaml#L90

>>>> I still don’t get most of this .yaml stuff...
>>>>
>>>>>
>>>>>> +      reg = <0x10000080 0x80>;
>>>>>> +
>>>>>> +      pinctrl: pinctrl {
>>>>>> +        compatible = "brcm,bcm6328-pinctrl";
>>>>>> +
>>>>>> +        gpio {
>>>>>> +          compatible = "brcm,bcm6328-gpio";
>>>>>
>>>>> I'm still trying to understand why you need 3 levels of nodes here?
>>>>> The gpio controller contains a pin controller plus other undefined
>>>>> functions (because of 'syscon') and the pin controller contains a gpio
>>>>> controller?
>>>>
>>>> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
>>>> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…
>>>>
>>>>>
>>>>> I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
>>>>> should be a single node.
>>>>
>>>> I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?
>>>
>>> It wouldn't be. I wasn't saying the pinctrl and gpio controller are
>>> the same node. My suggestion was combining syscon and pinctrl.
>>
>> But that wouldn’t be correct if there were more “things” inside the syscon, right?
> 
> Right.
> 
>>>> Something like:
>>>> syscon {
>>>
>>> Again with the syscon. If pinctrl and GPIO are the only functions
>>> within this h/w block, then this is not a syscon. You are just abusing
>>> that having 'syscon' compatible means you get a regmap created
>>> automagically for you. Nothing here looks like a 'system controller'
>>> to me. A 'system controller' is a random collection of register bits
>>> with functions that don't fit anywhere else.
>>
>> pinctrl and GPIO aren’t the only functions within this HW block.
>> Maybe I didn’t document/code it properly, but I’m sure I’m not abusing what a system controller is.
> 
> Okay, that's the detail missing from this patch.
> 
>> Please, take a look at http://www.datashed.science/misc/bcm/gpl/broadcom-sdk-416L05/shared/opensource/include/bcm963xx/6328_map_part.h:
>> typedef struct GpioControl {
>>      uint32      GPIODirHi;                  /* 0 */
>>      uint32      GPIODir;                    /* 4 */
>>      uint32      GPIOioHi;                   /* 8 */
>>      uint32      GPIOio;                     /* C */
>>      uint32      unused0;                    /* 10 */
>>      uint32      SpiSlaveCfg;                /* 14 */
>>      uint32      GPIOMode;                   /* 18 */
>>      uint64      PinMuxSel;                  /* 1C */
>>      uint32      PinMuxSelOther;             /* 24 */
>>      uint32      TestControl;                /* 28 */
>>      uint32      unused2;                    /* 2C */
>>      uint32      RoboSWLEDControl;           /* 30 */
>>      uint32      RoboSWLEDLSR;               /* 34 */
>>      uint32      unused3;                    /* 38 */
>>      uint32      RoboswEphyCtrl;             /* 3C */
>>      uint32      RoboswSwitchCtrl;           /* 40 */
>>      uint32      RegFileTmCtl;               /* 44 */
>>      uint32      RingOscCtrl0;               /* 48 */
>>      uint32      RingOscCtrl1;               /* 4C */
>>      uint32      unused4[6];                 /* 50 - 64 */
>>      uint32      DieRevID;                   /* 68 */
>>      uint32      unused5;                    /* 6c */
>>      uint32      DiagSelControl;             /* 70 */
>>      uint32      DiagReadBack;               /* 74 */
>>      uint32      DiagReadBackHi;             /* 78 */
>>      uint32      DiagMiscControl;            /* 7c */
>> } GpioControl;
>>
>> So we’re using GPIODirHi, GPIODir, GPIOioHi and GPIOio registers for GPIO regmap driver.
>> And we’re using GPIOMode, PinMuxSel (u64 -> x2 u32), PinMuxSelOther for pinctrl driver.
>> And this is for BCM6328, but some of the other SoCs are even more scattered.
> 
> So based on this I'd do something like this:
> 
> syscon {
>    reg = <base 0x80>;
>    ranges = <0 base 0x80>;
>    pinctrl@18 {
>      reg = <0x18 0x10>;
>      foo-pins {};
>    gpio@0 {
>      reg = <0x0 0x10>;
>    };
> };

You're missing a "};", so I'm not sure if you want me to go this way (1):
syscon {
	compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
	reg = <0x10000080 0x80>;
	ranges = <0 0x10000080 0x80>;

	gpio: gpio@0 {
		compatible = "brcm,bcm6328-gpio";
		reg = <0x0 0x10>;

		data = <0xc>;
		dirout = <0x4>;

		gpio-controller;
		gpio-ranges = <&pinctrl 0 0 32>;
		#gpio-cells = <2>;
	};

	pinctrl: pinctrl@18 {
		compatible = "brcm,bcm6328-pinctrl";
		reg = <0x18 0x10>;

		foo-pins {};
		...
	};
};

Or this way (2):
syscon {
	compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
	reg = <0x10000080 0x80>;
	ranges = <0 0x10000080 0x80>;

	pinctrl: pinctrl@18 {
		compatible = "brcm,bcm6328-pinctrl";
		reg = <0x0 0x28>;

		gpio: gpio@0 {
			compatible = "brcm,bcm6328-gpio";
			reg = <0x0 0x10>;

			data = <0xc>;
			dirout = <0x4>;

			gpio-controller;
			gpio-ranges = <&pinctrl 0 0 32>;
			#gpio-cells = <2>;
		};

		foo-pins {};
		...
	};
};

> 
> If things are more scattered within gpio or pinctrl, then maybe you
> need multiple reg entries. Whether the OS uses 'reg' and mmio or a
> regmap from the syscon is up to you. That's independent of the
> binding.
> 
>>>>         pinctrl: pinctrl {
>>>>                 compatible …
>>>>
>>>>                 gpio-controller;
>>>>                 gpio-ranges = <&pinctrl 0 0 32>;
>>>>                 #gpio-cells = <2>;
>>>
>>> I was assuming you have multiple GPIO controllers within 1 pinctlr?
>>> The pinctrl and gpio could be a single node like above if there's only
>>> 1 GPIO controller. But I'm still somewhat guessing what the h/w looks
>>> like because I have to go searching thru the driver to decipher.
>>> Please describe the h/w in the binding.
>>
>> GPIO dirout and data rely on 2x u32 registers or a single u64 register.
>> This is can be either be implemented as a single GPIO controller, or as 2 separate GPIO controllers.
>> However, since I’m overriding reg_mask_xlate with bcm63xx_reg_mask_xlate I can register it as a single GPIO controller, which makes more sense to me.
> 
> I think 1 makes more sense.
> 
> Rob
> 

Best regards,
Álvaro.
Rob Herring March 11, 2021, 6:14 p.m. UTC | #8
On Thu, Mar 11, 2021 at 10:09 AM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
>
> Hi Rob,
>
> El 10/03/2021 a las 21:52, Rob Herring escribió:
> > On Wed, Mar 10, 2021 at 12:10 PM Álvaro Fernández Rojas
> > <noltari@gmail.com> wrote:
> >>
> >> Hi Rob,
> >>
> >>> El 10 mar 2021, a las 19:45, Rob Herring <robh+dt@kernel.org> escribió:
> >>>
> >>> On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
> >>> <noltari@gmail.com> wrote:
> >>>>
> >>>> Hi Rob,
> >>>>
> >>>>> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
> >>>>>
> >>>>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
> >>>>> <noltari@gmail.com> wrote:
> >>>>>>
> >>>>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
> >>>>>>
> >>>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
> >>>>>> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
> >>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >>>>>> ---
> >>>>>> v6: add changes suggested by Rob Herring
> >>>>>> v5: change Documentation to dt-bindings in commit title
> >>>>>> v4: no changes
> >>>>>> v3: add new gpio node
> >>>>>> v2: remove interrupts
> >>>>>>
> >>>>>> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
> >>>>>> 1 file changed, 174 insertions(+)
> >>>>>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..471f6efa1754
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
> >>>>>> @@ -0,0 +1,174 @@
> >>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>>>>> +%YAML 1.2
> >>>>>> +---
> >>>>>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
> >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>> +
> >>>>>> +title: Broadcom BCM6328 pin controller
> >>>>>> +
> >>>>>> +maintainers:
> >>>>>> +  - Álvaro Fernández Rojas <noltari@gmail.com>
> >>>>>> +  - Jonas Gorski <jonas.gorski@gmail.com>
> >>>>>> +
> >>>>>> +description: |+
> >>>>>> +  The pin controller node should be the child of a syscon node.
> >>>>>> +
> >>>>>> +  Refer to the the bindings described in
> >>>>>> +  Documentation/devicetree/bindings/mfd/syscon.yaml
> >>>>>> +
> >>>>>> +properties:
> >>>>>> +  compatible:
> >>>>>> +    const: brcm,bcm6328-pinctrl
> >>>>>> +
> >>>>>> +  gpio:
> >>>>>> +    type: object
> >>>>>> +    properties:
> >>>>>> +      compatible:
> >>>>>> +        const: brcm,bcm6328-gpio
> >>>>>> +
> >>>>>> +      data:
> >>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>> +        description: |
> >>>>>> +          Offset in the register map for the data register (in bytes).
> >>>>>> +
> >>>>>> +      dirout:
> >>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>> +        description: |
> >>>>>> +          Offset in the register map for the dirout register (in bytes).
> >>>>>> +
> >>>>>> +      gpio-controller: true
> >>>>>> +
> >>>>>> +      "#gpio-cells":
> >>>>>> +        const: 2
> >>>>>> +
> >>>>>> +      gpio-ranges:
> >>>>>> +        maxItems: 1
> >>>>>> +
> >>>>>> +    required:
> >>>>>> +      - gpio-controller
> >>>>>> +      - gpio-ranges
> >>>>>> +      - '#gpio-cells'
> >>>>>> +
> >>>>>> +    additionalProperties: false
> >>>>>> +
> >>>>>> +patternProperties:
> >>>>>> +  '^.*-pins$':
> >>>>>> +    if:
> >>>>>> +      type: object
> >>>>>> +    then:
> >>>>>> +      properties:
> >>>>>> +        function:
> >>>>>> +          $ref: "pinmux-node.yaml#/properties/function"
> >>>>>> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
> >>>>>> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
> >>>>>> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
> >>>>>> +
> >>>>>> +        pins:
> >>>>>> +          $ref: "pinmux-node.yaml#/properties/pins"
> >>>>>> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
> >>>>>> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
> >>>>>> +                  usb_port1 ]
> >>>>>> +
> >>>>>> +required:
> >>>>>> +  - compatible
> >>>>>> +  - gpio
> >>>>>> +
> >>>>>> +additionalProperties: false
> >>>>>> +
> >>>>>> +examples:
> >>>>>> +  - |
> >>>>>> +    gpio_cntl@10000080 {
> >>>>>> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
> >>>>>
> >>>>> You just added "brcm,bcm6328-gpio-controller", it would need to be documented.
> >>>>
> >>>> I just added that because you requested me to do it ¯\_(ツ)_/¯
> >>>
> >>> I said 'syscon' by itself was not allowed, then asked about the multiple levels.
> >>
> >> Why not?
> >
> > Because 'syscon' alone doesn't mean anything and doesn't describe what
> > registers it contains. You need something that says this is the XYZ
> > block in the ABC SoC.
> >
> >> What if you have several controllers inside a syscon?
> >
> > You either just add properties (e.g. just add #clock-cells and it's a
> > clock provider) or you have child nodes. Which one you do generally
> > depends on if the providers have DT resources themselves.
> >
> >> The root should also have “something" in it?
> >>
> >>>
> >>>> What should I do to document it?
>
> You didn't answer my question about adding documentation...

You have to document it. Whether it's 1 or 3 schema files depends on
where we end up, but the top-level one should reference the others if
it's more than 1 file:

child-node:
  type: object
  $ref: "/schemas/foo/child-node.yaml#"

> An example driver which adds a custom compatible string and doesn't
> document it:
> https://github.com/torvalds/linux/blob/a74e6a014c9d4d4161061f770c9b4f98372ac778/Documentation/devicetree/bindings/clock/sprd%2Csc9863a-clk.yaml#L90

*it Happens.

Those cases are now blocking my adding a check that they are
undocumented. There's no shortage of examples of what not to do.

>
> >>>> I still don’t get most of this .yaml stuff...
> >>>>
> >>>>>
> >>>>>> +      reg = <0x10000080 0x80>;
> >>>>>> +
> >>>>>> +      pinctrl: pinctrl {
> >>>>>> +        compatible = "brcm,bcm6328-pinctrl";
> >>>>>> +
> >>>>>> +        gpio {
> >>>>>> +          compatible = "brcm,bcm6328-gpio";
> >>>>>
> >>>>> I'm still trying to understand why you need 3 levels of nodes here?
> >>>>> The gpio controller contains a pin controller plus other undefined
> >>>>> functions (because of 'syscon') and the pin controller contains a gpio
> >>>>> controller?
> >>>>
> >>>> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
> >>>> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…
> >>>>
> >>>>>
> >>>>> I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
> >>>>> should be a single node.
> >>>>
> >>>> I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?
> >>>
> >>> It wouldn't be. I wasn't saying the pinctrl and gpio controller are
> >>> the same node. My suggestion was combining syscon and pinctrl.
> >>
> >> But that wouldn’t be correct if there were more “things” inside the syscon, right?
> >
> > Right.
> >
> >>>> Something like:
> >>>> syscon {
> >>>
> >>> Again with the syscon. If pinctrl and GPIO are the only functions
> >>> within this h/w block, then this is not a syscon. You are just abusing
> >>> that having 'syscon' compatible means you get a regmap created
> >>> automagically for you. Nothing here looks like a 'system controller'
> >>> to me. A 'system controller' is a random collection of register bits
> >>> with functions that don't fit anywhere else.
> >>
> >> pinctrl and GPIO aren’t the only functions within this HW block.
> >> Maybe I didn’t document/code it properly, but I’m sure I’m not abusing what a system controller is.
> >
> > Okay, that's the detail missing from this patch.
> >
> >> Please, take a look at http://www.datashed.science/misc/bcm/gpl/broadcom-sdk-416L05/shared/opensource/include/bcm963xx/6328_map_part.h:
> >> typedef struct GpioControl {
> >>      uint32      GPIODirHi;                  /* 0 */
> >>      uint32      GPIODir;                    /* 4 */
> >>      uint32      GPIOioHi;                   /* 8 */
> >>      uint32      GPIOio;                     /* C */
> >>      uint32      unused0;                    /* 10 */
> >>      uint32      SpiSlaveCfg;                /* 14 */
> >>      uint32      GPIOMode;                   /* 18 */
> >>      uint64      PinMuxSel;                  /* 1C */
> >>      uint32      PinMuxSelOther;             /* 24 */
> >>      uint32      TestControl;                /* 28 */
> >>      uint32      unused2;                    /* 2C */
> >>      uint32      RoboSWLEDControl;           /* 30 */
> >>      uint32      RoboSWLEDLSR;               /* 34 */
> >>      uint32      unused3;                    /* 38 */
> >>      uint32      RoboswEphyCtrl;             /* 3C */
> >>      uint32      RoboswSwitchCtrl;           /* 40 */
> >>      uint32      RegFileTmCtl;               /* 44 */
> >>      uint32      RingOscCtrl0;               /* 48 */
> >>      uint32      RingOscCtrl1;               /* 4C */
> >>      uint32      unused4[6];                 /* 50 - 64 */
> >>      uint32      DieRevID;                   /* 68 */
> >>      uint32      unused5;                    /* 6c */
> >>      uint32      DiagSelControl;             /* 70 */
> >>      uint32      DiagReadBack;               /* 74 */
> >>      uint32      DiagReadBackHi;             /* 78 */
> >>      uint32      DiagMiscControl;            /* 7c */
> >> } GpioControl;
> >>
> >> So we’re using GPIODirHi, GPIODir, GPIOioHi and GPIOio registers for GPIO regmap driver.
> >> And we’re using GPIOMode, PinMuxSel (u64 -> x2 u32), PinMuxSelOther for pinctrl driver.
> >> And this is for BCM6328, but some of the other SoCs are even more scattered.
> >
> > So based on this I'd do something like this:
> >
> > syscon {
> >    reg = <base 0x80>;
> >    ranges = <0 base 0x80>;
> >    pinctrl@18 {
> >      reg = <0x18 0x10>;
> >      foo-pins {};
> >    gpio@0 {
> >      reg = <0x0 0x10>;
> >    };
> > };
>
> You're missing a "};", so I'm not sure if you want me to go this way (1):
> syscon {
>         compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
>         reg = <0x10000080 0x80>;
>         ranges = <0 0x10000080 0x80>;
>
>         gpio: gpio@0 {
>                 compatible = "brcm,bcm6328-gpio";
>                 reg = <0x0 0x10>;
>
>                 data = <0xc>;
>                 dirout = <0x4>;
>
>                 gpio-controller;
>                 gpio-ranges = <&pinctrl 0 0 32>;
>                 #gpio-cells = <2>;
>         };
>
>         pinctrl: pinctrl@18 {
>                 compatible = "brcm,bcm6328-pinctrl";
>                 reg = <0x18 0x10>;
>
>                 foo-pins {};
>                 ...
>         };
> };

This way.

>
> Or this way (2):
> syscon {
>         compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
>         reg = <0x10000080 0x80>;
>         ranges = <0 0x10000080 0x80>;
>
>         pinctrl: pinctrl@18 {
>                 compatible = "brcm,bcm6328-pinctrl";
>                 reg = <0x0 0x28>;
>
>                 gpio: gpio@0 {

This doesn't make sense IMO because GPIO is not a sub-function of the
pinctrl h/w. They are peers.

>                         compatible = "brcm,bcm6328-gpio";
>                         reg = <0x0 0x10>;
>
>                         data = <0xc>;
>                         dirout = <0x4>;
>
>                         gpio-controller;
>                         gpio-ranges = <&pinctrl 0 0 32>;
>                         #gpio-cells = <2>;
>                 };
>
>                 foo-pins {};
>                 ...
>         };
> };
Álvaro Fernández Rojas March 11, 2021, 6:34 p.m. UTC | #9
Hi Rob,

> El 11 mar 2021, a las 19:14, Rob Herring <robh+dt@kernel.org> escribió:
> 
> On Thu, Mar 11, 2021 at 10:09 AM Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>> 
>> Hi Rob,
>> 
>> El 10/03/2021 a las 21:52, Rob Herring escribió:
>>> On Wed, Mar 10, 2021 at 12:10 PM Álvaro Fernández Rojas
>>> <noltari@gmail.com> wrote:
>>>> 
>>>> Hi Rob,
>>>> 
>>>>> El 10 mar 2021, a las 19:45, Rob Herring <robh+dt@kernel.org> escribió:
>>>>> 
>>>>> On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
>>>>> <noltari@gmail.com> wrote:
>>>>>> 
>>>>>> Hi Rob,
>>>>>> 
>>>>>>> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@kernel.org> escribió:
>>>>>>> 
>>>>>>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
>>>>>>> <noltari@gmail.com> wrote:
>>>>>>>> 
>>>>>>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>>>>>>>> 
>>>>>>>> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>>>>>> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
>>>>>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>>>>>> ---
>>>>>>>> v6: add changes suggested by Rob Herring
>>>>>>>> v5: change Documentation to dt-bindings in commit title
>>>>>>>> v4: no changes
>>>>>>>> v3: add new gpio node
>>>>>>>> v2: remove interrupts
>>>>>>>> 
>>>>>>>> .../pinctrl/brcm,bcm6328-pinctrl.yaml         | 174 ++++++++++++++++++
>>>>>>>> 1 file changed, 174 insertions(+)
>>>>>>>> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>>>>>> 
>>>>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..471f6efa1754
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
>>>>>>>> @@ -0,0 +1,174 @@
>>>>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>>>>> +%YAML 1.2
>>>>>>>> +---
>>>>>>>> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>> +
>>>>>>>> +title: Broadcom BCM6328 pin controller
>>>>>>>> +
>>>>>>>> +maintainers:
>>>>>>>> +  - Álvaro Fernández Rojas <noltari@gmail.com>
>>>>>>>> +  - Jonas Gorski <jonas.gorski@gmail.com>
>>>>>>>> +
>>>>>>>> +description: |+
>>>>>>>> +  The pin controller node should be the child of a syscon node.
>>>>>>>> +
>>>>>>>> +  Refer to the the bindings described in
>>>>>>>> +  Documentation/devicetree/bindings/mfd/syscon.yaml
>>>>>>>> +
>>>>>>>> +properties:
>>>>>>>> +  compatible:
>>>>>>>> +    const: brcm,bcm6328-pinctrl
>>>>>>>> +
>>>>>>>> +  gpio:
>>>>>>>> +    type: object
>>>>>>>> +    properties:
>>>>>>>> +      compatible:
>>>>>>>> +        const: brcm,bcm6328-gpio
>>>>>>>> +
>>>>>>>> +      data:
>>>>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>> +        description: |
>>>>>>>> +          Offset in the register map for the data register (in bytes).
>>>>>>>> +
>>>>>>>> +      dirout:
>>>>>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>> +        description: |
>>>>>>>> +          Offset in the register map for the dirout register (in bytes).
>>>>>>>> +
>>>>>>>> +      gpio-controller: true
>>>>>>>> +
>>>>>>>> +      "#gpio-cells":
>>>>>>>> +        const: 2
>>>>>>>> +
>>>>>>>> +      gpio-ranges:
>>>>>>>> +        maxItems: 1
>>>>>>>> +
>>>>>>>> +    required:
>>>>>>>> +      - gpio-controller
>>>>>>>> +      - gpio-ranges
>>>>>>>> +      - '#gpio-cells'
>>>>>>>> +
>>>>>>>> +    additionalProperties: false
>>>>>>>> +
>>>>>>>> +patternProperties:
>>>>>>>> +  '^.*-pins$':
>>>>>>>> +    if:
>>>>>>>> +      type: object
>>>>>>>> +    then:
>>>>>>>> +      properties:
>>>>>>>> +        function:
>>>>>>>> +          $ref: "pinmux-node.yaml#/properties/function"
>>>>>>>> +          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
>>>>>>>> +                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
>>>>>>>> +                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
>>>>>>>> +
>>>>>>>> +        pins:
>>>>>>>> +          $ref: "pinmux-node.yaml#/properties/pins"
>>>>>>>> +          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
>>>>>>>> +                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
>>>>>>>> +                  usb_port1 ]
>>>>>>>> +
>>>>>>>> +required:
>>>>>>>> +  - compatible
>>>>>>>> +  - gpio
>>>>>>>> +
>>>>>>>> +additionalProperties: false
>>>>>>>> +
>>>>>>>> +examples:
>>>>>>>> +  - |
>>>>>>>> +    gpio_cntl@10000080 {
>>>>>>>> +      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
>>>>>>> 
>>>>>>> You just added "brcm,bcm6328-gpio-controller", it would need to be documented.
>>>>>> 
>>>>>> I just added that because you requested me to do it ¯\_(ツ)_/¯
>>>>> 
>>>>> I said 'syscon' by itself was not allowed, then asked about the multiple levels.
>>>> 
>>>> Why not?
>>> 
>>> Because 'syscon' alone doesn't mean anything and doesn't describe what
>>> registers it contains. You need something that says this is the XYZ
>>> block in the ABC SoC.
>>> 
>>>> What if you have several controllers inside a syscon?
>>> 
>>> You either just add properties (e.g. just add #clock-cells and it's a
>>> clock provider) or you have child nodes. Which one you do generally
>>> depends on if the providers have DT resources themselves.
>>> 
>>>> The root should also have “something" in it?
>>>> 
>>>>> 
>>>>>> What should I do to document it?
>> 
>> You didn't answer my question about adding documentation...
> 
> You have to document it. Whether it's 1 or 3 schema files depends on
> where we end up, but the top-level one should reference the others if
> it's more than 1 file:
> 
> child-node:
>  type: object
>  $ref: "/schemas/foo/child-node.yaml#"

Can you give me an example of a driver which is documenting that?
I’m sorry, but I still don’t know how to do it properly...

> 
>> An example driver which adds a custom compatible string and doesn't
>> document it:
>> https://github.com/torvalds/linux/blob/a74e6a014c9d4d4161061f770c9b4f98372ac778/Documentation/devicetree/bindings/clock/sprd%2Csc9863a-clk.yaml#L90
> 
> *it Happens.
> 
> Those cases are now blocking my adding a check that they are
> undocumented. There's no shortage of examples of what not to do.
> 
>> 
>>>>>> I still don’t get most of this .yaml stuff...
>>>>>> 
>>>>>>> 
>>>>>>>> +      reg = <0x10000080 0x80>;
>>>>>>>> +
>>>>>>>> +      pinctrl: pinctrl {
>>>>>>>> +        compatible = "brcm,bcm6328-pinctrl";
>>>>>>>> +
>>>>>>>> +        gpio {
>>>>>>>> +          compatible = "brcm,bcm6328-gpio";
>>>>>>> 
>>>>>>> I'm still trying to understand why you need 3 levels of nodes here?
>>>>>>> The gpio controller contains a pin controller plus other undefined
>>>>>>> functions (because of 'syscon') and the pin controller contains a gpio
>>>>>>> controller?
>>>>>> 
>>>>>> In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers.
>>>>>> However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new…
>>>>>> 
>>>>>>> 
>>>>>>> I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl"
>>>>>>> should be a single node.
>>>>>> 
>>>>>> I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself?
>>>>> 
>>>>> It wouldn't be. I wasn't saying the pinctrl and gpio controller are
>>>>> the same node. My suggestion was combining syscon and pinctrl.
>>>> 
>>>> But that wouldn’t be correct if there were more “things” inside the syscon, right?
>>> 
>>> Right.
>>> 
>>>>>> Something like:
>>>>>> syscon {
>>>>> 
>>>>> Again with the syscon. If pinctrl and GPIO are the only functions
>>>>> within this h/w block, then this is not a syscon. You are just abusing
>>>>> that having 'syscon' compatible means you get a regmap created
>>>>> automagically for you. Nothing here looks like a 'system controller'
>>>>> to me. A 'system controller' is a random collection of register bits
>>>>> with functions that don't fit anywhere else.
>>>> 
>>>> pinctrl and GPIO aren’t the only functions within this HW block.
>>>> Maybe I didn’t document/code it properly, but I’m sure I’m not abusing what a system controller is.
>>> 
>>> Okay, that's the detail missing from this patch.
>>> 
>>>> Please, take a look at http://www.datashed.science/misc/bcm/gpl/broadcom-sdk-416L05/shared/opensource/include/bcm963xx/6328_map_part.h:
>>>> typedef struct GpioControl {
>>>>     uint32      GPIODirHi;                  /* 0 */
>>>>     uint32      GPIODir;                    /* 4 */
>>>>     uint32      GPIOioHi;                   /* 8 */
>>>>     uint32      GPIOio;                     /* C */
>>>>     uint32      unused0;                    /* 10 */
>>>>     uint32      SpiSlaveCfg;                /* 14 */
>>>>     uint32      GPIOMode;                   /* 18 */
>>>>     uint64      PinMuxSel;                  /* 1C */
>>>>     uint32      PinMuxSelOther;             /* 24 */
>>>>     uint32      TestControl;                /* 28 */
>>>>     uint32      unused2;                    /* 2C */
>>>>     uint32      RoboSWLEDControl;           /* 30 */
>>>>     uint32      RoboSWLEDLSR;               /* 34 */
>>>>     uint32      unused3;                    /* 38 */
>>>>     uint32      RoboswEphyCtrl;             /* 3C */
>>>>     uint32      RoboswSwitchCtrl;           /* 40 */
>>>>     uint32      RegFileTmCtl;               /* 44 */
>>>>     uint32      RingOscCtrl0;               /* 48 */
>>>>     uint32      RingOscCtrl1;               /* 4C */
>>>>     uint32      unused4[6];                 /* 50 - 64 */
>>>>     uint32      DieRevID;                   /* 68 */
>>>>     uint32      unused5;                    /* 6c */
>>>>     uint32      DiagSelControl;             /* 70 */
>>>>     uint32      DiagReadBack;               /* 74 */
>>>>     uint32      DiagReadBackHi;             /* 78 */
>>>>     uint32      DiagMiscControl;            /* 7c */
>>>> } GpioControl;
>>>> 
>>>> So we’re using GPIODirHi, GPIODir, GPIOioHi and GPIOio registers for GPIO regmap driver.
>>>> And we’re using GPIOMode, PinMuxSel (u64 -> x2 u32), PinMuxSelOther for pinctrl driver.
>>>> And this is for BCM6328, but some of the other SoCs are even more scattered.
>>> 
>>> So based on this I'd do something like this:
>>> 
>>> syscon {
>>>   reg = <base 0x80>;
>>>   ranges = <0 base 0x80>;
>>>   pinctrl@18 {
>>>     reg = <0x18 0x10>;
>>>     foo-pins {};
>>>   gpio@0 {
>>>     reg = <0x0 0x10>;
>>>   };
>>> };
>> 
>> You're missing a "};", so I'm not sure if you want me to go this way (1):
>> syscon {
>>        compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
>>        reg = <0x10000080 0x80>;
>>        ranges = <0 0x10000080 0x80>;
>> 
>>        gpio: gpio@0 {
>>                compatible = "brcm,bcm6328-gpio";
>>                reg = <0x0 0x10>;
>> 
>>                data = <0xc>;
>>                dirout = <0x4>;
>> 
>>                gpio-controller;
>>                gpio-ranges = <&pinctrl 0 0 32>;
>>                #gpio-cells = <2>;
>>        };
>> 
>>        pinctrl: pinctrl@18 {
>>                compatible = "brcm,bcm6328-pinctrl";
>>                reg = <0x18 0x10>;
>> 
>>                foo-pins {};
>>                ...
>>        };
>> };
> 
> This way.
> 
>> 
>> Or this way (2):
>> syscon {
>>        compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
>>        reg = <0x10000080 0x80>;
>>        ranges = <0 0x10000080 0x80>;
>> 
>>        pinctrl: pinctrl@18 {
>>                compatible = "brcm,bcm6328-pinctrl";
>>                reg = <0x0 0x28>;
>> 
>>                gpio: gpio@0 {
> 
> This doesn't make sense IMO because GPIO is not a sub-function of the
> pinctrl h/w. They are peers.

Well, that’s where @Linus and I disagree...

> 
>>                        compatible = "brcm,bcm6328-gpio";
>>                        reg = <0x0 0x10>;
>> 
>>                        data = <0xc>;
>>                        dirout = <0x4>;
>> 
>>                        gpio-controller;
>>                        gpio-ranges = <&pinctrl 0 0 32>;
>>                        #gpio-cells = <2>;
>>                };
>> 
>>                foo-pins {};
>>                ...
>>        };
>> };
Linus Walleij March 15, 2021, 3:57 p.m. UTC | #10
On Thu, Mar 11, 2021 at 7:14 PM Rob Herring <robh+dt@kernel.org> wrote:

> > Or this way (2):
> > syscon {
> >         compatible = "brcm,bcm6328-gpio-regs", "syscon", "simple-mfd";
> >         reg = <0x10000080 0x80>;
> >         ranges = <0 0x10000080 0x80>;
> >
> >         pinctrl: pinctrl@18 {
> >                 compatible = "brcm,bcm6328-pinctrl";
> >                 reg = <0x0 0x28>;
> >
> >                 gpio: gpio@0 {
>
> This doesn't make sense IMO because GPIO is not a sub-function of the
> pinctrl h/w. They are peers.

This becomes an ontological discussion, as in "what does the world
consist of and what are the proper definitions of the
things in it".

A couple of years back I had this presentation:
https://dflund.se/~triad/papers/pincontrol.pdf
where I try to investigate how hardware engineers build
these blocks.

TL;DR: it depends on what the hardware engineer
did.

A HW block can be pin controller, GPIO controller
and interrupt chip at the same time, that case is
straight-forward. One compatible, lots of
properties.
.
A second case is when the pin controller and the
GPIO+irqchip are two completely different HW
entities, and then they also get two different
device nodes on the same level in the device tree.
(We usually see this when the different blocks
live in totally different memory locations.)

However in the third case HW can also be bolted
with a front-end pin controller (facing the pins) with
several GPIO+interrupt controller back-ends.
Then it gets the structure in this patch,
subnodes for each GPIO controller.

Our current bindings have all three examples
and it simply reflects the different ways HW
engineers have chosen to integrate their stuff.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
new file mode 100644
index 000000000000..471f6efa1754
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml
@@ -0,0 +1,174 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom BCM6328 pin controller
+
+maintainers:
+  - Álvaro Fernández Rojas <noltari@gmail.com>
+  - Jonas Gorski <jonas.gorski@gmail.com>
+
+description: |+
+  The pin controller node should be the child of a syscon node.
+
+  Refer to the the bindings described in
+  Documentation/devicetree/bindings/mfd/syscon.yaml
+
+properties:
+  compatible:
+    const: brcm,bcm6328-pinctrl
+
+  gpio:
+    type: object
+    properties:
+      compatible:
+        const: brcm,bcm6328-gpio
+
+      data:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          Offset in the register map for the data register (in bytes).
+
+      dirout:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          Offset in the register map for the dirout register (in bytes).
+
+      gpio-controller: true
+
+      "#gpio-cells":
+        const: 2
+
+      gpio-ranges:
+        maxItems: 1
+
+    required:
+      - gpio-controller
+      - gpio-ranges
+      - '#gpio-cells'
+
+    additionalProperties: false
+
+patternProperties:
+  '^.*-pins$':
+    if:
+      type: object
+    then:
+      properties:
+        function:
+          $ref: "pinmux-node.yaml#/properties/function"
+          enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq,
+                  led, ephy0_act_led, ephy1_act_led, ephy2_act_led,
+                  ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ]
+
+        pins:
+          $ref: "pinmux-node.yaml#/properties/pins"
+          enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19,
+                  gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1,
+                  usb_port1 ]
+
+required:
+  - compatible
+  - gpio
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio_cntl@10000080 {
+      compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd";
+      reg = <0x10000080 0x80>;
+
+      pinctrl: pinctrl {
+        compatible = "brcm,bcm6328-pinctrl";
+
+        gpio {
+          compatible = "brcm,bcm6328-gpio";
+          data = <0xc>;
+          dirout = <0x4>;
+
+          gpio-controller;
+          gpio-ranges = <&pinctrl 0 0 32>;
+          #gpio-cells = <2>;
+        };
+
+        pinctrl_serial_led: serial_led-pins {
+          pinctrl_serial_led_data: serial_led_data-pins {
+            function = "serial_led_data";
+            pins = "gpio6";
+          };
+
+          pinctrl_serial_led_clk: serial_led_clk-pins {
+            function = "serial_led_clk";
+            pins = "gpio7";
+          };
+        };
+
+        pinctrl_inet_act_led: inet_act_led-pins {
+          function = "inet_act_led";
+          pins = "gpio11";
+        };
+
+        pinctrl_pcie_clkreq: pcie_clkreq-pins {
+          function = "pcie_clkreq";
+          pins = "gpio16";
+        };
+
+        pinctrl_ephy0_spd_led: ephy0_spd_led-pins {
+          function = "led";
+          pins = "gpio17";
+        };
+
+        pinctrl_ephy1_spd_led: ephy1_spd_led-pins {
+          function = "led";
+          pins = "gpio18";
+        };
+
+        pinctrl_ephy2_spd_led: ephy2_spd_led-pins {
+          function = "led";
+          pins = "gpio19";
+        };
+
+        pinctrl_ephy3_spd_led: ephy3_spd_led-pins {
+          function = "led";
+          pins = "gpio20";
+        };
+
+        pinctrl_ephy0_act_led: ephy0_act_led-pins {
+          function = "ephy0_act_led";
+          pins = "gpio25";
+        };
+
+        pinctrl_ephy1_act_led: ephy1_act_led-pins {
+          function = "ephy1_act_led";
+          pins = "gpio26";
+        };
+
+        pinctrl_ephy2_act_led: ephy2_act_led-pins {
+          function = "ephy2_act_led";
+          pins = "gpio27";
+        };
+
+        pinctrl_ephy3_act_led: ephy3_act_led-pins {
+          function = "ephy3_act_led";
+          pins = "gpio28";
+        };
+
+        pinctrl_hsspi_cs1: hsspi_cs1-pins {
+          function = "hsspi_cs1";
+          pins = "hsspi_cs1";
+        };
+
+        pinctrl_usb_port1_device: usb_port1_device-pins {
+          function = "usb_device_port";
+          pins = "usb_port1";
+        };
+
+        pinctrl_usb_port1_host: usb_port1_host-pins {
+          function = "usb_host_port";
+          pins = "usb_port1";
+        };
+      };
+    };