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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
---
+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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
