Re: [PATCH v2 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller driver

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Jun 09, 2021 at 02:22:00PM +0530, Manivannan Sadhasivam wrote:
> On Sat, Jun 05, 2021 at 10:07:15PM -0500, Bjorn Andersson wrote:
> > On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> > 
> > > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > > the Designware core with added Qualcomm specific wrapper around the
> > > core. The driver support is very basic such that it supports only
> > > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > > for now but these will be added later.
> > > 
> > > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > > operation and written on top of the DWC PCI framework.
> > > 
> > > Co-developed-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > [mani: restructured the driver and fixed several bugs for upstream]
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Really nice to see this working!
> > 
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig        |  10 +
> > >  drivers/pci/controller/dwc/Makefile       |   1 +
> > >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 780 ++++++++++++++++++++++
> > >  3 files changed, 791 insertions(+)
> > >  create mode 100644 drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > 
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 423d35872ce4..32e735b1fd85 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -180,6 +180,16 @@ config PCIE_QCOM
> > >  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
> > >  	  hardware wrappers.
> > >  
> > > +config PCIE_QCOM_EP
> > > +	bool "Qualcomm PCIe controller - Endpoint mode"
> > > +	depends on OF && (ARCH_QCOM || COMPILE_TEST)
> > > +	depends on PCI_ENDPOINT
> > > +	select PCIE_DW_EP
> > > +	help
> > > +	  Say Y here to enable support for the PCIe controllers on Qualcomm SoCs
> > > +	  to work in endpoint mode. The PCIe controller uses the DesignWare core
> > > +	  plus Qualcomm-specific hardware wrappers.
> > > +
> > >  config PCIE_ARMADA_8K
> > >  	bool "Marvell Armada-8K PCIe controller"
> > >  	depends on ARCH_MVEBU || COMPILE_TEST
> > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > index eca805c1a023..abb27642d46b 100644
> > > --- a/drivers/pci/controller/dwc/Makefile
> > > +++ b/drivers/pci/controller/dwc/Makefile
> > > @@ -12,6 +12,7 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
> > >  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> > >  obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > >  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > > +obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> > >  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > >  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > >  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > new file mode 100644
> > > index 000000000000..b68511bacc2a
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > > @@ -0,0 +1,780 @@

[...]

> > > +static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev,
> > > +					     struct qcom_pcie_ep *pcie_ep)
> > > +{
> > > +	int irq, ret;
> > > +
> > > +	irq = platform_get_irq_byname(pdev, "global");
> > > +	if (irq < 0) {
> > > +		dev_err(&pdev->dev, "Failed to get Global IRQ\n");
> > > +		return irq;
> > > +	}
> > > +
> > > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > > +					qcom_pcie_ep_global_threaded_irq,
> > > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > 
> > Leave out the trigger and rely on DT.
> > 
> 
> Okay
> 
> > > +					"global_irq", pcie_ep);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Failed to request Global IRQ\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset);
> > > +	irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN);
> > 
> > Is the global interrupt needed for dw_pcie_ep_init()? Or could this
> > simply be done when things are ready to handle the interrupts?
> > 
> 
> No it is not needed. I can move this after dw_pcie_ep_init().
> 

I don't know why but I'm seeing issues when this gets called after
dw_pcie_ep_init(). So I'm keeping this function as it is for now.

Thanks,
Mani