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.
clk_bulk_* API.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
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
