by Álvaro Fernández Rojas

[permalink] [raw]

Subject: Re: [PATCH v6 04/15] dt-bindings: add BCM6328 pincontroller binding documentation

Hi Rob,

> El 11 mar 2021, a las 19:14, Rob Herring <[email protected]> escribió:
>
> On Thu, Mar 11, 2021 at 10:09 AM Álvaro Fernández Rojas
> <[email protected]> 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
>>> <[email protected]> wrote:
>>>>
>>>> Hi Rob,
>>>>
>>>>> El 10 mar 2021, a las 19:45, Rob Herring <[email protected]> escribió:
>>>>>
>>>>> On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Hi Rob,
>>>>>>
>>>>>>> El 10 mar 2021, a las 18:45, Rob Herring <[email protected]> escribió:
>>>>>>>
>>>>>>> On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
>>>>>>> <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Add binding documentation for the pincontrol core found in BCM6328 SoCs.
>>>>>>>>
>>>>>>>> Signed-off-by: Jonas Gorski <[email protected]>
>>>>>>>> Co-developed-by: Jonas Gorski <[email protected]>
>>>>>>>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>>>>>>>> ---
>>>>>>>> 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 <[email protected]>
>>>>>>>> + - Jonas Gorski <[email protected]>
>>>>>>>> +
>>>>>>>> +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 {};
>> ...
>> };
>> };

2021-03-15 15:59:26

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v6 04/15] dt-bindings: add BCM6328 pincontroller binding documentation

On Thu, Mar 11, 2021 at 7:14 PM Rob Herring <[email protected]> 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