RE: [PATCH v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> Subject: Re: [PATCH v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
>
> On Friday 27 November 2015 20:32:03 Bharat Kumar Gogada wrote:
> > + do {
> > + err = nwl_pcie_link_up(pcie, PHY_RDY_LINKUP);
> > + if (err != 1) {
> > + check_link_up++;
> > + if (check_link_up > LINKUP_ITER_CHECK)
> > + return -ENODEV;
> > + mdelay(1000);
> > + }
> > + } while (!err);
>
> mdelay(1000) is not something anyone should do. Why can't you call a
> sleeping function here?
>
Agreed will use sleep function, will address it in next patch.
Regards,
Bharat
--
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 v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> Subject: Re: [PATCH v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
>
> On 27/11/15 15:02, 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
> > Acked-by: Rob Herring
> > ---
> > Changes for v10:
> > -> Changed MSI address to PCIe controller base.
> > -> Removed nwl_check_hwirq function, instead using
> > -> bitmap_find_next_zero_area
> >API to do the same.
> > ---
> > .../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 68 ++
> > drivers/pci/host/Kconfig | 10 +
> > drivers/pci/host/Makefile |1 +
> > drivers/pci/host/pcie-xilinx-nwl.c | 1068
> >
> > 4 files changed, 1147 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
> > create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > new file mode 100644
> > index 000..d070344
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
>
> [...]
>
> > +#define MSI_ADDRESS0xFD48
> > +
>
> Really? Why do you bother having DT support then? You might as well
> hardcode everything, while you're at it.
>
> /me felling depressed now.
Agreed, I'm already having this parameter in nwl_pcie structure, will use it.
>
> > +struct nwl_msi { /* MSI information */
> > + struct irq_domain *msi_domain;
> > + unsigned long *bitmap;
> > + struct irq_domain *dev_domain;
> > + struct mutex lock; /* protect bitmap variable */
> > + int irq_msi0;
> > + int irq_msi1;
> > +};
> > +
> > +struct nwl_pcie {
> > + struct device *dev;
> > + void __iomem *breg_base;
> > + void __iomem *pcireg_base;
> > + void __iomem *ecam_base;
> > + u32 phys_breg_base; /* Physical Bridge Register Base */
> > + u32 phys_pcie_reg_base; /* Physical PCIe Controller
> Base */
> > + u32 phys_ecam_base; /* Physical Configuration Base */
>
> All these u32 should be phys_addr_t. Not all the world is 32bit, fortunately.
Agreed will address it next patch.
>
> > + u32 breg_size;
> [...]
>
>
> > +static int nwl_pcie_bridge_init(struct nwl_pcie *pcie) {
> > + struct platform_device *pdev = to_platform_device(pcie->dev);
> > + u32 breg_val, ecam_val, first_busno = 0;
> > + int err;
> > + int check_link_up = 0;
> > +
> > + /* Check for BREG present bit */
> > + breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) &
> BREG_PRESENT;
> > + if (!breg_val) {
> > + dev_err(pcie->dev, "BREG is not present\n");
> > + return breg_val;
> > + }
> > + /* Write bridge_off to breg base */
> > + nwl_bridge_writel(pcie, (u32)(pcie->phys_breg_base),
> > + E_BREG_BASE_LO);
> > +
>
> I love the casting of a u32 to a u32. You have to program E_BREG_BASE_HI as
> well, once you've fixed the data type.
> Agreed, will address this in next patch.
> > + /* Enable BREG */
> > + nwl_bridge_writel(pcie, ~BREG_ENABLE_FORCE & BREG_ENABLE,
> > + E_BREG_CONTROL);
> > + /* Check for ECAM present bit */
> > + ecam_val = nwl_bridge_readl(pcie, E_ECAM_CAPABILITIES) &
> E_ECAM_PRESENT;
> > + if (!ecam_val) {
> > + dev_err(pcie->dev, "ECAM is not present\n");
> > + return ecam_val;
> > + }
> |
> > + /* Write phy_reg_base to ecam base */
> > + nwl_bridge_writel(pcie, (u32)pcie->phys_ecam_base,
> E_ECAM_BASE_LO);
>
> Same here.
> Agreed, will address this in next patch.
Regards,
Bharat
--
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 v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Friday 27 November 2015 20:32:03 Bharat Kumar Gogada wrote:
> + do {
> + err = nwl_pcie_link_up(pcie, PHY_RDY_LINKUP);
> + if (err != 1) {
> + check_link_up++;
> + if (check_link_up > LINKUP_ITER_CHECK)
> + return -ENODEV;
> + mdelay(1000);
> + }
> + } while (!err);
mdelay(1000) is not something anyone should do. Why can't you call
a sleeping function here?
Arnd
--
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 v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On 27/11/15 15:02, 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
> Acked-by: Rob Herring
> ---
> Changes for v10:
> -> Changed MSI address to PCIe controller base.
> -> Removed nwl_check_hwirq function, instead using bitmap_find_next_zero_area
>API to do the same.
> ---
> .../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 68 ++
> drivers/pci/host/Kconfig | 10 +
> drivers/pci/host/Makefile |1 +
> drivers/pci/host/pcie-xilinx-nwl.c | 1068
>
> 4 files changed, 1147 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
> create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
>
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> b/drivers/pci/host/pcie-xilinx-nwl.c
> new file mode 100644
> index 000..d070344
> --- /dev/null
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
[...]
> +#define MSI_ADDRESS 0xFD48
> +
Really? Why do you bother having DT support then? You might as well
hardcode everything, while you're at it.
/me felling depressed now.
> +struct nwl_msi { /* MSI information */
> + struct irq_domain *msi_domain;
> + unsigned long *bitmap;
> + struct irq_domain *dev_domain;
> + struct mutex lock; /* protect bitmap variable */
> + int irq_msi0;
> + int irq_msi1;
> +};
> +
> +struct nwl_pcie {
> + struct device *dev;
> + void __iomem *breg_base;
> + void __iomem *pcireg_base;
> + void __iomem *ecam_base;
> + u32 phys_breg_base; /* Physical Bridge Register Base */
> + u32 phys_pcie_reg_base; /* Physical PCIe Controller Base */
> + u32 phys_ecam_base; /* Physical Configuration Base */
All these u32 should be phys_addr_t. Not all the world is 32bit,
fortunately.
> + u32 breg_size;
> + u32 pcie_reg_size;
> + u32 ecam_size;
> + int irq_intx;
> + int irq_misc;
> + u32 ecam_value;
> + u8 last_busno;
> + u8 root_busno;
> + u8 link_up;
> + struct nwl_msi msi;
> + struct irq_domain *legacy_irq_domain;
> +};
[...]
> +static void nwl_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + phys_addr_t msi_addr = MSI_ADDRESS;
Fix this.
> +
> + msg->address_lo = lower_32_bits(msi_addr);
> + msg->address_hi = upper_32_bits(msi_addr);
> + msg->data = data->hwirq;
> +}
[...]
> +static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> +{
> + struct platform_device *pdev = to_platform_device(pcie->dev);
> + u32 breg_val, ecam_val, first_busno = 0;
> + int err;
> + int check_link_up = 0;
> +
> + /* Check for BREG present bit */
> + breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
> + if (!breg_val) {
> + dev_err(pcie->dev, "BREG is not present\n");
> + return breg_val;
> + }
> + /* Write bridge_off to breg base */
> + nwl_bridge_writel(pcie, (u32)(pcie->phys_breg_base),
> + E_BREG_BASE_LO);
> +
I love the casting of a u32 to a u32. You have to program E_BREG_BASE_HI
as well, once you've fixed the data type.
> + /* Enable BREG */
> + nwl_bridge_writel(pcie, ~BREG_ENABLE_FORCE & BREG_ENABLE,
> + E_BREG_CONTROL);
> +
> + /* Disable DMA channel registers */
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_PCIE_RX0) |
> + CFG_DMA_REG_BAR, BRCFG_PCIE_RX0);
> +
> + /* Enable the bridge config interrupt */
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_INTERRUPT) |
> + BRCFG_INTERRUPT_MASK, BRCFG_INTERRUPT);
> + /* Enable Ingress subtractive decode translation */
> + nwl_bridge_writel(pcie, SET_ISUB_CONTROL, I_ISUB_CONTROL);
> +
> + /* Enable msg filtering details */
> + nwl_bridge_writel(pcie, CFG_ENABLE_MSG_FILTER_MASK,
> + BRCFG_PCIE_RX_MSG_FILTER);
> + do {
> + err = nwl_pcie_link_up(pcie, PHY_RDY_LINKUP);
> + if (err != 1) {
> + check_link_up++;
> + if (check_link_up > LINKUP_ITER_CHECK)
> + return -ENODEV;
> + mdelay(1000);
> + }
> + } while (!err);
> +
> + /* Check for ECAM present bit */
> + ecam_val = nwl_bridge_readl(pcie, E_ECAM_CAPABILITIES) & E_ECAM_PRESENT;
> + if (!ecam_val) {
> + dev_err(pcie->dev, "ECAM is not present\n");
> + return ecam_val;
> + }
> +
> + /* Enable ECAM */
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
> + E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
> + /* Write ecam_value on ecam_control */
> + nwl_bridge_writel(pcie, nwl_bridge_readl
[PATCH v10] 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
Acked-by: Rob Herring
---
Changes for v10:
-> Changed MSI address to PCIe controller base.
-> Removed nwl_check_hwirq function, instead using bitmap_find_next_zero_area
API to do the same.
---
.../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 68 ++
drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile |1 +
drivers/pci/host/pcie-xilinx-nwl.c | 1068
4 files changed, 1147 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
create mode 100644 drivers/pci/host/pcie-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..3b2a9ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,68 @@
+* 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,
+ configuration sapce, and length
+- reg-names: Must include the following entries:
+ "breg": bridge registers
+ "pcireg": PCIe controller registers
+ "cfg": configuration space region
+- device_type: must be "pci"
+- interrupts: Should contain NWL PCIe interrupt
+- interrupt-names: Must include the following entries:
+ "msi1, msi0": interrupt asserted when msi is received
+ "intx": interrupt that is asserted when an legacy interrupt is received
+ "misc": interrupt asserted when miscellaneous is received
+- interrupt-map-mask and interrupt-map: standard PCI properties to define the
+ mapping of the PCI interface to interrupt numbers.
+- 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
+- msi-controller: indicates that this is MSI controller node
+- msi-parent: MSI parent of the root complex itself
+- legacy-interrupt-controller: Interrupt controller device node for Legacy
interrupts
+ - interrupt-controller: identifies the node as an interrupt controller
+ - #interrupt-cells: should be set to 1
+ - #address-cells: specifies the number of cells needed to encode an
+ address. The value must be 0.
+
+
+Example:
+
+
+nwl_pcie: pcie@fd0e {
+ #address-cells = <3>;
+ #size-cells = <2>;
+ compatible = "xlnx,nwl-pcie-2.11";
+ #interrupt-cells = <1>;
+ msi-controller;
+ device_type = "pci";
+ interrupt-parent = <&gic>;
+ interrupts = <0 114 4>, <0 115 4>, <0 116 4>, <0 117 4>, <0 118 4>;
+ interrupt-names = "msi0", "msi1", "intx", "dummy", "misc";
+ interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+ interrupt-map = <0x0 0x0 0x0 0x1 &pcie_intc 0x1>,
+ <0x0 0x0 0x0 0x2 &pcie_intc 0x2>,
+ <0x0 0x0 0x0 0x3 &pcie_intc 0x3>,
+ <0x0 0x0 0x0 0x4 &pcie_intc 0x4>;
+
+ msi-parent = <&nwl_pcie>;
+ reg = <0x0 0xfd0e 0x1000>,
+ <0x0 0xfd48 0x1000>,
+ <0x0 0xe000 0x100>;
+ reg-names = "breg", "pcireg", "cfg";
+ ranges = <0x0200 0x 0xe100 0x 0xe100 0
0x0f00>;
+
+ pcie_intc: legacy-interrupt-controller {
+ interrupt-controller;
+ #address-cells = <0>;
+ #interrupt-cells = <1>;
+ };
+
+};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d5e58ba..24cbcba 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -15,6 +15,16 @@ config PCI_MVEBU
depends on ARCH_MVEBU || ARCH_DOVE
depends on OF
+config PCIE_XILINX_NWL
+ bool "NWL PCIe Core"
+ depends on ARCH_ZYNQMP
+ select PCI_MSI_IRQ_DOMAIN if 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..6a56df7 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-keysto
