| Message ID | 20220805101459.3386-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add MT8195 DisplayPort driver | expand |
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Guillaume Ranquet <granquet@baylibre.com> > > This patch adds External DisplayPort support to the mt8195 eDP > driver. > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_dp.c | 190 +++++++++++++++++++++--- > -- > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 4 + > 2 files changed, 158 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index 59fee814075b..00971ea2fadf 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -77,6 +77,7 @@ struct mtk_dp { > struct dp_cal_data cal_data; > u8 max_lanes; > u8 max_linkrate; > + const struct mtk_dp_data *data; > > struct drm_device *drm_dev; > struct drm_bridge bridge; > @@ -96,6 +97,12 @@ struct mtk_dp { > struct drm_connector *conn; > }; > > +struct mtk_dp_data { > + int bridge_type; > + unsigned int smp_cmd; > + unsigned int cali_data_fmt; > +}; > + > static struct regmap_config mtk_dp_regmap_config = { > .reg_bits = 32, > .val_bits = 32, > @@ -347,6 +354,14 @@ static bool mtk_dp_plug_state(struct mtk_dp > *mtk_dp) > return mtk_dp->train_info.cable_plugged_in; > } > > +static bool mtk_dp_plug_state_avoid_pulse(struct mtk_dp *mtk_dp) > +{ > + bool ret; > + > + return !(readx_poll_timeout(mtk_dp_plug_state, mtk_dp, ret, > ret, > + 4000, 7 * 4000)); > +} Separate this to an independent patch which avoid pulse. > + > static void mtk_dp_aux_irq_clear(struct mtk_dp *mtk_dp) > { > mtk_dp_write(mtk_dp, MTK_DP_AUX_P0_3640, > @@ -784,35 +799,73 @@ static void mtk_dp_get_calibration_data(struct > mtk_dp *mtk_dp) > return; > } > > - cal_data->glb_bias_trim = > - check_cal_data_valid(mtk_dp, 1, 0x1e, > - (buf[3] >> 27) & 0x1f, 0xf); > - cal_data->clktx_impse = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[0] >> 9) & 0xf, 0x8); > - cal_data->ln_tx_impsel_pmos[0] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 28) & 0xf, 0x8); > - cal_data->ln_tx_impsel_nmos[0] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 24) & 0xf, 0x8); > - cal_data->ln_tx_impsel_pmos[1] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 20) & 0xf, 0x8); > - cal_data->ln_tx_impsel_nmos[1] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 16) & 0xf, 0x8); > - cal_data->ln_tx_impsel_pmos[2] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 12) & 0xf, 0x8); > - cal_data->ln_tx_impsel_nmos[2] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 8) & 0xf, 0x8); > - cal_data->ln_tx_impsel_pmos[3] = > - check_cal_data_valid(mtk_dp, 1, 0xe, > - (buf[2] >> 4) & 0xf, 0x8); > - cal_data->ln_tx_impsel_nmos[3] = > - check_cal_data_valid(mtk_dp, 1, 0xe, buf[2] & 0xf, > 0x8); > + /* > + * To save the efuse bits, we place the calibration data for DP > and eDP > + * using method which could save the efuse bits. For this, the > efuse > + * orders of DP and eDP are different. > + */ > + > + if (mtk_dp->data->cali_data_fmt == > MTK_DP_CALI_DATA_FMT_MT8195_EDP) { Separate this to an independent patch which support different cali_data_fmt. > + cal_data->glb_bias_trim = > + check_cal_data_valid(mtk_dp, 1, 0x1e, > + (buf[3] >> 27) & 0x1f, > 0xf); > + cal_data->clktx_impse = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[0] >> 9) & 0xf, 0x8); > + cal_data->ln_tx_impsel_pmos[0] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 28) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_nmos[0] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 24) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_pmos[1] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 20) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_nmos[1] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 16) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_pmos[2] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 12) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_nmos[2] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 8) & 0xf, 0x8); > + cal_data->ln_tx_impsel_pmos[3] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[2] >> 4) & 0xf, 0x8); > + cal_data->ln_tx_impsel_nmos[3] = > + check_cal_data_valid(mtk_dp, 1, 0xe, buf[2] & > 0xf, 0x8); > + } else { > + cal_data->glb_bias_trim = > + check_cal_data_valid(mtk_dp, 1, 0x1e, > + (buf[0] >> 27) & 0x1f, > 0xf); > + cal_data->clktx_impse = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[0] >> 13) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_pmos[0] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 28) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_nmos[0] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 24) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_pmos[1] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 20) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_nmos[1] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 16) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_pmos[2] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 12) & 0xf, > 0x8); > + cal_data->ln_tx_impsel_nmos[2] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 8) & 0xf, 0x8); > + cal_data->ln_tx_impsel_pmos[3] = > + check_cal_data_valid(mtk_dp, 1, 0xe, > + (buf[1] >> 4) & 0xf, 0x8); > + cal_data->ln_tx_impsel_nmos[3] = > + check_cal_data_valid(mtk_dp, 1, 0xe, buf[1] & > 0xf, 0x8); > + } > > kfree(buf); > } > @@ -932,7 +985,7 @@ static void mtk_dp_video_mute(struct mtk_dp > *mtk_dp, bool enable) > VIDEO_MUTE_SEL_DP_ENC0_P0_MASK | > VIDEO_MUTE_SW_DP_ENC0_P0_MASK); > > - mtk_dp_sip_atf_call(mtk_dp, MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE, > enable); > + mtk_dp_sip_atf_call(mtk_dp, mtk_dp->data->smp_cmd, enable); Separate this to an independent patch which support different smp_cmd. > } > > static void mtk_dp_power_enable(struct mtk_dp *mtk_dp) > @@ -1232,6 +1285,9 @@ static int mtk_dp_parse_capabilities(struct > mtk_dp *mtk_dp) > drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, > DP_SET_POWER_D0); > usleep_range(2000, 5000); > > + if (!mtk_dp_plug_state(mtk_dp)) > + return -ENODEV; > + > drm_dp_read_dpcd_caps(&mtk_dp->aux, mtk_dp->rx_cap); > > train_info->link_rate = min_t(int, mtk_dp->max_linkrate, > @@ -1283,6 +1339,9 @@ static int mtk_dp_train_start(struct mtk_dp > *mtk_dp) > u8 train_limit; > u8 max_link_rate; > > + if (!mtk_dp_plug_state_avoid_pulse(mtk_dp)) > + return -ENODEV; > + > link_rate = mtk_dp->rx_cap[1]; > lane_count = mtk_dp->rx_cap[2] & 0x1F; > > @@ -1457,9 +1516,20 @@ static irqreturn_t mtk_dp_hpd_event(int hpd, > void *dev) > else > train_info->cable_plugged_in = false; > > - mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > - DP_PWR_STATE_BANDGAP_TPLL_LANE, > - DP_PWR_STATE_MASK); > + if (!train_info->cable_plugged_in) { > + mtk_dp_video_mute(mtk_dp, true); > + > + mtk_dp_initialize_priv_data(mtk_dp); > + mtk_dp_set_idle_pattern(mtk_dp, true); > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL, > + DP_PWR_STATE_MASK); > + } else { > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LANE, > + DP_PWR_STATE_MASK); > + } Separate this to an independent patch. > > return IRQ_HANDLED; > } > @@ -1503,6 +1573,21 @@ static int mtk_dp_dt_parse(struct mtk_dp > *mtk_dp, > return 0; > } > > +static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge > *bridge) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + enum drm_connector_status ret = connector_status_disconnected; > + u8 sink_count = 0; > + > + if (mtk_dp_plug_state_avoid_pulse(mtk_dp)) { > + drm_dp_dpcd_readb(&mtk_dp->aux, DP_SINK_COUNT, > &sink_count); > + if (DP_GET_SINK_COUNT(sink_count)) > + ret = connector_status_connected; > + } > + > + return ret; > +} > + > static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge, > struct drm_connector *connector) > { > @@ -1857,6 +1942,7 @@ static const struct drm_bridge_funcs > mtk_dp_bridge_funcs = { > .atomic_disable = mtk_dp_bridge_atomic_disable, > .mode_valid = mtk_dp_bridge_mode_valid, > .get_edid = mtk_dp_get_edid, > + .detect = mtk_dp_bdg_detect, > }; > > static int mtk_dp_probe(struct platform_device *pdev) > @@ -1871,6 +1957,7 @@ static int mtk_dp_probe(struct platform_device > *pdev) > return -ENOMEM; > > mtk_dp->dev = dev; > + mtk_dp->data = (struct mtk_dp_data > *)of_device_get_match_data(dev); > > irq_num = platform_get_irq(pdev, 0); > if (irq_num < 0) > @@ -1878,9 +1965,15 @@ static int mtk_dp_probe(struct platform_device > *pdev) > "failed to request dp irq > resource\n"); > > mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, > 1, 0); > - if (IS_ERR(mtk_dp->next_bridge)) > + if (IS_ERR(mtk_dp->next_bridge) && > + PTR_ERR(mtk_dp->next_bridge) == -ENODEV) { > + dev_info(dev, > + "No panel connected in devicetree, continue as > external DP\n"); > + mtk_dp->next_bridge = NULL; Separate this to an independent patch. > + } else if (IS_ERR(mtk_dp->next_bridge)) { > return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge), > "Failed to get bridge\n"); > + } > > ret = mtk_dp_dt_parse(mtk_dp, pdev); > if (ret) > @@ -1923,7 +2016,7 @@ static int mtk_dp_probe(struct platform_device > *pdev) > > mtk_dp->bridge.ops = > DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | > DRM_BRIDGE_OP_HPD; > - mtk_dp->bridge.type = DRM_MODE_CONNECTOR_eDP; > + mtk_dp->bridge.type = mtk_dp->data->bridge_type; Separate this to an independent patch which add bridge_type to support multiple bridge type. > > drm_bridge_add(&mtk_dp->bridge); > > @@ -1950,6 +2043,12 @@ static int mtk_dp_suspend(struct device *dev) > { > struct mtk_dp *mtk_dp = dev_get_drvdata(dev); > > + if (mtk_dp_plug_state(mtk_dp)) { > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, > DP_SET_POWER_D3); > + /* Ensure the sink is off before shutting down power */ > + usleep_range(2000, 3000); > + } Separate this to an independent patch which ensure the sink is off before shutting down power. > + > mtk_dp_power_disable(mtk_dp); > > mtk_dp_hwirq_enable(mtk_dp, false); > @@ -1981,8 +2080,27 @@ static int mtk_dp_resume(struct device *dev) > > static SIMPLE_DEV_PM_OPS(mtk_dp_pm_ops, mtk_dp_suspend, > mtk_dp_resume); > > +static const struct mtk_dp_data mt8195_edp_data = { > + .bridge_type = DRM_MODE_CONNECTOR_eDP, > + .smp_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE, > + .cali_data_fmt = MTK_DP_CALI_DATA_FMT_MT8195_EDP, > +}; > + > +static const struct mtk_dp_data mt8195_dp_data = { > + .bridge_type = DRM_MODE_CONNECTOR_DisplayPort, > + .smp_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE, > + .cali_data_fmt = MTK_DP_CALI_DATA_FMT_MT8195_DP, > +}; > + > static const struct of_device_id mtk_dp_of_match[] = { > - { .compatible = "mediatek,mt8195-edp-tx" }, > + { > + .compatible = "mediatek,mt8195-edp-tx", > + .data = &mt8195_edp_data, > + }, > + { > + .compatible = "mediatek,mt8195-dp-tx", > + .data = &mt8195_dp_data, > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, mtk_dp_of_match); > diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h > b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > index 3676d71bd816..c12742adaa3c 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h > +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > @@ -14,6 +14,10 @@ > #define SEC_OFFSET 0x4000 > > #define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE (BIT(0) | BIT(5)) > +#define MTK_DP_SIP_ATF_VIDEO_UNMUTE BIT(5) > + > +#define MTK_DP_CALI_DATA_FMT_MT8195_EDP 0 > +#define MTK_DP_CALI_DATA_FMT_MT8195_DP 1 This is not register definition, so move to mtk_dp.c Regards, CK > > #define DP_PHY_GLB_BIAS_GEN_00 0 > #define RG_XTP_GLB_BIAS_INTR_CTRL GENMASK(20, 16)
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > + > +#define MTK_DP_ENC0_P0_30B8 (ENC0_OFFSET + 0xB8) Useless, so remove it. > + > +#define MTK_DP_ENC0_P0_30BC (ENC0_OFFSET + 0xBC) Ditto. > +#define ISRC_CONT_DP_ENC0_P0_MASK BIT(0) > +#define ISRC_CONT_DP_ENC0_P0_SHIFT 0 > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK GENMASK(10, 8) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT BIT(3) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 \ > + (1 << AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 \ > + (2 << AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 \ > + (3 << AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 \ > + (5 << AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 \ > + (6 << AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT) > +#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 \ > + (7 << AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_SHIFT) > + > +#define MTK_DP_ENC0_P0_30D8 (ENC0_OFFSET + 0xD8) Ditto. > +#define MTK_DP_ENC0_P0_312C (ENC0_OFFSET + 0x12C) Ditto. > +#define ASP_HB2_DP_ENC0_P0_MASK GENMASK(7, 0) > +#define ASP_HB3_DP_ENC0_P0_MASK GENMASK(15, 8) > +#define ASP_HB3_DP_ENC0_P0_SHIFT BIT(3) > + > [snip] > + > +#define MTK_DP_ENC1_P0_3364 (ENC1_OFFSET + > 0x164) > +#define SDP_DOWN_CNT_INIT_IN_HBLANK_DP_ENC1_P0_MASK GENMASK(11, 0) > +#define SDP_DOWN_CNT_INIT_IN_HBLANK_DP_ENC1_P0_SHIFT 0 > +#define FIFO_READ_START_POINT_DP_ENC1_P0_MASK GENMASK > (15, 12) > +#define FIFO_READ_START_POINT_DP_ENC1_P0_SHIFT GENMASK > (3, 2) I would like bit-wise value has one more indent like [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/mediatek/mtk_disp_rdma.c?h=v5.19 Regards, CK > + > +#define MTK_DP_ENC1_P0_3368 (ENC1_OFFSET + > 0x168) > +#define VIDEO_SRAM_FIFO_CNT_RESET_SEL_DP_ENC1_P0_SHIFT 0 > +#define VIDEO_STABLE_CNT_THRD_DP_ENC1_P0_SHIFT BIT(2) > +#define SDP_DP13_EN_DP_ENC1_P0_SHIFT BIT(3) > +#define BS2BS_MODE_DP_ENC1_P0_MASK GENMASK(13, 12) > +#define BS2BS_MODE_DP_ENC1_P0_SHIFT GENMASK(3, 2) > + >
Hi. Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > + > +static int mtk_dp_parse_capabilities(struct mtk_dp *mtk_dp) > +{ > + u8 val; > + ssize_t ret; > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, > DP_SET_POWER_D0); > + usleep_range(2000, 5000); > + > + drm_dp_read_dpcd_caps(&mtk_dp->aux, mtk_dp->rx_cap); > + > + train_info->link_rate = min_t(int, mtk_dp->max_linkrate, > + drm_dp_max_link_rate(mtk_dp- > >rx_cap)); > + train_info->lane_count = min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(mtk_dp- > >rx_cap)); > + > + train_info->tps3 = drm_dp_tps3_supported(mtk_dp->rx_cap); > + train_info->tps4 = drm_dp_tps4_supported(mtk_dp->rx_cap); You could drop tps3, tps4 and add channel_eq_pattern like this: if (drm_dp_tps4_supported(mtk_dp->rx_cap)) train_info->channel_eq_pattern = DP_TRAINING_PATTERN_4; else if (drm_dp_tps4_supported(mtk_dp->rx_cap)) train_info->channel_eq_pattern = DP_TRAINING_PATTERN_3; else train_info->channel_eq_pattern = DP_TRAINING_PATTERN_2; Regards, CK > + > + train_info->sink_ssc = drm_dp_max_downspread(mtk_dp->rx_cap); > + > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, DP_MSTM_CAP, &val); > + if (ret < 1) { > + drm_err(mtk_dp->drm_dev, "Read mstm cap failed\n"); > + return ret == 0 ? -EIO : ret; > + } > + > + if (val & DP_MST_CAP) { > + /* Clear DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0 */ > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, > + DP_DEVICE_SERVICE_IRQ_VECTOR_ES > I0, > + &val); > + if (ret < 1) { > + drm_err(mtk_dp->drm_dev, "Read irq vector > failed\n"); > + return ret == 0 ? -EIO : ret; > + } > + > + if (val) > + drm_dp_dpcd_writeb(&mtk_dp->aux, > + DP_DEVICE_SERVICE_IRQ_VECTOR > _ESI0, > + val); > + } > + > + return 0; > +} > +
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > + > +static int mtk_dp_train_tps_1(struct mtk_dp *mtk_dp, u8 > target_lane_count, > + u8 *lane_adjust, int *status_control, > + u8 *prev_lane_adjust) > +{ > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + > + mtk_dp_training_set_scramble(mtk_dp, false); I think this statement could be moved to mtk_dp_train_flow() before the training loop. After this moving, mtk_dp_train_tps_1() is almost the same as mtk_dp_train_tps_2_3(), so try to merge these two function. > + > + if (*status_control == 0) { > + mtk_dp_pattern(mtk_dp, true, target_lane_count, > lane_adjust); > + *status_control = 1; if calling mtk_dp_pattern() directly from mtk_dp_train_flow(), we could drop status_control. Regards, CK > + } > + > + drm_dp_link_train_clock_recovery_delay(&mtk_dp->aux, mtk_dp- > >rx_cap); > + drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); > + > + if (drm_dp_clock_recovery_ok(link_status, > + target_lane_count)) { > + mtk_dp->train_info.cr_done = true; > + dev_dbg(mtk_dp->dev, "Link train CR pass\n"); > + return 0; > + } else if (*prev_lane_adjust == link_status[4]) { > + if (*prev_lane_adjust & > DP_ADJUST_VOLTAGE_SWING_LANE0_MASK) { > + dev_dbg(mtk_dp->dev, "Link train CQ fail\n"); > + return -EINVAL; > + } > + } else { > + *prev_lane_adjust = link_status[4]; > + } > + return -EAGAIN; > +} > + > +static int mtk_dp_train_tps_2_3(struct mtk_dp *mtk_dp, u8 > target_linkrate, > + u8 target_lane_count, u8 *lane_adjust, > + int *status_control, u8 > *prev_lane_adjust) > +{ > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + > + if (*status_control == 1) { > + mtk_dp_pattern(mtk_dp, false, target_lane_count, > lane_adjust); > + *status_control = 2; > + } > + > + drm_dp_link_train_channel_eq_delay(&mtk_dp->aux, mtk_dp- > >rx_cap); > + > + drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); > + > + if (drm_dp_channel_eq_ok(link_status, target_lane_count)) { > + mtk_dp->train_info.eq_done = true; > + dev_dbg(mtk_dp->dev, "Link train EQ pass\n"); > + return 0; > + } > + > + dev_dbg(mtk_dp->dev, "Link train EQ fail\n"); > + > + if (*prev_lane_adjust != link_status[4]) > + *prev_lane_adjust = link_status[4]; > + > + return -EAGAIN; > +} > + > +static int mtk_dp_train_flow(struct mtk_dp *mtk_dp, u8 > target_link_rate, > + u8 target_lane_count) > +{ > + u8 lane_adjust[2] = {}; > + bool pass_tps1 = false; > + bool pass_tps2_3 = false; > + int train_retries; > + int status_control; > + int ret; > + u8 prev_lane_adjust; > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LINK_BW_SET, > target_link_rate); > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LANE_COUNT_SET, > + target_lane_count | > DP_LANE_COUNT_ENHANCED_FRAME_EN); > + > + if (mtk_dp->train_info.sink_ssc) > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_DOWNSPREAD_CTRL, > + DP_SPREAD_AMP_0_5); > + > + train_retries = 0; > + status_control = 0; > + prev_lane_adjust = 0xFF; > + > + mtk_dp_set_lanes(mtk_dp, target_lane_count / 2); > + ret = mtk_dp_phy_configure(mtk_dp, target_link_rate, > target_lane_count); > + if (ret) > + return ret; > + > + dev_dbg(mtk_dp->dev, > + "Link train target_link_rate = 0x%x, target_lane_count > = 0x%x\n", > + target_link_rate, target_lane_count); > + > + do { > + train_retries++; > + if (!mtk_dp->train_info.cable_plugged_in) > + return -ENODEV; > + > + if (!pass_tps1) { > + ret = mtk_dp_train_tps_1(mtk_dp, > target_lane_count, > + lane_adjust, > &status_control, > + &prev_lane_adjust); > + if (!ret) { > + pass_tps1 = true; > + train_retries = 0; > + } else if (ret == -EINVAL) { > + break; > + } > + } else { > + ret = mtk_dp_train_tps_2_3(mtk_dp, > target_link_rate, > + target_lane_count, > + lane_adjust, > &status_control, > + &prev_lane_adjust); > + if (!ret) { > + pass_tps2_3 = true; > + break; > + } > + } > + > + drm_dp_dpcd_read(&mtk_dp->aux, > DP_ADJUST_REQUEST_LANE0_1, > + lane_adjust, sizeof(lane_adjust)); > + mtk_dp_train_update_swing_pre(mtk_dp, > target_lane_count, > + lane_adjust); > + } while (train_retries < MTK_DP_TRAIN_DOWNSCALE_RETRY); > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_TRAINING_PATTERN_SET, > + DP_TRAINING_PATTERN_DISABLE); > + mtk_dp_train_set_pattern(mtk_dp, 0); > + > + if (!pass_tps2_3) > + return -ETIMEDOUT; > + > + mtk_dp->train_info.link_rate = target_link_rate; > + mtk_dp->train_info.lane_count = target_lane_count; > + > + mtk_dp_training_set_scramble(mtk_dp, true); > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LANE_COUNT_SET, > + target_lane_count | > + DP_LANE_COUNT_ENHANCED_FRAME_EN); > + mtk_dp_set_enhanced_frame_mode(mtk_dp, true); > + > + return ret; > +} > +
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > + > +static void mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > +{ > + ssize_t ret; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + u32 link_status_reg = DP_LANE0_1_STATUS; > + > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > link_status, > + sizeof(link_status)); > + if (!ret) { > + drm_err(mtk_dp->drm_dev, "Read link status failed\n"); > + return; > + } > + > + if (!drm_dp_channel_eq_ok(link_status, mtk_dp- > >train_info.lane_count)) { > + drm_err(mtk_dp->drm_dev, "Channel EQ failed\n"); > + return; > + } > + > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) I does not see any other DP driver process DP_REMOTE_CONTROL_COMMAND_PENDING, why mtk dp driver process it? If this is an advanced function, separate this to an independent patch. Regards, CK > + drm_dp_dpcd_writeb(&mtk_dp->aux, > DP_DEVICE_SERVICE_IRQ_VECTOR, > + DP_REMOTE_CONTROL_COMMAND_PENDING); > +} > + >
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > + > +static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev) > +{ > + struct mtk_dp *mtk_dp = dev; > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + u32 irq_status; > + > + irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS); > + > + if (!(irq_status & RGS_IRQ_STATUS_TRANSMITTER)) > + return IRQ_HANDLED; If one of MTK_DP_HPD_INTERRUPT, MTK_DP_HPD_CONNECT, MTK_DP_HPD_DISCONNECT exist, does it imply RGS_IRQ_STATUS_TRANSMITTER exist? If so, I think this checking is redundant because we could directly check MTK_DP_HPD_INTERRUPT, MTK_DP_HPD_CONNECT, MTK_DP_HPD_DISCONNECT. > + > + irq_status = mtk_dp_swirq_get_clear(mtk_dp) | > + mtk_dp_hwirq_get_clear(mtk_dp); > + > + if (!irq_status) > + return IRQ_HANDLED; > + > + if (irq_status & MTK_DP_HPD_INTERRUPT) > + train_info->hpd_inerrupt = true; train_info->hpd_inerrupt is useless, so drop it. > + > + if (!(irq_status & MTK_DP_HPD_CONNECT || > + irq_status & MTK_DP_HPD_DISCONNECT)) > + return IRQ_WAKE_THREAD; this could be changed to if (irq_status == MTK_DP_HPD_INTERRUPT) return IRQ_WAKE_THREAD; But I find one problem. If irq_status == MTK_DP_HPD_INTERRUPT | MTK_DP_HPD_CONNECT, the thread would not be waked up. Is this what you want? Regards, CK > + > + if (!!(mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3414) & > + HPD_DB_DP_TRANS_P0_MASK)) > + train_info->cable_plugged_in = true; > + else > + train_info->cable_plugged_in = false; > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LANE, > + DP_PWR_STATE_MASK); > + > + return IRQ_HANDLED; > +} > +
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > +#define MTK_DP_ENC0_P0_3038 (ENC0_OFFSET + 0x38) > +#define VIDEO_SOURCE_SEL_DP_ENC0_P0_MASK BIT(11) > +#define VIDEO_SOURCE_SEL_DP_ENC0_P0_SHIFT (BIT(0) | BIT(1) | > BIT(3)) It's not necessary to define a symbol for shift because it's trivial that we understand it's a shift. > + > +#define MTK_DP_ENC0_P0_303C (ENC0_OFFSET + 0x3C) > +#define SRAM_START_READ_THRD_DP_ENC0_P0_MASK GENMASK(5, 0) > +#define SRAM_START_READ_THRD_DP_ENC0_P0_SHIFT 0 Ditto. > +#define VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK GENMASK(10, 8) > +#define VIDEO_COLOR_DEPTH_DP_ENC0_P0_SHIFT BIT(3) Ditto. Regards, CK > + >
Hi, Bo-Chen: On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a embedded displayport driver for the MediaTek mt8195 > SoC. > > It supports the MT8195, the embedded DisplayPort units. It offers > DisplayPort 1.4 with up to 4 lanes. > > The driver creates a child device for the phy. The child device will > never exist without the parent being active. As they are sharing a > register range, the parent passes a regmap pointer to the child so > that > both can work with the same register range. The phy driver sets > device > data that is read by the parent to get the phy device that can be > used > to control the phy properties. > > This driver is based on an initial version by > Jitao shi <jitao.shi@mediatek.com> > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > Tested-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- [snip] > + > +static enum drm_mode_status > +mtk_dp_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + u32 rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000; > + u32 bpp = info->color_formats & DRM_COLOR_FORMAT_YCBCR422 ? 16 > : 24; > + > + if (rx_linkrate * mtk_dp->train_info.lane_count < mode->clock * > bpp / 8) > + return MODE_CLOCK_HIGH; > + > + if (mode->clock > 600000) If the clock has pass the linkrate and land_count limitation, the clock would be OK because the linkrate and lane_count is trained. Why need to check 600000? Regards, CK > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > +
On Mon, 2022-08-08 at 13:46 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a embedded displayport driver for the MediaTek > > mt8195 > > SoC. > > > > It supports the MT8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > This driver is based on an initial version by > > Jitao shi <jitao.shi@mediatek.com> > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > Tested-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > [snip] > > > + > > +static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev) > > +{ > > + struct mtk_dp *mtk_dp = dev; > > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > > + u32 irq_status; > > + > > + irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS); > > + > > + if (!(irq_status & RGS_IRQ_STATUS_TRANSMITTER)) > > + return IRQ_HANDLED; > > If one of MTK_DP_HPD_INTERRUPT, MTK_DP_HPD_CONNECT, > MTK_DP_HPD_DISCONNECT exist, does it imply RGS_IRQ_STATUS_TRANSMITTER > exist? If so, I think this checking is redundant because we could > directly check MTK_DP_HPD_INTERRUPT, MTK_DP_HPD_CONNECT, > MTK_DP_HPD_DISCONNECT. > Hello CK, After checking with Jitao, we can remove this check and use mtk_dp_swirq_get_clear|mtk_dp_hwirq_get_clear directly. > > + > > + irq_status = mtk_dp_swirq_get_clear(mtk_dp) | > > + mtk_dp_hwirq_get_clear(mtk_dp); > > + > > + if (!irq_status) > > + return IRQ_HANDLED; > > + > > + if (irq_status & MTK_DP_HPD_INTERRUPT) > > + train_info->hpd_inerrupt = true; > > train_info->hpd_inerrupt is useless, so drop it. > > > + > > + if (!(irq_status & MTK_DP_HPD_CONNECT || > > + irq_status & MTK_DP_HPD_DISCONNECT)) > > + return IRQ_WAKE_THREAD; > > this could be changed to > > if (irq_status == MTK_DP_HPD_INTERRUPT) > return IRQ_WAKE_THREAD; > > But I find one problem. If irq_status == MTK_DP_HPD_INTERRUPT | > MTK_DP_HPD_CONNECT, the thread would not be waked up. Is this what > you > want? > > Regards, > CK > It is possible we will encounter (irq_status & MTK_DP_HPD_CONNECT) && (irq_status & MTK_DP_HPD_INTERRUPT) So I will modify like this: if (!(irq_status & MTK_DP_HPD_CONNECT || irq_status & MTK_DP_HPD_DISCONNECT)) return IRQ_WAKE_THREAD; xxxxxx if (irq_status & MTK_DP_HPD_INTERRUPT && irq_status & MTK_DP_HPD_CONNECT) return IRQ_WAKE_THREAD; return IRQ_HANDLED; BRs, Bo-Chen > > + > > + if (!!(mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3414) & > > + HPD_DB_DP_TRANS_P0_MASK)) > > + train_info->cable_plugged_in = true; > > + else > > + train_info->cable_plugged_in = false; > > + > > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL_LANE, > > + DP_PWR_STATE_MASK); > > + > > + return IRQ_HANDLED; > > +} > > + > >
On Mon, 2022-08-08 at 16:04 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a embedded displayport driver for the MediaTek > > mt8195 > > SoC. > > > > It supports the MT8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > This driver is based on an initial version by > > Jitao shi <jitao.shi@mediatek.com> > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > Tested-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > [snip] > > > + > > +static enum drm_mode_status > > +mtk_dp_bridge_mode_valid(struct drm_bridge *bridge, > > + const struct drm_display_info *info, > > + const struct drm_display_mode *mode) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + u32 rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000; > > + u32 bpp = info->color_formats & DRM_COLOR_FORMAT_YCBCR422 ? 16 > > : 24; > > + > > + if (rx_linkrate * mtk_dp->train_info.lane_count < mode->clock * > > bpp / 8) > > + return MODE_CLOCK_HIGH; > > + > > + if (mode->clock > 600000) > > If the clock has pass the linkrate and land_count limitation, the > clock > would be OK because the linkrate and lane_count is trained. Why need > to > check 600000? > > Regards, > CK > Hello CK, The condition above is enough to cover this. I will drop this check. BRs, Bo-Chen > > + return MODE_CLOCK_HIGH; > > + > > + return MODE_OK; > > +} > > + > >
On Mon, 2022-08-08 at 13:21 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Fri, 2022-08-05 at 18:14 +0800, Bo-Chen Chen wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a embedded displayport driver for the MediaTek > > mt8195 > > SoC. > > > > It supports the MT8195, the embedded DisplayPort units. It offers > > DisplayPort 1.4 with up to 4 lanes. > > > > The driver creates a child device for the phy. The child device > > will > > never exist without the parent being active. As they are sharing a > > register range, the parent passes a regmap pointer to the child so > > that > > both can work with the same register range. The phy driver sets > > device > > data that is read by the parent to get the phy device that can be > > used > > to control the phy properties. > > > > This driver is based on an initial version by > > Jitao shi <jitao.shi@mediatek.com> > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > Tested-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > [snip] > > > + > > +static void mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > > +{ > > + ssize_t ret; > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > + u32 link_status_reg = DP_LANE0_1_STATUS; > > + > > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > > link_status, > > + sizeof(link_status)); > > + if (!ret) { > > + drm_err(mtk_dp->drm_dev, "Read link status failed\n"); > > + return; > > + } > > + > > + if (!drm_dp_channel_eq_ok(link_status, mtk_dp- > > > train_info.lane_count)) { > > > > + drm_err(mtk_dp->drm_dev, "Channel EQ failed\n"); > > + return; > > + } > > + > > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > > I does not see any other DP driver process > DP_REMOTE_CONTROL_COMMAND_PENDING, why mtk dp driver process it? If > this is an advanced function, separate this to an independent patch. > > Regards, > CK > Hello CK, We are not using this. The dpcd write is just for clearing the irq status of sink device, but we are not doing anything for this hpd event. So I will drop this. BRs, Bo-Chen > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > + DP_REMOTE_CONTROL_COMMAND_PENDING); > > +} > > + > > > >