| Message ID | 20230203081807.2248625-11-abel.vesa@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sm8550: Add PCIe HC and PHY support | expand |
On Fri, Feb 03, 2023 at 10:18:05AM +0200, Abel Vesa wrote: > Add compatible for both PCIe found on SM8550. > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. nit: You're now also adding 'noc_aggr' > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > --- > > The v6 of this patchset is: > https://lore.kernel.org/all/20230202123902.3831491-11-abel.vesa@linaro.org/ > > Changes since v6: > * none > > Changes since v5: > * none > > Changes since v4: > * added Mani's R-b tag > > Changes since v3: > * renamed cnoc_pcie_sf_axi to cnoc_sf_axi > > Changes since v2: > * none > > Changes since v1: > * changed the subject line prefix for the patch to match the history, > like Bjorn Helgaas suggested. > * added Konrad's R-b tag > > drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index a232b04af048..6a70c9c6f98d 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -182,10 +182,10 @@ 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[12]; > + struct clk_bulk_data clks[14]; > int num_clks; > struct regulator_bulk_data supplies[2]; > - struct reset_control *pci_reset; > + struct reset_control *rst; Please name this one 'reset' or 'resets' (e.g. to avoid hard to parse things like res->rst below). > }; > > struct qcom_pcie_resources_2_9_0 { > @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > unsigned int idx; > int ret; > > - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); > - if (IS_ERR(res->pci_reset)) > - return PTR_ERR(res->pci_reset); > + res->rst = devm_reset_control_array_get_exclusive(dev); > + if (IS_ERR(res->rst)) > + return PTR_ERR(res->rst); So the reset array implementation apparently both asserts and deasserts the resets in the order specified in DT (i.e. does not deassert in reverse order). Is that ok also for the new "pci" and "link_down" resets? > res->supplies[0].supply = "vdda"; > res->supplies[1].supply = "vddpe-3v3"; > @@ -1205,9 +1205,11 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > res->clks[idx++].id = "ddrss_sf_tbu"; > res->clks[idx++].id = "aggre0"; > res->clks[idx++].id = "aggre1"; > + res->clks[idx++].id = "noc_aggr"; > res->clks[idx++].id = "noc_aggr_4"; > res->clks[idx++].id = "noc_aggr_south_sf"; > res->clks[idx++].id = "cnoc_qx"; > + res->clks[idx++].id = "cnoc_sf_axi"; > > num_opt_clks = idx - num_clks; > res->num_clks = idx; > @@ -1237,17 +1239,17 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > if (ret < 0) > goto err_disable_regulators; > > - ret = reset_control_assert(res->pci_reset); > - if (ret < 0) { > - dev_err(dev, "cannot assert pci reset\n"); > + ret = reset_control_assert(res->rst); > + if (ret) { > + dev_err(dev, "reset assert failed (%d)\n", ret); > goto err_disable_clocks; > } > > usleep_range(1000, 1500); > > - ret = reset_control_deassert(res->pci_reset); > - if (ret < 0) { > - dev_err(dev, "cannot deassert pci reset\n"); > + ret = reset_control_deassert(res->rst); > + if (ret) { > + dev_err(dev, "reset deassert failed (%d)\n", ret); > goto err_disable_clocks; > } Johan
On 23-02-03 10:49:24, Johan Hovold wrote: > On Fri, Feb 03, 2023 at 10:18:05AM +0200, Abel Vesa wrote: > > Add compatible for both PCIe found on SM8550. > > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. > > nit: You're now also adding 'noc_aggr' > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > --- > > > > The v6 of this patchset is: > > https://lore.kernel.org/all/20230202123902.3831491-11-abel.vesa@linaro.org/ > > > > Changes since v6: > > * none > > > > Changes since v5: > > * none > > > > Changes since v4: > > * added Mani's R-b tag > > > > Changes since v3: > > * renamed cnoc_pcie_sf_axi to cnoc_sf_axi > > > > Changes since v2: > > * none > > > > Changes since v1: > > * changed the subject line prefix for the patch to match the history, > > like Bjorn Helgaas suggested. > > * added Konrad's R-b tag > > > > drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++----------- > > 1 file changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > index a232b04af048..6a70c9c6f98d 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > @@ -182,10 +182,10 @@ 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[12]; > > + struct clk_bulk_data clks[14]; > > int num_clks; > > struct regulator_bulk_data supplies[2]; > > - struct reset_control *pci_reset; > > + struct reset_control *rst; > > Please name this one 'reset' or 'resets' (e.g. to avoid hard to parse > things like res->rst below). Well, it would then be inconsitent with 2_3_3 and 2_9_0, which both use rst. > > > }; > > > > struct qcom_pcie_resources_2_9_0 { > > @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > > unsigned int idx; > > int ret; > > > > - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); > > - if (IS_ERR(res->pci_reset)) > > - return PTR_ERR(res->pci_reset); > > + res->rst = devm_reset_control_array_get_exclusive(dev); > > + if (IS_ERR(res->rst)) > > + return PTR_ERR(res->rst); > > So the reset array implementation apparently both asserts and deasserts > the resets in the order specified in DT (i.e. does not deassert in > reverse order). > > Is that ok also for the new "pci" and "link_down" resets? According to the HPG, yes, this is perfectly fine. It specifically says to assert the pcie reset and then continues saying to assert the link_down reset. > > > res->supplies[0].supply = "vdda"; > > res->supplies[1].supply = "vddpe-3v3"; > > @@ -1205,9 +1205,11 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > > res->clks[idx++].id = "ddrss_sf_tbu"; > > res->clks[idx++].id = "aggre0"; > > res->clks[idx++].id = "aggre1"; > > + res->clks[idx++].id = "noc_aggr"; > > res->clks[idx++].id = "noc_aggr_4"; > > res->clks[idx++].id = "noc_aggr_south_sf"; > > res->clks[idx++].id = "cnoc_qx"; > > + res->clks[idx++].id = "cnoc_sf_axi"; > > > > num_opt_clks = idx - num_clks; > > res->num_clks = idx; > > @@ -1237,17 +1239,17 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) > > if (ret < 0) > > goto err_disable_regulators; > > > > - ret = reset_control_assert(res->pci_reset); > > - if (ret < 0) { > > - dev_err(dev, "cannot assert pci reset\n"); > > + ret = reset_control_assert(res->rst); > > + if (ret) { > > + dev_err(dev, "reset assert failed (%d)\n", ret); > > goto err_disable_clocks; > > } > > > > usleep_range(1000, 1500); > > > > - ret = reset_control_deassert(res->pci_reset); > > - if (ret < 0) { > > - dev_err(dev, "cannot deassert pci reset\n"); > > + ret = reset_control_deassert(res->rst); > > + if (ret) { > > + dev_err(dev, "reset deassert failed (%d)\n", ret); > > goto err_disable_clocks; > > } > > Johan
On Mon, Feb 06, 2023 at 05:11:01PM +0200, Abel Vesa wrote: > On 23-02-03 10:49:24, Johan Hovold wrote: > > On Fri, Feb 03, 2023 at 10:18:05AM +0200, Abel Vesa wrote: > > > Add compatible for both PCIe found on SM8550. > > > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. > > > > nit: You're now also adding 'noc_aggr' > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > > --- > > > @@ -182,10 +182,10 @@ 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[12]; > > > + struct clk_bulk_data clks[14]; > > > int num_clks; > > > struct regulator_bulk_data supplies[2]; > > > - struct reset_control *pci_reset; > > > + struct reset_control *rst; > > > > Please name this one 'reset' or 'resets' (e.g. to avoid hard to parse > > things like res->rst below). > > Well, it would then be inconsitent with 2_3_3 and 2_9_0, which both use > rst. Yeah, I saw that. Fortunately these resources are completely independent, but whatever. > > > }; > > > > > > struct qcom_pcie_resources_2_9_0 { > > > @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > > > unsigned int idx; > > > int ret; > > > > > > - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); > > > - if (IS_ERR(res->pci_reset)) > > > - return PTR_ERR(res->pci_reset); > > > + res->rst = devm_reset_control_array_get_exclusive(dev); > > > + if (IS_ERR(res->rst)) > > > + return PTR_ERR(res->rst); > > > > So the reset array implementation apparently both asserts and deasserts > > the resets in the order specified in DT (i.e. does not deassert in > > reverse order). > > > > Is that ok also for the new "pci" and "link_down" resets? > > According to the HPG, yes, this is perfectly fine. It specifically says > to assert the pcie reset and then continues saying to assert the > link_down reset. Ok, but that doesn't really say anything about whether it's ok to *deassert* them in the same order, which was what I asked about. Johan
On 23-02-08 17:40:03, Johan Hovold wrote: > On Mon, Feb 06, 2023 at 05:11:01PM +0200, Abel Vesa wrote: > > On 23-02-03 10:49:24, Johan Hovold wrote: > > > On Fri, Feb 03, 2023 at 10:18:05AM +0200, Abel Vesa wrote: > > > > Add compatible for both PCIe found on SM8550. > > > > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. > > > > > > nit: You're now also adding 'noc_aggr' > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > > > --- > > > > > @@ -182,10 +182,10 @@ 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[12]; > > > > + struct clk_bulk_data clks[14]; > > > > int num_clks; > > > > struct regulator_bulk_data supplies[2]; > > > > - struct reset_control *pci_reset; > > > > + struct reset_control *rst; > > > > > > Please name this one 'reset' or 'resets' (e.g. to avoid hard to parse > > > things like res->rst below). > > > > Well, it would then be inconsitent with 2_3_3 and 2_9_0, which both use > > rst. > > Yeah, I saw that. Fortunately these resources are completely > independent, but whatever. Will do it in the next version then. > > > > > }; > > > > > > > > struct qcom_pcie_resources_2_9_0 { > > > > @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > > > > unsigned int idx; > > > > int ret; > > > > > > > > - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); > > > > - if (IS_ERR(res->pci_reset)) > > > > - return PTR_ERR(res->pci_reset); > > > > + res->rst = devm_reset_control_array_get_exclusive(dev); > > > > + if (IS_ERR(res->rst)) > > > > + return PTR_ERR(res->rst); > > > > > > So the reset array implementation apparently both asserts and deasserts > > > the resets in the order specified in DT (i.e. does not deassert in > > > reverse order). > > > > > > Is that ok also for the new "pci" and "link_down" resets? > > > > According to the HPG, yes, this is perfectly fine. It specifically says > > to assert the pcie reset and then continues saying to assert the > > link_down reset. > > Ok, but that doesn't really say anything about whether it's ok to > *deassert* them in the same order, which was what I asked about. Actually, what I wanted to say is that the HPG says something like this: "assert pcie reset, then assert link_down" and then at the end it literaly repeats the same phrase. > > Johan
On 23-02-08 19:10:17, Abel Vesa wrote: > On 23-02-08 17:40:03, Johan Hovold wrote: > > On Mon, Feb 06, 2023 at 05:11:01PM +0200, Abel Vesa wrote: > > > On 23-02-03 10:49:24, Johan Hovold wrote: > > > > On Fri, Feb 03, 2023 at 10:18:05AM +0200, Abel Vesa wrote: > > > > > Add compatible for both PCIe found on SM8550. > > > > > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. > > > > > > > > nit: You're now also adding 'noc_aggr' > > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > > > > --- > > > > > > > @@ -182,10 +182,10 @@ 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[12]; > > > > > + struct clk_bulk_data clks[14]; > > > > > int num_clks; > > > > > struct regulator_bulk_data supplies[2]; > > > > > - struct reset_control *pci_reset; > > > > > + struct reset_control *rst; > > > > > > > > Please name this one 'reset' or 'resets' (e.g. to avoid hard to parse > > > > things like res->rst below). > > > > > > Well, it would then be inconsitent with 2_3_3 and 2_9_0, which both use > > > rst. > > > > Yeah, I saw that. Fortunately these resources are completely > > independent, but whatever. > > Will do it in the next version then. > > > > > > > > }; > > > > > > > > > > struct qcom_pcie_resources_2_9_0 { > > > > > @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > > > > > unsigned int idx; > > > > > int ret; > > > > > > > > > > - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); > > > > > - if (IS_ERR(res->pci_reset)) > > > > > - return PTR_ERR(res->pci_reset); > > > > > + res->rst = devm_reset_control_array_get_exclusive(dev); > > > > > + if (IS_ERR(res->rst)) > > > > > + return PTR_ERR(res->rst); > > > > > > > > So the reset array implementation apparently both asserts and deasserts > > > > the resets in the order specified in DT (i.e. does not deassert in > > > > reverse order). > > > > > > > > Is that ok also for the new "pci" and "link_down" resets? > > > > > > According to the HPG, yes, this is perfectly fine. It specifically says > > > to assert the pcie reset and then continues saying to assert the > > > link_down reset. > > > > Ok, but that doesn't really say anything about whether it's ok to > > *deassert* them in the same order, which was what I asked about. > > Actually, what I wanted to say is that the HPG says something like this: > > "assert pcie reset, then assert link_down" > > and then at the end it literaly repeats the same phrase. but uses deassert instead of assert ... > > > > > > > > > Johan
On Wed, Feb 08, 2023 at 07:11:08PM +0200, Abel Vesa wrote: > On 23-02-08 19:10:17, Abel Vesa wrote: > > On 23-02-08 17:40:03, Johan Hovold wrote: > > > On Mon, Feb 06, 2023 at 05:11:01PM +0200, Abel Vesa wrote: > > > > On 23-02-03 10:49:24, Johan Hovold wrote: > > > > > On Fri, Feb 03, 2023 at 10:18:05AM +0200, Abel Vesa wrote: > > > > > > Add compatible for both PCIe found on SM8550. > > > > > > Also add the cnoc_pcie_sf_axi clock needed by the SM8550. > > > > > > > > > > nit: You're now also adding 'noc_aggr' > > > > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > > > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > > > > > --- > > > > > > > > > @@ -182,10 +182,10 @@ 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[12]; > > > > > > + struct clk_bulk_data clks[14]; > > > > > > int num_clks; > > > > > > struct regulator_bulk_data supplies[2]; > > > > > > - struct reset_control *pci_reset; > > > > > > + struct reset_control *rst; > > > > > > > > > > Please name this one 'reset' or 'resets' (e.g. to avoid hard to parse > > > > > things like res->rst below). > > > > > > > > Well, it would then be inconsitent with 2_3_3 and 2_9_0, which both use > > > > rst. > > > > > > Yeah, I saw that. Fortunately these resources are completely > > > independent, but whatever. > > > > Will do it in the next version then. Or just leave it as is. > > > > > > }; > > > > > > > > > > > > struct qcom_pcie_resources_2_9_0 { > > > > > > @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) > > > > > > unsigned int idx; > > > > > > int ret; > > > > > > > > > > > > - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); > > > > > > - if (IS_ERR(res->pci_reset)) > > > > > > - return PTR_ERR(res->pci_reset); > > > > > > + res->rst = devm_reset_control_array_get_exclusive(dev); > > > > > > + if (IS_ERR(res->rst)) > > > > > > + return PTR_ERR(res->rst); > > > > > > > > > > So the reset array implementation apparently both asserts and deasserts > > > > > the resets in the order specified in DT (i.e. does not deassert in > > > > > reverse order). > > > > > > > > > > Is that ok also for the new "pci" and "link_down" resets? > > > > > > > > According to the HPG, yes, this is perfectly fine. It specifically says > > > > to assert the pcie reset and then continues saying to assert the > > > > link_down reset. > > > > > > Ok, but that doesn't really say anything about whether it's ok to > > > *deassert* them in the same order, which was what I asked about. > > > > Actually, what I wanted to say is that the HPG says something like this: > > > > "assert pcie reset, then assert link_down" > > > > and then at the end it literaly repeats the same phrase. > > but uses deassert instead of assert ... Ok, then it seems to match the implementation. Thanks for clarifying. Johan
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index a232b04af048..6a70c9c6f98d 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -182,10 +182,10 @@ 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[12]; + struct clk_bulk_data clks[14]; int num_clks; struct regulator_bulk_data supplies[2]; - struct reset_control *pci_reset; + struct reset_control *rst; }; struct qcom_pcie_resources_2_9_0 { @@ -1177,9 +1177,9 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) unsigned int idx; int ret; - res->pci_reset = devm_reset_control_get_exclusive(dev, "pci"); - if (IS_ERR(res->pci_reset)) - return PTR_ERR(res->pci_reset); + res->rst = devm_reset_control_array_get_exclusive(dev); + if (IS_ERR(res->rst)) + return PTR_ERR(res->rst); res->supplies[0].supply = "vdda"; res->supplies[1].supply = "vddpe-3v3"; @@ -1205,9 +1205,11 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie) res->clks[idx++].id = "ddrss_sf_tbu"; res->clks[idx++].id = "aggre0"; res->clks[idx++].id = "aggre1"; + res->clks[idx++].id = "noc_aggr"; res->clks[idx++].id = "noc_aggr_4"; res->clks[idx++].id = "noc_aggr_south_sf"; res->clks[idx++].id = "cnoc_qx"; + res->clks[idx++].id = "cnoc_sf_axi"; num_opt_clks = idx - num_clks; res->num_clks = idx; @@ -1237,17 +1239,17 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie) if (ret < 0) goto err_disable_regulators; - ret = reset_control_assert(res->pci_reset); - if (ret < 0) { - dev_err(dev, "cannot assert pci reset\n"); + ret = reset_control_assert(res->rst); + if (ret) { + dev_err(dev, "reset assert failed (%d)\n", ret); goto err_disable_clocks; } usleep_range(1000, 1500); - ret = reset_control_deassert(res->pci_reset); - if (ret < 0) { - dev_err(dev, "cannot deassert pci reset\n"); + ret = reset_control_deassert(res->rst); + if (ret) { + dev_err(dev, "reset deassert failed (%d)\n", ret); goto err_disable_clocks; } @@ -1841,6 +1843,7 @@ static const struct of_device_id qcom_pcie_match[] = { { .compatible = "qcom,pcie-sm8350", .data = &cfg_1_9_0 }, { .compatible = "qcom,pcie-sm8450-pcie0", .data = &cfg_1_9_0 }, { .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 }, + { .compatible = "qcom,pcie-sm8550", .data = &cfg_1_9_0 }, { } };