Hi Rob, > El 11 mar 2021, a las 19:14, Rob Herring <robh+dt@xxxxxxxxxx> escribió: > > On Thu, Mar 11, 2021 at 10:09 AM Álvaro Fernández Rojas > <noltari@xxxxxxxxx> 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@xxxxxxxxx> wrote: >>>> >>>> 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? >>> >>> 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 {}; >> ... >> }; >> };
