| Message ID | 20220701062808.18596-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add MT8195 DisplayPort driver | expand |
Il 01/07/22 08:28, 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> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 01/07/22 08:28, 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> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dp.c | 197 +++++++++++++++++++++----- > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 + > 2 files changed, 161 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > index b672d5a6f5bd..c3be97dd055c 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -105,6 +105,7 @@ struct mtk_dp { > struct regmap *regs; > > bool enabled; > + bool has_fec; You're introducing this has_fec variable here.... > > struct drm_connector *conn; > }; > @@ -1018,6 +1074,8 @@ static void mtk_dp_initialize_priv_data(struct mtk_dp *mtk_dp) > mtk_dp->info.depth = DP_MSA_MISC_8_BPC; > memset(&mtk_dp->info.timings, 0, sizeof(struct mtk_dp_timings)); > mtk_dp->info.timings.frame_rate = 60; > + > + mtk_dp->has_fec = false; .... setting it as false here .... > } > > static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp) > @@ -1498,15 +1562,38 @@ static int mtk_dp_init_port(struct mtk_dp *mtk_dp) > static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > { > struct mtk_dp *mtk_dp = dev; > + int event; > u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > + event = mtk_dp_plug_state(mtk_dp) ? > + connector_status_connected : connector_status_disconnected; > + > + if (event < 0) > + return IRQ_HANDLED; > + > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); P.S.: This should be a dev_dbg(). > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > + > 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); > + if (!mtk_dp->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); > + if (mtk_dp->has_fec) ...and you're checking it here, but there's nothing ever setting that as true! Is there anything you forgot? :-) Cheers, Angelo
Il 01/07/22 08:28, Bo-Chen Chen ha scritto: > From: Guillaume Ranquet <granquet@baylibre.com> > > This patch adds audio support to the DP driver for MT8195 with up to 8 > channels. > > 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 01/07/22 08:28, Bo-Chen Chen ha scritto: > When switching resolutions, config the audio setting with the > previous audio parameters. > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> Change the title to: drm/mediatek: Use cached audio config when changing resolution ...and a more suitable description would be: Use the cached audio configuration during a resolution switch to avoid loss of sound. (perhaps also explain why we're losing it and why using cached data is necessary). After which... Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Fri, 2022-07-01 at 16:14 +0800, AngeloGioacchino Del Regno wrote: > Il 01/07/22 08:28, 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> > > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dp.c | 197 > > +++++++++++++++++++++----- > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 + > > 2 files changed, 161 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > > b/drivers/gpu/drm/mediatek/mtk_dp.c > > index b672d5a6f5bd..c3be97dd055c 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > > @@ -105,6 +105,7 @@ struct mtk_dp { > > struct regmap *regs; > > > > bool enabled; > > + bool has_fec; > > You're introducing this has_fec variable here.... > > > > > struct drm_connector *conn; > > }; > > > > @@ -1018,6 +1074,8 @@ static void > > mtk_dp_initialize_priv_data(struct mtk_dp *mtk_dp) > > mtk_dp->info.depth = DP_MSA_MISC_8_BPC; > > memset(&mtk_dp->info.timings, 0, sizeof(struct > > mtk_dp_timings)); > > mtk_dp->info.timings.frame_rate = 60; > > + > > + mtk_dp->has_fec = false; > > .... setting it as false here .... > > > } > > > > static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp) > > @@ -1498,15 +1562,38 @@ static int mtk_dp_init_port(struct mtk_dp > > *mtk_dp) > > static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) > > { > > struct mtk_dp *mtk_dp = dev; > > + int event; > > u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > > > + event = mtk_dp_plug_state(mtk_dp) ? > > + connector_status_connected : > > connector_status_disconnected; > > + > > + if (event < 0) > > + return IRQ_HANDLED; > > + > > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > > P.S.: This should be a dev_dbg(). > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > + > > 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); > > + if (!mtk_dp->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); > > + if (mtk_dp->has_fec) > > ...and you're checking it here, but there's nothing ever setting that > as true! > > Is there anything you forgot? :-) > > Cheers, > Angelo Hello Angelo, Thanks for your review. We do not support fec currently, so I will drop them. Thanks! BRs, Bo-Chen
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- > + > +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); clk-mt8195-vdo0 driver [1] is part of mtk-mmsys driver [2] and it is still separated out to ccf driver. In addition, you does not manage the parent clock. If the parent clock is not enable, these leaf clock would not work. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/mediatek/clk-mt8195-vdo0.c?h=v5.19-rc5#n138 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/mediatek/mtk-mmsys.c?h=v5.19-rc5#n140 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 Fri, 2022-07-01 at 14:28 +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> > --- > + > +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); > + mtk_dp_write(mtk_dp, MTK_DP_1040, 0x7); You have define the bit definition. Use the bit definition instead of a magic number. +#define MTK_DP_1040 0x1040 +#define RG_DPAUX_RX_VALID_DEGLITCH_EN BIT(2) +#define RG_XTP_GLB_CKDET_EN BIT(1) +#define RG_DPAUX_RX_EN BIT(0) > +} > +
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [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); Why do you read dpcd caps into 'buf'. 'buf' is not used elsewhere. 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 Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +struct mtk_dp_timings { > + struct videomode vm; > + u8 frame_rate; You just assign value to frame_rate, but you never use this frame_rate, so drop it. Regards, CK > +}; > + [snip] > + > +static void mtk_dp_initialize_priv_data(struct mtk_dp *mtk_dp) > +{ > + mtk_dp->train_info.link_rate = DP_LINK_BW_5_4; > + mtk_dp->train_info.lane_count = mtk_dp->max_lanes; > + mtk_dp->train_info.cable_plugged_in = false; > + mtk_dp->train_info.cable_state_change = false; > + memset(&mtk_dp->train_info.irq_sta, 0, sizeof(struct > mtk_dp_irq_sta)); > + > + mtk_dp->info.format = DP_PIXELFORMAT_RGB; > + mtk_dp->info.depth = DP_MSA_MISC_8_BPC; > + memset(&mtk_dp->info.timings, 0, sizeof(struct > mtk_dp_timings)); > + mtk_dp->info.timings.frame_rate = 60; > +} > + [snip] > + > +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp, > + struct drm_display_mode > *mode) > +{ > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > + > + drm_display_mode_to_videomode(mode, &timings->vm); > + timings->frame_rate = drm_mode_vrefresh(mode); > +} > +
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +static ssize_t mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) The caller never use the return value, so let this function to void. > +{ > + ssize_t ret; > + u8 sink_count; > + 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); You read sink_count but never use it, so this read is redundant. Remove it. > + if (ret < 1) { > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > + return ret == 0 ? -EIO : 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; > + } > + > + drm_dp_channel_eq_ok(link_status, mtk_dp- > >train_info.lane_count); This function just return true or false, and you does not process the return value, so this is redundant. Remove it. Regard, CK > + > + 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); > + > + return 0; > +}
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux, > + struct drm_dp_aux_msg *msg) > +{ > + struct mtk_dp *mtk_dp; > + bool is_read; > + u8 request; > + size_t accessed_bytes = 0; > + int ret = 0; > + > + mtk_dp = container_of(mtk_aux, struct mtk_dp, aux); > + > + if (!mtk_dp->train_info.cable_plugged_in) { > + ret = -EAGAIN; > + goto err; > + } > + > + switch (msg->request) { > + case DP_AUX_I2C_MOT: > + case DP_AUX_I2C_WRITE: > + case DP_AUX_NATIVE_WRITE: > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > + case DP_AUX_I2C_WRITE_STATUS_UPDATE | DP_AUX_I2C_MOT: > + request = msg->request & > ~DP_AUX_I2C_WRITE_STATUS_UPDATE; > + is_read = false; > + break; > + case DP_AUX_I2C_READ: > + case DP_AUX_NATIVE_READ: > + case DP_AUX_I2C_READ | DP_AUX_I2C_MOT: > + request = msg->request; > + is_read = true; > + break; > + default: > + drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n", > + msg->request); > + ret = -EINVAL; > + goto err; > + } > + > + if (msg->size == 0) { > + ret = mtk_dp_aux_do_transfer(mtk_dp, is_read, request, > + msg->address + > accessed_bytes, > + msg->buffer + > accessed_bytes, 0); > + } else { > + while (accessed_bytes < msg->size) { > + size_t to_access = > + min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES, > + msg->size - accessed_bytes); > + > + ret = mtk_dp_aux_do_transfer(mtk_dp, is_read, > request, > + msg->address + > accessed_bytes, > + msg->buffer + > accessed_bytes, > + to_access); > + > + if (ret) { > + drm_info(mtk_dp->drm_dev, > + "Failed to do AUX transfer: > %d\n", ret); > + break; > + } > + accessed_bytes += to_access; > + } > + } This if-else could be simplified as: do { size_t to_access = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES, msg->size - accessed_bytes); ret = mtk_dp_aux_do_transfer(mtk_dp, is_read, request, msg->address + accessed_bytes, msg->buffer + accessed_bytes, to_access); if (ret) { drm_info(mtk_dp->drm_dev, "Failed to do AUX transfer: %d\n", ret); break; } accessed_bytes += to_access; } while (accessed_bytes < msg->size); Regards, CK > +err: > + if (ret) { > + msg->reply = DP_AUX_NATIVE_REPLY_NACK | > DP_AUX_I2C_REPLY_NACK; > + return ret; > + } > + > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > + return msg->size; > +} > +
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +static int mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 > offset, u8 *buf, > + size_t length) The caller does not process the return value, so let this function to be void. Regards, CK > +{ > + int i, ret; > + int num_regs = (length + 1) / 2; > + > + /* 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); > + > + ret = mtk_dp_write(mtk_dp, offset + i * 4, val); > + if (ret) > + return ret; > + } > + > + return 0; > +} > +
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +static void mtk_dp_video_enable(struct mtk_dp *mtk_dp, bool enable) > +{ > + if (enable) { > + mtk_dp_set_tx_out(mtk_dp); > + mtk_dp_video_mute(mtk_dp, false); > + } else { > + mtk_dp_video_mute(mtk_dp, true); In mtk_dp_set_tx_out(), disable some function. Why not enable that function here? Regards, CK > + } > +} > +
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > +/* > + * We need to handle HPD signal in eDP even though eDP is a always > connected > + * device. Besides connected status, there is another feature for > HPD signal - > + * HPD pulse: it presents an IRQ from sink devices to source devices > (Refer to > + * 5.1.4 of DP1.4 spec). > + */ > +static irqreturn_t mtk_dp_hpd_isr_handler(struct mtk_dp *mtk_dp) > +{ > + bool connected; > + bool hpd_connect_sta; > + u32 irq_status = mtk_dp_swirq_get_clear(mtk_dp) | > + mtk_dp_hwirq_get_clear(mtk_dp); > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + > + if (irq_status & MTK_DP_HPD_INTERRUPT) > + train_info->irq_sta.hpd_inerrupt = true; > + if (irq_status & MTK_DP_HPD_CONNECT) > + hpd_connect_sta = true; > + if (irq_status & MTK_DP_HPD_DISCONNECT) > + train_info->irq_sta.hpd_disconnect = true; hpd_disconnect is used only in this function, so let it be local variable. > + > + if (!irq_status) > + return IRQ_HANDLED; Move this to top of this function. > + > + connected = mtk_dp_plug_state(mtk_dp); > + if (connected || !train_info->cable_plugged_in) > + train_info->irq_sta.hpd_disconnect = false; The truth table of (irq_status & MTK_DP_HPD_DISCONNECT, connected, cable_plugged_in, hpd_disconnect) is 0 0 0 0 0 0 1 0 0 1 0 0 0 1 1 0 1 0 0 0 1 0 1 1 1 1 0 0 1 1 1 0 So the only case that hpd_disconnect is true is (irq_status & MTK_DP_HPD_DISCONNECT) && !connected && train_info- >cable_plugged_in) And train_info->cable_plugged_in is the previous status. The previous status is connected. And irq_status and connected is the new status. The new status is disconnected. I have a question. Why we need both irq_status and connected for new status? I think connected is enough for new status, so we could ignore irq_status. Regards, CK > + else if (!connected || train_info->cable_plugged_in) > + hpd_connect_sta = false; > + > + if (!(hpd_connect_sta || train_info->irq_sta.hpd_disconnect)) > + return IRQ_WAKE_THREAD; > + > + if (hpd_connect_sta) { > + hpd_connect_sta = false; > + train_info->cable_plugged_in = true; > + } else { > + train_info->irq_sta.hpd_disconnect = false; > + train_info->cable_plugged_in = false; > + } > + train_info->cable_state_change = true; > + > + return IRQ_WAKE_THREAD; > +} > +
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +static int 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) { Drop the switch because color_depth is always DP_MSA_MISC_8_BPC. Regards, CK > + case DP_MSA_MISC_8_BPC: > + val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT; > + break; > + default: > + drm_warn(mtk_dp->drm_dev, "Unsupported color depth > %d\n", > + color_depth); > + return -EINVAL; > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); > + return 0; > +}
Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +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> > --- [snip] > + > +static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp, > + enum dp_pixelformat color_format) > +{ > + u32 val; > + > + /* 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_RGB: > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; > + break; > + default: The default case would never happen, remove it. Regards, CK > + drm_warn(mtk_dp->drm_dev, "Unsupported color format: > %d\n", > + color_format); > + return -EINVAL; > + } > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > + val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); > + return 0; > +} > +
On Thu, 2022-07-07 at 16:00 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Fri, 2022-07-01 at 14:28 +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> > > --- > > [snip] > > > + > > +static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp, > > + enum dp_pixelformat color_format) > > +{ > > + u32 val; > > + > > + /* 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_RGB: > > + val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB; > > + break; > > + default: > > The default case would never happen, remove it. > > Regards, > CK > Hello CK, after removing default, it will build error because we do not handle other 5 enum in enum dp_pixelformat. "error: 5 enumeration values not handled in switch" Therefore, I will keep this. BRs, Bo-Chen > > + drm_warn(mtk_dp->drm_dev, "Unsupported color format: > > %d\n", > > + color_format); > > + return -EINVAL; > > + } > > + > > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > > + val, PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK); > > + return 0; > > +} > > + > >
On Thu, 2022-07-07 at 13:14 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Fri, 2022-07-01 at 14:28 +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> > > --- > > [snip] > > > + > > +static ssize_t mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > > The caller never use the return value, so let this function to void. > > > +{ > > + ssize_t ret; > > + u8 sink_count; > > + 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); > > You read sink_count but never use it, so this read is redundant. > Remove > it. > Hello CK, this is a pre-request for the following codes, so I think we need to keep this. If we failed to read sink_count, we don't need to do the driver under this. > > + if (ret < 1) { > > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > > + return ret == 0 ? -EIO : 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; > > + } > > + > > + drm_dp_channel_eq_ok(link_status, mtk_dp- > > > train_info.lane_count); > > This function just return true or false, and you does not process the > return value, so this is redundant. Remove it. > I will handle this in next version. BRs, Bo-Chen > Regard, > CK > > > + > > + 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); > > + > > + return 0; > > +} > >
On Thu, 2022-07-07 at 10:21 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Fri, 2022-07-01 at 14:28 +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> > > --- > > + > > +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); > > clk-mt8195-vdo0 driver [1] is part of mtk-mmsys driver [2] and it is > still separated out to ccf driver. In addition, you does not manage > the > parent clock. If the parent clock is not enable, these leaf clock > would > not work. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/mediatek/clk-mt8195-vdo0.c?h=v5.19-rc5#n138 > > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/mediatek/mtk-mmsys.c?h=v5.19-rc5#n140 > > Regards, > CK > Hello CK, MTK_DP_0034 is just a enable control for dp hardware, so I think we don't need to move it to ccf driver. it's not related to ccf. After checking with Jitao, we only need to update DA_CKM_CKTX0_EN_FORCE_E. I will set this bit as 1 in mtk_dp_power_disable and 0 in mtk_dp_power_enable 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)); > > +} > > + > >