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.
The patchset also includes bindings changes and DT changes.
This has been successfully tested on an SM8550 board, but the
Thinkpad X13s deserved testing between non-PD USB, non-PD DisplayPort,
PD USB Hubs and PD Altmode Dongles to make sure the switch works
as expected.
The DisplayPort 4 lanes setup can be check with:
$ cat /sys/kernel/debug/dri/ae01000.display-controller/DP-1/dp_debug
name = msm_dp
drm_dp_link
rate = 540000
num_lanes = 4
..
This patchset depends on [1] to allow broadcasting the type-c mode
to the PHY, otherwise the PHY will keep the combo state while the
retimer would setup the 4 lanes in DP mode.
[1] https://lore.kernel.org/all/20240527-topic-sm8x50-upstream-retimer-broadcast-mode-v1-0-79ec91381aba@linaro.org/
To: Bjorn Andersson <[email protected]>
To: Konrad Dybcio <[email protected]>
To: Vinod Koul <[email protected]>
To: Kishon Vijay Abraham I <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Conor Dooley <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Neil Armstrong <[email protected]>
Changes in v2:
- Reference usb-switch.yaml in bindings patch
- Fix switch/case indenting
- Check svid for USB_TYPEC_DP_SID
- Fix X13s patch subject
- Update SM8650 patch to enable 4 lanes on HDK aswell
- Link to v1: https://lore.kernel.org/r/20240229-topic-sm8x50-upstream-phy-combo-typec-mux-v1-0-07e24a231840@linaro.org
---
Neil Armstrong (7):
dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Reference usb-switch.yaml to allow mode-switch
phy: qcom: qmp-combo: store DP phy power state
phy: qcom: qmp-combo: introduce QPHY_MODE
phy: qcom: qmp-combo: register a typec mux to change the QPHY_MODE
arm64: dts: qcom-sm8550: allow 4 lanes for DisplayPort and enable QMP PHY mode-switch
arm64: dts: qcom-sm8650: allow 4 lanes for DisplayPort and enable QMP PHY mode-switch
arm64: dts: qcom: sc8280xp-lenovo-thinkpad-x13: allow 4 lanes for DisplayPort and enable QMP PHY mode-switch
.../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 7 +-
.../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +-
arch/arm64/boot/dts/qcom/sm8550-hdk.dts | 3 +-
arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 3 +-
arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 3 +-
arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 3 +-
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 169 +++++++++++++++++++--
7 files changed, 174 insertions(+), 20 deletions(-)
---
base-commit: d4eef8b2e18d3e4d2343fb3bb975f8ac4522129a
change-id: 20240229-topic-sm8x50-upstream-phy-combo-typec-mux-31b5252513c9
Best regards,
--
Neil Armstrong <[email protected]>
The QMP USB3/DP Combo PHY can work in 3 modes:
- DisplayPort Only
- USB3 Only
- USB3 + DisplayPort Combo mode
In order to switch between those modes, the PHY needs to receive
Type-C events, allow marking to the phy with the mode-switch
property in order to allow the PHY to Type-C events.
Reference usb-switch.yaml as a simpler way to allow the mode-switch
property instead of duplicating the property definition.
Signed-off-by: Neil Armstrong <[email protected]>
---
.../devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
index 2d0d7e9e6431..5358d3a6fde3 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml
@@ -71,10 +71,8 @@ properties:
description:
See include/dt-bindings/phy/phy-qcom-qmp.h
- orientation-switch:
- description:
- Flag the PHY as possible handler of USB Type-C orientation switching
- type: boolean
+ mode-switch: true
+ orientation-switch: true
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -104,6 +102,7 @@ required:
- "#phy-cells"
allOf:
+ - $ref: /schemas/usb/usb-switch.yaml#
- if:
properties:
compatible:
--
2.34.1
Introduce an enum for the QMP Combo PHY modes, use it in the
QMP commmon phy init function and default to COMBO mode.
Signed-off-by: Neil Armstrong <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 +++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 5 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 183cd9cd1884..788e4c05eaf2 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -61,6 +61,12 @@
#define PHY_INIT_COMPLETE_TIMEOUT 10000
+enum qphy_mode {
+ QPHY_MODE_COMBO = 0,
+ QPHY_MODE_DP_ONLY,
+ QPHY_MODE_USB_ONLY,
+};
+
/* set of registers with offsets different per-PHY */
enum qphy_reg_layout {
/* PCS registers */
@@ -1503,6 +1509,7 @@ struct qmp_combo {
struct mutex phy_mutex;
int init_count;
+ enum qphy_mode init_mode;
struct phy *usb_phy;
enum phy_mode mode;
@@ -2589,12 +2596,33 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
if (qmp->orientation == TYPEC_ORIENTATION_REVERSE)
val |= SW_PORTSELECT_VAL;
writel(val, com + QPHY_V3_DP_COM_TYPEC_CTRL);
- writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
- /* bring both QMP USB and QMP DP PHYs PCS block out of reset */
- qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
- SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
- SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
+ switch (qmp->init_mode) {
+ case QPHY_MODE_COMBO:
+ writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
+
+ /* bring both QMP USB and QMP DP PHYs PCS block out of reset */
+ qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
+ SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
+ SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
+ break;
+
+ case QPHY_MODE_DP_ONLY:
+ writel(DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
+
+ /* bring QMP DP PHY PCS block out of reset */
+ qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
+ SW_DPPHY_RESET_MUX | SW_DPPHY_RESET);
+ break;
+
+ case QPHY_MODE_USB_ONLY:
+ writel(USB3_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
+
+ /* bring QMP USB PHY PCS block out of reset */
+ qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
+ SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
+ break;
+ }
qphy_clrbits(com, QPHY_V3_DP_COM_SWI_CTRL, 0x03);
qphy_clrbits(com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
@@ -3603,6 +3631,9 @@ static int qmp_combo_probe(struct platform_device *pdev)
if (ret)
goto err_node_put;
+ /* Set PHY_MODE as combo by default */
+ qmp->init_mode = QPHY_MODE_COMBO;
+
qmp->usb_phy = devm_phy_create(dev, usb_np, &qmp_combo_usb_phy_ops);
if (IS_ERR(qmp->usb_phy)) {
ret = PTR_ERR(qmp->usb_phy);
--
2.34.1
Switching the PHY Mode requires the DisplayPort PHY to be powered off,
keep track of the DisplayPort phy power state.
Signed-off-by: Neil Armstrong <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 7f999e8a433d..183cd9cd1884 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -1512,6 +1512,7 @@ struct qmp_combo {
unsigned int dp_aux_cfg;
struct phy_configure_opts_dp dp_opts;
unsigned int dp_init_count;
+ bool dp_powered_on;
struct clk_fixed_rate pipe_clk_fixed;
struct clk_hw dp_link_hw;
@@ -2685,6 +2686,8 @@ static int qmp_combo_dp_power_on(struct phy *phy)
/* Configure link rate, swing, etc. */
cfg->configure_dp_phy(qmp);
+ qmp->dp_powered_on = true;
+
mutex_unlock(&qmp->phy_mutex);
return 0;
@@ -2699,6 +2702,8 @@ static int qmp_combo_dp_power_off(struct phy *phy)
/* Assert DP PHY power down */
writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
+ qmp->dp_powered_on = false;
+
mutex_unlock(&qmp->phy_mutex);
return 0;
--
2.34.1
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 <[email protected]>
---
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--;
+ } 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;
--
2.34.1
Allow up to 4 lanes for the DisplayPort link from the PHY to the Controller
and allow mode-switch events to the QMP Combo PHY.
Signed-off-by: Neil Armstrong <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8550-hdk.dts | 3 ++-
arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8550-hdk.dts b/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
index ccff744dcd14..a95949c01f25 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
@@ -941,7 +941,7 @@ &mdss_dp0 {
&mdss_dp0_out {
remote-endpoint = <&usb_dp_qmpphy_dp_in>;
- data-lanes = <0 1>;
+ data-lanes = <0 1 2 3>;
};
&pcie0 {
@@ -1280,6 +1280,7 @@ &usb_dp_qmpphy {
vdda-phy-supply = <&vreg_l3e_1p2>;
vdda-pll-supply = <&vreg_l3f_0p88>;
+ mode-switch;
orientation-switch;
status = "okay";
diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
index 39ba3e9969b7..fbac5270b4d7 100644
--- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
@@ -795,7 +795,7 @@ &mdss_dp0 {
};
&mdss_dp0_out {
- data-lanes = <0 1>;
+ data-lanes = <0 1 2 3>;
remote-endpoint = <&usb_dp_qmpphy_dp_in>;
};
@@ -1142,6 +1142,7 @@ &usb_dp_qmpphy {
vdda-phy-supply = <&vreg_l3e_1p2>;
vdda-pll-supply = <&vreg_l3f_0p88>;
+ mode-switch;
orientation-switch;
status = "okay";
--
2.34.1
Allow up to 4 lanes for the DisplayPort link from the PHY to the Controller
and allow mode-switch events to the QMP Combo PHY.
Signed-off-by: Neil Armstrong <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 3 ++-
arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
index 3791c36579be..9ab07e265321 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
@@ -872,7 +872,7 @@ &mdss_dp0 {
};
&mdss_dp0_out {
- data-lanes = <0 1>;
+ data-lanes = <0 1 2 3>;
remote-endpoint = <&usb_dp_qmpphy_dp_in>;
};
@@ -1229,6 +1229,7 @@ &usb_dp_qmpphy {
vdda-phy-supply = <&vreg_l3i_1p2>;
vdda-pll-supply = <&vreg_l3g_0p91>;
+ mode-switch;
orientation-switch;
status = "okay";
diff --git a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
index bd87aa3aa548..884e846842f5 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
@@ -831,7 +831,7 @@ &mdss_dp0 {
};
&mdss_dp0_out {
- data-lanes = <0 1>;
+ data-lanes = <0 1 2 3>;
remote-endpoint = <&usb_dp_qmpphy_dp_in>;
};
@@ -1224,6 +1224,7 @@ &usb_dp_qmpphy {
vdda-phy-supply = <&vreg_l3i_1p2>;
vdda-pll-supply = <&vreg_l3g_0p91>;
+ mode-switch;
orientation-switch;
status = "okay";
--
2.34.1
Allow up to 4 lanes for the DisplayPort link from the PHYs to the Controllers
and allow mode-switch events to the QMP Combo PHYs.
Signed-off-by: Neil Armstrong <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index e937732abede..bcd38831f9d3 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -593,7 +593,7 @@ &mdss0_dp0 {
};
&mdss0_dp0_out {
- data-lanes = <0 1>;
+ data-lanes = <0 1 2 3>;
remote-endpoint = <&usb_0_qmpphy_dp_in>;
};
@@ -602,7 +602,7 @@ &mdss0_dp1 {
};
&mdss0_dp1_out {
- data-lanes = <0 1>;
+ data-lanes = <0 1 2 3>;
remote-endpoint = <&usb_1_qmpphy_dp_in>;
};
@@ -1143,6 +1143,7 @@ &usb_0_qmpphy {
vdda-phy-supply = <&vreg_l9d>;
vdda-pll-supply = <&vreg_l4d>;
+ mode-switch;
orientation-switch;
status = "okay";
@@ -1180,6 +1181,7 @@ &usb_1_qmpphy {
vdda-phy-supply = <&vreg_l4b>;
vdda-pll-supply = <&vreg_l3b>;
+ mode-switch;
orientation-switch;
status = "okay";
--
2.34.1
On Mon, May 27, 2024 at 10:42:35AM +0200, Neil Armstrong wrote:
> Introduce an enum for the QMP Combo PHY modes, use it in the
> QMP commmon phy init function and default to COMBO mode.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 +++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 183cd9cd1884..788e4c05eaf2 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -61,6 +61,12 @@
>
> #define PHY_INIT_COMPLETE_TIMEOUT 10000
>
> +enum qphy_mode {
> + QPHY_MODE_COMBO = 0,
> + QPHY_MODE_DP_ONLY,
> + QPHY_MODE_USB_ONLY,
> +};
> +
> /* set of registers with offsets different per-PHY */
> enum qphy_reg_layout {
> /* PCS registers */
> @@ -1503,6 +1509,7 @@ struct qmp_combo {
>
> struct mutex phy_mutex;
> int init_count;
> + enum qphy_mode init_mode;
>
> struct phy *usb_phy;
> enum phy_mode mode;
> @@ -2589,12 +2596,33 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
> if (qmp->orientation == TYPEC_ORIENTATION_REVERSE)
> val |= SW_PORTSELECT_VAL;
> writel(val, com + QPHY_V3_DP_COM_TYPEC_CTRL);
> - writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
>
> - /* bring both QMP USB and QMP DP PHYs PCS block out of reset */
> - qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
> - SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
> - SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> + switch (qmp->init_mode) {
> + case QPHY_MODE_COMBO:
> + writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
> +
> + /* bring both QMP USB and QMP DP PHYs PCS block out of reset */
> + qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
> + SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
> + SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> + break;
> +
> + case QPHY_MODE_DP_ONLY:
> + writel(DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
> +
> + /* bring QMP DP PHY PCS block out of reset */
> + qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
> + SW_DPPHY_RESET_MUX | SW_DPPHY_RESET);
> + break;
> +
> + case QPHY_MODE_USB_ONLY:
> + writel(USB3_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
> +
> + /* bring QMP USB PHY PCS block out of reset */
> + qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
> + SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
> + break;
> + }
>
> qphy_clrbits(com, QPHY_V3_DP_COM_SWI_CTRL, 0x03);
> qphy_clrbits(com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
> @@ -3603,6 +3631,9 @@ static int qmp_combo_probe(struct platform_device *pdev)
> if (ret)
> goto err_node_put;
>
> + /* Set PHY_MODE as combo by default */
> + qmp->init_mode = QPHY_MODE_COMBO;
> +
I see that COMBO mode is backwards compatible with existing code. But
shouldn't the USB-only be a default mode?
> qmp->usb_phy = devm_phy_create(dev, usb_np, &qmp_combo_usb_phy_ops);
> if (IS_ERR(qmp->usb_phy)) {
> ret = PTR_ERR(qmp->usb_phy);
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
On 27/05/2024 10:46, Dmitry Baryshkov wrote:
> On Mon, May 27, 2024 at 10:42:35AM +0200, Neil Armstrong wrote:
>> Introduce an enum for the QMP Combo PHY modes, use it in the
>> QMP commmon phy init function and default to COMBO mode.
>>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 +++++++++++++++++++++++++++----
>> 1 file changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> index 183cd9cd1884..788e4c05eaf2 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> @@ -61,6 +61,12 @@
>>
>> #define PHY_INIT_COMPLETE_TIMEOUT 10000
>>
>> +enum qphy_mode {
>> + QPHY_MODE_COMBO = 0,
>> + QPHY_MODE_DP_ONLY,
>> + QPHY_MODE_USB_ONLY,
>> +};
>> +
>> /* set of registers with offsets different per-PHY */
>> enum qphy_reg_layout {
>> /* PCS registers */
>> @@ -1503,6 +1509,7 @@ struct qmp_combo {
>>
>> struct mutex phy_mutex;
>> int init_count;
>> + enum qphy_mode init_mode;
>>
>> struct phy *usb_phy;
>> enum phy_mode mode;
>> @@ -2589,12 +2596,33 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>> if (qmp->orientation == TYPEC_ORIENTATION_REVERSE)
>> val |= SW_PORTSELECT_VAL;
>> writel(val, com + QPHY_V3_DP_COM_TYPEC_CTRL);
>> - writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
>>
>> - /* bring both QMP USB and QMP DP PHYs PCS block out of reset */
>> - qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
>> - SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
>> - SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>> + switch (qmp->init_mode) {
>> + case QPHY_MODE_COMBO:
>> + writel(USB3_MODE | DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
>> +
>> + /* bring both QMP USB and QMP DP PHYs PCS block out of reset */
>> + qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
>> + SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
>> + SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>> + break;
>> +
>> + case QPHY_MODE_DP_ONLY:
>> + writel(DP_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
>> +
>> + /* bring QMP DP PHY PCS block out of reset */
>> + qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
>> + SW_DPPHY_RESET_MUX | SW_DPPHY_RESET);
>> + break;
>> +
>> + case QPHY_MODE_USB_ONLY:
>> + writel(USB3_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
>> +
>> + /* bring QMP USB PHY PCS block out of reset */
>> + qphy_clrbits(com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
>> + SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>> + break;
>> + }
>>
>> qphy_clrbits(com, QPHY_V3_DP_COM_SWI_CTRL, 0x03);
>> qphy_clrbits(com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
>> @@ -3603,6 +3631,9 @@ static int qmp_combo_probe(struct platform_device *pdev)
>> if (ret)
>> goto err_node_put;
>>
>> + /* Set PHY_MODE as combo by default */
>> + qmp->init_mode = QPHY_MODE_COMBO;
>> +
>
> I see that COMBO mode is backwards compatible with existing code. But
> shouldn't the USB-only be a default mode?
No because it would break existing platforms without "mode-switch" in DT.
Neil
>
>> qmp->usb_phy = devm_phy_create(dev, usb_np, &qmp_combo_usb_phy_ops);
>> if (IS_ERR(qmp->usb_phy)) {
>> ret = PTR_ERR(qmp->usb_phy);
>>
>> --
>> 2.34.1
>>
>
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 <[email protected]>
> ---
> 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
On Mon, May 27, 2024 at 10:42:34AM +0200, Neil Armstrong wrote:
> Switching the PHY Mode requires the DisplayPort PHY to be powered off,
> keep track of the DisplayPort phy power state.
How is this different from dp_init_count?
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 7f999e8a433d..183cd9cd1884 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -1512,6 +1512,7 @@ struct qmp_combo {
> unsigned int dp_aux_cfg;
> struct phy_configure_opts_dp dp_opts;
> unsigned int dp_init_count;
> + bool dp_powered_on;
>
> struct clk_fixed_rate pipe_clk_fixed;
> struct clk_hw dp_link_hw;
> @@ -2685,6 +2686,8 @@ static int qmp_combo_dp_power_on(struct phy *phy)
> /* Configure link rate, swing, etc. */
> cfg->configure_dp_phy(qmp);
>
> + qmp->dp_powered_on = true;
> +
> mutex_unlock(&qmp->phy_mutex);
>
> return 0;
> @@ -2699,6 +2702,8 @@ static int qmp_combo_dp_power_off(struct phy *phy)
> /* Assert DP PHY power down */
> writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
>
> + qmp->dp_powered_on = false;
> +
> mutex_unlock(&qmp->phy_mutex);
>
> return 0;
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Mon, May 27, 2024 at 10:48:12AM +0200, Neil Armstrong wrote:
> On 27/05/2024 10:46, Dmitry Baryshkov wrote:
> > On Mon, May 27, 2024 at 10:42:35AM +0200, Neil Armstrong wrote:
> > > Introduce an enum for the QMP Combo PHY modes, use it in the
> > > QMP commmon phy init function and default to COMBO mode.
> > >
> > > Signed-off-by: Neil Armstrong <[email protected]>
> > > ---
> > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 +++++++++++++++++++++++++++----
> > > 1 file changed, 36 insertions(+), 5 deletions(-)
> > >
[trimmed]
> > > @@ -3603,6 +3631,9 @@ static int qmp_combo_probe(struct platform_device *pdev)
> > > if (ret)
> > > goto err_node_put;
> > > + /* Set PHY_MODE as combo by default */
> > > + qmp->init_mode = QPHY_MODE_COMBO;
> > > +
> >
> > I see that COMBO mode is backwards compatible with existing code. But
> > shouldn't the USB-only be a default mode?
>
> No because it would break existing platforms without "mode-switch" in DT.
Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> Neil
>
> >
> > > qmp->usb_phy = devm_phy_create(dev, usb_np, &qmp_combo_usb_phy_ops);
> > > if (IS_ERR(qmp->usb_phy)) {
> > > ret = PTR_ERR(qmp->usb_phy);
> > >
> > > --
> > > 2.34.1
> > >
> >
>
--
With best wishes
Dmitry
On Mon, May 27, 2024 at 10:42:37AM +0200, Neil Armstrong wrote:
> Allow up to 4 lanes for the DisplayPort link from the PHY to the Controller
> and allow mode-switch events to the QMP Combo PHY.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8550-hdk.dts | 3 ++-
> arch/arm64/boot/dts/qcom/sm8550-qrd.dts | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8550-hdk.dts b/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
> index ccff744dcd14..a95949c01f25 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8550-hdk.dts
> @@ -941,7 +941,7 @@ &mdss_dp0 {
>
> &mdss_dp0_out {
> remote-endpoint = <&usb_dp_qmpphy_dp_in>;
> - data-lanes = <0 1>;
> + data-lanes = <0 1 2 3>;
> };
>
> &pcie0 {
> @@ -1280,6 +1280,7 @@ &usb_dp_qmpphy {
> vdda-phy-supply = <&vreg_l3e_1p2>;
> vdda-pll-supply = <&vreg_l3f_0p88>;
>
> + mode-switch;
> orientation-switch;
Please rebase on top of https://lore.kernel.org/linux-arm-msm/[email protected]/
After that:
Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> status = "okay";
> diff --git a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> index 39ba3e9969b7..fbac5270b4d7 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> @@ -795,7 +795,7 @@ &mdss_dp0 {
> };
>
> &mdss_dp0_out {
> - data-lanes = <0 1>;
> + data-lanes = <0 1 2 3>;
> remote-endpoint = <&usb_dp_qmpphy_dp_in>;
> };
>
> @@ -1142,6 +1142,7 @@ &usb_dp_qmpphy {
> vdda-phy-supply = <&vreg_l3e_1p2>;
> vdda-pll-supply = <&vreg_l3f_0p88>;
>
> + mode-switch;
> orientation-switch;
>
> status = "okay";
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Mon, May 27, 2024 at 10:42:38AM +0200, Neil Armstrong wrote:
> Allow up to 4 lanes for the DisplayPort link from the PHY to the Controller
> and allow mode-switch events to the QMP Combo PHY.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 3 ++-
> arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
After rebase,
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Mon, May 27, 2024 at 10:42:39AM +0200, Neil Armstrong wrote:
> Allow up to 4 lanes for the DisplayPort link from the PHYs to the Controllers
> and allow mode-switch events to the QMP Combo PHYs.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 27/05/2024 10:42, Neil Armstrong wrote:
> The QMP USB3/DP Combo PHY can work in 3 modes:
> - DisplayPort Only
> - USB3 Only
> - USB3 + DisplayPort Combo mode
>
> In order to switch between those modes, the PHY needs to receive
> Type-C events, allow marking to the phy with the mode-switch
> property in order to allow the PHY to Type-C events.
>
> Reference usb-switch.yaml as a simpler way to allow the mode-switch
> property instead of duplicating the property definition.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Mon, May 27, 2024 at 10:42:32AM GMT, 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.
>
> The patchset also includes bindings changes and DT changes.
>
> This has been successfully tested on an SM8550 board, but the
> Thinkpad X13s deserved testing between non-PD USB, non-PD DisplayPort,
> PD USB Hubs and PD Altmode Dongles to make sure the switch works
> as expected.
>
> The DisplayPort 4 lanes setup can be check with:
> $ cat /sys/kernel/debug/dri/ae01000.display-controller/DP-1/dp_debug
> name = msm_dp
> drm_dp_link
> rate = 540000
> num_lanes = 4
Has the issue with the USB controller dying on us been resolved?
Regards,
Bjorn
On 28/05/2024 15:48, Bjorn Andersson wrote:
> On Mon, May 27, 2024 at 10:42:32AM GMT, 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.
>>
>> The patchset also includes bindings changes and DT changes.
>>
>> This has been successfully tested on an SM8550 board, but the
>> Thinkpad X13s deserved testing between non-PD USB, non-PD DisplayPort,
>> PD USB Hubs and PD Altmode Dongles to make sure the switch works
>> as expected.
>>
>> The DisplayPort 4 lanes setup can be check with:
>> $ cat /sys/kernel/debug/dri/ae01000.display-controller/DP-1/dp_debug
>> name = msm_dp
>> drm_dp_link
>> rate = 540000
>> num_lanes = 4
>
> Has the issue with the USB controller dying on us been resolved?
No, this would require some changes in dwc3 & ucsi to support the USB_ROLE_NONE
I haven't looked into this yet.
Neil
>
> Regards,
> Bjorn
On 27/05/2024 10:59, Dmitry Baryshkov wrote:
> On Mon, May 27, 2024 at 10:42:34AM +0200, Neil Armstrong wrote:
>> Switching the PHY Mode requires the DisplayPort PHY to be powered off,
>> keep track of the DisplayPort phy power state.
>
> How is this different from dp_init_count?
dp_init_count tracks the DP PHY init, while dp_powered_on tracks
the DP PHY beeing powered on by the DRM DP driver, those are
not the same state at all.
While testing, I figured that de-initializing the DP PHY while
is was powered-on by the DRM DP, caused the system to freeze and crash.
SO I've added this to track this state and try to de-init the DP phy
if still in use.
Neil
>
>>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> index 7f999e8a433d..183cd9cd1884 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> @@ -1512,6 +1512,7 @@ struct qmp_combo {
>> unsigned int dp_aux_cfg;
>> struct phy_configure_opts_dp dp_opts;
>> unsigned int dp_init_count;
>> + bool dp_powered_on;
>>
>> struct clk_fixed_rate pipe_clk_fixed;
>> struct clk_hw dp_link_hw;
>> @@ -2685,6 +2686,8 @@ static int qmp_combo_dp_power_on(struct phy *phy)
>> /* Configure link rate, swing, etc. */
>> cfg->configure_dp_phy(qmp);
>>
>> + qmp->dp_powered_on = true;
>> +
>> mutex_unlock(&qmp->phy_mutex);
>>
>> return 0;
>> @@ -2699,6 +2702,8 @@ static int qmp_combo_dp_power_off(struct phy *phy)
>> /* Assert DP PHY power down */
>> writel(DP_PHY_PD_CTL_PSR_PWRDN, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
>>
>> + qmp->dp_powered_on = false;
>> +
>> mutex_unlock(&qmp->phy_mutex);
>>
>> return 0;
>>
>> --
>> 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 <[email protected]>
>> ---
>> 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
>>
>
On Thu, Jun 06, 2024 at 03:29:52PM +0200, Neil Armstrong wrote:
> On 27/05/2024 10:59, Dmitry Baryshkov wrote:
> > On Mon, May 27, 2024 at 10:42:34AM +0200, Neil Armstrong wrote:
> > > Switching the PHY Mode requires the DisplayPort PHY to be powered off,
> > > keep track of the DisplayPort phy power state.
> >
> > How is this different from dp_init_count?
>
> dp_init_count tracks the DP PHY init, while dp_powered_on tracks
> the DP PHY beeing powered on by the DRM DP driver, those are
> not the same state at all.
>
> While testing, I figured that de-initializing the DP PHY while
> is was powered-on by the DRM DP, caused the system to freeze and crash.
>
> SO I've added this to track this state and try to de-init the DP phy
> if still in use.
If you are to send next iteration, please add these bits to the commit
message.
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry