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@xxxxxxxxxx> > --- > 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 > -- With best wishes Dmitry