I'm resubmitting this, as I actually have time to deal w/ any problems
it might have. Randy, please do your worst (and send me a .config if
you find problems!). Thanks.
Oh, also, I renamed it to cs5535-gpio at the request of q-funk, in
order to be consistent w/ naming standards for CS5530/CS5535/CS5536.
This creates a CS5535/CS5536 GPIO driver which uses a gpio_chip backend
(allowing GPIO users to use the generic GPIO API if desired) while also
allowing architecture-specific users directly (via the cs5535_gpio_*
functions).
Tested on an OLPC machine. Some Leemotes also use CS5536 (with a mips
cpu), which is why this is in drivers/gpio rather than arch/x86.
Currently, it conflicts with older geode GPIO support; once MFGPT support
is reworked to also be more generic, the older geode code will be removed.
Signed-off-by: Andres Salomon <[email protected]>
---
arch/x86/include/asm/geode.h | 28 +----
drivers/gpio/Kconfig | 10 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/cs5535-gpio.c | 282 ++++++++++++++++++++++++++++++++++++++++++
include/linux/cs5535.h | 58 +++++++++
5 files changed, 352 insertions(+), 27 deletions(-)
create mode 100644 drivers/gpio/cs5535-gpio.c
create mode 100644 include/linux/cs5535.h
diff --git a/arch/x86/include/asm/geode.h b/arch/x86/include/asm/geode.h
index ad3c2ed..5716214 100644
--- a/arch/x86/include/asm/geode.h
+++ b/arch/x86/include/asm/geode.h
@@ -12,6 +12,7 @@
#include <asm/processor.h>
#include <linux/io.h>
+#include <linux/cs5535.h>
/* Generic southbridge functions */
@@ -115,33 +116,6 @@ extern int geode_get_dev_base(unsigned int dev);
#define VSA_VR_MEM_SIZE 0x0200
#define AMD_VSA_SIG 0x4132 /* signature is ascii 'VSA2' */
#define GSW_VSA_SIG 0x534d /* General Software signature */
-/* GPIO */
-
-#define GPIO_OUTPUT_VAL 0x00
-#define GPIO_OUTPUT_ENABLE 0x04
-#define GPIO_OUTPUT_OPEN_DRAIN 0x08
-#define GPIO_OUTPUT_INVERT 0x0C
-#define GPIO_OUTPUT_AUX1 0x10
-#define GPIO_OUTPUT_AUX2 0x14
-#define GPIO_PULL_UP 0x18
-#define GPIO_PULL_DOWN 0x1C
-#define GPIO_INPUT_ENABLE 0x20
-#define GPIO_INPUT_INVERT 0x24
-#define GPIO_INPUT_FILTER 0x28
-#define GPIO_INPUT_EVENT_COUNT 0x2C
-#define GPIO_READ_BACK 0x30
-#define GPIO_INPUT_AUX1 0x34
-#define GPIO_EVENTS_ENABLE 0x38
-#define GPIO_LOCK_ENABLE 0x3C
-#define GPIO_POSITIVE_EDGE_EN 0x40
-#define GPIO_NEGATIVE_EDGE_EN 0x44
-#define GPIO_POSITIVE_EDGE_STS 0x48
-#define GPIO_NEGATIVE_EDGE_STS 0x4C
-
-#define GPIO_MAP_X 0xE0
-#define GPIO_MAP_Y 0xE4
-#define GPIO_MAP_Z 0xE8
-#define GPIO_MAP_W 0xEC
static inline u32 geode_gpio(unsigned int nr)
{
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index edb0253..185a458 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -145,6 +145,16 @@ config GPIO_TWL4030
comment "PCI GPIO expanders:"
+config GPIO_CS5535
+ tristate "AMD CS5535/CS5536 GPIO support"
+ depends on PCI && !CS5535_GPIO && !MGEODE_LX
+ help
+ The AMD CS5535 and CS5536 southbridges support 28 GPIO pins that
+ can be used for quite a number of things. The CS5535/6 is found on
+ AMD Geode and Lemote Yeeloong devices.
+
+ If unsure, say N.
+
config GPIO_BT8XX
tristate "BT8XX GPIO abuser"
depends on PCI && VIDEO_BT848=n
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 49ac64e..c7bc4bd 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -11,4 +11,5 @@ obj-$(CONFIG_GPIO_PCA953X) += pca953x.o
obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o
obj-$(CONFIG_GPIO_TWL4030) += twl4030-gpio.o
obj-$(CONFIG_GPIO_XILINX) += xilinx_gpio.o
+obj-$(CONFIG_GPIO_CS5535) += cs5535-gpio.o
obj-$(CONFIG_GPIO_BT8XX) += bt8xxgpio.o
diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
new file mode 100644
index 0000000..5613889
--- /dev/null
+++ b/drivers/gpio/cs5535-gpio.c
@@ -0,0 +1,282 @@
+/*
+ * AMD CS5535/CS5536 GPIO driver
+ * Copyright (C) 2006 Advanced Micro Devices, Inc.
+ * Copyright (C) 2007-2009 Andres Salomon <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/cs5535.h>
+
+#define DRV_NAME "cs5535-gpio"
+#define GPIO_BAR 1
+
+static struct cs5535_gpio_chip {
+ struct gpio_chip chip;
+ resource_size_t base;
+
+ struct pci_dev *pdev;
+ spinlock_t lock;
+} cs5535_gpio_chip;
+
+/*
+ * The CS5535/CS5536 GPIOs support a number of extra features not defined
+ * by the gpio_chip API, so these are exported. For a full list of the
+ * registers, see include/linux/cs5535.h.
+ */
+
+static void __cs5535_gpio_set(struct cs5535_gpio_chip *chip, unsigned offset,
+ unsigned int reg)
+{
+ if (offset < 16)
+ /* low bank register */
+ outl(1 << offset, chip->base + reg);
+ else
+ /* high bank register */
+ outl(1 << (offset - 16), chip->base + 0x80 + reg);
+}
+
+void cs5535_gpio_set(unsigned offset, unsigned int reg)
+{
+ struct cs5535_gpio_chip *chip = &cs5535_gpio_chip;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chip->lock, flags);
+ __cs5535_gpio_set(chip, offset, reg);
+ spin_unlock_irqrestore(&chip->lock, flags);
+}
+EXPORT_SYMBOL_GPL(cs5535_gpio_set);
+
+static void __cs5535_gpio_clear(struct cs5535_gpio_chip *chip, unsigned offset,
+ unsigned int reg)
+{
+ if (offset < 16)
+ /* low bank register */
+ outl(1 << (offset + 16), chip->base + reg);
+ else
+ /* high bank register */
+ outl(1 << offset, chip->base + 0x80 + reg);
+}
+
+void cs5535_gpio_clear(unsigned offset, unsigned int reg)
+{
+ struct cs5535_gpio_chip *chip = &cs5535_gpio_chip;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chip->lock, flags);
+ __cs5535_gpio_clear(chip, offset, reg);
+ spin_unlock_irqrestore(&chip->lock, flags);
+}
+EXPORT_SYMBOL_GPL(cs5535_gpio_clear);
+
+int cs5535_gpio_isset(unsigned offset, unsigned int reg)
+{
+ struct cs5535_gpio_chip *chip = &cs5535_gpio_chip;
+ unsigned long flags;
+ long val;
+
+ spin_lock_irqsave(&chip->lock, flags);
+ if (offset < 16)
+ /* low bank register */
+ val = inl(chip->base + reg);
+ else {
+ /* high bank register */
+ val = inl(chip->base + 0x80 + reg);
+ offset -= 16;
+ }
+ spin_unlock_irqrestore(&chip->lock, flags);
+
+ return (val & (1 << offset)) ? 1 : 0;
+}
+EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
+
+/*
+ * Generic gpio_chip API support.
+ */
+
+static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
+}
+
+static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
+{
+ if (val)
+ cs5535_gpio_set(offset, GPIO_OUTPUT_VAL);
+ else
+ cs5535_gpio_clear(offset, GPIO_OUTPUT_VAL);
+}
+
+static int chip_direction_input(struct gpio_chip *c, unsigned offset)
+{
+ struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chip->lock, flags);
+ __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
+ spin_unlock_irqrestore(&chip->lock, flags);
+
+ return 0;
+}
+
+static int chip_direction_output(struct gpio_chip *c, unsigned offset, int val)
+{
+ struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chip->lock, flags);
+
+ __cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
+ if (val)
+ __cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
+ else
+ __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_VAL);
+
+ spin_unlock_irqrestore(&chip->lock, flags);
+
+ return 0;
+}
+
+static struct cs5535_gpio_chip cs5535_gpio_chip = {
+ .chip = {
+ .owner = THIS_MODULE,
+ .label = DRV_NAME,
+
+ .base = 0,
+ .ngpio = 28,
+
+ .get = chip_gpio_get,
+ .set = chip_gpio_set,
+
+ .direction_input = chip_direction_input,
+ .direction_output = chip_direction_output,
+ },
+};
+
+static int __init cs5535_gpio_probe(struct pci_dev *pdev,
+ const struct pci_device_id *pci_id)
+{
+ int err;
+
+ /* There are two ways to get the GPIO base address; one is by
+ * fetching it from MSR_LBAR_GPIO, the other is by reading the
+ * PCI BAR info. The latter method is easier (especially across
+ * different architectures), so we'll stick with that for now. If
+ * it turns out to be unreliable in the face of crappy BIOSes, we
+ * can always go back to using MSRs.. */
+
+ err = pci_enable_device_io(pdev);
+ if (err) {
+ dev_err(&pdev->dev, "can't enable device IO\n");
+ goto done;
+ }
+
+ err = pci_request_region(pdev, GPIO_BAR, DRV_NAME);
+ if (err) {
+ dev_err(&pdev->dev, "can't alloc PCI BAR #%d\n", GPIO_BAR);
+ goto done;
+ }
+
+ /* set up the driver-specific struct */
+ cs5535_gpio_chip.base = pci_resource_start(pdev, GPIO_BAR);
+ cs5535_gpio_chip.pdev = pdev;
+ spin_lock_init(&cs5535_gpio_chip.lock);
+
+ dev_info(&pdev->dev, "allocated PCI BAR #%d: base 0x%llx\n", GPIO_BAR,
+ (unsigned long long) cs5535_gpio_chip.base);
+
+ /* finally, register with the generic GPIO API */
+ err = gpiochip_add(&cs5535_gpio_chip.chip);
+ if (err) {
+ dev_err(&pdev->dev, "failed to register gpio chip\n");
+ goto release_region;
+ }
+
+ printk(KERN_INFO DRV_NAME ": GPIO support successfully loaded.\n");
+ return 0;
+
+release_region:
+ pci_release_region(pdev, GPIO_BAR);
+done:
+ return err;
+}
+
+static void __exit cs5535_gpio_remove(struct pci_dev *pdev)
+{
+ int err;
+
+ err = gpiochip_remove(&cs5535_gpio_chip.chip);
+ if (err) {
+ /* uhh? */
+ dev_err(&pdev->dev, "unable to remove gpio_chip?\n");
+ }
+ pci_release_region(pdev, GPIO_BAR);
+}
+
+static struct pci_device_id cs5535_gpio_pci_tbl[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA) },
+ { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) },
+ { 0, },
+};
+MODULE_DEVICE_TABLE(pci, cs5535_gpio_pci_tbl);
+
+/*
+ * We can't use the standard PCI driver registration stuff here, since
+ * that allows only one driver to bind to each PCI device (and we want
+ * multiple drivers to be able to bind to the device). Instead, manually
+ * scan for the PCI device, request a single region, and keep track of the
+ * devices that we're using.
+ */
+
+static int __init cs5535_gpio_scan_pci(void)
+{
+ struct pci_dev *pdev;
+ int err = -ENODEV;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(cs5535_gpio_pci_tbl); i++) {
+ pdev = pci_get_device(cs5535_gpio_pci_tbl[i].vendor,
+ cs5535_gpio_pci_tbl[i].device, NULL);
+ if (pdev) {
+ err = cs5535_gpio_probe(pdev, &cs5535_gpio_pci_tbl[i]);
+ if (err)
+ pci_dev_put(pdev);
+
+ /* we only support a single CS5535/6 southbridge */
+ break;
+ }
+ }
+
+ return err;
+}
+
+static void __exit cs5535_gpio_free_pci(void)
+{
+ cs5535_gpio_remove(cs5535_gpio_chip.pdev);
+ pci_dev_put(cs5535_gpio_chip.pdev);
+}
+
+static int __init cs5535_gpio_init(void)
+{
+ return cs5535_gpio_scan_pci();
+}
+
+static void __exit cs5535_gpio_exit(void)
+{
+ cs5535_gpio_free_pci();
+}
+
+module_init(cs5535_gpio_init);
+module_exit(cs5535_gpio_exit);
+
+MODULE_AUTHOR("Andres Salomon <[email protected]>");
+MODULE_DESCRIPTION("AMD CS5535/CS5536 GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/cs5535.h b/include/linux/cs5535.h
new file mode 100644
index 0000000..cfea689
--- /dev/null
+++ b/include/linux/cs5535.h
@@ -0,0 +1,58 @@
+/*
+ * AMD CS5535/CS5536 definitions
+ * Copyright (C) 2006 Advanced Micro Devices, Inc.
+ * Copyright (C) 2009 Andres Salomon <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ */
+
+#ifndef _CS5535_H
+#define _CS5535_H
+
+/* MSRs */
+#define MSR_LBAR_SMB 0x5140000B
+#define MSR_LBAR_GPIO 0x5140000C
+#define MSR_LBAR_MFGPT 0x5140000D
+#define MSR_LBAR_ACPI 0x5140000E
+#define MSR_LBAR_PMS 0x5140000F
+
+/* resource sizes */
+#define LBAR_GPIO_SIZE 0xFF
+#define LBAR_MFGPT_SIZE 0x40
+#define LBAR_ACPI_SIZE 0x40
+#define LBAR_PMS_SIZE 0x80
+
+/* GPIOs */
+#define GPIO_OUTPUT_VAL 0x00
+#define GPIO_OUTPUT_ENABLE 0x04
+#define GPIO_OUTPUT_OPEN_DRAIN 0x08
+#define GPIO_OUTPUT_INVERT 0x0C
+#define GPIO_OUTPUT_AUX1 0x10
+#define GPIO_OUTPUT_AUX2 0x14
+#define GPIO_PULL_UP 0x18
+#define GPIO_PULL_DOWN 0x1C
+#define GPIO_INPUT_ENABLE 0x20
+#define GPIO_INPUT_INVERT 0x24
+#define GPIO_INPUT_FILTER 0x28
+#define GPIO_INPUT_EVENT_COUNT 0x2C
+#define GPIO_READ_BACK 0x30
+#define GPIO_INPUT_AUX1 0x34
+#define GPIO_EVENTS_ENABLE 0x38
+#define GPIO_LOCK_ENABLE 0x3C
+#define GPIO_POSITIVE_EDGE_EN 0x40
+#define GPIO_NEGATIVE_EDGE_EN 0x44
+#define GPIO_POSITIVE_EDGE_STS 0x48
+#define GPIO_NEGATIVE_EDGE_STS 0x4C
+
+#define GPIO_MAP_X 0xE0
+#define GPIO_MAP_Y 0xE4
+#define GPIO_MAP_Z 0xE8
+#define GPIO_MAP_W 0xEC
+
+void cs5535_gpio_set(unsigned offset, unsigned int reg);
+void cs5535_gpio_clear(unsigned offset, unsigned int reg);
+int cs5535_gpio_isset(unsigned offset, unsigned int reg);
+
+#endif
--
1.5.6.5
2009/6/10 Andres Salomon <[email protected]>:
> +config GPIO_CS5535
> + tristate "AMD CS5535/CS5536 GPIO support"
> + depends on PCI && !CS5535_GPIO && !MGEODE_LX
> + help
> + The AMD CS5535 and CS5536 southbridges support 28 GPIO pins that
> + can be used for quite a number of things. The CS5535/6 is found on
> + AMD Geode and Lemote Yeeloong devices.
Why do you depend on !MGEODE_LX? Are there any problems?
On Thu, 11 Jun 2009 16:46:35 +0200
Tobias Müller <[email protected]> wrote:
> 2009/6/10 Andres Salomon <[email protected]>:
> > +config GPIO_CS5535
> > + tristate "AMD CS5535/CS5536 GPIO support"
> > + depends on PCI && !CS5535_GPIO && !MGEODE_LX
> > + help
> > + The AMD CS5535 and CS5536 southbridges support 28 GPIO
> > pins that
> > + can be used for quite a number of things. The CS5535/6
> > is found on
> > + AMD Geode and Lemote Yeeloong devices.
>
> Why do you depend on !MGEODE_LX? Are there any problems?
Because arch/x86/kernel/{mfgpt,geode}_32.c are built if
CONFIG_MGEODE_LX is set, and they will happily clobber each other. We
need to get a broken-out mfgpt driver completed, and then we can rework
or remove the arch/x86/kernel/{mfgpt,geode}_32.c stuff.
It's probably possible to add a separate config for that
(CONFIG_GEODE_LEGACY or something?) rather than having it depend upon
CONFIG_MGEODE_LX, but I haven't looked into it. I'd rather spend the
time fixing it correctly than fretting about with the various config
permutations. :)
Here's the promised patch. :) I tried to solve all issues I had with
Andres' patch.
From: Tobias Mueller <[email protected]>
Added mask to enable/disable some GPIO pins.
Added names for GPIO pins.
Added code on GPIO initialisation to disable output/input aux functions.
Signed-off-by: Tobias Mueller <[email protected]>
---
diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index 5613889..8a0b290 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -18,6 +18,11 @@
#define DRV_NAME "cs5535-gpio"
#define GPIO_BAR 1
+#define GPIO_DEFAULT_MASK 0x0B003C66
+
+static ulong mask = GPIO_DEFAULT_MASK;
+module_param_named(mask, mask, ulong, 0444);
+MODULE_PARM_DESC(mask, "GPIO channel mask.");
static struct cs5535_gpio_chip {
struct gpio_chip chip;
@@ -102,6 +107,38 @@ EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
* Generic gpio_chip API support.
*/
+static int chip_gpio_request(struct gpio_chip *c, unsigned offset)
+{
+ struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chip->lock, flags);
+
+ /* check if this pin is available */
+ if ((mask & (1 << offset)) == 0) {
+ printk(KERN_INFO DRV_NAME
+ ": pin %u is not available (check mask)\n", offset);
+ return -EINVAL;
+ }
+
+ /* disable output aux 1 & 2 on this pin */
+ __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
+ __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
+
+ /* disable input aux 1 on this pin */
+ __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
+
+ /* disable output */
+ __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
+
+ /* enable input */
+ __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
+
+ spin_unlock_irqrestore(&chip->lock, flags);
+
+ return 0;
+}
+
static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
{
return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
@@ -145,19 +182,34 @@ static int chip_direction_output(struct
gpio_chip *c, unsigned offset, int val)
return 0;
}
+static char *cs5535_gpio_names[] = {
+ "GPIO0", "GPIO1", "GPIO2", "GPIO3",
+ "GPIO4", "GPIO5", "GPIO6", "GPIO7",
+ "GPIO8", "GPIO9", "GPIO10", "GPIO11",
+ "GPIO12", "GPIO13", "GPIO14", "GPIO15",
+ "GPIO16", "GPIO17", "GPIO18", "GPIO19",
+ "GPIO20", "GPIO21", "GPIO22", NULL,
+ "GPIO24", "GPIO25", "GPIO26", "GPIO27",
+ "GPIO28", NULL, NULL, NULL,
+};
+
static struct cs5535_gpio_chip cs5535_gpio_chip = {
.chip = {
.owner = THIS_MODULE,
.label = DRV_NAME,
.base = 0,
- .ngpio = 28,
+ .ngpio = 32,
+ .names = cs5535_gpio_names,
+
+ .request = chip_gpio_request,
.get = chip_gpio_get,
.set = chip_gpio_set,
.direction_input = chip_direction_input,
.direction_output = chip_direction_output,
+
},
};
@@ -165,6 +217,7 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
const struct pci_device_id *pci_id)
{
int err;
+ ulong mask_orig = mask;
/* There are two ways to get the GPIO base address; one is by
* fetching it from MSR_LBAR_GPIO, the other is by reading the
@@ -193,6 +246,18 @@ static int __init cs5535_gpio_probe(struct pci_dev *pdev,
dev_info(&pdev->dev, "allocated PCI BAR #%d: base 0x%llx\n", GPIO_BAR,
(unsigned long long) cs5535_gpio_chip.base);
+ /* mask out reserved pins */
+ mask &= 0x1F7FFFFF;
+
+ /* do not allow pin 28, Power Button, as there's special handling
+ * in the PMC needed. (note 12, p. 48) */
+ mask &= ~(1 << 28);
+
+ if (mask_orig != mask)
+ printk(KERN_INFO DRV_NAME
+ ": mask changed from 0x%08lX to 0x%08lX\n",
+ mask_orig, mask);
+
/* finally, register with the generic GPIO API */
err = gpiochip_add(&cs5535_gpio_chip.chip);
if (err) {
Awesome. Comments below..
On Thu, 11 Jun 2009 20:52:52 +0200
Tobias Müller <[email protected]> wrote:
> Here's the promised patch. :) I tried to solve all issues I had with
> Andres' patch.
>
>
>
> From: Tobias Mueller <[email protected]>
>
> Added mask to enable/disable some GPIO pins.
> Added names for GPIO pins.
> Added code on GPIO initialisation to disable output/input aux
> functions.
>
> Signed-off-by: Tobias Mueller <[email protected]>
> ---
> diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
> index 5613889..8a0b290 100644
> --- a/drivers/gpio/cs5535-gpio.c
> +++ b/drivers/gpio/cs5535-gpio.c
> @@ -18,6 +18,11 @@
>
> #define DRV_NAME "cs5535-gpio"
> #define GPIO_BAR 1
> +#define GPIO_DEFAULT_MASK 0x0B003C66
Where does this mask of available GPIOs originate from?
> +
> +static ulong mask = GPIO_DEFAULT_MASK;
> +module_param_named(mask, mask, ulong, 0444);
> +MODULE_PARM_DESC(mask, "GPIO channel mask.");
>
> static struct cs5535_gpio_chip {
> struct gpio_chip chip;
> @@ -102,6 +107,38 @@ EXPORT_SYMBOL_GPL(cs5535_gpio_isset);
> * Generic gpio_chip API support.
> */
>
> +static int chip_gpio_request(struct gpio_chip *c, unsigned offset)
> +{
> + struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *)
> c;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&chip->lock, flags);
> +
> + /* check if this pin is available */
> + if ((mask & (1 << offset)) == 0) {
> + printk(KERN_INFO DRV_NAME
> + ": pin %u is not available (check mask)\n",
> offset);
> + return -EINVAL;
There's a locking error here; you really want to spin_unlock_irqrestore
before returning.
> + }
> +
> + /* disable output aux 1 & 2 on this pin */
> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
> +
> + /* disable input aux 1 on this pin */
> + __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
> +
> + /* disable output */
> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
> +
> + /* enable input */
> + __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
I don't think this is the right place for all of this. Your earlier
email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
inputs. I'm fine with doing that here, but I don't see why you're also
disabling output and enabling input by default.
> +
> + spin_unlock_irqrestore(&chip->lock, flags);
> +
> + return 0;
> +}
> +
> static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
> @@ -145,19 +182,34 @@ static int chip_direction_output(struct
> gpio_chip *c, unsigned offset, int val)
> return 0;
> }
>
> +static char *cs5535_gpio_names[] = {
> + "GPIO0", "GPIO1", "GPIO2", "GPIO3",
> + "GPIO4", "GPIO5", "GPIO6", "GPIO7",
> + "GPIO8", "GPIO9", "GPIO10", "GPIO11",
> + "GPIO12", "GPIO13", "GPIO14", "GPIO15",
> + "GPIO16", "GPIO17", "GPIO18", "GPIO19",
> + "GPIO20", "GPIO21", "GPIO22", NULL,
> + "GPIO24", "GPIO25", "GPIO26", "GPIO27",
> + "GPIO28", NULL, NULL, NULL,
> +};
> +
> static struct cs5535_gpio_chip cs5535_gpio_chip = {
> .chip = {
> .owner = THIS_MODULE,
> .label = DRV_NAME,
>
> .base = 0,
> - .ngpio = 28,
> + .ngpio = 32,
Since GPIOs 29-31 aren't externally available, and 28 is
unavailable anyways, shouldn't we just set ngpio to 28?
> + .names = cs5535_gpio_names,
> +
> + .request = chip_gpio_request,
>
> .get = chip_gpio_get,
> .set = chip_gpio_set,
>
> .direction_input = chip_direction_input,
> .direction_output = chip_direction_output,
> +
> },
> };
>
> @@ -165,6 +217,7 @@ static int __init cs5535_gpio_probe(struct
> pci_dev *pdev, const struct pci_device_id *pci_id)
> {
> int err;
> + ulong mask_orig = mask;
>
> /* There are two ways to get the GPIO base address; one is by
> * fetching it from MSR_LBAR_GPIO, the other is by reading
> the @@ -193,6 +246,18 @@ static int __init cs5535_gpio_probe(struct
> pci_dev *pdev, dev_info(&pdev->dev, "allocated PCI BAR #%d: base
> 0x%llx\n", GPIO_BAR, (unsigned long long) cs5535_gpio_chip.base);
>
> + /* mask out reserved pins */
> + mask &= 0x1F7FFFFF;
> +
> + /* do not allow pin 28, Power Button, as there's special
> handling
> + * in the PMC needed. (note 12, p. 48) */
> + mask &= ~(1 << 28);
Nice, even a pointer to the note. :)
> +
> + if (mask_orig != mask)
> + printk(KERN_INFO DRV_NAME
> + ": mask changed from 0x%08lX to 0x%08lX\n",
> + mask_orig, mask);
> +
> /* finally, register with the generic GPIO API */
> err = gpiochip_add(&cs5535_gpio_chip.chip);
> if (err) {
>> #define DRV_NAME "cs5535-gpio"
>> #define GPIO_BAR 1
>> +#define GPIO_DEFAULT_MASK 0x0B003C66
>
> Where does this mask of available GPIOs originate from?
I had a comment in my original patch:
/**
* Some GPIO pins
* 31-29,23 : reserved (always mask out)
* 28 : Power Button
* 26 : PME#
* 22-16 : LPC
* 14,15 : SMBus
* 9,8 : UART1
* 7 : PCI INTB
* 3,4 : UART2/DDC
* 2 : IDE_IRQ0
* 1 : AC_BEEP
* 0 : PCI INTA
*
* If a mask was not specified, be conservative and only allow:
* 1,2,5,6,10-13,24,25,27
*/
I'll add this in my patch to clear it out.
>> + spin_lock_irqsave(&chip->lock, flags);
>> +
>> + /* check if this pin is available */
>> + if ((mask & (1 << offset)) == 0) {
>> + printk(KERN_INFO DRV_NAME
>> + ": pin %u is not available (check mask)\n",
>> offset);
>> + return -EINVAL;
>
> There's a locking error here; you really want to spin_unlock_irqrestore
> before returning.
Thanks.
>> + /* disable output aux 1 & 2 on this pin */
>> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
>> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
>> +
>> + /* disable input aux 1 on this pin */
>> + __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
>> +
>> + /* disable output */
>> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
>> +
>> + /* enable input */
>> + __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
>
> I don't think this is the right place for all of this. Your earlier
> email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
> inputs. I'm fine with doing that here, but I don't see why you're also
> disabling output and enabling input by default.
I mentioned this in an ealier mail too. When I request the GPIO from
userspace the direction file always contains "in", so I thought
this is the standard direction after resetting as I should be in a
defined state after requesting. But I didn't found anything
about this in GPIO lib documentation, so I would be fine to change
this if there is any common default behavoir.
>> - .ngpio = 28,
>> + .ngpio = 32,
>
> Since GPIOs 29-31 aren't externally available, and 28 is
> unavailable anyways, shouldn't we just set ngpio to 28?
I thought that 32 is in consistency with the datasheet as it always
talks about 32 GPIO pins.
On Thu, 11 Jun 2009 22:11:58 +0200
Tobias Müller <[email protected]> wrote:
> >> #define DRV_NAME "cs5535-gpio"
> >> #define GPIO_BAR 1
> >> +#define GPIO_DEFAULT_MASK 0x0B003C66
> >
> > Where does this mask of available GPIOs originate from?
>
> I had a comment in my original patch:
>
> /**
> * Some GPIO pins
> * 31-29,23 : reserved (always mask out)
> * 28 : Power Button
> * 26 : PME#
> * 22-16 : LPC
> * 14,15 : SMBus
> * 9,8 : UART1
> * 7 : PCI INTB
> * 3,4 : UART2/DDC
> * 2 : IDE_IRQ0
> * 1 : AC_BEEP
> * 0 : PCI INTA
> *
> * If a mask was not specified, be conservative and only allow:
> * 1,2,5,6,10-13,24,25,27
> */
>
> I'll add this in my patch to clear it out.
>
But why are you being conservative in the first place? If something's
using GPIOs, unless they're unmapped, you should allow it to use them
without requiring a boot arg.
For example, OLPC uses GPIO 7 for its DCON IRQ. With the masking
scheme, OLPC will need to set that mask from the default. I don't see
the point of having the mask at all if other drivers in the kernel are
going to be requesting GPIOs (presumably they know what they're doing).
> >> + /* disable output aux 1 & 2 on this pin */
> >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
> >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
> >> +
> >> + /* disable input aux 1 on this pin */
> >> + __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
> >> +
> >> + /* disable output */
> >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
> >> +
> >> + /* enable input */
> >> + __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
> >
> > I don't think this is the right place for all of this. Your earlier
> > email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
> > inputs. I'm fine with doing that here, but I don't see why you're
> > also disabling output and enabling input by default.
>
> I mentioned this in an ealier mail too. When I request the GPIO from
> userspace the direction file always contains "in", so I thought
> this is the standard direction after resetting as I should be in a
> defined state after requesting. But I didn't found anything
> about this in GPIO lib documentation, so I would be fine to change
> this if there is any common default behavoir.
To be honest, I'd have to play around with it a bit before I
knew whether it actually breaks anything or not. I'm not sure if
it would break anything on OLPC, and I don't have any other geode
machines that do anything interesting w/ GPIOs.
Maybe David can clear up whether this behavior is correct from the
userspace GPIO usage standpoint..
>
> >> - .ngpio = 28,
> >> + .ngpio = 32,
> >
> > Since GPIOs 29-31 aren't externally available, and 28 is
> > unavailable anyways, shouldn't we just set ngpio to 28?
> I thought that 32 is in consistency with the datasheet as it always
> talks about 32 GPIO pins.
Fair enough.
>> /**
>> * Some GPIO pins
>> * 31-29,23 : reserved (always mask out)
>> * 28 : Power Button
>> * 26 : PME#
>> * 22-16 : LPC
>> * 14,15 : SMBus
>> * 9,8 : UART1
>> * 7 : PCI INTB
>> * 3,4 : UART2/DDC
>> * 2 : IDE_IRQ0
>> * 1 : AC_BEEP
>> * 0 : PCI INTA
>> *
>> * If a mask was not specified, be conservative and only allow:
>> * 1,2,5,6,10-13,24,25,27
>> */
>>
>> I'll add this in my patch to clear it out.
>>
>
> But why are you being conservative in the first place? If something's
> using GPIOs, unless they're unmapped, you should allow it to use them
> without requiring a boot arg.
>
> For example, OLPC uses GPIO 7 for its DCON IRQ. With the masking
> scheme, OLPC will need to set that mask from the default. I don't see
> the point of having the mask at all if other drivers in the kernel are
> going to be requesting GPIOs (presumably they know what they're doing).
Hmm... OK, this makes sense. So default mask allow everything exept
reserved pins and pin 28 (power button).
I think the mask is quite useful if you've critical things on GPIO pins
and they should be changeable (especially from userspace and when
non-root users are allowed to use userspace gpio).
>> >> + /* disable output aux 1 & 2 on this pin */
>> >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX1);
>> >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_AUX2);
>> >> +
>> >> + /* disable input aux 1 on this pin */
>> >> + __cs5535_gpio_clear(chip, offset, GPIO_INPUT_AUX1);
>> >> +
>> >> + /* disable output */
>> >> + __cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
>> >> +
>> >> + /* enable input */
>> >> + __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
>> >
>> > I don't think this is the right place for all of this. Your earlier
>> > email mentioned disabling OUT_AUX{1,2} for outputs, and IN_AUX for
>> > inputs. I'm fine with doing that here, but I don't see why you're
>> > also disabling output and enabling input by default.
>>
>> I mentioned this in an ealier mail too. When I request the GPIO from
>> userspace the direction file always contains "in", so I thought
>> this is the standard direction after resetting as I should be in a
>> defined state after requesting. But I didn't found anything
>> about this in GPIO lib documentation, so I would be fine to change
>> this if there is any common default behavoir.
>
> To be honest, I'd have to play around with it a bit before I
> knew whether it actually breaks anything or not. I'm not sure if
> it would break anything on OLPC, and I don't have any other geode
> machines that do anything interesting w/ GPIOs.
>
> Maybe David can clear up whether this behavior is correct from the
> userspace GPIO usage standpoint..
This would be nice. But I wouldn't have a problem when those
two statements are removed. I just thought it's better to put them
in a defined state.
On Thu, 11 Jun 2009 23:35:55 +0200
Tobias Müller <[email protected]> wrote:
> >> /**
> >> * Some GPIO pins
> >> * 31-29,23 : reserved (always mask out)
> >> * 28 : Power Button
> >> * 26 : PME#
> >> * 22-16 : LPC
> >> * 14,15 : SMBus
> >> * 9,8 : UART1
> >> * 7 : PCI INTB
> >> * 3,4 : UART2/DDC
> >> * 2 : IDE_IRQ0
> >> * 1 : AC_BEEP
> >> * 0 : PCI INTA
> >> *
> >> * If a mask was not specified, be conservative and only allow:
> >> * 1,2,5,6,10-13,24,25,27
> >> */
> >>
> >> I'll add this in my patch to clear it out.
> >>
> >
> > But why are you being conservative in the first place? If
> > something's using GPIOs, unless they're unmapped, you should allow
> > it to use them without requiring a boot arg.
> >
> > For example, OLPC uses GPIO 7 for its DCON IRQ. With the masking
> > scheme, OLPC will need to set that mask from the default. I don't
> > see the point of having the mask at all if other drivers in the
> > kernel are going to be requesting GPIOs (presumably they know what
> > they're doing).
> Hmm... OK, this makes sense. So default mask allow everything exept
> reserved pins and pin 28 (power button).
>
> I think the mask is quite useful if you've critical things on GPIO
> pins and they should be changeable (especially from userspace and when
> non-root users are allowed to use userspace gpio).
I agree that it would be useful for userspace, just not for
kernelspace. Is there a way that you can have it only enforce the
mask if the request is coming from userspace?
2009/6/13 Andres Salomon <[email protected]>:
> On Thu, 11 Jun 2009 23:35:55 +0200
> Tobias Müller <[email protected]> wrote:
>
>> >> /**
>> >> * Some GPIO pins
>> >> * 31-29,23 : reserved (always mask out)
>> >> * 28 : Power Button
>> >> * 26 : PME#
>> >> * 22-16 : LPC
>> >> * 14,15 : SMBus
>> >> * 9,8 : UART1
>> >> * 7 : PCI INTB
>> >> * 3,4 : UART2/DDC
>> >> * 2 : IDE_IRQ0
>> >> * 1 : AC_BEEP
>> >> * 0 : PCI INTA
>> >> *
>> >> * If a mask was not specified, be conservative and only allow:
>> >> * 1,2,5,6,10-13,24,25,27
>> >> */
>> >>
>> >> I'll add this in my patch to clear it out.
>> >>
>> >
>> > But why are you being conservative in the first place? If
>> > something's using GPIOs, unless they're unmapped, you should allow
>> > it to use them without requiring a boot arg.
>> >
>> > For example, OLPC uses GPIO 7 for its DCON IRQ. With the masking
>> > scheme, OLPC will need to set that mask from the default. I don't
>> > see the point of having the mask at all if other drivers in the
>> > kernel are going to be requesting GPIOs (presumably they know what
>> > they're doing).
>> Hmm... OK, this makes sense. So default mask allow everything exept
>> reserved pins and pin 28 (power button).
>>
>> I think the mask is quite useful if you've critical things on GPIO
>> pins and they should be changeable (especially from userspace and when
>> non-root users are allowed to use userspace gpio).
>
> I agree that it would be useful for userspace, just not for
> kernelspace. Is there a way that you can have it only enforce the
> mask if the request is coming from userspace?
I didn't find a way to find out if a request is coming from userspace, so at
the moment I think it's only possible to set it global.
On Thursday 11 June 2009, Andres Salomon wrote:
>
> > I mentioned this in an ealier mail too. When I request the GPIO from
> > userspace the direction file always contains "in", so I thought
> > this is the standard direction after resetting as I should be in a
> > defined state after requesting. But I didn't found anything
> > about this in GPIO lib documentation, so I would be fine to change
> > this if there is any common default behavoir.
>
> To be honest, I'd have to play around with it a bit before I
> knew whether it actually breaks anything or not. I'm not sure if
> it would break anything on OLPC, and I don't have any other geode
> machines that do anything interesting w/ GPIOs.
>
> Maybe David can clear up whether this behavior is correct from the
> userspace GPIO usage standpoint..
Correct-but-annoying ... and maybe worth changing. The
direction *displayed* may not reflect the actual hardware
until after that GPIO signal is initialized. Boot firmware
may well have set the direction; Linux shouldn't change it
without explicit instructions to do so.
The issue is that when it first comes up, nobody has tod
Linux that direction ... so instead of defining an "unknown"
state, that code applies a heuristic. In gpiochip_add():
for (id = base; id < base + chip->ngpio; id++) {
gpio_desc[id].chip = chip;
/* REVISIT: most hardware initializes GPIOs as
* inputs (often with pullups enabled) so power
* usage is minimized. Linux code should set the
* gpio direction first thing; but until it does,
* we may expose the wrong direction in sysfs.
*/
gpio_desc[id].flags = !chip->direction_input
? (1 << FLAG_IS_OUT)
: 0;
}
Now, as far as heuristics go that one seems to cover most
of the common cases. But like all heuristics, the result
produced may be wrong.
So it would be nice to remove the heuristic. The best
way would be to add a new method to query gpio direction.
Then that when it's available, instead of the heuristic.
- Dave