Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition

[Vivek Gautam]

On 10/17/2018 9:41 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 16, 2018 at 11:28 PM Vivek Gautam
> <vivek.gautam@xxxxxxxxxxxxxx> wrote:
> > Hi Doug,
> >
> >
> > On 10/16/2018 10:29 PM, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Mon, Oct 15, 2018 at 10:51 PM Vivek Gautam
> > > <vivek.gautam@xxxxxxxxxxxxxx> wrote:
> > > > > > P.S.: While you are at it, can you please move 'ufs-qcom.txt'
> > > > > > to Documentation/devicetree/bindings/phy/qcom,ufs-phy.txt.
> > > > > > The current name and file location is misleading.
> > > > > I'd rather someone at Qualcomm do this.  Do you have a suggested
> > > > > person?  The reason I feel that Qualcomm needs to get involved is that
> > > > > I see that when I look at the file you refer to says it's for:
> > > > >
> > > > >     "qcom,ufs-phy-qmp-20nm" for 20nm ufs phy,
> > > > >     "qcom,ufs-phy-qmp-14nm" for legacy 14nm ufs phy,
> > > > >     "qcom,msm8996-ufs-phy-qmp-14nm" for 14nm ufs phy
> > > > >     present on MSM8996 chipset.
> > > > >
> > > > > ...but there's another Qualcomm file, 'qcom-qmp-phy.txt'.  That
> > > > > handles the compatible string:
> > > > >
> > > > >          "qcom,sdm845-qmp-ufs-phy" for UFS QMP phy on sdm845.
> > > > >
> > > > > So I'm a little confused.  Should the SDM845 UFS PHY been handled by
> > > > > the older UFS PHY driver?  ...or should all the older UFS PHYs be
> > > > > moved to be handled by the newer QMP PHY driver?  ...or are they
> > > > > really different hardware blocks, in which case how would you describe
> > > > > the difference (both are described as UFS QMP PHYs I think).
> > > > As you rightly said "ufs/ufs-qcom.txt" describes the bindings for
> > > > 14nm, and 20nm ufs phy. These phys are however handled by the older
> > > > ufs phy driver present at:
> > > > drivers/phy/qualcomm/phy-qcom-ufs-qmp-{14nm,20nm}.c
> > > > The sdm845 UFS phy driver is handled by the new consolidated qmp phy
> > > > driver: drivers/phy/qualcomm/phy-qcom-qmp.c whose bindings are
> > > > described by 'qcom-qmp-phy.txt'.
> > > > We didn't attempt to move the 14nm phy to new driver as we already had
> > > > 8996 using the bindings.
> > > >
> > > > So, really these are two separate drivers with different bindings. I
> > > > believe it should be okay to move the file. If you are fine, I can
> > > > attempt to post a small patch to do that.
> > > I guess what I should have said was that the new name you're proposing
> > > "qcom,ufs-phy.txt" is confusing and opening the file doesn't help
> > > clarify things.  The name and the binding make it sound like this is
> > > _the_ file to look at for Qualcomm UFS PHYs.  ...and then you look in
> > > the examples in the file and it seems that this even includes Qualcomm
> > > QMP PHYs for UFS.
> > >
> > > ...so while I agree that the file "ufs-qcom.txt" needs to be moved to
> > > the "PHY" directory, I think at the same time we need to change the
> > > name of the file and maybe the contents to disambiguate which things
> > > belong in this file vs. the "qcom-qmp-phy.txt".  ...and I feel like
> > > someone at Qualcomm will have the most information to properly do
> > > that.
> > >
> > > For instance, you could call the older bindings
> > > "qcom-qmp-phy-14nm-20nm.txt" or something like that.
> > Sure, I get your point. I will propose something that removes the confusion.
> >
> > > One point of clarification I'd like to know is if there's really a
> > > good reason to have two drivers here.  Certainly if the hardware is
> > > really different then a new driver can make sense, but if there are
> > > two drivers for arbitrary reasons then maybe they should be combined
> > > into one eventually?
> > Right, the 14nm phy driver can be happily merged into the new qmp-phy
> > driver.
> > But we should take care of older bindings. Removing the driver will break
> > things on targets with older bindings, precisely 8996.
> >
> > 20nm is bit tricky as it exported few APIs directly to ufs host
> > controller, and
> > that's the reason we have declared that as BROKEN after the ufs cleanup.
> > So, until we are really in a kill mode, the old ufs-phy driver will have
> > to live.
> OK, sounds like a plan.  I'll assume you're posting the patch to move
> the old PHY bindings and add some of the above information to them so
> people aren't confused.
>
> ...all this is sort off the original subject, though.  ;-)  Just a
> quick summary here is that nothing suggests ${SUBJECT} patch shouldn't
> land and all the additional discussion has been about making further
> improvements to the bindings situation for UFS on Qualcomm.

Yes, this patch is good to go.

Thanks
Vivek
> -Doug