Re: [PATCH v3 3/5] irqchip: ocelot: Add support for Luton platforms
From: Marc Zyngier <maz@kernel.org>
Date: 2020-11-18 12:38:52
Also in:
lkml
On 2020-11-16 16:24, Gregory CLEMENT wrote:
quoted hunk ↗ jump to hunk
This patch extends irqchip driver for oceleot to be used with an other vcoreiii base platform: Luton. Based on a larger patch from Lars Povlsen [off-list ref] Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- drivers/irqchip/irq-mscc-ocelot.c | 145 +++++++++++++++++++++++++----- 1 file changed, 123 insertions(+), 22 deletions(-)diff --git a/drivers/irqchip/irq-mscc-ocelot.cb/drivers/irqchip/irq-mscc-ocelot.c index 88143c0b700c..9964800c53c2 100644--- a/drivers/irqchip/irq-mscc-ocelot.c +++ b/drivers/irqchip/irq-mscc-ocelot.c@@ -12,39 +12,115 @@ #include <linux/irqchip/chained_irq.h> #include <linux/interrupt.h> -#define ICPU_CFG_INTR_INTR_STICKY 0x10 -#define ICPU_CFG_INTR_INTR_ENA 0x18 -#define ICPU_CFG_INTR_INTR_ENA_CLR 0x1c -#define ICPU_CFG_INTR_INTR_ENA_SET 0x20 -#define ICPU_CFG_INTR_DST_INTR_IDENT(x) (0x38 + 0x4 * (x)) -#define ICPU_CFG_INTR_INTR_TRIGGER(x) (0x5c + 0x4 * (x)) - -#define OCELOT_NR_IRQ 24 +#define ICPU_CFG_INTR_DST_INTR_IDENT(_p, x) (_p->reg_off_ident + 0x4 *(x)) +#define ICPU_CFG_INTR_INTR_TRIGGER(_p, x) (_p->reg_off_trigger + 0x4 * (x)) + +#define FLAGS_NEED_INIT_ENABLE BIT(0) +#define FLAGS_FORCE_LUTON_STYLE BIT(1) +#define FLAGS_HAS_TRIGGER BIT(2) + +struct chip_props { + u32 flags; + u32 reg_off_sticky; + u32 reg_off_ena; + u32 reg_off_ena_clr; + u32 reg_off_ena_set; + u32 reg_off_ident; + u32 reg_off_trigger; + u32 reg_off_force; + u32 reg_off_ena_irq0;
Do all these fields need to be u32s?
+ u32 n_irq;
+};
+
+static const struct chip_props ocelot_props = {
+ .flags = FLAGS_HAS_TRIGGER,
+ .reg_off_sticky = 0x10,
+ .reg_off_ena = 0x18,
+ .reg_off_ena_clr = 0x1c,
+ .reg_off_ena_set = 0x20,
+ .reg_off_ident = 0x38,
+ .reg_off_trigger = 0x5c,
+ .reg_off_force = 0xc,
+ .n_irq = 24,
+};
+
+static const struct chip_props luton_props = {
+ .flags = FLAGS_NEED_INIT_ENABLE |
+ FLAGS_FORCE_LUTON_STYLE,LUTON_STYLE doesn't quite convey the idea of what this implies. Please use a name that actually means something (other than the city north of London). And force what exactly?
+ .reg_off_sticky = 0, + .reg_off_ena = 0x4, + .reg_off_ena_clr = 0x8, + .reg_off_ena_set = 0xc, + .reg_off_ident = 0x18, + .reg_off_trigger = 0,
If this field doesn't exist (because the matching flag isn't set), don't list it.
+ .reg_off_force = 0x38, + .reg_off_ena_irq0 = 0x14, + .n_irq = 28, +};
Please split this patch in two: rework the Ocelot driver to the "new" style in one patch, then introduce new platform in another.
static void ocelot_irq_unmask(struct irq_data *data)
{
struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+ struct irq_domain *d = data->domain;
+ struct chip_props *p = d->host_data;
struct irq_chip_type *ct = irq_data_get_chip_type(data);
unsigned int mask = data->mask;
u32 val;
irq_gc_lock(gc);
- val = irq_reg_readl(gc, ICPU_CFG_INTR_INTR_TRIGGER(0)) |
- irq_reg_readl(gc, ICPU_CFG_INTR_INTR_TRIGGER(1));
- if (!(val & mask))
- irq_reg_writel(gc, mask, ICPU_CFG_INTR_INTR_STICKY);
+ if (p->flags & FLAGS_HAS_TRIGGER) {
+ val = irq_reg_readl(gc, ICPU_CFG_INTR_INTR_TRIGGER(p, 0)) |
+ irq_reg_readl(gc, ICPU_CFG_INTR_INTR_TRIGGER(p, 1));
+ if (!(val & mask))
+ irq_reg_writel(gc, mask, p->reg_off_sticky);
+ }
*ct->mask_cache &= ~mask;
- irq_reg_writel(gc, mask, ICPU_CFG_INTR_INTR_ENA_SET);
+ irq_reg_writel(gc, mask, p->reg_off_ena_set);
irq_gc_unlock(gc);
}
+static void luton_irq_force(struct irq_data *data,
+ struct irq_chip_generic *gc,
+ struct chip_props *p)
+{
+ int off = p->reg_off_force + (data->hwirq * sizeof(u32));
+ u32 val = irq_reg_readl(gc, off);
+
+ irq_reg_writel(gc, val | BIT(3), off);
+}
+
+static int ocelot_irq_force(struct irq_data *data,
+ enum irqchip_irq_state which, bool state)Please use a name that matches the same of the API.
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+ struct irq_domain *d = data->domain;
+ struct chip_props *p = d->host_data;
+ int ret = -EINVAL;
+
+ /* Only supports triggering */
+ if ((which == IRQCHIP_STATE_PENDING ||
+ which == IRQCHIP_STATE_ACTIVE) &&PENDING and ACTIVE are two very different things, and yet you handle them the same way. Which one does your interrupt controller support?
quoted hunk ↗ jump to hunk
+ state && p->reg_off_force) { + if (p->flags & FLAGS_FORCE_LUTON_STYLE) + /* Config register style */ + luton_irq_force(data, gc, p); + else + /* New, bitmask style */ + irq_reg_writel(gc, data->mask, p->reg_off_force); + ret = 0; + } + + return ret; +} + static void ocelot_irq_handler(struct irq_desc *desc) { struct irq_chip *chip = irq_desc_get_chip(desc); struct irq_domain *d = irq_desc_get_handler_data(desc); + struct chip_props *p = d->host_data; struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, 0); - u32 reg = irq_reg_readl(gc, ICPU_CFG_INTR_DST_INTR_IDENT(0)); + u32 reg = irq_reg_readl(gc, ICPU_CFG_INTR_DST_INTR_IDENT(p, 0)); chained_irq_enter(chip, desc);@@ -58,25 +134,28 @@ static void ocelot_irq_handler(struct irq_desc*desc) chained_irq_exit(chip, desc); } -static int __init ocelot_irq_init(struct device_node *node, - struct device_node *parent) +static int __init vcoreiii_irq_init(struct device_node *node, + struct device_node *parent, + const struct chip_props *p) { struct irq_domain *domain; struct irq_chip_generic *gc; int parent_irq, ret; + pr_info("%s: Load, %d irqs\n", node->name, p->n_irq);
If you must print something, use %pOFn, just like in the rest of the driver.
quoted hunk ↗ jump to hunk
+ parent_irq = irq_of_parse_and_map(node, 0); if (!parent_irq) return -EINVAL; - domain = irq_domain_add_linear(node, OCELOT_NR_IRQ, + domain = irq_domain_add_linear(node, p->n_irq, &irq_generic_chip_ops, NULL); if (!domain) { pr_err("%pOFn: unable to add irq domain\n", node); return -ENOMEM; } - ret = irq_alloc_domain_generic_chips(domain, OCELOT_NR_IRQ, 1, + ret = irq_alloc_domain_generic_chips(domain, p->n_irq, 1, "icpu", handle_level_irq, 0, 0, 0); if (ret) {@@ -92,16 +171,23 @@ static int __init ocelot_irq_init(structdevice_node *node, goto err_gc_free; } - gc->chip_types[0].regs.ack = ICPU_CFG_INTR_INTR_STICKY; - gc->chip_types[0].regs.mask = ICPU_CFG_INTR_INTR_ENA_CLR; + gc->chip_types[0].regs.ack = p->reg_off_sticky; + gc->chip_types[0].regs.mask = p->reg_off_ena_clr; gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit; gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit; gc->chip_types[0].chip.irq_unmask = ocelot_irq_unmask; + gc->chip_types[0].chip.irq_unmask = ocelot_irq_unmask; + gc->chip_types[0].chip.irq_set_irqchip_state = ocelot_irq_force; /* Mask and ack all interrupts */ - irq_reg_writel(gc, 0, ICPU_CFG_INTR_INTR_ENA); - irq_reg_writel(gc, 0xffffffff, ICPU_CFG_INTR_INTR_STICKY); + irq_reg_writel(gc, 0, p->reg_off_ena); + irq_reg_writel(gc, 0xffffffff, p->reg_off_sticky); + + /* Overall init */ + if (p->flags & FLAGS_NEED_INIT_ENABLE) + irq_reg_writel(gc, BIT(0), p->reg_off_ena_irq0); + domain->host_data = (void *)p;
Useless cast.
quoted hunk ↗ jump to hunk
irq_set_chained_handler_and_data(parent_irq, ocelot_irq_handler, domain);@@ -115,4 +201,19 @@ static int __init ocelot_irq_init(structdevice_node *node, return ret; } + +static int __init ocelot_irq_init(struct device_node *node, + struct device_node *parent) +{ + return vcoreiii_irq_init(node, parent, &ocelot_props); +} + IRQCHIP_DECLARE(ocelot_icpu, "mscc,ocelot-icpu-intr", ocelot_irq_init); + +static int __init luton_irq_init(struct device_node *node, + struct device_node *parent) +{ + return vcoreiii_irq_init(node, parent, &luton_props); +} + +IRQCHIP_DECLARE(luton_icpu, "mscc,luton-icpu-intr", luton_irq_init);
Thanks,
M.
--
Jazz is not dead. It just smells funny...