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@xxxxxxxxxx> wrote:
> On Fri, Jul 20, 2018 at 11:54 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On Fri, Jul 20, 2018 at 10:26 AM, Rob Herring <robh@xxxxxxxxxx> wrote:
>> > On Fri, Jul 20, 2018 at 9:13 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>> >>
>> >> Rob,
>> >>
>> >> On Fri, Jul 20, 2018 at 7:10 AM, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>> >> >> ---
>> >> >>
>> >> >>  .../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