| Message ID | 20221229183410.683584-2-bryan.odonoghue@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | qcom: Add a method to manually toggle the DP pullup on HS USB PHY | expand |
On Thu, Dec 29, 2022 at 06:34:09PM +0000, Bryan O'Donoghue wrote: > The original Qualcomm driver for the HighSpeed USB PHY contains a flag > which tells the driver that the controller and PHY do not connect to VBUS. > > In this case an external IC such as a Type-C port manager supplies VBUS and > the VBUS signal is not routed to the SoC. This means we cannot detect the > presence or absence of VBUS and cannot take action based on it. > > Document the downstream boolean qcom,dp-manual-pullup to allow the HS PHY > implement the necessary logic. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> AFAIK it is not possible to route VBUS directly to the controller on these SoCs so this property would likely be added to the SoC dtsi (i.e. msm8916.dtsi and msm8939.dtsi) and used by all boards. This means we could just bind this behavior to the existing SoC-specific compatible (i.e. of_device_is_compatible(..., "qcom,usb-hs-phy-msm8916")) and avoid having an extra property. Thoughts? Thanks, Stephan > --- > Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml > index aa97478dd0161..c55a59df71ad0 100644 > --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml > @@ -80,6 +80,13 @@ properties: > the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address > - description: value > > + qcom,dp-manual-pullup: > + type: boolean > + description: This flag indicates to the HS USB PHY driver that it should > + enable or disable an internal pullup when powering on or > + powering off the HS PHY instead of toggling the value when VBUS > + is absent or present. > + > required: > - clocks > - clock-names > -- > 2.34.1 >
On 29/12/2022 18:57, Stephan Gerhold wrote: > AFAIK it is not possible to route VBUS directly to the controller on > these SoCs so this property would likely be added to the SoC dtsi > (i.e. msm8916.dtsi and msm8939.dtsi) and used by all boards. So db410c signals the SoC via GPIO 121 / USB_HS_ID https://fccid.io/2AFQA-DB410C/Schematics/Schematics-2816094.pdf Which causes ULPI_MISC_A_VBUSVLDEXT to be updated depending on the state VBUS. But not ULPI_MISC_A_VBUSVLDEXTSEL this is the additional register that downstream updates when "VBUS is not routed to the controller" I don't have a bit-level description of these registers at the moment so, I'm guessing that ULPI_MISC_A_VBUSVLDEXTSEL *is* being updated. The reason for that is if I just set ULPI_MISC_A_VBUSVLDEXT then as a device a host never sees my SoC via the internal USB hub. In other words, for me at any rate I need to see both - ULPI_MISC_A_VBUSVLDEXT - ULPI_MISC_A_VBUSVLDEXTSEL to get the pullup to work and hence the Hub/Host to detect the 8939. > This means we could just bind this behavior to the existing SoC-specific > compatible (i.e. of_device_is_compatible(..., "qcom,usb-hs-phy-msm8916")) > and avoid having an extra property. > > Thoughts? So. I'm OOO at the moment and didn't bring my db410c but TBH to me I don't see why we do this whole dance with the pullup on/off with VBUS. The right thing to do is to run an experiment statically setting - ULPI_MISC_A_VBUSVLDEXT - ULPI_MISC_A_VBUSVLDEXTSEL On/off at power on/off respectively on - db410c - My reference where I already know it works I'm not really seeing the utility of - partially waggling one of two registers with VBUS. Why not just push the pullup on with power-on and off with power-off.. Its worth an experiement if you have the time, if not I'll check it when I get back home. --- bod
On Thu, Dec 29, 2022 at 07:48:46PM +0000, Bryan O'Donoghue wrote: > On 29/12/2022 18:57, Stephan Gerhold wrote: > > AFAIK it is not possible to route VBUS directly to the controller on > > these SoCs so this property would likely be added to the SoC dtsi > > (i.e. msm8916.dtsi and msm8939.dtsi) and used by all boards. > > So db410c signals the SoC via GPIO 121 / USB_HS_ID > > https://fccid.io/2AFQA-DB410C/Schematics/Schematics-2816094.pdf > > Which causes ULPI_MISC_A_VBUSVLDEXT to be updated depending on the state > VBUS. > Correct. If I'm reading the DB410c schematic correctly the USB_HS_ID on DB410c is actually derived from VBUS on the micro USB port (and not the ID pin of the port as one would normally expect). > But not ULPI_MISC_A_VBUSVLDEXTSEL this is the additional register that > downstream updates when "VBUS is not routed to the controller" AFAICT ULPI_MISC_A_VBUSVLDEXTSEL is set in qcom_usb_hs_phy_set_mode() if the USB controller enables device mode? (And disabled again in host mode.) > > I don't have a bit-level description of these registers at the moment so, > I'm guessing that ULPI_MISC_A_VBUSVLDEXTSEL *is* being updated. > > The reason for that is if I just set ULPI_MISC_A_VBUSVLDEXT then as a device > a host never sees my SoC via the internal USB hub. > > In other words, for me at any rate I need to see both > > - ULPI_MISC_A_VBUSVLDEXT > - ULPI_MISC_A_VBUSVLDEXTSEL > > to get the pullup to work and hence the Hub/Host to detect the 8939. > The bit-level description of this register in the public APQ8016E TRM [1] is not very useful (page 1957): VBUSVLDEXTSEL = "External VBUS valid select" VBUSVLDEXT = "External VBUS valid indicator" But I think VBUSVLDEXTSEL basically means "Use external VBUS state from VBUSVLDEXT instead of internal detection". And VBUSVLDEXT then contains the actual VBUS state. The VBUS state is then probably used somewhere inside the USB controller or PHY to enable necessary USB protocol things such as enabling this "DP pull-up" (to be fair I have no idea how the USB protocol really works :)). [1]: https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf > > This means we could just bind this behavior to the existing SoC-specific > > compatible (i.e. of_device_is_compatible(..., "qcom,usb-hs-phy-msm8916")) > > and avoid having an extra property. > > > > Thoughts? > > So. I'm OOO at the moment and didn't bring my db410c but TBH to me I don't > see why we do this whole dance with the pullup on/off with VBUS. > > The right thing to do is to run an experiment statically setting > > - ULPI_MISC_A_VBUSVLDEXT > - ULPI_MISC_A_VBUSVLDEXTSEL > > On/off at power on/off respectively on > > - db410c > - My reference where I already know it works > > I'm not really seeing the utility of - partially waggling one of two > registers with VBUS. > > Why not just push the pullup on with power-on and off with power-off.. > I don't feel qualified to comment on this. I'd just follow the suggestion from the docs here to make VBUSVLDEXT effectively represent the result of the external VBUS detection (see APQ8016E TRM [1] section 6.4.5.3.3 Software control for SESS_VALID, page 1927). DB410c and other devices with USB hub are obviously a bit special, but I think it works correctly for DB410c at the moment because its USB_HS_ID GPIO basically indicates the incoming VBUS on the micro USB port. There might be some funky design where it is completely impossible to detect the incoming VBUS. In that case we have no choice but to force the detected VBUS state on permanently. Thanks, Stephan [1]: https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf
On 29/12/2022 19:34, Bryan O'Donoghue wrote: > The original Qualcomm driver for the HighSpeed USB PHY contains a flag > which tells the driver that the controller and PHY do not connect to VBUS. > > In this case an external IC such as a Type-C port manager supplies VBUS and > the VBUS signal is not routed to the SoC. This means we cannot detect the > presence or absence of VBUS and cannot take action based on it. > > Document the downstream boolean qcom,dp-manual-pullup to allow the HS PHY > implement the necessary logic. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml > index aa97478dd0161..c55a59df71ad0 100644 > --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml > @@ -80,6 +80,13 @@ properties: > the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address > - description: value > > + qcom,dp-manual-pullup: > + type: boolean > + description: This flag indicates to the HS USB PHY driver that it should Drop references to the driver and rephrase it to describe the controller behavior/feature or the board layout/configuration, not the driver. The same applies to property name - "manual pullup" is describing driver, not hardware. > + enable or disable an internal pullup when powering on or > + powering off the HS PHY instead of toggling the value when VBUS > + is absent or present. Wrong indentation. It's two spaces. > + > required: > - clocks > - clock-names Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml index aa97478dd0161..c55a59df71ad0 100644 --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml @@ -80,6 +80,13 @@ properties: the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address - description: value + qcom,dp-manual-pullup: + type: boolean + description: This flag indicates to the HS USB PHY driver that it should + enable or disable an internal pullup when powering on or + powering off the HS PHY instead of toggling the value when VBUS + is absent or present. + required: - clocks - clock-names
The original Qualcomm driver for the HighSpeed USB PHY contains a flag which tells the driver that the controller and PHY do not connect to VBUS. In this case an external IC such as a Type-C port manager supplies VBUS and the VBUS signal is not routed to the SoC. This means we cannot detect the presence or absence of VBUS and cannot take action based on it. Document the downstream boolean qcom,dp-manual-pullup to allow the HS PHY implement the necessary logic. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml | 7 +++++++ 1 file changed, 7 insertions(+)