| Message ID | 20190125162302.14036-4-masneyb@onstation.org |
|---|---|
| State | New |
| Headers | show |
| Series | qcom: ssbi-gpio: add support for hierarchical IRQ chip | expand |
On Fri, 25 Jan 2019, Brian Masney wrote: > Convert the PM8XXX IRQ code to use the version 2 IRQ interface in order > to support hierarchical IRQ chips. This is necessary so that ssbi-gpio > can be setup as a hierarchical IRQ chip with PM8xxx as the parent. IRQ > chips in device tree should be usable from the start without the > having to make an additional call to gpio[d]_to_irq() to get the proper > IRQ on the parent. > > The IRQ handler was hardcoded as handle_level_irq and this patch > properly sets the handler to either handle_edge_irq or handle_level_irq > depending on the IRQ type. > > pm8821_irq_domain_ops and pm8821_irq_domain_map are removed by this > patch since the irq_chip is now contained in the pm_irq_data struct, and > that allows us to use a common IRQ mapping function. > > This change was not tested on any actual hardware, however the same > change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead) > phone. > > Signed-off-by: Brian Masney <masneyb@onstation.org> > --- > drivers/mfd/qcom-pm8xxx.c | 86 +++++++++++++++++++++++---------------- > 1 file changed, 50 insertions(+), 36 deletions(-) > > diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c > index e6e8d81c15fd..a976890c4019 100644 > --- a/drivers/mfd/qcom-pm8xxx.c > +++ b/drivers/mfd/qcom-pm8xxx.c > @@ -70,20 +70,20 @@ > #define PM8XXX_NR_IRQS 256 > #define PM8821_NR_IRQS 112 > > +struct pm_irq_data { > + int num_irqs; > + struct irq_chip *irq_chip; > + void (*irq_handler)(struct irq_desc *desc); > +}; > + > struct pm_irq_chip { > struct regmap *regmap; > spinlock_t pm_irq_lock; > struct irq_domain *irqdomain; > - unsigned int num_irqs; > unsigned int num_blocks; > unsigned int num_masters; > u8 config[0]; > -}; > - > -struct pm_irq_data { > - int num_irqs; > - const struct irq_domain_ops *irq_domain_ops; > - void (*irq_handler)(struct irq_desc *desc); > + const struct pm_irq_data *pm_irq_data; > }; > > static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp, > @@ -303,6 +303,7 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) > { > struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); > unsigned int pmirq = irqd_to_hwirq(d); > + irq_flow_handler_t flow_handler; > int irq_bit; > u8 block, config; > > @@ -316,6 +317,8 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) > chip->config[pmirq] &= ~PM_IRQF_MASK_RE; > if (flow_type & IRQF_TRIGGER_FALLING) > chip->config[pmirq] &= ~PM_IRQF_MASK_FE; > + > + flow_handler = handle_edge_irq; > } else { > chip->config[pmirq] |= PM_IRQF_LVL_SEL; > > @@ -323,8 +326,12 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) > chip->config[pmirq] &= ~PM_IRQF_MASK_RE; > else > chip->config[pmirq] &= ~PM_IRQF_MASK_FE; > + > + flow_handler = handle_level_irq; > } > > + irq_set_handler_locked(d, flow_handler); > + Why don't you save yourself 3 lines of code and a variable and just call irq_set_handler_locked() where you set flow_handler? Apart from that nit, the code looks good to me.
On Wed, Jan 30, 2019 at 01:27:39PM +0000, Lee Jones wrote: > > @@ -303,6 +303,7 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) > > { > > struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); > > unsigned int pmirq = irqd_to_hwirq(d); > > + irq_flow_handler_t flow_handler; > > int irq_bit; > > u8 block, config; > > > > @@ -316,6 +317,8 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) > > chip->config[pmirq] &= ~PM_IRQF_MASK_RE; > > if (flow_type & IRQF_TRIGGER_FALLING) > > chip->config[pmirq] &= ~PM_IRQF_MASK_FE; > > + > > + flow_handler = handle_edge_irq; > > } else { > > chip->config[pmirq] |= PM_IRQF_LVL_SEL; > > > > @@ -323,8 +326,12 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) > > chip->config[pmirq] &= ~PM_IRQF_MASK_RE; > > else > > chip->config[pmirq] &= ~PM_IRQF_MASK_FE; > > + > > + flow_handler = handle_level_irq; > > } > > > > + irq_set_handler_locked(d, flow_handler); > > + > > Why don't you save yourself 3 lines of code and a variable and just > call irq_set_handler_locked() where you set flow_handler? > > Apart from that nit, the code looks good to me. OK.... I'll fix this up in v2. I'll let this ssbi-gpio series sit on the list for a few weeks before I send out v2. I ordered a Sony Xperia Z on eBay but it won't be here until the beginning of March. I'd like to have this series tested on actual hardware before this series is merged just to be sure it works properly. If someone else has time to test it sooner, then please do. Brian
Hi Brian! Thanks for this patch! On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote: > The IRQ handler was hardcoded as handle_level_irq and this patch > properly sets the handler to either handle_edge_irq or handle_level_irq > depending on the IRQ type. Done like so: > + flow_handler = handle_edge_irq; This creates a problem and makes my kernel crash, because handle_edge_irq() *unconditionally* calls chip->irq_ack(), and since this chip does not implement .irq_ack(), it crashes. I tried to just add a void .irq_ack(). That instead made the kernel hang. If I just assign handle_level_irq() as before (just replaced all assignments of handle_edge_irq with handle_level_irq) it boots nicely, AND the interrupts are working fine on the ADC calibration path, and further readings of the ADC generates new IRQs, as expected! The handle_edge_irq and .irq_ack() call path is for hardware that require an explicit ACK (such as writing a bit in some register) when handling edge IRQs. This is necessary in some hardware because of the transient nature of edge IRQs. This is not necessary on some hardware that will simply clear the flag when something is reading the status register. I think this is the case with PM8xxx and you can safely use handle_level_irq with all IRQs. It looks counterintuitive and maybe the function handle_edge_irq() has a stupid name. Just drop this handle_edge_irq() business for now. This was tested on the APQ8060 DragonBoard. However this happens: [ 3.007727] mmci-pl18x 12180000.sdcc: ignoring dependency for device, assuming no driver [ 3.007970] mmci-pl18x 12200000.sdcc: ignoring dependency for device, assuming no driver [ 3.014984] mpu3050-i2c 1-0068: ignoring dependency for device, assuming no driver [ 3.023543] cm3605 cm3605: ignoring dependency for device, assuming no driver [ 3.030532] ak8975 1-000c: ignoring dependency for device, assuming no driver [ 3.037729] bmp280 1-0077: ignoring dependency for device, assuming no driver [ 3.044789] reg-fixed-voltage regulators:xc622a331mrg: ignoring dependency for device, assuming no driver [ 3.052485] smsc911x 1b800000.ethernet-ebi2: ignoring dependency for device, assuming no driver But I think that is because these all use ssbi-gpio interrupts and these are now handled properly so I will try to fix the device tree and send a separate patch that you can include. Yours, Linus Walleij
Hi Brian! I found one more bug in this patch, still not the last bug but I'm still digging around: On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote: > +struct pm_irq_data { > + int num_irqs; > + struct irq_chip *irq_chip; > + void (*irq_handler)(struct irq_desc *desc); > +}; > + > struct pm_irq_chip { > struct regmap *regmap; > spinlock_t pm_irq_lock; > struct irq_domain *irqdomain; > - unsigned int num_irqs; > unsigned int num_blocks; > unsigned int num_masters; > u8 config[0]; > -}; > - > -struct pm_irq_data { > - int num_irqs; > - const struct irq_domain_ops *irq_domain_ops; > - void (*irq_handler)(struct irq_desc *desc); > + const struct pm_irq_data *pm_irq_data; > }; This doesn't work: the config[0] must be the tail element of the struct since we allocate dynamically the trailing config[] bytes. As it looks now, the *pm_irq_data gets overwritten by the configs and it crashes. Yours, Linus Walleij
Hi Linus, On Wed, Feb 06, 2019 at 02:07:52PM +0100, Linus Walleij wrote: > > +struct pm_irq_data { > > + int num_irqs; > > + struct irq_chip *irq_chip; > > + void (*irq_handler)(struct irq_desc *desc); > > +}; > > + > > struct pm_irq_chip { > > struct regmap *regmap; > > spinlock_t pm_irq_lock; > > struct irq_domain *irqdomain; > > - unsigned int num_irqs; > > unsigned int num_blocks; > > unsigned int num_masters; > > u8 config[0]; > > -}; > > - > > -struct pm_irq_data { > > - int num_irqs; > > - const struct irq_domain_ops *irq_domain_ops; > > - void (*irq_handler)(struct irq_desc *desc); > > + const struct pm_irq_data *pm_irq_data; > > }; > > This doesn't work: the config[0] must be the tail element > of the struct since we allocate dynamically the trailing > config[] bytes. > > As it looks now, the *pm_irq_data gets overwritten by > the configs and it crashes. Thank you for testing all of this on actual hardware. You can either send out the little issues like this that need corrected and I'll collect everything up, and send out a V2 once you are done with testing. Or, you can just take my patches, incorporate the fixes, and add me with a Co-developed-by tag to the relevant patches. Whatever is easier for you. I assume that the latter approach may be easier since you're already making the changes in your tree for testing. Brian
On Wed, Feb 6, 2019 at 3:10 PM Brian Masney <masneyb@onstation.org> wrote: > Thank you for testing all of this on actual hardware. You can either > send out the little issues like this that need corrected and I'll > collect everything up, and send out a V2 once you are done with testing. > Or, you can just take my patches, incorporate the fixes, and add me > with a Co-developed-by tag to the relevant patches. Whatever is easier > for you. I assume that the latter approach may be easier since you're > already making the changes in your tree for testing. Well there are some remarks from Marc you need to address in v2 as well so you better keep the master patch set for now. I can copy/paste the diff I have though, but there is some bug I still need to dig into... Yours, Linus Walleij
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c index e6e8d81c15fd..a976890c4019 100644 --- a/drivers/mfd/qcom-pm8xxx.c +++ b/drivers/mfd/qcom-pm8xxx.c @@ -70,20 +70,20 @@ #define PM8XXX_NR_IRQS 256 #define PM8821_NR_IRQS 112 +struct pm_irq_data { + int num_irqs; + struct irq_chip *irq_chip; + void (*irq_handler)(struct irq_desc *desc); +}; + struct pm_irq_chip { struct regmap *regmap; spinlock_t pm_irq_lock; struct irq_domain *irqdomain; - unsigned int num_irqs; unsigned int num_blocks; unsigned int num_masters; u8 config[0]; -}; - -struct pm_irq_data { - int num_irqs; - const struct irq_domain_ops *irq_domain_ops; - void (*irq_handler)(struct irq_desc *desc); + const struct pm_irq_data *pm_irq_data; }; static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp, @@ -303,6 +303,7 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) { struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d); unsigned int pmirq = irqd_to_hwirq(d); + irq_flow_handler_t flow_handler; int irq_bit; u8 block, config; @@ -316,6 +317,8 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) chip->config[pmirq] &= ~PM_IRQF_MASK_RE; if (flow_type & IRQF_TRIGGER_FALLING) chip->config[pmirq] &= ~PM_IRQF_MASK_FE; + + flow_handler = handle_edge_irq; } else { chip->config[pmirq] |= PM_IRQF_LVL_SEL; @@ -323,8 +326,12 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type) chip->config[pmirq] &= ~PM_IRQF_MASK_RE; else chip->config[pmirq] &= ~PM_IRQF_MASK_FE; + + flow_handler = handle_level_irq; } + irq_set_handler_locked(d, flow_handler); + config = chip->config[pmirq] | PM_IRQF_CLR; return pm8xxx_config_irq(chip, block, config); } @@ -375,21 +382,45 @@ static struct irq_chip pm8xxx_irq_chip = { .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE, }; -static int pm8xxx_irq_domain_map(struct irq_domain *d, unsigned int irq, - irq_hw_number_t hwirq) +static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip, + struct irq_domain *domain, unsigned int irq, + irq_hw_number_t hwirq, unsigned int type) { - struct pm_irq_chip *chip = d->host_data; + irq_flow_handler_t handler; + + if (type & IRQ_TYPE_EDGE_BOTH) + handler = handle_edge_irq; + else + handler = handle_level_irq; - irq_set_chip_and_handler(irq, &pm8xxx_irq_chip, handle_level_irq); - irq_set_chip_data(irq, chip); + irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip, + chip, handler, NULL, NULL); irq_set_noprobe(irq); +} + +static int pm8xxx_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *data) +{ + struct pm_irq_chip *chip = domain->host_data; + struct irq_fwspec *fwspec = data; + irq_hw_number_t hwirq; + unsigned int type; + int ret, i; + + ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type); + if (ret) + return ret; + + for (i = 0; i < nr_irqs; i++) + pm8xxx_irq_domain_map(chip, domain, virq + i, hwirq + i, type); return 0; } static const struct irq_domain_ops pm8xxx_irq_domain_ops = { - .xlate = irq_domain_xlate_twocell, - .map = pm8xxx_irq_domain_map, + .alloc = pm8xxx_irq_domain_alloc, + .free = irq_domain_free_irqs_common, + .translate = irq_domain_translate_twocell, }; static void pm8821_irq_mask_ack(struct irq_data *d) @@ -473,23 +504,6 @@ static struct irq_chip pm8821_irq_chip = { .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE, }; -static int pm8821_irq_domain_map(struct irq_domain *d, unsigned int irq, - irq_hw_number_t hwirq) -{ - struct pm_irq_chip *chip = d->host_data; - - irq_set_chip_and_handler(irq, &pm8821_irq_chip, handle_level_irq); - irq_set_chip_data(irq, chip); - irq_set_noprobe(irq); - - return 0; -} - -static const struct irq_domain_ops pm8821_irq_domain_ops = { - .xlate = irq_domain_xlate_twocell, - .map = pm8821_irq_domain_map, -}; - static const struct regmap_config ssbi_regmap_config = { .reg_bits = 16, .val_bits = 8, @@ -501,13 +515,13 @@ static const struct regmap_config ssbi_regmap_config = { static const struct pm_irq_data pm8xxx_data = { .num_irqs = PM8XXX_NR_IRQS, - .irq_domain_ops = &pm8xxx_irq_domain_ops, + .irq_chip = &pm8xxx_irq_chip, .irq_handler = pm8xxx_irq_handler, }; static const struct pm_irq_data pm8821_data = { .num_irqs = PM8821_NR_IRQS, - .irq_domain_ops = &pm8821_irq_domain_ops, + .irq_chip = &pm8821_irq_chip, .irq_handler = pm8821_irq_handler, }; @@ -571,14 +585,14 @@ static int pm8xxx_probe(struct platform_device *pdev) platform_set_drvdata(pdev, chip); chip->regmap = regmap; - chip->num_irqs = data->num_irqs; - chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8); + chip->num_blocks = DIV_ROUND_UP(data->num_irqs, 8); chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8); + chip->pm_irq_data = data; spin_lock_init(&chip->pm_irq_lock); chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node, data->num_irqs, - data->irq_domain_ops, + &pm8xxx_irq_domain_ops, chip); if (!chip->irqdomain) return -ENODEV;
Convert the PM8XXX IRQ code to use the version 2 IRQ interface in order to support hierarchical IRQ chips. This is necessary so that ssbi-gpio can be setup as a hierarchical IRQ chip with PM8xxx as the parent. IRQ chips in device tree should be usable from the start without the having to make an additional call to gpio[d]_to_irq() to get the proper IRQ on the parent. The IRQ handler was hardcoded as handle_level_irq and this patch properly sets the handler to either handle_edge_irq or handle_level_irq depending on the IRQ type. pm8821_irq_domain_ops and pm8821_irq_domain_map are removed by this patch since the irq_chip is now contained in the pm_irq_data struct, and that allows us to use a common IRQ mapping function. This change was not tested on any actual hardware, however the same change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead) phone. Signed-off-by: Brian Masney <masneyb@onstation.org> --- drivers/mfd/qcom-pm8xxx.c | 86 +++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 36 deletions(-)