Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1585205326-25326-1-git-send-email-srinath.mannam@broadcom.com> <1585205326-25326-3-git-send-email-srinath.mannam@broadcom.com>
In-Reply-To: <1585205326-25326-3-git-send-email-srinath.mannam@broadcom.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 26 Mar 2020 16:37:15 +0200
Message-ID: <CAHp75VfUCwcXN_OF-tq1wuiCFdicMMEpJpWNccQT=6cv0DNnWQ@mail.gmail.com>
Subject: Re: [PATCH v5 2/6] PCI: iproc: Add INTx support with better modeling
To:     Srinath Mannam <srinath.mannam@broadcom.com>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Ray Jui <rjui@broadcom.com>, Rob Herring <robh+dt@kernel.org>,
        Andrew Murray <andrew.murray@arm.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        linux-pci@vger.kernel.org, devicetree <devicetree@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Ray Jui <ray.jui@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 26, 2020 at 8:49 AM Srinath Mannam
<srinath.mannam@broadcom.com> wrote:
>
> From: Ray Jui <ray.jui@broadcom.com>
>
> Add PCIe legacy interrupt INTx support to the iProc PCIe driver by
> modeling it with its own IRQ domain. All 4 interrupts INTA, INTB, INTC,
> INTD share the same interrupt line connected to the GIC in the system,
> while the status of each INTx can be obtained through the INTX CSR
> register.

...
> +       val &= ~(BIT(irqd_to_hwirq(d)));

Too many parentheses.

...

> +       val |= (BIT(irqd_to_hwirq(d)));

Ditto.

...

> +       /* go through INTx A, B, C, D until all interrupts are handled */
> +       do {
> +               status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR);
> +               for_each_set_bit(bit, &status, PCI_NUM_INTX) {
> +                       virq = irq_find_mapping(pcie->irq_domain, bit);
> +                       if (virq)
> +                               generic_handle_irq(virq);

> +                       else
> +                               dev_err(dev, "unexpected INTx%u\n", bit);

Any guarantee it will be no storm of undesired messages here?

> +               }

> +       } while ((status & SYS_RC_INTX_MASK) != 0);

' != 0' part is not needed.

If there an interrupt storm the handler will never end, right?
Is it the idea by design?

...

> +       node = of_get_compatible_child(dev->of_node, "brcm,iproc-intc");
> +       if (node)
> +               pcie->irq = of_irq_get(node, 0);
> +
> +       if (!node || pcie->irq <= 0)
> +               return 0;

Perhaps
       node = of_get_compatible_child(dev->of_node, "brcm,iproc-intc");
       if (!node)
               return 0;

       pcie->irq = of_irq_get(node, 0);
       if (pcie->irq <= 0)
              return 0;
?

-- 
With Best Regards,
Andy Shevchenko