From: Chris Packham <Chris.Packham@alliedtelesis.co.nz> To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>, "robh+dt@kernel.org" <robh+dt@kernel.org>, "linus.walleij@linaro.org" <linus.walleij@linaro.org>, "catalin.marinas@arm.com" <catalin.marinas@arm.com>, "andrew@lunn.ch" <andrew@lunn.ch>, "gregory.clement@bootlin.com" <gregory.clement@bootlin.com>, "sebastian.hesselbarth@gmail.com" <sebastian.hesselbarth@gmail.com> Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5 Date: Wed, 16 Mar 2022 20:21:58 +0000 [thread overview] Message-ID: <6d902e7d-b71f-9dcd-9175-cc706e3d60cc@alliedtelesis.co.nz> (raw) In-Reply-To: <cb0af80e-3e5a-fbd9-cd8b-7b252ebe33fe@canonical.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Chris Packham <Chris.Packham@alliedtelesis.co.nz> To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>, "robh+dt@kernel.org" <robh+dt@kernel.org>, "linus.walleij@linaro.org" <linus.walleij@linaro.org>, "catalin.marinas@arm.com" <catalin.marinas@arm.com>, "andrew@lunn.ch" <andrew@lunn.ch>, "gregory.clement@bootlin.com" <gregory.clement@bootlin.com>, "sebastian.hesselbarth@gmail.com" <sebastian.hesselbarth@gmail.com> Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5 Date: Wed, 16 Mar 2022 20:21:58 +0000 [thread overview] Message-ID: <6d902e7d-b71f-9dcd-9175-cc706e3d60cc@alliedtelesis.co.nz> (raw) In-Reply-To: <cb0af80e-3e5a-fbd9-cd8b-7b252ebe33fe@canonical.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-16 20:22 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-14 21:31 [PATCH v2 0/8] arm64: mvebu: Support for Marvell 98DX2530 (and variants) Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-14 21:31 ` [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5 Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:07 ` Andrew Lunn 2022-03-15 0:07 ` Andrew Lunn 2022-03-15 0:22 ` Chris Packham 2022-03-15 0:22 ` Chris Packham 2022-03-15 0:27 ` Andrew Lunn 2022-03-15 0:27 ` Andrew Lunn 2022-03-23 18:34 ` Rob Herring 2022-03-23 18:34 ` Rob Herring 2022-03-15 10:46 ` Krzysztof Kozlowski 2022-03-15 10:46 ` Krzysztof Kozlowski 2022-03-15 21:12 ` Chris Packham 2022-03-15 21:12 ` Chris Packham 2022-03-16 8:16 ` Krzysztof Kozlowski 2022-03-16 8:16 ` Krzysztof Kozlowski 2022-03-16 20:21 ` Chris Packham [this message] 2022-03-16 20:21 ` Chris Packham 2022-03-17 7:26 ` Krzysztof Kozlowski 2022-03-17 7:26 ` Krzysztof Kozlowski 2022-03-17 14:14 ` Andrew Lunn 2022-03-17 14:14 ` Andrew Lunn 2022-03-17 15:16 ` Krzysztof Kozlowski 2022-03-17 15:16 ` Krzysztof Kozlowski 2022-03-14 21:31 ` [PATCH v2 2/8] dt-bindings: net: mvneta: Add marvell,armada-ac5-neta Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:10 ` Andrew Lunn 2022-03-15 0:10 ` Andrew Lunn 2022-03-14 21:31 ` [PATCH v2 3/8] dt-bindings: mmc: xenon: add AC5 compatible string Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:14 ` Andrew Lunn 2022-03-15 0:14 ` Andrew Lunn 2022-03-14 21:31 ` [PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:16 ` Andrew Lunn 2022-03-15 0:16 ` Andrew Lunn 2022-03-15 10:49 ` Krzysztof Kozlowski 2022-03-15 10:49 ` Krzysztof Kozlowski 2022-03-15 14:33 ` Andrew Lunn 2022-03-15 14:33 ` Andrew Lunn 2022-03-15 14:39 ` Krzysztof Kozlowski 2022-03-15 14:39 ` Krzysztof Kozlowski 2022-03-14 21:31 ` [PATCH v2 5/8] net: mvneta: Add support for 98DX2530 Ethernet port Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:12 ` Andrew Lunn 2022-03-15 0:12 ` Andrew Lunn 2022-03-15 0:27 ` Chris Packham 2022-03-15 0:27 ` Chris Packham 2022-03-14 21:31 ` [PATCH v2 6/8] mmc: xenon: add AC5 compatible string Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:14 ` Andrew Lunn 2022-03-15 0:14 ` Andrew Lunn 2022-03-14 21:31 ` [PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:24 ` Andrew Lunn 2022-03-15 0:24 ` Andrew Lunn 2022-03-15 2:11 ` Chris Packham 2022-03-15 2:11 ` Chris Packham 2022-03-15 14:28 ` Andrew Lunn 2022-03-15 14:28 ` Andrew Lunn 2022-03-16 11:49 ` Marc Zyngier 2022-03-16 11:49 ` Marc Zyngier 2022-03-14 21:31 ` [PATCH v2 8/8] arm64: marvell: enable the 98DX2530 pinctrl driver Chris Packham 2022-03-14 21:31 ` Chris Packham 2022-03-15 0:25 ` Andrew Lunn 2022-03-15 0:25 ` Andrew Lunn
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6d902e7d-b71f-9dcd-9175-cc706e3d60cc@alliedtelesis.co.nz \ --to=chris.packham@alliedtelesis.co.nz \ --cc=andrew@lunn.ch \ --cc=catalin.marinas@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=gregory.clement@bootlin.com \ --cc=krzysztof.kozlowski@canonical.com \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robh+dt@kernel.org \ --cc=sebastian.hesselbarth@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.