* (unknown), [not found] <[PATCH v2 0/2]: auto request GPIO as input if used as IRQ via DT> @ 2013-06-21 22:50 ` Javier Martinez Canillas 2013-06-21 22:50 ` [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT Javier Martinez Canillas 2013-06-21 22:50 ` [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT Javier Martinez Canillas 0 siblings, 2 replies; 6+ messages in thread From: Javier Martinez Canillas @ 2013-06-21 22:50 UTC (permalink / raw) To: Grant Likely Cc: jgchunter, Santosh Shilimkar, Kevin Hilman, Linus Walleij, Jean-Christophe PLAGNIOL-VILLARD, eballetbo, thomas.petazzoni, linux-omap, Florian Vaussard When an OMAP GPIO is used as an IRQ line, a call to gpio_request() has to be made to initialize the OMAP GPIO bank before a driver request the IRQ. Otherwise the call to request_irq() fails. Drivers should not be aware of this neither care wether an IRQ line is a GPIO or not. They should just request the IRQ and this has to be handled by the irq_chip driver. With the current OMAP GPIO DT binding, if we define: gpio6: gpio@49058000 { compatible = "ti,omap3-gpio"; reg = <0x49058000 0x200>; interrupts = <34>; ti,hwmods = "gpio6"; gpio-controller; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; }; interrupt-parent = <&gpio6>; interrupts = <16 8>; The GPIO is correctly mapped as an IRQ but a call to gpio_request() is never made. Ideally this has to be handled by the IRQ core and there are some work-in-progress to add this logic to the core but until this general solution gets into mainline we need to solve this on a per irq_chip driver basis. Some drivers solve this by calling gpio_request() using a custom .xlate function handler. But .xlate could get called many times while the irq domain .map function handler is called just once when a IRQ mapping is created with a call to irq_create_mapping(). This patch-set adds a custom .map function handler for the gpio-omap irq_chip driver that automatically call gpio_request() when a IRQ mapping is created for the GPIO line used as interrupt. This is just a temporary solution (a.k.a a hack) until a kernel wide approach is implemented and added to mainline. The patch-set is composed of the following patches: [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT This was tested on an OMAP3 DM3735 board (IGEPv2) and all the supported peripherals are working correctly with both legacy and DT booting. Further testing will be highly appreciated. Many thanks to Jon Hunter and Grant Likely for their feedback and suggestions on how to solve this. Best regards, Javier ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT 2013-06-21 22:50 ` (unknown), Javier Martinez Canillas @ 2013-06-21 22:50 ` Javier Martinez Canillas 2013-06-24 12:53 ` Grant Likely 2013-06-21 22:50 ` [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT Javier Martinez Canillas 1 sibling, 1 reply; 6+ messages in thread From: Javier Martinez Canillas @ 2013-06-21 22:50 UTC (permalink / raw) To: Grant Likely Cc: jgchunter, Santosh Shilimkar, Kevin Hilman, Linus Walleij, Jean-Christophe PLAGNIOL-VILLARD, eballetbo, thomas.petazzoni, linux-omap, Florian Vaussard, Javier Martinez Canillas When a GPIO is defined as an interrupt line using Device Tree, a call to irq_create_of_mapping() is made that calls irq_create_mapping(). So, is not necessary to do the mapping on the OMAP GPIO platform_driver and in fact is wrong to assume that all GPIO lines will be used as an IRQ. Add a custom IRQ domain .map function handler that will be called by the IRQ core to map only the GPIO lines used as IRQ. This also allows to execute needed setup code such as configuring a GPIO as input and enabling the GPIO bank. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- Changes since v1: - Split the addition of the .map function handler and the automatic gpio request in two different patches. - Add GPIO IRQ setup logic to the irq domain mapping function. - Only call irq_create_mapping for every GPIO on legacy boot. - Only setup a GPIO IRQ on the .map function for DeviceTree boot. drivers/gpio/gpio-omap.c | 52 ++++++++++++++++++++++++++++++++++++--------- 1 files changed, 41 insertions(+), 11 deletions(-) diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index d3f7d2d..31cbe65 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1068,16 +1068,18 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) gpiochip_add(&bank->chip); - for (j = 0; j < bank->width; j++) { - int irq = irq_create_mapping(bank->domain, j); - irq_set_lockdep_class(irq, &gpio_lock_class); - irq_set_chip_data(irq, bank); - if (bank->is_mpuio) { - omap_mpuio_alloc_gc(bank, irq, bank->width); - } else { - irq_set_chip_and_handler(irq, &gpio_irq_chip, - handle_simple_irq); - set_irq_flags(irq, IRQF_VALID); + if (!of_have_populated_dt()) { + for (j = 0; j < bank->width; j++) { + int irq = irq_create_mapping(bank->domain, j); + irq_set_lockdep_class(irq, &gpio_lock_class); + irq_set_chip_data(irq, bank); + if (bank->is_mpuio) { + omap_mpuio_alloc_gc(bank, irq, bank->width); + } else { + irq_set_chip_and_handler(irq, &gpio_irq_chip, + handle_simple_irq); + set_irq_flags(irq, IRQF_VALID); + } } } irq_set_chained_handler(bank->irq, gpio_irq_handler); @@ -1086,6 +1088,34 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) static const struct of_device_id omap_gpio_match[]; +static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, + irq_hw_number_t hwirq) +{ + struct gpio_bank *bank = d->host_data; + + if (!bank) + return -EINVAL; + + if (of_have_populated_dt()) { + irq_set_lockdep_class(virq, &gpio_lock_class); + irq_set_chip_data(virq, bank); + if (bank->is_mpuio) { + omap_mpuio_alloc_gc(bank, virq, bank->width); + } else { + irq_set_chip_and_handler(virq, &gpio_irq_chip, + handle_simple_irq); + set_irq_flags(virq, IRQF_VALID); + } + } + + return 0; +} + +static struct irq_domain_ops omap_gpio_irq_ops = { + .xlate = irq_domain_xlate_onetwocell, + .map = omap_gpio_irq_map, +}; + static int omap_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -1137,7 +1167,7 @@ static int omap_gpio_probe(struct platform_device *pdev) bank->domain = irq_domain_add_linear(node, bank->width, - &irq_domain_simple_ops, NULL); + &omap_gpio_irq_ops, bank); if (!bank->domain) return -ENODEV; -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT 2013-06-21 22:50 ` [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT Javier Martinez Canillas @ 2013-06-24 12:53 ` Grant Likely 0 siblings, 0 replies; 6+ messages in thread From: Grant Likely @ 2013-06-24 12:53 UTC (permalink / raw) Cc: jgchunter, Santosh Shilimkar, Kevin Hilman, Linus Walleij, Jean-Christophe PLAGNIOL-VILLARD, eballetbo, thomas.petazzoni, linux-omap, Florian Vaussard, Javier Martinez Canillas On Sat, 22 Jun 2013 00:50:53 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > When a GPIO is defined as an interrupt line using Device > Tree, a call to irq_create_of_mapping() is made that calls > irq_create_mapping(). So, is not necessary to do the mapping > on the OMAP GPIO platform_driver and in fact is wrong to > assume that all GPIO lines will be used as an IRQ. > > Add a custom IRQ domain .map function handler that will be > called by the IRQ core to map only the GPIO lines used as > IRQ. This also allows to execute needed setup code such as > configuring a GPIO as input and enabling the GPIO bank. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > > Changes since v1: > - Split the addition of the .map function handler and the > automatic gpio request in two different patches. > - Add GPIO IRQ setup logic to the irq domain mapping function. > - Only call irq_create_mapping for every GPIO on legacy boot. > - Only setup a GPIO IRQ on the .map function for DeviceTree boot. > > drivers/gpio/gpio-omap.c | 52 ++++++++++++++++++++++++++++++++++++--------- > 1 files changed, 41 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index d3f7d2d..31cbe65 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1068,16 +1068,18 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) > > gpiochip_add(&bank->chip); > > - for (j = 0; j < bank->width; j++) { > - int irq = irq_create_mapping(bank->domain, j); > - irq_set_lockdep_class(irq, &gpio_lock_class); > - irq_set_chip_data(irq, bank); > - if (bank->is_mpuio) { > - omap_mpuio_alloc_gc(bank, irq, bank->width); > - } else { > - irq_set_chip_and_handler(irq, &gpio_irq_chip, > - handle_simple_irq); > - set_irq_flags(irq, IRQF_VALID); > + if (!of_have_populated_dt()) { > + for (j = 0; j < bank->width; j++) { > + int irq = irq_create_mapping(bank->domain, j); > + irq_set_lockdep_class(irq, &gpio_lock_class); > + irq_set_chip_data(irq, bank); > + if (bank->is_mpuio) { > + omap_mpuio_alloc_gc(bank, irq, bank->width); > + } else { > + irq_set_chip_and_handler(irq, &gpio_irq_chip, > + handle_simple_irq); > + set_irq_flags(irq, IRQF_VALID); > + } > } > } > irq_set_chained_handler(bank->irq, gpio_irq_handler); > @@ -1086,6 +1088,34 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) > > static const struct of_device_id omap_gpio_match[]; > > +static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, > + irq_hw_number_t hwirq) > +{ > + struct gpio_bank *bank = d->host_data; > + > + if (!bank) > + return -EINVAL; > + > + if (of_have_populated_dt()) { > + irq_set_lockdep_class(virq, &gpio_lock_class); > + irq_set_chip_data(virq, bank); > + if (bank->is_mpuio) { > + omap_mpuio_alloc_gc(bank, virq, bank->width); > + } else { > + irq_set_chip_and_handler(virq, &gpio_irq_chip, > + handle_simple_irq); > + set_irq_flags(virq, IRQF_VALID); > + } > + } Actually, this looks wrong. You'll notice that the same block of code is now duplicated in the map function and outside the map. The problem is that the original code is manually walking through all the irqs and doing the mapping work, but all of that stuff is what the .map() hook is for! Instead, the entire of the above block should be executed unconditionally inside the map hook. In the DT case, it will get called automatically only on referenced irqs. In the non-DT case, the for loop in omap_gpio_chip_init() will only need to call irq_create_mapping() for each irq, which in turn will call ->map(). g. > + > + return 0; > +} > + > +static struct irq_domain_ops omap_gpio_irq_ops = { > + .xlate = irq_domain_xlate_onetwocell, > + .map = omap_gpio_irq_map, > +}; > + > static int omap_gpio_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1137,7 +1167,7 @@ static int omap_gpio_probe(struct platform_device *pdev) > > > bank->domain = irq_domain_add_linear(node, bank->width, > - &irq_domain_simple_ops, NULL); > + &omap_gpio_irq_ops, bank); > if (!bank->domain) > return -ENODEV; > > -- > 1.7.7.6 > -- email sent from notmuch.vim plugin ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT 2013-06-21 22:50 ` (unknown), Javier Martinez Canillas 2013-06-21 22:50 ` [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT Javier Martinez Canillas @ 2013-06-21 22:50 ` Javier Martinez Canillas 2013-06-24 12:55 ` Grant Likely 1 sibling, 1 reply; 6+ messages in thread From: Javier Martinez Canillas @ 2013-06-21 22:50 UTC (permalink / raw) To: Grant Likely Cc: jgchunter, Santosh Shilimkar, Kevin Hilman, Linus Walleij, Jean-Christophe PLAGNIOL-VILLARD, eballetbo, thomas.petazzoni, linux-omap, Florian Vaussard, Javier Martinez Canillas When an OMAP GPIO is used as an IRQ line, a call to gpio_request() has to be made to initialize the OMAP GPIO bank before a driver request the IRQ. Otherwise the call to request_irq() fails. Drives should not be aware of this neither care wether an IRQ line is a GPIO or not. They should just request the IRQ and this has to be handled by the irq_chip driver. With the current OMAP GPIO DT binding, if we define: gpio6: gpio@49058000 { compatible = "ti,omap3-gpio"; reg = <0x49058000 0x200>; interrupts = <34>; ti,hwmods = "gpio6"; gpio-controller; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; }; interrupt-parent = <&gpio6>; interrupts = <16 8>; The GPIO is correctly mapped as an IRQ but a call to gpio_request() is never made. Since a call to the custom IRQ domain .map function handler is made for each GPIO used as an IRQ, the GPIO can be setup and configured as input there automatically. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- Changes since v1: - Split the irq domain mapping function handler and the GPIO request in two different patches. drivers/gpio/gpio-omap.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 31cbe65..5ec6a00 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1092,6 +1092,8 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t hwirq) { struct gpio_bank *bank = d->host_data; + int gpio; + int ret; if (!bank) return -EINVAL; @@ -1106,6 +1108,13 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, handle_simple_irq); set_irq_flags(virq, IRQF_VALID); } + + gpio = irq_to_gpio(bank, hwirq); + ret = gpio_request_one(gpio, GPIOF_IN, NULL); + if (ret) { + dev_err(bank->dev, "Could not request GPIO%d\n", gpio); + return ret; + } } return 0; -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT 2013-06-21 22:50 ` [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT Javier Martinez Canillas @ 2013-06-24 12:55 ` Grant Likely 2013-06-24 13:58 ` Javier Martinez Canillas 0 siblings, 1 reply; 6+ messages in thread From: Grant Likely @ 2013-06-24 12:55 UTC (permalink / raw) Cc: jgchunter, Santosh Shilimkar, Kevin Hilman, Linus Walleij, Jean-Christophe PLAGNIOL-VILLARD, eballetbo, thomas.petazzoni, linux-omap, Florian Vaussard, Javier Martinez Canillas On Sat, 22 Jun 2013 00:50:54 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > When an OMAP GPIO is used as an IRQ line, a call to gpio_request() > has to be made to initialize the OMAP GPIO bank before a driver > request the IRQ. Otherwise the call to request_irq() fails. > > Drives should not be aware of this neither care wether an IRQ line > is a GPIO or not. They should just request the IRQ and this has to > be handled by the irq_chip driver. > > With the current OMAP GPIO DT binding, if we define: > > gpio6: gpio@49058000 { > compatible = "ti,omap3-gpio"; > reg = <0x49058000 0x200>; > interrupts = <34>; > ti,hwmods = "gpio6"; > gpio-controller; > #gpio-cells = <2>; > interrupt-controller; > #interrupt-cells = <2>; > }; > > interrupt-parent = <&gpio6>; > interrupts = <16 8>; > > The GPIO is correctly mapped as an IRQ but a call to gpio_request() > is never made. Since a call to the custom IRQ domain .map function > handler is made for each GPIO used as an IRQ, the GPIO can be setup > and configured as input there automatically. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > > Changes since v1: > - Split the irq domain mapping function handler and the GPIO > request in two different patches. > > drivers/gpio/gpio-omap.c | 9 +++++++++ > 1 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 31cbe65..5ec6a00 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1092,6 +1092,8 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, > irq_hw_number_t hwirq) > { > struct gpio_bank *bank = d->host_data; > + int gpio; > + int ret; > > if (!bank) > return -EINVAL; > @@ -1106,6 +1108,13 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, > handle_simple_irq); > set_irq_flags(virq, IRQF_VALID); > } > + > + gpio = irq_to_gpio(bank, hwirq); > + ret = gpio_request_one(gpio, GPIOF_IN, NULL); > + if (ret) { > + dev_err(bank->dev, "Could not request GPIO%d\n", gpio); > + return ret; > + } Following from my comment on patch 1, this is the only bit that you'd want to be conditional on the presence of a DT, not the whole block. g. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT 2013-06-24 12:55 ` Grant Likely @ 2013-06-24 13:58 ` Javier Martinez Canillas 0 siblings, 0 replies; 6+ messages in thread From: Javier Martinez Canillas @ 2013-06-24 13:58 UTC (permalink / raw) To: Grant Likely Cc: Javier Martinez Canillas, jgchunter, Santosh Shilimkar, Kevin Hilman, Linus Walleij, Jean-Christophe PLAGNIOL-VILLARD, eballetbo, thomas.petazzoni, linux-omap, Florian Vaussard On Mon, Jun 24, 2013 at 2:55 PM, Grant Likely <grant.likely@linaro.org> wrote: > On Sat, 22 Jun 2013 00:50:54 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >> When an OMAP GPIO is used as an IRQ line, a call to gpio_request() >> has to be made to initialize the OMAP GPIO bank before a driver >> request the IRQ. Otherwise the call to request_irq() fails. >> >> Drives should not be aware of this neither care wether an IRQ line >> is a GPIO or not. They should just request the IRQ and this has to >> be handled by the irq_chip driver. >> >> With the current OMAP GPIO DT binding, if we define: >> >> gpio6: gpio@49058000 { >> compatible = "ti,omap3-gpio"; >> reg = <0x49058000 0x200>; >> interrupts = <34>; >> ti,hwmods = "gpio6"; >> gpio-controller; >> #gpio-cells = <2>; >> interrupt-controller; >> #interrupt-cells = <2>; >> }; >> >> interrupt-parent = <&gpio6>; >> interrupts = <16 8>; >> >> The GPIO is correctly mapped as an IRQ but a call to gpio_request() >> is never made. Since a call to the custom IRQ domain .map function >> handler is made for each GPIO used as an IRQ, the GPIO can be setup >> and configured as input there automatically. >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> --- >> >> Changes since v1: >> - Split the irq domain mapping function handler and the GPIO >> request in two different patches. >> >> drivers/gpio/gpio-omap.c | 9 +++++++++ >> 1 files changed, 9 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index 31cbe65..5ec6a00 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -1092,6 +1092,8 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, >> irq_hw_number_t hwirq) >> { >> struct gpio_bank *bank = d->host_data; >> + int gpio; >> + int ret; >> >> if (!bank) >> return -EINVAL; >> @@ -1106,6 +1108,13 @@ static int omap_gpio_irq_map(struct irq_domain *d, unsigned int virq, >> handle_simple_irq); >> set_irq_flags(virq, IRQF_VALID); >> } >> + >> + gpio = irq_to_gpio(bank, hwirq); >> + ret = gpio_request_one(gpio, GPIOF_IN, NULL); >> + if (ret) { >> + dev_err(bank->dev, "Could not request GPIO%d\n", gpio); >> + return ret; >> + } > > Following from my comment on patch 1, this is the only bit that you'd > want to be conditional on the presence of a DT, not the whole block. > > g. > Hello Grant, Thanks a lot for your feedback. You are absolutely right, sorry for not realizing that before. I'll prepare a v3 of the patch-set addressing your comments and I will also include a patch that only converts gpio irq domain to linear mapping for OMAP2+ since Jon's patch was reverted due a regression on OMAP1 when using linear domain mapping [1]. Thanks a lot and best regards, Javier [1]: https://patchwork.kernel.org/patch/2594491/ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-24 13:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <[PATCH v2 0/2]: auto request GPIO as input if used as IRQ via DT>
2013-06-21 22:50 ` (unknown), Javier Martinez Canillas
2013-06-21 22:50 ` [PATCH v2 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT Javier Martinez Canillas
2013-06-24 12:53 ` Grant Likely
2013-06-21 22:50 ` [PATCH v2 2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT Javier Martinez Canillas
2013-06-24 12:55 ` Grant Likely
2013-06-24 13:58 ` Javier Martinez Canillas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox