| Message ID | 20220629141000.18111-7-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: qcom: Add support for SC8280XP and SA8540P | expand |
On Wed, Jun 29, 2022 at 04:09:56PM +0200, Johan Hovold wrote: > The SC8280XP platform has seven PCIe controllers: two used with USB4, > two 4-lane, two 2-lane and one 1-lane. > > Add a new "qcom,pcie-sc8280xp" compatible string and reuse the 1.9.0 > ops. > > Note that the SC8280XP controllers need two or three interconnect > clocks to be enabled. Model these as optional clocks to avoid encoding > devicetree data in the PCIe driver. Shouldn't the interconnect binding handle these? Probably, bus clocks have to be the biggest single reason why clocks are such a mess in terms of 'the same' block having different clocks. > > Note that the same could be done for the SM8450 interconnect clocks and > possibly also for the TBU clocks. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index ff1b40f213c1..da3f1cdc4ba6 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -160,7 +160,7 @@ struct qcom_pcie_resources_2_3_3 { > > /* 6 clocks typically, 7 for sm8250 */ > struct qcom_pcie_resources_2_7_0 { > - struct clk_bulk_data clks[9]; > + struct clk_bulk_data clks[12]; > int num_clks; > struct regulator_bulk_data supplies[2]; > struct reset_control *pci_reset; > @@ -1119,6 +1119,7 @@ static int qcom_pcie_get_resources_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; > + unsigned int num_clks, num_opt_clks; > unsigned int idx; > int ret; > > @@ -1148,9 +1149,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > if (pcie->cfg->has_aggre1_clk) > res->clks[idx++].id = "aggre1"; > > + num_clks = idx; > + > + ret = devm_clk_bulk_get(dev, num_clks, res->clks); > + if (ret < 0) > + return ret; > + > + res->clks[idx++].id = "noc_aggr_4"; > + res->clks[idx++].id = "noc_aggr_south_sf"; > + res->clks[idx++].id = "cnoc_qx"; > + > + num_opt_clks = idx - num_clks; > res->num_clks = idx; > > - ret = devm_clk_bulk_get(dev, res->num_clks, res->clks); > + ret = devm_clk_bulk_get_optional(dev, num_opt_clks, res->clks + num_clks); > if (ret < 0) > return ret; > > @@ -1449,6 +1461,11 @@ static const struct qcom_pcie_cfg ipq4019_cfg = { > .ops = &ops_2_4_0, > }; > > +static const struct qcom_pcie_cfg sc8280xp_cfg = { > + .ops = &ops_1_9_0, > + .has_ddrss_sf_tbu_clk = true, > +}; > + > static const struct qcom_pcie_cfg sdm845_cfg = { > .ops = &ops_2_7_0, > .has_tbu_clk = true, > @@ -1613,6 +1630,7 @@ static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-sm8150", .data = &sm8150_cfg }, > { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, > { .compatible = "qcom,pcie-sc8180x", .data = &sc8180x_cfg }, > + { .compatible = "qcom,pcie-sc8280xp", .data = &sc8280xp_cfg }, > { .compatible = "qcom,pcie-sm8450-pcie0", .data = &sm8450_pcie0_cfg }, > { .compatible = "qcom,pcie-sm8450-pcie1", .data = &sm8450_pcie1_cfg }, > { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg }, > -- > 2.35.1 > >
On Fri, Jul 01, 2022 at 12:29:09PM -0600, Rob Herring wrote: > On Wed, Jun 29, 2022 at 04:09:56PM +0200, Johan Hovold wrote: > > The SC8280XP platform has seven PCIe controllers: two used with USB4, > > two 4-lane, two 2-lane and one 1-lane. > > > > Add a new "qcom,pcie-sc8280xp" compatible string and reuse the 1.9.0 > > ops. > > > > Note that the SC8280XP controllers need two or three interconnect > > clocks to be enabled. Model these as optional clocks to avoid encoding > > devicetree data in the PCIe driver. > > Shouldn't the interconnect binding handle these? Probably, bus clocks > have to be the biggest single reason why clocks are such a mess in terms > of 'the same' block having different clocks. I fully agree, but I ended up following the current scheme of letting the driver manage these, partly as I lack documentation for the interconnect. Note that two interconnect clocks were recently included in the SM8450 binding: https://lore.kernel.org/all/YcIwcUzYCq1v4Kfs@robh.at.kernel.org/ and that the generically sounding clock names chosen make it tempting to repurpose them: https://lore.kernel.org/all/1656691899-21315-4-git-send-email-quic_krichai@quicinc.com/ which seems like another step backwards. For MSM8996, it looks like this was instead handled by never disabling the interconnect clocks: https://lore.kernel.org/all/20171207105922.31986-1-srinivas.kandagatla@linaro.org/ Johan
On Wed, Jun 29, 2022 at 04:09:56PM +0200, Johan Hovold wrote: > The SC8280XP platform has seven PCIe controllers: two used with USB4, > two 4-lane, two 2-lane and one 1-lane. > > Add a new "qcom,pcie-sc8280xp" compatible string and reuse the 1.9.0 > ops. > > Note that the SC8280XP controllers need two or three interconnect > clocks to be enabled. Model these as optional clocks to avoid encoding > devicetree data in the PCIe driver. > There isn't much info available for these clocks. Since some of these types of clocks are already present in bindings, I think this patch is fine by itself. I will also try to find how these clocks are laid out. But that shouldn't stop this patch IMO. > Note that the same could be done for the SM8450 interconnect clocks and > possibly also for the TBU clocks. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index ff1b40f213c1..da3f1cdc4ba6 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -160,7 +160,7 @@ struct qcom_pcie_resources_2_3_3 { > > /* 6 clocks typically, 7 for sm8250 */ > struct qcom_pcie_resources_2_7_0 { > - struct clk_bulk_data clks[9]; > + struct clk_bulk_data clks[12]; > int num_clks; > struct regulator_bulk_data supplies[2]; > struct reset_control *pci_reset; > @@ -1119,6 +1119,7 @@ static int qcom_pcie_get_resources_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; > + unsigned int num_clks, num_opt_clks; > unsigned int idx; > int ret; > > @@ -1148,9 +1149,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > if (pcie->cfg->has_aggre1_clk) > res->clks[idx++].id = "aggre1"; > > + num_clks = idx; > + > + ret = devm_clk_bulk_get(dev, num_clks, res->clks); > + if (ret < 0) > + return ret; > + > + res->clks[idx++].id = "noc_aggr_4"; > + res->clks[idx++].id = "noc_aggr_south_sf"; > + res->clks[idx++].id = "cnoc_qx"; > + > + num_opt_clks = idx - num_clks; > res->num_clks = idx; > > - ret = devm_clk_bulk_get(dev, res->num_clks, res->clks); > + ret = devm_clk_bulk_get_optional(dev, num_opt_clks, res->clks + num_clks); > if (ret < 0) > return ret; > > @@ -1449,6 +1461,11 @@ static const struct qcom_pcie_cfg ipq4019_cfg = { > .ops = &ops_2_4_0, > }; > > +static const struct qcom_pcie_cfg sc8280xp_cfg = { > + .ops = &ops_1_9_0, > + .has_ddrss_sf_tbu_clk = true, > +}; > + > static const struct qcom_pcie_cfg sdm845_cfg = { > .ops = &ops_2_7_0, > .has_tbu_clk = true, > @@ -1613,6 +1630,7 @@ static const struct of_device_id qcom_pcie_match[] = { > { .compatible = "qcom,pcie-sm8150", .data = &sm8150_cfg }, > { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, > { .compatible = "qcom,pcie-sc8180x", .data = &sc8180x_cfg }, > + { .compatible = "qcom,pcie-sc8280xp", .data = &sc8280xp_cfg }, > { .compatible = "qcom,pcie-sm8450-pcie0", .data = &sm8450_pcie0_cfg }, > { .compatible = "qcom,pcie-sm8450-pcie1", .data = &sm8450_pcie1_cfg }, > { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg }, > -- > 2.35.1 >
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index ff1b40f213c1..da3f1cdc4ba6 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -160,7 +160,7 @@ struct qcom_pcie_resources_2_3_3 { /* 6 clocks typically, 7 for sm8250 */ struct qcom_pcie_resources_2_7_0 { - struct clk_bulk_data clks[9]; + struct clk_bulk_data clks[12]; int num_clks; struct regulator_bulk_data supplies[2]; struct reset_control *pci_reset; @@ -1119,6 +1119,7 @@ static int qcom_pcie_get_resources_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; + unsigned int num_clks, num_opt_clks; unsigned int idx; int ret; @@ -1148,9 +1149,20 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) if (pcie->cfg->has_aggre1_clk) res->clks[idx++].id = "aggre1"; + num_clks = idx; + + ret = devm_clk_bulk_get(dev, num_clks, res->clks); + if (ret < 0) + return ret; + + res->clks[idx++].id = "noc_aggr_4"; + res->clks[idx++].id = "noc_aggr_south_sf"; + res->clks[idx++].id = "cnoc_qx"; + + num_opt_clks = idx - num_clks; res->num_clks = idx; - ret = devm_clk_bulk_get(dev, res->num_clks, res->clks); + ret = devm_clk_bulk_get_optional(dev, num_opt_clks, res->clks + num_clks); if (ret < 0) return ret; @@ -1449,6 +1461,11 @@ static const struct qcom_pcie_cfg ipq4019_cfg = { .ops = &ops_2_4_0, }; +static const struct qcom_pcie_cfg sc8280xp_cfg = { + .ops = &ops_1_9_0, + .has_ddrss_sf_tbu_clk = true, +}; + static const struct qcom_pcie_cfg sdm845_cfg = { .ops = &ops_2_7_0, .has_tbu_clk = true, @@ -1613,6 +1630,7 @@ static const struct of_device_id qcom_pcie_match[] = { { .compatible = "qcom,pcie-sm8150", .data = &sm8150_cfg }, { .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg }, { .compatible = "qcom,pcie-sc8180x", .data = &sc8180x_cfg }, + { .compatible = "qcom,pcie-sc8280xp", .data = &sc8280xp_cfg }, { .compatible = "qcom,pcie-sm8450-pcie0", .data = &sm8450_pcie0_cfg }, { .compatible = "qcom,pcie-sm8450-pcie1", .data = &sm8450_pcie1_cfg }, { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
The SC8280XP platform has seven PCIe controllers: two used with USB4, two 4-lane, two 2-lane and one 1-lane. Add a new "qcom,pcie-sc8280xp" compatible string and reuse the 1.9.0 ops. Note that the SC8280XP controllers need two or three interconnect clocks to be enabled. Model these as optional clocks to avoid encoding devicetree data in the PCIe driver. Note that the same could be done for the SM8450 interconnect clocks and possibly also for the TBU clocks. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)