| Message ID | 20210929163847.2807812-11-maz@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Add support for Apple M1 | expand |
On Wed, Sep 29, 2021 at 6:56 PM Marc Zyngier <maz@kernel.org> wrote: > From: Mark Kettenis <kettenis@openbsd.org> > > Add pinctrl nodes corresponding to the gpio,t8101 nodes in the > Apple device tree for the Mac mini (M1, 2020). > > Clock references are left out at the moment and will be added once > the appropriate bindings have been settled upon. > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/r/20210520171310.772-3-mark.kettenis@xs4all.nl (...) > + pinctrl_ap: pinctrl@23c100000 { > + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; > + reg = <0x2 0x3c100000 0x0 0x100000>; > + > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ranges = <&pinctrl_ap 0 0 212>; In other discussions it turns out that the driver is abusing these gpio-ranges to find out how many pins are in each pinctrl instance. This is not the idea with gpio-ranges, these can be multiple and map different sets, so we need something like apple,npins = <212>; (+ bindings) or so... Yours, Linus Walleij
On Wed, 29 Sep 2021 20:05:42 +0100, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Sep 29, 2021 at 6:56 PM Marc Zyngier <maz@kernel.org> wrote: > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > Add pinctrl nodes corresponding to the gpio,t8101 nodes in the > > Apple device tree for the Mac mini (M1, 2020). > > > > Clock references are left out at the moment and will be added once > > the appropriate bindings have been settled upon. > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > Link: https://lore.kernel.org/r/20210520171310.772-3-mark.kettenis@xs4all.nl > (...) > > + pinctrl_ap: pinctrl@23c100000 { > > + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; > > + reg = <0x2 0x3c100000 0x0 0x100000>; > > + > > + gpio-controller; > > + #gpio-cells = <2>; > > + gpio-ranges = <&pinctrl_ap 0 0 212>; > > In other discussions it turns out that the driver is abusing these gpio-ranges > to find out how many pins are in each pinctrl instance. This is not the > idea with gpio-ranges, these can be multiple and map different sets, > so we need something like > > apple,npins = <212>; > (+ bindings) > > or so... Is it the driver that needs updating? Or the binding? I don't really care about the former, but the latter is more disruptive as it has impacts over both u-boot and at least OpenBSD. How is that solved on other pinctrl blocks? I can't see anyone having a similar a similar property. Thanks, M.
> Date: Thu, 30 Sep 2021 09:00:49 +0100 > From: Marc Zyngier <maz@kernel.org> Hi Linus and Marc, > On Wed, 29 Sep 2021 20:05:42 +0100, > Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Wed, Sep 29, 2021 at 6:56 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > Add pinctrl nodes corresponding to the gpio,t8101 nodes in the > > > Apple device tree for the Mac mini (M1, 2020). > > > > > > Clock references are left out at the moment and will be added once > > > the appropriate bindings have been settled upon. > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > Link: https://lore.kernel.org/r/20210520171310.772-3-mark.kettenis@xs4all.nl > > (...) > > > + pinctrl_ap: pinctrl@23c100000 { > > > + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; > > > + reg = <0x2 0x3c100000 0x0 0x100000>; > > > + > > > + gpio-controller; > > > + #gpio-cells = <2>; > > > + gpio-ranges = <&pinctrl_ap 0 0 212>; > > > > In other discussions it turns out that the driver is abusing these gpio-ranges > > to find out how many pins are in each pinctrl instance. This is not the > > idea with gpio-ranges, these can be multiple and map different sets, > > so we need something like > > > > apple,npins = <212>; > > (+ bindings) > > > > or so... > > Is it the driver that needs updating? Or the binding? I don't really > care about the former, but the latter is more disruptive as it has > impacts over both u-boot and at least OpenBSD. I don't have a finished OpenBSD driver yet, and U-Boot support is still in the process of being upstreamed, so tweaking the binding a bit is not out of the question at this point. And as long as the gpio-ranges property continues to be there, things won't break anyway. > How is that solved on other pinctrl blocks? I can't see anyone having > a similar a similar property. I suspect most pinctrl blocks have a well-defined number of pins that is simply a #define in the driver. Here we don't really know what the hardware provides but the "Apple device tree" has a property that describes the number of pins, which varies between the different blocks. Since there is a simple 1:1 mapping between pins and gpios it seemed natural to me to simply use the value from gpio-ranges. My thinking was that having a separate property to encode the number of pins just increases the chances of the two getting out of sync. But maybe that is the whole point Linus is trying to make; not all pins may actually provide GPIO functionality and gpio-ranges can be used to map only those pins that actually do. In this particular case I don't think it makes sense to map multiple ranges though as we will probably never know full details for all the pins. FWIW, there are other U-Boot drivers that use gpio-ranges to get the pin count. But I suppose U-Boot has somewhat different standards than the Linux kernel, prioritising code size and such.
On Thu, Sep 30, 2021 at 10:00 AM Marc Zyngier <maz@kernel.org> wrote: > > In other discussions it turns out that the driver is abusing these gpio-ranges > > to find out how many pins are in each pinctrl instance. This is not the > > idea with gpio-ranges, these can be multiple and map different sets, > > so we need something like > > > > apple,npins = <212>; > > (+ bindings) > > > > or so... > > Is it the driver that needs updating? Or the binding? Both, I guess. > I don't really > care about the former, but the latter is more disruptive as it has > impacts over both u-boot and at least OpenBSD. > > How is that solved on other pinctrl blocks? I can't see anyone having > a similar a similar property. The Apple pincontroller is unique in having four instances using the same compatible string (I raised this as an issue too). Most SoCs has one instance of a pin controller, with one compatible string and then we also know how many pins it has. The maintainer seeme unhappy about my suggestion to name the four pin controllers after function and insist to use the same compatible for all four, which means they instead need to be parametrized, which means this parameter has to be added because ranges should not be used in this way. I guess the code can survive using the ranges as a fallback at the cost of some more complex code. Yours, Linus Walleij
On 30/09/2021 01.38, Marc Zyngier wrote: > From: Mark Kettenis <kettenis@openbsd.org> > > Add pinctrl nodes corresponding to the gpio,t8101 nodes in the > Apple device tree for the Mac mini (M1, 2020). > > Clock references are left out at the moment and will be added once > the appropriate bindings have been settled upon. > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/r/20210520171310.772-3-mark.kettenis@xs4all.nl > --- > arch/arm64/boot/dts/apple/t8103.dtsi | 83 ++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi > index a1e22a2ea2e5..503a76fc30e6 100644 [snip] Looks good. Can you resend just this patch with the apple,npins properties added? Once that's settled in the binding (which seems to just be waiting on some linter issues) I can merge this and the rest of the DT changes through my tree.
diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi index a1e22a2ea2e5..503a76fc30e6 100644 --- a/arch/arm64/boot/dts/apple/t8103.dtsi +++ b/arch/arm64/boot/dts/apple/t8103.dtsi @@ -9,6 +9,7 @@ #include <dt-bindings/interrupt-controller/apple-aic.h> #include <dt-bindings/interrupt-controller/irq.h> +#include <dt-bindings/pinctrl/apple.h> / { compatible = "apple,t8103", "apple,arm-platform"; @@ -131,5 +132,87 @@ aic: interrupt-controller@23b100000 { interrupt-controller; reg = <0x2 0x3b100000 0x0 0x8000>; }; + + pinctrl_ap: pinctrl@23c100000 { + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; + reg = <0x2 0x3c100000 0x0 0x100000>; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&pinctrl_ap 0 0 212>; + + interrupt-controller; + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 190 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 191 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 192 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 193 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 194 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 195 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 196 IRQ_TYPE_LEVEL_HIGH>; + + pcie_pins: pcie-pins { + pinmux = <APPLE_PINMUX(150, 1)>, + <APPLE_PINMUX(151, 1)>, + <APPLE_PINMUX(32, 1)>; + }; + }; + + pinctrl_aop: pinctrl@24a820000 { + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; + reg = <0x2 0x4a820000 0x0 0x4000>; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&pinctrl_aop 0 0 42>; + + interrupt-controller; + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 268 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 269 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 270 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 271 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 272 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 273 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 274 IRQ_TYPE_LEVEL_HIGH>; + }; + + pinctrl_nub: pinctrl@23d1f0000 { + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; + reg = <0x2 0x3d1f0000 0x0 0x4000>; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&pinctrl_nub 0 0 23>; + + interrupt-controller; + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 330 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 331 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 332 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 333 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 334 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 335 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 336 IRQ_TYPE_LEVEL_HIGH>; + }; + + pinctrl_smc: pinctrl@23e820000 { + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; + reg = <0x2 0x3e820000 0x0 0x4000>; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&pinctrl_smc 0 0 16>; + + interrupt-controller; + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 391 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 392 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 393 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 394 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 395 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 396 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 397 IRQ_TYPE_LEVEL_HIGH>; + }; }; };