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@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
>
