Messages in this thread | | | Date | Fri, 2 Feb 2018 16:40:07 +0000 | From | Lina Iyer <> | Subject | Re: [PATCH RFC v2 1/3] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs |
| |
On Fri, Feb 02 2018 at 16:21 +0000, Marc Zyngier wrote: >Hi Lina, > >On 02/02/18 14:21, Lina Iyer wrote: >> From : Archana Sathyakumar <asathyak@codeaurora.org> >> >> The Power Domain Controller (PDC) on QTI SoCs like SDM845 houses an >> interrupt controller along with other domain control functions to handle >> interrupt related functions like handle falling edge or active low which >> are not detected at the GIC and handle wakeup interrupts. >> >> The interrupt controller is on an always-on domain for the purpose of >> waking up the processor. Only a subset of the processor's interrupts are >> routed through the PDC to the GIC. The PDC powers on the processors' >> domain, when in low power mode and replays pending interrupts so the GIC >> may wake up the processor. >> >> Signed-off-by: Archana Sathyakumar <asathyak@codeaurora.org> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> +struct pdc_pin_data { >> + int pin; >> + int hwirq; >> +}; > >I seriously doubt you need this structure, see below. > >> + >> +static DEFINE_SPINLOCK(pdc_lock); >> +static void __iomem *pdc_base; > >You only have one of those per SoC? > There is only one that Linux can control.
>> +static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type) >> +{ >> + int pin_out = d->hwirq; >> + enum pdc_irq_config_bits pdc_type; >> + >> + switch (type) { >> + case IRQ_TYPE_EDGE_RISING: >> + pdc_type = PDC_RISING_EDGE; >> + type = IRQ_TYPE_EDGE_RISING; >> + break; >> + case IRQ_TYPE_EDGE_FALLING: >> + pdc_type = PDC_FALLING_EDGE; >> + type = IRQ_TYPE_EDGE_RISING; >> + break; >> + case IRQ_TYPE_EDGE_BOTH: >> + pdc_type = PDC_DUAL_EDGE; >> + break; >> + case IRQ_TYPE_LEVEL_HIGH: >> + pdc_type = PDC_POLARITY_HIGH; >> + type = IRQ_TYPE_LEVEL_HIGH; >> + break; >> + case IRQ_TYPE_LEVEL_LOW: >> + pdc_type = PDC_POLARITY_LOW; >> + type = IRQ_TYPE_LEVEL_HIGH; >> + break; >> + default: >> + pdc_type = PDC_POLARITY_HIGH; >> + break; > >If this default clause triggers, something is pretty wrong. You may want >to warn and bail out instead. > The hardware defaults to this. I can bail out as well.
>> + } >> + >> + pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type); >> + >> + return irq_chip_set_type_parent(d, type); >> +} >> + >> +static struct irq_chip qcom_pdc_gic_chip = { >> + .name = "pdc-gic", >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_mask = qcom_pdc_gic_mask, >> + .irq_unmask = qcom_pdc_gic_unmask, >> + .irq_retrigger = irq_chip_retrigger_hierarchy, >> + .irq_set_type = qcom_pdc_gic_set_type, >> + .flags = IRQCHIP_MASK_ON_SUSPEND | >> + IRQCHIP_SET_TYPE_MASKED | >> + IRQCHIP_SKIP_SET_WAKE, >> + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, >> +#ifdef CONFIG_SMP >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> +#endif >> +}; >> + >> +static irq_hw_number_t get_irq_for_pin(int pin, struct pdc_pin_data *pdc_data) >> +{ >> + int i; >> + >> + for (i = 0; pdc_data[i].pin >= 0; i++) >> + if (pdc_data[i].pin == pin) >> + return pdc_data[i].hwirq; >> + >> + return pin; >> +} > >This is the function that irks me. The DT already gives you a nice set >of ranges with all the information you need. And yet, you expand the >whole thing into at most 127 structures, wasting memory and making the >search time a function of the number of interrupts instead of being that >of the number of regions. Not very nice. How about something like this >(untested): > Duh! Why didn't I think of this.. Thanks.
>struct pin_region { > u32 pin_base; > u32 parent_base; > u32 cnt; >}; > >struct pin_region *pin_regions; >int pin_region_count; > >static int pdc_pin_to_parent_hwirq(int pin) >{ > int i; > > for (i = 0; i < pin_region_count; i++) { > if (pin >= pin_regions[i].pin_base && > pin < (pin_regions[i].pin_base + pin_regions[i].cnt) > return (pin_regions[i].parent_base + pin - > pin_regions[i].pin_base); > } > > WARN(); > return -1; >} > >Less memory, less comparisons. > Agreed.
>> + >> +static int qcom_pdc_translate(struct irq_domain *d, >> + struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) >> +{ >> + if (is_of_node(fwspec->fwnode)) { >> + if (fwspec->param_count < 3) >> + return -EINVAL; > >Why 3? Reading the DT binding, this is indeed set to 3 without any >reason. I'd suggest this becomes 2, encoding the pin number and the >trigger information, as the leading 0 is quite useless. Yes, I know >other examples in the kernel are using this 0, and that was a >consequence of retrofitting the omitted interrupt controllers (back in >the days of the stupid gic_arch_extn...). Don't do that. > >> + >> + *hwirq = fwspec->param[1]; >> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > >... and fix this accordingly. > >> + return 0; >> + } >> + >> + return -EINVAL; >> +} >> + >> +static int qcom_pdc_alloc(struct irq_domain *domain, >> + unsigned int virq, unsigned int nr_irqs, void *data) >> +{ >> + struct irq_fwspec *fwspec = data; >> + struct irq_fwspec parent_fwspec; >> + irq_hw_number_t hwirq, parent_hwirq; >> + unsigned int type; >> + int ret; >> + >> + ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type); >> + if (ret) >> + return -EINVAL; >> + >> + parent_hwirq = get_irq_for_pin(hwirq, domain->host_data); >> + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, >> + &qcom_pdc_gic_chip, (void *)parent_hwirq); >> + if (ret) >> + return ret; >> + >> + if (type & IRQ_TYPE_EDGE_BOTH) >> + type = IRQ_TYPE_EDGE_RISING; >> + >> + if (type & IRQ_TYPE_LEVEL_MASK) >> + type = IRQ_TYPE_LEVEL_HIGH; >> + >> + parent_fwspec = *fwspec; >> + parent_fwspec.param[1] = parent_hwirq; >> + parent_fwspec.param[2] &= ~IRQ_TYPE_SENSE_MASK; >> + parent_fwspec.param[2] |= type; >> + parent_fwspec.fwnode = domain->parent->fwnode; > >This becomes: > > parent_fwspec.fwnode = domain->parent->fwnode; > parent_fwspec.param_count = 3; // That's what the GIC wants > parent_fwspec.param[0] = 0; //GIC_SPI > parent_fwspec.param[1] = parent_hwirq; > parent_fwspec.param[2] = type; > Sure.
>> + >> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, >> + &parent_fwspec); >> +} >> + >> +static const struct irq_domain_ops qcom_pdc_ops = { >> + .translate = qcom_pdc_translate, >> + .alloc = qcom_pdc_alloc, >> + .free = irq_domain_free_irqs_common, >> +}; >> + >> +static int pdc_setup_pin_mapping(struct device_node *np, >> + struct pdc_pin_data **data) >> +{ >> + int ret; >> + int n, i, j, k, pins = 0; >> + int *val; >> + struct pdc_pin_data *map; >> + >> + n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32)); >> + if (!n || n % 3) >> + return -EINVAL; >> + >> + val = kcalloc(n, sizeof(u32), GFP_KERNEL); >> + if (!val) >> + return -ENOMEM; >> + >> + ret = of_property_read_u32_array(np, "qcom,pdc-ranges", val, n); >> + if (ret) >> + return ret; > >Memory leak. > Ok.
>> + >> + for (i = 0; i < n; i += 3) >> + pins += val[i + 2]; >> + >> + if (pins > PDC_MAX_IRQS) >> + return -EFAULT; > >EFAULT is for an actual page fault. If you get one here, you have >bigger problems. EINVAL is better. is You're also leaking memory. > Ok. >> + >> + map = kcalloc(pins + 1, sizeof(*map), GFP_KERNEL); >> + if (!map) { >> + ret = -ENOMEM; >> + goto fail; >> + } >> + >> + for (i = 0, k = 0; i < n; i += 3) { >> + for (j = 0; j < val[i + 2]; j++, k++) { >> + map[k].pin = val[i] + j; >> + map[k].hwirq = val[i + 1] + j; >> + } >> + } > >That's the thing that needs to go. Just store the ranges as exposed by >the DT, maybe with some checking that there is no overlap in the >ranges (not that it matters much...). > Makes sense.
Thanks, Lina
| |