| Message ID | 1424943294-8805-1-git-send-email-plagnioj@jcrosoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jean-Christophe, On Thu, Feb 26, 2015 at 10:34:54AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote: > Today if we want to disable a pio bank we may will siliently break pinctrl > configuration in the DT. This will be detected only at runtime. Do you have a case where it breaks pinctrl? I can do more tests but with the latest patch "pinctrl: at91: allow to have disabled gpio bank", I had no issue. > > So move the pinctrl configuration to the bank instead of the bus. > This allow to detect pinctrl issue at DT compiling time when disable a bank. > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: devicetree@vger.kernel.org > --- > .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > index b7a93e8..78355ee 100644 > --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { > pinctrl-0 = <&pinctrl_dbgu>; > status = "disabled"; > }; > + > +II) New Bindings per PIO Block > + > +This allow to detect pinctrl issue at DT compiling time when disable a bank > + > +Required properties for iomux controller: > +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl" > + or "atmel,sama5d3-pio-pinctrl" > +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be > + configured in this periph mode. All the periph and bank need to be describe. > + > +How to create such array: > + > +Each column will represent the possible peripheral of the pinctrl for the bank > + > +Take an example on the 9260 > +Peripheral: 2 ( A and B) > +=> > + > + /* A B */ > + 0xffffffff 0xffc00c3b /* pioA */ > + > +For each peripheral/bank we will descibe in a u32 if a pin can be > +configured in it by putting 1 to the pin bit (1 << pin) > + > +Required properties for pin configuration node: > +- atmel,pins: 3 integers array, represents a group of pins mux and config > + setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>. > + The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B... > + > +Bits used for CONFIG: > +cf atmel,at91-pinctrl > + > +pioB: gpio@fffff600 { > + compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl"; > + reg = <0xfffff600 0x200>; > + interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>; > + #gpio-cells = <2>; > + gpio-controller; > + interrupt-controller; > + #interrupt-cells = <2>; > + clocks = <&pioB_clk>; > + > + /* A B */ > + atmel,mux-mask = <0xffffffff 0x7fff3ccf>; > + > + dbgu { > + pinctrl_dbgu: dbgu-0 { > + atmel,pins = > + <14 0x1 0x0 /* PB14 periph A */ > + 15 0x1 0x1>; /* PB15 periph A with pullup */ > + }; > + }; So we have to update all device tree files, that's a lot of work... Moreover it adds complexity if we have a device using pins from different pio controllers. > +}; > + > +dbgu: serial@fffff200 { > + compatible = "atmel,at91sam9260-usart"; > + reg = <0xfffff200 0x200>; > + interrupts = <1 4 7>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_dbgu>; > + status = "disabled"; > +}; > + > +if you have to use multiple bank > + pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>; You mean pio I think. Ludovic
> On Feb 26, 2015, at 6:43 PM, Ludovic Desroches <ludovic.desroches@atmel.com> wrote: > > Hi Jean-Christophe, > > On Thu, Feb 26, 2015 at 10:34:54AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote: >> Today if we want to disable a pio bank we may will siliently break pinctrl >> configuration in the DT. This will be detected only at runtime. > > Do you have a case where it breaks pinctrl? I can do more tests but with > the latest patch "pinctrl: at91: allow to have disabled gpio bank", I > had no issue. simple disable the PIO bank and if one device use it as pinctrl and no gpio is used in the DT then you get a broken platform but dtb is generated as all the node are enabled for pinctrl > >> >> So move the pinctrl configuration to the bank instead of the bus. >> This allow to detect pinctrl issue at DT compiling time when disable a bank. >> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: devicetree@vger.kernel.org >> --- >> .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ >> 1 file changed, 66 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >> index b7a93e8..78355ee 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { >> pinctrl-0 = <&pinctrl_dbgu>; >> status = "disabled"; >> }; >> + >> +II) New Bindings per PIO Block >> + >> +This allow to detect pinctrl issue at DT compiling time when disable a bank >> + >> +Required properties for iomux controller: >> +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl" >> + or "atmel,sama5d3-pio-pinctrl" >> +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be >> + configured in this periph mode. All the periph and bank need to be describe. >> + >> +How to create such array: >> + >> +Each column will represent the possible peripheral of the pinctrl for the bank >> + >> +Take an example on the 9260 >> +Peripheral: 2 ( A and B) >> +=> >> + >> + /* A B */ >> + 0xffffffff 0xffc00c3b /* pioA */ >> + >> +For each peripheral/bank we will descibe in a u32 if a pin can be >> +configured in it by putting 1 to the pin bit (1 << pin) >> + >> +Required properties for pin configuration node: >> +- atmel,pins: 3 integers array, represents a group of pins mux and config >> + setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>. >> + The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B... >> + >> +Bits used for CONFIG: >> +cf atmel,at91-pinctrl >> + >> +pioB: gpio@fffff600 { >> + compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl"; >> + reg = <0xfffff600 0x200>; >> + interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>; >> + #gpio-cells = <2>; >> + gpio-controller; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + clocks = <&pioB_clk>; >> + >> + /* A B */ >> + atmel,mux-mask = <0xffffffff 0x7fff3ccf>; >> + >> + dbgu { >> + pinctrl_dbgu: dbgu-0 { >> + atmel,pins = >> + <14 0x1 0x0 /* PB14 periph A */ >> + 15 0x1 0x1>; /* PB15 periph A with pullup */ >> + }; >> + }; > > So we have to update all device tree files, that's a lot of work… break nothing both binding support will be in the kernel but only one enable at a time, so basically mainline will drop the old one and as today 99% of the pinctrl are at SoC level so it’s not a big work at all > Moreover it adds complexity if we have a device using pins from > different pio controllers. put 2 phandle means complexity please and it’s the key point of the new binding Best Regards, J.
Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit : > Today if we want to disable a pio bank we may will siliently break pinctrl > configuration in the DT. This will be detected only at runtime. > > So move the pinctrl configuration to the bank instead of the bus. > This allow to detect pinctrl issue at DT compiling time when disable a bank. > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: devicetree@vger.kernel.org > --- > .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > index b7a93e8..78355ee 100644 > --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { > pinctrl-0 = <&pinctrl_dbgu>; > status = "disabled"; > }; > + > +II) New Bindings per PIO Block Sorry but NACK. I don't want to manage another flavor of the pinmux biding with no real benefit. I would have been good if we had it from day-1. Now it's too late. Moreover, splitting a binding definition if you have a function given by multiple banks can be weird and not well understood in regard to our current group+function definition scheme (Cf. your last example). > +This allow to detect pinctrl issue at DT compiling time when disable a bank > + > +Required properties for iomux controller: > +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl" > + or "atmel,sama5d3-pio-pinctrl" > +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be > + configured in this periph mode. All the periph and bank need to be describe. > + > +How to create such array: > + > +Each column will represent the possible peripheral of the pinctrl for the bank > + > +Take an example on the 9260 > +Peripheral: 2 ( A and B) > +=> > + > + /* A B */ > + 0xffffffff 0xffc00c3b /* pioA */ > + > +For each peripheral/bank we will descibe in a u32 if a pin can be > +configured in it by putting 1 to the pin bit (1 << pin) > + > +Required properties for pin configuration node: > +- atmel,pins: 3 integers array, represents a group of pins mux and config > + setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>. > + The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B... > + > +Bits used for CONFIG: > +cf atmel,at91-pinctrl > + > +pioB: gpio@fffff600 { > + compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl"; > + reg = <0xfffff600 0x200>; > + interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>; > + #gpio-cells = <2>; > + gpio-controller; > + interrupt-controller; > + #interrupt-cells = <2>; > + clocks = <&pioB_clk>; > + > + /* A B */ > + atmel,mux-mask = <0xffffffff 0x7fff3ccf>; > + > + dbgu { > + pinctrl_dbgu: dbgu-0 { > + atmel,pins = > + <14 0x1 0x0 /* PB14 periph A */ > + 15 0x1 0x1>; /* PB15 periph A with pullup */ > + }; > + }; > +}; > + > +dbgu: serial@fffff200 { > + compatible = "atmel,at91sam9260-usart"; > + reg = <0xfffff200 0x200>; > + interrupts = <1 4 7>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_dbgu>; > + status = "disabled"; > +}; > + > +if you have to use multiple bank > + pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>; > Best regards,
> On Mar 6, 2015, at 11:08 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > > Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit : >> Today if we want to disable a pio bank we may will siliently break pinctrl >> configuration in the DT. This will be detected only at runtime. >> >> So move the pinctrl configuration to the bank instead of the bus. >> This allow to detect pinctrl issue at DT compiling time when disable a bank. >> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: devicetree@vger.kernel.org >> --- >> .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ >> 1 file changed, 66 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >> index b7a93e8..78355ee 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { >> pinctrl-0 = <&pinctrl_dbgu>; >> status = "disabled"; >> }; >> + >> +II) New Bindings per PIO Block > > Sorry but NACK. > > I don't want to manage another flavor of the pinmux biding with no real > benefit. I would have been good if we had it from day-1. Now it's too late. yes we do, we catch but a compiling time instead of RUNTIME which is critical so I’ll pass on the NACK > > Moreover, splitting a binding definition if you have a function given by > multiple banks can be weird and not well understood in regard to our > current group+function definition scheme (Cf. your last example). > Others already do so and this is not complex at all Best Regards, J. > >> +This allow to detect pinctrl issue at DT compiling time when disable a bank >> + >> +Required properties for iomux controller: >> +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl" >> + or "atmel,sama5d3-pio-pinctrl" >> +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be >> + configured in this periph mode. All the periph and bank need to be describe. >> + >> +How to create such array: >> + >> +Each column will represent the possible peripheral of the pinctrl for the bank >> + >> +Take an example on the 9260 >> +Peripheral: 2 ( A and B) >> +=> >> + >> + /* A B */ >> + 0xffffffff 0xffc00c3b /* pioA */ >> + >> +For each peripheral/bank we will descibe in a u32 if a pin can be >> +configured in it by putting 1 to the pin bit (1 << pin) >> + >> +Required properties for pin configuration node: >> +- atmel,pins: 3 integers array, represents a group of pins mux and config >> + setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>. >> + The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B... >> + >> +Bits used for CONFIG: >> +cf atmel,at91-pinctrl >> + >> +pioB: gpio@fffff600 { >> + compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl"; >> + reg = <0xfffff600 0x200>; >> + interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>; >> + #gpio-cells = <2>; >> + gpio-controller; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + clocks = <&pioB_clk>; >> + >> + /* A B */ >> + atmel,mux-mask = <0xffffffff 0x7fff3ccf>; >> + >> + dbgu { >> + pinctrl_dbgu: dbgu-0 { >> + atmel,pins = >> + <14 0x1 0x0 /* PB14 periph A */ >> + 15 0x1 0x1>; /* PB15 periph A with pullup */ >> + }; >> + }; >> +}; >> + >> +dbgu: serial@fffff200 { >> + compatible = "atmel,at91sam9260-usart"; >> + reg = <0xfffff200 0x200>; >> + interrupts = <1 4 7>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_dbgu>; >> + status = "disabled"; >> +}; >> + >> +if you have to use multiple bank >> + pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>; >> > > Best regards, > -- > Nicolas Ferre > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 07/03/2015 at 00:49:55 +0800, Jean-Christophe PLAGNIOL-VILLARD wrote : > > > > Sorry but NACK. > > > > I don't want to manage another flavor of the pinmux biding with no real > > benefit. I would have been good if we had it from day-1. Now it's too late. > > yes we do, we catch but a compiling time instead of RUNTIME which is critical > > so I’ll pass on the NACK > If you are changing the binding, how about doing it right this time and completely drop the current mess? > > > > Moreover, splitting a binding definition if you have a function given by > > multiple banks can be weird and not well understood in regard to our > > current group+function definition scheme (Cf. your last example). > > > > Others already do so and this is not complex at all >
Hi Jean-Christophe, On Sat, 7 Mar 2015 00:49:55 +0800 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > On Mar 6, 2015, at 11:08 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > > > > Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit : > >> Today if we want to disable a pio bank we may will siliently break pinctrl > >> configuration in the DT. This will be detected only at runtime. > >> > >> So move the pinctrl configuration to the bank instead of the bus. > >> This allow to detect pinctrl issue at DT compiling time when disable a bank. > >> > >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > >> Cc: Linus Walleij <linus.walleij@linaro.org> > >> Cc: devicetree@vger.kernel.org > >> --- > >> .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ > >> 1 file changed, 66 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > >> index b7a93e8..78355ee 100644 > >> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > >> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt > >> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { > >> pinctrl-0 = <&pinctrl_dbgu>; > >> status = "disabled"; > >> }; > >> + > >> +II) New Bindings per PIO Block > > > > Sorry but NACK. > > > > I don't want to manage another flavor of the pinmux biding with no real > > benefit. I would have been good if we had it from day-1. Now it's too late. > > yes we do, we catch but a compiling time instead of RUNTIME which is critical > > so I’ll pass on the NACK Tell me, how can you pass on a NACK coming from the at91 maintainer (which is also your co-maintainer) when you modify bindings of an at91 driver ? Please let's try to be constructive here, so that we can find an acceptable solution. > > > > > Moreover, splitting a binding definition if you have a function given by > > multiple banks can be weird and not well understood in regard to our > > current group+function definition scheme (Cf. your last example). > > I don't think it's a good idea either: you'll have to split pinconf definitions and that definitely doesn't improve readability. > > Others already do so and this is not complex at all Could you point out these bindings (and real examples please). Best Regards, Boris
> On Mar 7, 2015, at 2:23 AM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > Hi Jean-Christophe, > > On Sat, 7 Mar 2015 00:49:55 +0800 > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > >> >>> On Mar 6, 2015, at 11:08 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote: >>> >>> Le 26/02/2015 10:34, Jean-Christophe PLAGNIOL-VILLARD a écrit : >>>> Today if we want to disable a pio bank we may will siliently break pinctrl >>>> configuration in the DT. This will be detected only at runtime. >>>> >>>> So move the pinctrl configuration to the bank instead of the bus. >>>> This allow to detect pinctrl issue at DT compiling time when disable a bank. >>>> >>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> >>>> Cc: Linus Walleij <linus.walleij@linaro.org> >>>> Cc: devicetree@vger.kernel.org >>>> --- >>>> .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ >>>> 1 file changed, 66 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >>>> index b7a93e8..78355ee 100644 >>>> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >>>> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt >>>> @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { >>>> pinctrl-0 = <&pinctrl_dbgu>; >>>> status = "disabled"; >>>> }; >>>> + >>>> +II) New Bindings per PIO Block >>> >>> Sorry but NACK. >>> >>> I don't want to manage another flavor of the pinmux biding with no real >>> benefit. I would have been good if we had it from day-1. Now it's too late. >> >> yes we do, we catch but a compiling time instead of RUNTIME which is critical >> >> so I’ll pass on the NACK > > Tell me, how can you pass on a NACK coming from the at91 maintainer > (which is also your co-maintainer) when you modify bindings of an at91 > driver ? > Please let's try to be constructive here, so that we can find an > acceptable solution. I do pass on it which means, I do not accept to stop here the discussion because of this as Nicolas did for dropping the soc detection when I NACK > >> >>> >>> Moreover, splitting a binding definition if you have a function given by >>> multiple banks can be weird and not well understood in regard to our >>> current group+function definition scheme (Cf. your last example). >>> > > I don't think it's a good idea either: you'll have to split pinconf > definitions and that definitely doesn't improve readability. in HW it’s already the CASE. And today disable a bank you BROKE a board without even known it. This is NOT acceptable when we can detect it at compiling TIME. > >> >> Others already do so and this is not complex at all > > Could you point out these bindings (and real examples please). look at ST-E 9500 pinctrl they DO use it > > Best Regards, > > Boris > > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 26, 2015 at 10:34 AM, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > +For each peripheral/bank we will descibe in a u32 if a pin can be > +configured in it by putting 1 to the pin bit (1 << pin) This seems to be describing driver intrinsics in the device tree, like how the hardware is routed on the inside and what it can do. IMO that is driver territory, the driver should know these limitations and protest if you try to do something illegal. Anyway as the AT91 maintainers seem to disagree I will allow some more time for discussion before merging the patch. I can't really have one AT91 maintainer NACKing another, it doesn't matter that this is a separate driver, in my book the MAINTAINERS entry for AT91 as a whole overrides that so can you please find an agreement on how to handle this or I will stall the patch until you're in agreement. ARM SoC maintainers input would be welcomed. Yours, Linus Walleij
On Tue, Mar 17, 2015 at 5:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Feb 26, 2015 at 10:34 AM, Jean-Christophe PLAGNIOL-VILLARD > <plagnioj@jcrosoft.com> wrote: > >> +For each peripheral/bank we will descibe in a u32 if a pin can be >> +configured in it by putting 1 to the pin bit (1 << pin) > > This seems to be describing driver intrinsics in the device tree, like > how the hardware is routed on the inside and what it can do. > > IMO that is driver territory, the driver should know these limitations > and protest if you try to do something illegal. > > Anyway as the AT91 maintainers seem to disagree I will allow some > more time for discussion before merging the patch. > > I can't really have one AT91 maintainer NACKing another, it doesn't > matter that this is a separate driver, in my book the MAINTAINERS > entry for AT91 as a whole overrides that so can you please find an > agreement on how to handle this or I will stall the patch until > you're in agreement. Nicolas has been the de-facto maintainer of AT91 for quite a while now, even though more of them are listed on the maintainers entry. It would be inappropriate to merge something that he disagreed with on that platform. > ARM SoC maintainers input would be welcomed. It seems appropriate to ask the at91 folks to come back with a solution that everybody is OK with, and until then hold off merging this. -Olof
On 13:55 Sun 29 Mar , Olof Johansson wrote: > On Tue, Mar 17, 2015 at 5:07 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Feb 26, 2015 at 10:34 AM, Jean-Christophe PLAGNIOL-VILLARD > > <plagnioj@jcrosoft.com> wrote: > > > >> +For each peripheral/bank we will descibe in a u32 if a pin can be > >> +configured in it by putting 1 to the pin bit (1 << pin) > > > > This seems to be describing driver intrinsics in the device tree, like > > how the hardware is routed on the inside and what it can do. > > > > IMO that is driver territory, the driver should know these limitations > > and protest if you try to do something illegal. > > > > Anyway as the AT91 maintainers seem to disagree I will allow some > > more time for discussion before merging the patch. > > > > I can't really have one AT91 maintainer NACKing another, it doesn't > > matter that this is a separate driver, in my book the MAINTAINERS > > entry for AT91 as a whole overrides that so can you please find an > > agreement on how to handle this or I will stall the patch until > > you're in agreement. > > Nicolas has been the de-facto maintainer of AT91 for quite a while > now, even though more of them are listed on the maintainers entry. It > would be inappropriate to merge something that he disagreed with on > that platform. I'm still following it > > > ARM SoC maintainers input would be welcomed. > > It seems appropriate to ask the at91 folks to come back with a > solution that everybody is OK with, and until then hold off merging > this. when the nack is motivated not just I don't like it Best Regards, J.
diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt index b7a93e8..78355ee 100644 --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pinctrl.txt @@ -148,3 +148,69 @@ dbgu: serial@fffff200 { pinctrl-0 = <&pinctrl_dbgu>; status = "disabled"; }; + +II) New Bindings per PIO Block + +This allow to detect pinctrl issue at DT compiling time when disable a bank + +Required properties for iomux controller: +- compatible: "atmel,at91rm9200-pio-pinctrl" or "atmel,at91sam9x5-pio-pinctrl" + or "atmel,sama5d3-pio-pinctrl" +- atmel,mux-mask: array of mask (periph per bank) to describe if a pin can be + configured in this periph mode. All the periph and bank need to be describe. + +How to create such array: + +Each column will represent the possible peripheral of the pinctrl for the bank + +Take an example on the 9260 +Peripheral: 2 ( A and B) +=> + + /* A B */ + 0xffffffff 0xffc00c3b /* pioA */ + +For each peripheral/bank we will descibe in a u32 if a pin can be +configured in it by putting 1 to the pin bit (1 << pin) + +Required properties for pin configuration node: +- atmel,pins: 3 integers array, represents a group of pins mux and config + setting. The format is atmel,pins = <PIN_NUM PERIPH CONFIG>. + The PERIPH 0 means gpio, PERIPH 1 is periph A, PERIPH 2 is periph B... + +Bits used for CONFIG: +cf atmel,at91-pinctrl + +pioB: gpio@fffff600 { + compatible = "atmel,at91rm9200-gpio", "atmel,at91rm9200-pio-pinctrl"; + reg = <0xfffff600 0x200>; + interrupts = <3 IRQ_TYPE_LEVEL_HIGH 1>; + #gpio-cells = <2>; + gpio-controller; + interrupt-controller; + #interrupt-cells = <2>; + clocks = <&pioB_clk>; + + /* A B */ + atmel,mux-mask = <0xffffffff 0x7fff3ccf>; + + dbgu { + pinctrl_dbgu: dbgu-0 { + atmel,pins = + <14 0x1 0x0 /* PB14 periph A */ + 15 0x1 0x1>; /* PB15 periph A with pullup */ + }; + }; +}; + +dbgu: serial@fffff200 { + compatible = "atmel,at91sam9260-usart"; + reg = <0xfffff200 0x200>; + interrupts = <1 4 7>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_dbgu>; + status = "disabled"; +}; + +if you have to use multiple bank + pinctrl-0 = <&pinctrl_ip_piaa>, <&pinctrl_ip_piab>;
Today if we want to disable a pio bank we may will siliently break pinctrl configuration in the DT. This will be detected only at runtime. So move the pinctrl configuration to the bank instead of the bus. This allow to detect pinctrl issue at DT compiling time when disable a bank. Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: devicetree@vger.kernel.org --- .../bindings/pinctrl/atmel,at91-pinctrl.txt | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+)