Re: [PATCH v3 2/2] PCI: qcom: add support for ddrss_sf_tbu clock

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

Hi Dmitry,

On Thu, Dec 31, 2020 at 03:37:31PM +0300, Dmitry Baryshkov wrote:
> On SM8250 additional clock is required for PCIe devices to access NOC.
> Update PCIe controller driver to control this clock.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Fixes: e1dd639e374a ("PCI: qcom: Add SM8250 SoC support")
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index affa2713bf80..84c5a0a897dd 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -159,8 +159,9 @@ struct qcom_pcie_resources_2_3_3 {
>  	struct reset_control *rst[7];
>  };
>  
> +#define QCOM_PCIE_2_7_0_MAX_CLOCKS	6
>  struct qcom_pcie_resources_2_7_0 {
> -	struct clk_bulk_data clks[6];
> +	struct clk_bulk_data clks[QCOM_PCIE_2_7_0_MAX_CLOCKS + 1]; /* + 1 for sf_tbu */
>  	struct regulator_bulk_data supplies[2];
>  	struct reset_control *pci_reset;
>  	struct clk *pipe_clk;
> @@ -1153,7 +1154,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	res->clks[4].id = "slave_q2a";
>  	res->clks[5].id = "tbu";
>  
> -	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> +	ret = devm_clk_bulk_get(dev, QCOM_PCIE_2_7_0_MAX_CLOCKS, res->clks);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1161,6 +1162,22 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
>  
> +static int qcom_pcie_get_resources_1_9_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;
> +	int ret;
> +
> +	ret = qcom_pcie_get_resources_2_7_0(pcie);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Required clock for SM8250 */

Since you are using a dedicated ops for getting the resources, why can't
you use a dedicated resource struct as "qcom_pcie_resources_1_9_0" and
just use the total no of clocks as 7?

Also as I mentioned in previous iteration, the "sf_tbu" clock is not
optional.

Thanks,
Mani

> +	res->clks[6].clk = devm_clk_get_optional(dev, "ddrss_sf_tbu");
> +	return PTR_ERR_OR_ZERO(res->clks[6].clk);
> +}
> +
>  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;
> @@ -1437,7 +1454,7 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
>  
>  /* Qcom IP rev.: 1.9.0 */
>  static const struct qcom_pcie_ops ops_1_9_0 = {
> -	.get_resources = qcom_pcie_get_resources_2_7_0,
> +	.get_resources = qcom_pcie_get_resources_1_9_0,
>  	.init = qcom_pcie_init_2_7_0,
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> -- 
> 2.29.2
>