Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

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
>