Bjorn, thanks for the comments! On 12/16/2015 11:53 PM, Bjorn Helgaas wrote: > On Thu, Dec 03, 2015 at 03:35:22PM +0200, Stanimir Varbanov wrote: >> From: Stanimir Varbanov <svarbanov@xxxxxxxxxx> >> >> The PCIe driver reuse the Designware common code for host >> and MSI initialization, and also program the Qualcomm >> application specific registers. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@xxxxxxxxxx> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx> >> --- >> MAINTAINERS | 7 + >> drivers/pci/host/Kconfig | 10 + >> drivers/pci/host/Makefile | 1 + >> drivers/pci/host/pcie-qcom.c | 624 ++++++++++++++++++++++++++++++++++++++++++ > >> +#define PCIE20_CAP 0x70 >> +#define PCIE20_CAP_LINKCTRLSTATUS (PCIE20_CAP + 0x10) >> +#define PCIE20_CAP_LINKCTRLSTATUS_LINK_UP BIT(29) > > This looks like it could be referring to a standard PCIe Capability; > could you use the existing PCI_EXP_LNKSTA and PCI_EXP_LNKSTA_DLLLA > symbols here? And readw() instead of readl()? Yes, that is possible but I still need to keep PCIE20_CAP capabilities offset. > >> +static int qcom_pcie_enable_link_training(struct qcom_pcie *pcie) >> +{ >> + struct device *dev = pcie->dev; >> + u32 val; >> + int ret; >> + >> + /* enable link training */ >> + val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL); >> + val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE; >> + writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL); >> + >> + /* wait for up to 100ms for the link to come up */ >> + ret = readl_poll_timeout(pcie->elbi + PCIE20_ELBI_SYS_STTS, val, >> + val & XMLH_LINK_UP, LINKUP_DELAY_US, >> + LINKUP_TIMEOUT_US); >> + >> + if (ret < 0 || !dw_pcie_link_up(&pcie->pp)) { >> + dev_err(dev, "link initialization failed\n"); >> + return -ETIMEDOUT; >> + } >> + >> + return 0; >> +} > > This looks a lot like the *_establish_link() functions in other > DesignWare-based drivers. Can you make it look even more similar, > e.g., by renaming it to qcom_pcie_establish_link() and maybe moving > some of the PHY functionality here? > > readl_poll_timeout() is nice and avoids the hand-coded timeout loop > the other drivers use. But is there benefit in checking for > XMLH_LINK_UP, or could you simply poll dw_pcie_link_up() like the > others do? If it's sufficient, I'd prefer using dw_pcie_link_up() > by itself because it's a little more generic. OK I will modify the code to use dw_pcie_link_up() and ensure that this check is sufficient. -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html