| Message ID | 20190624132531.6184-4-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4,v1] gpio: Add support for hierarchical IRQ domains | expand |
Hi Linus, On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > Use the new infrastructure for hierarchical irqchips in > gpiolib. > > I have no chance to test or debug this so I need > help. > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Thierry Reding <treding@nvidia.com> > Cc: Brian Masney <masneyb@onstation.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpio-uniphier.c | 172 +++++++++-------------------------- > 2 files changed, 45 insertions(+), 128 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index ee34716a39aa..806d642498a6 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -552,6 +552,7 @@ config GPIO_UNIPHIER > tristate "UniPhier GPIO support" > depends on ARCH_UNIPHIER || COMPILE_TEST > depends on OF_GPIO > + select GPIOLIB_IRQCHIP > select IRQ_DOMAIN_HIERARCHY > help > Say yes here to support UniPhier GPIOs. > diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c > index 93cdcc41e9fb..e960836b9c01 100644 > --- a/drivers/gpio/gpio-uniphier.c > +++ b/drivers/gpio/gpio-uniphier.c > @@ -6,7 +6,6 @@ > #include <linux/bits.h> > #include <linux/gpio/driver.h> > #include <linux/irq.h> > -#include <linux/irqdomain.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -30,7 +29,6 @@ > struct uniphier_gpio_priv { > struct gpio_chip chip; > struct irq_chip irq_chip; > - struct irq_domain *domain; > void __iomem *regs; > spinlock_t lock; > u32 saved_vals[0]; > @@ -162,49 +160,33 @@ static void uniphier_gpio_set_multiple(struct gpio_chip *chip, > } > } > > -static int uniphier_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > +static void uniphier_gpio_irq_mask(struct irq_data *d) > { > - struct irq_fwspec fwspec; > - > - if (offset < UNIPHIER_GPIO_IRQ_OFFSET) > - return -ENXIO; > - > - fwspec.fwnode = of_node_to_fwnode(chip->parent->of_node); > - fwspec.param_count = 2; > - fwspec.param[0] = offset - UNIPHIER_GPIO_IRQ_OFFSET; > - /* > - * IRQ_TYPE_NONE is rejected by the parent irq domain. Set LEVEL_HIGH > - * temporarily. Anyway, ->irq_set_type() will override it later. > - */ > - fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH; > - > - return irq_create_fwspec_mapping(&fwspec); > -} > - > -static void uniphier_gpio_irq_mask(struct irq_data *data) > -{ > - struct uniphier_gpio_priv *priv = data->chip_data; > - u32 mask = BIT(data->hwirq); > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); > + u32 mask = BIT(d->hwirq); > > uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, 0); > > - return irq_chip_mask_parent(data); > + return irq_chip_mask_parent(d); > } > > -static void uniphier_gpio_irq_unmask(struct irq_data *data) > +static void uniphier_gpio_irq_unmask(struct irq_data *d) Are you renaming 'data' -> 'd' just for your personal preference? > { > - struct uniphier_gpio_priv *priv = data->chip_data; > - u32 mask = BIT(data->hwirq); > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); > + u32 mask = BIT(d->hwirq); > > uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, mask); > > - return irq_chip_unmask_parent(data); > + return irq_chip_unmask_parent(d); > } > > -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type) > +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type) Again, this seems a noise change. > { > - struct uniphier_gpio_priv *priv = data->chip_data; > - u32 mask = BIT(data->hwirq); > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); > + u32 mask = BIT(d->hwirq); > u32 val = 0; > > if (type == IRQ_TYPE_EDGE_BOTH) { > @@ -216,7 +198,7 @@ static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type) > /* To enable both edge detection, the noise filter must be enabled. */ > uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_FLT_EN, mask, val); > > - return irq_chip_set_type_parent(data, type); > + return irq_chip_set_type_parent(d, type); > } > > static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv, > @@ -245,82 +227,6 @@ static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv, > return -ENOENT; > } > > -static int uniphier_gpio_irq_domain_translate(struct irq_domain *domain, > - struct irq_fwspec *fwspec, > - unsigned long *out_hwirq, > - unsigned int *out_type) > -{ > - if (WARN_ON(fwspec->param_count < 2)) > - return -EINVAL; > - > - *out_hwirq = fwspec->param[0]; > - *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; > - > - return 0; > -} > - > -static int uniphier_gpio_irq_domain_alloc(struct irq_domain *domain, > - unsigned int virq, > - unsigned int nr_irqs, void *arg) > -{ > - struct uniphier_gpio_priv *priv = domain->host_data; > - struct irq_fwspec parent_fwspec; > - irq_hw_number_t hwirq; > - unsigned int type; > - int ret; > - > - if (WARN_ON(nr_irqs != 1)) > - return -EINVAL; > - > - ret = uniphier_gpio_irq_domain_translate(domain, arg, &hwirq, &type); > - if (ret) > - return ret; > - > - ret = uniphier_gpio_irq_get_parent_hwirq(priv, hwirq); > - if (ret < 0) > - return ret; > - > - /* parent is UniPhier AIDET */ > - parent_fwspec.fwnode = domain->parent->fwnode; > - parent_fwspec.param_count = 2; > - parent_fwspec.param[0] = ret; > - parent_fwspec.param[1] = (type == IRQ_TYPE_EDGE_BOTH) ? > - IRQ_TYPE_EDGE_FALLING : type; > - > - ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, > - &priv->irq_chip, priv); > - if (ret) > - return ret; > - > - return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); > -} > - > -static int uniphier_gpio_irq_domain_activate(struct irq_domain *domain, > - struct irq_data *data, bool early) > -{ > - struct uniphier_gpio_priv *priv = domain->host_data; > - struct gpio_chip *chip = &priv->chip; > - > - return gpiochip_lock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET); > -} > - > -static void uniphier_gpio_irq_domain_deactivate(struct irq_domain *domain, > - struct irq_data *data) > -{ > - struct uniphier_gpio_priv *priv = domain->host_data; > - struct gpio_chip *chip = &priv->chip; > - > - gpiochip_unlock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET); > -} I did not test this patch, but probably it would break my board. ->(de)activate hook has offset UNIPHIER_GPIO_IRQ_OFFSET (=120), but you are replacing it with generic gpiochip_irq_domain_activate, which as zero offset. > ret = devm_gpiochip_add_data(dev, chip, priv); > if (ret) > return ret; > > - priv->domain = irq_domain_create_hierarchy( > - parent_domain, 0, > - UNIPHIER_GPIO_IRQ_MAX_NUM, You are replacing UNIPHIER_GPIO_IRQ_MAX_NUM with gc->ngpio, which will much more irqs than needed. Is it possible to provide more flexibility? -- Best Regards Masahiro Yamada
Hi Masahiro, thanks for your review! On Thu, Jul 18, 2019 at 1:10 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > -static void uniphier_gpio_irq_unmask(struct irq_data *data) > > +static void uniphier_gpio_irq_unmask(struct irq_data *d) > > Are you renaming 'data' -> 'd' > just for your personal preference? Yes, I am still looking for proof of what kind of terseness gives the optimal perceptive qualities in written code, so I have only intuitive ideas about what is the easiest code to read and maintain. But it's your file, and since you seem not to like it I will back out this change. > > -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type) > > +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > Again, this seems a noise change. Contrary to popular belief, coding style changes while writing new code is not universally seen as "noise", actually the opposite, sending pure code style changes as singular patches, is seen as noise. See the following paragraph from Documentation/process/4.Coding.rst: "pure coding style fixes are seen as noise by the development community; they tend to get a chilly reception. So this type of patch is best avoided. It is natural to fix the style of a piece of code while working on it for other reasons, but coding style changes should not be made for their own sake." Given the number of pure coding style patches I get, one could believe this does not apply ... but I try to be accepting of it anyway. > I did not test this patch, but probably it would break my board. Oh too bad, let's see if we can make it more plausible to work (I will not apply it while it is in RFT state). > ->(de)activate hook has offset UNIPHIER_GPIO_IRQ_OFFSET (=120), > but you are replacing it with generic gpiochip_irq_domain_activate, > which as zero offset. Ah! Brian gave me the tool to fix this properly, I will try to iterate and get this right. > > - priv->domain = irq_domain_create_hierarchy( > > - parent_domain, 0, > > - UNIPHIER_GPIO_IRQ_MAX_NUM, > > You are replacing UNIPHIER_GPIO_IRQ_MAX_NUM with gc->ngpio, > which will much more irqs than needed. > > Is it possible to provide more flexibility? UNIPHIER_GPIO_IRQ_MAX_NUM is 24 and ngpio comes from the device tree and is compulsory. The current device trees have: arch/arm/boot/dts/uniphier-ld4.dtsi: ngpios = <136>; arch/arm/boot/dts/uniphier-pro4.dtsi: ngpios = <248>; arch/arm/boot/dts/uniphier-pro5.dtsi: ngpios = <248>; arch/arm/boot/dts/uniphier-pxs2.dtsi: ngpios = <232>; arch/arm/boot/dts/uniphier-sld8.dtsi: ngpios = <136>; So I suppose that you mean that since only 24 GPIOs can ever have assigned IRQs, making headroom for say 248 is a waste of resources. However irq descriptors are dynamically allocated, so saying that the irqchip can have 24 descriptors rather than 248 is not going to save any memory. What you might want is to only allow offset 0..23 to be mapped to irqs. If I understand correctly this is how the hardware works: the first 24 GPIOs have IRQs, the rest don't, is that right? We have a facility for that: struct gpio_irq_chip field .valid_mask I will try to come up with a separate patch for that so you can see if it works. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index ee34716a39aa..806d642498a6 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -552,6 +552,7 @@ config GPIO_UNIPHIER tristate "UniPhier GPIO support" depends on ARCH_UNIPHIER || COMPILE_TEST depends on OF_GPIO + select GPIOLIB_IRQCHIP select IRQ_DOMAIN_HIERARCHY help Say yes here to support UniPhier GPIOs. diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c index 93cdcc41e9fb..e960836b9c01 100644 --- a/drivers/gpio/gpio-uniphier.c +++ b/drivers/gpio/gpio-uniphier.c @@ -6,7 +6,6 @@ #include <linux/bits.h> #include <linux/gpio/driver.h> #include <linux/irq.h> -#include <linux/irqdomain.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> @@ -30,7 +29,6 @@ struct uniphier_gpio_priv { struct gpio_chip chip; struct irq_chip irq_chip; - struct irq_domain *domain; void __iomem *regs; spinlock_t lock; u32 saved_vals[0]; @@ -162,49 +160,33 @@ static void uniphier_gpio_set_multiple(struct gpio_chip *chip, } } -static int uniphier_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) +static void uniphier_gpio_irq_mask(struct irq_data *d) { - struct irq_fwspec fwspec; - - if (offset < UNIPHIER_GPIO_IRQ_OFFSET) - return -ENXIO; - - fwspec.fwnode = of_node_to_fwnode(chip->parent->of_node); - fwspec.param_count = 2; - fwspec.param[0] = offset - UNIPHIER_GPIO_IRQ_OFFSET; - /* - * IRQ_TYPE_NONE is rejected by the parent irq domain. Set LEVEL_HIGH - * temporarily. Anyway, ->irq_set_type() will override it later. - */ - fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH; - - return irq_create_fwspec_mapping(&fwspec); -} - -static void uniphier_gpio_irq_mask(struct irq_data *data) -{ - struct uniphier_gpio_priv *priv = data->chip_data; - u32 mask = BIT(data->hwirq); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); + u32 mask = BIT(d->hwirq); uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, 0); - return irq_chip_mask_parent(data); + return irq_chip_mask_parent(d); } -static void uniphier_gpio_irq_unmask(struct irq_data *data) +static void uniphier_gpio_irq_unmask(struct irq_data *d) { - struct uniphier_gpio_priv *priv = data->chip_data; - u32 mask = BIT(data->hwirq); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); + u32 mask = BIT(d->hwirq); uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, mask); - return irq_chip_unmask_parent(data); + return irq_chip_unmask_parent(d); } -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type) +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type) { - struct uniphier_gpio_priv *priv = data->chip_data; - u32 mask = BIT(data->hwirq); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); + u32 mask = BIT(d->hwirq); u32 val = 0; if (type == IRQ_TYPE_EDGE_BOTH) { @@ -216,7 +198,7 @@ static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type) /* To enable both edge detection, the noise filter must be enabled. */ uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_FLT_EN, mask, val); - return irq_chip_set_type_parent(data, type); + return irq_chip_set_type_parent(d, type); } static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv, @@ -245,82 +227,6 @@ static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv, return -ENOENT; } -static int uniphier_gpio_irq_domain_translate(struct irq_domain *domain, - struct irq_fwspec *fwspec, - unsigned long *out_hwirq, - unsigned int *out_type) -{ - if (WARN_ON(fwspec->param_count < 2)) - return -EINVAL; - - *out_hwirq = fwspec->param[0]; - *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; - - return 0; -} - -static int uniphier_gpio_irq_domain_alloc(struct irq_domain *domain, - unsigned int virq, - unsigned int nr_irqs, void *arg) -{ - struct uniphier_gpio_priv *priv = domain->host_data; - struct irq_fwspec parent_fwspec; - irq_hw_number_t hwirq; - unsigned int type; - int ret; - - if (WARN_ON(nr_irqs != 1)) - return -EINVAL; - - ret = uniphier_gpio_irq_domain_translate(domain, arg, &hwirq, &type); - if (ret) - return ret; - - ret = uniphier_gpio_irq_get_parent_hwirq(priv, hwirq); - if (ret < 0) - return ret; - - /* parent is UniPhier AIDET */ - parent_fwspec.fwnode = domain->parent->fwnode; - parent_fwspec.param_count = 2; - parent_fwspec.param[0] = ret; - parent_fwspec.param[1] = (type == IRQ_TYPE_EDGE_BOTH) ? - IRQ_TYPE_EDGE_FALLING : type; - - ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, - &priv->irq_chip, priv); - if (ret) - return ret; - - return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec); -} - -static int uniphier_gpio_irq_domain_activate(struct irq_domain *domain, - struct irq_data *data, bool early) -{ - struct uniphier_gpio_priv *priv = domain->host_data; - struct gpio_chip *chip = &priv->chip; - - return gpiochip_lock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET); -} - -static void uniphier_gpio_irq_domain_deactivate(struct irq_domain *domain, - struct irq_data *data) -{ - struct uniphier_gpio_priv *priv = domain->host_data; - struct gpio_chip *chip = &priv->chip; - - gpiochip_unlock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET); -} - -static const struct irq_domain_ops uniphier_gpio_irq_domain_ops = { - .alloc = uniphier_gpio_irq_domain_alloc, - .free = irq_domain_free_irqs_common, - .activate = uniphier_gpio_irq_domain_activate, - .deactivate = uniphier_gpio_irq_domain_deactivate, - .translate = uniphier_gpio_irq_domain_translate, -}; - static void uniphier_gpio_hw_init(struct uniphier_gpio_priv *priv) { /* @@ -338,6 +244,26 @@ static unsigned int uniphier_gpio_get_nbanks(unsigned int ngpio) return DIV_ROUND_UP(ngpio, UNIPHIER_GPIO_LINES_PER_BANK); } +static int uniphier_gpio_child_to_parent_hwirq(struct gpio_chip *gc, + unsigned int child, + unsigned int child_type, + unsigned int *parent, + unsigned int *parent_type) +{ + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc); + int ret; + + ret = uniphier_gpio_irq_get_parent_hwirq(priv, child); + if (ret < 0) + return ret; + *parent = ret; + if (child_type == IRQ_TYPE_EDGE_BOTH) + *parent_type = IRQ_TYPE_EDGE_FALLING; + else + *parent_type = child_type; + return 0; +} + static int uniphier_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -345,6 +271,7 @@ static int uniphier_gpio_probe(struct platform_device *pdev) struct irq_domain *parent_domain; struct uniphier_gpio_priv *priv; struct gpio_chip *chip; + struct gpio_irq_chip *girq; struct irq_chip *irq_chip; unsigned int nregs; u32 ngpios; @@ -386,7 +313,6 @@ static int uniphier_gpio_probe(struct platform_device *pdev) chip->get = uniphier_gpio_get; chip->set = uniphier_gpio_set; chip->set_multiple = uniphier_gpio_set_multiple; - chip->to_irq = uniphier_gpio_to_irq; chip->base = -1; chip->ngpio = ngpios; @@ -398,34 +324,25 @@ static int uniphier_gpio_probe(struct platform_device *pdev) irq_chip->irq_set_affinity = irq_chip_set_affinity_parent; irq_chip->irq_set_type = uniphier_gpio_irq_set_type; + girq = &chip->irq; + girq->chip = irq_chip; + girq->fwnode = of_node_to_fwnode(dev->of_node); + girq->parent_domain = parent_domain; + girq->child_to_parent_hwirq = uniphier_gpio_child_to_parent_hwirq; + girq->handler = handle_bad_irq; + girq->default_type = IRQ_TYPE_NONE; + uniphier_gpio_hw_init(priv); ret = devm_gpiochip_add_data(dev, chip, priv); if (ret) return ret; - priv->domain = irq_domain_create_hierarchy( - parent_domain, 0, - UNIPHIER_GPIO_IRQ_MAX_NUM, - of_node_to_fwnode(dev->of_node), - &uniphier_gpio_irq_domain_ops, priv); - if (!priv->domain) - return -ENOMEM; - platform_set_drvdata(pdev, priv); return 0; } -static int uniphier_gpio_remove(struct platform_device *pdev) -{ - struct uniphier_gpio_priv *priv = platform_get_drvdata(pdev); - - irq_domain_remove(priv->domain); - - return 0; -} - static int __maybe_unused uniphier_gpio_suspend(struct device *dev) { struct uniphier_gpio_priv *priv = dev_get_drvdata(dev); @@ -485,7 +402,6 @@ MODULE_DEVICE_TABLE(of, uniphier_gpio_match); static struct platform_driver uniphier_gpio_driver = { .probe = uniphier_gpio_probe, - .remove = uniphier_gpio_remove, .driver = { .name = "uniphier-gpio", .of_match_table = uniphier_gpio_match,
Use the new infrastructure for hierarchical irqchips in gpiolib. I have no chance to test or debug this so I need help. Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Thierry Reding <treding@nvidia.com> Cc: Brian Masney <masneyb@onstation.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-uniphier.c | 172 +++++++++-------------------------- 2 files changed, 45 insertions(+), 128 deletions(-) -- 2.21.0