| Message ID | 20221020103120.1541862-3-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: qcom: use clk_bulk API | expand |
On Thu, Oct 20, 2022 at 02:22:47PM +0300, Dmitry Baryshkov wrote: > On 20/10/2022 14:08, Johan Hovold wrote: > > On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote: > > > Change hand-coded implementation of bulk clocks to use the existing > > > > Let's hope everything is "hand-coded" at least for a few years still > > (job security). ;) > > > > Perhaps rephrase using "open-coded"? > > Yes, thank you. If that's the only change required I can fix it up when merging the series. Please let me know. Thanks, Lorenzo > > > > > clk_bulk_* API. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++-------------------- > > > 1 file changed, 16 insertions(+), 51 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > index 939f19241356..74588438db07 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > @@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 { > > > }; > > > struct qcom_pcie_resources_1_0_0 { > > > - struct clk *iface; > > > - struct clk *aux; > > > - struct clk *master_bus; > > > - struct clk *slave_bus; > > > + struct clk_bulk_data clks[4]; > > > struct reset_control *core; > > > struct regulator *vdda; > > > }; > > > @@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie) > > > struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0; > > > struct dw_pcie *pci = pcie->pci; > > > struct device *dev = pci->dev; > > > + int ret; > > > res->vdda = devm_regulator_get(dev, "vdda"); > > > if (IS_ERR(res->vdda)) > > > return PTR_ERR(res->vdda); > > > - res->iface = devm_clk_get(dev, "iface"); > > > - if (IS_ERR(res->iface)) > > > - return PTR_ERR(res->iface); > > > - > > > - res->aux = devm_clk_get(dev, "aux"); > > > - if (IS_ERR(res->aux)) > > > - return PTR_ERR(res->aux); > > > - > > > - res->master_bus = devm_clk_get(dev, "master_bus"); > > > - if (IS_ERR(res->master_bus)) > > > - return PTR_ERR(res->master_bus); > > > + res->clks[0].id = "aux"; > > > + res->clks[1].id = "iface"; > > > + res->clks[2].id = "master_bus"; > > > + res->clks[3].id = "slave_bus"; > > > - res->slave_bus = devm_clk_get(dev, "slave_bus"); > > > - if (IS_ERR(res->slave_bus)) > > > - return PTR_ERR(res->slave_bus); > > > + ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); > > > + if (ret < 0) > > > + return ret; > > > > Are you sure there are no dependencies between these clocks and that > > they can be enabled and disabled in any order? > > The order is enforced by the bulk API. Forward to enable, backward to > disable. > > > > > Are you also convinced that they will always be enabled and disabled > > together (e.g. not controlled individually during suspend)? > > From what I see downstream, yes. They separate host and pipe clocks, but for > each of these groups all clocks are disabled and enabled in sequence. > > For the newer platforms the only exceptions are refgen (handled by the PHY > in our kernels) and ddrss_sf_tbu (only on some platforms), which is not > touched by these patches. > > > > > > - ret = clk_prepare_enable(res->aux); > > > + ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks); > > > if (ret) { > > > - dev_err(dev, "cannot prepare/enable aux clock\n"); > > > + dev_err(dev, "cannot prepare/enable clocks\n"); > > > > The bulk API already logs an error so you can drop the dev_err(). > > Ack. > > > > > These comments apply also to the other patches. > > > > Johan > > -- > With best wishes > Dmitry >
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 939f19241356..74588438db07 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 { }; struct qcom_pcie_resources_1_0_0 { - struct clk *iface; - struct clk *aux; - struct clk *master_bus; - struct clk *slave_bus; + struct clk_bulk_data clks[4]; struct reset_control *core; struct regulator *vdda; }; @@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie) struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0; struct dw_pcie *pci = pcie->pci; struct device *dev = pci->dev; + int ret; res->vdda = devm_regulator_get(dev, "vdda"); if (IS_ERR(res->vdda)) return PTR_ERR(res->vdda); - res->iface = devm_clk_get(dev, "iface"); - if (IS_ERR(res->iface)) - return PTR_ERR(res->iface); - - res->aux = devm_clk_get(dev, "aux"); - if (IS_ERR(res->aux)) - return PTR_ERR(res->aux); - - res->master_bus = devm_clk_get(dev, "master_bus"); - if (IS_ERR(res->master_bus)) - return PTR_ERR(res->master_bus); + res->clks[0].id = "aux"; + res->clks[1].id = "iface"; + res->clks[2].id = "master_bus"; + res->clks[3].id = "slave_bus"; - res->slave_bus = devm_clk_get(dev, "slave_bus"); - if (IS_ERR(res->slave_bus)) - return PTR_ERR(res->slave_bus); + ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks); + if (ret < 0) + return ret; res->core = devm_reset_control_get_exclusive(dev, "core"); return PTR_ERR_OR_ZERO(res->core); @@ -502,10 +493,7 @@ static void qcom_pcie_deinit_1_0_0(struct qcom_pcie *pcie) struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0; reset_control_assert(res->core); - clk_disable_unprepare(res->slave_bus); - clk_disable_unprepare(res->master_bus); - clk_disable_unprepare(res->iface); - clk_disable_unprepare(res->aux); + clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks); regulator_disable(res->vdda); } @@ -522,45 +510,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie) return ret; } - ret = clk_prepare_enable(res->aux); + ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks); if (ret) { - dev_err(dev, "cannot prepare/enable aux clock\n"); + dev_err(dev, "cannot prepare/enable clocks\n"); goto err_res; } - ret = clk_prepare_enable(res->iface); - if (ret) { - dev_err(dev, "cannot prepare/enable iface clock\n"); - goto err_aux; - } - - ret = clk_prepare_enable(res->master_bus); - if (ret) { - dev_err(dev, "cannot prepare/enable master_bus clock\n"); - goto err_iface; - } - - ret = clk_prepare_enable(res->slave_bus); - if (ret) { - dev_err(dev, "cannot prepare/enable slave_bus clock\n"); - goto err_master; - } - ret = regulator_enable(res->vdda); if (ret) { dev_err(dev, "cannot enable vdda regulator\n"); - goto err_slave; + goto err_clocks; } return 0; -err_slave: - clk_disable_unprepare(res->slave_bus); -err_master: - clk_disable_unprepare(res->master_bus); -err_iface: - clk_disable_unprepare(res->iface); -err_aux: - clk_disable_unprepare(res->aux); + +err_clocks: + clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks); err_res: reset_control_assert(res->core);
Change hand-coded implementation of bulk clocks to use the existing clk_bulk_* API. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++-------------------- 1 file changed, 16 insertions(+), 51 deletions(-)