Re: [PATCH] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bjorn Helgaas]

Hi Bharat,

I'm resending this since you sent a ping three days after I responded,
so I don't know whether you got this the first time around.

DMARC got turned on for mail coming from @google.com, which apparently
caused some email providers to drop it.  I switched to sending from
helg...@kernel.org to avoid that problem.

Bjorn

On Fri, Sep 18, 2015 at 03:37:36PM -0500, Bjorn Helgaas wrote:
> Hi Bharat,
> 
> On Thu, Aug 27, 2015 at 05:14:20PM +0530, Bharat Kumar Gogada wrote:
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> > index 140d66f..0f3a789 100644
> > --- a/drivers/pci/host/Makefile
> > +++ b/drivers/pci/host/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
> >  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
> >  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
> >  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> > +obj-$(CONFIG_PCI_XILINX_NWL) += pci-xilinx-nwl.o
> 
> The other Xilinx driver uses "CONFIG_PCIE_*" and "pcie-xilinx.o".  Please
> use "pcie" similarly for this driver.
> 
> > +/**
> > + * struct nwl_msi - MSI information
> > + *
> > + * @chip: MSI controller
> > + * @used: Declare Bitmap for MSI
> > + * @domain: IRQ domain pointer
> > + * @pages: MSI pages
> > + * @lock: mutex lock
> > + * @irq_msi0: msi0 interrupt number
> > + * @irq_msi1: msi1 interrupt number
> 
> Please put these short comments inside the struct definition, on the same
> line as the element they describe.  It's needlessly repetitive to do it
> separately.
> 
> > + */
> > +struct nwl_msi {
> > +   struct msi_controller chip;
> > +   DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> > +   struct irq_domain *domain;
> > +   unsigned long pages;
> > +   struct mutex lock;
> > +   int irq_msi0;
> > +   int irq_msi1;
> > +};
> 
> > +static inline bool nwl_pcie_is_link_up(struct nwl_pcie *pcie, u32 
> > check_bit)
> 
> Please name this "nwl_pcie_link_up" so it's consistent with most others
> (xilinx_pcie_link_is_up() is an exception).  I'm not sure the "check_bit"
> argument is really an improvement, since it makes the code a bit ugly and
> more different from other drivers than it would otherwise be.
> 
> > +{
> > +   unsigned int status = -EINVAL;
> > +
> > +   if (check_bit == PCIE_USER_LINKUP)
> > +   status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> > + PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> > +   else if (check_bit == PHY_RDY_LINKUP)
> > +   status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> > + PHY_RDY_LINKUP_BIT) ? 1 : 0;
> > +   return status;
> > +}
> 
> > +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> > +{
> > +   unsigned int status;
> > +   int check = 0;
> > +
> > +   do {
> > +   status = nwl_bridge_readl(pcie, TX_PCIE_MSG) & MSG_BUSY_BIT;
> > +   if (!status) {
> > +   /*
> > +* Generate the TLP message for a single EP
> > +* [TODO] Add a multi-endpoint code
> > +*/
> > +   nwl_bridge_writel(pcie, 0x0,
> > + TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> > +   nwl_bridge_writel(pcie, 0x0,
> > + TX_PCIE_MSG + TX_PCIE_MSG_SPEC_LO);
> > +   nwl_bridge_writel(pcie, 0x0,
> > + TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> > +   nwl_bridge_writel(pcie, 0x0,
> > + TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> > +   /* Pattern to generate SSLP TLP */
> > +   nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> > + TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> > +   nwl_bridge_writel(pcie, RANDOM_DIGIT,
> > + TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> > +   nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> > + TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> > +   status = 0;
> > +   do {
> > +   status = nwl_bridge_readl(pcie, TX_PCIE_MSG) &
> > + MSG_DONE_BIT;
> > +   if (!status && (check < 1)) {
> > +   mdelay(1);
> > +   check++;
> > +   } else {
> > +   return false;
> > +   }
> > +
> > +   } while (!status);
> > +   status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> > + & MSG_DONE_STATUS_BIT;
> > +   }
> > +   } while (status);
> > +
> > +   return true;
> 
> This function is defined to return "int."  It should not return "true" or
> "false."
> 
> I'm really confused about 

Re: [PATCH] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Mark Rutland]

On Thu, Aug 27, 2015 at 12:44:20PM +0100, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada 
> Signed-off-by: Ravi Kiran Gummaluri 
> ---
>  .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   39 +
>  drivers/pci/host/Kconfig   |9 +
>  drivers/pci/host/Makefile  |1 +
>  drivers/pci/host/pci-xilinx-nwl.c  | 1038 
> 
>  4 files changed, 1087 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
>  create mode 100644 drivers/pci/host/pci-xilinx-nwl.c
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt 
> b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
> new file mode 100644
> index 000..c554d6b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
> @@ -0,0 +1,39 @@
> +* Xilinx NWL PCIe Root Port Bridge DT description
> +
> +Required properties:
> +- compatible: Should contain "xlnx,nwl-pcie-2.11"
> +- #address-cells: Address representation for root ports, set to <3>
> +- #size-cells: Size representation for root ports, set to <2>
> +- #interrupt-cells: specifies the number of cells needed to encode an
> +   interrupt source. The value must be 1.

This isn't in the example, and I cna't see why/how you would use this.

> +- reg: Should contain Bridge, PCIe Controller registers location and length

You need to define reg-names, given the example and driver rely on it.

> +- device_type: must be "pci"
> +- interrupts: Should contain NWL PCIe interrupt

You need to define interrupt-names, given the example and driver rely on it.

> +- ranges: ranges for the PCI memory regions (I/O space region is not
> +   supported by hardware)
> +   Please refer to the standard PCI bus binding document for a more
> +   detailed explanation
> +
> +Optional properties:
> +- xlnx,msi-fifo: To enable MSI FIFO mode

Why is this in the binding?

When should or shouldn't I set this?

I take it the root complex is its own MSI controller?

> +
> +Example:
> +
> +nwl_pcie: pcie@fd0e {
> +   compatible = "xlnx,nwl-pcie-2.11";
> +   #address-cells = <3>;
> +   #size-cells = <2>;
> +   #interrupt-cells = <1>;
> +   device_type = "pci";
> +   interrupt-parent = <>;
> +   interrupts = < 0 118 4
> +  0 116 4
> +  0 115 4  // MSI_1 [63...32]
> +  0 114 4 >;   // MSI_0 [31...0]

Nit: please bracket list entires individually (that also applies to reg).

> +   interrupt-names = "misc", "intx", "msi_1", "msi_0";
> +   reg = <0x0 0xfd0e 0x1000
> +  0x0 0xfd48 0x1000
> +  0x0 0xE000 0x100>;
> +   reg-names = "breg", "pcireg", "cfg";
> +   ranges = <0x0200 0x 0xE100 0x 0xE100 0 
> 0x0F00>;
> +};

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

Ping!

-Original Message-
From: Bharat Kumar Gogada [mailto:bharat.kumar.gog...@xilinx.com]
Sent: Thursday, August 27, 2015 5:14 PM
To: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; 
ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek; Soren 
Brinkmann; bhelg...@google.com; a...@arndb.de; tinam...@apm.com; 
tred...@nvidia.com; r...@broadcom.com; minghuan.l...@freescale.com; 
m-kariche...@ti.com; ha...@hauke-m.de
Cc: devicetree@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
linux-ker...@vger.kernel.org; linux-...@vger.kernel.org; Bharat Kumar Gogada; 
Ravikiran Gummaluri
Subject: [PATCH] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host 
Controller

Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.

Signed-off-by: Bharat Kumar Gogada 
Signed-off-by: Ravi Kiran Gummaluri 
---
 .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   39 +
 drivers/pci/host/Kconfig   |9 +
 drivers/pci/host/Makefile  |1 +
 drivers/pci/host/pci-xilinx-nwl.c  | 1038 
 4 files changed, 1087 insertions(+), 0 deletions(-)  create mode 100644 
Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
 create mode 100644 drivers/pci/host/pci-xilinx-nwl.c

diff --git a/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt 
b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
new file mode 100644
index 000..c554d6b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,39 @@
+* Xilinx NWL PCIe Root Port Bridge DT description
+
+Required properties:
+- compatible: Should contain "xlnx,nwl-pcie-2.11"
+- #address-cells: Address representation for root ports, set to <3>
+- #size-cells: Size representation for root ports, set to <2>
+- #interrupt-cells: specifies the number of cells needed to encode an
+   interrupt source. The value must be 1.
+- reg: Should contain Bridge, PCIe Controller registers location and
+length
+- device_type: must be "pci"
+- interrupts: Should contain NWL PCIe interrupt
+- ranges: ranges for the PCI memory regions (I/O space region is not
+   supported by hardware)
+   Please refer to the standard PCI bus binding document for a more
+   detailed explanation
+
+Optional properties:
+- xlnx,msi-fifo: To enable MSI FIFO mode
+
+Example:
+
+nwl_pcie: pcie@fd0e {
+   compatible = "xlnx,nwl-pcie-2.11";
+   #address-cells = <3>;
+   #size-cells = <2>;
+   #interrupt-cells = <1>;
+   device_type = "pci";
+   interrupt-parent = <>;
+   interrupts = < 0 118 4
+  0 116 4
+  0 115 4  // MSI_1 [63...32]
+  0 114 4 >;   // MSI_0 [31...0]
+   interrupt-names = "misc", "intx", "msi_1", "msi_0";
+   reg = <0x0 0xfd0e 0x1000
+  0x0 0xfd48 0x1000
+  0x0 0xE000 0x100>;
+   reg-names = "breg", "pcireg", "cfg";
+   ranges = <0x0200 0x 0xE100 0x 0xE100 0
+0x0F00>; };
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index 
c132bdd..5ff4e7e 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -15,6 +15,15 @@ config PCI_MVEBU
depends on ARCH_MVEBU || ARCH_DOVE
depends on OF

+config PCI_XILINX_NWL
+   bool "NWL PCIe Core"
+   depends on ARCH_ZYNQMP && PCI_MSI
+   help
+Say 'Y' here if you want kernel to support for Xilinx
+NWL PCIe controller.The controller can act as Root Port
+or End Point.The current option selection will only
+support root port enabling.
+
 config PCIE_DW
bool

diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 
140d66f..0f3a789 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
+obj-$(CONFIG_PCI_XILINX_NWL) += pci-xilinx-nwl.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o diff --git 
a/drivers/pci/host/pci-xilinx-nwl.c b/drivers/pci/host/pci-xilinx-nwl.c
new file mode 100644
index 000..6c2fa80
--- /dev/null
+++ b/drivers/pci/host/pci-xilinx-nwl.c
@@ -0,0 +1,1038 @@
+/*
+ * PCIe host controller driver for NWL PCIe Bridge
+ * Based on pci-xilinx.c, pci-tegra.c
+ *
+ * (C) Copyright 2014 - 2015, Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 

Re: [PATCH] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bjorn Helgaas]

Hi Bharat,

On Thu, Aug 27, 2015 at 05:14:20PM +0530, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.

> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..0f3a789 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
>  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
> +obj-$(CONFIG_PCI_XILINX_NWL) += pci-xilinx-nwl.o

The other Xilinx driver uses "CONFIG_PCIE_*" and "pcie-xilinx.o".  Please
use "pcie" similarly for this driver.

> +/**
> + * struct nwl_msi - MSI information
> + *
> + * @chip: MSI controller
> + * @used: Declare Bitmap for MSI
> + * @domain: IRQ domain pointer
> + * @pages: MSI pages
> + * @lock: mutex lock
> + * @irq_msi0: msi0 interrupt number
> + * @irq_msi1: msi1 interrupt number

Please put these short comments inside the struct definition, on the same
line as the element they describe.  It's needlessly repetitive to do it
separately.

> + */
> +struct nwl_msi {
> + struct msi_controller chip;
> + DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> + struct irq_domain *domain;
> + unsigned long pages;
> + struct mutex lock;
> + int irq_msi0;
> + int irq_msi1;
> +};

> +static inline bool nwl_pcie_is_link_up(struct nwl_pcie *pcie, u32 check_bit)

Please name this "nwl_pcie_link_up" so it's consistent with most others
(xilinx_pcie_link_is_up() is an exception).  I'm not sure the "check_bit"
argument is really an improvement, since it makes the code a bit ugly and
more different from other drivers than it would otherwise be.

> +{
> + unsigned int status = -EINVAL;
> +
> + if (check_bit == PCIE_USER_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> + else if (check_bit == PHY_RDY_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PHY_RDY_LINKUP_BIT) ? 1 : 0;
> + return status;
> +}

> +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> +{
> + unsigned int status;
> + int check = 0;
> +
> + do {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG) & MSG_BUSY_BIT;
> + if (!status) {
> + /*
> +  * Generate the TLP message for a single EP
> +  * [TODO] Add a multi-endpoint code
> +  */
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_LO);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + /* Pattern to generate SSLP TLP */
> + nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, RANDOM_DIGIT,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> +   TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> + status = 0;
> + do {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG) &
> +   MSG_DONE_BIT;
> + if (!status && (check < 1)) {
> + mdelay(1);
> + check++;
> + } else {
> + return false;
> + }
> +
> + } while (!status);
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_STATUS_BIT;
> + }
> + } while (status);
> +
> + return true;

This function is defined to return "int."  It should not return "true" or
"false."

I'm really confused about what this function does.  I can see it does
at least:

  - wait until MSG_BUSY_BIT is clear
  - write a bunch of registers
  - some magic handshake involving MSG_DONE_BIT and
MSG_DONE_STATUS_BIT (I really don't understand this)
  - possibly repeat if MSG_DONE_STATUS_BIT is set?

Can you clean this up and straighten it out somehow?  My head hurts
from trying to understand it.

> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> + unsigned int devfn,
> +