| Message ID | 20231218-mbly-pinctrl-v1-0-2f7d366c2051@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for Mobileye EyeQ5 pin controller | expand |
On 18/12/2023 18:19, Théo Lebrun wrote: > Add dt-schema type bindings for the Mobileye EyeQ5 pin controller. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 125 +++++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 126 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml > new file mode 100644 > index 000000000000..5faddebe2413 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml > @@ -0,0 +1,125 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mobileye EyeQ5 pinctrl (pinmux & pinconf) controller pinctrl means pin controller, so you basically wrote: pin controller pinmux and pin configuration controller Just "pin controller" > + > +description: > + The EyeQ5 pin controller handles a pin bank. It is custom to this platform, Can part of SoC be not custom to given platform? I mean... describe the hardware, not write essay. > + its registers live in a shared region called OLB. > + There are two pin banks on the platform, each having a specific compatible. Instead of repeating something obvious - visible from the binding - explain why. Say something different than the binding is saying. > + Pins and groups are bijective. > + > +maintainers: > + - Grégory Clement <gregory.clement@bootlin.com> > + - Théo Lebrun <theo.lebrun@bootlin.com> > + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> > + > +properties: > + $nodename: > + pattern: "^pinctrl([0-9]+)?$" > + description: > + We have no unique address, we rely on OLB; we therefore can't keep the > + standard pattern and cannot inherit from pinctrl.yaml. No, instead fix pinctrl.yaml > + > + compatible: > + enum: > + - mobileye,eyeq5-a-pinctrl > + - mobileye,eyeq5-b-pinctrl Why two compatibles? Description provided no rationale for this. > + > + "#pinctrl-cells": > + const: 1 > + > + mobileye,olb: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the OLB syscon. This is a fallback to using the parent as > + syscon node. So here is the explanation for missing unit address. If all registers, as you claim in description, belong to OLB, then this should be part of OLB. Drop the phandle. > + > +required: > + - compatible > + - "#pinctrl-cells" So now please test your code without olb phandle... > + > +patternProperties: patternProperties go after properties > + "-pins?$": > + type: object > + description: Pin muxing configuration. > + $ref: pinmux-node.yaml# > + additionalProperties: false Why not unevaluatedProperties? > + properties: > + pins: true > + function: true > + bias-disable: true > + bias-pull-down: true > + bias-pull-up: true > + drive-strength: true > + required: > + - pins > + - function > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: mobileye,eyeq5-a-pinctrl > + then: > + patternProperties: > + "-pins?$": > + properties: > + function: > + enum: [gpio, timer0, timer1, timer2, timer5, uart0, uart1, can0, > + can1, spi0, spi1, refclk0] > + - if: > + properties: > + compatible: > + contains: > + const: mobileye,eyeq5-b-pinctrl > + then: > + patternProperties: > + "-pins?$": > + properties: > + function: > + enum: [gpio, timer3, timer4, timer6, uart2, can2, spi2, spi3, > + mclk0] > + > +additionalProperties: false > + > +examples: > + - | > + olb@e00000 { > + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; Drop, not erlated. > + reg = <0xe00000 0x400>; > + reg-io-width = <4>; > + > + pinctrl0 { Suffixes are always after - > + compatible = "mobileye,eyeq5-a-pinctrl"; > + #pinctrl-cells = <1>; Where is the phandle? > + }; > + > + pinctrl1 { > + compatible = "mobileye,eyeq5-b-pinctrl"; > + #pinctrl-cells = <1>; > + }; > + }; > + - | > + olb: olb@e00000 { > + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > + reg = <0xe00000 0x400>; > + reg-io-width = <4>; > + }; > + > + pinctrl0 { > + compatible = "mobileye,eyeq5-a-pinctrl"; > + #pinctrl-cells = <1>; > + mobileye,olb = <&olb>; Really, why? This is just confusing. There is no explanation for supporting both. Hardware is either this or that, not both! Best regards, Krzysztof
Hello, I've seen all your comments, thanks for that. I'll answer to some. On Tue Dec 19, 2023 at 8:34 AM CET, Krzysztof Kozlowski wrote: > On 18/12/2023 18:19, Théo Lebrun wrote: > > Add dt-schema type bindings for the Mobileye EyeQ5 pin controller. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 125 +++++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 126 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml > > new file mode 100644 > > index 000000000000..5faddebe2413 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml > > @@ -0,0 +1,125 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Mobileye EyeQ5 pinctrl (pinmux & pinconf) controller > > pinctrl means pin controller, so you basically wrote: > pin controller pinmux and pin configuration controller > > Just "pin controller" > > > > + > > +description: > > + The EyeQ5 pin controller handles a pin bank. It is custom to this platform, > > Can part of SoC be not custom to given platform? I mean... describe the > hardware, not write essay. > > > + its registers live in a shared region called OLB. > > + There are two pin banks on the platform, each having a specific compatible. > > Instead of repeating something obvious - visible from the binding - > explain why. Say something different than the binding is saying. > > > > + Pins and groups are bijective. > > + > > +maintainers: > > + - Grégory Clement <gregory.clement@bootlin.com> > > + - Théo Lebrun <theo.lebrun@bootlin.com> > > + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> > > + > > +properties: > > + $nodename: > > + pattern: "^pinctrl([0-9]+)?$" > > + description: > > + We have no unique address, we rely on OLB; we therefore can't keep the > > + standard pattern and cannot inherit from pinctrl.yaml. > > No, instead fix pinctrl.yaml I've tried some things, but I'm unsure how to proceed. Options I see: - Modify pinctrl.yaml so that if reg/ranges is required, $nodename must be the current value ("^(pinctrl|pinmux)(@[0-9a-f]+)?$"). Else, $nodename should be "^(pinctrl|pinmux)(-[0-9a-f]+)?$". I've tried some things but nothing conclusive for the moment. - Leave pinctrl.yaml alone and override $nodename from our binding. I've not found a way to do that though. - Use the current $nodename, ie with a unit address. With that approach I get the "node has a unit name, but no reg or ranges property" warning which, reading the code, I don't see a way of avoiding. Were you thinking about option 1? Any advice on how to proceed would be helpful, I've not been able to get a working patch to use option 1. > > > + > > + compatible: > > + enum: > > + - mobileye,eyeq5-a-pinctrl > > + - mobileye,eyeq5-b-pinctrl > > Why two compatibles? Description provided no rationale for this. I'll add that info. The gist of it is to have one node per bank. Each pin has two function: GPIO or pin-dependent. So we must know which bank we are to know what each pin function can be. Both nodes are child to the same OLB. The compatible also tells us which registers to use. > > > + > > + "#pinctrl-cells": > > + const: 1 > > + > > + mobileye,olb: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: > > + A phandle to the OLB syscon. This is a fallback to using the parent as > > + syscon node. > > So here is the explanation for missing unit address. If all registers, > as you claim in description, belong to OLB, then this should be part of > OLB. Drop the phandle. The reason I provided both options was that I see four drivers that do this kind of fallback. I guess it was for legacy reasons. I'm dropping the phandle and keeping only the child option. drivers/gpio/gpio-syscon.c drivers/phy/rockchip/phy-rockchip-usb.c drivers/phy/samsung/phy-exynos-dp-video.c drivers/soc/rockchip/io-domain.c > > > + > > +required: > > + - compatible > > + - "#pinctrl-cells" > > So now please test your code without olb phandle... That is the main way I am running my code. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 20/12/2023 10:21, Théo Lebrun wrote: > Hello, > > I've seen all your comments, thanks for that. I'll answer to some. > > On Tue Dec 19, 2023 at 8:34 AM CET, Krzysztof Kozlowski wrote: >> On 18/12/2023 18:19, Théo Lebrun wrote: >>> Add dt-schema type bindings for the Mobileye EyeQ5 pin controller. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >>> .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 125 +++++++++++++++++++++ >>> MAINTAINERS | 1 + >>> 2 files changed, 126 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml >>> new file mode 100644 >>> index 000000000000..5faddebe2413 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml >>> @@ -0,0 +1,125 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Mobileye EyeQ5 pinctrl (pinmux & pinconf) controller >> >> pinctrl means pin controller, so you basically wrote: >> pin controller pinmux and pin configuration controller >> >> Just "pin controller" >> >> >>> + >>> +description: >>> + The EyeQ5 pin controller handles a pin bank. It is custom to this platform, >> >> Can part of SoC be not custom to given platform? I mean... describe the >> hardware, not write essay. >> >>> + its registers live in a shared region called OLB. >>> + There are two pin banks on the platform, each having a specific compatible. >> >> Instead of repeating something obvious - visible from the binding - >> explain why. Say something different than the binding is saying. >> >> >>> + Pins and groups are bijective. >>> + >>> +maintainers: >>> + - Grégory Clement <gregory.clement@bootlin.com> >>> + - Théo Lebrun <theo.lebrun@bootlin.com> >>> + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> >>> + >>> +properties: >>> + $nodename: >>> + pattern: "^pinctrl([0-9]+)?$" >>> + description: >>> + We have no unique address, we rely on OLB; we therefore can't keep the >>> + standard pattern and cannot inherit from pinctrl.yaml. >> >> No, instead fix pinctrl.yaml > > I've tried some things, but I'm unsure how to proceed. Options I see: > > - Modify pinctrl.yaml so that if reg/ranges is required, $nodename must > be the current value ("^(pinctrl|pinmux)(@[0-9a-f]+)?$"). Else, > $nodename should be "^(pinctrl|pinmux)(-[0-9a-f]+)?$". Yes, but: "-[0-9]", these are not hex. I don't understand what is the problem here. It's just a regex and there are plenty of examples how this should look like. > > I've tried some things but nothing conclusive for the moment. > > - Leave pinctrl.yaml alone and override $nodename from our binding. > I've not found a way to do that though. > > - Use the current $nodename, ie with a unit address. With that approach > I get the "node has a unit name, but no reg or ranges property" > warning which, reading the code, I don't see a way of avoiding. > > Were you thinking about option 1? Any advice on how to proceed would be > helpful, I've not been able to get a working patch to use option 1. Why? > >> >>> + >>> + compatible: >>> + enum: >>> + - mobileye,eyeq5-a-pinctrl >>> + - mobileye,eyeq5-b-pinctrl >> >> Why two compatibles? Description provided no rationale for this. > > I'll add that info. The gist of it is to have one node per bank. Each > pin has two function: GPIO or pin-dependent. So we must know which bank > we are to know what each pin function can be. OK > > Both nodes are child to the same OLB. The compatible also tells us which > registers to use. > >> >>> + >>> + "#pinctrl-cells": >>> + const: 1 >>> + >>> + mobileye,olb: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + A phandle to the OLB syscon. This is a fallback to using the parent as >>> + syscon node. >> >> So here is the explanation for missing unit address. If all registers, >> as you claim in description, belong to OLB, then this should be part of >> OLB. Drop the phandle. > > The reason I provided both options was that I see four drivers that do > this kind of fallback. I guess it was for legacy reasons. I'm dropping > the phandle and keeping only the child option. > > drivers/gpio/gpio-syscon.c > drivers/phy/rockchip/phy-rockchip-usb.c > drivers/phy/samsung/phy-exynos-dp-video.c > drivers/soc/rockchip/io-domain.c > Best regards, Krzysztof
Hi, This series adds pinctrl support to the Mobileye EyeQ5 platform, following up on the platform support series by Grégory Clement [1]. All registers involved live in a shared register region called OLB ("Other Logic Block"). We have control over bias, drive strength and muxing. The latter allows two functions per pin; the first function is always GPIO while the second one is pin-dependent. Functions are statically declared in the driver, associated to compatibles. Two compatibles exist, one for each bank. The pin controller's functionality is not limited so each pin maps to one group. That makes pin & group indexes the same, simplifying logic. Having two instances, one per bank, also is done to simplify the driver's logic. The series ends by adding the two banks as devicetree nodes and declaring a pin-mux node for each function. We also add pinctrl references to the existing UART nodes. We are based on the reset series [2] for the sole reason of avoiding merge conflicts in the devicetree. [1]: https://lore.kernel.org/lkml/20231212163459.1923041-1-gregory.clement@bootlin.com/ [2]: https://lore.kernel.org/lkml/20231218-mbly-reset-v1-0-b4688b916213@bootlin.com/ Have a nice day, Théo Lebrun Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Théo Lebrun (4): dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings pinctrl: eyeq5: add driver MIPS: mobileye: eyeq5: add pinctrl nodes & pinmux function nodes MIPS: mobileye: eyeq5: add pinctrl properties to uarts .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml | 125 +++++ MAINTAINERS | 2 + arch/mips/boot/dts/mobileye/eyeq5-pins.dtsi | 128 +++++ arch/mips/boot/dts/mobileye/eyeq5.dtsi | 17 + drivers/pinctrl/Kconfig | 15 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-eyeq5.c | 593 +++++++++++++++++++++ 7 files changed, 881 insertions(+) --- base-commit: cfa954ebcdc3504dbf38ff5ba1589ed0cdfc8313 change-id: 20231023-mbly-pinctrl-7afe9c738936 Best regards,