From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.136]:33037 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755530AbbJ1Mce (ORCPT ); Wed, 28 Oct 2015 08:32:34 -0400 Date: Wed, 28 Oct 2015 07:32:28 -0500 From: Bjorn Helgaas To: Bharat Kumar Gogada Cc: "robh+dt@kernel.org" , "pawel.moll@arm.com" , "mark.rutland@arm.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , Michal Simek , Soren Brinkmann , "bhelgaas@google.com" , "arnd@arndb.de" , "tinamdar@apm.com" , "treding@nvidia.com" , "rjui@broadcom.com" , "Minghuan.Lian@freescale.com" , "m-karicheri2@ti.com" , "hauke@hauke-m.de" , "marc.zyngier@arm.com" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-pci@vger.kernel.org" , Ravikiran Gummaluri Subject: Re: [PATCH v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller Message-ID: <20151028123228.GA30510@localhost> References: <1445871386-2377-1-git-send-email-bharatku@xilinx.com> <20151027234032.GA7238@localhost> <8520D5D51A55D047800579B0941471980169316D@XAP-PVEXMBX01.xlnx.xilinx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <8520D5D51A55D047800579B0941471980169316D@XAP-PVEXMBX01.xlnx.xilinx.com> Sender: linux-pci-owner@vger.kernel.org List-ID: On Wed, Oct 28, 2015 at 10:17:22AM +0000, Bharat Kumar Gogada wrote: > > On Mon, Oct 26, 2015 at 08:26:26PM +0530, Bharat Kumar Gogada wrote: > > > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP. > > > > + > > > + while ((status = nwl_bridge_readl(pcie, MSGF_MSI_STATUS_LO)) != > > 0) { > > > + for_each_set_bit(bit, &status, 32) { > > > + nwl_bridge_writel(pcie, 1 << bit, > > MSGF_MSI_STATUS_LO); > > > + virq = irq_find_mapping(msi->dev_domain, bit); > > > + if (virq) > > > + generic_handle_irq(virq); > > > + } > > > + } > > > + > > > + chained_irq_exit(chip, desc); > > > +} > > > > These are basically identical. Can you factor them out somehow to avoid > > repeating the code? > > Is it okay if irq_set_chained_handler_and_data being invoked with two different interrupt numbers, but pointing to > same interrupt handler? Yes, that should be fine. > > > + > > > + pcie->legacy_irq_domain = > > irq_domain_add_linear(legacy_intc_node, 4, > > > + > > &legacy_domain_ops, > > > + pcie); > > > + > > > + if (!pcie->legacy_irq_domain) { > > > + dev_err(pcie->dev, "failed to create IRQ domain\n"); > > > + return -ENOMEM; > > > + } > > > + > > > +#ifdef CONFIG_PCI_MSI > > > + msi->dev_domain = irq_domain_add_linear(NULL, > > INT_PCI_MSI_NR, > > > + &dev_msi_domain_ops, pcie); > > > + if (!msi->dev_domain) { > > > + dev_err(pcie->dev, "failed to create dev IRQ domain\n"); > > > + return -ENOMEM; > > > + } > > > + msi->msi_chip.domain = pci_msi_create_irq_domain(node, > > > + > > &nwl_msi_domain_info, > > > + msi->dev_domain); > > > + if (!msi->msi_chip.domain) { > > > + dev_err(pcie->dev, "failed to create msi IRQ domain\n"); > > > + irq_domain_remove(msi->dev_domain); > > > + return -ENOMEM; > > > + } > > > +#endif > > > + return 0; > > > +} > > > + > > > +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus > > > +*bus) { > > > > It looks strange to have all the "#ifdef CONFIG_PCI_MSI" above, and here > > we have this long MSI-related function without any ifdefs around it. Seems > > like this should be ifdef'ed also? What about nwl_pcie_msi_handler_high(), > > nwl_pcie_msi_handler_low(), nwl_compose_msi_msg(), > > nwl_msi_set_affinity(), etc.? > > > In probe I'm invoking "nwl_pcie_enable_msi" using "if (IS_ENABLED(CONFIG_PCI_MSI)) " check, since this is at run time > I haven't kept above mentioned functions under #ifdef CONFIG_PCI_MSI. > The above MSI domain allocation was under ifdef, since if driver was compiled for legacy some of the MSI hierarchy API's and structures aren't available. OK. It *looks* strange, but maybe it's the best we can do. I'm not enamored of IS_ENABLED() thing yet; I guess I just haven't internalized the combination compile-time and run-time behavior. Bjorn