| Message ID | 20170310162629.31455-1-thierry.reding@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 10, 2017 at 05:26:29PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > 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 <treding@nvidia.com> > --- > Changes in v2: > - add pin names to allow easy lookup using the chardev interface By the way, I tested this using Bartosz Gołaszewski's excellent libgpiod that provides a really nice set of userspace utilities to detect and manipulate GPIO chips: https://github.com/brgl/libgpiod So for userspace access to the Tegra GPIOs we can now run something like this in a shell script: #!/bin/sh gpio=$(gpiofind PCC.00) gpioget $gpio There's a bunch of other features that the library supports. I'm glad we finally have a standard set of tools to deal with GPIOs in userspace. It seems the internet is full of libraries that deal with the GPIO sysfs interface, but hopefully we can standardize on one library and the tools for the chardev interface. Thierry
On Friday 10 March 2017 09:56 PM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > 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 <treding@nvidia.com> > --- > Changes in v2: > - add pin names to allow easy lookup using the chardev interface > - distinguish AON and main GPIO controllers by label > - use gpiochip_get_data() instead of container_of() > - use C99 initializers > Overall it looks fine to me. Only thing I did not like is the of_xlate manipulation where the gpio number is translated to another number for register access. The reason is that it complicate in debugging when we instrument the callbacks for some gpios and this is very common in our debugging. I still request here to use simple mapping instead of complicating. Otherwise: Acked-by: Laxman Dewangan <ldewangan@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 13, 2017 at 09:52:04PM +0530, Laxman Dewangan wrote: > > On Friday 10 March 2017 09:56 PM, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > 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 <treding@nvidia.com> > > --- > > Changes in v2: > > - add pin names to allow easy lookup using the chardev interface > > - distinguish AON and main GPIO controllers by label > > - use gpiochip_get_data() instead of container_of() > > - use C99 initializers > > > > Overall it looks fine to me. > Only thing I did not like is the of_xlate manipulation where the gpio number > is translated to another number for register access. > The reason is that it complicate in debugging when we instrument the > callbacks for some gpios and this is very common in our debugging. > > I still request here to use simple mapping instead of complicating. I fail to see how this is complicating things. Yes, you can no longer directly map from an offset to the port/index, but since we have a name associated with each pin, we can simply print the name if we want to know what pin we're dealing with. So you could simply do something like this: static int tegra186_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { struct gpio_desc *desc = gpiochip_get_desc(chip, offset); ... dev_dbg(chip->parent, "GPIO %s (%s)\n", desc->name, desc->label); ... } The alternative that you're talking about would be something like this: static int tegra186_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) { struct tegra_gpio_port_soc_info *info; unsigned int port = GPIO_PORT(offset); unsigned int pin = GPIO_PIN(offset); ... info = tgi->soc->port[port]; ... dev_dbg(chip->parent, "GPIO P%s.%02u\n", info->port_name, pin); ... } And with the above you'd either duplicate the logic that constructs the pin name: once in the instrumentation code and once in the code that sets up the pin names. Or you could decide not to set up any pin names at all, in which case userspace would need to deal with the numbering schemes itself, which makes things really complicated, especially when we start supporting multiple SoC families. The proposal in this patch is really quite elegant, if I may say so, in that individual pins are identified by their name. Userspace can query the list and get only the ones that actually exist. The wholes in the GPIO range are not there, so there's not even a chance of userspace messing with non-existing GPIOs. Similarly the kernel has access to the name of these GPIOs, so the same "string representation" can be used as for userspace. I think this makes the driver very consistent across all use-cases. The only remaining downside I see is that we're moving away from the 8-pins per port from earlier chips. But I actually see that as a feature, even something that we should backport to older chips. The only reason why the driver for older chips works is because the registers are laid out such that the pins of a port share one register block, so if a non-existing GPIO is accessed, we'd be accessing a non- existing field in an existing register, which seems to be less of an issue than accessing a non-existing register. Thierry
On Mon, Mar 13, 2017 at 8:06 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > By the way, I tested this using Bartosz Gołaszewski's excellent libgpiod > that provides a really nice set of userspace utilities to detect and > manipulate GPIO chips: > > https://github.com/brgl/libgpiod > > So for userspace access to the Tegra GPIOs we can now run something like > this in a shell script: > > #!/bin/sh > gpio=$(gpiofind PCC.00) > gpioget $gpio > > There's a bunch of other features that the library supports. I'm glad we > finally have a standard set of tools to deal with GPIOs in userspace. I share your high appraisal of this library, Bartosz work is really important for consolidating this mess to something we can contain. > It > seems the internet is full of libraries that deal with the GPIO sysfs > interface, I do not advocate Internet censorship, but ... ;) The world should be purged from them all. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 10, 2017 at 05:26:29PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > 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 <treding@nvidia.com> > --- > Changes in v2: > - add pin names to allow easy lookup using the chardev interface > - distinguish AON and main GPIO controllers by label > - use gpiochip_get_data() instead of container_of() > - use C99 initializers > > Hi Linus, > > This addresses all comments I got on the last revision, except for your > request to use gpiolib's IRQ chip helpers. I investigated using it with > the driver but still can't figure out how it's supposed to work given > our requirement to service multiple parent interrupts. > > Thierry > > drivers/gpio/Kconfig | 8 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-tegra186.c | 636 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 645 insertions(+) > create mode 100644 drivers/gpio/gpio-tegra186.c Hi Linus, any objections to merging this for v4.12? I know Laxman has some reservations about the internal numbering, but I hope my explanation in the other subthread shows that this is the simplest way to achieve what we want with the least amount of surprises. Thierry > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 05043071fc98..f74b61cfd113 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -431,6 +431,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 becb96c724fe..cad0eb2c3531 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -111,6 +111,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..87e02d9165d8 > --- /dev/null > +++ b/drivers/gpio/gpio-tegra186.c > @@ -0,0 +1,636 @@ > +/* > + * Copyright (c) 2016-2017 NVIDIA Corporation > + * > + * Author: Thierry Reding <treding@nvidia.com> > + * > + * 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 { > + const char *name; > + unsigned int offset; > + unsigned int pins; > +}; > + > +struct tegra_gpio_soc { > + const struct tegra_gpio_port *ports; > + unsigned int num_ports; > + const char *name; > +}; > + > +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 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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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) > +{ > + unsigned int i, j, irq, offset = 0; > + struct tegra_gpio *gpio; > + struct resource *res; > + char **names; > + 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 = gpio->soc->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; > + > + names = devm_kcalloc(gpio->gpio.parent, gpio->gpio.ngpio, > + sizeof(*names), GFP_KERNEL); > + if (!names) > + return -ENOMEM; > + > + for (i = 0; i < gpio->soc->num_ports; i++) { > + const struct tegra_gpio_port *port = &gpio->soc->ports[i]; > + char *name; > + > + for (j = 0; j < port->pins; j++) { > + name = devm_kasprintf(gpio->gpio.parent, GFP_KERNEL, > + "P%s.%02x", port->name, j); > + if (!name) > + return -ENOMEM; > + > + names[offset + j] = name; > + } > + > + offset += port->pins; > + } > + > + gpio->gpio.names = (const char * const *)names; > + > + 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; > +} > + > +#define TEGRA_MAIN_GPIO_PORT(port, base, count) \ > + [TEGRA_MAIN_GPIO_PORT_##port] = { \ > + .name = #port, \ > + .offset = base, \ > + .pins = count, \ > + } > + > +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, > + .name = "tegra186-gpio", > +}; > + > +#define TEGRA_AON_GPIO_PORT(port, base, count) \ > + [TEGRA_AON_GPIO_PORT_##port] = { \ > + .name = #port, \ > + .offset = base, \ > + .pins = count, \ > + } > + > +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, > + .name = "tegra186-gpio-aon", > +}; > + > +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 <treding@nvidia.com>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.12.0 >
On Fri, Mar 31, 2017 at 3:10 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > any objections to merging this for v4.12? Sorry I should have done a proper review. I'll look at it now. >I know Laxman has some > reservations about the internal numbering, but I hope my explanation in > the other subthread shows that this is the simplest way to achieve what > we want with the least amount of surprises. As long as he's not NACKing it it's fine I guess. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 10, 2017 at 5:26 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > 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 <treding@nvidia.com> > --- > Changes in v2: So this still doesn't use GPIOLIB_IRQCHIP even though I pointed out that you can assign several parent interrupts to the same irqchip. For a recent example see: http://marc.info/?l=devicetree&m=149012117004066&w=2 (Notice Gregory's elegant manipulation of the mask.) My earlier reply here: ---------------------- >> 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. ------------------------ Just call gpiochip_set_chained_irqchip() for all the irqs, and figure out a way to get the right interrupt hardware offset in the interrupt handler. (...) > +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 select GPIOLIB_IRQCHIP > +#include <linux/gpio/driver.h> > +#include <linux/gpio.h> Only <linux/gpio/driver.h> You should not use <linux/gpio.h> in drivers, then you are doing something wrong. > +struct tegra_gpio { > + struct gpio_chip gpio; > + struct irq_chip intc; > + unsigned int num_irq; > + unsigned int *irq; Why are you keeping the irqs around after requesting? Use devm_* > + > + const struct tegra_gpio_soc *soc; > + > + void __iomem *base; > + > + struct irq_domain *domain; I don't like custom irq domains it is messy. Please do your best to try to use GPIOLIB_IRQCHIP If you still decide to go with a custom irqdomain, there is stuff missing, especially the gpiochip_lock_as_irq() calls from the irqchip .irq_request/release_resources() callbacks, see the gpiolib.c implementation in the helpers there for reference. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote: > On Fri, Mar 10, 2017 at 5:26 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > > From: Thierry Reding <treding@nvidia.com> > > > > 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 <treding@nvidia.com> > > --- > > Changes in v2: > > So this still doesn't use GPIOLIB_IRQCHIP even though I pointed > out that you can assign several parent interrupts to the same > irqchip. > > For a recent example see: > http://marc.info/?l=devicetree&m=149012117004066&w=2 > (Notice Gregory's elegant manipulation of the mask.) > > My earlier reply here: > > ---------------------- > >> 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. > ------------------------ > > Just call gpiochip_set_chained_irqchip() for all the irqs, and > figure out a way to get the right interrupt hardware offset in the > interrupt handler. Okay, let me quote the gpiochip_set_chained_irqchip() function here and explain why I think it's not a proper fit here. This is actually quoting gpiochip_set_cascaded_irqchip(), but that's what ends up being called. > static void gpiochip_set_cascaded_irqchip(struct gpio_chip *gpiochip, > struct irq_chip *irqchip, > int parent_irq, > irq_flow_handler_t parent_handler) > { > unsigned int offset; > > if (!gpiochip->irqdomain) { > chip_err(gpiochip, "called %s before setting up irqchip\n", > __func__); > return; > } > > if (parent_handler) { > if (gpiochip->can_sleep) { > chip_err(gpiochip, > "you cannot have chained interrupts on a " > "chip that may sleep\n"); > return; > } > /* > * The parent irqchip is already using the chip_data for this > * irqchip, so our callbacks simply use the handler_data. > */ > irq_set_chained_handler_and_data(parent_irq, parent_handler, > gpiochip); > > gpiochip->irq_chained_parent = parent_irq; If I call this multiple times with different parent_irq parameters, then only the last one will be stored in gpiochip->irq_chained_parent. Later on, this is used to disconnect the chained handler and data upon GPIO chip removal, which means that handler and data for all but the last one end up dangling. > } > > /* Set the parent IRQ for all affected IRQs */ > for (offset = 0; offset < gpiochip->ngpio; offset++) { > if (!gpiochip_irqchip_irq_valid(gpiochip, offset)) > continue; > irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset), > parent_irq); > } > } Similarly here, we'd be setting the parent IRQ of all associated GPIOs to the first, second... last parent IRQ. This is completely unnecessary work and it's also setting the wrong parent. Note that we don't actually care about that in the driver right now, but it's still wrong. > > +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 > > select GPIOLIB_IRQCHIP > > > +#include <linux/gpio/driver.h> > > +#include <linux/gpio.h> > > Only <linux/gpio/driver.h> > > You should not use <linux/gpio.h> in drivers, then you are > doing something wrong. Yeah, I don't seem to need this at all. > > +struct tegra_gpio { > > + struct gpio_chip gpio; > > + struct irq_chip intc; > > + unsigned int num_irq; > > + unsigned int *irq; > > Why are you keeping the irqs around after requesting? > Use devm_* First I prefer to keep resource request and driver initialization separate because it makes error recovery more robust. So this is used to first store the results from platform_get_irq() and later on to iterate over when installing the chained handlers. Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I need to keep these around in order to properly uninstall the handlers again on removal. > > + > > + const struct tegra_gpio_soc *soc; > > + > > + void __iomem *base; > > + > > + struct irq_domain *domain; > > I don't like custom irq domains it is messy. > Please do your best to try to use GPIOLIB_IRQCHIP Like I explained above, I don't think it works the way it is supposed to for this case. The armada-37xx patch that you linked to earlier seems to suffer from the same issues in that all but the last parent IRQ handlers will be left dangling after removal, which would cause the kernel to run non-existing code if by any chance the interrupts were to still fire for some reason. > If you still decide to go with a custom irqdomain, there is > stuff missing, especially the gpiochip_lock_as_irq() > calls from the irqchip .irq_request/release_resources() > callbacks, see the gpiolib.c implementation in the > helpers there for reference. Good catch. I will add those. Thierry
On Friday 31 March 2017 07:06 PM, Linus Walleij wrote: > On Fri, Mar 31, 2017 at 3:10 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > >> any objections to merging this for v4.12? > Sorry I should have done a proper review. I'll look at it now. > >> I know Laxman has some >> reservations about the internal numbering, but I hope my explanation in >> the other subthread shows that this is the simplest way to achieve what >> we want with the least amount of surprises. > As long as he's not NACKing it it's fine I guess. I am fine with patch. You can consider my ack. Acked-by: Laxman Dewangan<ldewangan@nvidia.com> thanks, Laxman -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 31, 2017 at 4:54 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote: > If I call this multiple times with different parent_irq parameters, then > only the last one will be stored in gpiochip->irq_chained_parent. Later > on, this is used to disconnect the chained handler and data upon GPIO > chip removal, which means that handler and data for all but the last one > end up dangling. People are using this code already for chained handlers with several parents. I guess it works so nicely because gpiochips are often just added and seldom removed. Overall that means this is a bug and should be fixed, not that new drivers should avoid using this approach, or that we should discourage new drivers to use this API. >> /* Set the parent IRQ for all affected IRQs */ >> for (offset = 0; offset < gpiochip->ngpio; offset++) { >> if (!gpiochip_irqchip_irq_valid(gpiochip, offset)) >> continue; >> irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset), >> parent_irq); >> } >> } > > Similarly here, we'd be setting the parent IRQ of all associated GPIOs > to the first, second... last parent IRQ. This is completely unnecessary > work and it's also setting the wrong parent. Note that we don't actually > care about that in the driver right now, but it's still wrong. Is that a way of saying there are bugs in the gpiolib? I know there are, maintainers working on core code are always lacking. I wrote this code and I admit I make mistakes, please help me fix them instead of trying to make this into a reason not to use this code. >> Why are you keeping the irqs around after requesting? >> Use devm_* > > First I prefer to keep resource request and driver initialization > separate because it makes error recovery more robust. So this is used to > first store the results from platform_get_irq() and later on to iterate > over when installing the chained handlers. OK no big deal. But it could be in a local variable rather than in the driver state container, right? > Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I > need to keep these around in order to properly uninstall the handlers > again on removal. OK is this something that should be fixed in the irqchip code? > Like I explained above, I don't think it works the way it is supposed to > for this case. The armada-37xx patch that you linked to earlier seems to > suffer from the same issues in that all but the last parent IRQ handlers > will be left dangling after removal, which would cause the kernel to run > non-existing code if by any chance the interrupts were to still fire for > some reason. Again, bugs is not a reason not to use library code. We need to fix those bugs and centralize more, or we get too much strange code to maintain. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 01, 2017 at 07:46:24PM +0200, Linus Walleij wrote: > On Fri, Mar 31, 2017 at 4:54 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Fri, Mar 31, 2017 at 03:59:40PM +0200, Linus Walleij wrote: > > > If I call this multiple times with different parent_irq parameters, then > > only the last one will be stored in gpiochip->irq_chained_parent. Later > > on, this is used to disconnect the chained handler and data upon GPIO > > chip removal, which means that handler and data for all but the last one > > end up dangling. > > People are using this code already for chained handlers with several > parents. > > I guess it works so nicely because gpiochips are often just added > and seldom removed. > > Overall that means this is a bug and should be fixed, not that new > drivers should avoid using this approach, or that we should discourage > new drivers to use this API. > > >> /* Set the parent IRQ for all affected IRQs */ > >> for (offset = 0; offset < gpiochip->ngpio; offset++) { > >> if (!gpiochip_irqchip_irq_valid(gpiochip, offset)) > >> continue; > >> irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset), > >> parent_irq); > >> } > >> } > > > > Similarly here, we'd be setting the parent IRQ of all associated GPIOs > > to the first, second... last parent IRQ. This is completely unnecessary > > work and it's also setting the wrong parent. Note that we don't actually > > care about that in the driver right now, but it's still wrong. > > Is that a way of saying there are bugs in the gpiolib? > > I know there are, maintainers working on core code are always > lacking. I wrote this code and I admit I make mistakes, please help > me fix them instead of trying to make this into a reason not to > use this code. Fair enough. I'll see if I can turn this into something that I can use for this particular driver. I suspect it'll be a bit cumbersome because of the parent/child relationship, but I have a couple of ideas that I'd like to try out. > >> Why are you keeping the irqs around after requesting? > >> Use devm_* > > > > First I prefer to keep resource request and driver initialization > > separate because it makes error recovery more robust. So this is used to > > first store the results from platform_get_irq() and later on to iterate > > over when installing the chained handlers. > > OK no big deal. But it could be in a local variable rather than > in the driver state container, right? Well, I also need the same array of interrupts to remove the handlers again, so I don't think local would be enough. But... > > > Also, devm_* doesn't exist for irq_set_chained_handler_and_data(). So I > > need to keep these around in order to properly uninstall the handlers > > again on removal. > > OK is this something that should be fixed in the irqchip code? ... with this in place it wouldn't be necessary. Cross-subsystem series are unlikely to make it for v4.12 at this point, so here go my hopes to get this merged sometime soon... Thierry
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 05043071fc98..f74b61cfd113 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -431,6 +431,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 becb96c724fe..cad0eb2c3531 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -111,6 +111,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..87e02d9165d8 --- /dev/null +++ b/drivers/gpio/gpio-tegra186.c @@ -0,0 +1,636 @@ +/* + * Copyright (c) 2016-2017 NVIDIA Corporation + * + * Author: Thierry Reding <treding@nvidia.com> + * + * 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 { + const char *name; + unsigned int offset; + unsigned int pins; +}; + +struct tegra_gpio_soc { + const struct tegra_gpio_port *ports; + unsigned int num_ports; + const char *name; +}; + +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 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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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 = gpiochip_get_data(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) +{ + unsigned int i, j, irq, offset = 0; + struct tegra_gpio *gpio; + struct resource *res; + char **names; + 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 = gpio->soc->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; + + names = devm_kcalloc(gpio->gpio.parent, gpio->gpio.ngpio, + sizeof(*names), GFP_KERNEL); + if (!names) + return -ENOMEM; + + for (i = 0; i < gpio->soc->num_ports; i++) { + const struct tegra_gpio_port *port = &gpio->soc->ports[i]; + char *name; + + for (j = 0; j < port->pins; j++) { + name = devm_kasprintf(gpio->gpio.parent, GFP_KERNEL, + "P%s.%02x", port->name, j); + if (!name) + return -ENOMEM; + + names[offset + j] = name; + } + + offset += port->pins; + } + + gpio->gpio.names = (const char * const *)names; + + 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; +} + +#define TEGRA_MAIN_GPIO_PORT(port, base, count) \ + [TEGRA_MAIN_GPIO_PORT_##port] = { \ + .name = #port, \ + .offset = base, \ + .pins = count, \ + } + +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, + .name = "tegra186-gpio", +}; + +#define TEGRA_AON_GPIO_PORT(port, base, count) \ + [TEGRA_AON_GPIO_PORT_##port] = { \ + .name = #port, \ + .offset = base, \ + .pins = count, \ + } + +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, + .name = "tegra186-gpio-aon", +}; + +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 <treding@nvidia.com>"); +MODULE_LICENSE("GPL v2");