On Tue, Feb 06 2018 at 20:34 +0000, Marc Zyngier wrote:
On Tue, 06 Feb 2018 18:09:04 +0000,
Lina Iyer wrote:
+#define PDC_MAX_IRQS 126
From v2: "Is that an absolute, architectural maximum? Or should it
come from the DT (being the sum of all ranges that are provided by
this PDC)?"
Architectural maximum. Sorry I forgot to respond earlier.
+static inline void pdc_reg_write(int reg, u32 i, u32 val)
+{
+ writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
+}
+
+static inline u32 pdc_reg_read(int reg, u32 i)
+{
+ return readl_relaxed(pdc_base + reg + i * sizeof(u32));
+}
+
+static inline void pdc_enable_intr(struct irq_data *d, bool on)
You can loose the "inline" on these 3 function, I believe the compiler
will do a pretty good job specialising them.
Ok.
+ * 3'b0 00 Level sensitive active low LOW
+ * 3'b0 01 Rising edge sensitive NOT USED
+ * 3'b0 10 Falling edge sensitive LOW
+ * 3'b0 11 Dual Edge sensitive NOT USED
+ * 3'b1 00 Level senstive active High HIGH
sensitive
Ok
+
+static struct irq_chip qcom_pdc_gic_chip = {
const?
+ .name = "pdc-gic",
Just call it "PDC".
OK to both.
+ .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
Is this supposed to work on any architecture other than arm64? If not,
then you can loose the CONFIG_SMP, as there is no !SMP config on arm64.
Only ARM64 afaict. But who knows ? :)
+};
+
+static irq_hw_number_t get_parent_hwirq(int pin)
+{
+ int i;
+ struct pdc_pin_region *region;
+
+ for (i = 0; i < pdc_region_cnt; i++) {
+ region = &pdc_region[i];
+ if (pin >= region->pin_base &&
+ pin < region->pin_base + region->cnt)
+ return (region->parent_base + pin - region->pin_base);
+ }
+
+ WARN_ON(1);
+ return pin;
Do not return a valid value in case of error. Please return an error
value that you will check in the caller. Otherwise you're feeding the
GIC with a SPI number that is actually a PDC pin number. That is not
going to have any positive effect.
How about the max of irq_hw_number_t ?
+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_parent_hwirq(hwirq);
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ &qcom_pdc_gic_chip,
+ (void *)parent_hwirq);
Nothing else in this driver should be concerned with the parent hwirq,
so NULL should be an appropriate value for chip_data.
Yes, I forgot to remove it. I don't need this anymore.
+ region = kcalloc(n, sizeof(*region), GFP_KERNEL);
+ if (!region)
+ return -ENOMEM;
+
+ ret = of_property_read_u32_array(np, "qcom,pdc-ranges", region, n);
+ if (ret) {
+ kfree(region);
+ return -EINVAL;
+ }
+
+ pdc_region = (struct pdc_pin_region *)region;
Oh please... Don't resort to that kind of hack. Next thing you know,
someone will add a u8 field to that structure and you'll spend the
following 2 hours trying to understand why it all went so wrong.
Allocate a bounce buffer if you want, copy fields one by one, I don't
care. Just don't do that. :-(
:) ok.
+ pdc_region_cnt = n / 3;
+
+ return 0;
+}
+
+int qcom_pdc_init(struct device_node *node, struct device_node *parent)
static.
OK.
+{
+ struct irq_domain *parent_domain, *pdc_domain;
+ int ret;
+
+ pdc_base = of_iomap(node, 0);
+ if (!pdc_base) {
+ pr_err("%s: unable to map PDC registers\n", node->full_name);
+ return -ENXIO;
+ }
+
+ parent_domain = irq_find_host(parent);
+ if (!parent_domain) {
+ pr_err("%s: unable to obtain PDC parent domain\n",
+ node->full_name);
Use %pOF instead of %s and node->full_name in all occurrences in this
function (see commit ce4fecf1fe15).
Sure.
Thanks,
Once again thanks Marc.
-- Lina
