Patchwork Add MPC52xx simple interrupt GPIO support

login
register
mail settings
Submitter Roman Fietze
Date Feb. 26, 2009, 2:24 p.m.
Message ID <200902261524.51750.roman.fietze@telemotive.de>
Download mbox | patch
Permalink /patch/23774/
State Changes Requested
Delegated to: Grant Likely
Headers show

Comments

Roman Fietze - Feb. 26, 2009, 2:24 p.m.
Hello,

I've got a target derived from the Lite5200 that needs to use simple
interrupt GPIO pins. I created a patch to support this kind of GPIO.

I would need your opinion and like to hear any criticism. Esp. the
facts that I ad to split up struct mpc52xx_gpio and that the GPIO
numbering might get mixed up concern me.

Because I'm somewhat new to git, at least when it comes to create and
incorporate patches via mail, you have to point out those errors as
well. The first thing I found lready out ist, that my patch isn't
signed off. I'll do that with an final version if I get the chance to
deliver one.


And here the patch:


Add MPC52xx simple interrupt GPIO support

Split of struct mpc52xx_sint_gpio from struct mpc52xx_gpio to allow a
+	.match_table = mpc52xx_simple_interrupt_gpiochip_match,
+	.probe = mpc52xx_simple_interrupt_gpiochip_probe,
+	.remove = mpc52xx_gpiochip_remove,
+};
+
+/*
  * GPIO LIB API implementation for gpt GPIOs.
  *
  * Each gpt only has a single GPIO.
@@ -444,6 +599,9 @@ static int __init mpc52xx_gpio_init(void)
 	if (of_register_platform_driver(&mpc52xx_simple_gpiochip_driver))
 		printk(KERN_ERR "Unable to register simple GPIO driver\n");
 
+	if (of_register_platform_driver(&mpc52xx_simple_interrupt_gpiochip_driver))
+		printk(KERN_ERR "Unable to register simple interrupt GPIO driver\n");
+
 	if (of_register_platform_driver(&mpc52xx_gpt_gpiochip_driver))
 		printk(KERN_ERR "Unable to register gpt GPIO driver\n");
Grant Likely - Feb. 27, 2009, 5:06 a.m.
Hi Roman,

Thanks for this work.  Comments below.

On Thu, Feb 26, 2009 at 7:24 AM, Roman Fietze
<roman.fietze@telemotive.de> wrote:
> Hello,
>
> I've got a target derived from the Lite5200 that needs to use simple
> interrupt GPIO pins. I created a patch to support this kind of GPIO.
>
> I would need your opinion and like to hear any criticism. Esp. the
> facts that I ad to split up struct mpc52xx_gpio and that the GPIO
> numbering might get mixed up concern me.

While I understand why this patch is written the way it is, I don't
think it is the right approach.

This patch changes the mpc5200 gpio binding to adapt to a Linux
internal implementation detail.  Specifically, the of_mm
infrastructure only allows a 1:1 relationship between a 'struct
of_gpio_chip' and a 'struct gpio_chip'.  When working with device
trees, this is the wrong way around.  The device tree describes the
hardware, not the Linux implementation details.

An argument could be made that the current binding isn't ideal and
that it would be better to split the simple, interrupt and output-only
gpio pins into separate nodes, but that pretty much comes down to a
matter of opinion as the existing binding describes the hardware just
fine.  I'm actually of the opinion that it would be better to fewer
gpio nodes, not more, by merging the simple and wakeup pins into a
single node, but what's done is done and there is no technical reason
for changing the current binding.

So, that leaves the problem working with the of_mm infrastructure.  I
think the correct solution is to modify of_gpio_chip to hold an array
of struct gpio_chip and to change of_gpio_simple_xlate() to handle it.
 Maybe something like:

struct of_gpio_chip {
        int gpio_cells;
        int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
                     const void *gpio_spec, enum of_gpio_flags *flags);
        struct gpio_chip gc[1];
};

and add an allocator function that takes care of allocating the
required size of the gc array.  There are details to work out of
course, but I thing this would be the more robust solution.

Cheers,
g.
Anton Vorontsov - March 2, 2009, 5:16 p.m.
On Thu, Feb 26, 2009 at 10:06:59PM -0700, Grant Likely wrote:
> Hi Roman,
> 
> Thanks for this work.  Comments below.
> 
> On Thu, Feb 26, 2009 at 7:24 AM, Roman Fietze
> <roman.fietze@telemotive.de> wrote:
> > Hello,
> >
> > I've got a target derived from the Lite5200 that needs to use simple
> > interrupt GPIO pins. I created a patch to support this kind of GPIO.
> >
> > I would need your opinion and like to hear any criticism. Esp. the
> > facts that I ad to split up struct mpc52xx_gpio and that the GPIO
> > numbering might get mixed up concern me.
> 
> While I understand why this patch is written the way it is, I don't
> think it is the right approach.
> 
> This patch changes the mpc5200 gpio binding to adapt to a Linux
> internal implementation detail.  Specifically, the of_mm
> infrastructure only allows a 1:1 relationship between a 'struct
> of_gpio_chip' and a 'struct gpio_chip'.  When working with device
> trees, this is the wrong way around.  The device tree describes the
> hardware, not the Linux implementation details.
> 
> An argument could be made that the current binding isn't ideal and
> that it would be better to split the simple, interrupt and output-only
> gpio pins into separate nodes, but that pretty much comes down to a
> matter of opinion as the existing binding describes the hardware just
> fine.  I'm actually of the opinion that it would be better to fewer
> gpio nodes, not more, by merging the simple and wakeup pins into a
> single node, but what's done is done and there is no technical reason
> for changing the current binding.
> 
> So, that leaves the problem working with the of_mm infrastructure.  I
> think the correct solution is to modify of_gpio_chip to hold an array
> of struct gpio_chip and to change of_gpio_simple_xlate() to handle it.
>  Maybe something like:
> 
> struct of_gpio_chip {
>         int gpio_cells;
>         int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
>                      const void *gpio_spec, enum of_gpio_flags *flags);
>         struct gpio_chip gc[1];
> };

I'd suggest to not touch of_gpio_chip structure, I'd like to keep
of_gpio_chip struct 1:1 bound to a pure gpio_chip structure. This keeps
things simple and understandable on the low level.

And when you need several gpio controllers bound to some Linux struct,
I would rather suggest this:

struct mpc5200_gpio_controller {
	void __iomem *regs;
	void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
	struct of_gpio_chip of_gc[1];
};

In the of_gc->xlate callback you'll always get &of_gc[0], but since you
know that this is mpc5200 controller, you can add needed offset depending
on gpio_spec.

(s/mpc5200/of_multi_mm/ or something like this, if you'll manage to
do this for the general case.)


OTOH, there is even more straightforward solution, all you actually need
is to define "HW GPIO" bindings (which are wkup, which are interrupt, etc.),
and then:

void mpc5200_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
{
	if (mpc5200_is_wkup(gpio))
		write to the wkup registers block;
	else if (mpc5200_is_int(gpio))
		write to the int registers block;
	else
		...
}

That is, the same thing we do for the interrupt controllers.

(Note that these "if"s can be replaced by a table, as in
arch/powerpc/sysdev/qe_lib/qe_ic.c).
Grant Likely - March 2, 2009, 5:45 p.m.
On Mon, Mar 2, 2009 at 10:16 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Feb 26, 2009 at 10:06:59PM -0700, Grant Likely wrote:
>>  Maybe something like:
>>
>> struct of_gpio_chip {
>>         int gpio_cells;
>>         int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
>>                      const void *gpio_spec, enum of_gpio_flags *flags);
>>         struct gpio_chip gc[1];
>> };
>
> I'd suggest to not touch of_gpio_chip structure, I'd like to keep
> of_gpio_chip struct 1:1 bound to a pure gpio_chip structure. This keeps
> things simple and understandable on the low level.
>
> And when you need several gpio controllers bound to some Linux struct,
> I would rather suggest this:
>
> struct mpc5200_gpio_controller {
>        void __iomem *regs;
>        void (*save_regs)(struct of_mm_gpio_chip *mm_gc);
>        struct of_gpio_chip of_gc[1];
> };
>
> In the of_gc->xlate callback you'll always get &of_gc[0], but since you
> know that this is mpc5200 controller, you can add needed offset depending
> on gpio_spec.

Fair enough.  That works too.

> OTOH, there is even more straightforward solution, all you actually need
> is to define "HW GPIO" bindings (which are wkup, which are interrupt, etc.),
> and then:
>
> void mpc5200_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
> {
>        if (mpc5200_is_wkup(gpio))
>                write to the wkup registers block;
>        else if (mpc5200_is_int(gpio))
>                write to the int registers block;
>        else
>                ...
> }
>
> That is, the same thing we do for the interrupt controllers.

Ugh, I'd really do not want to use this approach.  The GPIOs path is
too long as is.  When GPIOs are used for things like JTAG or other bus
emulation, every cycle counts.  As much as possible the long path,
such as figuring out which chip, should be preprocessed so that it is
already known by the time the set/get/direction hooks are called.

IRQ controllers typically need to deal with far lower frequencies on
the IRQ line.

>
> (Note that these "if"s can be replaced by a table, as in
> arch/powerpc/sysdev/qe_lib/qe_ic.c).

Even with the table it is a cost I don't want in the GPIO handler.  If
it were possible to do so, I'd even like to remove the spinlocks from
the hooks, but that isn't an option at the moment.

g.

Patch

different reg base address in the device tree.
Add the appropriate functions.
Update Documentation.
---
 .../powerpc/mpc52xx-device-tree-bindings.txt       |    2 +
 arch/powerpc/include/asm/mpc52xx.h                 |   40 +++--
 arch/powerpc/platforms/52xx/mpc52xx_gpio.c         |  164 
+++++++++++++++++++-
 3 files changed, 185 insertions(+), 21 deletions(-)

diff --git a/Documentation/powerpc/mpc52xx-device-tree-bindings.txt 
b/Documentation/powerpc/mpc52xx-device-tree-bindings.txt
index 6f12f1c..b5b31b2 100644
--- a/Documentation/powerpc/mpc52xx-device-tree-bindings.txt
+++ b/Documentation/powerpc/mpc52xx-device-tree-bindings.txt
@@ -190,6 +190,8 @@  gpt@<addr>	gpt		fsl,mpc5200-gpt-gpio	General purpose
 							timers in GPIO mode
 gpio@<addr>			fsl,mpc5200-gpio	MPC5200 simple gpio
 							controller
+gpio@<addr>			fsl,mpc5200-gpio-sint	MPC5200 simple interrupt
+							gpio controller
 gpio@<addr>			fsl,mpc5200-gpio-wkup	MPC5200 wakeup gpio
 							controller
 rtc@<addr>	rtc		mpc5200-rtc	  Real time clock
diff --git a/arch/powerpc/include/asm/mpc52xx.h 
b/arch/powerpc/include/asm/mpc52xx.h
index 7655504..01abe61 100644
--- a/arch/powerpc/include/asm/mpc52xx.h
+++ b/arch/powerpc/include/asm/mpc52xx.h
@@ -126,24 +126,28 @@  struct mpc52xx_gpio {
 	u8 reserved1[3];	/* GPIO + 0x19 */
 	u8 outo_dvo;		/* GPIO + 0x1c */
 	u8 reserved2[3];	/* GPIO + 0x1d */
-	u8 sint_gpioe;		/* GPIO + 0x20 */
-	u8 reserved3[3];	/* GPIO + 0x21 */
-	u8 sint_ode;		/* GPIO + 0x24 */
-	u8 reserved4[3];	/* GPIO + 0x25 */
-	u8 sint_ddr;		/* GPIO + 0x28 */
-	u8 reserved5[3];	/* GPIO + 0x29 */
-	u8 sint_dvo;		/* GPIO + 0x2c */
-	u8 reserved6[3];	/* GPIO + 0x2d */
-	u8 sint_inten;		/* GPIO + 0x30 */
-	u8 reserved7[3];	/* GPIO + 0x31 */
-	u16 sint_itype;		/* GPIO + 0x34 */
-	u16 reserved8;		/* GPIO + 0x36 */
-	u8 gpio_control;	/* GPIO + 0x38 */
-	u8 reserved9[3];	/* GPIO + 0x39 */
-	u8 sint_istat;		/* GPIO + 0x3c */
-	u8 sint_ival;		/* GPIO + 0x3d */
-	u8 bus_errs;		/* GPIO + 0x3e */
-	u8 reserved10;		/* GPIO + 0x3f */
+};
+
+/* Simple Interrupt GPIO */
+struct mpc52xx_sint_gpio {
+	u8 sint_gpioe;		/* GPIO + 0x00 */
+	u8 reserved3[3];	/* GPIO + 0x01 */
+	u8 sint_ode;		/* GPIO + 0x04 */
+	u8 reserved4[3];	/* GPIO + 0x05 */
+	u8 sint_ddr;		/* GPIO + 0x08 */
+	u8 reserved5[3];	/* GPIO + 0x09 */
+	u8 sint_dvo;		/* GPIO + 0x0c */
+	u8 reserved6[3];	/* GPIO + 0x0d */
+	u8 sint_inten;		/* GPIO + 0x10 */
+	u8 reserved7[3];	/* GPIO + 0x11 */
+	u16 sint_itype;		/* GPIO + 0x14 */
+	u16 reserved8;		/* GPIO + 0x16 */
+	u8 gpio_control;	/* GPIO + 0x18 */
+	u8 reserved9[3];	/* GPIO + 0x19 */
+	u8 sint_istat;		/* GPIO + 0x1c */
+	u8 sint_ival;		/* GPIO + 0x1d */
+	u8 bus_errs;		/* GPIO + 0x1e */
+	u8 reserved10;		/* GPIO + 0x1f */
 };
 
 #define MPC52xx_GPIO_PSC_CONFIG_UART_WITHOUT_CD	4
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c 
b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
index 07f89ae..8595aad 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
@@ -31,9 +31,9 @@  static DEFINE_SPINLOCK(gpio_lock);
 
 struct mpc52xx_gpiochip {
 	struct of_mm_gpio_chip mmchip;
-	unsigned int shadow_dvo;
-	unsigned int shadow_gpioe;
-	unsigned int shadow_ddr;
+	uint32_t shadow_dvo;
+	uint32_t shadow_gpioe;
+	uint32_t shadow_ddr;
 };
 
 /*
@@ -355,6 +355,161 @@  static struct of_platform_driver 
mpc52xx_simple_gpiochip_driver = {
 };
 
 /*
+ * GPIO LIB API implementation for simple interrupt GPIOs
+ *
+ * There's a maximum of 8 simple interrupt GPIOs. Which of these are
+ * available for use depends on your board setup.  The numbering
+ * reflects the bit numbering in the port registers:
+ *
+ *  0.. 3 > ETH_16..ETH_13
+ *  4     > USB1_9
+ *  5     > PSC3_8
+ *  6.. 7 > PSC3_5..PSC3_4
+ */
+static int mpc52xx_simple_interrupt_gpio_get(struct gpio_chip *gc, unsigned 
int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct mpc52xx_sint_gpio __iomem *regs = mm_gc->regs;
+	unsigned int ret;
+
+	ret = (in_8(&regs->sint_ival) >> (7 - gpio)) & 1;
+
+	pr_info("%s(..,%u) data=0x%02x ret=%d\n", __func__, gpio, in_8
(&regs->sint_ival), ret);
+
+	return ret;
+}
+
+static inline void
+__mpc52xx_simple_interrupt_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 mpc52xx_gpiochip *chip = container_of(mm_gc,
+			struct mpc52xx_gpiochip, mmchip);
+	struct mpc52xx_sint_gpio __iomem *regs = mm_gc->regs;
+
+	if (val)
+		chip->shadow_dvo |= 1 << (7 - gpio);
+	else
+		chip->shadow_dvo &= ~(1 << (7 - gpio));
+	out_8(&regs->sint_dvo, chip->shadow_dvo);
+}
+
+static void
+mpc52xx_simple_interrupt_gpio_set(struct gpio_chip *gc, unsigned int gpio, 
int val)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	__mpc52xx_simple_interrupt_gpio_set(gc, gpio, val);
+
+	spin_unlock_irqrestore(&gpio_lock, flags);
+}
+
+static int mpc52xx_simple_interrupt_gpio_dir_in(struct gpio_chip *gc, 
unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+			struct mpc52xx_gpiochip, mmchip);
+	struct mpc52xx_sint_gpio __iomem *regs = mm_gc->regs;
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	/* set the direction */
+	chip->shadow_ddr &= ~(1 << (7 - gpio));
+	out_8(&regs->sint_ddr, chip->shadow_ddr);
+
+	/* and enable the pin */
+	chip->shadow_gpioe |= 1 << (7 - gpio);
+	out_8(&regs->sint_gpioe, chip->shadow_gpioe);
+
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	return 0;
+}
+
+static int
+mpc52xx_simple_interrupt_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);
+	struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+			struct mpc52xx_gpiochip, mmchip);
+	struct mpc52xx_sint_gpio __iomem *regs = mm_gc->regs;
+	unsigned long flags;
+
+	pr_info("%s(...,%u,%d)\n", __func__, gpio, val);
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	/* First set initial value */
+	__mpc52xx_simple_interrupt_gpio_set(gc, gpio, val);
+
+	/* Then set direction */
+	chip->shadow_ddr |= 1 << (7 - gpio);
+	out_8(&regs->sint_ddr, chip->shadow_ddr);
+
+	/* Finally enable the pin */
+	chip->shadow_gpioe |= 1 << (7 - gpio);
+	out_8(&regs->sint_gpioe, chip->shadow_gpioe);
+
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	pr_info("%s: gpio: %d val: %d\n", __func__, gpio, val);
+
+	return 0;
+}
+
+static int __devinit mpc52xx_simple_interrupt_gpiochip_probe(struct of_device 
*ofdev,
+							     const struct of_device_id *match)
+{
+	struct mpc52xx_gpiochip *chip;
+	struct of_gpio_chip *ofchip;
+	struct mpc52xx_sint_gpio __iomem *regs;
+	int ret;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	ofchip = &chip->mmchip.of_gc;
+
+	ofchip->gpio_cells          = 2;
+	ofchip->gc.ngpio            = 8;
+	ofchip->gc.direction_input  = mpc52xx_simple_interrupt_gpio_dir_in;
+	ofchip->gc.direction_output = mpc52xx_simple_interrupt_gpio_dir_out;
+	ofchip->gc.get              = mpc52xx_simple_interrupt_gpio_get;
+	ofchip->gc.set              = mpc52xx_simple_interrupt_gpio_set;
+
+	ret = of_mm_gpiochip_add(ofdev->node, &chip->mmchip);
+	if (ret)
+		return ret;
+
+	regs = chip->mmchip.regs;
+	pr_info("%s() regs=%p\n", __func__, regs);
+
+	chip->shadow_gpioe = in_8(&regs->sint_gpioe);
+	chip->shadow_ddr = in_8(&regs->sint_ddr);
+	chip->shadow_dvo = in_8(&regs->sint_dvo);
+
+	return 0;
+}
+
+static const struct of_device_id mpc52xx_simple_interrupt_gpiochip_match[] = 
{
+	{
+		.compatible = "fsl,mpc5200-gpio-sint",
+	},
+	{}
+};
+
+static struct of_platform_driver mpc52xx_simple_interrupt_gpiochip_driver = {
+	.name = "gpio_sint",