Message ID | 20191102002721.4091180-1-bjorn.andersson@linaro.org |
---|---|
Headers | show |
Series | PCI: qcom: Add support for SDM845 PCIe | expand |
On 01-11-19, 17:27, Bjorn Andersson wrote: > This second iteration of the patch series fixes review comments on v1 and has > been tested with both PCIe controllers found on the Qualcomm SDM845. Reviewed-by: Vinod Koul <vkoul@kernel.org>
Hi Bjorn, On Fri, 2019-11-01 at 17:27 -0700, Bjorn Andersson wrote: > The SDM845 has one Gen2 and one Gen3 controller, add support for these. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v1: > - Style changes requested by Stan > - Tested with second PCIe controller as well > > drivers/pci/controller/dwc/pcie-qcom.c | 152 +++++++++++++++++++++++++ > 1 file changed, 152 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 7e581748ee9f..35f4980480bb 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -54,6 +54,7 @@ [...] > +static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > +{ > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; > + struct dw_pcie *pci = pcie->pci; > + struct device *dev = pci->dev; > + u32 val; > + int ret; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies); > + if (ret < 0) { > + dev_err(dev, "cannot enable regulators\n"); > + return ret; > + } > + > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks); > + if (ret < 0) > + goto err_disable_regulators; > + > + ret = reset_control_assert(res->pci_reset); > + if (ret < 0) { > + dev_err(dev, "cannot deassert pci reset\n"); > + goto err_disable_clocks; > + } If for any of the above fails, the reset line is left in its default state, presumably unasserted. Is there a reason to assert and keep it asserted if enabling the clocks fails below? > + msleep(20); > + > + ret = reset_control_deassert(res->pci_reset); > + if (ret < 0) { > + dev_err(dev, "cannot deassert pci reset\n"); > + goto err_assert_resets; Nitpick: this seems superfluous since the reset line was just asserted 20 ms before. Maybe just: goto err_disable_clocks; > + } > + > + ret = clk_prepare_enable(res->pipe_clk); > + if (ret) { > + dev_err(dev, "cannot prepare/enable pipe clock\n"); > + goto err_assert_resets; > + } > + > + /* configure PCIe to RC mode */ > + writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE); > + > + /* enable PCIe clocks and resets */ > + val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL); > + val &= ~BIT(0); > + writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL); > + > + /* change DBI base address */ > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR); > + > + /* MAC PHY_POWERDOWN MUX DISABLE */ > + val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL); > + val &= ~BIT(29); > + writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL); > + > + val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); > + val |= BIT(4); > + writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); > + > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); > + val |= BIT(31); > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); > + } > + > + return 0; > +err_assert_resets: > + reset_control_assert(res->pci_reset); So maybe this can just be removed. The reset isn't asserted in deinit either. regards Philipp
On Mon 04 Nov 01:41 PST 2019, Philipp Zabel wrote: > Hi Bjorn, > > On Fri, 2019-11-01 at 17:27 -0700, Bjorn Andersson wrote: > > The SDM845 has one Gen2 and one Gen3 controller, add support for these. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v1: > > - Style changes requested by Stan > > - Tested with second PCIe controller as well > > > > drivers/pci/controller/dwc/pcie-qcom.c | 152 +++++++++++++++++++++++++ > > 1 file changed, 152 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > index 7e581748ee9f..35f4980480bb 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -54,6 +54,7 @@ > [...] > > +static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > > +{ > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0; > > + struct dw_pcie *pci = pcie->pci; > > + struct device *dev = pci->dev; > > + u32 val; > > + int ret; > > + > > + ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies); > > + if (ret < 0) { > > + dev_err(dev, "cannot enable regulators\n"); > > + return ret; > > + } > > + > > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks); > > + if (ret < 0) > > + goto err_disable_regulators; > > + > > + ret = reset_control_assert(res->pci_reset); > > + if (ret < 0) { > > + dev_err(dev, "cannot deassert pci reset\n"); > > + goto err_disable_clocks; > > + } > > If for any of the above fails, the reset line is left in its default > state, presumably unasserted. Is there a reason to assert and keep it > asserted if enabling the clocks fails below? > No, I don't think there's any reason for doing this and looking at the downstream driver, they don't even propagate this error. > > + msleep(20); And I see now that downstream has this as 1ms, will update and retest again. > > + > > + ret = reset_control_deassert(res->pci_reset); > > + if (ret < 0) { > > + dev_err(dev, "cannot deassert pci reset\n"); > > + goto err_assert_resets; > > Nitpick: this seems superfluous since the reset line was just asserted > 20 ms before. Maybe just: > > goto err_disable_clocks; Yes, this seems reasonable. > > > + } > > + > > + ret = clk_prepare_enable(res->pipe_clk); > > + if (ret) { > > + dev_err(dev, "cannot prepare/enable pipe clock\n"); > > + goto err_assert_resets; > > + } > > + > > + /* configure PCIe to RC mode */ > > + writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE); > > + > > + /* enable PCIe clocks and resets */ > > + val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL); > > + val &= ~BIT(0); > > + writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL); > > + > > + /* change DBI base address */ > > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR); > > + > > + /* MAC PHY_POWERDOWN MUX DISABLE */ > > + val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL); > > + val &= ~BIT(29); > > + writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL); > > + > > + val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); > > + val |= BIT(4); > > + writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL); > > + > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > + val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); > > + val |= BIT(31); > > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT); > > + } > > + > > + return 0; > > +err_assert_resets: > > + reset_control_assert(res->pci_reset); > > So maybe this can just be removed. The reset isn't asserted in deinit > either. > Sounds good. Thanks for your review, Philipp! Regards, Bjorn