Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
SoC.
Signed-off-by: Chris Packham <[email protected]>
---
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 <[email protected]>
+
+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";
+ };
+ };
+ };
--
2.35.1
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
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 <[email protected]>
>>>> ---
>>>>
>>>> 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 <[email protected]>
>>>> +
>>>> +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
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 <[email protected]>
> ---
>
> 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 <[email protected]>
> +
> +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
> 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
> + 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
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 <[email protected]>
>>> ---
>>>
>>> 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 <[email protected]>
>>> +
>>> +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
(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 <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>> +
>> +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
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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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 <[email protected]>
>>>>> +
>>>>> +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
> 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
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
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