Add GPIO driver for T186 based platforms.
Adds support for MAIN and AON GPIO's from T186.
Signed-off-by: Suresh Mangipudi <[email protected]>
---
drivers/gpio/Kconfig | 8 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-tegra186.c | 647 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 656 insertions(+)
create mode 100644 drivers/gpio/gpio-tegra186.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a9a1c8a..99aeded 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -409,6 +409,14 @@ config GPIO_TB10X
select GENERIC_IRQ_CHIP
select OF_GPIO
+config GPIO_TEGRA186
+ bool "NVIDIA Tegra186 GPIO support"
+ default ARCH_TEGRA
+ depends on ARCH_TEGRA || COMPILE_TEST
+ depends on OF_GPIO
+ help
+ Support for the NVIDIA Tegra186 GPIO controller driver.
+
config GPIO_TEGRA
bool "NVIDIA Tegra GPIO support"
default ARCH_TEGRA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 8043a95..35ccc47 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_GPIO_SYSCON) += gpio-syscon.o
obj-$(CONFIG_GPIO_TB10X) += gpio-tb10x.o
obj-$(CONFIG_GPIO_TC3589X) += gpio-tc3589x.o
obj-$(CONFIG_GPIO_TEGRA) += gpio-tegra.o
+obj-$(CONFIG_GPIO_TEGRA186) += gpio-tegra186.o
obj-$(CONFIG_GPIO_TIMBERDALE) += gpio-timberdale.o
obj-$(CONFIG_GPIO_PALMAS) += gpio-palmas.o
obj-$(CONFIG_GPIO_TPIC2810) += gpio-tpic2810.o
diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
new file mode 100644
index 0000000..c66600d
--- /dev/null
+++ b/drivers/gpio/gpio-tegra186.c
@@ -0,0 +1,647 @@
+/*
+ * GPIO driver for NVIDIA Tegra186
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ *
+ * Author: Suresh Mangipudi <[email protected]>
+ * Author: Laxman Dewangan <[email protected]>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
+#include <dt-bindings/gpio/tegra186-gpio.h>
+
+/* GPIO control registers */
+#define GPIO_ENB_CONFIG_REG 0x00
+#define GPIO_DBC_THRES_REG 0x04
+#define GPIO_INPUT_REG 0x08
+#define GPIO_OUT_CTRL_REG 0x0c
+#define GPIO_OUT_VAL_REG 0x10
+#define GPIO_INT_CLEAR_REG 0x14
+#define GPIO_REG_DIFF 0x20
+#define GPIO_INT_STATUS_OFFSET 0x100
+
+/* GPIO SCR registers */
+#define GPIO_SCR_REG 0x04
+#define GPIO_SCR_DIFF 0x08
+
+#define GPIO_INOUT_BIT BIT(1)
+#define GPIO_TRG_TYPE_BIT(x) ((x) & 0x3)
+#define GPIO_TRG_TYPE_BIT_OFFSET 0x2
+#define GPIO_TRG_LVL_BIT BIT(4)
+#define GPIO_DEB_FUNC_BIT BIT(5)
+#define GPIO_INT_FUNC_BIT BIT(6)
+
+#define GPIO_SCR_SEC_WEN BIT(28)
+#define GPIO_SCR_SEC_REN BIT(27)
+#define GPIO_SCR_SEC_G1W BIT(9)
+#define GPIO_SCR_SEC_G1R BIT(1)
+#define GPIO_FULL_ACCESS (GPIO_SCR_SEC_WEN | \
+ GPIO_SCR_SEC_REN | \
+ GPIO_SCR_SEC_G1R | \
+ GPIO_SCR_SEC_G1W)
+
+#define GPIO_INT_LVL_LEVEL_TRIGGER 0x1
+#define GPIO_INT_LVL_SINGLE_EDGE_TRIGGER 0x2
+#define GPIO_INT_LVL_BOTH_EDGE_TRIGGER 0x3
+
+#define TRIGGER_LEVEL_LOW 0x0
+#define TRIGGER_LEVEL_HIGH 0x1
+
+#define GPIO_STATUS_G1 0x04
+
+#define MAX_GPIO_CONTROLLERS 7
+#define MAX_GPIO_PORTS 8
+
+#define GPIO_PORT(g) ((g) >> 3)
+#define GPIO_PIN(g) ((g) & 0x7)
+
+struct tegra_gpio_port_soc_info {
+ const char *port_name;
+ int cont_id;
+ int port_index;
+ int valid_pins;
+ int scr_offset;
+ u32 reg_offset;
+};
+
+#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins) \
+[TEGRA_MAIN_GPIO_PORT_##port] = { \
+ .port_name = #port, \
+ .cont_id = cid, \
+ .port_index = cind, \
+ .valid_pins = npins, \
+ .scr_offset = cid * 0x1000 + cind * 0x40, \
+ .reg_offset = cid * 0x1000 + cind * 0x200, \
+}
+
+#define TEGRA_AON_GPIO_PORT_INFO(port, cid, cind, npins) \
+[TEGRA_AON_GPIO_PORT_##port] = { \
+ .port_name = #port, \
+ .cont_id = cid, \
+ .port_index = cind, \
+ .valid_pins = npins, \
+ .scr_offset = cind * 0x40, \
+ .reg_offset = cind * 0x200, \
+}
+
+static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
+ TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(C, 3, 1, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(D, 3, 2, 6),
+ TEGRA_MAIN_GPIO_PORT_INFO(E, 2, 1, 8),
+ TEGRA_MAIN_GPIO_PORT_INFO(F, 2, 2, 6),
+ TEGRA_MAIN_GPIO_PORT_INFO(G, 4, 1, 6),
+ TEGRA_MAIN_GPIO_PORT_INFO(H, 1, 0, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(I, 0, 4, 8),
+ TEGRA_MAIN_GPIO_PORT_INFO(J, 5, 0, 8),
+ TEGRA_MAIN_GPIO_PORT_INFO(K, 5, 1, 1),
+ TEGRA_MAIN_GPIO_PORT_INFO(L, 1, 1, 8),
+ TEGRA_MAIN_GPIO_PORT_INFO(M, 5, 3, 6),
+ TEGRA_MAIN_GPIO_PORT_INFO(N, 0, 0, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(O, 0, 1, 4),
+ TEGRA_MAIN_GPIO_PORT_INFO(P, 4, 0, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(Q, 0, 2, 6),
+ TEGRA_MAIN_GPIO_PORT_INFO(R, 0, 5, 6),
+ TEGRA_MAIN_GPIO_PORT_INFO(T, 0, 3, 4),
+ TEGRA_MAIN_GPIO_PORT_INFO(X, 1, 2, 8),
+ TEGRA_MAIN_GPIO_PORT_INFO(Y, 1, 3, 7),
+ TEGRA_MAIN_GPIO_PORT_INFO(BB, 2, 3, 2),
+ TEGRA_MAIN_GPIO_PORT_INFO(CC, 5, 2, 4),
+};
+
+static struct tegra_gpio_port_soc_info tegra_aon_gpio_cinfo[] = {
+ TEGRA_AON_GPIO_PORT_INFO(S, 0, 1, 5),
+ TEGRA_AON_GPIO_PORT_INFO(U, 0, 2, 6),
+ TEGRA_AON_GPIO_PORT_INFO(V, 0, 4, 8),
+ TEGRA_AON_GPIO_PORT_INFO(W, 0, 5, 8),
+ TEGRA_AON_GPIO_PORT_INFO(Z, 0, 7, 4),
+ TEGRA_AON_GPIO_PORT_INFO(AA, 0, 6, 8),
+ TEGRA_AON_GPIO_PORT_INFO(EE, 0, 3, 3),
+ TEGRA_AON_GPIO_PORT_INFO(FF, 0, 0, 5),
+};
+
+struct tegra_gpio_info;
+
+struct tegra_gpio_soc_info {
+ const char *name;
+ const struct tegra_gpio_port_soc_info *port;
+ int nports;
+};
+
+struct tegra_gpio_controller {
+ int controller;
+ int irq;
+ struct tegra_gpio_info *tgi;
+};
+
+struct tegra_gpio_info {
+ struct device *dev;
+ int nbanks;
+ void __iomem *gpio_regs;
+ void __iomem *scr_regs;
+ struct irq_domain *irq_domain;
+ const struct tegra_gpio_soc_info *soc;
+ struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
+ struct gpio_chip gc;
+ struct irq_chip ic;
+};
+
+#define GPIO_CNTRL_REG(tgi, gpio, roffset) \
+ ((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
+ (GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))
+
+static u32 tegra_gpio_readl(struct tegra_gpio_info *tgi, u32 gpio,
+ u32 reg_offset)
+{
+ return __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+}
+
+static void tegra_gpio_writel(struct tegra_gpio_info *tgi, u32 val,
+ u32 gpio, u32 reg_offset)
+{
+ __raw_writel(val, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+}
+
+static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
+ u32 reg_offset, u32 mask, u32 val)
+{
+ u32 rval;
+
+ rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+ rval = (rval & ~mask) | (val & mask);
+ __raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
+}
+
+/* This function will return if the GPIO is accessible by CPU */
+static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)
+{
+ int port = GPIO_PORT(offset);
+ int pin = GPIO_PIN(offset);
+ u32 val;
+ int cont_id;
+ u32 scr_offset = tgi->soc->port[port].scr_offset;
+
+ if (pin >= tgi->soc->port[port].valid_pins)
+ return false;
+
+ cont_id = tgi->soc->port[port].cont_id;
+ if (cont_id < 0)
+ return false;
+
+ val = __raw_readl(tgi->scr_regs + scr_offset +
+ (pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
+
+ if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
+ return true;
+
+ return false;
+}
+
+static void tegra_gpio_enable(struct tegra_gpio_info *tgi, int gpio)
+{
+ tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, 0x1, 0x1);
+}
+
+static void tegra_gpio_disable(struct tegra_gpio_info *tgi, int gpio)
+{
+ tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, 0x1, 0x0);
+}
+
+static void tegra_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ tegra_gpio_disable(tgi, offset);
+}
+
+static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ u32 val = (value) ? 0x1 : 0x0;
+
+ tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
+ tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);
+}
+
+static int tegra_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ u32 val;
+
+ val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);
+ if (val & GPIO_INOUT_BIT)
+ return tegra_gpio_readl(tgi, offset, GPIO_OUT_VAL_REG) & 0x1;
+
+ return tegra_gpio_readl(tgi, offset, GPIO_INPUT_REG) & 0x1;
+}
+
+static void set_gpio_direction_mode(struct gpio_chip *chip, u32 offset,
+ bool mode)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ u32 val;
+
+ val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);
+ if (mode)
+ val |= GPIO_INOUT_BIT;
+ else
+ val &= ~GPIO_INOUT_BIT;
+ tegra_gpio_writel(tgi, val, offset, GPIO_ENB_CONFIG_REG);
+}
+
+static int tegra_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ set_gpio_direction_mode(chip, offset, 0);
+ tegra_gpio_enable(tgi, offset);
+
+ return 0;
+}
+
+static int tegra_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ tegra_gpio_set(chip, offset, value);
+ set_gpio_direction_mode(chip, offset, 1);
+ tegra_gpio_enable(tgi, offset);
+
+ return 0;
+}
+
+static int tegra_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
+ unsigned int debounce)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ unsigned int dbc_ms = DIV_ROUND_UP(debounce, 1000);
+
+ tegra_gpio_update(tgi, offset, GPIO_ENB_CONFIG_REG, 0x1, 0x1);
+ tegra_gpio_update(tgi, offset, GPIO_DEB_FUNC_BIT, 0x5, 0x1);
+
+ /* Update debounce threshold */
+ tegra_gpio_writel(tgi, dbc_ms, offset, GPIO_DBC_THRES_REG);
+
+ return 0;
+}
+
+static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+ u32 val;
+
+ if (!gpio_is_accessible(tgi, offset))
+ return 0;
+
+ val = tegra_gpio_readl(tgi, offset, GPIO_OUT_CTRL_REG);
+
+ return (val & 0x1);
+}
+
+static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+ struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
+
+ return irq_find_mapping(tgi->irq_domain, offset);
+}
+
+static void tegra_gpio_irq_ack(struct irq_data *d)
+{
+ struct tegra_gpio_controller *ctrlr = irq_data_get_irq_chip_data(d);
+
+ tegra_gpio_writel(ctrlr->tgi, 1, d->hwirq, GPIO_INT_CLEAR_REG);
+}
+
+static void tegra_gpio_irq_mask(struct irq_data *d)
+{
+ struct tegra_gpio_controller *c = irq_data_get_irq_chip_data(d);
+
+ tegra_gpio_update(c->tgi, d->hwirq, GPIO_ENB_CONFIG_REG,
+ GPIO_INT_FUNC_BIT, 0);
+}
+
+static void tegra_gpio_irq_unmask(struct irq_data *d)
+{
+ struct tegra_gpio_controller *c = irq_data_get_irq_chip_data(d);
+
+ tegra_gpio_update(c->tgi, d->hwirq, GPIO_ENB_CONFIG_REG,
+ GPIO_INT_FUNC_BIT, GPIO_INT_FUNC_BIT);
+}
+
+static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+ struct tegra_gpio_controller *ctrlr = irq_data_get_irq_chip_data(d);
+ int gpio = d->hwirq;
+ u32 lvl_type;
+ u32 trg_type;
+ u32 val;
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_RISING:
+ trg_type = TRIGGER_LEVEL_HIGH;
+ lvl_type = GPIO_INT_LVL_SINGLE_EDGE_TRIGGER;
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ trg_type = TRIGGER_LEVEL_LOW;
+ lvl_type = GPIO_INT_LVL_SINGLE_EDGE_TRIGGER;
+ break;
+
+ case IRQ_TYPE_EDGE_BOTH:
+ lvl_type = GPIO_INT_LVL_BOTH_EDGE_TRIGGER;
+ trg_type = 0;
+ break;
+
+ case IRQ_TYPE_LEVEL_HIGH:
+ trg_type = TRIGGER_LEVEL_HIGH;
+ lvl_type = GPIO_INT_LVL_LEVEL_TRIGGER;
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ trg_type = TRIGGER_LEVEL_LOW;
+ lvl_type = GPIO_INT_LVL_LEVEL_TRIGGER;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ trg_type = trg_type << 0x4;
+ lvl_type = lvl_type << 0x2;
+
+ /* Clear and Program the values */
+ val = tegra_gpio_readl(ctrlr->tgi, gpio, GPIO_ENB_CONFIG_REG);
+ val &= ~((0x3 << GPIO_TRG_TYPE_BIT_OFFSET) | (GPIO_TRG_LVL_BIT));
+ val |= trg_type | lvl_type;
+ tegra_gpio_writel(ctrlr->tgi, val, gpio, GPIO_ENB_CONFIG_REG);
+
+ tegra_gpio_enable(ctrlr->tgi, gpio);
+
+ if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+ irq_set_handler_locked(d, handle_level_irq);
+ else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+ irq_set_handler_locked(d, handle_edge_irq);
+
+ return 0;
+}
+
+static void tegra_gpio_irq_handler(struct irq_desc *desc)
+{
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct tegra_gpio_controller *tg_cont = irq_desc_get_handler_data(desc);
+ struct tegra_gpio_info *tgi = tg_cont->tgi;
+ int pin;
+ int port;
+ u32 i;
+ unsigned long val;
+ u32 gpio;
+ u32 addr;
+ int port_map[MAX_GPIO_PORTS];
+
+ for (i = 0; i < MAX_GPIO_PORTS; ++i)
+ port_map[i] = -1;
+
+ for (i = 0; i < tgi->soc->nports; ++i) {
+ if (tgi->soc->port[i].cont_id == tg_cont->controller)
+ port_map[tgi->soc->port[i].port_index] = i;
+ }
+
+ chained_irq_enter(chip, desc);
+ for (i = 0; i < MAX_GPIO_PORTS; i++) {
+ port = port_map[i];
+ if (port == -1)
+ continue;
+
+ addr = tgi->soc->port[port].reg_offset;
+ val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
+ GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
+ gpio = tgi->gc.base + (port * 8);
+ for_each_set_bit(pin, &val, 8)
+ generic_handle_irq(gpio_to_irq(gpio + pin));
+ }
+
+ chained_irq_exit(chip, desc);
+}
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+static int dbg_gpio_show(struct seq_file *s, void *unused)
+{
+ struct tegra_gpio_info *tgi = s->private;
+ int i;
+
+ seq_puts(s, "Port:Pin:ENB DBC IN OUT_CTRL OUT_VAL INT_CLR\n");
+ for (i = 0; i < tgi->gc.ngpio; i++) {
+ if (!gpio_is_accessible(tgi, i))
+ continue;
+ seq_printf(s, "%s:%d 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n",
+ tgi->soc->port[GPIO_PORT(i)].port_name, i % 8,
+ tegra_gpio_readl(tgi, i, GPIO_ENB_CONFIG_REG),
+ tegra_gpio_readl(tgi, i, GPIO_DBC_THRES_REG),
+ tegra_gpio_readl(tgi, i, GPIO_INPUT_REG),
+ tegra_gpio_readl(tgi, i, GPIO_OUT_CTRL_REG),
+ tegra_gpio_readl(tgi, i, GPIO_OUT_VAL_REG),
+ tegra_gpio_readl(tgi, i, GPIO_INT_CLEAR_REG));
+ }
+
+ return 0;
+}
+
+static int dbg_gpio_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dbg_gpio_show, inode->i_private);
+}
+
+static const struct file_operations debug_fops = {
+ .open = dbg_gpio_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int __init tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
+{
+ (void)debugfs_create_file(tgi->soc->name, 0444, NULL, tgi, &debug_fops);
+
+ return 0;
+}
+#else
+static int tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
+{
+ return 0;
+}
+#endif
+
+static int tegra_gpio_probe(struct platform_device *pdev)
+{
+ struct tegra_gpio_info *tgi;
+ struct resource *res;
+ int bank;
+ int gpio;
+ int ret;
+
+ for (bank = 0;; bank++) {
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
+ if (!res)
+ break;
+ }
+ if (!bank) {
+ dev_err(&pdev->dev, "No GPIO Controller found\n");
+ return -ENODEV;
+ }
+
+ tgi = devm_kzalloc(&pdev->dev, sizeof(*tgi), GFP_KERNEL);
+ if (!tgi)
+ return -ENOMEM;
+ tgi->dev = &pdev->dev;
+ tgi->nbanks = bank;
+ tgi->soc = of_device_get_match_data(&pdev->dev);
+
+ tgi->gc.label = tgi->soc->name;
+ tgi->gc.free = tegra_gpio_free;
+ tgi->gc.direction_input = tegra_gpio_direction_input;
+ tgi->gc.get = tegra_gpio_get;
+ tgi->gc.direction_output = tegra_gpio_direction_output;
+ tgi->gc.set = tegra_gpio_set;
+ tgi->gc.get_direction = tegra_gpio_get_direction;
+ tgi->gc.to_irq = tegra_gpio_to_irq;
+ tgi->gc.set_debounce = tegra_gpio_set_debounce;
+ tgi->gc.base = -1;
+ tgi->gc.ngpio = tgi->soc->nports * 8;
+ tgi->gc.parent = &pdev->dev;
+ tgi->gc.of_node = pdev->dev.of_node;
+
+ tgi->ic.name = tgi->soc->name;
+ tgi->ic.irq_ack = tegra_gpio_irq_ack;
+ tgi->ic.irq_mask = tegra_gpio_irq_mask;
+ tgi->ic.irq_unmask = tegra_gpio_irq_unmask;
+ tgi->ic.irq_set_type = tegra_gpio_irq_set_type;
+ tgi->ic.irq_shutdown = tegra_gpio_irq_mask;
+ tgi->ic.irq_disable = tegra_gpio_irq_mask;
+
+ platform_set_drvdata(pdev, tgi);
+
+ for (bank = 0; bank < tgi->nbanks; bank++) {
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
+ tgi->tg_contrlr[bank].controller = bank;
+ tgi->tg_contrlr[bank].irq = res->start;
+ tgi->tg_contrlr[bank].tgi = tgi;
+ }
+
+ tgi->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
+ tgi->gc.ngpio,
+ &irq_domain_simple_ops, NULL);
+ if (!tgi->irq_domain)
+ return -ENODEV;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "security");
+ if (!res) {
+ dev_err(&pdev->dev, "Missing security MEM resource\n");
+ return -ENODEV;
+ }
+ tgi->scr_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tgi->scr_regs)) {
+ ret = PTR_ERR(tgi->scr_regs);
+ dev_err(&pdev->dev, "Failed to iomap for security: %d\n", ret);
+ return ret;
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio");
+ if (!res) {
+ dev_err(&pdev->dev, "Missing gpio MEM resource\n");
+ return -ENODEV;
+ }
+ tgi->gpio_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tgi->gpio_regs)) {
+ ret = PTR_ERR(tgi->gpio_regs);
+ dev_err(&pdev->dev, "Failed to iomap for gpio: %d\n", ret);
+ return ret;
+ }
+
+ ret = devm_gpiochip_add_data(&pdev->dev, &tgi->gc, tgi);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+ return ret;
+ }
+
+ for (gpio = 0; gpio < tgi->gc.ngpio; gpio++) {
+ int irq = irq_create_mapping(tgi->irq_domain, gpio);
+ int cont_id = tgi->soc->port[GPIO_PORT(gpio)].cont_id;
+
+ if (gpio_is_accessible(tgi, gpio))
+ /* mask interrupts for this GPIO */
+ tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG,
+ GPIO_INT_FUNC_BIT, 0);
+
+ irq_set_chip_data(irq, &tgi->tg_contrlr[cont_id]);
+ irq_set_chip_and_handler(irq, &tgi->ic, handle_simple_irq);
+ }
+
+ for (bank = 0; bank < tgi->nbanks; bank++)
+ irq_set_chained_handler_and_data(tgi->tg_contrlr[bank].irq,
+ tegra_gpio_irq_handler,
+ &tgi->tg_contrlr[bank]);
+
+ tegra_gpio_debuginit(tgi);
+
+ return 0;
+}
+
+static const struct tegra_gpio_soc_info t186_main_gpio_soc = {
+ .name = "tegra-main-gpio",
+ .port = tegra_main_gpio_cinfo,
+ .nports = ARRAY_SIZE(tegra_main_gpio_cinfo),
+};
+
+static const struct tegra_gpio_soc_info t186_aon_gpio_soc = {
+ .name = "tegra-aon-gpio",
+ .port = tegra_aon_gpio_cinfo,
+ .nports = ARRAY_SIZE(tegra_aon_gpio_cinfo),
+};
+
+static const struct of_device_id tegra_gpio_of_match[] = {
+ { .compatible = "nvidia,tegra186-gpio", .data = &t186_main_gpio_soc},
+ { .compatible = "nvidia,tegra186-gpio-aon", .data = &t186_aon_gpio_soc},
+ { },
+};
+
+static struct platform_driver tegra_gpio_driver = {
+ .driver = {
+ .name = "tegra186-gpio",
+ .of_match_table = tegra_gpio_of_match,
+ },
+ .probe = tegra_gpio_probe,
+};
+
+static int __init tegra_gpio_init(void)
+{
+ return platform_driver_register(&tegra_gpio_driver);
+}
+postcore_initcall(tegra_gpio_init);
+
+MODULE_AUTHOR("Suresh Mangipudi <[email protected]>");
+MODULE_AUTHOR("Laxman Dewangan <[email protected]>");
+MODULE_DESCRIPTION("NVIDIA Tegra186 GPIO driver");
+MODULE_LICENSE("GPL v2");
--
2.1.4
On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <[email protected]> wrote:
> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.
>
> Signed-off-by: Suresh Mangipudi <[email protected]>
Stephen/Thierry/Alexandre:
Can I get your review on this Tegra thing?
Yours,
Linus Walleij
On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <[email protected]> wrote:
>
> > Add GPIO driver for T186 based platforms.
> > Adds support for MAIN and AON GPIO's from T186.
> >
> > Signed-off-by: Suresh Mangipudi <[email protected]>
>
> Stephen/Thierry/Alexandre:
> Can I get your review on this Tegra thing?
Can we hold off on this for a bit? I've got my own implementation of
this in my Tegra186 tree because I thought nobody else was working on
it. From a brief look they seem mostly similar but there are a couple
of differences that I need to take a closer look at (and do some more
intensive testing on).
Thanks,
Thierry
On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <[email protected]> wrote:
> On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
>> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <[email protected]> wrote:
>>
>> > Add GPIO driver for T186 based platforms.
>> > Adds support for MAIN and AON GPIO's from T186.
>> >
>> > Signed-off-by: Suresh Mangipudi <[email protected]>
>>
>> Stephen/Thierry/Alexandre:
>> Can I get your review on this Tegra thing?
>
> Can we hold off on this for a bit? I've got my own implementation of
> this in my Tegra186 tree because I thought nobody else was working on
> it. From a brief look they seem mostly similar but there are a couple
> of differences that I need to take a closer look at (and do some more
> intensive testing on).
Be careful about discouraging other developers internally from
participating upstream, Thierry.
Please don't become a bottleneck for your company's contributions.
Rewriting stuff on your own isn't a scalable model.
-Olof
On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
> On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <[email protected]> wrote:
> > On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> >> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <[email protected]> wrote:
> >>
> >> > Add GPIO driver for T186 based platforms.
> >> > Adds support for MAIN and AON GPIO's from T186.
> >> >
> >> > Signed-off-by: Suresh Mangipudi <[email protected]>
> >>
> >> Stephen/Thierry/Alexandre:
> >> Can I get your review on this Tegra thing?
> >
> > Can we hold off on this for a bit? I've got my own implementation of
> > this in my Tegra186 tree because I thought nobody else was working on
> > it. From a brief look they seem mostly similar but there are a couple
> > of differences that I need to take a closer look at (and do some more
> > intensive testing on).
>
> Be careful about discouraging other developers internally from
> participating upstream, Thierry.
That was certainly not my intention. I do welcome any contributions,
internal or external.
> Please don't become a bottleneck for your company's contributions.
> Rewriting stuff on your own isn't a scalable model.
Like I said, I didn't rewrite this, I merely wrote the GPIO driver
because there wasn't one at the time and I wasn't aware of anyone else
working on one. Waiting for a GPIO driver to emerge would've prevented
me from making progress on other fronts.
Thierry
On 11/08/2016 08:55 AM, Thierry Reding wrote:
> On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
>> On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <[email protected]> wrote:
>>> On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
>>>> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <[email protected]> wrote:
>>>>
>>>>> Add GPIO driver for T186 based platforms.
>>>>> Adds support for MAIN and AON GPIO's from T186.
>>>>>
>>>>> Signed-off-by: Suresh Mangipudi <[email protected]>
>>>>
>>>> Stephen/Thierry/Alexandre:
>>>> Can I get your review on this Tegra thing?
>>>
>>> Can we hold off on this for a bit? I've got my own implementation of
>>> this in my Tegra186 tree because I thought nobody else was working on
>>> it. From a brief look they seem mostly similar but there are a couple
>>> of differences that I need to take a closer look at (and do some more
>>> intensive testing on).
>>
>> Be careful about discouraging other developers internally from
>> participating upstream, Thierry.
>
> That was certainly not my intention. I do welcome any contributions,
> internal or external.
>
>> Please don't become a bottleneck for your company's contributions.
>> Rewriting stuff on your own isn't a scalable model.
>
> Like I said, I didn't rewrite this, I merely wrote the GPIO driver
> because there wasn't one at the time and I wasn't aware of anyone else
> working on one. Waiting for a GPIO driver to emerge would've prevented
> me from making progress on other fronts.
One issue here is that there are lots of patches destined for upstream
in your own personal tree, but they aren't actually upstream yet. I
think pushing more of your work into linux-next (and further upstream)
faster would help people out. linux-next and other "standard" repos
should be easily discoverable by anyway following any upstreaming
HOWTOs, but those HOWTOs aren't going to mention your personal trees, so
patches there effectively don't exist. Making the already extant work
more discoverable will help prevent people duplicating work.
Related to this, I've been waiting rather a while for the Tegra186 DT
binding patches I sent to be applied. I'd love to see them go in ASAP
even if there's no kernel driver behind them. The bindings have been
reviewed, ack'd, and they're in use in U-Boot already.
A thought on Tegra186: For a older supported SoCs, we obviously don't
want to push changes upstream that aren't too well baked. However, for a
new SoC that doesn't work yet, I'm tend to prioritize getting as much
early work upstream as fast as possible (to try and unblock people
working in other areas) over getting those patches perfect first.
Release early, release often will help unblock people and parallelize
work. Pipeline your own work rather than batching it all up to release
at once.
P.S. don't take this email too personally or anything; I'm not trying to
be complaining/critical or anything like that. It's just a few mild
thoughts.
On Tue, Nov 08, 2016 at 09:49:27AM -0700, Stephen Warren wrote:
> On 11/08/2016 08:55 AM, Thierry Reding wrote:
> > On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
> > > On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <[email protected]> wrote:
> > > > On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> > > > > On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <[email protected]> wrote:
> > > > >
> > > > > > Add GPIO driver for T186 based platforms.
> > > > > > Adds support for MAIN and AON GPIO's from T186.
> > > > > >
> > > > > > Signed-off-by: Suresh Mangipudi <[email protected]>
> > > > >
> > > > > Stephen/Thierry/Alexandre:
> > > > > Can I get your review on this Tegra thing?
> > > >
> > > > Can we hold off on this for a bit? I've got my own implementation of
> > > > this in my Tegra186 tree because I thought nobody else was working on
> > > > it. From a brief look they seem mostly similar but there are a couple
> > > > of differences that I need to take a closer look at (and do some more
> > > > intensive testing on).
> > >
> > > Be careful about discouraging other developers internally from
> > > participating upstream, Thierry.
> >
> > That was certainly not my intention. I do welcome any contributions,
> > internal or external.
> >
> > > Please don't become a bottleneck for your company's contributions.
> > > Rewriting stuff on your own isn't a scalable model.
> >
> > Like I said, I didn't rewrite this, I merely wrote the GPIO driver
> > because there wasn't one at the time and I wasn't aware of anyone else
> > working on one. Waiting for a GPIO driver to emerge would've prevented
> > me from making progress on other fronts.
>
> One issue here is that there are lots of patches destined for upstream in
> your own personal tree, but they aren't actually upstream yet. I think
> pushing more of your work into linux-next (and further upstream) faster
> would help people out. linux-next and other "standard" repos should be
> easily discoverable by anyway following any upstreaming HOWTOs, but those
> HOWTOs aren't going to mention your personal trees, so patches there
> effectively don't exist. Making the already extant work more discoverable
> will help prevent people duplicating work.
I had assumed that I had properly communicated what the canonical
temporary location for Tegra186 patches would be, but you're right that
it's probably easy to miss, and linux-next would be a more obvious
target.
> Related to this, I've been waiting rather a while for the Tegra186 DT
> binding patches I sent to be applied. I'd love to see them go in ASAP even
> if there's no kernel driver behind them. The bindings have been reviewed,
> ack'd, and they're in use in U-Boot already.
It's true, I've been promising this for weeks now. I'll get around to it
within the week. Please do prod me in case I don't. I promise I won't
get mad =)
> A thought on Tegra186: For a older supported SoCs, we obviously don't want
> to push changes upstream that aren't too well baked. However, for a new SoC
> that doesn't work yet, I'm tend to prioritize getting as much early work
> upstream as fast as possible (to try and unblock people working in other
> areas) over getting those patches perfect first. Release early, release
> often will help unblock people and parallelize work. Pipeline your own work
> rather than batching it all up to release at once.
I'm always hesitant to merge code that isn't functional or tested, but
perhaps I'm being a little overzealous here, and I'm evidently not doing
so great, so let me try to be more aggressive.
> P.S. don't take this email too personally or anything; I'm not trying to be
> complaining/critical or anything like that. It's just a few mild thoughts.
No offense taken, thanks for being constructive about it.
Thierry
On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.
I'm not sure how you/Thierry will approach merging this with the other
GPIO driver he has, but here's a very quick review of this one in case
it's useful.
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> +#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins) \
A comment indicating what "cid" and "cind" mean (and perhaps the other
parameters too) would be useful.
A brief description of the overall register layout and structure and
differences between the MAIN/AON controllers would be useful.
> +[TEGRA_MAIN_GPIO_PORT_##port] = { \
> + .port_name = #port, \
> + .cont_id = cid, \
> + .port_index = cind, \
Why not make the parameter names match the field names they're assigned to?
> + .valid_pins = npins, \
> + .scr_offset = cid * 0x1000 + cind * 0x40, \
> + .reg_offset = cid * 0x1000 + cind * 0x200, \
While C does define operator precedence rules that make that expression
OK, I personally prefer using () to make it explict:
+ .reg_offset = (cid * 0x1000) + (cind * 0x200), \
That way, the reader doesn't have to think/remember so much.
Also, if these values can be calculated based on .cont_id and
.port_index, I wonder why we need to pre-calculate them here and/or what
else could be pre-calculated from .cont_id/.port_index? I'm tend to
either (a) just store .cont_id and .port_index and calculate everything
from them always, or (b) store just derived data and not both storing
.cont_id/.port_index.
> +static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
> + TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
> + TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),
I assume the entries in this file must be in the same order as the DT
binding port IDs? A comment to that effect would be useful.
> +struct tegra_gpio_info;
> +
> +struct tegra_gpio_soc_info {
> + const char *name;
> + const struct tegra_gpio_port_soc_info *port;
> + int nports;
> +};
This isn't information about an SoC; it's information about a
controller, and there are 2 controllers within Tegra186. Rename to
tegra_gpio_ctlr_info?
> +struct tegra_gpio_controller {
> + int controller;
> + int irq;
> + struct tegra_gpio_info *tgi;
> +};
> +
> +struct tegra_gpio_info {
Is this structure per-bank/-port? Also, "info" seems to be used above
for static configuration info/data. I think this should be called
"tegra_gpio_port"?
> + struct device *dev;
> + int nbanks;
> + void __iomem *gpio_regs;
> + void __iomem *scr_regs;
> + struct irq_domain *irq_domain;
> + const struct tegra_gpio_soc_info *soc;
> + struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
> + struct gpio_chip gc;
> + struct irq_chip ic;
> +};
> +#define GPIO_CNTRL_REG(tgi, gpio, roffset) \
> + ((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
> + (GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))
Writing a static inline function would make formatting and type safety
easier.
> +static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
> + u32 reg_offset, u32 mask, u32 val)
> +{
> + u32 rval;
> +
> + rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> + rval = (rval & ~mask) | (val & mask);
> + __raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +}
If you use a regmap object rather than a raw MMIO pointer, I believe
there's already a function that does this read-modify-write.
> +/* This function will return if the GPIO is accessible by CPU */
> +static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)
I'd prefer all functions use the same name prefix. "tegra_gpio" seems to
be used so far. Actually, given there's already an existing Tegra GPIO
driver, and this driver covers the new controller(s) in Tegra186, I'd
prefer everything be named tegra186_gpio_xxx.
> + if (cont_id < 0)
> + return false;
That should trigger a checkpatch error due to the presence of two spaces
in the expression. Try running checkpatch and fixing any issues.
> + val = __raw_readl(tgi->scr_regs + scr_offset +
> + (pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
> +
> + if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
> + return true;
I'm not entirely convinced about this. It's possible to have only read
or only write access. I believe the CPU can be assigned to an arbitrary
bus master group, whereas the value of GPIO_FULL_ACCESS assumes it's on
group 1. Do we actually need this function at all except for debug? I'd
be tempted to just drop it and let all GPIO accesses be attempted. If
the SCR is configured such that the CPU doesn't have access, writes
should be ignored and reads return valid dummy data. That seems fine to me.
Also, this function isn't consistently used by all client-callable APIs.
I'd expect it to be used from every function that's accessible via a
function pointer in struct gpio_chip, if it's useful.
> +static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> + tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
> + tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);
Shouldn't this function *just* set the output value; any other setup
should be done solely as part of direction_input/direction_output?
> +static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> + tegra_gpio_enable(ctrlr->tgi, gpio);
Shouldn't this only happen when the client actually calls enable/disable?
> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> + irq_set_handler_locked(d, handle_level_irq);
> + else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> + irq_set_handler_locked(d, handle_edge_irq);
Shouldn't the handler be set before the IRQ is enabled?
> +static void tegra_gpio_irq_handler(struct irq_desc *desc)
> + for (i = 0; i < MAX_GPIO_PORTS; ++i)
> + port_map[i] = -1;
> +
> + for (i = 0; i < tgi->soc->nports; ++i) {
> + if (tgi->soc->port[i].cont_id == tg_cont->controller)
> + port_map[tgi->soc->port[i].port_index] = i;
> + }
I would have expected the code to use simple math here (iterate over all
ports in the controller) rather than creating some kind of
lookup/mapping table.
> + chained_irq_enter(chip, desc);
> + for (i = 0; i < MAX_GPIO_PORTS; i++) {
> + port = port_map[i];
> + if (port == -1)
> + continue;
> +
> + addr = tgi->soc->port[port].reg_offset;
> + val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
> + GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
> + gpio = tgi->gc.base + (port * 8);
> + for_each_set_bit(pin, &val, 8)
> + generic_handle_irq(gpio_to_irq(gpio + pin));
For edge-sensitive IRQs, doesn't the status need to be cleared before
calling the handler, so that (a) the latched status is cleared, (b) if a
new edge occurs after this point, that fact is recorded and the new IRQ
handled?
> +#ifdef CONFIG_DEBUG_FS
Using a regmap might give you some of the debug logic for free.
> +static int tegra_gpio_probe(struct platform_device *pdev)
> +{
> + struct tegra_gpio_info *tgi;
> + struct resource *res;
> + int bank;
> + int gpio;
> + int ret;
> +
> + for (bank = 0;; bank++) {
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
> + if (!res)
> + break;
> + }
> + if (!bank) {
> + dev_err(&pdev->dev, "No GPIO Controller found\n");
> + return -ENODEV;
> + }
...
> + tgi->nbanks = bank;
There should be a fixed number of IRQs in DT based on the controller
definition; the value shouldn't be variable.
See Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
The U-Boot Tegra186 GPIO driver might also be useful as a reference to
how to use the DT binding and structure the driver:
> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/gpio/tegra186_gpio.c;h=1c681514db9f50e61a9db041ac2067c084db494c;hb=HEAD
> + tgi->gc.ngpio = tgi->soc->nports * 8;
This will leave some gaps in the GPIO numbering, since not all ports
have 8 GPIOs. I think this is the correct thing to do, but IIRC Thierry
found this caused some issues in the GPIO core since it attempts to
query initial status of each GPIO. Did you see this issue during testing?
> +static int __init tegra_gpio_init(void)
> +{
> + return platform_driver_register(&tegra_gpio_driver);
> +}
> +postcore_initcall(tegra_gpio_init);
I would have expected everything to use module_initcall() these days.
On Tue, Nov 08, 2016 at 12:07:55PM -0700, Stephen Warren wrote:
> On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> > Add GPIO driver for T186 based platforms.
> > Adds support for MAIN and AON GPIO's from T186.
>
> I'm not sure how you/Thierry will approach merging this with the other GPIO
> driver he has, but here's a very quick review of this one in case it's
> useful.
This puts me in an unfortunate situation. I'd really like to avoid being
the maintainer for this driver, but on the other hand, the version of
the driver that I wrote is pretty much what we'd end up if Stephen's
comments were all addressed. Suresh's driver does a couple of things in
addition (like the accessibility checks), but then I find my driver to
be more easily related to the TRM because it uses the register names
from that.
So I don't really know how to go about merging both. I'll reply to this
email later with a copy of the patch that I wrote, maybe we can take it
from there.
> > + tgi->gc.ngpio = tgi->soc->nports * 8;
>
> This will leave some gaps in the GPIO numbering, since not all ports have 8
> GPIOs. I think this is the correct thing to do, but IIRC Thierry found this
> caused some issues in the GPIO core since it attempts to query initial
> status of each GPIO. Did you see this issue during testing?
I think the immediate issue that I was seeing is avoided in this driver
by the call to gpio_is_accessible() in the ->get_direction() callback.
In the driver that I have there's no such check, and hence I would get
an exception on probe.
However there's another problem with this patch. If you try and export
a non-existing GPIO via sysfs and try to read the value file you can
easily make the driver hang a CPU. This only seems to happen for the
AON GPIO controller.
The approach that I chose was to compact the range of GPIOs that the
GPIO subsystem knows about to the ones that actually exist. This has the
slight disadvantage that we can't use a simple port * 8 + offset to
compute the pin number anymore. However for the primary use-case (GPIO
specifier in DT) that's not a problem because we can translate the pin
number into the compacted space. That means the only issue will be with
sysfs support because if we use the simple formula we'll eventually get
a pin number that's outside of the range.
One way to solve this is to make a massive change to the GPIO subsystem
to check for the validity of a GPIO before any access. I'm not sure if
that's desirable, maybe Linus has some thoughts about that.
If we stick with a compacted number space, there are two solutions that
I can think of to remedy the sysfs problem. One would be to register a
separate struct gpio_chip for each controller. That's kind of a sledge-
hammer solution because it will create multiple number spaces and hence
completely avoid the sparse number space for the whole controller. I
think Stephen had originally proposed this as a solution.
The other possibility would be for the GPIO subsystem to gain per-chip
GPIO export via sysfs. That is, instead of the global export file that
you write a global GPIO number to, each per-chip directory would get
an export file. Values written into that file could get translated via
driver-specific callbacks (much like the ->xlate() callback for GPIO
specifiers). I think that's a change that makes sense anyway. Usually
users will know what GPIO controller they want to access and the offset
of the pin therein. Currently they have to somewhat jump through hoops
to get at the right pin (find controller, read GPIO base, add offset to
base and write that to the export file). The new sequence would be much
more straightforward: find controller, write offset to export file. The
new per-chip export file would be flexible enough to deal with compacted
number spaces, which is obviously something we can't do with the global
export file.
Thierry
From: Thierry Reding <[email protected]>
Tegra186 has two GPIO controllers that are largely register compatible
between one another but are completely different from the controller
found on earlier generations.
Signed-off-by: Thierry Reding <[email protected]>
---
drivers/gpio/Kconfig | 8 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-tegra186.c | 599 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 608 insertions(+)
create mode 100644 drivers/gpio/gpio-tegra186.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f327b1eddaa7..ff17580ae671 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -413,6 +413,14 @@ config GPIO_TEGRA
help
Say yes here to support GPIO pins on NVIDIA Tegra SoCs.
+config GPIO_TEGRA186
+ tristate "NVIDIA Tegra186 GPIO support"
+ default ARCH_TEGRA_186_SOC
+ depends on ARCH_TEGRA_186_SOC || COMPILE_TEST
+ depends on OF_GPIO
+ help
+ Say yes here to support GPIO pins on NVIDIA Tegra186 SoCs.
+
config GPIO_TS4800
tristate "TS-4800 DIO blocks and compatibles"
depends on OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b82de6f..bf9486c5f05f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_GPIO_SYSCON) += gpio-syscon.o
obj-$(CONFIG_GPIO_TB10X) += gpio-tb10x.o
obj-$(CONFIG_GPIO_TC3589X) += gpio-tc3589x.o
obj-$(CONFIG_GPIO_TEGRA) += gpio-tegra.o
+obj-$(CONFIG_GPIO_TEGRA186) += gpio-tegra186.o
obj-$(CONFIG_GPIO_TIMBERDALE) += gpio-timberdale.o
obj-$(CONFIG_GPIO_PALMAS) += gpio-palmas.o
obj-$(CONFIG_GPIO_TPIC2810) += gpio-tpic2810.o
diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
new file mode 100644
index 000000000000..b79156ef6dd0
--- /dev/null
+++ b/drivers/gpio/gpio-tegra186.c
@@ -0,0 +1,599 @@
+/*
+ * Copyright (c) 2016 NVIDIA Corporation
+ *
+ * Author: Thierry Reding <[email protected]>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/gpio/tegra186-gpio.h>
+
+#define TEGRA186_GPIO_ENABLE_CONFIG 0x00
+#define TEGRA186_GPIO_ENABLE_CONFIG_ENABLE BIT(0)
+#define TEGRA186_GPIO_ENABLE_CONFIG_OUT BIT(1)
+#define TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_NONE (0x0 << 2)
+#define TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL (0x1 << 2)
+#define TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE (0x2 << 2)
+#define TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_DOUBLE_EDGE (0x3 << 2)
+#define TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK (0x3 << 2)
+#define TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL BIT(4)
+#define TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT BIT(6)
+
+#define TEGRA186_GPIO_DEBOUNCE_CONTROL 0x04
+#define TEGRA186_GPIO_DEBOUNCE_CONTROL_THRESHOLD(x) ((x) & 0xff)
+
+#define TEGRA186_GPIO_INPUT 0x08
+#define TEGRA186_GPIO_INPUT_HIGH BIT(0)
+
+#define TEGRA186_GPIO_OUTPUT_CONTROL 0x0c
+#define TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED BIT(0)
+
+#define TEGRA186_GPIO_OUTPUT_VALUE 0x10
+#define TEGRA186_GPIO_OUTPUT_VALUE_HIGH BIT(0)
+
+#define TEGRA186_GPIO_INTERRUPT_CLEAR 0x14
+
+#define TEGRA186_GPIO_INTERRUPT_STATUS(x) (0x100 + (x) * 4)
+
+struct tegra_gpio_port {
+ unsigned int offset;
+ unsigned int pins;
+};
+
+struct tegra_gpio_soc {
+ const struct tegra_gpio_port *ports;
+ unsigned int num_ports;
+};
+
+struct tegra_gpio {
+ struct gpio_chip gpio;
+ struct irq_chip intc;
+ unsigned int num_irq;
+ unsigned int *irq;
+
+ const struct tegra_gpio_soc *soc;
+
+ void __iomem *base;
+
+ struct irq_domain *domain;
+};
+
+static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip)
+{
+ return container_of(chip, struct tegra_gpio, gpio);
+}
+
+static const struct tegra_gpio_port *
+tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
+{
+ unsigned int start = 0, i;
+
+ for (i = 0; i < gpio->soc->num_ports; i++) {
+ const struct tegra_gpio_port *port = &gpio->soc->ports[i];
+
+ if (*pin >= start && *pin < start + port->pins) {
+ *pin -= start;
+ return port;
+ }
+
+ start += port->pins;
+ }
+
+ return NULL;
+}
+
+static void __iomem *tegra186_gpio_get_base(struct tegra_gpio *gpio,
+ unsigned int pin)
+{
+ const struct tegra_gpio_port *port;
+
+ port = tegra186_gpio_get_port(gpio, &pin);
+ if (!port)
+ return NULL;
+
+ return gpio->base + port->offset + pin * 0x20;
+}
+
+static int tegra186_gpio_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, offset);
+ if (WARN_ON(base == NULL))
+ return -ENODEV;
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ if (value & TEGRA186_GPIO_ENABLE_CONFIG_OUT)
+ return GPIOF_DIR_OUT;
+
+ return GPIOF_DIR_IN;
+}
+
+static int tegra186_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, offset);
+ if (WARN_ON(base == NULL))
+ return -ENODEV;
+
+ value = readl(base + TEGRA186_GPIO_OUTPUT_CONTROL);
+ value |= TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED;
+ writel(value, base + TEGRA186_GPIO_OUTPUT_CONTROL);
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_ENABLE;
+ value &= ~TEGRA186_GPIO_ENABLE_CONFIG_OUT;
+ writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+
+ return 0;
+}
+
+static int tegra186_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int level)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+ void __iomem *base;
+ u32 value;
+
+ /* configure output level first */
+ chip->set(chip, offset, level);
+
+ base = tegra186_gpio_get_base(gpio, offset);
+ if (WARN_ON(base == NULL))
+ return -EINVAL;
+
+ /* set the direction */
+ value = readl(base + TEGRA186_GPIO_OUTPUT_CONTROL);
+ value &= ~TEGRA186_GPIO_OUTPUT_CONTROL_FLOATED;
+ writel(value, base + TEGRA186_GPIO_OUTPUT_CONTROL);
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_ENABLE;
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_OUT;
+ writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+
+ return 0;
+}
+
+static int tegra186_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, offset);
+ if (WARN_ON(base == NULL))
+ return -ENODEV;
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ if (value & TEGRA186_GPIO_ENABLE_CONFIG_OUT)
+ value = readl(base + TEGRA186_GPIO_OUTPUT_VALUE);
+ else
+ value = readl(base + TEGRA186_GPIO_INPUT);
+
+ return value & BIT(0);
+}
+
+static void tegra186_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int level)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, offset);
+ if (WARN_ON(base == NULL))
+ return;
+
+ value = readl(base + TEGRA186_GPIO_OUTPUT_VALUE);
+ if (level == 0)
+ value &= ~TEGRA186_GPIO_OUTPUT_VALUE_HIGH;
+ else
+ value |= TEGRA186_GPIO_OUTPUT_VALUE_HIGH;
+
+ writel(value, base + TEGRA186_GPIO_OUTPUT_VALUE);
+}
+
+static int tegra186_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+
+ return irq_find_mapping(gpio->domain, offset);
+}
+
+static int tegra186_gpio_of_xlate(struct gpio_chip *chip,
+ const struct of_phandle_args *spec,
+ u32 *flags)
+{
+ struct tegra_gpio *gpio = to_tegra_gpio(chip);
+ unsigned int port, pin, i, offset = 0;
+
+ if (WARN_ON(chip->of_gpio_n_cells < 2))
+ return -EINVAL;
+
+ if (WARN_ON(spec->args_count < chip->of_gpio_n_cells))
+ return -EINVAL;
+
+ port = spec->args[0] / 8;
+ pin = spec->args[0] % 8;
+
+ if (port >= gpio->soc->num_ports) {
+ dev_err(chip->parent, "invalid port number: %u\n", port);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < port; i++)
+ offset += gpio->soc->ports[i].pins;
+
+ if (flags)
+ *flags = spec->args[1];
+
+ return offset + pin;
+}
+
+static void tegra186_irq_ack(struct irq_data *data)
+{
+ struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+ void __iomem *base;
+
+ base = tegra186_gpio_get_base(gpio, data->hwirq);
+ if (WARN_ON(base == NULL))
+ return;
+
+ writel(1, base + TEGRA186_GPIO_INTERRUPT_CLEAR);
+}
+
+static void tegra186_irq_mask(struct irq_data *data)
+{
+ struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, data->hwirq);
+ if (WARN_ON(base == NULL))
+ return;
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ value &= ~TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT;
+ writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+}
+
+static void tegra186_irq_unmask(struct irq_data *data)
+{
+ struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, data->hwirq);
+ if (WARN_ON(base == NULL))
+ return;
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_INTERRUPT;
+ writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+}
+
+static int tegra186_irq_set_type(struct irq_data *data, unsigned int flow)
+{
+ struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
+ void __iomem *base;
+ u32 value;
+
+ base = tegra186_gpio_get_base(gpio, data->hwirq);
+ if (WARN_ON(base == NULL))
+ return -ENODEV;
+
+ value = readl(base + TEGRA186_GPIO_ENABLE_CONFIG);
+ value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK;
+ value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
+
+ switch (flow & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_NONE:
+ break;
+
+ case IRQ_TYPE_EDGE_RISING:
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE;
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
+ break;
+
+ case IRQ_TYPE_EDGE_FALLING:
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_SINGLE_EDGE;
+ break;
+
+ case IRQ_TYPE_EDGE_BOTH:
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_DOUBLE_EDGE;
+ break;
+
+ case IRQ_TYPE_LEVEL_HIGH:
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL;
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ value |= TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_LEVEL;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
+
+ if ((flow & IRQ_TYPE_EDGE_BOTH) == 0)
+ irq_set_handler_locked(data, handle_level_irq);
+ else
+ irq_set_handler_locked(data, handle_edge_irq);
+
+ return 0;
+}
+
+static void tegra186_gpio_irq(struct irq_desc *desc)
+{
+ struct tegra_gpio *gpio = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ unsigned int i, offset = 0;
+
+ chained_irq_enter(chip, desc);
+
+ for (i = 0; i < gpio->soc->num_ports; i++) {
+ const struct tegra_gpio_port *port = &gpio->soc->ports[i];
+ void __iomem *base = gpio->base + port->offset;
+ unsigned int pin, irq;
+ unsigned long value;
+
+ value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
+
+ for_each_set_bit(pin, &value, port->pins) {
+ irq = irq_find_mapping(gpio->domain, offset + pin);
+ if (WARN_ON(irq == 0))
+ continue;
+
+ generic_handle_irq(irq);
+ }
+
+ offset += port->pins;
+ }
+
+ chained_irq_exit(chip, desc);
+}
+
+static int tegra186_gpio_irq_domain_xlate(struct irq_domain *domain,
+ struct device_node *np,
+ const u32 *spec, unsigned int size,
+ unsigned long *hwirq,
+ unsigned int *type)
+{
+ struct tegra_gpio *gpio = domain->host_data;
+ unsigned int port, pin, i, offset = 0;
+
+ if (size < 2)
+ return -EINVAL;
+
+ port = spec[0] / 8;
+ pin = spec[0] % 8;
+
+ if (port >= gpio->soc->num_ports) {
+ dev_err(gpio->gpio.parent, "invalid port number: %u\n", port);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < port; i++)
+ offset += gpio->soc->ports[i].pins;
+
+ *type = spec[1] & IRQ_TYPE_SENSE_MASK;
+ *hwirq = offset + pin;
+
+ return 0;
+}
+
+static const struct irq_domain_ops tegra186_gpio_irq_domain_ops = {
+ .xlate = tegra186_gpio_irq_domain_xlate,
+};
+
+static struct lock_class_key tegra186_gpio_lock_class;
+
+static int tegra186_gpio_probe(struct platform_device *pdev)
+{
+ struct tegra_gpio *gpio;
+ struct resource *res;
+ unsigned int i, irq;
+ int err;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->soc = of_device_get_match_data(&pdev->dev);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpio");
+ gpio->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ err = of_irq_count(pdev->dev.of_node);
+ if (err < 0)
+ return err;
+
+ gpio->num_irq = err;
+
+ gpio->irq = devm_kcalloc(&pdev->dev, gpio->num_irq, sizeof(*gpio->irq),
+ GFP_KERNEL);
+ if (!gpio->irq)
+ return -ENOMEM;
+
+ for (i = 0; i < gpio->num_irq; i++) {
+ err = platform_get_irq(pdev, i);
+ if (err < 0)
+ return err;
+
+ gpio->irq[i] = err;
+ }
+
+ gpio->gpio.label = "tegra186-gpio";
+ gpio->gpio.parent = &pdev->dev;
+
+ gpio->gpio.get_direction = tegra186_gpio_get_direction;
+ gpio->gpio.direction_input = tegra186_gpio_direction_input;
+ gpio->gpio.direction_output = tegra186_gpio_direction_output;
+ gpio->gpio.get = tegra186_gpio_get,
+ gpio->gpio.set = tegra186_gpio_set;
+ gpio->gpio.to_irq = tegra186_gpio_to_irq;
+
+ gpio->gpio.base = -1;
+
+ for (i = 0; i < gpio->soc->num_ports; i++)
+ gpio->gpio.ngpio += gpio->soc->ports[i].pins;
+
+ gpio->gpio.of_node = pdev->dev.of_node;
+ gpio->gpio.of_gpio_n_cells = 2;
+ gpio->gpio.of_xlate = tegra186_gpio_of_xlate;
+
+ gpio->intc.name = pdev->dev.of_node->name;
+ gpio->intc.irq_ack = tegra186_irq_ack;
+ gpio->intc.irq_mask = tegra186_irq_mask;
+ gpio->intc.irq_unmask = tegra186_irq_unmask;
+ gpio->intc.irq_set_type = tegra186_irq_set_type;
+
+ platform_set_drvdata(pdev, gpio);
+
+ err = devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
+ if (err < 0)
+ return err;
+
+ gpio->domain = irq_domain_add_linear(pdev->dev.of_node,
+ gpio->gpio.ngpio,
+ &tegra186_gpio_irq_domain_ops,
+ gpio);
+ if (!gpio->domain)
+ return -ENODEV;
+
+ for (i = 0; i < gpio->gpio.ngpio; i++) {
+ irq = irq_create_mapping(gpio->domain, i);
+ if (irq == 0) {
+ dev_err(&pdev->dev,
+ "failed to create IRQ mapping for GPIO#%u\n",
+ i);
+ continue;
+ }
+
+ irq_set_lockdep_class(irq, &tegra186_gpio_lock_class);
+ irq_set_chip_data(irq, gpio);
+ irq_set_chip_and_handler(irq, &gpio->intc, handle_simple_irq);
+ }
+
+ for (i = 0; i < gpio->num_irq; i++)
+ irq_set_chained_handler_and_data(gpio->irq[i],
+ tegra186_gpio_irq,
+ gpio);
+
+ return 0;
+}
+
+static int tegra186_gpio_remove(struct platform_device *pdev)
+{
+ struct tegra_gpio *gpio = platform_get_drvdata(pdev);
+ unsigned int i, irq;
+
+ for (i = 0; i < gpio->num_irq; i++)
+ irq_set_chained_handler_and_data(gpio->irq[i], NULL, NULL);
+
+ for (i = 0; i < gpio->gpio.ngpio; i++) {
+ irq = irq_find_mapping(gpio->domain, i);
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(gpio->domain);
+
+ return 0;
+}
+
+static const struct tegra_gpio_port tegra186_main_ports[] = {
+ [TEGRA_MAIN_GPIO_PORT_A] = { 0x2000, 7 },
+ [TEGRA_MAIN_GPIO_PORT_B] = { 0x3000, 7 },
+ [TEGRA_MAIN_GPIO_PORT_C] = { 0x3200, 7 },
+ [TEGRA_MAIN_GPIO_PORT_D] = { 0x3400, 6 },
+ [TEGRA_MAIN_GPIO_PORT_E] = { 0x2200, 8 },
+ [TEGRA_MAIN_GPIO_PORT_F] = { 0x2400, 6 },
+ [TEGRA_MAIN_GPIO_PORT_G] = { 0x4200, 6 },
+ [TEGRA_MAIN_GPIO_PORT_H] = { 0x1000, 7 },
+ [TEGRA_MAIN_GPIO_PORT_I] = { 0x0800, 8 },
+ [TEGRA_MAIN_GPIO_PORT_J] = { 0x5000, 8 },
+ [TEGRA_MAIN_GPIO_PORT_K] = { 0x5200, 1 },
+ [TEGRA_MAIN_GPIO_PORT_L] = { 0x1200, 8 },
+ [TEGRA_MAIN_GPIO_PORT_M] = { 0x5600, 6 },
+ [TEGRA_MAIN_GPIO_PORT_N] = { 0x0000, 7 },
+ [TEGRA_MAIN_GPIO_PORT_O] = { 0x0200, 4 },
+ [TEGRA_MAIN_GPIO_PORT_P] = { 0x4000, 7 },
+ [TEGRA_MAIN_GPIO_PORT_Q] = { 0x0400, 6 },
+ [TEGRA_MAIN_GPIO_PORT_R] = { 0x0a00, 6 },
+ [TEGRA_MAIN_GPIO_PORT_T] = { 0x0600, 4 },
+ [TEGRA_MAIN_GPIO_PORT_X] = { 0x1400, 8 },
+ [TEGRA_MAIN_GPIO_PORT_Y] = { 0x1600, 7 },
+ [TEGRA_MAIN_GPIO_PORT_BB] = { 0x2600, 2 },
+ [TEGRA_MAIN_GPIO_PORT_CC] = { 0x5400, 4 },
+};
+
+static const struct tegra_gpio_soc tegra186_main_soc = {
+ .num_ports = ARRAY_SIZE(tegra186_main_ports),
+ .ports = tegra186_main_ports,
+};
+
+static const struct tegra_gpio_port tegra186_aon_ports[] = {
+ [TEGRA_AON_GPIO_PORT_S] = { 0x0200, 5 },
+ [TEGRA_AON_GPIO_PORT_U] = { 0x0400, 6 },
+ [TEGRA_AON_GPIO_PORT_V] = { 0x0800, 8 },
+ [TEGRA_AON_GPIO_PORT_W] = { 0x0a00, 8 },
+ [TEGRA_AON_GPIO_PORT_Z] = { 0x0e00, 4 },
+ [TEGRA_AON_GPIO_PORT_AA] = { 0x0c00, 8 },
+ [TEGRA_AON_GPIO_PORT_EE] = { 0x0600, 3 },
+ [TEGRA_AON_GPIO_PORT_FF] = { 0x0000, 5 },
+};
+
+static const struct tegra_gpio_soc tegra186_aon_soc = {
+ .num_ports = ARRAY_SIZE(tegra186_aon_ports),
+ .ports = tegra186_aon_ports,
+};
+
+static const struct of_device_id tegra186_gpio_of_match[] = {
+ {
+ .compatible = "nvidia,tegra186-gpio",
+ .data = &tegra186_main_soc
+ }, {
+ .compatible = "nvidia,tegra186-gpio-aon",
+ .data = &tegra186_aon_soc
+ }, {
+ /* sentinel */
+ }
+};
+
+static struct platform_driver tegra186_gpio_driver = {
+ .driver = {
+ .name = "tegra186-gpio",
+ .of_match_table = tegra186_gpio_of_match,
+ },
+ .probe = tegra186_gpio_probe,
+ .remove = tegra186_gpio_remove,
+};
+module_platform_driver(tegra186_gpio_driver);
+
+MODULE_DESCRIPTION("NVIDIA Tegra186 GPIO controller driver");
+MODULE_AUTHOR("Thierry Reding <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.10.2
On Tue, Nov 22, 2016 at 6:30 PM, Thierry Reding
<[email protected]> wrote:
> So I don't really know how to go about merging both. I'll reply to this
> email later with a copy of the patch that I wrote, maybe we can take it
> from there.
I trust you nVidia people to sort this out.
I guess in worst case you can have a company strategy meeting about it.
Let me just say: when one of you have a patch that bears the ACK of the
other, I will merge it.
> However there's another problem with this patch. If you try and export
> a non-existing GPIO via sysfs and try to read the value file you can
> easily make the driver hang a CPU. This only seems to happen for the
> AON GPIO controller.
That sounds like a bug. But I strongly suggest you first and foremost
to test your code with the character device and not through sysfs.
sysfs is second priority, and while I don't want it to screw things up, it
is an optional legacy bolt-on not a compiled-in recommended thing.
The character device, on the other hand, is a recommended
practice for userspace GPIO.
> One way to solve this is to make a massive change to the GPIO subsystem
> to check for the validity of a GPIO before any access. I'm not sure if
> that's desirable, maybe Linus has some thoughts about that.
This is already possible and several drivers are doing this.
Everything, all kernel users and all character device users, end up
calling gpiod_request().
We agree violently that if this GPIO is not valid, inaccessible (etc)
it should not return a valid GPIO descriptor.
Consider drivers/gpio/gpio-stmpe.c which has a device tree property
"st,norequest-mask" that mark some GPIO lines as "nonusable".
This is because they are statically muxed for something else.
(It is a subject of debate whether that is a good way to mark the
lines as unusable, probably not, but it is legacy code.)
We know a number of lines are not elegible for request
or to be used for triggering interrupts.
The code does this in .request():
if (stmpe_gpio->norequest_mask & BIT(offset))
return -EINVAL;
Thus any gpiod_get() will fail. And we are fine.
Further, since it can also be used as an interrupt parent, it
does this in .probe():
of_property_read_u32(np, "st,norequest-mask",
&stmpe_gpio->norequest_mask);
if (stmpe_gpio->norequest_mask)
stmpe_gpio->chip.irq_need_valid_mask = true;
for (i = 0; i < sizeof(u32); i++)
if (stmpe_gpio->norequest_mask & BIT(i))
clear_bit(i, stmpe_gpio->chip.irq_valid_mask);
How this blocks the IRQs from being requested can be seen
in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
gracefully handles this too.
If the sysfs ABI does not block the use of the same lines
as efficiently, it is a bug in the sysfs code. This works fine
to block access for all in-kernel and chardev users.
But as far as I can tell, sysfs ALSO uses gpiod_get() so it should
work fine.
> If we stick with a compacted number space, there are two solutions that
> I can think of to remedy the sysfs problem. One would be to register a
> separate struct gpio_chip for each controller. That's kind of a sledge-
> hammer solution because it will create multiple number spaces and hence
> completely avoid the sparse number space for the whole controller. I
> think Stephen had originally proposed this as a solution.
Note ambigous use of "controller" above. I'm confused.
"One would be to register a separate struct gpio_chip for each controller."
/ "and hence completely avoid the sparse number space for the whole
controller."
Eheheh
It seems that "controller" refer to two different things in the two
sentences. Do you mean your hardware has ONE controller with
several BANKS? (as described in Documentation/gpio/driver.txt?)
> The other possibility would be for the GPIO subsystem to gain per-chip
> GPIO export via sysfs. That is, instead of the global export file that
> you write a global GPIO number to, each per-chip directory would get
> an export file.
No. The sysfs ABI is in
Documentation/ABI/obsolete/sysfs-gpio
for a reason: I hate it and it should not be extended whatsoever.
Use the new character device, and for a new SoC like the tegra186
you can CERTAINLY convince any downstream consumers to
switch to that.
Please just disable CONFIG_GPIO_SYSFS from your defconfig
and in any company-internal builds and point everyone and their
dog to the character device: tools/gpio/*,
and the UAPI <linux/gpio.h>
Yours,
Linus Walleij
On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
<[email protected]> wrote:
> From: Thierry Reding <[email protected]>
>
> Tegra186 has two GPIO controllers that are largely register compatible
> between one another but are completely different from the controller
> found on earlier generations.
>
> Signed-off-by: Thierry Reding <[email protected]>
I would prefer if you could try to convert this driver to use
CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
It would save us so much trouble and so much complicated
code to maintain for this custom irqdomain.
I suggest you to look into the mechanisms mentioned in my
previous mail for how to poke holes in the single linear
irqdomain used by this mechanism.
As it seems, you only have one parent interrupt with all
these IRQs cascading off it as far as I can tell.
Yours,
Linus Walleij
On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
> <[email protected]> wrote:
>
> > From: Thierry Reding <[email protected]>
> >
> > Tegra186 has two GPIO controllers that are largely register compatible
> > between one another but are completely different from the controller
> > found on earlier generations.
> >
> > Signed-off-by: Thierry Reding <[email protected]>
>
> I would prefer if you could try to convert this driver to use
> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
> It would save us so much trouble and so much complicated
> code to maintain for this custom irqdomain.
>
> I suggest you to look into the mechanisms mentioned in my
> previous mail for how to poke holes in the single linear
> irqdomain used by this mechanism.
>
> As it seems, you only have one parent interrupt with all
> these IRQs cascading off it as far as I can tell.
Like I said in other email, I don't think this will work because the
GPIOLIB_IRQCHIP support seems to be limited to cases where a single
parent interrupt is used. We could possibly live with a single IRQ
handler and data, but we definitely need to request multiple IRQs if
we want to be able to use all GPIOs as interrupts.
Thierry
On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2016 at 6:30 PM, Thierry Reding
> <[email protected]> wrote:
>
> > So I don't really know how to go about merging both. I'll reply to this
> > email later with a copy of the patch that I wrote, maybe we can take it
> > from there.
>
> I trust you nVidia people to sort this out.
>
> I guess in worst case you can have a company strategy meeting about it.
>
> Let me just say: when one of you have a patch that bears the ACK of the
> other, I will merge it.
>
> > However there's another problem with this patch. If you try and export
> > a non-existing GPIO via sysfs and try to read the value file you can
> > easily make the driver hang a CPU. This only seems to happen for the
> > AON GPIO controller.
>
> That sounds like a bug. But I strongly suggest you first and foremost
> to test your code with the character device and not through sysfs.
>
> sysfs is second priority, and while I don't want it to screw things up, it
> is an optional legacy bolt-on not a compiled-in recommended thing.
>
> The character device, on the other hand, is a recommended
> practice for userspace GPIO.
The root cause here would be that we don't implement .request(), hence
we're not actually preventing access to the non-existing GPIOs.
> > One way to solve this is to make a massive change to the GPIO subsystem
> > to check for the validity of a GPIO before any access. I'm not sure if
> > that's desirable, maybe Linus has some thoughts about that.
>
> This is already possible and several drivers are doing this.
>
> Everything, all kernel users and all character device users, end up
> calling gpiod_request().
It looks like I stumbled across the only case where this isn't true.
What I was seeing, and which ultimately led me to implement the compact
numberspace is that gpiochip_add_data() calls ->get_direction() directly
without first going through ->request(). We'd have to guard that one
case as well in order for this to work.
> We agree violently that if this GPIO is not valid, inaccessible (etc)
> it should not return a valid GPIO descriptor.
Yes.
> Consider drivers/gpio/gpio-stmpe.c which has a device tree property
> "st,norequest-mask" that mark some GPIO lines as "nonusable".
> This is because they are statically muxed for something else.
>
> (It is a subject of debate whether that is a good way to mark the
> lines as unusable, probably not, but it is legacy code.)
>
> We know a number of lines are not elegible for request
> or to be used for triggering interrupts.
>
> The code does this in .request():
>
> if (stmpe_gpio->norequest_mask & BIT(offset))
> return -EINVAL;
>
> Thus any gpiod_get() will fail. And we are fine.
>
> Further, since it can also be used as an interrupt parent, it
> does this in .probe():
>
> of_property_read_u32(np, "st,norequest-mask",
> &stmpe_gpio->norequest_mask);
>
> if (stmpe_gpio->norequest_mask)
> stmpe_gpio->chip.irq_need_valid_mask = true;
> for (i = 0; i < sizeof(u32); i++)
> if (stmpe_gpio->norequest_mask & BIT(i))
> clear_bit(i, stmpe_gpio->chip.irq_valid_mask);
>
> How this blocks the IRQs from being requested can be seen
> in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
> gracefully handles this too.
I don't think it does. The issue I mentioned for gpiochip_add_data()
exists in gpiochip_lock_as_irq(). That code assumes that all GPIOs from
0 to chip.ngpio - 1 are valid. And perhaps that's exactly how it should
be. But that's not true in this version of the driver. Perhaps I should
clarify what exactly we're talking about here, because I'm not sure it's
obvious.
On Tegra186 there are two "controllers" (perhaps I should say hardware
blocks) that provide GPIOs. One is in the always-on partition of the
chip and is therefore typically called "AON", whereas the other is
called "main". Both of these hardware blocks implement a number of
"controllers": 1 for AON and 6 for main. This is really only apparent in
the number of interrupts that they receive. AON has one interrupt line
whereas main has 6 of them.
Each hardware block also implements a number of "ports": 8 for AON and
23 for main. Each of these ports has a different number of pins. Often
there will be 6, 7 or 8, but a couple of ports have as little as a
single pin. Each port is associated with a given "controller", that is
interrupt, so when a GPIO of a port is configured as input and enabled
to generate an interrupt, the interrupt of it's parent controller will
be asserted. I'm not exactly sure how this association is done, I have
not found anything in the TRM. Perhaps Laxman, Suresh or Stephen can
clarify. Looking at Suresh's patch there's no clear pattern to it, so I
guess I just missed the table that has this information. In my driver I
chose the easy route and just requested each interrupt with the same
handler, which means we potentially waste some time by reading some
interrupt status registers that we don't have to. This could be made
cleverer by filtering for those that match the controller whose
interrupt we're handling.
Anyway, to get back to what we're really talking about here: Suresh's
patch registers 8 pins per port, regardless of the actual number of pins
that the port has. This simplifies things in some ways. For example the
numbering in DT (see include/dt-bindings/gpio/tegra186-gpio.h) is the
same as gpiolib's internal numbering. Which means that everyone can just
use the index of the desired port, multiply it by 8 and add the pin
index to get at the GPIO number relative to the GPIO chip. The downside
is that we make gpiolib believe that there are more GPIOs in the chip
than actually exist. It's not so much that these pins aren't accessible
because they are used for other purposes (that's something that could
happen in addition) but because they simply don't exist.
With the AON controller this seems critical because it will hang a CPU
(though I've also seen exceptions rather than hangs) if registers that
belong to these non-existing GPIOs are accessed (presumably there are
hardware checks for registers that don't exist).
> If the sysfs ABI does not block the use of the same lines
> as efficiently, it is a bug in the sysfs code. This works fine
> to block access for all in-kernel and chardev users.
>
> But as far as I can tell, sysfs ALSO uses gpiod_get() so it should
> work fine.
Yeah, I think the implementation is fine, it's just that it relies on a
mechanism that this driver doesn't implement.
> > If we stick with a compacted number space, there are two solutions that
> > I can think of to remedy the sysfs problem. One would be to register a
> > separate struct gpio_chip for each controller. That's kind of a sledge-
> > hammer solution because it will create multiple number spaces and hence
> > completely avoid the sparse number space for the whole controller. I
> > think Stephen had originally proposed this as a solution.
>
> Note ambigous use of "controller" above. I'm confused.
>
> "One would be to register a separate struct gpio_chip for each controller."
> / "and hence completely avoid the sparse number space for the whole
> controller."
>
> Eheheh
>
> It seems that "controller" refer to two different things in the two
> sentences. Do you mean your hardware has ONE controller with
> several BANKS? (as described in Documentation/gpio/driver.txt?)
I hope the above clarifies things. struct gpio_chip represents the
entire hardware block (in current drivers) and the driver deals with
individual controllers and ports internally. The proposal I was talking
about above is to have one driver create multiple struct gpio_chip, each
representing an individual port. Hence each struct gpio_chip would be
assigned the exact number of pins that the port supports, removing all
of the problems with numberspaces. It has its own set of disadvantages,
such as creating a large number of GPIO chips, which may in the end be
just as confusing as the current implementation.
> > The other possibility would be for the GPIO subsystem to gain per-chip
> > GPIO export via sysfs. That is, instead of the global export file that
> > you write a global GPIO number to, each per-chip directory would get
> > an export file.
>
> No. The sysfs ABI is in
> Documentation/ABI/obsolete/sysfs-gpio
> for a reason: I hate it and it should not be extended whatsoever.
>
> Use the new character device, and for a new SoC like the tegra186
> you can CERTAINLY convince any downstream consumers to
> switch to that.
I've looked a little at the character device implementation and I think
it would work equally bad with the compact numberspace as sysfs. The
reason is that gpiolib doesn't allow remapping of chip-relative indices
except for DT. I think this might be possible by generalizing the
->of_xlate() to be used in translating internal numbers as well. The way
I could imagine this to work is that an IOCTL providing offsets of the
lines to use would pass the offsets through the chip's ->xlate()
function to retrieve the index (0 to chip->ngpio). The alternative is to
stick with the sparse numberspace and deal with non-existing GPIOs via a
->request() callback.
> Please just disable CONFIG_GPIO_SYSFS from your defconfig
> and in any company-internal builds and point everyone and their
> dog to the character device: tools/gpio/*,
> and the UAPI <linux/gpio.h>
I don't think we can easily do that. People may still rely on the sysfs
interface, or even if they aren't this might be enabled in a multi-
platform image. So I think regardless of whether or not we like the
interface, we have to make sure that our solution plays nicely with it.
There is no problem with the compact numberspace, though it comes with
the inconvenience that numbering in sysfs is different from numbering in
DT.
In summary I think we have three options:
1) use a sparse numberspace and deal with non-existing GPIOs via the
->request() callback
2) use a compact numberspace and live with separate methods of
referencing GPIOs
3) use a compact numberspace and introduce a mechanism to translate
all hardware numbers into per-chip indices
I think 1) is the simplest, but at the cost of wasting memory and CPU
cycles by maintaining descriptors for GPIOs we know don't exist. 2) is
fairly simple as well, but it's pretty inconvenient for the user. The
most invasive solution would be 3), though I personally like that best
because it is the most explicit. It does have the disavantage of using
separate numbering for sysfs and everyone else, though. Unless you're
willing to make sysfs use per-chip export/unexport files and the
->xlate() callback.
Thierry
On Thursday 24 November 2016 01:10 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
>>
>> This is already possible and several drivers are doing this.
>>
>> Everything, all kernel users and all character device users, end up
>> calling gpiod_request().
> It looks like I stumbled across the only case where this isn't true.
> What I was seeing, and which ultimately led me to implement the compact
> numberspace is that gpiochip_add_data() calls ->get_direction() directly
> without first going through ->request(). We'd have to guard that one
> case as well in order for this to work.
>
In T186, we have 8 pins per PORT and for some of ports, all pins are not
available. Like Port A has 7 pins valid (0 to 6) and port E have 8 pins
(0 to 7). The great part is that each port has valid pins start from 0.
So just having the number of valid pins for each port as part of SOC
data will help to find out whether GPIO exist or not.
int port = GPIO_PORT(offset);
int pin = GPIO_PIN(offset);
if (pin >= tgi->soc->port[port].valid_pins)
return false;
Similar logic can be used for APIs which can get called without
gpio_request().
On Tuesday 22 November 2016 11:25 PM, Thierry Reding wrote:
> +static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct tegra_gpio, gpio);
> +}
You dont need this as gpiochip_get_data(chip); can provide the required
driver specific data.
> +
> +static const struct tegra_gpio_port *
> +tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
> +{
> + unsigned int start = 0, i;
> +
> + for (i = 0; i < gpio->soc->num_ports; i++) {
> + const struct tegra_gpio_port *port = &gpio->soc->ports[i];
> +
> + if (*pin >= start && *pin < start + port->pins) {
> + *pin -= start;
> + return port;
> + }
> +
> + start += port->pins;
> + }
> +
Why not get the port from pins and then calculate with fixed offset.
We will not need the loop if we know the port number.
>
> +
> +static int tegra186_gpio_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int level)
> +{
> + struct tegra_gpio *gpio = to_tegra_gpio(chip);
> + void __iomem *base;
> + u32 value;
> +
> + /* configure output level first */
> + chip->set(chip, offset, level);
We can directly call the apis instead of function pointer at this point.
>
> +
> +static struct lock_class_key tegra186_gpio_lock_class;
We will have two instance of the driver (normal and AON) and so this
will be shared between them.
Do we really support multiple instance with same variable?
> +
> + gpio->gpio.label = "tegra186-gpio";
Two instance will have same name. Better to say tegra186-gpio and
tegra186-gpio-aon.
> + gpio->gpio.parent = &pdev->dev;
> +
> + gpio->gpio.get_direction = tegra186_gpio_get_direction;
> + gpio->gpio.direction_input = tegra186_gpio_direction_input;
> + gpio->gpio.direction_output = tegra186_gpio_direction_output;
> + gpio->gpio.get = tegra186_gpio_get,
> + gpio->gpio.set = tegra186_gpio_set;
> + gpio->gpio.to_irq = tegra186_gpio_to_irq;
> +
> + gpio->gpio.base = -1;
> +
> + for (i = 0; i < gpio->soc->num_ports; i++)
> + gpio->gpio.ngpio += gpio->soc->ports[i].pins;
> +
Our DT binding does not say this. We assume that we have 8 gpios per
port. so this will not work at all.
>
> +
> +static const struct tegra_gpio_port tegra186_main_ports[] = {
> + [TEGRA_MAIN_GPIO_PORT_A] = { 0x2000, 7 },
Use C99 style initialization which is like
.offset =
.pins =
>
On Thu, Nov 24, 2016 at 12:23:56PM +0530, Laxman Dewangan wrote:
>
> On Tuesday 22 November 2016 11:25 PM, Thierry Reding wrote:
> > +static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip)
> > +{
> > + return container_of(chip, struct tegra_gpio, gpio);
> > +}
>
> You dont need this as gpiochip_get_data(chip); can provide the required
> driver specific data.
It's common practice to embed the struct gpio_chip within a driver-
specific structure, and it's equally common to use a container_of() to
get at the embedding structure.
> > +static const struct tegra_gpio_port *
> > +tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
> > +{
> > + unsigned int start = 0, i;
> > +
> > + for (i = 0; i < gpio->soc->num_ports; i++) {
> > + const struct tegra_gpio_port *port = &gpio->soc->ports[i];
> > +
> > + if (*pin >= start && *pin < start + port->pins) {
> > + *pin -= start;
> > + return port;
> > + }
> > +
> > + start += port->pins;
> > + }
> > +
> Why not get the port from pins and then calculate with fixed offset.
> We will not need the loop if we know the port number.
Because we don't know what the port is until we've determined that the
pin is within the range of the port. This function determines what port
a given pin is in, then returns the relative index of the pin within
that port.
> > +static int tegra186_gpio_direction_output(struct gpio_chip *chip,
> > + unsigned int offset, int level)
> > +{
> > + struct tegra_gpio *gpio = to_tegra_gpio(chip);
> > + void __iomem *base;
> > + u32 value;
> > +
> > + /* configure output level first */
> > + chip->set(chip, offset, level);
> We can directly call the apis instead of function pointer at this point.
That would mean reshuffling the functions or having an unneeded forward
declaration of tegra186_gpio_set().
> > +static struct lock_class_key tegra186_gpio_lock_class;
> We will have two instance of the driver (normal and AON) and so this will be
> shared between them.
> Do we really support multiple instance with same variable?
As the type and name indicate this is to track a specific class of lock.
It's fine to use a single variable with multiple instances. It is in
fact an error to try and make these per-instance.
> > + gpio->gpio.label = "tegra186-gpio";
> Two instance will have same name. Better to say tegra186-gpio and
> tegra186-gpio-aon.
Good point. I suppose we could either add this to struct tegra_gpio_soc
or simply use the device tree node name.
> > + gpio->gpio.parent = &pdev->dev;
> > +
> > + gpio->gpio.get_direction = tegra186_gpio_get_direction;
> > + gpio->gpio.direction_input = tegra186_gpio_direction_input;
> > + gpio->gpio.direction_output = tegra186_gpio_direction_output;
> > + gpio->gpio.get = tegra186_gpio_get,
> > + gpio->gpio.set = tegra186_gpio_set;
> > + gpio->gpio.to_irq = tegra186_gpio_to_irq;
> > +
> > + gpio->gpio.base = -1;
> > +
> > + for (i = 0; i < gpio->soc->num_ports; i++)
> > + gpio->gpio.ngpio += gpio->soc->ports[i].pins;
> > +
>
> Our DT binding does not say this. We assume that we have 8 gpios per port.
> so this will not work at all.
This has nothing to do with the device tree binding. What the device
tree binding defines is the indices to use to obtain a given GPIO within
a given port. What numbering the driver uses internally is completely up
to the driver implementation.
Oh, and the above works just fine.
> > +static const struct tegra_gpio_port tegra186_main_ports[] = {
> > + [TEGRA_MAIN_GPIO_PORT_A] = { 0x2000, 7 },
> Use C99 style initialization which is like
> .offset =
> .pins =
I suppose I could do that.
Thanks,
Thierry
On Thursday 24 November 2016 08:14 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Nov 24, 2016 at 12:23:56PM +0530, Laxman Dewangan wrote:
>> On Tuesday 22 November 2016 11:25 PM, Thierry Reding wrote:
>>> +static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip)
>>> +{
>>> + return container_of(chip, struct tegra_gpio, gpio);
>>> +}
>> You dont need this as gpiochip_get_data(chip); can provide the required
>> driver specific data.
> It's common practice to embed the struct gpio_chip within a driver-
> specific structure, and it's equally common to use a container_of() to
> get at the embedding structure.
I am saying that you dont need this new APIs, GPIO framework already
support this via the call gpiochip_get_data(chip); which you provided
when adding gpiochip().
>
>>> + gpio->gpio.parent = &pdev->dev;
>>> +
>>> + gpio->gpio.get_direction = tegra186_gpio_get_direction;
>>> + gpio->gpio.direction_input = tegra186_gpio_direction_input;
>>> + gpio->gpio.direction_output = tegra186_gpio_direction_output;
>>> + gpio->gpio.get = tegra186_gpio_get,
>>> + gpio->gpio.set = tegra186_gpio_set;
>>> + gpio->gpio.to_irq = tegra186_gpio_to_irq;
>>> +
>>> + gpio->gpio.base = -1;
>>> +
>>> + for (i = 0; i < gpio->soc->num_ports; i++)
>>> + gpio->gpio.ngpio += gpio->soc->ports[i].pins;
>>> +
>> Our DT binding does not say this. We assume that we have 8 gpios per port.
>> so this will not work at all.
> This has nothing to do with the device tree binding. What the device
> tree binding defines is the indices to use to obtain a given GPIO within
> a given port. What numbering the driver uses internally is completely up
> to the driver implementation.
>
> Oh, and the above works just fine.
Nop, it will not work. The reason is:
include/dt-binding/gpio/tegra186-gpio.h
#define TEGRA_MAIN_GPIO(port, offset) \
((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)
so in your DTS file, if you use this macro for the gpio number then you
will have pin per port as 8.
And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is
very less.
On Thu, Nov 24, 2016 at 12:06:16PM +0530, Laxman Dewangan wrote:
>
> On Thursday 24 November 2016 01:10 AM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
> > >
> > > This is already possible and several drivers are doing this.
> > >
> > > Everything, all kernel users and all character device users, end up
> > > calling gpiod_request().
> > It looks like I stumbled across the only case where this isn't true.
> > What I was seeing, and which ultimately led me to implement the compact
> > numberspace is that gpiochip_add_data() calls ->get_direction() directly
> > without first going through ->request(). We'd have to guard that one
> > case as well in order for this to work.
> >
> In T186, we have 8 pins per PORT and for some of ports, all pins are not
> available. Like Port A has 7 pins valid (0 to 6) and port E have 8 pins (0
> to 7). The great part is that each port has valid pins start from 0.
From what I can tell that's not true. We don't always have 8 pins per
port. That's just the definition that we use to simplify the external
numbering of GPIOs.
> So just having the number of valid pins for each port as part of SOC data
> will help to find out whether GPIO exist or not.
>
> int port = GPIO_PORT(offset);
> int pin = GPIO_PIN(offset);
>
> if (pin >= tgi->soc->port[port].valid_pins)
> return false;
Yes, that's exactly what tegra186_gpio_of_xlate() does, because its job
is to translate from the (sparse) numbering defined in the bindings to
the internal compact numbering.
>
> Similar logic can be used for APIs which can get called without
> gpio_request().
No it doesn't. There are currently a couple of cases where these
functions get called without first requesting the GPIO. These are all
internal to gpiolib as far as I can tell, but they all assume that a
GPIO with an index that's within the range from 0 to chip->ngpio will
exist.
On Tegra186 that's not the case, so we have to add code to every
callback to check whether or not a given pin exists. And the subsystem
is not equipped to deal with that properly, because it doesn't allow all
callbacks to return an error.
So, like I said in the other subthread the only way to make this sparse
number scheme work correctly is to implement ->request() and make sure
that all calls to GPIO operations are properly guarded with a call to
->request().
I still don't like very much how we'd be registering GPIOs that don't
exist physically, not only because we waste memory and CPU cycles, but
also because the driver becomes more prone to errors. If ever somebody
added new direct calls to one of the operations in the gpiolib core
without guarding them with ->request() our driver would likely break.
If we absolutely have to use the same numbering internally as in DT, we
need to at least make sure to properly handle accesses to them.
Thierry
On Wed, Nov 23, 2016 at 8:40 PM, Thierry Reding
<[email protected]> wrote:
> On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
>> Everything, all kernel users and all character device users, end up
>> calling gpiod_request().
>
> It looks like I stumbled across the only case where this isn't true.
> What I was seeing, and which ultimately led me to implement the compact
> numberspace is that gpiochip_add_data() calls ->get_direction() directly
> without first going through ->request(). We'd have to guard that one
> case as well in order for this to work.
Hm. So there are two ways we could address that:
- Make irq_valid_mask a pure .valid_mask so we can mask off
gpios not just for IRQs but for anything, if this is consistent
with what Mika needs or
- Make the .get_direction() callback return -EINVAL or whatever
for the non-available lines, exactly how .request() does it,
and manage that in the core when looping over them to get
the direction in the gpiochip_add() call.
>> How this blocks the IRQs from being requested can be seen
>> in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
>> gracefully handles this too.
>
> I don't think it does. The issue I mentioned for gpiochip_add_data()
> exists in gpiochip_lock_as_irq(). That code assumes that all GPIOs from
> 0 to chip.ngpio - 1 are valid.
It shouldn't matter since we should deny these to be even
mapped as IRQs before the irqchip callbacks for requesting
resources are called so it's not happening. This is happening
with GPIOLIB_IRQCHIP.
> Each hardware block also implements a number of "ports": 8 for AON and
> 23 for main. Each of these ports has a different number of pins. Often
> there will be 6, 7 or 8, but a couple of ports have as little as a
> single pin.
What a maze. I would be happy if all this weirdness resides
in the device driver ;)
> Each port is associated with a given "controller", that is
> interrupt, so when a GPIO of a port is configured as input and enabled
> to generate an interrupt, the interrupt of it's parent controller will
> be asserted. I'm not exactly sure how this association is done, I have
> not found anything in the TRM.
Is nVidia one of those throw-over-the-wall engineering companies
where hardware engineers toss a chip over the wall to the software
developers and don't expect them to ask any questions?
I'm asking since there are so many nVidia people on this thread.
I would normally expect that you could simply go and ask the
hardware people?
>> It seems that "controller" refer to two different things in the two
>> sentences. Do you mean your hardware has ONE controller with
>> several BANKS? (as described in Documentation/gpio/driver.txt?)
>
> I hope the above clarifies things. struct gpio_chip represents the
> entire hardware block (in current drivers) and the driver deals with
> individual controllers and ports internally. The proposal I was talking
> about above is to have one driver create multiple struct gpio_chip, each
> representing an individual port. Hence each struct gpio_chip would be
> assigned the exact number of pins that the port supports, removing all
> of the problems with numberspaces. It has its own set of disadvantages,
> such as creating a large number of GPIO chips, which may in the end be
> just as confusing as the current implementation.
I have a soft preference toward making one chip for each port/bank.
But it is not strong.
That opionon is stronger if the port/bank has resources such as a
clock, power supply or IRQ line. Then it tends toward mandatory
to have a gpio_chip for each.
>> Use the new character device, and for a new SoC like the tegra186
>> you can CERTAINLY convince any downstream consumers to
>> switch to that.
>
> I've looked a little at the character device implementation and I think
> it would work equally bad with the compact numberspace as sysfs. The
> reason is that gpiolib doesn't allow remapping of chip-relative indices
> except for DT. I think this might be possible by generalizing the
> ->of_xlate() to be used in translating internal numbers as well. The way
> I could imagine this to work is that an IOCTL providing offsets of the
> lines to use would pass the offsets through the chip's ->xlate()
> function to retrieve the index (0 to chip->ngpio). The alternative is to
> stick with the sparse numberspace and deal with non-existing GPIOs via a
> ->request() callback.
I don't understand. Userspace should have no concern about the
numberspace. Lines can be named from DT or ACPI so you can
look up line chip+offset from the names.
Further sysfs sys/bus/gpio (notice: NOT /sys/class/gpio !)
contains topological information, that is the approach taken by
userspace helpers such as udev.
>> Please just disable CONFIG_GPIO_SYSFS from your defconfig
>> and in any company-internal builds and point everyone and their
>> dog to the character device: tools/gpio/*,
>> and the UAPI <linux/gpio.h>
>
> I don't think we can easily do that. People may still rely on the sysfs
> interface, or even if they aren't this might be enabled in a multi-
> platform image.
Good suggestion. I will go and look at the upstream multiplatform
configs and eradicate this.
> So I think regardless of whether or not we like the
> interface, we have to make sure that our solution plays nicely with it.
I would be concerned if you are designing your driver around the
the way the legacy sysfs happens to work.
The thing is broken. Don't design FOR it.
You are making me tempted to require that for new hardware the
driver has to
depends on !GPIO_SYSFS
In order to make this a non-argument.
Well it would be undiplomatic on my part I guess. But I have to
encourage/nudge a switch somehow.
> There is no problem with the compact numberspace, though it comes with
> the inconvenience that numbering in sysfs is different from numbering in
> DT.
That is fixed in the chardev ABI. Offset on the chardev is the same
as the internal offset of gpio_chip, and therefore the same as used
when doing xlate in the DT for a chip, i.e. the DT offset numbering.
> 1) use a sparse numberspace and deal with non-existing GPIOs via the
> ->request() callback
>
> 2) use a compact numberspace and live with separate methods of
> referencing GPIOs
>
> 3) use a compact numberspace and introduce a mechanism to translate
> all hardware numbers into per-chip indices
>
> I think 1) is the simplest, but at the cost of wasting memory and CPU
> cycles by maintaining descriptors for GPIOs we know don't exist.
You could make a quick calculation of how much that is actually.
I doubt it matters for a contemporary non-IoT platform, but I may
be wrong.
> 2) is
> fairly simple as well, but it's pretty inconvenient for the user. The
> most invasive solution would be
Inconvenient to in-kernel users or userspace users?
Inconvenient to sysfs users or chardev users?
If it is invonvenient to sysfs users is of no concern, as long
as it is no broken.
> 3), though I personally like that best
> because it is the most explicit. It does have the disavantage of using
> separate numbering for sysfs and everyone else, though. Unless you're
> willing to make sysfs use per-chip export/unexport files and the
> ->xlate() callback.
I don't know if I even understand (3).
Consistency in the sysfs ABI does not concern me. It is inherently
inconsistent already. It will be consistently messy for this hardware,
but it shouldn't be used.
Yours,
Linus Walleij
On Thu, Nov 24, 2016 at 08:14:31PM +0530, Laxman Dewangan wrote:
>
> On Thursday 24 November 2016 08:14 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Thu, Nov 24, 2016 at 12:23:56PM +0530, Laxman Dewangan wrote:
> > > On Tuesday 22 November 2016 11:25 PM, Thierry Reding wrote:
> > > > +static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip)
> > > > +{
> > > > + return container_of(chip, struct tegra_gpio, gpio);
> > > > +}
> > > You dont need this as gpiochip_get_data(chip); can provide the required
> > > driver specific data.
> > It's common practice to embed the struct gpio_chip within a driver-
> > specific structure, and it's equally common to use a container_of() to
> > get at the embedding structure.
>
> I am saying that you dont need this new APIs, GPIO framework already support
> this via the call gpiochip_get_data(chip); which you provided when adding
> gpiochip().
Okay, it looks like this is the standard way to do this within the GPIO
subsystem. I can switch to that.
> > > > + gpio->gpio.parent = &pdev->dev;
> > > > +
> > > > + gpio->gpio.get_direction = tegra186_gpio_get_direction;
> > > > + gpio->gpio.direction_input = tegra186_gpio_direction_input;
> > > > + gpio->gpio.direction_output = tegra186_gpio_direction_output;
> > > > + gpio->gpio.get = tegra186_gpio_get,
> > > > + gpio->gpio.set = tegra186_gpio_set;
> > > > + gpio->gpio.to_irq = tegra186_gpio_to_irq;
> > > > +
> > > > + gpio->gpio.base = -1;
> > > > +
> > > > + for (i = 0; i < gpio->soc->num_ports; i++)
> > > > + gpio->gpio.ngpio += gpio->soc->ports[i].pins;
> > > > +
> > > Our DT binding does not say this. We assume that we have 8 gpios per port.
> > > so this will not work at all.
> > This has nothing to do with the device tree binding. What the device
> > tree binding defines is the indices to use to obtain a given GPIO within
> > a given port. What numbering the driver uses internally is completely up
> > to the driver implementation.
> >
> > Oh, and the above works just fine.
>
>
> Nop, it will not work. The reason is:
> include/dt-binding/gpio/tegra186-gpio.h
>
>
> #define TEGRA_MAIN_GPIO(port, offset) \
> ((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)
>
>
> so in your DTS file, if you use this macro for the gpio number then you will
> have pin per port as 8.
> And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is very
> less.
Yes, within the source code, ngpio will be the exact number of pins that
the GPIO controller physically exposes. But that still works fine, feel
free to test the driver if you don't believe me. The translation from
one numberspace to the other is done in tegra186_gpio_of_xlate().
Thierry
On Wed, Nov 23, 2016 at 8:44 PM, Thierry Reding
<[email protected]> wrote:
> On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
>> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
>> <[email protected]> wrote:
>>
>> > From: Thierry Reding <[email protected]>
>> >
>> > Tegra186 has two GPIO controllers that are largely register compatible
>> > between one another but are completely different from the controller
>> > found on earlier generations.
>> >
>> > Signed-off-by: Thierry Reding <[email protected]>
>>
>> I would prefer if you could try to convert this driver to use
>> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
>> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
>> It would save us so much trouble and so much complicated
>> code to maintain for this custom irqdomain.
>>
>> I suggest you to look into the mechanisms mentioned in my
>> previous mail for how to poke holes in the single linear
>> irqdomain used by this mechanism.
>>
>> As it seems, you only have one parent interrupt with all
>> these IRQs cascading off it as far as I can tell.
>
> Like I said in other email, I don't think this will work because the
> GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> parent interrupt is used. We could possibly live with a single IRQ
> handler and data, but we definitely need to request multiple IRQs if
> we want to be able to use all GPIOs as interrupts.
Sorry if I missed that.
Actually it works.
If you look at gpiochip_set_chained_irqchip() it just calls
irq_set_chained_handler_and_data() for the parent interrupt.
There is a loop afterwards that call irq_set_parent()
on all child IRQs but to the kernel this is just a noop I
never fully grasped that thing. Maybe it should even be deleted.
It just sets the parent in the irq descriptor, and the IRQ core only
ever use this on threaded interrupts, so maybe it should
really just be set on those (nested) interrupts.
Maybe Marc/tglx could shed some light on the intended
use of irq_set_parent() given that only two MFD drivers
and the gpiolib uses it. I think I need to dig into some
commitlogs.
If it is semantically required to call irq_set_parent() with the
right child/parent, we could add an
gpiochip_set_chained_irqchip_interval(first, last) to set the
parent for just an interval of the offsets.
Yours,
Linus Walleij
On Thu, Nov 24, 2016 at 04:08:08PM +0100, Linus Walleij wrote:
> On Wed, Nov 23, 2016 at 8:40 PM, Thierry Reding
> <[email protected]> wrote:
> > On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
>
> >> Everything, all kernel users and all character device users, end up
> >> calling gpiod_request().
> >
> > It looks like I stumbled across the only case where this isn't true.
> > What I was seeing, and which ultimately led me to implement the compact
> > numberspace is that gpiochip_add_data() calls ->get_direction() directly
> > without first going through ->request(). We'd have to guard that one
> > case as well in order for this to work.
>
> Hm. So there are two ways we could address that:
>
> - Make irq_valid_mask a pure .valid_mask so we can mask off
> gpios not just for IRQs but for anything, if this is consistent
> with what Mika needs or
>
> - Make the .get_direction() callback return -EINVAL or whatever
> for the non-available lines, exactly how .request() does it,
> and manage that in the core when looping over them to get
> the direction in the gpiochip_add() call.
Another alternative might be to do a ->request() on the line first
before even attempting to call ->get_direction(), which would make
checking for valid lines uniform with all other places. That way a
driver wouldn't have to perform the same check in both callbacks.
> >> How this blocks the IRQs from being requested can be seen
> >> in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
> >> gracefully handles this too.
> >
> > I don't think it does. The issue I mentioned for gpiochip_add_data()
> > exists in gpiochip_lock_as_irq(). That code assumes that all GPIOs from
> > 0 to chip.ngpio - 1 are valid.
>
> It shouldn't matter since we should deny these to be even
> mapped as IRQs before the irqchip callbacks for requesting
> resources are called so it's not happening. This is happening
> with GPIOLIB_IRQCHIP.
Yes, it looks as though the irqchip->irq_request_resources() will only
be called at IRQ request time, so the gpiochip_lock_as_irq() will be
called after the chip was registered and so I assume the irq_valid_mask
would be properly set up to avoid callbacks for non-existing GPIOs.
> > Each hardware block also implements a number of "ports": 8 for AON and
> > 23 for main. Each of these ports has a different number of pins. Often
> > there will be 6, 7 or 8, but a couple of ports have as little as a
> > single pin.
>
> What a maze. I would be happy if all this weirdness resides
> in the device driver ;)
>
> > Each port is associated with a given "controller", that is
> > interrupt, so when a GPIO of a port is configured as input and enabled
> > to generate an interrupt, the interrupt of it's parent controller will
> > be asserted. I'm not exactly sure how this association is done, I have
> > not found anything in the TRM.
>
> Is nVidia one of those throw-over-the-wall engineering companies
> where hardware engineers toss a chip over the wall to the software
> developers and don't expect them to ask any questions?
>
> I'm asking since there are so many nVidia people on this thread.
>
> I would normally expect that you could simply go and ask the
> hardware people?
I can certainly find out, but I was hoping that since Suresh had these
numbers in the patch, he would know where to find the information. It's
not so much that we can't get answers from engineering, quite the
opposite actually. I only refer to the TRM because it is supposed to be
the canonical documentation and if this information isn't there we need
to get it added. Doing that can be somewhat tedious, and me being lazy
I was hoping that I had simply missed it.
> >> It seems that "controller" refer to two different things in the two
> >> sentences. Do you mean your hardware has ONE controller with
> >> several BANKS? (as described in Documentation/gpio/driver.txt?)
> >
> > I hope the above clarifies things. struct gpio_chip represents the
> > entire hardware block (in current drivers) and the driver deals with
> > individual controllers and ports internally. The proposal I was talking
> > about above is to have one driver create multiple struct gpio_chip, each
> > representing an individual port. Hence each struct gpio_chip would be
> > assigned the exact number of pins that the port supports, removing all
> > of the problems with numberspaces. It has its own set of disadvantages,
> > such as creating a large number of GPIO chips, which may in the end be
> > just as confusing as the current implementation.
>
> I have a soft preference toward making one chip for each port/bank.
>
> But it is not strong.
>
> That opionon is stronger if the port/bank has resources such as a
> clock, power supply or IRQ line. Then it tends toward mandatory
> to have a gpio_chip for each.
There really aren't any per-port resources. In fact now that I think
about it adding a gpio_chip per port might actually make things more
difficult because the interrupts are per-controller which may control
one or more ports.
> >> Use the new character device, and for a new SoC like the tegra186
> >> you can CERTAINLY convince any downstream consumers to
> >> switch to that.
> >
> > I've looked a little at the character device implementation and I think
> > it would work equally bad with the compact numberspace as sysfs. The
> > reason is that gpiolib doesn't allow remapping of chip-relative indices
> > except for DT. I think this might be possible by generalizing the
> > ->of_xlate() to be used in translating internal numbers as well. The way
> > I could imagine this to work is that an IOCTL providing offsets of the
> > lines to use would pass the offsets through the chip's ->xlate()
> > function to retrieve the index (0 to chip->ngpio). The alternative is to
> > stick with the sparse numberspace and deal with non-existing GPIOs via a
> > ->request() callback.
>
> I don't understand. Userspace should have no concern about the
> numberspace. Lines can be named from DT or ACPI so you can
> look up line chip+offset from the names.
Userspace would have to know about the numberspace if it wants to use
the character device, right? The numberspace in DT assumes that each
port has 8 pins (this makes it very easy to create a unique index by
doing (port * 8 + offset) as in:
#define TEGRA_MAIN_GPIO_PORT_A 0
#define TEGRA_MAIN_GPIO_PORT_B 1
...
#define TEGRA_MAIN_GPIO(port, offset) \
((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)
It also provides an easy way for the driver to check for validity: a
port can be obtained by dividing the DT index by 8, and the offset of
the pin is the remainder of the division. If the pin number is higher
than the number of pins supported by the given port we can return an
error. That's what tegra186_gpio_of_xlate() does, it translates from
the DT sparse numberspace into the compact numberspace in gpiolib and
rejects invalid pins at the same time.
> Further sysfs sys/bus/gpio (notice: NOT /sys/class/gpio !)
> contains topological information, that is the approach taken by
> userspace helpers such as udev.
>
> >> Please just disable CONFIG_GPIO_SYSFS from your defconfig
> >> and in any company-internal builds and point everyone and their
> >> dog to the character device: tools/gpio/*,
> >> and the UAPI <linux/gpio.h>
> >
> > I don't think we can easily do that. People may still rely on the sysfs
> > interface, or even if they aren't this might be enabled in a multi-
> > platform image.
>
> Good suggestion. I will go and look at the upstream multiplatform
> configs and eradicate this.
>
> > So I think regardless of whether or not we like the
> > interface, we have to make sure that our solution plays nicely with it.
>
> I would be concerned if you are designing your driver around the
> the way the legacy sysfs happens to work.
>
> The thing is broken. Don't design FOR it.
That's not what I meant. I just don't want the driver to crash the
kernel if somebody happened to use it via sysfs.
> You are making me tempted to require that for new hardware the
> driver has to
>
> depends on !GPIO_SYSFS
>
> In order to make this a non-argument.
>
> Well it would be undiplomatic on my part I guess. But I have to
> encourage/nudge a switch somehow.
>
> > There is no problem with the compact numberspace, though it comes with
> > the inconvenience that numbering in sysfs is different from numbering in
> > DT.
>
> That is fixed in the chardev ABI. Offset on the chardev is the same
> as the internal offset of gpio_chip, and therefore the same as used
> when doing xlate in the DT for a chip, i.e. the DT offset numbering.
That's not quite true. In the version of the driver that I have the
->of_xlate() translates the DT numberspace into an internal one that
doesn't have the holes which are problematic (because the associated
registers don't exist). What you are referring to would be what the
of_gpio_simple_xlate() does.
> > 1) use a sparse numberspace and deal with non-existing GPIOs via the
> > ->request() callback
> >
> > 2) use a compact numberspace and live with separate methods of
> > referencing GPIOs
> >
> > 3) use a compact numberspace and introduce a mechanism to translate
> > all hardware numbers into per-chip indices
> >
> > I think 1) is the simplest, but at the cost of wasting memory and CPU
> > cycles by maintaining descriptors for GPIOs we know don't exist.
>
> You could make a quick calculation of how much that is actually.
> I doubt it matters for a contemporary non-IoT platform, but I may
> be wrong.
For the main controller the number of physically available GPIO lines is
140, whereas the DT numbering assumes there are 184. That's 44 more than
there really are, which is about 30% overhead. For AON it's 47 to 64 and
about 25% overhead.
It's not so much that it's a real problem, but more of a matter of
principle. I'm not at all concerned about this having an adverse effect
in practice, I just think it's wrong.
But if everyone else thinks it's okay, I'm sure I can find ways to live
with it.
> > 2) is
> > fairly simple as well, but it's pretty inconvenient for the user. The
> > most invasive solution would be
>
> Inconvenient to in-kernel users or userspace users?
in-kernel users would be using DT to refer to the GPIOs and that's a
solved problem because ->of_xlate() gives us a way of translating
between them.
> Inconvenient to sysfs users or chardev users?
It would be inconvenient for both because even though chardev gives us
per-chip offsets, there is no way to translate these offsets from the
external to the internal numberspace. Hence chardev would inevitably
have to use a numberspace different from that of DT.
> If it is invonvenient to sysfs users is of no concern, as long
> as it is no broken.
For sysfs there's no way we could use a compact numberspace and be
compatible with the DT numberspace because it is not possible to
translate from the global numberspace to a per-chip numberspace if
it isn't a 1:1 mapping.
> > 3), though I personally like that best
> > because it is the most explicit. It does have the disavantage of using
> > separate numbering for sysfs and everyone else, though. Unless you're
> > willing to make sysfs use per-chip export/unexport files and the
> > ->xlate() callback.
>
> I don't know if I even understand (3).
Essentially this would mean generalizing the ->of_xlate() to apply to
all users (DT and chardev alike, and I suppose ACPI as well). I think
it's a superior solution because it's completely generic. Currently I
think all of the GPIO users are forced into a 1:1 mapping and working
around it if that's not how the hardware works. A generic ->xlate()
would completely separate the external numberspace from the internal
contiguous, compact numberspace. And it would allow us to do so in a
unified way.
Thierry
Mostly it seems we are in understanding here, looking forward
to the patches...
On Thu, Nov 24, 2016 at 5:32 PM, Thierry Reding
<[email protected]> wrote:
> On Thu, Nov 24, 2016 at 04:08:08PM +0100, Linus Walleij wrote:
>> I don't understand. Userspace should have no concern about the
>> numberspace. Lines can be named from DT or ACPI so you can
>> look up line chip+offset from the names.
>
> Userspace would have to know about the numberspace if it wants to use
> the character device, right?
No it shouldn't really. Adding gpio-line-names = "foo", "bar" etc
and following a reasonable naming policy (such as using the
rail names on the schematic, which is usually helpful) the
GPIO can easily be looked up from its name.
The chardev tries to overcome the hardships in finding the
GPIO you want this way.
Yours,
Linus Walleij
On Thursday 24 November 2016 08:38 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Nov 24, 2016 at 08:14:31PM +0530, Laxman Dewangan wrote:
>>>
>>> This has nothing to do with the device tree binding. What the device
>>> tree binding defines is the indices to use to obtain a given GPIO within
>>> a given port. What numbering the driver uses internally is completely up
>>> to the driver implementation.
>>>
>>> Oh, and the above works just fine.
>>
>> Nop, it will not work. The reason is:
>> include/dt-binding/gpio/tegra186-gpio.h
>>
>>
>> #define TEGRA_MAIN_GPIO(port, offset) \
>> ((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)
>>
>>
>> so in your DTS file, if you use this macro for the gpio number then you will
>> have pin per port as 8.
>> And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is very
>> less.
> Yes, within the source code, ngpio will be the exact number of pins that
> the GPIO controller physically exposes. But that still works fine, feel
> free to test the driver if you don't believe me. The translation from
> one numberspace to the other is done in tegra186_gpio_of_xlate().
OK, so you are mapping the DT gpio number to new number using xlate.
This method is fine but it complicate the driver and always it needs to
calculate the base for the port.
If we have one to one mapping then probably, we can have fixed lookup table.
I am just incline to make stuff simple so that we can have better
maintainability and debugging. This is very common driver and almost all
BSPS engineer debug here so want to make this driver extremely simple.