Re: [PATCH v2 06/10] PCI: qcom: Add SM8450 PCIe support

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Sat, Dec 11, 2021 at 05:01:01AM +0300, Dmitry Baryshkov wrote:
> On 10/12/2021 14:30, Manivannan Sadhasivam wrote:
> > On Wed, Dec 08, 2021 at 08:14:38PM +0300, Dmitry Baryshkov wrote:
> > > On SM8450 platform PCIe hosts do not use all the clocks (and add several
> > > additional clocks), so expand the driver to handle these requirements.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 47 +++++++++++++++++++-------
> > >   1 file changed, 34 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 803d3ac18c56..ada9c816395d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -161,7 +161,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[7];
> > > +	struct clk_bulk_data clks[9];
> > >   	int num_clks;
> > >   	struct regulator_bulk_data supplies[2];
> > >   	struct reset_control *pci_reset;
> > > @@ -196,7 +196,10 @@ struct qcom_pcie_cfg {
> > >   	const struct qcom_pcie_ops *ops;
> > >   	/* flags for ops 2.7.0 and 1.9.0 */
> > >   	unsigned int pipe_clk_need_muxing:1;
> > > +	unsigned int has_tbu_clk:1;
> > >   	unsigned int has_ddrss_sf_tbu_clk:1;
> > > +	unsigned int has_aggre0_clk:1;
> > > +	unsigned int has_aggre1_clk:1;
> > >   };
> > >   struct qcom_pcie {
> > > @@ -1147,6 +1150,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 idx;
> > 
> > u32?
> 
> Why? it's just a counter.
> 

Yeah but IMO using the kernel defined datatype is mostly preferred. This
is not touching any MMIO but still it is a good practice.

> > 
> > >   	int ret;
> > >   	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
> > > @@ -1160,18 +1164,22 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
> > >   	if (ret)
> > >   		return ret;
> > > -	res->clks[0].id = "aux";
> > > -	res->clks[1].id = "cfg";
> > > -	res->clks[2].id = "bus_master";
> > > -	res->clks[3].id = "bus_slave";
> > > -	res->clks[4].id = "slave_q2a";
> > > -	res->clks[5].id = "tbu";
> > > -	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
> > > -		res->clks[6].id = "ddrss_sf_tbu";
> > > -		res->num_clks = 7;
> > > -	} else {
> > > -		res->num_clks = 6;
> > > -	}
> > > +	idx = 0;
> > > +	res->clks[idx++].id = "aux";
> > > +	res->clks[idx++].id = "cfg";
> > > +	res->clks[idx++].id = "bus_master";
> > > +	res->clks[idx++].id = "bus_slave";
> > > +	res->clks[idx++].id = "slave_q2a";
> > > +	if (pcie->cfg->has_tbu_clk)
> > > +		res->clks[idx++].id = "tbu";
> > > +	if (pcie->cfg->has_ddrss_sf_tbu_clk)
> > > +		res->clks[idx++].id = "ddrss_sf_tbu";
> > > +	if (pcie->cfg->has_aggre0_clk)
> > > +		res->clks[idx++].id = "aggre0";
> > > +	if (pcie->cfg->has_aggre1_clk)
> > > +		res->clks[idx++].id = "aggre1";
> > > +
> > > +	res->num_clks = idx;
> > 
> > res->num_clks = idx + 1?
> 
> No. the idx is equal to the amount of clocks we added to the array, so this
> is correct.
> 

Oops, brain fade. Sorry, the usual post-increment confusion :) Ignore my
comment.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > >   	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> > >   	if (ret < 0)
> > > @@ -1510,15 +1518,27 @@ static const struct qcom_pcie_cfg ipq4019_cfg = {
> > >   static const struct qcom_pcie_cfg sdm845_cfg = {
> > >   	.ops = &ops_2_7_0,
> > > +	.has_tbu_clk = true,
> > >   };
> > >   static const struct qcom_pcie_cfg sm8250_cfg = {
> > >   	.ops = &ops_1_9_0,
> > > +	.has_tbu_clk = true,
> > >   	.has_ddrss_sf_tbu_clk = true,
> > >   };
> > > +/* Only for the PCIe0! */
> > > +static const struct qcom_pcie_cfg sm8450_cfg = {
> > > +	.ops = &ops_1_9_0,
> > > +	.has_ddrss_sf_tbu_clk = true,
> > > +	.pipe_clk_need_muxing = true,
> > > +	.has_aggre0_clk = true,
> > > +	.has_aggre1_clk = true,
> > > +};
> > > +
> > >   static const struct qcom_pcie_cfg sc7280_cfg = {
> > >   	.ops = &ops_1_9_0,
> > > +	.has_tbu_clk = true,
> > >   	.pipe_clk_need_muxing = true,
> > >   };
> > > @@ -1626,6 +1646,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > >   	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
> > >   	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
> > >   	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
> > > +	{ .compatible = "qcom,pcie-sm8450", .data = &sm8450_cfg },
> > >   	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> > >   	{ }
> > >   };
> > > -- 
> > > 2.33.0
> > > 
> 
> 
> -- 
> With best wishes
> Dmitry