| Message ID | 20220627080341.5087-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add MT8195 DisplayPort driver | expand |
Il 27/06/22 10:03, Bo-Chen Chen ha scritto: > 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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/Kconfig | 10 + > drivers/gpu/drm/mediatek/Makefile | 1 + > drivers/gpu/drm/mediatek/mtk_dp.c | 2198 ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 543 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 3 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + > 6 files changed, 2758 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > new file mode 100644 > index 000000000000..9e9b516409e2 > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -0,0 +1,2198 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019-2022 MediaTek Inc. > + * Copyright (c) 2022 BayLibre > + */ > + > +#include <drm/display/drm_dp.h> > +#include <drm/display/drm_dp_helper.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_edid.h> > +#include <drm/drm_of.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_print.h> > +#include <drm/drm_probe_helper.h> > +#include <linux/arm-smccc.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/kernel.h> > +#include <linux/nvmem-consumer.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <sound/hdmi-codec.h> > +#include <video/videomode.h> > + > +#include "mtk_dp_reg.h" > + > +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 Why have you forced this SIP call to AArch32 SMC convention? Is there any particular reason why this should always be AA32 and *never* AA64? In any case, you've got MediaTek SIP macros in include/soc/mediatek/mtk_sip_svc.h so, please, include that header and either redefine this as MTK_SIP_CMD(0x523) or add a new macro in there to force ARM_SMCCC_SMC_32 convention with a very explanatory comment saying why some calls need to be forced to use the AArch32 SMC convention. > + > +#define MTK_VDOSYS1_MAX_FRAMERATE 60 > +#define MTK_DP_4P1T 4 > +#define MTK_DP_HDE 2 > +#define MTK_DP_PIX_PER_ADDR 2 > +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 > +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3 > +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 > +#define MTK_DP_TRAIN_RETRY_LIMIT 8 > +#define MTK_DP_TRAIN_MAX_ITERATIONS 5 > +#define MTK_DP_DP_VERSION_11 0x11 MTK_DP_HW_VERSION_11 0x11 ? ...but anyway, this definition is unused, so please either use it or drop it. > + > +enum mtk_dp_train_state { > + MTK_DP_TRAIN_STATE_TRAINING, > + MTK_DP_TRAIN_STATE_NORMAL, > +}; > + > +struct mtk_dp_timings { > + struct videomode vm; > + u8 frame_rate; > +}; > + > +struct mtk_dp_irq_sta { > + bool hpd_disconnect; > + bool hpd_inerrupt; > +}; > + > +struct mtk_dp_train_info { > + bool tps3; > + bool tps4; > + bool sink_ssc; > + bool cable_plugged_in; > + bool cable_state_change; > + bool cr_done; > + bool eq_done; > + /* link_rate is in multiple of 0.27Gbps */ > + int link_rate; > + int lane_count; > + struct mtk_dp_irq_sta irq_sta; > +}; > + > +struct mtk_dp_info { > + u32 depth; > + enum dp_pixelformat format; > + struct mtk_dp_timings timings; > +}; > + > +struct dp_cal_data { > + unsigned int glb_bias_trim; > + unsigned int clktx_impse; > + > + unsigned int ln_tx_impsel_pmos[4]; > + unsigned int ln_tx_impsel_nmos[4]; > +}; > + > +struct mtk_dp { > + struct device *dev; > + struct platform_device *phy_dev; > + struct phy *phy; > + struct dp_cal_data cal_data; > + u8 max_lanes; > + u8 max_linkrate; > + > + struct drm_device *drm_dev; > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct drm_dp_aux aux; > + > + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; > + > + struct mtk_dp_info info; > + > + struct mtk_dp_train_info train_info; > + enum mtk_dp_train_state train_state; > + > + struct regmap *regs; > + > + bool enabled; > + > + struct drm_connector *conn; > +}; > + > +static struct regmap_config mtk_dp_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = SEC_OFFSET + 0x90, > + .name = "mtk-dp-registers", > +}; > + > +static struct mtk_dp *mtk_dp_from_bridge(struct drm_bridge *b) > +{ > + return container_of(b, struct mtk_dp, bridge); > +} > + > +static u32 mtk_dp_read(struct mtk_dp *mtk_dp, u32 offset) > +{ > + u32 read_val; > + int ret; > + > + ret = regmap_read(mtk_dp->regs, offset, &read_val); > + if (ret) { > + dev_err(mtk_dp->dev, "Failed to read register 0x%x: %d\n", > + offset, ret); > + return 0; > + } > + > + return read_val; > +} > + > +static void mtk_dp_write(struct mtk_dp *mtk_dp, u32 offset, u32 val) > +{ This should be int... you should propagate the error to the caller, and also eventually take action in case you get an error. > + if (regmap_write(mtk_dp->regs, offset, val)) > + dev_err(mtk_dp->dev, > + "Failed to write register 0x%x with value 0x%x\n", > + offset, val); > +} > + > +static void mtk_dp_update_bits(struct mtk_dp *mtk_dp, u32 offset, > + u32 val, u32 mask) > +{ Same here. > + if (regmap_update_bits(mtk_dp->regs, offset, mask, val)) > + dev_err(mtk_dp->dev, > + "Failed to update register 0x%x with value 0x%x, mask 0x%x\n", > + offset, val, mask); > +} > + > +static void mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 offset, u8 *buf, > + size_t length) > +{ > + int i; > + int num_regs = (length + 1) / 2; > + ... and here. > + /* 2 bytes per register */ > + for (i = 0; i < num_regs; i++) { > + u32 val = buf[i * 2] | > + (i * 2 + 1 < length ? buf[i * 2 + 1] << 8 : 0); > + > + mtk_dp_write(mtk_dp, offset + i * 4, val); P.S.: Does it make sense to keep writing if you get an error? I'd say that doing this may lead to unexpected hardware status. > + } > +} > + > +static unsigned long mtk_dp_sip_atf_call(struct mtk_dp *mtk_dp, > + unsigned int cmd, unsigned int para) > +{ > + struct arm_smccc_res res; > + > + arm_smccc_smc(MTK_DP_SIP_CONTROL_AARCH32, cmd, para, 0, 0, 0, 0, 0, > + &res); > + > + dev_dbg(mtk_dp->dev, "sip cmd 0x%x, p1 0x%x, ret 0x%lx-0x%lx", > + cmd, para, res.a0, res.a1); > + > + return res.a1; We have SIP_SVC_E_(xxxxx) error codes defined in mtk_sip_svc.h... this makes me think that res.a1 is not an unsigned long for real: please confirm. > +} > + ..snip.. > + > +static void mtk_dp_set_color_format(struct mtk_dp *mtk_dp, > + enum dp_pixelformat color_format) > +{ > + u32 val; > + > + mtk_dp->info.format = color_format; > + > + /* update MISC0 */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + color_format << DP_TEST_COLOR_FORMAT_SHIFT, > + DP_TEST_COLOR_FORMAT_MASK); > + > + switch (color_format) { > + case DP_PIXELFORMAT_YUV422: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422; > + break; > + case DP_PIXELFORMAT_YUV420: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420; > + break; > + case DP_PIXELFORMAT_RGB: > + case DP_PIXELFORMAT_YUV444: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; > + break; > + case DP_PIXELFORMAT_Y_ONLY: > + case DP_PIXELFORMAT_RAW: > + case DP_PIXELFORMAT_RESERVED: > + default: > + drm_warn(mtk_dp->drm_dev, "Unsupported color format: %d\n", > + color_format); > + return; return -EINVAL here? > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > + val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); ... and return 0 here. > +} > + > +static void mtk_dp_set_color_depth(struct mtk_dp *mtk_dp) > +{ > + u32 val; > + /* Only support 8 bits currently */ > + u32 color_depth = DP_MSA_MISC_8_BPC; > + > + mtk_dp->info.depth = color_depth; > + > + /* Update MISC0 */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + color_depth, DP_TEST_BIT_DEPTH_MASK); > + > + switch (color_depth) { > + case DP_MSA_MISC_6_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_6BIT; > + break; > + case DP_MSA_MISC_8_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT; > + break; > + case DP_MSA_MISC_10_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_10BIT; > + break; > + case DP_MSA_MISC_12_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_12BIT; > + break; > + case DP_MSA_MISC_16_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_16BIT; > + break; ditto > + default: > + drm_warn(mtk_dp->drm_dev, "Unsupported color depth %d\n", > + color_depth); > + return; > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); > +} > + ..snip.. > + > +static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp, > + u32 link_rate, int lane_count) > +{ > + int ret; > + union phy_configure_opts phy_opts = { > + .dp = { > + .link_rate = link_rate_to_mb_per_s(mtk_dp, link_rate), > + .set_rate = 1, > + .lanes = lane_count, > + .set_lanes = 1, > + .ssc = mtk_dp->train_info.sink_ssc, > + } > + }; > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, DP_PWR_STATE_BANDGAP, > + DP_PWR_STATE_MASK); > + > + ret = phy_configure(mtk_dp->phy, &phy_opts); > + This new blank line is unnecessary, please remove. > + if (ret) > + return ret; > + > + mtk_dp_set_cal_data(mtk_dp); > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LANE, DP_PWR_STATE_MASK); > + > + return 0; > +} > + ..snip.. > + > +static void mtk_dp_calculate_pixrate(struct mtk_dp *mtk_dp) > +{ > + u8 target_frame_rate = 60; Don't assign any value here: this will make sure to avoid double assignments later. > + u32 target_pixel_clk; > + struct drm_display_mode mode; > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > + > + drm_display_mode_from_videomode(&timings->vm, &mode); > + > + if (mtk_dp->info.timings.frame_rate > 0) { > + target_frame_rate = mtk_dp->info.timings.frame_rate; > + target_pixel_clk = mode.htotal * mode.vtotal * > + target_frame_rate; > + } else { > + target_pixel_clk = mode.htotal * mode.vtotal * > + target_frame_rate; > + } This should be if (mtk_dp->info.timings.frame_rate > 0) target_frame_rate = mtk_dp->info.timings.frame_rate; else target_frame_rate = 60; target_pixel_clk = mode.htotal * mode.vtotal * target_frame_rate; > +} > + > +static void mtk_dp_set_tx_out(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_msa_bypass_disable(mtk_dp); > + mtk_dp_calculate_pixrate(mtk_dp); > + mtk_dp_pg_disable(mtk_dp); > + mtk_dp_setup_tu(mtk_dp); > +} > + > +static ssize_t mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > +{ > + ssize_t ret; > + u8 sink_count; > + bool locked; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > + u32 link_status_reg = DP_LANE0_1_STATUS; > + > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, &sink_count); > + if (ret < 0) { This function can never return anything > 1, so this should probably be: if (ret < 1) { drm_err .... return ret == 0 ? -EIO : ret; } > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > + return ret; > + } > + > + 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 ret; > + } > + > + locked = drm_dp_channel_eq_ok(link_status, > + mtk_dp->train_info.lane_count); > + if (!locked && mtk_dp->train_state > MTK_DP_TRAIN_STATE_TRAINING) > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; > + > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR, > + DP_REMOTE_CONTROL_COMMAND_PENDING); > + > + if (DP_GET_SINK_COUNT(sink_count) && > + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; > + msleep(20); > + } > + > + return 0; > +} > + ..snip.. > + > +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 iteration_count; > + 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; > + iteration_count = 1; > + 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 -EINVAL; Why are you overriding the error value here? > + > + dev_dbg(mtk_dp->dev, > + "Link train target_link_rate = 0x%x, target_lane_count = 0x%x\n", > + target_link_rate, target_lane_count); > + Cheers, Angelo
Il 27/06/22 10:03, Bo-Chen Chen ha scritto: > 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> > [Bo-Chen: Move some dp features here and modify for reviewers' comments.] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dp.c | 217 ++++++++++++++++++++++++------ > 1 file changed, 174 insertions(+), 43 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > index 9e9b516409e2..1697c61462b7 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -111,6 +111,7 @@ struct mtk_dp { > struct regmap *regs; > > bool enabled; > + bool has_fec; > > struct drm_connector *conn; > }; > @@ -123,6 +124,11 @@ static struct regmap_config mtk_dp_regmap_config = { > .name = "mtk-dp-registers", > }; > > +static bool mtk_dp_is_edp(struct mtk_dp *mtk_dp) > +{ > + return mtk_dp->next_bridge; > +} > + > static struct mtk_dp *mtk_dp_from_bridge(struct drm_bridge *b) > { > return container_of(b, struct mtk_dp, bridge); > @@ -401,6 +407,20 @@ static bool mtk_dp_plug_state(struct mtk_dp *mtk_dp) > HPD_DB_DP_TRANS_P0_MASK); > } > > +static bool mtk_dp_plug_state_avoid_pulse(struct mtk_dp *mtk_dp) > +{ > + int wait; > + > + for (wait = 7; !mtk_dp_plug_state(mtk_dp) && wait > 0; --wait) > + /* Avoid short pulses on the HPD isr */ > + usleep_range(1000, 5000); > + > + if (wait == 0) > + return false; > + You can as well use existing APIs for that: static bool mtk_dp_plug_state_avoid_pulse(struct mtk_dp *mtk_dp) { int ret; return !!(readx_poll_timeout(mtk_dp_plug_state, mtk_dp, ret, ret, 4000, (7 * 4000))); } P.S.: I've used 4000 instead of 5000 on purpose, check the definition of readx_poll_timeout for more information. All the rest of this commit looks good to me. Cheers, Angelo
Il 27/06/22 10:03, Bo-Chen Chen ha scritto: > From: Jitao Shi <jitao.shi@mediatek.com> > > From the DP spec 1.4a chapter 3.3, upstream devices should implement > HPD signal de-bouncing on an external connection. > A period of 100ms should be used to detect an HPD connect event. > To cover these cases, HPD de-bounce should be implemented only after > HPD low has been detected for at least 100ms. > > Therefore, > 1. If HPD is low (which means plugging out) for longer than 100ms: > we need to do de-bouncing (which means we need to wait for 100ms). > 2. If HPD low is for less than 100ms: > we don't need to care about the de-bouncing. > > In this patch, we start a 100ms timer and use a need_debounce boolean > to implement the feature. > > Two cases when HPD is high: > 1. If the timer is expired (>100ms): > - need_debounce is true. > - When HPD high (plugging event comes), need_debounce will be true > and then we need to do de-bouncing (wait for 100ms). > 2. If the timer is not expired (<100ms): > - need_debounce is false. > - When HPD high (plugging event comes), need_debounce will be false > and no need to do de-bouncing. > > HPD_______ __________________ > | |<- 100ms -> > |____________| > <- 100ms -> > > Without HPD de-bouncing, USB-C to HDMI Adapaters will not be detected. > > The change has been successfully tested with the following devices: > - Dell Adapter - USB-C to HDMI > - Acer 1in1 HDMI dongle > - Ugreen 1in1 HDMI dongle > - innowatt HDMI + USB3 hub > - Acer 2in1 HDMI dongle > - Apple 3in1 HDMI dongle (A2119) > - J5Create 3in1 HDMI dongle (JAC379) > > Tested-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > 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>
Il 27/06/22 10:03, Bo-Chen Chen ha scritto: > Set the monitor power state to DP_SET_POWER_D3 to avoid garbage. > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Mon, 2022-06-27 at 18:07 +0800, AngeloGioacchino Del Regno wrote: > Il 27/06/22 10:03, Bo-Chen Chen ha scritto: > > 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> > > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/Kconfig | 10 + > > drivers/gpu/drm/mediatek/Makefile | 1 + > > drivers/gpu/drm/mediatek/mtk_dp.c | 2198 > > ++++++++++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 543 ++++++ > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 3 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + > > 6 files changed, 2758 insertions(+) > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > > b/drivers/gpu/drm/mediatek/mtk_dp.c > > new file mode 100644 > > index 000000000000..9e9b516409e2 > > --- /dev/null > > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > > @@ -0,0 +1,2198 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2019-2022 MediaTek Inc. > > + * Copyright (c) 2022 BayLibre > > + */ > > + > > +#include <drm/display/drm_dp.h> > > +#include <drm/display/drm_dp_helper.h> > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_crtc.h> > > +#include <drm/drm_edid.h> > > +#include <drm/drm_of.h> > > +#include <drm/drm_panel.h> > > +#include <drm/drm_print.h> > > +#include <drm/drm_probe_helper.h> > > +#include <linux/arm-smccc.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/errno.h> > > +#include <linux/kernel.h> > > +#include <linux/nvmem-consumer.h> > > +#include <linux/of.h> > > +#include <linux/of_irq.h> > > +#include <linux/of_platform.h> > > +#include <linux/phy/phy.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <sound/hdmi-codec.h> > > +#include <video/videomode.h> > > + > > +#include "mtk_dp_reg.h" > > + > > +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 > > Why have you forced this SIP call to AArch32 SMC convention? > Is there any particular reason why this should always be AA32 and > *never* AA64? > > In any case, you've got MediaTek SIP macros in > include/soc/mediatek/mtk_sip_svc.h > so, please, include that header and either redefine this as > > MTK_SIP_CMD(0x523) or add a new macro in there to force > ARM_SMCCC_SMC_32 > convention with a very explanatory comment saying why some calls need > to > be forced to use the AArch32 SMC convention. > Hello Angelo, Thanks for your review. ok, I will do this in next version. > > + > > +#define MTK_VDOSYS1_MAX_FRAMERATE 60 > > +#define MTK_DP_4P1T 4 > > +#define MTK_DP_HDE 2 > > +#define MTK_DP_PIX_PER_ADDR 2 > > +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 > > +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3 > > +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 > > +#define MTK_DP_TRAIN_RETRY_LIMIT 8 > > +#define MTK_DP_TRAIN_MAX_ITERATIONS 5 > > +#define MTK_DP_DP_VERSION_11 0x11 > > MTK_DP_HW_VERSION_11 0x11 ? > > ...but anyway, this definition is unused, so please either use it or > drop it. > it's fro audio patch, and I will move this there. > > + > > +enum mtk_dp_train_state { > > + MTK_DP_TRAIN_STATE_TRAINING, > > + MTK_DP_TRAIN_STATE_NORMAL, > > +}; > > + > > +struct mtk_dp_timings { > > + struct videomode vm; > > + u8 frame_rate; > > +}; > > + > > +struct mtk_dp_irq_sta { > > + bool hpd_disconnect; > > + bool hpd_inerrupt; > > +}; > > + > > +struct mtk_dp_train_info { > > + bool tps3; > > + bool tps4; > > + bool sink_ssc; > > + bool cable_plugged_in; > > + bool cable_state_change; > > + bool cr_done; > > + bool eq_done; > > + /* link_rate is in multiple of 0.27Gbps */ > > + int link_rate; > > + int lane_count; > > + struct mtk_dp_irq_sta irq_sta; > > +}; > > + > > +struct mtk_dp_info { > > + u32 depth; > > + enum dp_pixelformat format; > > + struct mtk_dp_timings timings; > > +}; > > + > > +struct dp_cal_data { > > + unsigned int glb_bias_trim; > > + unsigned int clktx_impse; > > + > > + unsigned int ln_tx_impsel_pmos[4]; > > + unsigned int ln_tx_impsel_nmos[4]; > > +}; > > + > > +struct mtk_dp { > > + struct device *dev; > > + struct platform_device *phy_dev; > > + struct phy *phy; > > + struct dp_cal_data cal_data; > > + u8 max_lanes; > > + u8 max_linkrate; > > + > > + struct drm_device *drm_dev; > > + struct drm_bridge bridge; > > + struct drm_bridge *next_bridge; > > + struct drm_dp_aux aux; > > + > > + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; > > + > > + struct mtk_dp_info info; > > + > > + struct mtk_dp_train_info train_info; > > + enum mtk_dp_train_state train_state; > > + > > + struct regmap *regs; > > + > > + bool enabled; > > + > > + struct drm_connector *conn; > > +}; > > + > > +static struct regmap_config mtk_dp_regmap_config = { > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > + .max_register = SEC_OFFSET + 0x90, > > + .name = "mtk-dp-registers", > > +}; > > + > > +static struct mtk_dp *mtk_dp_from_bridge(struct drm_bridge *b) > > +{ > > + return container_of(b, struct mtk_dp, bridge); > > +} > > + > > +static u32 mtk_dp_read(struct mtk_dp *mtk_dp, u32 offset) > > +{ > > + u32 read_val; > > + int ret; > > + > > + ret = regmap_read(mtk_dp->regs, offset, &read_val); > > + if (ret) { > > + dev_err(mtk_dp->dev, "Failed to read register 0x%x: > > %d\n", > > + offset, ret); > > + return 0; > > + } > > + > > + return read_val; > > +} > > + > > +static void mtk_dp_write(struct mtk_dp *mtk_dp, u32 offset, u32 > > val) > > +{ > > This should be int... you should propagate the error to the caller, > and also > eventually take action in case you get an error. > > > + if (regmap_write(mtk_dp->regs, offset, val)) > > + dev_err(mtk_dp->dev, > > + "Failed to write register 0x%x with value > > 0x%x\n", > > + offset, val); > > +} > > + > > +static void mtk_dp_update_bits(struct mtk_dp *mtk_dp, u32 offset, > > + u32 val, u32 mask) > > +{ > > Same here. > I don't think we need to control this. From most drivers, I see there are many example which are not control the error of write register function. If there is any error, the root cause is power domain is not enabled. In this case, we can not go to these register setting. Besides, we also can saves hundreds of driver lines to handle the write register error. > > + if (regmap_update_bits(mtk_dp->regs, offset, mask, val)) > > + dev_err(mtk_dp->dev, > > + "Failed to update register 0x%x with value > > 0x%x, mask 0x%x\n", > > + offset, val, mask); > > +} > > + > > +static void mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 > > offset, u8 *buf, > > + size_t length) > > +{ > > + int i; > > + int num_regs = (length + 1) / 2; > > + > > ... and here. > > > + /* 2 bytes per register */ > > + for (i = 0; i < num_regs; i++) { > > + u32 val = buf[i * 2] | > > + (i * 2 + 1 < length ? buf[i * 2 + 1] << 8 : > > 0); > > + > > + mtk_dp_write(mtk_dp, offset + i * 4, val); > > P.S.: Does it make sense to keep writing if you get an error? > I'd say that doing this may lead to unexpected hardware > status. > If one register failed to write, it should be for *all* registers and not only for *one* register. > > + } > > +} > > + > > +static unsigned long mtk_dp_sip_atf_call(struct mtk_dp *mtk_dp, > > + unsigned int cmd, unsigned int > > para) > > +{ > > + struct arm_smccc_res res; > > + > > + arm_smccc_smc(MTK_DP_SIP_CONTROL_AARCH32, cmd, para, 0, 0, 0, > > 0, 0, > > + &res); > > + > > + dev_dbg(mtk_dp->dev, "sip cmd 0x%x, p1 0x%x, ret 0x%lx-0x%lx", > > + cmd, para, res.a0, res.a1); > > + > > + return res.a1; > > We have SIP_SVC_E_(xxxxx) error codes defined in mtk_sip_svc.h... > this makes me > think that res.a1 is not an unsigned long for real: please confirm. > ok, I will confirm that. > > +} > > + > > ..snip.. > > > + > > +static void mtk_dp_set_color_format(struct mtk_dp *mtk_dp, > > + enum dp_pixelformat color_format) > > +{ > > + u32 val; > > + > > + mtk_dp->info.format = color_format; > > + > > + /* update MISC0 */ > > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > > + color_format << DP_TEST_COLOR_FORMAT_SHIFT, > > + DP_TEST_COLOR_FORMAT_MASK); > > + > > + switch (color_format) { > > + case DP_PIXELFORMAT_YUV422: > > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422; > > + break; > > + case DP_PIXELFORMAT_YUV420: > > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420; > > + break; > > + case DP_PIXELFORMAT_RGB: > > + case DP_PIXELFORMAT_YUV444: > > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; > > + break; > > + case DP_PIXELFORMAT_Y_ONLY: > > + case DP_PIXELFORMAT_RAW: > > + case DP_PIXELFORMAT_RESERVED: > > + default: > > + drm_warn(mtk_dp->drm_dev, "Unsupported color format: > > %d\n", > > + color_format); > > + return; > > return -EINVAL here? > ok, I will take care the error handle. > > + } > > + > > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > > + val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); > > ... and return 0 here. > > > +} > > + > > +static void mtk_dp_set_color_depth(struct mtk_dp *mtk_dp) > > +{ > > + u32 val; > > + /* Only support 8 bits currently */ > > + u32 color_depth = DP_MSA_MISC_8_BPC; > > + > > + mtk_dp->info.depth = color_depth; > > + > > + /* Update MISC0 */ > > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > > + color_depth, DP_TEST_BIT_DEPTH_MASK); > > + > > + switch (color_depth) { > > + case DP_MSA_MISC_6_BPC: > > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_6BIT; > > + break; > > + case DP_MSA_MISC_8_BPC: > > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT; > > + break; > > + case DP_MSA_MISC_10_BPC: > > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_10BIT; > > + break; > > + case DP_MSA_MISC_12_BPC: > > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_12BIT; > > + break; > > + case DP_MSA_MISC_16_BPC: > > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_16BIT; > > + break; > > ditto > > > + default: > > + drm_warn(mtk_dp->drm_dev, "Unsupported color depth > > %d\n", > > + color_depth); > > + return; > > + } > > + > > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val, > > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); > > +} > > + > > ..snip.. > > > + > > +static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp, > > + u32 link_rate, int lane_count) > > +{ > > + int ret; > > + union phy_configure_opts phy_opts = { > > + .dp = { > > + .link_rate = link_rate_to_mb_per_s(mtk_dp, > > link_rate), > > + .set_rate = 1, > > + .lanes = lane_count, > > + .set_lanes = 1, > > + .ssc = mtk_dp->train_info.sink_ssc, > > + } > > + }; > > + > > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > > DP_PWR_STATE_BANDGAP, > > + DP_PWR_STATE_MASK); > > + > > + ret = phy_configure(mtk_dp->phy, &phy_opts); > > + > > This new blank line is unnecessary, please remove. > > > + if (ret) > > + return ret; > > + > > + mtk_dp_set_cal_data(mtk_dp); > > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL_LANE, > > DP_PWR_STATE_MASK); > > + > > + return 0; > > +} > > + > > ..snip.. > > > + > > +static void mtk_dp_calculate_pixrate(struct mtk_dp *mtk_dp) > > +{ > > + u8 target_frame_rate = 60; > > Don't assign any value here: this will make sure to avoid double > assignments later. > > > + u32 target_pixel_clk; > > + struct drm_display_mode mode; > > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > > + > > + drm_display_mode_from_videomode(&timings->vm, &mode); > > + > > + if (mtk_dp->info.timings.frame_rate > 0) { > > + target_frame_rate = mtk_dp->info.timings.frame_rate; > > + target_pixel_clk = mode.htotal * mode.vtotal * > > + target_frame_rate; > > + } else { > > + target_pixel_clk = mode.htotal * mode.vtotal * > > + target_frame_rate; > > + } > > This should be > > if (mtk_dp->info.timings.frame_rate > 0) > target_frame_rate = mtk_dp->info.timings.frame_rate; > else > target_frame_rate = 60; > > target_pixel_clk = mode.htotal * mode.vtotal * > target_frame_rate; > ok. > > +} > > + > > +static void mtk_dp_set_tx_out(struct mtk_dp *mtk_dp) > > +{ > > + mtk_dp_msa_bypass_disable(mtk_dp); > > + mtk_dp_calculate_pixrate(mtk_dp); > > + mtk_dp_pg_disable(mtk_dp); > > + mtk_dp_setup_tu(mtk_dp); > > +} > > + > > +static ssize_t mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > > +{ > > + ssize_t ret; > > + u8 sink_count; > > + bool locked; > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > > + u32 link_status_reg = DP_LANE0_1_STATUS; > > + > > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, > > &sink_count); > > + if (ret < 0) { > > This function can never return anything > 1, so this should probably > be: > > if (ret < 1) { > drm_err .... > return ret == 0 ? -EIO : ret; > } > ok, I will check this. BRs, Bo-Chen > > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > > + return ret; > > + } > > + > > + 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 ret; > > + } > > + > > + locked = drm_dp_channel_eq_ok(link_status, > > + mtk_dp->train_info.lane_count); > > + if (!locked && mtk_dp->train_state > > > MTK_DP_TRAIN_STATE_TRAINING) > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; > > + > > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > + DP_REMOTE_CONTROL_COMMAND_PENDING); > > + > > + if (DP_GET_SINK_COUNT(sink_count) && > > + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; > > + msleep(20); > > + } > > + > > + return 0; > > +} > > + > > ..snip.. > > > + > > +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 iteration_count; > > + 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; > > + iteration_count = 1; > > + 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 -EINVAL; > > Why are you overriding the error value here? > > > + > > + dev_dbg(mtk_dp->dev, > > + "Link train target_link_rate = 0x%x, target_lane_count > > = 0x%x\n", > > + target_link_rate, target_lane_count); > > + > > Cheers, > Angelo
Il 27/06/22 12:30, Rex-BC Chen ha scritto: > On Mon, 2022-06-27 at 18:07 +0800, AngeloGioacchino Del Regno wrote: >> Il 27/06/22 10:03, Bo-Chen Chen ha scritto: >>> 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> >>> [Bo-Chen: Cleanup the drivers and modify comments from reviewers] >>> Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> >>> --- >>> drivers/gpu/drm/mediatek/Kconfig | 10 + >>> drivers/gpu/drm/mediatek/Makefile | 1 + >>> drivers/gpu/drm/mediatek/mtk_dp.c | 2198 >>> ++++++++++++++++++++++++ >>> drivers/gpu/drm/mediatek/mtk_dp_reg.h | 543 ++++++ >>> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 3 + >>> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + >>> 6 files changed, 2758 insertions(+) >>> create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c >>> create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h >>> >> >> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c >>> b/drivers/gpu/drm/mediatek/mtk_dp.c >>> new file mode 100644 >>> index 000000000000..9e9b516409e2 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c >>> @@ -0,0 +1,2198 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Copyright (c) 2019-2022 MediaTek Inc. >>> + * Copyright (c) 2022 BayLibre >>> + */ >>> + >>> +#include <drm/display/drm_dp.h> >>> +#include <drm/display/drm_dp_helper.h> >>> +#include <drm/drm_atomic_helper.h> >>> +#include <drm/drm_bridge.h> >>> +#include <drm/drm_crtc.h> >>> +#include <drm/drm_edid.h> >>> +#include <drm/drm_of.h> >>> +#include <drm/drm_panel.h> >>> +#include <drm/drm_print.h> >>> +#include <drm/drm_probe_helper.h> >>> +#include <linux/arm-smccc.h> >>> +#include <linux/clk.h> >>> +#include <linux/delay.h> >>> +#include <linux/errno.h> >>> +#include <linux/kernel.h> >>> +#include <linux/nvmem-consumer.h> >>> +#include <linux/of.h> >>> +#include <linux/of_irq.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/phy/phy.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/pm_runtime.h> >>> +#include <linux/regmap.h> >>> +#include <sound/hdmi-codec.h> >>> +#include <video/videomode.h> >>> + >>> +#include "mtk_dp_reg.h" >>> + >>> +#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523 >> >> Why have you forced this SIP call to AArch32 SMC convention? >> Is there any particular reason why this should always be AA32 and >> *never* AA64? >> >> In any case, you've got MediaTek SIP macros in >> include/soc/mediatek/mtk_sip_svc.h >> so, please, include that header and either redefine this as >> >> MTK_SIP_CMD(0x523) or add a new macro in there to force >> ARM_SMCCC_SMC_32 >> convention with a very explanatory comment saying why some calls need >> to >> be forced to use the AArch32 SMC convention. >> > > Hello Angelo, > > Thanks for your review. > ok, I will do this in next version. > >>> + >>> +#define MTK_VDOSYS1_MAX_FRAMERATE 60 >>> +#define MTK_DP_4P1T 4 >>> +#define MTK_DP_HDE 2 >>> +#define MTK_DP_PIX_PER_ADDR 2 >>> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 >>> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3 >>> +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 >>> +#define MTK_DP_TRAIN_RETRY_LIMIT 8 >>> +#define MTK_DP_TRAIN_MAX_ITERATIONS 5 >>> +#define MTK_DP_DP_VERSION_11 0x11 >> >> MTK_DP_HW_VERSION_11 0x11 ? >> >> ...but anyway, this definition is unused, so please either use it or >> drop it. >> > > it's fro audio patch, and I will move this there. > >>> + >>> +enum mtk_dp_train_state { >>> + MTK_DP_TRAIN_STATE_TRAINING, >>> + MTK_DP_TRAIN_STATE_NORMAL, >>> +}; >>> + >>> +struct mtk_dp_timings { >>> + struct videomode vm; >>> + u8 frame_rate; >>> +}; >>> + >>> +struct mtk_dp_irq_sta { >>> + bool hpd_disconnect; >>> + bool hpd_inerrupt; >>> +}; >>> + >>> +struct mtk_dp_train_info { >>> + bool tps3; >>> + bool tps4; >>> + bool sink_ssc; >>> + bool cable_plugged_in; >>> + bool cable_state_change; >>> + bool cr_done; >>> + bool eq_done; >>> + /* link_rate is in multiple of 0.27Gbps */ >>> + int link_rate; >>> + int lane_count; >>> + struct mtk_dp_irq_sta irq_sta; >>> +}; >>> + >>> +struct mtk_dp_info { >>> + u32 depth; >>> + enum dp_pixelformat format; >>> + struct mtk_dp_timings timings; >>> +}; >>> + >>> +struct dp_cal_data { >>> + unsigned int glb_bias_trim; >>> + unsigned int clktx_impse; >>> + >>> + unsigned int ln_tx_impsel_pmos[4]; >>> + unsigned int ln_tx_impsel_nmos[4]; >>> +}; >>> + >>> +struct mtk_dp { >>> + struct device *dev; >>> + struct platform_device *phy_dev; >>> + struct phy *phy; >>> + struct dp_cal_data cal_data; >>> + u8 max_lanes; >>> + u8 max_linkrate; >>> + >>> + struct drm_device *drm_dev; >>> + struct drm_bridge bridge; >>> + struct drm_bridge *next_bridge; >>> + struct drm_dp_aux aux; >>> + >>> + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; >>> + >>> + struct mtk_dp_info info; >>> + >>> + struct mtk_dp_train_info train_info; >>> + enum mtk_dp_train_state train_state; >>> + >>> + struct regmap *regs; >>> + >>> + bool enabled; >>> + >>> + struct drm_connector *conn; >>> +}; >>> + >>> +static struct regmap_config mtk_dp_regmap_config = { >>> + .reg_bits = 32, >>> + .val_bits = 32, >>> + .reg_stride = 4, >>> + .max_register = SEC_OFFSET + 0x90, >>> + .name = "mtk-dp-registers", >>> +}; >>> + >>> +static struct mtk_dp *mtk_dp_from_bridge(struct drm_bridge *b) >>> +{ >>> + return container_of(b, struct mtk_dp, bridge); >>> +} >>> + >>> +static u32 mtk_dp_read(struct mtk_dp *mtk_dp, u32 offset) >>> +{ >>> + u32 read_val; >>> + int ret; >>> + >>> + ret = regmap_read(mtk_dp->regs, offset, &read_val); >>> + if (ret) { >>> + dev_err(mtk_dp->dev, "Failed to read register 0x%x: >>> %d\n", >>> + offset, ret); >>> + return 0; >>> + } >>> + >>> + return read_val; >>> +} >>> + >>> +static void mtk_dp_write(struct mtk_dp *mtk_dp, u32 offset, u32 >>> val) >>> +{ >> >> This should be int... you should propagate the error to the caller, >> and also >> eventually take action in case you get an error. >> >>> + if (regmap_write(mtk_dp->regs, offset, val)) >>> + dev_err(mtk_dp->dev, >>> + "Failed to write register 0x%x with value >>> 0x%x\n", >>> + offset, val); >>> +} >>> + >>> +static void mtk_dp_update_bits(struct mtk_dp *mtk_dp, u32 offset, >>> + u32 val, u32 mask) >>> +{ >> >> Same here. >> > > I don't think we need to control this. > From most drivers, I see there are many example which are not control > the error of write register function. > > If there is any error, the root cause is power domain is not enabled. > In this case, we can not go to these register setting. Besides, we also > can saves hundreds of driver lines to handle the write register error. > I agree with your vision - but you may have misunderstood the purpose of what I've proposed.... I'm not proposing to *always check* because, yes, sometimes (most of the times?) you can *safely* assume that the write gives no error and just works as we expect, but to only check that in some particular situations, one of which is in the mtk_dp_bulk_16bit_write() function (yeah, we have mtk_dp_write, not mtk_dp_update_bits, but the comment was put on the former as well). >>> + if (regmap_update_bits(mtk_dp->regs, offset, mask, val)) >>> + dev_err(mtk_dp->dev, >>> + "Failed to update register 0x%x with value >>> 0x%x, mask 0x%x\n", >>> + offset, val, mask); >>> +} >>> + >>> +static void mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 >>> offset, u8 *buf, >>> + size_t length) >>> +{ >>> + int i; >>> + int num_regs = (length + 1) / 2; >>> + >> >> ... and here. >> >>> + /* 2 bytes per register */ >>> + for (i = 0; i < num_regs; i++) { >>> + u32 val = buf[i * 2] | >>> + (i * 2 + 1 < length ? buf[i * 2 + 1] << 8 : >>> 0); >>> + >>> + mtk_dp_write(mtk_dp, offset + i * 4, val); >> >> P.S.: Does it make sense to keep writing if you get an error? >> I'd say that doing this may lead to unexpected hardware >> status. >> > > If one register failed to write, it should be for *all* registers and > not only for *one* register. > Yes, that's true - but your mtk_dp_write() function prints an error in the kmsg: for example, if `num_regs` is 4, we will get four prints. Also, since logically one register write failing means all will fail (and usually it either fails at the first write, or never) if we check for the return value here, we can avoid iterating again with an expected, repeated, failure. That's what I was meaning - hope it's clear now :-) >>> + } >>> +} >>> + >>> +static unsigned long mtk_dp_sip_atf_call(struct mtk_dp *mtk_dp, >>> + unsigned int cmd, unsigned int >>> para) >>> +{ >>> + struct arm_smccc_res res; >>> + >>> + arm_smccc_smc(MTK_DP_SIP_CONTROL_AARCH32, cmd, para, 0, 0, 0, >>> 0, 0, >>> + &res); >>> + >>> + dev_dbg(mtk_dp->dev, "sip cmd 0x%x, p1 0x%x, ret 0x%lx-0x%lx", >>> + cmd, para, res.a0, res.a1); >>> + >>> + return res.a1; >> >> We have SIP_SVC_E_(xxxxx) error codes defined in mtk_sip_svc.h... >> this makes me >> think that res.a1 is not an unsigned long for real: please confirm. >> > > ok, I will confirm that. > >>> +} >>> + >> >> ..snip.. >> >>> + >>> +static void mtk_dp_set_color_format(struct mtk_dp *mtk_dp, >>> + enum dp_pixelformat color_format) >>> +{ >>> + u32 val; >>> + >>> + mtk_dp->info.format = color_format; >>> + >>> + /* update MISC0 */ >>> + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, >>> + color_format << DP_TEST_COLOR_FORMAT_SHIFT, >>> + DP_TEST_COLOR_FORMAT_MASK); >>> + >>> + switch (color_format) { >>> + case DP_PIXELFORMAT_YUV422: >>> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422; >>> + break; >>> + case DP_PIXELFORMAT_YUV420: >>> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420; >>> + break; >>> + case DP_PIXELFORMAT_RGB: >>> + case DP_PIXELFORMAT_YUV444: >>> + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; >>> + break; >>> + case DP_PIXELFORMAT_Y_ONLY: >>> + case DP_PIXELFORMAT_RAW: >>> + case DP_PIXELFORMAT_RESERVED: >>> + default: >>> + drm_warn(mtk_dp->drm_dev, "Unsupported color format: >>> %d\n", >>> + color_format); >>> + return; >> >> return -EINVAL here? >> > > ok, I will take care the error handle. > >>> + } >>> + >>> + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, >>> + val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); >> >> ... and return 0 here. >> >>> +} >>> + >>> +static void mtk_dp_set_color_depth(struct mtk_dp *mtk_dp) >>> +{ >>> + u32 val; >>> + /* Only support 8 bits currently */ >>> + u32 color_depth = DP_MSA_MISC_8_BPC; >>> + >>> + mtk_dp->info.depth = color_depth; >>> + >>> + /* Update MISC0 */ >>> + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, >>> + color_depth, DP_TEST_BIT_DEPTH_MASK); >>> + >>> + switch (color_depth) { >>> + case DP_MSA_MISC_6_BPC: >>> + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_6BIT; >>> + break; >>> + case DP_MSA_MISC_8_BPC: >>> + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT; >>> + break; >>> + case DP_MSA_MISC_10_BPC: >>> + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_10BIT; >>> + break; >>> + case DP_MSA_MISC_12_BPC: >>> + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_12BIT; >>> + break; >>> + case DP_MSA_MISC_16_BPC: >>> + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_16BIT; >>> + break; >> >> ditto >> >>> + default: >>> + drm_warn(mtk_dp->drm_dev, "Unsupported color depth >>> %d\n", >>> + color_depth); >>> + return; >>> + } >>> + >>> + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val, >>> + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); >>> +} >>> + >> >> ..snip.. >> >>> + >>> +static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp, >>> + u32 link_rate, int lane_count) >>> +{ >>> + int ret; >>> + union phy_configure_opts phy_opts = { >>> + .dp = { >>> + .link_rate = link_rate_to_mb_per_s(mtk_dp, >>> link_rate), >>> + .set_rate = 1, >>> + .lanes = lane_count, >>> + .set_lanes = 1, >>> + .ssc = mtk_dp->train_info.sink_ssc, >>> + } >>> + }; >>> + >>> + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, >>> DP_PWR_STATE_BANDGAP, >>> + DP_PWR_STATE_MASK); >>> + >>> + ret = phy_configure(mtk_dp->phy, &phy_opts); >>> + >> >> This new blank line is unnecessary, please remove. >> >>> + if (ret) >>> + return ret; >>> + >>> + mtk_dp_set_cal_data(mtk_dp); >>> + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, >>> + DP_PWR_STATE_BANDGAP_TPLL_LANE, >>> DP_PWR_STATE_MASK); >>> + >>> + return 0; >>> +} >>> + >> >> ..snip.. >> >>> + >>> +static void mtk_dp_calculate_pixrate(struct mtk_dp *mtk_dp) >>> +{ >>> + u8 target_frame_rate = 60; >> >> Don't assign any value here: this will make sure to avoid double >> assignments later. >> >>> + u32 target_pixel_clk; >>> + struct drm_display_mode mode; >>> + struct mtk_dp_timings *timings = &mtk_dp->info.timings; >>> + >>> + drm_display_mode_from_videomode(&timings->vm, &mode); >>> + >>> + if (mtk_dp->info.timings.frame_rate > 0) { >>> + target_frame_rate = mtk_dp->info.timings.frame_rate; >>> + target_pixel_clk = mode.htotal * mode.vtotal * >>> + target_frame_rate; >>> + } else { >>> + target_pixel_clk = mode.htotal * mode.vtotal * >>> + target_frame_rate; >>> + } >> >> This should be >> >> if (mtk_dp->info.timings.frame_rate > 0) >> target_frame_rate = mtk_dp->info.timings.frame_rate; >> else >> target_frame_rate = 60; >> >> target_pixel_clk = mode.htotal * mode.vtotal * >> target_frame_rate; >> > > ok. > >>> +} >>> + >>> +static void mtk_dp_set_tx_out(struct mtk_dp *mtk_dp) >>> +{ >>> + mtk_dp_msa_bypass_disable(mtk_dp); >>> + mtk_dp_calculate_pixrate(mtk_dp); >>> + mtk_dp_pg_disable(mtk_dp); >>> + mtk_dp_setup_tu(mtk_dp); >>> +} >>> + >>> +static ssize_t mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) >>> +{ >>> + ssize_t ret; >>> + u8 sink_count; >>> + bool locked; >>> + u8 link_status[DP_LINK_STATUS_SIZE] = {}; >>> + u32 sink_count_reg = DP_SINK_COUNT_ESI; >>> + u32 link_status_reg = DP_LANE0_1_STATUS; >>> + >>> + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, >>> &sink_count); >>> + if (ret < 0) { >> >> This function can never return anything > 1, so this should probably >> be: >> >> if (ret < 1) { >> drm_err .... >> return ret == 0 ? -EIO : ret; >> } >> > > ok, I will check this. > > BRs, > Bo-Chen > >>> + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); >>> + return ret; >>> + } >>> + >>> + 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 ret; >>> + } >>> + >>> + locked = drm_dp_channel_eq_ok(link_status, >>> + mtk_dp->train_info.lane_count); >>> + if (!locked && mtk_dp->train_state > >>> MTK_DP_TRAIN_STATE_TRAINING) >>> + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; >>> + >>> + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) >>> + drm_dp_dpcd_writeb(&mtk_dp->aux, >>> DP_DEVICE_SERVICE_IRQ_VECTOR, >>> + DP_REMOTE_CONTROL_COMMAND_PENDING); >>> + >>> + if (DP_GET_SINK_COUNT(sink_count) && >>> + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { >>> + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING; >>> + msleep(20); >>> + } >>> + >>> + return 0; >>> +} >>> + >> >> ..snip.. >> >>> + >>> +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 iteration_count; >>> + 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; >>> + iteration_count = 1; >>> + 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 -EINVAL; >> >> Why are you overriding the error value here? >> >>> + >>> + dev_dbg(mtk_dp->dev, >>> + "Link train target_link_rate = 0x%x, target_lane_count >>> = 0x%x\n", >>> + target_link_rate, target_lane_count); >>> + >> >> Cheers, >> Angelo >
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/Kconfig | 10 + > drivers/gpu/drm/mediatek/Makefile | 1 + > drivers/gpu/drm/mediatek/mtk_dp.c | 2198 > ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 543 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 3 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + > 6 files changed, 2758 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > diff --git a/drivers/gpu/drm/mediatek/Kconfig > b/drivers/gpu/drm/mediatek/Kconfig > index 2976d21e9a34..6d3af73e7e8c 100644 > --- a/drivers/gpu/drm/mediatek/Kconfig > +++ b/drivers/gpu/drm/mediatek/Kconfig > @@ -15,12 +15,22 @@ config DRM_MEDIATEK > select MTK_SMI > select PHY_MTK_MIPI_DSI > select VIDEOMODE_HELPERS > + select DRM_MEDIATEK_DP Remove this. > help > Choose this option if you have a Mediatek SoCs. > The module will be called mediatek-drm > This driver provides kernel mode setting and > buffer management to userspace. > > +config DRM_MEDIATEK_DP > + tristate "DRM DPTX Support for MediaTek SoCs" > + depends on DRM_MEDIATEK > + select PHY_MTK_DP > + select DRM_DISPLAY_HELPER > + select DRM_DISPLAY_DP_HELPER > + help > + DRM/KMS Display Port driver for MediaTek SoCs. > + > [snip] > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index 78e79c8449c8..8023f1bd5f7e 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -1033,6 +1033,9 @@ static struct platform_driver * const > mtk_drm_drivers[] = { > &mtk_disp_ovl_driver, > &mtk_disp_rdma_driver, > &mtk_dpi_driver, > +#if IS_REACHABLE(CONFIG_DRM_MEDIATEK_DP) > + &mtk_dp_driver, > +#endif Remove this, and treat dp driver like hdmi driver. Regards, CK > &mtk_drm_platform_driver, > &mtk_dsi_driver, > &mtk_ethdr_driver, >
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static int mtk_dp_training(struct mtk_dp *mtk_dp) > +{ > + bool training_done = false; > + short max_retry = 50; > + int ret = 0; > + > + do { > + switch (mtk_dp->train_state) { > + case MTK_DP_TRAIN_STATE_TRAINING: > + ret = mtk_dp_train_start(mtk_dp); > + if (!ret) > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_NORMAL; > + break; > + case MTK_DP_TRAIN_STATE_NORMAL: > + mtk_dp_video_config(mtk_dp); > + mtk_dp_video_enable(mtk_dp, true); > + training_done = true; > + break; > + default: > + break; > + } > + > + if (ret) { > + if (ret == -EAGAIN) > + continue; > + /* > + * If we get any other error number, it doesn't > + * make any sense to keep iterating. > + */ > + break; > + } > + } while (!training_done || --max_retry); > + > + return ret; > +} This function could re rewritten as: static bool mtk_dp_training(struct mtk_dp *mtk_dp) { short max_retry = 50; do { ret = mtk_dp_train_start(mtk_dp); if (!ret) break; else if (ret != -EAGAIN) return false; } while (--max_retry); if (!max_retry) return false; mtk_dp_video_config(mtk_dp); mtk_dp_video_enable(mtk_dp, true); return true; } Regards, CK
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > +{ > + struct mtk_dp *mtk_dp = dev; > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > + > + if (mtk_dp->train_info.cable_state_change) { > + mtk_dp->train_info.cable_state_change = false; > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LANE, > + DP_PWR_STATE_MASK); > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + mtk_dp->train_info.link_rate = > + min_t(int, mtk_dp->max_linkrate, > + buf[mtk_dp->max_linkrate]); > + mtk_dp->train_info.lane_count = > + min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(buf)); If the state_change is unplug, why do you modify link_rate and lane_count? If the state_change is plug, there is a training flow to decide link_rate and lane_count. I think the training flow is correct and any modification here is redundant. Regards, CK > + } > + > + if (mtk_dp->train_info.irq_sta.hpd_inerrupt) { > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > + mtk_dp->train_info.irq_sta.hpd_inerrupt = false; > + mtk_dp_hpd_sink_event(mtk_dp); > + } > + > + return IRQ_HANDLED; > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static void mtk_dp_calculate_pixrate(struct mtk_dp *mtk_dp) > +{ > + u8 target_frame_rate = 60; > + u32 target_pixel_clk; > + struct drm_display_mode mode; > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > + > + drm_display_mode_from_videomode(&timings->vm, &mode); > + > + if (mtk_dp->info.timings.frame_rate > 0) { > + target_frame_rate = mtk_dp->info.timings.frame_rate; > + target_pixel_clk = mode.htotal * mode.vtotal * > + target_frame_rate; target_pixel_clk is not used in other place, so remove it. Regards, CK > + } else { > + target_pixel_clk = mode.htotal * mode.vtotal * > + target_frame_rate; > + } > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp) > +{ > + u32 sram_read_start = MTK_DP_TBC_BUF_READ_START_ADDR; This initial value is redundant. So remove this initial value. > + > + if (mtk_dp->train_info.lane_count > 0) { This checking would always be true. This function would be called after training success, so mtk_dp- >train_info.lane_count would be greater than 0. So remove this checking. Regards, CK > + sram_read_start = min_t(u32, > + MTK_DP_TBC_BUF_READ_START_ADDR, > + mtk_dp->info.timings.vm.hactive > / > + mtk_dp->train_info.lane_count / > + MTK_DP_4P1T / MTK_DP_HDE / > MTK_DP_PIX_PER_ADDR); > + mtk_dp_set_sram_read_start(mtk_dp, sram_read_start); > + } > + > + mtk_dp_setup_encoder(mtk_dp); > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static int link_rate_to_mb_per_s(struct mtk_dp *mtk_dp, u32 > linkrate) > +{ > + switch (linkrate) { > + case DP_LINK_BW_1_62: > + return 1620; > + case DP_LINK_BW_2_7: > + return 2700; > + case DP_LINK_BW_5_4: > + return 5400; > + case DP_LINK_BW_8_1: > + return 8100; > + default: > + drm_err(mtk_dp->drm_dev, > + "Implementation error, unknown linkrate %d\n", > + linkrate); > + return -EINVAL; > + } > +} It looks like this function is equal to drm_dp_bw_code_to_link_rate(), so remove this function and use drm_dp_bw_code_to_link_rate(). Regards, CK
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h > @@ -0,0 +1,543 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2019-2022 MediaTek Inc. > + * Copyright (c) 2022 BayLibre > + */ > +#ifndef _MTK_DP_REG_H_ > +#define _MTK_DP_REG_H_ > + > +#define TOP_OFFSET 0x2000 > +#define ENC0_OFFSET 0x3000 > +#define ENC1_OFFSET 0x3200 > +#define TRANS_OFFSET 0x3400 > +#define AUX_OFFSET 0x3600 > +#define SEC_OFFSET 0x4000 > + > +#define MTK_DP_SIP_ATF_VIDEO_UNMUTE BIT(5) Useless, so remove it. Regards, CK > +#define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE (BIT(0) | BIT(5)) > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static void mtk_dp_power_enable(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE, > + 0, SW_RST_B_PHYD); > + > + /* Wait for power enable */ > + usleep_range(10, 200); > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE, > + SW_RST_B_PHYD, SW_RST_B_PHYD); > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL, > DP_PWR_STATE_MASK); > +} > + > +static void mtk_dp_power_disable(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0); > + > + mtk_dp_write(mtk_dp, MTK_DP_0034, > + DA_CKM_CKTX0_EN_FORCE_EN | > + DA_CKM_BIAS_LPF_EN_FORCE_VAL | > + DA_CKM_BIAS_EN_FORCE_VAL | > + DA_XTP_GLB_LDO_EN_FORCE_VAL | > + DA_XTP_GLB_AVD10_ON_FORCE_VAL); > + > + /* Disable RX */ > + mtk_dp_write(mtk_dp, MTK_DP_1040, 0); MTK_DP_1040 is set to 0 in mtk_dp_power_disable(), but it is not set to other value in mtk_dp_power_enable(). Does any thing would be wrong when mtk_dp_power_disable() and mtk_dp_power_enable()? Regards, CK > + mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD, > + 0x550 | BIT(FUSE_SEL_SHIFT) | > BIT(MEM_ISO_EN_SHIFT)); > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static void mtk_dp_power_disable(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0); > + > + mtk_dp_write(mtk_dp, MTK_DP_0034, > + DA_CKM_CKTX0_EN_FORCE_EN | > + DA_CKM_BIAS_LPF_EN_FORCE_VAL | > + DA_CKM_BIAS_EN_FORCE_VAL | > + DA_XTP_GLB_LDO_EN_FORCE_VAL | > + DA_XTP_GLB_AVD10_ON_FORCE_VAL); Is this clock gating? If so, separate this to ccf driver. Regards, CK > + > + /* Disable RX */ > + mtk_dp_write(mtk_dp, MTK_DP_1040, 0); > + mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD, > + 0x550 | BIT(FUSE_SEL_SHIFT) | > BIT(MEM_ISO_EN_SHIFT)); > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static int mtk_dp_dt_parse(struct mtk_dp *mtk_dp, > + struct platform_device *pdev) > +{ > + struct device_node *of_node = pdev->dev.of_node; > + struct device *dev = &pdev->dev; > + int ret = 0; > + void __iomem *base; > + u32 linkrate; > + int len; > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + mtk_dp->regs = devm_regmap_init_mmio(dev, base, > &mtk_dp_regmap_config); > + if (IS_ERR(mtk_dp->regs)) > + return PTR_ERR(mtk_dp->regs); > + > + len = of_property_count_elems_of_size(of_node, > + "data-lanes", > sizeof(u32)); > + if (len < 0 || len > 4 || len == 3) { > + dev_err(dev, "invalid data lane size: %d\n", len); > + return -EINVAL; > + } > + > + mtk_dp->max_lanes = len; > + > + ret = device_property_read_u32(dev, "max-linkrate-mhz", > &linkrate); > + if (ret) { > + dev_err(dev, "failed to read max linkrate: %d\n", ret); > + return ret; > + } > + > + switch (linkrate) { > + case 8100: /* 8.1G */ > + mtk_dp->max_linkrate = DP_LINK_BW_8_1; > + break; > + case 5400: /* 5.4G */ > + mtk_dp->max_linkrate = DP_LINK_BW_5_4; > + break; > + case 2700: /* 2.7G */ > + mtk_dp->max_linkrate = DP_LINK_BW_2_7; > + break; > + case 1620: /* 1.62G */ > + mtk_dp->max_linkrate = DP_LINK_BW_1_62; > + break; > + default: > + dev_err(dev, "invalid linkrate: %d\n", linkrate); > + return -EINVAL; > + } Use drm_dp_link_rate_to_bw_code() instead of self-implementation. Regards, CK > + > + return 0; > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static void mtk_dp_set_color_depth(struct mtk_dp *mtk_dp) > +{ > + u32 val; > + /* Only support 8 bits currently */ > + u32 color_depth = DP_MSA_MISC_8_BPC; > + > + mtk_dp->info.depth = color_depth; > + > + /* Update MISC0 */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + color_depth, DP_TEST_BIT_DEPTH_MASK); > + > + switch (color_depth) { > + case DP_MSA_MISC_6_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_6BIT; > + break; > + case DP_MSA_MISC_8_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT; This driver just use DP_MSA_MISC_8_BPC, so keep this and drop others. Regards, CK > + break; > + case DP_MSA_MISC_10_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_10BIT; > + break; > + case DP_MSA_MISC_12_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_12BIT; > + break; > + case DP_MSA_MISC_16_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_16BIT; > + break; > + default: > + drm_warn(mtk_dp->drm_dev, "Unsupported color depth > %d\n", > + color_depth); > + return; > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); > +} > +
Hi, Bo-Chen: On Mon, 2022-06-27 at 16:03 +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> > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > + > +static void mtk_dp_set_color_format(struct mtk_dp *mtk_dp, > + enum dp_pixelformat color_format) > +{ > + u32 val; > + > + mtk_dp->info.format = color_format; Drop this. mtk_dp->info.format is alwasy set out of this function. > + > + /* update MISC0 */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + color_format << DP_TEST_COLOR_FORMAT_SHIFT, > + DP_TEST_COLOR_FORMAT_MASK); > + > + switch (color_format) { > + case DP_PIXELFORMAT_YUV422: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422; > + break; This driver use only DP_PIXELFORMAT_YUV422 and DP_PIXELFORMAT_RGB, so keep these two and drop others. Regards, CK > + case DP_PIXELFORMAT_YUV420: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420; > + break; > + case DP_PIXELFORMAT_RGB: > + case DP_PIXELFORMAT_YUV444: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; > + break; > + case DP_PIXELFORMAT_Y_ONLY: > + case DP_PIXELFORMAT_RAW: > + case DP_PIXELFORMAT_RESERVED: > + default: > + drm_warn(mtk_dp->drm_dev, "Unsupported color format: > %d\n", > + color_format); > + return; > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > + val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); > +} > +
On Thu, 2022-06-30 at 09:47 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Mon, 2022-06-27 at 16:03 +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> > > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > --- > > [snip] > > > + > > +static void mtk_dp_power_disable(struct mtk_dp *mtk_dp) > > +{ > > + mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0); > > + > > + mtk_dp_write(mtk_dp, MTK_DP_0034, > > + DA_CKM_CKTX0_EN_FORCE_EN | > > + DA_CKM_BIAS_LPF_EN_FORCE_VAL | > > + DA_CKM_BIAS_EN_FORCE_VAL | > > + DA_XTP_GLB_LDO_EN_FORCE_VAL | > > + DA_XTP_GLB_AVD10_ON_FORCE_VAL); > > Is this clock gating? If so, separate this to ccf driver. > > Regards, > CK > Hello CK, this register is inside dp hardware, so it should not move to ccf driver. BRs, Bo-Chen > > + > > + /* Disable RX */ > > + mtk_dp_write(mtk_dp, MTK_DP_1040, 0); > > + mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD, > > + 0x550 | BIT(FUSE_SEL_SHIFT) | > > BIT(MEM_ISO_EN_SHIFT)); > > +} > > + > >
On Wed, 2022-06-29 at 13:34 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Mon, 2022-06-27 at 16:03 +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> > > [Bo-Chen: Cleanup the drivers and modify comments from reviewers] > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > --- > > [snip] > > > + > > +static void mtk_dp_power_enable(struct mtk_dp *mtk_dp) > > +{ > > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE, > > + 0, SW_RST_B_PHYD); > > + > > + /* Wait for power enable */ > > + usleep_range(10, 200); > > + > > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE, > > + SW_RST_B_PHYD, SW_RST_B_PHYD); > > + mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, > > + DP_PWR_STATE_BANDGAP_TPLL, > > DP_PWR_STATE_MASK); > > +} > > + > > +static void mtk_dp_power_disable(struct mtk_dp *mtk_dp) > > +{ > > + mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0); > > + > > + mtk_dp_write(mtk_dp, MTK_DP_0034, > > + DA_CKM_CKTX0_EN_FORCE_EN | > > + DA_CKM_BIAS_LPF_EN_FORCE_VAL | > > + DA_CKM_BIAS_EN_FORCE_VAL | > > + DA_XTP_GLB_LDO_EN_FORCE_VAL | > > + DA_XTP_GLB_AVD10_ON_FORCE_VAL); > > + > > + /* Disable RX */ > > + mtk_dp_write(mtk_dp, MTK_DP_1040, 0); > > MTK_DP_1040 is set to 0 in mtk_dp_power_disable(), but it is not set > to > other value in mtk_dp_power_enable(). Does any thing would be wrong > when mtk_dp_power_disable() and mtk_dp_power_enable()? > > Regards, > CK > Hello CK, in mtk_dp_power_enable(), after we reset the hw: mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_RESET_AND_PROBE, SW_RST_B_PHYD, SW_RST_B_PHYD); mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, DP_PWR_STATE_BANDGAP_TPLL, DP_PWR_STATE_MASK); the value will be set back to default value 0x7. I will add "mtk_dp_write(mtk_dp, MTK_DP_1040, 7);" to prevent misunderstanding. BRs, Bo-Chen > > + mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD, > > + 0x550 | BIT(FUSE_SEL_SHIFT) | > > BIT(MEM_ISO_EN_SHIFT)); > > +} > > + > >