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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
