| Message ID | 20190117003234.22127-8-masneyb@onstation.org |
|---|---|
| State | New |
| Headers | show |
| Series | qcom: spmi: add support for hierarchical IRQ chip | expand |
On 17/01/2019 00:32, Brian Masney wrote: > spmi-gpio did not have any irqchip support so consumers of this in > device tree would need to call gpio[d]_to_irq() in order to get the > proper IRQ on the underlying PMIC. IRQ chips in device tree should > be usable from the start without the consumer having to make an > additional call to get the proper IRQ on the parent. This patch adds > hierarchical IRQ chip support to the spmi-gpio code to correct this > issue. > > Driver was tested using the volume buttons (via gpio-keys) on the LG > Nexus 5 (hammerhead) phone with the following two configurations. > > volume-up { > interrupts-extended = <&pm8941_gpios 2 IRQ_TYPE_EDGE_BOTH>; > ... > }; > > volume-up { > gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>; > ... > }; > > Both configurations now show that spmi-gpio is the IRQ domain and that > the IRQ is setup in a hierarchy. > > $ grep volume_up /proc/interrupts > 72: 6 0 spmi-gpio 1 Edge volume_up > > $ cat /sys/kernel/debug/irq/irqs/72 > handler: handle_edge_irq > device: (null) > status: 0x00000403 > _IRQ_NOPROBE > istate: 0x00000000 > ddepth: 0 > wdepth: 0 > dstate: 0x02400203 > IRQ_TYPE_EDGE_RISING > IRQ_TYPE_EDGE_FALLING > IRQD_ACTIVATED > IRQD_IRQ_STARTED > node: 0 > affinity: 0-3 > effectiv: > domain: :soc:spmi@fc4cf000:pm8941@0:gpios@c000 > hwirq: 0x1 > chip: spmi-gpio > flags: 0x4 > IRQCHIP_MASK_ON_SUSPEND > parent: > domain: :soc:spmi@fc4cf000 > hwirq: 0xc100057 > chip: pmic_arb > flags: 0x4 > IRQCHIP_MASK_ON_SUSPEND > > Signed-off-by: Brian Masney <masneyb@onstation.org> > Reviewed-by: Stephen Boyd <sboyd@kernel.org> > --- > Changes since v4: > - None > > Changes since v3: > - None > > Changes since v2: > - Use PMIC_GPIO_PHYSICAL_OFFSET instead of the 1 constant > - Use gpiochip_irq_domain_{activate,deactivate} > - Changed 'fwspec->param[0] + 0xc0 - 1' to 'hwirq + c0' in call to > irq_domain_alloc_irqs_parent > > Changes since v1: > - Use two cells for interrupts instead of four. > - Pin numbers in interrupts-extended are now one based instead of zero > based so that they match the GPIO pin number. > - Drop unnecessary parenthesis in pmic_gpio_domain_translate > - Add missing of_node_put() > - Remove irq field from pmic_gpio_pad struct that is no longer > necessary. > > drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 111 +++++++++++++++++++++-- > 1 file changed, 101 insertions(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > index b6fa2c7dbb26..3604c3cdbbc0 100644 > --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c > @@ -12,6 +12,7 @@ > */ > > #include <linux/gpio/driver.h> > +#include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_irq.h> > @@ -136,7 +137,6 @@ enum pmic_gpio_func_index { > /** > * struct pmic_gpio_pad - keep current GPIO settings > * @base: Address base in SPMI device. > - * @irq: IRQ number which this GPIO generate. > * @is_enabled: Set to false when GPIO should be put in high Z state. > * @out_value: Cached pin output value > * @have_buffer: Set to true if GPIO output could be configured in push-pull, > @@ -156,7 +156,6 @@ enum pmic_gpio_func_index { > */ > struct pmic_gpio_pad { > u16 base; > - int irq; > bool is_enabled; > bool out_value; > bool have_buffer; > @@ -179,6 +178,8 @@ struct pmic_gpio_state { > struct regmap *map; > struct pinctrl_dev *ctrl; > struct gpio_chip chip; > + struct fwnode_handle *fwnode; > + struct irq_domain *domain; > }; > > static const struct pinconf_generic_params pmic_gpio_bindings[] = { > @@ -761,11 +762,14 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip, > static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin) > { > struct pmic_gpio_state *state = gpiochip_get_data(chip); > - struct pmic_gpio_pad *pad; > + struct irq_fwspec fwspec; > > - pad = state->ctrl->desc->pins[pin].drv_data; > + fwspec.fwnode = state->fwnode; > + fwspec.param_count = 2; > + fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET; > + fwspec.param[1] = IRQ_TYPE_NONE; In my experience, IRQ_TYPE_NONE is rarely a good thing, unless you expect the trigger information to be found by some other mean. I guess that's one of the reasons why everything falls back to level in the SPMI driver... Thanks, M.
Hi Marc, On Thu, Jan 17, 2019 at 11:32:01AM +0000, Marc Zyngier wrote: > > static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin) > > { > > struct pmic_gpio_state *state = gpiochip_get_data(chip); > > - struct pmic_gpio_pad *pad; > > + struct irq_fwspec fwspec; > > > > - pad = state->ctrl->desc->pins[pin].drv_data; > > + fwspec.fwnode = state->fwnode; > > + fwspec.param_count = 2; > > + fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET; > > + fwspec.param[1] = IRQ_TYPE_NONE; > > In my experience, IRQ_TYPE_NONE is rarely a good thing, unless you > expect the trigger information to be found by some other mean. I guess > that's one of the reasons why everything falls back to level in the SPMI > driver... I'm not sure how to determine what trigger to put here. I thought that it would be up to the caller of request_any_context_irq() to explicitly set the expected trigger type when a GPIO is used, which will overwrite IRQ_TYPE_NONE with the proper trigger type. For example, I've tested the hierarchical IRQ domains with gpio-keys and when the gpio property is used, devm_request_any_context_irq() is called with the flags IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING. This calls __setup_irq(), which will call irq_set_type() and overwrite the trigger type. irq_set_type() is only called when the IRQ is not shared, so I'm not sure if this would work as expected with a shared IRQ. Brian
On 18/01/2019 12:42, Brian Masney wrote: > Hi Marc, > > On Thu, Jan 17, 2019 at 11:32:01AM +0000, Marc Zyngier wrote: >>> static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin) >>> { >>> struct pmic_gpio_state *state = gpiochip_get_data(chip); >>> - struct pmic_gpio_pad *pad; >>> + struct irq_fwspec fwspec; >>> >>> - pad = state->ctrl->desc->pins[pin].drv_data; >>> + fwspec.fwnode = state->fwnode; >>> + fwspec.param_count = 2; >>> + fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET; >>> + fwspec.param[1] = IRQ_TYPE_NONE; >> >> In my experience, IRQ_TYPE_NONE is rarely a good thing, unless you >> expect the trigger information to be found by some other mean. I guess >> that's one of the reasons why everything falls back to level in the SPMI >> driver... > > I'm not sure how to determine what trigger to put here. I thought that > it would be up to the caller of request_any_context_irq() to explicitly > set the expected trigger type when a GPIO is used, which will overwrite > IRQ_TYPE_NONE with the proper trigger type. The main issue is that IRQ_TYPE_NONE is a bit loosely defined, and mostly means "keep whatever was there before", which is a bit like rolling a dice each time you allocate an interrupt. > > For example, I've tested the hierarchical IRQ domains with gpio-keys and > when the gpio property is used, devm_request_any_context_irq() is called > with the flags IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING. This calls > __setup_irq(), which will call irq_set_type() and overwrite the trigger > type. > > irq_set_type() is only called when the IRQ is not shared, so I'm not > sure if this would work as expected with a shared IRQ. I'd suggest you force the type to a "safe" value such as rising edge, and let the irq_set_type() call do the right thing, assuming you've plugged the issue in the core SPMI driver. Thanks, M.
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c index b6fa2c7dbb26..3604c3cdbbc0 100644 --- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c +++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c @@ -12,6 +12,7 @@ */ #include <linux/gpio/driver.h> +#include <linux/interrupt.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_irq.h> @@ -136,7 +137,6 @@ enum pmic_gpio_func_index { /** * struct pmic_gpio_pad - keep current GPIO settings * @base: Address base in SPMI device. - * @irq: IRQ number which this GPIO generate. * @is_enabled: Set to false when GPIO should be put in high Z state. * @out_value: Cached pin output value * @have_buffer: Set to true if GPIO output could be configured in push-pull, @@ -156,7 +156,6 @@ enum pmic_gpio_func_index { */ struct pmic_gpio_pad { u16 base; - int irq; bool is_enabled; bool out_value; bool have_buffer; @@ -179,6 +178,8 @@ struct pmic_gpio_state { struct regmap *map; struct pinctrl_dev *ctrl; struct gpio_chip chip; + struct fwnode_handle *fwnode; + struct irq_domain *domain; }; static const struct pinconf_generic_params pmic_gpio_bindings[] = { @@ -761,11 +762,14 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip, static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin) { struct pmic_gpio_state *state = gpiochip_get_data(chip); - struct pmic_gpio_pad *pad; + struct irq_fwspec fwspec; - pad = state->ctrl->desc->pins[pin].drv_data; + fwspec.fwnode = state->fwnode; + fwspec.param_count = 2; + fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET; + fwspec.param[1] = IRQ_TYPE_NONE; - return pad->irq; + return irq_create_fwspec_mapping(&fwspec); } static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) @@ -935,8 +939,78 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state, return 0; } +static struct irq_chip pmic_gpio_irq_chip = { + .name = "spmi-gpio", + .irq_ack = irq_chip_ack_parent, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_set_type = irq_chip_set_type_parent, + .irq_set_wake = irq_chip_set_wake_parent, + .flags = IRQCHIP_MASK_ON_SUSPEND, +}; + +static int pmic_gpio_domain_translate(struct irq_domain *domain, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + struct pmic_gpio_state *state = container_of(domain->host_data, + struct pmic_gpio_state, + chip); + + if (fwspec->param_count != 2 || fwspec->param[0] >= state->chip.ngpio) + return -EINVAL; + + *hwirq = fwspec->param[0] - PMIC_GPIO_PHYSICAL_OFFSET; + *type = fwspec->param[1]; + + return 0; +} + +static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *data) +{ + struct pmic_gpio_state *state = container_of(domain->host_data, + struct pmic_gpio_state, + chip); + struct irq_fwspec *fwspec = data; + struct irq_fwspec parent_fwspec; + irq_hw_number_t hwirq; + unsigned int type; + int ret, i; + + ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type); + if (ret) + return ret; + + for (i = 0; i < nr_irqs; i++) + irq_domain_set_info(domain, virq + i, hwirq + i, + &pmic_gpio_irq_chip, state, + handle_level_irq, NULL, NULL); + + parent_fwspec.fwnode = domain->parent->fwnode; + parent_fwspec.param_count = 4; + parent_fwspec.param[0] = 0; + parent_fwspec.param[1] = hwirq + 0xc0; + parent_fwspec.param[2] = 0; + parent_fwspec.param[3] = fwspec->param[1]; + + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, + &parent_fwspec); +} + +static const struct irq_domain_ops pmic_gpio_domain_ops = { + .activate = gpiochip_irq_domain_activate, + .alloc = pmic_gpio_domain_alloc, + .deactivate = gpiochip_irq_domain_deactivate, + .free = irq_domain_free_irqs_common, + .translate = pmic_gpio_domain_translate, +}; + static int pmic_gpio_probe(struct platform_device *pdev) { + struct irq_domain *parent_domain; + struct device_node *parent_node; struct device *dev = &pdev->dev; struct pinctrl_pin_desc *pindesc; struct pinctrl_desc *pctrldesc; @@ -993,10 +1067,6 @@ static int pmic_gpio_probe(struct platform_device *pdev) pindesc->number = i; pindesc->name = pmic_gpio_groups[i]; - pad->irq = platform_get_irq(pdev, i); - if (pad->irq < 0) - return pad->irq; - pad->base = reg + i * PMIC_GPIO_ADDRESS_RANGE; ret = pmic_gpio_populate(state, pad); @@ -1016,10 +1086,28 @@ static int pmic_gpio_probe(struct platform_device *pdev) if (IS_ERR(state->ctrl)) return PTR_ERR(state->ctrl); + parent_node = of_irq_find_parent(state->dev->of_node); + if (!parent_node) + return -ENXIO; + + parent_domain = irq_find_host(parent_node); + of_node_put(parent_node); + if (!parent_domain) + return -ENXIO; + + state->fwnode = of_node_to_fwnode(state->dev->of_node); + state->domain = irq_domain_create_hierarchy(parent_domain, 0, + state->chip.ngpio, + state->fwnode, + &pmic_gpio_domain_ops, + &state->chip); + if (!state->domain) + return -ENODEV; + ret = gpiochip_add_data(&state->chip, state); if (ret) { dev_err(state->dev, "can't add gpio chip\n"); - return ret; + goto err_chip_add_data; } /* @@ -1045,6 +1133,8 @@ static int pmic_gpio_probe(struct platform_device *pdev) err_range: gpiochip_remove(&state->chip); +err_chip_add_data: + irq_domain_remove(state->domain); return ret; } @@ -1053,6 +1143,7 @@ static int pmic_gpio_remove(struct platform_device *pdev) struct pmic_gpio_state *state = platform_get_drvdata(pdev); gpiochip_remove(&state->chip); + irq_domain_remove(state->domain); return 0; }
spmi-gpio did not have any irqchip support so consumers of this in device tree would need to call gpio[d]_to_irq() in order to get the proper IRQ on the underlying PMIC. IRQ chips in device tree should be usable from the start without the consumer having to make an additional call to get the proper IRQ on the parent. This patch adds hierarchical IRQ chip support to the spmi-gpio code to correct this issue. Driver was tested using the volume buttons (via gpio-keys) on the LG Nexus 5 (hammerhead) phone with the following two configurations. volume-up { interrupts-extended = <&pm8941_gpios 2 IRQ_TYPE_EDGE_BOTH>; ... }; volume-up { gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>; ... }; Both configurations now show that spmi-gpio is the IRQ domain and that the IRQ is setup in a hierarchy. $ grep volume_up /proc/interrupts 72: 6 0 spmi-gpio 1 Edge volume_up $ cat /sys/kernel/debug/irq/irqs/72 handler: handle_edge_irq device: (null) status: 0x00000403 _IRQ_NOPROBE istate: 0x00000000 ddepth: 0 wdepth: 0 dstate: 0x02400203 IRQ_TYPE_EDGE_RISING IRQ_TYPE_EDGE_FALLING IRQD_ACTIVATED IRQD_IRQ_STARTED node: 0 affinity: 0-3 effectiv: domain: :soc:spmi@fc4cf000:pm8941@0:gpios@c000 hwirq: 0x1 chip: spmi-gpio flags: 0x4 IRQCHIP_MASK_ON_SUSPEND parent: domain: :soc:spmi@fc4cf000 hwirq: 0xc100057 chip: pmic_arb flags: 0x4 IRQCHIP_MASK_ON_SUSPEND Signed-off-by: Brian Masney <masneyb@onstation.org> Reviewed-by: Stephen Boyd <sboyd@kernel.org> --- Changes since v4: - None Changes since v3: - None Changes since v2: - Use PMIC_GPIO_PHYSICAL_OFFSET instead of the 1 constant - Use gpiochip_irq_domain_{activate,deactivate} - Changed 'fwspec->param[0] + 0xc0 - 1' to 'hwirq + c0' in call to irq_domain_alloc_irqs_parent Changes since v1: - Use two cells for interrupts instead of four. - Pin numbers in interrupts-extended are now one based instead of zero based so that they match the GPIO pin number. - Drop unnecessary parenthesis in pmic_gpio_domain_translate - Add missing of_node_put() - Remove irq field from pmic_gpio_pad struct that is no longer necessary. drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 111 +++++++++++++++++++++-- 1 file changed, 101 insertions(+), 10 deletions(-)