* [PATCH v2 0/3] Add QCOM SNPS PHY overriding params support
@ 2022-03-03 6:13 ` Sandeep Maheswaram
0 siblings, 0 replies; 36+ messages in thread
From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw)
To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I,
Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd,
Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski
Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb,
quic_pkondeti, quic_ppratap, Sandeep Maheswaram
Added support for overriding tuning parameters in QCOM SNPS PHY
from device tree.
changes in v2:
Reading the individual fields in each overriding register from device tree.
Sandeep Maheswaram (3):
dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params
bindings
phy: qcom-snps: Add support for overriding phy tuning parameters
arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device
.../bindings/phy/qcom,usb-snps-femto-v2.yaml | 125 ++++++++++++++
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 +
drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 +++++++++++++++++++++
3 files changed, 323 insertions(+)
--
2.7.4
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH v2 0/3] Add QCOM SNPS PHY overriding params support @ 2022-03-03 6:13 ` Sandeep Maheswaram 0 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Added support for overriding tuning parameters in QCOM SNPS PHY from device tree. changes in v2: Reading the individual fields in each overriding register from device tree. Sandeep Maheswaram (3): dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings phy: qcom-snps: Add support for overriding phy tuning parameters arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device .../bindings/phy/qcom,usb-snps-femto-v2.yaml | 125 ++++++++++++++ arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 + drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 +++++++++++++++++++++ 3 files changed, 323 insertions(+) -- 2.7.4 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-03 6:13 ` Sandeep Maheswaram @ 2022-03-03 6:13 ` Sandeep Maheswaram -1 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Add device tree bindings for SNPS phy tuning parameters. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- .../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. + 7 -> +21.56% + 6 -> +17.43% + 5 -> +13.32% + 4 -> +9.73% + 3 -> +6.3 + 2 -> +3.17% + 1 -> 0, Design default% + 0 -> -2.72% + + 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 + + qcom,pre-emphasis-amplitude: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This signal controls the amount of current sourced to + DP<#> and DM<#> after a J-to-K or K-to-J transition. + The HS Transmitter pre-emphasis current is defined in terms of unit + amounts. One unit amount is approximately 2 mA and is defined as + 1X pre-emphasis current. + Possible values are + 3-> HS Transmitter pre-emphasis circuit sources 3x pre-emphasis + current. + 2-> (design default) HS Transmitter pre-emphasis circuit sources 2x + pre-emphasis current. + 1-> HS Transmitter pre-emphasis circuit sources 1x pre-emphasis + current. + 0-> HS Transmitter pre-emphasis circuit sources 4x pre-emphasis + current. + + qcom,hs-rise-fall-time: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This adjusts the rise/fall times of the high-speed waveform. + Possible values are + 3-> -41.0% + 2-> 0, Design default + 1-> +28.1 + 0-> +54.3% + + qcom,hs-crossover-voltage: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This adjusts the voltage at which the DP<#> and DM<#> + signals cross while transmitting in HS mode. + Possible values are + 3-> 0, Default setting + 2-> +28 mV + 1-> -31 mV + 0-> Reserved + + qcom,hs-output-impedance: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + In some applications, there can be significant series resistance + on the D+ and D- paths between the transceiver and cable. This adjusts + the driver source impedance to compensate for added series + resistance on the USB. + 3-> Source impedance is decreased by approximately 2.3 ohms + 2-> 0, Design default + 1-> Source impedance is increased by approximately 2.6 ohms + 0-> Source impedance is increased by approximately 6.1 ohms + + qcom,ls-fs-output-impedance: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This adjusts the low- and full-speed single-ended source + impedance while driving high. The following adjustment values are based + on nominal process, voltage, and temperature. + 15-> -10.53% + 7-> -5.57% + 3-> 0, Design default + 1-> +6.12% + 0-> +13.10% + required: - compatible - reg -- 2.7.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-03 6:13 ` Sandeep Maheswaram 0 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Add device tree bindings for SNPS phy tuning parameters. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- .../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. + 7 -> +21.56% + 6 -> +17.43% + 5 -> +13.32% + 4 -> +9.73% + 3 -> +6.3 + 2 -> +3.17% + 1 -> 0, Design default% + 0 -> -2.72% + + 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 + + qcom,pre-emphasis-amplitude: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This signal controls the amount of current sourced to + DP<#> and DM<#> after a J-to-K or K-to-J transition. + The HS Transmitter pre-emphasis current is defined in terms of unit + amounts. One unit amount is approximately 2 mA and is defined as + 1X pre-emphasis current. + Possible values are + 3-> HS Transmitter pre-emphasis circuit sources 3x pre-emphasis + current. + 2-> (design default) HS Transmitter pre-emphasis circuit sources 2x + pre-emphasis current. + 1-> HS Transmitter pre-emphasis circuit sources 1x pre-emphasis + current. + 0-> HS Transmitter pre-emphasis circuit sources 4x pre-emphasis + current. + + qcom,hs-rise-fall-time: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This adjusts the rise/fall times of the high-speed waveform. + Possible values are + 3-> -41.0% + 2-> 0, Design default + 1-> +28.1 + 0-> +54.3% + + qcom,hs-crossover-voltage: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This adjusts the voltage at which the DP<#> and DM<#> + signals cross while transmitting in HS mode. + Possible values are + 3-> 0, Default setting + 2-> +28 mV + 1-> -31 mV + 0-> Reserved + + qcom,hs-output-impedance: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + In some applications, there can be significant series resistance + on the D+ and D- paths between the transceiver and cable. This adjusts + the driver source impedance to compensate for added series + resistance on the USB. + 3-> Source impedance is decreased by approximately 2.3 ohms + 2-> 0, Design default + 1-> Source impedance is increased by approximately 2.6 ohms + 0-> Source impedance is increased by approximately 6.1 ohms + + qcom,ls-fs-output-impedance: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + This adjusts the low- and full-speed single-ended source + impedance while driving high. The following adjustment values are based + on nominal process, voltage, and temperature. + 15-> -10.53% + 7-> -5.57% + 3-> 0, Design default + 1-> +6.12% + 0-> +13.10% + required: - compatible - reg -- 2.7.4 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-03 6:13 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Sandeep Maheswaram @ 2022-03-03 15:59 ` Krzysztof Kozlowski -1 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-03 15:59 UTC (permalink / raw) To: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap 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@quicinc.com> > --- > .../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. This applies to all cases below. > + > + 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. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-03 15:59 ` Krzysztof Kozlowski 0 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-03 15:59 UTC (permalink / raw) To: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap 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@quicinc.com> > --- > .../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. This applies to all cases below. > + > + 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. Best regards, Krzysztof -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-03 15:59 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski @ 2022-03-03 22:23 ` Stephen Boyd -1 siblings, 0 replies; 36+ messages in thread From: Stephen Boyd @ 2022-03-03 22:23 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Doug Anderson, Greg Kroah-Hartman, Kishon Vijay Abraham I, Krzysztof Kozlowski, Matthias Kaehlcke, Rob Herring, Sandeep Maheswaram, Vinod Koul, Wesley Cheng Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap Quoting Krzysztof Kozlowski (2022-03-03 07:59:22) > 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@quicinc.com> > > --- > > .../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. +1 To emphasize one point, meaningful values typically have a unit of measure, like Hz, ms, mV, etc. What are the percentages adjusting from? Is it a percentage decrease from the input voltage? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-03 22:23 ` Stephen Boyd 0 siblings, 0 replies; 36+ messages in thread From: Stephen Boyd @ 2022-03-03 22:23 UTC (permalink / raw) To: Andy Gross, Bjorn Andersson, Doug Anderson, Greg Kroah-Hartman, Kishon Vijay Abraham I, Krzysztof Kozlowski, Matthias Kaehlcke, Rob Herring, Sandeep Maheswaram, Vinod Koul, Wesley Cheng Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap Quoting Krzysztof Kozlowski (2022-03-03 07:59:22) > 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@quicinc.com> > > --- > > .../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. +1 To emphasize one point, meaningful values typically have a unit of measure, like Hz, ms, mV, etc. What are the percentages adjusting from? Is it a percentage decrease from the input voltage? -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-03 15:59 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski @ 2022-03-14 3:29 ` Pavan Kondeti -1 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 3:29 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, quic_kriskura 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@quicinc.com> > > --- > > .../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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 3:29 ` Pavan Kondeti 0 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 3:29 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, quic_kriskura 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@quicinc.com> > > --- > > .../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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 3:29 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti @ 2022-03-14 7:39 ` Krzysztof Kozlowski -1 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 7:39 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 04:29, Pavan Kondeti wrote: > 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@quicinc.com> >>> --- >>> .../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. I did not say you cannot store here percentages. Quite opposite - store here the percentages. Just do not store register value. No. Please read my comment again - meaningful values are needed. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 7:39 ` Krzysztof Kozlowski 0 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 7:39 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 04:29, Pavan Kondeti wrote: > 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@quicinc.com> >>> --- >>> .../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. I did not say you cannot store here percentages. Quite opposite - store here the percentages. Just do not store register value. No. Please read my comment again - meaningful values are needed. Best regards, Krzysztof -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 7:39 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski @ 2022-03-14 8:16 ` Pavan Kondeti -1 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 8:16 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 04:29, Pavan Kondeti wrote: > > 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@quicinc.com> > >>> --- > >>> .../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. > > I did not say you cannot store here percentages. Quite opposite - store > here the percentages. Just do not store register value. No. Please read > my comment again - meaningful values are needed. > IIUC, you are asking us to come up with a meaningful values to encode the percentage values. However, all the % increments are not linear, so we can't come up with {min, max} scheme. Lets take an example of hostdisconnect threshold. As per the data book, + 7 -> +21.56% + 6 -> +17.43% + 5 -> +13.32% + 4 -> +9.73% + 3 -> +6.3 + 2 -> +3.17% + 1 -> 0, Design default% + 0 -> -2.72% so how do we give meaningful values here? Does the below scheme make sense to you? #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 .. .. In the driver, we have a mapping (which can be per SoC if required in future) that takes these values and convert to the correct values for a given register. Thanks, Pavan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 8:16 ` Pavan Kondeti 0 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 8:16 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 04:29, Pavan Kondeti wrote: > > 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@quicinc.com> > >>> --- > >>> .../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. > > I did not say you cannot store here percentages. Quite opposite - store > here the percentages. Just do not store register value. No. Please read > my comment again - meaningful values are needed. > IIUC, you are asking us to come up with a meaningful values to encode the percentage values. However, all the % increments are not linear, so we can't come up with {min, max} scheme. Lets take an example of hostdisconnect threshold. As per the data book, + 7 -> +21.56% + 6 -> +17.43% + 5 -> +13.32% + 4 -> +9.73% + 3 -> +6.3 + 2 -> +3.17% + 1 -> 0, Design default% + 0 -> -2.72% so how do we give meaningful values here? Does the below scheme make sense to you? #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 .. .. In the driver, we have a mapping (which can be per SoC if required in future) that takes these values and convert to the correct values for a given register. Thanks, Pavan -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 8:16 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti @ 2022-03-14 8:36 ` Krzysztof Kozlowski -1 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 8:36 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 09:16, Pavan Kondeti wrote: > Hi Krzysztof, > > On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 04:29, Pavan Kondeti wrote: >>> 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@quicinc.com> >>>>> --- >>>>> .../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. >> >> I did not say you cannot store here percentages. Quite opposite - store >> here the percentages. Just do not store register value. No. Please read >> my comment again - meaningful values are needed. >> > > IIUC, you are asking us to come up with a meaningful values to encode the > percentage values. However, all the % increments are not linear, so we can't > come up with {min, max} scheme. Lets take an example of hostdisconnect > threshold. > > As per the data book, > > + 7 -> +21.56% > + 6 -> +17.43% > + 5 -> +13.32% > + 4 -> +9.73% > + 3 -> +6.3 > + 2 -> +3.17% > + 1 -> 0, Design default% > + 0 -> -2.72% > > so how do we give meaningful values here? Does the below scheme make sense > to you? By "meaningful value" I mean something which has a understandable meaning to reader of this code or to hardware designer. For example percentage values or some units (ms, ns, Hz, mA, mV). The value used in register is not meaningful in that way to us because it has a meaning only to the hardware block. Storing register values is more difficult to read later, non-portable and non-scalable. > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 This is some define in driver, does not look related to bindings. > In the driver, we have a mapping (which can be per SoC if required in future) > that takes these values and convert to the correct values for a given > register. You focus on driver but I am talking here only about bindings. What could be the meaningful value? Percentage could work. You have there a negative value, so I wonder what type of percentage is it? What is the formula? Your defines above look absolute, so maybe encode there absolute uV value? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 8:36 ` Krzysztof Kozlowski 0 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 8:36 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 09:16, Pavan Kondeti wrote: > Hi Krzysztof, > > On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 04:29, Pavan Kondeti wrote: >>> 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@quicinc.com> >>>>> --- >>>>> .../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. >> >> I did not say you cannot store here percentages. Quite opposite - store >> here the percentages. Just do not store register value. No. Please read >> my comment again - meaningful values are needed. >> > > IIUC, you are asking us to come up with a meaningful values to encode the > percentage values. However, all the % increments are not linear, so we can't > come up with {min, max} scheme. Lets take an example of hostdisconnect > threshold. > > As per the data book, > > + 7 -> +21.56% > + 6 -> +17.43% > + 5 -> +13.32% > + 4 -> +9.73% > + 3 -> +6.3 > + 2 -> +3.17% > + 1 -> 0, Design default% > + 0 -> -2.72% > > so how do we give meaningful values here? Does the below scheme make sense > to you? By "meaningful value" I mean something which has a understandable meaning to reader of this code or to hardware designer. For example percentage values or some units (ms, ns, Hz, mA, mV). The value used in register is not meaningful in that way to us because it has a meaning only to the hardware block. Storing register values is more difficult to read later, non-portable and non-scalable. > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 This is some define in driver, does not look related to bindings. > In the driver, we have a mapping (which can be per SoC if required in future) > that takes these values and convert to the correct values for a given > register. You focus on driver but I am talking here only about bindings. What could be the meaningful value? Percentage could work. You have there a negative value, so I wonder what type of percentage is it? What is the formula? Your defines above look absolute, so maybe encode there absolute uV value? Best regards, Krzysztof -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 8:36 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski @ 2022-03-14 9:40 ` Pavan Kondeti -1 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 9:40 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, Thanks for your suggestions and guidance on this. On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 09:16, Pavan Kondeti wrote: > > Hi Krzysztof, > > > > On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: > >> On 14/03/2022 04:29, Pavan Kondeti wrote: > >>> 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@quicinc.com> > >>>>> --- > >>>>> .../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. > >> > >> I did not say you cannot store here percentages. Quite opposite - store > >> here the percentages. Just do not store register value. No. Please read > >> my comment again - meaningful values are needed. > >> > > > > IIUC, you are asking us to come up with a meaningful values to encode the > > percentage values. However, all the % increments are not linear, so we can't > > come up with {min, max} scheme. Lets take an example of hostdisconnect > > threshold. > > > > As per the data book, > > > > + 7 -> +21.56% > > + 6 -> +17.43% > > + 5 -> +13.32% > > + 4 -> +9.73% > > + 3 -> +6.3 > > + 2 -> +3.17% > > + 1 -> 0, Design default% > > + 0 -> -2.72% > > > > so how do we give meaningful values here? Does the below scheme make sense > > to you? > > By "meaningful value" I mean something which has a understandable > meaning to reader of this code or to hardware designer. For example > percentage values or some units (ms, ns, Hz, mA, mV). The value used in > register is not meaningful in that way to us because it has a meaning > only to the hardware block. Storing register values is more difficult to > read later, non-portable and non-scalable. > > > > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 > > This is some define in driver, does not look related to bindings. > > > In the driver, we have a mapping (which can be per SoC if required in future) > > that takes these values and convert to the correct values for a given > > register. > > You focus on driver but I am talking here only about bindings. I was saying we define those defines in include/dt-bindings/phy/... header and use it in the device tree and as well in the driver. > > What could be the meaningful value? Percentage could work. You have > there a negative value, so I wonder what type of percentage is it? What > is the formula? I just multiplied by 100 since device tree has no support for floating (as per my knowledge). The negative value represents it lowers the disconnect threshold by 2.72% of the default value. if it makes sense, we could also start from 0 like below. #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 The driver can have a table to map these bindings. This looks much better than those x100 formula values. > Your defines above look absolute, so maybe encode there absolute uV value? Like I said, the data book it self does not give any absolute values. Since pct values are more useful in electrical compliance tuning, better to stick to pct values with proper encodings. Thanks, Pavan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 9:40 ` Pavan Kondeti 0 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 9:40 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, Thanks for your suggestions and guidance on this. On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 09:16, Pavan Kondeti wrote: > > Hi Krzysztof, > > > > On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: > >> On 14/03/2022 04:29, Pavan Kondeti wrote: > >>> 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@quicinc.com> > >>>>> --- > >>>>> .../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. > >> > >> I did not say you cannot store here percentages. Quite opposite - store > >> here the percentages. Just do not store register value. No. Please read > >> my comment again - meaningful values are needed. > >> > > > > IIUC, you are asking us to come up with a meaningful values to encode the > > percentage values. However, all the % increments are not linear, so we can't > > come up with {min, max} scheme. Lets take an example of hostdisconnect > > threshold. > > > > As per the data book, > > > > + 7 -> +21.56% > > + 6 -> +17.43% > > + 5 -> +13.32% > > + 4 -> +9.73% > > + 3 -> +6.3 > > + 2 -> +3.17% > > + 1 -> 0, Design default% > > + 0 -> -2.72% > > > > so how do we give meaningful values here? Does the below scheme make sense > > to you? > > By "meaningful value" I mean something which has a understandable > meaning to reader of this code or to hardware designer. For example > percentage values or some units (ms, ns, Hz, mA, mV). The value used in > register is not meaningful in that way to us because it has a meaning > only to the hardware block. Storing register values is more difficult to > read later, non-portable and non-scalable. > > > > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 > > This is some define in driver, does not look related to bindings. > > > In the driver, we have a mapping (which can be per SoC if required in future) > > that takes these values and convert to the correct values for a given > > register. > > You focus on driver but I am talking here only about bindings. I was saying we define those defines in include/dt-bindings/phy/... header and use it in the device tree and as well in the driver. > > What could be the meaningful value? Percentage could work. You have > there a negative value, so I wonder what type of percentage is it? What > is the formula? I just multiplied by 100 since device tree has no support for floating (as per my knowledge). The negative value represents it lowers the disconnect threshold by 2.72% of the default value. if it makes sense, we could also start from 0 like below. #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 The driver can have a table to map these bindings. This looks much better than those x100 formula values. > Your defines above look absolute, so maybe encode there absolute uV value? Like I said, the data book it self does not give any absolute values. Since pct values are more useful in electrical compliance tuning, better to stick to pct values with proper encodings. Thanks, Pavan -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 9:40 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti @ 2022-03-14 10:08 ` Krzysztof Kozlowski -1 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 10:08 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 10:40, Pavan Kondeti wrote: > Hi Krzysztof, > > Thanks for your suggestions and guidance on this. > > On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 09:16, Pavan Kondeti wrote: >>> Hi Krzysztof, >>> >>> On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: >>>> On 14/03/2022 04:29, Pavan Kondeti wrote: >>>>> 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@quicinc.com> >>>>>>> --- >>>>>>> .../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. >>>> >>>> I did not say you cannot store here percentages. Quite opposite - store >>>> here the percentages. Just do not store register value. No. Please read >>>> my comment again - meaningful values are needed. >>>> >>> >>> IIUC, you are asking us to come up with a meaningful values to encode the >>> percentage values. However, all the % increments are not linear, so we can't >>> come up with {min, max} scheme. Lets take an example of hostdisconnect >>> threshold. >>> >>> As per the data book, >>> >>> + 7 -> +21.56% >>> + 6 -> +17.43% >>> + 5 -> +13.32% >>> + 4 -> +9.73% >>> + 3 -> +6.3 >>> + 2 -> +3.17% >>> + 1 -> 0, Design default% >>> + 0 -> -2.72% >>> >>> so how do we give meaningful values here? Does the below scheme make sense >>> to you? >> >> By "meaningful value" I mean something which has a understandable >> meaning to reader of this code or to hardware designer. For example >> percentage values or some units (ms, ns, Hz, mA, mV). The value used in >> register is not meaningful in that way to us because it has a meaning >> only to the hardware block. Storing register values is more difficult to >> read later, non-portable and non-scalable. >> >>> >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 >> >> This is some define in driver, does not look related to bindings. >> >>> In the driver, we have a mapping (which can be per SoC if required in future) >>> that takes these values and convert to the correct values for a given >>> register. >> >> You focus on driver but I am talking here only about bindings. > > I was saying we define those defines in include/dt-bindings/phy/... header and > use it in the device tree and as well in the driver. Ah, I did not get it. That's not the solution for this case. defines in dt-bindings are for constants which already can be in DT, e.g. IDs. Your register values should not be stored in DT. > >> >> What could be the meaningful value? Percentage could work. You have >> there a negative value, so I wonder what type of percentage is it? What >> is the formula? > > I just multiplied by 100 since device tree has no support for floating (as per > my knowledge). The negative value represents it lowers the disconnect > threshold by 2.72% of the default value. if it makes sense, we could also > start from 0 like below. ok > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 > > The driver can have a table to map these bindings. This looks much better > than those x100 formula values. Again mention driver how he can map it. I mostly don't care about the driver. :) I think we are getting around the problem, so to emphasize again: do not store register values in the bindings/DT but its meaning, so in your case most likely percentages (or permille or ratio or some other value). Best regards, Krzysztof ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 10:08 ` Krzysztof Kozlowski 0 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 10:08 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 10:40, Pavan Kondeti wrote: > Hi Krzysztof, > > Thanks for your suggestions and guidance on this. > > On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 09:16, Pavan Kondeti wrote: >>> Hi Krzysztof, >>> >>> On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: >>>> On 14/03/2022 04:29, Pavan Kondeti wrote: >>>>> 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@quicinc.com> >>>>>>> --- >>>>>>> .../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. >>>> >>>> I did not say you cannot store here percentages. Quite opposite - store >>>> here the percentages. Just do not store register value. No. Please read >>>> my comment again - meaningful values are needed. >>>> >>> >>> IIUC, you are asking us to come up with a meaningful values to encode the >>> percentage values. However, all the % increments are not linear, so we can't >>> come up with {min, max} scheme. Lets take an example of hostdisconnect >>> threshold. >>> >>> As per the data book, >>> >>> + 7 -> +21.56% >>> + 6 -> +17.43% >>> + 5 -> +13.32% >>> + 4 -> +9.73% >>> + 3 -> +6.3 >>> + 2 -> +3.17% >>> + 1 -> 0, Design default% >>> + 0 -> -2.72% >>> >>> so how do we give meaningful values here? Does the below scheme make sense >>> to you? >> >> By "meaningful value" I mean something which has a understandable >> meaning to reader of this code or to hardware designer. For example >> percentage values or some units (ms, ns, Hz, mA, mV). The value used in >> register is not meaningful in that way to us because it has a meaning >> only to the hardware block. Storing register values is more difficult to >> read later, non-portable and non-scalable. >> >>> >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 >> >> This is some define in driver, does not look related to bindings. >> >>> In the driver, we have a mapping (which can be per SoC if required in future) >>> that takes these values and convert to the correct values for a given >>> register. >> >> You focus on driver but I am talking here only about bindings. > > I was saying we define those defines in include/dt-bindings/phy/... header and > use it in the device tree and as well in the driver. Ah, I did not get it. That's not the solution for this case. defines in dt-bindings are for constants which already can be in DT, e.g. IDs. Your register values should not be stored in DT. > >> >> What could be the meaningful value? Percentage could work. You have >> there a negative value, so I wonder what type of percentage is it? What >> is the formula? > > I just multiplied by 100 since device tree has no support for floating (as per > my knowledge). The negative value represents it lowers the disconnect > threshold by 2.72% of the default value. if it makes sense, we could also > start from 0 like below. ok > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 > > The driver can have a table to map these bindings. This looks much better > than those x100 formula values. Again mention driver how he can map it. I mostly don't care about the driver. :) I think we are getting around the problem, so to emphasize again: do not store register values in the bindings/DT but its meaning, so in your case most likely percentages (or permille or ratio or some other value). Best regards, Krzysztof -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 10:08 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski @ 2022-03-14 10:30 ` Pavan Kondeti -1 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 10:30 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, On Mon, Mar 14, 2022 at 11:08:05AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 10:40, Pavan Kondeti wrote: > > Hi Krzysztof, > > > > Thanks for your suggestions and guidance on this. > > > > On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote: > >> On 14/03/2022 09:16, Pavan Kondeti wrote: > >>> Hi Krzysztof, > >>> > >>> On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: > >>>> On 14/03/2022 04:29, Pavan Kondeti wrote: > >>>>> 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@quicinc.com> > >>>>>>> --- > >>>>>>> .../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. > >>>> > >>>> I did not say you cannot store here percentages. Quite opposite - store > >>>> here the percentages. Just do not store register value. No. Please read > >>>> my comment again - meaningful values are needed. > >>>> > >>> > >>> IIUC, you are asking us to come up with a meaningful values to encode the > >>> percentage values. However, all the % increments are not linear, so we can't > >>> come up with {min, max} scheme. Lets take an example of hostdisconnect > >>> threshold. > >>> > >>> As per the data book, > >>> > >>> + 7 -> +21.56% > >>> + 6 -> +17.43% > >>> + 5 -> +13.32% > >>> + 4 -> +9.73% > >>> + 3 -> +6.3 > >>> + 2 -> +3.17% > >>> + 1 -> 0, Design default% > >>> + 0 -> -2.72% > >>> > >>> so how do we give meaningful values here? Does the below scheme make sense > >>> to you? > >> > >> By "meaningful value" I mean something which has a understandable > >> meaning to reader of this code or to hardware designer. For example > >> percentage values or some units (ms, ns, Hz, mA, mV). The value used in > >> register is not meaningful in that way to us because it has a meaning > >> only to the hardware block. Storing register values is more difficult to > >> read later, non-portable and non-scalable. > >> > >>> > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 > >> > >> This is some define in driver, does not look related to bindings. > >> > >>> In the driver, we have a mapping (which can be per SoC if required in future) > >>> that takes these values and convert to the correct values for a given > >>> register. > >> > >> You focus on driver but I am talking here only about bindings. > > > > I was saying we define those defines in include/dt-bindings/phy/... header and > > use it in the device tree and as well in the driver. > > Ah, I did not get it. That's not the solution for this case. defines in > dt-bindings are for constants which already can be in DT, e.g. IDs. Your > register values should not be stored in DT. > These are again not register definitions. These are encodings that dT and driver can use. These would be constants only, no? > > > >> > >> What could be the meaningful value? Percentage could work. You have > >> there a negative value, so I wonder what type of percentage is it? What > >> is the formula? > > > > I just multiplied by 100 since device tree has no support for floating (as per > > my knowledge). The negative value represents it lowers the disconnect > > threshold by 2.72% of the default value. if it makes sense, we could also > > start from 0 like below. > > ok > > > > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 > > > > The driver can have a table to map these bindings. This looks much better > > than those x100 formula values. > > Again mention driver how he can map it. I mostly don't care about the > driver. :) > > I think we are getting around the problem, so to emphasize again: do not > store register values in the bindings/DT but its meaning, so in your > case most likely percentages (or permille or ratio or some other value). > I am really confused on what is that you mean by not storing the registers here. We are only giving enum values for specific percentages supported by the PHY. if you see -2.72 corresponds to 0 value on 0:2 bits of a register. I did not mention that in the device tree. we are giving constant values (enums) for all the possible percentage values. The user can see the dt-bindings file and pass the approriate value based on the compliance results. What is the objection? can you please give an example if you have something in mind? Thanks, Pavan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 10:30 ` Pavan Kondeti 0 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 10:30 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, On Mon, Mar 14, 2022 at 11:08:05AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 10:40, Pavan Kondeti wrote: > > Hi Krzysztof, > > > > Thanks for your suggestions and guidance on this. > > > > On Mon, Mar 14, 2022 at 09:36:02AM +0100, Krzysztof Kozlowski wrote: > >> On 14/03/2022 09:16, Pavan Kondeti wrote: > >>> Hi Krzysztof, > >>> > >>> On Mon, Mar 14, 2022 at 08:39:57AM +0100, Krzysztof Kozlowski wrote: > >>>> On 14/03/2022 04:29, Pavan Kondeti wrote: > >>>>> 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@quicinc.com> > >>>>>>> --- > >>>>>>> .../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. > >>>> > >>>> I did not say you cannot store here percentages. Quite opposite - store > >>>> here the percentages. Just do not store register value. No. Please read > >>>> my comment again - meaningful values are needed. > >>>> > >>> > >>> IIUC, you are asking us to come up with a meaningful values to encode the > >>> percentage values. However, all the % increments are not linear, so we can't > >>> come up with {min, max} scheme. Lets take an example of hostdisconnect > >>> threshold. > >>> > >>> As per the data book, > >>> > >>> + 7 -> +21.56% > >>> + 6 -> +17.43% > >>> + 5 -> +13.32% > >>> + 4 -> +9.73% > >>> + 3 -> +6.3 > >>> + 2 -> +3.17% > >>> + 1 -> 0, Design default% > >>> + 0 -> -2.72% > >>> > >>> so how do we give meaningful values here? Does the below scheme make sense > >>> to you? > >> > >> By "meaningful value" I mean something which has a understandable > >> meaning to reader of this code or to hardware designer. For example > >> percentage values or some units (ms, ns, Hz, mA, mV). The value used in > >> register is not meaningful in that way to us because it has a meaning > >> only to the hardware block. Storing register values is more difficult to > >> read later, non-portable and non-scalable. > >> > >>> > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72 (-272) > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 0 > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17 317 > >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3 63 > >> > >> This is some define in driver, does not look related to bindings. > >> > >>> In the driver, we have a mapping (which can be per SoC if required in future) > >>> that takes these values and convert to the correct values for a given > >>> register. > >> > >> You focus on driver but I am talking here only about bindings. > > > > I was saying we define those defines in include/dt-bindings/phy/... header and > > use it in the device tree and as well in the driver. > > Ah, I did not get it. That's not the solution for this case. defines in > dt-bindings are for constants which already can be in DT, e.g. IDs. Your > register values should not be stored in DT. > These are again not register definitions. These are encodings that dT and driver can use. These would be constants only, no? > > > >> > >> What could be the meaningful value? Percentage could work. You have > >> there a negative value, so I wonder what type of percentage is it? What > >> is the formula? > > > > I just multiplied by 100 since device tree has no support for floating (as per > > my knowledge). The negative value represents it lowers the disconnect > > threshold by 2.72% of the default value. if it makes sense, we could also > > start from 0 like below. > > ok > > > > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 > > #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 > > > > The driver can have a table to map these bindings. This looks much better > > than those x100 formula values. > > Again mention driver how he can map it. I mostly don't care about the > driver. :) > > I think we are getting around the problem, so to emphasize again: do not > store register values in the bindings/DT but its meaning, so in your > case most likely percentages (or permille or ratio or some other value). > I am really confused on what is that you mean by not storing the registers here. We are only giving enum values for specific percentages supported by the PHY. if you see -2.72 corresponds to 0 value on 0:2 bits of a register. I did not mention that in the device tree. we are giving constant values (enums) for all the possible percentage values. The user can see the dt-bindings file and pass the approriate value based on the compliance results. What is the objection? can you please give an example if you have something in mind? Thanks, Pavan -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 10:30 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti @ 2022-03-14 10:41 ` Krzysztof Kozlowski -1 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 10:41 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 11:30, Pavan Kondeti wrote: > Hi Krzysztof, > >> >> Ah, I did not get it. That's not the solution for this case. defines in >> dt-bindings are for constants which already can be in DT, e.g. IDs. Your >> register values should not be stored in DT. >> > These are again not register definitions. These are encodings that dT and > driver can use. These would be constants only, no? What do you mean it is not a register value? I don't have access to datasheet/manual but I can clearly see code: + if (or->hs_disconnect.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, + HS_DISCONNECT_MASK, + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect) and you write it to a register. Directly. 3 is a value for the hardware, meaningless outside of it. It has meaning only in this one hardware programming model. For humans it means nothing. For humans 6.3% means something. >>> >>>> >>>> What could be the meaningful value? Percentage could work. You have >>>> there a negative value, so I wonder what type of percentage is it? What >>>> is the formula? >>> >>> I just multiplied by 100 since device tree has no support for floating (as per >>> my knowledge). The negative value represents it lowers the disconnect >>> threshold by 2.72% of the default value. if it makes sense, we could also >>> start from 0 like below. >> >> ok >> >>> >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 >>> >>> The driver can have a table to map these bindings. This looks much better >>> than those x100 formula values. >> >> Again mention driver how he can map it. I mostly don't care about the >> driver. :) >> >> I think we are getting around the problem, so to emphasize again: do not >> store register values in the bindings/DT but its meaning, so in your >> case most likely percentages (or permille or ratio or some other value). >> > > I am really confused on what is that you mean by not storing the registers > here. We are only giving enum values for specific percentages supported by > the PHY. The enum consists of values used in hardware registers. Values having meaning only to this one particular hardware. Any other hardware will not understand them. IOW, you embed the hardware programming model in the DT. No. > if you see -2.72 corresponds to 0 value on 0:2 bits of a register. > I did not mention that in the device tree. we are giving constant values > (enums) for all the possible percentage values. The user can see the > dt-bindings file and pass the approriate value based on the compliance > results. What is the objection? You use some unrelated arguments. How does it matter what user can see or cannot see? > > can you please give an example if you have something in mind? I gave you an example - use percentages. Another example how it was done wrong is here: https://lore.kernel.org/linux-devicetree/c6607953-927e-4d85-21cb-72e01a121453@kernel.org/ Best regards, Krzysztof ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 10:41 ` Krzysztof Kozlowski 0 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 10:41 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 11:30, Pavan Kondeti wrote: > Hi Krzysztof, > >> >> Ah, I did not get it. That's not the solution for this case. defines in >> dt-bindings are for constants which already can be in DT, e.g. IDs. Your >> register values should not be stored in DT. >> > These are again not register definitions. These are encodings that dT and > driver can use. These would be constants only, no? What do you mean it is not a register value? I don't have access to datasheet/manual but I can clearly see code: + if (or->hs_disconnect.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, + HS_DISCONNECT_MASK, + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect) and you write it to a register. Directly. 3 is a value for the hardware, meaningless outside of it. It has meaning only in this one hardware programming model. For humans it means nothing. For humans 6.3% means something. >>> >>>> >>>> What could be the meaningful value? Percentage could work. You have >>>> there a negative value, so I wonder what type of percentage is it? What >>>> is the formula? >>> >>> I just multiplied by 100 since device tree has no support for floating (as per >>> my knowledge). The negative value represents it lowers the disconnect >>> threshold by 2.72% of the default value. if it makes sense, we could also >>> start from 0 like below. >> >> ok >> >>> >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_NEG_2P72_PCT 0 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_DEFAULT 1 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_3P17_PCT 2 >>> #define QCOM_SNPS_FEMTO_HS_DISCONNECT_6P3_PCT 3 >>> >>> The driver can have a table to map these bindings. This looks much better >>> than those x100 formula values. >> >> Again mention driver how he can map it. I mostly don't care about the >> driver. :) >> >> I think we are getting around the problem, so to emphasize again: do not >> store register values in the bindings/DT but its meaning, so in your >> case most likely percentages (or permille or ratio or some other value). >> > > I am really confused on what is that you mean by not storing the registers > here. We are only giving enum values for specific percentages supported by > the PHY. The enum consists of values used in hardware registers. Values having meaning only to this one particular hardware. Any other hardware will not understand them. IOW, you embed the hardware programming model in the DT. No. > if you see -2.72 corresponds to 0 value on 0:2 bits of a register. > I did not mention that in the device tree. we are giving constant values > (enums) for all the possible percentage values. The user can see the > dt-bindings file and pass the approriate value based on the compliance > results. What is the objection? You use some unrelated arguments. How does it matter what user can see or cannot see? > > can you please give an example if you have something in mind? I gave you an example - use percentages. Another example how it was done wrong is here: https://lore.kernel.org/linux-devicetree/c6607953-927e-4d85-21cb-72e01a121453@kernel.org/ Best regards, Krzysztof -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 10:41 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski @ 2022-03-14 11:13 ` Pavan Kondeti -1 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 11:13 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, On Mon, Mar 14, 2022 at 11:41:27AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 11:30, Pavan Kondeti wrote: > > Hi Krzysztof, > > > >> > >> Ah, I did not get it. That's not the solution for this case. defines in > >> dt-bindings are for constants which already can be in DT, e.g. IDs. Your > >> register values should not be stored in DT. > >> > > These are again not register definitions. These are encodings that dT and > > driver can use. These would be constants only, no? > > What do you mean it is not a register value? I don't have access to > datasheet/manual but I can clearly see code: > > + if (or->hs_disconnect.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > + HS_DISCONNECT_MASK, > + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); > > You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect) > and you write it to a register. Directly. 3 is a value for the hardware, > meaningless outside of it. It has meaning only in this one hardware > programming model. For humans it means nothing. For humans 6.3% means > something. > Right, This is what I have been saying will change. we don't pass the direct register values anymore. Instead I am saying, we pass the percentage multiplied by 100. For 6.3%, user will be passing 630 in device tree. for -2.75% user will pass (-275). Are we on the same page now? Thanks, Pavan ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 11:13 ` Pavan Kondeti 0 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-03-14 11:13 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Pavan Kondeti, Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura Hi Krzysztof, On Mon, Mar 14, 2022 at 11:41:27AM +0100, Krzysztof Kozlowski wrote: > On 14/03/2022 11:30, Pavan Kondeti wrote: > > Hi Krzysztof, > > > >> > >> Ah, I did not get it. That's not the solution for this case. defines in > >> dt-bindings are for constants which already can be in DT, e.g. IDs. Your > >> register values should not be stored in DT. > >> > > These are again not register definitions. These are encodings that dT and > > driver can use. These would be constants only, no? > > What do you mean it is not a register value? I don't have access to > datasheet/manual but I can clearly see code: > > + if (or->hs_disconnect.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > + HS_DISCONNECT_MASK, > + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); > > You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect) > and you write it to a register. Directly. 3 is a value for the hardware, > meaningless outside of it. It has meaning only in this one hardware > programming model. For humans it means nothing. For humans 6.3% means > something. > Right, This is what I have been saying will change. we don't pass the direct register values anymore. Instead I am saying, we pass the percentage multiplied by 100. For 6.3%, user will be passing 630 in device tree. for -2.75% user will pass (-275). Are we on the same page now? Thanks, Pavan -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings 2022-03-14 11:13 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti @ 2022-03-14 11:21 ` Krzysztof Kozlowski -1 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 11:21 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 12:13, Pavan Kondeti wrote: > Hi Krzysztof, > > On Mon, Mar 14, 2022 at 11:41:27AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 11:30, Pavan Kondeti wrote: >>> Hi Krzysztof, >>> >>>> >>>> Ah, I did not get it. That's not the solution for this case. defines in >>>> dt-bindings are for constants which already can be in DT, e.g. IDs. Your >>>> register values should not be stored in DT. >>>> >>> These are again not register definitions. These are encodings that dT and >>> driver can use. These would be constants only, no? >> >> What do you mean it is not a register value? I don't have access to >> datasheet/manual but I can clearly see code: >> >> + if (or->hs_disconnect.override) >> + qcom_snps_hsphy_write_mask(hsphy->base, >> + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, >> + HS_DISCONNECT_MASK, >> + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); >> >> You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect) >> and you write it to a register. Directly. 3 is a value for the hardware, >> meaningless outside of it. It has meaning only in this one hardware >> programming model. For humans it means nothing. For humans 6.3% means >> something. >> > > Right, This is what I have been saying will change. we don't pass the direct > register values anymore. Instead I am saying, we pass the percentage > multiplied by 100. For 6.3%, user will be passing 630 in device tree. for > -2.75% user will pass (-275). > > Are we on the same page now? Yes, it sounds good. Thanks! Best regards, Krzysztof ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: Add phy override params bindings @ 2022-03-14 11:21 ` Krzysztof Kozlowski 0 siblings, 0 replies; 36+ messages in thread From: Krzysztof Kozlowski @ 2022-03-14 11:21 UTC (permalink / raw) To: Pavan Kondeti Cc: Sandeep Maheswaram, Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_ppratap, quic_kriskura On 14/03/2022 12:13, Pavan Kondeti wrote: > Hi Krzysztof, > > On Mon, Mar 14, 2022 at 11:41:27AM +0100, Krzysztof Kozlowski wrote: >> On 14/03/2022 11:30, Pavan Kondeti wrote: >>> Hi Krzysztof, >>> >>>> >>>> Ah, I did not get it. That's not the solution for this case. defines in >>>> dt-bindings are for constants which already can be in DT, e.g. IDs. Your >>>> register values should not be stored in DT. >>>> >>> These are again not register definitions. These are encodings that dT and >>> driver can use. These would be constants only, no? >> >> What do you mean it is not a register value? I don't have access to >> datasheet/manual but I can clearly see code: >> >> + if (or->hs_disconnect.override) >> + qcom_snps_hsphy_write_mask(hsphy->base, >> + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, >> + HS_DISCONNECT_MASK, >> + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); >> >> You read the value from DT (e.g. "3" which means 6.3% for hs-disconnect) >> and you write it to a register. Directly. 3 is a value for the hardware, >> meaningless outside of it. It has meaning only in this one hardware >> programming model. For humans it means nothing. For humans 6.3% means >> something. >> > > Right, This is what I have been saying will change. we don't pass the direct > register values anymore. Instead I am saying, we pass the percentage > multiplied by 100. For 6.3%, user will be passing 630 in device tree. for > -2.75% user will pass (-275). > > Are we on the same page now? Yes, it sounds good. Thanks! Best regards, Krzysztof -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters 2022-03-03 6:13 ` Sandeep Maheswaram @ 2022-03-03 6:13 ` Sandeep Maheswaram -1 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading values from device tree. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c index 7e61202..b5aa06d 100644 --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c @@ -51,6 +51,48 @@ #define USB2_SUSPEND_N BIT(2) #define USB2_SUSPEND_N_SEL BIT(3) +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0 (0x6c) + +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/ +#define HS_DISCONNECT_MASK GENMASK(2, 0) +#define HS_DISCONNECT_SHIFT 0x0 + +#define SQUELCH_DETECTOR_MASK GENMASK(7, 5) +#define SQUELCH_DETECTOR_SHIFT 0x5 + + +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1 (0x70) + +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/ +#define HS_AMPLITUDE_MASK GENMASK(3, 0) +#define HS_AMPLITUDE_SHIFT 0x0 + +#define PREEMPHASIS_DURATION_MASK BIT(5) +#define PREEMPHASIS_DURATION_SHIFT 0x5 + +#define PREEMPHASIS_AMPLITUDE_MASK GENMASK(7, 6) +#define PREEMPHASIS_AMPLITUDE_SHIFT 0x6 + + +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2 (0x74) + +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/ +#define HS_RISE_FALL_MASK GENMASK(1, 0) +#define HS_RISE_FALL_SHIFT 0x0 + +#define HS_CROSSOVER_VOLTAGE_MASK GENMASK(3, 2) +#define HS_CROSSOVER_VOLTAGE_SHIFT 0x2 + +#define HS_OUTPUT_IMPEDANCE_MASK GENMASK(5, 4) +#define HS_OUTPUT_IMPEDANCE_SHIFT 0x4 + + +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3 (0x78) + +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/ +#define LS_FS_OUTPUT_IMPEDANCE_MASK GENMASK(3, 0) +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT 0x0 + #define USB2_PHY_USB_PHY_CFG0 (0x94) #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN BIT(0) #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN BIT(1) @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = { #define SNPS_HS_NUM_VREGS ARRAY_SIZE(qcom_snps_hsphy_vreg_names) +/* struct override_param - structure holding snps phy overriding param + * set override true if the device tree property exists and read and assign + * to value + */ +struct override_param { + bool override; + u8 value; +}; + +/*struct override_params - structure holding snps phy overriding params + * @hs_disconnect: disconnect threshold + * @squelch_detector: threshold to detect valid high-speed data + * @hs_amplitude: high-speed DC level voltage + * @preemphasis_duration: duration for which the HS pre-emphasis current + * is sourced onto DP<#> or DM<#> + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after + * a J-to-K or K-to-J transition. + * @hs_rise_fall_time: rise/fall times of the high-speed waveform + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#> + * signals cross while transmitting in HS mode + * @hs_output_impedance: driver source impedance to compensate for added series + * resistance on the USB + * @ls_fs_output_impedance: low and full-speed single-ended source + * impedance while driving high + */ +struct override_params { + struct override_param hs_disconnect; + struct override_param squelch_detector; + struct override_param hs_amplitude; + struct override_param preemphasis_duration; + struct override_param preemphasis_amplitude; + struct override_param hs_rise_fall_time; + struct override_param hs_crossover_voltage; + struct override_param hs_output_impedance; + struct override_param ls_fs_output_impedance; +}; + /** * struct qcom_snps_hsphy - snps hs phy attributes * @@ -87,6 +166,7 @@ struct qcom_snps_hsphy { struct clk *ref_clk; struct reset_control *phy_reset; struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; + struct override_params overrides; bool phy_initialized; enum phy_mode mode; @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, static int qcom_snps_hsphy_init(struct phy *phy) { struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); + struct override_params *or = &hsphy->overrides; int ret; dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__); @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy) qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1, VBUSVLDEXT0, VBUSVLDEXT0); + if (or->hs_disconnect.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, + HS_DISCONNECT_MASK, + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); + + if (or->squelch_detector.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, + SQUELCH_DETECTOR_MASK, + or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT); + + if (or->hs_amplitude.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, + HS_AMPLITUDE_MASK, + or->hs_amplitude.value << HS_AMPLITUDE_SHIFT); + + if (or->preemphasis_duration.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, + PREEMPHASIS_DURATION_MASK, + or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT); + + if (or->preemphasis_amplitude.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, + PREEMPHASIS_AMPLITUDE_MASK, + or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT); + + if (or->hs_rise_fall_time.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, + HS_RISE_FALL_MASK, + or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT); + + if (or->hs_crossover_voltage.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, + HS_CROSSOVER_VOLTAGE_MASK, + or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT); + + if (or->hs_output_impedance.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, + HS_OUTPUT_IMPEDANCE_MASK, + or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT); + + if (or->ls_fs_output_impedance.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3, + LS_FS_OUTPUT_IMPEDANCE_MASK, + or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT); + qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, VREGBYPASS, VREGBYPASS); @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) struct qcom_snps_hsphy *hsphy; struct phy_provider *phy_provider; struct phy *generic_phy; + struct override_params *or; int ret, i; int num; + u32 value; hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL); if (!hsphy) return -ENOMEM; + or = &hsphy->overrides; hsphy->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(hsphy->base)) @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) return ret; } + if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect", + &value)) { + or->hs_disconnect.value = (u8)value; + or->hs_disconnect.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector", + &value)) { + or->squelch_detector.value = (u8)value; + or->squelch_detector.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude", + &value)) { + or->hs_amplitude.value = (u8)value; + or->hs_amplitude.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration", + &value)) { + or->preemphasis_duration.value = (u8)value; + or->preemphasis_duration.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude", + &value)) { + or->preemphasis_amplitude.value = (u8)value; + or->preemphasis_amplitude.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time", + &value)) { + or->hs_rise_fall_time.value = (u8)value; + or->hs_rise_fall_time.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage", + &value)) { + or->hs_crossover_voltage.value = (u8)value; + or->hs_crossover_voltage.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance", + &value)) { + or->hs_output_impedance.value = (u8)value; + or->hs_output_impedance.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance", + &value)) { + or->ls_fs_output_impedance.value = (u8)value; + or->ls_fs_output_impedance.override = true; + } + pm_runtime_set_active(dev); pm_runtime_enable(dev); /* -- 2.7.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters @ 2022-03-03 6:13 ` Sandeep Maheswaram 0 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading values from device tree. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c index 7e61202..b5aa06d 100644 --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c @@ -51,6 +51,48 @@ #define USB2_SUSPEND_N BIT(2) #define USB2_SUSPEND_N_SEL BIT(3) +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0 (0x6c) + +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/ +#define HS_DISCONNECT_MASK GENMASK(2, 0) +#define HS_DISCONNECT_SHIFT 0x0 + +#define SQUELCH_DETECTOR_MASK GENMASK(7, 5) +#define SQUELCH_DETECTOR_SHIFT 0x5 + + +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1 (0x70) + +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/ +#define HS_AMPLITUDE_MASK GENMASK(3, 0) +#define HS_AMPLITUDE_SHIFT 0x0 + +#define PREEMPHASIS_DURATION_MASK BIT(5) +#define PREEMPHASIS_DURATION_SHIFT 0x5 + +#define PREEMPHASIS_AMPLITUDE_MASK GENMASK(7, 6) +#define PREEMPHASIS_AMPLITUDE_SHIFT 0x6 + + +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2 (0x74) + +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/ +#define HS_RISE_FALL_MASK GENMASK(1, 0) +#define HS_RISE_FALL_SHIFT 0x0 + +#define HS_CROSSOVER_VOLTAGE_MASK GENMASK(3, 2) +#define HS_CROSSOVER_VOLTAGE_SHIFT 0x2 + +#define HS_OUTPUT_IMPEDANCE_MASK GENMASK(5, 4) +#define HS_OUTPUT_IMPEDANCE_SHIFT 0x4 + + +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3 (0x78) + +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/ +#define LS_FS_OUTPUT_IMPEDANCE_MASK GENMASK(3, 0) +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT 0x0 + #define USB2_PHY_USB_PHY_CFG0 (0x94) #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN BIT(0) #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN BIT(1) @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = { #define SNPS_HS_NUM_VREGS ARRAY_SIZE(qcom_snps_hsphy_vreg_names) +/* struct override_param - structure holding snps phy overriding param + * set override true if the device tree property exists and read and assign + * to value + */ +struct override_param { + bool override; + u8 value; +}; + +/*struct override_params - structure holding snps phy overriding params + * @hs_disconnect: disconnect threshold + * @squelch_detector: threshold to detect valid high-speed data + * @hs_amplitude: high-speed DC level voltage + * @preemphasis_duration: duration for which the HS pre-emphasis current + * is sourced onto DP<#> or DM<#> + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after + * a J-to-K or K-to-J transition. + * @hs_rise_fall_time: rise/fall times of the high-speed waveform + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#> + * signals cross while transmitting in HS mode + * @hs_output_impedance: driver source impedance to compensate for added series + * resistance on the USB + * @ls_fs_output_impedance: low and full-speed single-ended source + * impedance while driving high + */ +struct override_params { + struct override_param hs_disconnect; + struct override_param squelch_detector; + struct override_param hs_amplitude; + struct override_param preemphasis_duration; + struct override_param preemphasis_amplitude; + struct override_param hs_rise_fall_time; + struct override_param hs_crossover_voltage; + struct override_param hs_output_impedance; + struct override_param ls_fs_output_impedance; +}; + /** * struct qcom_snps_hsphy - snps hs phy attributes * @@ -87,6 +166,7 @@ struct qcom_snps_hsphy { struct clk *ref_clk; struct reset_control *phy_reset; struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; + struct override_params overrides; bool phy_initialized; enum phy_mode mode; @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, static int qcom_snps_hsphy_init(struct phy *phy) { struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); + struct override_params *or = &hsphy->overrides; int ret; dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__); @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy) qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1, VBUSVLDEXT0, VBUSVLDEXT0); + if (or->hs_disconnect.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, + HS_DISCONNECT_MASK, + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); + + if (or->squelch_detector.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, + SQUELCH_DETECTOR_MASK, + or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT); + + if (or->hs_amplitude.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, + HS_AMPLITUDE_MASK, + or->hs_amplitude.value << HS_AMPLITUDE_SHIFT); + + if (or->preemphasis_duration.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, + PREEMPHASIS_DURATION_MASK, + or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT); + + if (or->preemphasis_amplitude.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, + PREEMPHASIS_AMPLITUDE_MASK, + or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT); + + if (or->hs_rise_fall_time.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, + HS_RISE_FALL_MASK, + or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT); + + if (or->hs_crossover_voltage.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, + HS_CROSSOVER_VOLTAGE_MASK, + or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT); + + if (or->hs_output_impedance.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, + HS_OUTPUT_IMPEDANCE_MASK, + or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT); + + if (or->ls_fs_output_impedance.override) + qcom_snps_hsphy_write_mask(hsphy->base, + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3, + LS_FS_OUTPUT_IMPEDANCE_MASK, + or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT); + qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, VREGBYPASS, VREGBYPASS); @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) struct qcom_snps_hsphy *hsphy; struct phy_provider *phy_provider; struct phy *generic_phy; + struct override_params *or; int ret, i; int num; + u32 value; hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL); if (!hsphy) return -ENOMEM; + or = &hsphy->overrides; hsphy->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(hsphy->base)) @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) return ret; } + if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect", + &value)) { + or->hs_disconnect.value = (u8)value; + or->hs_disconnect.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector", + &value)) { + or->squelch_detector.value = (u8)value; + or->squelch_detector.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude", + &value)) { + or->hs_amplitude.value = (u8)value; + or->hs_amplitude.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration", + &value)) { + or->preemphasis_duration.value = (u8)value; + or->preemphasis_duration.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude", + &value)) { + or->preemphasis_amplitude.value = (u8)value; + or->preemphasis_amplitude.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time", + &value)) { + or->hs_rise_fall_time.value = (u8)value; + or->hs_rise_fall_time.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage", + &value)) { + or->hs_crossover_voltage.value = (u8)value; + or->hs_crossover_voltage.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance", + &value)) { + or->hs_output_impedance.value = (u8)value; + or->hs_output_impedance.override = true; + } + + if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance", + &value)) { + or->ls_fs_output_impedance.value = (u8)value; + or->ls_fs_output_impedance.override = true; + } + pm_runtime_set_active(dev); pm_runtime_enable(dev); /* -- 2.7.4 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters 2022-03-03 6:13 ` Sandeep Maheswaram @ 2022-04-13 9:42 ` Vinod Koul -1 siblings, 0 replies; 36+ messages in thread From: Vinod Koul @ 2022-04-13 9:42 UTC (permalink / raw) To: Sandeep Maheswaram, Bjorn Andersson Cc: Rob Herring, Andy Gross, Kishon Vijay Abraham I, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap On 03-03-22, 11:43, Sandeep Maheswaram wrote: > Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading > values from device tree. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > --- > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++ > 1 file changed, 192 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > index 7e61202..b5aa06d 100644 > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > @@ -51,6 +51,48 @@ > #define USB2_SUSPEND_N BIT(2) > #define USB2_SUSPEND_N_SEL BIT(3) > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0 (0x6c) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/ space after /* and before */ (checkpatch.pl --strict would warn you) Pls fix it everywhere > +#define HS_DISCONNECT_MASK GENMASK(2, 0) > +#define HS_DISCONNECT_SHIFT 0x0 > + > +#define SQUELCH_DETECTOR_MASK GENMASK(7, 5) > +#define SQUELCH_DETECTOR_SHIFT 0x5 > + > + > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1 (0x70) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/ > +#define HS_AMPLITUDE_MASK GENMASK(3, 0) > +#define HS_AMPLITUDE_SHIFT 0x0 > + > +#define PREEMPHASIS_DURATION_MASK BIT(5) > +#define PREEMPHASIS_DURATION_SHIFT 0x5 > + > +#define PREEMPHASIS_AMPLITUDE_MASK GENMASK(7, 6) > +#define PREEMPHASIS_AMPLITUDE_SHIFT 0x6 > + > + > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2 (0x74) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/ > +#define HS_RISE_FALL_MASK GENMASK(1, 0) > +#define HS_RISE_FALL_SHIFT 0x0 > + > +#define HS_CROSSOVER_VOLTAGE_MASK GENMASK(3, 2) > +#define HS_CROSSOVER_VOLTAGE_SHIFT 0x2 > + > +#define HS_OUTPUT_IMPEDANCE_MASK GENMASK(5, 4) > +#define HS_OUTPUT_IMPEDANCE_SHIFT 0x4 > + > + > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3 (0x78) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/ > +#define LS_FS_OUTPUT_IMPEDANCE_MASK GENMASK(3, 0) > +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT 0x0 > + > #define USB2_PHY_USB_PHY_CFG0 (0x94) > #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN BIT(0) > #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN BIT(1) > @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = { > > #define SNPS_HS_NUM_VREGS ARRAY_SIZE(qcom_snps_hsphy_vreg_names) > > +/* struct override_param - structure holding snps phy overriding param > + * set override true if the device tree property exists and read and assign > + * to value > + */ > +struct override_param { > + bool override; > + u8 value; > +}; > + > +/*struct override_params - structure holding snps phy overriding params > + * @hs_disconnect: disconnect threshold > + * @squelch_detector: threshold to detect valid high-speed data > + * @hs_amplitude: high-speed DC level voltage > + * @preemphasis_duration: duration for which the HS pre-emphasis current > + * is sourced onto DP<#> or DM<#> > + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after > + * a J-to-K or K-to-J transition. > + * @hs_rise_fall_time: rise/fall times of the high-speed waveform > + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#> > + * signals cross while transmitting in HS mode > + * @hs_output_impedance: driver source impedance to compensate for added series > + * resistance on the USB > + * @ls_fs_output_impedance: low and full-speed single-ended source > + * impedance while driving high > + */ > +struct override_params { > + struct override_param hs_disconnect; > + struct override_param squelch_detector; > + struct override_param hs_amplitude; > + struct override_param preemphasis_duration; > + struct override_param preemphasis_amplitude; > + struct override_param hs_rise_fall_time; > + struct override_param hs_crossover_voltage; > + struct override_param hs_output_impedance; > + struct override_param ls_fs_output_impedance; > +}; > + > /** > * struct qcom_snps_hsphy - snps hs phy attributes > * > @@ -87,6 +166,7 @@ struct qcom_snps_hsphy { > struct clk *ref_clk; > struct reset_control *phy_reset; > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > + struct override_params overrides; > > bool phy_initialized; > enum phy_mode mode; > @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, > static int qcom_snps_hsphy_init(struct phy *phy) > { > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > + struct override_params *or = &hsphy->overrides; > int ret; > > dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__); > @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy) > qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1, > VBUSVLDEXT0, VBUSVLDEXT0); > > + if (or->hs_disconnect.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > + HS_DISCONNECT_MASK, > + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); > + > + if (or->squelch_detector.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > + SQUELCH_DETECTOR_MASK, > + or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT); > + > + if (or->hs_amplitude.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > + HS_AMPLITUDE_MASK, > + or->hs_amplitude.value << HS_AMPLITUDE_SHIFT); > + > + if (or->preemphasis_duration.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > + PREEMPHASIS_DURATION_MASK, > + or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT); > + > + if (or->preemphasis_amplitude.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > + PREEMPHASIS_AMPLITUDE_MASK, > + or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT); > + > + if (or->hs_rise_fall_time.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > + HS_RISE_FALL_MASK, > + or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT); > + > + if (or->hs_crossover_voltage.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > + HS_CROSSOVER_VOLTAGE_MASK, > + or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT); > + > + if (or->hs_output_impedance.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > + HS_OUTPUT_IMPEDANCE_MASK, > + or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT); > + > + if (or->ls_fs_output_impedance.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3, > + LS_FS_OUTPUT_IMPEDANCE_MASK, > + or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT); > + > qcom_snps_hsphy_write_mask(hsphy->base, > USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, > VREGBYPASS, VREGBYPASS); > @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > struct qcom_snps_hsphy *hsphy; > struct phy_provider *phy_provider; > struct phy *generic_phy; > + struct override_params *or; > int ret, i; > int num; > + u32 value; > > hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL); > if (!hsphy) > return -ENOMEM; > + or = &hsphy->overrides; > > hsphy->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(hsphy->base)) > @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > return ret; > } > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect", > + &value)) { > + or->hs_disconnect.value = (u8)value; > + or->hs_disconnect.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector", > + &value)) { > + or->squelch_detector.value = (u8)value; > + or->squelch_detector.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude", > + &value)) { > + or->hs_amplitude.value = (u8)value; > + or->hs_amplitude.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration", > + &value)) { > + or->preemphasis_duration.value = (u8)value; > + or->preemphasis_duration.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude", > + &value)) { > + or->preemphasis_amplitude.value = (u8)value; > + or->preemphasis_amplitude.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time", > + &value)) { > + or->hs_rise_fall_time.value = (u8)value; > + or->hs_rise_fall_time.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage", > + &value)) { > + or->hs_crossover_voltage.value = (u8)value; > + or->hs_crossover_voltage.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance", > + &value)) { > + or->hs_output_impedance.value = (u8)value; > + or->hs_output_impedance.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance", > + &value)) { > + or->ls_fs_output_impedance.value = (u8)value; > + or->ls_fs_output_impedance.override = true; > + } Are all these values board specific or IP specific? Can we add these values as tables in driver? Bjorn what do you think about the above proposal? -- ~Vinod ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters @ 2022-04-13 9:42 ` Vinod Koul 0 siblings, 0 replies; 36+ messages in thread From: Vinod Koul @ 2022-04-13 9:42 UTC (permalink / raw) To: Sandeep Maheswaram, Bjorn Andersson Cc: Rob Herring, Andy Gross, Kishon Vijay Abraham I, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap On 03-03-22, 11:43, Sandeep Maheswaram wrote: > Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading > values from device tree. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > --- > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++ > 1 file changed, 192 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > index 7e61202..b5aa06d 100644 > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > @@ -51,6 +51,48 @@ > #define USB2_SUSPEND_N BIT(2) > #define USB2_SUSPEND_N_SEL BIT(3) > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0 (0x6c) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/ space after /* and before */ (checkpatch.pl --strict would warn you) Pls fix it everywhere > +#define HS_DISCONNECT_MASK GENMASK(2, 0) > +#define HS_DISCONNECT_SHIFT 0x0 > + > +#define SQUELCH_DETECTOR_MASK GENMASK(7, 5) > +#define SQUELCH_DETECTOR_SHIFT 0x5 > + > + > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1 (0x70) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/ > +#define HS_AMPLITUDE_MASK GENMASK(3, 0) > +#define HS_AMPLITUDE_SHIFT 0x0 > + > +#define PREEMPHASIS_DURATION_MASK BIT(5) > +#define PREEMPHASIS_DURATION_SHIFT 0x5 > + > +#define PREEMPHASIS_AMPLITUDE_MASK GENMASK(7, 6) > +#define PREEMPHASIS_AMPLITUDE_SHIFT 0x6 > + > + > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2 (0x74) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/ > +#define HS_RISE_FALL_MASK GENMASK(1, 0) > +#define HS_RISE_FALL_SHIFT 0x0 > + > +#define HS_CROSSOVER_VOLTAGE_MASK GENMASK(3, 2) > +#define HS_CROSSOVER_VOLTAGE_SHIFT 0x2 > + > +#define HS_OUTPUT_IMPEDANCE_MASK GENMASK(5, 4) > +#define HS_OUTPUT_IMPEDANCE_SHIFT 0x4 > + > + > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3 (0x78) > + > +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/ > +#define LS_FS_OUTPUT_IMPEDANCE_MASK GENMASK(3, 0) > +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT 0x0 > + > #define USB2_PHY_USB_PHY_CFG0 (0x94) > #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN BIT(0) > #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN BIT(1) > @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = { > > #define SNPS_HS_NUM_VREGS ARRAY_SIZE(qcom_snps_hsphy_vreg_names) > > +/* struct override_param - structure holding snps phy overriding param > + * set override true if the device tree property exists and read and assign > + * to value > + */ > +struct override_param { > + bool override; > + u8 value; > +}; > + > +/*struct override_params - structure holding snps phy overriding params > + * @hs_disconnect: disconnect threshold > + * @squelch_detector: threshold to detect valid high-speed data > + * @hs_amplitude: high-speed DC level voltage > + * @preemphasis_duration: duration for which the HS pre-emphasis current > + * is sourced onto DP<#> or DM<#> > + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after > + * a J-to-K or K-to-J transition. > + * @hs_rise_fall_time: rise/fall times of the high-speed waveform > + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#> > + * signals cross while transmitting in HS mode > + * @hs_output_impedance: driver source impedance to compensate for added series > + * resistance on the USB > + * @ls_fs_output_impedance: low and full-speed single-ended source > + * impedance while driving high > + */ > +struct override_params { > + struct override_param hs_disconnect; > + struct override_param squelch_detector; > + struct override_param hs_amplitude; > + struct override_param preemphasis_duration; > + struct override_param preemphasis_amplitude; > + struct override_param hs_rise_fall_time; > + struct override_param hs_crossover_voltage; > + struct override_param hs_output_impedance; > + struct override_param ls_fs_output_impedance; > +}; > + > /** > * struct qcom_snps_hsphy - snps hs phy attributes > * > @@ -87,6 +166,7 @@ struct qcom_snps_hsphy { > struct clk *ref_clk; > struct reset_control *phy_reset; > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > + struct override_params overrides; > > bool phy_initialized; > enum phy_mode mode; > @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, > static int qcom_snps_hsphy_init(struct phy *phy) > { > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > + struct override_params *or = &hsphy->overrides; > int ret; > > dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__); > @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy) > qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1, > VBUSVLDEXT0, VBUSVLDEXT0); > > + if (or->hs_disconnect.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > + HS_DISCONNECT_MASK, > + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); > + > + if (or->squelch_detector.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > + SQUELCH_DETECTOR_MASK, > + or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT); > + > + if (or->hs_amplitude.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > + HS_AMPLITUDE_MASK, > + or->hs_amplitude.value << HS_AMPLITUDE_SHIFT); > + > + if (or->preemphasis_duration.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > + PREEMPHASIS_DURATION_MASK, > + or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT); > + > + if (or->preemphasis_amplitude.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > + PREEMPHASIS_AMPLITUDE_MASK, > + or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT); > + > + if (or->hs_rise_fall_time.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > + HS_RISE_FALL_MASK, > + or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT); > + > + if (or->hs_crossover_voltage.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > + HS_CROSSOVER_VOLTAGE_MASK, > + or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT); > + > + if (or->hs_output_impedance.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > + HS_OUTPUT_IMPEDANCE_MASK, > + or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT); > + > + if (or->ls_fs_output_impedance.override) > + qcom_snps_hsphy_write_mask(hsphy->base, > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3, > + LS_FS_OUTPUT_IMPEDANCE_MASK, > + or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT); > + > qcom_snps_hsphy_write_mask(hsphy->base, > USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, > VREGBYPASS, VREGBYPASS); > @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > struct qcom_snps_hsphy *hsphy; > struct phy_provider *phy_provider; > struct phy *generic_phy; > + struct override_params *or; > int ret, i; > int num; > + u32 value; > > hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL); > if (!hsphy) > return -ENOMEM; > + or = &hsphy->overrides; > > hsphy->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(hsphy->base)) > @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > return ret; > } > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect", > + &value)) { > + or->hs_disconnect.value = (u8)value; > + or->hs_disconnect.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector", > + &value)) { > + or->squelch_detector.value = (u8)value; > + or->squelch_detector.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude", > + &value)) { > + or->hs_amplitude.value = (u8)value; > + or->hs_amplitude.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration", > + &value)) { > + or->preemphasis_duration.value = (u8)value; > + or->preemphasis_duration.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude", > + &value)) { > + or->preemphasis_amplitude.value = (u8)value; > + or->preemphasis_amplitude.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time", > + &value)) { > + or->hs_rise_fall_time.value = (u8)value; > + or->hs_rise_fall_time.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage", > + &value)) { > + or->hs_crossover_voltage.value = (u8)value; > + or->hs_crossover_voltage.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance", > + &value)) { > + or->hs_output_impedance.value = (u8)value; > + or->hs_output_impedance.override = true; > + } > + > + if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance", > + &value)) { > + or->ls_fs_output_impedance.value = (u8)value; > + or->ls_fs_output_impedance.override = true; > + } Are all these values board specific or IP specific? Can we add these values as tables in driver? Bjorn what do you think about the above proposal? -- ~Vinod -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters 2022-04-13 9:42 ` Vinod Koul @ 2022-04-13 9:53 ` Pavan Kondeti -1 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-04-13 9:53 UTC (permalink / raw) To: Vinod Koul Cc: Sandeep Maheswaram, Bjorn Andersson, Rob Herring, Andy Gross, Kishon Vijay Abraham I, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, quic_kriskura Hi Vinod, On Wed, Apr 13, 2022 at 03:12:09PM +0530, Vinod Koul wrote: > On 03-03-22, 11:43, Sandeep Maheswaram wrote: > > Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading > > values from device tree. > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > --- > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++ > > 1 file changed, 192 insertions(+) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > index 7e61202..b5aa06d 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > @@ -51,6 +51,48 @@ > > #define USB2_SUSPEND_N BIT(2) > > #define USB2_SUSPEND_N_SEL BIT(3) > > > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0 (0x6c) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/ > > space after /* and before */ (checkpatch.pl --strict would warn you) > Pls fix it everywhere > > > +#define HS_DISCONNECT_MASK GENMASK(2, 0) > > +#define HS_DISCONNECT_SHIFT 0x0 > > + > > +#define SQUELCH_DETECTOR_MASK GENMASK(7, 5) > > +#define SQUELCH_DETECTOR_SHIFT 0x5 > > + > > + > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1 (0x70) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/ > > +#define HS_AMPLITUDE_MASK GENMASK(3, 0) > > +#define HS_AMPLITUDE_SHIFT 0x0 > > + > > +#define PREEMPHASIS_DURATION_MASK BIT(5) > > +#define PREEMPHASIS_DURATION_SHIFT 0x5 > > + > > +#define PREEMPHASIS_AMPLITUDE_MASK GENMASK(7, 6) > > +#define PREEMPHASIS_AMPLITUDE_SHIFT 0x6 > > + > > + > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2 (0x74) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/ > > +#define HS_RISE_FALL_MASK GENMASK(1, 0) > > +#define HS_RISE_FALL_SHIFT 0x0 > > + > > +#define HS_CROSSOVER_VOLTAGE_MASK GENMASK(3, 2) > > +#define HS_CROSSOVER_VOLTAGE_SHIFT 0x2 > > + > > +#define HS_OUTPUT_IMPEDANCE_MASK GENMASK(5, 4) > > +#define HS_OUTPUT_IMPEDANCE_SHIFT 0x4 > > + > > + > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3 (0x78) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/ > > +#define LS_FS_OUTPUT_IMPEDANCE_MASK GENMASK(3, 0) > > +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT 0x0 > > + > > #define USB2_PHY_USB_PHY_CFG0 (0x94) > > #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN BIT(0) > > #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN BIT(1) > > @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = { > > > > #define SNPS_HS_NUM_VREGS ARRAY_SIZE(qcom_snps_hsphy_vreg_names) > > > > +/* struct override_param - structure holding snps phy overriding param > > + * set override true if the device tree property exists and read and assign > > + * to value > > + */ > > +struct override_param { > > + bool override; > > + u8 value; > > +}; > > + > > +/*struct override_params - structure holding snps phy overriding params > > + * @hs_disconnect: disconnect threshold > > + * @squelch_detector: threshold to detect valid high-speed data > > + * @hs_amplitude: high-speed DC level voltage > > + * @preemphasis_duration: duration for which the HS pre-emphasis current > > + * is sourced onto DP<#> or DM<#> > > + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after > > + * a J-to-K or K-to-J transition. > > + * @hs_rise_fall_time: rise/fall times of the high-speed waveform > > + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#> > > + * signals cross while transmitting in HS mode > > + * @hs_output_impedance: driver source impedance to compensate for added series > > + * resistance on the USB > > + * @ls_fs_output_impedance: low and full-speed single-ended source > > + * impedance while driving high > > + */ > > +struct override_params { > > + struct override_param hs_disconnect; > > + struct override_param squelch_detector; > > + struct override_param hs_amplitude; > > + struct override_param preemphasis_duration; > > + struct override_param preemphasis_amplitude; > > + struct override_param hs_rise_fall_time; > > + struct override_param hs_crossover_voltage; > > + struct override_param hs_output_impedance; > > + struct override_param ls_fs_output_impedance; > > +}; > > + > > /** > > * struct qcom_snps_hsphy - snps hs phy attributes > > * > > @@ -87,6 +166,7 @@ struct qcom_snps_hsphy { > > struct clk *ref_clk; > > struct reset_control *phy_reset; > > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > + struct override_params overrides; > > > > bool phy_initialized; > > enum phy_mode mode; > > @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, > > static int qcom_snps_hsphy_init(struct phy *phy) > > { > > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > > + struct override_params *or = &hsphy->overrides; > > int ret; > > > > dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__); > > @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1, > > VBUSVLDEXT0, VBUSVLDEXT0); > > > > + if (or->hs_disconnect.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > > + HS_DISCONNECT_MASK, > > + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); > > + > > + if (or->squelch_detector.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > > + SQUELCH_DETECTOR_MASK, > > + or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT); > > + > > + if (or->hs_amplitude.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > > + HS_AMPLITUDE_MASK, > > + or->hs_amplitude.value << HS_AMPLITUDE_SHIFT); > > + > > + if (or->preemphasis_duration.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > > + PREEMPHASIS_DURATION_MASK, > > + or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT); > > + > > + if (or->preemphasis_amplitude.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > > + PREEMPHASIS_AMPLITUDE_MASK, > > + or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT); > > + > > + if (or->hs_rise_fall_time.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > > + HS_RISE_FALL_MASK, > > + or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT); > > + > > + if (or->hs_crossover_voltage.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > > + HS_CROSSOVER_VOLTAGE_MASK, > > + or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT); > > + > > + if (or->hs_output_impedance.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > > + HS_OUTPUT_IMPEDANCE_MASK, > > + or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT); > > + > > + if (or->ls_fs_output_impedance.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3, > > + LS_FS_OUTPUT_IMPEDANCE_MASK, > > + or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT); > > + > > qcom_snps_hsphy_write_mask(hsphy->base, > > USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, > > VREGBYPASS, VREGBYPASS); > > @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > > struct qcom_snps_hsphy *hsphy; > > struct phy_provider *phy_provider; > > struct phy *generic_phy; > > + struct override_params *or; > > int ret, i; > > int num; > > + u32 value; > > > > hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL); > > if (!hsphy) > > return -ENOMEM; > > + or = &hsphy->overrides; > > > > hsphy->base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(hsphy->base)) > > @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > > return ret; > > } > > > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect", > > + &value)) { > > + or->hs_disconnect.value = (u8)value; > > + or->hs_disconnect.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector", > > + &value)) { > > + or->squelch_detector.value = (u8)value; > > + or->squelch_detector.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude", > > + &value)) { > > + or->hs_amplitude.value = (u8)value; > > + or->hs_amplitude.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration", > > + &value)) { > > + or->preemphasis_duration.value = (u8)value; > > + or->preemphasis_duration.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude", > > + &value)) { > > + or->preemphasis_amplitude.value = (u8)value; > > + or->preemphasis_amplitude.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time", > > + &value)) { > > + or->hs_rise_fall_time.value = (u8)value; > > + or->hs_rise_fall_time.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage", > > + &value)) { > > + or->hs_crossover_voltage.value = (u8)value; > > + or->hs_crossover_voltage.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance", > > + &value)) { > > + or->hs_output_impedance.value = (u8)value; > > + or->hs_output_impedance.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance", > > + &value)) { > > + or->ls_fs_output_impedance.value = (u8)value; > > + or->ls_fs_output_impedance.override = true; > > + } > > Are all these values board specific or IP specific? Can we add these > values as tables in driver? These are IP specific. The tuning is board specific. The plan is to add tables in the driver which lookup the values passed by device tree and program the override registers according. We are currently working on this. previous discussion: https://lore.kernel.org/linux-usb/1a43277a-94b9-4f95-314a-876291227982@canonical.com/ Thanks, Pavan > -- > ~Vinod ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters @ 2022-04-13 9:53 ` Pavan Kondeti 0 siblings, 0 replies; 36+ messages in thread From: Pavan Kondeti @ 2022-04-13 9:53 UTC (permalink / raw) To: Vinod Koul Cc: Sandeep Maheswaram, Bjorn Andersson, Rob Herring, Andy Gross, Kishon Vijay Abraham I, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski, devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, quic_kriskura Hi Vinod, On Wed, Apr 13, 2022 at 03:12:09PM +0530, Vinod Koul wrote: > On 03-03-22, 11:43, Sandeep Maheswaram wrote: > > Added support for overriding x0,x1,x2,x3 params for SNPS PHY by reading > > values from device tree. > > > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > > --- > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 192 ++++++++++++++++++++++++++ > > 1 file changed, 192 insertions(+) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > index 7e61202..b5aa06d 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > @@ -51,6 +51,48 @@ > > #define USB2_SUSPEND_N BIT(2) > > #define USB2_SUSPEND_N_SEL BIT(3) > > > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0 (0x6c) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X0 register bits*/ > > space after /* and before */ (checkpatch.pl --strict would warn you) > Pls fix it everywhere > > > +#define HS_DISCONNECT_MASK GENMASK(2, 0) > > +#define HS_DISCONNECT_SHIFT 0x0 > > + > > +#define SQUELCH_DETECTOR_MASK GENMASK(7, 5) > > +#define SQUELCH_DETECTOR_SHIFT 0x5 > > + > > + > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1 (0x70) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X1 register bits*/ > > +#define HS_AMPLITUDE_MASK GENMASK(3, 0) > > +#define HS_AMPLITUDE_SHIFT 0x0 > > + > > +#define PREEMPHASIS_DURATION_MASK BIT(5) > > +#define PREEMPHASIS_DURATION_SHIFT 0x5 > > + > > +#define PREEMPHASIS_AMPLITUDE_MASK GENMASK(7, 6) > > +#define PREEMPHASIS_AMPLITUDE_SHIFT 0x6 > > + > > + > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2 (0x74) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X2 register bits*/ > > +#define HS_RISE_FALL_MASK GENMASK(1, 0) > > +#define HS_RISE_FALL_SHIFT 0x0 > > + > > +#define HS_CROSSOVER_VOLTAGE_MASK GENMASK(3, 2) > > +#define HS_CROSSOVER_VOLTAGE_SHIFT 0x2 > > + > > +#define HS_OUTPUT_IMPEDANCE_MASK GENMASK(5, 4) > > +#define HS_OUTPUT_IMPEDANCE_SHIFT 0x4 > > + > > + > > +#define USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3 (0x78) > > + > > +/*USB_PHY_HS_PHY_OVERRIDE_X3 register bits*/ > > +#define LS_FS_OUTPUT_IMPEDANCE_MASK GENMASK(3, 0) > > +#define LS_FS_OUTPUT_IMPEDANCE_SHIFT 0x0 > > + > > #define USB2_PHY_USB_PHY_CFG0 (0x94) > > #define UTMI_PHY_DATAPATH_CTRL_OVERRIDE_EN BIT(0) > > #define UTMI_PHY_CMN_CTRL_OVERRIDE_EN BIT(1) > > @@ -65,6 +107,43 @@ static const char * const qcom_snps_hsphy_vreg_names[] = { > > > > #define SNPS_HS_NUM_VREGS ARRAY_SIZE(qcom_snps_hsphy_vreg_names) > > > > +/* struct override_param - structure holding snps phy overriding param > > + * set override true if the device tree property exists and read and assign > > + * to value > > + */ > > +struct override_param { > > + bool override; > > + u8 value; > > +}; > > + > > +/*struct override_params - structure holding snps phy overriding params > > + * @hs_disconnect: disconnect threshold > > + * @squelch_detector: threshold to detect valid high-speed data > > + * @hs_amplitude: high-speed DC level voltage > > + * @preemphasis_duration: duration for which the HS pre-emphasis current > > + * is sourced onto DP<#> or DM<#> > > + * @preemphasis_amplitude: current sourced to DP<#> and DM<#> after > > + * a J-to-K or K-to-J transition. > > + * @hs_rise_fall_time: rise/fall times of the high-speed waveform > > + * @hs_crossover_voltage: voltage at which the DP<#> and DM<#> > > + * signals cross while transmitting in HS mode > > + * @hs_output_impedance: driver source impedance to compensate for added series > > + * resistance on the USB > > + * @ls_fs_output_impedance: low and full-speed single-ended source > > + * impedance while driving high > > + */ > > +struct override_params { > > + struct override_param hs_disconnect; > > + struct override_param squelch_detector; > > + struct override_param hs_amplitude; > > + struct override_param preemphasis_duration; > > + struct override_param preemphasis_amplitude; > > + struct override_param hs_rise_fall_time; > > + struct override_param hs_crossover_voltage; > > + struct override_param hs_output_impedance; > > + struct override_param ls_fs_output_impedance; > > +}; > > + > > /** > > * struct qcom_snps_hsphy - snps hs phy attributes > > * > > @@ -87,6 +166,7 @@ struct qcom_snps_hsphy { > > struct clk *ref_clk; > > struct reset_control *phy_reset; > > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > + struct override_params overrides; > > > > bool phy_initialized; > > enum phy_mode mode; > > @@ -175,6 +255,7 @@ static int qcom_snps_hsphy_set_mode(struct phy *phy, enum phy_mode mode, > > static int qcom_snps_hsphy_init(struct phy *phy) > > { > > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > > + struct override_params *or = &hsphy->overrides; > > int ret; > > > > dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__); > > @@ -222,6 +303,60 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL1, > > VBUSVLDEXT0, VBUSVLDEXT0); > > > > + if (or->hs_disconnect.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > > + HS_DISCONNECT_MASK, > > + or->hs_disconnect.value << HS_DISCONNECT_SHIFT); > > + > > + if (or->squelch_detector.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0, > > + SQUELCH_DETECTOR_MASK, > > + or->squelch_detector.value << SQUELCH_DETECTOR_SHIFT); > > + > > + if (or->hs_amplitude.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > > + HS_AMPLITUDE_MASK, > > + or->hs_amplitude.value << HS_AMPLITUDE_SHIFT); > > + > > + if (or->preemphasis_duration.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > > + PREEMPHASIS_DURATION_MASK, > > + or->preemphasis_duration.value << PREEMPHASIS_DURATION_SHIFT); > > + > > + if (or->preemphasis_amplitude.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1, > > + PREEMPHASIS_AMPLITUDE_MASK, > > + or->preemphasis_amplitude.value << PREEMPHASIS_AMPLITUDE_SHIFT); > > + > > + if (or->hs_rise_fall_time.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > > + HS_RISE_FALL_MASK, > > + or->hs_rise_fall_time.value << HS_RISE_FALL_SHIFT); > > + > > + if (or->hs_crossover_voltage.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > > + HS_CROSSOVER_VOLTAGE_MASK, > > + or->hs_crossover_voltage.value << HS_CROSSOVER_VOLTAGE_SHIFT); > > + > > + if (or->hs_output_impedance.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2, > > + HS_OUTPUT_IMPEDANCE_MASK, > > + or->hs_output_impedance.value << HS_OUTPUT_IMPEDANCE_SHIFT); > > + > > + if (or->ls_fs_output_impedance.override) > > + qcom_snps_hsphy_write_mask(hsphy->base, > > + USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X3, > > + LS_FS_OUTPUT_IMPEDANCE_MASK, > > + or->ls_fs_output_impedance.value << LS_FS_OUTPUT_IMPEDANCE_SHIFT); > > + > > qcom_snps_hsphy_write_mask(hsphy->base, > > USB2_PHY_USB_PHY_HS_PHY_CTRL_COMMON2, > > VREGBYPASS, VREGBYPASS); > > @@ -292,12 +427,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > > struct qcom_snps_hsphy *hsphy; > > struct phy_provider *phy_provider; > > struct phy *generic_phy; > > + struct override_params *or; > > int ret, i; > > int num; > > + u32 value; > > > > hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL); > > if (!hsphy) > > return -ENOMEM; > > + or = &hsphy->overrides; > > > > hsphy->base = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(hsphy->base)) > > @@ -329,6 +467,60 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > > return ret; > > } > > > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-disconnect", > > + &value)) { > > + or->hs_disconnect.value = (u8)value; > > + or->hs_disconnect.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,squelch-detector", > > + &value)) { > > + or->squelch_detector.value = (u8)value; > > + or->squelch_detector.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-amplitude", > > + &value)) { > > + or->hs_amplitude.value = (u8)value; > > + or->hs_amplitude.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-duration", > > + &value)) { > > + or->preemphasis_duration.value = (u8)value; > > + or->preemphasis_duration.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,preemphasis-amplitude", > > + &value)) { > > + or->preemphasis_amplitude.value = (u8)value; > > + or->preemphasis_amplitude.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-rise-fall-time", > > + &value)) { > > + or->hs_rise_fall_time.value = (u8)value; > > + or->hs_rise_fall_time.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-crossover-voltage", > > + &value)) { > > + or->hs_crossover_voltage.value = (u8)value; > > + or->hs_crossover_voltage.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,hs-output-impedance", > > + &value)) { > > + or->hs_output_impedance.value = (u8)value; > > + or->hs_output_impedance.override = true; > > + } > > + > > + if (!of_property_read_u32(dev->of_node, "qcom,ls-fs-output-impedance", > > + &value)) { > > + or->ls_fs_output_impedance.value = (u8)value; > > + or->ls_fs_output_impedance.override = true; > > + } > > Are all these values board specific or IP specific? Can we add these > values as tables in driver? These are IP specific. The tuning is board specific. The plan is to add tables in the driver which lookup the values passed by device tree and program the override registers according. We are currently working on this. previous discussion: https://lore.kernel.org/linux-usb/1a43277a-94b9-4f95-314a-876291227982@canonical.com/ Thanks, Pavan > -- > ~Vinod -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 3/3] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device 2022-03-03 6:13 ` Sandeep Maheswaram @ 2022-03-03 6:13 ` Sandeep Maheswaram -1 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Overriding the SNPS Phy tuning parameters for SC7280 IDP device. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index ecbf2b8..0fbc5a1 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -317,6 +317,12 @@ vdda-pll-supply = <&vreg_l10c_0p8>; vdda33-supply = <&vreg_l2b_3p0>; vdda18-supply = <&vreg_l1c_1p8>; + qcom,hs-disconnect = <6>; + qcom,squelch-detector = <7>; + qcom,hs-amplitude = <11>; + qcom,hs-rise-fall-time = <2>; + qcom,hs-crossover-voltage = <1>; + qcom,hs-output-impedance = <1>; }; &usb_1_qmpphy { -- 2.7.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 3/3] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device @ 2022-03-03 6:13 ` Sandeep Maheswaram 0 siblings, 0 replies; 36+ messages in thread From: Sandeep Maheswaram @ 2022-03-03 6:13 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul, Greg Kroah-Hartman, Wesley Cheng, Stephen Boyd, Doug Anderson, Matthias Kaehlcke, Krzysztof Kozlowski Cc: devicetree, linux-arm-msm, linux-kernel, linux-phy, linux-usb, quic_pkondeti, quic_ppratap, Sandeep Maheswaram Overriding the SNPS Phy tuning parameters for SC7280 IDP device. Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> --- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi index ecbf2b8..0fbc5a1 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi @@ -317,6 +317,12 @@ vdda-pll-supply = <&vreg_l10c_0p8>; vdda33-supply = <&vreg_l2b_3p0>; vdda18-supply = <&vreg_l1c_1p8>; + qcom,hs-disconnect = <6>; + qcom,squelch-detector = <7>; + qcom,hs-amplitude = <11>; + qcom,hs-rise-fall-time = <2>; + qcom,hs-crossover-voltage = <1>; + qcom,hs-output-impedance = <1>; }; &usb_1_qmpphy { -- 2.7.4 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 36+ messages in thread
end of thread, other threads:[~2022-04-13 9:54 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-03 6:13 [PATCH v2 0/3] Add QCOM SNPS PHY overriding params support Sandeep Maheswaram 2022-03-03 6:13 ` Sandeep Maheswaram 2022-03-03 6:13 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: Add phy override params bindings Sandeep Maheswaram 2022-03-03 6:13 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Sandeep Maheswaram 2022-03-03 15:59 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-03 15:59 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-03 22:23 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Stephen Boyd 2022-03-03 22:23 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Stephen Boyd 2022-03-14 3:29 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 3:29 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 7:39 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 7:39 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 8:16 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 8:16 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 8:36 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 8:36 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 9:40 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 9:40 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 10:08 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 10:08 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 10:30 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 10:30 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 10:41 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 10:41 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 11:13 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 11:13 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Pavan Kondeti 2022-03-14 11:21 ` [PATCH v2 1/3] dt-bindings: phy: qcom,usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-14 11:21 ` [PATCH v2 1/3] dt-bindings: phy: qcom, usb-snps-femto-v2: " Krzysztof Kozlowski 2022-03-03 6:13 ` [PATCH v2 2/3] phy: qcom-snps: Add support for overriding phy tuning parameters Sandeep Maheswaram 2022-03-03 6:13 ` Sandeep Maheswaram 2022-04-13 9:42 ` Vinod Koul 2022-04-13 9:42 ` Vinod Koul 2022-04-13 9:53 ` Pavan Kondeti 2022-04-13 9:53 ` Pavan Kondeti 2022-03-03 6:13 ` [PATCH v2 3/3] arm64: dts: qcom: sc7280: Update SNPS Phy params for SC7280 IDP device Sandeep Maheswaram 2022-03-03 6:13 ` Sandeep Maheswaram
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.