On Mon, 29 Jan 2024 at 10:06, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > On 24-01-29 06:05:09, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 02:26, Abel Vesa <abel.vesa@xxxxxxxxxx> 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. > > > > > > The existing platforms will remain with separate compatibles for each mode. > > > > > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx> > > > --- > > > drivers/phy/qualcomm/phy-qcom-edp.c | 71 ++++++++++++++++++++++++++----------- > > > 1 file changed, 51 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > > > index 8e5078304646..af941d6c5588 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; > > > + const struct qcom_edp_swing_pre_emph_cfg *swing_pre_emph_cfg; > > > +}; > > > + > > > struct qcom_edp { > > > struct device *dev; > > > - const struct qcom_edp_cfg *cfg; > > > + const struct qcom_edp_phy_cfg *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,28 @@ 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 const struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = { > > > +}; > > > + > > > +static const struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = { > > > + .swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg, > > > +}; > > > + > > > +static const struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = { > > > + .is_edp = true, > > > + .swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg, > > > +}; > > > + > > > 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; > > > int ret; > > > u8 cfg8; > > > > > > @@ -200,7 +214,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 (edp->cfg->swing_pre_emph_cfg && !edp->is_edp) > > > > I think (!edp->is_edp) should be enough here. > > Actually, in case of DP, the cfg8 needs to be 0xb7 for sc8280xp, while for sc7280 it should be 0x37. > > So to differentiate between first and second we check if the config > provides a swing_pre_emph_cfg Using swing_pre_emph_cfg to distinguish between those two cases is a pure hack. Is there any sensible meaning behind those bits? If not, just put those values into the configuration data. > > > > > > cfg8 = 0xb7; > > > else > > > cfg8 = 0x37; > > > @@ -234,7 +248,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->cfg->swing_pre_emph_cfg; > > > unsigned int v_level = 0; > > > unsigned int p_level = 0; > > > u8 ldo_config; > > > @@ -245,6 +259,9 @@ static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configur > > > if (!cfg) > > > return 0; > > > > > > + if (edp->is_edp) > > > + cfg = &edp_phy_swing_pre_emph_cfg; > > > + > > > for (i = 0; i < dp_opts->lanes; i++) { > > > v_level = max(v_level, dp_opts->voltage[i]); > > > p_level = max(p_level, dp_opts->pre[i]); > > > @@ -261,7 +278,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 +464,9 @@ 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; > > > 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 +484,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > > > return timeout; > > > > > > > > > - ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0; > > > + if (edp->cfg->swing_pre_emph_cfg && !edp->cfg->is_edp) > > > > I'd assume this should be `if (!edp->is_edp)`, see qcom_edp_set_voltages() > > > > Yep. will fix. > > > > + ldo_config = 0x1; > > > > > > writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG); > > > writel(ldo_config, edp->tx1 + TXn_LDO_CONFIG); > > > @@ -589,6 +606,18 @@ 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; > > > + > > > + edp->is_edp = submode == PHY_SUBMODE_EDP ? true : false; > > > + > > > + return 0; > > > +} > > > + > > > static int qcom_edp_phy_exit(struct phy *phy) > > > { > > > struct qcom_edp *edp = phy_get_drvdata(phy); > > > @@ -604,6 +633,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, > > > }; > > > @@ -781,6 +811,7 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > > > > > > edp->dev = dev; > > > edp->cfg = of_device_get_match_data(&pdev->dev); > > > + edp->is_edp = edp->cfg->is_edp; > > > > > > edp->edp = devm_platform_ioremap_resource(pdev, 0); > > > if (IS_ERR(edp->edp)) > > > @@ -839,10 +870,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