Hi Rob, > El 10 mar 2021, a las 19:45, Rob Herring <robh+dt@xxxxxxxxxx> escribió: > > On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas > <noltari@xxxxxxxxx> wrote: >> >> Hi Rob, >> >>> El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@xxxxxxxxxx> escribió: >>> >>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas >>> <noltari@xxxxxxxxx> wrote: >>>> >>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs. >>>> >>>> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx> >>>> Co-developed-by: Jonas Gorski <jonas.gorski@xxxxxxxxx> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxx> >>>> + - Jonas Gorski <jonas.gorski@xxxxxxxxx> >>>> + >>>> +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.
