Re: [PATCH v2] irqchip: xilinx: Add support for multiple instances

[Michal Simek]

On 05. 02. 20 17:53, Marc Zyngier wrote:
> On 2020-02-05 14:05, Mubin Usman Sayyed wrote:
>> From: Mubin Sayyed <mubin.usman.sayyed@xxxxxxxxxx>
>>
>> This patch adds support for multiple instances of
>> xilinx interrupt controller. Below configurations are
>> supported by driver,
>>
>> - peripheral->xilinx-intc->xilinx-intc->gic
>> - peripheral->xilinx-intc->xilinx-intc
>>
>> Signed-off-by: Anirudha Sarangi <anirudha.sarangi@xxxxxxxxxx>
>> Signed-off-by: Mubin Sayyed <mubin.usman.sayyed@xxxxxxxxxx>
>> ---
>> Changes for v2:
>> ÂÂÂÂÂÂÂ - Removed write_fn/read_fn hooks, used xintc_write/
>> ÂÂÂÂÂÂÂÂÂ xintc_read directly
>> ÂÂÂÂÂÂÂ - Moved primary_intc declaration after xintc_irq_chip
>> ---
>> Âdrivers/irqchip/irq-xilinx-intc.c | 121
>> +++++++++++++++++++++++---------------
>> Â1 file changed, 73 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-xilinx-intc.c
>> b/drivers/irqchip/irq-xilinx-intc.c
>> index e3043de..14cb630 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(void __iomem *addr, u32 data)
>> Â{
>> ÂÂÂÂÂÂÂ if (static_branch_unlikely(&xintc_is_be))
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iowrite32be(data, xintc_irqc->base + reg);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iowrite32be(data, addr);
>> ÂÂÂÂÂÂÂ else
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iowrite32(data, xintc_irqc->base + reg);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iowrite32(data, addr);
>> Â}
>>
>> -static unsigned int xintc_read(int reg)
>> +static unsigned int xintc_read(void __iomem *addr)
> 
> Since you are changing this, please change the return value to reflect
> the size of what you're returning (u32 instead of unsigned int).
> 
>> Â{
>> ÂÂÂÂÂÂÂ if (static_branch_unlikely(&xintc_is_be))
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ioread32be(xintc_irqc->base + reg);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ioread32be(addr);
>> ÂÂÂÂÂÂÂ else
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ioread32(xintc_irqc->base + reg);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ioread32(addr);
>> Â}
>>
>> Âstatic void intc_enable_or_unmask(struct irq_data *d)
>> Â{
>> ÂÂÂÂÂÂÂ unsigned long mask = 1 << d->hwirq;
>> +ÂÂÂÂÂÂ struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>>
>> ÂÂÂÂÂÂÂ pr_debug("irq-xilinx: enable_or_unmask: %ld\n", d->hwirq);
>>
>> @@ -69,47 +72,57 @@ 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(local_intc->base + IAR, mask);
>>
>> -ÂÂÂÂÂÂ xintc_write(SIE, mask);
>> +ÂÂÂÂÂÂ xintc_write(local_intc->base + SIE, mask);
>> Â}
>>
>> Âstatic void intc_disable_or_mask(struct irq_data *d)
>> Â{
>> +ÂÂÂÂÂÂ struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>> +
>> ÂÂÂÂÂÂÂ pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
>> -ÂÂÂÂÂÂ xintc_write(CIE, 1 << d->hwirq);
>> +ÂÂÂÂÂÂ xintc_write(local_intc->base + CIE, 1 << d->hwirq);
>> Â}
>>
>> Âstatic void intc_ack(struct irq_data *d)
>> Â{
>> +ÂÂÂÂÂÂ struct xintc_irq_chip *local_intc =
>> irq_data_get_irq_chip_data(d);
>> +
>> ÂÂÂÂÂÂÂ pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
>> -ÂÂÂÂÂÂ xintc_write(IAR, 1 << d->hwirq);
>> +ÂÂÂÂÂÂ xintc_write(local_intc->base + IAR, 1 << d->hwirq);
>> Â}
>>
>> Âstatic void intc_mask_ack(struct irq_data *d)
>> Â{
>> ÂÂÂÂÂÂÂ unsigned long mask = 1 << d->hwirq;
>> +ÂÂÂÂÂÂ struct xintc_irq_chip *local_intc =
>> 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(local_intc->base + CIE, mask);
>> +ÂÂÂÂÂÂ xintc_write(local_intc->base + IAR, mask);
>> Â}
>>
>> -static struct irq_chip intc_dev = {
>> -ÂÂÂÂÂÂ .name = "Xilinx INTC",
>> -ÂÂÂÂÂÂ .irq_unmask = intc_enable_or_unmask,
>> -ÂÂÂÂÂÂ .irq_mask = intc_disable_or_mask,
>> -ÂÂÂÂÂÂ .irq_ack = intc_ack,
>> -ÂÂÂÂÂÂ .irq_mask_ack = intc_mask_ack,
>> -};
>> +static unsigned int xintc_get_irq_local(struct xintc_irq_chip
>> *local_intc)
>> +{
>> +ÂÂÂÂÂÂ int hwirq, irq = -1;
> 
> Type consistency for hwirq.
> 
>> +
>> +ÂÂÂÂÂÂ hwirq = xintc_read(local_intc->base + IVR);
>> +ÂÂÂÂÂÂ if (hwirq != -1U)
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irq = irq_find_mapping(local_intc->root_domain, hwirq);
>> +
>> +ÂÂÂÂÂÂ pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>> +
>> +ÂÂÂÂÂÂ return irq;
> 
> That now gives you both -1 and 0 for error values. Please stick with 0.
> 
>> +}
>>
>> Âunsigned int xintc_get_irq(void)
>> Â{
>> -ÂÂÂÂÂÂ unsigned int hwirq, irq = -1;
>> +ÂÂÂÂÂÂ int hwirq, irq = -1;
>>
>> -ÂÂÂÂÂÂ hwirq = xintc_read(IVR);
>> +ÂÂÂÂÂÂ hwirq = xintc_read(primary_intc->base + 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);
> 
> I have the ugly feeling I'm reading the same code twice... Surely you can
> make these two functions common code.

I have some questions regarding this.
I have updated one patchset which is adding support for Microblaze SMP.
And when I was looking at current wiring of this driver I have decided
to change it.

I have enabled  GENERIC_IRQ_MULTI_HANDLER and HANDLE_DOMAIN_IRQ.
This driver calls set_handle_irq(xil_intc_handle_irq)
and MB do_IRQ() call handle_arch_irq()
and IRQ routine here is using handle_domain_irq().

I would expect that this chained IRQ handler can also use
handle_domain_irq().

Is that correct understanding?



>>
>> @@ -118,15 +131,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 *local_intc = d->host_data;
>> +
>> +ÂÂÂÂÂÂ if (local_intc->intr_mask & (1 << hw)) {
> 
> BIT(hw)
> 
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irq_set_chip_and_handler_name(irq, local_intc->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, local_intc->intc_dev,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ handle_level_irq,
>> "level");
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irq_set_status_flags(irq, IRQ_LEVEL);
>> ÂÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂ irq_set_chip_data(irq, local_intc);
>> ÂÂÂÂÂÂÂ return 0;
>> Â}
>>
>> @@ -138,11 +154,13 @@ 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 *local_intc =
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irq_data_get_irq_handler_data(&desc->irq_data);
>> ÂÂÂÂÂÂÂ u32 pending;
>>
>> ÂÂÂÂÂÂÂ chained_irq_enter(chip, desc);
>> ÂÂÂÂÂÂÂ do {
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pending = xintc_get_irq();
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pending = xintc_get_irq_local(local_intc);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (pending == -1U)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ generic_handle_irq(pending);
>> @@ -153,28 +171,20 @@ 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;
>> -ÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂ struct irq_chip *intc_dev;
>>
>> ÂÂÂÂÂÂÂ 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,30 +193,42 @@ 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);
>> +
>> +ÂÂÂÂÂÂ intc_dev = kzalloc(sizeof(*intc_dev), GFP_KERNEL);
>> +ÂÂÂÂÂÂ if (!intc_dev) {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -ENOMEM;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto error;
>> +ÂÂÂÂÂÂ }
>>
>> +ÂÂÂÂÂÂ intc_dev->name = intc->full_name;
> 
> No. The world doesn't need to see the OF path of your interrupt
> controller in /proc/cpuinfo.
> The name that was there before was perfectly descriptive, please stick
> to it.

It should be showing name like interrupt-controller@41800000.
Do you think that we really should stick with just fixed name?
There could be multiple instances in the system and you will have no
idea how they are connected.

Thanks,
Michal