| 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 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"? > 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? Are you also convinced that they will always be enabled and disabled together (e.g. not controlled individually during suspend)? > - 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(). These comments apply also to the other patches. Johan
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@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
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: > >> + 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. Right you are. (I had it mixed up with a different API which had no such guarantees and now I can't seem to remember which it was, maybe I dreamt it.) > > 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. Sounds good. Johan
On 20/10/2022 14:32, Johan Hovold wrote: > 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: > >>>> + 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. > > Right you are. (I had it mixed up with a different API which had no such > guarantees and now I can't seem to remember which it was, maybe I dreamt > it.) Most probably you were thinking about regulators, which are a separate crazy beast. The regulator_bulk_enable() enables all the regulators in parallel using async calls.
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 >
On Fri, Jan 13, 2023 at 04:54:25PM +0100, Lorenzo Pieralisi wrote: > 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. I believe there was also a couple of bugs in patch 3/4 which was spotted by Jingoo Han and that would need to be fixed: https://lore.kernel.org/all/CAPOBaE5Zg+r0F35MvKWAozFa9x4xvym1LbA_UHvUSmnLbTpqzA@mail.gmail.com/ Johan
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(-)