| Message ID | 20181229114755.8711-2-masneyb@onstation.org |
|---|---|
| State | New |
| Headers | show |
| Series | qcom: spmi: add support for hierarchical IRQ chip | expand |
Quoting Brian Masney (2018-12-29 03:47:53) > Convert the spmi-pmic-arb IRQ code to use the version 2 IRQ interface > in order to support hierarchical IRQ chips. Code was tested on a LG > Nexus 5 (hammerhead) phone. Can you add the motivation for this change here? Why should we care? > > Signed-off-by: Brian Masney <masneyb@onstation.org> > --- > drivers/spmi/spmi-pmic-arb.c | 91 +++++++++++++++++++++++++----------- > 1 file changed, 64 insertions(+), 27 deletions(-) > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > index 360b8218f322..c651d19f0623 100644 > --- a/drivers/spmi/spmi-pmic-arb.c > +++ b/drivers/spmi/spmi-pmic-arb.c > @@ -666,7 +666,8 @@ static int qpnpint_get_irqchip_state(struct irq_data *d, > return 0; > } > > -static int qpnpint_irq_request_resources(struct irq_data *d) > +static int qpnpint_irq_domain_activate(struct irq_domain *domain, > + struct irq_data *d, bool reserve) > { > struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d); > u16 periph = hwirq_to_per(d->hwirq); > @@ -692,36 +693,37 @@ static struct irq_chip pmic_arb_irqchip = { > .irq_set_type = qpnpint_irq_set_type, > .irq_set_wake = qpnpint_irq_set_wake, > .irq_get_irqchip_state = qpnpint_get_irqchip_state, > - .irq_request_resources = qpnpint_irq_request_resources, > .flags = IRQCHIP_MASK_ON_SUSPEND, > }; > > -static int qpnpint_irq_domain_dt_translate(struct irq_domain *d, > - struct device_node *controller, > - const u32 *intspec, > - unsigned int intsize, > - unsigned long *out_hwirq, > - unsigned int *out_type) > +static int qpnpint_irq_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *out_hwirq, > + unsigned int *out_type) > { > struct spmi_pmic_arb *pmic_arb = d->host_data; > u16 apid, ppid; > int rc; > > - dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n", > - intspec[0], intspec[1], intspec[2]); > + dev_dbg(&pmic_arb->spmic->dev, > + "param[0] 0x%1x param[1] 0x%02x param[2] 0x%02x\n", > + fwspec->param[0], fwspec->param[1], fwspec->param[2]); > > - if (irq_domain_get_of_node(d) != controller) > + if (irq_domain_get_of_node(d) != pmic_arb->spmic->dev.of_node) > return -EINVAL; > - if (intsize != 4) > + if (fwspec->param_count != 4) > return -EINVAL; > - if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7) > + if (fwspec->param[0] > 0xF || fwspec->param[1] > 0xFF || > + fwspec->param[2] > 0x7) > return -EINVAL; > > - ppid = intspec[0] << 8 | intspec[1]; > + ppid = fwspec->param[0] << 8 | fwspec->param[1]; > rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid); > if (rc < 0) { > - dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n", > - intspec[0], intspec[1], intspec[2], rc); > + dev_err(&pmic_arb->spmic->dev, > + "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n", > + fwspec->param[0], fwspec->param[1], fwspec->param[2], Why not assign a u32 *intspec to fwspec->param and then not change many lines in this function? Reducing the diff is useful to avoid glossing over some mistake in variable renames. > + rc); > return rc; > } > > @@ -732,25 +734,58 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d, > if (apid < pmic_arb->min_apid) > pmic_arb->min_apid = apid; > > - *out_hwirq = spec_to_hwirq(intspec[0], intspec[1], intspec[2], apid); > - *out_type = intspec[3] & IRQ_TYPE_SENSE_MASK; > + *out_hwirq = spec_to_hwirq(fwspec->param[0], fwspec->param[1], > + fwspec->param[2], apid); > + *out_type = fwspec->param[3] & IRQ_TYPE_SENSE_MASK; > > dev_dbg(&pmic_arb->spmic->dev, "out_hwirq = %lu\n", *out_hwirq); > > return 0; > } > > -static int qpnpint_irq_domain_map(struct irq_domain *d, > - unsigned int virq, > - irq_hw_number_t hwirq) > + > +static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb, > + struct irq_domain *domain, unsigned int virq, > + irq_hw_number_t hwirq) > { > - struct spmi_pmic_arb *pmic_arb = d->host_data; > + unsigned int old_virq; > > dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq); > > - irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq); > - irq_set_chip_data(virq, d->host_data); > - irq_set_noprobe(virq); > + /* > + * Check to see if the hwirq is already associated with another virq on > + * this IRQ domain. If so, then disassociate it before associating the > + * hwirq with the new virq. IRQs are all initially setup without an IRQ > + * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is > + * probed. Later in the boot process, an IRQ hierarchy is requested by > + * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new > + * virq. Is that because we count the irqs in the pmic gpio driver? It looks like we should remove that code from the driver then and use the compatible string to figure out how many gpios are present. It will make this comment and disassociate code unnecessary, but unfortunately it will mean we have to add all the compatible strings to the pmic gpio driver just so we know how many gpios are there. > + */ > + old_virq = irq_find_mapping(domain, hwirq); > + if (old_virq) > + irq_domain_disassociate(domain, old_virq); > + > + irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb, > + handle_level_irq, NULL, NULL); > +} > + > +static int qpnpint_irq_domain_alloc(struct irq_domain *domain, > + unsigned int virq, unsigned int nr_irqs, > + void *data) > +{ > + struct spmi_pmic_arb *pmic_arb = domain->host_data; > + struct irq_fwspec *fwspec = data; > + irq_hw_number_t hwirq; > + unsigned int type; > + int ret, i; > + > + ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type); > + if (ret) > + return ret; > + > + for (i = 0; i < nr_irqs; i++) > + qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i); > + > return 0; > } > Otherwise patch looks good to me. Thanks for working on this. Are you going to convert the ssbi master and gpio chip too?
On Fri, Jan 04, 2019 at 04:25:23PM -0800, Stephen Boyd wrote: > > + /* > > + * Check to see if the hwirq is already associated with another virq on > > + * this IRQ domain. If so, then disassociate it before associating the > > + * hwirq with the new virq. IRQs are all initially setup without an IRQ > > + * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is > > + * probed. Later in the boot process, an IRQ hierarchy is requested by > > + * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new > > + * virq. > > Is that because we count the irqs in the pmic gpio driver? Yes. The call to devm_of_platform_populate() in drivers/mfd/qcom-spmi-pmic.c also triggers this condition. I'll experiment with this some more this weekend. > Otherwise patch looks good to me. Thanks for working on this. Are you > going to convert the ssbi master and gpio chip too? Yes, I will do that work as well once this patch series is accepted. I currently don't have the hardware to test it, but I'm willing to pick up a cheap device on ebay. Any suggestions for something that boots a mainline kernel with the ssbi? It looks like the Sony Xperia Z phone is one of the supported devices. Thanks for your review on the other patches. It all makes sense. Brian
On Sat, Jan 5, 2019 at 2:45 AM Brian Masney <masneyb@onstation.org> wrote: > [Stephen] > > Otherwise patch looks good to me. Thanks for working on this. Are you > > going to convert the ssbi master and gpio chip too? > > Yes, I will do that work as well once this patch series is accepted. > I currently don't have the hardware to test it, but I'm willing to pick > up a cheap device on ebay. Any suggestions for something that boots a > mainline kernel with the ssbi? It looks like the Sony Xperia Z phone is > one of the supported devices. I think my APQ8060 DragonBoard uses SSBI: https://dflund.se/~triad/krad/dragonboard/ arch/arm/boot/dts/qcom-apq8060-dragonboard.dts So I can test it. Or even write the patch once the SSPI is merged and known working. Yours, Linus Walleij
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index 360b8218f322..c651d19f0623 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -666,7 +666,8 @@ static int qpnpint_get_irqchip_state(struct irq_data *d, return 0; } -static int qpnpint_irq_request_resources(struct irq_data *d) +static int qpnpint_irq_domain_activate(struct irq_domain *domain, + struct irq_data *d, bool reserve) { struct spmi_pmic_arb *pmic_arb = irq_data_get_irq_chip_data(d); u16 periph = hwirq_to_per(d->hwirq); @@ -692,36 +693,37 @@ static struct irq_chip pmic_arb_irqchip = { .irq_set_type = qpnpint_irq_set_type, .irq_set_wake = qpnpint_irq_set_wake, .irq_get_irqchip_state = qpnpint_get_irqchip_state, - .irq_request_resources = qpnpint_irq_request_resources, .flags = IRQCHIP_MASK_ON_SUSPEND, }; -static int qpnpint_irq_domain_dt_translate(struct irq_domain *d, - struct device_node *controller, - const u32 *intspec, - unsigned int intsize, - unsigned long *out_hwirq, - unsigned int *out_type) +static int qpnpint_irq_domain_translate(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *out_hwirq, + unsigned int *out_type) { struct spmi_pmic_arb *pmic_arb = d->host_data; u16 apid, ppid; int rc; - dev_dbg(&pmic_arb->spmic->dev, "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n", - intspec[0], intspec[1], intspec[2]); + dev_dbg(&pmic_arb->spmic->dev, + "param[0] 0x%1x param[1] 0x%02x param[2] 0x%02x\n", + fwspec->param[0], fwspec->param[1], fwspec->param[2]); - if (irq_domain_get_of_node(d) != controller) + if (irq_domain_get_of_node(d) != pmic_arb->spmic->dev.of_node) return -EINVAL; - if (intsize != 4) + if (fwspec->param_count != 4) return -EINVAL; - if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7) + if (fwspec->param[0] > 0xF || fwspec->param[1] > 0xFF || + fwspec->param[2] > 0x7) return -EINVAL; - ppid = intspec[0] << 8 | intspec[1]; + ppid = fwspec->param[0] << 8 | fwspec->param[1]; rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid); if (rc < 0) { - dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n", - intspec[0], intspec[1], intspec[2], rc); + dev_err(&pmic_arb->spmic->dev, + "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n", + fwspec->param[0], fwspec->param[1], fwspec->param[2], + rc); return rc; } @@ -732,25 +734,58 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d, if (apid < pmic_arb->min_apid) pmic_arb->min_apid = apid; - *out_hwirq = spec_to_hwirq(intspec[0], intspec[1], intspec[2], apid); - *out_type = intspec[3] & IRQ_TYPE_SENSE_MASK; + *out_hwirq = spec_to_hwirq(fwspec->param[0], fwspec->param[1], + fwspec->param[2], apid); + *out_type = fwspec->param[3] & IRQ_TYPE_SENSE_MASK; dev_dbg(&pmic_arb->spmic->dev, "out_hwirq = %lu\n", *out_hwirq); return 0; } -static int qpnpint_irq_domain_map(struct irq_domain *d, - unsigned int virq, - irq_hw_number_t hwirq) + +static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb, + struct irq_domain *domain, unsigned int virq, + irq_hw_number_t hwirq) { - struct spmi_pmic_arb *pmic_arb = d->host_data; + unsigned int old_virq; dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq); - irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq); - irq_set_chip_data(virq, d->host_data); - irq_set_noprobe(virq); + /* + * Check to see if the hwirq is already associated with another virq on + * this IRQ domain. If so, then disassociate it before associating the + * hwirq with the new virq. IRQs are all initially setup without an IRQ + * hierarchy when this driver is probed and when mfd/qcom-spmi-pmic.c is + * probed. Later in the boot process, an IRQ hierarchy is requested by + * pinctrl-spmi-gpio.c, and the same hwirq is now associated with a new + * virq. + */ + old_virq = irq_find_mapping(domain, hwirq); + if (old_virq) + irq_domain_disassociate(domain, old_virq); + + irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb, + handle_level_irq, NULL, NULL); +} + +static int qpnpint_irq_domain_alloc(struct irq_domain *domain, + unsigned int virq, unsigned int nr_irqs, + void *data) +{ + struct spmi_pmic_arb *pmic_arb = domain->host_data; + struct irq_fwspec *fwspec = data; + irq_hw_number_t hwirq; + unsigned int type; + int ret, i; + + ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type); + if (ret) + return ret; + + for (i = 0; i < nr_irqs; i++) + qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i); + return 0; } @@ -1126,8 +1161,10 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = { }; static const struct irq_domain_ops pmic_arb_irq_domain_ops = { - .map = qpnpint_irq_domain_map, - .xlate = qpnpint_irq_domain_dt_translate, + .activate = qpnpint_irq_domain_activate, + .alloc = qpnpint_irq_domain_alloc, + .free = irq_domain_free_irqs_common, + .translate = qpnpint_irq_domain_translate, }; static int spmi_pmic_arb_probe(struct platform_device *pdev)
Convert the spmi-pmic-arb IRQ code to use the version 2 IRQ interface in order to support hierarchical IRQ chips. Code was tested on a LG Nexus 5 (hammerhead) phone. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/spmi/spmi-pmic-arb.c | 91 +++++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 27 deletions(-)