Hi Krzysztof, On Thu, Mar 03, 2022 at 04:59:22PM +0100, Krzysztof Kozlowski wrote: > On 03/03/2022 07:13, Sandeep Maheswaram wrote: > > Add device tree bindings for SNPS phy tuning parameters. > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@xxxxxxxxxxx> > > --- > > .../bindings/phy/qcom,usb-snps-femto-v2.yaml | 125 +++++++++++++++++++++ > > 1 file changed, 125 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml > > index 0dfe691..227c097 100644 > > --- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml > > +++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml > > @@ -50,6 +50,131 @@ properties: > > vdda33-supply: > > description: phandle to the regulator 3.3V supply node. > > > > + qcom,hs-disconnect: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + This adjusts the voltage level for the threshold used to > > + detect a disconnect event at the host. Possible values are. > > ':', instead of full stop. > > > + 7 -> +21.56% > > + 6 -> +17.43% > > + 5 -> +13.32% > > + 4 -> +9.73% > > + 3 -> +6.3 > > + 2 -> +3.17% > > + 1 -> 0, Design default% > > Use "default:" instead. Here and in other places. > > > + 0 -> -2.72% > > In current form this should be an enum... but actually current form is > wrong. You should not store register values in DT. What if next version > of hardware has a different meaning of these values? > > Instead, you should store here meaningful values, not register values. > Thanks for the feedback. The values in % really makes the tuning easy. People look at the eye diagram and decided whether to increase/decrease the margin. The absolute values may not be that useful. All we need is an "adjustment" here. The databook it self does not give any absolute values. I agree to the "enum" suggestion which we have been following for the qusb2 driver already. The values have not changed in the last 5 years for this hardware block, so defining enums for the % values would be really helpful. > > > + > > + qcom,squelch-detector: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + This adjusts the voltage level for the threshold used to > > + detect valid high-speed data. Possible values are > > + 7-> -20.90% > > + 6-> -15.60% > > + 5-> -10.30% > > + 4-> -5.30% > > + 3-> 0, Design default% > > + 2-> +5.30% > > + 1-> +10.60% > > + 0-> +15.90% > > + > > + qcom,hs-amplitude: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + This adjusts the high-speed DC level voltage. > > + Possible values are > > + 15-> +26.70% > > + 14-> +24.30% > > + 13-> +22.20% > > + 12-> +20.00% > > + 11-> +17.80% > > + 10-> +15.60% > > + 9-> +13.30% > > + 8-> +11.10% > > + 7-> +8.90% > > + 6-> +6.50% > > + 5-> +4.40% > > + 4-> +2.30% > > + 3-> 0, Design default% > > + 2-> -2.20% > > + 1-> -4.40% > > + 0-> -6.60% > > + > > + qcom,pre-emphasis-duration: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + This signal controls the duration for which the > > + HS pre-emphasis current is sourced onto DP<#> or DM<#>. > > + The HS Transmitter pre-emphasis duration is defined in terms of > > + unit amounts. One unit of pre-emphasis duration is approximately > > + 650 ps and is defined as 1X pre-emphasis duration. > > + Possible values are > > + 1-> 1x, short pre-emphasis current duration > > + 0-> 2x, long pre-emphasis current duration > > I could understand encoding of percentages in way of register value, but > a boolean flag is too much. > Agreed. This needs to be encoded in % as well (100% or 200%). Thanks, Pavan
