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

[Vivek Gautam <vivek.gautam@codeaurora.org>]

On 10/17/2018 9:41 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 16, 2018 at 11:28 PM Vivek Gautam
> <vivek.gautam@codeaurora.org> 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@codeaurora.org> 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