Re: [PATCH v7 10/12] PCI: qcom: Add SM8550 PCIe support

[Abel Vesa <abel.vesa@linaro.org>]

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