* [PATCH] dt-bindings: ufs: Fix the compatible string definition @ 2018-10-12 21:39 Douglas Anderson 2018-10-15 15:23 ` Vivek Gautam ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Douglas Anderson @ 2018-10-12 21:39 UTC (permalink / raw) To: Rob Herring, Martin K . Petersen Cc: Can Guo, evgreen, Vivek Gautam, linux-arm-msm, sayalil, asutoshd, Douglas Anderson, devicetree, liwei, linux-kernel, Mathieu Malaterre, Mark Rutland If you look at the bindings for the UFS Host Controller it says: - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may also list one or more of the following: "qcom,msm8994-ufshc" "qcom,msm8996-ufshc" "qcom,ufshc" My reading of that is that it's fine to just have either of these: 1. "qcom,msm8996-ufshc", "jedec,ufs-2.0" 2. "qcom,ufshc", "jedec,ufs-2.0" As far as I can tell neither of the above is actually a good idea. For #1 it turns out that the driver currently only keys off the compatible string "qcom,ufshc" so it won't actually probe. For #2 the driver won't probe but it's not a good idea to keep the SoC name out of the compatible string. Let's update the compatible string to make it really explicit. We'll include a nod to the existing driver and the old binding and say that we should always include the "qcom,ufshc" string in addition to the SoC compatible string. While we're at it we'll also include another example SoC known to have UFS: sdm845. Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt index 2df00524bd21..69a06a1b732e 100644 --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt @@ -4,11 +4,14 @@ UFSHC nodes are defined to describe on-chip UFS host controllers. Each UFS controller instance should have its own node. Required properties: -- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may - also list one or more of the following: - "qcom,msm8994-ufshc" - "qcom,msm8996-ufshc" - "qcom,ufshc" +- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0" + + For Qualcomm SoCs must contain, as below, an + SoC-specific compatible along with "qcom,ufshc" and + the appropriate jedec string: + "qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0" + "qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0" + "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0" - interrupts : <interrupt mapping for UFS host controller IRQ> - reg : <registers mapping> -- 2.19.0.605.g01d371f741-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-12 21:39 [PATCH] dt-bindings: ufs: Fix the compatible string definition Douglas Anderson @ 2018-10-15 15:23 ` Vivek Gautam 2018-10-15 17:23 ` Doug Anderson 2018-10-18 0:42 ` Rob Herring 2019-02-06 13:59 ` Marc Gonzalez 2 siblings, 1 reply; 12+ messages in thread From: Vivek Gautam @ 2018-10-15 15:23 UTC (permalink / raw) To: Doug Anderson Cc: robh+dt, Martin K. Petersen, Can Guo, evgreen, linux-arm-msm, sayalil, asutoshd, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, liwei213, open list, malat, Mark Rutland Hi Doug, On Sat, Oct 13, 2018 at 3:09 AM Douglas Anderson <dianders@chromium.org> wrote: > > If you look at the bindings for the UFS Host Controller it says: > > - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > also list one or more of the following: > "qcom,msm8994-ufshc" > "qcom,msm8996-ufshc" > "qcom,ufshc" > > My reading of that is that it's fine to just have either of these: > 1. "qcom,msm8996-ufshc", "jedec,ufs-2.0" > 2. "qcom,ufshc", "jedec,ufs-2.0" > > As far as I can tell neither of the above is actually a good idea. > > For #1 it turns out that the driver currently only keys off the > compatible string "qcom,ufshc" so it won't actually probe. > > For #2 the driver won't probe but it's not a good idea to keep the SoC > name out of the compatible string. > > Let's update the compatible string to make it really explicit. We'll > include a nod to the existing driver and the old binding and say that > we should always include the "qcom,ufshc" string in addition to the > SoC compatible string. > > While we're at it we'll also include another example SoC known to have > UFS: sdm845. > > Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > index 2df00524bd21..69a06a1b732e 100644 > --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > @@ -4,11 +4,14 @@ UFSHC nodes are defined to describe on-chip UFS host controllers. > Each UFS controller instance should have its own node. > > Required properties: > -- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > - also list one or more of the following: > - "qcom,msm8994-ufshc" > - "qcom,msm8996-ufshc" > - "qcom,ufshc" > +- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0" > + > + For Qualcomm SoCs must contain, as below, an > + SoC-specific compatible along with "qcom,ufshc" and > + the appropriate jedec string: > + "qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > + "qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > + "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0" Thanks for the patch. It looks good to me. Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org> 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. Thanks & Regards Vivek -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-15 15:23 ` Vivek Gautam @ 2018-10-15 17:23 ` Doug Anderson 2018-10-16 5:51 ` Vivek Gautam 0 siblings, 1 reply; 12+ messages in thread From: Doug Anderson @ 2018-10-15 17:23 UTC (permalink / raw) To: Vivek Gautam Cc: Rob Herring, Martin K. Petersen, cang, Evan Green, linux-arm-msm, sayalil, Asutosh Das, devicetree, liwei213, LKML, Mathieu Malaterre, Mark Rutland Vivek, On Mon, Oct 15, 2018 at 8:23 AM Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > Hi Doug, > > On Sat, Oct 13, 2018 at 3:09 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > If you look at the bindings for the UFS Host Controller it says: > > > > - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > > also list one or more of the following: > > "qcom,msm8994-ufshc" > > "qcom,msm8996-ufshc" > > "qcom,ufshc" > > > > My reading of that is that it's fine to just have either of these: > > 1. "qcom,msm8996-ufshc", "jedec,ufs-2.0" > > 2. "qcom,ufshc", "jedec,ufs-2.0" > > > > As far as I can tell neither of the above is actually a good idea. > > > > For #1 it turns out that the driver currently only keys off the > > compatible string "qcom,ufshc" so it won't actually probe. > > > > For #2 the driver won't probe but it's not a good idea to keep the SoC > > name out of the compatible string. > > > > Let's update the compatible string to make it really explicit. We'll > > include a nod to the existing driver and the old binding and say that > > we should always include the "qcom,ufshc" string in addition to the > > SoC compatible string. > > > > While we're at it we'll also include another example SoC known to have > > UFS: sdm845. > > > > Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > > index 2df00524bd21..69a06a1b732e 100644 > > --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > > +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > > @@ -4,11 +4,14 @@ UFSHC nodes are defined to describe on-chip UFS host controllers. > > Each UFS controller instance should have its own node. > > > > Required properties: > > -- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > > - also list one or more of the following: > > - "qcom,msm8994-ufshc" > > - "qcom,msm8996-ufshc" > > - "qcom,ufshc" > > +- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0" > > + > > + For Qualcomm SoCs must contain, as below, an > > + SoC-specific compatible along with "qcom,ufshc" and > > + the appropriate jedec string: > > + "qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > > + "qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > > + "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > > Thanks for the patch. It looks good to me. > Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org> Thanks for the review! > 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). BTW: I have a patch attempting to fixup the QMP PHY bindings at <https://lkml.kernel.org/r/20181012213632.252346-1-dianders@chromium.org>. -Doug ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-15 17:23 ` Doug Anderson @ 2018-10-16 5:51 ` Vivek Gautam 2018-10-16 16:59 ` Doug Anderson 0 siblings, 1 reply; 12+ messages in thread From: Vivek Gautam @ 2018-10-16 5:51 UTC (permalink / raw) To: Doug Anderson Cc: robh+dt, Martin K. Petersen, Can Guo, evgreen, linux-arm-msm, sayalil, asutoshd, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, liwei213, open list, malat, Mark Rutland On Mon, Oct 15, 2018 at 10:54 PM Doug Anderson <dianders@chromium.org> wrote: > > Vivek, > > On Mon, Oct 15, 2018 at 8:23 AM Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: > > > > Hi Doug, > > > > On Sat, Oct 13, 2018 at 3:09 AM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > If you look at the bindings for the UFS Host Controller it says: > > > > > > - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > > > also list one or more of the following: > > > "qcom,msm8994-ufshc" > > > "qcom,msm8996-ufshc" > > > "qcom,ufshc" > > > > > > My reading of that is that it's fine to just have either of these: > > > 1. "qcom,msm8996-ufshc", "jedec,ufs-2.0" > > > 2. "qcom,ufshc", "jedec,ufs-2.0" > > > > > > As far as I can tell neither of the above is actually a good idea. > > > > > > For #1 it turns out that the driver currently only keys off the > > > compatible string "qcom,ufshc" so it won't actually probe. > > > > > > For #2 the driver won't probe but it's not a good idea to keep the SoC > > > name out of the compatible string. > > > > > > Let's update the compatible string to make it really explicit. We'll > > > include a nod to the existing driver and the old binding and say that > > > we should always include the "qcom,ufshc" string in addition to the > > > SoC compatible string. > > > > > > While we're at it we'll also include another example SoC known to have > > > UFS: sdm845. > > > > > > Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > > > > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++----- > > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > > > index 2df00524bd21..69a06a1b732e 100644 > > > --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > > > +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > > > @@ -4,11 +4,14 @@ UFSHC nodes are defined to describe on-chip UFS host controllers. > > > Each UFS controller instance should have its own node. > > > > > > Required properties: > > > -- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > > > - also list one or more of the following: > > > - "qcom,msm8994-ufshc" > > > - "qcom,msm8996-ufshc" > > > - "qcom,ufshc" > > > +- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0" > > > + > > > + For Qualcomm SoCs must contain, as below, an > > > + SoC-specific compatible along with "qcom,ufshc" and > > > + the appropriate jedec string: > > > + "qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > > > + "qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > > > + "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > > > > Thanks for the patch. It looks good to me. > > Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org> > > Thanks for the review! > > > > 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. Thanks. > > BTW: I have a patch attempting to fixup the QMP PHY bindings at > <https://lkml.kernel.org/r/20181012213632.252346-1-dianders@chromium.org>. Sure, I will review this as well. Thanks for reminiding. Best regards Vivek -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-16 5:51 ` Vivek Gautam @ 2018-10-16 16:59 ` Doug Anderson 2018-10-17 6:28 ` Vivek Gautam 0 siblings, 1 reply; 12+ messages in thread From: Doug Anderson @ 2018-10-16 16:59 UTC (permalink / raw) To: Vivek Gautam Cc: Rob Herring, Martin K. Petersen, cang, Evan Green, linux-arm-msm, sayalil, Asutosh Das, devicetree, liwei213, LKML, Mathieu Malaterre, Mark Rutland 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. 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? -Doug ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-16 16:59 ` Doug Anderson @ 2018-10-17 6:28 ` Vivek Gautam 2018-10-17 16:11 ` Doug Anderson 0 siblings, 1 reply; 12+ messages in thread From: Vivek Gautam @ 2018-10-17 6:28 UTC (permalink / raw) To: Doug Anderson Cc: Rob Herring, Martin K. Petersen, cang, Evan Green, linux-arm-msm, sayalil, Asutosh Das, devicetree, liwei213, LKML, Mathieu Malaterre, Mark Rutland 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. Regards Vivek > > > -Doug ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-17 6:28 ` Vivek Gautam @ 2018-10-17 16:11 ` Doug Anderson 2018-10-18 7:52 ` Vivek Gautam 0 siblings, 1 reply; 12+ messages in thread From: Doug Anderson @ 2018-10-17 16:11 UTC (permalink / raw) To: Vivek Gautam Cc: Rob Herring, Martin K. Petersen, cang, Evan Green, linux-arm-msm, sayalil, Asutosh Das, devicetree, liwei213, LKML, Mathieu Malaterre, Mark Rutland 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. -Doug ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-17 16:11 ` Doug Anderson @ 2018-10-18 7:52 ` Vivek Gautam 2019-01-23 22:17 ` Evan Green 0 siblings, 1 reply; 12+ messages in thread From: Vivek Gautam @ 2018-10-18 7:52 UTC (permalink / raw) To: Doug Anderson Cc: Rob Herring, Martin K. Petersen, cang, Evan Green, linux-arm-msm, sayalil, Asutosh Das, devicetree, liwei213, LKML, Mathieu Malaterre, Mark Rutland 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-18 7:52 ` Vivek Gautam @ 2019-01-23 22:17 ` Evan Green 0 siblings, 0 replies; 12+ messages in thread From: Evan Green @ 2019-01-23 22:17 UTC (permalink / raw) To: Vivek Gautam Cc: Doug Anderson, Rob Herring, Martin K. Petersen, Can Guo, linux-arm-msm, sayali, Asutosh Das, devicetree, liwei213, LKML, Mathieu Malaterre, Mark Rutland On Thu, Oct 18, 2018 at 12:52 AM Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > ... > > Yes, this patch is good to go. > I don't think this patch ever got picked up, despite being reviewed and good to go. Can it be applied somewhere? -Evan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-12 21:39 [PATCH] dt-bindings: ufs: Fix the compatible string definition Douglas Anderson 2018-10-15 15:23 ` Vivek Gautam @ 2018-10-18 0:42 ` Rob Herring 2019-02-06 13:59 ` Marc Gonzalez 2 siblings, 0 replies; 12+ messages in thread From: Rob Herring @ 2018-10-18 0:42 UTC (permalink / raw) To: Douglas Anderson Cc: Martin K . Petersen, Can Guo, evgreen, Vivek Gautam, linux-arm-msm, sayalil, asutoshd, Douglas Anderson, devicetree, liwei, linux-kernel, Mathieu Malaterre, Mark Rutland On Fri, 12 Oct 2018 14:39:26 -0700, Douglas Anderson wrote: > If you look at the bindings for the UFS Host Controller it says: > > - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > also list one or more of the following: > "qcom,msm8994-ufshc" > "qcom,msm8996-ufshc" > "qcom,ufshc" > > My reading of that is that it's fine to just have either of these: > 1. "qcom,msm8996-ufshc", "jedec,ufs-2.0" > 2. "qcom,ufshc", "jedec,ufs-2.0" > > As far as I can tell neither of the above is actually a good idea. > > For #1 it turns out that the driver currently only keys off the > compatible string "qcom,ufshc" so it won't actually probe. > > For #2 the driver won't probe but it's not a good idea to keep the SoC > name out of the compatible string. > > Let's update the compatible string to make it really explicit. We'll > include a nod to the existing driver and the old binding and say that > we should always include the "qcom,ufshc" string in addition to the > SoC compatible string. > > While we're at it we'll also include another example SoC known to have > UFS: sdm845. > > Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2018-10-12 21:39 [PATCH] dt-bindings: ufs: Fix the compatible string definition Douglas Anderson 2018-10-15 15:23 ` Vivek Gautam 2018-10-18 0:42 ` Rob Herring @ 2019-02-06 13:59 ` Marc Gonzalez 2019-02-08 22:40 ` Martin K. Petersen 2 siblings, 1 reply; 12+ messages in thread From: Marc Gonzalez @ 2019-02-06 13:59 UTC (permalink / raw) To: Martin K . Petersen Cc: Douglas Anderson, Evan Green, Rob Herring, SCSI, LKML, Alim Akhtar, Avri Altman, Pedro Sousa, Bjorn Andersson, Andy Gross, Jeffrey Hugo, Vivek Gautam On 12/10/2018 23:39, Douglas Anderson wrote: > If you look at the bindings for the UFS Host Controller it says: > > - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > also list one or more of the following: > "qcom,msm8994-ufshc" > "qcom,msm8996-ufshc" > "qcom,ufshc" > > My reading of that is that it's fine to just have either of these: > 1. "qcom,msm8996-ufshc", "jedec,ufs-2.0" > 2. "qcom,ufshc", "jedec,ufs-2.0" > > As far as I can tell neither of the above is actually a good idea. > > For #1 it turns out that the driver currently only keys off the > compatible string "qcom,ufshc" so it won't actually probe. > > For #2 the driver won't probe but it's not a good idea to keep the SoC > name out of the compatible string. > > Let's update the compatible string to make it really explicit. We'll > include a nod to the existing driver and the old binding and say that > we should always include the "qcom,ufshc" string in addition to the > SoC compatible string. > > While we're at it we'll also include another example SoC known to have > UFS: sdm845. > > Fixes: 47555a5c8a11 ("scsi: ufs: make the UFS variant a platform device") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > index 2df00524bd21..69a06a1b732e 100644 > --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt > @@ -4,11 +4,14 @@ UFSHC nodes are defined to describe on-chip UFS host controllers. > Each UFS controller instance should have its own node. > > Required properties: > -- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may > - also list one or more of the following: > - "qcom,msm8994-ufshc" > - "qcom,msm8996-ufshc" > - "qcom,ufshc" > +- compatible : must contain "jedec,ufs-1.1" or "jedec,ufs-2.0" > + > + For Qualcomm SoCs must contain, as below, an > + SoC-specific compatible along with "qcom,ufshc" and > + the appropriate jedec string: > + "qcom,msm8994-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > + "qcom,msm8996-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > + "qcom,sdm845-ufshc", "qcom,ufshc", "jedec,ufs-2.0" > - interrupts : <interrupt mapping for UFS host controller IRQ> > - reg : <registers mapping> > Tweaking CC list. Martin, this one's for you, according to Rob ;-) https://lore.kernel.org/patchwork/patch/999046/ FWIW, it has already been: Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org> Reviewed-by: Rob Herring <robh@kernel.org> Regards. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dt-bindings: ufs: Fix the compatible string definition 2019-02-06 13:59 ` Marc Gonzalez @ 2019-02-08 22:40 ` Martin K. Petersen 0 siblings, 0 replies; 12+ messages in thread From: Martin K. Petersen @ 2019-02-08 22:40 UTC (permalink / raw) To: Marc Gonzalez Cc: Martin K . Petersen, Douglas Anderson, Evan Green, Rob Herring, SCSI, LKML, Alim Akhtar, Avri Altman, Pedro Sousa, Bjorn Andersson, Andy Gross, Jeffrey Hugo, Vivek Gautam Marc, >> - compatible: must contain "jedec,ufs-1.1" or "jedec,ufs-2.0", may >> also list one or more of the following: >> "qcom,msm8994-ufshc" >> "qcom,msm8996-ufshc" >> "qcom,ufshc" Applied to 5.1/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-02-08 22:40 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-12 21:39 [PATCH] dt-bindings: ufs: Fix the compatible string definition Douglas Anderson 2018-10-15 15:23 ` Vivek Gautam 2018-10-15 17:23 ` Doug Anderson 2018-10-16 5:51 ` Vivek Gautam 2018-10-16 16:59 ` Doug Anderson 2018-10-17 6:28 ` Vivek Gautam 2018-10-17 16:11 ` Doug Anderson 2018-10-18 7:52 ` Vivek Gautam 2019-01-23 22:17 ` Evan Green 2018-10-18 0:42 ` Rob Herring 2019-02-06 13:59 ` Marc Gonzalez 2019-02-08 22:40 ` Martin K. Petersen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).