On Wed, Jan 13, 2021 at 06:49:35PM +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@xxxxxxxxxx>
> Fixes: e1dd639e374a ("PCI: qcom: Add SM8250 SoC support")
Just couple of comments below, with that addressed:
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
Thanks,
Mani
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index affa2713bf80..e2140aba220a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -159,8 +159,10 @@ struct qcom_pcie_resources_2_3_3 {
> struct reset_control *rst[7];
> };
>
> +#define QCOM_PCIE_2_7_0_MAX_CLOCKS 7
> 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];
Just use 7 directly and mention in comment that SM8250 needs an extra clock.
> + int num_clks;
> struct regulator_bulk_data supplies[2];
> struct reset_control *pci_reset;
> struct clk *pipe_clk;
> @@ -1133,6 +1135,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;
> + bool has_sf_tbu = of_device_is_compatible(dev->of_node, "qcom,pcie-sm8250");
> int ret;
>
> res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
> @@ -1152,8 +1155,14 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
> res->clks[3].id = "bus_slave";
> res->clks[4].id = "slave_q2a";
> res->clks[5].id = "tbu";
> + if (has_sf_tbu) {
Here also you can use the of_device_is_compatible() call directly and avoid a
local variable.
Thanks,
Mani
> + res->clks[6].id = "ddrss_sf_tbu";
> + res->num_clks = 7;
> + } else {
> + res->num_clks = 6;
> + }
>
> - ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> + ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> if (ret < 0)
> return ret;
>
> @@ -1175,7 +1184,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> return ret;
> }
>
> - ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> + ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> if (ret < 0)
> goto err_disable_regulators;
>
> @@ -1227,7 +1236,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>
> return 0;
> err_disable_clocks:
> - clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> err_disable_regulators:
> regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>
> @@ -1238,7 +1247,7 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> {
> struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>
> - clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> }
>
> --
> 2.29.2
>
