| Message ID | 20240527-topic-sm8x50-upstream-phy-combo-typec-mux-v2-4-a03e68d7b8fc@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | arm64: qcom: allow up to 4 lanes for the Type-C DisplayPort Altmode | expand |
On Mon, May 27, 2024 at 10:42:36AM +0200, Neil Armstrong wrote: > Register a typec mux in order to change the PHY mode on the Type-C > mux events depending on the mode and the svid when in Altmode setup. > > The DisplayPort phy should be left enabled if is still powered on > by the DRM DisplayPort controller, so bail out until the DisplayPort > PHY is not powered off. > > The Type-C Mode/SVID only changes on plug/unplug, and USB SAFE states > will be set in between of USB-Only, Combo and DisplayPort Only so > this will leave enough time to the DRM DisplayPort controller to > turn of the DisplayPort PHY. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 123 ++++++++++++++++++++++++++++-- > 1 file changed, 118 insertions(+), 5 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > index 788e4c05eaf2..b55ab08d44c2 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > @@ -19,6 +19,7 @@ > #include <linux/reset.h> > #include <linux/slab.h> > #include <linux/usb/typec.h> > +#include <linux/usb/typec_dp.h> > #include <linux/usb/typec_mux.h> > > #include <drm/bridge/aux-bridge.h> > @@ -1527,6 +1528,10 @@ struct qmp_combo { > > struct typec_switch_dev *sw; > enum typec_orientation orientation; > + > + struct typec_mux_dev *mux; > + unsigned long mux_mode; > + unsigned int svid; > }; > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); > @@ -3353,17 +3358,112 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, > return 0; > } > > -static void qmp_combo_typec_unregister(void *data) > +static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state) > +{ > + struct qmp_combo *qmp = typec_mux_get_drvdata(mux); > + const struct qmp_phy_cfg *cfg = qmp->cfg; > + enum qphy_mode new_mode; > + unsigned int svid; > + > + if (state->mode == qmp->mode) > + return 0; > + > + mutex_lock(&qmp->phy_mutex); > + > + if (state->alt) > + svid = state->alt->svid; > + else > + svid = 0; // No SVID > + > + if (svid == USB_TYPEC_DP_SID) { > + switch (state->mode) { > + /* DP Only */ > + case TYPEC_DP_STATE_C: > + case TYPEC_DP_STATE_E: > + new_mode = QPHY_MODE_DP_ONLY; > + break; > + > + /* DP + USB */ > + case TYPEC_DP_STATE_D: > + case TYPEC_DP_STATE_F: > + > + /* Safe fallback...*/ > + default: > + new_mode = QPHY_MODE_COMBO; > + break; > + } > + } else { > + /* Only switch to USB_ONLY when we know we only have USB3 */ > + if (qmp->mux_mode == TYPEC_MODE_USB3) > + new_mode = QPHY_MODE_USB_ONLY; > + else > + new_mode = QPHY_MODE_COMBO; > + } > + > + if (new_mode == qmp->init_mode) { > + dev_dbg(qmp->dev, "typec_mux_set: same phy mode, bail out\n"); > + qmp->mode = state->mode; > + goto out; > + } > + > + if (qmp->init_mode != QPHY_MODE_USB_ONLY && qmp->dp_powered_on) { > + dev_dbg(qmp->dev, "typec_mux_set: DP is still powered on, delaying switch\n"); > + goto out; > + } > + > + dev_dbg(qmp->dev, "typec_mux_set: switching from phy mode %d to %d\n", > + qmp->init_mode, new_mode); > + > + qmp->mux_mode = state->mode; > + qmp->init_mode = new_mode; > + > + if (qmp->init_count) { > + if (qmp->usb_init_count) > + qmp_combo_usb_power_off(qmp->usb_phy); > + if (qmp->dp_init_count) > + writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL); > + qmp_combo_com_exit(qmp, true); > + > + /* Now everything's powered down, power up the right PHYs */ > + > + qmp_combo_com_init(qmp, true); > + if (qmp->init_mode == QPHY_MODE_DP_ONLY && qmp->usb_init_count) { > + qmp->usb_init_count--; Can we move this clause next to actually powering USB part off? > + } else if (qmp->init_mode != QPHY_MODE_DP_ONLY) { > + qmp_combo_usb_power_on(qmp->usb_phy); > + if (!qmp->usb_init_count) > + qmp->usb_init_count++; > + } > + if (qmp->init_mode != QPHY_MODE_USB_ONLY && qmp->dp_init_count) > + cfg->dp_aux_init(qmp); Does dp_init_count reflect the actual necessity to bring up the DP part up? Maybe we can unify the code between this function and qmp_combo_typec_switch_set()? I don't like that it is unobvious whether these two functions will results in the same state or not depending on the order in which they are being called. > + } > + > +out: > + mutex_unlock(&qmp->phy_mutex); > + > + return 0; > +} > + > +static void qmp_combo_typec_switch_unregister(void *data) > { > struct qmp_combo *qmp = data; > > typec_switch_unregister(qmp->sw); > } > > -static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) > +static void qmp_combo_typec_mux_unregister(void *data) > +{ > + struct qmp_combo *qmp = data; > + > + typec_mux_unregister(qmp->mux); > +} > + > +static int qmp_combo_typec_register(struct qmp_combo *qmp) > { > struct typec_switch_desc sw_desc = {}; > + struct typec_mux_desc mux_desc = { }; > struct device *dev = qmp->dev; > + int ret; > > sw_desc.drvdata = qmp; > sw_desc.fwnode = dev->fwnode; > @@ -3374,10 +3474,23 @@ static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) > return PTR_ERR(qmp->sw); > } > > - return devm_add_action_or_reset(dev, qmp_combo_typec_unregister, qmp); > + ret = devm_add_action_or_reset(dev, qmp_combo_typec_switch_unregister, qmp); > + if (ret) > + return ret; > + > + mux_desc.drvdata = qmp; > + mux_desc.fwnode = dev->fwnode; > + mux_desc.set = qmp_combo_typec_mux_set; > + qmp->mux = typec_mux_register(dev, &mux_desc); > + if (IS_ERR(qmp->mux)) { > + dev_err(dev, "Unable to register typec mux: %pe\n", qmp->mux); > + return PTR_ERR(qmp->mux); > + } > + > + return devm_add_action_or_reset(dev, qmp_combo_typec_mux_unregister, qmp); > } > #else > -static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) > +static int qmp_combo_typec_register(struct qmp_combo *qmp) > { > return 0; > } > @@ -3609,7 +3722,7 @@ static int qmp_combo_probe(struct platform_device *pdev) > if (ret) > goto err_node_put; > > - ret = qmp_combo_typec_switch_register(qmp); > + ret = qmp_combo_typec_register(qmp); > if (ret) > goto err_node_put; > > > -- > 2.34.1 >
On 27/05/2024 10:57, Dmitry Baryshkov wrote: > On Mon, May 27, 2024 at 10:42:36AM +0200, Neil Armstrong wrote: >> Register a typec mux in order to change the PHY mode on the Type-C >> mux events depending on the mode and the svid when in Altmode setup. >> >> The DisplayPort phy should be left enabled if is still powered on >> by the DRM DisplayPort controller, so bail out until the DisplayPort >> PHY is not powered off. >> >> The Type-C Mode/SVID only changes on plug/unplug, and USB SAFE states >> will be set in between of USB-Only, Combo and DisplayPort Only so >> this will leave enough time to the DRM DisplayPort controller to >> turn of the DisplayPort PHY. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 123 ++++++++++++++++++++++++++++-- >> 1 file changed, 118 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c >> index 788e4c05eaf2..b55ab08d44c2 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c >> @@ -19,6 +19,7 @@ >> #include <linux/reset.h> >> #include <linux/slab.h> >> #include <linux/usb/typec.h> >> +#include <linux/usb/typec_dp.h> >> #include <linux/usb/typec_mux.h> >> >> #include <drm/bridge/aux-bridge.h> >> @@ -1527,6 +1528,10 @@ struct qmp_combo { >> >> struct typec_switch_dev *sw; >> enum typec_orientation orientation; >> + >> + struct typec_mux_dev *mux; >> + unsigned long mux_mode; >> + unsigned int svid; >> }; >> >> static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); >> @@ -3353,17 +3358,112 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, >> return 0; >> } >> >> -static void qmp_combo_typec_unregister(void *data) >> +static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state) >> +{ >> + struct qmp_combo *qmp = typec_mux_get_drvdata(mux); >> + const struct qmp_phy_cfg *cfg = qmp->cfg; >> + enum qphy_mode new_mode; >> + unsigned int svid; >> + >> + if (state->mode == qmp->mode) >> + return 0; >> + >> + mutex_lock(&qmp->phy_mutex); >> + >> + if (state->alt) >> + svid = state->alt->svid; >> + else >> + svid = 0; // No SVID >> + >> + if (svid == USB_TYPEC_DP_SID) { >> + switch (state->mode) { >> + /* DP Only */ >> + case TYPEC_DP_STATE_C: >> + case TYPEC_DP_STATE_E: >> + new_mode = QPHY_MODE_DP_ONLY; >> + break; >> + >> + /* DP + USB */ >> + case TYPEC_DP_STATE_D: >> + case TYPEC_DP_STATE_F: >> + >> + /* Safe fallback...*/ >> + default: >> + new_mode = QPHY_MODE_COMBO; >> + break; >> + } >> + } else { >> + /* Only switch to USB_ONLY when we know we only have USB3 */ >> + if (qmp->mux_mode == TYPEC_MODE_USB3) >> + new_mode = QPHY_MODE_USB_ONLY; >> + else >> + new_mode = QPHY_MODE_COMBO; >> + } >> + >> + if (new_mode == qmp->init_mode) { >> + dev_dbg(qmp->dev, "typec_mux_set: same phy mode, bail out\n"); >> + qmp->mode = state->mode; >> + goto out; >> + } >> + >> + if (qmp->init_mode != QPHY_MODE_USB_ONLY && qmp->dp_powered_on) { >> + dev_dbg(qmp->dev, "typec_mux_set: DP is still powered on, delaying switch\n"); >> + goto out; >> + } >> + >> + dev_dbg(qmp->dev, "typec_mux_set: switching from phy mode %d to %d\n", >> + qmp->init_mode, new_mode); >> + >> + qmp->mux_mode = state->mode; >> + qmp->init_mode = new_mode; >> + >> + if (qmp->init_count) { >> + if (qmp->usb_init_count) >> + qmp_combo_usb_power_off(qmp->usb_phy); >> + if (qmp->dp_init_count) >> + writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL); >> + qmp_combo_com_exit(qmp, true); >> + >> + /* Now everything's powered down, power up the right PHYs */ >> + >> + qmp_combo_com_init(qmp, true); >> + if (qmp->init_mode == QPHY_MODE_DP_ONLY && qmp->usb_init_count) { >> + qmp->usb_init_count--; > > Can we move this clause next to actually powering USB part off? > >> + } else if (qmp->init_mode != QPHY_MODE_DP_ONLY) { >> + qmp_combo_usb_power_on(qmp->usb_phy); >> + if (!qmp->usb_init_count) >> + qmp->usb_init_count++; >> + } >> + if (qmp->init_mode != QPHY_MODE_USB_ONLY && qmp->dp_init_count) >> + cfg->dp_aux_init(qmp); > > Does dp_init_count reflect the actual necessity to bring up the DP part > up? Maybe we can unify the code between this function and > qmp_combo_typec_switch_set()? I don't like that it is unobvious whether > these two functions will results in the same state or not depending on > the order in which they are being called. Yep, I'll try to use a common function, so any switch/mux call would use the same process, and I can probably simplify it. Thanks, Neil > >> + } >> + >> +out: >> + mutex_unlock(&qmp->phy_mutex); >> + >> + return 0; >> +} >> + >> +static void qmp_combo_typec_switch_unregister(void *data) >> { >> struct qmp_combo *qmp = data; >> >> typec_switch_unregister(qmp->sw); >> } >> >> -static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) >> +static void qmp_combo_typec_mux_unregister(void *data) >> +{ >> + struct qmp_combo *qmp = data; >> + >> + typec_mux_unregister(qmp->mux); >> +} >> + >> +static int qmp_combo_typec_register(struct qmp_combo *qmp) >> { >> struct typec_switch_desc sw_desc = {}; >> + struct typec_mux_desc mux_desc = { }; >> struct device *dev = qmp->dev; >> + int ret; >> >> sw_desc.drvdata = qmp; >> sw_desc.fwnode = dev->fwnode; >> @@ -3374,10 +3474,23 @@ static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) >> return PTR_ERR(qmp->sw); >> } >> >> - return devm_add_action_or_reset(dev, qmp_combo_typec_unregister, qmp); >> + ret = devm_add_action_or_reset(dev, qmp_combo_typec_switch_unregister, qmp); >> + if (ret) >> + return ret; >> + >> + mux_desc.drvdata = qmp; >> + mux_desc.fwnode = dev->fwnode; >> + mux_desc.set = qmp_combo_typec_mux_set; >> + qmp->mux = typec_mux_register(dev, &mux_desc); >> + if (IS_ERR(qmp->mux)) { >> + dev_err(dev, "Unable to register typec mux: %pe\n", qmp->mux); >> + return PTR_ERR(qmp->mux); >> + } >> + >> + return devm_add_action_or_reset(dev, qmp_combo_typec_mux_unregister, qmp); >> } >> #else >> -static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) >> +static int qmp_combo_typec_register(struct qmp_combo *qmp) >> { >> return 0; >> } >> @@ -3609,7 +3722,7 @@ static int qmp_combo_probe(struct platform_device *pdev) >> if (ret) >> goto err_node_put; >> >> - ret = qmp_combo_typec_switch_register(qmp); >> + ret = qmp_combo_typec_register(qmp); >> if (ret) >> goto err_node_put; >> >> >> -- >> 2.34.1 >> >
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index 788e4c05eaf2..b55ab08d44c2 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -19,6 +19,7 @@ #include <linux/reset.h> #include <linux/slab.h> #include <linux/usb/typec.h> +#include <linux/usb/typec_dp.h> #include <linux/usb/typec_mux.h> #include <drm/bridge/aux-bridge.h> @@ -1527,6 +1528,10 @@ struct qmp_combo { struct typec_switch_dev *sw; enum typec_orientation orientation; + + struct typec_mux_dev *mux; + unsigned long mux_mode; + unsigned int svid; }; static void qmp_v3_dp_aux_init(struct qmp_combo *qmp); @@ -3353,17 +3358,112 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, return 0; } -static void qmp_combo_typec_unregister(void *data) +static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state) +{ + struct qmp_combo *qmp = typec_mux_get_drvdata(mux); + const struct qmp_phy_cfg *cfg = qmp->cfg; + enum qphy_mode new_mode; + unsigned int svid; + + if (state->mode == qmp->mode) + return 0; + + mutex_lock(&qmp->phy_mutex); + + if (state->alt) + svid = state->alt->svid; + else + svid = 0; // No SVID + + if (svid == USB_TYPEC_DP_SID) { + switch (state->mode) { + /* DP Only */ + case TYPEC_DP_STATE_C: + case TYPEC_DP_STATE_E: + new_mode = QPHY_MODE_DP_ONLY; + break; + + /* DP + USB */ + case TYPEC_DP_STATE_D: + case TYPEC_DP_STATE_F: + + /* Safe fallback...*/ + default: + new_mode = QPHY_MODE_COMBO; + break; + } + } else { + /* Only switch to USB_ONLY when we know we only have USB3 */ + if (qmp->mux_mode == TYPEC_MODE_USB3) + new_mode = QPHY_MODE_USB_ONLY; + else + new_mode = QPHY_MODE_COMBO; + } + + if (new_mode == qmp->init_mode) { + dev_dbg(qmp->dev, "typec_mux_set: same phy mode, bail out\n"); + qmp->mode = state->mode; + goto out; + } + + if (qmp->init_mode != QPHY_MODE_USB_ONLY && qmp->dp_powered_on) { + dev_dbg(qmp->dev, "typec_mux_set: DP is still powered on, delaying switch\n"); + goto out; + } + + dev_dbg(qmp->dev, "typec_mux_set: switching from phy mode %d to %d\n", + qmp->init_mode, new_mode); + + qmp->mux_mode = state->mode; + qmp->init_mode = new_mode; + + if (qmp->init_count) { + if (qmp->usb_init_count) + qmp_combo_usb_power_off(qmp->usb_phy); + if (qmp->dp_init_count) + writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL); + qmp_combo_com_exit(qmp, true); + + /* Now everything's powered down, power up the right PHYs */ + + qmp_combo_com_init(qmp, true); + if (qmp->init_mode == QPHY_MODE_DP_ONLY && qmp->usb_init_count) { + qmp->usb_init_count--; + } else if (qmp->init_mode != QPHY_MODE_DP_ONLY) { + qmp_combo_usb_power_on(qmp->usb_phy); + if (!qmp->usb_init_count) + qmp->usb_init_count++; + } + if (qmp->init_mode != QPHY_MODE_USB_ONLY && qmp->dp_init_count) + cfg->dp_aux_init(qmp); + } + +out: + mutex_unlock(&qmp->phy_mutex); + + return 0; +} + +static void qmp_combo_typec_switch_unregister(void *data) { struct qmp_combo *qmp = data; typec_switch_unregister(qmp->sw); } -static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) +static void qmp_combo_typec_mux_unregister(void *data) +{ + struct qmp_combo *qmp = data; + + typec_mux_unregister(qmp->mux); +} + +static int qmp_combo_typec_register(struct qmp_combo *qmp) { struct typec_switch_desc sw_desc = {}; + struct typec_mux_desc mux_desc = { }; struct device *dev = qmp->dev; + int ret; sw_desc.drvdata = qmp; sw_desc.fwnode = dev->fwnode; @@ -3374,10 +3474,23 @@ static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) return PTR_ERR(qmp->sw); } - return devm_add_action_or_reset(dev, qmp_combo_typec_unregister, qmp); + ret = devm_add_action_or_reset(dev, qmp_combo_typec_switch_unregister, qmp); + if (ret) + return ret; + + mux_desc.drvdata = qmp; + mux_desc.fwnode = dev->fwnode; + mux_desc.set = qmp_combo_typec_mux_set; + qmp->mux = typec_mux_register(dev, &mux_desc); + if (IS_ERR(qmp->mux)) { + dev_err(dev, "Unable to register typec mux: %pe\n", qmp->mux); + return PTR_ERR(qmp->mux); + } + + return devm_add_action_or_reset(dev, qmp_combo_typec_mux_unregister, qmp); } #else -static int qmp_combo_typec_switch_register(struct qmp_combo *qmp) +static int qmp_combo_typec_register(struct qmp_combo *qmp) { return 0; } @@ -3609,7 +3722,7 @@ static int qmp_combo_probe(struct platform_device *pdev) if (ret) goto err_node_put; - ret = qmp_combo_typec_switch_register(qmp); + ret = qmp_combo_typec_register(qmp); if (ret) goto err_node_put;
Register a typec mux in order to change the PHY mode on the Type-C mux events depending on the mode and the svid when in Altmode setup. The DisplayPort phy should be left enabled if is still powered on by the DRM DisplayPort controller, so bail out until the DisplayPort PHY is not powered off. The Type-C Mode/SVID only changes on plug/unplug, and USB SAFE states will be set in between of USB-Only, Combo and DisplayPort Only so this will leave enough time to the DRM DisplayPort controller to turn of the DisplayPort PHY. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 123 ++++++++++++++++++++++++++++-- 1 file changed, 118 insertions(+), 5 deletions(-)