* [PATCH] qe_lib: Set gpio data before changing the direction to output @ 2009-08-18 20:59 Michael Barkowski [not found] ` <20090818210805.GA1725@oksana.dev.rtsoft.ru> [not found] ` <4A8B183D.2030202@freescale.com> 0 siblings, 2 replies; 9+ messages in thread From: Michael Barkowski @ 2009-08-18 20:59 UTC (permalink / raw) To: linuxppc-dev; +Cc: Timur Tabi This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register. Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> --- I can't think of a reason not to do this. The data register has no effect except when the pin is configured as an output, right? Please enlighten me if this is not correct. The behaviour I see gels with my thinking, but there may be a case I haven't thought of. arch/powerpc/sysdev/qe_lib/gpio.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/sysdev/qe_lib/gpio.c b/arch/powerpc/sysdev/qe_lib/gpio.c index 3485288..e7bf136 100644 --- a/arch/powerpc/sysdev/qe_lib/gpio.c +++ b/arch/powerpc/sysdev/qe_lib/gpio.c @@ -107,12 +107,11 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) spin_lock_irqsave(&qe_gc->lock, flags); + qe_gpio_set(gc, gpio, val); __par_io_config_pin(mm_gc->regs, gpio, QE_PIO_DIR_OUT, 0, 0, 0); spin_unlock_irqrestore(&qe_gc->lock, flags); - qe_gpio_set(gc, gpio, val); - return 0; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <20090818210805.GA1725@oksana.dev.rtsoft.ru>]
* [PATCH v2] qe_lib: Set gpio data before changing the direction to output [not found] ` <20090818210805.GA1725@oksana.dev.rtsoft.ru> @ 2009-08-18 21:20 ` Michael Barkowski 2009-08-18 21:33 ` Anton Vorontsov 2009-08-25 14:44 ` Kumar Gala 0 siblings, 2 replies; 9+ messages in thread From: Michael Barkowski @ 2009-08-18 21:20 UTC (permalink / raw) To: avorontsov; +Cc: linuxppc-dev, Timur Tabi This avoids having a short glitch if the desired initial value is not the same as what was previously in the data register. Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> --- Anton Vorontsov wrote: > There is a recursive locking bug: _set() takes the same spinlock. > So you'd better move this call two lines upper. Otherwise the > patch looks OK. > > Thanks! Thanks - here is v2. arch/powerpc/sysdev/qe_lib/gpio.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/sysdev/qe_lib/gpio.c b/arch/powerpc/sysdev/qe_lib/gpio.c index 3485288..8e7a776 100644 --- a/arch/powerpc/sysdev/qe_lib/gpio.c +++ b/arch/powerpc/sysdev/qe_lib/gpio.c @@ -105,14 +105,14 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc); unsigned long flags; + qe_gpio_set(gc, gpio, val); + spin_lock_irqsave(&qe_gc->lock, flags); __par_io_config_pin(mm_gc->regs, gpio, QE_PIO_DIR_OUT, 0, 0, 0); spin_unlock_irqrestore(&qe_gc->lock, flags); - qe_gpio_set(gc, gpio, val); - return 0; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] qe_lib: Set gpio data before changing the direction to output 2009-08-18 21:20 ` [PATCH v2] " Michael Barkowski @ 2009-08-18 21:33 ` Anton Vorontsov 2009-08-18 22:33 ` Timur Tabi 2009-08-25 14:44 ` Kumar Gala 1 sibling, 1 reply; 9+ messages in thread From: Anton Vorontsov @ 2009-08-18 21:33 UTC (permalink / raw) To: Michael Barkowski; +Cc: linuxppc-dev, Timur Tabi On Tue, Aug 18, 2009 at 05:20:44PM -0400, Michael Barkowski wrote: > This avoids having a short glitch if the desired initial value is not > the same as what was previously in the data register. > > Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com> Thanks! > --- > Anton Vorontsov wrote: > > There is a recursive locking bug: _set() takes the same spinlock. > > So you'd better move this call two lines upper. Otherwise the > > patch looks OK. > > > > Thanks! > > Thanks - here is v2. > > arch/powerpc/sysdev/qe_lib/gpio.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/sysdev/qe_lib/gpio.c b/arch/powerpc/sysdev/qe_lib/gpio.c > index 3485288..8e7a776 100644 > --- a/arch/powerpc/sysdev/qe_lib/gpio.c > +++ b/arch/powerpc/sysdev/qe_lib/gpio.c > @@ -105,14 +105,14 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc); > unsigned long flags; > > + qe_gpio_set(gc, gpio, val); > + > spin_lock_irqsave(&qe_gc->lock, flags); > > __par_io_config_pin(mm_gc->regs, gpio, QE_PIO_DIR_OUT, 0, 0, 0); > > spin_unlock_irqrestore(&qe_gc->lock, flags); > > - qe_gpio_set(gc, gpio, val); > - > return 0; > } > > -- > 1.6.3.3 > -- Anton Vorontsov email: cbouatmailru@gmail.com irc://irc.freenode.net/bd2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] qe_lib: Set gpio data before changing the direction to output 2009-08-18 21:33 ` Anton Vorontsov @ 2009-08-18 22:33 ` Timur Tabi 2009-08-18 22:56 ` Anton Vorontsov 0 siblings, 1 reply; 9+ messages in thread From: Timur Tabi @ 2009-08-18 22:33 UTC (permalink / raw) To: avorontsov; +Cc: linuxppc-dev, Michael Barkowski Anton Vorontsov wrote: > On Tue, Aug 18, 2009 at 05:20:44PM -0400, Michael Barkowski wrote: >> This avoids having a short glitch if the desired initial value is not >> the same as what was previously in the data register. >> >> Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> > > Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com> I don't have the time to test this patch, so I abstain from acking. :-) If Anton likes it, that's good enough for me. -- Timur Tabi Linux kernel developer at Freescale ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] qe_lib: Set gpio data before changing the direction to output 2009-08-18 22:33 ` Timur Tabi @ 2009-08-18 22:56 ` Anton Vorontsov 2009-08-19 13:30 ` Michael Barkowski 0 siblings, 1 reply; 9+ messages in thread From: Anton Vorontsov @ 2009-08-18 22:56 UTC (permalink / raw) To: Timur Tabi; +Cc: linuxppc-dev, Michael Barkowski On Tue, Aug 18, 2009 at 05:33:00PM -0500, Timur Tabi wrote: > Anton Vorontsov wrote: > > On Tue, Aug 18, 2009 at 05:20:44PM -0400, Michael Barkowski wrote: > >> This avoids having a short glitch if the desired initial value is not > >> the same as what was previously in the data register. > >> > >> Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> > > > > Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com> > > I don't have the time to test this patch, so I abstain from acking. :-) > If Anton likes it, that's good enough for me. You made me doubt for a moment. :-) Thanks for the suspiciousness. What happens if a pin was previously configured as input? Does our write to the data register survive? For MPC8xxx GPIO controllers it does. And randomly taken QE spec says: A write to CPDAT is latched, and if the corresponding CPDIR bits have configured the port pin as an output, the latched value is driven onto the respective pin. However, if the corresponding CPDIR bits have configured the port pin as an input, the latched value is prevented from reaching the pin. I guess we're safe, but Michael, could you actually test it (if not already)? Thanks! -- Anton Vorontsov email: cbouatmailru@gmail.com irc://irc.freenode.net/bd2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] qe_lib: Set gpio data before changing the direction to output 2009-08-18 22:56 ` Anton Vorontsov @ 2009-08-19 13:30 ` Michael Barkowski 2009-08-19 13:32 ` Anton Vorontsov 0 siblings, 1 reply; 9+ messages in thread From: Michael Barkowski @ 2009-08-19 13:30 UTC (permalink / raw) To: avorontsov; +Cc: linuxppc-dev, Timur Tabi Anton Vorontsov wrote: > On Tue, Aug 18, 2009 at 05:33:00PM -0500, Timur Tabi wrote: >> Anton Vorontsov wrote: >>> On Tue, Aug 18, 2009 at 05:20:44PM -0400, Michael Barkowski wrote: >>>> This avoids having a short glitch if the desired initial value is not >>>> the same as what was previously in the data register. >>>> >>>> Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> >>> Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com> >> I don't have the time to test this patch, so I abstain from acking. :-) >> If Anton likes it, that's good enough for me. > > You made me doubt for a moment. :-) Thanks for the suspiciousness. > > What happens if a pin was previously configured as input? Does our > write to the data register survive? For MPC8xxx GPIO controllers > it does. And randomly taken QE spec says: > > A write to CPDAT is latched, and if the corresponding CPDIR > bits have configured the port pin as an output, the latched > value is driven onto the respective pin. However, if the > corresponding CPDIR bits have configured the port pin as an > input, the latched value is prevented from reaching the pin. > > I guess we're safe, but Michael, could you actually test it > (if not already)? > I had tested it before with the pin initially configured as "disabled". I have now also tested it with the pin initially configured as "input". The value written to CPDAT seems to survive and is driven onto the pin once CPDIR is changed to 1, just as noted in the spec. Tested on 8360, by probing with a logic analyzer. There are lots of users of this code. I understand if you'd like it to stay open for testing by others. -- Michael Barkowski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] qe_lib: Set gpio data before changing the direction to output 2009-08-19 13:30 ` Michael Barkowski @ 2009-08-19 13:32 ` Anton Vorontsov 0 siblings, 0 replies; 9+ messages in thread From: Anton Vorontsov @ 2009-08-19 13:32 UTC (permalink / raw) To: Michael Barkowski; +Cc: linuxppc-dev, Timur Tabi On Wed, Aug 19, 2009 at 09:30:20AM -0400, Michael Barkowski wrote: > Anton Vorontsov wrote: > > On Tue, Aug 18, 2009 at 05:33:00PM -0500, Timur Tabi wrote: > >> Anton Vorontsov wrote: > >>> On Tue, Aug 18, 2009 at 05:20:44PM -0400, Michael Barkowski wrote: > >>>> This avoids having a short glitch if the desired initial value is not > >>>> the same as what was previously in the data register. > >>>> > >>>> Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> > >>> Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com> > >> I don't have the time to test this patch, so I abstain from acking. :-) > >> If Anton likes it, that's good enough for me. > > > > You made me doubt for a moment. :-) Thanks for the suspiciousness. > > > > What happens if a pin was previously configured as input? Does our > > write to the data register survive? For MPC8xxx GPIO controllers > > it does. And randomly taken QE spec says: > > > > A write to CPDAT is latched, and if the corresponding CPDIR > > bits have configured the port pin as an output, the latched > > value is driven onto the respective pin. However, if the > > corresponding CPDIR bits have configured the port pin as an > > input, the latched value is prevented from reaching the pin. > > > > I guess we're safe, but Michael, could you actually test it > > (if not already)? > > > > I had tested it before with the pin initially configured as "disabled". > > I have now also tested it with the pin initially configured as "input". > > The value written to CPDAT seems to survive and is driven onto the pin > once CPDIR is changed to 1, just as noted in the spec. > > Tested on 8360, by probing with a logic analyzer. Great, thanks a lot! I think the patch is perfect. -- Anton Vorontsov email: cbouatmailru@gmail.com irc://irc.freenode.net/bd2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] qe_lib: Set gpio data before changing the direction to output 2009-08-18 21:20 ` [PATCH v2] " Michael Barkowski 2009-08-18 21:33 ` Anton Vorontsov @ 2009-08-25 14:44 ` Kumar Gala 1 sibling, 0 replies; 9+ messages in thread From: Kumar Gala @ 2009-08-25 14:44 UTC (permalink / raw) To: Michael Barkowski; +Cc: linuxppc-dev, Timur Tabi On Aug 18, 2009, at 4:20 PM, Michael Barkowski wrote: > This avoids having a short glitch if the desired initial value is not > the same as what was previously in the data register. > > Signed-off-by: Michael Barkowski <michaelbarkowski@ruggedcom.com> > --- > Anton Vorontsov wrote: >> There is a recursive locking bug: _set() takes the same spinlock. >> So you'd better move this call two lines upper. Otherwise the >> patch looks OK. >> >> Thanks! > > Thanks - here is v2. > > arch/powerpc/sysdev/qe_lib/gpio.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) applied to next. - k ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <4A8B183D.2030202@freescale.com>]
* Re: [PATCH] qe_lib: Set gpio data before changing the direction to output [not found] ` <4A8B183D.2030202@freescale.com> @ 2009-08-18 21:23 ` Michael Barkowski 0 siblings, 0 replies; 9+ messages in thread From: Michael Barkowski @ 2009-08-18 21:23 UTC (permalink / raw) To: Timur Tabi; +Cc: linuxppc-dev Timur Tabi wrote: > Michael Barkowski wrote: > >> diff --git a/arch/powerpc/sysdev/qe_lib/gpio.c b/arch/powerpc/sysdev/qe_lib/gpio.c >> index 3485288..e7bf136 100644 >> --- a/arch/powerpc/sysdev/qe_lib/gpio.c >> +++ b/arch/powerpc/sysdev/qe_lib/gpio.c >> @@ -107,12 +107,11 @@ static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) >> >> spin_lock_irqsave(&qe_gc->lock, flags); >> >> + qe_gpio_set(gc, gpio, val); > > qe_gpio_set already calls spin_lock_irqsave(), so you'll have nested spinlocks, which will lock up on SMP. > > Let me guess, you didn't test this on a dual-core system? That is correct. See v2 and please test, YMMV, etc -- Michael Barkowski ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-08-25 14:45 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-18 20:59 [PATCH] qe_lib: Set gpio data before changing the direction to output Michael Barkowski [not found] ` <20090818210805.GA1725@oksana.dev.rtsoft.ru> 2009-08-18 21:20 ` [PATCH v2] " Michael Barkowski 2009-08-18 21:33 ` Anton Vorontsov 2009-08-18 22:33 ` Timur Tabi 2009-08-18 22:56 ` Anton Vorontsov 2009-08-19 13:30 ` Michael Barkowski 2009-08-19 13:32 ` Anton Vorontsov 2009-08-25 14:44 ` Kumar Gala [not found] ` <4A8B183D.2030202@freescale.com> 2009-08-18 21:23 ` [PATCH] " Michael Barkowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).