On Fri, 22 Apr 2022 at 13:20, Johan Hovold <[email protected]> wrote:
>
> 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 <[email protected]>
> > > ---
> > > 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).
It is not about DT validation. It is about passing a correct DT. The
file can come up from the kernel. It can come from the older kernel.
OR it can come from the vendor. Or it even might be being a part of
firmware flashed into the device.
So we can not assume that the DT is correct just because the in-kernel
DT passes YAML validation.
So, as I wrote, the whole patchset needs much more care about compatibility.
> > 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).
We should not update compatible strings just because the driver
changes. Compat strings describe the hardware, not the Linux point of
view on it.
--
With best wishes
Dmitry
[ Again, please trim unnecessary context from your replies. ]
On Fri, Apr 22, 2022 at 01:35:01PM +0300, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <[email protected]> wrote:
> >
> > On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote:
> > > On 21/04/2022 13:20, Johan Hovold wrote:
> > > > + /* 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;
> > > > + } 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).
>
> It is not about DT validation. It is about passing a correct DT.
Heh. That's the same thing.
> The file can come up from the kernel. It can come from the older
> kernel. OR it can come from the vendor. Or it even might be being a
> part of firmware flashed into the device. So we can not assume that
> the DT is correct just because the in-kernel DT passes YAML
> validation.
Again, no. The kernel does not need to implement DT validation and can
assume that the DT describes the hardware accurately. If the DT says
there's a mux, the driver can use it. If there's no mux in DT, the
driver can assume it isn't there.
The only thing that complicates things here is the sc7280 dts which has
been released in 5.16. We don't care about Qualcomm's kernels and dts.
> So, as I wrote, the whole patchset needs much more care about compatibility.
>
> > > 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).
>
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.
We can, it's a documented practise in case a binding needs to be
updated in an incompatible way:
https://www.kernel.org/doc/html/latest/devicetree/bindings/ABI.html
But I don't think it'll be needed here.
Johan
On Fri, Apr 22, 2022 at 5:35 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Fri, 22 Apr 2022 at 13:20, Johan Hovold <[email protected]> wrote:
> >
> > 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 <[email protected]>
[...]
> > > > + } 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).
>
> It is not about DT validation. It is about passing a correct DT. The
> file can come up from the kernel. It can come from the older kernel.
> OR it can come from the vendor. Or it even might be being a part of
> firmware flashed into the device.
As of dtschema 2022.03, validation of dtb's from firmware (or anywhere
else) is supported. Of course, as the old saying goes, if it's not
upstream, it doesn't exist. We can't control what vendors do in their
DTs.
> So we can not assume that the DT is correct just because the in-kernel
> DT passes YAML validation.
I agree with Johan on this. In terms of ensuring correctness, the
kernel does a horrible job. It never will be as long as it is done in
ad hoc code.
> So, as I wrote, the whole patchset needs much more care about compatibility.
>
> > > 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).
>
> We should not update compatible strings just because the driver
> changes. Compat strings describe the hardware, not the Linux point of
> view on it.
Yes and no. It is the OS/client view of the h/w. If a binding is
deemed horribly broken we could do a new compatible string and
binding. That's not something we want to be doing frequently though.
Rob