RE: [PATCH v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> Subject: Re: [PATCH v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL > PCIe Host Controller > > On Wed, 25 Nov 2015 05:40:49 + > Bharat Kumar Gogada wrote: > > > > On Thu, 19 Nov 2015 11:05:23 +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 > > > > Acked-by: Rob Herring > > > > --- > > > > + > > > > +#define MSI_ADDRESS0xDEED > > > > > > How did you pick this value? What if it intersect with some actual RAM? > > > What if a device actually does DMA to that location? > > > > > > Wouldn't it make sense to actually pick a real *device* address (hint: > > > your MSI controller itself) for this purpose, as the device will > > > never DMA there? > > > > > > > > We have already mentioned in previous patch discussion, we don't have > > any device address on our SOC for MSI, that's the reason we are > > allocating a page for MSI in RAM. Since our memory write is consumed > > by bridge and doesn't write to memory, you suggested to use some > > random address, so using some random address. > > This is becoming painful. > > - "write is consumed by bridge and doesn't write to memory": So why are > you using something that has a chance of actually being memory??? Are > you in the business of corrupting unsuspecting data? > > - "we don't have any device address on our SOC for MSI": You have > plenty, and that's the whole of your device space. *All of it*. So > just take the base address of your PCIe controller, and be done with > it. Or your UART. Anything that cannot be DMA'ed to from a PCIe > device, and that is downstream of your PCIe bridge. > Yes, PCIe controller base will be fine, will send next patch addressing this. -- 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 v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Wed, 25 Nov 2015 14:23:29 +0530 Amit Tomer wrote: > Sorry to intervene but just trying to learn from your comments. > > > You have plenty, and that's the whole of your device space. *All of it*. So > > just take the base address of your PCIe controller, and be done with > > it. > > but isn't few of PCIe controller's registers itself are mapped > here(base address). So, how can we use this address for MSI? You can, because the PCIe controller never writes to itself. If it writes to that base address, then it *is* the MSI doorbell and the bridge will hopefully do the right thing. > Or you said from base address of PCIe controller, find an offset that > can be used as MSI address? That works as well. Given the description of the HW we've been given, any address will do, as long as it is behind the PCIe RC. Thanks, M. -- Jazz is not dead. It just smells funny. -- 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 v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
Sorry to intervene but just trying to learn from your comments. > You have plenty, and that's the whole of your device space. *All of it*. So > just take the base address of your PCIe controller, and be done with > it. but isn't few of PCIe controller's registers itself are mapped here(base address). So, how can we use this address for MSI? Or you said from base address of PCIe controller, find an offset that can be used as MSI address? Thanks Amit. -- 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 v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Wed, 25 Nov 2015 05:40:49 + Bharat Kumar Gogada wrote: > > On Thu, 19 Nov 2015 11:05:23 +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 > > > Acked-by: Rob Herring > > > --- > > > + > > > +#define MSI_ADDRESS 0xDEED > > > > How did you pick this value? What if it intersect with some actual RAM? > > What if a device actually does DMA to that location? > > > > Wouldn't it make sense to actually pick a real *device* address (hint: > > your MSI controller itself) for this purpose, as the device will never DMA > > there? > > > > > We have already mentioned in previous patch discussion, we don't have > any device address on our SOC for MSI, that's the reason we are > allocating a page for MSI in RAM. Since our memory write is consumed > by bridge and doesn't write to memory, you suggested to use some > random address, so using some random address. This is becoming painful. - "write is consumed by bridge and doesn't write to memory": So why are you using something that has a chance of actually being memory??? Are you in the business of corrupting unsuspecting data? - "we don't have any device address on our SOC for MSI": You have plenty, and that's the whole of your device space. *All of it*. So just take the base address of your PCIe controller, and be done with it. Or your UART. Anything that cannot be DMA'ed to from a PCIe device, and that is downstream of your PCIe bridge. M. -- Jazz is not dead. It just smells funny. -- 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 v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
> On Thu, 19 Nov 2015 11:05:23 +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
> > Acked-by: Rob Herring
> > ---
> > +
> > +#define MSI_ADDRESS0xDEED
>
> How did you pick this value? What if it intersect with some actual RAM?
> What if a device actually does DMA to that location?
>
> Wouldn't it make sense to actually pick a real *device* address (hint:
> your MSI controller itself) for this purpose, as the device will never DMA
> there?
>
>
We have already mentioned in previous patch discussion, we don't have any
device address on our SOC for MSI, that's
the reason we are allocating a page for MSI in RAM. Since our memory write is
consumed by bridge and doesn't write to memory, you suggested to use
some random address, so using some random address.
>
>
> > +
> > +static int nwl_irq_domain_alloc(struct irq_domain *domain, unsigned int
> virq,
> > + unsigned int nr_irqs, void *args) {
> > + struct nwl_pcie *pcie = domain->host_data;
> > + struct nwl_msi *msi = &pcie->msi;
> > + int bit;
> > + int i;
> > + int ret;
> > +
> > + mutex_lock(&msi->lock);
> > + if (nr_irqs > 1) {
> > + ret = nwl_check_hwirq(msi, nr_irqs);
> > + if (ret < 0) {
> > + mutex_unlock(&msi->lock);
> > + return ret;
> > + }
> > + } else {
> > + ret = find_first_zero_bit(msi->used, INT_PCI_MSI_NR);
> > + if (ret == INT_PCI_MSI_NR) {
> > + mutex_unlock(&msi->lock);
> > + return -ENOSPC;
> > + }
> > + }
>
> Let's be serious for a minute. What's wrong with
> bitmap_find_next_zero_area, for example?
Ok, will explore this API and do accordingly, and address in next patch.
>
> > +
> > + for (i = 0; i < nr_irqs; i++) {
> > + bit = ret + i;
> > + set_bit(bit, msi->used);
> > +
> > + irq_domain_set_info(domain, virq + i, bit, &nwl_irq_chip,
> > + domain->host_data, handle_simple_irq,
> > + NULL, NULL);
> > + }
> > + mutex_unlock(&msi->lock);
> > +
> > + return 0;
> > +}
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny.
--
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 v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
On Thu, 19 Nov 2015 11:05:23 +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
> Acked-by: Rob Herring
> ---
> Changes for v9:
> - Modified logic in nwl_irq_domain_alloc to check availabilty of contiguous
> hw irq for multi MSI.
> - Removed allocation of page for MSI address, using dummy address.
> ---
> .../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 | 1100
>
> 4 files changed, 1179 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/Mak
