LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Doug Anderson <email@example.com> To: Manu Gautam <firstname.lastname@example.org> Cc: Kishon Vijay Abraham I <email@example.com>, Rob Herring <firstname.lastname@example.org>, Stephen Boyd <email@example.com>, LKML <firstname.lastname@example.org>, email@example.com, Rob Herring <firstname.lastname@example.org>, Vivek Gautam <email@example.com>, Evan Green <firstname.lastname@example.org>, email@example.com, Mark Rutland <firstname.lastname@example.org> Subject: Re: [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values Date: Thu, 29 Mar 2018 13:38:23 -0700 [thread overview] Message-ID: <CAD=FV=VGtmWTaa_aRG6UVAtCzy3cS8sTZDCqAgRMYeg7U02qTw@mail.gmail.com> (raw) In-Reply-To: <email@example.com> Hi, On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <firstname.lastname@example.org> wrote: > To improve eye diagram for PHYs on different boards of same SOC, > some parameters may need to be changed. Provide device tree > properties to override these from board specific device tree > files. While at it, replace "qcom,qusb2-v2-phy" with compatible > string for USB2 PHY on sdm845 which was earlier added for > sdm845 only. > > Signed-off-by: Manu Gautam <email@example.com> > --- > .../devicetree/bindings/phy/qcom-qusb2-phy.txt | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > index 42c9742..0ed140a 100644 > --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > @@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets. > Required properties: > - compatible: compatible list, contains > "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996, > - "qcom,qusb2-v2-phy" for QUSB2 V2 PHY. > + "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845. > > - reg: offset and length of the PHY register set. > - #phy-cells: must be 0. > @@ -27,6 +27,23 @@ Optional properties: > tuning parameter value for qusb2 phy. > > - qcom,tcsr-syscon: Phandle to TCSR syscon register region. Just to confirm: the new properties below work just fine on the old msm8996 PHY too, right? > + - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to be > + added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY > + tuning paramter that may vary for different boards of same SOC. > + - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX > + output current. 0x0 value corresponding to 24mA which is maximum > + current and 0xf corresponds to lowest current which is 15mA. > + - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis level. > + Possible values are: > + 00: NONE > + 01: +5% > + 10: +10% > + 11: +15% The user of the device tree will expect to specify this in decimal, right? So list the above as 0, 1, 2, 3. ...not 00, 01, 10, 11. (though below I suggest that specifying 0, 1, 2, 3 is probably not quite the right way to describe this property). > +- qcom,preemphasis-width: It is a 1 bit value that specifies how long the HSTX > + pre-emphasis (specified using qcom,preemphasis-level) must be in > + effect. Possible values are: > + 0: Full-bit width > + 1: Half-bit width Perhaps just make this a boolean property. If it exists then you get the non-default case. AKA: if the default is full bit width, then you'd allow a boolean property "qcom,preemphasis-half-width" to override. If the default is half bit width then you'd allow "qcom,preemphasis-full-width" to override. For all the above optional bits, please indicate what the default is if they aren't specified. If you have to specify a different default for sdm845 vs. msm8996 then so be it (though it would be nice to avoid it by changing the default for sdm845 unless that's totally crazy). Overall this looks pretty good to me. Certainly the descriptions are very understandable and this will make tuning on a per-board / per-port basis much easier! Thanks! One last overall comment is that ideally we could make the device tree a little bit more human readable. Right now we'll have a set of properties that looks like this (numbers made up): qcom,imp-res-offset-value = <0x13>; qcom,hstx-trim-value = <0x7>; qcom,preemphasis-level = <1>; qcom,preemphasis-width = <0>; One option to make that more readable would be to change the units, AKA: qcom,hstx-trim-ma = <18>; qcom,preemphasis-percent = <5>; ...then the code would translate from these human-readable values to the real numbers. Another option would be to add a #include to the bindings. I'd defer to the wisdom of the bindings guys about if this is better or worse than adding the units, but personally I like it better because: * You get a compile-time error if you use an unsupported value. * You don't need to add the code to translate. So you'd add a ".h" file as part of this bindings patch (see git history for other examples) and you'd have stuff like: #define SDM845_QUSB2_TRIM_24MA 0 ... #define SDM845_QUSB2_TRIM_15MA 0xf #define SDM845_QUSB2_PREEMPHASIS_NONE 0 #define SDM845_QUSB2_PREEMPHASIS_5_PERCENT 1 #define SDM845_QUSB2_PREEMPHASIS_10_PERCENT 2 #define SDM845_QUSB2_PREEMPHASIS_15_PERCENT 3 ...then you'd use those #defines in the description of the properties. -- One last note: from the current code <https://patchwork.kernel.org/patch/10314925/> all of these new properties need to be specified as 8-bit numbers, not 32-bit. AKA: qcom,imp-res-offset-value = /bits/ 8 <0x13>; qcom,hstx-trim-value = /bits/ 8 <0x7>; qcom,preemphasis-level = /bits/ 8 <1>; qcom,preemphasis-width = /bits/ 8 <0>; If we decide to keep it that way then it needs to be documented in the bindings file. IMHO we should just specify that these are normal (32-bit) properties even if we expect that they'll always be 8-bits. That seems to be commonplace elsewhere. -Doug
next prev parent reply other threads:[~2018-03-29 20:38 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-29 11:04 [PATCH v4 0/7] phy: qcom: Updates for USB PHYs on SDM845 Manu Gautam 2018-03-29 11:04 ` [PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk Manu Gautam 2018-03-29 20:55 ` Doug Anderson 2018-04-05 20:07 ` Stephen Boyd 2018-04-10 6:44 ` Manu Gautam 2018-03-29 11:04 ` [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization Manu Gautam 2018-03-29 17:28 ` Evan Green 2018-03-29 18:44 ` Doug Anderson 2018-03-29 20:54 ` Doug Anderson 2018-04-10 6:36 ` Manu Gautam 2018-04-10 15:05 ` Doug Anderson 2018-04-10 18:32 ` Stephen Boyd 2018-04-11 15:37 ` Manu Gautam 2018-04-12 20:38 ` Stephen Boyd 2018-04-13 7:00 ` Manu Gautam 2018-03-29 11:04 ` [PATCH v4 3/7] phy: qcom-qusb2: Fix crash if nvmem cell not specified Manu Gautam 2018-03-29 11:04 ` [PATCH v4 4/7] dt-bindings: phy-qcom-qmp: Update bindings for sdm845 Manu Gautam 2018-03-29 19:39 ` Doug Anderson 2018-03-29 11:04 ` [PATCH v4 5/7] phy: qcom-qmp: Add QMP V3 USB3 UNI PHY support " Manu Gautam 2018-03-29 11:04 ` [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values Manu Gautam 2018-03-29 20:38 ` Doug Anderson [this message] 2018-04-09 20:18 ` Rob Herring 2018-04-10 7:16 ` Manu Gautam 2018-04-12 20:47 ` Doug Anderson 2018-04-16 1:36 ` Manu Gautam 2018-03-29 11:04 ` [PATCH v4 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845 Manu Gautam 2018-03-29 20:38 ` Doug Anderson 2018-04-10 6:55 ` Manu Gautam
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAD=FV=VGtmWTaa_aRG6UVAtCzy3cS8sTZDCqAgRMYeg7U02qTw@mail.gmail.com' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).