From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9F033C432C3 for ; Tue, 3 Dec 2019 15:55:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 80B7420803 for ; Tue, 3 Dec 2019 15:55:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726079AbfLCPzS (ORCPT ); Tue, 3 Dec 2019 10:55:18 -0500 Received: from foss.arm.com ([217.140.110.172]:44676 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726024AbfLCPzR (ORCPT ); Tue, 3 Dec 2019 10:55:17 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E2A2131B; Tue, 3 Dec 2019 07:55:16 -0800 (PST) Received: from localhost (unknown [10.37.6.20]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 59A683F52E; Tue, 3 Dec 2019 07:55:16 -0800 (PST) Date: Tue, 3 Dec 2019 15:55:14 +0000 From: Andrew Murray To: Srinath Mannam Cc: Lorenzo Pieralisi , Bjorn Helgaas , Florian Fainelli , Ray Jui , Rob Herring , Mark Rutland , Andy Shevchenko , Arnd Bergmann , bcm-kernel-feedback-list@broadcom.com, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Ray Jui Subject: Re: [PATCH v3 2/6] PCI: iproc: Add INTx support with better modeling Message-ID: <20191203155514.GE18399@e119886-lin.cambridge.arm.com> References: <1575349026-8743-1-git-send-email-srinath.mannam@broadcom.com> <1575349026-8743-3-git-send-email-srinath.mannam@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1575349026-8743-3-git-send-email-srinath.mannam@broadcom.com> User-Agent: Mutt/1.10.1+81 (426a6c1) (2018-08-26) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Dec 03, 2019 at 10:27:02AM +0530, Srinath Mannam wrote: > From: Ray Jui > > 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 > > Signed-off-by: Ray Jui > Signed-off-by: Srinath Mannam > --- > drivers/pci/controller/pcie-iproc.c | 100 +++++++++++++++++++++++++++++++++++- > drivers/pci/controller/pcie-iproc.h | 6 +++ > 2 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-iproc.c b/drivers/pci/controller/pcie-iproc.c > index 2d457bf..e90c22e 100644 > --- a/drivers/pci/controller/pcie-iproc.c > +++ b/drivers/pci/controller/pcie-iproc.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -270,6 +271,7 @@ enum iproc_pcie_reg { > > /* enable INTx */ > IPROC_PCIE_INTX_EN, > + IPROC_PCIE_INTX_CSR, > > /* outbound address mapping */ > IPROC_PCIE_OARR0, > @@ -314,6 +316,7 @@ static const u16 iproc_pcie_reg_paxb_bcma[] = { > [IPROC_PCIE_CFG_ADDR] = 0x1f8, > [IPROC_PCIE_CFG_DATA] = 0x1fc, > [IPROC_PCIE_INTX_EN] = 0x330, > + [IPROC_PCIE_INTX_CSR] = 0x334, > [IPROC_PCIE_LINK_STATUS] = 0xf0c, > }; > > @@ -325,6 +328,7 @@ static const u16 iproc_pcie_reg_paxb[] = { > [IPROC_PCIE_CFG_ADDR] = 0x1f8, > [IPROC_PCIE_CFG_DATA] = 0x1fc, > [IPROC_PCIE_INTX_EN] = 0x330, > + [IPROC_PCIE_INTX_CSR] = 0x334, > [IPROC_PCIE_OARR0] = 0xd20, > [IPROC_PCIE_OMAP0] = 0xd40, > [IPROC_PCIE_OARR1] = 0xd28, > @@ -341,6 +345,7 @@ static const u16 iproc_pcie_reg_paxb_v2[] = { > [IPROC_PCIE_CFG_ADDR] = 0x1f8, > [IPROC_PCIE_CFG_DATA] = 0x1fc, > [IPROC_PCIE_INTX_EN] = 0x330, > + [IPROC_PCIE_INTX_CSR] = 0x334, > [IPROC_PCIE_OARR0] = 0xd20, > [IPROC_PCIE_OMAP0] = 0xd40, > [IPROC_PCIE_OARR1] = 0xd28, > @@ -846,9 +851,95 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie) > return link_is_active ? 0 : -ENODEV; > } > > -static void iproc_pcie_enable(struct iproc_pcie *pcie) > +static int iproc_pcie_intx_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > { > + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); > + irq_set_chip_data(irq, domain->host_data); > + > + return 0; > +} > + > +static const struct irq_domain_ops intx_domain_ops = { > + .map = iproc_pcie_intx_map, > +}; > + > +static void iproc_pcie_isr(struct irq_desc *desc) > +{ > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct iproc_pcie *pcie; > + struct device *dev; > + unsigned long status; > + u32 bit, virq; > + > + chained_irq_enter(chip, desc); > + pcie = irq_desc_get_handler_data(desc); > + dev = pcie->dev; > + > + /* go through INTx A, B, C, D until all interrupts are handled */ > + do { > + status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR); By performing this read once and outside of the do/while loop you may improve performance. I wonder how probable it is to get another INTx whilst handling one? > + 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); > + } > + } while ((status & SYS_RC_INTX_MASK) != 0); > + > + chained_irq_exit(chip, desc); > +} > + > +static int iproc_pcie_intx_enable(struct iproc_pcie *pcie) > +{ > + struct device *dev = pcie->dev; > + struct device_node *node; > + int ret; > + > iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK); > + /* > + * BCMA devices do not map INTx the same way as platform devices. All > + * BCMA needs is the above code to enable INTx > + */ NIT: Move this comment above the line of code? > + > + node = of_get_compatible_child(dev->of_node, "brcm,iproc-intc"); As the interrupt controller is built into the PCI controller, what is the rationale for representing this as a separate device tree device? Thanks, Andrew Murray > + if (node) > + pcie->irq = of_irq_get(node, 0); > + > + if (!node || pcie->irq <= 0) > + return 0; > + > + /* set IRQ handler */ > + irq_set_chained_handler_and_data(pcie->irq, iproc_pcie_isr, pcie); > + > + /* add IRQ domain for INTx */ > + pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX, > + &intx_domain_ops, pcie); > + if (!pcie->irq_domain) { > + dev_err(dev, "failed to add INTx IRQ domain\n"); > + ret = -ENOMEM; > + goto err_rm_handler_data; > + } > + > + return 0; > + > +err_rm_handler_data: > + of_node_put(node); > + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); > + > + return ret; > +} > + > +static void iproc_pcie_intx_disable(struct iproc_pcie *pcie) > +{ > + iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, 0x0); > + > + if (pcie->irq <= 0) > + return; > + > + irq_domain_remove(pcie->irq_domain); > + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL); > } > > static inline bool iproc_pcie_ob_is_valid(struct iproc_pcie *pcie, > @@ -1537,7 +1628,11 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res) > goto err_power_off_phy; > } > > - iproc_pcie_enable(pcie); > + ret = iproc_pcie_intx_enable(pcie); > + if (ret) { > + dev_err(dev, "failed to enable INTx\n"); > + goto err_power_off_phy; > + } > > if (IS_ENABLED(CONFIG_PCI_MSI)) > if (iproc_pcie_msi_enable(pcie)) > @@ -1582,6 +1677,7 @@ int iproc_pcie_remove(struct iproc_pcie *pcie) > pci_remove_root_bus(pcie->root_bus); > > iproc_pcie_msi_disable(pcie); > + iproc_pcie_intx_disable(pcie); > > phy_power_off(pcie->phy); > phy_exit(pcie->phy); > diff --git a/drivers/pci/controller/pcie-iproc.h b/drivers/pci/controller/pcie-iproc.h > index 4f03ea5..103e568 100644 > --- a/drivers/pci/controller/pcie-iproc.h > +++ b/drivers/pci/controller/pcie-iproc.h > @@ -74,6 +74,9 @@ struct iproc_msi; > * @ib: inbound mapping related parameters > * @ib_map: outbound mapping region related parameters > * > + * @irq: interrupt line wired to the generic GIC for INTx > + * @irq_domain: IRQ domain for INTx > + * > * @need_msi_steer: indicates additional configuration of the iProc PCIe > * controller is required to steer MSI writes to external interrupt controller > * @msi: MSI data > @@ -102,6 +105,9 @@ struct iproc_pcie { > struct iproc_pcie_ib ib; > const struct iproc_pcie_ib_map *ib_map; > > + int irq; > + struct irq_domain *irq_domain; > + > bool need_msi_steer; > struct iproc_msi *msi; > }; > -- > 2.7.4 >