From: Harini Katakam <[email protected]>
Add support for GPIO controller used by Xilinx Zynq.
Signed-off-by: Harini Katakam <[email protected]>
Signed-off-by: Soren Brinkmann <[email protected]>
---
v2 changes:
- convert to pm_runtime_force_(suspend|resume)
- add pm_runtime_set_active in probe()
- also (un)prepare clocks when they are dis-/enabled
- add some missing calls to pm_runtime_get()
- use pm_runtime_put() instead of sync variant
- remove gpio chip in driver remove()
- remove redundant type casts
- directly use IO helpers
- use BIT macro to set/clear bits
- migrate to GPIOLIB_IRQCHIP
---
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-zynq.c | 651 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 659 insertions(+)
create mode 100644 drivers/gpio/gpio-zynq.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 4a1b511..bdeef02 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -340,6 +340,13 @@ config GPIO_XILINX
help
Say yes here to support the Xilinx FPGA GPIO device
+config GPIO_ZYNQ
+ tristate "Xilinx Zynq GPIO support"
+ depends on ARCH_ZYNQ
+ select GPIOLIB_IRQCHIP
+ help
+ Say yes here to support Xilinx Zynq GPIO controller.
+
config GPIO_XTENSA
bool "Xtensa GPIO32 support"
depends on XTENSA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d10f6a9..033fd7c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -101,3 +101,4 @@ obj-$(CONFIG_GPIO_WM8994) += gpio-wm8994.o
obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o
obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o
obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
+obj-$(CONFIG_GPIO_ZYNQ) += gpio-zynq.o
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
new file mode 100644
index 0000000..3af13b6
--- /dev/null
+++ b/drivers/gpio/gpio-zynq.c
@@ -0,0 +1,651 @@
+/*
+ * Xilinx Zynq GPIO device driver
+ *
+ * Copyright (C) 2009 - 2014 Xilinx, Inc.
+ *
+ * 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; either version 2 of the License, or (at your option) any later
+ * version.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#define DRIVER_NAME "zynq-gpio"
+
+/* Maximum banks */
+#define ZYNQ_GPIO_MAX_BANK 4
+
+#define ZYNQ_GPIO_BANK0_NGPIO 32
+#define ZYNQ_GPIO_BANK1_NGPIO 22
+#define ZYNQ_GPIO_BANK2_NGPIO 32
+#define ZYNQ_GPIO_BANK3_NGPIO 32
+
+#define ZYNQ_GPIO_NR_GPIOS (ZYNQ_GPIO_BANK0_NGPIO + \
+ ZYNQ_GPIO_BANK1_NGPIO + \
+ ZYNQ_GPIO_BANK2_NGPIO + \
+ ZYNQ_GPIO_BANK3_NGPIO)
+
+#define ZYNQ_GPIO_BANK0_PIN_MIN 0
+#define ZYNQ_GPIO_BANK0_PIN_MAX (ZYNQ_GPIO_BANK0_PIN_MIN + \
+ ZYNQ_GPIO_BANK0_NGPIO - 1)
+#define ZYNQ_GPIO_BANK1_PIN_MIN (ZYNQ_GPIO_BANK0_PIN_MAX + 1)
+#define ZYNQ_GPIO_BANK1_PIN_MAX (ZYNQ_GPIO_BANK1_PIN_MIN + \
+ ZYNQ_GPIO_BANK1_NGPIO - 1)
+#define ZYNQ_GPIO_BANK2_PIN_MIN (ZYNQ_GPIO_BANK1_PIN_MAX + 1)
+#define ZYNQ_GPIO_BANK2_PIN_MAX (ZYNQ_GPIO_BANK2_PIN_MIN + \
+ ZYNQ_GPIO_BANK2_NGPIO - 1)
+#define ZYNQ_GPIO_BANK3_PIN_MIN (ZYNQ_GPIO_BANK2_PIN_MAX + 1)
+#define ZYNQ_GPIO_BANK3_PIN_MAX (ZYNQ_GPIO_BANK3_PIN_MIN + \
+ ZYNQ_GPIO_BANK3_NGPIO - 1)
+
+
+/* Register offsets for the GPIO device */
+/* LSW Mask & Data -WO */
+#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK) (0x000 + (8 * BANK))
+/* MSW Mask & Data -WO */
+#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK) (0x004 + (8 * BANK))
+/* Data Register-RW */
+#define ZYNQ_GPIO_DATA_RO_OFFSET(BANK) (0x060 + (4 * BANK))
+/* Direction mode reg-RW */
+#define ZYNQ_GPIO_DIRM_OFFSET(BANK) (0x204 + (0x40 * BANK))
+/* Output enable reg-RW */
+#define ZYNQ_GPIO_OUTEN_OFFSET(BANK) (0x208 + (0x40 * BANK))
+/* Interrupt mask reg-RO */
+#define ZYNQ_GPIO_INTMASK_OFFSET(BANK) (0x20C + (0x40 * BANK))
+/* Interrupt enable reg-WO */
+#define ZYNQ_GPIO_INTEN_OFFSET(BANK) (0x210 + (0x40 * BANK))
+/* Interrupt disable reg-WO */
+#define ZYNQ_GPIO_INTDIS_OFFSET(BANK) (0x214 + (0x40 * BANK))
+/* Interrupt status reg-RO */
+#define ZYNQ_GPIO_INTSTS_OFFSET(BANK) (0x218 + (0x40 * BANK))
+/* Interrupt type reg-RW */
+#define ZYNQ_GPIO_INTTYPE_OFFSET(BANK) (0x21C + (0x40 * BANK))
+/* Interrupt polarity reg-RW */
+#define ZYNQ_GPIO_INTPOL_OFFSET(BANK) (0x220 + (0x40 * BANK))
+/* Interrupt on any, reg-RW */
+#define ZYNQ_GPIO_INTANY_OFFSET(BANK) (0x224 + (0x40 * BANK))
+
+/* Disable all interrupts mask */
+#define ZYNQ_GPIO_IXR_DISABLE_ALL 0xFFFFFFFF
+
+/* Mid pin number of a bank */
+#define ZYNQ_GPIO_MID_PIN_NUM 16
+
+/* GPIO upper 16 bit mask */
+#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
+
+/**
+ * struct zynq_gpio - gpio device private data structure
+ * @chip: instance of the gpio_chip
+ * @base_addr: base address of the GPIO device
+ * @irq: irq associated with the controller
+ * @clk: clock resource for this controller
+ */
+struct zynq_gpio {
+ struct gpio_chip chip;
+ void __iomem *base_addr;
+ int irq;
+ struct clk *clk;
+};
+
+/**
+ * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
+ * for a given pin in the GPIO device
+ * @pin_num: gpio pin number within the device
+ * @bank_num: an output parameter used to return the bank number of the gpio
+ * pin
+ * @bank_pin_num: an output parameter used to return pin number within a bank
+ * for the given gpio pin
+ *
+ * Returns the bank number and pin offset within the bank.
+ */
+static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
+ unsigned int *bank_num,
+ unsigned int *bank_pin_num)
+{
+ switch (pin_num) {
+ case ZYNQ_GPIO_BANK0_PIN_MIN ... ZYNQ_GPIO_BANK0_PIN_MAX:
+ *bank_num = 0;
+ *bank_pin_num = pin_num;
+ break;
+ case ZYNQ_GPIO_BANK1_PIN_MIN ... ZYNQ_GPIO_BANK1_PIN_MAX:
+ *bank_num = 1;
+ *bank_pin_num = pin_num - ZYNQ_GPIO_BANK1_PIN_MIN;
+ break;
+ case ZYNQ_GPIO_BANK2_PIN_MIN ... ZYNQ_GPIO_BANK2_PIN_MAX:
+ *bank_num = 2;
+ *bank_pin_num = pin_num - ZYNQ_GPIO_BANK2_PIN_MIN;
+ break;
+ case ZYNQ_GPIO_BANK3_PIN_MIN ... ZYNQ_GPIO_BANK3_PIN_MAX:
+ *bank_num = 3;
+ *bank_pin_num = pin_num - ZYNQ_GPIO_BANK3_PIN_MIN;
+ break;
+ default:
+ WARN(true, "invalid GPIO pin number: %u", pin_num);
+ *bank_num = 0;
+ *bank_pin_num = 0;
+ break;
+ }
+}
+
+/**
+ * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ *
+ * This function reads the state of the specified pin of the GPIO device.
+ *
+ * Return: 0 if the pin is low, 1 if pin is high.
+ */
+static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
+{
+ u32 data;
+ unsigned int bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+ zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+ data = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
+
+ return (data >> bank_pin_num) & 1;
+}
+
+/**
+ * zynq_gpio_set_value - Modify the state of the pin with specified value
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ * @state: value used to modify the state of the specified pin
+ *
+ * This function calculates the register offset (i.e to lower 16 bits or
+ * upper 16 bits) based on the given pin number and sets the state of a
+ * gpio pin to the specified value. The state is either 0 or non-zero.
+ */
+static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
+ int state)
+{
+ unsigned int reg_offset, bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+ zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+ if (bank_pin_num >= ZYNQ_GPIO_MID_PIN_NUM) {
+ /* only 16 data bits in bit maskable reg */
+ bank_pin_num -= ZYNQ_GPIO_MID_PIN_NUM;
+ reg_offset = ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num);
+ } else {
+ reg_offset = ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num);
+ }
+
+ /*
+ * get the 32 bit value to be written to the mask/data register where
+ * the upper 16 bits is the mask and lower 16 bits is the data
+ */
+ state = !!state;
+ state = ~(1 << (bank_pin_num + ZYNQ_GPIO_MID_PIN_NUM)) &
+ ((state << bank_pin_num) | ZYNQ_GPIO_UPPER_MASK);
+
+ writel_relaxed(state, gpio->base_addr + reg_offset);
+}
+
+/**
+ * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ *
+ * This function uses the read-modify-write sequence to set the direction of
+ * the gpio pin as input.
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
+{
+ u32 reg;
+ unsigned int bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+ zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+ /* bank 0 pins 7 and 8 are special and cannot be used as inputs */
+ if (bank_num == 0 && (bank_pin_num == 7 || bank_pin_num == 8))
+ return -EINVAL;
+
+ /* clear the bit in direction mode reg to set the pin as input */
+ reg = readl_relaxed(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+ reg &= ~BIT(bank_pin_num);
+ writel_relaxed(reg, gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+
+ return 0;
+}
+
+/**
+ * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ * @state: value to be written to specified pin
+ *
+ * This function sets the direction of specified GPIO pin as output, configures
+ * the Output Enable register for the pin and uses zynq_gpio_set to set
+ * the state of the pin to the value specified.
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
+ int state)
+{
+ u32 reg;
+ unsigned int bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+ zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+ /* set the GPIO pin as output */
+ reg = readl_relaxed(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+ reg |= BIT(bank_pin_num);
+ writel_relaxed(reg, gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+
+ /* configure the output enable reg for the pin */
+ reg = readl_relaxed(gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
+ reg |= BIT(bank_pin_num);
+ writel_relaxed(reg, gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
+
+ /* set the state of the pin */
+ zynq_gpio_set_value(chip, pin, state);
+ return 0;
+}
+
+/**
+ * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
+ * @irq_data: per irq and chip data passed down to chip functions
+ *
+ * This function calculates gpio pin number from irq number and sets the
+ * bit in the Interrupt Disable register of the corresponding bank to disable
+ * interrupts for that pin.
+ */
+static void zynq_gpio_irq_mask(struct irq_data *irq_data)
+{
+ unsigned int device_pin_num, bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+
+ device_pin_num = irq_data->hwirq;
+ zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+ writel_relaxed(BIT(bank_pin_num),
+ gpio->base_addr + ZYNQ_GPIO_INTDIS_OFFSET(bank_num));
+}
+
+/**
+ * zynq_gpio_irq_unmask - Enable the interrupts for a gpio pin
+ * @irq_data: irq data containing irq number of gpio pin for the interrupt
+ * to enable
+ *
+ * This function calculates the gpio pin number from irq number and sets the
+ * bit in the Interrupt Enable register of the corresponding bank to enable
+ * interrupts for that pin.
+ */
+static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
+{
+ unsigned int device_pin_num, bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+
+ device_pin_num = irq_data->hwirq;
+ zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+ writel_relaxed(BIT(bank_pin_num),
+ gpio->base_addr + ZYNQ_GPIO_INTEN_OFFSET(bank_num));
+}
+
+/**
+ * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
+ * @irq_data: irq data containing irq number of gpio pin
+ * @type: interrupt type that is to be set for the gpio pin
+ *
+ * This function gets the gpio pin number and its bank from the gpio pin number
+ * and configures the INT_TYPE, INT_POLARITY and INT_ANY registers.
+ *
+ * Return: 0, negative error otherwise.
+ * TYPE-EDGE_RISING, INT_TYPE - 1, INT_POLARITY - 1, INT_ANY - 0;
+ * TYPE-EDGE_FALLING, INT_TYPE - 1, INT_POLARITY - 0, INT_ANY - 0;
+ * TYPE-EDGE_BOTH, INT_TYPE - 1, INT_POLARITY - NA, INT_ANY - 1;
+ * TYPE-LEVEL_HIGH, INT_TYPE - 0, INT_POLARITY - 1, INT_ANY - NA;
+ * TYPE-LEVEL_LOW, INT_TYPE - 0, INT_POLARITY - 0, INT_ANY - NA
+ */
+static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
+{
+ u32 int_type, int_pol, int_any;
+ unsigned int device_pin_num, bank_num, bank_pin_num;
+ struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+
+ device_pin_num = irq_data->hwirq;
+ zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+
+ int_type = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
+ int_pol = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
+ int_any = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+
+ /*
+ * based on the type requested, configure the INT_TYPE, INT_POLARITY
+ * and INT_ANY registers
+ */
+ switch (type) {
+ case IRQ_TYPE_EDGE_RISING:
+ int_type |= BIT(bank_pin_num);
+ int_pol |= BIT(bank_pin_num);
+ int_any &= ~BIT(bank_pin_num);
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ int_type |= BIT(bank_pin_num);
+ int_pol &= ~BIT(bank_pin_num);
+ int_any &= ~BIT(bank_pin_num);
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ int_type |= BIT(bank_pin_num);
+ int_any |= BIT(bank_pin_num);
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ int_type &= ~BIT(bank_pin_num);
+ int_pol |= BIT(bank_pin_num);
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ int_type &= ~BIT(bank_pin_num);
+ int_pol &= ~BIT(bank_pin_num);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ writel_relaxed(int_type,
+ gpio->base_addr + ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
+ writel_relaxed(int_pol,
+ gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
+ writel_relaxed(int_any,
+ gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+ return 0;
+}
+
+static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
+{
+ if (on)
+ zynq_gpio_irq_unmask(data);
+ else
+ zynq_gpio_irq_mask(data);
+
+ return 0;
+}
+
+/* irq chip descriptor */
+static struct irq_chip zynq_gpio_irqchip = {
+ .name = DRIVER_NAME,
+ .irq_mask = zynq_gpio_irq_mask,
+ .irq_unmask = zynq_gpio_irq_unmask,
+ .irq_set_type = zynq_gpio_set_irq_type,
+ .irq_set_wake = zynq_gpio_set_wake,
+};
+
+/**
+ * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
+ * @irq: irq number of the gpio bank where interrupt has occurred
+ * @desc: irq descriptor instance of the 'irq'
+ *
+ * This function reads the Interrupt Status Register of each bank to get the
+ * gpio pin number which has triggered an interrupt. It then acks the triggered
+ * interrupt and calls the pin specific handler set by the higher layer
+ * application for that pin.
+ * Note: A bug is reported if no handler is set for the gpio pin.
+ */
+static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
+{
+ u32 int_sts, int_enb;
+ unsigned int bank_num;
+ struct zynq_gpio *gpio = irq_get_handler_data(irq);
+ struct irq_chip *irqchip = irq_desc_get_chip(desc);
+
+ chained_irq_enter(irqchip, desc);
+
+ for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
+ int_sts = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
+ int_enb = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
+ int_sts &= ~int_enb;
+ if (int_sts) {
+ int offset;
+ unsigned long pending = int_sts;
+
+ for_each_set_bit(offset, &pending, 32) {
+ unsigned int gpio_irq =
+ irq_find_mapping(gpio->chip.irqdomain,
+ offset);
+ generic_handle_irq(gpio_irq);
+ }
+
+ /* clear IRQ in HW */
+ writel_relaxed(int_sts, gpio->base_addr +
+ ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
+ }
+ }
+
+ chained_irq_exit(irqchip, desc);
+}
+
+static int __maybe_unused zynq_gpio_suspend(struct device *dev)
+{
+ if (!device_may_wakeup(dev))
+ return pm_runtime_force_suspend(dev);
+
+ return 0;
+}
+
+static int __maybe_unused zynq_gpio_resume(struct device *dev)
+{
+ if (!device_may_wakeup(dev))
+ return pm_runtime_force_resume(dev);
+
+ return 0;
+}
+
+static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(gpio->clk);
+
+ return 0;
+}
+
+static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+ return clk_prepare_enable(gpio->clk);
+}
+
+static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+ int ret;
+
+ ret = pm_runtime_get_sync(chip->dev);
+
+ /*
+ * If the device is already active pm_runtime_get() will return 1 on
+ * success, but gpio_request still needs to return 0.
+ */
+ return ret < 0 ? ret : 0;
+}
+
+static void zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+ pm_runtime_put(chip->dev);
+}
+
+static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
+ SET_PM_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend,
+ zynq_gpio_runtime_resume, NULL)
+};
+
+/**
+ * zynq_gpio_probe - Initialization method for a zynq_gpio device
+ * @pdev: platform device instance
+ *
+ * This function allocates memory resources for the gpio device and registers
+ * all the banks of the device. It will also set up interrupts for the gpio
+ * pins.
+ * Note: Interrupts are disabled for all the banks during initialization.
+ *
+ * Return: 0 on success, negative error otherwise.
+ */
+static int zynq_gpio_probe(struct platform_device *pdev)
+{
+ int ret, bank_num;
+ struct zynq_gpio *gpio;
+ struct gpio_chip *chip;
+ struct resource *res;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, gpio);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(gpio->base_addr))
+ return PTR_ERR(gpio->base_addr);
+
+ gpio->irq = platform_get_irq(pdev, 0);
+ if (gpio->irq < 0) {
+ dev_err(&pdev->dev, "invalid IRQ\n");
+ return gpio->irq;
+ }
+
+ /* configure the gpio chip */
+ chip = &gpio->chip;
+ chip->label = "zynq_gpio";
+ chip->owner = THIS_MODULE;
+ chip->dev = &pdev->dev;
+ chip->get = zynq_gpio_get_value;
+ chip->set = zynq_gpio_set_value;
+ chip->request = zynq_gpio_request;
+ chip->free = zynq_gpio_free;
+ chip->direction_input = zynq_gpio_dir_in;
+ chip->direction_output = zynq_gpio_dir_out;
+ chip->base = -1;
+ chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
+
+ /* Enable GPIO clock */
+ gpio->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->clk)) {
+ dev_err(&pdev->dev, "input clock not found.\n");
+ return PTR_ERR(gpio->clk);
+ }
+ ret = clk_prepare_enable(gpio->clk);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to enable clock.\n");
+ return ret;
+ }
+
+ /* report a bug if gpio chip registration fails */
+ ret = gpiochip_add(chip);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to add gpio chip\n");
+ goto err_disable_clk;
+ }
+
+ /* disable interrupts for all banks */
+ for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++)
+ writel_relaxed(ZYNQ_GPIO_IXR_DISABLE_ALL, gpio->base_addr +
+ ZYNQ_GPIO_INTDIS_OFFSET(bank_num));
+
+ ret = gpiochip_irqchip_add(chip, &zynq_gpio_irqchip, 0,
+ handle_simple_irq, IRQ_TYPE_NONE);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to add irq chip\n");
+ goto err_rm_gpiochip;
+ }
+
+ gpiochip_set_chained_irqchip(chip, &zynq_gpio_irqchip, gpio->irq,
+ zynq_gpio_irqhandler);
+
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+
+ device_set_wakeup_capable(&pdev->dev, 1);
+
+ return 0;
+
+err_rm_gpiochip:
+ if (gpiochip_remove(chip))
+ dev_err(&pdev->dev, "Failed to remove gpio chip\n");
+err_disable_clk:
+ clk_disable_unprepare(gpio->clk);
+
+ return ret;
+}
+
+/**
+ * zynq_gpio_remove - Driver removal function
+ * @pdev: platform device instance
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_remove(struct platform_device *pdev)
+{
+ int ret;
+ struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+ pm_runtime_get_sync(&pdev->dev);
+
+ ret = gpiochip_remove(&gpio->chip);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to remove gpio chip\n");
+ return ret;
+ }
+ clk_disable_unprepare(gpio->clk);
+ device_set_wakeup_capable(&pdev->dev, 0);
+ return 0;
+}
+
+static struct of_device_id zynq_gpio_of_match[] = {
+ { .compatible = "xlnx,zynq-gpio-1.0", },
+ { /* end of table */ }
+};
+MODULE_DEVICE_TABLE(of, zynq_gpio_of_match);
+
+static struct platform_driver zynq_gpio_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .pm = &zynq_gpio_dev_pm_ops,
+ .of_match_table = zynq_gpio_of_match,
+ },
+ .probe = zynq_gpio_probe,
+ .remove = zynq_gpio_remove,
+};
+
+/**
+ * zynq_gpio_init - Initial driver registration call
+ *
+ * Return: value from platform_driver_register
+ */
+static int __init zynq_gpio_init(void)
+{
+ return platform_driver_register(&zynq_gpio_driver);
+}
+postcore_initcall(zynq_gpio_init);
+
+MODULE_AUTHOR("Xilinx Inc.");
+MODULE_DESCRIPTION("Zynq GPIO driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5
From: Harini Katakam <[email protected]>
Add gpio-zynq bindings documentation.
Signed-off-by: Harini Katakam <[email protected]>
Signed-off-by: Soren Brinkmann <[email protected]>
---
v2 changes:
Improve description.
---
.../devicetree/bindings/gpio/gpio-zynq.txt | 24 ++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-zynq.txt
diff --git a/Documentation/devicetree/bindings/gpio/gpio-zynq.txt b/Documentation/devicetree/bindings/gpio/gpio-zynq.txt
new file mode 100644
index 0000000..b2c023a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-zynq.txt
@@ -0,0 +1,24 @@
+Xilinx Zynq GPIO controller Device Tree Bindings
+-------------------------------------------
+
+Required properties:
+- #gpio-cells : Should be two. First cell is used to mention
+ pin number.
+- compatible : Should be "xlnx,zynq-gpio-1.0"
+- clocks : Clock specifier (see clock bindings for details)
+- gpio-controller : Marks the device node as a GPIO controller.
+- interrupts : Interrupt specifier (see interrupt bindings for
+ details)
+- interrupt-parent : Must be core interrupt controller
+- reg : Address and length of the register set for the device
+
+Example:
+ gpio@e000a000 {
+ #gpio-cells = <2>;
+ compatible = "xlnx,zynq-gpio-1.0";
+ clocks = <&clkc 42>;
+ gpio-controller;
+ interrupt-parent = <&intc>;
+ interrupts = <0 20 4>;
+ reg = <0xe000a000 0x1000>;
+ };
--
1.7.9.5
Hi Linus,
I did some of the changes for this v2 and a few things are not clear to
me.
The first is, how is userspace supposed to find the correct offset for a
GPIO pin. E.g. let's say GPIO 10 of this SOC-internal GPIO controller is
something I want to control. So, I'd export GPIO (chip-base + 10). But
this chip-base seems pretty variable. v1 of this patch had it hardcoded
to 0, which resulted in a predictable offset, but apparently was utterly
wrong. Now I see an offset of 138 for this chip when using my config.
And when I use multi_v7_defconfig the offset is somewhere in the 800s,
IIRC. The best I found was the 'label' in the gpiochip's sysfs entry,
but documentation says that is not necessarily unique, and parsing labes
seems sub-optimal too.
The second issue is a stack trace (below) I see when triggering
interrupts (e.g. echo rising > edge; and then pushing the connected
button). Looking at the stack trace, I don't see an obvious error (I
think I pretty much copied the IRQ registration from the gpio-pl061.c
driver you mentioned). Is this an issue in this driver or the core code?
This happens on Linus' latest tip. Despite all this chatter the system
still works and doesn't not seem to lock up.
Here the stack trace:
# echo rising > edge
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:586
in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/0
no locks held by swapper/0/0.
irq event stamp: 55488
hardirqs last enabled at (55485): [<c03bf10c>] cpuidle_enter_state+0x60/0xec
hardirqs last disabled at (55486): [<c0014374>] __irq_svc+0x34/0x78
softirqs last enabled at (55488): [<c002fc40>] _local_bh_enable+0x58/0x64
softirqs last disabled at (55487): [<c0030534>] irq_enter+0x48/0x88
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.16.0-rc1-xilinx-00021-gf5ddad957172 #88
[<c0017840>] (unwind_backtrace) from [<c0013770>] (show_stack+0x20/0x24)
[<c0013770>] (show_stack) from [<c04d94a0>] (dump_stack+0x8c/0xd0)
[<c04d94a0>] (dump_stack) from [<c005d5e4>] (__might_sleep+0x1ac/0x1e4)
[<c005d5e4>] (__might_sleep) from [<c04dc19c>] (mutex_lock_nested+0x40/0x46c)
[<c04dc19c>] (mutex_lock_nested) from [<c019df34>] (kernfs_notify+0xac/0x148)
[<c019df34>] (kernfs_notify) from [<c02d89e0>] (gpio_sysfs_irq+0x1c/0x24)
[<c02d89e0>] (gpio_sysfs_irq) from [<c008588c>] (handle_irq_event_percpu+0xa8/0x3c0)
[<c008588c>] (handle_irq_event_percpu) from [<c0085bf0>] (handle_irq_event+0x4c/0x6c)
[<c0085bf0>] (handle_irq_event) from [<c0088a28>] (handle_simple_irq+0xac/0xbc)
[<c0088a28>] (handle_simple_irq) from [<c0084fec>] (generic_handle_irq+0x30/0x40)
[<c0084fec>] (generic_handle_irq) from [<c02de3fc>] (zynq_gpio_irqhandler+0xe8/0x130)
[<c02de3fc>] (zynq_gpio_irqhandler) from [<c0084fec>] (generic_handle_irq+0x30/0x40)
[<c0084fec>] (generic_handle_irq) from [<c000fd24>] (handle_IRQ+0x78/0xa0)
[<c000fd24>] (handle_IRQ) from [<c000868c>] (gic_handle_irq+0x4c/0x70)
[<c000868c>] (gic_handle_irq) from [<c0014384>] (__irq_svc+0x44/0x78)
Exception stack(0xc0755ec8 to 0xc0755f10)
5ec0: 00000001 00000004 00000000 c075fb70 00000001 edfc9310
5ee0: 8ed05707 0000001d 9e7f7ffa 0000001d c0752308 c0755f44 c0755ee0 c0755f10
5f00: c0077340 c03bf110 200d0013 ffffffff
[<c0014384>] (__irq_svc) from [<c03bf110>] (cpuidle_enter_state+0x64/0xec)
[<c03bf110>] (cpuidle_enter_state) from [<c03bf2a0>] (cpuidle_enter+0x24/0x28)
[<c03bf2a0>] (cpuidle_enter) from [<c0071d94>] (cpu_idle_loop+0x2e0/0x6fc)
[<c0071d94>] (cpu_idle_loop) from [<c00721cc>] (cpupri_find+0x0/0x108)
=================================
[ INFO: inconsistent lock state ]
3.16.0-rc1-xilinx-00021-gf5ddad957172 #88 Not tainted
---------------------------------
inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
(kernfs_mutex){?.+.+.}, at: [<c019df34>] kernfs_notify+0xac/0x148
{HARDIRQ-ON-W} state was registered at:
[<c0079a08>] lock_acquire+0xfc/0x21c
[<c04dc1e0>] mutex_lock_nested+0x84/0x46c
[<c019d668>] kernfs_activate+0x2c/0xf8
[<c019d9b8>] kernfs_create_root+0xbc/0xe0
[<c070edac>] sysfs_init+0x20/0x5c
[<c070cef4>] mnt_init+0x108/0x258
[<c070caa8>] vfs_caches_init+0x9c/0x110
[<c06f5c3c>] start_kernel+0x364/0x3fc
[<00008074>] 0x8074
irq event stamp: 55488
hardirqs last enabled at (55485): [<c03bf10c>] cpuidle_enter_state+0x60/0xec
hardirqs last disabled at (55486): [<c0014374>] __irq_svc+0x34/0x78
softirqs last enabled at (55488): [<c002fc40>] _local_bh_enable+0x58/0x64
softirqs last disabled at (55487): [<c0030534>] irq_enter+0x48/0x88
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(kernfs_mutex);
<Interrupt>
lock(kernfs_mutex);
*** DEADLOCK ***
no locks held by swapper/0/0.
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.16.0-rc1-xilinx-00021-gf5ddad957172 #88
[<c0017840>] (unwind_backtrace) from [<c0013770>] (show_stack+0x20/0x24)
[<c0013770>] (show_stack) from [<c04d94a0>] (dump_stack+0x8c/0xd0)
[<c04d94a0>] (dump_stack) from [<c0076a50>] (print_usage_bug+0x254/0x2c4)
[<c0076a50>] (print_usage_bug) from [<c0076e6c>] (mark_lock+0x3ac/0x674)
[<c0076e6c>] (mark_lock) from [<c0078024>] (__lock_acquire+0x934/0x1b58)
[<c0078024>] (__lock_acquire) from [<c0079a08>] (lock_acquire+0xfc/0x21c)
[<c0079a08>] (lock_acquire) from [<c04dc1e0>] (mutex_lock_nested+0x84/0x46c)
[<c04dc1e0>] (mutex_lock_nested) from [<c019df34>] (kernfs_notify+0xac/0x148)
[<c019df34>] (kernfs_notify) from [<c02d89e0>] (gpio_sysfs_irq+0x1c/0x24)
[<c02d89e0>] (gpio_sysfs_irq) from [<c008588c>] (handle_irq_event_percpu+0xa8/0x3c0)
[<c008588c>] (handle_irq_event_percpu) from [<c0085bf0>] (handle_irq_event+0x4c/0x6c)
[<c0085bf0>] (handle_irq_event) from [<c0088a28>] (handle_simple_irq+0xac/0xbc)
[<c0088a28>] (handle_simple_irq) from [<c0084fec>] (generic_handle_irq+0x30/0x40)
[<c0084fec>] (generic_handle_irq) from [<c02de3fc>] (zynq_gpio_irqhandler+0xe8/0x130)
[<c02de3fc>] (zynq_gpio_irqhandler) from [<c0084fec>] (generic_handle_irq+0x30/0x40)
[<c0084fec>] (generic_handle_irq) from [<c000fd24>] (handle_IRQ+0x78/0xa0)
[<c000fd24>] (handle_IRQ) from [<c000868c>] (gic_handle_irq+0x4c/0x70)
[<c000868c>] (gic_handle_irq) from [<c0014384>] (__irq_svc+0x44/0x78)
Exception stack(0xc0755ec8 to 0xc0755f10)
5ec0: 00000001 00000004 00000000 c075fb70 00000001 edfc9310
5ee0: 8ed05707 0000001d 9e7f7ffa 0000001d c0752308 c0755f44 c0755ee0 c0755f10
5f00: c0077340 c03bf110 200d0013 ffffffff
[<c0014384>] (__irq_svc) from [<c03bf110>] (cpuidle_enter_state+0x64/0xec)
[<c03bf110>] (cpuidle_enter_state) from [<c03bf2a0>] (cpuidle_enter+0x24/0x28)
[<c03bf2a0>] (cpuidle_enter) from [<c0071d94>] (cpu_idle_loop+0x2e0/0x6fc)
[<c0071d94>] (cpu_idle_loop) from [<c00721cc>] (cpupri_find+0x0/0x108)
Thanks,
Sören
On Wed, Jun 18, 2014 at 1:39 PM, Harini Katakam <[email protected]> wrote:
> From: Harini Katakam <[email protected]>
>
> Add support for GPIO controller used by Xilinx Zynq.
>
> Signed-off-by: Harini Katakam <[email protected]>
> Signed-off-by: Soren Brinkmann <[email protected]>
> ---
>
> v2 changes:
> - convert to pm_runtime_force_(suspend|resume)
> - add pm_runtime_set_active in probe()
> - also (un)prepare clocks when they are dis-/enabled
> - add some missing calls to pm_runtime_get()
> - use pm_runtime_put() instead of sync variant
> - remove gpio chip in driver remove()
> - remove redundant type casts
> - directly use IO helpers
> - use BIT macro to set/clear bits
> - migrate to GPIOLIB_IRQCHIP
This is a great improvement! Only small stuff remains.
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
This should be:
#include <linux/gpio/driver.h>
If that doesn't work ... why?
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip: instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq: irq associated with the controller
> + * @clk: clock resource for this controller
> + */
> +struct zynq_gpio {
> + struct gpio_chip chip;
> + void __iomem *base_addr;
> + int irq;
Why is irq kept around in this struct? It looks like it could just
be a local variable in probe()?
> + struct clk *clk;
> +};
Apart from that this nitpicking the driver looks very nice.
Yours,
Linus Walleij
On Wed, Jun 18, 2014 at 5:36 PM, Sören Brinkmann
<[email protected]> wrote:
> I did some of the changes for this v2 and a few things are not clear to
> me.
>
> The first is, how is userspace supposed to find the correct offset for a
> GPIO pin.
The sysfs interface to GPIO is *NOT* *GOOD* this is universally
agreed upon.
This needs someone to step in and provide a replacement, my preferred
mechanism would be a /dev/gpiochip0/... hierarchy using char devices.
> E.g. let's say GPIO 10 of this SOC-internal GPIO controller is
> something I want to control. So, I'd export GPIO (chip-base + 10). But
> this chip-base seems pretty variable. v1 of this patch had it hardcoded
> to 0, which resulted in a predictable offset, but apparently was utterly
> wrong. Now I see an offset of 138 for this chip when using my config.
> And when I use multi_v7_defconfig the offset is somewhere in the 800s,
> IIRC. The best I found was the 'label' in the gpiochip's sysfs entry,
> but documentation says that is not necessarily unique, and parsing labes
> seems sub-optimal too.
You see. It is badly designed and needs to be rewritten.
It was merged into the kernel at a time when the GPIO subsystem
was unmaintained, sadly.
> The second issue is a stack trace (below) I see when triggering
> interrupts (e.g. echo rising > edge; and then pushing the connected
> button). Looking at the stack trace, I don't see an obvious error (I
> think I pretty much copied the IRQ registration from the gpio-pl061.c
> driver you mentioned). Is this an issue in this driver or the core code?
Probably.
Using GPIOs for IRQs in userspace is an even worse idea than using
GPIOs from userspace in general :-D
But before you add any hairy code in userspace, please have a look
at Documentation/gpio/sysfs.txt:
"Note that standard kernel drivers exist for common "LEDs and Buttons"
GPIO tasks: "leds-gpio" and "gpio_keys", respectively. Use those
instead of talking directly to the GPIOs; they integrate with kernel
frameworks better than your userspace code could."
So: what is the usecase for these GPIOs?
Yours,
Linus Walleij
On Wed, Jun 18, 2014 at 1:39 PM, Harini Katakam <[email protected]> wrote:
> From: Harini Katakam <[email protected]>
>
> Add gpio-zynq bindings documentation.
>
> Signed-off-by: Harini Katakam <[email protected]>
> Signed-off-by: Soren Brinkmann <[email protected]>
> ---
>
> v2 changes:
> Improve description.
(...)
> +- #gpio-cells : Should be two. First cell is used to mention
> + pin number.
Don't call this "pin number", call it "GPIO line number".
Yours,
Linus Walleij
Hi Linux,
On Mon, Jul 07, 2014 at 04:51:56PM +0200, Linus Walleij wrote:
> On Wed, Jun 18, 2014 at 5:36 PM, S?ren Brinkmann <[email protected]> wrote:
> > The first is, how is userspace supposed to find the correct offset for a
> > GPIO pin.
>
> The sysfs interface to GPIO is *NOT* *GOOD* this is universally
> agreed upon.
>
> This needs someone to step in and provide a replacement, my preferred
> mechanism would be a /dev/gpiochip0/... hierarchy using char devices.
I really like the ability to control GPIOs from shell, both interactively and
scripted. I find it useful for quick hardware level debugging, and for boot
time scripting (mainly in initramfs). Do you intend to preserve this
capability in the future? Will I need a special utility (or Busybox applet) to
do this with char device ioctl?
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
Hi Linus,
On Mon, 2014-07-07 at 04:51PM +0200, Linus Walleij wrote:
> On Wed, Jun 18, 2014 at 5:36 PM, Sören Brinkmann
> <[email protected]> wrote:
>
> > I did some of the changes for this v2 and a few things are not clear to
> > me.
> >
> > The first is, how is userspace supposed to find the correct offset for a
> > GPIO pin.
>
> The sysfs interface to GPIO is *NOT* *GOOD* this is universally
> agreed upon.
>
> This needs someone to step in and provide a replacement, my preferred
> mechanism would be a /dev/gpiochip0/... hierarchy using char devices.
>
> > E.g. let's say GPIO 10 of this SOC-internal GPIO controller is
> > something I want to control. So, I'd export GPIO (chip-base + 10). But
> > this chip-base seems pretty variable. v1 of this patch had it hardcoded
> > to 0, which resulted in a predictable offset, but apparently was utterly
> > wrong. Now I see an offset of 138 for this chip when using my config.
> > And when I use multi_v7_defconfig the offset is somewhere in the 800s,
> > IIRC. The best I found was the 'label' in the gpiochip's sysfs entry,
> > but documentation says that is not necessarily unique, and parsing labes
> > seems sub-optimal too.
>
> You see. It is badly designed and needs to be rewritten.
>
> It was merged into the kernel at a time when the GPIO subsystem
> was unmaintained, sadly.
>
> > The second issue is a stack trace (below) I see when triggering
> > interrupts (e.g. echo rising > edge; and then pushing the connected
> > button). Looking at the stack trace, I don't see an obvious error (I
> > think I pretty much copied the IRQ registration from the gpio-pl061.c
> > driver you mentioned). Is this an issue in this driver or the core code?
>
> Probably.
>
> Using GPIOs for IRQs in userspace is an even worse idea than using
> GPIOs from userspace in general :-D
>
> But before you add any hairy code in userspace, please have a look
> at Documentation/gpio/sysfs.txt:
>
> "Note that standard kernel drivers exist for common "LEDs and Buttons"
> GPIO tasks: "leds-gpio" and "gpio_keys", respectively. Use those
> instead of talking directly to the GPIOs; they integrate with kernel
> frameworks better than your userspace code could."
>
> So: what is the usecase for these GPIOs?
Yea, in this case it was a button. I have to look at these drivers. It's
very likely that they cover what I want. But this case is trivial. I
really don't do anything but enabling the IRQ by writing to the edge
attribute and press the push-button connected to that GPIO line.
But as a general note: I think we have quite some customers trying to do
GPIO in userspace. With Zynq's FPGA portion, a lot of things come down
to make signals accessible in Linux and a lot of people do not want or
need a full blown kernel driver and use GPIO. The request for 'how to
handle GPIO IRQs in userspace' is pretty common. Often this gets passed
on to UIO though.
Thanks for the answers.
Sören
On Mon, 2014-07-07 at 04:45PM +0200, Linus Walleij wrote:
> On Wed, Jun 18, 2014 at 1:39 PM, Harini Katakam <[email protected]> wrote:
>
> > From: Harini Katakam <[email protected]>
> >
> > Add support for GPIO controller used by Xilinx Zynq.
> >
> > Signed-off-by: Harini Katakam <[email protected]>
> > Signed-off-by: Soren Brinkmann <[email protected]>
> > ---
> >
> > v2 changes:
> > - convert to pm_runtime_force_(suspend|resume)
> > - add pm_runtime_set_active in probe()
> > - also (un)prepare clocks when they are dis-/enabled
> > - add some missing calls to pm_runtime_get()
> > - use pm_runtime_put() instead of sync variant
> > - remove gpio chip in driver remove()
> > - remove redundant type casts
> > - directly use IO helpers
> > - use BIT macro to set/clear bits
> > - migrate to GPIOLIB_IRQCHIP
>
> This is a great improvement! Only small stuff remains.
>
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/gpio.h>
>
> This should be:
> #include <linux/gpio/driver.h>
>
> If that doesn't work ... why?
Works just fine.
>
> > +/**
> > + * struct zynq_gpio - gpio device private data structure
> > + * @chip: instance of the gpio_chip
> > + * @base_addr: base address of the GPIO device
> > + * @irq: irq associated with the controller
> > + * @clk: clock resource for this controller
> > + */
> > +struct zynq_gpio {
> > + struct gpio_chip chip;
> > + void __iomem *base_addr;
> > + int irq;
>
> Why is irq kept around in this struct? It looks like it could just
> be a local variable in probe()?
You're right.
I think that were both part of the legacy in this driver.
Sören
On Mon, 2014-07-07 at 04:53PM +0200, Linus Walleij wrote:
> On Wed, Jun 18, 2014 at 1:39 PM, Harini Katakam <[email protected]> wrote:
>
> > From: Harini Katakam <[email protected]>
> >
> > Add gpio-zynq bindings documentation.
> >
> > Signed-off-by: Harini Katakam <[email protected]>
> > Signed-off-by: Soren Brinkmann <[email protected]>
> > ---
> >
> > v2 changes:
> > Improve description.
> (...)
> > +- #gpio-cells : Should be two. First cell is used to mention
> > + pin number.
>
> Don't call this "pin number", call it "GPIO line number".
It's also not mentioning the second cell at all. I'll take the part from
gpio-msm to document this property and replace 'pin' with 'GPIO line'.
Sören
On Mon, Jul 7, 2014 at 5:23 PM, Baruch Siach <[email protected]> wrote:
> On Mon, Jul 07, 2014 at 04:51:56PM +0200, Linus Walleij wrote:
>> This needs someone to step in and provide a replacement, my preferred
>> mechanism would be a /dev/gpiochip0/... hierarchy using char devices.
>
> I really like the ability to control GPIOs from shell, both interactively and
> scripted. I find it useful for quick hardware level debugging,
I would be happy to carry it in the unstable-ABI debugfs for sure.
It's the supported ABI that bothers me.
> and for boot
> time scripting (mainly in initramfs).
What is the usecase here?
> Do you intend to preserve this
> capability in the future? Will I need a special utility (or Busybox applet) to
> do this with char device ioctl?
It is ABI so we have to preserve it. But sometimes people come with
new zuper-complex userspace usecases and want to add this or
that crazy ABI to do stuff in userspace that the kernel can do better.
And all that I intend to just NACK.
Yours,
Linus Walleij
On Mon, Jul 7, 2014 at 6:08 PM, Sören Brinkmann
<[email protected]> wrote:
>> So: what is the usecase for these GPIOs?
>
> Yea, in this case it was a button. I have to look at these drivers. It's
> very likely that they cover what I want. But this case is trivial. I
> really don't do anything but enabling the IRQ by writing to the edge
> attribute and press the push-button connected to that GPIO line.
In case of a system using device tree it is very trivial to add a gpio
key binding. After compiling in the gpio keys driver this small
snipper type is all that is really needed in most cases:
/* User key mapped in as "escape" */
gpio-keys {
compatible = "gpio-keys";
user-button {
label = "user_button";
gpios = <&gpio0 3 0x1>;
linux,code = <1>; /* KEY_ESC */
gpio-key,wakeup;
};
};
> But as a general note: I think we have quite some customers trying to do
> GPIO in userspace.
For what? I mean the use cases. Usually it is a bad idea, and
as shown above, just using the right existing device driver with
device tree is much easier, also for an end user, given they know
how to alter DTs and compile in kernel modules.
> With Zynq's FPGA portion, a lot of things come down
> to make signals accessible in Linux and a lot of people do not want or
> need a full blown kernel driver and use GPIO. The request for 'how to
> handle GPIO IRQs in userspace' is pretty common. Often this gets passed
> on to UIO though.
The short answer is don't handle GPIO IRQs in userspace.
Userspace drivers is generally a bad idea and should not be written.
The kernel is intended to handle hardware.
The above is doubly true for anything involving IRQs. Just think
of what IRQs are. They are one of the reasons why we have a
kernel and not just write all software on a system from scratch
ourselves.
Yours,
Linus Walleij
Hi Linus,
On Tue, 2014-07-08 at 11:34AM +0200, Linus Walleij wrote:
> On Mon, Jul 7, 2014 at 6:08 PM, Sören Brinkmann
> <[email protected]> wrote:
>
> >> So: what is the usecase for these GPIOs?
> >
> > Yea, in this case it was a button. I have to look at these drivers. It's
> > very likely that they cover what I want. But this case is trivial. I
> > really don't do anything but enabling the IRQ by writing to the edge
> > attribute and press the push-button connected to that GPIO line.
>
> In case of a system using device tree it is very trivial to add a gpio
> key binding. After compiling in the gpio keys driver this small
> snipper type is all that is really needed in most cases:
>
> /* User key mapped in as "escape" */
> gpio-keys {
> compatible = "gpio-keys";
> user-button {
> label = "user_button";
> gpios = <&gpio0 3 0x1>;
> linux,code = <1>; /* KEY_ESC */
> gpio-key,wakeup;
> };
> };
Thanks for the example. I think for my cases these drivers are exactly
the right thing.
>
>
> > But as a general note: I think we have quite some customers trying to do
> > GPIO in userspace.
>
> For what? I mean the use cases. Usually it is a bad idea, and
> as shown above, just using the right existing device driver with
> device tree is much easier, also for an end user, given they know
> how to alter DTs and compile in kernel modules.
>
> > With Zynq's FPGA portion, a lot of things come down
> > to make signals accessible in Linux and a lot of people do not want or
> > need a full blown kernel driver and use GPIO. The request for 'how to
> > handle GPIO IRQs in userspace' is pretty common. Often this gets passed
> > on to UIO though.
>
> The short answer is don't handle GPIO IRQs in userspace.
>
> Userspace drivers is generally a bad idea and should not be written.
> The kernel is intended to handle hardware.
>
> The above is doubly true for anything involving IRQs. Just think
> of what IRQs are. They are one of the reasons why we have a
> kernel and not just write all software on a system from scratch
> ourselves.
I fully agree and you don't have to convince me. But to a lot of our
customers that are used to use FPGAs, SOCs and Linux are pretty new. You
see a lot of scary stuff. Accessing /dev/mem seems to be a lot of
people's big hammer solution for everything. Then I always perceive it
as great progress if things like the sysfs interface are used instead.
Thanks,
Sören
On Tue, Jul 8, 2014 at 5:55 PM, Sören Brinkmann
<[email protected]> wrote:
> I fully agree and you don't have to convince me. But to a lot of our
> customers that are used to use FPGAs, SOCs and Linux are pretty new. You
> see a lot of scary stuff. Accessing /dev/mem seems to be a lot of
> people's big hammer solution for everything. Then I always perceive it
> as great progress if things like the sysfs interface are used instead.
Well as upstream maintainer I really don't differentiate on the level
of badness of different bad solutions, none of them are really helpful
to the community, only to some specific user, so they get to carry the
quirks...
Yours,
Linus Walleij
Hi Linus,
On Tue, Jul 08, 2014 at 11:27:57AM +0200, Linus Walleij wrote:
> On Mon, Jul 7, 2014 at 5:23 PM, Baruch Siach <[email protected]> wrote:
> > On Mon, Jul 07, 2014 at 04:51:56PM +0200, Linus Walleij wrote:
>
> >> This needs someone to step in and provide a replacement, my preferred
> >> mechanism would be a /dev/gpiochip0/... hierarchy using char devices.
> >
> > I really like the ability to control GPIOs from shell, both interactively and
> > scripted. I find it useful for quick hardware level debugging,
>
> I would be happy to carry it in the unstable-ABI debugfs for sure.
> It's the supported ABI that bothers me.
>
> > and for boot
> > time scripting (mainly in initramfs).
>
> What is the usecase here?
During boot I need to know, for example, when an FPGA is ready to be
programmed, and when said FPGA has finished its initialization. I also read
the sate of on-board DIP switches to determine the desired boot method. I find
the current sysfs interface quite convenient for these and similar tasks.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
On Fri, Jul 11, 2014 at 8:28 AM, Baruch Siach <[email protected]> wrote:
> On Tue, Jul 08, 2014 at 11:27:57AM +0200, Linus Walleij wrote:
>> What is the usecase here?
>
> During boot I need to know, for example, when an FPGA is ready to be
> programmed, and when said FPGA has finished its initialization. I also read
> the sate of on-board DIP switches to determine the desired boot method. I find
> the current sysfs interface quite convenient for these and similar tasks.
So given that an FPGA is a piece of hardware, it should have its firmware
loaded from the kernel and a kernel driver communicating with it I guess?
I feel the smell of a huge bundle of userspace drivers for something that
should be handled by the kernel.
Yours,
Linus Walleij
Hi Linus,
On Fri, Jul 11, 2014 at 02:06:29PM +0200, Linus Walleij wrote:
> On Fri, Jul 11, 2014 at 8:28 AM, Baruch Siach <[email protected]> wrote:
> > On Tue, Jul 08, 2014 at 11:27:57AM +0200, Linus Walleij wrote:
>
> >> What is the usecase here?
> >
> > During boot I need to know, for example, when an FPGA is ready to be
> > programmed, and when said FPGA has finished its initialization. I also read
> > the sate of on-board DIP switches to determine the desired boot method. I find
> > the current sysfs interface quite convenient for these and similar tasks.
>
> So given that an FPGA is a piece of hardware, it should have its firmware
> loaded from the kernel and a kernel driver communicating with it I guess?
>
> I feel the smell of a huge bundle of userspace drivers for something that
> should be handled by the kernel.
The FPGA can be loaded using simple SPI bit-banging, for which I used the
userspace SPI interface. Nothing fancy, just a few hundred LoC. Kernel driver
along with a userspace interface to be maintained forever for this task seems
an overkill to me.
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
On Fri, Jul 11, 2014 at 2:27 PM, Baruch Siach <[email protected]> wrote:
> On Fri, Jul 11, 2014 at 02:06:29PM +0200, Linus Walleij wrote:
>> So given that an FPGA is a piece of hardware, it should have its firmware
>> loaded from the kernel and a kernel driver communicating with it I guess?
>>
>> I feel the smell of a huge bundle of userspace drivers for something that
>> should be handled by the kernel.
>
> The FPGA can be loaded using simple SPI bit-banging, for which I used the
> userspace SPI interface. Nothing fancy, just a few hundred LoC. Kernel driver
> along with a userspace interface to be maintained forever for this task seems
> an overkill to me.
OK I'd say yes maybe it's like a modem on the other side of a serial
line then, and then it makes sense to have that as a userspace thing.
It's just that when it comes to anything relating to the electrical
connections on the board, that stuff should be in some device tree
or similar HW description format, and then this needs to be parsed
by userspace too, and ... yuck. It's just so incoherent in a
helicopter view.
Yours,
Linus Walleij