| 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 |
> + 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/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
> 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
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
(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
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
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
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
> 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
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"; + }; + }; + };
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