LinuxLists.cc - [PATCH v5 0/3] Add gpio support to Broadcom Cygnus SoC

[permalink] [raw]

Subject: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

Document the GPIO device tree binding for Broadcom Cygnus SoC

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
.../devicetree/bindings/gpio/brcm,cygnus-gpio.txt | 87 ++++++++++++++++++++
1 file changed, 87 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt
new file mode 100644
index 0000000..0e446d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/brcm,cygnus-gpio.txt
@@ -0,0 +1,87 @@
+Broadcom Cygnus GPIO Controller
+
+Required properties:
+
+- compatible:
+ Must be "brcm,cygnus-gpio"
+
+- reg:
+ Define the base and range of the I/O address space that contain the Cygnus
+GPIO controller registers
+
+- ngpios:
+ Total number of GPIOs the controller provides
+
+- linux,gpio-base:
+ Base GPIO number of this controller
+
+- #gpio-cells:
+ Must be two. The first cell is the GPIO pin number (within the
+controller's domain) and the second cell is used for the following:
+ bit[0]: polarity (0 for normal and 1 for inverted)
+ bit[18:16]: internal pull up/down: 0 - pull up/down disabled
+ 1 - pull up enabled
+ 2 - pull down enabled
+ bit[22:20]: drive strength: 0 - 2 mA
+ 1 - 4 mA
+ 2 - 6 mA
+ 3 - 8 mA
+ 4 - 10 mA
+ 5 - 12 mA
+ 6 - 14 mA
+ 7 - 16 mA
+
+- gpio-controller:
+ Specifies that the node is a GPIO controller
+
+Optional properties:
+
+- interrupt-controller:
+ Specifies that the node is an interrupt controller. Not all Cygnus GPIO
+interfaces support interrupt, e.g., the CRMU GPIO controller does not have its
+interrupt routed to the main processor's GIC
+
+- interrupts:
+ The interrupt outputs from the GPIO controller.
+
+- no-drv-strength:
+ Specifies the GPIO controller does not support drive strength configuration
+
+Example:
+ gpio_asiu: gpio@180a5000 {
+ compatible = "brcm,cygnus-gpio";
+ reg = <0x180a5000 0x668>;
+ ngpios = <122>;
+ linux,gpio-base = <0>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ interrupt-controller;
+ interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
+ gpio_crmu: gpio@03024800 {
+ compatible = "brcm,cygnus-gpio";
+ reg = <0x03024800 0x50>;
+ ngpios = <6>;
+ linux,gpio-base = <146>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ no-drv-strength;
+ };
+
+ /*
+ * Touchscreen that uses the ASIU GPIO 100, with internal pull-up
+ * enabled
+ */
+ tsc {
+ ...
+ ...
+ gpio-event = <&gpio_asiu 100 0x10000>;
+ };
+
+ /* Bluetooth that uses the CRMU GPIO 2, with polarity inverted */
+ bluetooth {
+ ...
+ ...
+ bcm,rfkill-bank-sel = <&gpio_crmu 2 1>
+ }
--
1.7.9.5

2014-12-12 00:02:58

[permalink] [raw]

Subject: [PATCH v5 2/3] gpio: Cygnus: add GPIO driver

This GPIO driver supports all 3 GPIO controllers in the Broadcom Cygnus
SoC. The 3 GPIO controllers are 1) the ASIU GPIO controller, 2) the
chipCommonG GPIO controller, and 3) the ALWAYS-ON GPIO controller

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/gpio/Kconfig | 12 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-bcm-cygnus.c | 613 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 626 insertions(+)
create mode 100644 drivers/gpio/gpio-bcm-cygnus.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 633ec21..1790ffd 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -126,6 +126,18 @@ config GPIO_74XX_MMIO
8 bits: 74244 (Input), 74273 (Output)
16 bits: 741624 (Input), 7416374 (Output)

+config GPIO_BCM_CYGNUS
+ bool "Broadcom Cygnus GPIO support"
+ depends on ARCH_BCM_CYGNUS && OF_GPIO
+ default y
+ help
+ Say yes here to turn on GPIO support for Broadcom Cygnus SoC
+
+ The Broadcom Cygnus SoC has 3 GPIO controllers including the ASIU
+ GPIO controller (ASIU), the chipCommonG GPIO controller (CCM), and
+ the always-ON GPIO controller (CRMU). All 3 GPIO controllers are
+ supported by this driver
+
config GPIO_CLPS711X
tristate "CLPS711X GPIO support"
depends on ARCH_CLPS711X || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 81755f1..31eb7e0 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
+obj-$(CONFIG_GPIO_BCM_CYGNUS) += gpio-bcm-cygnus.o
obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
diff --git a/drivers/gpio/gpio-bcm-cygnus.c b/drivers/gpio/gpio-bcm-cygnus.c
new file mode 100644
index 0000000..a6a7732
--- /dev/null
+++ b/drivers/gpio/gpio-bcm-cygnus.c
@@ -0,0 +1,613 @@
+/*
+ * Copyright (C) 2014 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/ioport.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/irqchip/chained_irq.h>
+
+#define CYGNUS_GPIO_DATA_IN_OFFSET 0x00
+#define CYGNUS_GPIO_DATA_OUT_OFFSET 0x04
+#define CYGNUS_GPIO_OUT_EN_OFFSET 0x08
+#define CYGNUS_GPIO_IN_TYPE_OFFSET 0x0c
+#define CYGNUS_GPIO_INT_DE_OFFSET 0x10
+#define CYGNUS_GPIO_INT_EDGE_OFFSET 0x14
+#define CYGNUS_GPIO_INT_MSK_OFFSET 0x18
+#define CYGNUS_GPIO_INT_STAT_OFFSET 0x1c
+#define CYGNUS_GPIO_INT_MSTAT_OFFSET 0x20
+#define CYGNUS_GPIO_INT_CLR_OFFSET 0x24
+#define CYGNUS_GPIO_PAD_RES_OFFSET 0x34
+#define CYGNUS_GPIO_RES_EN_OFFSET 0x38
+
+/* drive strength control for ASIU GPIO */
+#define CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET 0x58
+
+/* drive strength control for CCM GPIO */
+#define CYGNUS_GPIO_CCM_DRV0_CTRL_OFFSET 0x00
+
+#define GPIO_BANK_SIZE 0x200
+#define NGPIOS_PER_BANK 32
+#define GPIO_BANK(pin) ((pin) / NGPIOS_PER_BANK)
+
+#define CYGNUS_GPIO_REG(pin, reg) (GPIO_BANK(pin) * GPIO_BANK_SIZE + (reg))
+#define CYGNUS_GPIO_SHIFT(pin) ((pin) % NGPIOS_PER_BANK)
+
+#define GPIO_FLAG_BIT_MASK 0xffff
+#define GPIO_PULL_BIT_SHIFT 16
+#define GPIO_PULL_BIT_MASK 0x3
+
+#define GPIO_DRV_STRENGTH_BIT_SHIFT 20
+#define GPIO_DRV_STRENGTH_BITS 3
+#define GPIO_DRV_STRENGTH_BIT_MASK ((1 << GPIO_DRV_STRENGTH_BITS) - 1)
+
+/*
+ * For GPIO internal pull up/down registers
+ */
+enum gpio_pull {
+ GPIO_PULL_NONE = 0,
+ GPIO_PULL_UP,
+ GPIO_PULL_DOWN,
+ GPIO_PULL_INVALID,
+};
+
+/*
+ * GPIO drive strength
+ */
+enum gpio_drv_strength {
+ GPIO_DRV_STRENGTH_2MA = 0,
+ GPIO_DRV_STRENGTH_4MA,
+ GPIO_DRV_STRENGTH_6MA,
+ GPIO_DRV_STRENGTH_8MA,
+ GPIO_DRV_STRENGTH_10MA,
+ GPIO_DRV_STRENGTH_12MA,
+ GPIO_DRV_STRENGTH_14MA,
+ GPIO_DRV_STRENGTH_16MA,
+ GPIO_DRV_STRENGTH_INVALID,
+};
+
+struct cygnus_gpio {
+ struct device *dev;
+ void __iomem *base;
+ void __iomem *io_ctrl;
+ spinlock_t lock;
+ struct gpio_chip gc;
+ unsigned num_banks;
+ int irq;
+ struct irq_domain *irq_domain;
+};
+
+static struct cygnus_gpio *to_cygnus_gpio(struct gpio_chip *gc)
+{
+ return container_of(gc, struct cygnus_gpio, gc);
+}
+
+static u32 cygnus_readl(struct cygnus_gpio *cygnus_gpio, unsigned int offset)
+{
+ return readl(cygnus_gpio->base + offset);
+}
+
+static void cygnus_writel(struct cygnus_gpio *cygnus_gpio,
+ unsigned int offset, u32 val)
+{
+ writel(val, cygnus_gpio->base + offset);
+}
+
+/**
+ * cygnus_set_bit - set or clear one bit (corresponding to the GPIO pin) in a
+ * Cygnus GPIO register
+ *
+ * @cygnus_gpio: Cygnus GPIO device
+ * @reg: register offset
+ * @gpio: GPIO pin
+ * @set: set or clear. 1 - set; 0 -clear
+ */
+static void cygnus_set_bit(struct cygnus_gpio *cygnus_gpio,
+ unsigned int reg, unsigned gpio, int set)
+{
+ unsigned int offset = CYGNUS_GPIO_REG(gpio, reg);
+ unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+ u32 val;
+
+ val = cygnus_readl(cygnus_gpio, offset);
+ if (set)
+ val |= BIT(shift);
+ else
+ val &= ~BIT(shift);
+ cygnus_writel(cygnus_gpio, offset, val);
+}
+
+static int cygnus_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+ struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
+
+ return irq_find_mapping(cygnus_gpio->irq_domain, offset);
+}
+
+static void cygnus_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct cygnus_gpio *cygnus_gpio;
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ int i, bit;
+
+ chained_irq_enter(chip, desc);
+
+ cygnus_gpio = irq_get_handler_data(irq);
+
+ /* go through the entire GPIO banks and handle all interrupts */
+ for (i = 0; i < cygnus_gpio->num_banks; i++) {
+ unsigned long val = cygnus_readl(cygnus_gpio,
+ (i * GPIO_BANK_SIZE) +
+ CYGNUS_GPIO_INT_MSTAT_OFFSET);
+
+ for_each_set_bit(bit, &val, NGPIOS_PER_BANK) {
+ unsigned pin = NGPIOS_PER_BANK * i + bit;
+ int child_irq =
+ cygnus_gpio_to_irq(&cygnus_gpio->gc, pin);
+
+ /*
+ * Clear the interrupt before invoking the
+ * handler, so we do not leave any window
+ */
+ cygnus_writel(cygnus_gpio, (i * GPIO_BANK_SIZE) +
+ CYGNUS_GPIO_INT_CLR_OFFSET, BIT(bit));
+
+ generic_handle_irq(child_irq);
+ }
+ }
+
+ chained_irq_exit(chip, desc);
+}
+
+
+static void cygnus_gpio_irq_ack(struct irq_data *d)
+{
+ struct cygnus_gpio *cygnus_gpio = irq_data_get_irq_chip_data(d);
+ unsigned gpio = d->hwirq;
+ unsigned int offset = CYGNUS_GPIO_REG(gpio,
+ CYGNUS_GPIO_INT_CLR_OFFSET);
+ unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+ u32 val = BIT(shift);
+
+ cygnus_writel(cygnus_gpio, offset, val);
+}
+
+/**
+ * cygnus_gpio_irq_set_mask - mask/unmask a GPIO interrupt
+ *
+ * @d: IRQ chip data
+ * @mask: mask/unmask GPIO interrupt. 0 - mask (disable); 1 - unmask (enable)
+ */
+static void cygnus_gpio_irq_set_mask(struct irq_data *d, int mask)
+{
+ struct cygnus_gpio *cygnus_gpio = irq_data_get_irq_chip_data(d);
+ unsigned gpio = d->hwirq;
+
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_INT_MSK_OFFSET, gpio, mask);
+}
+
+static void cygnus_gpio_irq_mask(struct irq_data *d)
+{
+ struct cygnus_gpio *cygnus_gpio = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ cygnus_gpio_irq_set_mask(d, 0);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+}
+
+static void cygnus_gpio_irq_unmask(struct irq_data *d)
+{
+ struct cygnus_gpio *cygnus_gpio = irq_data_get_irq_chip_data(d);
+ unsigned long flags;
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ cygnus_gpio_irq_set_mask(d, 1);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+}
+
+static int cygnus_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ struct cygnus_gpio *cygnus_gpio = irq_data_get_irq_chip_data(d);
+ unsigned gpio = d->hwirq;
+ int int_type = 0, dual_edge = 0, edge_lvl = 0;
+ unsigned long flags;
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_RISING:
+ edge_lvl = 1;
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ break;
+
+ case IRQ_TYPE_EDGE_BOTH:
+ dual_edge = 1;
+ break;
+
+ case IRQ_TYPE_LEVEL_HIGH:
+ int_type = 1;
+ edge_lvl = 1;
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ int_type = 1;
+ break;
+
+ default:
+ dev_err(cygnus_gpio->dev, "invalid GPIO IRQ type 0x%x\n",
+ type);
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_IN_TYPE_OFFSET, gpio,
+ int_type);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_INT_DE_OFFSET, gpio,
+ dual_edge);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_INT_EDGE_OFFSET, gpio,
+ edge_lvl);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+
+ dev_dbg(cygnus_gpio->dev,
+ "gpio:%u set int_type:%d dual_edge:%d edge_lvl:%d\n",
+ gpio, int_type, dual_edge, edge_lvl);
+
+ return 0;
+}
+
+static struct irq_chip cygnus_gpio_irq_chip = {
+ .name = "bcm-cygnus-gpio",
+ .irq_ack = cygnus_gpio_irq_ack,
+ .irq_mask = cygnus_gpio_irq_mask,
+ .irq_unmask = cygnus_gpio_irq_unmask,
+ .irq_set_type = cygnus_gpio_irq_set_type,
+};
+
+static int cygnus_gpio_direction_input(struct gpio_chip *gc, unsigned gpio)
+{
+ struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_OUT_EN_OFFSET, gpio, 0);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+
+ dev_dbg(cygnus_gpio->dev, "gpio:%u set input\n", gpio);
+
+ return 0;
+}
+
+static int cygnus_gpio_direction_output(struct gpio_chip *gc,
+ unsigned gpio, int value)
+{
+ struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_OUT_EN_OFFSET, gpio, 1);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_DATA_OUT_OFFSET, gpio, value);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+
+ dev_dbg(cygnus_gpio->dev, "gpio:%u set output, value:%d\n",
+ gpio, value);
+
+ return 0;
+}
+
+static void cygnus_gpio_set(struct gpio_chip *gc, unsigned gpio,
+ int value)
+{
+ struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_DATA_OUT_OFFSET, gpio, value);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+
+ dev_dbg(cygnus_gpio->dev, "gpio:%u set, value:%d\n", gpio, value);
+}
+
+static int cygnus_gpio_get(struct gpio_chip *gc, unsigned gpio)
+{
+ struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
+ unsigned int offset = CYGNUS_GPIO_REG(gpio,
+ CYGNUS_GPIO_DATA_IN_OFFSET);
+ unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
+ u32 val;
+
+ val = cygnus_readl(cygnus_gpio, offset);
+ val = (val >> shift) & 1;
+
+ dev_dbg(cygnus_gpio->dev, "gpio:%u get, value:%d\n", gpio, val);
+
+ return val;
+}
+
+static struct lock_class_key gpio_lock_class;
+
+static int cygnus_gpio_irq_map(struct irq_domain *d, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ int ret;
+
+ ret = irq_set_chip_data(irq, d->host_data);
+ if (ret < 0)
+ return ret;
+ irq_set_lockdep_class(irq, &gpio_lock_class);
+ irq_set_chip_and_handler(irq, &cygnus_gpio_irq_chip,
+ handle_simple_irq);
+ set_irq_flags(irq, IRQF_VALID);
+
+ return 0;
+}
+
+static void cygnus_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
+{
+ irq_set_chip_and_handler(irq, NULL, NULL);
+ irq_set_chip_data(irq, NULL);
+}
+
+static struct irq_domain_ops cygnus_irq_ops = {
+ .map = cygnus_gpio_irq_map,
+ .unmap = cygnus_gpio_irq_unmap,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+#ifdef CONFIG_OF_GPIO
+static void cygnus_gpio_set_pull(struct cygnus_gpio *cygnus_gpio,
+ unsigned gpio, enum gpio_pull pull)
+{
+ int pullup;
+ unsigned long flags;
+
+ switch (pull) {
+ case GPIO_PULL_UP:
+ pullup = 1;
+ break;
+ case GPIO_PULL_DOWN:
+ pullup = 0;
+ break;
+ case GPIO_PULL_NONE:
+ case GPIO_PULL_INVALID:
+ default:
+ return;
+ }
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+ /* set pull up/down */
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_PAD_RES_OFFSET, gpio, pullup);
+ /* enable pad */
+ cygnus_set_bit(cygnus_gpio, CYGNUS_GPIO_RES_EN_OFFSET, gpio, 1);
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+
+ dev_dbg(cygnus_gpio->dev, "gpio:%u set pullup:%d\n", gpio, pullup);
+}
+
+static void cygnus_gpio_set_strength(struct cygnus_gpio *cygnus_gpio,
+ unsigned gpio, enum gpio_drv_strength strength)
+{
+ struct device *dev = cygnus_gpio->dev;
+ void __iomem *base;
+ unsigned int i, offset, shift;
+ u32 val;
+ unsigned long flags;
+
+ /* some GPIO controllers do not support drive strength configuration */
+ if (of_find_property(dev->of_node, "no-drv-strength", NULL))
+ return;
+
+ /*
+ * Some GPIO controllers use a different register block for drive
+ * strength control
+ */
+ if (cygnus_gpio->io_ctrl) {
+ base = cygnus_gpio->io_ctrl;
+ offset = CYGNUS_GPIO_CCM_DRV0_CTRL_OFFSET;
+ } else {
+ base = cygnus_gpio->base;
+ offset = CYGNUS_GPIO_REG(gpio,
+ CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET);
+ }
+
+ shift = CYGNUS_GPIO_SHIFT(gpio);
+
+ spin_lock_irqsave(&cygnus_gpio->lock, flags);
+
+ for (i = 0; i < GPIO_DRV_STRENGTH_BITS; i++) {
+ val = readl(base + offset);
+ val &= ~BIT(shift);
+ val |= ((strength >> i) & 0x1) << shift;
+ writel(val, base + offset);
+ offset += 4;
+ }
+
+ spin_unlock_irqrestore(&cygnus_gpio->lock, flags);
+
+ dev_dbg(cygnus_gpio->dev,
+ "gpio:%u set drive strength:%d\n", gpio, strength);
+}
+
+static int cygnus_gpio_of_xlate(struct gpio_chip *gc,
+ const struct of_phandle_args *gpiospec, u32 *flags)
+{
+ struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
+ enum gpio_pull pull;
+ enum gpio_drv_strength strength;
+
+ if (gc->of_gpio_n_cells < 2)
+ return -EINVAL;
+
+ if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+ return -EINVAL;
+
+ if (gpiospec->args[0] >= gc->ngpio)
+ return -EINVAL;
+
+ pull = (gpiospec->args[1] >> GPIO_PULL_BIT_SHIFT) & GPIO_PULL_BIT_MASK;
+ if (WARN_ON(pull >= GPIO_PULL_INVALID))
+ return -EINVAL;
+
+ strength = (gpiospec->args[1] >> GPIO_DRV_STRENGTH_BIT_SHIFT) &
+ GPIO_DRV_STRENGTH_BIT_MASK;
+
+ if (flags)
+ *flags = gpiospec->args[1] & GPIO_FLAG_BIT_MASK;
+
+ cygnus_gpio_set_pull(cygnus_gpio, gpiospec->args[0], pull);
+ cygnus_gpio_set_strength(cygnus_gpio, gpiospec->args[0], strength);
+
+ return gpiospec->args[0];
+}
+#endif
+
+static const struct of_device_id cygnus_gpio_of_match[] = {
+ { .compatible = "brcm,cygnus-gpio" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, cygnus_gpio_of_match);
+
+static int cygnus_gpio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct cygnus_gpio *cygnus_gpio;
+ struct gpio_chip *gc;
+ u32 i, ngpios, gpio_base;
+ int ret;
+
+ cygnus_gpio = devm_kzalloc(dev, sizeof(*cygnus_gpio), GFP_KERNEL);
+ if (!cygnus_gpio)
+ return -ENOMEM;
+
+ cygnus_gpio->dev = dev;
+ platform_set_drvdata(pdev, cygnus_gpio);
+
+ if (of_property_read_u32(dev->of_node, "ngpios", &ngpios)) {
+ dev_err(&pdev->dev, "missing ngpios DT property\n");
+ return -ENODEV;
+ }
+ cygnus_gpio->num_banks = (ngpios + NGPIOS_PER_BANK - 1) /
+ NGPIOS_PER_BANK;
+
+ if (of_property_read_u32(dev->of_node, "linux,gpio-base",
+ &gpio_base)) {
+ dev_err(&pdev->dev, "missing linux,gpio-base DT property\n");
+ return -ENODEV;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ cygnus_gpio->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(cygnus_gpio->base)) {
+ dev_err(&pdev->dev, "unable to map I/O memory\n");
+ return PTR_ERR(cygnus_gpio->base);
+ }
+
+ /*
+ * Only certain types of Cygnus GPIO interfaces have I/O control
+ * registers
+ */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (res) {
+ cygnus_gpio->io_ctrl = devm_ioremap_resource(dev, res);
+ if (IS_ERR(cygnus_gpio->io_ctrl)) {
+ dev_err(&pdev->dev, "unable to map I/O memory\n");
+ return PTR_ERR(cygnus_gpio->io_ctrl);
+ }
+ }
+
+ spin_lock_init(&cygnus_gpio->lock);
+
+ gc = &cygnus_gpio->gc;
+ gc->base = gpio_base;
+ gc->ngpio = ngpios;
+ gc->label = dev_name(dev);
+ gc->dev = dev;
+#ifdef CONFIG_OF_GPIO
+ gc->of_node = dev->of_node;
+ gc->of_gpio_n_cells = 2;
+ gc->of_xlate = cygnus_gpio_of_xlate;
+#endif
+ gc->direction_input = cygnus_gpio_direction_input;
+ gc->direction_output = cygnus_gpio_direction_output;
+ gc->set = cygnus_gpio_set;
+ gc->get = cygnus_gpio_get;
+ gc->to_irq = cygnus_gpio_to_irq;
+
+ ret = gpiochip_add(gc);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "unable to add GPIO chip\n");
+ return ret;
+ }
+
+ /*
+ * Some of the GPIO interfaces do not have interrupt wired to the main
+ * processor
+ */
+ cygnus_gpio->irq = platform_get_irq(pdev, 0);
+ if (cygnus_gpio->irq < 0) {
+ ret = cygnus_gpio->irq;
+ if (ret == -EPROBE_DEFER)
+ goto err_rm_gpiochip;
+
+ dev_info(&pdev->dev, "no interrupt hook\n");
+ }
+
+ cygnus_gpio->irq_domain = irq_domain_add_linear(dev->of_node,
+ gc->ngpio, &cygnus_irq_ops, cygnus_gpio);
+ if (!cygnus_gpio->irq_domain) {
+ dev_err(&pdev->dev, "unable to allocate IRQ domain\n");
+ ret = -ENXIO;
+ goto err_rm_gpiochip;
+ }
+
+ for (i = 0; i < gc->ngpio; i++) {
+ int irq = irq_create_mapping(cygnus_gpio->irq_domain, i);
+
+ irq_set_lockdep_class(irq, &gpio_lock_class);
+ irq_set_chip_data(irq, cygnus_gpio);
+ irq_set_chip_and_handler(irq, &cygnus_gpio_irq_chip,
+ handle_simple_irq);
+ set_irq_flags(irq, IRQF_VALID);
+ }
+
+ irq_set_chained_handler(cygnus_gpio->irq, cygnus_gpio_irq_handler);
+ irq_set_handler_data(cygnus_gpio->irq, cygnus_gpio);
+
+ return 0;
+
+err_rm_gpiochip:
+ gpiochip_remove(gc);
+ return ret;
+}
+
+static struct platform_driver cygnus_gpio_driver = {
+ .driver = {
+ .name = "bcm-cygnus-gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = cygnus_gpio_of_match,
+ },
+ .probe = cygnus_gpio_probe,
+};
+
+module_platform_driver(cygnus_gpio_driver);
+
+MODULE_AUTHOR("Ray Jui <[email protected]>");
+MODULE_DESCRIPTION("Broadcom Cygnus GPIO Driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5

2014-12-12 00:03:12

[permalink] [raw]

Subject: [PATCH v5 3/3] ARM: dts: enable GPIO for Broadcom Cygnus

This enables all 3 GPIO controllers including the ASIU GPIO, the
chipcommonG GPIO, and the ALWAYS-ON GPIO, for Broadcom Cygnus SoC

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
arch/arm/boot/dts/bcm-cygnus.dtsi | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 5126f9e..35272b7 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -54,6 +54,39 @@

/include/ "bcm-cygnus-clock.dtsi"

+ gpio_ccm: gpio@1800a000 {
+ compatible = "brcm,cygnus-gpio";
+ reg = <0x1800a000 0x50>,
+ <0x0301d164 0x20>;
+ ngpios = <24>;
+ linux,gpio-base = <0>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-controller;
+ };
+
+ gpio_asiu: gpio@180a5000 {
+ compatible = "brcm,cygnus-gpio";
+ reg = <0x180a5000 0x668>;
+ ngpios = <122>;
+ linux,gpio-base = <24>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ interrupt-controller;
+ interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
+ gpio_crmu: gpio@03024800 {
+ compatible = "brcm,cygnus-gpio";
+ reg = <0x03024800 0x50>;
+ ngpios = <6>;
+ linux,gpio-base = <146>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ no-drv-strength;
+ };
+
amba {
#address-cells = <1>;
#size-cells = <1>;
--
1.7.9.5

2014-12-12 12:09:01

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
> +
> +- linux,gpio-base:
> + Base GPIO number of this controller
> +
>

We've NAK'ed properties like this multiple times before, and it
doesn't get any better this time. What are you trying to achieve
here?

Arnd

2014-12-12 13:06:00

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann <[email protected]> wrote:
> On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
>> +
>> +- linux,gpio-base:
>> + Base GPIO number of this controller
>> +
>>
>
> We've NAK'ed properties like this multiple times before, and it
> doesn't get any better this time. What are you trying to achieve
> here?

I am to blame for suggesting using this property to Ray, and I am
fully aware that this has been rejected before, but look at what
people came with recently to palliate the lack of control over the
GPIO number space for DT platforms:

http://www.spinics.net/lists/arm-kernel/msg384847.html
https://lkml.org/lkml/2014/12/10/133

Right now GPIO numbering for platforms using DT is a very inconsistent
process, subject to change by the simple action of adjusting the value
of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
controller, or changing the probe order of devices. For users of the
integer or sysfs interfaces, this results in GPIO numbers that change,
and drivers and/or user-space programs that behave incorrectly.
Ironically, the only way to have consistent numbers is to use the old
platform files, where you can specify the base number of a gpio_chip.

DT is actually probably not such a bad place to provide consistency in
GPIO numbering. It has a global vision of the system layout, including
all GPIO controllers and the number of GPIOs they include, and thus
can make informed decisions. It provides a consistent result
regardless of probe order. And allowing it to assign GPIO bases to
controllers will free us from the nonsensical dependency of some
arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
since we don't need it anymore after the removal of the global
gpio_descs array. This will again interfere with the numbering of GPIO
chips that do not have a base number provided.

Note that I don't really like this, either - but the problem is the
GPIO integer interface. Until everyone has upgraded to gpiod and we
have a replacement for the current sysfs interface (this will take a
while) we have to cope with this. This issue has been bothering users
for years, so this time I'd like to try and solve it the less ugly
way. If there is a better solution, of course I'm all for it.

2014-12-12 17:17:48

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On 12/12/2014 5:05 AM, Alexandre Courbot wrote:
> On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann <[email protected]> wrote:
>> On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
>>> +
>>> +- linux,gpio-base:
>>> + Base GPIO number of this controller
>>> +
>>>
>>
>> We've NAK'ed properties like this multiple times before, and it
>> doesn't get any better this time. What are you trying to achieve
>> here?
>
> I am to blame for suggesting using this property to Ray, and I am
> fully aware that this has been rejected before, but look at what
> people came with recently to palliate the lack of control over the
> GPIO number space for DT platforms:
>
> http://www.spinics.net/lists/arm-kernel/msg384847.html
> https://lkml.org/lkml/2014/12/10/133
>
> Right now GPIO numbering for platforms using DT is a very inconsistent
> process, subject to change by the simple action of adjusting the value
> of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
> controller, or changing the probe order of devices. For users of the
> integer or sysfs interfaces, this results in GPIO numbers that change,
> and drivers and/or user-space programs that behave incorrectly.
> Ironically, the only way to have consistent numbers is to use the old
> platform files, where you can specify the base number of a gpio_chip.
>
> DT is actually probably not such a bad place to provide consistency in
> GPIO numbering. It has a global vision of the system layout, including
> all GPIO controllers and the number of GPIOs they include, and thus
> can make informed decisions. It provides a consistent result
> regardless of probe order. And allowing it to assign GPIO bases to
> controllers will free us from the nonsensical dependency of some
> arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
> us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
> since we don't need it anymore after the removal of the global
> gpio_descs array. This will again interfere with the numbering of GPIO
> chips that do not have a base number provided.
>
> Note that I don't really like this, either - but the problem is the
> GPIO integer interface. Until everyone has upgraded to gpiod and we
> have a replacement for the current sysfs interface (this will take a
> while) we have to cope with this. This issue has been bothering users
> for years, so this time I'd like to try and solve it the less ugly
> way. If there is a better solution, of course I'm all for it.
>
Agreed.

Since we are just starting to upstream all of our drivers for
iProc/Cygnus, enforcing all of our new drivers to use the gpiod
interface is not an issue and is something that should be done.

Our current issue is really on the sysfs interface, as I mentioned
earlier, a lot of our customers use the sysfs interface for GPIO access.
Until the sysfs interface issue is resolved, we sort of need a way to
maintain the GPIO base between different GPIO controllers.

2014-12-15 17:53:12

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Friday 12 December 2014 22:05:37 Alexandre Courbot wrote:
> On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann <[email protected]> wrote:
> > On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
> >> +
> >> +- linux,gpio-base:
> >> + Base GPIO number of this controller
> >> +
> >>
> >
> > We've NAK'ed properties like this multiple times before, and it
> > doesn't get any better this time. What are you trying to achieve
> > here?
>
> I am to blame for suggesting using this property to Ray, and I am
> fully aware that this has been rejected before, but look at what
> people came with recently to palliate the lack of control over the
> GPIO number space for DT platforms:
>
> http://www.spinics.net/lists/arm-kernel/msg384847.html
> https://lkml.org/lkml/2014/12/10/133
>
> Right now GPIO numbering for platforms using DT is a very inconsistent
> process, subject to change by the simple action of adjusting the value
> of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
> controller, or changing the probe order of devices. For users of the
> integer or sysfs interfaces, this results in GPIO numbers that change,
> and drivers and/or user-space programs that behave incorrectly.
> Ironically, the only way to have consistent numbers is to use the old
> platform files, where you can specify the base number of a gpio_chip.
>
> DT is actually probably not such a bad place to provide consistency in
> GPIO numbering. It has a global vision of the system layout, including
> all GPIO controllers and the number of GPIOs they include, and thus
> can make informed decisions. It provides a consistent result
> regardless of probe order. And allowing it to assign GPIO bases to
> controllers will free us from the nonsensical dependency of some
> arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
> us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
> since we don't need it anymore after the removal of the global
> gpio_descs array. This will again interfere with the numbering of GPIO
> chips that do not have a base number provided.
>
> Note that I don't really like this, either - but the problem is the
> GPIO integer interface. Until everyone has upgraded to gpiod and we
> have a replacement for the current sysfs interface (this will take a
> while) we have to cope with this. This issue has been bothering users
> for years, so this time I'd like to try and solve it the less ugly
> way. If there is a better solution, of course I'm all for it.

I think the scheme will fail if you ever get gpio controllers that are
not part of the DT: We have hotpluggable devices (PCI, USB, ...) that
are not represented in DT and that may also provide GPIOs for internal
uses.

The current state of affairs is definitely problematic, but defining
the GPIO numbers in DT properties would only be a relative improvement,
not a solution, and I fear it would make it harder to change the kernel
to remove the gpio numbers eventually.

I wonder if we could instead come up with an approach that completely
randomizes the gpio numbers (as a compile-time option) to find any
places that still rely on specific numbers.

Arnd

2014-12-15 21:35:52

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On 12/12/2014 7:28 AM, Arnd Bergmann wrote:
> On Friday 12 December 2014 22:05:37 Alexandre Courbot wrote:
>> On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann <[email protected]> wrote:
>>> On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
>>>> +
>>>> +- linux,gpio-base:
>>>> + Base GPIO number of this controller
>>>> +
>>>>
>>>
>>> We've NAK'ed properties like this multiple times before, and it
>>> doesn't get any better this time. What are you trying to achieve
>>> here?
>>
>> I am to blame for suggesting using this property to Ray, and I am
>> fully aware that this has been rejected before, but look at what
>> people came with recently to palliate the lack of control over the
>> GPIO number space for DT platforms:
>>
>> http://www.spinics.net/lists/arm-kernel/msg384847.html
>> https://lkml.org/lkml/2014/12/10/133
>>
>> Right now GPIO numbering for platforms using DT is a very inconsistent
>> process, subject to change by the simple action of adjusting the value
>> of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
>> controller, or changing the probe order of devices. For users of the
>> integer or sysfs interfaces, this results in GPIO numbers that change,
>> and drivers and/or user-space programs that behave incorrectly.
>> Ironically, the only way to have consistent numbers is to use the old
>> platform files, where you can specify the base number of a gpio_chip.
>>
>> DT is actually probably not such a bad place to provide consistency in
>> GPIO numbering. It has a global vision of the system layout, including
>> all GPIO controllers and the number of GPIOs they include, and thus
>> can make informed decisions. It provides a consistent result
>> regardless of probe order. And allowing it to assign GPIO bases to
>> controllers will free us from the nonsensical dependency of some
>> arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
>> us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
>> since we don't need it anymore after the removal of the global
>> gpio_descs array. This will again interfere with the numbering of GPIO
>> chips that do not have a base number provided.
>>
>> Note that I don't really like this, either - but the problem is the
>> GPIO integer interface. Until everyone has upgraded to gpiod and we
>> have a replacement for the current sysfs interface (this will take a
>> while) we have to cope with this. This issue has been bothering users
>> for years, so this time I'd like to try and solve it the less ugly
>> way. If there is a better solution, of course I'm all for it.
>
> I think the scheme will fail if you ever get gpio controllers that are
> not part of the DT: We have hotpluggable devices (PCI, USB, ...) that
> are not represented in DT and that may also provide GPIOs for internal
> uses.
>
> The current state of affairs is definitely problematic, but defining
> the GPIO numbers in DT properties would only be a relative improvement,
> not a solution, and I fear it would make it harder to change the kernel
> to remove the gpio numbers eventually.
>
> I wonder if we could instead come up with an approach that completely
> randomizes the gpio numbers (as a compile-time option) to find any
> places that still rely on specific numbers.
>
> Arnd
>
Okay, if people think defining the GPIO base number in DT properties as
a temporary, transient solution is not acceptable, I can switch the
driver to use dynamic GPIO number allocation (by setting gpio base to a
negative number and let gpiochip_add find a usable base number).

Like I said previously, dynamic GPIO allocation works fine in the
kernel, as long as all of our GPIO clients in the kernel use gpiod based
API, which is what we will enforce going forward. The only problem is
with some of our customers who use GPIO through sysfs and expect fixed
global GPIO numbers. Thinking about this more, it's probably not that
difficult to add a script for those customers to convert/map the GPIO
numbers based on readings parsed from sysfs, so I guess that's fine.

I'll submit v6 patchset with DT property "linux,gpio-base" removed.

2014-12-15 21:58:36

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Monday 15 December 2014 13:35:47 Ray Jui wrote:
>
> Like I said previously, dynamic GPIO allocation works fine in the
> kernel, as long as all of our GPIO clients in the kernel use gpiod based
> API, which is what we will enforce going forward. The only problem is
> with some of our customers who use GPIO through sysfs and expect fixed
> global GPIO numbers. Thinking about this more, it's probably not that
> difficult to add a script for those customers to convert/map the GPIO
> numbers based on readings parsed from sysfs, so I guess that's fine.
>

I think we discussed the user space interface a number of times
in the past, but I forgot the outcome. Either there is already
a way to name gpio lines uniquely in sysfs, or there should be
one.

Can you reach the gpio interfaces using /sys/devices/0001234.bus/1234566.gpiocontroller/...?

Arnd

2014-12-16 00:08:45

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On 12/15/2014 1:57 PM, Arnd Bergmann wrote:
> On Monday 15 December 2014 13:35:47 Ray Jui wrote:
>>
>> Like I said previously, dynamic GPIO allocation works fine in the
>> kernel, as long as all of our GPIO clients in the kernel use gpiod based
>> API, which is what we will enforce going forward. The only problem is
>> with some of our customers who use GPIO through sysfs and expect fixed
>> global GPIO numbers. Thinking about this more, it's probably not that
>> difficult to add a script for those customers to convert/map the GPIO
>> numbers based on readings parsed from sysfs, so I guess that's fine.
>>
>
> I think we discussed the user space interface a number of times
> in the past, but I forgot the outcome. Either there is already
> a way to name gpio lines uniquely in sysfs, or there should be
> one.
>
> Can you reach the gpio interfaces using /sys/devices/0001234.bus/1234566.gpiocontroller/...?
>
> Arnd
>
We use entries under /sys/class/gpio/ to control GPIOs. All base, label,
and ngpio info specific to a GPIO controller can be found there.

2014-12-17 02:45:24

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Sat, Dec 13, 2014 at 12:28 AM, Arnd Bergmann <[email protected]> wrote:
> On Friday 12 December 2014 22:05:37 Alexandre Courbot wrote:
>> On Fri, Dec 12, 2014 at 9:08 PM, Arnd Bergmann <[email protected]> wrote:
>> > On Thursday 11 December 2014 16:05:04 Ray Jui wrote:
>> >> +
>> >> +- linux,gpio-base:
>> >> + Base GPIO number of this controller
>> >> +
>> >>
>> >
>> > We've NAK'ed properties like this multiple times before, and it
>> > doesn't get any better this time. What are you trying to achieve
>> > here?
>>
>> I am to blame for suggesting using this property to Ray, and I am
>> fully aware that this has been rejected before, but look at what
>> people came with recently to palliate the lack of control over the
>> GPIO number space for DT platforms:
>>
>> http://www.spinics.net/lists/arm-kernel/msg384847.html
>> https://lkml.org/lkml/2014/12/10/133
>>
>> Right now GPIO numbering for platforms using DT is a very inconsistent
>> process, subject to change by the simple action of adjusting the value
>> of ARCH_NR_GPIOS (which we did recently, btw), adding a new GPIO
>> controller, or changing the probe order of devices. For users of the
>> integer or sysfs interfaces, this results in GPIO numbers that change,
>> and drivers and/or user-space programs that behave incorrectly.
>> Ironically, the only way to have consistent numbers is to use the old
>> platform files, where you can specify the base number of a gpio_chip.
>>
>> DT is actually probably not such a bad place to provide consistency in
>> GPIO numbering. It has a global vision of the system layout, including
>> all GPIO controllers and the number of GPIOs they include, and thus
>> can make informed decisions. It provides a consistent result
>> regardless of probe order. And allowing it to assign GPIO bases to
>> controllers will free us from the nonsensical dependency of some
>> arbitrary upper-bound for GPIO numbers that ARCH_NR_GPIOS imposes on
>> us. Also about ARCH_NR_GPIOS, the plan is to eventually remove it
>> since we don't need it anymore after the removal of the global
>> gpio_descs array. This will again interfere with the numbering of GPIO
>> chips that do not have a base number provided.
>>
>> Note that I don't really like this, either - but the problem is the
>> GPIO integer interface. Until everyone has upgraded to gpiod and we
>> have a replacement for the current sysfs interface (this will take a
>> while) we have to cope with this. This issue has been bothering users
>> for years, so this time I'd like to try and solve it the less ugly
>> way. If there is a better solution, of course I'm all for it.
>
> I think the scheme will fail if you ever get gpio controllers that are
> not part of the DT: We have hotpluggable devices (PCI, USB, ...) that
> are not represented in DT and that may also provide GPIOs for internal
> uses.
>
> The current state of affairs is definitely problematic, but defining
> the GPIO numbers in DT properties would only be a relative improvement,
> not a solution, and I fear it would make it harder to change the kernel
> to remove the gpio numbers eventually.

You are absolutely right that this would be only a partial solution.
However this is a situation where there is no absolute fix (besides
dropping the GPIO numbers completely) and the relief this property
would brings makes it up for its shortcomings IMHO.

> I wonder if we could instead come up with an approach that completely
> randomizes the gpio numbers (as a compile-time option) to find any
> places that still rely on specific numbers.

A.k.a. Linus and Alex' hate mail generator. :P

Actually we are not that far from being able to do completely without
any GPIO number, and maybe that's what we should aim for. I think the
only remaining offender is the sysfs interface. If we could reach GPIO
controllers through a fixed path and just export their GPIOs there, I
believe we would have fixed the whole issue.

2014-12-17 02:53:21

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Tue, Dec 16, 2014 at 6:57 AM, Arnd Bergmann <[email protected]> wrote:
> On Monday 15 December 2014 13:35:47 Ray Jui wrote:
>>
>> Like I said previously, dynamic GPIO allocation works fine in the
>> kernel, as long as all of our GPIO clients in the kernel use gpiod based
>> API, which is what we will enforce going forward. The only problem is
>> with some of our customers who use GPIO through sysfs and expect fixed
>> global GPIO numbers. Thinking about this more, it's probably not that
>> difficult to add a script for those customers to convert/map the GPIO
>> numbers based on readings parsed from sysfs, so I guess that's fine.
>>
>
> I think we discussed the user space interface a number of times
> in the past, but I forgot the outcome. Either there is already
> a way to name gpio lines uniquely in sysfs, or there should be
> one.
>
> Can you reach the gpio interfaces using /sys/devices/0001234.bus/1234566.gpiocontroller/...?

No, but it seems like this is exactly the solution we need. We could
have an "export" node there that takes a relative GPIO number and
exports it under
/sys/devices/0001234.bus/1234566.gpiocontroller/exported/ the same way
the current sysfs exporter does. Then for convenience we could also
allow exported GPIOs to take names to be used under the shorter
/sys/class/gpio/ (named GPIOs is another request we pushed back many
times but that keeps coming).

Let's see if I can come with a patch. That would at least give us
something to reply to the many people that hit this issue.

2014-12-17 10:27:42

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Wednesday 17 December 2014 11:45:01 Alexandre Courbot wrote:
>
> Actually we are not that far from being able to do completely without
> any GPIO number, and maybe that's what we should aim for. I think the
> only remaining offender is the sysfs interface. If we could reach GPIO
> controllers through a fixed path and just export their GPIOs there, I
> believe we would have fixed the whole issue.

What about the hundreds of board files and device drivers that still
reference hardcoded gpio numbers? The problem seems mostly solved for
anything that uses DT, but there are some architectures and a number
of ARM platforms that don't use DT and probably never will.

I would assume they could all be changed to use gpiod_lookup tables,
but that's a lot of work.

Arnd

2014-12-17 10:44:49

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Wed, Dec 17, 2014 at 11:45:01AM +0900, Alexandre Courbot wrote:
> Actually we are not that far from being able to do completely without
> any GPIO number, and maybe that's what we should aim for. I think the
> only remaining offender is the sysfs interface.

And that is a user API, and there's lots of users of it (eg, on Raspberry
Pi platforms.) So changing it isn't going to be easy - I'd say that it's
impractical.

What you're suggesting would be like re-numbering Linux syscalls.

--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

2014-12-17 13:13:49

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] gpio: Cygnus: define Broadcom Cygnus GPIO binding

On Wed, Dec 17, 2014 at 7:44 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Wed, Dec 17, 2014 at 11:45:01AM +0900, Alexandre Courbot wrote:
>> Actually we are not that far from being able to do completely without
>> any GPIO number, and maybe that's what we should aim for. I think the
>> only remaining offender is the sysfs interface.
>
> And that is a user API, and there's lots of users of it (eg, on Raspberry
> Pi platforms.) So changing it isn't going to be easy - I'd say that it's
> impractical.
>
> What you're suggesting would be like re-numbering Linux syscalls.

Uh, I expressed myself poorly. What I intended to say is that once we
have a sysfs alternative that does not rely on GPIO numbers (and thus
have the same feature coverage as the integer interface), we can
require new platforms to exclusively rely on gpiod/sysfs2, and
encourage older users to switch to it if they have an issue with the
way integers are handled or need one of the new features.

I don't foresee that we will ever be able to retire the integer
interface, however I would like to be able to say "your problem will
be solved if you switch to gpiod" instead of having to juggle with
potentially conflicting integer range requirements from different
platforms. Right now the only thing that prevents us to say that is
the lack of a consistent sysfs interface.

2014-12-17 13:16:32