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

[Rob Herring]

On Tue, Nov 03, 2015 at 08:53:47PM +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 
> ---
> Removed msi_controller and added irq_domian for MSI domain hierarchy.
> Modified code for filling MSI address in struct msg.
> Added check for hwirq before passing it to irq_domain_set_info.
> ---
>  .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   68 ++

Acked-by: Rob Herring 

>  drivers/pci/host/Kconfig   |   10 +
>  drivers/pci/host/Makefile  |1 +
>  drivers/pci/host/pcie-xilinx-nwl.c | 1053 
> 
>  4 files changed, 1132 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 = <>;
> + 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 _intc 0x1>,
> + <0x0 0x0 0x0 0x2 _intc 0x2>,
> + <0x0 0x0 0x0 0x3 _intc 0x3>,
> + <0x0 0x0 0x0 0x4 _intc 0x4>;
> +
> + msi-parent = <_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 

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

[Marc Zyngier]

On 04/11/15 12:30, Bharat Kumar Gogada wrote:
>>
 Also, you still lack support for MSI-X (which would come for free...).
>>>
>>> We don't support MSI-X in root port mode.
>>
>> I don't believe you. If you support single MSI, you support MSI-X (because
>> that's mostly a property of the endpoint).
> 
> In our architecture specification MSI-X is unsupported for Root Port.

I still can't believe it, but in the end, that's your own problem.

> + .chip = _msi_irq_chip,
> +};
> +#endif
> +
> +
> + /* 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);

 I just read this, and I'm puzzled. Actually, puzzled is an
 understatement. Why on Earth do you need to give RAM to your MSI
>> HW?
 This should be a device, not RAM. By the look of it, this could be
 absolutely anything. Are you sure you have to supply RAM here?

>>> This is required in our hardware, so that bridge identifies incoming MWr as
>> MSI.
>>
>> I'm asking why this has to be RAM. What is the actual requirement?
> 
> We are allocating RAM for MSI (which is expected) and assigning this address 
> to msi->pages
> this is what EP MSI capability will be programmed with.
> For the bridge to detect an incoming MWr from EP as MSI, hardware needs to 
> know MSI address
> so the same address is programmed in the hardware register for comparison 
> purpose.
> It is the actual requirement from hardware.

I understand that you need to provide an address for the bridge to
match. I'm asking why this has to be RAM as opposed to any other device
address. This seems to be a unique requirement (nobody else does that),
and I'd like to understand why there is this requirement.

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 v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> 
> >> Also, you still lack support for MSI-X (which would come for free...).
> >
> > We don't support MSI-X in root port mode.
> 
> I don't believe you. If you support single MSI, you support MSI-X (because
> that's mostly a property of the endpoint).

In our architecture specification MSI-X is unsupported for Root Port.
> 
> >>> + .chip = _msi_irq_chip,
> >>> +};
> >>> +#endif
> >>> +
> >>> +
> >>> + /* 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);
> >>
> >> I just read this, and I'm puzzled. Actually, puzzled is an
> >> understatement. Why on Earth do you need to give RAM to your MSI
> HW?
> >> This should be a device, not RAM. By the look of it, this could be
> >> absolutely anything. Are you sure you have to supply RAM here?
> >>
> > This is required in our hardware, so that bridge identifies incoming MWr as
> MSI.
> 
> I'm asking why this has to be RAM. What is the actual requirement?

We are allocating RAM for MSI (which is expected) and assigning this address to 
msi->pages
this is what EP MSI capability will be programmed with.
For the bridge to detect an incoming MWr from EP as MSI, hardware needs to know 
MSI address
so the same address is programmed in the hardware register for comparison 
purpose.
It is the actual requirement from hardware.

--
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 v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Marc Zyngier]

On 04/11/15 06:38, Bharat Kumar Gogada wrote:
>>> +static struct msi_domain_info nwl_msi_domain_info = {
>>> +   .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
>> MSI_FLAG_USE_DEF_CHIP_OPS |
>>> + MSI_FLAG_MULTI_PCI_MSI),
>>
>> If you're supporting multi-MSI, how do you ensure that all hwirqs are
>> contiguous as required by the spec? Clearly, your allocator doesn't provide
>> this guarantee.
>>
> Oh ok, then how can we know if one EP is requesting for multiple irq's, 
> because in pci_enable_msi_range,
>  it does do while loop according to msi_capability_init return value 
> which in turn requests for multiple irq's. Is there any way to find out when 
> single EP requesting for multiple MSI?

Please read what I've written: hwirqs *must* be contiguous for
multi-MSI. If the device requests 4 MSIs, they *must* be in the [x,x+3]
range. Your allocator only allocates one interrupt at time (ignoring the
nr_irqs parameter).

You shouldn't try to find what the device does, that's irrelevant at
that level.

>> Also, you still lack support for MSI-X (which would come for free...).
> 
> We don't support MSI-X in root port mode.

I don't believe you. If you support single MSI, you support MSI-X
(because that's mostly a property of the endpoint).

>>> +   .chip = _msi_irq_chip,
>>> +};
>>> +#endif
>>> +
>>> +   irq_domain_remove(pcie->legacy_irq_domain);
>>> +
>>> +#ifdef CONFIG_PCI_MSI
>>> +   irq_set_chained_handler_and_data(msi->irq_msi0, NULL, NULL);
>>> +   irq_set_chained_handler_and_data(msi->irq_msi1, NULL, NULL);
>>> +
>>> +   irq_domain_remove(msi->msi_domain);
>>> +   irq_domain_remove(msi->dev_domain);
>>> +#endif
>>> +
>>> +}
>>
>> Remove these #ifdefs. You can test the validity of these fields before 
>> calling
>> the various functions. You can also move this to a separate function.
> 
> Without #ifdefs if we compile driver for legacy, MSI structures will not be 
> available and we get compile time error.

Legacy? Legacy interrupts? The msi structure is still there, you've
embedded it in your pcie structure. Let me spell it out for you:

static void nwl_msi_free_domain(struct nwl_pcie *pcie)
{
struct nwl_msi *msi = >msi;

if (msi->irq_msi0)
irq_set_chained_handler_and_data(msi->irq_msi0, NULL, NULL);
if (msi->irq_msi1)
irq_set_chained_handler_and_data(msi->irq_msi1, NULL, NULL);

if (msi->msi_domain)
irq_domain_remove(msi->msi_domain);
if (msi->dev_domain)
irq_domain_remove(msi->dev_domain);
}

static void nwl_pcie_free_irq_domain(struct nwl_pcie *pcie)
{
int i;
u32 irq;

for (i = 0; i < 4; i++) {
irq = irq_find_mapping(pcie->legacy_irq_domain, i + 1);
if (irq > 0)
irq_dispose_mapping(irq);
}

irq_domain_remove(pcie->legacy_irq_domain);

nwl_msi_free_domain(pcie);
}

>>> +
>>> +   /* 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);
>>
>> I just read this, and I'm puzzled. Actually, puzzled is an understatement. 
>> Why
>> on Earth do you need to give RAM to your MSI HW?
>> This should be a device, not RAM. By the look of it, this could be absolutely
>> anything. Are you sure you have to supply RAM here?
>>
> This is required in our hardware, so that bridge identifies incoming MWr as 
> MSI.

I'm asking why this has to be RAM. What is the actual requirement?

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 v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Marc Zyngier]

On 04/11/15 07:54, Bharat Kumar Gogada wrote:
>>> Without #ifdefs if we compile driver for legacy, MSI structures will not be
>> available and we get compile time error.
>>
>> Sorry for nitpicking but at least can we use elegant version of #ifdefs 
>> .i.e. #if
>> IS_ENABLED() here ?
>>
> Since IS_ENABLED() is checked at runtime, compile time error would come for 
> legacy.

You're seriously misguided. IS_ENABLED() is specifically designed *not*
to be checked at runtime, but instead to use the compiler optimization
phase to get rid of code sections at compile time.

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

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

[Bharat Kumar Gogada]

Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.

Signed-off-by: Bharat Kumar Gogada 
Signed-off-by: Ravi Kiran Gummaluri 
---
Removed msi_controller and added irq_domian for MSI domain hierarchy.
Modified code for filling MSI address in struct msg.
Added check for hwirq before passing it to irq_domain_set_info.
---
 .../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 | 1053 
 4 files changed, 1132 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 = <>;
+   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 _intc 0x1>,
+   <0x0 0x0 0x0 0x2 _intc 0x2>,
+   <0x0 0x0 0x0 0x3 _intc 0x3>,
+   <0x0 0x0 0x0 0x4 _intc 0x4>;
+
+   msi-parent = <_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) += 

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

[Amit Tomer]

> Without #ifdefs if we compile driver for legacy, MSI structures will not be 
> available and we get compile time error.

Sorry for nitpicking but at least can we use elegant version of
#ifdefs .i.e. #if IS_ENABLED() here ?

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 v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> > Without #ifdefs if we compile driver for legacy, MSI structures will not be
> available and we get compile time error.
> 
> Sorry for nitpicking but at least can we use elegant version of #ifdefs .i.e. 
> #if
> IS_ENABLED() here ?
> 
Since IS_ENABLED() is checked at runtime, compile time error would come for 
legacy.

Thanks
Bharat

RE: [PATCH v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> > +static struct msi_domain_info nwl_msi_domain_info = {
> > +   .flags = (MSI_FLAG_USE_DEF_DOM_OPS |
> MSI_FLAG_USE_DEF_CHIP_OPS |
> > + MSI_FLAG_MULTI_PCI_MSI),
> 
> If you're supporting multi-MSI, how do you ensure that all hwirqs are
> contiguous as required by the spec? Clearly, your allocator doesn't provide
> this guarantee.
> 
Oh ok, then how can we know if one EP is requesting for multiple irq's, because 
in pci_enable_msi_range,
 it does do while loop according to msi_capability_init return value 
which in turn requests for multiple irq's. Is there any way to find out when 
single EP requesting for multiple MSI?

> Also, you still lack support for MSI-X (which would come for free...).

We don't support MSI-X in root port mode.
> 
> > +   .chip = _msi_irq_chip,
> > +};
> > +#endif
> > +
> > +   irq_domain_remove(pcie->legacy_irq_domain);
> > +
> > +#ifdef CONFIG_PCI_MSI
> > +   irq_set_chained_handler_and_data(msi->irq_msi0, NULL, NULL);
> > +   irq_set_chained_handler_and_data(msi->irq_msi1, NULL, NULL);
> > +
> > +   irq_domain_remove(msi->msi_domain);
> > +   irq_domain_remove(msi->dev_domain);
> > +#endif
> > +
> > +}
> 
> Remove these #ifdefs. You can test the validity of these fields before calling
> the various functions. You can also move this to a separate function.

Without #ifdefs if we compile driver for legacy, MSI structures will not be 
available and we get compile time error.
> 
> > +
> > +   /* 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);
> 
> I just read this, and I'm puzzled. Actually, puzzled is an understatement. Why
> on Earth do you need to give RAM to your MSI HW?
> This should be a device, not RAM. By the look of it, this could be absolutely
> anything. Are you sure you have to supply RAM here?
> 
This is required in our hardware, so that bridge identifies incoming MWr as MSI.
> > +
> > +   /* Disable high range msi interrupts */
> > +   nwl_bridge_writel(pcie, (u32)~MSGF_MSI_SR_HI_MASK,
> > +MSGF_MSI_MASK_HI);
> > +
> > +

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 v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Marc Zyngier]

On 03/11/15 15:23, 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 
> ---
> Removed msi_controller and added irq_domian for MSI domain hierarchy.
> Modified code for filling MSI address in struct msg.
> Added check for hwirq before passing it to irq_domain_set_info.
> ---
>  .../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 | 1053 
> 
>  4 files changed, 1132 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
>  create mode 100644 drivers/pci/host/pcie-xilinx-nwl.c

[...]

> +#ifdef CONFIG_PCI_MSI
> +static struct irq_chip nwl_msi_irq_chip = {
> + .name = "nwl_pcie:msi",
> + .irq_enable = unmask_msi_irq,
> + .irq_disable = mask_msi_irq,
> + .irq_mask = mask_msi_irq,
> + .irq_unmask = unmask_msi_irq,
> +
> +};
> +
> +static struct msi_domain_info nwl_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +   MSI_FLAG_MULTI_PCI_MSI),

If you're supporting multi-MSI, how do you ensure that all hwirqs are
contiguous as required by the spec? Clearly, your allocator doesn't
provide this guarantee.

Also, you still lack support for MSI-X (which would come for free...).

> + .chip = _msi_irq_chip,
> +};
> +#endif
> +
> +static void nwl_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + struct nwl_pcie *pcie = irq_data_get_irq_chip_data(data);
> + struct nwl_msi *msi = >msi;
> + phys_addr_t msi_addr = virt_to_phys((void *)msi->pages);
> +
> + msg->address_lo = lower_32_bits(msi_addr);
> + msg->address_hi = upper_32_bits(msi_addr);
> + msg->data = data->hwirq;
> +}
> +
> +static int nwl_msi_set_affinity(struct irq_data *irq_data,
> + const struct cpumask *mask, bool force)
> +{
> + return -EINVAL;
> +}
> +
> +static struct irq_chip nwl_irq_chip = {
> + .name = "Xilinx MSI",
> + .irq_compose_msi_msg = nwl_compose_msi_msg,
> + .irq_set_affinity = nwl_msi_set_affinity,
> +};
> +
> +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 = >msi;
> + unsigned long bit;
> +
> + mutex_lock(>lock);
> + bit = find_first_zero_bit(msi->used, INT_PCI_MSI_NR);
> + if (bit < INT_PCI_MSI_NR)
> + set_bit(bit, msi->used);
> + else
> + bit = -ENOSPC;
> +
> + mutex_unlock(>lock);
> +
> + if (bit < 0)
> + return bit;
> +
> + irq_domain_set_info(domain, virq, bit, _irq_chip,
> + domain->host_data, handle_simple_irq,
> + NULL, NULL);
> + return 0;
> +}
> +
> +static void nwl_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs)
> +{
> + struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> + struct nwl_pcie *pcie = irq_data_get_irq_chip_data(data);
> + struct nwl_msi *msi = >msi;
> +
> + mutex_lock(>lock);
> + if (!test_bit(data->hwirq, msi->used))
> + dev_err(pcie->dev, "freeing unused MSI %lu\n", data->hwirq);
> + else
> + clear_bit(data->hwirq, msi->used);
> +
> + mutex_unlock(>lock);
> +}
> +
> +static const struct irq_domain_ops dev_msi_domain_ops = {
> + .alloc  = nwl_irq_domain_alloc,
> + .free   = nwl_irq_domain_free,
> +};
> +
> +static void nwl_pcie_free_irq_domain(struct nwl_pcie *pcie)
> +{
> + int i;
> + u32 irq;
> +
> +#ifdef CONFIG_PCI_MSI
> + struct nwl_msi *msi = >msi;
> +#endif
> +
> + for (i = 0; i < 4; i++) {
> + irq = irq_find_mapping(pcie->legacy_irq_domain, i + 1);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(pcie->legacy_irq_domain);
> +
> +#ifdef CONFIG_PCI_MSI
> + irq_set_chained_handler_and_data(msi->irq_msi0, NULL, NULL);
> + irq_set_chained_handler_and_data(msi->irq_msi1, NULL, NULL);
> +
> + irq_domain_remove(msi->msi_domain);
> + irq_domain_remove(msi->dev_domain);
> +#endif
> +
> +}

Remove these #ifdefs. You can test the validity of these fields before
calling the various functions. You can also move this to a separate
function.

> +
> +static int nwl_pcie_init_irq_domain(struct nwl_pcie *pcie)
> +{
> + struct device_node *node = pcie->dev->of_node;
> + struct device_node *legacy_intc_node;
> +
> +#ifdef CONFIG_PCI_MSI
> + struct nwl_msi *msi = >msi;
> +#endif
> +
> + legacy_intc_node =