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

[Michal Simek]

On 10/26/2015 11:26 AM, Bharat Kumar Gogada wrote:
>>> +   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 ?


FYI: Just this.
  reg = <0x0 0xfd0e 0x1000>,
<0x0 0xfd48 0x1000>,
<0x0 0xe000 0x100>;

Also please make sure that you are using the same case for addresses.
That 0xE000 case above.

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

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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Michal Simek]

On 10/26/2015 11:26 AM, Bharat Kumar Gogada wrote:
>>> +   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 ?


FYI: Just this.
  reg = <0x0 0xfd0e 0x1000>,
<0x0 0xfd48 0x1000>,
<0x0 0xe000 0x100>;

Also please make sure that you are using the same case for addresses.
That 0xE000 case above.

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

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

[Josh Cartwright]

Hello,

I've got a few comments below.

On Sat, Oct 17, 2015 at 12:52:18PM +0530, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada 
> Signed-off-by: Ravi Kiran Gummaluri 
> ---
> 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
[..]
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
[..]
> +static inline bool nwl_pcie_link_up(struct nwl_pcie *pcie, u32 check_link_up)
> +{
> + unsigned int status = -EINVAL;
> +
> + if (check_link_up == PCIE_USER_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> + else if (check_link_up == PHY_RDY_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PHY_RDY_LINKUP_BIT) ? 1 : 0;
> + return status;

It would seem marginally simpler if the the bit to check was passed as
the argument, instead of a check_link_up -> bit mapping.

> +}
> +
> +/**
> + * nwl_pcie_get_config_base - Get configuration base
> + *
> + * @bus: Bus structure of current bus
> + * @devfn: Device/function
> + * @where: Offset from base
> + *
> + * Return: Base address of the configuration space needed to be
> + *  accessed.
> + */
> +static void __iomem *nwl_pcie_get_config_base(struct pci_bus *bus,
> +  unsigned int devfn,
> +  int where)
> +{
> + struct nwl_pcie *pcie = bus->sysdata;
> + int relbus;
> +
> + relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
> + (devfn << ECAM_DEV_LOC_SHIFT);
> +
> + return pcie->ecam_base + relbus + where;
> +}

It would seem you could simplify if you provide this as your map_bus
implementation of pci_ops.  Then you could use the
pci_generic_config_read/_write callbacks.

> +/**
> + * 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,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + /* Pattern to generate SSLP TLP */
> + nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, RANDOM_DIGIT,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> +   TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> + status = 0;

This assignment looks useless?

> + mdelay(2);
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_BIT;
> + if (status) {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_STATUS_BIT;
> + if (status == SLVERR) {
> + dev_err(pcie->dev, "AXI slave error");
> + retval = SLVERR;
> + } else if (status == DECERR) {
> + dev_err(pcie->dev, "AXI Decode error");
> + retval = DECERR;
> + }
> + } else {
> + retval = 1;
> + }
> + }
> + } while (status);
> +
> + return retval;
> +}
> +
[..]
> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> + unsigned int devfn,
> + int where,
> + int size,
> + u32 val)
> +{
> + void __iomem *addr;
> + int retval;
> + struct nwl_pcie *pcie = bus->sysdata;
> +
> + if (!bus->number && devfn > 0)
> + 

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

[Josh Cartwright]

Hello,

I've got a few comments below.

On Sat, Oct 17, 2015 at 12:52:18PM +0530, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada 
> Signed-off-by: Ravi Kiran Gummaluri 
> ---
> 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
[..]
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
[..]
> +static inline bool nwl_pcie_link_up(struct nwl_pcie *pcie, u32 check_link_up)
> +{
> + unsigned int status = -EINVAL;
> +
> + if (check_link_up == PCIE_USER_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> + else if (check_link_up == PHY_RDY_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PHY_RDY_LINKUP_BIT) ? 1 : 0;
> + return status;

It would seem marginally simpler if the the bit to check was passed as
the argument, instead of a check_link_up -> bit mapping.

> +}
> +
> +/**
> + * nwl_pcie_get_config_base - Get configuration base
> + *
> + * @bus: Bus structure of current bus
> + * @devfn: Device/function
> + * @where: Offset from base
> + *
> + * Return: Base address of the configuration space needed to be
> + *  accessed.
> + */
> +static void __iomem *nwl_pcie_get_config_base(struct pci_bus *bus,
> +  unsigned int devfn,
> +  int where)
> +{
> + struct nwl_pcie *pcie = bus->sysdata;
> + int relbus;
> +
> + relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
> + (devfn << ECAM_DEV_LOC_SHIFT);
> +
> + return pcie->ecam_base + relbus + where;
> +}

It would seem you could simplify if you provide this as your map_bus
implementation of pci_ops.  Then you could use the
pci_generic_config_read/_write callbacks.

> +/**
> + * 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,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + /* Pattern to generate SSLP TLP */
> + nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, RANDOM_DIGIT,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> +   TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> + status = 0;

This assignment looks useless?

> + mdelay(2);
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_BIT;
> + if (status) {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_STATUS_BIT;
> + if (status == SLVERR) {
> + dev_err(pcie->dev, "AXI slave error");
> + retval = SLVERR;
> + } else if (status == DECERR) {
> + dev_err(pcie->dev, "AXI Decode error");
> + retval = DECERR;
> + }
> + } else {
> + retval = 1;
> + }
> + }
> + } while (status);
> +
> + return retval;
> +}
> +
[..]
> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> + unsigned int devfn,
> + int where,
> + int size,
> + u32 val)
> +{
> + void __iomem *addr;
> + int retval;
> + struct nwl_pcie *pcie = bus->sysdata;
> +
> + if 

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

[Arnd Bergmann]

On Saturday 17 October 2015 12:52:18 Bharat Kumar Gogada wrote:
> +   "msi_1, msi_0": interrupt asserted when msi is recieved

Better avoid underscores in DT, use "msi0" instead of "msi_0".

> +- 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.

The name doesn't match: below, the name is "legacy-interrupt-controller",
not "pcie_intc". I suppose it should really be "interrupt-controller"
anyway.

> +
> +Example:
> +
> +
> +nwl_pcie: pcie@fd0e {
> +   #address-cells = >;
> +   #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]

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.

> +   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.

> +   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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Arnd Bergmann]

On Saturday 17 October 2015 12:52:18 Bharat Kumar Gogada wrote:
> +   "msi_1, msi_0": interrupt asserted when msi is recieved

Better avoid underscores in DT, use "msi0" instead of "msi_0".

> +- 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.

The name doesn't match: below, the name is "legacy-interrupt-controller",
not "pcie_intc". I suppose it should really be "interrupt-controller"
anyway.

> +
> +Example:
> +
> +
> +nwl_pcie: pcie@fd0e {
> +   #address-cells = >;
> +   #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]

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.

> +   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.

> +   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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/