lkml.org 
[lkml]   [2018]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2018-02-02 17:40    [W:0.104 / U:0.704 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site