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

[Bharat Kumar Gogada]

Hi Bjorn, can you comment on this. Marc has also replied for query on
irq_dispose_mapping().

> > Subject: RE: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for
> > Xilinx NWL PCIe Host Controller
> >
> > > Subject: Re: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for
> > > Xilinx NWL PCIe Host Controller
> > >
> > > [+cc Marc for irq_dispose_mapping() question]
> > >
> > > On Thu, Dec 10, 2015 at 02:10:34PM +, Bharat Kumar Gogada wrote:
> > > I'm trying to figure out what the difference is between these two
> > > checks and why you have both of them:
> > >
> > > > +   if (bus->number == pcie->root_busno && devfn > 0)
> > > > +   if (bus->primary == pcie->root_busno && devfn > 0)
> > >
> > > If I understand correctly, pcie->root_busno is the bus number of the
> > > Root Port device (likely 00).  I think the "bus->number ==
> > > pcie->root_busno && devfn > 0" check means that the Root Port, e.g.,
> > > 00:00.0, is the only device allowed on bus 00.  Often a Root Complex
> > > contains several Root Ports and other integrated devices that
> > > typically are
> > on bus 00.
> > > But in your case, I think you're saying there is only the single
> > > Root Port and no other devices.
> > >
> > > I think that first check takes care of everything on bus 00, so I'm
> > > trying to figure out what the second check is for.  Assume your Root
> > > Port is device
> > > 00:00.0 and it is a bridge to [bus 01-ff].  Then we have two pci_bus
> > > structs with these values:
> > >
> > >   bus->number = 00
> > >   bus->primary = 00
> > >   bus->busn_res = [bus 00-ff]
> > >
> > >   bus->number = 01
> > >   bus->primary = 00
> > >   bus->busn_res = [bus 01-ff]
> > >
> > > Because of the first check, 00:00.0 is the only possible device on
> > > bus 00, and because of the second check, 01:00.0 is the only
> > > possible device on
> > bus 01.
> > > Therefore, you don't support a multifunction device connected to the
> > > Root Port.  Right?
> > >
> > We support multifunction devices also, so this check should not be
> > there, will remove this check in next patch.
> >
> > > > > > +   return false;
> > > > > > +
> > > > > > +   return true;
> > > > > > +}
> > > > > > + * nwl_setup_sspl - Set Slot Power limit
> > > > > > + *
> > > > > > + * @pcie: PCIe port information  */ static int
> > > > > > +nwl_setup_sspl(struct nwl_pcie *pcie)
> > > > >
> > > > The Set_Slot_Power_Limit Message includes a one DW data payload.
> > > > The data payload is copied from the Slot Capabilities register of
> > > > the Downstream Port and is written into the Device Capabilities
> > > > register of the Upstream Port on the other side of the Link. Bits
> > > > 9:8 of the data payload map to the Slot Power Limit Scale field
> > > > and bits 7:0 map to the Slot Power Limit Value field. Bits 31:10
> > > > of the data payload must be set to all 0's by the Transmitter and
> > > > ignored by the
> > Receiver.
> > >
> > > > This Message is sent automatically by the Downstream Port (of a
> > > > Root Complex or a Switch) when one of the following events occurs:
> > > > -> On a Configuration Write to the Slot Capabilities register (see
> > > > Section 7.8.9) when the Data Link Layer reports DL_Up status.
> > >
> > > I interpret this as meaning "the *hardware* automatically sends a
> > > Set_Slot_Power_Limit Message."  There's no mention of software doing
> > > anything other than the configuration write.
> > >
> > > If your hardware doesn't do that, I think it's a defect.  It's fine
> > > to work around it, but we should have a comment to that effect so
> > > people don't copy the code to new drivers that don't need it.
> >
> > Our hardware is not capable of doing it, so we are doing it software.
> > Yes I will add some comments.
> >
> > >
> > > It's a little strange that 7.8.9 talks about writing to this
> > > register when all of its fields are HwInit and supposedly read-only.
> > > I had assumed devices would use strapping or implementation-specific
> > > registers to set the

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

[Bharat Kumar Gogada]

Hi Bjorn, can you comment on this. Marc has also replied for query on 
irq_dispose_mapping().

> Subject: RE: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> 
> > Subject: Re: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for
> > Xilinx NWL PCIe Host Controller
> >
> > [+cc Marc for irq_dispose_mapping() question]
> >
> > On Thu, Dec 10, 2015 at 02:10:34PM +, Bharat Kumar Gogada wrote:
> > I'm trying to figure out what the difference is between these two
> > checks and why you have both of them:
> >
> > > + if (bus->number == pcie->root_busno && devfn > 0)
> > > + if (bus->primary == pcie->root_busno && devfn > 0)
> >
> > If I understand correctly, pcie->root_busno is the bus number of the
> > Root Port device (likely 00).  I think the "bus->number ==
> > pcie->root_busno && devfn > 0" check means that the Root Port, e.g.,
> > 00:00.0, is the only device allowed on bus 00.  Often a Root Complex
> > contains several Root Ports and other integrated devices that typically are
> on bus 00.
> > But in your case, I think you're saying there is only the single Root
> > Port and no other devices.
> >
> > I think that first check takes care of everything on bus 00, so I'm
> > trying to figure out what the second check is for.  Assume your Root
> > Port is device
> > 00:00.0 and it is a bridge to [bus 01-ff].  Then we have two pci_bus
> > structs with these values:
> >
> >   bus->number = 00
> >   bus->primary = 00
> >   bus->busn_res = [bus 00-ff]
> >
> >   bus->number = 01
> >   bus->primary = 00
> >   bus->busn_res = [bus 01-ff]
> >
> > Because of the first check, 00:00.0 is the only possible device on bus
> > 00, and because of the second check, 01:00.0 is the only possible device on
> bus 01.
> > Therefore, you don't support a multifunction device connected to the
> > Root Port.  Right?
> >
> We support multifunction devices also, so this check should not be there, will
> remove this check in next patch.
> 
> > > > > + return false;
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > + * nwl_setup_sspl - Set Slot Power limit
> > > > > + *
> > > > > + * @pcie: PCIe port information  */ static int
> > > > > +nwl_setup_sspl(struct nwl_pcie *pcie)
> > > >
> > > The Set_Slot_Power_Limit Message includes a one DW data payload. The
> > > data payload is copied from the Slot Capabilities register of the
> > > Downstream Port and is written into the Device Capabilities register
> > > of the Upstream Port on the other side of the Link. Bits 9:8 of the
> > > data payload map to the Slot Power Limit Scale field and bits 7:0
> > > map to the Slot Power Limit Value field. Bits 31:10 of the data
> > > payload must be set to all 0's by the Transmitter and ignored by the
> Receiver.
> >
> > > This Message is sent automatically by the Downstream Port (of a Root
> > > Complex or a Switch) when one of the following events occurs:
> > > -> On a Configuration Write to the Slot Capabilities register (see
> > > Section 7.8.9) when the Data Link Layer reports DL_Up status.
> >
> > I interpret this as meaning "the *hardware* automatically sends a
> > Set_Slot_Power_Limit Message."  There's no mention of software doing
> > anything other than the configuration write.
> >
> > If your hardware doesn't do that, I think it's a defect.  It's fine to
> > work around it, but we should have a comment to that effect so people
> > don't copy the code to new drivers that don't need it.
> 
> Our hardware is not capable of doing it, so we are doing it software. Yes I 
> will
> add some comments.
> 
> >
> > It's a little strange that 7.8.9 talks about writing to this register
> > when all of its fields are HwInit and supposedly read-only.  I had
> > assumed devices would use strapping or implementation-specific
> > registers to set the Slot Power values, but maybe some devices use direct
> writes to Slot Capabilities instead.
> >
> > BTW, I noticed a related lspci bug: it didn't decode the Capture Slot
> > Power Limit in Device Capabilities of Endpoints.  I posted a fix for that
> separately.
> >
> > The Slot Power Limit (in Slot Capabilities) indicates how much power
> > the slot can supply to a downstream device.  That's a function of

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

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> > +
> It's a bit sloppy here to return 0, 1, or -EINVAL from a function declared to
> return "bool".  A bool function should return "true" or "false".
> 
> I think the best thing is to split this into two functions:
> 
>   static bool nwl_pcie_link_up(struct nwl_pcie *pcie)
>   {
> if (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> PCIE_PHY_LINKUP_BIT)
>   return true;
> return false;
>   }
> 
>   static bool nwl_phy_link_up(struct nwl_pcie *pcie)
>   {
> if (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> PHY_RDY_LINKUP_BIT)
>   return true;
> return false;
>   }
> 
> Then there's no possibility of a caller passing an invalid "check_link_up"
> argument, so you don't have to worry about an -EINVAL return.

Yes, agreed will address it in next patch
> 
> > +}
> > +
> > +static bool nwl_pcie_valid_device(struct pci_bus *bus, unsigned int
> > +devfn) {
> > +   struct nwl_pcie *pcie = bus->sysdata;
> > +
> > +   /* Check link,before accessing downstream ports */
> > +   if (bus->number != pcie->root_busno) {
> > +   if (!nwl_pcie_link_up(pcie, PCIE_USER_LINKUP))
> > +   return false;
> > +   }
> > +
> > +   /* Only one device down on each root port */
> > +   if (bus->number == pcie->root_busno && devfn > 0)
> > +   return false;
> > +
> > +   /*
> > +* Do not read more than one device on the bus directly attached
> > +* to root port.
> > +*/
> > +   if (bus->primary == pcie->root_busno && devfn > 0)
> 
> Why is this?  Do you have some restriction on the PCIe topology below the
> Root Port?  If there's a spec-compliant link coming from the Root Port, it
> seems like any legal PCIe device could be attached, including a multifunction
> device.
> 
Primary refers to root port itself, this check is not for devices downstream on 
link. 

> > +   return false;
> > +
> > +   return true;
> > +}
> > + * nwl_setup_sspl - Set Slot Power limit
> > + *
> > + * @pcie: PCIe port information
> > + */
> > +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> 
> This function needs a little explanation because it doesn't really make any
> sense when compared with the PCIe spec.  The spec says the Slot Power
> Limit Value is HwInit, which means it's supposed to be initialized by firmware
> or hardware, and it should be read-only by the time we get here.
> I think the value there should reflect parameters of the hardware design; we
> wouldn't want a user to write an arbitrary limit that would tell a card it can
> pull more power than the slot can supply.
> 
> But I saw another driver recently (pcie-snpsdev.c) that's doing something
> similar, so there must be something else going on here.  In any case, a
> comment here about exactly *what* is going on would be much appreciated.

The spec says the following:
The Set_Slot_Power_Limit Message includes a one DW data payload. The data 
payload is copied
from the Slot Capabilities register of the Downstream Port and is written into 
the Device
Capabilities register of the Upstream Port on the other side of the Link. Bits 
9:8 of the data payload
map to the Slot Power Limit Scale field and bits 7:0 map to the Slot Power 
Limit Value field. Bits
31:10 of the data payload must be set to all 0's by the Transmitter and ignored 
by the Receiver. This
Message is sent automatically by the Downstream Port (of a Root Complex or a 
Switch) when one
of the following events occurs:
-> On a Configuration Write to the Slot Capabilities register (see Section 
7.8.9) when the Data Link
Layer reports DL_Up status.
->  Anytime when a Link transitions from a non-DL_Up status to a DL_Up status 
(see
Section 2.9.2).

Captured Slot Power Limit Value (Upstream Ports only) - In
combination with the Slot Power Limit Scale value, specifies the
upper limit on power supplied by slot.
Power limit (in Watts) calculated by multiplying the value in this
field by the value in the Slot Power Limit Scale field.
This value is set by the Set_Slot_Power_Limit Message or
hardwired to  b (see Section 6.9). The default value is
 b.> 

"With Set_Slot_Power_Limit Message also this value is set"

> > +{
> > +   unsigned int status;
> > +   mdelay(2);
> 
> Arnd suggested a sleep instead of delay here, but I didn't see a response.
> See http://lkml.kernel.org/r/2677327.lnRZhqx5GI@wuerfel
Agreed, will address in next patch.
> 
> > +   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 

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

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> 
> [+cc Marc for irq_dispose_mapping() question]
> 
> On Thu, Dec 10, 2015 at 02:10:34PM +0000, Bharat Kumar Gogada wrote:
> I'm trying to figure out what the difference is between these two checks and
> why you have both of them:
> 
> > +   if (bus->number == pcie->root_busno && devfn > 0)
> > +   if (bus->primary == pcie->root_busno && devfn > 0)
> 
> If I understand correctly, pcie->root_busno is the bus number of the Root
> Port device (likely 00).  I think the "bus->number ==
> pcie->root_busno && devfn > 0" check means that the Root Port, e.g.,
> 00:00.0, is the only device allowed on bus 00.  Often a Root Complex contains
> several Root Ports and other integrated devices that typically are on bus 00.
> But in your case, I think you're saying there is only the single Root Port 
> and no
> other devices.
> 
> I think that first check takes care of everything on bus 00, so I'm trying to
> figure out what the second check is for.  Assume your Root Port is device
> 00:00.0 and it is a bridge to [bus 01-ff].  Then we have two pci_bus structs
> with these values:
> 
>   bus->number = 00
>   bus->primary = 00
>   bus->busn_res = [bus 00-ff]
> 
>   bus->number = 01
>   bus->primary = 00
>   bus->busn_res = [bus 01-ff]
> 
> Because of the first check, 00:00.0 is the only possible device on bus 00, and
> because of the second check, 01:00.0 is the only possible device on bus 01.
> Therefore, you don't support a multifunction device connected to the Root
> Port.  Right?
> 
We support multifunction devices also, so this check should not be there, will 
remove this check in next patch.

> > > > +   return false;
> > > > +
> > > > +   return true;
> > > > +}
> > > > + * nwl_setup_sspl - Set Slot Power limit
> > > > + *
> > > > + * @pcie: PCIe port information
> > > > + */
> > > > +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> > > 
> > The Set_Slot_Power_Limit Message includes a one DW data payload. The
> > data payload is copied from the Slot Capabilities register of the
> > Downstream Port and is written into the Device Capabilities register
> > of the Upstream Port on the other side of the Link. Bits 9:8 of the
> > data payload map to the Slot Power Limit Scale field and bits 7:0 map
> > to the Slot Power Limit Value field. Bits 31:10 of the data payload
> > must be set to all 0's by the Transmitter and ignored by the Receiver.
> 
> > This Message is sent automatically by the Downstream Port (of a Root
> > Complex or a Switch) when one of the following events occurs:
> > -> On a Configuration Write to the Slot Capabilities register (see
> > Section 7.8.9) when the Data Link Layer reports DL_Up status.
> 
> I interpret this as meaning "the *hardware* automatically sends a
> Set_Slot_Power_Limit Message."  There's no mention of software doing
> anything other than the configuration write.
> 
> If your hardware doesn't do that, I think it's a defect.  It's fine to work 
> around
> it, but we should have a comment to that effect so people don't copy the
> code to new drivers that don't need it.

Our hardware is not capable of doing it, so we are doing it software. Yes I 
will add some comments.

> 
> It's a little strange that 7.8.9 talks about writing to this register when 
> all of its
> fields are HwInit and supposedly read-only.  I had assumed devices would
> use strapping or implementation-specific registers to set the Slot Power
> values, but maybe some devices use direct writes to Slot Capabilities instead.
> 
> BTW, I noticed a related lspci bug: it didn't decode the Capture Slot Power
> Limit in Device Capabilities of Endpoints.  I posted a fix for that 
> separately.
> 
> The Slot Power Limit (in Slot Capabilities) indicates how much power the slot
> can supply to a downstream device.  That's a function of the platform design,
> so it seems like this is something you want to set via DT or some other
> mechanism that knows about the platform.
> Intercepting all config writes and updating it with whatever the caller 
> supplies
> doesn't sound wise.  The value might be coming from setpci or some other
> source with no knowledge of the platform.

Agreed, but this is what can be done, it is difficult to determine who does 
what. 
> 
> > > > +   status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> > > > + & MSG_DO

[PATCH v11] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
Acked-by: Rob Herring <r...@kernel.org>
---
Changes for v11:
-> Changed data types of bridge, pcie controller and ecam base address
-> Added programming of E_BREG_BASE_HI, E_ECAM_BASE_HI registers
-> Removed MSI_ADDRESS macro, and using value from device tree
-> Removed unneccessary type casting in nwl_pcie_bridge_init function.
-> Removed mdelay and using msleep in nwl_pcie_bridge_init function.
---
 .../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 | 1072 
 4 files changed, 1151 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' he

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

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> 
> On Friday 27 November 2015 20:32:03 Bharat Kumar Gogada wrote:
> > +   do {
> > +   err = nwl_pcie_link_up(pcie, PHY_RDY_LINKUP);
> > +   if (err != 1) {
> > +   check_link_up++;
> > +   if (check_link_up > LINKUP_ITER_CHECK)
> > +   return -ENODEV;
> > +   mdelay(1000);
> > +   }
> > +   } while (!err);
> 
> mdelay(1000) is not something anyone should do. Why can't you call a
> sleeping function here?
> 
Agreed will use sleep function, will address it in next patch.

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

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v10] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> 
> On 27/11/15 15:02, Bharat Kumar Gogada wrote:
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >
> > Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> > Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> > Acked-by: Rob Herring <r...@kernel.org>
> > ---
> > Changes for v10:
> > -> Changed MSI address to PCIe controller base.
> > -> Removed nwl_check_hwirq function, instead using
> > -> bitmap_find_next_zero_area
> >API to do the same.
> > ---
> >  .../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 | 1068 
> > 
> >  4 files changed, 1147 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/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > new file mode 100644
> > index 000..d070344
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> 
> [...]
> 
> > +#define MSI_ADDRESS0xFD48
> > +
> 
> Really? Why do you bother having DT support then? You might as well
> hardcode everything, while you're at it.
> 
> /me felling depressed now.
Agreed, I'm already having this parameter in nwl_pcie structure, will use it. 
> 
> > +struct nwl_msi {   /* MSI information */
> > +   struct irq_domain *msi_domain;
> > +   unsigned long *bitmap;
> > +   struct irq_domain *dev_domain;
> > +   struct mutex lock;  /* protect bitmap variable */
> > +   int irq_msi0;
> > +   int irq_msi1;
> > +};
> > +
> > +struct nwl_pcie {
> > +   struct device *dev;
> > +   void __iomem *breg_base;
> > +   void __iomem *pcireg_base;
> > +   void __iomem *ecam_base;
> > +   u32 phys_breg_base; /* Physical Bridge Register Base */
> > +   u32 phys_pcie_reg_base; /* Physical PCIe Controller
> Base */
> > +   u32 phys_ecam_base; /* Physical Configuration Base */
> 
> All these u32 should be phys_addr_t. Not all the world is 32bit, fortunately.
Agreed will address it next patch.
> 
> > +   u32 breg_size;
> [...]
> 
> 
> > +static int nwl_pcie_bridge_init(struct nwl_pcie *pcie) {
> > +   struct platform_device *pdev = to_platform_device(pcie->dev);
> > +   u32 breg_val, ecam_val, first_busno = 0;
> > +   int err;
> > +   int check_link_up = 0;
> > +
> > +   /* Check for BREG present bit */
> > +   breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) &
> BREG_PRESENT;
> > +   if (!breg_val) {
> > +   dev_err(pcie->dev, "BREG is not present\n");
> > +   return breg_val;
> > +   }
> > +   /* Write bridge_off to breg base */
> > +   nwl_bridge_writel(pcie, (u32)(pcie->phys_breg_base),
> > + E_BREG_BASE_LO);
> > +
> 
> I love the casting of a u32 to a u32. You have to program E_BREG_BASE_HI as
> well, once you've fixed the data type.
> Agreed, will address this in next patch.
> > +   /* Enable BREG */
> > +   nwl_bridge_writel(pcie, ~BREG_ENABLE_FORCE & BREG_ENABLE,
> > + E_BREG_CONTROL);
> > +   /* Check for ECAM present bit */
> > +   ecam_val = nwl_bridge_readl(pcie, E_ECAM_CAPABILITIES) &
> E_ECAM_PRESENT;
> > +   if (!ecam_val) {
> > +   dev_err(pcie->dev, "ECAM is not present\n");
> > +   return ecam_val;
> > +   }
> |
> > +   /* Write phy_reg_base to ecam base */
> > +   nwl_bridge_writel(pcie, (u32)pcie->phys_ecam_base,
> E_ECAM_BASE_LO);
> 
> Same here.
> Agreed, will address this in next patch.

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

[PATCH v10] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
Acked-by: Rob Herring <r...@kernel.org>
---
Changes for v10:
-> Changed MSI address to PCIe controller base.
-> Removed nwl_check_hwirq function, instead using bitmap_find_next_zero_area
   API to do the same.
---
 .../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 | 1068 
 4 files changed, 1147 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

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

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v9] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL
> PCIe Host Controller
> 
> On Wed, 25 Nov 2015 05:40:49 +0000
> Bharat Kumar Gogada <bharat.kumar.gog...@xilinx.com> wrote:
> 
> > > On Thu, 19 Nov 2015 11:05:23 +0530
> > > Bharat Kumar Gogada <bharat.kumar.gog...@xilinx.com> wrote:
> > >
> > > > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> > > >
> > > > Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> > > > Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> > > > Acked-by: Rob Herring <r...@kernel.org>
> > > > ---
> > > > +
> > > > +#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

[Bharat Kumar Gogada]

> On Thu, 19 Nov 2015 11:05:23 +0530
> Bharat Kumar Gogada <bharat.kumar.gog...@xilinx.com> wrote:
> 
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >
> > Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> > Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> > Acked-by: Rob Herring <r...@kernel.org>
> > ---
> > +
> > +#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 = >msi;
> > +   int bit;
> > +   int i;
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   if (nr_irqs > 1) {
> > +   ret = nwl_check_hwirq(msi, nr_irqs);
> > +   if (ret < 0) {
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +   }
> > +   } else {
> > +   ret = find_first_zero_bit(msi->used, INT_PCI_MSI_NR);
> > +   if (ret == INT_PCI_MSI_NR) {
> > +   mutex_unlock(>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, _irq_chip,
> > +   domain->host_data, handle_simple_irq,
> > +   NULL, NULL);
> > +   }
> > +   mutex_unlock(>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

[PATCH v9] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
Acked-by: Rob Herring <r...@kernel.org>
---
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 = <>;
+   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
+s

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

[Bharat Kumar Gogada]

> 
> On Tue, 17 Nov 2015 04:59:39 +0000
> Bharat Kumar Gogada <bharat.kumar.gog...@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 <bhara...@xilinx.com>
> > > >> Signed-off-by: Ravi Kiran Gummaluri <rgum...@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 = >msi;
> > > >> +  unsigned long base;
> > > >> +  int ret;
> > > >> +
> > > >> +  mutex_init(>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 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 v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> 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 +
> >>> Bharat Kumar Gogada <bharat.kumar.gog...@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 <bhara...@xilinx.com>
> >>>>>>> Signed-off-by: Ravi Kiran Gummaluri <rgum...@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 = >msi;
> >>>>>>> + unsigned long base;
> >>>>>>> + int ret;
> >>>>>>> +
> >>>>>>> + mutex_init(>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. 0x0deadbeef000 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 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 v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

 > On Wed, Nov 11, 2015 at 12:03:39PM +0530, Bharat Kumar Gogada wrote:
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >
> > Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> > Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> 
> I acked v7. Please add acks when sending a new version.
Hi Rob, 
I'm sorry for not adding I forgot to do so, I will definitely add this in next 
patch.

Regards,
Bharat Kumar Gogada
> 
> Acked-by: Rob Herring <r...@kernel.org>
--
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 v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> 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 <bhara...@xilinx.com>
> >> Signed-off-by: Ravi Kiran Gummaluri <rgum...@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 = >msi;
> >> +  unsigned long base;
> >> +  int ret;
> >> +
> >> +  mutex_init(>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 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 v8] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

 
> On 11/10/2015 10:33 PM, Bharat Kumar Gogada wrote:
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >
> > Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> > Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> > ---
> > Added logic to allocate contiguous hwirq in nwl_irq_domain_alloc function.
> > Moved MSI functionality to separate functions.
> > Changed error return values.
> > +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;
> > +   int i;
> > +
> > +   mutex_lock(>lock);
> > +   for (i = 0; i < nr_irqs; i++) {
> > +   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;
> 
> Don't you get a compiler warning with this? Note bit is unsigned long and you
> assign -ENOSPC to it.
> 
Ya that's a mistake, I will address these in next patch.
> > +
> > +   if (bit < 0) {
> 
> how can bit < 0 if it's unsigned long?
> 
> > +   mutex_unlock(>lock);
> > +   return bit;
> > +   }
> > +
> > +   irq_domain_set_info(domain, virq, bit, _irq_chip,
> > +   domain->host_data, handle_simple_irq,
> > +   NULL, NULL);
> 
> nit, but if you do irq_domain_set_info(domain, virq + i, ...
> 
> then you can get rid of the code below.
> 
> > +   virq = virq + 1;
> > +   }
> > +   mutex_unlock(>lock);
> > +   return 0;
> > +}
> > +

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

[Bharat Kumar Gogada]

> On Wed, 11 Nov 2015 12:03:39 +0530
> Bharat Kumar Gogada <bharat.kumar.gog...@xilinx.com> wrote:
> 
> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >
> > Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> > Signed-off-by: Ravi Kiran Gummaluri <rgum...@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
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > new file mode 100644
> > index 000..8bc509c
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> 
> [...]
> 
> > +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),
> 
> Given that you claim to support multi-MSI...
> 
> [...]
> 
> > +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;
> > +   int i;
> > +
> > +   mutex_lock(>lock);
> > +   for (i = 0; i < nr_irqs; i++) {
> > +   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;
> > +
> > +   if (bit < 0) {
> > +   mutex_unlock(>lock);
> > +   return bit;
> > +   }
> > +
> > +   irq_domain_set_info(domain, virq, bit, _irq_chip,
> > +   domain->host_data, handle_simple_irq,
> > +   NULL, NULL);
> > +   virq = virq + 1;
> > +   }
> 
> I really don't see how this allocator guarantees that all hwirqs are 
> contiguous.
> I already mentioned this when reviewing v7, and you still haven't got it 
> right.
> So either you allocate *contiguous* hwirqs in an atomic fashion, or you drop
> support for multi-MSI.
> 
Yes, I understood now there is no check for allocating contiguous hwirq, I will 
address this in the next patch.

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

[PATCH v8] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@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

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..39799cf 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -15,12 +15,22 @@ 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
+boo

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

[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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
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

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

[PATCH v6] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
Changes for v6:
Removed repetitive code for msi handlers.
Corrected typo mistakes in device tree documentation.
---
 .../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 | 1025 
 4 files changed, 1103 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 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/dri

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

[Bharat Kumar Gogada]

> 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, , 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,
> > +
>   _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,
> > +   _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,
> > +
>   _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 = >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 v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> > +   device_type = "pci";
> > +   interrupt-parent = <>;
> > +   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 _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>;
> 
> 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

[PATCH v5] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
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 = <>;
+   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 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/

[PATCH v4] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
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 = <>;
+   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 _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 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
   

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

[Bharat Kumar Gogada]

> On 09/10/15 06:11, Bharat Kumar Gogada wrote:
> >>>> +struct nwl_msi {  /* struct nwl_msi - MSI information
> >> */
> >>>> +  struct msi_controller chip; /* chip: MSI controller */
> >>>
> >>>> We're moving away from msi_controller altogether, as the kernel now
> >>>> has all the necessary infrastructure to do this properly.
> >>>
> >>> Our current GIC version does not have separate msi controller (we
> >>> are not using GICv2m or GICv3), so is it necessary to have separate
> >>> msi controller node ? Please give me clarity on this.
> >>
> >> This has nothing to do with the version of the GIC you are using
> >> (XGene doesn't have GICv2m or v3 either). This is about reducing code
> >> duplication and having something that we can maintain. See also
> >> https://lkml.org/lkml/2015/9/20/193 for yet another example.
> >>
> >> I still plan to kill msi_controller, and I'd like to avoid more
> >> dependencies with it. MSI domains are the way to do it.
> >>
> > Sorry previously I haven't configured my email client properly so resending.
> 
> Thanks for doing so, much appreciated.
> 
> > Since we don't have separate MSI controller, and our PCIe controller
> > is handling MSI, is it necessary to create a separate MSI controller
> > node because we don't have any 'reg' space.
> 
> No, your PCI controller can perfectly be part of the PCIe node.
You meant 'msi-controller' property to be part of PCIe node?

> 
> > Please let me know whether we require a separate msi file as suggested
> > in your previous comments to separate MSI controller and PCIE
> > controller in two files, if we don't have separate node. If we do not
> > need a separate node do we need to embed MSI controller child node  in
> > PCIe controller node itself, and what properties does this child node
> > will require other than 'interrupts'.
> 
> If you want to keep them in the same file, please at least have two separate
> patches. These are two different functions, and they should be reviewed
> separately.
> 
What I meant is if we don't have separate msi node do we need separate file? 
If you meant msi controller to be part of same node then we will use single 
file and will
try to have two separate patches.

Thanks,
Bharat

> It will help everyone to understand your code, and speed up the reviewing
> process.
> 
> 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 v3] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> Hi Bharat,
>> > Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> >
> > +/* SSPL ERROR */
> > +#define SLVERR 0x02
> > +#define DECERR 0x03
> > +
> > +struct nwl_msi {   /* struct nwl_msi - MSI information
> */
> > +   struct msi_controller chip; /* chip: MSI controller */
> 
> We're moving away from msi_controller altogether, as the kernel now has all
> the necessary infrastructure to do this properly.
> 
> Please convert this driver to msi domains (see drivers/pci/host/pci-xgene-
> msi.c or the gic-v2m driver as examples of how this is being done).

As suggested I have gone through the code in pci-gene-msi.c for msi domains, 
and have gone through IRQ Domain documentation. 
I mainly observe when we have more than one interrupt controller involved
between device and cpu we use "Hierarchy IRQ domain" (parent and child msi 
domains). But in our case we 
don't have any such Hierarchy and we have single interrupt controller.
In such case is it necessary to use multiple domains in software which actually 
isn't case in hardware.

Thanks 
Bharat
> 
> > +   DECLARE_BITMAP(used, INT_PCI_MSI_NR);   /* used: Declare
> Bitmap
> > +   for MSI */
> > +   struct irq_domain *domain;  /* domain: IRQ domain pointer */
> > +   unsigned long pages;/* pages: MSI pages */
> 
> Overall, I think it would make sense to split the PCIe controller driver and 
> the
> MSI controller (in separate files as well).
> 
> Please keep me posted on the updates.
> 
> 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 v3] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

On 06/10/15 17:27, Bharat Kumar Gogada wrote:
> Subject: Re: [PATCH v3] PCI: Xilinx-NWL-PCIe: Added support for Xilinx 
> NWL PCIe Host Controller

[...]

>> +struct nwl_msi {/* struct nwl_msi - MSI information */
>> +struct msi_controller chip; /* chip: MSI controller */
> 
>> We're moving away from msi_controller altogether, as the kernel now 
>> has all the necessary infrastructure to do this properly.
> 
> Our current GIC version does not have separate msi controller (we are 
> not using GICv2m or GICv3), so is it necessary to have separate msi 
> controller node ? Please give me clarity on this.

This has nothing to do with the version of the GIC you are using (XGene doesn't 
have GICv2m or v3 either). This is about reducing code duplication and having 
something that we can maintain. See also
https://lkml.org/lkml/2015/9/20/193 for yet another example.

I still plan to kill msi_controller, and I'd like to avoid more dependencies 
with it. MSI domains are the way to do it.

Since we don't have separate MSI controller, and our PCIe controller is 
handling MSI, is it necessary to create a separate MSI controller node because 
we don't have any 'reg' space. 
Please let me know whether we require a separate msi file as suggested in your 
previous comments to separate MSI controller and PCIE controller in two files, 
if we don't have separate node. 
If we do not need a separate node do we need to embed MSI controller child node 
 in PCIe controller node itself, and what properties does this child node will 
require other than 'interrupts'.

Bharat

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

[Bharat Kumar Gogada]

> >> +struct nwl_msi {  /* struct nwl_msi - MSI information
> */
> >> +  struct msi_controller chip; /* chip: MSI controller */
> >
> >> We're moving away from msi_controller altogether, as the kernel now
> >> has all the necessary infrastructure to do this properly.
> >
> > Our current GIC version does not have separate msi controller (we are
> > not using GICv2m or GICv3), so is it necessary to have separate msi
> > controller node ? Please give me clarity on this.
>
> This has nothing to do with the version of the GIC you are using (XGene
> doesn't have GICv2m or v3 either). This is about reducing code duplication
> and having something that we can maintain. See also
> https://lkml.org/lkml/2015/9/20/193 for yet another example.
>
> I still plan to kill msi_controller, and I'd like to avoid more dependencies 
> with
> it. MSI domains are the way to do it.
>
Sorry previously I haven't configured my email client properly so resending.

Since we don't have separate MSI controller, and our PCIe controller is 
handling MSI, is it necessary to create a separate MSI controller node because 
we don't have any 'reg' space.
Please let me know whether we require a separate msi file as suggested in your 
previous comments to separate MSI controller and PCIE controller in two files, 
if we don't have separate node.
If we do not need a separate node do we need to embed MSI controller child node 
 in PCIe controller node itself, and what properties does this child node will 
require other than 'interrupts'.

Bharat
> Thanks,
>
>   M.
> --
> Jazz is not dead. It just smells funny...


This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

--
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 v3] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
Added interrupt-map, interrupt-map-mask properties
---
 .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   56 ++
 drivers/pci/host/Kconfig   |9 +
 drivers/pci/host/Makefile  |1 +
 drivers/pci/host/pcie-xilinx-nwl.c | 1029 
 4 files changed, 1095 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..81006ab
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,56 @@
+* 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
+
+Optional properties:
+- xlnx,msi-fifo: To enable MSI FIFO mode
+   - This feature can be used to queue multiple MSI interrupts
+
+Example:
+
+nwl_pcie: pcie@fd0e {
+   compatible = "xlnx,nwl-pcie-2.11";
+   #address-cells = <3>;
+   #size-cells = <2>;
+   #interrupt-cells = <1>;
+   device_type = "pci";
+   interrupt-parent = <>;
+   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  0x0 116 0x4
+   0x0 0x0 0x0 0x2  0x0 116 0x4
+   0x0 0x0 0x0 0x3  0x0 116 0x4
+   0x0 0x0 0x0 0x4  0x0 116 0x4>;
+   reg = <0x0 0xfd0e 0x1000
+  0x0 0xfd48 0x1000
+  0x0 0xE000 0x100>;
+   reg-names = "breg", "pcireg", "cfg";
+   ranges = <0x0200 0x 0xE100 0x 0xE100 0 
0x0F00>;
+};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c132bdd..7f5f35e 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 && PCI_MSI"
+   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_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
diff --git a/drivers/pci/host/pcie-xilinx-nwl.c 
b/drivers/pci/host/pcie-xilinx-nwl.c
new file mode 1

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

[Bharat Kumar Gogada]

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

Hi Bharat,

On 06/10/15 16:44, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> ---
> Added interrupt-map, interrupt-map-mask properties
> ---
>  .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   56 ++
>  drivers/pci/host/Kconfig   |9 +
>  drivers/pci/host/Makefile  |1 +
>  drivers/pci/host/pcie-xilinx-nwl.c | 1029 
> 
>  4 files changed, 1095 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..81006ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
> @@ -0,0 +1,56 @@
> +* 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
> +
> +Optional properties:
> +- xlnx,msi-fifo: To enable MSI FIFO mode
> + - This feature can be used to queue multiple MSI interrupts
> +
> +Example:
> +
> +nwl_pcie: pcie@fd0e {
> + compatible = "xlnx,nwl-pcie-2.11";
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <1>;
> + device_type = "pci";
> + interrupt-parent = <>;
> + 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  0x0 116 0x4
> + 0x0 0x0 0x0 0x2  0x0 116 0x4
> + 0x0 0x0 0x0 0x3  0x0 116 0x4
> + 0x0 0x0 0x0 0x4  0x0 116 0x4>;
> + reg = <0x0 0xfd0e 0x1000
> +0x0 0xfd48 0x1000
> +0x0 0xE000 0x100>;
> + reg-names = "breg", "pcireg", "cfg";
> + ranges = <0x0200 0x 0xE100 0x 0xE100 0 
> +0x0F00>; };

Please move this to a separate patch. This is big enough, and hard enough to 
review.

> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index 
> c132bdd..7f5f35e 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 && PCI_MSI"
> + 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

Add a space after the dot.

> +  support root port enabling.
> +
>  config PCIE_DW
>   bool
&g

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

[Bharat Kumar Gogada]

On Thursday 01 October 2015 14:29:21 Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada <bhara...@xilinx.com>
> Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
> ---
> Removed unneccessary comments
> Modified setup_sspl implementation
> Added more details in binding Documentation
> ---
>  .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   49 +
>  drivers/pci/host/Kconfig   |9 +
>  drivers/pci/host/Makefile  |1 +
>  drivers/pci/host/pcie-xilinx-nwl.c | 1029 
> 
>  4 files changed, 1088 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..ed87184
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
> @@ -0,0 +1,49 @@
> +* 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

The msi and intx interrupts don't really belong here: For intx, please use an 
interrupt-map property as the other host drivers do.

For MSI, the usual answer is that there should be a separate device node for 
the MSI controller, and an msi-parent property in the PCI host.
Our current GIC version does not have separate msi controller, so is it 
necessary to have separate msi node ?

This lets you use the same code for hosts that have a GICv2m or GICv3 that 
comes with its own MSI controller.

> +/**
> + * 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
> + * @enable_msi_fifo: Enable MSI FIFO mode
> + * @bus: PCI bus
> + * @msi: MSI interrupt info
> + */
> +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;

These should probably be phys_addr_t.

> + * 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,
> +

[PATCH v2] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
Removed unneccessary comments
Modified setup_sspl implementation
Added more details in binding Documentation
---
 .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   49 +
 drivers/pci/host/Kconfig   |9 +
 drivers/pci/host/Makefile  |1 +
 drivers/pci/host/pcie-xilinx-nwl.c | 1029 
 4 files changed, 1088 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..ed87184
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,49 @@
+* 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
+- 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
+
+Optional properties:
+- xlnx,msi-fifo: To enable MSI FIFO mode
+   - This feature can be used to queue multiple MSI interrupts
+
+Example:
+
+nwl_pcie: pcie@fd0e {
+   compatible = "xlnx,nwl-pcie-2.11";
+   #address-cells = <3>;
+   #size-cells = <2>;
+   #interrupt-cells = <1>;
+   device_type = "pci";
+   interrupt-parent = <>;
+   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";
+   reg = <0x0 0xfd0e 0x1000
+  0x0 0xfd48 0x1000
+  0x0 0xE000 0x100>;
+   reg-names = "breg", "pcireg", "cfg";
+   ranges = <0x0200 0x 0xE100 0x 0xE100 0 
0x0F00>;
+};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c132bdd..e0bf896 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 && 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) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCIE_XILINX) += pcie-xilinx.o
+obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
diff --git a/drivers/pci/host/pcie-xilinx-nwl.c 
b/drivers/pci/host/pcie-xilinx-nwl.c
new file mode 100644
index 000..ff25ec1
--- /dev/null
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -0,0 +1,1029 @@
+/*
+ * PCIe host controller driver for NWL PCIe Bridge
+ * Based on pci-xilinx.c, pci-tegra.c
+ *
+ * (C) Copyright 2014 - 2015, Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Pu

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

[Bharat Kumar Gogada]

Ping!

-Original Message-
From: Bharat Kumar Gogada [mailto:bharat.kumar.gog...@xilinx.com]
Sent: Thursday, August 27, 2015 5:14 PM
To: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com; 
ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek; Soren 
Brinkmann; bhelg...@google.com; a...@arndb.de; tinam...@apm.com; 
tred...@nvidia.com; r...@broadcom.com; minghuan.l...@freescale.com; 
m-kariche...@ti.com; ha...@hauke-m.de
Cc: devicetree@vger.kernel.org; linux-arm-ker...@lists.infradead.org; 
linux-ker...@vger.kernel.org; linux-...@vger.kernel.org; Bharat Kumar Gogada; 
Ravikiran Gummaluri
Subject: [PATCH] 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 <bhara...@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgum...@xilinx.com>
---
 .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   39 +
 drivers/pci/host/Kconfig   |9 +
 drivers/pci/host/Makefile  |1 +
 drivers/pci/host/pci-xilinx-nwl.c  | 1038 
 4 files changed, 1087 insertions(+), 0 deletions(-)  create mode 100644 
Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
 create mode 100644 drivers/pci/host/pci-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..c554d6b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,39 @@
+* 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 and
+length
+- device_type: must be "pci"
+- interrupts: Should contain NWL PCIe interrupt
+- 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
+
+Optional properties:
+- xlnx,msi-fifo: To enable MSI FIFO mode
+
+Example:
+
+nwl_pcie: pcie@fd0e {
+   compatible = "xlnx,nwl-pcie-2.11";
+   #address-cells = <3>;
+   #size-cells = <2>;
+   #interrupt-cells = <1>;
+   device_type = "pci";
+   interrupt-parent = <>;
+   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";
+   reg = <0x0 0xfd0e 0x1000
+  0x0 0xfd48 0x1000
+  0x0 0xE000 0x100>;
+   reg-names = "breg", "pcireg", "cfg";
+   ranges = <0x0200 0x 0xE100 0x 0xE100 0
+0x0F00>; };
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index 
c132bdd..5ff4e7e 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 PCI_XILINX_NWL
+   bool "NWL PCIe Core"
+   depends on ARCH_ZYNQMP && 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..0f3a789 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_XILINX_NWL) += pci-xilinx-nwl.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o diff --git 
a/drivers/pci/host/pci-xilinx-nwl.c b/drivers/pci/host/pci-xilinx-nwl.c
new file mode 100644
index 000..6c2fa80
--- /dev/null
+++ b/drivers/pci/host/pci-xilinx-nwl.c
@@ -0,0 +1,1038 @@
+/*
+ * PCIe host controller driver for NWL PCIe Bridge
+ * Based on pci-xilinx.c, pci-tegra.c
+ *
+ * (C) Copyright 2014 - 2015, Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of t

[PATCH] 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 bhara...@xilinx.com
Signed-off-by: Ravi Kiran Gummaluri rgum...@xilinx.com
---
 .../devicetree/bindings/pci/xilinx-nwl-pcie.txt|   39 +
 drivers/pci/host/Kconfig   |9 +
 drivers/pci/host/Makefile  |1 +
 drivers/pci/host/pci-xilinx-nwl.c  | 1038 
 4 files changed, 1087 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
 create mode 100644 drivers/pci/host/pci-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..c554d6b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -0,0 +1,39 @@
+* 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 and length
+- device_type: must be pci
+- interrupts: Should contain NWL PCIe interrupt
+- 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
+
+Optional properties:
+- xlnx,msi-fifo: To enable MSI FIFO mode
+
+Example:
+
+nwl_pcie: pcie@fd0e {
+   compatible = xlnx,nwl-pcie-2.11;
+   #address-cells = 3;
+   #size-cells = 2;
+   #interrupt-cells = 1;
+   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;
+   reg = 0x0 0xfd0e 0x1000
+  0x0 0xfd48 0x1000
+  0x0 0xE000 0x100;
+   reg-names = breg, pcireg, cfg;
+   ranges = 0x0200 0x 0xE100 0x 0xE100 0 
0x0F00;
+};
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c132bdd..5ff4e7e 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 PCI_XILINX_NWL
+   bool NWL PCIe Core
+   depends on ARCH_ZYNQMP  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..0f3a789 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_XILINX_NWL) += pci-xilinx-nwl.o
 obj-$(CONFIG_PCI_XGENE) += pci-xgene.o
 obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
diff --git a/drivers/pci/host/pci-xilinx-nwl.c 
b/drivers/pci/host/pci-xilinx-nwl.c
new file mode 100644
index 000..6c2fa80
--- /dev/null
+++ b/drivers/pci/host/pci-xilinx-nwl.c
@@ -0,0 +1,1038 @@
+/*
+ * PCIe host controller driver for NWL PCIe Bridge
+ * Based on pci-xilinx.c, pci-tegra.c
+ *
+ * (C) Copyright 2014 - 2015, Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/delay.h
+#include linux/interrupt.h
+#include linux/irq.h
+#include linux/irqdomain.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/msi.h
+#include linux/of_address.h
+#include linux/of_pci.h
+#include linux/of_platform.h
+#include linux/of_irq.h
+#include linux/pci.h
+#include linux/platform_device.h
+
+/* Bridge core config registers */
+#define BRCFG_PCIE_RX0 0x
+#define BRCFG_PCIE_RX1 0x0004
+#define BRCFG_AXI_MASTER   0x0008
+#define BRCFG_PCIE_TX  0x000C
+#define BRCFG_INTERRUPT0x0010
+#define BRCFG_RAM_DISABLE0 0x0014
+#define BRCFG_RAM_DISABLE1 0x0018
+#define BRCFG_PCIE_RELAXED_ORDER   0x001C