[PATCH v2 0/2] phy: qcom: edp: Allow eDP/DP configuring via set_mode op

[PATCH v2 0/2] phy: qcom: edp: Allow eDP/DP configuring via set_mode op

[Abel Vesa]

Until now, all platform that supported both eDP and DP had different
compatibles for each mode. Using different compatibles for basically
the same IP block but for a different configuration is bad way all
around. There is a new compute platform from Qualcomm that supports
both eDP and DP with the same PHY. So instead of following the old
method, we should allow the mode to be configured via set_mode from
the controller driver.

The controller part will follow after we conclude the PHY part first.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v2:
- Dropped the dedicated xlate function and added set_mode op instead
- Dropped the eDP PHY type and mode addition
- Added the DP PHY submodes (eDP and DP)
- Removed the device match data storing from the container struct
- Link to v1: https://lore.kernel.org/r/20231219-x1e80100-phy-edp-compatible-refactor-v1-0-f9e77752953d@linaro.org

Initial attepmpt was here:
https://lore.kernel.org/all/20231122-phy-qualcomm-edp-x1e80100-v3-3-576fc4e9559d@linaro.org/
Compared to that version, this one uses the phy-cells method and drops
the X1E80100 support. The X1E80100 support will be a separate patchset.

---
Abel Vesa (2):
      phy: Add Embedded DisplayPort and DisplayPort submodes
      phy: qcom: edp: Add set_mode op for configuring eDP/DP submode

 drivers/phy/qualcomm/phy-qcom-edp.c | 90 ++++++++++++++++++++++++++++---------
 include/linux/phy/phy-dp.h          |  3 ++
 2 files changed, 72 insertions(+), 21 deletions(-)
---
base-commit: 8a9be2a3cb673dba9d22311beb74be261f0b3f15
change-id: 20231219-x1e80100-phy-edp-compatible-refactor-8733eca7ccda

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

[PATCH v2 1/2] phy: Add Embedded DisplayPort and DisplayPort submodes

[Abel Vesa]

In some cases, a DP PHY needs to be configured to work in eDP mode.
So add submodes for both DP and eDP so they can be used by the
controllers for specifying the mode the PHY should be configured in.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 include/linux/phy/phy-dp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy/phy-dp.h b/include/linux/phy/phy-dp.h
index 18cad23642cd..9cce5766bc0b 100644
--- a/include/linux/phy/phy-dp.h
+++ b/include/linux/phy/phy-dp.h
@@ -8,6 +8,9 @@
 
 #include <linux/types.h>
 
+#define PHY_SUBMODE_DP	0
+#define PHY_SUBMODE_EDP	1
+
 /**
  * struct phy_configure_opts_dp - DisplayPort PHY configuration set
  *

-- 
2.34.1

[PATCH v2 2/2] phy: qcom: edp: Add set_mode op for configuring eDP/DP submode

[Abel Vesa]

Future platforms should not use different compatibles to differentiate
between eDP and DP mode. Instead, they should use a single compatible as the
IP block is the same. It will be the job of the controller to set the submode
of the PHY accordingly. Rework the device match config data so that it only
keeps the different knobs rather than swing and pre-emphasis tables.

The existing platforms will remain with separate compatibles for each mode.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-edp.c | 90 ++++++++++++++++++++++++++++---------
 1 file changed, 69 insertions(+), 21 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
index 8e5078304646..efd7015c73ec 100644
--- a/drivers/phy/qualcomm/phy-qcom-edp.c
+++ b/drivers/phy/qualcomm/phy-qcom-edp.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
+#include <linux/phy/phy-dp.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
@@ -68,19 +69,21 @@
 
 #define TXn_TRAN_DRVR_EMP_EN                    0x0078
 
-struct qcom_edp_cfg {
-	bool is_dp;
-
-	/* DP PHY swing and pre_emphasis tables */
+struct qcom_edp_swing_pre_emph_cfg {
 	const u8 (*swing_hbr_rbr)[4][4];
 	const u8 (*swing_hbr3_hbr2)[4][4];
 	const u8 (*pre_emphasis_hbr_rbr)[4][4];
 	const u8 (*pre_emphasis_hbr3_hbr2)[4][4];
 };
 
+struct qcom_edp_phy_cfg {
+	bool is_edp;
+	bool needs_swing_pre_emph_cfg;
+};
+
 struct qcom_edp {
 	struct device *dev;
-	const struct qcom_edp_cfg *cfg;
+	const struct qcom_edp_swing_pre_emph_cfg *swing_pre_emph_cfg;
 
 	struct phy *phy;
 
@@ -96,6 +99,8 @@ struct qcom_edp {
 
 	struct clk_bulk_data clks[2];
 	struct regulator_bulk_data supplies[2];
+
+	bool is_edp;
 };
 
 static const u8 dp_swing_hbr_rbr[4][4] = {
@@ -126,8 +131,7 @@ static const u8 dp_pre_emp_hbr2_hbr3[4][4] = {
 	{ 0x04, 0xff, 0xff, 0xff }
 };
 
-static const struct qcom_edp_cfg dp_phy_cfg = {
-	.is_dp = true,
+static const struct qcom_edp_swing_pre_emph_cfg dp_phy_swing_pre_emph_cfg = {
 	.swing_hbr_rbr = &dp_swing_hbr_rbr,
 	.swing_hbr3_hbr2 = &dp_swing_hbr2_hbr3,
 	.pre_emphasis_hbr_rbr = &dp_pre_emp_hbr_rbr,
@@ -162,18 +166,29 @@ static const u8 edp_pre_emp_hbr2_hbr3[4][4] = {
 	{ 0x00, 0xff, 0xff, 0xff }
 };
 
-static const struct qcom_edp_cfg edp_phy_cfg = {
-	.is_dp = false,
+static const struct qcom_edp_swing_pre_emph_cfg edp_phy_swing_pre_emph_cfg = {
 	.swing_hbr_rbr = &edp_swing_hbr_rbr,
 	.swing_hbr3_hbr2 = &edp_swing_hbr2_hbr3,
 	.pre_emphasis_hbr_rbr = &edp_pre_emp_hbr_rbr,
 	.pre_emphasis_hbr3_hbr2 = &edp_pre_emp_hbr2_hbr3,
 };
 
+static struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = {
+};
+
+static struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = {
+	.needs_swing_pre_emph_cfg = true,
+};
+
+static struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = {
+	.is_edp = true,
+	.needs_swing_pre_emph_cfg = true,
+};
+
 static int qcom_edp_phy_init(struct phy *phy)
 {
 	struct qcom_edp *edp = phy_get_drvdata(phy);
-	const struct qcom_edp_cfg *cfg = edp->cfg;
+	const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
 	int ret;
 	u8 cfg8;
 
@@ -200,7 +215,7 @@ static int qcom_edp_phy_init(struct phy *phy)
 	       DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
 	       edp->edp + DP_PHY_PD_CTL);
 
-	if (cfg && cfg->is_dp)
+	if (cfg && !edp->is_edp)
 		cfg8 = 0xb7;
 	else
 		cfg8 = 0x37;
@@ -234,7 +249,7 @@ static int qcom_edp_phy_init(struct phy *phy)
 
 static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts)
 {
-	const struct qcom_edp_cfg *cfg = edp->cfg;
+	const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
 	unsigned int v_level = 0;
 	unsigned int p_level = 0;
 	u8 ldo_config;
@@ -261,7 +276,7 @@ static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configur
 	if (swing == 0xff || emph == 0xff)
 		return -EINVAL;
 
-	ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
+	ldo_config = edp->is_edp ? 0x0 : 0x1;
 
 	writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
 	writel(swing, edp->tx0 + TXn_TX_DRV_LVL);
@@ -447,10 +462,10 @@ static int qcom_edp_set_vco_div(const struct qcom_edp *edp, unsigned long *pixel
 static int qcom_edp_phy_power_on(struct phy *phy)
 {
 	const struct qcom_edp *edp = phy_get_drvdata(phy);
-	const struct qcom_edp_cfg *cfg = edp->cfg;
+	const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
 	u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
 	unsigned long pixel_freq;
-	u8 ldo_config;
+	u8 ldo_config = 0x0;
 	int timeout;
 	int ret;
 	u32 val;
@@ -468,7 +483,8 @@ static int qcom_edp_phy_power_on(struct phy *phy)
 		return timeout;
 
 
-	ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
+	if (cfg && !edp->is_edp)
+		ldo_config = 0x1;
 
 	writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
 	writel(ldo_config, edp->tx1 + TXn_LDO_CONFIG);
@@ -589,6 +605,31 @@ static int qcom_edp_phy_power_off(struct phy *phy)
 	return 0;
 }
 
+static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct qcom_edp *edp = phy_get_drvdata(phy);
+
+	if (mode != PHY_MODE_DP)
+		return -EINVAL;
+
+	switch (submode) {
+	case PHY_SUBMODE_DP:
+		edp->swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg;
+		edp->is_edp = false;
+		break;
+
+	case PHY_SUBMODE_EDP:
+		edp->swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg;
+		edp->is_edp = true;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int qcom_edp_phy_exit(struct phy *phy)
 {
 	struct qcom_edp *edp = phy_get_drvdata(phy);
@@ -604,6 +645,7 @@ static const struct phy_ops qcom_edp_ops = {
 	.configure	= qcom_edp_phy_configure,
 	.power_on	= qcom_edp_phy_power_on,
 	.power_off	= qcom_edp_phy_power_off,
+	.set_mode	= qcom_edp_phy_set_mode,
 	.exit		= qcom_edp_phy_exit,
 	.owner		= THIS_MODULE,
 };
@@ -770,6 +812,7 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np)
 
 static int qcom_edp_phy_probe(struct platform_device *pdev)
 {
+	const struct qcom_edp_phy_cfg *cfg = of_device_get_match_data(&pdev->dev);
 	struct phy_provider *phy_provider;
 	struct device *dev = &pdev->dev;
 	struct qcom_edp *edp;
@@ -780,7 +823,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	edp->dev = dev;
-	edp->cfg = of_device_get_match_data(&pdev->dev);
+	edp->is_edp = cfg->is_edp;
+
+	if (cfg->needs_swing_pre_emph_cfg)
+		edp->swing_pre_emph_cfg = edp->is_edp ?
+						&edp_phy_swing_pre_emph_cfg :
+						&dp_phy_swing_pre_emph_cfg;
 
 	edp->edp = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(edp->edp))
@@ -839,10 +887,10 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_edp_phy_match_table[] = {
-	{ .compatible = "qcom,sc7280-edp-phy" },
-	{ .compatible = "qcom,sc8180x-edp-phy" },
-	{ .compatible = "qcom,sc8280xp-dp-phy", .data = &dp_phy_cfg },
-	{ .compatible = "qcom,sc8280xp-edp-phy", .data = &edp_phy_cfg },
+	{ .compatible = "qcom,sc7280-edp-phy" , .data = &sc7280_dp_phy_cfg, },
+	{ .compatible = "qcom,sc8180x-edp-phy", .data = &sc7280_dp_phy_cfg, },
+	{ .compatible = "qcom,sc8280xp-dp-phy", .data = &sc8280xp_dp_phy_cfg, },
+	{ .compatible = "qcom,sc8280xp-edp-phy", .data = &sc8280xp_edp_phy_cfg, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table);

-- 
2.34.1

Re: [PATCH v2 2/2] phy: qcom: edp: Add set_mode op for configuring eDP/DP submode

[Dmitry Baryshkov]

On Fri, 22 Dec 2023 at 15:01, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> Future platforms should not use different compatibles to differentiate
> between eDP and DP mode. Instead, they should use a single compatible as the
> IP block is the same. It will be the job of the controller to set the submode
> of the PHY accordingly. Rework the device match config data so that it only
> keeps the different knobs rather than swing and pre-emphasis tables.
>
> The existing platforms will remain with separate compatibles for each mode.
>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-edp.c | 90 ++++++++++++++++++++++++++++---------
>  1 file changed, 69 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> index 8e5078304646..efd7015c73ec 100644
> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
> +#include <linux/phy/phy-dp.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> @@ -68,19 +69,21 @@
>
>  #define TXn_TRAN_DRVR_EMP_EN                    0x0078
>
> -struct qcom_edp_cfg {
> -       bool is_dp;
> -
> -       /* DP PHY swing and pre_emphasis tables */
> +struct qcom_edp_swing_pre_emph_cfg {
>         const u8 (*swing_hbr_rbr)[4][4];
>         const u8 (*swing_hbr3_hbr2)[4][4];
>         const u8 (*pre_emphasis_hbr_rbr)[4][4];
>         const u8 (*pre_emphasis_hbr3_hbr2)[4][4];
>  };
>
> +struct qcom_edp_phy_cfg {
> +       bool is_edp;
> +       bool needs_swing_pre_emph_cfg;

I think something like needs_voltage_config sounds simpler and prettier.

> +};
> +
>  struct qcom_edp {
>         struct device *dev;
> -       const struct qcom_edp_cfg *cfg;
> +       const struct qcom_edp_swing_pre_emph_cfg *swing_pre_emph_cfg;
>
>         struct phy *phy;
>
> @@ -96,6 +99,8 @@ struct qcom_edp {
>
>         struct clk_bulk_data clks[2];
>         struct regulator_bulk_data supplies[2];
> +
> +       bool is_edp;
>  };
>
>  static const u8 dp_swing_hbr_rbr[4][4] = {
> @@ -126,8 +131,7 @@ static const u8 dp_pre_emp_hbr2_hbr3[4][4] = {
>         { 0x04, 0xff, 0xff, 0xff }
>  };
>
> -static const struct qcom_edp_cfg dp_phy_cfg = {
> -       .is_dp = true,
> +static const struct qcom_edp_swing_pre_emph_cfg dp_phy_swing_pre_emph_cfg = {
>         .swing_hbr_rbr = &dp_swing_hbr_rbr,
>         .swing_hbr3_hbr2 = &dp_swing_hbr2_hbr3,
>         .pre_emphasis_hbr_rbr = &dp_pre_emp_hbr_rbr,
> @@ -162,18 +166,29 @@ static const u8 edp_pre_emp_hbr2_hbr3[4][4] = {
>         { 0x00, 0xff, 0xff, 0xff }
>  };
>
> -static const struct qcom_edp_cfg edp_phy_cfg = {
> -       .is_dp = false,
> +static const struct qcom_edp_swing_pre_emph_cfg edp_phy_swing_pre_emph_cfg = {
>         .swing_hbr_rbr = &edp_swing_hbr_rbr,
>         .swing_hbr3_hbr2 = &edp_swing_hbr2_hbr3,
>         .pre_emphasis_hbr_rbr = &edp_pre_emp_hbr_rbr,
>         .pre_emphasis_hbr3_hbr2 = &edp_pre_emp_hbr2_hbr3,
>  };
>
> +static struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = {
> +};
> +
> +static struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = {
> +       .needs_swing_pre_emph_cfg = true,
> +};
> +
> +static struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = {
> +       .is_edp = true,
> +       .needs_swing_pre_emph_cfg = true,
> +};
> +
>  static int qcom_edp_phy_init(struct phy *phy)
>  {
>         struct qcom_edp *edp = phy_get_drvdata(phy);
> -       const struct qcom_edp_cfg *cfg = edp->cfg;
> +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
>         int ret;
>         u8 cfg8;
>
> @@ -200,7 +215,7 @@ static int qcom_edp_phy_init(struct phy *phy)
>                DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
>                edp->edp + DP_PHY_PD_CTL);
>
> -       if (cfg && cfg->is_dp)
> +       if (cfg && !edp->is_edp)
>                 cfg8 = 0xb7;
>         else
>                 cfg8 = 0x37;
> @@ -234,7 +249,7 @@ static int qcom_edp_phy_init(struct phy *phy)
>
>  static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts)
>  {
> -       const struct qcom_edp_cfg *cfg = edp->cfg;
> +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
>         unsigned int v_level = 0;
>         unsigned int p_level = 0;
>         u8 ldo_config;
> @@ -261,7 +276,7 @@ static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configur
>         if (swing == 0xff || emph == 0xff)
>                 return -EINVAL;
>
> -       ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
> +       ldo_config = edp->is_edp ? 0x0 : 0x1;
>
>         writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
>         writel(swing, edp->tx0 + TXn_TX_DRV_LVL);
> @@ -447,10 +462,10 @@ static int qcom_edp_set_vco_div(const struct qcom_edp *edp, unsigned long *pixel
>  static int qcom_edp_phy_power_on(struct phy *phy)
>  {
>         const struct qcom_edp *edp = phy_get_drvdata(phy);
> -       const struct qcom_edp_cfg *cfg = edp->cfg;
> +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
>         u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
>         unsigned long pixel_freq;
> -       u8 ldo_config;
> +       u8 ldo_config = 0x0;
>         int timeout;
>         int ret;
>         u32 val;
> @@ -468,7 +483,8 @@ static int qcom_edp_phy_power_on(struct phy *phy)
>                 return timeout;
>
>
> -       ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
> +       if (cfg && !edp->is_edp)
> +               ldo_config = 0x1;
>
>         writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
>         writel(ldo_config, edp->tx1 + TXn_LDO_CONFIG);
> @@ -589,6 +605,31 @@ static int qcom_edp_phy_power_off(struct phy *phy)
>         return 0;
>  }
>
> +static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +       struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> +       if (mode != PHY_MODE_DP)
> +               return -EINVAL;
> +
> +       switch (submode) {
> +       case PHY_SUBMODE_DP:
> +               edp->swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg;
> +               edp->is_edp = false;
> +               break;
> +
> +       case PHY_SUBMODE_EDP:
> +               edp->swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg;

Won't this override the sc7280 config which doesn't set the
.needs_swing_pre_emph_cfg?
So even

> +               edp->is_edp = true;
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  static int qcom_edp_phy_exit(struct phy *phy)
>  {
>         struct qcom_edp *edp = phy_get_drvdata(phy);
> @@ -604,6 +645,7 @@ static const struct phy_ops qcom_edp_ops = {
>         .configure      = qcom_edp_phy_configure,
>         .power_on       = qcom_edp_phy_power_on,
>         .power_off      = qcom_edp_phy_power_off,
> +       .set_mode       = qcom_edp_phy_set_mode,
>         .exit           = qcom_edp_phy_exit,
>         .owner          = THIS_MODULE,
>  };
> @@ -770,6 +812,7 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np)
>
>  static int qcom_edp_phy_probe(struct platform_device *pdev)
>  {
> +       const struct qcom_edp_phy_cfg *cfg = of_device_get_match_data(&pdev->dev);
>         struct phy_provider *phy_provider;
>         struct device *dev = &pdev->dev;
>         struct qcom_edp *edp;
> @@ -780,7 +823,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         edp->dev = dev;
> -       edp->cfg = of_device_get_match_data(&pdev->dev);
> +       edp->is_edp = cfg->is_edp;
> +
> +       if (cfg->needs_swing_pre_emph_cfg)
> +               edp->swing_pre_emph_cfg = edp->is_edp ?
> +                                               &edp_phy_swing_pre_emph_cfg :
> +                                               &dp_phy_swing_pre_emph_cfg;
>
>         edp->edp = devm_platform_ioremap_resource(pdev, 0);
>         if (IS_ERR(edp->edp))
> @@ -839,10 +887,10 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id qcom_edp_phy_match_table[] = {
> -       { .compatible = "qcom,sc7280-edp-phy" },
> -       { .compatible = "qcom,sc8180x-edp-phy" },
> -       { .compatible = "qcom,sc8280xp-dp-phy", .data = &dp_phy_cfg },
> -       { .compatible = "qcom,sc8280xp-edp-phy", .data = &edp_phy_cfg },
> +       { .compatible = "qcom,sc7280-edp-phy" , .data = &sc7280_dp_phy_cfg, },
> +       { .compatible = "qcom,sc8180x-edp-phy", .data = &sc7280_dp_phy_cfg, },
> +       { .compatible = "qcom,sc8280xp-dp-phy", .data = &sc8280xp_dp_phy_cfg, },
> +       { .compatible = "qcom,sc8280xp-edp-phy", .data = &sc8280xp_edp_phy_cfg, },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table);
>
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] phy: qcom: edp: Add set_mode op for configuring eDP/DP submode

[Abel Vesa]

On 23-12-22 16:45:47, Dmitry Baryshkov wrote:
> On Fri, 22 Dec 2023 at 15:01, Abel Vesa <abel.vesa@linaro.org> wrote:
> >
> > Future platforms should not use different compatibles to differentiate
> > between eDP and DP mode. Instead, they should use a single compatible as the
> > IP block is the same. It will be the job of the controller to set the submode
> > of the PHY accordingly. Rework the device match config data so that it only
> > keeps the different knobs rather than swing and pre-emphasis tables.
> >
> > The existing platforms will remain with separate compatibles for each mode.
> >
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-edp.c | 90 ++++++++++++++++++++++++++++---------
> >  1 file changed, 69 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> > index 8e5078304646..efd7015c73ec 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/phy/phy.h>
> > +#include <linux/phy/phy-dp.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/reset.h>
> > @@ -68,19 +69,21 @@
> >
> >  #define TXn_TRAN_DRVR_EMP_EN                    0x0078
> >
> > -struct qcom_edp_cfg {
> > -       bool is_dp;
> > -
> > -       /* DP PHY swing and pre_emphasis tables */
> > +struct qcom_edp_swing_pre_emph_cfg {
> >         const u8 (*swing_hbr_rbr)[4][4];
> >         const u8 (*swing_hbr3_hbr2)[4][4];
> >         const u8 (*pre_emphasis_hbr_rbr)[4][4];
> >         const u8 (*pre_emphasis_hbr3_hbr2)[4][4];
> >  };
> >
> > +struct qcom_edp_phy_cfg {
> > +       bool is_edp;
> > +       bool needs_swing_pre_emph_cfg;
> 
> I think something like needs_voltage_config sounds simpler and prettier.

Sure. Will do that in the next version.

> 
> > +};
> > +
> >  struct qcom_edp {
> >         struct device *dev;
> > -       const struct qcom_edp_cfg *cfg;
> > +       const struct qcom_edp_swing_pre_emph_cfg *swing_pre_emph_cfg;
> >
> >         struct phy *phy;
> >
> > @@ -96,6 +99,8 @@ struct qcom_edp {
> >
> >         struct clk_bulk_data clks[2];
> >         struct regulator_bulk_data supplies[2];
> > +
> > +       bool is_edp;
> >  };
> >
> >  static const u8 dp_swing_hbr_rbr[4][4] = {
> > @@ -126,8 +131,7 @@ static const u8 dp_pre_emp_hbr2_hbr3[4][4] = {
> >         { 0x04, 0xff, 0xff, 0xff }
> >  };
> >
> > -static const struct qcom_edp_cfg dp_phy_cfg = {
> > -       .is_dp = true,
> > +static const struct qcom_edp_swing_pre_emph_cfg dp_phy_swing_pre_emph_cfg = {
> >         .swing_hbr_rbr = &dp_swing_hbr_rbr,
> >         .swing_hbr3_hbr2 = &dp_swing_hbr2_hbr3,
> >         .pre_emphasis_hbr_rbr = &dp_pre_emp_hbr_rbr,
> > @@ -162,18 +166,29 @@ static const u8 edp_pre_emp_hbr2_hbr3[4][4] = {
> >         { 0x00, 0xff, 0xff, 0xff }
> >  };
> >
> > -static const struct qcom_edp_cfg edp_phy_cfg = {
> > -       .is_dp = false,
> > +static const struct qcom_edp_swing_pre_emph_cfg edp_phy_swing_pre_emph_cfg = {
> >         .swing_hbr_rbr = &edp_swing_hbr_rbr,
> >         .swing_hbr3_hbr2 = &edp_swing_hbr2_hbr3,
> >         .pre_emphasis_hbr_rbr = &edp_pre_emp_hbr_rbr,
> >         .pre_emphasis_hbr3_hbr2 = &edp_pre_emp_hbr2_hbr3,
> >  };
> >
> > +static struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = {
> > +};
> > +
> > +static struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = {
> > +       .needs_swing_pre_emph_cfg = true,
> > +};
> > +
> > +static struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = {
> > +       .is_edp = true,
> > +       .needs_swing_pre_emph_cfg = true,
> > +};
> > +
> >  static int qcom_edp_phy_init(struct phy *phy)
> >  {
> >         struct qcom_edp *edp = phy_get_drvdata(phy);
> > -       const struct qcom_edp_cfg *cfg = edp->cfg;
> > +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
> >         int ret;
> >         u8 cfg8;
> >
> > @@ -200,7 +215,7 @@ static int qcom_edp_phy_init(struct phy *phy)
> >                DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
> >                edp->edp + DP_PHY_PD_CTL);
> >
> > -       if (cfg && cfg->is_dp)
> > +       if (cfg && !edp->is_edp)
> >                 cfg8 = 0xb7;
> >         else
> >                 cfg8 = 0x37;
> > @@ -234,7 +249,7 @@ static int qcom_edp_phy_init(struct phy *phy)
> >
> >  static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts)
> >  {
> > -       const struct qcom_edp_cfg *cfg = edp->cfg;
> > +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
> >         unsigned int v_level = 0;
> >         unsigned int p_level = 0;
> >         u8 ldo_config;
> > @@ -261,7 +276,7 @@ static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configur
> >         if (swing == 0xff || emph == 0xff)
> >                 return -EINVAL;
> >
> > -       ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
> > +       ldo_config = edp->is_edp ? 0x0 : 0x1;
> >
> >         writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
> >         writel(swing, edp->tx0 + TXn_TX_DRV_LVL);
> > @@ -447,10 +462,10 @@ static int qcom_edp_set_vco_div(const struct qcom_edp *edp, unsigned long *pixel
> >  static int qcom_edp_phy_power_on(struct phy *phy)
> >  {
> >         const struct qcom_edp *edp = phy_get_drvdata(phy);
> > -       const struct qcom_edp_cfg *cfg = edp->cfg;
> > +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
> >         u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
> >         unsigned long pixel_freq;
> > -       u8 ldo_config;
> > +       u8 ldo_config = 0x0;
> >         int timeout;
> >         int ret;
> >         u32 val;
> > @@ -468,7 +483,8 @@ static int qcom_edp_phy_power_on(struct phy *phy)
> >                 return timeout;
> >
> >
> > -       ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
> > +       if (cfg && !edp->is_edp)
> > +               ldo_config = 0x1;
> >
> >         writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
> >         writel(ldo_config, edp->tx1 + TXn_LDO_CONFIG);
> > @@ -589,6 +605,31 @@ static int qcom_edp_phy_power_off(struct phy *phy)
> >         return 0;
> >  }
> >
> > +static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > +{
> > +       struct qcom_edp *edp = phy_get_drvdata(phy);
> > +
> > +       if (mode != PHY_MODE_DP)
> > +               return -EINVAL;
> > +
> > +       switch (submode) {
> > +       case PHY_SUBMODE_DP:
> > +               edp->swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg;
> > +               edp->is_edp = false;
> > +               break;
> > +
> > +       case PHY_SUBMODE_EDP:
> > +               edp->swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg;
> 
> Won't this override the sc7280 config which doesn't set the
> .needs_swing_pre_emph_cfg?
> So even

Yeah, the way I thought about this would be that the controller won't
call phy_set_mode_ext if the node doesn't have is-edp property.

But I can see now that is sloppy. Will change this so if the legacy
platforms have the is_edp set in their device match data, it will just
check that the requested mode matches it.

> 
> > +               edp->is_edp = true;
> > +               break;
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int qcom_edp_phy_exit(struct phy *phy)
> >  {
> >         struct qcom_edp *edp = phy_get_drvdata(phy);
> > @@ -604,6 +645,7 @@ static const struct phy_ops qcom_edp_ops = {
> >         .configure      = qcom_edp_phy_configure,
> >         .power_on       = qcom_edp_phy_power_on,
> >         .power_off      = qcom_edp_phy_power_off,
> > +       .set_mode       = qcom_edp_phy_set_mode,
> >         .exit           = qcom_edp_phy_exit,
> >         .owner          = THIS_MODULE,
> >  };
> > @@ -770,6 +812,7 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np)
> >
> >  static int qcom_edp_phy_probe(struct platform_device *pdev)
> >  {
> > +       const struct qcom_edp_phy_cfg *cfg = of_device_get_match_data(&pdev->dev);
> >         struct phy_provider *phy_provider;
> >         struct device *dev = &pdev->dev;
> >         struct qcom_edp *edp;
> > @@ -780,7 +823,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         edp->dev = dev;
> > -       edp->cfg = of_device_get_match_data(&pdev->dev);
> > +       edp->is_edp = cfg->is_edp;
> > +
> > +       if (cfg->needs_swing_pre_emph_cfg)
> > +               edp->swing_pre_emph_cfg = edp->is_edp ?
> > +                                               &edp_phy_swing_pre_emph_cfg :
> > +                                               &dp_phy_swing_pre_emph_cfg;
> >
> >         edp->edp = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(edp->edp))
> > @@ -839,10 +887,10 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
> >  }
> >
> >  static const struct of_device_id qcom_edp_phy_match_table[] = {
> > -       { .compatible = "qcom,sc7280-edp-phy" },
> > -       { .compatible = "qcom,sc8180x-edp-phy" },
> > -       { .compatible = "qcom,sc8280xp-dp-phy", .data = &dp_phy_cfg },
> > -       { .compatible = "qcom,sc8280xp-edp-phy", .data = &edp_phy_cfg },
> > +       { .compatible = "qcom,sc7280-edp-phy" , .data = &sc7280_dp_phy_cfg, },
> > +       { .compatible = "qcom,sc8180x-edp-phy", .data = &sc7280_dp_phy_cfg, },
> > +       { .compatible = "qcom,sc8280xp-dp-phy", .data = &sc8280xp_dp_phy_cfg, },
> > +       { .compatible = "qcom,sc8280xp-edp-phy", .data = &sc8280xp_edp_phy_cfg, },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table);
> >
> > --
> > 2.34.1
> >
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v2 2/2] phy: qcom: edp: Add set_mode op for configuring eDP/DP submode

[Dmitry Baryshkov]

On Fri, 22 Dec 2023 at 20:51, Abel Vesa <abel.vesa@linaro.org> wrote:
>
> On 23-12-22 16:45:47, Dmitry Baryshkov wrote:
> > On Fri, 22 Dec 2023 at 15:01, Abel Vesa <abel.vesa@linaro.org> wrote:
> > >
> > > Future platforms should not use different compatibles to differentiate
> > > between eDP and DP mode. Instead, they should use a single compatible as the
> > > IP block is the same. It will be the job of the controller to set the submode
> > > of the PHY accordingly. Rework the device match config data so that it only
> > > keeps the different knobs rather than swing and pre-emphasis tables.
> > >
> > > The existing platforms will remain with separate compatibles for each mode.
> > >
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-edp.c | 90 ++++++++++++++++++++++++++++---------
> > >  1 file changed, 69 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> > > index 8e5078304646..efd7015c73ec 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/phy/phy.h>
> > > +#include <linux/phy/phy-dp.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/reset.h>
> > > @@ -68,19 +69,21 @@
> > >
> > >  #define TXn_TRAN_DRVR_EMP_EN                    0x0078
> > >
> > > -struct qcom_edp_cfg {
> > > -       bool is_dp;
> > > -
> > > -       /* DP PHY swing and pre_emphasis tables */
> > > +struct qcom_edp_swing_pre_emph_cfg {
> > >         const u8 (*swing_hbr_rbr)[4][4];
> > >         const u8 (*swing_hbr3_hbr2)[4][4];
> > >         const u8 (*pre_emphasis_hbr_rbr)[4][4];
> > >         const u8 (*pre_emphasis_hbr3_hbr2)[4][4];
> > >  };
> > >
> > > +struct qcom_edp_phy_cfg {
> > > +       bool is_edp;
> > > +       bool needs_swing_pre_emph_cfg;
> >
> > I think something like needs_voltage_config sounds simpler and prettier.
>
> Sure. Will do that in the next version.
>
> >
> > > +};
> > > +
> > >  struct qcom_edp {
> > >         struct device *dev;
> > > -       const struct qcom_edp_cfg *cfg;
> > > +       const struct qcom_edp_swing_pre_emph_cfg *swing_pre_emph_cfg;
> > >
> > >         struct phy *phy;
> > >
> > > @@ -96,6 +99,8 @@ struct qcom_edp {
> > >
> > >         struct clk_bulk_data clks[2];
> > >         struct regulator_bulk_data supplies[2];
> > > +
> > > +       bool is_edp;
> > >  };
> > >
> > >  static const u8 dp_swing_hbr_rbr[4][4] = {
> > > @@ -126,8 +131,7 @@ static const u8 dp_pre_emp_hbr2_hbr3[4][4] = {
> > >         { 0x04, 0xff, 0xff, 0xff }
> > >  };
> > >
> > > -static const struct qcom_edp_cfg dp_phy_cfg = {
> > > -       .is_dp = true,
> > > +static const struct qcom_edp_swing_pre_emph_cfg dp_phy_swing_pre_emph_cfg = {
> > >         .swing_hbr_rbr = &dp_swing_hbr_rbr,
> > >         .swing_hbr3_hbr2 = &dp_swing_hbr2_hbr3,
> > >         .pre_emphasis_hbr_rbr = &dp_pre_emp_hbr_rbr,
> > > @@ -162,18 +166,29 @@ static const u8 edp_pre_emp_hbr2_hbr3[4][4] = {
> > >         { 0x00, 0xff, 0xff, 0xff }
> > >  };
> > >
> > > -static const struct qcom_edp_cfg edp_phy_cfg = {
> > > -       .is_dp = false,
> > > +static const struct qcom_edp_swing_pre_emph_cfg edp_phy_swing_pre_emph_cfg = {
> > >         .swing_hbr_rbr = &edp_swing_hbr_rbr,
> > >         .swing_hbr3_hbr2 = &edp_swing_hbr2_hbr3,
> > >         .pre_emphasis_hbr_rbr = &edp_pre_emp_hbr_rbr,
> > >         .pre_emphasis_hbr3_hbr2 = &edp_pre_emp_hbr2_hbr3,
> > >  };
> > >
> > > +static struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = {
> > > +};
> > > +
> > > +static struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = {
> > > +       .needs_swing_pre_emph_cfg = true,
> > > +};
> > > +
> > > +static struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = {
> > > +       .is_edp = true,
> > > +       .needs_swing_pre_emph_cfg = true,
> > > +};
> > > +
> > >  static int qcom_edp_phy_init(struct phy *phy)
> > >  {
> > >         struct qcom_edp *edp = phy_get_drvdata(phy);
> > > -       const struct qcom_edp_cfg *cfg = edp->cfg;
> > > +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
> > >         int ret;
> > >         u8 cfg8;
> > >
> > > @@ -200,7 +215,7 @@ static int qcom_edp_phy_init(struct phy *phy)
> > >                DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
> > >                edp->edp + DP_PHY_PD_CTL);
> > >
> > > -       if (cfg && cfg->is_dp)
> > > +       if (cfg && !edp->is_edp)
> > >                 cfg8 = 0xb7;
> > >         else
> > >                 cfg8 = 0x37;
> > > @@ -234,7 +249,7 @@ static int qcom_edp_phy_init(struct phy *phy)
> > >
> > >  static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts)
> > >  {
> > > -       const struct qcom_edp_cfg *cfg = edp->cfg;
> > > +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
> > >         unsigned int v_level = 0;
> > >         unsigned int p_level = 0;
> > >         u8 ldo_config;
> > > @@ -261,7 +276,7 @@ static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configur
> > >         if (swing == 0xff || emph == 0xff)
> > >                 return -EINVAL;
> > >
> > > -       ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
> > > +       ldo_config = edp->is_edp ? 0x0 : 0x1;
> > >
> > >         writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
> > >         writel(swing, edp->tx0 + TXn_TX_DRV_LVL);
> > > @@ -447,10 +462,10 @@ static int qcom_edp_set_vco_div(const struct qcom_edp *edp, unsigned long *pixel
> > >  static int qcom_edp_phy_power_on(struct phy *phy)
> > >  {
> > >         const struct qcom_edp *edp = phy_get_drvdata(phy);
> > > -       const struct qcom_edp_cfg *cfg = edp->cfg;
> > > +       const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg;
> > >         u32 bias0_en, drvr0_en, bias1_en, drvr1_en;
> > >         unsigned long pixel_freq;
> > > -       u8 ldo_config;
> > > +       u8 ldo_config = 0x0;
> > >         int timeout;
> > >         int ret;
> > >         u32 val;
> > > @@ -468,7 +483,8 @@ static int qcom_edp_phy_power_on(struct phy *phy)
> > >                 return timeout;
> > >
> > >
> > > -       ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0;
> > > +       if (cfg && !edp->is_edp)
> > > +               ldo_config = 0x1;
> > >
> > >         writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG);
> > >         writel(ldo_config, edp->tx1 + TXn_LDO_CONFIG);
> > > @@ -589,6 +605,31 @@ static int qcom_edp_phy_power_off(struct phy *phy)
> > >         return 0;
> > >  }
> > >
> > > +static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > > +{
> > > +       struct qcom_edp *edp = phy_get_drvdata(phy);
> > > +
> > > +       if (mode != PHY_MODE_DP)
> > > +               return -EINVAL;
> > > +
> > > +       switch (submode) {
> > > +       case PHY_SUBMODE_DP:
> > > +               edp->swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg;
> > > +               edp->is_edp = false;
> > > +               break;
> > > +
> > > +       case PHY_SUBMODE_EDP:
> > > +               edp->swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg;
> >
> > Won't this override the sc7280 config which doesn't set the
> > .needs_swing_pre_emph_cfg?
> > So even
>
> Yeah, the way I thought about this would be that the controller won't
> call phy_set_mode_ext if the node doesn't have is-edp property.
>
> But I can see now that is sloppy. Will change this so if the legacy
> platforms have the is_edp set in their device match data, it will just
> check that the requested mode matches it.

Let's separate two issues here. One is the is_edp flag. I think it's
fine to set the default based on the compat string, then allow the
controller to override it.

For the swing / pre_emph tables it might be easier to store pointers
to both DP and eDP tables in the config data (based on the match data)
and later select one basing on the is_edp flag.
This way you don't have to override the pre-set tables during
phy_set_mode_ext().

>
> >
> > > +               edp->is_edp = true;
> > > +               break;
> > > +
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int qcom_edp_phy_exit(struct phy *phy)
> > >  {
> > >         struct qcom_edp *edp = phy_get_drvdata(phy);
> > > @@ -604,6 +645,7 @@ static const struct phy_ops qcom_edp_ops = {
> > >         .configure      = qcom_edp_phy_configure,
> > >         .power_on       = qcom_edp_phy_power_on,
> > >         .power_off      = qcom_edp_phy_power_off,
> > > +       .set_mode       = qcom_edp_phy_set_mode,
> > >         .exit           = qcom_edp_phy_exit,
> > >         .owner          = THIS_MODULE,
> > >  };
> > > @@ -770,6 +812,7 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np)
> > >
> > >  static int qcom_edp_phy_probe(struct platform_device *pdev)
> > >  {
> > > +       const struct qcom_edp_phy_cfg *cfg = of_device_get_match_data(&pdev->dev);
> > >         struct phy_provider *phy_provider;
> > >         struct device *dev = &pdev->dev;
> > >         struct qcom_edp *edp;
> > > @@ -780,7 +823,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
> > >                 return -ENOMEM;
> > >
> > >         edp->dev = dev;
> > > -       edp->cfg = of_device_get_match_data(&pdev->dev);
> > > +       edp->is_edp = cfg->is_edp;
> > > +
> > > +       if (cfg->needs_swing_pre_emph_cfg)
> > > +               edp->swing_pre_emph_cfg = edp->is_edp ?
> > > +                                               &edp_phy_swing_pre_emph_cfg :
> > > +                                               &dp_phy_swing_pre_emph_cfg;
> > >
> > >         edp->edp = devm_platform_ioremap_resource(pdev, 0);
> > >         if (IS_ERR(edp->edp))
> > > @@ -839,10 +887,10 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
> > >  }
> > >
> > >  static const struct of_device_id qcom_edp_phy_match_table[] = {
> > > -       { .compatible = "qcom,sc7280-edp-phy" },
> > > -       { .compatible = "qcom,sc8180x-edp-phy" },
> > > -       { .compatible = "qcom,sc8280xp-dp-phy", .data = &dp_phy_cfg },
> > > -       { .compatible = "qcom,sc8280xp-edp-phy", .data = &edp_phy_cfg },
> > > +       { .compatible = "qcom,sc7280-edp-phy" , .data = &sc7280_dp_phy_cfg, },
> > > +       { .compatible = "qcom,sc8180x-edp-phy", .data = &sc7280_dp_phy_cfg, },
> > > +       { .compatible = "qcom,sc8280xp-dp-phy", .data = &sc8280xp_dp_phy_cfg, },
> > > +       { .compatible = "qcom,sc8280xp-edp-phy", .data = &sc8280xp_edp_phy_cfg, },
> > >         { }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table);
> > >
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

Re: [PATCH v2 0/2] phy: qcom: edp: Allow eDP/DP configuring via set_mode op

[Konrad Dybcio]

On 22.12.2023 14:01, Abel Vesa wrote:
> Until now, all platform that supported both eDP and DP had different
> compatibles for each mode. Using different compatibles for basically
> the same IP block but for a different configuration is bad way all
> around. There is a new compute platform from Qualcomm that supports
> both eDP and DP with the same PHY. So instead of following the old
> method, we should allow the mode to be configured via set_mode from
> the controller driver.
> 
> The controller part will follow after we conclude the PHY part first.
I got a bit lost between all the propositions, how are we going to
discern the controller modes going forward?

Konrad

Re: [PATCH v2 0/2] phy: qcom: edp: Allow eDP/DP configuring via set_mode op

[Abel Vesa]

On 23-12-22 16:02:56, Konrad Dybcio wrote:
> On 22.12.2023 14:01, Abel Vesa wrote:
> > Until now, all platform that supported both eDP and DP had different
> > compatibles for each mode. Using different compatibles for basically
> > the same IP block but for a different configuration is bad way all
> > around. There is a new compute platform from Qualcomm that supports
> > both eDP and DP with the same PHY. So instead of following the old
> > method, we should allow the mode to be configured via set_mode from
> > the controller driver.
> > 
> > The controller part will follow after we conclude the PHY part first.
> I got a bit lost between all the propositions, how are we going to
> discern the controller modes going forward?

Have a look here:
https://lore.kernel.org/all/CAA8EJpr8rKMBzcm-=HGu7-C5hPkNMrnG1cA78O00UjgJVT7p6Q@mail.gmail.com/

I currently implemented this locally, using the is-edp property.

Whatever property we will end up using for specifying the mode in the
controller mode, the set_mode op is still needed in the phy driver.

> 
> Konrad