Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1270277 > unrolled thread

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

Started byMarc Zyngier <marc.zyngier@arm.com>
First post2015-11-16 16:20 +0100
Last post2015-11-18 08:00 +0100
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Marc Zyngier <marc.zyngier@arm.com> - 2015-11-16 16:20 +0100
    Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Ray Jui <rjui@broadcom.com> - 2015-11-16 23:10 +0100
      RE: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> - 2015-11-17 06:00 +0100
        Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Marc Zyngier <marc.zyngier@arm.com> - 2015-11-17 10:30 +0100
          RE: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> - 2015-11-17 14:30 +0100
            Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Marc Zyngier <marc.zyngier@arm.com> - 2015-11-17 15:00 +0100
              Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Ray Jui <rjui@broadcom.com> - 2015-11-17 17:30 +0100
                RE: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL  PCIe Host Controller Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> - 2015-11-18 08:00 +0100

#1270277 — Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-11-16 16:20 +0100
SubjectRe: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
Message-ID<qvtJw-wB-23@gated-at.bofh.it>
On 11/11/15 06:33, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> ---
> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc function.
> Moved MSI functionality to separate functions.
> Changed error return values.
> ---
>  .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
>  drivers/pci/host/Kconfig                           |   16 +-
>  drivers/pci/host/Makefile                          |    1 +
>  drivers/pci/host/pcie-xilinx-nwl.c                 | 1062 ++++++++++++++++++++
>  4 files changed, 1144 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
>  create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
> 

[...]

> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus *bus)
> +{
> +	struct platform_device *pdev = to_platform_device(pcie->dev);
> +	struct nwl_msi *msi = &pcie->msi;
> +	unsigned long base;
> +	int ret;
> +
> +	mutex_init(&msi->lock);
> +
> +	/* Check for msii_present bit */
> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) & MSII_PRESENT;
> +	if (!ret) {
> +		dev_err(pcie->dev, "MSI not present\n");
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	/* Enable MSII */
> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> +			  MSII_ENABLE, I_MSII_CONTROL);
> +
> +	/* Enable MSII status */
> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
> +
> +	/* setup AFI/FPCI range */
> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> +	base = virt_to_phys((void *)msi->pages);
> +	nwl_bridge_writel(pcie, lower_32_bits(base), I_MSII_BASE_LO);
> +	nwl_bridge_writel(pcie, upper_32_bits(base), I_MSII_BASE_HI);

BTW, you still haven't answered my question as to why you need to waste
a page of memory here, and why putting a device address doesn't work.

As this is (to the best of my knowledge) the only driver doing so, I'd
really like you to explain the rational behind this.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1270635

FromRay Jui <rjui@broadcom.com>
Date2015-11-16 23:10 +0100
Message-ID<qvA8h-4Gl-1@gated-at.bofh.it>
In reply to#1270277

On 11/16/2015 7:14 AM, Marc Zyngier wrote:
> On 11/11/15 06:33, Bharat Kumar Gogada wrote:
>> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
>>
>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
>> ---
>> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc function.
>> Moved MSI functionality to separate functions.
>> Changed error return values.
>> ---
>>   .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
>>   drivers/pci/host/Kconfig                           |   16 +-
>>   drivers/pci/host/Makefile                          |    1 +
>>   drivers/pci/host/pcie-xilinx-nwl.c                 | 1062 ++++++++++++++++++++
>>   4 files changed, 1144 insertions(+), 3 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
>>   create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
>>
>
> [...]
>
>> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus *bus)
>> +{
>> +	struct platform_device *pdev = to_platform_device(pcie->dev);
>> +	struct nwl_msi *msi = &pcie->msi;
>> +	unsigned long base;
>> +	int ret;
>> +
>> +	mutex_init(&msi->lock);
>> +
>> +	/* Check for msii_present bit */
>> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) & MSII_PRESENT;
>> +	if (!ret) {
>> +		dev_err(pcie->dev, "MSI not present\n");
>> +		ret = -EIO;
>> +		goto err;
>> +	}
>> +
>> +	/* Enable MSII */
>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
>> +			  MSII_ENABLE, I_MSII_CONTROL);
>> +
>> +	/* Enable MSII status */
>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
>> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
>> +
>> +	/* setup AFI/FPCI range */
>> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>> +	base = virt_to_phys((void *)msi->pages);
>> +	nwl_bridge_writel(pcie, lower_32_bits(base), I_MSII_BASE_LO);
>> +	nwl_bridge_writel(pcie, upper_32_bits(base), I_MSII_BASE_HI);
>
> BTW, you still haven't answered my question as to why you need to waste
> a page of memory here, and why putting a device address doesn't work.
>
> As this is (to the best of my knowledge) the only driver doing so, I'd
> really like you to explain the rational behind this.

Might not be the only driver doing so after I start sending out patches 
for the iProc MSI support (soon), :)

I'm not sure how it works for the Xilinx NWL controller, which Bharat 
should be able to help to explain. But for the iProc MSI controller, 
there's no device I/O memory reserved for MSI posted writes in the ASIC. 
Therefore one needs to reserve host memory for these writes.

Ray

>
> Thanks,
>
> 	M.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1270871

FromBharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Date2015-11-17 06:00 +0100
Message-ID<qvGx4-gl-3@gated-at.bofh.it>
In reply to#1270635
> On 11/16/2015 7:14 AM, Marc Zyngier wrote:
> > On 11/11/15 06:33, Bharat Kumar Gogada wrote:
> >> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >>
> >> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> >> ---
> >> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc
> function.
> >> Moved MSI functionality to separate functions.
> >> Changed error return values.
> >> ---
> >>   .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
> >>   drivers/pci/host/Kconfig                           |   16 +-
> >>   drivers/pci/host/Makefile                          |    1 +
> >>   drivers/pci/host/pcie-xilinx-nwl.c                 | 1062
> ++++++++++++++++++++
> >>   4 files changed, 1144 insertions(+), 3 deletions(-)
> >>   create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-
> pcie.txt
> >>   create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
> >>
> >
> > [...]
> >
> >> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus
> >> +*bus) {
> >> +	struct platform_device *pdev = to_platform_device(pcie->dev);
> >> +	struct nwl_msi *msi = &pcie->msi;
> >> +	unsigned long base;
> >> +	int ret;
> >> +
> >> +	mutex_init(&msi->lock);
> >> +
> >> +	/* Check for msii_present bit */
> >> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) & MSII_PRESENT;
> >> +	if (!ret) {
> >> +		dev_err(pcie->dev, "MSI not present\n");
> >> +		ret = -EIO;
> >> +		goto err;
> >> +	}
> >> +
> >> +	/* Enable MSII */
> >> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> >> +			  MSII_ENABLE, I_MSII_CONTROL);
> >> +
> >> +	/* Enable MSII status */
> >> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> >> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
> >> +
> >> +	/* setup AFI/FPCI range */
> >> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> >> +	base = virt_to_phys((void *)msi->pages);
> >> +	nwl_bridge_writel(pcie, lower_32_bits(base), I_MSII_BASE_LO);
> >> +	nwl_bridge_writel(pcie, upper_32_bits(base), I_MSII_BASE_HI);
> >
> > BTW, you still haven't answered my question as to why you need to
> > waste a page of memory here, and why putting a device address doesn't
> work.
> >
> > As this is (to the best of my knowledge) the only driver doing so, I'd
> > really like you to explain the rational behind this.
> 
> Might not be the only driver doing so after I start sending out patches for the
> iProc MSI support (soon), :)
> 
> I'm not sure how it works for the Xilinx NWL controller, which Bharat should
> be able to help to explain. But for the iProc MSI controller, there's no device
> I/O memory reserved for MSI posted writes in the ASIC.
> Therefore one needs to reserve host memory for these writes.
> >

Our SoC doesn't reserve any memory for MSI, hence we need to assign a memory space for it out of RAM.

Regards,
Bharat


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271003

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-11-17 10:30 +0100
Message-ID<qvKKm-397-21@gated-at.bofh.it>
In reply to#1270871
On Tue, 17 Nov 2015 04:59:39 +0000
Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> wrote:

> > On 11/16/2015 7:14 AM, Marc Zyngier wrote:
> > > On 11/11/15 06:33, Bharat Kumar Gogada wrote:
> > >> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> > >>
> > >> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > >> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> > >> ---
> > >> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc
> > function.
> > >> Moved MSI functionality to separate functions.
> > >> Changed error return values.
> > >> ---
> > >>   .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
> > >>   drivers/pci/host/Kconfig                           |   16 +-
> > >>   drivers/pci/host/Makefile                          |    1 +
> > >>   drivers/pci/host/pcie-xilinx-nwl.c                 | 1062
> > ++++++++++++++++++++
> > >>   4 files changed, 1144 insertions(+), 3 deletions(-)
> > >>   create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-
> > pcie.txt
> > >>   create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
> > >>
> > >
> > > [...]
> > >
> > >> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct pci_bus
> > >> +*bus) {
> > >> +	struct platform_device *pdev = to_platform_device(pcie->dev);
> > >> +	struct nwl_msi *msi = &pcie->msi;
> > >> +	unsigned long base;
> > >> +	int ret;
> > >> +
> > >> +	mutex_init(&msi->lock);
> > >> +
> > >> +	/* Check for msii_present bit */
> > >> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) & MSII_PRESENT;
> > >> +	if (!ret) {
> > >> +		dev_err(pcie->dev, "MSI not present\n");
> > >> +		ret = -EIO;
> > >> +		goto err;
> > >> +	}
> > >> +
> > >> +	/* Enable MSII */
> > >> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> > >> +			  MSII_ENABLE, I_MSII_CONTROL);
> > >> +
> > >> +	/* Enable MSII status */
> > >> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, I_MSII_CONTROL) |
> > >> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
> > >> +
> > >> +	/* setup AFI/FPCI range */
> > >> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> > >> +	base = virt_to_phys((void *)msi->pages);
> > >> +	nwl_bridge_writel(pcie, lower_32_bits(base), I_MSII_BASE_LO);
> > >> +	nwl_bridge_writel(pcie, upper_32_bits(base), I_MSII_BASE_HI);
> > >
> > > BTW, you still haven't answered my question as to why you need to
> > > waste a page of memory here, and why putting a device address doesn't
> > work.
> > >
> > > As this is (to the best of my knowledge) the only driver doing so, I'd
> > > really like you to explain the rational behind this.
> > 
> > Might not be the only driver doing so after I start sending out patches for the
> > iProc MSI support (soon), :)
> > 
> > I'm not sure how it works for the Xilinx NWL controller, which Bharat should
> > be able to help to explain. But for the iProc MSI controller, there's no device
> > I/O memory reserved for MSI posted writes in the ASIC.
> > Therefore one needs to reserve host memory for these writes.
> > >
> 
> Our SoC doesn't reserve any memory for MSI, hence we need to assign a
> memory space for it out of RAM.

Question to both of you: Does the write make it to memory? Or is it
sampled by the bridge and dropped?

What happens if you replace the page in RAM with a dummy address?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271181

FromBharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Date2015-11-17 14:30 +0100
Message-ID<qvOuB-5zN-3@gated-at.bofh.it>
In reply to#1271003
> 
> On Tue, 17 Nov 2015 04:59:39 +0000
> Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> wrote:
> 
> > > On 11/16/2015 7:14 AM, Marc Zyngier wrote:
> > > > On 11/11/15 06:33, Bharat Kumar Gogada wrote:
> > > >> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> > > >>
> > > >> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > > >> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> > > >> ---
> > > >> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc
> > > function.
> > > >> Moved MSI functionality to separate functions.
> > > >> Changed error return values.
> > > >> ---
> > > >>   .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
> > > >>   drivers/pci/host/Kconfig                           |   16 +-
> > > >>   drivers/pci/host/Makefile                          |    1 +
> > > >>   drivers/pci/host/pcie-xilinx-nwl.c                 | 1062
> > > ++++++++++++++++++++
> > > >>   4 files changed, 1144 insertions(+), 3 deletions(-)
> > > >>   create mode 100644
> > > >> Documentation/devicetree/bindings/pci/xilinx-nwl-
> > > pcie.txt
> > > >>   create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
> > > >>
> > > >
> > > > [...]
> > > >
> > > >> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct
> > > >> +pci_bus
> > > >> +*bus) {
> > > >> +	struct platform_device *pdev = to_platform_device(pcie-
> >dev);
> > > >> +	struct nwl_msi *msi = &pcie->msi;
> > > >> +	unsigned long base;
> > > >> +	int ret;
> > > >> +
> > > >> +	mutex_init(&msi->lock);
> > > >> +
> > > >> +	/* Check for msii_present bit */
> > > >> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) &
> MSII_PRESENT;
> > > >> +	if (!ret) {
> > > >> +		dev_err(pcie->dev, "MSI not present\n");
> > > >> +		ret = -EIO;
> > > >> +		goto err;
> > > >> +	}
> > > >> +
> > > >> +	/* Enable MSII */
> > > >> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> I_MSII_CONTROL) |
> > > >> +			  MSII_ENABLE, I_MSII_CONTROL);
> > > >> +
> > > >> +	/* Enable MSII status */
> > > >> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> I_MSII_CONTROL) |
> > > >> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
> > > >> +
> > > >> +	/* setup AFI/FPCI range */
> > > >> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> > > >> +	base = virt_to_phys((void *)msi->pages);
> > > >> +	nwl_bridge_writel(pcie, lower_32_bits(base),
> I_MSII_BASE_LO);
> > > >> +	nwl_bridge_writel(pcie, upper_32_bits(base),
> I_MSII_BASE_HI);
> > > >
> > > > BTW, you still haven't answered my question as to why you need to
> > > > waste a page of memory here, and why putting a device address
> > > > doesn't
> > > work.
> > > >
> > > > As this is (to the best of my knowledge) the only driver doing so,
> > > > I'd really like you to explain the rational behind this.
> > >
> > > Might not be the only driver doing so after I start sending out
> > > patches for the iProc MSI support (soon), :)
> > >
> > > I'm not sure how it works for the Xilinx NWL controller, which
> > > Bharat should be able to help to explain. But for the iProc MSI
> > > controller, there's no device I/O memory reserved for MSI posted writes
> in the ASIC.
> > > Therefore one needs to reserve host memory for these writes.
> > > >
> >
> > Our SoC doesn't reserve any memory for MSI, hence we need to assign a
> > memory space for it out of RAM.
> 
> Question to both of you: Does the write make it to memory? Or is it sampled
> by the bridge and dropped?
> 
No, write will not do any modification in memory, it is consumed by bridge.

> What happens if you replace the page in RAM with a dummy address?
What do you mean by dummy address ?

Regards,
Bharat


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271222

FromMarc Zyngier <marc.zyngier@arm.com>
Date2015-11-17 15:00 +0100
Message-ID<qvOXE-5L7-15@gated-at.bofh.it>
In reply to#1271181
On 17/11/15 13:27, Bharat Kumar Gogada wrote:
>>
>> On Tue, 17 Nov 2015 04:59:39 +0000
>> Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> wrote:
>>
>>>> On 11/16/2015 7:14 AM, Marc Zyngier wrote:
>>>>> On 11/11/15 06:33, Bharat Kumar Gogada wrote:
>>>>>> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
>>>>>>
>>>>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>>>>> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
>>>>>> ---
>>>>>> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc
>>>> function.
>>>>>> Moved MSI functionality to separate functions.
>>>>>> Changed error return values.
>>>>>> ---
>>>>>>   .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
>>>>>>   drivers/pci/host/Kconfig                           |   16 +-
>>>>>>   drivers/pci/host/Makefile                          |    1 +
>>>>>>   drivers/pci/host/pcie-xilinx-nwl.c                 | 1062
>>>> ++++++++++++++++++++
>>>>>>   4 files changed, 1144 insertions(+), 3 deletions(-)
>>>>>>   create mode 100644
>>>>>> Documentation/devicetree/bindings/pci/xilinx-nwl-
>>>> pcie.txt
>>>>>>   create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct
>>>>>> +pci_bus
>>>>>> +*bus) {
>>>>>> +	struct platform_device *pdev = to_platform_device(pcie-
>>> dev);
>>>>>> +	struct nwl_msi *msi = &pcie->msi;
>>>>>> +	unsigned long base;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	mutex_init(&msi->lock);
>>>>>> +
>>>>>> +	/* Check for msii_present bit */
>>>>>> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) &
>> MSII_PRESENT;
>>>>>> +	if (!ret) {
>>>>>> +		dev_err(pcie->dev, "MSI not present\n");
>>>>>> +		ret = -EIO;
>>>>>> +		goto err;
>>>>>> +	}
>>>>>> +
>>>>>> +	/* Enable MSII */
>>>>>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
>> I_MSII_CONTROL) |
>>>>>> +			  MSII_ENABLE, I_MSII_CONTROL);
>>>>>> +
>>>>>> +	/* Enable MSII status */
>>>>>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
>> I_MSII_CONTROL) |
>>>>>> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
>>>>>> +
>>>>>> +	/* setup AFI/FPCI range */
>>>>>> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>>>>> +	base = virt_to_phys((void *)msi->pages);
>>>>>> +	nwl_bridge_writel(pcie, lower_32_bits(base),
>> I_MSII_BASE_LO);
>>>>>> +	nwl_bridge_writel(pcie, upper_32_bits(base),
>> I_MSII_BASE_HI);
>>>>>
>>>>> BTW, you still haven't answered my question as to why you need to
>>>>> waste a page of memory here, and why putting a device address
>>>>> doesn't
>>>> work.
>>>>>
>>>>> As this is (to the best of my knowledge) the only driver doing so,
>>>>> I'd really like you to explain the rational behind this.
>>>>
>>>> Might not be the only driver doing so after I start sending out
>>>> patches for the iProc MSI support (soon), :)
>>>>
>>>> I'm not sure how it works for the Xilinx NWL controller, which
>>>> Bharat should be able to help to explain. But for the iProc MSI
>>>> controller, there's no device I/O memory reserved for MSI posted writes
>> in the ASIC.
>>>> Therefore one needs to reserve host memory for these writes.
>>>>>
>>>
>>> Our SoC doesn't reserve any memory for MSI, hence we need to assign a
>>> memory space for it out of RAM.
>>
>> Question to both of you: Does the write make it to memory? Or is it sampled
>> by the bridge and dropped?
>>
> No, write will not do any modification in memory, it is consumed by bridge.

Then you do not need to allocate memory at all. Use whatever memory you
already have. CC-ing Robin, as this may have interaction with the SMMU.

> 
>> What happens if you replace the page in RAM with a dummy address?
> What do you mean by dummy address ?

Any random (and suitably aligned) address. 0x00000deadbeef000 for example.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271393

FromRay Jui <rjui@broadcom.com>
Date2015-11-17 17:30 +0100
Message-ID<qvRiQ-7og-49@gated-at.bofh.it>
In reply to#1271222

On 11/17/2015 5:55 AM, Marc Zyngier wrote:
> On 17/11/15 13:27, Bharat Kumar Gogada wrote:
>>>
>>> On Tue, 17 Nov 2015 04:59:39 +0000
>>> Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> wrote:
>>>
>>>>> On 11/16/2015 7:14 AM, Marc Zyngier wrote:
>>>>>> On 11/11/15 06:33, Bharat Kumar Gogada wrote:
>>>>>>> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
>>>>>>>
>>>>>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>>>>>> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
>>>>>>> ---
>>>>>>> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc
>>>>> function.
>>>>>>> Moved MSI functionality to separate functions.
>>>>>>> Changed error return values.
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
>>>>>>>    drivers/pci/host/Kconfig                           |   16 +-
>>>>>>>    drivers/pci/host/Makefile                          |    1 +
>>>>>>>    drivers/pci/host/pcie-xilinx-nwl.c                 | 1062
>>>>> ++++++++++++++++++++
>>>>>>>    4 files changed, 1144 insertions(+), 3 deletions(-)
>>>>>>>    create mode 100644
>>>>>>> Documentation/devicetree/bindings/pci/xilinx-nwl-
>>>>> pcie.txt
>>>>>>>    create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
>>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct
>>>>>>> +pci_bus
>>>>>>> +*bus) {
>>>>>>> +	struct platform_device *pdev = to_platform_device(pcie-
>>>> dev);
>>>>>>> +	struct nwl_msi *msi = &pcie->msi;
>>>>>>> +	unsigned long base;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	mutex_init(&msi->lock);
>>>>>>> +
>>>>>>> +	/* Check for msii_present bit */
>>>>>>> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) &
>>> MSII_PRESENT;
>>>>>>> +	if (!ret) {
>>>>>>> +		dev_err(pcie->dev, "MSI not present\n");
>>>>>>> +		ret = -EIO;
>>>>>>> +		goto err;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/* Enable MSII */
>>>>>>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
>>> I_MSII_CONTROL) |
>>>>>>> +			  MSII_ENABLE, I_MSII_CONTROL);
>>>>>>> +
>>>>>>> +	/* Enable MSII status */
>>>>>>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
>>> I_MSII_CONTROL) |
>>>>>>> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
>>>>>>> +
>>>>>>> +	/* setup AFI/FPCI range */
>>>>>>> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>>>>>> +	base = virt_to_phys((void *)msi->pages);
>>>>>>> +	nwl_bridge_writel(pcie, lower_32_bits(base),
>>> I_MSII_BASE_LO);
>>>>>>> +	nwl_bridge_writel(pcie, upper_32_bits(base),
>>> I_MSII_BASE_HI);
>>>>>>
>>>>>> BTW, you still haven't answered my question as to why you need to
>>>>>> waste a page of memory here, and why putting a device address
>>>>>> doesn't
>>>>> work.
>>>>>>
>>>>>> As this is (to the best of my knowledge) the only driver doing so,
>>>>>> I'd really like you to explain the rational behind this.
>>>>>
>>>>> Might not be the only driver doing so after I start sending out
>>>>> patches for the iProc MSI support (soon), :)
>>>>>
>>>>> I'm not sure how it works for the Xilinx NWL controller, which
>>>>> Bharat should be able to help to explain. But for the iProc MSI
>>>>> controller, there's no device I/O memory reserved for MSI posted writes
>>> in the ASIC.
>>>>> Therefore one needs to reserve host memory for these writes.
>>>>>>
>>>>
>>>> Our SoC doesn't reserve any memory for MSI, hence we need to assign a
>>>> memory space for it out of RAM.
>>>
>>> Question to both of you: Does the write make it to memory? Or is it sampled
>>> by the bridge and dropped?
>>>
>> No, write will not do any modification in memory, it is consumed by bridge.
>
> Then you do not need to allocate memory at all. Use whatever memory you
> already have. CC-ing Robin, as this may have interaction with the SMMU.
>
>>
>>> What happens if you replace the page in RAM with a dummy address?
>> What do you mean by dummy address ?
>
> Any random (and suitably aligned) address. 0x00000deadbeef000 for example.

In our case, I'm pretty sure the writes make it to memory (RAM). I can 
try replacing it with a dummy address, but I'm pretty sure that will not 
work.

Thanks,

Ray

>
> Thanks,
>
> 	M.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1271883

FromBharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Date2015-11-18 08:00 +0100
Message-ID<qw4SK-7Nb-5@gated-at.bofh.it>
In reply to#1271393
> Subject: Re: [PATCH v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> 
> 
> 
> On 11/17/2015 5:55 AM, Marc Zyngier wrote:
> > On 17/11/15 13:27, Bharat Kumar Gogada wrote:
> >>>
> >>> On Tue, 17 Nov 2015 04:59:39 +0000
> >>> Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> wrote:
> >>>
> >>>>> On 11/16/2015 7:14 AM, Marc Zyngier wrote:
> >>>>>> On 11/11/15 06:33, Bharat Kumar Gogada wrote:
> >>>>>>> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >>>>>>>
> >>>>>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>>>>>> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> >>>>>>> ---
> >>>>>>> Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc
> >>>>> function.
> >>>>>>> Moved MSI functionality to separate functions.
> >>>>>>> Changed error return values.
> >>>>>>> ---
> >>>>>>>    .../devicetree/bindings/pci/xilinx-nwl-pcie.txt    |   68 ++
> >>>>>>>    drivers/pci/host/Kconfig                           |   16 +-
> >>>>>>>    drivers/pci/host/Makefile                          |    1 +
> >>>>>>>    drivers/pci/host/pcie-xilinx-nwl.c                 | 1062
> >>>>> ++++++++++++++++++++
> >>>>>>>    4 files changed, 1144 insertions(+), 3 deletions(-)
> >>>>>>>    create mode 100644
> >>>>>>> Documentation/devicetree/bindings/pci/xilinx-nwl-
> >>>>> pcie.txt
> >>>>>>>    create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c
> >>>>>>>
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>> +static int nwl_pcie_enable_msi(struct nwl_pcie *pcie, struct
> >>>>>>> +pci_bus
> >>>>>>> +*bus) {
> >>>>>>> +	struct platform_device *pdev = to_platform_device(pcie-
> >>>> dev);
> >>>>>>> +	struct nwl_msi *msi = &pcie->msi;
> >>>>>>> +	unsigned long base;
> >>>>>>> +	int ret;
> >>>>>>> +
> >>>>>>> +	mutex_init(&msi->lock);
> >>>>>>> +
> >>>>>>> +	/* Check for msii_present bit */
> >>>>>>> +	ret = nwl_bridge_readl(pcie, I_MSII_CAPABILITIES) &
> >>> MSII_PRESENT;
> >>>>>>> +	if (!ret) {
> >>>>>>> +		dev_err(pcie->dev, "MSI not present\n");
> >>>>>>> +		ret = -EIO;
> >>>>>>> +		goto err;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	/* Enable MSII */
> >>>>>>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> >>> I_MSII_CONTROL) |
> >>>>>>> +			  MSII_ENABLE, I_MSII_CONTROL);
> >>>>>>> +
> >>>>>>> +	/* Enable MSII status */
> >>>>>>> +	nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> >>> I_MSII_CONTROL) |
> >>>>>>> +			  MSII_STATUS_ENABLE, I_MSII_CONTROL);
> >>>>>>> +
> >>>>>>> +	/* setup AFI/FPCI range */
> >>>>>>> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
> >>>>>>> +	base = virt_to_phys((void *)msi->pages);
> >>>>>>> +	nwl_bridge_writel(pcie, lower_32_bits(base),
> >>> I_MSII_BASE_LO);
> >>>>>>> +	nwl_bridge_writel(pcie, upper_32_bits(base),
> >>> I_MSII_BASE_HI);
> >>>>>>
> >>>>>> BTW, you still haven't answered my question as to why you need to
> >>>>>> waste a page of memory here, and why putting a device address
> >>>>>> doesn't
> >>>>> work.
> >>>>>>
> >>>>>> As this is (to the best of my knowledge) the only driver doing
> >>>>>> so, I'd really like you to explain the rational behind this.
> >>>>>
> >>>>> Might not be the only driver doing so after I start sending out
> >>>>> patches for the iProc MSI support (soon), :)
> >>>>>
> >>>>> I'm not sure how it works for the Xilinx NWL controller, which
> >>>>> Bharat should be able to help to explain. But for the iProc MSI
> >>>>> controller, there's no device I/O memory reserved for MSI posted
> >>>>> writes
> >>> in the ASIC.
> >>>>> Therefore one needs to reserve host memory for these writes.
> >>>>>>
> >>>>
> >>>> Our SoC doesn't reserve any memory for MSI, hence we need to assign
> >>>> a memory space for it out of RAM.
> >>>
> >>> Question to both of you: Does the write make it to memory? Or is it
> >>> sampled by the bridge and dropped?
> >>>
> >> No, write will not do any modification in memory, it is consumed by
> bridge.
> >
> > Then you do not need to allocate memory at all. Use whatever memory
> > you already have. CC-ing Robin, as this may have interaction with the
> SMMU.
> >
> >>
Ok, I will try with some random address, without allocating any memory and test it, and will
update accordingly in next patch. 
> >>> What happens if you replace the page in RAM with a dummy address?
> >> What do you mean by dummy address ?
> >
> > Any random (and suitably aligned) address. 0x00000deadbeef000 for
> example.
> 
> In our case, I'm pretty sure the writes make it to memory (RAM). I can try
> replacing it with a dummy address, but I'm pretty sure that will not work.
> 
Thanks,
Bharat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web