[RFT,4/7] phy: qcom: qmp-combo: register a typec mux to change the QPHY_MODE

  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 | 122 ++++++++++++++++++++++++++++--
 1 file changed, 117 insertions(+), 5 deletions(-)
  

On Thu, 29 Feb 2024 at 15:08, Neil Armstrong <neil.armstrong@linaro.org> 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.

I think this is not fully correct. Please correct me if I'm wrong, but
it is possible to switch between USB / USB+DP / DP-only at runtime.
See the Status Update and Configure commands.

>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 122 ++++++++++++++++++++++++++++--
>  1 file changed, 117 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index ac5d528fd7a1..b5fb6cbcf867 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>
> @@ -1515,6 +1516,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);
> @@ -3295,17 +3300,111 @@ 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) {

We should check svid against USB_TYPEC_DP_SID. Otherwise the driver
will mishandle the USB_SID_PD states.

> +               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;
> @@ -3316,10 +3415,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;
>  }
> @@ -3532,7 +3644,7 @@ static int qmp_combo_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> -       ret = qmp_combo_typec_switch_register(qmp);
> +       ret = qmp_combo_typec_register(qmp);
>         if (ret)
>                 return ret;
>
>
> --
> 2.34.1
>
>
  

On 29/02/2024 16:25, Dmitry Baryshkov wrote:
> On Thu, 29 Feb 2024 at 15:08, Neil Armstrong <neil.armstrong@linaro.org> 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.
> 
> I think this is not fully correct. Please correct me if I'm wrong, but
> it is possible to switch between USB / USB+DP / DP-only at runtime.
> See the Status Update and Configure commands.

Yes, but the current implementation is still valid because we need to
have the DP powered-off before changing the PHY mode.

I never encountered such setup and I have no idea how to test this.

> 
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 122 ++++++++++++++++++++++++++++--
>>   1 file changed, 117 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> index ac5d528fd7a1..b5fb6cbcf867 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>
>> @@ -1515,6 +1516,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);
>> @@ -3295,17 +3300,111 @@ 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) {
> 
> We should check svid against USB_TYPEC_DP_SID. Otherwise the driver
> will mishandle the USB_SID_PD states.

Ack will do

> 
>> +               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;
>> @@ -3316,10 +3415,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;
>>   }
>> @@ -3532,7 +3644,7 @@ static int qmp_combo_probe(struct platform_device *pdev)
>>          if (ret)
>>                  return ret;
>>
>> -       ret = qmp_combo_typec_switch_register(qmp);
>> +       ret = qmp_combo_typec_register(qmp);
>>          if (ret)
>>                  return ret;
>>
>>
>> --
>> 2.34.1
>>
>>
> 
>
  

On Thu, 29 Feb 2024 at 17:47, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 29/02/2024 16:25, Dmitry Baryshkov wrote:
> > On Thu, 29 Feb 2024 at 15:08, Neil Armstrong <neil.armstrong@linaro.org> 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.
> >
> > I think this is not fully correct. Please correct me if I'm wrong, but
> > it is possible to switch between USB / USB+DP / DP-only at runtime.
> > See the Status Update and Configure commands.
>
> Yes, but the current implementation is still valid because we need to
> have the DP powered-off before changing the PHY mode.

Even for switching between 2 lane and 4 lane modes?

I'll check how my USB-A+DP dongles work with respect to the altmode
configuration.

>
> I never encountered such setup and I have no idea how to test this.
>
> >
> >>
> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
  

On 29/02/2024 16:54, Dmitry Baryshkov wrote:
> On Thu, 29 Feb 2024 at 17:47, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> On 29/02/2024 16:25, Dmitry Baryshkov wrote:
>>> On Thu, 29 Feb 2024 at 15:08, Neil Armstrong <neil.armstrong@linaro.org> 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.
>>>
>>> I think this is not fully correct. Please correct me if I'm wrong, but
>>> it is possible to switch between USB / USB+DP / DP-only at runtime.
>>> See the Status Update and Configure commands.
>>
>> Yes, but the current implementation is still valid because we need to
>> have the DP powered-off before changing the PHY mode.
> 
> Even for switching between 2 lane and 4 lane modes?

So the Altmode pin assignment says how much lanes you can get (2 or 4),
and AUX data will say how much the adapter/display supports.

My native DP monitor uses the 4 lanes, while my USB-C->HDMI dongles
declares the 4lanes pin assigment but DP controller only ever tries 2 lanes.

I don't have USB-C->HDMI with 4lanes DP, but it should work fine.

Neil

> 
> I'll check how my USB-A+DP dongles work with respect to the altmode
> configuration.
> 
>>
>> I never encountered such setup and I have no idea how to test this.
>>
>>>
>>>>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
>