RE: [PATCH v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host 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. > 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
Re: [PATCH v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller
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 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
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
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 devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html