Query on direction_output fn of PL061 GPIO driver.

DISCUSSION Viresh KUMAR 3 participants 8 messages Apr 13, 2010 – Apr 17, 2010 GPIO

Share & tools

Viresh KUMAR

author

2010-04-13 10:50 UTC #0 lore raw

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.

Baruch Siach

2010-04-13 11:32 UTC #1 lore raw

Hi Viresh,

Quoted 12 lines · Viresh KUMAR

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.

Quoted 1 line

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

Signature · 3 lines

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

Viresh KUMAR

author

2010-04-13 11:40 UTC #2 lore raw

Quoted 8 lines · Baruch Siach

Quoted 8 lines
Quoted 8 lines
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.

Quoted 6 lines

Quoted 1 line
Quoted 1 line
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.

Quoted 2 lines

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.

Baruch Siach

2010-04-13 11:53 UTC #3 lore raw

Hi Viresh,

Quoted 20 lines · Viresh KUMAR

Quoted 8 lines · Baruch Siach
Quoted 8 lines
Quoted 8 lines
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.
Quoted 6 lines
Quoted 1 line
Quoted 1 line
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.

Quoted 6 lines

Quoted 2 lines
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

Signature · 3 lines

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

Russell King - ARM Linux

2010-04-13 18:57 UTC #4 lore raw

Quoted 30 lines · Baruch Siach

Hi Viresh,
Quoted 20 lines · Viresh KUMAR
Quoted 8 lines · Baruch Siach
Quoted 8 lines
Quoted 8 lines
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.
Quoted 6 lines
Quoted 1 line
Quoted 1 line
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.
Quoted 6 lines
Quoted 2 lines
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?

Quoted 5 lines

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 <[email protected]>

drivers/gpio/pl061.c +1 -0

index aa8e7cb..cac9c0c 100644
@@ -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;

Baruch Siach

2010-04-14 05:29 UTC #5 lore raw

Hi Russel,

Quoted 51 lines · Russell King - ARM Linux

Quoted 28 lines · Baruch Siach
Quoted 20 lines · Viresh KUMAR
Quoted 8 lines · Baruch Siach
Quoted 8 lines
Quoted 8 lines
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.
Quoted 6 lines
Quoted 1 line
Quoted 1 line
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.
Quoted 6 lines
Quoted 2 lines
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?
Quoted 5 lines
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 <[email protected]>
drivers/gpio/pl061.c +0 -0
index aa8e7cb..cac9c0c 100644

drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 93

Quoted 4 lines

drivers/gpio/pl061.c +1 -0

@@ -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

Quoted 3 lines

drivers/gpio/pl061.c +0 -0

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

Signature · 3 lines

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

Viresh KUMAR

author

2010-04-15 04:14 UTC #6 lore raw

Quoted 8 lines · Baruch Siach

Quoted 8 lines
Quoted 8 lines
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 <[email protected]>

Quoted 11 lines

Quoted 9 lines
drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 93
Quoted 9 lines
drivers/gpio/pl061.c +1 -0
index aa8e7cb..cac9c0c 100644
@@ -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.

Baruch Siach

2010-04-17 18:20 UTC #7 lore raw

Hi Viresh,

Quoted 21 lines · Viresh KUMAR

Quoted 8 lines · Baruch Siach
Quoted 8 lines
Quoted 8 lines
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 <[email protected]>
Quoted 11 lines
Quoted 9 lines
drivers/gpio/pl061.c static int pl061_direction_output(struct gpio_chip *gc, unsigned offset, · line 93
Quoted 9 lines
drivers/gpio/pl061.c +1 -0
index aa8e7cb..cac9c0c 100644
@@ -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.

Quoted 6 lines

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

Signature · 3 lines

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