phy: qcom-qmp: Fix dts bindings to reflect reality

Message ID 20180706233142.166296-1-dianders@chromium.org
State Changes Requested
Headers show
Series
  • phy: qcom-qmp: Fix dts bindings to reflect reality
Related show

Commit Message

Doug Anderson July 6, 2018, 11:31 p.m.
A few patches have landed for the qcom-qmp PHY that affect how you
would write a device tree node.  ...yet the bindings weren't updated.
Let's remedy the situation and make the bindings refelect reality.

Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Rob Herring July 20, 2018, 2:10 p.m. | #1
On Fri, Jul 06, 2018 at 04:31:42PM -0700, Douglas Anderson wrote:
> A few patches have landed for the qcom-qmp PHY that affect how you
> would write a device tree node.  ...yet the bindings weren't updated.
> Let's remedy the situation and make the bindings refelect reality.

"dt-bindings: phy: ..." for the subject.

> 
> Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
> Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> index 266a1bb8bb6e..0c7629e88bf3 100644
> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> @@ -12,7 +12,14 @@ Required properties:
>  	       "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
>  	       "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
>  
> - - reg: offset and length of register set for PHY's common serdes block.
> + - reg:
> +   - For "qcom,sdm845-qmp-usb3-phy":
> +     - index 0: address and length of register set for PHY's common serdes
> +       block.
> +     - named register "dp_com" (using reg-names): address and length of the
> +       DP_COM control block.

You need to list reg-names and what are the names for the other 2 
regions?

> +   - For all others:
> +     - offset and length of register set for PHY's common serdes block.
>  
>   - #clock-cells: must be 1
>      - Phy pll outputs a bunch of clocks for Tx, Rx and Pipe
> @@ -60,7 +67,10 @@ Required nodes:
>  
>  Required properties for child node:
>   - reg: list of offset and length pairs of register sets for PHY blocks -
> -	tx, rx and pcs.
> +	- index 0: tx
> +	- index 1: rx
> +	- index 2: pcs
> +	- index 3: pcs_misc (optional)
>  
>   - #phy-cells: must be 0
>  
> -- 
> 2.18.0.203.gfac676dfb9-goog
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Doug Anderson July 20, 2018, 3:13 p.m. | #2
Rob,

On Fri, Jul 20, 2018 at 7:10 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 06, 2018 at 04:31:42PM -0700, Douglas Anderson wrote:
>> A few patches have landed for the qcom-qmp PHY that affect how you
>> would write a device tree node.  ...yet the bindings weren't updated.
>> Let's remedy the situation and make the bindings refelect reality.
>
> "dt-bindings: phy: ..." for the subject.

Sorry.  Every subsystem has different conventions for this so I
usually just do a "git log" on the file and make my best guess.  I'll
try to remember this for next time though.

In this case, though, it looks like this already landed.  I see this
patch in Kishon's next branch.


>> Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
>> Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> index 266a1bb8bb6e..0c7629e88bf3 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> @@ -12,7 +12,14 @@ Required properties:
>>              "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
>>              "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
>>
>> - - reg: offset and length of register set for PHY's common serdes block.
>> + - reg:
>> +   - For "qcom,sdm845-qmp-usb3-phy":
>> +     - index 0: address and length of register set for PHY's common serdes
>> +       block.
>> +     - named register "dp_com" (using reg-names): address and length of the
>> +       DP_COM control block.
>
> You need to list reg-names and what are the names for the other 2
> regions?

Here's the code works.  You can tell me how you want this expressed in
the bindings:

* In all cases the driver maps its main memory range (for the common
serdes block) without specifying any name.  This is equivalent to
asking for index 0.

* For "qcom,sdm845-qmp-usb3-phy" the driver requests a second memory
range by name using the name "dp_com".

...basically the driver is inconsistent between using names and
indices and I was trying to document that fact.


I guess options:

1. I could reword this so it's clearer (open to suggestions)

2. I could add something to the bindings saying that the first reg
name should be "reg-base" or something.  Then the question is whether
we should go to the code and start enforcing that.  If we do choose to
enforce it then it's technically breaking compatibility (though I
doubt there is any real dts in the wild).  If we don't choose to
enforce it then why did we bother saying what it should be named?


Anyway, let me know.  As I said above, Kishon already applied this
change but I'm happy to post a followup.


-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring July 20, 2018, 5:26 p.m. | #3
On Fri, Jul 20, 2018 at 9:13 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Rob,
>
> On Fri, Jul 20, 2018 at 7:10 AM, Rob Herring <robh@kernel.org> wrote:
> > On Fri, Jul 06, 2018 at 04:31:42PM -0700, Douglas Anderson wrote:
> >> A few patches have landed for the qcom-qmp PHY that affect how you
> >> would write a device tree node.  ...yet the bindings weren't updated.
> >> Let's remedy the situation and make the bindings refelect reality.
> >
> > "dt-bindings: phy: ..." for the subject.
>
> Sorry.  Every subsystem has different conventions for this so I
> usually just do a "git log" on the file and make my best guess.  I'll
> try to remember this for next time though.

NP. I'd like to add this info to MAINTAINERS or maybe a git commit
hook could figure this out automagically.

> In this case, though, it looks like this already landed.  I see this
> patch in Kishon's next branch.
>
>
> >> Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
> >> Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >>
> >>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 14 ++++++++++++--
> >>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> >> index 266a1bb8bb6e..0c7629e88bf3 100644
> >> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> >> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
> >> @@ -12,7 +12,14 @@ Required properties:
> >>              "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
> >>              "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
> >>
> >> - - reg: offset and length of register set for PHY's common serdes block.
> >> + - reg:
> >> +   - For "qcom,sdm845-qmp-usb3-phy":
> >> +     - index 0: address and length of register set for PHY's common serdes
> >> +       block.
> >> +     - named register "dp_com" (using reg-names): address and length of the
> >> +       DP_COM control block.
> >
> > You need to list reg-names and what are the names for the other 2
> > regions?
>
> Here's the code works.  You can tell me how you want this expressed in
> the bindings:
>
> * In all cases the driver maps its main memory range (for the common
> serdes block) without specifying any name.  This is equivalent to
> asking for index 0.
>
> * For "qcom,sdm845-qmp-usb3-phy" the driver requests a second memory
> range by name using the name "dp_com".
>
> ...basically the driver is inconsistent between using names and
> indices and I was trying to document that fact.

That's fine as long as the indices are fixed.

>
> I guess options:
>
> 1. I could reword this so it's clearer (open to suggestions)
>
> 2. I could add something to the bindings saying that the first reg
> name should be "reg-base" or something.  Then the question is whether
> we should go to the code and start enforcing that.  If we do choose to
> enforce it then it's technically breaking compatibility (though I
> doubt there is any real dts in the wild).  If we don't choose to
> enforce it then why did we bother saying what it should be named?

I think you need to state index 1 is dp_com (and only for
"qcom,sdm845-qmp-usb3-phy") and a name for index 0. 'reg-base' doesn't
seem great, but I don't have another suggestion.

For the driver, it's not the driver's job to enforce any of this.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Doug Anderson July 20, 2018, 5:54 p.m. | #4
Hi,

On Fri, Jul 20, 2018 at 10:26 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 20, 2018 at 9:13 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> Rob,
>>
>> On Fri, Jul 20, 2018 at 7:10 AM, Rob Herring <robh@kernel.org> wrote:
>> > On Fri, Jul 06, 2018 at 04:31:42PM -0700, Douglas Anderson wrote:
>> >> A few patches have landed for the qcom-qmp PHY that affect how you
>> >> would write a device tree node.  ...yet the bindings weren't updated.
>> >> Let's remedy the situation and make the bindings refelect reality.
>> >
>> > "dt-bindings: phy: ..." for the subject.
>>
>> Sorry.  Every subsystem has different conventions for this so I
>> usually just do a "git log" on the file and make my best guess.  I'll
>> try to remember this for next time though.
>
> NP. I'd like to add this info to MAINTAINERS or maybe a git commit
> hook could figure this out automagically.
>
>> In this case, though, it looks like this already landed.  I see this
>> patch in Kishon's next branch.
>>
>>
>> >> Fixes: efb05a50c956 ("phy: qcom-qmp: Add support for QMP V3 USB3 PHY")
>> >> Fixes: ac0d239936bd ("phy: qcom-qmp: Add support for runtime PM")
>> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> >> ---
>> >>
>> >>  .../devicetree/bindings/phy/qcom-qmp-phy.txt       | 14 ++++++++++++--
>> >>  1 file changed, 12 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> index 266a1bb8bb6e..0c7629e88bf3 100644
>> >> --- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> +++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
>> >> @@ -12,7 +12,14 @@ Required properties:
>> >>              "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
>> >>              "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
>> >>
>> >> - - reg: offset and length of register set for PHY's common serdes block.
>> >> + - reg:
>> >> +   - For "qcom,sdm845-qmp-usb3-phy":
>> >> +     - index 0: address and length of register set for PHY's common serdes
>> >> +       block.
>> >> +     - named register "dp_com" (using reg-names): address and length of the
>> >> +       DP_COM control block.
>> >
>> > You need to list reg-names and what are the names for the other 2
>> > regions?
>>
>> Here's the code works.  You can tell me how you want this expressed in
>> the bindings:
>>
>> * In all cases the driver maps its main memory range (for the common
>> serdes block) without specifying any name.  This is equivalent to
>> asking for index 0.
>>
>> * For "qcom,sdm845-qmp-usb3-phy" the driver requests a second memory
>> range by name using the name "dp_com".
>>
>> ...basically the driver is inconsistent between using names and
>> indices and I was trying to document that fact.
>
> That's fine as long as the indices are fixed.
>
>>
>> I guess options:
>>
>> 1. I could reword this so it's clearer (open to suggestions)
>>
>> 2. I could add something to the bindings saying that the first reg
>> name should be "reg-base" or something.  Then the question is whether
>> we should go to the code and start enforcing that.  If we do choose to
>> enforce it then it's technically breaking compatibility (though I
>> doubt there is any real dts in the wild).  If we don't choose to
>> enforce it then why did we bother saying what it should be named?
>
> I think you need to state index 1 is dp_com (and only for
> "qcom,sdm845-qmp-usb3-phy") and a name for index 0. 'reg-base' doesn't
> seem great, but I don't have another suggestion.

...but why do we bother giving "dp_com" a name if we're saying it has
to be index 1?  It feels like the author of the driver was trying to
transition from specifying to specifying registers by index to
specifying them by name, but left the first register specified by
index for compatibility (or code simplicity?).  It seems like the
whole point of referring to things by name is to _not_ force the index
number.

I'm imagining a future compatible string that adds yet another address
range.  Presumably we'd refer to this by name too.  In that case both
of these would be fine:

reg = <0xa x>, <0xb x>, <0xc x>;
reg-names = "reg-base", "dp_com", "new_range"

reg = <0xa x>, <0xc x>, <0xb x>;
reg-names = "reg-base", "new_range", "dp_com"


> For the driver, it's not the driver's job to enforce any of this.

Sure, but anything that the driver doesn't enforce people will get
wrong.  In other words if we say that the name of the first register
ought to be "reg-base" but don't enforce it then someone will later
come in and say it's stupid that one of the names uses a dash and the
other an underscore.  They'll change "reg-base" and it will work.
Someone else will decide that "reg-base" is a dumb name and will
change it to "serdes".  It will still work.  Now we have a bindings
that claims "reg-base" and lots of different device trees in practice.

If the whole "keep things compatible" is important then what matters
more is what's happening in practice, not what's should happen in
theory.

IMO if the driver isn't enforcing that the first field be called
"reg-base" then it shouldn't be in the bindings doc.


BTW: I use the name "reg-base" in my example because that's what the
register was named in downstream device trees that I found.  Even if
the name isn't terribly appealing, if it's not terribly objectionable
I'd rather pick something that's closer to what people used in
practice.

---

How about this?

- reg:
  - For "qcom,sdm845-qmp-usb3-phy" (names mapped by reg-names):
    - "reg-base": address and length of register set for PHY's common serdes
      block.  MUST BE THE FIRST RANGE LISTED IN THE LIST (AKA index 0).  Note
      that the name of "reg-base" is suggested but not enforced.
    - "dp-com": address and length of the DP_COM control block.
  - For all others (only one range--don't use reg-names)
    - offset and length of register set for PHY's common serdes block.



-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index 266a1bb8bb6e..0c7629e88bf3 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -12,7 +12,14 @@  Required properties:
 	       "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
 	       "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
 
- - reg: offset and length of register set for PHY's common serdes block.
+ - reg:
+   - For "qcom,sdm845-qmp-usb3-phy":
+     - index 0: address and length of register set for PHY's common serdes
+       block.
+     - named register "dp_com" (using reg-names): address and length of the
+       DP_COM control block.
+   - For all others:
+     - offset and length of register set for PHY's common serdes block.
 
  - #clock-cells: must be 1
     - Phy pll outputs a bunch of clocks for Tx, Rx and Pipe
@@ -60,7 +67,10 @@  Required nodes:
 
 Required properties for child node:
  - reg: list of offset and length pairs of register sets for PHY blocks -
-	tx, rx and pcs.
+	- index 0: tx
+	- index 1: rx
+	- index 2: pcs
+	- index 3: pcs_misc (optional)
 
  - #phy-cells: must be 0