| Message ID | 20210310125504.31886-5-noltari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: add BCM63XX pincontrol support | expand |
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
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.
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
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.
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
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
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.
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 {}; > ... > }; > };
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 {}; >> ... >> }; >> };
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 --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"; + }; + }; + };