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