Query on direction_output fn of PL061 GPIO driver.

Query on direction_output fn of PL061 GPIO driver.

[Viresh KUMAR]

Hello Baruch,

I have a query on PL061 GPIO drivers API pl061_direction_output().
Purpose of this function is to set GPIO pin in OUT mode and set/reset
its value.

In current implementation, firstly value of GPIO pin is modified and then its
direction is set to OUT. In our SOCs (ST SPEAr), this implementation doesn't
work correctly (we have pl061 in our soc). Suppose previously pin is in IN mode,
now writing val to it doesn't change anything because it is in IN mode and we
can write to a pin only when it is in OUT mode. Now if its direction is
changed to OUT then also its value will remain same and will not be changed to
what we wanted.


Am i missing something?

regards,
viresh kumar.

Query on direction_output fn of PL061 GPIO driver.

[Baruch Siach]

Hi Viresh,

On Tue, Apr 13, 2010 at 04:20:38PM +0530, Viresh KUMAR wrote:
> I have a query on PL061 GPIO drivers API pl061_direction_output().
> Purpose of this function is to set GPIO pin in OUT mode and set/reset
> its value.
> 
> In current implementation, firstly value of GPIO pin is modified and then its
> direction is set to OUT. In our SOCs (ST SPEAr), this implementation doesn't
> work correctly (we have pl061 in our soc). Suppose previously pin is in IN mode,
> now writing val to it doesn't change anything because it is in IN mode and we
> can write to a pin only when it is in OUT mode. Now if its direction is
> changed to OUT then also its value will remain same and will not be changed to
> what we wanted.
> 
> Am i missing something?

Probably not. This is a design bug in the PL061 hardware. Changing the 
direction before the value makes the output value of the GPIO undefined 
(apparently 0) for a short period of time. This is bad news for some 
applications.

Anyway, the right thing to do here is to reverse do just that, since the 
current way of doing things is broken.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Query on direction_output fn of PL061 GPIO driver.

[Viresh KUMAR]

On 4/13/2010 5:02 PM, Baruch Siach wrote:
>> > In current implementation, firstly value of GPIO pin is modified and then its
>> > direction is set to OUT. In our SOCs (ST SPEAr), this implementation doesn't
>> > work correctly (we have pl061 in our soc). Suppose previously pin is in IN mode,
>> > now writing val to it doesn't change anything because it is in IN mode and we
>> > can write to a pin only when it is in OUT mode. Now if its direction is
>> > changed to OUT then also its value will remain same and will not be changed to
>> > what we wanted.
>> > 
>> > Am i missing something?
> Probably not. This is a design bug in the PL061 hardware. Changing the 
> direction before the value makes the output value of the GPIO undefined 
> (apparently 0) for a short period of time. This is bad news for some 
> applications.
> 

thanks for your prompt reply.
Ok. so this may cause problem on the board. 
Anyway if this is the issue, then writing value to val register has no effects.

> Anyway, the right thing to do here is to reverse do just that, since the 
> current way of doing things is broken.

I didn't get it completely. If i got it correctly, maybe myself or you can just
send a patch to reverse this, i.e. writing val and setting direction.
Am i correct?

regards,
viresh kumar.

Query on direction_output fn of PL061 GPIO driver.

[Baruch Siach]

Hi Viresh,

On Tue, Apr 13, 2010 at 05:10:41PM +0530, Viresh KUMAR wrote:
> On 4/13/2010 5:02 PM, Baruch Siach wrote:
> >> > In current implementation, firstly value of GPIO pin is modified and then its
> >> > direction is set to OUT. In our SOCs (ST SPEAr), this implementation doesn't
> >> > work correctly (we have pl061 in our soc). Suppose previously pin is in IN mode,
> >> > now writing val to it doesn't change anything because it is in IN mode and we
> >> > can write to a pin only when it is in OUT mode. Now if its direction is
> >> > changed to OUT then also its value will remain same and will not be changed to
> >> > what we wanted.
> >> > 
> >> > Am i missing something?
> > Probably not. This is a design bug in the PL061 hardware. Changing the 
> > direction before the value makes the output value of the GPIO undefined 
> > (apparently 0) for a short period of time. This is bad news for some 
> > applications.
> > 
> 
> thanks for your prompt reply.
> Ok. so this may cause problem on the board. 
> Anyway if this is the issue, then writing value to val register has no effects.
> 
> > Anyway, the right thing to do here is to reverse do just that, since the 
> > current way of doing things is broken.
> 
> I didn't get it completely. If i got it correctly, maybe myself or you can just
> send a patch to reverse this, i.e. writing val and setting direction.
> Am i correct?

Correct. Please send a patch, and I'll ack it after review. Make sure to test 
the patch before sending. I don't have access to this hardware anymore.  
Estimating the length of the jitter on your platform (when switching direction 
and value) would be even better. Please explain the issue and the jitter 
problem in your commit log.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Query on direction_output fn of PL061 GPIO driver.

[Russell King - ARM Linux]

On Tue, Apr 13, 2010 at 02:53:21PM +0300, Baruch Siach wrote:
> Hi Viresh,
> 
> On Tue, Apr 13, 2010 at 05:10:41PM +0530, Viresh KUMAR wrote:
> > On 4/13/2010 5:02 PM, Baruch Siach wrote:
> > >> > In current implementation, firstly value of GPIO pin is modified and then its
> > >> > direction is set to OUT. In our SOCs (ST SPEAr), this implementation doesn't
> > >> > work correctly (we have pl061 in our soc). Suppose previously pin is in IN mode,
> > >> > now writing val to it doesn't change anything because it is in IN mode and we
> > >> > can write to a pin only when it is in OUT mode. Now if its direction is
> > >> > changed to OUT then also its value will remain same and will not be changed to
> > >> > what we wanted.
> > >> > 
> > >> > Am i missing something?
> > > Probably not. This is a design bug in the PL061 hardware. Changing the 
> > > direction before the value makes the output value of the GPIO undefined 
> > > (apparently 0) for a short period of time. This is bad news for some 
> > > applications.
> > > 
> > 
> > thanks for your prompt reply.
> > Ok. so this may cause problem on the board. 
> > Anyway if this is the issue, then writing value to val register has no effects.
> > 
> > > Anyway, the right thing to do here is to reverse do just that, since the 
> > > current way of doing things is broken.
> > 
> > I didn't get it completely. If i got it correctly, maybe myself or you can just
> > send a patch to reverse this, i.e. writing val and setting direction.
> > Am i correct?
> 
> Correct. Please send a patch, and I'll ack it after review. Make sure to test 
> the patch before sending. I don't have access to this hardware anymore.  
> Estimating the length of the jitter on your platform (when switching direction 
> and value) would be even better. Please explain the issue and the jitter 
> problem in your commit log.

I've had this patch floating around for some time to resolve this issue;
though I wasn't sure if it was just a bug in the Realview board I had.

GPIO: Fix PL061 GPIO direction_output to set output level

PL061 hardware does not allow setting the output level prior to
changing the direction to output; we have to set the level after
changing the direction.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
index aa8e7cb..cac9c0c 100644
--- a/drivers/gpio/pl061.c
+++ b/drivers/gpio/pl061.c
@@ -90,6 +90,7 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 	gpiodir = readb(chip->base + GPIODIR);
 	gpiodir |= 1 << offset;
 	writeb(gpiodir, chip->base + GPIODIR);
+	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
 	spin_unlock_irqrestore(&chip->lock, flags);
 
 	return 0;

Query on direction_output fn of PL061 GPIO driver.

[Baruch Siach]

Hi Russel,

On Tue, Apr 13, 2010 at 07:57:46PM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 13, 2010 at 02:53:21PM +0300, Baruch Siach wrote:
> > On Tue, Apr 13, 2010 at 05:10:41PM +0530, Viresh KUMAR wrote:
> > > On 4/13/2010 5:02 PM, Baruch Siach wrote:
> > > >> > In current implementation, firstly value of GPIO pin is modified and then its
> > > >> > direction is set to OUT. In our SOCs (ST SPEAr), this implementation doesn't
> > > >> > work correctly (we have pl061 in our soc). Suppose previously pin is in IN mode,
> > > >> > now writing val to it doesn't change anything because it is in IN mode and we
> > > >> > can write to a pin only when it is in OUT mode. Now if its direction is
> > > >> > changed to OUT then also its value will remain same and will not be changed to
> > > >> > what we wanted.
> > > >> > 
> > > >> > Am i missing something?
> > > > Probably not. This is a design bug in the PL061 hardware. Changing the 
> > > > direction before the value makes the output value of the GPIO undefined 
> > > > (apparently 0) for a short period of time. This is bad news for some 
> > > > applications.
> > > > 
> > > 
> > > thanks for your prompt reply.
> > > Ok. so this may cause problem on the board. 
> > > Anyway if this is the issue, then writing value to val register has no effects.
> > > 
> > > > Anyway, the right thing to do here is to reverse do just that, since the 
> > > > current way of doing things is broken.
> > > 
> > > I didn't get it completely. If i got it correctly, maybe myself or you can just
> > > send a patch to reverse this, i.e. writing val and setting direction.
> > > Am i correct?
> > 
> > Correct. Please send a patch, and I'll ack it after review. Make sure to test 
> > the patch before sending. I don't have access to this hardware anymore.  
> > Estimating the length of the jitter on your platform (when switching direction 
> > and value) would be even better. Please explain the issue and the jitter 
> > problem in your commit log.
> 
> I've had this patch floating around for some time to resolve this issue;
> though I wasn't sure if it was just a bug in the Realview board I had.
> 
> GPIO: Fix PL061 GPIO direction_output to set output level
> 
> PL061 hardware does not allow setting the output level prior to
> changing the direction to output; we have to set the level after
> changing the direction.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
> index aa8e7cb..cac9c0c 100644
> --- a/drivers/gpio/pl061.c
> +++ b/drivers/gpio/pl061.c
> @@ -90,6 +90,7 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
>  	gpiodir = readb(chip->base + GPIODIR);
>  	gpiodir |= 1 << offset;
>  	writeb(gpiodir, chip->base + GPIODIR);
> +	writeb(!!value << offset, chip->base + (1 << (offset + 2)));

IMO we should add a comment here explaining this strange duplicated writeb() 
call. A copy of the commit log above should be enough, I guess.

baruch

>  	spin_unlock_irqrestore(&chip->lock, flags);
>  
>  	return 0;

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Query on direction_output fn of PL061 GPIO driver.

[Viresh KUMAR]

On 4/14/2010 10:59 AM, Baruch Siach wrote:
>> > GPIO: Fix PL061 GPIO direction_output to set output level
>> > 
>> > PL061 hardware does not allow setting the output level prior to
>> > changing the direction to output; we have to set the level after
>> > changing the direction.
>> > 
>> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>> > 
>> > diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
>> > index aa8e7cb..cac9c0c 100644
>> > --- a/drivers/gpio/pl061.c
>> > +++ b/drivers/gpio/pl061.c
>> > @@ -90,6 +90,7 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
>> >  	gpiodir = readb(chip->base + GPIODIR);
>> >  	gpiodir |= 1 << offset;
>> >  	writeb(gpiodir, chip->base + GPIODIR);
>> > +	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
> IMO we should add a comment here explaining this strange duplicated writeb() 
> call. A copy of the commit log above should be enough, I guess.

I feel duplicating writeb() for setting GPIO value is not required. The
first writeb() called before setting direction, doesn't have any affect.
(As we can't write to data register when GPIO is in IN mode). So we may need
only one writeb() for changing value.

Is my understanding correct??

viresh.

Query on direction_output fn of PL061 GPIO driver.

[Baruch Siach]

Hi Viresh,

On Thu, Apr 15, 2010 at 09:44:16AM +0530, Viresh KUMAR wrote:
> On 4/14/2010 10:59 AM, Baruch Siach wrote:
> >> > GPIO: Fix PL061 GPIO direction_output to set output level
> >> > 
> >> > PL061 hardware does not allow setting the output level prior to
> >> > changing the direction to output; we have to set the level after
> >> > changing the direction.
> >> > 
> >> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> >> > 
> >> > diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
> >> > index aa8e7cb..cac9c0c 100644
> >> > --- a/drivers/gpio/pl061.c
> >> > +++ b/drivers/gpio/pl061.c
> >> > @@ -90,6 +90,7 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
> >> >  	gpiodir = readb(chip->base + GPIODIR);
> >> >  	gpiodir |= 1 << offset;
> >> >  	writeb(gpiodir, chip->base + GPIODIR);
> >> > +	writeb(!!value << offset, chip->base + (1 << (offset + 2)));
> > IMO we should add a comment here explaining this strange duplicated writeb() 
> > call. A copy of the commit log above should be enough, I guess.
> 
> I feel duplicating writeb() for setting GPIO value is not required. The
> first writeb() called before setting direction, doesn't have any affect.
> (As we can't write to data register when GPIO is in IN mode). So we may need
> only one writeb() for changing value.
> 
> Is my understanding correct??

Given the present situation you are probably right. However, since this 
behaviour is not documented in the PL061 documentation is may get fixed in 
future implementations. To take advantage of this we need the double writeb().

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -