lkml.org 
[lkml]   [2022]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing
    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

    \
     
     \ /
      Last update: 2022-04-22 12:21    [W:2.546 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site