This adds support for USB and DisplayPort orientation switching to the
QMP combo PHY, as well as updating the sc8280xp devices to include the
QMP in the SuperSpeed graph.
Bjorn Andersson (7):
dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and
orientation-switch
phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
phy: qcom-qmp-combo: Introduce orientation variable
phy: qcom-qmp-combo: Introduce orientation switching
phy: qcom-qmp-combo: Introduce drm_bridge
arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
.../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 51 ++++
arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++-
.../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 28 ++-
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 227 ++++++++++++++----
5 files changed, 309 insertions(+), 59 deletions(-)
--
2.39.2
In multiple places throughout the driver code has been written in
prepration for handling of orientation switching.
Introduce a typec_orientation in qmp_combo and fill out the various
"placeholders" with the associated logic. By initializing the
orientation to "normal" this change has no functional impact, but
reduces the size of the upcoming introduction of dynamic orientation
switching.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++----------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 7280f7141961..6748f31da7a3 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/regulator/consumer.h>
#include <linux/reset.h>
#include <linux/slab.h>
+#include <linux/usb/typec.h>
#include <dt-bindings/phy/phy-qcom-qmp.h>
@@ -63,6 +64,10 @@
/* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
#define CLAMP_EN BIT(0) /* enables i/o clamp_n */
+/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */
+#define SW_PORTSELECT_VAL BIT(0)
+#define SW_PORTSELECT_MUX BIT(1)
+
#define PHY_INIT_COMPLETE_TIMEOUT 10000
struct qmp_phy_init_tbl {
@@ -1323,6 +1328,8 @@ struct qmp_combo {
struct clk_fixed_rate pipe_clk_fixed;
struct clk_hw dp_link_hw;
struct clk_hw dp_pixel_hw;
+
+ enum typec_orientation orientation;
};
static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
@@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
{
u32 val;
- bool reverse = false;
+ bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
+ const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
val = DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN |
DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN;
- /*
- * TODO: Assume orientation is CC1 for now and two lanes, need to
- * use type-c connector to understand orientation and lanes.
- *
- * Otherwise val changes to be like below if this code understood
- * the orientation of the type-c cable.
- *
- * if (lane_cnt == 4 || orientation == ORIENTATION_CC2)
- * val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN;
- * if (lane_cnt == 4 || orientation == ORIENTATION_CC1)
- * val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN;
- * if (orientation == ORIENTATION_CC2)
- * writel(0x4c, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_MODE);
- */
- val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN;
+ if (dp_opts->lanes == 4 || reverse)
+ val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN;
+ if (dp_opts->lanes == 4 || !reverse)
+ val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN;
+
writel(val, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
- writel(0x5c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE);
+ if (reverse)
+ writel(0x4c, qmp->pcs + QSERDES_DP_PHY_MODE);
+ else
+ writel(0x5c, qmp->pcs + QSERDES_DP_PHY_MODE);
return reverse;
}
@@ -2235,7 +2236,7 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp)
{
const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
- bool reverse = false;
+ bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
u32 status;
int ret;
@@ -2299,7 +2300,7 @@ static int qmp_v5_configure_dp_phy(struct qmp_combo *qmp)
{
const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
- bool reverse = false;
+ bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
u32 status;
int ret;
@@ -2358,7 +2359,7 @@ static int qmp_v6_configure_dp_phy(struct qmp_combo *qmp)
{
const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
- bool reverse = false;
+ bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
u32 status;
int ret;
@@ -2462,6 +2463,7 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
const struct qmp_phy_cfg *cfg = qmp->cfg;
void __iomem *com = qmp->com;
int ret;
+ u32 val;
if (qmp->init_count++)
return 0;
@@ -2495,10 +2497,12 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
- /* Default type-c orientation, i.e CC1 */
- qphy_setbits(com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x02);
-
- qphy_setbits(com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE | DP_MODE);
+ /* Use software based port select and switch on typec orientation */
+ val = SW_PORTSELECT_MUX;
+ 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,
@@ -3361,6 +3365,8 @@ static int qmp_combo_probe(struct platform_device *pdev)
qmp->dev = dev;
+ qmp->orientation = TYPEC_ORIENTATION_NORMAL;
+
qmp->cfg = of_device_get_match_data(dev);
if (!qmp->cfg)
return -EINVAL;
--
2.39.2
The QMP combo PHY sits in an of_graph connected between the DisplayPort
controller and a USB Type-C connector (or possibly a redriver).
The TCPM needs to be able to convey the HPD signal to the DisplayPort
controller, but no directly link is provided by DeviceTree so the signal
needs to "pass through" the QMP combo phy.
Handle this by introducing a drm_bridge which upon initialization finds
the next bridge (i.e. the usb-c-connector) and chain this together. This
way HPD changes in the connector will propagate to the DisplayPort
driver.
The connector bridge is resolved lazily, as the TCPM is expected to be
able to resolve the typec mux and switch at probe time, so the QMP combo
phy will probe before the TCPM.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 5d6d6ef3944b..84bc08002537 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -22,6 +22,8 @@
#include <linux/usb/typec.h>
#include <linux/usb/typec_mux.h>
+#include <drm/drm_bridge.h>
+
#include <dt-bindings/phy/phy-qcom-qmp.h>
#include "phy-qcom-qmp.h"
@@ -1332,6 +1334,8 @@ struct qmp_combo {
struct clk_hw dp_link_hw;
struct clk_hw dp_pixel_hw;
+ struct drm_bridge bridge;
+
struct typec_switch_dev *sw;
enum typec_orientation orientation;
};
@@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
}
+static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
+ struct drm_bridge *next_bridge;
+
+ if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
+ return -EINVAL;
+
+ next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
+ if (IS_ERR(next_bridge))
+ return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
+
+ return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+}
+
+static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
+ .attach = qmp_combo_bridge_attach,
+};
+
+static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
+{
+ qmp->bridge.funcs = &qmp_combo_bridge_funcs;
+ qmp->bridge.of_node = qmp->dev->of_node;
+
+ return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
+}
+
static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
{
struct device *dev = qmp->dev;
@@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
if (ret)
return ret;
+ ret = qmp_combo_dp_register_bridge(qmp);
+ if (ret)
+ return ret;
+
/* Check for legacy binding with child nodes. */
usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
if (usb_np) {
--
2.39.2
With support for the QMP combo phy to react to USB Type-C switch events,
introduce it as the next hop for the SuperSpeed lanes of the two USB
Type-C connectors, and connect the output of the DisplayPort controller
to the QMP combo phy.
This allows the TCPM to perform orientation switching of both USB and
DisplayPort signals.
Signed-off-by: Bjorn Andersson <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++++++++++++++++++++++
2 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 547277924ea3..33c973661fa5 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -64,7 +64,7 @@ port@1 {
reg = <1>;
pmic_glink_con0_ss: endpoint {
- remote-endpoint = <&mdss0_dp0_out>;
+ remote-endpoint = <&usb_0_qmpphy_out>;
};
};
@@ -99,7 +99,7 @@ port@1 {
reg = <1>;
pmic_glink_con1_ss: endpoint {
- remote-endpoint = <&mdss0_dp1_out>;
+ remote-endpoint = <&usb_1_qmpphy_out>;
};
};
@@ -412,7 +412,7 @@ &mdss0_dp0 {
&mdss0_dp0_out {
data-lanes = <0 1>;
- remote-endpoint = <&pmic_glink_con0_ss>;
+ remote-endpoint = <&usb_0_qmpphy_dp_in>;
};
&mdss0_dp1 {
@@ -421,7 +421,7 @@ &mdss0_dp1 {
&mdss0_dp1_out {
data-lanes = <0 1>;
- remote-endpoint = <&pmic_glink_con1_ss>;
+ remote-endpoint = <&usb_1_qmpphy_dp_in>;
};
&mdss0_dp3 {
@@ -670,9 +670,19 @@ &usb_0_qmpphy {
vdda-phy-supply = <&vreg_l9d>;
vdda-pll-supply = <&vreg_l4d>;
+ orientation-switch;
+
status = "okay";
};
+&usb_0_qmpphy_dp_in {
+ remote-endpoint = <&mdss0_dp0_out>;
+};
+
+&usb_0_qmpphy_out {
+ remote-endpoint = <&pmic_glink_con0_ss>;
+};
+
&usb_0_role_switch {
remote-endpoint = <&pmic_glink_con0_hs>;
};
@@ -697,9 +707,19 @@ &usb_1_qmpphy {
vdda-phy-supply = <&vreg_l4b>;
vdda-pll-supply = <&vreg_l3b>;
+ orientation-switch;
+
status = "okay";
};
+&usb_1_qmpphy_dp_in {
+ remote-endpoint = <&mdss0_dp1_out>;
+};
+
+&usb_1_qmpphy_out {
+ remote-endpoint = <&pmic_glink_con1_ss>;
+};
+
&usb_1_role_switch {
remote-endpoint = <&pmic_glink_con1_hs>;
};
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 0e691bb0120c..1eb3a295e8fa 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
#phy-cells = <1>;
status = "disabled";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ usb_0_qmpphy_out: endpoint {};
+ };
+
+ port@1 {
+ reg = <1>;
+
+ usb_0_qmpphy_dp_in: endpoint {};
+ };
+ };
};
usb_1_hsphy: phy@8902000 {
@@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
#phy-cells = <1>;
status = "disabled";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ usb_1_qmpphy_out: endpoint {};
+ };
+
+ port@1 {
+ reg = <1>;
+
+ usb_1_qmpphy_dp_in: endpoint {};
+ };
+ };
};
mdss1_dp0_phy: phy@8909a00 {
--
2.39.2
With the upcoming introduction of USB Type-C orientation switching the
region of mutual exclusion needs to be extended to cover both the common
init/exit as well as the individual functions.
So move the phy_mutex one step up the stack.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 51 +++++++++++++----------
1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 6850e04c329b..7280f7141961 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -2463,16 +2463,13 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
void __iomem *com = qmp->com;
int ret;
- mutex_lock(&qmp->phy_mutex);
- if (qmp->init_count++) {
- mutex_unlock(&qmp->phy_mutex);
+ if (qmp->init_count++)
return 0;
- }
ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
if (ret) {
dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
- goto err_unlock;
+ goto err;
}
ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets);
@@ -2514,16 +2511,13 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
qphy_setbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
SW_PWRDN);
- mutex_unlock(&qmp->phy_mutex);
-
return 0;
err_assert_reset:
reset_control_bulk_assert(cfg->num_resets, qmp->resets);
err_disable_regulators:
regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
-err_unlock:
- mutex_unlock(&qmp->phy_mutex);
+err:
return ret;
}
@@ -2532,11 +2526,8 @@ static int qmp_combo_com_exit(struct qmp_combo *qmp)
{
const struct qmp_phy_cfg *cfg = qmp->cfg;
- mutex_lock(&qmp->phy_mutex);
- if (--qmp->init_count) {
- mutex_unlock(&qmp->phy_mutex);
+ if (--qmp->init_count)
return 0;
- }
reset_control_bulk_assert(cfg->num_resets, qmp->resets);
@@ -2544,8 +2535,6 @@ static int qmp_combo_com_exit(struct qmp_combo *qmp)
regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
- mutex_unlock(&qmp->phy_mutex);
-
return 0;
}
@@ -2555,21 +2544,29 @@ static int qmp_combo_dp_init(struct phy *phy)
const struct qmp_phy_cfg *cfg = qmp->cfg;
int ret;
+ mutex_lock(&qmp->phy_mutex);
+
ret = qmp_combo_com_init(qmp);
if (ret)
- return ret;
+ goto out_unlock;
cfg->dp_aux_init(qmp);
- return 0;
+out_unlock:
+ mutex_unlock(&qmp->phy_mutex);
+ return ret;
}
static int qmp_combo_dp_exit(struct phy *phy)
{
struct qmp_combo *qmp = phy_get_drvdata(phy);
+ mutex_lock(&qmp->phy_mutex);
+
qmp_combo_com_exit(qmp);
+ mutex_unlock(&qmp->phy_mutex);
+
return 0;
}
@@ -2686,14 +2683,19 @@ static int qmp_combo_usb_init(struct phy *phy)
struct qmp_combo *qmp = phy_get_drvdata(phy);
int ret;
+ mutex_lock(&qmp->phy_mutex);
ret = qmp_combo_com_init(qmp);
if (ret)
- return ret;
+ goto out_unlock;
ret = qmp_combo_usb_power_on(phy);
- if (ret)
+ if (ret) {
qmp_combo_com_exit(qmp);
+ goto out_unlock;
+ }
+out_unlock:
+ mutex_unlock(&qmp->phy_mutex);
return ret;
}
@@ -2702,11 +2704,18 @@ static int qmp_combo_usb_exit(struct phy *phy)
struct qmp_combo *qmp = phy_get_drvdata(phy);
int ret;
+ mutex_lock(&qmp->phy_mutex);
ret = qmp_combo_usb_power_off(phy);
if (ret)
- return ret;
+ goto out_unlock;
- return qmp_combo_com_exit(qmp);
+ ret = qmp_combo_com_exit(qmp);
+ if (ret)
+ goto out_unlock;
+
+out_unlock:
+ mutex_unlock(&qmp->phy_mutex);
+ return ret;
}
static int qmp_combo_usb_set_mode(struct phy *phy, enum phy_mode mode, int submode)
--
2.39.2
Following the CRD, connect the two QMP phys inbetween the USB Type-C
connectors and the DisplayPort controller, to handle orientation
switching.
Signed-off-by: Bjorn Andersson <[email protected]>
---
.../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 28 ++++++++++++++++---
1 file changed, 24 insertions(+), 4 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 5ef3f4c07d75..382f27946468 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -106,7 +106,7 @@ port@1 {
reg = <1>;
pmic_glink_con0_ss: endpoint {
- remote-endpoint = <&mdss0_dp0_out>;
+ remote-endpoint = <&usb_0_qmpphy_out>;
};
};
@@ -141,7 +141,7 @@ port@1 {
reg = <1>;
pmic_glink_con1_ss: endpoint {
- remote-endpoint = <&mdss0_dp1_out>;
+ remote-endpoint = <&usb_1_qmpphy_out>;
};
};
@@ -554,7 +554,7 @@ &mdss0_dp0 {
&mdss0_dp0_out {
data-lanes = <0 1>;
- remote-endpoint = <&pmic_glink_con0_ss>;
+ remote-endpoint = <&usb_0_qmpphy_dp_in>;
};
&mdss0_dp1 {
@@ -563,7 +563,7 @@ &mdss0_dp1 {
&mdss0_dp1_out {
data-lanes = <0 1>;
- remote-endpoint = <&pmic_glink_con1_ss>;
+ remote-endpoint = <&usb_1_qmpphy_dp_in>;
};
&mdss0_dp3 {
@@ -1140,9 +1140,19 @@ &usb_0_qmpphy {
vdda-phy-supply = <&vreg_l9d>;
vdda-pll-supply = <&vreg_l4d>;
+ orientation-switch;
+
status = "okay";
};
+&usb_0_qmpphy_dp_in {
+ remote-endpoint = <&mdss0_dp0_out>;
+};
+
+&usb_0_qmpphy_out {
+ remote-endpoint = <&pmic_glink_con0_ss>;
+};
+
&usb_0_role_switch {
remote-endpoint = <&pmic_glink_con0_hs>;
};
@@ -1167,9 +1177,19 @@ &usb_1_qmpphy {
vdda-phy-supply = <&vreg_l4b>;
vdda-pll-supply = <&vreg_l3b>;
+ orientation-switch;
+
status = "okay";
};
+&usb_1_qmpphy_dp_in {
+ remote-endpoint = <&mdss0_dp1_out>;
+};
+
+&usb_1_qmpphy_out {
+ remote-endpoint = <&pmic_glink_con1_ss>;
+};
+
&usb_1_role_switch {
remote-endpoint = <&pmic_glink_con1_hs>;
};
--
2.39.2
The data lanes of the QMP PHY is swapped in order to handle changing
orientation of the USB Type-C cable. Register a typec_switch device to
allow a TCPM to configure the orientation.
The newly introduced orientation variable is adjusted based on the
request, and the initialized components are brought down and up again.
To keep track of what parts needs to be cycled new variables to keep
track of the individual init_count is introduced.
Both the USB and the DisplayPort altmode signals are properly switched.
For DisplayPort the controller will after the TCPM having established
orientation power on the PHY, so this is not done implicitly, but for
USB the PHY typically is kept initialized across the switch, and must
therefor then be reinitialized.
This is based on initial work by Wesley Cheng.
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 92 ++++++++++++++++++++---
1 file changed, 83 insertions(+), 9 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 6748f31da7a3..5d6d6ef3944b 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -20,6 +20,7 @@
#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/usb/typec.h>
+#include <linux/usb/typec_mux.h>
#include <dt-bindings/phy/phy-qcom-qmp.h>
@@ -1320,15 +1321,18 @@ struct qmp_combo {
struct phy *usb_phy;
enum phy_mode mode;
+ unsigned int usb_init_count;
struct phy *dp_phy;
unsigned int dp_aux_cfg;
struct phy_configure_opts_dp dp_opts;
+ unsigned int dp_init_count;
struct clk_fixed_rate pipe_clk_fixed;
struct clk_hw dp_link_hw;
struct clk_hw dp_pixel_hw;
+ struct typec_switch_dev *sw;
enum typec_orientation orientation;
};
@@ -2458,14 +2462,14 @@ static int qmp_combo_dp_calibrate(struct phy *phy)
return 0;
}
-static int qmp_combo_com_init(struct qmp_combo *qmp)
+static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
{
const struct qmp_phy_cfg *cfg = qmp->cfg;
void __iomem *com = qmp->com;
int ret;
u32 val;
- if (qmp->init_count++)
+ if (!force && qmp->init_count++)
return 0;
ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
@@ -2526,11 +2530,11 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
return ret;
}
-static int qmp_combo_com_exit(struct qmp_combo *qmp)
+static int qmp_combo_com_exit(struct qmp_combo *qmp, bool force)
{
const struct qmp_phy_cfg *cfg = qmp->cfg;
- if (--qmp->init_count)
+ if (!force && --qmp->init_count)
return 0;
reset_control_bulk_assert(cfg->num_resets, qmp->resets);
@@ -2550,12 +2554,14 @@ static int qmp_combo_dp_init(struct phy *phy)
mutex_lock(&qmp->phy_mutex);
- ret = qmp_combo_com_init(qmp);
+ ret = qmp_combo_com_init(qmp, false);
if (ret)
goto out_unlock;
cfg->dp_aux_init(qmp);
+ qmp->dp_init_count++;
+
out_unlock:
mutex_unlock(&qmp->phy_mutex);
return ret;
@@ -2567,8 +2573,9 @@ static int qmp_combo_dp_exit(struct phy *phy)
mutex_lock(&qmp->phy_mutex);
- qmp_combo_com_exit(qmp);
+ qmp_combo_com_exit(qmp, false);
+ qmp->dp_init_count--;
mutex_unlock(&qmp->phy_mutex);
return 0;
@@ -2688,16 +2695,18 @@ static int qmp_combo_usb_init(struct phy *phy)
int ret;
mutex_lock(&qmp->phy_mutex);
- ret = qmp_combo_com_init(qmp);
+ ret = qmp_combo_com_init(qmp, false);
if (ret)
goto out_unlock;
ret = qmp_combo_usb_power_on(phy);
if (ret) {
- qmp_combo_com_exit(qmp);
+ qmp_combo_com_exit(qmp, false);
goto out_unlock;
}
+ qmp->usb_init_count++;
+
out_unlock:
mutex_unlock(&qmp->phy_mutex);
return ret;
@@ -2713,10 +2722,12 @@ static int qmp_combo_usb_exit(struct phy *phy)
if (ret)
goto out_unlock;
- ret = qmp_combo_com_exit(qmp);
+ ret = qmp_combo_com_exit(qmp, false);
if (ret)
goto out_unlock;
+ qmp->usb_init_count--;
+
out_unlock:
mutex_unlock(&qmp->phy_mutex);
return ret;
@@ -3351,6 +3362,65 @@ static struct phy *qmp_combo_phy_xlate(struct device *dev, struct of_phandle_arg
return ERR_PTR(-EINVAL);
}
+#if IS_ENABLED(CONFIG_TYPEC)
+static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw,
+ enum typec_orientation orientation)
+{
+ struct qmp_combo *qmp = typec_switch_get_drvdata(sw);
+ const struct qmp_phy_cfg *cfg = qmp->cfg;
+
+ if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
+ return 0;
+
+ mutex_lock(&qmp->phy_mutex);
+ qmp->orientation = orientation;
+
+ if (qmp->init_count) {
+ if (qmp->usb_init_count)
+ qmp_combo_usb_power_off(qmp->usb_phy);
+ qmp_combo_com_exit(qmp, true);
+
+ qmp_combo_com_init(qmp, true);
+ if (qmp->usb_init_count)
+ qmp_combo_usb_power_on(qmp->usb_phy);
+ if (qmp->dp_init_count)
+ cfg->dp_aux_init(qmp);
+ }
+ mutex_unlock(&qmp->phy_mutex);
+
+ return 0;
+}
+
+static void qmp_combo_typec_unregister(void *data)
+{
+ struct qmp_combo *qmp = data;
+
+ typec_switch_unregister(qmp->sw);
+}
+
+static int qmp_combo_typec_switch_register(struct qmp_combo *qmp)
+{
+ struct typec_switch_desc sw_desc = {};
+ struct device *dev = qmp->dev;
+
+ sw_desc.drvdata = qmp;
+ sw_desc.fwnode = dev->fwnode;
+ sw_desc.set = qmp_combo_typec_switch_set;
+ qmp->sw = typec_switch_register(dev, &sw_desc);
+ if (IS_ERR(qmp->sw)) {
+ dev_err(dev, "Unable to register typec switch: %pe\n", qmp->sw);
+ return PTR_ERR(qmp->sw);
+ }
+
+ return devm_add_action_or_reset(dev, qmp_combo_typec_unregister, qmp);
+}
+#else
+static int qmp_combo_typec_switch_register(struct qmp_combo *qmp)
+{
+ return 0;
+}
+#endif
+
static int qmp_combo_probe(struct platform_device *pdev)
{
struct qmp_combo *qmp;
@@ -3385,6 +3455,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
if (ret)
return ret;
+ ret = qmp_combo_typec_switch_register(qmp);
+ if (ret)
+ return ret;
+
/* Check for legacy binding with child nodes. */
usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
if (usb_np) {
--
2.39.2
Hi Bjorn,
On Mon, Apr 24, 2023 at 10:40 PM Bjorn Andersson
<[email protected]> wrote:
>
> This adds support for USB and DisplayPort orientation switching to the
> QMP combo PHY, as well as updating the sc8280xp devices to include the
> QMP in the SuperSpeed graph.
>
> Bjorn Andersson (7):
> dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and
> orientation-switch
> phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
> phy: qcom-qmp-combo: Introduce orientation variable
> phy: qcom-qmp-combo: Introduce orientation switching
> phy: qcom-qmp-combo: Introduce drm_bridge
> arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
> arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
>
> .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 51 ++++
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++-
> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 28 ++-
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 227 ++++++++++++++----
> 5 files changed, 309 insertions(+), 59 deletions(-)
>
> --
> 2.39.2
>
Thank you! I have been looking forward to this patchset for a while :)
Tested with 05ac:1460 Apple, Inc. Digital AV Multiport Adapter and
0639:7210 Chrontel, Inc. Billboard and both work with orientation
switching.
Tested-by: Steev Klimaszewski <[email protected]>
On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
<[email protected]> wrote:
>
> The QMP combo PHY sits in an of_graph connected between the DisplayPort
> controller and a USB Type-C connector (or possibly a redriver).
>
> The TCPM needs to be able to convey the HPD signal to the DisplayPort
> controller, but no directly link is provided by DeviceTree so the signal
> needs to "pass through" the QMP combo phy.
>
> Handle this by introducing a drm_bridge which upon initialization finds
> the next bridge (i.e. the usb-c-connector) and chain this together. This
> way HPD changes in the connector will propagate to the DisplayPort
> driver.
>
> The connector bridge is resolved lazily, as the TCPM is expected to be
> able to resolve the typec mux and switch at probe time, so the QMP combo
> phy will probe before the TCPM.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 5d6d6ef3944b..84bc08002537 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -22,6 +22,8 @@
> #include <linux/usb/typec.h>
> #include <linux/usb/typec_mux.h>
>
> +#include <drm/drm_bridge.h>
> +
> #include <dt-bindings/phy/phy-qcom-qmp.h>
>
> #include "phy-qcom-qmp.h"
> @@ -1332,6 +1334,8 @@ struct qmp_combo {
> struct clk_hw dp_link_hw;
> struct clk_hw dp_pixel_hw;
>
> + struct drm_bridge bridge;
> +
> struct typec_switch_dev *sw;
> enum typec_orientation orientation;
> };
> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> }
>
> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> + struct drm_bridge *next_bridge;
> +
> + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> + return -EINVAL;
> +
> + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> + if (IS_ERR(next_bridge))
> + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> +
> + return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +}
> +
> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
> + .attach = qmp_combo_bridge_attach,
> +};
> +
> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
> +{
> + qmp->bridge.funcs = &qmp_combo_bridge_funcs;
> + qmp->bridge.of_node = qmp->dev->of_node;
> +
> + return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
> +}
> +
> static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
> {
> struct device *dev = qmp->dev;
> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ret = qmp_combo_dp_register_bridge(qmp);
> + if (ret)
> + return ret;
> +
> /* Check for legacy binding with child nodes. */
> usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
> if (usb_np) {
> --
> 2.39.2
>
You need to add some or all of these
select DRM_DISPLAY_DP_HELPER
select DRM_DISPLAY_HELPER
select DRM_DP_AUX_BUS
select DRM_KMS_HELPER
select DRM_MIPI_DSI
select DRM_PANEL
/opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
Unexpected GOT/PLT entries detected!
/opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
Unexpected run-time procedure linkages detected!
drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
`qmp_combo_bridge_attach':
phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
`devm_drm_of_get_bridge'
phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
`devm_drm_bridge_add'
---
bod
On 23-04-24 20:40:03, Bjorn Andersson wrote:
> This adds support for USB and DisplayPort orientation switching to the
> QMP combo PHY, as well as updating the sc8280xp devices to include the
> QMP in the SuperSpeed graph.
>
Tested this entire patchset on my X13s. Therefore:
Tested-by: Abel Vesa <[email protected]>
> Bjorn Andersson (7):
> dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and
> orientation-switch
> phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
> phy: qcom-qmp-combo: Introduce orientation variable
> phy: qcom-qmp-combo: Introduce orientation switching
> phy: qcom-qmp-combo: Introduce drm_bridge
> arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
> arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
>
> .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 51 ++++
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++-
> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 28 ++-
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 227 ++++++++++++++----
> 5 files changed, 309 insertions(+), 59 deletions(-)
>
> --
> 2.39.2
>
On 4/25/23 04:40, Bjorn Andersson wrote:
> With support for the QMP combo phy to react to USB Type-C switch events,
> introduce it as the next hop for the SuperSpeed lanes of the two USB
> Type-C connectors, and connect the output of the DisplayPort controller
> to the QMP combo phy.
>
> This allows the TCPM to perform orientation switching of both USB and
> DisplayPort signals.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 547277924ea3..33c973661fa5 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -64,7 +64,7 @@ port@1 {
> reg = <1>;
>
> pmic_glink_con0_ss: endpoint {
> - remote-endpoint = <&mdss0_dp0_out>;
> + remote-endpoint = <&usb_0_qmpphy_out>;
> };
> };
>
> @@ -99,7 +99,7 @@ port@1 {
> reg = <1>;
>
> pmic_glink_con1_ss: endpoint {
> - remote-endpoint = <&mdss0_dp1_out>;
> + remote-endpoint = <&usb_1_qmpphy_out>;
> };
> };
>
> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>
> &mdss0_dp0_out {
> data-lanes = <0 1>;
> - remote-endpoint = <&pmic_glink_con0_ss>;
> + remote-endpoint = <&usb_0_qmpphy_dp_in>;
> };
>
> &mdss0_dp1 {
> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>
> &mdss0_dp1_out {
> data-lanes = <0 1>;
> - remote-endpoint = <&pmic_glink_con1_ss>;
> + remote-endpoint = <&usb_1_qmpphy_dp_in>;
> };
>
> &mdss0_dp3 {
> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
> vdda-phy-supply = <&vreg_l9d>;
> vdda-pll-supply = <&vreg_l4d>;
>
> + orientation-switch;
I believe this belongs in the SoC DTSI, as it's supported by
the PHY block itself
The rest seems to lgtm..
On a note, why did we end up placing pmic_glink in device
DTs? It's already assumed that we're using the full Qualcomm
stack as we use PAS for remoteprocs so I *think* we can always
assume pmic_glink would be there!
Konrad
> +
> status = "okay";
> };
>
> +&usb_0_qmpphy_dp_in {
> + remote-endpoint = <&mdss0_dp0_out>;
> +};
> +
> +&usb_0_qmpphy_out {
> + remote-endpoint = <&pmic_glink_con0_ss>;
> +};
> +
> &usb_0_role_switch {
> remote-endpoint = <&pmic_glink_con0_hs>;
> };
> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
> vdda-phy-supply = <&vreg_l4b>;
> vdda-pll-supply = <&vreg_l3b>;
>
> + orientation-switch;
> +
> status = "okay";
> };
>
> +&usb_1_qmpphy_dp_in {
> + remote-endpoint = <&mdss0_dp1_out>;
> +};
> +
> +&usb_1_qmpphy_out {
> + remote-endpoint = <&pmic_glink_con1_ss>;
> +};
> +
> &usb_1_role_switch {
> remote-endpoint = <&pmic_glink_con1_hs>;
> };
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 0e691bb0120c..1eb3a295e8fa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
> #phy-cells = <1>;
>
> status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + usb_0_qmpphy_out: endpoint {};
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + usb_0_qmpphy_dp_in: endpoint {};
> + };
> + };
> };
>
> usb_1_hsphy: phy@8902000 {
> @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
> #phy-cells = <1>;
>
> status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + usb_1_qmpphy_out: endpoint {};
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + usb_1_qmpphy_dp_in: endpoint {};
> + };
> + };
> };
>
> mdss1_dp0_phy: phy@8909a00 {
On 26/04/2023 12:33, Bryan O'Donoghue wrote:
> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
> <[email protected]> wrote:
>>
>> The QMP combo PHY sits in an of_graph connected between the DisplayPort
>> controller and a USB Type-C connector (or possibly a redriver).
>>
>> The TCPM needs to be able to convey the HPD signal to the DisplayPort
>> controller, but no directly link is provided by DeviceTree so the signal
>> needs to "pass through" the QMP combo phy.
>>
>> Handle this by introducing a drm_bridge which upon initialization finds
>> the next bridge (i.e. the usb-c-connector) and chain this together. This
>> way HPD changes in the connector will propagate to the DisplayPort
>> driver.
>>
>> The connector bridge is resolved lazily, as the TCPM is expected to be
>> able to resolve the typec mux and switch at probe time, so the QMP combo
>> phy will probe before the TCPM.
>>
>> Signed-off-by: Bjorn Andersson <[email protected]>
>> ---
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> index 5d6d6ef3944b..84bc08002537 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> @@ -22,6 +22,8 @@
>> #include <linux/usb/typec.h>
>> #include <linux/usb/typec_mux.h>
>>
>> +#include <drm/drm_bridge.h>
>> +
>> #include <dt-bindings/phy/phy-qcom-qmp.h>
>>
>> #include "phy-qcom-qmp.h"
>> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>> struct clk_hw dp_link_hw;
>> struct clk_hw dp_pixel_hw;
>>
>> + struct drm_bridge bridge;
>> +
>> struct typec_switch_dev *sw;
>> enum typec_orientation orientation;
>> };
>> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
>> return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
>> }
>>
>> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
>> + enum drm_bridge_attach_flags flags)
>> +{
>> + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
>> + struct drm_bridge *next_bridge;
>> +
>> + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>> + return -EINVAL;
>> +
>> + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
>> + if (IS_ERR(next_bridge))
>> + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
>> +
>> + return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> +}
>> +
>> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
>> + .attach = qmp_combo_bridge_attach,
>> +};
>> +
>> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
>> +{
>> + qmp->bridge.funcs = &qmp_combo_bridge_funcs;
>> + qmp->bridge.of_node = qmp->dev->of_node;
>> +
>> + return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
>> +}
>> +
>> static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
>> {
>> struct device *dev = qmp->dev;
>> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
>> if (ret)
>> return ret;
>>
>> + ret = qmp_combo_dp_register_bridge(qmp);
>> + if (ret)
>> + return ret;
I think the DRM part should be only built if CONFIG_DRM is enabled, I don't
have a strong opinion on this, I think Vinod could help here.
>> +
>> /* Check for legacy binding with child nodes. */
>> usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>> if (usb_np) {
>> --
>> 2.39.2
>>
>
> You need to add some or all of these
> select DRM_DISPLAY_DP_HELPER
> select DRM_DISPLAY_HELPER
> select DRM_DP_AUX_BUS
> select DRM_KMS_HELPER
> select DRM_MIPI_DSI
> select DRM_PANEL
>
>
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected GOT/PLT entries detected!
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected run-time procedure linkages detected!
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
> `qmp_combo_bridge_attach':
> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
> `devm_drm_of_get_bridge'
> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
> `devm_drm_bridge_add'
I think CONFIG_DRM_PANEL_BRIDGE in addition to CONFIG_DRM. should be enough.
With this config added and my drm-bridge hat:
Acked-by: Neil Armstrong <[email protected]>
Neil
>
> ---
> bod
On 25/04/2023 05:40, Bjorn Andersson wrote:
> In multiple places throughout the driver code has been written in
> prepration for handling of orientation switching.
>
> Introduce a typec_orientation in qmp_combo and fill out the various
> "placeholders" with the associated logic. By initializing the
> orientation to "normal" this change has no functional impact, but
> reduces the size of the upcoming introduction of dynamic orientation
> switching.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++----------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 7280f7141961..6748f31da7a3 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/regulator/consumer.h>
> #include <linux/reset.h>
> #include <linux/slab.h>
> +#include <linux/usb/typec.h>
>
> #include <dt-bindings/phy/phy-qcom-qmp.h>
>
> @@ -63,6 +64,10 @@
> /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
> #define CLAMP_EN BIT(0) /* enables i/o clamp_n */
>
> +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */
> +#define SW_PORTSELECT_VAL BIT(0)
> +#define SW_PORTSELECT_MUX BIT(1)
> +
> #define PHY_INIT_COMPLETE_TIMEOUT 10000
>
> struct qmp_phy_init_tbl {
> @@ -1323,6 +1328,8 @@ struct qmp_combo {
> struct clk_fixed_rate pipe_clk_fixed;
> struct clk_hw dp_link_hw;
> struct clk_hw dp_pixel_hw;
> +
> + enum typec_orientation orientation;
> };
>
> static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> {
> u32 val;
> - bool reverse = false;
> + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> + const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
>
> val = DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN |
> DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN;
>
> - /*
> - * TODO: Assume orientation is CC1 for now and two lanes, need to
> - * use type-c connector to understand orientation and lanes.
> - *
> - * Otherwise val changes to be like below if this code understood
> - * the orientation of the type-c cable.
> - *
> - * if (lane_cnt == 4 || orientation == ORIENTATION_CC2)
> - * val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN;
> - * if (lane_cnt == 4 || orientation == ORIENTATION_CC1)
> - * val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN;
> - * if (orientation == ORIENTATION_CC2)
> - * writel(0x4c, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_MODE);
> - */
> - val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN;
> + if (dp_opts->lanes == 4 || reverse)
> + val |= DP_PHY_PD_CTL_LANE_0_1_PWRDN;
> + if (dp_opts->lanes == 4 || !reverse)
> + val |= DP_PHY_PD_CTL_LANE_2_3_PWRDN;
> +
> writel(val, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
>
> - writel(0x5c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE);
> + if (reverse)
> + writel(0x4c, qmp->pcs + QSERDES_DP_PHY_MODE);
> + else
> + writel(0x5c, qmp->pcs + QSERDES_DP_PHY_MODE);
>
> return reverse;
> }
> @@ -2235,7 +2236,7 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp)
> {
> const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
> u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
> - bool reverse = false;
> + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> u32 status;
> int ret;
>
> @@ -2299,7 +2300,7 @@ static int qmp_v5_configure_dp_phy(struct qmp_combo *qmp)
> {
> const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
> u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
> - bool reverse = false;
> + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> u32 status;
> int ret;
>
> @@ -2358,7 +2359,7 @@ static int qmp_v6_configure_dp_phy(struct qmp_combo *qmp)
> {
> const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
> u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
> - bool reverse = false;
> + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> u32 status;
> int ret;
>
> @@ -2462,6 +2463,7 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
> const struct qmp_phy_cfg *cfg = qmp->cfg;
> void __iomem *com = qmp->com;
> int ret;
> + u32 val;
>
> if (qmp->init_count++)
> return 0;
> @@ -2495,10 +2497,12 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
> SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
> SW_USB3PHY_RESET_MUX | SW_USB3PHY_RESET);
>
> - /* Default type-c orientation, i.e CC1 */
> - qphy_setbits(com, QPHY_V3_DP_COM_TYPEC_CTRL, 0x02);
> -
> - qphy_setbits(com, QPHY_V3_DP_COM_PHY_MODE_CTRL, USB3_MODE | DP_MODE);
> + /* Use software based port select and switch on typec orientation */
> + val = SW_PORTSELECT_MUX;
> + 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,
> @@ -3361,6 +3365,8 @@ static int qmp_combo_probe(struct platform_device *pdev)
>
> qmp->dev = dev;
>
> + qmp->orientation = TYPEC_ORIENTATION_NORMAL;
> +
> qmp->cfg = of_device_get_match_data(dev);
> if (!qmp->cfg)
> return -EINVAL;
Reviewed-by: Neil Armstrong <[email protected]>
Hi,
On 25/04/2023 05:40, Bjorn Andersson wrote:
> The data lanes of the QMP PHY is swapped in order to handle changing
> orientation of the USB Type-C cable. Register a typec_switch device to
> allow a TCPM to configure the orientation.
>
> The newly introduced orientation variable is adjusted based on the
> request, and the initialized components are brought down and up again.
> To keep track of what parts needs to be cycled new variables to keep
> track of the individual init_count is introduced.
>
> Both the USB and the DisplayPort altmode signals are properly switched.
> For DisplayPort the controller will after the TCPM having established
> orientation power on the PHY, so this is not done implicitly, but for
> USB the PHY typically is kept initialized across the switch, and must
> therefor then be reinitialized.
therefore
>
> This is based on initial work by Wesley Cheng.
>
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 92 ++++++++++++++++++++---
> 1 file changed, 83 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 6748f31da7a3..5d6d6ef3944b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -20,6 +20,7 @@
> #include <linux/reset.h>
> #include <linux/slab.h>
> #include <linux/usb/typec.h>
> +#include <linux/usb/typec_mux.h>
>
> #include <dt-bindings/phy/phy-qcom-qmp.h>
>
> @@ -1320,15 +1321,18 @@ struct qmp_combo {
>
> struct phy *usb_phy;
> enum phy_mode mode;
> + unsigned int usb_init_count;
>
> struct phy *dp_phy;
> unsigned int dp_aux_cfg;
> struct phy_configure_opts_dp dp_opts;
> + unsigned int dp_init_count;
>
> struct clk_fixed_rate pipe_clk_fixed;
> struct clk_hw dp_link_hw;
> struct clk_hw dp_pixel_hw;
>
> + struct typec_switch_dev *sw;
> enum typec_orientation orientation;
> };
>
> @@ -2458,14 +2462,14 @@ static int qmp_combo_dp_calibrate(struct phy *phy)
> return 0;
> }
>
> -static int qmp_combo_com_init(struct qmp_combo *qmp)
> +static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
> {
> const struct qmp_phy_cfg *cfg = qmp->cfg;
> void __iomem *com = qmp->com;
> int ret;
> u32 val;
>
> - if (qmp->init_count++)
> + if (!force && qmp->init_count++)
> return 0;
>
> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> @@ -2526,11 +2530,11 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
> return ret;
> }
>
> -static int qmp_combo_com_exit(struct qmp_combo *qmp)
> +static int qmp_combo_com_exit(struct qmp_combo *qmp, bool force)
> {
> const struct qmp_phy_cfg *cfg = qmp->cfg;
>
> - if (--qmp->init_count)
> + if (!force && --qmp->init_count)
> return 0;
>
> reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> @@ -2550,12 +2554,14 @@ static int qmp_combo_dp_init(struct phy *phy)
>
> mutex_lock(&qmp->phy_mutex);
>
> - ret = qmp_combo_com_init(qmp);
> + ret = qmp_combo_com_init(qmp, false);
> if (ret)
> goto out_unlock;
>
> cfg->dp_aux_init(qmp);
>
> + qmp->dp_init_count++;
> +
> out_unlock:
> mutex_unlock(&qmp->phy_mutex);
> return ret;
> @@ -2567,8 +2573,9 @@ static int qmp_combo_dp_exit(struct phy *phy)
>
> mutex_lock(&qmp->phy_mutex);
>
> - qmp_combo_com_exit(qmp);
> + qmp_combo_com_exit(qmp, false);
>
> + qmp->dp_init_count--;
> mutex_unlock(&qmp->phy_mutex);
>
> return 0;
> @@ -2688,16 +2695,18 @@ static int qmp_combo_usb_init(struct phy *phy)
> int ret;
>
> mutex_lock(&qmp->phy_mutex);
> - ret = qmp_combo_com_init(qmp);
> + ret = qmp_combo_com_init(qmp, false);
> if (ret)
> goto out_unlock;
>
> ret = qmp_combo_usb_power_on(phy);
> if (ret) {
> - qmp_combo_com_exit(qmp);
> + qmp_combo_com_exit(qmp, false);
> goto out_unlock;
> }
>
> + qmp->usb_init_count++;
> +
> out_unlock:
> mutex_unlock(&qmp->phy_mutex);
> return ret;
> @@ -2713,10 +2722,12 @@ static int qmp_combo_usb_exit(struct phy *phy)
> if (ret)
> goto out_unlock;
>
> - ret = qmp_combo_com_exit(qmp);
> + ret = qmp_combo_com_exit(qmp, false);
> if (ret)
> goto out_unlock;
>
> + qmp->usb_init_count--;
> +
> out_unlock:
> mutex_unlock(&qmp->phy_mutex);
> return ret;
> @@ -3351,6 +3362,65 @@ static struct phy *qmp_combo_phy_xlate(struct device *dev, struct of_phandle_arg
> return ERR_PTR(-EINVAL);
> }
>
> +#if IS_ENABLED(CONFIG_TYPEC)
> +static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw,
> + enum typec_orientation orientation)
> +{
> + struct qmp_combo *qmp = typec_switch_get_drvdata(sw);
> + const struct qmp_phy_cfg *cfg = qmp->cfg;
> +
> + if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
> + return 0;
> +
> + mutex_lock(&qmp->phy_mutex);
> + qmp->orientation = orientation;
> +
> + if (qmp->init_count) {
> + if (qmp->usb_init_count)
> + qmp_combo_usb_power_off(qmp->usb_phy);
> + qmp_combo_com_exit(qmp, true);
> +
> + qmp_combo_com_init(qmp, true);
> + if (qmp->usb_init_count)
> + qmp_combo_usb_power_on(qmp->usb_phy);
> + if (qmp->dp_init_count)
> + cfg->dp_aux_init(qmp);
> + }
> + mutex_unlock(&qmp->phy_mutex);
> +
> + return 0;
> +}
> +
> +static void qmp_combo_typec_unregister(void *data)
> +{
> + struct qmp_combo *qmp = data;
> +
> + typec_switch_unregister(qmp->sw);
> +}
> +
> +static int qmp_combo_typec_switch_register(struct qmp_combo *qmp)
> +{
> + struct typec_switch_desc sw_desc = {};
> + struct device *dev = qmp->dev;
> +
> + sw_desc.drvdata = qmp;
> + sw_desc.fwnode = dev->fwnode;
> + sw_desc.set = qmp_combo_typec_switch_set;
> + qmp->sw = typec_switch_register(dev, &sw_desc);
> + if (IS_ERR(qmp->sw)) {
> + dev_err(dev, "Unable to register typec switch: %pe\n", qmp->sw);
> + return PTR_ERR(qmp->sw);
> + }
> +
> + return devm_add_action_or_reset(dev, qmp_combo_typec_unregister, qmp);
> +}
> +#else
> +static int qmp_combo_typec_switch_register(struct qmp_combo *qmp)
> +{
> + return 0;
> +}
> +#endif
> +
> static int qmp_combo_probe(struct platform_device *pdev)
> {
> struct qmp_combo *qmp;
> @@ -3385,6 +3455,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ret = qmp_combo_typec_switch_register(qmp);
> + if (ret)
> + return ret;
> +
> /* Check for legacy binding with child nodes. */
> usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
> if (usb_np) {
Thanks for taking care of this, with the commit typo fix:
Reviewed-by: Neil Armstrong <[email protected]>
On 27/04/2023 01:33, Konrad Dybcio wrote:
>
> On 4/25/23 04:40, Bjorn Andersson wrote:
>> With support for the QMP combo phy to react to USB Type-C switch events,
>> introduce it as the next hop for the SuperSpeed lanes of the two USB
>> Type-C connectors, and connect the output of the DisplayPort controller
>> to the QMP combo phy.
>>
>> This allows the TCPM to perform orientation switching of both USB and
>> DisplayPort signals.
>>
>> Signed-off-by: Bjorn Andersson <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
>> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++++++++++++++++++++++
>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>> index 547277924ea3..33c973661fa5 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>> @@ -64,7 +64,7 @@ port@1 {
>> reg = <1>;
>> pmic_glink_con0_ss: endpoint {
>> - remote-endpoint = <&mdss0_dp0_out>;
>> + remote-endpoint = <&usb_0_qmpphy_out>;
>> };
>> };
>> @@ -99,7 +99,7 @@ port@1 {
>> reg = <1>;
>> pmic_glink_con1_ss: endpoint {
>> - remote-endpoint = <&mdss0_dp1_out>;
>> + remote-endpoint = <&usb_1_qmpphy_out>;
>> };
>> };
>> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>> &mdss0_dp0_out {
>> data-lanes = <0 1>;
>> - remote-endpoint = <&pmic_glink_con0_ss>;
>> + remote-endpoint = <&usb_0_qmpphy_dp_in>;
>> };
>> &mdss0_dp1 {
>> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>> &mdss0_dp1_out {
>> data-lanes = <0 1>;
>> - remote-endpoint = <&pmic_glink_con1_ss>;
>> + remote-endpoint = <&usb_1_qmpphy_dp_in>;
>> };
>> &mdss0_dp3 {
>> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
>> vdda-phy-supply = <&vreg_l9d>;
>> vdda-pll-supply = <&vreg_l4d>;
>> + orientation-switch;
>
> I believe this belongs in the SoC DTSI, as it's supported by
> the PHY block itself
>
>
> The rest seems to lgtm..
>
>
> On a note, why did we end up placing pmic_glink in device
> DTs? It's already assumed that we're using the full Qualcomm
> stack as we use PAS for remoteprocs so I *think* we can always
> assume pmic_glink would be there!
As we did on other board, I think because having pmic_glink depends
on the board firmware capabilities ? Boards without USB-C won't need/have
pmic_link right ?
Neil
>
> Konrad
>
>> +
>> status = "okay";
>> };
>> +&usb_0_qmpphy_dp_in {
>> + remote-endpoint = <&mdss0_dp0_out>;
>> +};
>> +
>> +&usb_0_qmpphy_out {
>> + remote-endpoint = <&pmic_glink_con0_ss>;
>> +};
>> +
>> &usb_0_role_switch {
>> remote-endpoint = <&pmic_glink_con0_hs>;
>> };
>> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
>> vdda-phy-supply = <&vreg_l4b>;
>> vdda-pll-supply = <&vreg_l3b>;
>> + orientation-switch;
>> +
>> status = "okay";
>> };
>> +&usb_1_qmpphy_dp_in {
>> + remote-endpoint = <&mdss0_dp1_out>;
>> +};
>> +
>> +&usb_1_qmpphy_out {
>> + remote-endpoint = <&pmic_glink_con1_ss>;
>> +};
>> +
>> &usb_1_role_switch {
>> remote-endpoint = <&pmic_glink_con1_hs>;
>> };
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index 0e691bb0120c..1eb3a295e8fa 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
>> #phy-cells = <1>;
>> status = "disabled";
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@0 {
>> + reg = <0>;
>> +
>> + usb_0_qmpphy_out: endpoint {};
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> +
>> + usb_0_qmpphy_dp_in: endpoint {};
>> + };
>> + };
>> };
>> usb_1_hsphy: phy@8902000 {
>> @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
>> #phy-cells = <1>;
>> status = "disabled";
>> +
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + port@0 {
>> + reg = <0>;
>> +
>> + usb_1_qmpphy_out: endpoint {};
>> + };
>> +
>> + port@1 {
>> + reg = <1>;
>> +
>> + usb_1_qmpphy_dp_in: endpoint {};
>> + };
>> + };
>> };
>> mdss1_dp0_phy: phy@8909a00 {
On 27/04/2023 16:11, Neil Armstrong wrote:
> On 26/04/2023 12:33, Bryan O'Donoghue wrote:
>> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
>> <[email protected]> wrote:
>>>
>>> The QMP combo PHY sits in an of_graph connected between the DisplayPort
>>> controller and a USB Type-C connector (or possibly a redriver).
>>>
>>> The TCPM needs to be able to convey the HPD signal to the DisplayPort
>>> controller, but no directly link is provided by DeviceTree so the signal
>>> needs to "pass through" the QMP combo phy.
>>>
>>> Handle this by introducing a drm_bridge which upon initialization finds
>>> the next bridge (i.e. the usb-c-connector) and chain this together. This
>>> way HPD changes in the connector will propagate to the DisplayPort
>>> driver.
>>>
>>> The connector bridge is resolved lazily, as the TCPM is expected to be
>>> able to resolve the typec mux and switch at probe time, so the QMP combo
>>> phy will probe before the TCPM.
>>>
>>> Signed-off-by: Bjorn Andersson <[email protected]>
>>> ---
>>> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>>> 1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> index 5d6d6ef3944b..84bc08002537 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> @@ -22,6 +22,8 @@
>>> #include <linux/usb/typec.h>
>>> #include <linux/usb/typec_mux.h>
>>>
>>> +#include <drm/drm_bridge.h>
>>> +
>>> #include <dt-bindings/phy/phy-qcom-qmp.h>
>>>
>>> #include "phy-qcom-qmp.h"
>>> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>>> struct clk_hw dp_link_hw;
>>> struct clk_hw dp_pixel_hw;
>>>
>>> + struct drm_bridge bridge;
>>> +
>>> struct typec_switch_dev *sw;
>>> enum typec_orientation orientation;
>>> };
>>> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct
>>> qmp_combo *qmp, struct device_node *
>>> return devm_add_action_or_reset(qmp->dev,
>>> phy_clk_release_provider, dp_np);
>>> }
>>>
>>> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
>>> + enum drm_bridge_attach_flags flags)
>>> +{
>>> + struct qmp_combo *qmp = container_of(bridge, struct
>>> qmp_combo, bridge);
>>> + struct drm_bridge *next_bridge;
>>> +
>>> + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>> + return -EINVAL;
>>> +
>>> + next_bridge = devm_drm_of_get_bridge(qmp->dev,
>>> qmp->dev->of_node, 0, 0);
>>> + if (IS_ERR(next_bridge))
>>> + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge),
>>> "failed to acquire drm_bridge\n");
>>> +
>>> + return drm_bridge_attach(bridge->encoder, next_bridge,
>>> bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> +}
>>> +
>>> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
>>> + .attach = qmp_combo_bridge_attach,
>>> +};
>>> +
>>> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
>>> +{
>>> + qmp->bridge.funcs = &qmp_combo_bridge_funcs;
>>> + qmp->bridge.of_node = qmp->dev->of_node;
>>> +
>>> + return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
>>> +}
>>> +
>>> static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp,
>>> struct device_node *np)
>>> {
>>> struct device *dev = qmp->dev;
>>> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct
>>> platform_device *pdev)
>>> if (ret)
>>> return ret;
>>>
>>> + ret = qmp_combo_dp_register_bridge(qmp);
>>> + if (ret)
>>> + return ret;
>
> I think the DRM part should be only built if CONFIG_DRM is enabled, I don't
> have a strong opinion on this, I think Vinod could help here.
>
>>> +
>>> /* Check for legacy binding with child nodes. */
>>> usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>>> if (usb_np) {
>>> --
>>> 2.39.2
>>>
>>
>> You need to add some or all of these
>> select DRM_DISPLAY_DP_HELPER
>> select DRM_DISPLAY_HELPER
>> select DRM_DP_AUX_BUS
>> select DRM_KMS_HELPER
>> select DRM_MIPI_DSI
>> select DRM_PANEL
>>
>>
>> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
>> Unexpected GOT/PLT entries detected!
>> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
>> Unexpected run-time procedure linkages detected!
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
>> `qmp_combo_bridge_attach':
>> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
>> `devm_drm_of_get_bridge'
>> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to
>> `drm_bridge_attach'
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
>> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
>> `devm_drm_bridge_add'
>
> I think CONFIG_DRM_PANEL_BRIDGE in addition to CONFIG_DRM. should be
> enough.
>
I'd say DRM_PANEL_BRIDGE || !DRM_PANEL_BRIDGE in addition to DRM. And we
probably should fix the devm_drm_of_get_bridge() stub to use
drm_of_find_panel_or_bridge() with panel = NULL.
> With this config added and my drm-bridge hat:
>
> Acked-by: Neil Armstrong <[email protected]>
>
> Neil
>
>
>>
>> ---
>> bod
>
--
With best wishes
Dmitry
On Wed, Apr 26, 2023 at 11:33:40AM +0100, Bryan O'Donoghue wrote:
> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
> <[email protected]> wrote:
> >
[..]
> You need to add some or all of these
> select DRM_DISPLAY_DP_HELPER
> select DRM_DISPLAY_HELPER
> select DRM_DP_AUX_BUS
> select DRM_KMS_HELPER
> select DRM_MIPI_DSI
> select DRM_PANEL
>
>
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected GOT/PLT entries detected!
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected run-time procedure linkages detected!
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
> `qmp_combo_bridge_attach':
> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
> `devm_drm_of_get_bridge'
> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
> `devm_drm_bridge_add'
>
You're correct, and TYPEC. Realized that I forgot these once I had
posted the patches. Will figure out the actual set for v2.
Thanks,
Bjorn
On Thu, Apr 27, 2023 at 12:33:44AM +0100, Konrad Dybcio wrote:
>
> On 4/25/23 04:40, Bjorn Andersson wrote:
> > With support for the QMP combo phy to react to USB Type-C switch events,
> > introduce it as the next hop for the SuperSpeed lanes of the two USB
> > Type-C connectors, and connect the output of the DisplayPort controller
> > to the QMP combo phy.
> >
> > This allows the TCPM to perform orientation switching of both USB and
> > DisplayPort signals.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
> > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++++++++++++++++++++++
> > 2 files changed, 58 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > index 547277924ea3..33c973661fa5 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > @@ -64,7 +64,7 @@ port@1 {
> > reg = <1>;
> > pmic_glink_con0_ss: endpoint {
> > - remote-endpoint = <&mdss0_dp0_out>;
> > + remote-endpoint = <&usb_0_qmpphy_out>;
> > };
> > };
> > @@ -99,7 +99,7 @@ port@1 {
> > reg = <1>;
> > pmic_glink_con1_ss: endpoint {
> > - remote-endpoint = <&mdss0_dp1_out>;
> > + remote-endpoint = <&usb_1_qmpphy_out>;
> > };
> > };
> > @@ -412,7 +412,7 @@ &mdss0_dp0 {
> > &mdss0_dp0_out {
> > data-lanes = <0 1>;
> > - remote-endpoint = <&pmic_glink_con0_ss>;
> > + remote-endpoint = <&usb_0_qmpphy_dp_in>;
> > };
> > &mdss0_dp1 {
> > @@ -421,7 +421,7 @@ &mdss0_dp1 {
> > &mdss0_dp1_out {
> > data-lanes = <0 1>;
> > - remote-endpoint = <&pmic_glink_con1_ss>;
> > + remote-endpoint = <&usb_1_qmpphy_dp_in>;
> > };
> > &mdss0_dp3 {
> > @@ -670,9 +670,19 @@ &usb_0_qmpphy {
> > vdda-phy-supply = <&vreg_l9d>;
> > vdda-pll-supply = <&vreg_l4d>;
> > + orientation-switch;
>
> I believe this belongs in the SoC DTSI, as it's supported by
> the PHY block itself
>
Sounds reasonable to me, will move it.
>
> The rest seems to lgtm..
>
>
> On a note, why did we end up placing pmic_glink in device
> DTs? It's already assumed that we're using the full Qualcomm
> stack as we use PAS for remoteprocs so I *think* we can always
> assume pmic_glink would be there!
>
In this particular case, sa8295p and sa8540p are derived from sc8280xp
dtsi, but those does not implement pmic_glink...
In most other cases I think your expectation would hold though.
Perhaps would be suitable to put the pmic_glink in sc8280xp.dtsi but
disable it, and then add connectors in the device.
Regards,
Bjorn
> Konrad
>
> > +
> > status = "okay";
> > };
> > +&usb_0_qmpphy_dp_in {
> > + remote-endpoint = <&mdss0_dp0_out>;
> > +};
> > +
> > +&usb_0_qmpphy_out {
> > + remote-endpoint = <&pmic_glink_con0_ss>;
> > +};
> > +
> > &usb_0_role_switch {
> > remote-endpoint = <&pmic_glink_con0_hs>;
> > };
> > @@ -697,9 +707,19 @@ &usb_1_qmpphy {
> > vdda-phy-supply = <&vreg_l4b>;
> > vdda-pll-supply = <&vreg_l3b>;
> > + orientation-switch;
> > +
> > status = "okay";
> > };
> > +&usb_1_qmpphy_dp_in {
> > + remote-endpoint = <&mdss0_dp1_out>;
> > +};
> > +
> > +&usb_1_qmpphy_out {
> > + remote-endpoint = <&pmic_glink_con1_ss>;
> > +};
> > +
> > &usb_1_role_switch {
> > remote-endpoint = <&pmic_glink_con1_hs>;
> > };
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index 0e691bb0120c..1eb3a295e8fa 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
> > #phy-cells = <1>;
> > status = "disabled";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + usb_0_qmpphy_out: endpoint {};
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + usb_0_qmpphy_dp_in: endpoint {};
> > + };
> > + };
> > };
> > usb_1_hsphy: phy@8902000 {
> > @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
> > #phy-cells = <1>;
> > status = "disabled";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + usb_1_qmpphy_out: endpoint {};
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + usb_1_qmpphy_dp_in: endpoint {};
> > + };
> > + };
> > };
> > mdss1_dp0_phy: phy@8909a00 {
On Thu, Apr 27, 2023 at 8:56 PM Bjorn Andersson
<[email protected]> wrote:
>
> On Wed, Apr 26, 2023 at 11:33:40AM +0100, Bryan O'Donoghue wrote:
> > On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
> > <[email protected]> wrote:
> > >
> [..]
> > You need to add some or all of these
> > select DRM_DISPLAY_DP_HELPER
> > select DRM_DISPLAY_HELPER
> > select DRM_DP_AUX_BUS
> > select DRM_KMS_HELPER
> > select DRM_MIPI_DSI
> > select DRM_PANEL
> >
> >
> > /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> > Unexpected GOT/PLT entries detected!
> > /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> > Unexpected run-time procedure linkages detected!
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
> > `qmp_combo_bridge_attach':
> > phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
> > `devm_drm_of_get_bridge'
> > phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
> > phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
> > `devm_drm_bridge_add'
> >
>
> You're correct, and TYPEC. Realized that I forgot these once I had
> posted the patches. Will figure out the actual set for v2.
>
> Thanks,
> Bjorn
So I added CONFIG_DRM to Kconfig for the combo phy and then replaced
the old patch we had with your series.
Works for me with my TCPM set with zero changes - aside from slotting
the old PHY patch with your expanded series on SM8250
Tested-by: Bryan O'Donoghue <[email protected]>
Reviewed-by: Bryan O'Donoghue <[email protected]>
https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-04-28-pm8150b-tcpm-qcom-wrapper-typec-mux-bjorn
On Mon, Apr 24, 2023 at 08:40:05PM -0700, Bjorn Andersson wrote:
> With the upcoming introduction of USB Type-C orientation switching the
> region of mutual exclusion needs to be extended to cover both the common
> init/exit as well as the individual functions.
>
> So move the phy_mutex one step up the stack.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 51 +++++++++++++----------
> 1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 6850e04c329b..7280f7141961 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -2463,16 +2463,13 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
> void __iomem *com = qmp->com;
> int ret;
>
> - mutex_lock(&qmp->phy_mutex);
> - if (qmp->init_count++) {
> - mutex_unlock(&qmp->phy_mutex);
> + if (qmp->init_count++)
> return 0;
> - }
>
> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> if (ret) {
> dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
> - goto err_unlock;
> + goto err;
I was going to say that you can just return ret directly but then
realised we have a counter imbalance here that should be fixed.
I've just sent a couple of fixes which you could rebase on:
https://lore.kernel.org/r/[email protected]
> }
>
> ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> @@ -2514,16 +2511,13 @@ static int qmp_combo_com_init(struct qmp_combo *qmp)
> qphy_setbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> SW_PWRDN);
>
> - mutex_unlock(&qmp->phy_mutex);
> -
> return 0;
>
> err_assert_reset:
> reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> err_disable_regulators:
> regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> -err_unlock:
> - mutex_unlock(&qmp->phy_mutex);
> +err:
>
> return ret;
> }
> @@ -2686,14 +2683,19 @@ static int qmp_combo_usb_init(struct phy *phy)
> struct qmp_combo *qmp = phy_get_drvdata(phy);
> int ret;
>
> + mutex_lock(&qmp->phy_mutex);
Nit: I think adding a newline here would improve readability.
> ret = qmp_combo_com_init(qmp);
> if (ret)
> - return ret;
> + goto out_unlock;
>
> ret = qmp_combo_usb_power_on(phy);
> - if (ret)
> + if (ret) {
> qmp_combo_com_exit(qmp);
> + goto out_unlock;
> + }
>
> +out_unlock:
> + mutex_unlock(&qmp->phy_mutex);
Same here.
> return ret;
> }
>
> @@ -2702,11 +2704,18 @@ static int qmp_combo_usb_exit(struct phy *phy)
> struct qmp_combo *qmp = phy_get_drvdata(phy);
> int ret;
>
> + mutex_lock(&qmp->phy_mutex);
And here.
> ret = qmp_combo_usb_power_off(phy);
> if (ret)
> - return ret;
> + goto out_unlock;
>
> - return qmp_combo_com_exit(qmp);
> + ret = qmp_combo_com_exit(qmp);
> + if (ret)
> + goto out_unlock;
> +
> +out_unlock:
> + mutex_unlock(&qmp->phy_mutex);
And here.
> + return ret;
> }
>
> static int qmp_combo_usb_set_mode(struct phy *phy, enum phy_mode mode, int submode)
Looks good otherwise:
Reviewed-by: Johan Hovold <[email protected]>
Johan
On 27.04.2023 15:27, Neil Armstrong wrote:
> On 27/04/2023 01:33, Konrad Dybcio wrote:
>>
>> On 4/25/23 04:40, Bjorn Andersson wrote:
>>> With support for the QMP combo phy to react to USB Type-C switch events,
>>> introduce it as the next hop for the SuperSpeed lanes of the two USB
>>> Type-C connectors, and connect the output of the DisplayPort controller
>>> to the QMP combo phy.
>>>
>>> This allows the TCPM to perform orientation switching of both USB and
>>> DisplayPort signals.
>>>
>>> Signed-off-by: Bjorn Andersson <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
>>> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++++++++++++++++++++++
>>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>>> index 547277924ea3..33c973661fa5 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>>> @@ -64,7 +64,7 @@ port@1 {
>>> reg = <1>;
>>> pmic_glink_con0_ss: endpoint {
>>> - remote-endpoint = <&mdss0_dp0_out>;
>>> + remote-endpoint = <&usb_0_qmpphy_out>;
>>> };
>>> };
>>> @@ -99,7 +99,7 @@ port@1 {
>>> reg = <1>;
>>> pmic_glink_con1_ss: endpoint {
>>> - remote-endpoint = <&mdss0_dp1_out>;
>>> + remote-endpoint = <&usb_1_qmpphy_out>;
>>> };
>>> };
>>> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>>> &mdss0_dp0_out {
>>> data-lanes = <0 1>;
>>> - remote-endpoint = <&pmic_glink_con0_ss>;
>>> + remote-endpoint = <&usb_0_qmpphy_dp_in>;
>>> };
>>> &mdss0_dp1 {
>>> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>>> &mdss0_dp1_out {
>>> data-lanes = <0 1>;
>>> - remote-endpoint = <&pmic_glink_con1_ss>;
>>> + remote-endpoint = <&usb_1_qmpphy_dp_in>;
>>> };
>>> &mdss0_dp3 {
>>> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
>>> vdda-phy-supply = <&vreg_l9d>;
>>> vdda-pll-supply = <&vreg_l4d>;
>>> + orientation-switch;
>>
>> I believe this belongs in the SoC DTSI, as it's supported by
>> the PHY block itself
>>
>>
>> The rest seems to lgtm..
>>
>>
>> On a note, why did we end up placing pmic_glink in device
>> DTs? It's already assumed that we're using the full Qualcomm
>> stack as we use PAS for remoteprocs so I *think* we can always
>> assume pmic_glink would be there!
>
> As we did on other board, I think because having pmic_glink depends
> on the board firmware capabilities ? Boards without USB-C won't need/have
> pmic_link right ?
PMIC_GLINK takes care of USB-C, but also enables battmgr and friends
Konrad
>
> Neil
>
>>
>> Konrad
>>
>>> +
>>> status = "okay";
>>> };
>>> +&usb_0_qmpphy_dp_in {
>>> + remote-endpoint = <&mdss0_dp0_out>;
>>> +};
>>> +
>>> +&usb_0_qmpphy_out {
>>> + remote-endpoint = <&pmic_glink_con0_ss>;
>>> +};
>>> +
>>> &usb_0_role_switch {
>>> remote-endpoint = <&pmic_glink_con0_hs>;
>>> };
>>> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
>>> vdda-phy-supply = <&vreg_l4b>;
>>> vdda-pll-supply = <&vreg_l3b>;
>>> + orientation-switch;
>>> +
>>> status = "okay";
>>> };
>>> +&usb_1_qmpphy_dp_in {
>>> + remote-endpoint = <&mdss0_dp1_out>;
>>> +};
>>> +
>>> +&usb_1_qmpphy_out {
>>> + remote-endpoint = <&pmic_glink_con1_ss>;
>>> +};
>>> +
>>> &usb_1_role_switch {
>>> remote-endpoint = <&pmic_glink_con1_hs>;
>>> };
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> index 0e691bb0120c..1eb3a295e8fa 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
>>> #phy-cells = <1>;
>>> status = "disabled";
>>> +
>>> + ports {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + port@0 {
>>> + reg = <0>;
>>> +
>>> + usb_0_qmpphy_out: endpoint {};
>>> + };
>>> +
>>> + port@1 {
>>> + reg = <1>;
>>> +
>>> + usb_0_qmpphy_dp_in: endpoint {};
>>> + };
>>> + };
>>> };
>>> usb_1_hsphy: phy@8902000 {
>>> @@ -3042,6 +3059,23 @@ usb_1_qmpphy: phy@8903000 {
>>> #phy-cells = <1>;
>>> status = "disabled";
>>> +
>>> + ports {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + port@0 {
>>> + reg = <0>;
>>> +
>>> + usb_1_qmpphy_out: endpoint {};
>>> + };
>>> +
>>> + port@1 {
>>> + reg = <1>;
>>> +
>>> + usb_1_qmpphy_dp_in: endpoint {};
>>> + };
>>> + };
>>> };
>>> mdss1_dp0_phy: phy@8909a00 {
>
On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote:
> In multiple places throughout the driver code has been written in
> prepration for handling of orientation switching.
>
> Introduce a typec_orientation in qmp_combo and fill out the various
> "placeholders" with the associated logic. By initializing the
> orientation to "normal" this change has no functional impact, but
> reduces the size of the upcoming introduction of dynamic orientation
> switching.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++----------
> 1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 7280f7141961..6748f31da7a3 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/regulator/consumer.h>
> #include <linux/reset.h>
> #include <linux/slab.h>
> +#include <linux/usb/typec.h>
>
> #include <dt-bindings/phy/phy-qcom-qmp.h>
>
> @@ -63,6 +64,10 @@
> /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
> #define CLAMP_EN BIT(0) /* enables i/o clamp_n */
>
> +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */
> +#define SW_PORTSELECT_VAL BIT(0)
> +#define SW_PORTSELECT_MUX BIT(1)
> +
> #define PHY_INIT_COMPLETE_TIMEOUT 10000
>
> struct qmp_phy_init_tbl {
> @@ -1323,6 +1328,8 @@ struct qmp_combo {
> struct clk_fixed_rate pipe_clk_fixed;
> struct clk_hw dp_link_hw;
> struct clk_hw dp_pixel_hw;
> +
> + enum typec_orientation orientation;
> };
>
> static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> {
> u32 val;
> - bool reverse = false;
> + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
Adding parentheses around the right-hand side should make this a little
easier to parse.
It also looks like these callbacks end up being called without holding
the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a
concurrent switch notification and dp phy power-on but it's not that
obvious.
> + const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
Also could you add these before u32 val to maintain an approximation of
reverse xmas style?
And similar below.
Johan
On Mon, Apr 24, 2023 at 08:40:07PM -0700, Bjorn Andersson wrote:
> The data lanes of the QMP PHY is swapped in order to handle changing
> orientation of the USB Type-C cable. Register a typec_switch device to
> allow a TCPM to configure the orientation.
>
> The newly introduced orientation variable is adjusted based on the
> request, and the initialized components are brought down and up again.
> To keep track of what parts needs to be cycled new variables to keep
> track of the individual init_count is introduced.
>
> Both the USB and the DisplayPort altmode signals are properly switched.
> For DisplayPort the controller will after the TCPM having established
> orientation power on the PHY, so this is not done implicitly, but for
> USB the PHY typically is kept initialized across the switch, and must
> therefor then be reinitialized.
>
> This is based on initial work by Wesley Cheng.
>
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 92 ++++++++++++++++++++---
> 1 file changed, 83 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 6748f31da7a3..5d6d6ef3944b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -2567,8 +2573,9 @@ static int qmp_combo_dp_exit(struct phy *phy)
>
> mutex_lock(&qmp->phy_mutex);
>
> - qmp_combo_com_exit(qmp);
> + qmp_combo_com_exit(qmp, false);
>
> + qmp->dp_init_count--;
Nit: add a newline for symmetry.
> mutex_unlock(&qmp->phy_mutex);
>
> return 0;
> @@ -3351,6 +3362,65 @@ static struct phy *qmp_combo_phy_xlate(struct device *dev, struct of_phandle_arg
> return ERR_PTR(-EINVAL);
> }
>
> +#if IS_ENABLED(CONFIG_TYPEC)
> +static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw,
> + enum typec_orientation orientation)
> +{
> + struct qmp_combo *qmp = typec_switch_get_drvdata(sw);
> + const struct qmp_phy_cfg *cfg = qmp->cfg;
> +
> + if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
> + return 0;
> +
> + mutex_lock(&qmp->phy_mutex);
> + qmp->orientation = orientation;
> +
> + if (qmp->init_count) {
> + if (qmp->usb_init_count)
> + qmp_combo_usb_power_off(qmp->usb_phy);
> + qmp_combo_com_exit(qmp, true);
> +
> + qmp_combo_com_init(qmp, true);
> + if (qmp->usb_init_count)
> + qmp_combo_usb_power_on(qmp->usb_phy);
> + if (qmp->dp_init_count)
> + cfg->dp_aux_init(qmp);
> + }
> + mutex_unlock(&qmp->phy_mutex);
> +
> + return 0;
> +}
> +
> +static void qmp_combo_typec_unregister(void *data)
> +{
> + struct qmp_combo *qmp = data;
> +
> + typec_switch_unregister(qmp->sw);
> +}
> +
> +static int qmp_combo_typec_switch_register(struct qmp_combo *qmp)
> +{
> + struct typec_switch_desc sw_desc = {};
> + struct device *dev = qmp->dev;
> +
> + sw_desc.drvdata = qmp;
> + sw_desc.fwnode = dev->fwnode;
> + sw_desc.set = qmp_combo_typec_switch_set;
Nit: I'd add a newline here for readability.
> + qmp->sw = typec_switch_register(dev, &sw_desc);
> + if (IS_ERR(qmp->sw)) {
> + dev_err(dev, "Unable to register typec switch: %pe\n", qmp->sw);
> + return PTR_ERR(qmp->sw);
> + }
> +
> + return devm_add_action_or_reset(dev, qmp_combo_typec_unregister, qmp);
> +}
> +#else
> +static int qmp_combo_typec_switch_register(struct qmp_combo *qmp)
> +{
> + return 0;
> +}
> +#endif
Perhaps you can move the type-c block after qmp_combo_register_clocks()
above to keep the type-c and later added bridge functions together and
better reflect the probe init order (e.g. keeping the dt-functions just
above probe()).
> +
> static int qmp_combo_probe(struct platform_device *pdev)
> {
> struct qmp_combo *qmp;
> @@ -3385,6 +3455,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ret = qmp_combo_typec_switch_register(qmp);
> + if (ret)
> + return ret;
> +
> /* Check for legacy binding with child nodes. */
> usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
> if (usb_np) {
Looks good otherwise:
Reviewed-by: Johan Hovold <[email protected]>
Johan
On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> The QMP combo PHY sits in an of_graph connected between the DisplayPort
> controller and a USB Type-C connector (or possibly a redriver).
>
> The TCPM needs to be able to convey the HPD signal to the DisplayPort
> controller, but no directly link is provided by DeviceTree so the signal
> needs to "pass through" the QMP combo phy.
>
> Handle this by introducing a drm_bridge which upon initialization finds
> the next bridge (i.e. the usb-c-connector) and chain this together. This
> way HPD changes in the connector will propagate to the DisplayPort
> driver.
>
> The connector bridge is resolved lazily, as the TCPM is expected to be
> able to resolve the typec mux and switch at probe time, so the QMP combo
> phy will probe before the TCPM.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 5d6d6ef3944b..84bc08002537 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -22,6 +22,8 @@
> #include <linux/usb/typec.h>
> #include <linux/usb/typec_mux.h>
>
> +#include <drm/drm_bridge.h>
> +
> #include <dt-bindings/phy/phy-qcom-qmp.h>
>
> #include "phy-qcom-qmp.h"
> @@ -1332,6 +1334,8 @@ struct qmp_combo {
> struct clk_hw dp_link_hw;
> struct clk_hw dp_pixel_hw;
>
> + struct drm_bridge bridge;
> +
> struct typec_switch_dev *sw;
> enum typec_orientation orientation;
> };
> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> }
>
> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> + enum drm_bridge_attach_flags flags)
> +{
> + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> + struct drm_bridge *next_bridge;
> +
> + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> + return -EINVAL;
> +
> + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> + if (IS_ERR(next_bridge))
> + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
Using dev_err_probe() in an attach callback looks wrong as these
functions should not be returning -EPROBE_DEFER (and this is not a probe
function).
> +
> + return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
This line is over 100 chars, but there should be no reason not to break
it before 80 here.
> +}
> +
> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
> + .attach = qmp_combo_bridge_attach,
> +};
> +
> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
> +{
> + qmp->bridge.funcs = &qmp_combo_bridge_funcs;
> + qmp->bridge.of_node = qmp->dev->of_node;
> +
> + return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
> +}
Guess you need a dummy function also for qmp_combo_dp_register_bridge()
in case of !CONFIG_DRM.
> +
> static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
> {
> struct device *dev = qmp->dev;
> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + ret = qmp_combo_dp_register_bridge(qmp);
> + if (ret)
> + return ret;
> +
> /* Check for legacy binding with child nodes. */
> usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
> if (usb_np) {
Johan
On Mon, Apr 24, 2023 at 08:40:09PM -0700, Bjorn Andersson wrote:
> With support for the QMP combo phy to react to USB Type-C switch events,
> introduce it as the next hop for the SuperSpeed lanes of the two USB
> Type-C connectors, and connect the output of the DisplayPort controller
> to the QMP combo phy.
>
> This allows the TCPM to perform orientation switching of both USB and
> DisplayPort signals.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++++++++++++++++---
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 547277924ea3..33c973661fa5 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -64,7 +64,7 @@ port@1 {
> reg = <1>;
>
> pmic_glink_con0_ss: endpoint {
> - remote-endpoint = <&mdss0_dp0_out>;
> + remote-endpoint = <&usb_0_qmpphy_out>;
> };
> };
>
> @@ -99,7 +99,7 @@ port@1 {
> reg = <1>;
>
> pmic_glink_con1_ss: endpoint {
> - remote-endpoint = <&mdss0_dp1_out>;
> + remote-endpoint = <&usb_1_qmpphy_out>;
> };
> };
>
> @@ -412,7 +412,7 @@ &mdss0_dp0 {
>
> &mdss0_dp0_out {
> data-lanes = <0 1>;
> - remote-endpoint = <&pmic_glink_con0_ss>;
> + remote-endpoint = <&usb_0_qmpphy_dp_in>;
> };
It's a bit hard to follow what going on when using place holder nodes
from the dtsi like this (instead of describing all the ports directly in
the board dts). IIRC we went a bit back and forth over this earlier and
we already use this scheme for the display port controllers, so I guess
this is the price we pay for being consistent.
> &mdss0_dp1 {
> @@ -421,7 +421,7 @@ &mdss0_dp1 {
>
> &mdss0_dp1_out {
> data-lanes = <0 1>;
> - remote-endpoint = <&pmic_glink_con1_ss>;
> + remote-endpoint = <&usb_1_qmpphy_dp_in>;
> };
>
> &mdss0_dp3 {
> @@ -670,9 +670,19 @@ &usb_0_qmpphy {
> vdda-phy-supply = <&vreg_l9d>;
> vdda-pll-supply = <&vreg_l4d>;
>
> + orientation-switch;
> +
> status = "okay";
> };
>
> +&usb_0_qmpphy_dp_in {
> + remote-endpoint = <&mdss0_dp0_out>;
> +};
> +
> +&usb_0_qmpphy_out {
> + remote-endpoint = <&pmic_glink_con0_ss>;
> +};
> +
> &usb_0_role_switch {
> remote-endpoint = <&pmic_glink_con0_hs>;
> };
> @@ -697,9 +707,19 @@ &usb_1_qmpphy {
> vdda-phy-supply = <&vreg_l4b>;
> vdda-pll-supply = <&vreg_l3b>;
>
> + orientation-switch;
> +
> status = "okay";
> };
>
> +&usb_1_qmpphy_dp_in {
> + remote-endpoint = <&mdss0_dp1_out>;
> +};
> +
> +&usb_1_qmpphy_out {
> + remote-endpoint = <&pmic_glink_con1_ss>;
> +};
> +
> &usb_1_role_switch {
> remote-endpoint = <&pmic_glink_con1_hs>;
> };
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 0e691bb0120c..1eb3a295e8fa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3006,6 +3006,23 @@ usb_0_qmpphy: phy@88eb000 {
> #phy-cells = <1>;
>
> status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + usb_0_qmpphy_out: endpoint {};
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + usb_0_qmpphy_dp_in: endpoint {};
> + };
> + };
> };
The binding describes three ports, where dp-in is port 2.
Perhaps you don't need to describe ss-in yet, but shouldn't the port
numbers match? Should some of these be described as required in the
binding?
Johan
On Mon, Apr 24, 2023 at 08:40:03PM -0700, Bjorn Andersson wrote:
> This adds support for USB and DisplayPort orientation switching to the
> QMP combo PHY, as well as updating the sc8280xp devices to include the
> QMP in the SuperSpeed graph.
Nice and clean series!
I've tested it a bit on the X13s and verified that DP works on both
ports and in both orientations. Coldplug also appears to work reliably.
Tested-by: Johan Hovold <[email protected]> # X13s
> Bjorn Andersson (7):
> dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and
> orientation-switch
> phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
> phy: qcom-qmp-combo: Introduce orientation variable
> phy: qcom-qmp-combo: Introduce orientation switching
> phy: qcom-qmp-combo: Introduce drm_bridge
> arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
> arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
>
> .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 51 ++++
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++-
> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 28 ++-
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 227 ++++++++++++++----
> 5 files changed, 309 insertions(+), 59 deletions(-)
Johan
On 25/04/2023 05:40, Bjorn Andersson wrote:
> This adds support for USB and DisplayPort orientation switching to the
> QMP combo PHY, as well as updating the sc8280xp devices to include the
> QMP in the SuperSpeed graph.
>
> Bjorn Andersson (7):
> dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and
> orientation-switch
> phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
> phy: qcom-qmp-combo: Introduce orientation variable
> phy: qcom-qmp-combo: Introduce orientation switching
> phy: qcom-qmp-combo: Introduce drm_bridge
> arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
> arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
>
> .../phy/qcom,sc8280xp-qmp-usb43dp-phy.yaml | 51 ++++
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 28 ++-
> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 28 ++-
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 34 +++
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 227 ++++++++++++++----
> 5 files changed, 309 insertions(+), 59 deletions(-)
>
I succesfully tested this on the SM8450 HDK, and the following works:
- USB-C to HDMI, both directions
- USB-C to DP cable to DP display, both directions
- USB-C to USB SuperSpeed, both directions
- USB-C to USB-C, both directions, enables SuperSpeed Plus Gen 2x1
Tested-by: Neil Armstrong <[email protected]> # on HDK8450
Thanks,
Neil
On Tue, May 02, 2023 at 02:22:22PM +0200, Johan Hovold wrote:
> On Mon, Apr 24, 2023 at 08:40:09PM -0700, Bjorn Andersson wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
[..]
> > &mdss0_dp0_out {
> > data-lanes = <0 1>;
> > - remote-endpoint = <&pmic_glink_con0_ss>;
> > + remote-endpoint = <&usb_0_qmpphy_dp_in>;
> > };
>
> It's a bit hard to follow what going on when using place holder nodes
> from the dtsi like this (instead of describing all the ports directly in
> the board dts). IIRC we went a bit back and forth over this earlier and
> we already use this scheme for the display port controllers, so I guess
> this is the price we pay for being consistent.
>
I agree, this is why I argued in favour of keeping the of_graphs
together in a single node. But as long as we label things appropriately
it's pretty ok - and the alternative would be yet another undocumented
"rule".
So let's stick with this...
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
[..]
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + usb_0_qmpphy_out: endpoint {};
> > + };
> > +
> > + port@1 {
> > + reg = <1>;
> > +
> > + usb_0_qmpphy_dp_in: endpoint {};
> > + };
> > + };
> > };
>
> The binding describes three ports, where dp-in is port 2.
>
> Perhaps you don't need to describe ss-in yet, but shouldn't the port
> numbers match? Should some of these be described as required in the
> binding?
>
This should certainly be port@2, thanks for spotting.
Regards,
Bjorn
On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > controller and a USB Type-C connector (or possibly a redriver).
> >
> > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > controller, but no directly link is provided by DeviceTree so the signal
> > needs to "pass through" the QMP combo phy.
> >
> > Handle this by introducing a drm_bridge which upon initialization finds
> > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > way HPD changes in the connector will propagate to the DisplayPort
> > driver.
> >
> > The connector bridge is resolved lazily, as the TCPM is expected to be
> > able to resolve the typec mux and switch at probe time, so the QMP combo
> > phy will probe before the TCPM.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > index 5d6d6ef3944b..84bc08002537 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > @@ -22,6 +22,8 @@
> > #include <linux/usb/typec.h>
> > #include <linux/usb/typec_mux.h>
> >
> > +#include <drm/drm_bridge.h>
> > +
> > #include <dt-bindings/phy/phy-qcom-qmp.h>
> >
> > #include "phy-qcom-qmp.h"
> > @@ -1332,6 +1334,8 @@ struct qmp_combo {
> > struct clk_hw dp_link_hw;
> > struct clk_hw dp_pixel_hw;
> >
> > + struct drm_bridge bridge;
> > +
> > struct typec_switch_dev *sw;
> > enum typec_orientation orientation;
> > };
> > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > }
> >
> > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > + struct drm_bridge *next_bridge;
> > +
> > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > + return -EINVAL;
> > +
> > + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > + if (IS_ERR(next_bridge))
> > + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
>
> Using dev_err_probe() in an attach callback looks wrong as these
> functions should not be returning -EPROBE_DEFER (and this is not a probe
> function).
>
The problem is that this might return EPROBE_DEFER, and at least today
propagates out to returning EPROBE_DEFER from our DP controller's
bind().
This is not optimal, but unfortunately we have a two way dependency
across the of_graph, so we need to make one of the sides lazy...
> > +
> > + return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>
> This line is over 100 chars, but there should be no reason not to break
> it before 80 here.
>
> > +}
> > +
> > +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
> > + .attach = qmp_combo_bridge_attach,
> > +};
> > +
> > +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
> > +{
> > + qmp->bridge.funcs = &qmp_combo_bridge_funcs;
> > + qmp->bridge.of_node = qmp->dev->of_node;
> > +
> > + return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
> > +}
>
> Guess you need a dummy function also for qmp_combo_dp_register_bridge()
> in case of !CONFIG_DRM.
>
Right, missed that dependency. Will wrap this and provide a dummy.
Thanks,
Bjorn
> > +
> > static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
> > {
> > struct device *dev = qmp->dev;
> > @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > + ret = qmp_combo_dp_register_bridge(qmp);
> > + if (ret)
> > + return ret;
> > +
> > /* Check for legacy binding with child nodes. */
> > usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
> > if (usb_np) {
>
> Johan
On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote:
> On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote:
> > In multiple places throughout the driver code has been written in
> > prepration for handling of orientation switching.
> >
> > Introduce a typec_orientation in qmp_combo and fill out the various
> > "placeholders" with the associated logic. By initializing the
> > orientation to "normal" this change has no functional impact, but
> > reduces the size of the upcoming introduction of dynamic orientation
> > switching.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 54 +++++++++++++----------
> > 1 file changed, 30 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > index 7280f7141961..6748f31da7a3 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/regulator/consumer.h>
> > #include <linux/reset.h>
> > #include <linux/slab.h>
> > +#include <linux/usb/typec.h>
> >
> > #include <dt-bindings/phy/phy-qcom-qmp.h>
> >
> > @@ -63,6 +64,10 @@
> > /* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
> > #define CLAMP_EN BIT(0) /* enables i/o clamp_n */
> >
> > +/* QPHY_V3_DP_COM_TYPEC_CTRL register bits */
> > +#define SW_PORTSELECT_VAL BIT(0)
> > +#define SW_PORTSELECT_MUX BIT(1)
> > +
> > #define PHY_INIT_COMPLETE_TIMEOUT 10000
> >
> > struct qmp_phy_init_tbl {
> > @@ -1323,6 +1328,8 @@ struct qmp_combo {
> > struct clk_fixed_rate pipe_clk_fixed;
> > struct clk_hw dp_link_hw;
> > struct clk_hw dp_pixel_hw;
> > +
> > + enum typec_orientation orientation;
> > };
> >
> > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> > {
> > u32 val;
> > - bool reverse = false;
> > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
>
> Adding parentheses around the right-hand side should make this a little
> easier to parse.
>
> It also looks like these callbacks end up being called without holding
> the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a
> concurrent switch notification and dp phy power-on but it's not that
> obvious.
>
It seems we're arriving here from hpd_event_thread(), while
phy_power_on() and phy_power_off() will be called in some other context.
I've not been able to convince myself if DP driver ensures ordering, or
if we have an existing race here...
Unless you insist, I would prefer to follow up with an additional patch
once we've landed this series. The fix will depend on the phy_mutex
shuffling patch anyways...
> > + const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
>
> Also could you add these before u32 val to maintain an approximation of
> reverse xmas style?
>
I'd be happy to do so :)
Regards,
Bjorn
> And similar below.
>
> Johan
On Thu, 4 May 2023 at 11:38, Johan Hovold <[email protected]> wrote:
>
> On Wed, May 03, 2023 at 08:13:54PM -0700, Bjorn Andersson wrote:
> > On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> > > On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > > > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > > > controller and a USB Type-C connector (or possibly a redriver).
> > > >
> > > > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > > > controller, but no directly link is provided by DeviceTree so the signal
> > > > needs to "pass through" the QMP combo phy.
> > > >
> > > > Handle this by introducing a drm_bridge which upon initialization finds
> > > > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > > > way HPD changes in the connector will propagate to the DisplayPort
> > > > driver.
> > > >
> > > > The connector bridge is resolved lazily, as the TCPM is expected to be
> > > > able to resolve the typec mux and switch at probe time, so the QMP combo
> > > > phy will probe before the TCPM.
> > > >
> > > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > > ---
> > > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > > > 1 file changed, 36 insertions(+)
> > > >
> > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > index 5d6d6ef3944b..84bc08002537 100644
> > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>
> > > > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > > > return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > > > }
> > > >
> > > > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > > > + enum drm_bridge_attach_flags flags)
> > > > +{
> > > > + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > > > + struct drm_bridge *next_bridge;
> > > > +
> > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > > + return -EINVAL;
> > > > +
> > > > + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > > > + if (IS_ERR(next_bridge))
> > > > + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> > >
> > > Using dev_err_probe() in an attach callback looks wrong as these
> > > functions should not be returning -EPROBE_DEFER (and this is not a probe
> > > function).
> >
> > The problem is that this might return EPROBE_DEFER, and at least today
> > propagates out to returning EPROBE_DEFER from our DP controller's
> > bind().
>
> Due to the known issue with the MSM driver panel lookup, or due to some
> more fundamental problem with the stack?
Ideally MSM DP driver should call component_add() only when the next
bridge is available. This is how we handle it for the DSI case.
However I'm yet to see the changes to dp_display_probe() which make
actual use of the done_probing callback. And even that will only fix
the eDP case. For the normal DP case we have no way of being properly
notified when the next bridge becomes available. So the driver will
try to drm_bridge_attach() from the component's bind() callback and
return an error if the chain is not (yet) fully available.
>
> At least in the former case, I don't think we should hide the fact that
> we have an unresolved issue with the MSM driver this way even if it
> means printing an extra error message until it has been resolved (cf.
> the panel lookup errors that we've intentionally kept in place).
>
> > This is not optimal, but unfortunately we have a two way dependency
> > across the of_graph, so we need to make one of the sides lazy...
>
> But this comments seems to suggest this is a bigger issue than the panel
> lookup.
>
> Could you describe the issue in some more detail (e.g. when would you
> see -EPROBE_DEFER here)?
>
> Johan
--
With best wishes
Dmitry
On Wed, May 03, 2023 at 08:13:54PM -0700, Bjorn Andersson wrote:
> On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> > On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > > controller and a USB Type-C connector (or possibly a redriver).
> > >
> > > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > > controller, but no directly link is provided by DeviceTree so the signal
> > > needs to "pass through" the QMP combo phy.
> > >
> > > Handle this by introducing a drm_bridge which upon initialization finds
> > > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > > way HPD changes in the connector will propagate to the DisplayPort
> > > driver.
> > >
> > > The connector bridge is resolved lazily, as the TCPM is expected to be
> > > able to resolve the typec mux and switch at probe time, so the QMP combo
> > > phy will probe before the TCPM.
> > >
> > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > ---
> > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > > 1 file changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > index 5d6d6ef3944b..84bc08002537 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > > return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > > }
> > >
> > > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > > + enum drm_bridge_attach_flags flags)
> > > +{
> > > + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > > + struct drm_bridge *next_bridge;
> > > +
> > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > + return -EINVAL;
> > > +
> > > + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > > + if (IS_ERR(next_bridge))
> > > + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> >
> > Using dev_err_probe() in an attach callback looks wrong as these
> > functions should not be returning -EPROBE_DEFER (and this is not a probe
> > function).
>
> The problem is that this might return EPROBE_DEFER, and at least today
> propagates out to returning EPROBE_DEFER from our DP controller's
> bind().
Due to the known issue with the MSM driver panel lookup, or due to some
more fundamental problem with the stack?
At least in the former case, I don't think we should hide the fact that
we have an unresolved issue with the MSM driver this way even if it
means printing an extra error message until it has been resolved (cf.
the panel lookup errors that we've intentionally kept in place).
> This is not optimal, but unfortunately we have a two way dependency
> across the of_graph, so we need to make one of the sides lazy...
But this comments seems to suggest this is a bigger issue than the panel
lookup.
Could you describe the issue in some more detail (e.g. when would you
see -EPROBE_DEFER here)?
Johan
On Wed, May 03, 2023 at 08:29:07PM -0700, Bjorn Andersson wrote:
> On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote:
> > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote:
> > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> > > {
> > > u32 val;
> > > - bool reverse = false;
> > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> > It also looks like these callbacks end up being called without holding
> > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a
> > concurrent switch notification and dp phy power-on but it's not that
> > obvious.
> It seems we're arriving here from hpd_event_thread(), while
> phy_power_on() and phy_power_off() will be called in some other context.
> I've not been able to convince myself if DP driver ensures ordering, or
> if we have an existing race here...
> Unless you insist, I would prefer to follow up with an additional patch
> once we've landed this series. The fix will depend on the phy_mutex
> shuffling patch anyways...
Sure.
But perhaps you can just move the orientation == qmp->orientation check
under the mutex in qmp_combo_typec_switch_set() for now (in case I
forgot to point that out earlier).
Johan
On Thu, May 04, 2023 at 03:44:53PM +0200, Johan Hovold wrote:
> On Wed, May 03, 2023 at 08:29:07PM -0700, Bjorn Andersson wrote:
> > On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote:
> > > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote:
>
> > > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> > > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> > > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> > > > {
> > > > u32 val;
> > > > - bool reverse = false;
> > > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
>
> > > It also looks like these callbacks end up being called without holding
> > > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a
> > > concurrent switch notification and dp phy power-on but it's not that
> > > obvious.
>
> > It seems we're arriving here from hpd_event_thread(), while
> > phy_power_on() and phy_power_off() will be called in some other context.
> > I've not been able to convince myself if DP driver ensures ordering, or
> > if we have an existing race here...
>
> > Unless you insist, I would prefer to follow up with an additional patch
> > once we've landed this series. The fix will depend on the phy_mutex
> > shuffling patch anyways...
>
> Sure.
>
> But perhaps you can just move the orientation == qmp->orientation check
> under the mutex in qmp_combo_typec_switch_set() for now (in case I
> forgot to point that out earlier).
>
qmp_combo_probe() and qmp_combo_typec_switch_set() are the only writers
to qmp->orientation, so that check can't race with any updates and hence
doesn't need to be protected.
Reading the code again, qmp_combo_configure_dp_mode() is invoked from
phy_power_on(), not the hpd_event_thread(), as I claimed yesterday.
But we shouldn't do qmp_combo_dp_power_on() in parallel with the
reinitialization following a switch in orientation, qmp->orientation
might change, but we definitely would have two contexts reconfiguring
the hardware simultaneously - perhaps this was the cause for the 10%
crashes I hit when trying to extend this to handle typec_mux as well...
I will grab the phy_mux in qmp_combo_configure_dp_mode() as well, thanks
for "insisting" :)
Regards,
Bjorn
On Thu, May 04, 2023 at 08:16:33AM -0700, Bjorn Andersson wrote:
> On Thu, May 04, 2023 at 03:44:53PM +0200, Johan Hovold wrote:
> > On Wed, May 03, 2023 at 08:29:07PM -0700, Bjorn Andersson wrote:
> > > On Tue, May 02, 2023 at 01:48:16PM +0200, Johan Hovold wrote:
> > > > On Mon, Apr 24, 2023 at 08:40:06PM -0700, Bjorn Andersson wrote:
> >
> > > > > static void qmp_v3_dp_aux_init(struct qmp_combo *qmp);
> > > > > @@ -1955,29 +1962,23 @@ static void qmp_v3_configure_dp_tx(struct qmp_combo *qmp)
> > > > > static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> > > > > {
> > > > > u32 val;
> > > > > - bool reverse = false;
> > > > > + bool reverse = qmp->orientation == TYPEC_ORIENTATION_REVERSE;
> >
> > > > It also looks like these callbacks end up being called without holding
> > > > the qmp->phy_mutex via phy->power_on(). Perhaps there is no risk for a
> > > > concurrent switch notification and dp phy power-on but it's not that
> > > > obvious.
> >
> > > It seems we're arriving here from hpd_event_thread(), while
> > > phy_power_on() and phy_power_off() will be called in some other context.
> > > I've not been able to convince myself if DP driver ensures ordering, or
> > > if we have an existing race here...
> >
> > > Unless you insist, I would prefer to follow up with an additional patch
> > > once we've landed this series. The fix will depend on the phy_mutex
> > > shuffling patch anyways...
> >
> > Sure.
> >
> > But perhaps you can just move the orientation == qmp->orientation check
> > under the mutex in qmp_combo_typec_switch_set() for now (in case I
> > forgot to point that out earlier).
> >
>
> qmp_combo_probe() and qmp_combo_typec_switch_set() are the only writers
> to qmp->orientation, so that check can't race with any updates and hence
> doesn't need to be protected.
Only if you happen to know that the callers of
qmp_combo_typec_switch_set() are serialised, right? That happens to be
the case for pmic_glink, but it may not be the case generally.
> Reading the code again, qmp_combo_configure_dp_mode() is invoked from
> phy_power_on(), not the hpd_event_thread(), as I claimed yesterday.
Yeah, but phy_power_on() is typically called from that thread. But
perhaps not only from there.
> But we shouldn't do qmp_combo_dp_power_on() in parallel with the
> reinitialization following a switch in orientation, qmp->orientation
> might change, but we definitely would have two contexts reconfiguring
> the hardware simultaneously - perhaps this was the cause for the 10%
> crashes I hit when trying to extend this to handle typec_mux as well...
>
> I will grab the phy_mux in qmp_combo_configure_dp_mode() as well, thanks
> for "insisting" :)
:)
Johan
On Thu, May 04, 2023 at 10:38:54AM +0200, Johan Hovold wrote:
> On Wed, May 03, 2023 at 08:13:54PM -0700, Bjorn Andersson wrote:
> > On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> > > On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > > > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > > > controller and a USB Type-C connector (or possibly a redriver).
> > > >
> > > > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > > > controller, but no directly link is provided by DeviceTree so the signal
> > > > needs to "pass through" the QMP combo phy.
> > > >
> > > > Handle this by introducing a drm_bridge which upon initialization finds
> > > > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > > > way HPD changes in the connector will propagate to the DisplayPort
> > > > driver.
> > > >
> > > > The connector bridge is resolved lazily, as the TCPM is expected to be
> > > > able to resolve the typec mux and switch at probe time, so the QMP combo
> > > > phy will probe before the TCPM.
> > > >
> > > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > > ---
> > > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > > > 1 file changed, 36 insertions(+)
> > > >
> > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > index 5d6d6ef3944b..84bc08002537 100644
> > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>
> > > > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > > > return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > > > }
> > > >
> > > > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > > > + enum drm_bridge_attach_flags flags)
> > > > +{
> > > > + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > > > + struct drm_bridge *next_bridge;
> > > > +
> > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > > + return -EINVAL;
> > > > +
> > > > + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > > > + if (IS_ERR(next_bridge))
> > > > + return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> > >
> > > Using dev_err_probe() in an attach callback looks wrong as these
> > > functions should not be returning -EPROBE_DEFER (and this is not a probe
> > > function).
> >
> > The problem is that this might return EPROBE_DEFER, and at least today
> > propagates out to returning EPROBE_DEFER from our DP controller's
> > bind().
>
> Due to the known issue with the MSM driver panel lookup, or due to some
> more fundamental problem with the stack?
>
No, but looks for the drm_bridge in the connector.
> At least in the former case, I don't think we should hide the fact that
> we have an unresolved issue with the MSM driver this way even if it
> means printing an extra error message until it has been resolved (cf.
> the panel lookup errors that we've intentionally kept in place).
>
> > This is not optimal, but unfortunately we have a two way dependency
> > across the of_graph, so we need to make one of the sides lazy...
>
> But this comments seems to suggest this is a bigger issue than the panel
> lookup.
>
> Could you describe the issue in some more detail (e.g. when would you
> see -EPROBE_DEFER here)?
>
pmic_glink needs to look up the typec_switch_dev through the of_graph,
which won't be present until the QMP phy is probed. And the QMP phy is
looking for the connector, which won't be present until pmic_glink has
found the QMP phy.
So what I'm saying is that either pmic_glink or QMP needs to look up the
other side lazily.
The attach happens during bind of the msm_drm component, so at least
today it's a consistent path to return EPROBE_DEFER in the DP
controller...
Regards,
Bjorn
On Mon, 24 Apr 2023 20:40:03 -0700, Bjorn Andersson wrote:
> This adds support for USB and DisplayPort orientation switching to the
> QMP combo PHY, as well as updating the sc8280xp devices to include the
> QMP in the SuperSpeed graph.
>
> Bjorn Andersson (7):
> dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and
> orientation-switch
> phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
> phy: qcom-qmp-combo: Introduce orientation variable
> phy: qcom-qmp-combo: Introduce orientation switching
> phy: qcom-qmp-combo: Introduce drm_bridge
> arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
> arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
>
> [...]
Applied, thanks!
[1/7] dt-bindings: phy: qcom,sc8280xp-qmp-usb43dp: Add ports and orientation-switch
(no commit info)
[2/7] phy: qcom-qmp-combo: Move phy_mutex out of com_init/exit
(no commit info)
[3/7] phy: qcom-qmp-combo: Introduce orientation variable
(no commit info)
[4/7] phy: qcom-qmp-combo: Introduce orientation switching
(no commit info)
[5/7] phy: qcom-qmp-combo: Introduce drm_bridge
(no commit info)
[6/7] arm64: dts: qcom: sc8280xp-crd: Add QMP to SuperSpeed graph
commit: 507ceaa5ca9fac0d9fe2521c29d7d6237c1214f4
[7/7] arm64: dts: qcom: sc8280xp-x13s: Add QMP to SuperSpeed graph
commit: 42b08375498e74f094425fad10d10c338fd29858
Best regards,
--
Bjorn Andersson <[email protected]>