Add support to configurate link rate, lane count, voltage swing and
pre-emphasis with phy_configure(). It is helpful in application scenarios
where analogix controller is mixed with the phy of other vendors.
Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
---
Changes in v2:
- remove needless assignments for phy_configure()
- remove unnecessary changes for phy_power_on()/phy_power_off()
---
.../drm/bridge/analogix/analogix_dp_core.c | 1 +
.../gpu/drm/bridge/analogix/analogix_dp_reg.c | 56 +++++++++++++++++++
2 files changed, 57 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6f10d88a34c5..9429c50cc1bc 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1696,6 +1696,7 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
if (dp->plat_data->power_on)
dp->plat_data->power_on(dp->plat_data);
+ phy_set_mode(dp->phy, PHY_MODE_DP);
phy_power_on(dp->phy);
analogix_dp_init_dp(dp);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 3afc73c858c4..613ce504bea6 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -11,6 +11,7 @@
#include <linux/gpio/consumer.h>
#include <linux/io.h>
#include <linux/iopoll.h>
+#include <linux/phy/phy.h>
#include <drm/bridge/analogix_dp.h>
@@ -513,10 +514,25 @@ void analogix_dp_enable_sw_function(struct analogix_dp_device *dp)
void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype)
{
u32 reg;
+ int ret;
reg = bwtype;
if ((bwtype == DP_LINK_BW_2_7) || (bwtype == DP_LINK_BW_1_62))
writel(reg, dp->reg_base + ANALOGIX_DP_LINK_BW_SET);
+
+ if (dp->phy) {
+ union phy_configure_opts phy_cfg = {0};
+
+ phy_cfg.dp.lanes = dp->link_train.lane_count;
+ phy_cfg.dp.link_rate =
+ drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
+ phy_cfg.dp.set_rate = true;
+ ret = phy_configure(dp->phy, &phy_cfg);
+ if (ret && ret != -EOPNOTSUPP) {
+ dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
+ return;
+ }
+ }
}
void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
@@ -530,9 +546,22 @@ void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
void analogix_dp_set_lane_count(struct analogix_dp_device *dp, u32 count)
{
u32 reg;
+ int ret;
reg = count;
writel(reg, dp->reg_base + ANALOGIX_DP_LANE_COUNT_SET);
+
+ if (dp->phy) {
+ union phy_configure_opts phy_cfg = {0};
+
+ phy_cfg.dp.lanes = dp->link_train.lane_count;
+ phy_cfg.dp.set_lanes = true;
+ ret = phy_configure(dp->phy, &phy_cfg);
+ if (ret && ret != -EOPNOTSUPP) {
+ dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
+ return;
+ }
+ }
}
void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
@@ -546,10 +575,37 @@ void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
void analogix_dp_set_lane_link_training(struct analogix_dp_device *dp)
{
u8 lane;
+ int ret;
for (lane = 0; lane < dp->link_train.lane_count; lane++)
writel(dp->link_train.training_lane[lane],
dp->reg_base + ANALOGIX_DP_LN0_LINK_TRAINING_CTL + 4 * lane);
+
+ if (dp->phy) {
+ union phy_configure_opts phy_cfg = {0};
+
+ for (lane = 0; lane < dp->link_train.lane_count; lane++) {
+ u8 training_lane = dp->link_train.training_lane[lane];
+ u8 vs, pe;
+
+ vs = (training_lane & DP_TRAIN_VOLTAGE_SWING_MASK) >>
+ DP_TRAIN_VOLTAGE_SWING_SHIFT;
+ pe = (training_lane & DP_TRAIN_PRE_EMPHASIS_MASK) >>
+ DP_TRAIN_PRE_EMPHASIS_SHIFT;
+ phy_cfg.dp.voltage[lane] = vs;
+ phy_cfg.dp.pre[lane] = pe;
+ }
+
+ phy_cfg.dp.lanes = dp->link_train.lane_count;
+ phy_cfg.dp.link_rate =
+ drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
+ phy_cfg.dp.set_voltages = true;
+ ret = phy_configure(dp->phy, &phy_cfg);
+ if (ret && ret != -EOPNOTSUPP) {
+ dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
+ return;
+ }
+ }
}
u32 analogix_dp_get_lane_link_training(struct analogix_dp_device *dp, u8 lane)
--
2.34.1On Thu, Dec 19, 2024 at 04:05:56PM +0800, Damon Ding wrote:
> Add support to configurate link rate, lane count, voltage swing and
> pre-emphasis with phy_configure(). It is helpful in application scenarios
> where analogix controller is mixed with the phy of other vendors.
>
> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>
> ---
>
> Changes in v2:
> - remove needless assignments for phy_configure()
> - remove unnecessary changes for phy_power_on()/phy_power_off()
> ---
> .../drm/bridge/analogix/analogix_dp_core.c | 1 +
> .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 56 +++++++++++++++++++
> 2 files changed, 57 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6f10d88a34c5..9429c50cc1bc 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1696,6 +1696,7 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
> if (dp->plat_data->power_on)
> dp->plat_data->power_on(dp->plat_data);
>
> + phy_set_mode(dp->phy, PHY_MODE_DP);
> phy_power_on(dp->phy);
>
> analogix_dp_init_dp(dp);
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 3afc73c858c4..613ce504bea6 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -11,6 +11,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/io.h>
> #include <linux/iopoll.h>
> +#include <linux/phy/phy.h>
>
> #include <drm/bridge/analogix_dp.h>
>
> @@ -513,10 +514,25 @@ void analogix_dp_enable_sw_function(struct analogix_dp_device *dp)
> void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype)
> {
> u32 reg;
> + int ret;
>
> reg = bwtype;
> if ((bwtype == DP_LINK_BW_2_7) || (bwtype == DP_LINK_BW_1_62))
> writel(reg, dp->reg_base + ANALOGIX_DP_LINK_BW_SET);
> +
> + if (dp->phy) {
> + union phy_configure_opts phy_cfg = {0};
> +
> + phy_cfg.dp.lanes = dp->link_train.lane_count;
Should not be necessary, you are only setting the .set_rate.
> + phy_cfg.dp.link_rate =
> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
> + phy_cfg.dp.set_rate = true;
> + ret = phy_configure(dp->phy, &phy_cfg);
> + if (ret && ret != -EOPNOTSUPP) {
> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> + return;
> + }
> + }
> }
>
> void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
> @@ -530,9 +546,22 @@ void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
> void analogix_dp_set_lane_count(struct analogix_dp_device *dp, u32 count)
> {
> u32 reg;
> + int ret;
>
> reg = count;
> writel(reg, dp->reg_base + ANALOGIX_DP_LANE_COUNT_SET);
> +
> + if (dp->phy) {
> + union phy_configure_opts phy_cfg = {0};
> +
> + phy_cfg.dp.lanes = dp->link_train.lane_count;
> + phy_cfg.dp.set_lanes = true;
> + ret = phy_configure(dp->phy, &phy_cfg);
> + if (ret && ret != -EOPNOTSUPP) {
> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> + return;
> + }
> + }
> }
>
> void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
> @@ -546,10 +575,37 @@ void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
> void analogix_dp_set_lane_link_training(struct analogix_dp_device *dp)
> {
> u8 lane;
> + int ret;
>
> for (lane = 0; lane < dp->link_train.lane_count; lane++)
> writel(dp->link_train.training_lane[lane],
> dp->reg_base + ANALOGIX_DP_LN0_LINK_TRAINING_CTL + 4 * lane);
> +
> + if (dp->phy) {
> + union phy_configure_opts phy_cfg = {0};
> +
> + for (lane = 0; lane < dp->link_train.lane_count; lane++) {
> + u8 training_lane = dp->link_train.training_lane[lane];
> + u8 vs, pe;
> +
> + vs = (training_lane & DP_TRAIN_VOLTAGE_SWING_MASK) >>
> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
> + pe = (training_lane & DP_TRAIN_PRE_EMPHASIS_MASK) >>
> + DP_TRAIN_PRE_EMPHASIS_SHIFT;
> + phy_cfg.dp.voltage[lane] = vs;
> + phy_cfg.dp.pre[lane] = pe;
> + }
> +
> + phy_cfg.dp.lanes = dp->link_train.lane_count;
> + phy_cfg.dp.link_rate =
> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
This two should not be necessary, please drop them.
> + phy_cfg.dp.set_voltages = true;
> + ret = phy_configure(dp->phy, &phy_cfg);
> + if (ret && ret != -EOPNOTSUPP) {
> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> + return;
> + }
> + }
> }
>
> u32 analogix_dp_get_lane_link_training(struct analogix_dp_device *dp, u8 lane)
> --
> 2.34.1
>
--
With best wishes
Dmitry
Hi Dmitry,
On 2024/12/20 8:13, Dmitry Baryshkov wrote:
> On Thu, Dec 19, 2024 at 04:05:56PM +0800, Damon Ding wrote:
>> Add support to configurate link rate, lane count, voltage swing and
>> pre-emphasis with phy_configure(). It is helpful in application scenarios
>> where analogix controller is mixed with the phy of other vendors.
>>
>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - remove needless assignments for phy_configure()
>> - remove unnecessary changes for phy_power_on()/phy_power_off()
>> ---
>> .../drm/bridge/analogix/analogix_dp_core.c | 1 +
>> .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 56 +++++++++++++++++++
>> 2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 6f10d88a34c5..9429c50cc1bc 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1696,6 +1696,7 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
>> if (dp->plat_data->power_on)
>> dp->plat_data->power_on(dp->plat_data);
>>
>> + phy_set_mode(dp->phy, PHY_MODE_DP);
>> phy_power_on(dp->phy);
>>
>> analogix_dp_init_dp(dp);
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> index 3afc73c858c4..613ce504bea6 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> @@ -11,6 +11,7 @@
>> #include <linux/gpio/consumer.h>
>> #include <linux/io.h>
>> #include <linux/iopoll.h>
>> +#include <linux/phy/phy.h>
>>
>> #include <drm/bridge/analogix_dp.h>
>>
>> @@ -513,10 +514,25 @@ void analogix_dp_enable_sw_function(struct analogix_dp_device *dp)
>> void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype)
>> {
>> u32 reg;
>> + int ret;
>>
>> reg = bwtype;
>> if ((bwtype == DP_LINK_BW_2_7) || (bwtype == DP_LINK_BW_1_62))
>> writel(reg, dp->reg_base + ANALOGIX_DP_LINK_BW_SET);
>> +
>> + if (dp->phy) {
>> + union phy_configure_opts phy_cfg = {0};
>> +
>> + phy_cfg.dp.lanes = dp->link_train.lane_count;
>
> Should not be necessary, you are only setting the .set_rate.
Indeed, this can be dropped.
>
>> + phy_cfg.dp.link_rate =
>> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
>> + phy_cfg.dp.set_rate = true;
>> + ret = phy_configure(dp->phy, &phy_cfg);
>> + if (ret && ret != -EOPNOTSUPP) {
>> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
>> + return;
>> + }
>> + }
>> }
>>
>> void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
>> @@ -530,9 +546,22 @@ void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
>> void analogix_dp_set_lane_count(struct analogix_dp_device *dp, u32 count)
>> {
>> u32 reg;
>> + int ret;
>>
>> reg = count;
>> writel(reg, dp->reg_base + ANALOGIX_DP_LANE_COUNT_SET);
>> +
>> + if (dp->phy) {
>> + union phy_configure_opts phy_cfg = {0};
>> +
>> + phy_cfg.dp.lanes = dp->link_train.lane_count;
>> + phy_cfg.dp.set_lanes = true;
>> + ret = phy_configure(dp->phy, &phy_cfg);
>> + if (ret && ret != -EOPNOTSUPP) {
>> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
>> + return;
>> + }
>> + }
>> }
>>
>> void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
>> @@ -546,10 +575,37 @@ void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
>> void analogix_dp_set_lane_link_training(struct analogix_dp_device *dp)
>> {
>> u8 lane;
>> + int ret;
>>
>> for (lane = 0; lane < dp->link_train.lane_count; lane++)
>> writel(dp->link_train.training_lane[lane],
>> dp->reg_base + ANALOGIX_DP_LN0_LINK_TRAINING_CTL + 4 * lane);
>> +
>> + if (dp->phy) {
>> + union phy_configure_opts phy_cfg = {0};
>> +
>> + for (lane = 0; lane < dp->link_train.lane_count; lane++) {
>> + u8 training_lane = dp->link_train.training_lane[lane];
>> + u8 vs, pe;
>> +
>> + vs = (training_lane & DP_TRAIN_VOLTAGE_SWING_MASK) >>
>> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
>> + pe = (training_lane & DP_TRAIN_PRE_EMPHASIS_MASK) >>
>> + DP_TRAIN_PRE_EMPHASIS_SHIFT;
>> + phy_cfg.dp.voltage[lane] = vs;
>> + phy_cfg.dp.pre[lane] = pe;
>> + }
>> +
>> + phy_cfg.dp.lanes = dp->link_train.lane_count;
>> + phy_cfg.dp.link_rate =
>> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
>
> This two should not be necessary, please drop them.
These two are necessary for rk_hdptx_phy_set_voltage(), so they cannot
be dropped.
>
>> + phy_cfg.dp.set_voltages = true;
>> + ret = phy_configure(dp->phy, &phy_cfg);
>> + if (ret && ret != -EOPNOTSUPP) {
>> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
>> + return;
>> + }
>> + }
>> }
>>
>> u32 analogix_dp_get_lane_link_training(struct analogix_dp_device *dp, u8 lane)
>> --
>> 2.34.1
>>
>
Best regards,
Damon
On Fri, 20 Dec 2024 at 05:37, Damon Ding <damon.ding@rock-chips.com> wrote:
>
> Hi Dmitry,
>
> On 2024/12/20 8:13, Dmitry Baryshkov wrote:
> > On Thu, Dec 19, 2024 at 04:05:56PM +0800, Damon Ding wrote:
> >> Add support to configurate link rate, lane count, voltage swing and
> >> pre-emphasis with phy_configure(). It is helpful in application scenarios
> >> where analogix controller is mixed with the phy of other vendors.
> >>
> >> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> >>
> >> ---
> >>
> >> Changes in v2:
> >> - remove needless assignments for phy_configure()
> >> - remove unnecessary changes for phy_power_on()/phy_power_off()
> >> ---
> >> .../drm/bridge/analogix/analogix_dp_core.c | 1 +
> >> .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 56 +++++++++++++++++++
> >> 2 files changed, 57 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >> index 6f10d88a34c5..9429c50cc1bc 100644
> >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> >> @@ -1696,6 +1696,7 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
> >> if (dp->plat_data->power_on)
> >> dp->plat_data->power_on(dp->plat_data);
> >>
> >> + phy_set_mode(dp->phy, PHY_MODE_DP);
> >> phy_power_on(dp->phy);
> >>
> >> analogix_dp_init_dp(dp);
> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> >> index 3afc73c858c4..613ce504bea6 100644
> >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> >> @@ -11,6 +11,7 @@
> >> #include <linux/gpio/consumer.h>
> >> #include <linux/io.h>
> >> #include <linux/iopoll.h>
> >> +#include <linux/phy/phy.h>
> >>
> >> #include <drm/bridge/analogix_dp.h>
> >>
> >> @@ -513,10 +514,25 @@ void analogix_dp_enable_sw_function(struct analogix_dp_device *dp)
> >> void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype)
> >> {
> >> u32 reg;
> >> + int ret;
> >>
> >> reg = bwtype;
> >> if ((bwtype == DP_LINK_BW_2_7) || (bwtype == DP_LINK_BW_1_62))
> >> writel(reg, dp->reg_base + ANALOGIX_DP_LINK_BW_SET);
> >> +
> >> + if (dp->phy) {
> >> + union phy_configure_opts phy_cfg = {0};
> >> +
> >> + phy_cfg.dp.lanes = dp->link_train.lane_count;
> >
> > Should not be necessary, you are only setting the .set_rate.
>
> Indeed, this can be dropped.
>
> >
> >> + phy_cfg.dp.link_rate =
> >> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
> >> + phy_cfg.dp.set_rate = true;
> >> + ret = phy_configure(dp->phy, &phy_cfg);
> >> + if (ret && ret != -EOPNOTSUPP) {
> >> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> >> + return;
> >> + }
> >> + }
> >> }
> >>
> >> void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
> >> @@ -530,9 +546,22 @@ void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
> >> void analogix_dp_set_lane_count(struct analogix_dp_device *dp, u32 count)
> >> {
> >> u32 reg;
> >> + int ret;
> >>
> >> reg = count;
> >> writel(reg, dp->reg_base + ANALOGIX_DP_LANE_COUNT_SET);
> >> +
> >> + if (dp->phy) {
> >> + union phy_configure_opts phy_cfg = {0};
> >> +
> >> + phy_cfg.dp.lanes = dp->link_train.lane_count;
> >> + phy_cfg.dp.set_lanes = true;
> >> + ret = phy_configure(dp->phy, &phy_cfg);
> >> + if (ret && ret != -EOPNOTSUPP) {
> >> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> >> + return;
> >> + }
> >> + }
> >> }
> >>
> >> void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
> >> @@ -546,10 +575,37 @@ void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
> >> void analogix_dp_set_lane_link_training(struct analogix_dp_device *dp)
> >> {
> >> u8 lane;
> >> + int ret;
> >>
> >> for (lane = 0; lane < dp->link_train.lane_count; lane++)
> >> writel(dp->link_train.training_lane[lane],
> >> dp->reg_base + ANALOGIX_DP_LN0_LINK_TRAINING_CTL + 4 * lane);
> >> +
> >> + if (dp->phy) {
> >> + union phy_configure_opts phy_cfg = {0};
> >> +
> >> + for (lane = 0; lane < dp->link_train.lane_count; lane++) {
> >> + u8 training_lane = dp->link_train.training_lane[lane];
> >> + u8 vs, pe;
> >> +
> >> + vs = (training_lane & DP_TRAIN_VOLTAGE_SWING_MASK) >>
> >> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
> >> + pe = (training_lane & DP_TRAIN_PRE_EMPHASIS_MASK) >>
> >> + DP_TRAIN_PRE_EMPHASIS_SHIFT;
> >> + phy_cfg.dp.voltage[lane] = vs;
> >> + phy_cfg.dp.pre[lane] = pe;
> >> + }
> >> +
> >> + phy_cfg.dp.lanes = dp->link_train.lane_count;
> >> + phy_cfg.dp.link_rate =
> >> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
> >
> > This two should not be necessary, please drop them.
>
> These two are necessary for rk_hdptx_phy_set_voltage(), so they cannot
> be dropped.
Please review the documentation for struct phy_configure_opts_dp and
fix your PHY driver to skip the values for which the .set_foo isn't
set. Then you might have to change this part.
You are setting just .set_voltages. It means that the rate and .lanes
shouldn't be changed and can be used as they were set by the previous
calls to phy_configure().
>
> >
> >> + phy_cfg.dp.set_voltages = true;
> >> + ret = phy_configure(dp->phy, &phy_cfg);
> >> + if (ret && ret != -EOPNOTSUPP) {
> >> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> >> + return;
> >> + }
> >> + }
> >> }
> >>
> >> u32 analogix_dp_get_lane_link_training(struct analogix_dp_device *dp, u8 lane)
> >> --
> >> 2.34.1
> >>
> >
>
> Best regards,
> Damon
>
--
With best wishes
Dmitry
Hi Dmitry,
On 2024/12/20 13:37, Dmitry Baryshkov wrote:
> On Fri, 20 Dec 2024 at 05:37, Damon Ding <damon.ding@rock-chips.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 2024/12/20 8:13, Dmitry Baryshkov wrote:
>>> On Thu, Dec 19, 2024 at 04:05:56PM +0800, Damon Ding wrote:
>>>> Add support to configurate link rate, lane count, voltage swing and
>>>> pre-emphasis with phy_configure(). It is helpful in application scenarios
>>>> where analogix controller is mixed with the phy of other vendors.
>>>>
>>>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - remove needless assignments for phy_configure()
>>>> - remove unnecessary changes for phy_power_on()/phy_power_off()
>>>> ---
>>>> .../drm/bridge/analogix/analogix_dp_core.c | 1 +
>>>> .../gpu/drm/bridge/analogix/analogix_dp_reg.c | 56 +++++++++++++++++++
>>>> 2 files changed, 57 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> index 6f10d88a34c5..9429c50cc1bc 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> @@ -1696,6 +1696,7 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
>>>> if (dp->plat_data->power_on)
>>>> dp->plat_data->power_on(dp->plat_data);
>>>>
>>>> + phy_set_mode(dp->phy, PHY_MODE_DP);
>>>> phy_power_on(dp->phy);
>>>>
>>>> analogix_dp_init_dp(dp);
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> index 3afc73c858c4..613ce504bea6 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> @@ -11,6 +11,7 @@
>>>> #include <linux/gpio/consumer.h>
>>>> #include <linux/io.h>
>>>> #include <linux/iopoll.h>
>>>> +#include <linux/phy/phy.h>
>>>>
>>>> #include <drm/bridge/analogix_dp.h>
>>>>
>>>> @@ -513,10 +514,25 @@ void analogix_dp_enable_sw_function(struct analogix_dp_device *dp)
>>>> void analogix_dp_set_link_bandwidth(struct analogix_dp_device *dp, u32 bwtype)
>>>> {
>>>> u32 reg;
>>>> + int ret;
>>>>
>>>> reg = bwtype;
>>>> if ((bwtype == DP_LINK_BW_2_7) || (bwtype == DP_LINK_BW_1_62))
>>>> writel(reg, dp->reg_base + ANALOGIX_DP_LINK_BW_SET);
>>>> +
>>>> + if (dp->phy) {
>>>> + union phy_configure_opts phy_cfg = {0};
>>>> +
>>>> + phy_cfg.dp.lanes = dp->link_train.lane_count;
>>>
>>> Should not be necessary, you are only setting the .set_rate.
>>
>> Indeed, this can be dropped.
>>
>>>
>>>> + phy_cfg.dp.link_rate =
>>>> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
>>>> + phy_cfg.dp.set_rate = true;
>>>> + ret = phy_configure(dp->phy, &phy_cfg);
>>>> + if (ret && ret != -EOPNOTSUPP) {
>>>> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
>>>> + return;
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
>>>> @@ -530,9 +546,22 @@ void analogix_dp_get_link_bandwidth(struct analogix_dp_device *dp, u32 *bwtype)
>>>> void analogix_dp_set_lane_count(struct analogix_dp_device *dp, u32 count)
>>>> {
>>>> u32 reg;
>>>> + int ret;
>>>>
>>>> reg = count;
>>>> writel(reg, dp->reg_base + ANALOGIX_DP_LANE_COUNT_SET);
>>>> +
>>>> + if (dp->phy) {
>>>> + union phy_configure_opts phy_cfg = {0};
>>>> +
>>>> + phy_cfg.dp.lanes = dp->link_train.lane_count;
>>>> + phy_cfg.dp.set_lanes = true;
>>>> + ret = phy_configure(dp->phy, &phy_cfg);
>>>> + if (ret && ret != -EOPNOTSUPP) {
>>>> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
>>>> + return;
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
>>>> @@ -546,10 +575,37 @@ void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
>>>> void analogix_dp_set_lane_link_training(struct analogix_dp_device *dp)
>>>> {
>>>> u8 lane;
>>>> + int ret;
>>>>
>>>> for (lane = 0; lane < dp->link_train.lane_count; lane++)
>>>> writel(dp->link_train.training_lane[lane],
>>>> dp->reg_base + ANALOGIX_DP_LN0_LINK_TRAINING_CTL + 4 * lane);
>>>> +
>>>> + if (dp->phy) {
>>>> + union phy_configure_opts phy_cfg = {0};
>>>> +
>>>> + for (lane = 0; lane < dp->link_train.lane_count; lane++) {
>>>> + u8 training_lane = dp->link_train.training_lane[lane];
>>>> + u8 vs, pe;
>>>> +
>>>> + vs = (training_lane & DP_TRAIN_VOLTAGE_SWING_MASK) >>
>>>> + DP_TRAIN_VOLTAGE_SWING_SHIFT;
>>>> + pe = (training_lane & DP_TRAIN_PRE_EMPHASIS_MASK) >>
>>>> + DP_TRAIN_PRE_EMPHASIS_SHIFT;
>>>> + phy_cfg.dp.voltage[lane] = vs;
>>>> + phy_cfg.dp.pre[lane] = pe;
>>>> + }
>>>> +
>>>> + phy_cfg.dp.lanes = dp->link_train.lane_count;
>>>> + phy_cfg.dp.link_rate =
>>>> + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
>>>
>>> This two should not be necessary, please drop them.
>>
>> These two are necessary for rk_hdptx_phy_set_voltage(), so they cannot
>> be dropped.
>
> Please review the documentation for struct phy_configure_opts_dp and
> fix your PHY driver to skip the values for which the .set_foo isn't
> set. Then you might have to change this part.
> You are setting just .set_voltages. It means that the rate and .lanes
> shouldn't be changed and can be used as they were set by the previous
> calls to phy_configure().
>
Indeed, I will store the previous &phy_configure_opts.dp.link_rate and
&phy_configure_opts.dp.lanes in the struct rk_hdptx_phy, and will not
use both of them during the configuration process of the
&phy_cfg.dp.voltage[] and &phy_cfg.dp.pre[] in next version.
>>
>>>
>>>> + phy_cfg.dp.set_voltages = true;
>>>> + ret = phy_configure(dp->phy, &phy_cfg);
>>>> + if (ret && ret != -EOPNOTSUPP) {
>>>> + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
>>>> + return;
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> u32 analogix_dp_get_lane_link_training(struct analogix_dp_device *dp, u8 lane)
>>>> --
>>>> 2.34.1
>>>>
>>>
Best regards,
Damon
On Fri Dec 20, 2024 at 1:13 AM CET, Dmitry Baryshkov wrote:
> On Thu, Dec 19, 2024 at 04:05:56PM +0800, Damon Ding wrote:
> > void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
> > @@ -546,10 +575,37 @@ void analogix_dp_get_lane_count(struct analogix_dp_device *dp, u32 *count)
> > void analogix_dp_set_lane_link_training(struct analogix_dp_device *dp)
> > {
> > u8 lane;
> > + int ret;
> >
> > for (lane = 0; lane < dp->link_train.lane_count; lane++)
> > writel(dp->link_train.training_lane[lane],
> > dp->reg_base + ANALOGIX_DP_LN0_LINK_TRAINING_CTL + 4 * lane);
> > +
> > + if (dp->phy) {
> > + union phy_configure_opts phy_cfg = {0};
> > +
> > + for (lane = 0; lane < dp->link_train.lane_count; lane++) {
> > + u8 training_lane = dp->link_train.training_lane[lane];
> > + u8 vs, pe;
> > +
> > + vs = (training_lane & DP_TRAIN_VOLTAGE_SWING_MASK) >>
> > + DP_TRAIN_VOLTAGE_SWING_SHIFT;
> > + pe = (training_lane & DP_TRAIN_PRE_EMPHASIS_MASK) >>
> > + DP_TRAIN_PRE_EMPHASIS_SHIFT;
> > + phy_cfg.dp.voltage[lane] = vs;
> > + phy_cfg.dp.pre[lane] = pe;
> > + }
> > +
> > + phy_cfg.dp.lanes = dp->link_train.lane_count;
> > + phy_cfg.dp.link_rate =
> > + drm_dp_bw_code_to_link_rate(dp->link_train.link_rate) / 100;
>
> This two should not be necessary, please drop them.
This too or These two (statements)?
> > + phy_cfg.dp.set_voltages = true;
> > + ret = phy_configure(dp->phy, &phy_cfg);
> > + if (ret && ret != -EOPNOTSUPP) {
> > + dev_err(dp->dev, "%s: phy_configure() failed: %d\n", __func__, ret);
> > + return;
> > + }
> > + }
> > }
> >
> > u32 analogix_dp_get_lane_link_training(struct analogix_dp_device *dp, u8 lane)
> > --
> > 2.34.1
> >
© 2016 - 2025 Red Hat, Inc.