[PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Douglas Anderson]

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(-)

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
 
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Rob Herring]

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
> 

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Doug Anderson]

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

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Rob Herring]

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

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Doug Anderson]

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

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Rob Herring]

On Fri, Jul 20, 2018 at 11:54 AM Doug Anderson <dianders@chromium.org> wrote:
>
> 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.

No. Specifying the order and indexes is how bindings are done.
"-names" is extra information, not a license to change the rules.

> 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"

But why. There is absolutely no need to support both of these.

What you could need to support is this:

reg-names = "reg-base", "new_range";

The order is still fixed, but we have optional entries in the middle.
And that is the case when using -names is helpful. Otherwise, "-names"
is just unnecessary bloat to DT.

> > 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.

You are arguing against making the binding stricter and then worrying
about the driver getting things wrong. Stricter bindings leave less to
interpretation by the driver and less chance to get things wrong.
Unless "anything goes" and then the binding can never be "wrong".

Again, it is not the kernel's job to validate bindings even though we
have little else right now. That will change soon hopefully.

> 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.

I don't really care. People just make shit up anyway.

>
> ---
>
> How about this?

No.

>
> - 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

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Doug Anderson]

Hi,

On Mon, Jul 23, 2018 at 7:04 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 20, 2018 at 11:54 AM Doug Anderson <dianders@chromium.org> wrote:
>>
>> 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.
>
> No. Specifying the order and indexes is how bindings are done.
> "-names" is extra information, not a license to change the rules.

OK.

Just for context: I'm not trying to be argumentative or anything--I
just seem to be lacking a fundamental understanding of why reg-names
exists and when it should be used.  I'm trying to ask questions so I
can have a better intuition here and submit better patches / do better
reviews.  I'm sorry if I'm coming off as a jerk.  :(  I'm not trying
to be.

Do you happen to know if there's anything written up that explains all
the conventions around reg-names and I can just read that?


For context, it seems to me that clocks and registers have different
conventions, but I certainly could be wrong.  From what I've seen
clocks are often specified in bindings by name and referred to in code
by name.  Conversely reg-names seems to be highly discouraged and
people are told to just refer to them by index.  The difference
between the two always seemed strange to me, though I always assumed
it was because it was more common to just have one or two register
ranges but a whole chunk of clocks (and more get added over time as
the driver matures).


>> 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"
>
> But why. There is absolutely no need to support both of these.

I guess I'm trying to understand the motivation for reg-names.  If you
say that something must be index 1 and also have reg-name of "dp_com"
then the reg-name isn't buying us anything and maybe the more proper
fix is to change the driver to work by index and drop the whole name
thing here?  Should I do that?


> What you could need to support is this:
>
> reg-names = "reg-base", "new_range";
>
> The order is still fixed, but we have optional entries in the middle.
> And that is the case when using -names is helpful. Otherwise, "-names"
> is just unnecessary bloat to DT.

>From what you're saying the _only_ reason you'd ever want to use
reg-names is if there is an optional register range.  Is that right?


I think for me intuitively I expect that drivers will change and
expand over time and I'd expect more / different (and maybe optional)
ranges to be added.  In general I feel like named registers scales
better with less complexity.


>> > 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.
>
> You are arguing against making the binding stricter and then worrying
> about the driver getting things wrong. Stricter bindings leave less to
> interpretation by the driver and less chance to get things wrong.
> Unless "anything goes" and then the binding can never be "wrong".

I guess I'm saying that my intuition says that over-specifying things
seems bad to me.  The driver will either ask for a register by name or
by index and not both.  In my mind either is a fine way to do it so as
long as the driver and the binding is consistent then we have a
functioning system.  Specifying that people have to do get both the
index and name right basically gives people twice as many places to do
things wrong.


> Again, it is not the kernel's job to validate bindings even though we
> have little else right now. That will change soon hopefully.
>
>> 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.
>
> I don't really care. People just make shit up anyway.
>
>>
>> ---
>>
>> How about this?
>
> No.
>
>>
>> - 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.

OK, I guess I will try again then and you can tell me when I get it
right (unless you tell me I should just change the driver to not use
named registers at all).  How about:

- reg:
  - For "qcom,sdm845-qmp-usb3-phy":
    - index 0: address and length of register set for PHY's common serdes
               block.
    - index 1: address and length of the DP_COM control block.
  - For all others:
    - index 0: address and length of register set for PHY's common serdes
               block.
- reg-names:
  - For "qcom,sdm845-qmp-usb3-phy":
    - Should be: "reg-base", "dp_com"
  - For all others:
    - The reg-names property shouldn't be defined.


-Doug

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Rob Herring]

On Mon, Jul 23, 2018 at 11:37 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jul 23, 2018 at 7:04 AM, Rob Herring <robh@kernel.org> wrote:
> > On Fri, Jul 20, 2018 at 11:54 AM Doug Anderson <dianders@chromium.org> wrote:
> >>
> >> 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.
> >
> > No. Specifying the order and indexes is how bindings are done.
> > "-names" is extra information, not a license to change the rules.
>
> OK.
>
> Just for context: I'm not trying to be argumentative or anything--I
> just seem to be lacking a fundamental understanding of why reg-names
> exists and when it should be used.  I'm trying to ask questions so I
> can have a better intuition here and submit better patches / do better
> reviews.  I'm sorry if I'm coming off as a jerk.  :(  I'm not trying
> to be.
>
> Do you happen to know if there's anything written up that explains all
> the conventions around reg-names and I can just read that?

Probably not. We should add it to the DT spec I guess.

> For context, it seems to me that clocks and registers have different
> conventions, but I certainly could be wrong.  From what I've seen
> clocks are often specified in bindings by name and referred to in code
> by name.  Conversely reg-names seems to be highly discouraged and
> people are told to just refer to them by index.  The difference
> between the two always seemed strange to me, though I always assumed
> it was because it was more common to just have one or two register
> ranges but a whole chunk of clocks (and more get added over time as
> the driver matures).

That probably is because the clock binding has had clock-names from
the start (it may have been the first one). That was probably partly
due to the clock API also was mainly by name already if we want to
admit Linux influence on bindings. IIRC, reg-names came along for the
OMAP folks and was related to the hwmods stuff. Part of it was also
people wanting platform_get_resource_by_name to work when converting
to DT. If you haven't figured it out, I'm still not really a fan. It
is definitely more useful for some bindings than others.

There's a few cases where I think *-names is useful. The first is what
I mentioned where you could have holes in the list. The 2nd is things
like the standardized "wakeup" name for wakeup interrupts.

I really don't like seeing things like reset-names with a single entry
like "rst".

> >> 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"
> >
> > But why. There is absolutely no need to support both of these.
>
> I guess I'm trying to understand the motivation for reg-names.  If you
> say that something must be index 1 and also have reg-name of "dp_com"
> then the reg-name isn't buying us anything and maybe the more proper
> fix is to change the driver to work by index and drop the whole name
> thing here?  Should I do that?

That would be fine by me.

> > What you could need to support is this:
> >
> > reg-names = "reg-base", "new_range";
> >
> > The order is still fixed, but we have optional entries in the middle.
> > And that is the case when using -names is helpful. Otherwise, "-names"
> > is just unnecessary bloat to DT.
>
> From what you're saying the _only_ reason you'd ever want to use
> reg-names is if there is an optional register range.  Is that right?

IMO, yes. But I'm not going to tell people to remove them unless there
is only a single entry.

> I think for me intuitively I expect that drivers will change and
> expand over time and I'd expect more / different (and maybe optional)
> ranges to be added.  In general I feel like named registers scales
> better with less complexity.

They should only really change for new h/w and that should imply a new
compatible which can also define what is at each index.

> >> > 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.
> >
> > You are arguing against making the binding stricter and then worrying
> > about the driver getting things wrong. Stricter bindings leave less to
> > interpretation by the driver and less chance to get things wrong.
> > Unless "anything goes" and then the binding can never be "wrong".
>
> I guess I'm saying that my intuition says that over-specifying things
> seems bad to me.  The driver will either ask for a register by name or
> by index and not both.  In my mind either is a fine way to do it so as
> long as the driver and the binding is consistent then we have a
> functioning system.  Specifying that people have to do get both the
> index and name right basically gives people twice as many places to do
> things wrong.

Validation should solve that problem mostly. The only thing we can't
validate is if the names are the right order, but the addresses are
not.

> > Again, it is not the kernel's job to validate bindings even though we
> > have little else right now. That will change soon hopefully.
> >
> >> 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.
> >
> > I don't really care. People just make shit up anyway.
> >
> >>
> >> ---
> >>
> >> How about this?
> >
> > No.
> >
> >>
> >> - 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.
>
> OK, I guess I will try again then and you can tell me when I get it
> right (unless you tell me I should just change the driver to not use
> named registers at all).  How about:
>
> - reg:
>   - For "qcom,sdm845-qmp-usb3-phy":
>     - index 0: address and length of register set for PHY's common serdes
>                block.
>     - index 1: address and length of the DP_COM control block.

Instead of repeating index 0 below, just say "for
'qcom,sdm845-qmp-usb3-phy' only" here.

>   - For all others:
>     - index 0: address and length of register set for PHY's common serdes
>                block.
> - reg-names:
>   - For "qcom,sdm845-qmp-usb3-phy":
>     - Should be: "reg-base", "dp_com"
>   - For all others:
>     - The reg-names property shouldn't be defined.
>
>
> -Doug

Re: [PATCH] phy: qcom-qmp: Fix dts bindings to reflect reality

[Doug Anderson]

Hi,

On Mon, Jul 23, 2018 at 4:33 PM, Rob Herring <robh@kernel.org> wrote:
>> OK, I guess I will try again then and you can tell me when I get it
>> right (unless you tell me I should just change the driver to not use
>> named registers at all).  How about:
>>
>> - reg:
>>   - For "qcom,sdm845-qmp-usb3-phy":
>>     - index 0: address and length of register set for PHY's common serdes
>>                block.
>>     - index 1: address and length of the DP_COM control block.
>
> Instead of repeating index 0 below, just say "for
> 'qcom,sdm845-qmp-usb3-phy' only" here.
>
>>   - For all others:
>>     - index 0: address and length of register set for PHY's common serdes
>>                block.
>> - reg-names:
>>   - For "qcom,sdm845-qmp-usb3-phy":
>>     - Should be: "reg-base", "dp_com"
>>   - For all others:
>>     - The reg-names property shouldn't be defined.

To close the loop, I've just posted
<https://lore.kernel.org/patchwork/patch/972177/> AKA ("dt-bindings:
phy: qcom-qmp: Cleanup the 'reg' documentation as per review").
Hopefully that addresses everything properly but if not I'm happy to
spin it again.  Thanks for your patience in helping me understand what
you were looking for.

-Doug