Re: [PATCH v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On 10/26/2015 11:26 AM, Bharat Kumar Gogada wrote: >>> + device_type = "pci"; >>> + interrupt-parent = <&gic>; >>> + interrupts = < 0 118 4 >>> + 0 116 4 >>> + 0 115 4 // MSI_1 [63...32] >>> + 0 114 4 >; // MSI_0 [31...0] >> >> Better write these as tuples: >> >> interrupts = <0 118 4>, <0 116 4>, <0 115 4>, <0 114 4>; >> >> And maybe reverse the order? It looks that might be what the soc >> integration person had in mind. >> >> Also, what is interrupt <0 117 4>? Is that connected here as well? >> Better list it as well then, even if you don't use it. >> > We have it but not using it, we will list it. > >>> + 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>; >> >> Same grouping for reg and interrupt-map as above for interrupts. > > Grouping reg and interrupt-map as tuples will make lengthy line and reduces > readability, is it compulsory ? FYI: Just this. reg = <0x0 0xfd0e 0x1000>, <0x0 0xfd48 0x1000>, <0x0 0xe000 0x100>; Also please make sure that you are using the same case for addresses. That 0xE000 case above. Thanks, Michal -- 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 v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> > + device_type = "pci"; > > + interrupt-parent = <&gic>; > > + interrupts = < 0 118 4 > > + 0 116 4 > > + 0 115 4 // MSI_1 [63...32] > > + 0 114 4 >; // MSI_0 [31...0] > > Better write these as tuples: > > interrupts = <0 118 4>, <0 116 4>, <0 115 4>, <0 114 4>; > > And maybe reverse the order? It looks that might be what the soc > integration person had in mind. > > Also, what is interrupt <0 117 4>? Is that connected here as well? > Better list it as well then, even if you don't use it. > We have it but not using it, we will list it. > > + 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>; > > Same grouping for reg and interrupt-map as above for interrupts. Grouping reg and interrupt-map as tuples will make lengthy line and reduces readability, is it compulsory ? > > > + reg-names = "breg", "pcireg", "cfg"; > > + ranges = <0x0200 0x 0xE100 0x > > + 0xE100 0 0x0F00>; > > No I/O space or prefetcheable memory? > > 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 v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
Hello,
I've got a few comments below.
On Sat, Oct 17, 2015 at 12:52:18PM +0530, 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
> ---
> Added MSI domain implementation for handling MSI interrupts
> Added implementation for chained irq handlers
> Added legacy domain for handling legacy interrupts
> Added child node for handling legacy interrupt
[..]
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
[..]
> +static inline bool nwl_pcie_link_up(struct nwl_pcie *pcie, u32 check_link_up)
> +{
> + unsigned int status = -EINVAL;
> +
> + if (check_link_up == PCIE_USER_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> + PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> + else if (check_link_up == PHY_RDY_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> + PHY_RDY_LINKUP_BIT) ? 1 : 0;
> + return status;
It would seem marginally simpler if the the bit to check was passed as
the argument, instead of a check_link_up -> bit mapping.
> +}
> +
> +/**
> + * nwl_pcie_get_config_base - Get configuration base
> + *
> + * @bus: Bus structure of current bus
> + * @devfn: Device/function
> + * @where: Offset from base
> + *
> + * Return: Base address of the configuration space needed to be
> + * accessed.
> + */
> +static void __iomem *nwl_pcie_get_config_base(struct pci_bus *bus,
> + unsigned int devfn,
> + int where)
> +{
> + struct nwl_pcie *pcie = bus->sysdata;
> + int relbus;
> +
> + relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
> + (devfn << ECAM_DEV_LOC_SHIFT);
> +
> + return pcie->ecam_base + relbus + where;
> +}
It would seem you could simplify if you provide this as your map_bus
implementation of pci_ops. Then you could use the
pci_generic_config_read/_write callbacks.
> +/**
> + * nwl_setup_sspl - Set Slot Power limit
> + *
> + * @pcie: PCIe port information
> + */
> +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> +{
> + unsigned int status;
> + int retval = 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;
This assignment looks useless?
> + mdelay(2);
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> + & MSG_DONE_BIT;
> + if (status) {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> + & MSG_DONE_STATUS_BIT;
> + if (status == SLVERR) {
> + dev_err(pcie->dev, "AXI slave error");
> + retval = SLVERR;
> + } else if (status == DECERR) {
> + dev_err(pcie->dev, "AXI Decode error");
> + retval = DECERR;
> + }
> + } else {
> + retval = 1;
> + }
> + }
> + } while (status);
> +
> + return retval;
> +}
> +
[..]
> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> + unsigned int devfn,
> + int where,
> + int size,
> + u32 val)
> +{
> + void __iomem *addr;
> + int retval;
> + struct nwl_pcie *pcie = bus->sysdata;
> +
> + if (!bus->number && devfn > 0)
> + retu
Re: [PATCH v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Saturday 17 October 2015 12:52:18 Bharat Kumar Gogada wrote:
> + "msi_1, msi_0": interrupt asserted when msi is recieved
Better avoid underscores in DT, use "msi0" instead of "msi_0".
> +- 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
> +- pcie_intc: 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.
The name doesn't match: below, the name is "legacy-interrupt-controller",
not "pcie_intc". I suppose it should really be "interrupt-controller"
anyway.
> +
> +Example:
> +
> +
> +nwl_pcie: pcie@fd0e {
> + #address-cells = >;
> + #size-cells = <2>;
> + compatible = "xlnx,nwl-pcie-2.11";
> + #interrupt-cells = <1>;
> + msi-controller;
> + device_type = "pci";
> + interrupt-parent = <&gic>;
> + interrupts = < 0 118 4
> + 0 116 4
> + 0 115 4 // MSI_1 [63...32]
> + 0 114 4 >; // MSI_0 [31...0]
Better write these as tuples:
interrupts = <0 118 4>, <0 116 4>, <0 115 4>, <0 114 4>;
And maybe reverse the order? It looks that might be what the
soc integration person had in mind.
Also, what is interrupt <0 117 4>? Is that connected here as well?
Better list it as well then, even if you don't use it.
> + 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>;
Same grouping for reg and interrupt-map as above for interrupts.
> + reg-names = "breg", "pcireg", "cfg";
> + ranges = <0x0200 0x 0xE100 0x 0xE100 0
> 0x0F00>;
No I/O space or prefetcheable memory?
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
[PATCH v4] 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
---
Added MSI domain implementation for handling MSI interrupts
Added implementation for chained irq handlers
Added legacy domain for handling legacy interrupts
Added child node for handling legacy interrupt
---
.../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 71 ++
drivers/pci/host/Kconfig |9 +
drivers/pci/host/Makefile |1 +
drivers/pci/host/pcie-xilinx-nwl.c | 1066
4 files changed, 1147 insertions(+), 0 deletions(-)
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..d64fc97
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,71 @@
+* 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:
+ "misc": interrupt asserted when miscellaneous is recieved
+ "intx": interrupt that is asserted when an legacy interrupt is received
+ "msi_1, msi_0": interrupt asserted when msi is recieved
+- 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
+- pcie_intc: 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 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";
+ 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 c132bdd..33bbddb 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 PCIE_XILINX_NWL
+ bool "NWL PCIe Core"
+ depends on ARCH_ZYNQMP
+ 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-$
