Message ID | 20200211190303.7991-1-mubin.usman.sayyed@xilinx.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] irqchip: xilinx: Add support for multiple instances | expand |
Mubin, Mubin Usman Sayyed <mubin.usman.sayyed@xilinx.com> writes: > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com> > > This patch adds support for multiple instances of git grep 'This patch' Documentation/process/submitting-patches.rst > xilinx interrupt controller. Below configurations are > supported by driver, > > - peripheral->xilinx-intc->xilinx-intc->gic > - peripheral->xilinx-intc->xilinx-intc This is really not much of an explanation. > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com> > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com> This Signed-off-by chain is incorrect. See chapter 11 and 12 in the same document. > @@ -38,29 +38,32 @@ struct xintc_irq_chip { > void __iomem *base; > struct irq_domain *root_domain; > u32 intr_mask; > + struct irq_chip *intc_dev; > + u32 nr_irq; > }; > > -static struct xintc_irq_chip *xintc_irqc; > +static struct xintc_irq_chip *primary_intc; > > -static void xintc_write(int reg, u32 data) > +static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data) > { > if (static_branch_unlikely(&xintc_is_be)) > - iowrite32be(data, xintc_irqc->base + reg); > + iowrite32be(data, irqc->base + reg); > else > - iowrite32(data, xintc_irqc->base + reg); > + iowrite32(data, irqc->base + reg); > } > > -static unsigned int xintc_read(int reg) > +static u32 xintc_read(struct xintc_irq_chip *irqc, int reg) > { > if (static_branch_unlikely(&xintc_is_be)) > - return ioread32be(xintc_irqc->base + reg); > + return ioread32be(irqc->base + reg); > else > - return ioread32(xintc_irqc->base + reg); > + return ioread32(irqc->base + reg); > } > > static void intc_enable_or_unmask(struct irq_data *d) > { > - unsigned long mask = 1 << d->hwirq; > + unsigned long mask = BIT(d->hwirq); > + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); Please order your local variables in reverse fir tree order: struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); unsigned long mask = BIT(d->hwirq); which is the preferred coding style in this subsystem and way simpler to read. > static void intc_mask_ack(struct irq_data *d) > { > - unsigned long mask = 1 << d->hwirq; > + unsigned long mask = BIT(d->hwirq); > + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); Ditto. > pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq); > - xintc_write(CIE, mask); > - xintc_write(IAR, mask); > + xintc_write(irqc, CIE, mask); > + xintc_write(irqc, IAR, mask); > } > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc) > +{ > + u32 hwirq; > + unsigned int irq = 0; Same. > + hwirq = xintc_read(irqc, IVR); > + if (hwirq != -1U) > + irq = irq_find_mapping(irqc->root_domain, hwirq); > + > + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq); Are these pr_debugs all over the please really required? I can understand that you use them for development, but are they useful once the stuff works? > + return irq; > +} > + > unsigned int xintc_get_irq(void) > { > - unsigned int hwirq, irq = -1; > + u32 hwirq; > + unsigned int irq = -1; See above. > - hwirq = xintc_read(IVR); > + hwirq = xintc_read(primary_intc, IVR); > if (hwirq != -1U) > - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq); > + irq = irq_find_mapping(primary_intc->root_domain, hwirq); > > pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq); > > @@ -138,12 +164,14 @@ static const struct irq_domain_ops xintc_irq_domain_ops = { > static void xil_intc_irq_handler(struct irq_desc *desc) > { > struct irq_chip *chip = irq_desc_get_chip(desc); > + struct xintc_irq_chip *irqc = > + irq_data_get_irq_handler_data(&desc->irq_data); Please avoid these ugly line breaks and put the initialization of the variable in to the code below the declaration. > /* Turn on the Master Enable. */ > - xintc_write(MER, MER_HIE | MER_ME); > - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) { > + xintc_write(irqc, MER, MER_HIE | MER_ME); > + if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) { > static_branch_enable(&xintc_is_be); I see it's existing logic, but this lacks a comment how it's determined that xintc is big endian. Looks like some weird "write works?" probing. Why? > + xintc_write(irqc, MER, MER_HIE | MER_ME); So this writes MER_HIE | MER_ME into MER > + if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) { but this checks just whether ONE of the bits is set. Shouldn't it check for MER == (MER_HIE | MER_ME), i.e. read back what was written? Thanks, tglx
Hi Thomas, Thanks for the review. Please see my inline replies. > -----Original Message----- > From: Thomas Gleixner <tglx@linutronix.de> > Sent: Thursday, February 13, 2020 5:31 PM > To: Mubin Usman Sayyed <MUBINUSM@xilinx.com>; > jason@lakedaemon.net; maz@kernel.org; Michal Simek > <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org; Siva Durga Prasad Paladugu > <sivadur@xilinx.com>; Anirudha Sarangi <anirudh@xilinx.com>; Mubin > Usman Sayyed <MUBINUSM@xilinx.com> > Subject: Re: [PATCH v3] irqchip: xilinx: Add support for multiple instances > > Mubin, > > Mubin Usman Sayyed <mubin.usman.sayyed@xilinx.com> writes: > > > From: Mubin Sayyed <mubin.usman.sayyed@xilinx.com> > > > > This patch adds support for multiple instances of > > git grep 'This patch' Documentation/process/submitting-patches.rst [Mubin]: I will re-phrase it in next version. > > > xilinx interrupt controller. Below configurations are supported by > > driver, > > > > - peripheral->xilinx-intc->xilinx-intc->gic > > - peripheral->xilinx-intc->xilinx-intc > > This is really not much of an explanation. [Mubin]: Will elaborate in next version > > > Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xilinx.com> > > Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xilinx.com> > > This Signed-off-by chain is incorrect. See chapter 11 and 12 in the same > document. [Mubin]: Sure, I will check and fix it. I ran checkpatch, it didn't reported errors/warning related to that. > > > @@ -38,29 +38,32 @@ struct xintc_irq_chip { > > void __iomem *base; > > struct irq_domain *root_domain; > > u32 intr_mask; > > + struct irq_chip *intc_dev; > > + u32 nr_irq; > > }; > > > > -static struct xintc_irq_chip *xintc_irqc; > > +static struct xintc_irq_chip *primary_intc; > > > > -static void xintc_write(int reg, u32 data) > > +static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 > > +data) > > { > > if (static_branch_unlikely(&xintc_is_be)) > > - iowrite32be(data, xintc_irqc->base + reg); > > + iowrite32be(data, irqc->base + reg); > > else > > - iowrite32(data, xintc_irqc->base + reg); > > + iowrite32(data, irqc->base + reg); > > } > > > > -static unsigned int xintc_read(int reg) > > +static u32 xintc_read(struct xintc_irq_chip *irqc, int reg) > > { > > if (static_branch_unlikely(&xintc_is_be)) > > - return ioread32be(xintc_irqc->base + reg); > > + return ioread32be(irqc->base + reg); > > else > > - return ioread32(xintc_irqc->base + reg); > > + return ioread32(irqc->base + reg); > > } > > > > static void intc_enable_or_unmask(struct irq_data *d) { > > - unsigned long mask = 1 << d->hwirq; > > + unsigned long mask = BIT(d->hwirq); > > + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); > > Please order your local variables in reverse fir tree order: > > struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); > unsigned long mask = BIT(d->hwirq); > > which is the preferred coding style in this subsystem and way simpler to > read. [Mubin]: I will fix all such instances in next version > > > static void intc_mask_ack(struct irq_data *d) { > > - unsigned long mask = 1 << d->hwirq; > > + unsigned long mask = BIT(d->hwirq); > > + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); > > Ditto. > > > pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq); > > - xintc_write(CIE, mask); > > - xintc_write(IAR, mask); > > + xintc_write(irqc, CIE, mask); > > + xintc_write(irqc, IAR, mask); > > } > > +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc) > > +{ > > + u32 hwirq; > > + unsigned int irq = 0; > > Same. > > > + hwirq = xintc_read(irqc, IVR); > > + if (hwirq != -1U) > > + irq = irq_find_mapping(irqc->root_domain, hwirq); > > + > > + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq); > > Are these pr_debugs all over the please really required? I can understand > that you use them for development, but are they useful once the stuff > works? [Mubin] They might be useful to debug interrupt issues. Do you want me to remove them? > > > + return irq; > > +} > > + > > unsigned int xintc_get_irq(void) > > { > > - unsigned int hwirq, irq = -1; > > + u32 hwirq; > > + unsigned int irq = -1; > > See above. > > > - hwirq = xintc_read(IVR); > > + hwirq = xintc_read(primary_intc, IVR); > > if (hwirq != -1U) > > - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq); > > + irq = irq_find_mapping(primary_intc->root_domain, hwirq); > > > > pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq); > > > > @@ -138,12 +164,14 @@ static const struct irq_domain_ops > > xintc_irq_domain_ops = { static void xil_intc_irq_handler(struct > > irq_desc *desc) { > > struct irq_chip *chip = irq_desc_get_chip(desc); > > + struct xintc_irq_chip *irqc = > > + irq_data_get_irq_handler_data(&desc->irq_data); > > Please avoid these ugly line breaks and put the initialization of the variable in > to the code below the declaration. [Mubin]: Will do in next version > > > /* Turn on the Master Enable. */ > > - xintc_write(MER, MER_HIE | MER_ME); > > - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) { > > + xintc_write(irqc, MER, MER_HIE | MER_ME); > > + if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) { > > static_branch_enable(&xintc_is_be); > > I see it's existing logic, but this lacks a comment how it's determined that > xintc is big endian. Looks like some weird "write works?" > probing. Why? > > > + xintc_write(irqc, MER, MER_HIE | MER_ME); > > So this writes MER_HIE | MER_ME into MER > > > + if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) { > > but this checks just whether ONE of the bits is set. Shouldn't it check for MER > == (MER_HIE | MER_ME), i.e. read back what was written? [Mubin]: Agreed, will fix it in v4. Thanks, Mubin > > Thanks, > > tglx
diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c index e3043ded8973..51f461d2934f 100644 --- a/drivers/irqchip/irq-xilinx-intc.c +++ b/drivers/irqchip/irq-xilinx-intc.c @@ -38,29 +38,32 @@ struct xintc_irq_chip { void __iomem *base; struct irq_domain *root_domain; u32 intr_mask; + struct irq_chip *intc_dev; + u32 nr_irq; }; -static struct xintc_irq_chip *xintc_irqc; +static struct xintc_irq_chip *primary_intc; -static void xintc_write(int reg, u32 data) +static void xintc_write(struct xintc_irq_chip *irqc, int reg, u32 data) { if (static_branch_unlikely(&xintc_is_be)) - iowrite32be(data, xintc_irqc->base + reg); + iowrite32be(data, irqc->base + reg); else - iowrite32(data, xintc_irqc->base + reg); + iowrite32(data, irqc->base + reg); } -static unsigned int xintc_read(int reg) +static u32 xintc_read(struct xintc_irq_chip *irqc, int reg) { if (static_branch_unlikely(&xintc_is_be)) - return ioread32be(xintc_irqc->base + reg); + return ioread32be(irqc->base + reg); else - return ioread32(xintc_irqc->base + reg); + return ioread32(irqc->base + reg); } static void intc_enable_or_unmask(struct irq_data *d) { - unsigned long mask = 1 << d->hwirq; + unsigned long mask = BIT(d->hwirq); + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq); @@ -69,30 +72,35 @@ static void intc_enable_or_unmask(struct irq_data *d) * acks the irq before calling the interrupt handler */ if (irqd_is_level_type(d)) - xintc_write(IAR, mask); + xintc_write(irqc, IAR, mask); - xintc_write(SIE, mask); + xintc_write(irqc, SIE, mask); } static void intc_disable_or_mask(struct irq_data *d) { + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); + pr_debug("irq-xilinx: disable: %ld\n", d->hwirq); - xintc_write(CIE, 1 << d->hwirq); + xintc_write(irqc, CIE, BIT(d->hwirq)); } static void intc_ack(struct irq_data *d) { + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); + pr_debug("irq-xilinx: ack: %ld\n", d->hwirq); - xintc_write(IAR, 1 << d->hwirq); + xintc_write(irqc, IAR, BIT(d->hwirq)); } static void intc_mask_ack(struct irq_data *d) { - unsigned long mask = 1 << d->hwirq; + unsigned long mask = BIT(d->hwirq); + struct xintc_irq_chip *irqc = irq_data_get_irq_chip_data(d); pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq); - xintc_write(CIE, mask); - xintc_write(IAR, mask); + xintc_write(irqc, CIE, mask); + xintc_write(irqc, IAR, mask); } static struct irq_chip intc_dev = { @@ -103,13 +111,28 @@ static struct irq_chip intc_dev = { .irq_mask_ack = intc_mask_ack, }; +static unsigned int xintc_get_irq_local(struct xintc_irq_chip *irqc) +{ + u32 hwirq; + unsigned int irq = 0; + + hwirq = xintc_read(irqc, IVR); + if (hwirq != -1U) + irq = irq_find_mapping(irqc->root_domain, hwirq); + + pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq); + + return irq; +} + unsigned int xintc_get_irq(void) { - unsigned int hwirq, irq = -1; + u32 hwirq; + unsigned int irq = -1; - hwirq = xintc_read(IVR); + hwirq = xintc_read(primary_intc, IVR); if (hwirq != -1U) - irq = irq_find_mapping(xintc_irqc->root_domain, hwirq); + irq = irq_find_mapping(primary_intc->root_domain, hwirq); pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq); @@ -118,15 +141,18 @@ unsigned int xintc_get_irq(void) static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw) { - if (xintc_irqc->intr_mask & (1 << hw)) { - irq_set_chip_and_handler_name(irq, &intc_dev, + struct xintc_irq_chip *irqc = d->host_data; + + if (irqc->intr_mask & BIT(hw)) { + irq_set_chip_and_handler_name(irq, irqc->intc_dev, handle_edge_irq, "edge"); irq_clear_status_flags(irq, IRQ_LEVEL); } else { - irq_set_chip_and_handler_name(irq, &intc_dev, + irq_set_chip_and_handler_name(irq, irqc->intc_dev, handle_level_irq, "level"); irq_set_status_flags(irq, IRQ_LEVEL); } + irq_set_chip_data(irq, irqc); return 0; } @@ -138,12 +164,14 @@ static const struct irq_domain_ops xintc_irq_domain_ops = { static void xil_intc_irq_handler(struct irq_desc *desc) { struct irq_chip *chip = irq_desc_get_chip(desc); + struct xintc_irq_chip *irqc = + irq_data_get_irq_handler_data(&desc->irq_data); u32 pending; chained_irq_enter(chip, desc); do { - pending = xintc_get_irq(); - if (pending == -1U) + pending = xintc_get_irq_local(irqc); + if (pending == 0U) break; generic_handle_irq(pending); } while (true); @@ -153,28 +181,19 @@ static void xil_intc_irq_handler(struct irq_desc *desc) static int __init xilinx_intc_of_init(struct device_node *intc, struct device_node *parent) { - u32 nr_irq; int ret, irq; struct xintc_irq_chip *irqc; - if (xintc_irqc) { - pr_err("irq-xilinx: Multiple instances aren't supported\n"); - return -EINVAL; - } - irqc = kzalloc(sizeof(*irqc), GFP_KERNEL); if (!irqc) return -ENOMEM; - - xintc_irqc = irqc; - irqc->base = of_iomap(intc, 0); BUG_ON(!irqc->base); - ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq); + ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &irqc->nr_irq); if (ret < 0) { pr_err("irq-xilinx: unable to read xlnx,num-intr-inputs\n"); - goto err_alloc; + goto error; } ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask); @@ -183,34 +202,35 @@ static int __init xilinx_intc_of_init(struct device_node *intc, irqc->intr_mask = 0; } - if (irqc->intr_mask >> nr_irq) + if (irqc->intr_mask >> irqc->nr_irq) pr_warn("irq-xilinx: mismatch in kind-of-intr param\n"); pr_info("irq-xilinx: %pOF: num_irq=%d, edge=0x%x\n", - intc, nr_irq, irqc->intr_mask); + intc, irqc->nr_irq, irqc->intr_mask); + irqc->intc_dev = &intc_dev; /* * Disable all external interrupts until they are * explicity requested. */ - xintc_write(IER, 0); + xintc_write(irqc, IER, 0); /* Acknowledge any pending interrupts just in case. */ - xintc_write(IAR, 0xffffffff); + xintc_write(irqc, IAR, 0xffffffff); /* Turn on the Master Enable. */ - xintc_write(MER, MER_HIE | MER_ME); - if (!(xintc_read(MER) & (MER_HIE | MER_ME))) { + xintc_write(irqc, MER, MER_HIE | MER_ME); + if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) { static_branch_enable(&xintc_is_be); - xintc_write(MER, MER_HIE | MER_ME); + xintc_write(irqc, MER, MER_HIE | MER_ME); } - irqc->root_domain = irq_domain_add_linear(intc, nr_irq, + irqc->root_domain = irq_domain_add_linear(intc, irqc->nr_irq, &xintc_irq_domain_ops, irqc); if (!irqc->root_domain) { pr_err("irq-xilinx: Unable to create IRQ domain\n"); - goto err_alloc; + goto error; } if (parent) { @@ -222,16 +242,17 @@ static int __init xilinx_intc_of_init(struct device_node *intc, } else { pr_err("irq-xilinx: interrupts property not in DT\n"); ret = -EINVAL; - goto err_alloc; + goto error; } } else { - irq_set_default_host(irqc->root_domain); + primary_intc = irqc; + irq_set_default_host(primary_intc->root_domain); } return 0; -err_alloc: - xintc_irqc = NULL; +error: + iounmap(irqc->base); kfree(irqc); return ret;