diff mbox series

[v2,1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

Message ID 20220314213143.2404162-2-chris.packham@alliedtelesis.co.nz (mailing list archive)
State New, archived
Headers show
Series arm64: mvebu: Support for Marvell 98DX2530 (and variants) | expand

Commit Message

Chris Packham March 14, 2022, 9:31 p.m. UTC
Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
SoC.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - Remove syscon and simple-mfd compatibles

 .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml

Comments

Andrew Lunn March 15, 2022, 12:07 a.m. UTC | #1
> +    properties:
> +      marvell,function:
> +        $ref: "/schemas/types.yaml#/definitions/string"
> +        description:
> +          Indicates the function to select.
> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
> +
> +      marvell,pins:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        description:
> +          Array of MPP pins to be used for the given function.
> +        minItems: 1

Now that i've looked at the .txt files, i'm wondering if this should
be split into a marvell,mvebu-pinctrl.yaml and
marvell,ac5-pinctrl.yaml?

I don't know yaml well enough to know if this is possible. All the
mvebu pinctrl drivers have marvell,function and marvell,pins. The enum
will differ, this ethernet switch SoC does not have sata, audio etc,
where as the general purpose Socs do. Can that be represented in yaml?

      Andrew
Chris Packham March 15, 2022, 12:22 a.m. UTC | #2
On 15/03/22 13:07, Andrew Lunn wrote:
>> +    properties:
>> +      marvell,function:
>> +        $ref: "/schemas/types.yaml#/definitions/string"
>> +        description:
>> +          Indicates the function to select.
>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>> +
>> +      marvell,pins:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        description:
>> +          Array of MPP pins to be used for the given function.
>> +        minItems: 1
> Now that i've looked at the .txt files, i'm wondering if this should
> be split into a marvell,mvebu-pinctrl.yaml and
> marvell,ac5-pinctrl.yaml?
>
> I don't know yaml well enough to know if this is possible. All the
> mvebu pinctrl drivers have marvell,function and marvell,pins. The enum
> will differ, this ethernet switch SoC does not have sata, audio etc,
> where as the general purpose Socs do. Can that be represented in yaml?

I think it can. I vaguely remember seeing conditional clauses based on 
compatible strings in other yaml bindings.

I started a new binding document because I expected adding significant 
additions to the existing .txt files would be rejected. If I get some 
cycles I could look at converting the existing docs from txt to yaml.

I'm not sure that there will be much in the way of a common 
mvebu-pinctrl.yaml as you'd end up repeating most of the common stuff to 
make things conditional anyway.

>
>        Andrew
Andrew Lunn March 15, 2022, 12:27 a.m. UTC | #3
> I think it can. I vaguely remember seeing conditional clauses based on 
> compatible strings in other yaml bindings.
> 
> I started a new binding document because I expected adding significant 
> additions to the existing .txt files would be rejected. If I get some 
> cycles I could look at converting the existing docs from txt to yaml.
> 
> I'm not sure that there will be much in the way of a common 
> mvebu-pinctrl.yaml as you'd end up repeating most of the common stuff to 
> make things conditional anyway.

We should wait for Rob to comment. But is suspect you are right, there
will not be much savings.

     Andrew
Krzysztof Kozlowski March 15, 2022, 10:46 a.m. UTC | #4
On 14/03/2022 22:31, Chris Packham wrote:
> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
> SoC.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Remove syscon and simple-mfd compatibles
> 
>  .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
> new file mode 100644
> index 000000000000..65af1d5f5fe0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/marvell,ac5-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell AC5 pin controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description:
> +  Bindings for Marvell's AC5 memory-mapped pin controller.
> +
> +properties:
> +  compatible:
> +    const: marvell,ac5-pinctrl
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    $ref: pinmux-node.yaml#
> +
> +    properties:
> +      marvell,function:
> +        $ref: "/schemas/types.yaml#/definitions/string"
> +        description:
> +          Indicates the function to select.
> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
> +
> +      marvell,pins:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        description:
> +          Array of MPP pins to be used for the given function.
> +        minItems: 1
> +        items:
> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
> +
> +allOf:
> +  - $ref: "pinctrl.yaml#"
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    system-controller@80020100 {
> +      compatible = "syscon", "simple-mfd";
> +      reg = <0x80020000 0x20>;

This is unusual. Usually the pinctrl should be a device @80020100, not
child of syscon node. Why do you need it? In v1 you mentioned that
vendor sources do like this, but it's not correct to copy wrong DTS. :)



Best regards,
Krzysztof
Chris Packham March 15, 2022, 9:12 p.m. UTC | #5
(trimmed cc list to the arm, pinctrl and dt people)

On 15/03/22 23:46, Krzysztof Kozlowski wrote:
> On 14/03/2022 22:31, Chris Packham wrote:
>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>> SoC.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      - Remove syscon and simple-mfd compatibles
>>
>>   .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..65af1d5f5fe0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>> @@ -0,0 +1,70 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19ce8eGP0QA&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19cPrfjTyTg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell AC5 pin controller
>> +
>> +maintainers:
>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +
>> +description:
>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>> +
>> +properties:
>> +  compatible:
>> +    const: marvell,ac5-pinctrl
>> +
>> +patternProperties:
>> +  '-pins$':
>> +    type: object
>> +    $ref: pinmux-node.yaml#
>> +
>> +    properties:
>> +      marvell,function:
>> +        $ref: "/schemas/types.yaml#/definitions/string"
>> +        description:
>> +          Indicates the function to select.
>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>> +
>> +      marvell,pins:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        description:
>> +          Array of MPP pins to be used for the given function.
>> +        minItems: 1
>> +        items:
>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>> +
>> +allOf:
>> +  - $ref: "pinctrl.yaml#"
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    system-controller@80020100 {
>> +      compatible = "syscon", "simple-mfd";
>> +      reg = <0x80020000 0x20>;
> This is unusual. Usually the pinctrl should be a device @80020100, not
> child of syscon node. Why do you need it? In v1 you mentioned that
> vendor sources do like this, but it's not correct to copy wrong DTS. :)

The vendor dts has this

         pinctrl0: pinctrl@80020100 {
             compatible = "marvell,ac5-pinctrl",
                      "syscon", "simple-mfd";
             reg = <0 0x80020100 0 0x20>;
             i2c_mpps: i2c-mpps {
                 marvell,pins = "mpp26", "mpp27";
                 marvell,function = "i2c0-opt";
             };
      };

Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking 
and found marvell,armada-7k-pinctrl which has the pinctrl as a child of 
a syscon node and what you see in v2 is the result.

I probably went a bit too far off the deep end and should have just 
dropped the "syscon", "simple-mfd" compatibles. I even wrote that 
version but decided to add some gold plating before I submitted it.

>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski March 16, 2022, 8:16 a.m. UTC | #6
On 15/03/2022 22:12, Chris Packham wrote:
> (trimmed cc list to the arm, pinctrl and dt people)
> 
> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>> On 14/03/2022 22:31, Chris Packham wrote:
>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>> SoC.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Notes:
>>>      Changes in v2:
>>>      - Remove syscon and simple-mfd compatibles
>>>
>>>   .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>   1 file changed, 70 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>> new file mode 100644
>>> index 000000000000..65af1d5f5fe0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>> @@ -0,0 +1,70 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19ce8eGP0QA&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=vu6w4lGvpbdx5x7Y5wSGMQ_aPa00Bnj19cPrfjTyTg&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>> +
>>> +title: Marvell AC5 pin controller
>>> +
>>> +maintainers:
>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> +
>>> +description:
>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: marvell,ac5-pinctrl
>>> +
>>> +patternProperties:
>>> +  '-pins$':
>>> +    type: object
>>> +    $ref: pinmux-node.yaml#
>>> +
>>> +    properties:
>>> +      marvell,function:
>>> +        $ref: "/schemas/types.yaml#/definitions/string"
>>> +        description:
>>> +          Indicates the function to select.
>>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>>> +
>>> +      marvell,pins:
>>> +        $ref: /schemas/types.yaml#/definitions/string-array
>>> +        description:
>>> +          Array of MPP pins to be used for the given function.
>>> +        minItems: 1
>>> +        items:
>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>> +
>>> +allOf:
>>> +  - $ref: "pinctrl.yaml#"
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    system-controller@80020100 {
>>> +      compatible = "syscon", "simple-mfd";
>>> +      reg = <0x80020000 0x20>;
>> This is unusual. Usually the pinctrl should be a device @80020100, not
>> child of syscon node. Why do you need it? In v1 you mentioned that
>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
> 
> The vendor dts has this
> 
>          pinctrl0: pinctrl@80020100 {
>              compatible = "marvell,ac5-pinctrl",
>                       "syscon", "simple-mfd";
>              reg = <0 0x80020100 0 0x20>;
>              i2c_mpps: i2c-mpps {
>                  marvell,pins = "mpp26", "mpp27";
>                  marvell,function = "i2c0-opt";
>              };
>       };
> 
> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking 
> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of 
> a syscon node and what you see in v2 is the result.
> 
> I probably went a bit too far off the deep end and should have just 
> dropped the "syscon", "simple-mfd" compatibles. I even wrote that 
> version but decided to add some gold plating before I submitted it.

More or less it is explained in
Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
armada-7k uses it that way. The pinctrl is part of system registers
which apparently has to be shared with others (on shared SFR range).

It depends on your case, your SFR ranges for pinctrl and other blocks.


Best regards,
Krzysztof
Chris Packham March 16, 2022, 8:21 p.m. UTC | #7
On 16/03/22 21:16, Krzysztof Kozlowski wrote:
> On 15/03/2022 22:12, Chris Packham wrote:
>> (trimmed cc list to the arm, pinctrl and dt people)
>>
>> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>>> On 14/03/2022 22:31, Chris Packham wrote:
>>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>>> SoC.
>>>>
>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> ---
>>>>
>>>> Notes:
>>>>       Changes in v2:
>>>>       - Remove syscon and simple-mfd compatibles
>>>>
>>>>    .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>>    1 file changed, 70 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>> new file mode 100644
>>>> index 000000000000..65af1d5f5fe0
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>> @@ -0,0 +1,70 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6CXu-YC1w&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6TAvbEE2Q&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>>> +
>>>> +title: Marvell AC5 pin controller
>>>> +
>>>> +maintainers:
>>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>> +
>>>> +description:
>>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: marvell,ac5-pinctrl
>>>> +
>>>> +patternProperties:
>>>> +  '-pins$':
>>>> +    type: object
>>>> +    $ref: pinmux-node.yaml#
>>>> +
>>>> +    properties:
>>>> +      marvell,function:
>>>> +        $ref: "/schemas/types.yaml#/definitions/string"
>>>> +        description:
>>>> +          Indicates the function to select.
>>>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>>>> +
>>>> +      marvell,pins:
>>>> +        $ref: /schemas/types.yaml#/definitions/string-array
>>>> +        description:
>>>> +          Array of MPP pins to be used for the given function.
>>>> +        minItems: 1
>>>> +        items:
>>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>>> +
>>>> +allOf:
>>>> +  - $ref: "pinctrl.yaml#"
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    system-controller@80020100 {
>>>> +      compatible = "syscon", "simple-mfd";
>>>> +      reg = <0x80020000 0x20>;
>>> This is unusual. Usually the pinctrl should be a device @80020100, not
>>> child of syscon node. Why do you need it? In v1 you mentioned that
>>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
>> The vendor dts has this
>>
>>           pinctrl0: pinctrl@80020100 {
>>               compatible = "marvell,ac5-pinctrl",
>>                        "syscon", "simple-mfd";
>>               reg = <0 0x80020100 0 0x20>;
>>               i2c_mpps: i2c-mpps {
>>                   marvell,pins = "mpp26", "mpp27";
>>                   marvell,function = "i2c0-opt";
>>               };
>>        };
>>
>> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking
>> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of
>> a syscon node and what you see in v2 is the result.
>>
>> I probably went a bit too far off the deep end and should have just
>> dropped the "syscon", "simple-mfd" compatibles. I even wrote that
>> version but decided to add some gold plating before I submitted it.
> More or less it is explained in
> Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
> armada-7k uses it that way. The pinctrl is part of system registers
> which apparently has to be shared with others (on shared SFR range).
>
> It depends on your case, your SFR ranges for pinctrl and other blocks.
>
I can tell you that without a syscon node in the mix somewhere the 
driver will fail to load. And when I switch to 
mvebu_pinctrl_simple_mmio_probe() the driver loads but then kernel 
panics when something tries to use one of the pin functions.

So I think the syscon is needed. I just need to come up with a better 
justification than "because it's needed".

> Best regards,
> Krzysztof
Krzysztof Kozlowski March 17, 2022, 7:26 a.m. UTC | #8
On 16/03/2022 21:21, Chris Packham wrote:
> 
> On 16/03/22 21:16, Krzysztof Kozlowski wrote:
>> On 15/03/2022 22:12, Chris Packham wrote:
>>> (trimmed cc list to the arm, pinctrl and dt people)
>>>
>>> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>>>> On 14/03/2022 22:31, Chris Packham wrote:
>>>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>>>> SoC.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>       Changes in v2:
>>>>>       - Remove syscon and simple-mfd compatibles
>>>>>
>>>>>    .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>>>    1 file changed, 70 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..65af1d5f5fe0
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>> @@ -0,0 +1,70 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6CXu-YC1w&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6TAvbEE2Q&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>>>> +
>>>>> +title: Marvell AC5 pin controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> +
>>>>> +description:
>>>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: marvell,ac5-pinctrl
>>>>> +
>>>>> +patternProperties:
>>>>> +  '-pins$':
>>>>> +    type: object
>>>>> +    $ref: pinmux-node.yaml#
>>>>> +
>>>>> +    properties:
>>>>> +      marvell,function:
>>>>> +        $ref: "/schemas/types.yaml#/definitions/string"
>>>>> +        description:
>>>>> +          Indicates the function to select.
>>>>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>>>>> +
>>>>> +      marvell,pins:
>>>>> +        $ref: /schemas/types.yaml#/definitions/string-array
>>>>> +        description:
>>>>> +          Array of MPP pins to be used for the given function.
>>>>> +        minItems: 1
>>>>> +        items:
>>>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: "pinctrl.yaml#"
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    system-controller@80020100 {
>>>>> +      compatible = "syscon", "simple-mfd";
>>>>> +      reg = <0x80020000 0x20>;
>>>> This is unusual. Usually the pinctrl should be a device @80020100, not
>>>> child of syscon node. Why do you need it? In v1 you mentioned that
>>>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
>>> The vendor dts has this
>>>
>>>           pinctrl0: pinctrl@80020100 {
>>>               compatible = "marvell,ac5-pinctrl",
>>>                        "syscon", "simple-mfd";
>>>               reg = <0 0x80020100 0 0x20>;
>>>               i2c_mpps: i2c-mpps {
>>>                   marvell,pins = "mpp26", "mpp27";
>>>                   marvell,function = "i2c0-opt";
>>>               };
>>>        };
>>>
>>> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking
>>> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of
>>> a syscon node and what you see in v2 is the result.
>>>
>>> I probably went a bit too far off the deep end and should have just
>>> dropped the "syscon", "simple-mfd" compatibles. I even wrote that
>>> version but decided to add some gold plating before I submitted it.
>> More or less it is explained in
>> Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
>> armada-7k uses it that way. The pinctrl is part of system registers
>> which apparently has to be shared with others (on shared SFR range).
>>
>> It depends on your case, your SFR ranges for pinctrl and other blocks.
>>
> I can tell you that without a syscon node in the mix somewhere the 
> driver will fail to load. And when I switch to 
> mvebu_pinctrl_simple_mmio_probe() the driver loads but then kernel 
> panics when something tries to use one of the pin functions.
> 
> So I think the syscon is needed. I just need to come up with a better 
> justification than "because it's needed".

What do you mean "driver fails to load"? You control the driver, don't
you? You wrote it? If you write a driver which is not compatible with
bindings, it won't work obviously, so after changing bindings you need
to revisit the driver.

There is no need for syscon because driver "fails to load". You need to
fix your driver. Currently the driver code is definitely not a proper
platform driver.

Different question is whether something else requires here syscon
because it accesses these registers but this requires knowledge of
architecture and other components.


Best regards,
Krzysztof
Andrew Lunn March 17, 2022, 2:14 p.m. UTC | #9
> What do you mean "driver fails to load"? You control the driver, don't
> you?

It is a thin wrapper around the mvebu driver, which does all the real
work. So no, Chris does not really control what the core of the driver
does.

The existing binding documentation says:

    * Marvell Armada 37xx SoC pin and gpio controller

    Each Armada 37xx SoC come with two pin and gpio controller one for
    the south bridge and the other for the north bridge.

    Inside this set of register the gpio latch allows exposing some
    configuration of the SoC and especially the clock frequency of the
    xtal. Hence, this node is a represent as syscon allowing sharing
    the register between multiple hardware block.


So the syscon is there to allow the clock driver to share the register
space.

	Andrew
Krzysztof Kozlowski March 17, 2022, 3:16 p.m. UTC | #10
On 17/03/2022 15:14, Andrew Lunn wrote:
>> What do you mean "driver fails to load"? You control the driver, don't
>> you?
> 
> It is a thin wrapper around the mvebu driver, which does all the real
> work. So no, Chris does not really control what the core of the driver
> does.

This this design still require a pinctrl to be a child of some node?

> 
> The existing binding documentation says:
> 
>     * Marvell Armada 37xx SoC pin and gpio controller
> 
>     Each Armada 37xx SoC come with two pin and gpio controller one for
>     the south bridge and the other for the north bridge.
> 
>     Inside this set of register the gpio latch allows exposing some
>     configuration of the SoC and especially the clock frequency of the
>     xtal. Hence, this node is a represent as syscon allowing sharing
>     the register between multiple hardware block.
> 
> 
> So the syscon is there to allow the clock driver to share the register
> space.


This makes sense. Solution here would be to add syscon compatible to
pinctrl node. This parent simple-mfd+syscon node looks like a workaround
to share some registers in a highly flexible way. However isn't it
better to have more obvious owner of the register space (e.g. pinctrl)?
IOW, if there is only one child of syscon+simple-mfd node, why not
getting rid of it and making pinctrl owner of this address space? It's
also simpler code.


Best regards,
Krzysztof
Rob Herring (Arm) March 23, 2022, 6:34 p.m. UTC | #11
On Tue, Mar 15, 2022 at 01:27:48AM +0100, Andrew Lunn wrote:
> > I think it can. I vaguely remember seeing conditional clauses based on 
> > compatible strings in other yaml bindings.
> > 
> > I started a new binding document because I expected adding significant 
> > additions to the existing .txt files would be rejected. If I get some 
> > cycles I could look at converting the existing docs from txt to yaml.
> > 
> > I'm not sure that there will be much in the way of a common 
> > mvebu-pinctrl.yaml as you'd end up repeating most of the common stuff to 
> > make things conditional anyway.
> 
> We should wait for Rob to comment. But is suspect you are right, there
> will not be much savings.

It's always a judgement call of amount of if/then schema vs. duplicating 
the common parts. If it's the function/pin parts that vary, then that's 
probably best as separate schema for each case. Otherwise, I'm not sure 
without seeing something.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
new file mode 100644
index 000000000000..65af1d5f5fe0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
@@ -0,0 +1,70 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/marvell,ac5-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell AC5 pin controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description:
+  Bindings for Marvell's AC5 memory-mapped pin controller.
+
+properties:
+  compatible:
+    const: marvell,ac5-pinctrl
+
+patternProperties:
+  '-pins$':
+    type: object
+    $ref: pinmux-node.yaml#
+
+    properties:
+      marvell,function:
+        $ref: "/schemas/types.yaml#/definitions/string"
+        description:
+          Indicates the function to select.
+        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
+
+      marvell,pins:
+        $ref: /schemas/types.yaml#/definitions/string-array
+        description:
+          Array of MPP pins to be used for the given function.
+        minItems: 1
+        items:
+          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
+                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
+                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
+                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
+                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
+
+allOf:
+  - $ref: "pinctrl.yaml#"
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    system-controller@80020100 {
+      compatible = "syscon", "simple-mfd";
+      reg = <0x80020000 0x20>;
+
+      pinctrl0: pinctrl {
+        compatible = "marvell,ac5-pinctrl";
+
+        i2c0_pins: i2c0-pins {
+          marvell,pins = "mpp26", "mpp27";
+          marvell,function = "i2c0";
+        };
+
+        i2c0_gpio: i2c0-gpio-pins {
+          marvell,pins = "mpp26", "mpp27";
+          marvell,function = "gpio";
+        };
+      };
+    };