Message ID | 20221229183410.683584-3-bryan.odonoghue@linaro.org |
---|---|
State | New |
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:10PM +0000, Bryan O'Donoghue wrote: > Downstream has a flag called qcom,dp-manual-pullup which informs the > downstream driver if it should toggle ULPI_MISC_A_VBUSVLDEXTSEL and > ULPI_MISC_A_VBUSVLDEXT. > > Downstream states: > > "qcom,dp-manual-pullup: If present, vbus is not routed to USB > controller/phy and controller driver therefore enables pull-up > explicitly before starting controller using usbcmd run/stop bit." > > Working with a system that has both an external Type-C port controller and > an internal USB Hub results in a situation where VBUS is not connected to > the SoC. > > In this case we still need to set the DP pullup. > > This patch enables and disables the DP pullup on PHY power_on and power_off > respectively if the DT has declared the bool "qcom,enable-vbus-pullup" > effectively replicating the downstream logic to the same effect. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> I think ideally you would add an extcon device (or equivalent, e.g. power supply) somewhere in your Type-C setup so that the VBUS state is set correctly and does not need to be forced permanently. The chipidea USB driver will also benefit from this because AFAICT it uses that information to power down the USB PHY and controller entirely when in device mode and there is no USB cable connected. But I agree that setting up a proper extcon device can be difficult, especially during early bring-up where you just want USB to work and don't care much about power saving. So I still think that making USB work without extcon (like your patch does) is a useful change. For the implementation, I think this flag should effectively emulate the logic that is currently used if you assign an extcon, except that the VBUS state is permanently forced active. Right now your qcom_usb_hs_phy_enable_dp_pullup() function kind of duplicates logic that already exists in the driver, which already has code to set VBUSVLDEXTSEL and VBUSVLDEXT. qcom_usb_hs_phy_set_mode() should be modified to use the else branch (as if vbus_edev was set), which will set VBUSVLDEXTSEL for you. Without extcon you currently use the if (!uphy->vbus_edev) branch, which will enable the internal VBUS detection in the PHY (the ULPI_INT_SESS_VALID code). This does not make any sense IMO if VBUS is not connected to the SoC. And then you basically just need to call qcom_usb_hs_phy_vbus_notifier() to set the VBUSVLDEXT forcefully (rather than relying on the extcon detection). An alternative that I've occasionally used for early bring-up is to simply use a dummy extcon driver [1] that permanently reports active VBUS. The end result is the same. While it's clearly a hack perhaps this makes it a bit more clear that ideally you really should try to assign an extcon device, to avoid keeping the USB controller and PHY on permanently. Thanks, Stephan [1]: https://github.com/msm8916-mainline/linux/commit/3d029e6d4303e125aa013c501308d2d98e3cdc89 > --- > drivers/phy/qualcomm/phy-qcom-usb-hs.c | 36 ++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c b/drivers/phy/qualcomm/phy-qcom-usb-hs.c > index 53e46c220a3aa..45c94f6722c66 100644 > --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c > +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c > @@ -37,6 +37,7 @@ struct qcom_usb_hs_phy { > struct ulpi_seq *init_seq; > struct extcon_dev *vbus_edev; > struct notifier_block vbus_notify; > + u8 enable_dp_pullup:1; > }; > > static int qcom_usb_hs_phy_set_mode(struct phy *phy, > @@ -105,6 +106,23 @@ qcom_usb_hs_phy_vbus_notifier(struct notifier_block *nb, unsigned long event, > return ulpi_write(uphy->ulpi, addr, ULPI_MISC_A_VBUSVLDEXT); > } > > +static int qcom_usb_hs_phy_enable_dp_pullup(struct ulpi *ulpi, bool enable) > +{ > + u8 addr; > + int ret; > + > + if (enable) > + addr = ULPI_SET(ULPI_MISC_A); > + else > + addr = ULPI_CLR(ULPI_MISC_A); > + > + ret = ulpi_write(ulpi, addr, ULPI_MISC_A_VBUSVLDEXTSEL); > + if (ret) > + return ret; > + > + return ulpi_write(ulpi, addr, ULPI_MISC_A_VBUSVLDEXT); > +} > + > static int qcom_usb_hs_phy_power_on(struct phy *phy) > { > struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy); > @@ -154,6 +172,12 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy) > goto err_ulpi; > } > > + if (uphy->enable_dp_pullup) { > + ret = qcom_usb_hs_phy_enable_dp_pullup(ulpi, true); > + if (ret) > + goto err_ulpi; > + } > + > if (uphy->vbus_edev) { > state = extcon_get_state(uphy->vbus_edev, EXTCON_USB); > /* setup initial state */ > @@ -180,10 +204,19 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy) > static int qcom_usb_hs_phy_power_off(struct phy *phy) > { > struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy); > + struct ulpi *ulpi = uphy->ulpi; > + int ret; > > if (uphy->vbus_edev) > extcon_unregister_notifier(uphy->vbus_edev, EXTCON_USB, > &uphy->vbus_notify); > + > + if (uphy->enable_dp_pullup) { > + ret = qcom_usb_hs_phy_enable_dp_pullup(ulpi, false); > + if (ret) > + return ret; > + } > + > regulator_disable(uphy->v3p3); > regulator_disable(uphy->v1p8); > clk_disable_unprepare(uphy->sleep_clk); > @@ -229,6 +262,9 @@ static int qcom_usb_hs_phy_probe(struct ulpi *ulpi) > /* NUL terminate */ > uphy->init_seq[size / 2].addr = uphy->init_seq[size / 2].val = 0; > > + if (of_property_read_bool(ulpi->dev.of_node, "qcom,dp-manual-pullup")) > + uphy->enable_dp_pullup = 1; > + > uphy->ref_clk = clk = devm_clk_get(&ulpi->dev, "ref"); > if (IS_ERR(clk)) > return PTR_ERR(clk); > -- > 2.34.1 >
On 29/12/2022 19:45, Stephan Gerhold wrote: > For the implementation, I think this flag should effectively emulate the > logic that is currently used if you assign an extcon Actually that's fine, its a better solution than pushing the pullup constantly as I just suggested in my previous email, there's also zero potential impact to existing boards this way, so, that's obviously better. --- bod
On 29/12/2022 21:02, Stephan Gerhold wrote: > On Thu, Dec 29, 2022 at 08:45:15PM +0100, Stephan Gerhold wrote: >> On Thu, Dec 29, 2022 at 06:34:10PM +0000, Bryan O'Donoghue wrote: >>> Downstream has a flag called qcom,dp-manual-pullup which informs the >>> downstream driver if it should toggle ULPI_MISC_A_VBUSVLDEXTSEL and >>> ULPI_MISC_A_VBUSVLDEXT. >>> >>> Downstream states: >>> >>> "qcom,dp-manual-pullup: If present, vbus is not routed to USB >>> controller/phy and controller driver therefore enables pull-up >>> explicitly before starting controller using usbcmd run/stop bit." >>> >>> Working with a system that has both an external Type-C port controller and >>> an internal USB Hub results in a situation where VBUS is not connected to >>> the SoC. >>> >>> In this case we still need to set the DP pullup. >>> >>> This patch enables and disables the DP pullup on PHY power_on and power_off >>> respectively if the DT has declared the bool "qcom,enable-vbus-pullup" >>> effectively replicating the downstream logic to the same effect. >>> >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> [...] >> >> An alternative that I've occasionally used for early bring-up is to >> simply use a dummy extcon driver [1] that permanently reports active >> VBUS. The end result is the same. While it's clearly a hack perhaps this >> makes it a bit more clear that ideally you really should try to assign >> an extcon device, to avoid keeping the USB controller and PHY on >> permanently. >> > > BTW another nice trick that should work in all almost all cases is to > use the state of the PMIC USB_IN pin as VBUS detection. All battery- > powered devices I have seen route the USB VBUS to PM8916 USB_IN. And > even many boards like DB410c seem to do that or at least permanently > supply 5V there. In all these cases the &pm8916_usbin extcon will > report a VBUS state that should make USB work. > > Have you tried using that on your MSM8939 board with the Type-C setup? > > &pm8916_usbin { > status = "okay"; > }; > > &usb { > status = "okay"; > usb-role-switch; > extcon = <&pm8916_usbin>; > }; > > &usb_hs_phy { > extcon = <&pm8916_usbin>; > }; > > Stephan I checked USBIN before my last email reply. Its possible its connected but its not there on the 8939 schematic I have. --- bod
On Thu, Dec 29, 2022 at 09:20:03PM +0000, Bryan O'Donoghue wrote: > On 29/12/2022 21:11, Stephan Gerhold wrote: > > Then it doesn't seem to be a particularly complete schematic. 😄 > > PM8916 definitely has USB_IN pads (pad # N13, P13). 😄 > > Let me check again. > > No sorry USB_IN_0 and USB_IN_1 are connected to +5V in my schematic, I did > check USB_IN I just forgot why it wasn't usable... +5v not VBUS is what its > connected to. > That is still good enough to replace qcom,dp-manual-pullup though. If you have +5V connected to USB_IN then &pm8916_usbin should report active VBUS permanently. Set it as extcon for &usb and &usb_hs_phy and you should have VBUSVLDEXT "pulled-up" like in your patch here, without any driver changes. :-) It's a bit of a hack of course, which probably deserves a comment in the device tree. But since it should not make any functional difference compared to this patch the approach might be easier than getting this patch finalized & accepted. I leave that up to you :) Stephan
On 29/12/2022 21:41, Stephan Gerhold wrote:
> That is still good enough to replace qcom,dp-manual-pullup though.
But there's no requirement to tie USB_IN_x high if VBUS is not connected
to it.
---
bod
On Thu, Dec 29, 2022 at 11:38:04PM +0000, Bryan O'Donoghue wrote: > On 29/12/2022 21:41, Stephan Gerhold wrote: > > That is still good enough to replace qcom,dp-manual-pullup though. > > But there's no requirement to tie USB_IN_x high if VBUS is not connected to > it. > I've yet to find any device where USB_IN is not connected in a usable way. But as I said I leave it up to you - it would be nice to have a proper solution for devices where VBUS state cannot be determined at all. Those devices should be rare though so if &pm8916_usbin works in your case maybe the time would be better invested into other open problems right now. :) Thanks, Stephan
diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c b/drivers/phy/qualcomm/phy-qcom-usb-hs.c index 53e46c220a3aa..45c94f6722c66 100644 --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c @@ -37,6 +37,7 @@ struct qcom_usb_hs_phy { struct ulpi_seq *init_seq; struct extcon_dev *vbus_edev; struct notifier_block vbus_notify; + u8 enable_dp_pullup:1; }; static int qcom_usb_hs_phy_set_mode(struct phy *phy, @@ -105,6 +106,23 @@ qcom_usb_hs_phy_vbus_notifier(struct notifier_block *nb, unsigned long event, return ulpi_write(uphy->ulpi, addr, ULPI_MISC_A_VBUSVLDEXT); } +static int qcom_usb_hs_phy_enable_dp_pullup(struct ulpi *ulpi, bool enable) +{ + u8 addr; + int ret; + + if (enable) + addr = ULPI_SET(ULPI_MISC_A); + else + addr = ULPI_CLR(ULPI_MISC_A); + + ret = ulpi_write(ulpi, addr, ULPI_MISC_A_VBUSVLDEXTSEL); + if (ret) + return ret; + + return ulpi_write(ulpi, addr, ULPI_MISC_A_VBUSVLDEXT); +} + static int qcom_usb_hs_phy_power_on(struct phy *phy) { struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy); @@ -154,6 +172,12 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy) goto err_ulpi; } + if (uphy->enable_dp_pullup) { + ret = qcom_usb_hs_phy_enable_dp_pullup(ulpi, true); + if (ret) + goto err_ulpi; + } + if (uphy->vbus_edev) { state = extcon_get_state(uphy->vbus_edev, EXTCON_USB); /* setup initial state */ @@ -180,10 +204,19 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy) static int qcom_usb_hs_phy_power_off(struct phy *phy) { struct qcom_usb_hs_phy *uphy = phy_get_drvdata(phy); + struct ulpi *ulpi = uphy->ulpi; + int ret; if (uphy->vbus_edev) extcon_unregister_notifier(uphy->vbus_edev, EXTCON_USB, &uphy->vbus_notify); + + if (uphy->enable_dp_pullup) { + ret = qcom_usb_hs_phy_enable_dp_pullup(ulpi, false); + if (ret) + return ret; + } + regulator_disable(uphy->v3p3); regulator_disable(uphy->v1p8); clk_disable_unprepare(uphy->sleep_clk); @@ -229,6 +262,9 @@ static int qcom_usb_hs_phy_probe(struct ulpi *ulpi) /* NUL terminate */ uphy->init_seq[size / 2].addr = uphy->init_seq[size / 2].val = 0; + if (of_property_read_bool(ulpi->dev.of_node, "qcom,dp-manual-pullup")) + uphy->enable_dp_pullup = 1; + uphy->ref_clk = clk = devm_clk_get(&ulpi->dev, "ref"); if (IS_ERR(clk)) return PTR_ERR(clk);
Downstream has a flag called qcom,dp-manual-pullup which informs the downstream driver if it should toggle ULPI_MISC_A_VBUSVLDEXTSEL and ULPI_MISC_A_VBUSVLDEXT. Downstream states: "qcom,dp-manual-pullup: If present, vbus is not routed to USB controller/phy and controller driver therefore enables pull-up explicitly before starting controller using usbcmd run/stop bit." Working with a system that has both an external Type-C port controller and an internal USB Hub results in a situation where VBUS is not connected to the SoC. In this case we still need to set the DP pullup. This patch enables and disables the DP pullup on PHY power_on and power_off respectively if the DT has declared the bool "qcom,enable-vbus-pullup" effectively replicating the downstream logic to the same effect. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/phy/qualcomm/phy-qcom-usb-hs.c | 36 ++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)