| Message ID | 20220712111223.13080-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add MT8195 DisplayPort driver | expand |
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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; > + > + 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); You set cable_state_change to true in isr handler just to update a register in isr thread. I think you could just update this register in isr handler and drop cable_state_change. 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 Tue, 2022-07-12 at 19:12 +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; > +}; > + > +struct mtk_dp_irq_sta { > + 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; There is only one member in struct mtk_dp_irq_sta, so drop struct mtk_dp_irq_sta and use bool hpd_inerrupt directly here. > +}; > + > +struct mtk_dp_info { > + u32 depth; > + enum dp_pixelformat format; > + struct mtk_dp_timings timings; There is only one member in struct mtk_dp_timings, so drop struct mtk_dp_timings and use struct videomode vm directly here. Regards, CK > +}; > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_bridge_atomic_check(struct drm_bridge *bridge, > + struct drm_bridge_state > *bridge_state, > + struct drm_crtc_state > *crtc_state, > + struct drm_connector_state > *conn_state) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + struct drm_crtc *crtc = conn_state->crtc; > + unsigned int input_bus_format; > + > + input_bus_format = bridge_state->input_bus_cfg.format; > + > + dev_dbg(mtk_dp->dev, "input format 0x%04x, output format > 0x%04x\n", > + bridge_state->input_bus_cfg.format, > + bridge_state->output_bus_cfg.format); > + > + if (input_bus_format == MEDIA_BUS_FMT_YUYV8_1X16) > + mtk_dp->info.format = DP_PIXELFORMAT_YUV422; > + else > + mtk_dp->info.format = DP_PIXELFORMAT_RGB; > + > + if (!crtc) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state doesn't > have a crtc\n"); > + return -EINVAL; > + } > + > + mtk_dp_parse_drm_mode_timings(mtk_dp, &crtc_state- > >adjusted_mode); > + if (mtk_dp_parse_capabilities(mtk_dp)) { mtk_dp_bridge_atomic_enable() would call mtk_dp_parse_capabilities(), so this is redundant. Regards, CK > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as nothing is plugged > in\n"); > + return -EINVAL; > + } > + > + return 0; > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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) > +{ > + /* Only support 8 bits currently */ > + mtk_dp->info.depth = DP_MSA_MISC_8_BPC; Only support DP_MSA_MISC_8_BPC, so it's not necessary use a variable to store this information. Drop depth. Regards, CK > + > + /* Update MISC0 */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + DP_MSA_MISC_8_BPC, DP_TEST_BIT_DEPTH_MASK); > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); > + return 0; > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_msa_bypass_enable(struct mtk_dp *mtk_dp, bool > enable) > +{ > + u32 mask = BIT(HTOTAL_SEL_DP_ENC0_P0_SHIFT) | > + BIT(VTOTAL_SEL_DP_ENC0_P0_SHIFT) | > + BIT(HSTART_SEL_DP_ENC0_P0_SHIFT) | > + BIT(VSTART_SEL_DP_ENC0_P0_SHIFT) | > + BIT(HWIDTH_SEL_DP_ENC0_P0_SHIFT) | > + BIT(VHEIGHT_SEL_DP_ENC0_P0_SHIFT) | > + BIT(HSP_SEL_DP_ENC0_P0_SHIFT) | > + BIT(HSW_SEL_DP_ENC0_P0_SHIFT) | > + BIT(VSP_SEL_DP_ENC0_P0_SHIFT) | > + BIT(VSW_SEL_DP_ENC0_P0_SHIFT); I would like define a symbol like this #define HTOTAL_SEL_DP_ENC0_P0 BIT(0) #define VTOTAL_SEL_DP_ENC0_P0 BIT(1) #define HSTART_SEL_DP_ENC0_P0 BIT(2) Regards, CK > + u32 val = enable ? 0 : mask; > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3030, val, mask); > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 > offset, u8 *buf, > + size_t length) The offset would always be MTK_DP_AUX_P0_3708, so drop offset and use MTK_DP_AUX_P0_3708 directly. > +{ > + int i; > + 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); > + > + if (mtk_dp_write(mtk_dp, offset + i * 4, val)) > + return; > + } for (i = 0; i < length; i += 2) { val = buf[i] | (i + 1 < length ? buf[i + 1] << 8 : 0); if (mtk_dp_write(mtk_dp, MTK_DP_AUX_P0_3708 + i * 2, val)) return; } Regards, CK > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_aux_fill_write_fifo(struct mtk_dp *mtk_dp, u8 > *buf, > + size_t length) > +{ > + mtk_dp_bulk_16bit_write(mtk_dp, MTK_DP_AUX_P0_3708, buf, > length); mtk_dp_aux_fill_write_fifo() directly call mtk_dp_bulk_16bit_write(), so I think we could just keep one of them and drop another one. Regards, CK > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_set_msa(struct mtk_dp *mtk_dp) > +{ > + struct drm_display_mode mode; > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > + > + drm_display_mode_from_videomode(&timings->vm, &mode); > + > + /* horizontal */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3010, > + mode.htotal, HTOTAL_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3018, > + timings->vm.hsync_len + timings- > >vm.hback_porch, > + HSTART_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3028, > + timings->vm.hsync_len << > HSW_SW_DP_ENC0_P0_SHIFT, Directly use a number for shift because we know it's a shift, so it's not necessary to define a symbol for shift. > + HSW_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3028, > + 0, HSP_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3020, > + timings->vm.hactive, > HWIDTH_SW_DP_ENC0_P0_MASK); > + > + /* vertical */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3014, > + mode.vtotal, VTOTAL_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_301C, > + timings->vm.vsync_len + timings- > >vm.vback_porch, > + VSTART_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_302C, > + timings->vm.vsync_len << > VSW_SW_DP_ENC0_P0_SHIFT, Ditto. Regards, CK > + VSW_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_302C, > + 0, VSP_SW_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3024, > + timings->vm.vactive, > VHEIGHT_SW_DP_ENC0_P0_MASK); > + > + /* horizontal */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3064, > + timings->vm.hactive, > HDE_NUM_LAST_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3154, > + mode.htotal, PGEN_HTOTAL_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3158, > + timings->vm.hfront_porch, > + PGEN_HSYNC_RISING_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_315C, > + timings->vm.hsync_len, > + PGEN_HSYNC_PULSE_WIDTH_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3160, > + timings->vm.hback_porch + timings- > >vm.hsync_len, > + PGEN_HFDE_START_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3164, > + timings->vm.hactive, > + PGEN_HFDE_ACTIVE_WIDTH_DP_ENC0_P0_MASK); > + > + /* vertical */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3168, > + mode.vtotal, > + PGEN_VTOTAL_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_316C, > + timings->vm.vfront_porch, > + PGEN_VSYNC_RISING_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3170, > + timings->vm.vsync_len, > + PGEN_VSYNC_PULSE_WIDTH_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3174, > + timings->vm.vback_porch + timings- > >vm.vsync_len, > + PGEN_VFDE_START_DP_ENC0_P0_MASK); > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3178, > + timings->vm.vactive, > + PGEN_VFDE_ACTIVE_WIDTH_DP_ENC0_P0_MASK); > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_train_tps_2_3(struct mtk_dp *mtk_dp, u8 > target_linkrate, > + u8 target_lane_count, int > *iteration_count, > + u8 *lane_adjust, int *status_control, > + u8 *prev_lane_adjust) > +{ > + u8 val; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + > + if (*status_control == 1) { > + if (mtk_dp->train_info.tps4) { > + mtk_dp_train_set_pattern(mtk_dp, 4); > + val = DP_TRAINING_PATTERN_4; > + } else if (mtk_dp->train_info.tps3) { > + mtk_dp_train_set_pattern(mtk_dp, 3); > + val = DP_LINK_SCRAMBLING_DISABLE | > + DP_TRAINING_PATTERN_3; > + } else { > + mtk_dp_train_set_pattern(mtk_dp, 2); > + val = DP_LINK_SCRAMBLING_DISABLE | > + DP_TRAINING_PATTERN_2; > + } > + drm_dp_dpcd_writeb(&mtk_dp->aux, > + DP_TRAINING_PATTERN_SET, val); > + drm_dp_dpcd_read(&mtk_dp->aux, > + DP_ADJUST_REQUEST_LANE0_1, > lane_adjust, > + sizeof(*lane_adjust) * 2); > + > + mtk_dp_train_update_swing_pre(mtk_dp, > + target_lane_count, > lane_adjust); > + *status_control = 2; > + (*iteration_count)++; > + } > + > + drm_dp_link_train_channel_eq_delay(&mtk_dp->aux, mtk_dp- > >rx_cap); > + > + drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); > + > + if (!drm_dp_clock_recovery_ok(link_status, target_lane_count)) I think this checking is redundant. I think we could just keep drm_dp_channel_eq_ok() and drop drm_dp_clock_recovery_ok() here because if drm_dp_clock_recovery_ok() fail, it imply that drm_dp_channel_eq_ok() would fail. So just check drm_dp_channel_eq_ok() is enough. Regards, CK > { > + mtk_dp->train_info.cr_done = false; > + mtk_dp->train_info.eq_done = false; > + dev_dbg(mtk_dp->dev, "Link train EQ fail\n"); > + return -EINVAL; > + } > + > + if (drm_dp_channel_eq_ok(link_status, target_lane_count)) { > + mtk_dp->train_info.eq_done = true; > + dev_dbg(mtk_dp->dev, "Link train EQ pass\n"); > + return 0; > + } > + > + if (*prev_lane_adjust == link_status[4]) > + (*iteration_count)++; > + else > + *prev_lane_adjust = link_status[4]; > + > + return -EAGAIN; > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_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 ret; > + > + dev_dbg(mtk_dp->dev, > + "Link train target_link_rate = 0x%x, target_lane_count > = 0x%x\n", > + target_link_rate, target_lane_count); > + > + do { > + train_retries++; > + if (!mtk_dp->train_info.cable_plugged_in) > + return -ENODEV; > + > + if (!pass_tps1) { > + ret = mtk_dp_train_tps_1(mtk_dp, > target_lane_count, > + &iteration_count, > lane_adjust, > + &status_control, > + &prev_lane_adjust); > + if (!ret) { > + pass_tps1 = true; > + train_retries = 0; > + } else if (ret == -EINVAL) { > + break; > + } > + } else { > + ret = mtk_dp_train_tps_2_3(mtk_dp, > target_link_rate, > + target_lane_count, > + &iteration_count, > + lane_adjust, > &status_control, > + &prev_lane_adjust); > + if (!ret) { > + pass_tps2_3 = true; > + break; > + } else if (ret == -EINVAL) { > + break; > + } > + } > + > + drm_dp_dpcd_read(&mtk_dp->aux, > DP_ADJUST_REQUEST_LANE0_1, > + lane_adjust, sizeof(lane_adjust)); > + mtk_dp_train_update_swing_pre(mtk_dp, > target_lane_count, > + lane_adjust); > + } while (train_retries < MTK_DP_TRAIN_RETRY_LIMIT && > + iteration_count < MTK_DP_TRAIN_MAX_ITERATIONS); train_retries and iteration_count are the same thing, so keep one and drop another one. Regards, CK > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_TRAINING_PATTERN_SET, > + DP_TRAINING_PATTERN_DISABLE); > + mtk_dp_train_set_pattern(mtk_dp, 0); > + > + if (!pass_tps2_3) > + return -ETIMEDOUT; > + > + mtk_dp->train_info.link_rate = target_link_rate; > + mtk_dp->train_info.lane_count = target_lane_count; > + > + mtk_dp_training_set_scramble(mtk_dp, true); > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LANE_COUNT_SET, > + target_lane_count | > + DP_LANE_COUNT_ENHANCED_FRAME_EN); > + mtk_dp_set_enhanced_frame_mode(mtk_dp, true); > + > + return ret; > +} > +
On Wed, 2022-07-13 at 16:10 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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; > > +}; > > + > > +struct mtk_dp_irq_sta { > > + 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; > > There is only one member in struct mtk_dp_irq_sta, so drop struct > mtk_dp_irq_sta and use bool hpd_inerrupt directly here. > Hello CK, ok, I will drop this. > > +}; > > + > > +struct mtk_dp_info { > > + u32 depth; > > + enum dp_pixelformat format; > > + struct mtk_dp_timings timings; > > There is only one member in struct mtk_dp_timings, so drop struct > mtk_dp_timings and use struct videomode vm directly here. > This structure will add more variable in following patch. whole struct is like, struct mtk_dp_timings { struct videomode vm; u8 frame_rate; u32 pix_rate_khz; }; I want to keep this. BRs, Bo-Chen > Regards, > CK > > > +}; > > + > >
On Wed, 2022-07-13 at 17:31 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 > > offset, u8 *buf, > > + size_t length) > > The offset would always be MTK_DP_AUX_P0_3708, so drop offset and use > MTK_DP_AUX_P0_3708 directly. > Hello CK, I don't think it's a good idea. this function is a fucntion of writing registers. I want to keep the offset variable. > > +{ > > + int i; > > + 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); > > + > > + if (mtk_dp_write(mtk_dp, offset + i * 4, val)) > > + return; > > + } > > for (i = 0; i < length; i += 2) { > val = buf[i] | (i + 1 < length ? buf[i + 1] << 8 : 0); > if (mtk_dp_write(mtk_dp, MTK_DP_AUX_P0_3708 + i * 2, val)) > return; > } > ok. BRs, Bo-Chen > Regards, > CK > > > +} > > + > >
On Wed, 2022-07-13 at 17:33 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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_aux_fill_write_fifo(struct mtk_dp *mtk_dp, u8 > > *buf, > > + size_t length) > > +{ > > + mtk_dp_bulk_16bit_write(mtk_dp, MTK_DP_AUX_P0_3708, buf, > > length); > > mtk_dp_aux_fill_write_fifo() directly call mtk_dp_bulk_16bit_write(), > so I think we could just keep one of them and drop another one. > Hello CK, mtk_dp_bulk_16bit_write() will also be used in following patches. I want to keep the driver like this. BRs, Bo-Chen > Regards, > CK > > > +} > > + > >
On Thu, 2022-07-14 at 14:51 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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_train_tps_2_3(struct mtk_dp *mtk_dp, u8 > > target_linkrate, > > + u8 target_lane_count, int > > *iteration_count, > > + u8 *lane_adjust, int *status_control, > > + u8 *prev_lane_adjust) > > +{ > > + u8 val; > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > + > > + if (*status_control == 1) { > > + if (mtk_dp->train_info.tps4) { > > + mtk_dp_train_set_pattern(mtk_dp, 4); > > + val = DP_TRAINING_PATTERN_4; > > + } else if (mtk_dp->train_info.tps3) { > > + mtk_dp_train_set_pattern(mtk_dp, 3); > > + val = DP_LINK_SCRAMBLING_DISABLE | > > + DP_TRAINING_PATTERN_3; > > + } else { > > + mtk_dp_train_set_pattern(mtk_dp, 2); > > + val = DP_LINK_SCRAMBLING_DISABLE | > > + DP_TRAINING_PATTERN_2; > > + } > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > + DP_TRAINING_PATTERN_SET, val); > > + drm_dp_dpcd_read(&mtk_dp->aux, > > + DP_ADJUST_REQUEST_LANE0_1, > > lane_adjust, > > + sizeof(*lane_adjust) * 2); > > + > > + mtk_dp_train_update_swing_pre(mtk_dp, > > + target_lane_count, > > lane_adjust); > > + *status_control = 2; > > + (*iteration_count)++; > > + } > > + > > + drm_dp_link_train_channel_eq_delay(&mtk_dp->aux, mtk_dp- > > > rx_cap); > > > > + > > + drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); > > + > > + if (!drm_dp_clock_recovery_ok(link_status, target_lane_count)) > > I think this checking is redundant. I think we could just keep > drm_dp_channel_eq_ok() and drop drm_dp_clock_recovery_ok() here > because > if drm_dp_clock_recovery_ok() fail, it imply that > drm_dp_channel_eq_ok() would fail. So just check > drm_dp_channel_eq_ok() > is enough. > > Regards, > CK > > > { > > + mtk_dp->train_info.cr_done = false; > > + mtk_dp->train_info.eq_done = false; > > + dev_dbg(mtk_dp->dev, "Link train EQ fail\n"); > > + return -EINVAL; > > + } > > + > > + if (drm_dp_channel_eq_ok(link_status, target_lane_count)) { > > + mtk_dp->train_info.eq_done = true; > > + dev_dbg(mtk_dp->dev, "Link train EQ pass\n"); > > + return 0; > > + } > > + Hello CK, do you mean like this? if (drm_dp_channel_eq_ok(link_status, target_lane_count)) { mtk_dp- >train_info.eq_done = true; dev_dbg(mtk_dp->dev, "Link train EQ pass\n"); return 0; } else { mtk_dp->train_info.cr_done = false; mtk_dp->train_info.eq_done = false; dev_dbg(mtk_dp->dev, "Link train EQ fail\n"); return -EINVAL; } BRs, Bo-Chen > > + if (*prev_lane_adjust == link_status[4]) > > + (*iteration_count)++; > > + else > > + *prev_lane_adjust = link_status[4]; > > + > > + return -EAGAIN; > > +} > > + > >
Hi, Bo-Chen: On Thu, 2022-07-14 at 16:24 +0800, Rex-BC Chen wrote: > On Wed, 2022-07-13 at 16:10 +0800, CK Hu wrote: > > Hi, Bo-Chen: > > > > On Tue, 2022-07-12 at 19:12 +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; > > > +}; > > > + > > > +struct mtk_dp_irq_sta { > > > + 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; > > > > There is only one member in struct mtk_dp_irq_sta, so drop struct > > mtk_dp_irq_sta and use bool hpd_inerrupt directly here. > > > > Hello CK, > > ok, I will drop this. > > > > +}; > > > + > > > +struct mtk_dp_info { > > > + u32 depth; > > > + enum dp_pixelformat format; > > > + struct mtk_dp_timings timings; > > > > There is only one member in struct mtk_dp_timings, so drop struct > > mtk_dp_timings and use struct videomode vm directly here. > > > > This structure will add more variable in following patch. > whole struct is like, > struct mtk_dp_timings { > struct videomode vm; > u8 frame_rate; > u32 pix_rate_khz; > }; > > I want to keep this. I think we could just drop struct mtk_dp_timings and place these member directly in struct mtk_dp_info. Regards, CK > > BRs, > Bo-Chen > > > > Regards, > > CK > > > > > +}; > > > + > > > > > >
Hi, Bo-Chen: On Thu, 2022-07-14 at 17:09 +0800, Rex-BC Chen wrote: > On Thu, 2022-07-14 at 14:51 +0800, CK Hu wrote: > > Hi, Bo-Chen: > > > > On Tue, 2022-07-12 at 19:12 +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_train_tps_2_3(struct mtk_dp *mtk_dp, u8 > > > target_linkrate, > > > + u8 target_lane_count, int > > > *iteration_count, > > > + u8 *lane_adjust, int *status_control, > > > + u8 *prev_lane_adjust) > > > +{ > > > + u8 val; > > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > > + > > > + if (*status_control == 1) { > > > + if (mtk_dp->train_info.tps4) { > > > + mtk_dp_train_set_pattern(mtk_dp, 4); > > > + val = DP_TRAINING_PATTERN_4; > > > + } else if (mtk_dp->train_info.tps3) { > > > + mtk_dp_train_set_pattern(mtk_dp, 3); > > > + val = DP_LINK_SCRAMBLING_DISABLE | > > > + DP_TRAINING_PATTERN_3; > > > + } else { > > > + mtk_dp_train_set_pattern(mtk_dp, 2); > > > + val = DP_LINK_SCRAMBLING_DISABLE | > > > + DP_TRAINING_PATTERN_2; > > > + } > > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > > + DP_TRAINING_PATTERN_SET, val); > > > + drm_dp_dpcd_read(&mtk_dp->aux, > > > + DP_ADJUST_REQUEST_LANE0_1, > > > lane_adjust, > > > + sizeof(*lane_adjust) * 2); > > > + > > > + mtk_dp_train_update_swing_pre(mtk_dp, > > > + target_lane_count, > > > lane_adjust); > > > + *status_control = 2; > > > + (*iteration_count)++; > > > + } > > > + > > > + drm_dp_link_train_channel_eq_delay(&mtk_dp->aux, mtk_dp- > > > > rx_cap); > > > > > > + > > > + drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); > > > + > > > + if (!drm_dp_clock_recovery_ok(link_status, target_lane_count)) > > > > I think this checking is redundant. I think we could just keep > > drm_dp_channel_eq_ok() and drop drm_dp_clock_recovery_ok() here > > because > > if drm_dp_clock_recovery_ok() fail, it imply that > > drm_dp_channel_eq_ok() would fail. So just check > > drm_dp_channel_eq_ok() > > is enough. > > > > Regards, > > CK > > > > > { > > > + mtk_dp->train_info.cr_done = false; > > > + mtk_dp->train_info.eq_done = false; > > > + dev_dbg(mtk_dp->dev, "Link train EQ fail\n"); > > > + return -EINVAL; > > > + } > > > + > > > + if (drm_dp_channel_eq_ok(link_status, target_lane_count)) { > > > + mtk_dp->train_info.eq_done = true; > > > + dev_dbg(mtk_dp->dev, "Link train EQ pass\n"); > > > + return 0; > > > + } > > > + > > Hello CK, > > do you mean like this? > if (drm_dp_channel_eq_ok(link_status, target_lane_count)) { > mtk_dp- > > train_info.eq_done = true; > > dev_dbg(mtk_dp->dev, "Link train EQ pass\n"); > return 0; > } else { > mtk_dp->train_info.cr_done = false; > mtk_dp->train_info.eq_done = false; > dev_dbg(mtk_dp->dev, "Link train EQ fail\n"); > return -EINVAL; > } No, I mean just remove drm_dp_clock_recovery_ok() checking. When drm_dp_channel_eq_ok() fail, it should keep retry. If clock recovery has some problem, drm_dp_channel_eq_ok() would be finally out of retry count. Regards, CK > > BRs, > Bo-Chen > > > > + if (*prev_lane_adjust == link_status[4]) > > > + (*iteration_count)++; > > > + else > > > + *prev_lane_adjust = link_status[4]; > > > + > > > + return -EAGAIN; > > > +} > > > + > > > > > >
Il 12/07/22 13:12, Bo-Chen Chen ha scritto: > From: Guillaume Ranquet <granquet@baylibre.com> > > This patch adds two helper functions that extract the frequency and word > length from a struct cea_sad. > > For these helper functions new defines are added that help translate the > 'freq' and 'byte2' fields into real numbers. > > 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> > --- > drivers/gpu/drm/drm_edid.c | 73 ++++++++++++++++++++++++++++++++++++++ > include/drm/drm_edid.h | 14 ++++++++ > 2 files changed, 87 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index bc43e1b32092..79316d7f1fd8 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4916,6 +4916,79 @@ int drm_edid_to_speaker_allocation(const struct edid *edid, u8 **sadb) > } > EXPORT_SYMBOL(drm_edid_to_speaker_allocation); > > +/** > + * drm_cea_sad_get_sample_rate - Extract the sample rate from cea_sad > + * @sad: Pointer to the cea_sad struct > + * > + * Extracts the cea_sad frequency field and returns the sample rate in Hz. > + * > + * Return: Sample rate in Hz or a negative errno if parsing failed. > + */ > +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad) > +{ > + switch (sad->freq) { > + case DRM_CEA_SAD_FREQ_32KHZ: > + return 32000; > + case DRM_CEA_SAD_FREQ_44KHZ: > + return 44100; > + case DRM_CEA_SAD_FREQ_48KHZ: > + return 48000; > + case DRM_CEA_SAD_FREQ_88KHZ: > + return 88200; > + case DRM_CEA_SAD_FREQ_96KHZ: > + return 96000; > + case DRM_CEA_SAD_FREQ_176KHZ: > + return 176400; > + case DRM_CEA_SAD_FREQ_192KHZ: > + return 192000; > + default: > + return -EINVAL; > + } > +} > +EXPORT_SYMBOL(drm_cea_sad_get_sample_rate); > + > +static bool drm_cea_sad_is_pcm(const struct cea_sad *sad) > +{ > + switch (sad->format) { > + case HDMI_AUDIO_CODING_TYPE_PCM: > + return true; > + default: > + return false; > + } Are you sure that you need this helper? That's used only in one function... ...if you really need this one, though, I don't think that using a switch is the best option here. Unless anyone is against that (please, reason?), I would be for doing it like: return sad->format == HDMI_AUDIO_CODING_TYPE_PCM; Everything else looks good to me (and working, too). Cheers, Angelo
On Thu, 2022-07-14 at 13:12 +0200, AngeloGioacchino Del Regno wrote: > Il 12/07/22 13:12, Bo-Chen Chen ha scritto: > > From: Guillaume Ranquet <granquet@baylibre.com> > > > > This patch adds two helper functions that extract the frequency and > > word > > length from a struct cea_sad. > > > > For these helper functions new defines are added that help > > translate the > > 'freq' and 'byte2' fields into real numbers. > > > > 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> > > --- > > drivers/gpu/drm/drm_edid.c | 73 > > ++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_edid.h | 14 ++++++++ > > 2 files changed, 87 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_edid.c > > b/drivers/gpu/drm/drm_edid.c > > index bc43e1b32092..79316d7f1fd8 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -4916,6 +4916,79 @@ int drm_edid_to_speaker_allocation(const > > struct edid *edid, u8 **sadb) > > } > > EXPORT_SYMBOL(drm_edid_to_speaker_allocation); > > > > +/** > > + * drm_cea_sad_get_sample_rate - Extract the sample rate from > > cea_sad > > + * @sad: Pointer to the cea_sad struct > > + * > > + * Extracts the cea_sad frequency field and returns the sample > > rate in Hz. > > + * > > + * Return: Sample rate in Hz or a negative errno if parsing > > failed. > > + */ > > +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad) > > +{ > > + switch (sad->freq) { > > + case DRM_CEA_SAD_FREQ_32KHZ: > > + return 32000; > > + case DRM_CEA_SAD_FREQ_44KHZ: > > + return 44100; > > + case DRM_CEA_SAD_FREQ_48KHZ: > > + return 48000; > > + case DRM_CEA_SAD_FREQ_88KHZ: > > + return 88200; > > + case DRM_CEA_SAD_FREQ_96KHZ: > > + return 96000; > > + case DRM_CEA_SAD_FREQ_176KHZ: > > + return 176400; > > + case DRM_CEA_SAD_FREQ_192KHZ: > > + return 192000; > > + default: > > + return -EINVAL; > > + } > > +} > > +EXPORT_SYMBOL(drm_cea_sad_get_sample_rate); > > + > > +static bool drm_cea_sad_is_pcm(const struct cea_sad *sad) > > +{ > > + switch (sad->format) { > > + case HDMI_AUDIO_CODING_TYPE_PCM: > > + return true; > > + default: > > + return false; > > + } > > Are you sure that you need this helper? That's used only in one > function... > ...if you really need this one, though, I don't think that using a > switch > is the best option here. > > Unless anyone is against that (please, reason?), I would be for doing > it like: > > return sad->format == HDMI_AUDIO_CODING_TYPE_PCM; > > Everything else looks good to me (and working, too). > > Cheers, > Angelo Hello Angelo, I think you are right, in this case, we don't need this help function. I will merge this function into drm_cea_sad_get_uncompressed_word_length() BRs, Bo-Chen
Il 12/07/22 13:12, Bo-Chen Chen ha scritto: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > Similar to HDMI, DP uses audio infoframes as well which are structured > very similar to the HDMI ones. > > This patch adds a helper function to pack the HDMI audio infoframe for > DP, called hdmi_audio_infoframe_pack_for_dp(). > hdmi_audio_infoframe_pack_only() is split into two parts. One of them > packs the payload only and can be used for HDMI and DP. > > Also constify the frame parameter in hdmi_audio_infoframe_check() as > it is passed to hdmi_audio_infoframe_check_only() which expects a const. > > 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> > --- > drivers/video/hdmi.c | 82 +++++++++++++++++++++++++++--------- > include/drm/display/drm_dp.h | 2 + > include/linux/hdmi.h | 7 ++- > 3 files changed, 71 insertions(+), 20 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 947be761dfa4..86805d77cc86 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -21,6 +21,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include <drm/display/drm_dp.h> > #include <linux/bitops.h> > #include <linux/bug.h> > #include <linux/errno.h> > @@ -381,12 +382,34 @@ static int hdmi_audio_infoframe_check_only(const struct hdmi_audio_infoframe *fr > * > * Returns 0 on success or a negative error code on failure. > */ > -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame) > +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame) > { > return hdmi_audio_infoframe_check_only(frame); > } > EXPORT_SYMBOL(hdmi_audio_infoframe_check); > > +static void > +hdmi_audio_infoframe_pack_payload(const struct hdmi_audio_infoframe *frame, > + u8 *buffer) > +{ > + u8 channels; > + > + if (frame->channels >= 2) > + channels = frame->channels - 1; > + else > + channels = 0; > + > + buffer[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); > + buffer[1] = ((frame->sample_frequency & 0x7) << 2) | > + (frame->sample_size & 0x3); > + buffer[2] = frame->coding_type_ext & 0x1f; > + buffer[3] = frame->channel_allocation; > + buffer[4] = (frame->level_shift_value & 0xf) << 3; > + > + if (frame->downmix_inhibit) > + buffer[4] |= BIT(7); > +} > + > /** > * hdmi_audio_infoframe_pack_only() - write HDMI audio infoframe to binary buffer > * @frame: HDMI audio infoframe > @@ -404,7 +427,6 @@ EXPORT_SYMBOL(hdmi_audio_infoframe_check); > ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > void *buffer, size_t size) > { > - unsigned char channels; > u8 *ptr = buffer; > size_t length; > int ret; > @@ -420,28 +442,13 @@ ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > > memset(buffer, 0, size); > > - if (frame->channels >= 2) > - channels = frame->channels - 1; > - else > - channels = 0; > - > ptr[0] = frame->type; > ptr[1] = frame->version; > ptr[2] = frame->length; > ptr[3] = 0; /* checksum */ > > - /* start infoframe payload */ > - ptr += HDMI_INFOFRAME_HEADER_SIZE; > - > - ptr[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7); > - ptr[1] = ((frame->sample_frequency & 0x7) << 2) | > - (frame->sample_size & 0x3); > - ptr[2] = frame->coding_type_ext & 0x1f; > - ptr[3] = frame->channel_allocation; > - ptr[4] = (frame->level_shift_value & 0xf) << 3; > - > - if (frame->downmix_inhibit) > - ptr[4] |= BIT(7); > + hdmi_audio_infoframe_pack_payload(frame, > + ptr + HDMI_INFOFRAME_HEADER_SIZE); > > hdmi_infoframe_set_checksum(buffer, length); > > @@ -479,6 +486,43 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, > } > EXPORT_SYMBOL(hdmi_audio_infoframe_pack); > > +/** > + * hdmi_audio_infoframe_pack_for_dp - Pack a HDMI Audio infoframe for DisplayPort > + * > + * @frame: HDMI Audio infoframe > + * @sdp: secondary data packet for display port. This is filled with the > + * appropriate: data "This is filled with the appropriate data" ... well, that's pretty obvious, isn't it? You're describing that this function is filling sdp in the description, so you can just remove that part. Also, "Secondary data packet for DisplayPort", please. > + * @dp_version: Display Port version to be encoded in the header We're not meaning "a display port", but really "DisplayPort": please remove the space between "Display" and "Port" :-) (here and in the description below) > + * > + * Packs a HDMI Audio Infoframe to be sent over Display Port. This function > + * fills the secondary data packet to be used for Display Port. > + * > + * Return: Number of total written bytes or a negative errno on failure. > + */ > +ssize_t > +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame, > + struct dp_sdp *sdp, u8 dp_version) > +{ > + int ret; > + > + ret = hdmi_audio_infoframe_check(frame); > + if (ret) > + return ret; > + > + memset(sdp->db, 0, sizeof(sdp->db)); > + > + /* Secondary-data packet header */ > + sdp->sdp_header.HB0 = 0; > + sdp->sdp_header.HB1 = frame->type; > + sdp->sdp_header.HB2 = DP_SDP_AUDIO_INFOFRAME_HB2; > + sdp->sdp_header.HB3 = (dp_version & 0x3f) << 2; > + > + hdmi_audio_infoframe_pack_payload(frame, sdp->db); > + > + return frame->length + 4; What's this magic number 4 about? Please use a definition for that. > +} > +EXPORT_SYMBOL(hdmi_audio_infoframe_pack_for_dp); > + > /** > * hdmi_vendor_infoframe_init() - initialize an HDMI vendor infoframe > * @frame: HDMI vendor infoframe > diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h > index 9e3aff7e68bb..6c0871164771 100644 > --- a/include/drm/display/drm_dp.h > +++ b/include/drm/display/drm_dp.h > @@ -1536,6 +1536,8 @@ enum drm_dp_phy { > #define DP_SDP_VSC_EXT_CEA 0x21 /* DP 1.4 */ > /* 0x80+ CEA-861 infoframe types */ > > +#define DP_SDP_AUDIO_INFOFRAME_HB2 0x1b > + > /** > * struct dp_sdp_header - DP secondary data packet header > * @HB0: Secondary Data Packet ID > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index c8ec982ff498..2f4dcc8d060e 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -336,7 +336,12 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame, > void *buffer, size_t size); > ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame, > void *buffer, size_t size); > -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame); > +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame); > + > +struct dp_sdp; > +ssize_t > +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame, > + struct dp_sdp *sdp, u8 dp_version); > > enum hdmi_3d_structure { > HDMI_3D_STRUCTURE_INVALID = -1,
Il 12/07/22 13:12, 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> > --- > drivers/gpu/drm/mediatek/mtk_dp.c | 723 ++++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 2 + > 2 files changed, 725 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > index 5ab646bd2bc4..fa7bb102a105 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c ..snip.. > @@ -2082,6 +2693,104 @@ static void mtk_dp_debounce_timer(struct timer_list *t) > mtk_dp->need_debounce = true; > } > > +/* > + * HDMI audio codec callbacks > + */ > +static int mtk_dp_audio_hw_params(struct device *dev, void *data, > + struct hdmi_codec_daifmt *daifmt, > + struct hdmi_codec_params *params) > +{ > + struct mtk_dp *mtk_dp = dev_get_drvdata(dev); > + struct mtk_dp_audio_cfg cfg; > + > + if (!mtk_dp->enabled) { > + pr_err("%s, DP is not ready!\n", __func__); drm_err() here please. > + return -ENODEV; > + } > + > + cfg.channels = params->cea.channels; > + cfg.sample_rate = params->sample_rate; > + cfg.word_length_bits = 24; > + > + mtk_dp_audio_setup(mtk_dp, &cfg); > + > + return 0; > +} > + ..snip.. > + > +static int mtk_dp_register_audio_driver(struct device *dev) > +{ > + struct mtk_dp *mtk_dp = dev_get_drvdata(dev); > + struct hdmi_codec_pdata codec_data = { > + .ops = &mtk_dp_audio_codec_ops, > + .max_i2s_channels = 8, > + .i2s = 1, > + .data = mtk_dp, > + }; > + struct platform_device *pdev; > + > + pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME, > + PLATFORM_DEVID_AUTO, &codec_data, > + sizeof(codec_data)); > + if (IS_ERR(pdev)) > + return PTR_ERR(pdev); You're never unregistering this device, which is unacceptable. Please add a platform device pointer in struct mtk_dp, so that this function becomes, simply: mtk_dp->audio_pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO, &codec_data, sizeof(codec_data)); return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev); > + > + return 0; > +} > + > static int mtk_dp_probe(struct platform_device *pdev) > { > struct mtk_dp *mtk_dp; > @@ -2127,8 +2836,21 @@ static int mtk_dp_probe(struct platform_device *pdev) > return dev_err_probe(dev, -EPROBE_DEFER, > "failed to request mediatek dptx irq\n"); > > + mutex_init(&mtk_dp->edid_lock); > + mutex_init(&mtk_dp->eld_lock); > + mutex_init(&mtk_dp->update_plugged_status_lock); > + > platform_set_drvdata(pdev, mtk_dp); > > + if (!mtk_dp_is_edp(mtk_dp)) { > + ret = mtk_dp_register_audio_driver(dev); > + if (ret) { > + dev_err(dev, "Failed to register audio driver: %d\n", > + ret); > + return ret; > + } > + } > + > mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek-dp-phy", > PLATFORM_DEVID_AUTO, > &mtk_dp->regs, > @@ -2174,6 +2896,7 @@ static int mtk_dp_remove(struct platform_device *pdev) > > platform_device_unregister(mtk_dp->phy_dev); ... and unregister it here: if (mtk_dp->audio_pdev) platform_device_unregister(mtk_dp->audio_pdev); > mtk_dp_video_mute(mtk_dp, true); > + mtk_dp_audio_mute(mtk_dp, true); > del_timer_sync(&mtk_dp->debounce_timer); > > pm_runtime_disable(&pdev->dev); Regards, Angelo
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_hpd_sink_event(struct mtk_dp *mtk_dp) > +{ > + 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); According to your last reply, if drm_dp_dpcd_readb() fail, we could skip below statement. So this just for error checking? If so, the next drm_dp_dpcd_read() would also do the error checking, so the checking here is redundant. Regards, CK > + if (ret < 1) { > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > + return; > + } > + > + drm_dbg(mtk_dp->drm_dev, > + "read sink count from dpcd: %d\n", sink_count); > + > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > link_status, > + sizeof(link_status)); > + if (!ret) { > + drm_err(mtk_dp->drm_dev, "Read link status failed\n"); > + return; > + } > + > + if (!drm_dp_channel_eq_ok(link_status, mtk_dp- > >train_info.lane_count)) { > + drm_err(mtk_dp->drm_dev, "Channel EQ failed\n"); > + return; > + } > + > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > + drm_dp_dpcd_writeb(&mtk_dp->aux, > DP_DEVICE_SERVICE_IRQ_VECTOR, > + DP_REMOTE_CONTROL_COMMAND_PENDING); > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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) This function just return 0, so let this function to be void. Regards, CK > +{ > + /* Only support 8 bits currently */ > + mtk_dp->info.depth = DP_MSA_MISC_8_BPC; > + > + /* Update MISC0 */ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034, > + DP_MSA_MISC_8_BPC, DP_TEST_BIT_DEPTH_MASK); > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT, > + VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK); > + return 0; > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_mn_overwrite_disable(struct mtk_dp *mtk_dp) > +{ > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, > + 0, VIDEO_M_CODE_SEL_DP_ENC0_P0_MASK); > +} Why has mtk_dp_mn_overwrite_disable() but no mtk_dp_mn_overwrite_enable()? Regards, CK > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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 enum drm_mode_status > +mtk_dp_bridge_mode_valid(struct drm_bridge *bridge, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + u32 rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000; > + u32 bpp = info->color_formats & DRM_COLOR_FORMAT_YCBCR422 ? 16 > : 24; > + > + if (rx_linkrate * mtk_dp->train_info.lane_count < mode->clock * > bpp / 8) > + return MODE_CLOCK_HIGH; > + > + if (mode->clock > 600000) > + return MODE_CLOCK_HIGH; > + > + if ((mode->clock * 1000) / (mode->htotal * mode->vtotal) > > + MTK_VDOSYS1_MAX_FRAMERATE) Why limit frame rate to 60fps? If the resolution is small enough, why not support higher fps? Regards, CK > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > +
On Fri, 2022-07-15 at 16:51 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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_hpd_sink_event(struct mtk_dp *mtk_dp) > > +{ > > + 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); > > According to your last reply, if drm_dp_dpcd_readb() fail, we could > skip below statement. So this just for error checking? If so, the > next > drm_dp_dpcd_read() would also do the error checking, so the checking > here is redundant. > > Regards, > CK > Hello CK, sorry, I don't get your point. We use "drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, &sink_count)" to get sink count and use "drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, link_status, sizeof(link_status));" to get link status. If we don't get any sink count, we don't need to check the link status. Therefore, we just return if we are failed to get sink count. BRs, Bo-Chen > > + if (ret < 1) { > > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > > + return; > > + } > > + > > + drm_dbg(mtk_dp->drm_dev, > > + "read sink count from dpcd: %d\n", sink_count); > > + > > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > > link_status, > > + sizeof(link_status)); > > + if (!ret) { > > + drm_err(mtk_dp->drm_dev, "Read link status failed\n"); > > + return; > > + } > > + > > + if (!drm_dp_channel_eq_ok(link_status, mtk_dp- > > > train_info.lane_count)) { > > > > + drm_err(mtk_dp->drm_dev, "Channel EQ failed\n"); > > + return; > > + } > > + > > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > + DP_REMOTE_CONTROL_COMMAND_PENDING); > > +} > > + > >
Hi, Rex: On Thu, 2022-07-21 at 10:38 +0800, Rex-BC Chen wrote: > On Fri, 2022-07-15 at 16:51 +0800, CK Hu wrote: > > Hi, Bo-Chen: > > > > On Tue, 2022-07-12 at 19:12 +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_hpd_sink_event(struct mtk_dp *mtk_dp) > > > +{ > > > + 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); > > > > According to your last reply, if drm_dp_dpcd_readb() fail, we could > > skip below statement. So this just for error checking? If so, the > > next > > drm_dp_dpcd_read() would also do the error checking, so the > > checking > > here is redundant. > > > > Regards, > > CK > > > > Hello CK, > > sorry, I don't get your point. > We use "drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, &sink_count)" > to get sink count and use "drm_dp_dpcd_read(&mtk_dp->aux, > link_status_reg, link_status, sizeof(link_status));" to get link > status. > > If we don't get any sink count, we don't need to check the link > status. > Therefore, we just return if we are failed to get sink count. I assume that when error happen, both read sink_count and read link_status would fail, so you could directly read link_status. Do you think that when error happen, only read sink_count would fail and read link_status would success? It it is the later case, we should keep the checking of sink_count. Regards, CK > > BRs, > Bo-Chen > > > > + if (ret < 1) { > > > + drm_err(mtk_dp->drm_dev, "Read sink count failed\n"); > > > + return; > > > + } > > > + > > > + drm_dbg(mtk_dp->drm_dev, > > > + "read sink count from dpcd: %d\n", sink_count); > > > + > > > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > > > link_status, > > > + sizeof(link_status)); > > > + if (!ret) { > > > + drm_err(mtk_dp->drm_dev, "Read link status failed\n"); > > > + return; > > > + } > > > + > > > + if (!drm_dp_channel_eq_ok(link_status, mtk_dp- > > > > train_info.lane_count)) { > > > > > > + drm_err(mtk_dp->drm_dev, "Channel EQ failed\n"); > > > + return; > > > + } > > > + > > > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > > + DP_REMOTE_CONTROL_COMMAND_PENDING); > > > +} > > > + > > > > > >
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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_training(struct mtk_dp *mtk_dp) > +{ > + short max_retry = 50; > + int ret; > + > + do { > + ret = mtk_dp_train_start(mtk_dp); > + if (!ret) > + break; > + else if (ret != -EAGAIN) > + return ret; > + } while (--max_retry); mtk_dp_train_start() would never return -EAGAIN, so drop this while loop. Regards, CK > > + if (!max_retry) > + return -ETIMEDOUT; > + > + ret = mtk_dp_video_config(mtk_dp); > + if (ret) > + return ret; > + mtk_dp_video_enable(mtk_dp, true); > + > + return 0; > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +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 hpd_change = false; > + 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) > + return IRQ_HANDLED; > + > + if (irq_status & MTK_DP_HPD_INTERRUPT) > + train_info->irq_sta.hpd_inerrupt = true; > + if (irq_status & MTK_DP_HPD_CONNECT || > + irq_status & MTK_DP_HPD_DISCONNECT) > + hpd_change = true; > + > + if (!(hpd_change)) > + return IRQ_WAKE_THREAD; > + > + if (mtk_dp_plug_state(mtk_dp)) mtk_dp_plug_state() is called only here, and prevent function call in isr handler, so squash mtk_dp_plug_state() into this function. > + train_info->cable_plugged_in = true; > + else > + train_info->cable_plugged_in = false; > + > + train_info->cable_state_change = true; > + > + return IRQ_WAKE_THREAD; > +} > + > +static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev) > +{ > + struct mtk_dp *mtk_dp = dev; > + u32 irq_status; > + > + irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS); > + > + if (!irq_status) > + return IRQ_HANDLED; > + > + if (irq_status & RGS_IRQ_STATUS_TRANSMITTER) > + return mtk_dp_hpd_isr_handler(mtk_dp); Prevent function call in isr handler, squash mtk_dp_hpd_isr_handler() into this function. Regards, CK > + > + return IRQ_HANDLED; > +} > +
Hi, Bo-Chen: On Tue, 2022-07-12 at 19:12 +0800, Bo-Chen Chen wrote: > From: Guillaume Ranquet <granquet@baylibre.com> > > This patch adds External DisplayPort support to the mt8195 eDP > driver. > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com> > --- [snip] > @@ -1489,13 +1543,34 @@ 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; > + > + event = mtk_dp_plug_state(mtk_dp) ? > + connector_status_connected : > connector_status_disconnected; > + > + if (event < 0) > + return IRQ_HANDLED; event is useless, so drop it. Regards, CK > + > + dev_dbg(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > + 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); > + 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); > + > + mtk_dp_update_bits(mtk_dp, > MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL, > + DP_PWR_STATE_MASK); > + } else { > + mtk_dp_update_bits(mtk_dp, > MTK_DP_TOP_PWR_STATE, > + DP_PWR_STATE_BANDGAP_TPLL_LA > NE, > + DP_PWR_STATE_MASK); > + } > } > > if (mtk_dp->train_info.irq_sta.hpd_inerrupt) { > @@ -1597,6 +1672,24 @@ static int mtk_dp_dt_parse(struct mtk_dp > *mtk_dp, > return 0; > } >
On Mon, 2022-07-25 at 17:23 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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 hpd_change = false; > > + 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) > > + return IRQ_HANDLED; > > + > > + if (irq_status & MTK_DP_HPD_INTERRUPT) > > + train_info->irq_sta.hpd_inerrupt = true; > > + if (irq_status & MTK_DP_HPD_CONNECT || > > + irq_status & MTK_DP_HPD_DISCONNECT) > > + hpd_change = true; > > + > > + if (!(hpd_change)) > > + return IRQ_WAKE_THREAD; > > + > > + if (mtk_dp_plug_state(mtk_dp)) > > mtk_dp_plug_state() is called only here, and prevent function call in > isr handler, so squash mtk_dp_plug_state() into this function. > Hello CK, Thanks for review. I would like to keep this because we will use this function for mtk_dp_plug_state_avoid_pulse() in dp patch. > > + train_info->cable_plugged_in = true; > > + else > > + train_info->cable_plugged_in = false; > > + > > + train_info->cable_state_change = true; > > + > > + return IRQ_WAKE_THREAD; > > +} > > + > > +static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev) > > +{ > > + struct mtk_dp *mtk_dp = dev; > > + u32 irq_status; > > + > > + irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS); > > + > > + if (!irq_status) > > + return IRQ_HANDLED; > > + > > + if (irq_status & RGS_IRQ_STATUS_TRANSMITTER) > > + return mtk_dp_hpd_isr_handler(mtk_dp); > > Prevent function call in isr handler, squash mtk_dp_hpd_isr_handler() > into this function. > Is this really necessary? We also modify this function in following patches. I think it's not a good idea to expand this. BRs, Bo-Chen > Regards, > CK > > > + > > + return IRQ_HANDLED; > > +} > > + > >
On Mon, 2022-07-25 at 17:16 +0800, CK Hu wrote: > Hi, Bo-Chen: > > On Tue, 2022-07-12 at 19:12 +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_training(struct mtk_dp *mtk_dp) > > +{ > > + short max_retry = 50; > > + int ret; > > + > > + do { > > + ret = mtk_dp_train_start(mtk_dp); > > + if (!ret) > > + break; > > + else if (ret != -EAGAIN) > > + return ret; > > + } while (--max_retry); > > mtk_dp_train_start() would never return -EAGAIN, so drop this while > loop. > > Regards, > CK > Hello CK, the function will not return -EAGAIN, but we still want to retry 50 times if mtk_dp_train_start() is failed. If we retry 50 times and it is still failed. We can confirm there are some issues for the device. I will remove the else if of -EAGAIN and keep th while loop. BRs, Bo-Chen > > > > + if (!max_retry) > > + return -ETIMEDOUT; > > + > > + ret = mtk_dp_video_config(mtk_dp); > > + if (ret) > > + return ret; > > + mtk_dp_video_enable(mtk_dp, true); > > + > > + return 0; > > +} > > + > >
On Tue, 2022-07-26 at 11:30 +0800, Rex-BC Chen wrote: > On Mon, 2022-07-25 at 17:23 +0800, CK Hu wrote: > > Hi, Bo-Chen: > > > > On Tue, 2022-07-12 at 19:12 +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 hpd_change = false; > > > + 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) > > > + return IRQ_HANDLED; > > > + > > > + if (irq_status & MTK_DP_HPD_INTERRUPT) > > > + train_info->irq_sta.hpd_inerrupt = true; > > > + if (irq_status & MTK_DP_HPD_CONNECT || > > > + irq_status & MTK_DP_HPD_DISCONNECT) > > > + hpd_change = true; > > > + > > > + if (!(hpd_change)) > > > + return IRQ_WAKE_THREAD; > > > + > > > + if (mtk_dp_plug_state(mtk_dp)) > > > > mtk_dp_plug_state() is called only here, and prevent function call > > in > > isr handler, so squash mtk_dp_plug_state() into this function. > > > > Hello CK, > > Thanks for review. > > I would like to keep this because we will use this function for > mtk_dp_plug_state_avoid_pulse() in dp patch. Use train_info->cable_plugged_in instead of calling mtk_dp_plug_state() because I think train_info->cable_plugged_in is synced with mtk_dp_plug_state(). > > > > + train_info->cable_plugged_in = true; > > > + else > > > + train_info->cable_plugged_in = false; > > > + > > > + train_info->cable_state_change = true; > > > + > > > + return IRQ_WAKE_THREAD; > > > +} > > > + > > > +static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev) > > > +{ > > > + struct mtk_dp *mtk_dp = dev; > > > + u32 irq_status; > > > + > > > + irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS); > > > + > > > + if (!irq_status) > > > + return IRQ_HANDLED; > > > + > > > + if (irq_status & RGS_IRQ_STATUS_TRANSMITTER) > > > + return mtk_dp_hpd_isr_handler(mtk_dp); > > > > Prevent function call in isr handler, squash > > mtk_dp_hpd_isr_handler() > > into this function. > > > > Is this really necessary? We also modify this function in following > patches. I think it's not a good idea to expand this. mtk_dp_hpd_isr_handler() is only called in this function, is it really necessary to separate this to a independent function? The function call would increase jump instruction and stack push/pop instruction. I think we should not do many things in isr handler. I've reviewed the later patch and the later patch should be modified according to this. Regards, CK > > BRs, > Bo-Chen > > > Regards, > > CK > > > > > + > > > + return IRQ_HANDLED; > > > +} > > > + > > > > > >
On Tue, 2022-07-26 at 14:42 +0800, Rex-BC Chen wrote: > On Mon, 2022-07-25 at 17:16 +0800, CK Hu wrote: > > Hi, Bo-Chen: > > > > On Tue, 2022-07-12 at 19:12 +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_training(struct mtk_dp *mtk_dp) > > > +{ > > > + short max_retry = 50; > > > + int ret; > > > + > > > + do { > > > + ret = mtk_dp_train_start(mtk_dp); > > > + if (!ret) > > > + break; > > > + else if (ret != -EAGAIN) > > > + return ret; > > > + } while (--max_retry); > > > > mtk_dp_train_start() would never return -EAGAIN, so drop this while > > loop. > > > > Regards, > > CK > > > > Hello CK, > > the function will not return -EAGAIN, but we still want to retry 50 > times if mtk_dp_train_start() is failed. If we retry 50 times and it > is > still failed. We can confirm there are some issues for the device. > > I will remove the else if of -EAGAIN and keep th while loop. In this version, it never retry. And I believe you've tested this no- retry version. If this no-retry version works fine, why do you insist on retry? If you really need retry, merge this retry into mtk_dp_train_start() because mtk_dp_train_start() have already retry. Regards, CK > > BRs, > Bo-Chen > > > > > > + if (!max_retry) > > > + return -ETIMEDOUT; > > > + > > > + ret = mtk_dp_video_config(mtk_dp); > > > + if (ret) > > > + return ret; > > > + mtk_dp_video_enable(mtk_dp, true); > > > + > > > + return 0; > > > +} > > > + > > > > > >
On Tue, 2022-07-26 at 17:34 +0800, CK Hu wrote: > On Tue, 2022-07-26 at 14:42 +0800, Rex-BC Chen wrote: > > On Mon, 2022-07-25 at 17:16 +0800, CK Hu wrote: > > > Hi, Bo-Chen: > > > > > > On Tue, 2022-07-12 at 19:12 +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_training(struct mtk_dp *mtk_dp) > > > > +{ > > > > + short max_retry = 50; > > > > + int ret; > > > > + > > > > + do { > > > > + ret = mtk_dp_train_start(mtk_dp); > > > > + if (!ret) > > > > + break; > > > > + else if (ret != -EAGAIN) > > > > + return ret; > > > > + } while (--max_retry); > > > > > > mtk_dp_train_start() would never return -EAGAIN, so drop this > > > while > > > loop. > > > > > > Regards, > > > CK > > > > > > > Hello CK, > > > > the function will not return -EAGAIN, but we still want to retry 50 > > times if mtk_dp_train_start() is failed. If we retry 50 times and > > it > > is > > still failed. We can confirm there are some issues for the device. > > > > I will remove the else if of -EAGAIN and keep th while loop. > > In this version, it never retry. And I believe you've tested this no- > retry version. If this no-retry version works fine, why do you insist > on retry? If you really need retry, merge this retry into > mtk_dp_train_start() because mtk_dp_train_start() have already retry. > > Regards, > CK > Hello Ck, There are many different devices we are not testing for DP devices. I think we need to keep this. This retry is for restart with init state. I think it's better to keep it here and it's more clear. I will remain the comments above, and I think it's enough. BRs, Bo-Chen > > > > BRs, > > Bo-Chen > > > > > > > > + if (!max_retry) > > > > + return -ETIMEDOUT; > > > > + > > > > + ret = mtk_dp_video_config(mtk_dp); > > > > + if (ret) > > > > + return ret; > > > > + mtk_dp_video_enable(mtk_dp, true); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > > > > > > >