| Message ID | 1222184138-6151-1-git-send-email-jacmet@sunsite.dk (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 1e16dfc1baa745dd89b95f6e33e4142df6218066 |
| Headers | show |
On Sep 23, 2008, at 10:35 AM, Peter Korsgaard wrote: > Structured similar to the existing QE GPIO support. > > Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk> > Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com> > --- > Changes since v4: > - Respun against powerpc-next, no functional changes > - Added Anton's ack > > Changes since v3: > - Incorporated feedback from Anton > > Changes since v2: > - Clarified documentation as requested by Kumar. > > Changes since v1: > Incorporated feedback from Anton and Kumar: > - Core is also used on 8572/8610 so s/83xx/8xxx/ > - Use fsl,mpc8572-gpio / fsl,mpc8610-gpio for 85xx/86xx as compatible > - Use shadowed data register to handle open drain outputs > - Expandend dts binding doc, use 8347 as example instead of 8349 > - Misc small cleanups applied. - k
On Tue, 23 Sep 2008, Peter Korsgaard wrote: > +- compatible : "fsl,<CHIP>-gpio" followed by "fsl,mpc8349-gpio" for > + 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for 86xx. Why have the three different compatible settings when the code doesn't do anything different? > +#define MPC8XXX_GPIO_PINS 32 8572 has eight GPIOs. I wrote an MPC8572 GPIO driver back in March, and posted it internally at Freescale on June 2nd. But it was just ignored... I wonder what your secret is to get Kumar to apply your patches? It's too bad this work keeps getting duplicated. My patch started out *very* much like yours, except it pre-dated the OF gpio controller stuff and of_mm_gpiochip so it didn't use that. But, I'm using the GPIOs to bit-bang a JTAG bus in the 20-30 MHz range. The obvious GPIO driver is *much* too slow for that. I got less than 3 MHz, and your driver looks like it might be slightly slower than my initial driver. So I went to a lot of effort to speed it up and managed to increase GPIO performance by nearly a factor of 10. Trying to commit my driver at this point is probably hopeless and I doubt anyone else cares about gpio speed. But at least the number of gpios for 8572 can be fixed.
>>>>> "Trent" == Trent Piepho <tpiepho@freescale.com> writes: Trent> On Tue, 23 Sep 2008, Peter Korsgaard wrote: >> +- compatible : "fsl,<CHIP>-gpio" followed by "fsl,mpc8349-gpio" for >> + 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for 86xx. Trent> Why have the three different compatible settings when the code Trent> doesn't do anything different? Purely for cosmetics / ease of use - As requested by Kumar: http://ozlabs.org/pipermail/linuxppc-dev/2008-September/062934.html >> +#define MPC8XXX_GPIO_PINS 32 Trent> 8572 has eight GPIOs. Doesn't matter - It's register interface is compatible. Most real world design with the other SoCs also don't have all 32 gpio pins available because of pin multiplexing. Trent> I wrote an MPC8572 GPIO driver back in March, and posted it Trent> internally at Freescale on June 2nd. But it was just Trent> ignored... I wonder what your secret is to get Kumar to apply Trent> your patches? It's too bad this work keeps getting Trent> duplicated. Did you try bribing? ;) No, seriously, why didn't you post it to linuxppc-dev, so the rest of the world would know about it? Trent> My patch started out *very* much like yours, except it Trent> pre-dated the OF gpio controller stuff and of_mm_gpiochip so Trent> it didn't use that. Trent> But, I'm using the GPIOs to bit-bang a JTAG bus in the 20-30 Trent> MHz range. The obvious GPIO driver is *much* too slow for Trent> that. I got less than 3 MHz, and your driver looks like it Trent> might be slightly slower than my initial driver. I would write a dedicated driver for something like that instead of using gpiolib. Gpiolib has quite some overhead compared to the actual work for changing a SoC gpio pin, but it also has some very nice advantages. For most stuff people use GPIO pins for, the overhead is not an issue (SPI chip selects, leds, keys, reset signals, ..). It's very handy that we nowadays have generic drivers that work with any GPIO (being SoC gpios or stuff on spi/i2c). Trent> So I went to a lot of effort to speed it up and managed to Trent> increase GPIO performance by nearly a factor of 10. Trying to Trent> commit my driver at this point is probably hopeless and I Trent> doubt anyone else cares about gpio speed. But at least the Trent> number of gpios for 8572 can be fixed. Sure, going dedicated always can improve performance. I recently did some work in u-boot talking directly to the mpc83xx spi controller and got ~5x throughput compared to the Linux driver.
On Thu, 30 Oct 2008, Peter Korsgaard wrote: >>>>>> "Trent" == Trent Piepho <tpiepho@freescale.com> writes: > Trent> On Tue, 23 Sep 2008, Peter Korsgaard wrote: > >> +- compatible : "fsl,<CHIP>-gpio" followed by "fsl,mpc8349-gpio" for > >> + 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for 86xx. > > Trent> Why have the three different compatible settings when the code > Trent> doesn't do anything different? > > Purely for cosmetics / ease of use - As requested by Kumar: > > http://ozlabs.org/pipermail/linuxppc-dev/2008-September/062934.html Though I see Scott didn't agree. > >> +#define MPC8XXX_GPIO_PINS 32 > > Trent> 8572 has eight GPIOs. > > Doesn't matter - It's register interface is compatible. Most real > world design with the other SoCs also don't have all 32 gpio pins > available because of pin multiplexing. Doesn't it seem flawed that 32 gpios will show up under debugfs and via the sysfs interface, when it's known that there are only 8? Since there is an 8572 compat property, it's not like it's not solvable. > Trent> I wrote an MPC8572 GPIO driver back in March, and posted it > Trent> internally at Freescale on June 2nd. But it was just > Trent> ignored... I wonder what your secret is to get Kumar to apply > Trent> your patches? It's too bad this work keeps getting > Trent> duplicated. > > Did you try bribing? ;) No, seriously, why didn't you post it to > linuxppc-dev, so the rest of the world would know about it? Well, I get totally ignored posting to an internal Freescale list, it doesn't seem like it will be any better posting to an external list. > Trent> But, I'm using the GPIOs to bit-bang a JTAG bus in the 20-30 > Trent> MHz range. The obvious GPIO driver is *much* too slow for > Trent> that. I got less than 3 MHz, and your driver looks like it > Trent> might be slightly slower than my initial driver. > > I would write a dedicated driver for something like that instead of > using gpiolib. But the JTAG bus only uses four GPIOs, the other four get used for other things with gpiolib, like gpio leds for example. I also found it very handy to be able to see and modify all the gpio lines, even the ones the JTAG driver is using, via a sysfs interface to gpiolib. > Gpiolib has quite some overhead compared to the actual work for > changing a SoC gpio pin, but it also has some very nice Certainly is does, but there are ways to make it faster. > Trent> So I went to a lot of effort to speed it up and managed to > Trent> increase GPIO performance by nearly a factor of 10. Trying to > Trent> commit my driver at this point is probably hopeless and I > Trent> doubt anyone else cares about gpio speed. But at least the > Trent> number of gpios for 8572 can be fixed. > > Sure, going dedicated always can improve performance. I recently did > some work in u-boot talking directly to the mpc83xx spi controller and > got ~5x throughput compared to the Linux driver. It's not dedicated though. The gpiolib interface is still used.
On Oct 30, 2008, at 3:58 AM, Trent Piepho wrote: > On Thu, 30 Oct 2008, Peter Korsgaard wrote: >>>>>>> "Trent" == Trent Piepho <tpiepho@freescale.com> writes: >> Trent> On Tue, 23 Sep 2008, Peter Korsgaard wrote: >>>> +- compatible : "fsl,<CHIP>-gpio" followed by "fsl,mpc8349-gpio" >>>> for >>>> + 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for >>>> 86xx. >> >> Trent> Why have the three different compatible settings when the code >> Trent> doesn't do anything different? >> >> Purely for cosmetics / ease of use - As requested by Kumar: >> >> http://ozlabs.org/pipermail/linuxppc-dev/2008-September/062934.html > > Though I see Scott didn't agree. Yep ;) >>>> +#define MPC8XXX_GPIO_PINS 32 >> >> Trent> 8572 has eight GPIOs. >> >> Doesn't matter - It's register interface is compatible. Most real >> world design with the other SoCs also don't have all 32 gpio pins >> available because of pin multiplexing. > > Doesn't it seem flawed that 32 gpios will show up under debugfs and > via the > sysfs interface, when it's known that there are only 8? > > Since there is an 8572 compat property, it's not like it's not > solvable. seem like a patch is order >> Trent> I wrote an MPC8572 GPIO driver back in March, and posted it >> Trent> internally at Freescale on June 2nd. But it was just >> Trent> ignored... I wonder what your secret is to get Kumar to apply >> Trent> your patches? It's too bad this work keeps getting >> Trent> duplicated. >> >> Did you try bribing? ;) No, seriously, why didn't you post it to >> linuxppc-dev, so the rest of the world would know about it? > > Well, I get totally ignored posting to an internal Freescale list, > it doesn't > seem like it will be any better posting to an external list. Trent I'm not ignoring you, its just I've got a lot of things on my plate. I expect other to comment internally to FSL and wait til something gets posted externally to pay attention. This way its hopefully been reviewed and commented by a number of people before I look at it. Also, we have patchworks externally so I have an explicit todo list that I can track. Don't feel bad. I've ignored patches from others ;) - k
On Thu, 2008-10-30 at 08:45 +0100, Peter Korsgaard wrote: > I would write a dedicated driver for something like that instead of > using gpiolib. > > Gpiolib has quite some overhead compared to the actual work for > changing a SoC gpio pin, but it also has some very nice > advantages. For most stuff people use GPIO pins for, the overhead is > not an issue (SPI chip selects, leds, keys, reset signals, ..). It's > very handy that we nowadays have generic drivers that work with any > GPIO (being SoC gpios or stuff on spi/i2c). For what it's worth, I think it could be useful to have an optimally fast gpiolib driver. This would let you have truly generic bit-bang drivers. Take a look at drivers/spi for example -- there's ~7 bit-bang drivers in there that use spi_bitbang.c for a library of functions. I am not familiar with these drivers, but at a glance it seems feasible that with fast gpiolib implementations you could reduce all of those to a single spi_bitbang.c driver. Trent, it looks like the big performance wins for you were obtained by deviating from the gpiolib interface? In that case a generic SPI/JTAG bit bang driver wouldn't benefit from the speed boosts in your MPC8572 GPIO driver (without explicitly calling mpc8572_gpio_lock and _mpc8572_gpio_set, etc). Is that correct? In any case, I appreciate what you did with your driver, so please don't give up on getting some of your bits merged in :)
On Wed, Oct 29, 2008 at 06:07:47PM -0700, Trent Piepho wrote: [...] > But, I'm using the GPIOs to bit-bang a JTAG bus in the 20-30 MHz range. The > obvious GPIO driver is *much* too slow for that. I got less than 3 MHz, and > your driver looks like it might be slightly slower than my initial driver. You can implement rapid (inlined) SOC-specific versions of gpio_{get,set}_value() functions. And they can co-exist with the gpiolib. It is quite easy to do.
diff --git a/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt b/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt new file mode 100644 index 0000000..d015dce --- /dev/null +++ b/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt @@ -0,0 +1,40 @@ +GPIO controllers on MPC8xxx SoCs + +This is for the non-QE/CPM/GUTs GPIO controllers as found on +8349, 8572, 8610 and compatible. + +Every GPIO controller node must have #gpio-cells property defined, +this information will be used to translate gpio-specifiers. + +Required properties: +- compatible : "fsl,<CHIP>-gpio" followed by "fsl,mpc8349-gpio" for + 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for 86xx. +- #gpio-cells : Should be two. The first cell is the pin number and the + second cell is used to specify optional parameters (currently unused). + - interrupts : Interrupt mapping for GPIO IRQ (currently unused). + - interrupt-parent : Phandle for the interrupt controller that + services interrupts for this device. +- gpio-controller : Marks the port as GPIO controller. + +Example of gpio-controller nodes for a MPC8347 SoC: + + gpio1: gpio-controller@c00 { + #gpio-cells = <2>; + compatible = "fsl,mpc8347-gpio", "fsl,mpc8349-gpio"; + reg = <0xc00 0x100>; + interrupts = <74 0x8>; + interrupt-parent = <&ipic>; + gpio-controller; + }; + + gpio2: gpio-controller@d00 { + #gpio-cells = <2>; + compatible = "fsl,mpc8347-gpio", "fsl,mpc8349-gpio"; + reg = <0xd00 0x100>; + interrupts = <75 0x8>; + interrupt-parent = <&ipic>; + gpio-controller; + }; + +See booting-without-of.txt for details of how to specify GPIO +information for devices. diff --git a/arch/powerpc/sysdev/Kconfig b/arch/powerpc/sysdev/Kconfig index 3965828..d0e7bb0 100644 --- a/arch/powerpc/sysdev/Kconfig +++ b/arch/powerpc/sysdev/Kconfig @@ -7,6 +7,15 @@ config PPC4xx_PCI_EXPRESS depends on PCI && 4xx default n +config MPC8xxx_GPIO + bool "MPC8xxx GPIO support" + depends on PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || PPC_85xx || PPC_86xx + select GENERIC_GPIO + select ARCH_REQUIRE_GPIOLIB + help + Say Y here if you're going to use hardware that connects to the + MPC831x/834x/837x/8572/8610 GPIOs. + config PPC_MSI_BITMAP bool depends on PCI_MSI diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile index b6c269e..55618ba 100644 --- a/arch/powerpc/sysdev/Makefile +++ b/arch/powerpc/sysdev/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_FSL_SOC) += fsl_soc.o obj-$(CONFIG_FSL_PCI) += fsl_pci.o $(fsl-msi-obj-y) obj-$(CONFIG_FSL_LBC) += fsl_lbc.o obj-$(CONFIG_FSL_GTM) += fsl_gtm.o +obj-$(CONFIG_MPC8xxx_GPIO) += mpc8xxx_gpio.o obj-$(CONFIG_RAPIDIO) += fsl_rio.o obj-$(CONFIG_TSI108_BRIDGE) += tsi108_pci.o tsi108_dev.o obj-$(CONFIG_QUICC_ENGINE) += qe_lib/ diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c new file mode 100644 index 0000000..103eace --- /dev/null +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c @@ -0,0 +1,171 @@ +/* + * GPIOs on MPC8349/8572/8610 and compatible + * + * Copyright (C) 2008 Peter Korsgaard <jacmet@sunsite.dk> + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed "as is" without any warranty of any + * kind, whether express or implied. + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/spinlock.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/gpio.h> + +#define MPC8XXX_GPIO_PINS 32 + +#define GPIO_DIR 0x00 +#define GPIO_ODR 0x04 +#define GPIO_DAT 0x08 +#define GPIO_IER 0x0c +#define GPIO_IMR 0x10 +#define GPIO_ICR 0x14 + +struct mpc8xxx_gpio_chip { + struct of_mm_gpio_chip mm_gc; + spinlock_t lock; + + /* + * shadowed data register to be able to clear/set output pins in + * open drain mode safely + */ + u32 data; +}; + +static inline u32 mpc8xxx_gpio2mask(unsigned int gpio) +{ + return 1u << (MPC8XXX_GPIO_PINS - 1 - gpio); +} + +static inline struct mpc8xxx_gpio_chip * +to_mpc8xxx_gpio_chip(struct of_mm_gpio_chip *mm) +{ + return container_of(mm, struct mpc8xxx_gpio_chip, mm_gc); +} + +static void mpc8xxx_gpio_save_regs(struct of_mm_gpio_chip *mm) +{ + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); + + mpc8xxx_gc->data = in_be32(mm->regs + GPIO_DAT); +} + +static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); + + return in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio); +} + +static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) +{ + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); + unsigned long flags; + + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + + if (val) + mpc8xxx_gc->data |= mpc8xxx_gpio2mask(gpio); + else + mpc8xxx_gc->data &= ~mpc8xxx_gpio2mask(gpio); + + out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data); + + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); +} + +static int mpc8xxx_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio) +{ + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); + unsigned long flags; + + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + + clrbits32(mm->regs + GPIO_DIR, mpc8xxx_gpio2mask(gpio)); + + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); + + return 0; +} + +static int mpc8xxx_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) +{ + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); + unsigned long flags; + + mpc8xxx_gpio_set(gc, gpio, val); + + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + + setbits32(mm->regs + GPIO_DIR, mpc8xxx_gpio2mask(gpio)); + + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); + + return 0; +} + +static void __init mpc8xxx_add_controller(struct device_node *np) +{ + struct mpc8xxx_gpio_chip *mpc8xxx_gc; + struct of_mm_gpio_chip *mm_gc; + struct of_gpio_chip *of_gc; + struct gpio_chip *gc; + int ret; + + mpc8xxx_gc = kzalloc(sizeof(*mpc8xxx_gc), GFP_KERNEL); + if (!mpc8xxx_gc) { + ret = -ENOMEM; + goto err; + } + + spin_lock_init(&mpc8xxx_gc->lock); + + mm_gc = &mpc8xxx_gc->mm_gc; + of_gc = &mm_gc->of_gc; + gc = &of_gc->gc; + + mm_gc->save_regs = mpc8xxx_gpio_save_regs; + of_gc->gpio_cells = 2; + gc->ngpio = MPC8XXX_GPIO_PINS; + gc->direction_input = mpc8xxx_gpio_dir_in; + gc->direction_output = mpc8xxx_gpio_dir_out; + gc->get = mpc8xxx_gpio_get; + gc->set = mpc8xxx_gpio_set; + + ret = of_mm_gpiochip_add(np, mm_gc); + if (ret) + goto err; + + return; + +err: + pr_err("%s: registration failed with status %d\n", + np->full_name, ret); + kfree(mpc8xxx_gc); + + return; +} + +static int __init mpc8xxx_add_gpiochips(void) +{ + struct device_node *np; + + for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio") + mpc8xxx_add_controller(np); + + for_each_compatible_node(np, NULL, "fsl,mpc8572-gpio") + mpc8xxx_add_controller(np); + + for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio") + mpc8xxx_add_controller(np); + + return 0; +} +arch_initcall(mpc8xxx_add_gpiochips);