Re: [PATCH v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Wed, Oct 28, 2015 at 10:17:22AM +, 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
--
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 v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> 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?
> > +
> > + 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.
> > + struct platform_device *pdev = to_platform_device(pcie->dev);
> > + struct nwl_msi *msi = &pcie->msi;
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 v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
Hi Bharat,
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.
>
> Signed-off-by: Bharat Kumar Gogada
> Signed-off-by: Ravi Kiran Gummaluri
> ---
> Corrected device tree node name.
> Made tuples for interrupts, interrupt-map, reg properties.
> Added mutex lock in nwl_irq_domain_free function.
> Removed unneccessary casts.
> ---
> .../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 68 ++
> drivers/pci/host/Kconfig |9 +
> drivers/pci/host/Makefile |1 +
> drivers/pci/host/pcie-xilinx-nwl.c | 1057
>
Please update MAINTAINERS, too.
> +struct nwl_msi { /* struct nwl_msi - MSI information */
> + struct msi_controller msi_chip; /* msi_chip: MSI domain */
> + DECLARE_BITMAP(used, INT_PCI_MSI_NR); /* used: Declare Bitmap
> + for MSI */
> + struct irq_domain *dev_domain; /* domain: IRQ domain pointer */
> + unsigned long pages;/* pages: MSI pages */
> + struct mutex lock; /* lock: mutex lock */
> + int irq_msi0; /* irq_msi0: msi0 interrupt number */
> + int irq_msi1; /* irq_msi1: msi1 interrupt number */
You don't need to repeat the struct or member name in the comment.
> + * struct nwl_pcie - PCIe port information
> + *
> + * @dev: Device pointer
> + * @breg_base: IO Mapped Bridge Register Base
> + * @pcireg_base: IO Mapped PCIe controller attributes
> + * @ecam_base: IO Mapped configuration space
> + * @phys_breg_base: Physical Bridge Register Base
> + * @phys_pcie_reg_base: Physical PCIe Controller Attributes
> + * @phys_ecam_base: Physical Configuration Base
> + * @breg_size: Bridge Register space
> + * @pcie_reg_size: PCIe controller attributes space
> + * @ecam_size: PCIe Configuration space
> + * @irq_intx: Legacy interrupt number
> + * @irq_misc: Misc interrupt number
> + * @ecam_value: ECAM value
> + * @last_busno: Last Bus number configured
> + * @link_up: Link status flag
> + * @bus: PCI bus
> + * @msi: MSI domain
> + * @legacy_irq_domain: IRQ domain pointer
> + */
> +struct nwl_pcie {
> + struct device *dev;
> + void __iomem *breg_base;
> + void __iomem *pcireg_base;
> + void __iomem *ecam_base;
> + u32 phys_breg_base;
> + u32 phys_pcie_reg_base;
> + u32 phys_ecam_base;
> + u32 breg_size;
> + u32 pcie_reg_size;
> + u32 ecam_size;
> + int irq_intx;
> + int irq_misc;
> + u32 ecam_value;
> + u8 last_busno;
> + u8 link_up;
> + struct pci_bus *bus;
> + struct nwl_msi msi;
> + struct irq_domain *legacy_irq_domain;
Please put the comments on the same line as the member declaration.
You might not even need some of the comments, e.g., "Device pointer"
merely restates what we already know from the code without adding
anything useful.
> +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);
> + 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
Re: [PATCH v5] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Mon, Oct 26, 2015 at 9:56 AM, 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
> ---
> Corrected device tree node name.
> Made tuples for interrupts, interrupt-map, reg properties.
> Added mutex lock in nwl_irq_domain_free function.
> Removed unneccessary casts.
> ---
> .../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 68 ++
> drivers/pci/host/Kconfig |9 +
> drivers/pci/host/Makefile |1 +
> drivers/pci/host/pcie-xilinx-nwl.c | 1057
>
> 4 files changed, 1135 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..432fa1f
> --- /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.
Is this needed since you have the sub-node defining it?
> +- 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
...interrupts (add "s")
> +- interrupt-names: Must include the following entries:
> + "misc": interrupt asserted when miscellaneous is recieved
typo
> + "intx": interrupt that is asserted when an legacy interrupt is
> received
> + "msi1, msi0": interrupt asserted when msi is recieved
typo.
Please specify the order of the interrupts. The implied order here
doesn't match the example.
> +- 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 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 onl
[PATCH v5] 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
---
Corrected device tree node name.
Made tuples for interrupts, interrupt-map, reg properties.
Added mutex lock in nwl_irq_domain_free function.
Removed unneccessary casts.
---
.../devicetree/bindings/pci/xilinx-nwl-pcie.txt| 68 ++
drivers/pci/host/Kconfig |9 +
drivers/pci/host/Makefile |1 +
drivers/pci/host/pcie-xilinx-nwl.c | 1057
4 files changed, 1135 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..432fa1f
--- /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:
+ "misc": interrupt asserted when miscellaneous is recieved
+ "intx": interrupt that is asserted when an legacy interrupt is received
+ "msi1, msi0": 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
+- 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 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-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
+obj-$(CONFIG_PCI
