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@xxxxxxxxxx> 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 >
