Neil,

On 03/03/16 11:39, Neil Armstrong wrote:

As much as I hate macros (despite making a living out of writing
complicated/braindead ones), shoving random and unrelated values in an
enum doesn't make much sense. Please convert this to a set of #defines.

+
+/* Routines to acknowledge, disable and enable interrupts */
+static void rps_mask_irq(struct irq_data *d)
+{
+	u32 mask = BIT(d->hwirq);
+
+	iowrite32(mask, rps_data.base + RPS_MASK);

I do question the use of iowrite32 here (and its ioread32 pendent
anywhere else), as it actually translates in a writel, which contains a
memory barrier. Do you have any case that requires the use of such a
barrier? if not, consider switching to relaxed accessors (which are the

+}
+
+static void rps_unmask_irq(struct irq_data *d)
+{
+	u32 mask = BIT(d->hwirq);
+
+	iowrite32(mask, rps_data.base + RPS_UNMASK);
+}
+
+static void rps_ack_irq(struct irq_data *d)
+{
+	/* NOP */
+}

If that's a nop, you probably don't need it, see below.

+
+static void __exception_irq_entry handle_irq(struct pt_regs *regs)
+{
+	u32 irqstat;
+	int hwirq;
+
+	irqstat = ioread32(rps_data.base + RPS_STATUS);
+	hwirq = __ffs(irqstat);
+
+	do {
+		handle_IRQ(irq_find_mapping(rps_data.domain, hwirq), regs);

Please use handle_domain_irq() which will do the right thing (and save
you from RCU shouting at you).

+
+		irqstat = ioread32(rps_data.base + RPS_STATUS);
+		hwirq = __ffs(irqstat);
+	} while (irqstat);
+}

Can you get more that a single bit set in one read from the status
register? If so, you'd be better off handling all the pending interrupts
before reading from the MMIO again, since that's a slow operation.

+
+int __init rps_of_init(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+	struct irq_chip_generic *gc;
+
+	if (WARN_ON(!node))
+		return -ENODEV;
+
+	rps_data.base = of_iomap(node, 0);
+	WARN(!rps_data.base, "unable to map rps registers\n");
+
+	rps_data.domain = irq_domain_add_linear(node, RPS_IRQ_COUNT,
+						&irq_generic_chip_ops,
+						NULL);
+	if (!rps_data.domain) {
+		pr_err("%s: could add irq domain\n",
+		       node->full_name);
+		return -ENOMEM;
+	}
+
+	ret = irq_alloc_domain_generic_chips(rps_data.domain, RPS_IRQ_COUNT, 1,
+					     "RPS", handle_level_irq,

Given that all your interrupts are level triggered...

+					     0, 0, IRQ_GC_INIT_NESTED_LOCK);
+	if (ret) {
+		pr_err("%s: could not allocate generic chip\n",
+		       node->full_name);
+		irq_domain_remove(rps_data.domain);
+		return -EINVAL;
+	}
+
+	gc = irq_get_domain_generic_chip(rps_data.domain, 0);
+	gc->chip_types[0].chip.irq_ack = rps_ack_irq;

... I believe you can loose this callback, it is never used by the handler.

+	gc->chip_types[0].chip.irq_mask = rps_mask_irq;
+	gc->chip_types[0].chip.irq_unmask = rps_unmask_irq;
+
+	/* Disable all IRQs */
+	iowrite32(~0, rps_data.base + RPS_MASK);
+
+	set_handle_irq(handle_irq);
+
+	pr_info("Registered %d rps interrupts\n", RPS_IRQ_COUNT);

Given that this is always the same value, I don't think this is a very
useful message...

+
+	return 0;
+}
+
+IRQCHIP_DECLARE(nas782x, "plxtech,nas782x-rps", rps_of_init);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...