[PATCH] gpio: driver for Xtensa GPIO32

[PATCH] gpio: driver for Xtensa GPIO32

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/Kconfig       |   7 +++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-xtensa.c | 132 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)
 create mode 100644 drivers/gpio/gpio-xtensa.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8c827eb..488e07b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -290,6 +290,13 @@ config GPIO_XILINX
 	help
 	  Say yes here to support the Xilinx FPGA GPIO device
 
+config GPIO_XTENSA
+	bool "Xtensa GPIO32 support"
+	depends on XTENSA
+	help
+	  Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
+	  and EXPSTATE (output) ports
+
 config GPIO_VR41XX
 	tristate "NEC VR4100 series General-purpose I/O Uint support"
 	depends on CPU_VR41XX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index b4f852e..d8ed654 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -96,3 +96,4 @@ obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
 obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
 obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
+obj-$(CONFIG_GPIO_XTENSA)	+= gpio-xtensa.o
diff --git a/drivers/gpio/gpio-xtensa.c b/drivers/gpio/gpio-xtensa.c
new file mode 100644
index 0000000..f5f74b2
--- /dev/null
+++ b/drivers/gpio/gpio-xtensa.c
@@ -0,0 +1,132 @@
+/*
+ * Copyright (C) 2013 TangoTec Ltd.
+ * Author: Baruch Siach <baruch@tkos.co.il>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver for the Xtensa LX4 GPIO32 Option
+ *
+ * Documentation: Xtensa LX4 Microprocessor Data Book, Section 2.22
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+
+#include <asm/coprocessor.h>
+
+#define ENABLE_CP()					\
+{							\
+	unsigned long flags, cpenable;			\
+	local_irq_save(flags);				\
+	RSR_CPENABLE(cpenable);				\
+	WSR_CPENABLE(cpenable | XCHAL_CP_PORT_MASK);
+
+#define DISABLE_CP()					\
+	WSR_CPENABLE(cpenable);				\
+	local_irq_restore(flags);			\
+}
+
+static int xtensa_impwire_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	return 1;
+}
+
+static int xtensa_impwire_get_value(struct gpio_chip *gc, unsigned offset)
+{
+	u32 impwire;
+
+	ENABLE_CP();
+	__asm__ __volatile__("read_impwire %0" : "=a" (impwire));
+	DISABLE_CP();
+
+	return !!(impwire & (1 << offset));
+}
+
+static void xtensa_impwire_set_value(struct gpio_chip *gc, unsigned offset,
+				    int value)
+{
+}
+
+static int xtensa_expstate_get_direction(struct gpio_chip *gc, unsigned offset)
+{
+	return 0;
+}
+
+static int xtensa_expstate_get_value(struct gpio_chip *gc, unsigned offset)
+{
+	u32 expstate;
+
+	ENABLE_CP();
+	__asm__ __volatile__("rur.expstate %0" : "=a" (expstate));
+	DISABLE_CP();
+
+	return !!(expstate & (1 << offset));
+}
+
+static void xtensa_expstate_set_value(struct gpio_chip *gc, unsigned offset,
+				     int value)
+{
+	u32 mask = (1 << offset);
+	u32 val = value ? (1 << offset) : 0;
+
+	ENABLE_CP();
+	__asm__ __volatile__("wrmsk_expstate %0, %1"
+			     :: "a" (val), "a" (mask));
+	DISABLE_CP();
+}
+
+static struct gpio_chip impwire_chip = {
+	.label		= "impwire",
+	.base		= -1,
+	.ngpio		= 32,
+	.get_direction	= xtensa_impwire_get_direction,
+	.get		= xtensa_impwire_get_value,
+	.set		= xtensa_impwire_set_value,
+};
+
+static struct gpio_chip expstate_chip = {
+	.label		= "expstate",
+	.base		= -1,
+	.ngpio		= 32,
+	.get_direction	= xtensa_expstate_get_direction,
+	.get		= xtensa_expstate_get_value,
+	.set		= xtensa_expstate_set_value,
+};
+
+static int xtensa_gpio_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = gpiochip_add(&impwire_chip);
+	if (ret)
+		return ret;
+	return gpiochip_add(&expstate_chip);
+}
+
+static struct platform_driver xtensa_gpio_driver = {
+	.driver		= {
+		.name		= "xtensa-gpio",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= xtensa_gpio_probe,
+};
+
+static int __init xtensa_gpio_init(void)
+{
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_simple("xtensa-gpio", 0, NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return platform_driver_register(&xtensa_gpio_driver);
+}
+subsys_initcall(xtensa_gpio_init);
+
+MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>");
+MODULE_DESCRIPTION("Xtensa LX4 GPIO32 driver");
+MODULE_LICENSE("GPL");
-- 
1.8.4.3

Re: [PATCH] gpio: driver for Xtensa GPIO32

[Linus Walleij]

On Tue, Dec 3, 2013 at 9:04 AM, Baruch Siach <baruch@tkos.co.il> wrote:

> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

This is a pretty terse commit message for an entirely new driver.
For example I have no clue which architecture this is and what
machine/platforms in that arch that may use this, or if it's some
softcore or ... you know, such basic things a simple GPIO maintainer
can't be expected to know.

> +config GPIO_XTENSA
> +       bool "Xtensa GPIO32 support"
> +       depends on XTENSA
> +       help
> +         Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
> +         and EXPSTATE (output) ports

If there is also going to be 16 or 64 bit versions of this thing
you better call the Kconfig symbol GPIO_XTENSA32 or
something.

> +obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o

And this gpio-xtensa32.o

> +++ b/drivers/gpio/gpio-xtensa.c

> +#include <asm/coprocessor.h>

Hm so it seems we are using some coprocessor instructions
to get at the GPIO, don't be shy, write a few comments on it :-)

> +#define ENABLE_CP()                                    \
> +{                                                      \
> +       unsigned long flags, cpenable;                  \
> +       local_irq_save(flags);                          \
> +       RSR_CPENABLE(cpenable);                         \
> +       WSR_CPENABLE(cpenable | XCHAL_CP_PORT_MASK);
> +
> +#define DISABLE_CP()                                   \
> +       WSR_CPENABLE(cpenable);                         \
> +       local_irq_restore(flags);                       \
> +}

Rewrite these to static inlines.

> +static int xtensa_impwire_get_direction(struct gpio_chip *gc, unsigned offset)
> +{
> +       return 1;
> +}

Ummm.... explain this. Alll lines are always inputs?

> +static int xtensa_impwire_get_value(struct gpio_chip *gc, unsigned offset)
> +{
> +       u32 impwire;
> +
> +       ENABLE_CP();
> +       __asm__ __volatile__("read_impwire %0" : "=a" (impwire));
> +       DISABLE_CP();
> +
> +       return !!(impwire & (1 << offset));

Use this pattern everywhere you do (1 << offset):

#include <linux/bitops.h>

s/(1 << offset)/BIT(offset)/g

> +static void xtensa_impwire_set_value(struct gpio_chip *gc, unsigned offset,
> +                                   int value)
> +{
> +}

This is a rather nasty implementation for something that will always
fail. What about putting an dev_err(gc->dev, "..."); in that set_value?

> +static int xtensa_expstate_get_direction(struct gpio_chip *gc, unsigned offset)

What is an expstate?

> +{
> +       return 0;
> +}

So all lines are always outputs? (Comment in the code thx.)

> +static int __init xtensa_gpio_init(void)
> +{
> +       struct platform_device *pdev;
> +
> +       pdev = platform_device_register_simple("xtensa-gpio", 0, NULL, 0);
> +       if (IS_ERR(pdev))
> +               return PTR_ERR(pdev);
> +
> +       return platform_driver_register(&xtensa_gpio_driver);
> +}
> +subsys_initcall(xtensa_gpio_init);

Why not device_initcall()? (i.e. same as module_init())
Do you need it very early?

Yours,
Linus Walleij

Re: [PATCH] gpio: driver for Xtensa GPIO32

[Baruch Siach]

Hi Linus,

Thanks for taking the time to review the code.

On Wed, Dec 04, 2013 at 02:18:01PM +0100, Linus Walleij wrote:
> On Tue, Dec 3, 2013 at 9:04 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> 
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> 
> This is a pretty terse commit message for an entirely new driver.
> For example I have no clue which architecture this is and what
> machine/platforms in that arch that may use this, or if it's some
> softcore or ... you know, such basic things a simple GPIO maintainer
> can't be expected to know.

I'll add a more verbose commit log.

> > +config GPIO_XTENSA
> > +       bool "Xtensa GPIO32 support"
> > +       depends on XTENSA
> > +       help
> > +         Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
> > +         and EXPSTATE (output) ports
> 
> If there is also going to be 16 or 64 bit versions of this thing
> you better call the Kconfig symbol GPIO_XTENSA32 or
> something.

Not likely. This is a standard optional extension to the xtensa core 
architecture that is referred to in the documentation as GPIO32. This 
extension adds a few instructions to read into and write out from the core 
32bit general purpose registers. I believe that future 16 or 64 bit versions 
are very unlikely.

Anyway, given that xtensa is the name of the architecture, naming the driver 
gpio-xtensa32 would just confuse users.

> > +obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o
> 
> And this gpio-xtensa32.o
> 
> > +++ b/drivers/gpio/gpio-xtensa.c
> 
> > +#include <asm/coprocessor.h>
> 
> Hm so it seems we are using some coprocessor instructions
> to get at the GPIO, don't be shy, write a few comments on it :-)

Will do.

> > +#define ENABLE_CP()                                    \
> > +{                                                      \
> > +       unsigned long flags, cpenable;                  \
> > +       local_irq_save(flags);                          \
> > +       RSR_CPENABLE(cpenable);                         \
> > +       WSR_CPENABLE(cpenable | XCHAL_CP_PORT_MASK);
> > +
> > +#define DISABLE_CP()                                   \
> > +       WSR_CPENABLE(cpenable);                         \
> > +       local_irq_restore(flags);                       \
> > +}
> 
> Rewrite these to static inlines.

We need to preserve the irq flags. Let me know if the following is OK:

static inline unsigned long enable_cp(void)
{
	unsigned long flags, cpenable;

	local_irq_save(flags);
	RSR_CPENABLE(cpenable);
	WSR_CPENABLE(cpenable | XCHAL_CP_PORT_MASK);

	return flags;
}

static inline void disable_cp(unsigned long flags)
{
	WSR_CPENABLE(cpenable);
	local_irq_restore(flags);
}

	...

	unsigned long flags;

	flags = enable_cp()
	...
	disable_cp(flags);

> 
> > +static int xtensa_impwire_get_direction(struct gpio_chip *gc, unsigned offset)
> > +{
> > +       return 1;
> > +}
> 
> Ummm.... explain this. Alll lines are always inputs?

Exactly. I'll add a comment.

> > +static int xtensa_impwire_get_value(struct gpio_chip *gc, unsigned offset)
> > +{
> > +       u32 impwire;
> > +
> > +       ENABLE_CP();
> > +       __asm__ __volatile__("read_impwire %0" : "=a" (impwire));
> > +       DISABLE_CP();
> > +
> > +       return !!(impwire & (1 << offset));
> 
> Use this pattern everywhere you do (1 << offset):
> 
> #include <linux/bitops.h>
> 
> s/(1 << offset)/BIT(offset)/g

Will do.

> > +static void xtensa_impwire_set_value(struct gpio_chip *gc, unsigned offset,
> > +                                   int value)
> > +{
> > +}
> 
> This is a rather nasty implementation for something that will always
> fail. What about putting an dev_err(gc->dev, "..."); in that set_value?

This is an input only device, so this callback should never run. Ideally it 
would be nice to just have it NULL, but the core gpiolib doesn't support that. 
Are you sure bloating the kernel with an error message that never displays is 
worth it?

> > +static int xtensa_expstate_get_direction(struct gpio_chip *gc, unsigned offset)
> 
> What is an expstate?

This is how this extension specific register is called. I'll add a comment.

> 
> > +{
> > +       return 0;
> > +}
> 
> So all lines are always outputs? (Comment in the code thx.)

Yes. Will do.

> > +static int __init xtensa_gpio_init(void)
> > +{
> > +       struct platform_device *pdev;
> > +
> > +       pdev = platform_device_register_simple("xtensa-gpio", 0, NULL, 0);
> > +       if (IS_ERR(pdev))
> > +               return PTR_ERR(pdev);
> > +
> > +       return platform_driver_register(&xtensa_gpio_driver);
> > +}
> > +subsys_initcall(xtensa_gpio_init);
> 
> Why not device_initcall()? (i.e. same as module_init())
> Do you need it very early?

The main use case for this driver is for intra-core signaling, so it might be 
needed quite early I guess. I don't need any specific need for it to be 
available early at the moment, so I can change it device_initcall() if you 
prefer that.

baruch

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

Re: [PATCH] gpio: driver for Xtensa GPIO32

[Linus Walleij]

On Thu, Dec 5, 2013 at 6:43 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> [Me]

>> If there is also going to be 16 or 64 bit versions of this thing
>> you better call the Kconfig symbol GPIO_XTENSA32 or
>> something.
>
> Not likely. This is a standard optional extension to the xtensa core
> architecture that is referred to in the documentation as GPIO32. This
> extension adds a few instructions to read into and write out from the core
> 32bit general purpose registers. I believe that future 16 or 64 bit versions
> are very unlikely.
>
> Anyway, given that xtensa is the name of the architecture, naming the driver
> gpio-xtensa32 would just confuse users.

OK.

> We need to preserve the irq flags. Let me know if the following is OK:
>
> static inline unsigned long enable_cp(void)
> {
>         unsigned long flags, cpenable;
>
>         local_irq_save(flags);
>         RSR_CPENABLE(cpenable);
>         WSR_CPENABLE(cpenable | XCHAL_CP_PORT_MASK);
>
>         return flags;
> }
>
> static inline void disable_cp(unsigned long flags)
> {
>         WSR_CPENABLE(cpenable);
>         local_irq_restore(flags);
> }

Looks OK.

>> > +static void xtensa_impwire_set_value(struct gpio_chip *gc, unsigned offset,
>> > +                                   int value)
>> > +{
>> > +}
>>
>> This is a rather nasty implementation for something that will always
>> fail. What about putting an dev_err(gc->dev, "..."); in that set_value?
>
> This is an input only device, so this callback should never run. Ideally it
> would be nice to just have it NULL, but the core gpiolib doesn't support that.
> Are you sure bloating the kernel with an error message that never displays is
> worth it?

Yes I am sure. If you think a few lines of error message is too much,
you can add a BUG(); here instead.

>> Why not device_initcall()? (i.e. same as module_init())
>> Do you need it very early?
>
> The main use case for this driver is for intra-core signaling, so it might be
> needed quite early I guess. I don't need any specific need for it to be
> available early at the moment, so I can change it device_initcall() if you
> prefer that.

Please do that for now, thanks. We shouldn't stick strange stuff
in because "may need later", as this can lead to weird stuff that people
don't dare to change. (Cargo-cult phenomenon.)

Yours,
Linus Walleij

Re: [PATCH] gpio: driver for Xtensa GPIO32

[Baruch Siach]

Hi Linus,

On Wed, Dec 11, 2013 at 01:32:47PM +0100, Linus Walleij wrote:
> On Thu, Dec 5, 2013 at 6:43 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> > We need to preserve the irq flags. Let me know if the following is OK:
> >
> > static inline unsigned long enable_cp(void)
> > {
> >         unsigned long flags, cpenable;
> >
> >         local_irq_save(flags);
> >         RSR_CPENABLE(cpenable);
> >         WSR_CPENABLE(cpenable | XCHAL_CP_PORT_MASK);
> >
> >         return flags;
> > }
> >
> > static inline void disable_cp(unsigned long flags)
> > {
> >         WSR_CPENABLE(cpenable);
> >         local_irq_restore(flags);
> > }
> 
> Looks OK.

Done in v2.

> >> > +static void xtensa_impwire_set_value(struct gpio_chip *gc, unsigned offset,
> >> > +                                   int value)
> >> > +{
> >> > +}
> >>
> >> This is a rather nasty implementation for something that will always
> >> fail. What about putting an dev_err(gc->dev, "..."); in that set_value?
> >
> > This is an input only device, so this callback should never run. Ideally it
> > would be nice to just have it NULL, but the core gpiolib doesn't support that.
> > Are you sure bloating the kernel with an error message that never displays is
> > worth it?
> 
> Yes I am sure. If you think a few lines of error message is too much,
> you can add a BUG(); here instead.

Will do in v3.

> >> Why not device_initcall()? (i.e. same as module_init())
> >> Do you need it very early?
> >
> > The main use case for this driver is for intra-core signaling, so it might be
> > needed quite early I guess. I don't need any specific need for it to be
> > available early at the moment, so I can change it device_initcall() if you
> > prefer that.
> 
> Please do that for now, thanks. We shouldn't stick strange stuff
> in because "may need later", as this can lead to weird stuff that people
> don't dare to change. (Cargo-cult phenomenon.)

Done in v2.

Thanks,
baruch

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