Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing

[Johan Hovold <johan@kernel.org>]

On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2022 13:20, Johan Hovold wrote:
> > Some QMP PHYs need to remux to their pipe clock input to the pipe clock
> > output generated by the PHY before powering on the PHY and restore the
> > default source during power down.
> > 
> > Add support for an optional pipe clock mux which will be reparented to
> > the generated pipe clock before powering on the PHY and restored to the
> > default reference source on power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++---
> >   1 file changed, 65 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 7d2d1ab061f7..bc6db9670291 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg {
> >    * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> >    * @pcs_misc: iomapped memory space for lane's pcs_misc
> >    * @pipe_clk: pipe clock
> > + * @pipemux_clk: pipe clock source mux
> > + * @piperef_clk: pipe clock default reference source
> >    * @index: lane index
> >    * @qmp: QMP phy to which this lane belongs
> >    * @lane_rst: lane's reset controller
> > @@ -3311,6 +3313,8 @@ struct qmp_phy {
> >   	void __iomem *rx2;
> >   	void __iomem *pcs_misc;
> >   	struct clk *pipe_clk;
> > +	struct clk *pipemux_clk;
> > +	struct clk *piperef_clk;
> >   	unsigned int index;
> >   	struct qcom_qmp *qmp;
> >   	struct reset_control *lane_rst;
> > @@ -3346,6 +3350,7 @@ struct qcom_qmp {
> >   	void __iomem *dp_com;
> >   
> >   	struct clk_bulk_data *clks;
> > +	struct clk *pipe_clksrc;
> 
> Please move this to qmp_phy too.

Ok.
 
> > +	/* Get optional pipe clock mux and default reference source clock. */
> > +	qphy->pipemux_clk = of_clk_get_by_name(np, "mux");
> > +	if (IS_ERR(qphy->pipemux_clk)) {
> > +		ret = PTR_ERR(qphy->pipemux_clk);
> > +		if (ret == -EPROBE_DEFER)
> > +			return ret;
> > +
> > +		qphy->pipemux_clk = NULL;
> 
> This makes the driver ignore every possible erorr except -EPROBE_DEFER. 
> However the driver should behave in quite the oppposite way. Please use 
> devm_clk_get_optional() instead. It would do that in better way.

We'd need to add an optional version of devm_get_clk_from_child() for
that due to the questionable "lane" child nodes this driver uses.

The above works for an RFC, but testing for -EINVAL and -ENOENT handles
a few more theoretical errnos until an optional helper is in place.

> Not to mention that this code leaks a refcount on the clock.

True, just like the driver has been doing with the pipe clock and lane
reset since it was merged. I'll fix that up.

> > +	} else {
> > +		qphy->piperef_clk = of_clk_get_by_name(np, "ref");
> > +		if (IS_ERR(qphy->piperef_clk)) {
> > +			ret = PTR_ERR(qphy->piperef_clk);
> > +			return dev_err_probe(dev, ret,
> > +					     "failed to get lane%d piperef_clk\n",
> > +					     id);
> > +		}
> > +	}
> > +
> 
> As a second thought.
> This needs to be more explicit. If the chipset requires the pipe clock 
> remuxing, we must fail if the clocks were not provided. So depending on 
> the qmp instance/property the driver should either use devm_clk_get() 
> (instead of _optional) or skip this block completely.

No, the kernel is not a DT validator (and we have the YAML bindings for
that now).

> But this will not work with earlier DTS files.

So this is not a problem (but if we really wanted to have the driver
validate the DT it can be done by updating the compatible strings).

Johan