Message-ID: <CAD=FV=WLAgowK67U1GkF3h_CZU_nyFfDPpZ=bF8BXU1jd_uTZg@mail.gmail.com>
Date:   Thu, 2 Apr 2020 16:19:34 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Manu Gautam <mgautam@...eaurora.org>,
        Sandeep Maheswaram <sanm@...eaurora.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH] phy: qcom-qusb2: Re add "qcom,sdm845-qusb2-phy" compat string

Hi,

On Thu, Apr 2, 2020 at 4:08 PM John Stultz <john.stultz@...aro.org> wrote:
>
> On Thu, Apr 2, 2020 at 3:56 PM Doug Anderson <dianders@...omium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 2, 2020 at 3:37 PM John Stultz <john.stultz@...aro.org> wrote:
> > >
> > > In commit 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2
> > > PHY support"), the change was made to add "qcom,qusb2-v2-phy"
> > > as a generic compat string. However the change also removed
> > > the "qcom,sdm845-qusb2-phy" compat string, which is documented
> > > in the binding and already in use.
> > >
> > > This patch re-adds the "qcom,sdm845-qusb2-phy" compat string
> > > which allows the driver to continue to work with existing dts
> > > entries such as found on the db845c.
> > >
> > > Cc: Andy Gross <agross@...nel.org>
> > > Cc: Bjorn Andersson <bjorn.andersson@...aro.org>
> > > Cc: Rob Herring <robh+dt@...nel.org>
> > > Cc: Mark Rutland <mark.rutland@....com>
> > > Cc: Doug Anderson <dianders@...omium.org>
> > > Cc: Manu Gautam <mgautam@...eaurora.org>
> > > Cc: Sandeep Maheswaram <sanm@...eaurora.org>
> > > Cc: Matthias Kaehlcke <mka@...omium.org>
> > > Cc: Stephen Boyd <swboyd@...omium.org>
> > > Cc: Kishon Vijay Abraham I <kishon@...com>
> > > Cc: linux-arm-msm@...r.kernel.org
> > > Cc: devicetree@...r.kernel.org
> > > Fixes: 8fe75cd4cddf ("phy: qcom-qusb2: Add generic QUSB2 V2 PHY support")
> > > Reported-by: YongQin Liu <yongqin.liu@...aro.org>
> > > Signed-off-by: John Stultz <john.stultz@...aro.org>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> >
> > Do you have an out-of-tree dts file?  If not, I'd prefer that the fix
> > was for this patch to land instead:
> >
> > https://lore.kernel.org/r/1583747589-17267-9-git-send-email-sanm@codeaurora.org
>
> No, no out of tree dts.  The usage is already in tree:
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sdm845.dtsi#n2389
>
>
> > While we can land your patch if someone needs it for supporting an
> > out-of-tree dts, it gives people supporting future SoCs the idea that
> > they need to add themselves to this table too.  That's now discouraged
> > unless there's a specific quirk that needs to be handled just for this
> > SoC.
>
> My understanding with dts bindings is that they are effectively an
> ABI. While maybe it makes sense to deprecate the
> "qcom,sdm845-qusb2-phy" string in the Documentation to avoid new
> users, I'd think we'd want to keep the support in the driver as we
> aren't supposed to have tight coupling between the DTB and kernel (at
> least for official bindings).

If nothing else if we're going to land your patch, can you at least
put a comment in there that says "only needed to support legacy device
trees that didn't include "qcom,qusb2-v2-phy" in the compatible
string.  Then the person who adds the next Qualcomm SoC will know not
to add themselves to the table too.


> Granted, I've not gotten much experience with boards that were fully
> upstream and thus didn't have an eternally evolving dts file that had
> to be kept in sync with the kernel, so in practice either solution
> does work for me, but in theory it seems like we should at least
> pretend these things are stable. :)

Yeah, I don't want to get into the whole stable ABI argument, but what
you say is the official word.  The bindings are supposed to be a
stable ABI and it's a good goal to strive for.

...but in reality most people are OK with it not being quite so stable
as long as it's not hurting anyone.  What should have happened here is
that the bindings and dts should have landed in one Linux version and
the driver change landed in the next Linux version.  Now we're stuck
with the breakage, though.  :(  In general for "new" architectures
it's considered more OK to break compatibility, though I guess you can
argue whether sdm845 is really new enough.  I guess to get at the meat
of the issue though: if you need a patch to fix your problem anyway,
why not land the patch that doesn't end up chewing extra up extra code
space and providing a bad example for someone to copy?

Now certainly if changing your DTS was an undue burden (like you've
already baked device trees into firmware) there's no question we
should land your patch.  I'm just not sure the lofty goal of "it's
supposed to be a stable ABI so let's add an entry to the table that
nobody will ever care about after the dts change lands" is enough of a
reason to land it now.

OK, I'll duck from the tomatoes now.

-Doug

Powered by blists - more mailing lists