Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API
On Sun, Jan 27, 2008 at 01:59:51PM -0700, Grant Likely wrote:
a> On 1/27/08, Anton Vorontsov <[EMAIL PROTECTED]> wrote:
> > On Sun, Jan 27, 2008 at 02:42:12PM +0100, Jochen Friedrich wrote:
> > > Hi Anton,
> > >
> > > > +static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
> > > > val)
> > > > +{
> > > > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> > > > + struct port_regs *regs = mm_gc->regs;
> > > > + u32 pin_mask;
> > > > + u32 tmp_val;
> > > > +
> > > > + /* calculate pin location */
> > > > + pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
> > > > +
> > > > + tmp_val = in_be32(®s->cpdata);
> > > > +
> > > > + if (val == 0)
> > > > + out_be32(®s->cpdata, ~pin_mask & tmp_val);
> > > > + else
> > > > + out_be32(®s->cpdata, pin_mask | tmp_val);
> > > > +}
> > >
> > > I see a possible problem with this (and in the corresponding call in
> > > CPM1, as well):
> > >
> > > if there is a pin configured as open drain, you might accidently switch
> > > this pin to 0
> > > while switching a different pin, if an external device is pulling the pin
> > > to 0.
> >
> > Unfortunately I can't think out any workaround for this, except
> > implementing generic gpio_bank_{,un}lock(gpio_pin_on_the_bank), and
> > start using it in the drivers that might care about this issue. Though,
> > looking into i2c-gpio.c I don't clearly see were we can insert these
> > locks, there should be "start/end transaction" handlers or something,
> > but it seems that it's in the bitbanging code, not in the i2c-gpio
> > driver..
> >
> > Actually, I see this as a hardware limitation. For example, on ARMs
> > PXA2xx, there are separate, per bank, read/set/clear GPIO registers,
> > not all-in-one data register.
>
> I've run into this exact issue on other GPIO hardware too. It's not
> uncommon behaviour in GPIO hardware.
>
> The solution is to not depend on the hardware to remember what the
> output pin values should be. Add a shadow register in the driver
> private data. Set the pin state for each output pin in the shadow
> register and then write that value to the hardware. That way input
> state doesn't interfere with the output values.
Great idea, much thanks. Would be easy to implement also.
> Also, you do still need spinlocks around the manipulation of the
> shared registers; otherwise you'll have very hard to debug race
> conditions. Probably one spin lock per bank.
With GPIO LIB we already have per bank spinlock, so it isn't
a problem.
Thanks,
--
Anton Vorontsov
email: [EMAIL PROTECTED]
backup email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API
On 1/27/08, Anton Vorontsov <[EMAIL PROTECTED]> wrote:
> On Sun, Jan 27, 2008 at 02:42:12PM +0100, Jochen Friedrich wrote:
> > Hi Anton,
> >
> > > +static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> > > +{
> > > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> > > + struct port_regs *regs = mm_gc->regs;
> > > + u32 pin_mask;
> > > + u32 tmp_val;
> > > +
> > > + /* calculate pin location */
> > > + pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
> > > +
> > > + tmp_val = in_be32(®s->cpdata);
> > > +
> > > + if (val == 0)
> > > + out_be32(®s->cpdata, ~pin_mask & tmp_val);
> > > + else
> > > + out_be32(®s->cpdata, pin_mask | tmp_val);
> > > +}
> >
> > I see a possible problem with this (and in the corresponding call in CPM1,
> > as well):
> >
> > if there is a pin configured as open drain, you might accidently switch
> > this pin to 0
> > while switching a different pin, if an external device is pulling the pin
> > to 0.
>
> Unfortunately I can't think out any workaround for this, except
> implementing generic gpio_bank_{,un}lock(gpio_pin_on_the_bank), and
> start using it in the drivers that might care about this issue. Though,
> looking into i2c-gpio.c I don't clearly see were we can insert these
> locks, there should be "start/end transaction" handlers or something,
> but it seems that it's in the bitbanging code, not in the i2c-gpio
> driver..
>
> Actually, I see this as a hardware limitation. For example, on ARMs
> PXA2xx, there are separate, per bank, read/set/clear GPIO registers,
> not all-in-one data register.
I've run into this exact issue on other GPIO hardware too. It's not
uncommon behaviour in GPIO hardware.
The solution is to not depend on the hardware to remember what the
output pin values should be. Add a shadow register in the driver
private data. Set the pin state for each output pin in the shadow
register and then write that value to the hardware. That way input
state doesn't interfere with the output values.
Also, you do still need spinlocks around the manipulation of the
shared registers; otherwise you'll have very hard to debug race
conditions. Probably one spin lock per bank.
Here's what I did for a Xilinx GPIO block driver (but ignore the fact
that I'm not using driver private data so only one GPIO block can be
supported; that will be fixed before I post this driver)
+void gpio_set_value(unsigned gpio, int value)
+{
+ unsigned long flags;
+
+ if (!gpio_regs)
+ return;
+
+ spin_lock_irqsave(&gpio_spinlock, flags);
+ if (value)
+ gpio_output_state |= 1<
> --
> Anton Vorontsov
> email: [EMAIL PROTECTED]
> backup email: [EMAIL PROTECTED]
> irc://irc.freenode.net/bd2
> ___
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API
On Sun, Jan 27, 2008 at 02:42:12PM +0100, Jochen Friedrich wrote:
> Hi Anton,
>
> > +static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> > +{
> > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> > + struct port_regs *regs = mm_gc->regs;
> > + u32 pin_mask;
> > + u32 tmp_val;
> > +
> > + /* calculate pin location */
> > + pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
> > +
> > + tmp_val = in_be32(®s->cpdata);
> > +
> > + if (val == 0)
> > + out_be32(®s->cpdata, ~pin_mask & tmp_val);
> > + else
> > + out_be32(®s->cpdata, pin_mask | tmp_val);
> > +}
>
> I see a possible problem with this (and in the corresponding call in CPM1, as
> well):
>
> if there is a pin configured as open drain, you might accidently switch this
> pin to 0
> while switching a different pin, if an external device is pulling the pin to
> 0.
Unfortunately I can't think out any workaround for this, except
implementing generic gpio_bank_{,un}lock(gpio_pin_on_the_bank), and
start using it in the drivers that might care about this issue. Though,
looking into i2c-gpio.c I don't clearly see were we can insert these
locks, there should be "start/end transaction" handlers or something,
but it seems that it's in the bitbanging code, not in the i2c-gpio
driver..
Actually, I see this as a hardware limitation. For example, on ARMs
PXA2xx, there are separate, per bank, read/set/clear GPIO registers,
not all-in-one data register.
--
Anton Vorontsov
email: [EMAIL PROTECTED]
backup email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/3] [POWERPC] QE: implement GPIO LIB API
Hi Anton,
> +static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> + struct port_regs *regs = mm_gc->regs;
> + u32 pin_mask;
> + u32 tmp_val;
> +
> + /* calculate pin location */
> + pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
> +
> + tmp_val = in_be32(®s->cpdata);
> +
> + if (val == 0)
> + out_be32(®s->cpdata, ~pin_mask & tmp_val);
> + else
> + out_be32(®s->cpdata, pin_mask | tmp_val);
> +}
I see a possible problem with this (and in the corresponding call in CPM1, as
well):
if there is a pin configured as open drain, you might accidently switch this
pin to 0
while switching a different pin, if an external device is pulling the pin to 0.
i2c-gpio.c and w1-gpio.c (in -mm) are examples of drivers which use open drain
pins
and which would fail if another pin on the same port would be set during a
transfer.
Thanks,
Jochen
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH 3/3] [POWERPC] QE: implement GPIO LIB API
Signed-off-by: Anton Vorontsov <[EMAIL PROTECTED]>
---
Documentation/powerpc/booting-without-of.txt | 32 +++
arch/powerpc/platforms/Kconfig |2 +
arch/powerpc/sysdev/qe_lib/qe_io.c | 73 ++
include/asm-powerpc/qe.h |1 +
4 files changed, 96 insertions(+), 12 deletions(-)
diff --git a/Documentation/powerpc/booting-without-of.txt
b/Documentation/powerpc/booting-without-of.txt
index dd2613c..e279152 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1696,24 +1696,32 @@ platforms are moved over to use the
flattened-device-tree model.
information.
Required properties:
- - device_type : should be "par_io".
+ - #gpio-cells : should be "1".
+ - compatible : should be "fsl,qe-pario-bank"
- reg : offset to the register set and its length.
- - num-ports : number of Parallel I/O ports
+ - gpio-controller : node to identify gpio controllers.
- Example:
- [EMAIL PROTECTED] {
- reg = <1400 100>;
- #address-cells = <1>;
- #size-cells = <0>;
- device_type = "par_io";
- num-ports = <7>;
- [EMAIL PROTECTED] {
- ..
- };
+ For example, two QE Par I/O banks:
+ qe_pio_a: [EMAIL PROTECTED] {
+ #gpio-cells = <1>;
+ compatible = "fsl,qe-pario-bank";
+ reg = <0x1400 0x18>;
+ gpio-controller;
+ };
+ qe_pio_e: [EMAIL PROTECTED] {
+ #gpio-cells = <1>;
+ compatible = "fsl,qe-pario-bank";
+ reg = <0x1460 0x18>;
+ gpio-controller;
+ };
vi) Pin configuration nodes
+ NOTE: pin configuration nodes are obsolete. Usually, their existance
+ is an evidence of the firmware shortcomings. Such fixups are
+ better handled by the Linux board file, not the device tree.
+
Required properties:
- linux,phandle : phandle of this node; likely referenced by a QE
device.
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index ea22cad..8dff946 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -265,6 +265,8 @@ config TAU_AVERAGE
config QUICC_ENGINE
bool
select PPC_LIB_RHEAP
+ select GENERIC_GPIO
+ select GPIO_LIB
help
The QUICC Engine (QE) is a new generation of communications
coprocessors on Freescale embedded CPUs (akin to CPM in older chips).
diff --git a/arch/powerpc/sysdev/qe_lib/qe_io.c
b/arch/powerpc/sysdev/qe_lib/qe_io.c
index aef893b..2a1ff45 100644
--- a/arch/powerpc/sysdev/qe_lib/qe_io.c
+++ b/arch/powerpc/sysdev/qe_lib/qe_io.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#undef DEBUG
@@ -213,6 +214,78 @@ int par_io_of_config(struct device_node *np)
}
EXPORT_SYMBOL(par_io_of_config);
+/*
+ * GPIO LIB API implementation
+ */
+
+static int qe_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct port_regs *regs = mm_gc->regs;
+ u32 pin_mask;
+
+ /* calculate pin location */
+ pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
+
+ return !!(in_be32(®s->cpdata) & pin_mask);
+}
+
+static void qe_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct port_regs *regs = mm_gc->regs;
+ u32 pin_mask;
+ u32 tmp_val;
+
+ /* calculate pin location */
+ pin_mask = (u32) (1 << (NUM_OF_PINS - 1 - gpio));
+
+ tmp_val = in_be32(®s->cpdata);
+
+ if (val == 0)
+ out_be32(®s->cpdata, ~pin_mask & tmp_val);
+ else
+ out_be32(®s->cpdata, pin_mask | tmp_val);
+}
+
+static int qe_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ __par_io_config_pin(mm_gc->regs, gpio, 2, 0, 0, 0);
+
+ return 0;
+}
+
+static int qe_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+
+ __par_io_config_pin(mm_gc->regs, gpio, 1, 0, 0, 0);
+ qe_gpio_set(gc, gpio, val);
+
+ return 0;
+}
+
+static struct of_gpio_chip qe_gc = {
+ .gpio_cells = 1,
+ .xlate = of_gpio_simple_xlate,
+
+ .gc = {
+ .ngpio = NUM_OF_PINS,
+ .direction_input = qe_gpio_dir_in,
+ .direction_output = qe_gpio_dir_out,
+ .get = qe_gpio_get,
+ .set = qe_gpio_set,
+ },
+};
+
+int qe_gpiochip_add(struct device_node *np)
+{
+ return of_mm_gpiochip_add(np, &qe_gc);
+}
+EXPORT_SYMBOL(qe_gpiochip_add);
+
#ifdef DEBUG
static void dump_par_io(void)
{
diff --git a/include/asm-power
