| Message ID | 50CF0744.7040404@grandegger.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 17, 2012 at 12:51:32PM +0100, Wolfgang Grandegger wrote: > +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) > +{ > + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); > + void __iomem *pio = at91_gpio->regbase; > + u32 set_bits = val & mask; > + u32 clr_bits = ~val & mask; > + > + /* GPIO outputs can only be set at once or cleared at once */ > + if (set_bits) > + __raw_writel(set_bits, pio + PIO_SODR); > + if (clr_bits) > + __raw_writel(clr_bits, pio + PIO_CODR); > +} You obviously didn't see my email...
On 12/17/2012 12:51 PM, Wolfgang Grandegger wrote: >>> Without having an AT91 available right now, I guess the hardware >>> interface of this GPIO chip is different from the GPIO block API. While >>> the hardware has clear and set registers, the val parameter of >>> at91_gpiolib_set_block() should be interpreted as the actual output >>> values. See lpc32xx_gpo_set_block() for an example for handling set and >>> clear registers like this: First, set_bits and clear_bits words are >>> calculated from mask and val parameters, and finally written to the >>> respective hardware registers. >>> >>> Note that one .set_block() can result in writing both the set and clear >>> registers of the hardware when val contains both 0s and 1s in >>> respectively masked positions. >> >> Oops, I obviously did not test GPIO block write. The patch below does >> work now. Feel free to add it to the next version of your series. > > The patch lacks an important fix, sorry. Please consider the updated > patch below. Thanks! And I guess Russell is right: If possible, we should write outputs simultaneously via ODSR (plus OWER/OWDR/OWSR) instead of separate set/clear. I wonder if we need to save/restore the state of OWSR at every write operation or if we need/can cache it. Assuming that block GPIO are the only code in the kernel that manipulates ODSR. Further: Can we include this patch for arch/arm/mach-at91 via the gpio subsystem or does it need to go separately via arm-soc.git? Thanks, Roland
Hi Wolfgang, On 12/17/2012 02:32 PM, Roland Stigge wrote: > And I guess Russell is right: If possible, we should write outputs > simultaneously via ODSR (plus OWER/OWDR/OWSR) instead of separate set/clear. > > I wonder if we need to save/restore the state of OWSR at every write > operation or if we need/can cache it. Assuming that block GPIO are the > only code in the kernel that manipulates ODSR. Can you please test the following: +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) +{ + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); + void __iomem *pio = at91_gpio->regbase; + + __raw_writel(~mask, pio + PIO_OWDR); + __raw_writel(mask, pio + PIO_OWER); + __raw_writel(val, pio + PIO_ODSR); +} Would caching OWSR be a significant speedup here? Thanks in advance, Roland
On 12/17/2012 01:10 PM, Russell King - ARM Linux wrote: > On Mon, Dec 17, 2012 at 12:51:32PM +0100, Wolfgang Grandegger wrote: >> +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) >> +{ >> + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); >> + void __iomem *pio = at91_gpio->regbase; >> + u32 set_bits = val & mask; >> + u32 clr_bits = ~val & mask; >> + >> + /* GPIO outputs can only be set at once or cleared at once */ >> + if (set_bits) >> + __raw_writel(set_bits, pio + PIO_SODR); >> + if (clr_bits) >> + __raw_writel(clr_bits, pio + PIO_CODR); >> +} > > You obviously didn't see my email... Well, I did read you mail but it was not obvious to me how to maintain compatibility with the existing "set" method... at a first glance. But the synchronous data output is important and it's even simpler than I thought. The block-set callback can directly write to ODSR because "at91_set_gpio_output()" already has set OWER. Will give that a try later today. Wolfgang.
On 12/17/2012 02:51 PM, Roland Stigge wrote: > Hi Wolfgang, > > On 12/17/2012 02:32 PM, Roland Stigge wrote: >> And I guess Russell is right: If possible, we should write outputs >> simultaneously via ODSR (plus OWER/OWDR/OWSR) instead of separate set/clear. >> >> I wonder if we need to save/restore the state of OWSR at every write >> operation or if we need/can cache it. Assuming that block GPIO are the >> only code in the kernel that manipulates ODSR. > > Can you please test the following: > > +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) > +{ > + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); > + void __iomem *pio = at91_gpio->regbase; > + > + __raw_writel(~mask, pio + PIO_OWDR); This would also disable normal GPIOs configured for output! From the manual I understand that if the pin is configured for output, we could either use PIO_SODR/PIO_CODR to set/clear the bits individually or PIO_ODSR for synchronous data output. But than we need to care about the non-block GPIO outputs as well... requiring a read-modify-write cycle :(. Wolfgang.
On 12/17/2012 05:28 PM, Wolfgang Grandegger wrote: > On 12/17/2012 02:51 PM, Roland Stigge wrote: >> Hi Wolfgang, >>> And I guess Russell is right: If possible, we should write outputs >>> simultaneously via ODSR (plus OWER/OWDR/OWSR) instead of separate set/clear. >>> >>> I wonder if we need to save/restore the state of OWSR at every write >>> operation or if we need/can cache it. Assuming that block GPIO are the >>> only code in the kernel that manipulates ODSR. >> >> Can you please test the following: >> >> +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) >> +{ >> + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); >> + void __iomem *pio = at91_gpio->regbase; >> + >> + __raw_writel(~mask, pio + PIO_OWDR); > > This would also disable normal GPIOs configured for output! From the > manual I understand that if the pin is configured for output, we could > either use PIO_SODR/PIO_CODR to set/clear the bits individually or > PIO_ODSR for synchronous data output. But than we need to care about the > non-block GPIO outputs as well... requiring a read-modify-write cycle :(. From the manual, I read about OWER: "Enables writing PIO_ODSR for the I/O line" (analogous for OWDR). Would interpret this as affecting ODSR (for block GPIO) but not SODR/CODR (as currently with single GPIOs). Have you tried? ;-) Thanks in advance, Roland
On 12/17/2012 06:15 PM, Roland Stigge wrote: > On 12/17/2012 05:28 PM, Wolfgang Grandegger wrote: >> On 12/17/2012 02:51 PM, Roland Stigge wrote: >>> Hi Wolfgang, >>>> And I guess Russell is right: If possible, we should write outputs >>>> simultaneously via ODSR (plus OWER/OWDR/OWSR) instead of separate set/clear. >>>> >>>> I wonder if we need to save/restore the state of OWSR at every write >>>> operation or if we need/can cache it. Assuming that block GPIO are the >>>> only code in the kernel that manipulates ODSR. >>> >>> Can you please test the following: >>> >>> +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) >>> +{ >>> + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); >>> + void __iomem *pio = at91_gpio->regbase; >>> + >>> + __raw_writel(~mask, pio + PIO_OWDR); >> >> This would also disable normal GPIOs configured for output! From the >> manual I understand that if the pin is configured for output, we could >> either use PIO_SODR/PIO_CODR to set/clear the bits individually or >> PIO_ODSR for synchronous data output. But than we need to care about the >> non-block GPIO outputs as well... requiring a read-modify-write cycle :(. > >>From the manual, I read about OWER: "Enables writing PIO_ODSR for the > I/O line" (analogous for OWDR). Would interpret this as affecting ODSR > (for block GPIO) but not SODR/CODR (as currently with single GPIOs). > > Have you tried? ;-) Grrr, I mixed OER with OWER, sorry for the noise. Back to your approach, which works. /* Do synchronous data output with a single write access */ __raw_writel(~mask, pio + PIO_OWDR); __raw_writel(mask, pio + PIO_OWER); __raw_writel(val, pio + PIO_ODSR); For caching we would need a storage. Not sure if it's worth compared to a context switch into the kernel. Wolfgang.
On 12/17/2012 06:37 PM, Wolfgang Grandegger wrote: > /* Do synchronous data output with a single write access */ > __raw_writel(~mask, pio + PIO_OWDR); > __raw_writel(mask, pio + PIO_OWER); > __raw_writel(val, pio + PIO_ODSR); > > For caching we would need a storage. Not sure if it's worth compared to > a context switch into the kernel. Block GPIO is not only for you in userspace. ;-) You can also implement efficient n-bit bus I/O in kernel drivers, n-bit-banging. :-) So not always context switches involved. Roland
diff --git a/arch/arm/mach-at91/gpio.c b/arch/arm/mach-at91/gpio.c index be42cf0..77c6f91 100644 --- a/arch/arm/mach-at91/gpio.c +++ b/arch/arm/mach-at91/gpio.c @@ -48,7 +48,9 @@ struct at91_gpio_chip { static void at91_gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip); static void at91_gpiolib_set(struct gpio_chip *chip, unsigned offset, int val); +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val); static int at91_gpiolib_get(struct gpio_chip *chip, unsigned offset); +static unsigned long at91_gpiolib_get_block(struct gpio_chip *chip, unsigned long mask); static int at91_gpiolib_direction_output(struct gpio_chip *chip, unsigned offset, int val); static int at91_gpiolib_direction_input(struct gpio_chip *chip, @@ -62,7 +64,9 @@ static int at91_gpiolib_to_irq(struct gpio_chip *chip, unsigned offset); .direction_input = at91_gpiolib_direction_input, \ .direction_output = at91_gpiolib_direction_output, \ .get = at91_gpiolib_get, \ + .get_block = at91_gpiolib_get_block, \ .set = at91_gpiolib_set, \ + .set_block = at91_gpiolib_set_block, \ .dbg_show = at91_gpiolib_dbg_show, \ .to_irq = at91_gpiolib_to_irq, \ .ngpio = nr_gpio, \ @@ -896,6 +900,16 @@ static int at91_gpiolib_get(struct gpio_chip *chip, unsigned offset) return (pdsr & mask) != 0; } +static unsigned long at91_gpiolib_get_block(struct gpio_chip *chip, unsigned long mask) +{ + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); + void __iomem *pio = at91_gpio->regbase; + u32 pdsr; + + pdsr = __raw_readl(pio + PIO_PDSR); + return pdsr & mask; +} + static void at91_gpiolib_set(struct gpio_chip *chip, unsigned offset, int val) { struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); @@ -905,6 +919,20 @@ static void at91_gpiolib_set(struct gpio_chip *chip, unsigned offset, int val) __raw_writel(mask, pio + (val ? PIO_SODR : PIO_CODR)); } +static void at91_gpiolib_set_block(struct gpio_chip *chip, unsigned long mask, unsigned long val) +{ + struct at91_gpio_chip *at91_gpio = to_at91_gpio_chip(chip); + void __iomem *pio = at91_gpio->regbase; + u32 set_bits = val & mask; + u32 clr_bits = ~val & mask; + + /* GPIO outputs can only be set at once or cleared at once */ + if (set_bits) + __raw_writel(set_bits, pio + PIO_SODR); + if (clr_bits) + __raw_writel(clr_bits, pio + PIO_CODR); +} + static void at91_gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) { int i;