| Message ID | 20220523104758.29531-1-granquet@baylibre.com |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add mt8195 DisplayPort driver | expand |
On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > We need a commit message here. > --- > include/drm/drm_edid.h | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 144c495b99c4..37c420423625 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -359,12 +359,18 @@ struct edid { > > #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] > << 8)) > > -/* Short Audio Descriptor */ > +/** > + * struct cea_sad - Short Audio Descriptor. > + * @format: See HDMI_AUDIO_CODING_TYPE_*. > + * @channels: max number of channels - 1. > + * @freq: See CEA_SAD_FREQ_*. > + * @byte2: meaning depends on format. > + */ > struct cea_sad { > u8 format; > - u8 channels; /* max number of channels - 1 */ > + u8 channels; > u8 freq; > - u8 byte2; /* meaning depends on format */ > + u8 byte2; > }; > > struct drm_encoder;
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > [snip] > + > +static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev) I don't have a DP spec. I find one in [1] but I'm not sure it's real spec or not. If it's real, in section 3.1.3.2, it describe: 3.1.3.2 Hot Plug/Unplug Detection One signal (HPD) is used by a device (an Upstream device) to detect that a Downstream port on the device has been connected to another device (the Downstream device). Implementation of HPD is optional for an embedded link configuration. At least a “trickle power” must be present both in the Upstream and Downstream devices for a Hot Plug event to be detected. I focus on the statement "Implementation of HPD is optional for an embedded link configuration". I'm not sure what does 'optional' mean. Does it mean eDP panel without HPD signal is possible? If so, I think driver should support eDP panel without HPD signal. Maybe I misunderstanding this spec. Please explain for me. Regards, CK [1] https://glenwing.github.io/docs/DP-1.2.pdf > +{ > + 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_disc > onnected; > + > + if (event < 0) > + return IRQ_HANDLED; > + > + if (mtk_dp->drm_dev) { > + dev_info(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->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + > + if (!mtk_dp->train_info.cable_plugged_in || > + !mtk_dp_plug_state(mtk_dp)) { > + 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) > + mtk_dp_fec_enable(mtk_dp, false); > + > + 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); > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + mtk_dp->train_info.link_rate = > + min_t(int, mtk_dp->max_linkrate, > + buf[mtk_dp->max_linkrate]); > + mtk_dp->train_info.lane_count = > + min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(buf)); > + } > + } > + > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > + mtk_dp_hpd_sink_event(mtk_dp); > + } > + > + return IRQ_HANDLED; > +} > +
Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > include/drm/drm_edid.h | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 144c495b99c4..37c420423625 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -359,12 +359,18 @@ struct edid { > > #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8)) > > -/* Short Audio Descriptor */ > +/** > + * struct cea_sad - Short Audio Descriptor. Perhaps.... * struct cea_sad - CEA Short Audio Descriptor ...but that's relative to personal liking and nothing else, it's also fine as it is, if you like it more as it is. The ball is yours :-P Regardless of any choice about changing the description or not: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers, Angelo > + * @format: See HDMI_AUDIO_CODING_TYPE_*. > + * @channels: max number of channels - 1. > + * @freq: See CEA_SAD_FREQ_*. > + * @byte2: meaning depends on format. > + */ > struct cea_sad { > u8 format; > - u8 channels; /* max number of channels - 1 */ > + u8 channels; > u8 freq; > - u8 byte2; /* meaning depends on format */ > + u8 byte2; > }; > > struct drm_encoder;
Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > 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> > --- > drivers/gpu/drm/drm_edid.c | 74 ++++++++++++++++++++++++++++++++++++++ > include/drm/drm_edid.h | 14 ++++++++ > 2 files changed, 88 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 561f53831e29..61ef1b1c972c 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -4758,6 +4758,80 @@ int drm_edid_to_speaker_allocation(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_uncompressed(const struct cea_sad *sad) > +{ > + switch (sad->format) { > + case HDMI_AUDIO_CODING_TYPE_STREAM: As far as I know, bit 0 is reserved, so HDMI_AUDIO_CODING_TYPE_STREAM should never occur here? > + case HDMI_AUDIO_CODING_TYPE_PCM: > + return true; > + default: > + return false; > + } > +} > + Also, I think that implementing a drm_cea_sad_get_compressed_max_bitrate() function should be pretty straightforward... the spec says that this is 8 bits, byte 3 (your byte2) contains the max bitrate divided by 8kHz, so to extract it, you read byte2 and multiply it by 8000Hz. /** * drm_cea_sad_get_compressed_max_bitrate - Extract maximum bitrate * @sad: Pointer to the cea_sad structure * * Extracts the cea_sad byte2 field and returns the maximum bit rate * of a compressed audio stream. * * Note: This function may only be called for compressed audio. * * Return: Maximum bitrate of compressed audio stream in bit/s or * negative number for error */ int drm_cea_sad_get_compressed_max_bitrate(const struct cea_sad *sad) { if (drm_cea_sad_is_uncompressed(sad)) { DRM_ERROR("Not supported: tried to get max bitrate for uncompressed format: %u\n", sad->format); return -EINVAL; } return sad->byte2 * 8000; } > +/** > + * drm_cea_sad_get_uncompressed_word_length - Extract word length > + * @sad: Pointer to the cea_sad struct > + * > + * Extracts the cea_sad byte2 field and returns the word length for an > + * uncompressed stream. > + * > + * Note: This function may only be called for uncompressed audio. > + * > + * Return: Word length in bits or a negative errno if parsing failed. > + */ > +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad *sad) > +{ > + if (!drm_cea_sad_is_uncompressed(sad)) { > + DRM_WARN("Unable to get the uncompressed word length for a compressed format: %u\n", > + sad->format); > + return -EINVAL; > + } > + > + switch (sad->byte2) { > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT: > + return 16; > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT: > + return 20; > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT: > + return 24; > + default: > + return -EINVAL; > + } > +} > +EXPORT_SYMBOL(drm_cea_sad_get_uncompressed_word_length); > + > /** > * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync delay > * @connector: connector associated with the HDMI/DP sink > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 37c420423625..7a939cb95b38 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -373,6 +373,18 @@ struct cea_sad { > u8 byte2; > }; > > +#define DRM_CEA_SAD_FREQ_32KHZ BIT(0) > +#define DRM_CEA_SAD_FREQ_44KHZ BIT(1) > +#define DRM_CEA_SAD_FREQ_48KHZ BIT(2) > +#define DRM_CEA_SAD_FREQ_88KHZ BIT(3) > +#define DRM_CEA_SAD_FREQ_96KHZ BIT(4) > +#define DRM_CEA_SAD_FREQ_176KHZ BIT(5) > +#define DRM_CEA_SAD_FREQ_192KHZ BIT(6) > + > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT BIT(0) > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT BIT(1) > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT BIT(2) > + > struct drm_encoder; > struct drm_connector; > struct drm_connector_state; > @@ -380,6 +392,8 @@ struct drm_display_mode; > > int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); > int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb); > +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad); > +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad *sad); > int drm_av_sync_delay(struct drm_connector *connector, > const struct drm_display_mode *mode); >
Il 23/05/22 12:47, Guillaume Ranquet 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> > --- > drivers/video/hdmi.c | 82 ++++++++++++++++++++++++++-------- > include/drm/dp/drm_dp_helper.h | 2 + this has been moved... again... this time it's include/drm/display/drm_dp_helper.h > include/linux/hdmi.h | 7 ++- > 3 files changed, 71 insertions(+), 20 deletions(-) >
Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > dpintf is the displayport interface hardware unit. This unit is similar > to dpi and can reuse most of the code. > > This patch adds support for mt8195-dpintf to this dpi driver. Main > differences are: > - Some features/functional components are not available for dpintf > which are now excluded from code execution once is_dpintf is set > - dpintf can and needs to choose between different clockdividers based > on the clockspeed. This is done by choosing a different clock parent. > - There are two additional clocks that need to be managed. These are > only set for dpintf and will be set to NULL if not supplied. The > clk_* calls handle these as normal clocks then. > - Some register contents differ slightly between the two components. To > work around this I added register bits/masks with a DPINTF_ prefix > and use them where different. > > Based on a separate driver for dpintf created by > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > Only after finishing the review, I've noticed that I just wrote the same things that I wrote in my review for version 8... and if I recall correctly, this is not the first time that something like that happens. Please pay attention to what reviewers say, as to not waste anyone's time. > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 126 +++++++++++++++++--- > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 35 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 ++ > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +- > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > 6 files changed, 159 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > index eb969c5c5c2e..763bfb700135 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -71,6 +71,7 @@ struct mtk_dpi { > void __iomem *regs; > struct device *dev; > struct clk *engine_clk; > + struct clk *dpi_ck_cg; > struct clk *pixel_clk; > struct clk *tvd_clk; > int irq; > @@ -126,6 +127,7 @@ struct mtk_dpi_conf { > const u32 *output_fmts; > u32 num_output_fmts; > bool is_ck_de_pol; > + bool is_dpintf; > bool swap_input_support; > /* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH (no shift) */ > u32 dimension_mask; > @@ -438,6 +440,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi) > mtk_dpi_disable(dpi); > clk_disable_unprepare(dpi->pixel_clk); > clk_disable_unprepare(dpi->engine_clk); > + clk_disable_unprepare(dpi->dpi_ck_cg); > + clk_disable_unprepare(dpi->tvd_clk); > } > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > @@ -447,12 +451,24 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > if (++dpi->refcount != 1) > return 0; > > + ret = clk_prepare_enable(dpi->tvd_clk); > + if (ret) { > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", ret); > + goto err_pixel; > + } > + > ret = clk_prepare_enable(dpi->engine_clk); > if (ret) { > dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret); > goto err_refcount; > } > > + ret = clk_prepare_enable(dpi->dpi_ck_cg); > + if (ret) { > + dev_err(dpi->dev, "Failed to enable dpi_ck_cg clock: %d\n", ret); > + goto err_ck_cg; > + } > + > ret = clk_prepare_enable(dpi->pixel_clk); > if (ret) { > dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret); > @@ -466,6 +482,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > return 0; > > err_pixel: > + clk_disable_unprepare(dpi->dpi_ck_cg); > +err_ck_cg: > clk_disable_unprepare(dpi->engine_clk); > err_refcount: > dpi->refcount--; > @@ -498,11 +516,11 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > vm.pixelclock = pll_rate / factor; > if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > - (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) > + (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) { The indentation was perfect before that change... > clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2); > - else > + } else { > clk_set_rate(dpi->pixel_clk, vm.pixelclock); > - > + } > > vm.pixelclock = clk_get_rate(dpi->pixel_clk); > > @@ -515,9 +533,15 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > MTK_DPI_POLARITY_FALLING : MTK_DPI_POLARITY_RISING; > dpi_pol.vsync_pol = vm.flags & DISPLAY_FLAGS_VSYNC_HIGH ? > MTK_DPI_POLARITY_FALLING : MTK_DPI_POLARITY_RISING; > - hsync.sync_width = vm.hsync_len; > - hsync.back_porch = vm.hback_porch; > - hsync.front_porch = vm.hfront_porch; > + if (dpi->conf->is_dpintf) { > + hsync.sync_width = vm.hsync_len / 4; > + hsync.back_porch = vm.hback_porch / 4; > + hsync.front_porch = vm.hfront_porch / 4; > + } else { > + hsync.sync_width = vm.hsync_len; > + hsync.back_porch = vm.hback_porch; > + hsync.front_porch = vm.hfront_porch; > + } This looks way better: hsync.sync_width = vm.hsync_len; hsync.back_porch = vm.hback_porch; hsync.front_porch = vm.hfront_porch; /* For DPINTF, we need to divide everything by 4 .. lanes? */ if (dpi->conf->is_dpintf) { hsync.sync_width /= 4; hsync.back_porch /= 4; hsync.front_porch /= 4; } > hsync.shift_half_line = false; > vsync_lodd.sync_width = vm.vsync_len; > vsync_lodd.back_porch = vm.vback_porch; > @@ -559,13 +583,20 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > mtk_dpi_config_channel_limit(dpi); > mtk_dpi_config_bit_num(dpi, dpi->bit_num); > mtk_dpi_config_channel_swap(dpi, dpi->channel_swap); > - mtk_dpi_config_yc_map(dpi, dpi->yc_map); > mtk_dpi_config_color_format(dpi, dpi->color_format); > - mtk_dpi_config_2n_h_fre(dpi); > - mtk_dpi_dual_edge(dpi); > - mtk_dpi_config_disable_edge(dpi); > + if (dpi->conf->is_dpintf) { > + mtk_dpi_mask(dpi, DPI_CON, DPINTF_INPUT_2P_EN, > + DPINTF_INPUT_2P_EN); > + } else { > + mtk_dpi_config_yc_map(dpi, dpi->yc_map); > + mtk_dpi_config_2n_h_fre(dpi); > + mtk_dpi_dual_edge(dpi); > + mtk_dpi_config_disable_edge(dpi); > + } > mtk_dpi_sw_reset(dpi, false); > > + mtk_dpi_enable(dpi); > + > return 0; > } > > @@ -608,7 +639,6 @@ static u32 *mtk_dpi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > u32 *input_fmts; > > *num_input_fmts = 0; > - Removing this line is not part of adding dpintf support, so don't do that. > input_fmts = kcalloc(1, sizeof(*input_fmts), > GFP_KERNEL); > if (!input_fmts) > @@ -634,15 +664,18 @@ static int mtk_dpi_bridge_atomic_check(struct drm_bridge *bridge, > if (dpi->conf->num_output_fmts) > out_bus_format = dpi->conf->output_fmts[0]; > > - dev_dbg(dpi->dev, "input format 0x%04x, output format 0x%04x\n", > - bridge_state->input_bus_cfg.format, > - bridge_state->output_bus_cfg.format); > + dev_info(dpi->dev, "input format 0x%04x, output format 0x%04x\n", > + bridge_state->input_bus_cfg.format, > + bridge_state->output_bus_cfg.format); This message is not giving any constantly changing information, nor any one that is interesting for the user: keep this as dev_dbg(). > > dpi->output_fmt = out_bus_format; > dpi->bit_num = MTK_DPI_OUT_BIT_NUM_8BITS; > dpi->channel_swap = MTK_DPI_OUT_CHANNEL_SWAP_RGB; > dpi->yc_map = MTK_DPI_OUT_YC_MAP_RGB; > - dpi->color_format = MTK_DPI_COLOR_FORMAT_RGB; > + if (out_bus_format == MEDIA_BUS_FMT_YUYV8_1X16) > + dpi->color_format = MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL; > + else > + dpi->color_format = MTK_DPI_COLOR_FORMAT_RGB; > > return 0; > } > @@ -687,7 +720,7 @@ mtk_dpi_bridge_mode_valid(struct drm_bridge *bridge, > { > struct mtk_dpi *dpi = bridge_to_dpi(bridge); > > - if (mode->clock > dpi->conf->max_clock_khz) > + if (dpi->conf->max_clock_khz && mode->clock > dpi->conf->max_clock_khz) > return MODE_CLOCK_HIGH; > > return MODE_OK; > @@ -801,6 +834,16 @@ static unsigned int mt8183_calculate_factor(int clock) > return 2; > } > > +static unsigned int mt8195_dpintf_calculate_factor(int clock) > +{ > + if (clock < 70000) is 70000 intentional? Or did you mean 700000? > + return 4; > + else if (clock < 200000) > + return 2; > + else > + return 1; > +} > + > static const u32 mt8173_output_fmts[] = { > MEDIA_BUS_FMT_RGB888_1X24, > }; > @@ -810,6 +853,12 @@ static const u32 mt8183_output_fmts[] = { > MEDIA_BUS_FMT_RGB888_2X12_BE, > }; > > +static const u32 mt8195_output_fmts[] = { > + MEDIA_BUS_FMT_RGB888_1X24, > + MEDIA_BUS_FMT_YUV8_1X24, > + MEDIA_BUS_FMT_YUYV8_1X16, > +}; > + > static const struct mtk_dpi_yc_limit mtk_dpi_limit = { > .c_bottom = 0x0010, > .c_top = 0x0FE0, > @@ -817,6 +866,13 @@ static const struct mtk_dpi_yc_limit mtk_dpi_limit = { > .y_top = 0x0FE0, > }; > > +static const struct mtk_dpi_yc_limit mtk_dpintf_limit = { > + .c_bottom = 0x0000, > + .c_top = 0xFFF, > + .y_bottom = 0x0000, > + .y_top = 0xFFF, > +}; > + > static const struct mtk_dpi_conf mt8173_conf = { > .cal_factor = mt8173_calculate_factor, > .reg_h_fre_con = 0xe0, > @@ -882,6 +938,19 @@ static const struct mtk_dpi_conf mt8192_conf = { > .limit = &mtk_dpi_limit, > }; > > +static const struct mtk_dpi_conf mt8195_dpintf_conf = { > + .cal_factor = mt8195_dpintf_calculate_factor, > + .output_fmts = mt8195_output_fmts, > + .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts), > + .is_dpintf = true, > + .dimension_mask = DPINTF_HPW_MASK, > + .hvsize_mask = DPINTF_HSIZE_MASK, > + .channel_swap_shift = DPINTF_CH_SWAP, > + .yuv422_en_bit = DPINTF_YUV422_EN, > + .csc_enable_bit = DPINTF_CSC_ENABLE, > + .limit = &mtk_dpintf_limit, > +}; > + > static int mtk_dpi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -929,7 +998,27 @@ static int mtk_dpi_probe(struct platform_device *pdev) > if (IS_ERR(dpi->engine_clk)) { > ret = PTR_ERR(dpi->engine_clk); > if (ret != -EPROBE_DEFER) > - dev_err(dev, "Failed to get engine clock: %d\n", ret); > + dev_err(dev, "Failed to get engine clock: %d\n", > + ret); Why are you breaking this line? > + > + return ret; > + } > + > + dpi->dpi_ck_cg = devm_clk_get_optional(dev, "ck_cg"); > + if (IS_ERR(dpi->dpi_ck_cg)) { > + ret = PTR_ERR(dpi->dpi_ck_cg); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Failed to get dpi ck cg clock: %d\n", > + ret); > + > + return ret; > + } > + You're getting this clock twice, what happened here? P.S.: As I explained on the dt-bindings patch, you likely don't even need this clock at all. > + dpi->dpi_ck_cg = devm_clk_get_optional(dev, "ck_cg"); > + if (IS_ERR(dpi->dpi_ck_cg)) { > + ret = PTR_ERR(dpi->dpi_ck_cg); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Failed to get dpi ck cg clock: %d\n", ret); > > return ret; > } > @@ -1004,6 +1093,9 @@ static const struct of_device_id mtk_dpi_of_ids[] = { > { .compatible = "mediatek,mt8192-dpi", > .data = &mt8192_conf, > }, > + { .compatible = "mediatek,mt8195-dpintf", > + .data = &mt8195_dpintf_conf, > + }, > { }, > }; > MODULE_DEVICE_TABLE(of, mtk_dpi_of_ids); > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > index 3a02fabe1662..dd47dd3f2e4f 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > @@ -40,10 +40,15 @@ > #define FAKE_DE_LEVEN BIT(21) > #define FAKE_DE_RODD BIT(22) > #define FAKE_DE_REVEN BIT(23) > +#define DPINTF_YUV422_EN BIT(24) > +#define DPINTF_CSC_ENABLE BIT(26) > +#define DPINTF_INPUT_2P_EN BIT(29) > > #define DPI_OUTPUT_SETTING 0x14 > #define CH_SWAP 0 > +#define DPINTF_CH_SWAP BIT(1) > #define CH_SWAP_MASK (0x7 << 0) > +#define DPINTF_CH_SWAP_MASK (0x7 << 1) > #define SWAP_RGB 0x00 > #define SWAP_GBR 0x01 > #define SWAP_BRG 0x02 > @@ -80,8 +85,10 @@ > #define DPI_SIZE 0x18 > #define HSIZE 0 > #define HSIZE_MASK (0x1FFF << 0) > +#define DPINTF_HSIZE_MASK (0xFFFF << 0) > #define VSIZE 16 > #define VSIZE_MASK (0x1FFF << 16) > +#define DPINTF_VSIZE_MASK (0xFFFF << 16) > > #define DPI_DDR_SETTING 0x1C > #define DDR_EN BIT(0) > @@ -93,24 +100,30 @@ > #define DPI_TGEN_HWIDTH 0x20 > #define HPW 0 > #define HPW_MASK (0xFFF << 0) > +#define DPINTF_HPW_MASK (0xFFFF << 0) > > #define DPI_TGEN_HPORCH 0x24 > #define HBP 0 > #define HBP_MASK (0xFFF << 0) > +#define DPINTF_HBP_MASK (0xFFFF << 0) > #define HFP 16 > #define HFP_MASK (0xFFF << 16) > +#define DPINTF_HFP_MASK (0xFFFF << 16) > > #define DPI_TGEN_VWIDTH 0x28 > #define DPI_TGEN_VPORCH 0x2C > > #define VSYNC_WIDTH_SHIFT 0 > #define VSYNC_WIDTH_MASK (0xFFF << 0) > +#define DPINTF_VSYNC_WIDTH_MASK (0xFFFF << 0) > #define VSYNC_HALF_LINE_SHIFT 16 > #define VSYNC_HALF_LINE_MASK BIT(16) > #define VSYNC_BACK_PORCH_SHIFT 0 > #define VSYNC_BACK_PORCH_MASK (0xFFF << 0) > +#define DPINTF_VSYNC_BACK_PORCH_MASK (0xFFFF << 0) > #define VSYNC_FRONT_PORCH_SHIFT 16 > #define VSYNC_FRONT_PORCH_MASK (0xFFF << 16) > +#define DPINTF_VSYNC_FRONT_PORCH_MASK (0xFFFF << 16) > > #define DPI_BG_HCNTL 0x30 > #define BG_RIGHT (0x1FFF << 0) > @@ -217,4 +230,26 @@ > > #define EDGE_SEL_EN BIT(5) > #define H_FRE_2N BIT(25) > + > +#define RGB_TO_JPEG 0x00 This is 0x0 > +#define RGB_TO_FULL709 0x01 0x1 > +#define RGB_TO_BT601 0x02 0x2 ...remove the unnecessary leading zeros, but... you're not ever using this set of definitions, so why are you even adding them? > +#define RGB_TO_BT709 0x03 > +#define JPEG_TO_RGB 0x04 > +#define FULL709_TO_RGB 0x05 > +#define BT601_TO_RGB 0x06 > +#define BT709_TO_RGB 0x07 > +#define JPEG_TO_BT601 0x08 > +#define JPEG_TO_BT709 0x09 > +#define BT601_TO_JPEG 0xA > +#define BT709_TO_JPEG 0xB > +#define BT709_TO_BT601 0xC > +#define BT601_TO_BT709 0xD > +#define JPEG_TO_CERGB 0x14 > +#define FULL709_TO_CERGB 0x15 > +#define BT601_TO_CERGB 0x16 > +#define BT709_TO_CERGB 0x17 > +#define RGB_TO_CERGB 0x1C > +#define MATRIX_BIT BIT(8) > +#define EXT_MATRIX_EN BIT(12) > #endif /* __MTK_DPI_REGS_H */ > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index 245d0074e12d..3738665a712e 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > @@ -419,6 +419,11 @@ static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = { > [MTK_DISP_WDMA] = "wdma", > [MTK_DPI] = "dpi", > [MTK_DSI] = "dsi", > + [MTK_DP_INTF] = "dp-intf", > + [MTK_DISP_PWM] = "pwm", > + [MTK_DISP_MUTEX] = "mutex", > + [MTK_DISP_OD] = "od", > + [MTK_DISP_BLS] = "bls", Please keep alphabetic order. > }; > > struct mtk_ddp_comp_match { > @@ -439,6 +444,8 @@ static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { > [DDP_COMPONENT_DPI1] = { MTK_DPI, 1, &ddp_dpi }, > [DDP_COMPONENT_DSC0] = { MTK_DISP_DSC, 0, &ddp_dsc }, > [DDP_COMPONENT_DSC1] = { MTK_DISP_DSC, 1, &ddp_dsc }, > + [DDP_COMPONENT_DP_INTF0] = { MTK_DP_INTF, 0, &ddp_dpi }, > + [DDP_COMPONENT_DP_INTF1] = { MTK_DP_INTF, 1, &ddp_dpi }, Indentation issue. Fix it. > [DDP_COMPONENT_DSI0] = { MTK_DSI, 0, &ddp_dsi }, > [DDP_COMPONENT_DSI1] = { MTK_DSI, 1, &ddp_dsi }, > [DDP_COMPONENT_DSI2] = { MTK_DSI, 2, &ddp_dsi }, > @@ -565,6 +572,7 @@ int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp, > type == MTK_DISP_PWM || > type == MTK_DISP_RDMA || > type == MTK_DPI || > + type == MTK_DP_INTF || > type == MTK_DSI) > return 0; > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > index 825d763d2378..c4e683f46a95 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > @@ -37,6 +37,7 @@ enum mtk_ddp_comp_type { > MTK_DISP_UFOE, > MTK_DISP_WDMA, > MTK_DPI, > + MTK_DP_INTF, > MTK_DSI, > MTK_DDP_COMP_TYPE_MAX, > }; > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index a2a783fc580e..e25ac61aac08 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -690,6 +690,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { > .data = (void *)MTK_DPI }, > { .compatible = "mediatek,mt8183-dpi", > .data = (void *)MTK_DPI }, > + { .compatible = "mediatek,mt8195-dpintf", > + .data = (void *)MTK_DP_INTF }, > { .compatible = "mediatek,mt2701-dsi", > .data = (void *)MTK_DSI }, > { .compatible = "mediatek,mt8173-dsi", > @@ -801,8 +803,9 @@ static int mtk_drm_probe(struct platform_device *pdev) > comp_type == MTK_DISP_OVL_2L || > comp_type == MTK_DISP_OVL_ADAPTOR || > comp_type == MTK_DISP_RDMA || > + comp_type == MTK_DSI || > comp_type == MTK_DPI || > - comp_type == MTK_DSI) { > + comp_type == MTK_DP_INTF) { These were nice and alphabetically ordered, why are you moving MTK_DSI up there?! comp_type == MTK_DISP_RDMA || comp_type == MTK_DP_INTF || comp_type == MTK_DPI || comp_type == MTK_DSI) { ... that's how it should look like. > dev_info(dev, "Adding component match for %pOF\n", > node); > drm_of_component_match_add(dev, &match, component_compare_of, > diff --git a/include/linux/soc/mediatek/mtk-mmsys.h b/include/linux/soc/mediatek/mtk-mmsys.h > index 3e998bfb795a..e4b84c347201 100644 > --- a/include/linux/soc/mediatek/mtk-mmsys.h > +++ b/include/linux/soc/mediatek/mtk-mmsys.h > @@ -21,12 +21,12 @@ enum mtk_ddp_comp_id { > DDP_COMPONENT_COLOR0, > DDP_COMPONENT_COLOR1, > DDP_COMPONENT_DITHER, > - DDP_COMPONENT_DP_INTF0, > - DDP_COMPONENT_DP_INTF1, > DDP_COMPONENT_DPI0, > DDP_COMPONENT_DPI1, > DDP_COMPONENT_DSC0, > DDP_COMPONENT_DSC1, > + DDP_COMPONENT_DP_INTF0, > + DDP_COMPONENT_DP_INTF1, Why are you moving this?! > DDP_COMPONENT_DSI0, > DDP_COMPONENT_DSI1, > DDP_COMPONENT_DSI2,
Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > This patch adds External DisplayPort support to the mt8195 eDP driver. > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > drivers/gpu/drm/mediatek/mtk_dp.c | 104 +++++++++++++++++++++++------- > 1 file changed, 81 insertions(+), 23 deletions(-) > Your patch [18/21], contained in this series, introduces mtk_dp.c. This patch [19/21] cannot be applied cleanly on top of 18/21. What happened here?!?
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Adds a bit of flexibility to support SoCs without CK/DE pol support It seems that DP_INTF has no CK/DE pol function. If so, could you explain why DP_INTF has this difference with DPI? Regards, CK > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 4746eb342567..545a1337cc89 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -125,6 +125,7 @@ struct mtk_dpi_conf { > bool edge_sel_en; > const u32 *output_fmts; > u32 num_output_fmts; > + bool is_ck_de_pol; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -211,13 +212,20 @@ static void mtk_dpi_config_pol(struct mtk_dpi > *dpi, > struct mtk_dpi_polarities *dpi_pol) > { > unsigned int pol; > + unsigned int mask; > > - pol = (dpi_pol->ck_pol == MTK_DPI_POLARITY_RISING ? 0 : CK_POL) > | > - (dpi_pol->de_pol == MTK_DPI_POLARITY_RISING ? 0 : DE_POL) > | > - (dpi_pol->hsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > HSYNC_POL) | > + mask = HSYNC_POL | VSYNC_POL; > + pol = (dpi_pol->hsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > HSYNC_POL) | > (dpi_pol->vsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > VSYNC_POL); > - mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, pol, > - CK_POL | DE_POL | HSYNC_POL | VSYNC_POL); > + if (dpi->conf->is_ck_de_pol) { > + mask |= CK_POL | DE_POL; > + pol |= (dpi_pol->ck_pol == MTK_DPI_POLARITY_RISING ? > + 0 : CK_POL) | > + (dpi_pol->de_pol == MTK_DPI_POLARITY_RISING ? > + 0 : DE_POL); > + } > + > + mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, pol, mask); > } > > static void mtk_dpi_config_3d(struct mtk_dpi *dpi, bool en_3d) > @@ -799,6 +807,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .max_clock_khz = 300000, > .output_fmts = mt8173_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > + .is_ck_de_pol = true, > .limit = &mtk_dpi_limit, > }; > > @@ -809,6 +818,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .max_clock_khz = 150000, > .output_fmts = mt8173_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > + .is_ck_de_pol = true, > .limit = &mtk_dpi_limit, > }; > > @@ -818,6 +828,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .max_clock_khz = 100000, > .output_fmts = mt8183_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > + .is_ck_de_pol = true, > .limit = &mtk_dpi_limit, > }; > > @@ -827,6 +838,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .max_clock_khz = 150000, > .output_fmts = mt8173_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > + .is_ck_de_pol = true, > .limit = &mtk_dpi_limit, > }; >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Adds a bit of flexibility to support SoCs without swap_input support > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 545a1337cc89..454f8563efae 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -126,6 +126,7 @@ struct mtk_dpi_conf { > const u32 *output_fmts; > u32 num_output_fmts; > bool is_ck_de_pol; > + bool swap_input_support; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -378,18 +379,21 @@ static void mtk_dpi_config_color_format(struct > mtk_dpi *dpi, > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > mtk_dpi_config_yuv422_enable(dpi, false); > mtk_dpi_config_csc_enable(dpi, true); > - mtk_dpi_config_swap_input(dpi, false); > + if (dpi->conf->swap_input_support) > + mtk_dpi_config_swap_input(dpi, false); > mtk_dpi_config_channel_swap(dpi, > MTK_DPI_OUT_CHANNEL_SWAP_BGR); > } else if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_422) || > (format == MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL)) { > mtk_dpi_config_yuv422_enable(dpi, true); > mtk_dpi_config_csc_enable(dpi, true); > - mtk_dpi_config_swap_input(dpi, true); > + if (dpi->conf->swap_input_support) > + mtk_dpi_config_swap_input(dpi, true); As [1], please keep in touch with Mediatek engineer. Regards, CK [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-8-granquet@baylibre.com/ > mtk_dpi_config_channel_swap(dpi, > MTK_DPI_OUT_CHANNEL_SWAP_RGB); > } else { > mtk_dpi_config_yuv422_enable(dpi, false); > mtk_dpi_config_csc_enable(dpi, false); > - mtk_dpi_config_swap_input(dpi, false); > + if (dpi->conf->swap_input_support) > + mtk_dpi_config_swap_input(dpi, false); > mtk_dpi_config_channel_swap(dpi, > MTK_DPI_OUT_CHANNEL_SWAP_RGB); > } > } > @@ -808,6 +812,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .output_fmts = mt8173_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > .is_ck_de_pol = true, > + .swap_input_support = true, > .limit = &mtk_dpi_limit, > }; > > @@ -819,6 +824,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .output_fmts = mt8173_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > .is_ck_de_pol = true, > + .swap_input_support = true, > .limit = &mtk_dpi_limit, > }; > > @@ -829,6 +835,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .output_fmts = mt8183_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > .is_ck_de_pol = true, > + .swap_input_support = true, > .limit = &mtk_dpi_limit, > }; > > @@ -839,6 +846,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .output_fmts = mt8173_output_fmts, > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > .is_ck_de_pol = true, > + .swap_input_support = true, > .limit = &mtk_dpi_limit, > }; >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Add flexibility by moving the dimension mask to the SoC config Reviewed-by: CK Hu <ck.hu@mediatek.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 454f8563efae..bf098f36a466 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -127,6 +127,8 @@ struct mtk_dpi_conf { > u32 num_output_fmts; > bool is_ck_de_pol; > bool swap_input_support; > + /* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH > (no shift) */ > + u32 dimension_mask; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -156,30 +158,30 @@ static void mtk_dpi_disable(struct mtk_dpi > *dpi) > static void mtk_dpi_config_hsync(struct mtk_dpi *dpi, > struct mtk_dpi_sync_param *sync) > { > - mtk_dpi_mask(dpi, DPI_TGEN_HWIDTH, > - sync->sync_width << HPW, HPW_MASK); > - mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, > - sync->back_porch << HBP, HBP_MASK); > + mtk_dpi_mask(dpi, DPI_TGEN_HWIDTH, sync->sync_width << HPW, > + dpi->conf->dimension_mask << HPW); > + mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, sync->back_porch << HBP, > + dpi->conf->dimension_mask << HBP); > mtk_dpi_mask(dpi, DPI_TGEN_HPORCH, sync->front_porch << HFP, > - HFP_MASK); > + dpi->conf->dimension_mask << HFP); > } > > static void mtk_dpi_config_vsync(struct mtk_dpi *dpi, > struct mtk_dpi_sync_param *sync, > u32 width_addr, u32 porch_addr) > { > - mtk_dpi_mask(dpi, width_addr, > - sync->sync_width << VSYNC_WIDTH_SHIFT, > - VSYNC_WIDTH_MASK); > mtk_dpi_mask(dpi, width_addr, > sync->shift_half_line << VSYNC_HALF_LINE_SHIFT, > VSYNC_HALF_LINE_MASK); > + mtk_dpi_mask(dpi, width_addr, > + sync->sync_width << VSYNC_WIDTH_SHIFT, > + dpi->conf->dimension_mask << VSYNC_WIDTH_SHIFT); > mtk_dpi_mask(dpi, porch_addr, > sync->back_porch << VSYNC_BACK_PORCH_SHIFT, > - VSYNC_BACK_PORCH_MASK); > + dpi->conf->dimension_mask << > VSYNC_BACK_PORCH_SHIFT); > mtk_dpi_mask(dpi, porch_addr, > sync->front_porch << VSYNC_FRONT_PORCH_SHIFT, > - VSYNC_FRONT_PORCH_MASK); > + dpi->conf->dimension_mask << > VSYNC_FRONT_PORCH_SHIFT); > } > > static void mtk_dpi_config_vsync_lodd(struct mtk_dpi *dpi, > @@ -813,6 +815,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > .is_ck_de_pol = true, > .swap_input_support = true, > + .dimension_mask = HPW_MASK, > .limit = &mtk_dpi_limit, > }; > > @@ -825,6 +828,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > .is_ck_de_pol = true, > .swap_input_support = true, > + .dimension_mask = HPW_MASK, > .limit = &mtk_dpi_limit, > }; > > @@ -836,6 +840,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > .is_ck_de_pol = true, > .swap_input_support = true, > + .dimension_mask = HPW_MASK, > .limit = &mtk_dpi_limit, > }; > > @@ -847,6 +852,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > .is_ck_de_pol = true, > .swap_input_support = true, > + .dimension_mask = HPW_MASK, > .limit = &mtk_dpi_limit, > }; >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Add flexibility by moving the hvsize mask to SoC specific config Reviewed-by: CK Hu <ck.hu@mediatek.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index bf098f36a466..6eeda222a973 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -129,6 +129,8 @@ struct mtk_dpi_conf { > bool swap_input_support; > /* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH > (no shift) */ > u32 dimension_mask; > + /* HSIZE and VSIZE mask (no shift) */ > + u32 hvsize_mask; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -243,8 +245,10 @@ static void mtk_dpi_config_interface(struct > mtk_dpi *dpi, bool inter) > > static void mtk_dpi_config_fb_size(struct mtk_dpi *dpi, u32 width, > u32 height) > { > - mtk_dpi_mask(dpi, DPI_SIZE, width << HSIZE, HSIZE_MASK); > - mtk_dpi_mask(dpi, DPI_SIZE, height << VSIZE, VSIZE_MASK); > + mtk_dpi_mask(dpi, DPI_SIZE, width << HSIZE, > + dpi->conf->hvsize_mask << HSIZE); > + mtk_dpi_mask(dpi, DPI_SIZE, height << VSIZE, > + dpi->conf->hvsize_mask << VSIZE); > } > > static void mtk_dpi_config_channel_limit(struct mtk_dpi *dpi) > @@ -816,6 +820,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .is_ck_de_pol = true, > .swap_input_support = true, > .dimension_mask = HPW_MASK, > + .hvsize_mask = HSIZE_MASK, > .limit = &mtk_dpi_limit, > }; > > @@ -829,6 +834,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .is_ck_de_pol = true, > .swap_input_support = true, > .dimension_mask = HPW_MASK, > + .hvsize_mask = HSIZE_MASK, > .limit = &mtk_dpi_limit, > }; > > @@ -841,6 +847,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .is_ck_de_pol = true, > .swap_input_support = true, > .dimension_mask = HPW_MASK, > + .hvsize_mask = HSIZE_MASK, > .limit = &mtk_dpi_limit, > }; > > @@ -853,6 +860,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .is_ck_de_pol = true, > .swap_input_support = true, > .dimension_mask = HPW_MASK, > + .hvsize_mask = HSIZE_MASK, > .limit = &mtk_dpi_limit, > }; >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Add flexibility by moving the swap shift value to SoC specific config > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 6eeda222a973..6d4d8c6ec47d 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -131,6 +131,7 @@ struct mtk_dpi_conf { > u32 dimension_mask; > /* HSIZE and VSIZE mask (no shift) */ > u32 hvsize_mask; > + u32 channel_swap_shift; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -349,7 +350,8 @@ static void mtk_dpi_config_channel_swap(struct > mtk_dpi *dpi, > break; > } > > - mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << CH_SWAP, > CH_SWAP_MASK); > + mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf- > >channel_swap_shift, > + CH_SWAP_MASK); From the definiton: #define CH_SWAP 0 +#define DPINTF_CH_SWAP BIT(1) #define CH_SWAP_MASK (0x7 << 0) +#define DPINTF_CH_SWAP_MASK (0x7 << 1) This statement should be: mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf- >channel_swap_shift, CH_SWAP_MASK << dpi->conf->channel_swap_shift); dpi->conf->channel_swap_shift is 1 for MT8195-DP_INTF and 0 for others. And drop the definition of DPINTF_CH_SWAP and DPINTF_CH_SWAP_MASK, Regards, CK > } > > static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool > enable) > @@ -821,6 +823,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .swap_input_support = true, > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > + .channel_swap_shift = CH_SWAP, > .limit = &mtk_dpi_limit, > }; > > @@ -835,6 +838,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .swap_input_support = true, > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > + .channel_swap_shift = CH_SWAP, > .limit = &mtk_dpi_limit, > }; > > @@ -848,6 +852,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .swap_input_support = true, > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > + .channel_swap_shift = CH_SWAP, > .limit = &mtk_dpi_limit, > }; > > @@ -861,6 +866,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .swap_input_support = true, > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > + .channel_swap_shift = CH_SWAP, > .limit = &mtk_dpi_limit, > }; >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > Add flexibility by moving the yuv422 en bit to SoC specific config Reviewed-by: CK Hu <ck.hu@mediatek.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 6d4d8c6ec47d..40254cd9d168 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -132,6 +132,7 @@ struct mtk_dpi_conf { > /* HSIZE and VSIZE mask (no shift) */ > u32 hvsize_mask; > u32 channel_swap_shift; > + u32 yuv422_en_bit; > const struct mtk_dpi_yc_limit *limit; > }; > > @@ -356,7 +357,8 @@ static void mtk_dpi_config_channel_swap(struct > mtk_dpi *dpi, > > static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool > enable) > { > - mtk_dpi_mask(dpi, DPI_CON, enable ? YUV422_EN : 0, YUV422_EN); > + mtk_dpi_mask(dpi, DPI_CON, enable ? dpi->conf->yuv422_en_bit : > 0, > + dpi->conf->yuv422_en_bit); > } > > static void mtk_dpi_config_csc_enable(struct mtk_dpi *dpi, bool > enable) > @@ -824,6 +826,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > + .yuv422_en_bit = YUV422_EN, > .limit = &mtk_dpi_limit, > }; > > @@ -839,6 +842,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > + .yuv422_en_bit = YUV422_EN, > .limit = &mtk_dpi_limit, > }; > > @@ -853,6 +857,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > + .yuv422_en_bit = YUV422_EN, > .limit = &mtk_dpi_limit, > }; > > @@ -867,6 +872,7 @@ static const struct mtk_dpi_conf mt8192_conf = { > .dimension_mask = HPW_MASK, > .hvsize_mask = HSIZE_MASK, > .channel_swap_shift = CH_SWAP, > + .yuv422_en_bit = YUV422_EN, > .limit = &mtk_dpi_limit, > }; >
Hi, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > [snip] > + > +static void mtk_dp_read_link_status(struct mtk_dp *mtk_dp, > + u8 > link_status[DP_LINK_STATUS_SIZE]) > +{ > + drm_dp_dpcd_read(&mtk_dp->aux, DP_LANE0_1_STATUS, link_status, > + DP_LINK_STATUS_SIZE); > +} > + > +static int mtk_dp_train_tps_1(struct mtk_dp *mtk_dp, > + u8 target_lane_count, int > *iteration_count, u8 *lane_adjust, int *status_control, u8 > *prev_lane_adjust) > +{ > + int ret; > + u8 val; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + > + ret = mtk_dp_training_set_scramble(mtk_dp, false); > + if (ret) > + return ret; > + > + if (*status_control == 0) { > + ret = mtk_dp_train_set_pattern(mtk_dp, 1); > + if (ret) > + return ret; > + > + val = DP_LINK_SCRAMBLING_DISABLE | > + DP_TRAINING_PATTERN_1; > + drm_dp_dpcd_writeb(&mtk_dp->aux, > + DP_TRAINING_PATTERN_SET, > + DP_LINK_SCRAMBLING_DISABLE | > + DP_TRAINING_PATTERN_1); > + 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 = 1; > + (*iteration_count)++; > + } > + > + drm_dp_link_train_clock_recovery_delay(&mtk_dp->aux, > + mtk_dp->rx_cap); > + mtk_dp_read_link_status(mtk_dp, link_status); drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); > + > + if (drm_dp_clock_recovery_ok(link_status, > + target_lane_count)) { > + mtk_dp->train_info.cr_done = true; > + *iteration_count = 1; > + dev_dbg(mtk_dp->dev, "Link train CR pass\n"); > + return 0; > + } else if (*prev_lane_adjust == link_status[4]) { > + (*iteration_count)++; > + if (*prev_lane_adjust & > DP_ADJUST_VOLTAGE_SWING_LANE0_MASK) { > + dev_dbg(mtk_dp->dev, "Link train CQ fail\n"); > + return -EINVAL; > + } > + } else { > + *prev_lane_adjust = link_status[4]; > + } > + return -EAGAIN; > +} > + > +static int mtk_dp_train_tps_2_3(struct mtk_dp *mtk_dp, u8 > target_linkrate, > + u8 target_lane_count, int > *iteration_count, u8 *lane_adjust, int *status_control, u8 > *prev_lane_adjust) > +{ > + int ret; > + u8 val; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + > + if (*status_control == 1) { > + if (mtk_dp->train_info.tps4) { > + ret = mtk_dp_train_set_pattern(mtk_dp, 4); > + if (ret) > + return -EINVAL; > + > + val = DP_TRAINING_PATTERN_4; > + } else if (mtk_dp->train_info.tps3) { > + ret = mtk_dp_train_set_pattern(mtk_dp, 3); > + if (ret) > + return -EINVAL; > + > + val = DP_LINK_SCRAMBLING_DISABLE | > + DP_TRAINING_PATTERN_3; > + } else { > + ret = mtk_dp_train_set_pattern(mtk_dp, 2); > + if (ret) > + return -EINVAL; > + > + 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); > + > + mtk_dp_read_link_status(mtk_dp, link_status); drm_dp_dpcd_read_link_status(&mtk_dp->aux, link_status); Regards, CK > + > + if (!drm_dp_clock_recovery_ok(link_status, > + target_lane_count)) { > + 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, Guillaume: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > [snip] > + > +static irqreturn_t mtk_dp_hpd_isr_handler(struct mtk_dp *mtk_dp) > +{ > + bool connected; > + u16 swirq_status = mtk_dp_swirq_get_clear(mtk_dp); > + u8 hwirq_status = mtk_dp_hwirq_get_clear(mtk_dp); > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + > + train_info->irq_status |= hwirq_status | swirq_status; You mix software control variable and irq status here. Break each flag in irq_status to variables in train_info to decouple software control variable and irq status. > + > + if (!train_info->irq_status) > + return IRQ_HANDLED; > + > + connected = mtk_dp_plug_state(mtk_dp); > + if (connected || !train_info->cable_plugged_in) > + train_info->irq_status &= ~MTK_DP_HPD_DISCONNECT; > + else if (!connected || train_info->cable_plugged_in) > + train_info->irq_status &= ~MTK_DP_HPD_CONNECT; > + > + if (!(train_info->irq_status & > + (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT))) > + return IRQ_HANDLED; > + > + if (train_info->irq_status & MTK_DP_HPD_CONNECT) { > + train_info->irq_status &= ~MTK_DP_HPD_CONNECT; > + train_info->cable_plugged_in = true; > + } else { > + train_info->irq_status &= ~MTK_DP_HPD_DISCONNECT; > + train_info->cable_plugged_in = false; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + } > + train_info->cable_state_change = true; > + > + return IRQ_WAKE_THREAD; > +} > +
On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > this series is built around the DisplayPort driver. The dpi/dpintf > driver and the added helper functions are required for the > DisplayPort > driver to work. > > This v10 still has some un-answered comments and TODOs for v11. > > This has been tested sucessfully on a 5.18-next based "vendor > branch". > > There's a missing dependency in the mediatek clock framework to allow > a > mux clock to change it's parent automatically on rate change. > Without this change, the dpi driver won't properly set the clocks on > mode change and thus nothing will be displayed on screen. > > Changes from v9: > - The DP-Phy is back to being a child device of the DP driver (as in > v8) > - hot plug detection has been added back to Embedded Display Port... > as > after discussing with mediatek experts, this is needed eventhough > the > Embedded Display port is not un-pluggable > - rebased on linux-next > - simplified/split train_handler function, as suggested by Rex > - added comments on the sleep/delays present in the code > - removed previous patch introducing retries when receiving AUX_DEFER > as > this is already handled in the dp_aux framework > - added max-lane and max-linkrate device tree u8 properties instead > of > hardcoded #defines > > Things that are in my todolist for v11: > - retrieve CK/DE support from panel driver instead of hardcoding it > into > the dpi driver > - refcount the dp driver "enabled" status for "future proofing" > - review the drm_dp_helpers for features/functions that have been > re-implemented in the mediatek dp drivers > > Older revisions: > RFC - > https://lore.kernel.org/linux-mediatek/20210816192523.1739365-1-msp@baylibre.com/ > v1 - > https://lore.kernel.org/linux-mediatek/20210906193529.718845-1-msp@baylibre.com/ > v2 - > https://lore.kernel.org/linux-mediatek/20210920084424.231825-1-msp@baylibre.com/ > v3 - > https://lore.kernel.org/linux-mediatek/20211001094443.2770169-1-msp@baylibre.com/ > v4 - > https://lore.kernel.org/linux-mediatek/20211011094624.3416029-1-msp@baylibre.com/ > v5 - > https://lore.kernel.org/all/20211021092707.3562523-1-msp@baylibre.com/ > v6 - > https://lore.kernel.org/linux-mediatek/20211110130623.20553-1-granquet@baylibre.com/ > v7 - > https://lore.kernel.org/linux-mediatek/20211217150854.2081-1-granquet@baylibre.com/ > v8 - > https://lore.kernel.org/linux-mediatek/20220218145437.18563-1-granquet@baylibre.com/ > v9 - > https://lore.kernel.org/all/20220327223927.20848-1-granquet@baylibre.com/ > > Functional dependencies are: > - Add Mediatek Soc DRM (vdosys0) support for mt8195 > > https://lore.kernel.org/linux-mediatek/20220419094143.9561-2-jason-jh.lin@mediatek.com/ > - Add MediaTek SoC DRM (vdosys1) support for mt8195 > > https://lore.kernel.org/linux-mediatek/20220512053128.31415-1-nancy.lin@mediatek.com/ > > > Guillaume Ranquet (15): > drm/edid: Convert cea_sad helper struct to kernelDoc > drm/edid: Add cea_sad helpers for freq/length > drm/mediatek: dpi: move dpi limits to SoC config > drm/mediatek: dpi: implement a CK/DE pol toggle in SoC config > drm/mediatek: dpi: implement a swap_input toggle in SoC config > drm/mediatek: dpi: move dimension mask to SoC config > drm/mediatek: dpi: move hvsize_mask to SoC config > drm/mediatek: dpi: move swap_shift to SoC config > drm/mediatek: dpi: move the yuv422_en_bit to SoC config > drm/mediatek: dpi: move the csc_enable bit to SoC config > drm/mediatek: dpi: Add dpintf support > drm/mediatek: dpi: Only enable dpi after the bridge is enabled > drm/meditek: dpi: Add matrix_sel helper > drm/mediatek: Add mt8195 External DisplayPort support > drm/mediatek: DP audio support for mt8195 > > Jitao Shi (1): > drm/mediatek: add hpd debounce > > Markus Schneider-Pargmann (5): > dt-bindings: mediatek,dpi: Add DPINTF compatible > dt-bindings: mediatek,dp: Add Display Port binding > video/hdmi: Add audio_infoframe packing for DP > phy: phy-mtk-dp: Add driver for DP phy > drm/mediatek: Add mt8195 Embedded DisplayPort driver > > .../display/mediatek/mediatek,dp.yaml | 99 + > .../display/mediatek/mediatek,dpi.yaml | 13 +- > MAINTAINERS | 1 + > drivers/gpu/drm/drm_edid.c | 74 + > drivers/gpu/drm/mediatek/Kconfig | 8 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 3419 > +++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 570 +++ > drivers/gpu/drm/mediatek/mtk_dpi.c | 272 +- > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 38 + > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 + > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 8 +- > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + > drivers/phy/mediatek/Kconfig | 8 + > drivers/phy/mediatek/Makefile | 1 + > drivers/phy/mediatek/phy-mtk-dp.c | 200 + > drivers/video/hdmi.c | 82 +- > include/drm/dp/drm_dp_helper.h | 2 + > include/drm/drm_edid.h | 26 +- > include/linux/hdmi.h | 7 +- > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > 22 files changed, 4765 insertions(+), 81 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c Hello all, Due to the resource issue, I will keep upstreaming Guillaume's MT8195 dp/edp series. I will check the comments for v8/v9/v10 and have some discussion with you. Thanks for your all comments. BRs, Bo-Chen
On Thu, 2022-06-02 at 11:50 +0800, Rex-BC Chen wrote: > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > this series is built around the DisplayPort driver. The dpi/dpintf > > driver and the added helper functions are required for the > > DisplayPort > > driver to work. > > > > This v10 still has some un-answered comments and TODOs for v11. > > > > This has been tested sucessfully on a 5.18-next based "vendor > > branch". > > > > There's a missing dependency in the mediatek clock framework to > > allow > > a > > mux clock to change it's parent automatically on rate change. > > Without this change, the dpi driver won't properly set the clocks > > on > > mode change and thus nothing will be displayed on screen. > > > > Changes from v9: > > - The DP-Phy is back to being a child device of the DP driver (as > > in > > v8) > > - hot plug detection has been added back to Embedded Display > > Port... > > as > > after discussing with mediatek experts, this is needed eventhough > > the > > Embedded Display port is not un-pluggable > > - rebased on linux-next > > - simplified/split train_handler function, as suggested by Rex > > - added comments on the sleep/delays present in the code > > - removed previous patch introducing retries when receiving > > AUX_DEFER > > as > > this is already handled in the dp_aux framework > > - added max-lane and max-linkrate device tree u8 properties instead > > of > > hardcoded #defines > > > > Things that are in my todolist for v11: > > - retrieve CK/DE support from panel driver instead of hardcoding it > > into > > the dpi driver > > - refcount the dp driver "enabled" status for "future proofing" > > - review the drm_dp_helpers for features/functions that have been > > re-implemented in the mediatek dp drivers > > > > Older revisions: > > RFC - > > https://lore.kernel.org/linux-mediatek/20210816192523.1739365-1-msp@baylibre.com/ > > v1 - > > https://lore.kernel.org/linux-mediatek/20210906193529.718845-1-msp@baylibre.com/ > > v2 - > > https://lore.kernel.org/linux-mediatek/20210920084424.231825-1-msp@baylibre.com/ > > v3 - > > https://lore.kernel.org/linux-mediatek/20211001094443.2770169-1-msp@baylibre.com/ > > v4 - > > https://lore.kernel.org/linux-mediatek/20211011094624.3416029-1-msp@baylibre.com/ > > v5 - > > https://lore.kernel.org/all/20211021092707.3562523-1-msp@baylibre.com/ > > v6 - > > https://lore.kernel.org/linux-mediatek/20211110130623.20553-1-granquet@baylibre.com/ > > v7 - > > https://lore.kernel.org/linux-mediatek/20211217150854.2081-1-granquet@baylibre.com/ > > v8 - > > https://lore.kernel.org/linux-mediatek/20220218145437.18563-1-granquet@baylibre.com/ > > v9 - > > https://lore.kernel.org/all/20220327223927.20848-1-granquet@baylibre.com/ > > > > Functional dependencies are: > > - Add Mediatek Soc DRM (vdosys0) support for mt8195 > > > > https://lore.kernel.org/linux-mediatek/20220419094143.9561-2-jason-jh.lin@mediatek.com/ > > - Add MediaTek SoC DRM (vdosys1) support for mt8195 > > > > https://lore.kernel.org/linux-mediatek/20220512053128.31415-1-nancy.lin@mediatek.com/ > > > > > > Guillaume Ranquet (15): > > drm/edid: Convert cea_sad helper struct to kernelDoc > > drm/edid: Add cea_sad helpers for freq/length > > drm/mediatek: dpi: move dpi limits to SoC config > > drm/mediatek: dpi: implement a CK/DE pol toggle in SoC config > > drm/mediatek: dpi: implement a swap_input toggle in SoC config > > drm/mediatek: dpi: move dimension mask to SoC config > > drm/mediatek: dpi: move hvsize_mask to SoC config > > drm/mediatek: dpi: move swap_shift to SoC config > > drm/mediatek: dpi: move the yuv422_en_bit to SoC config > > drm/mediatek: dpi: move the csc_enable bit to SoC config > > drm/mediatek: dpi: Add dpintf support > > drm/mediatek: dpi: Only enable dpi after the bridge is enabled > > drm/meditek: dpi: Add matrix_sel helper > > drm/mediatek: Add mt8195 External DisplayPort support > > drm/mediatek: DP audio support for mt8195 > > > > Jitao Shi (1): > > drm/mediatek: add hpd debounce > > > > Markus Schneider-Pargmann (5): > > dt-bindings: mediatek,dpi: Add DPINTF compatible > > dt-bindings: mediatek,dp: Add Display Port binding > > video/hdmi: Add audio_infoframe packing for DP > > phy: phy-mtk-dp: Add driver for DP phy > > drm/mediatek: Add mt8195 Embedded DisplayPort driver > > > > .../display/mediatek/mediatek,dp.yaml | 99 + > > .../display/mediatek/mediatek,dpi.yaml | 13 +- > > MAINTAINERS | 1 + > > drivers/gpu/drm/drm_edid.c | 74 + > > drivers/gpu/drm/mediatek/Kconfig | 8 + > > drivers/gpu/drm/mediatek/Makefile | 2 + > > drivers/gpu/drm/mediatek/mtk_dp.c | 3419 > > +++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 570 +++ > > drivers/gpu/drm/mediatek/mtk_dpi.c | 272 +- > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 38 + > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 + > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 8 +- > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 3 + > > drivers/phy/mediatek/Kconfig | 8 + > > drivers/phy/mediatek/Makefile | 1 + > > drivers/phy/mediatek/phy-mtk-dp.c | 200 + > > drivers/video/hdmi.c | 82 +- > > include/drm/dp/drm_dp_helper.h | 2 + > > include/drm/drm_edid.h | 26 +- > > include/linux/hdmi.h | 7 +- > > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > > 22 files changed, 4765 insertions(+), 81 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c > > Hello all, > > Due to the resource issue, I will keep upstreaming Guillaume's MT8195 > dp/edp series. > > I will check the comments for v8/v9/v10 and have some discussion with > you. > > Thanks for your all comments. > > BRs, > Bo-Chen > Hello all, Because the patches of dp_intf seem to be almost completed, I want to split this series into two series: dp_intf and edp/dp. It will be easier to review and maintain this series. Thanks! BRs, Bo-Chen
Le 23/05/2022 à 12:47, Guillaume Ranquet a écrit : > dpintf is the displayport interface hardware unit. This unit is similar > to dpi and can reuse most of the code. > > This patch adds support for mt8195-dpintf to this dpi driver. Main > differences are: > - Some features/functional components are not available for dpintf > which are now excluded from code execution once is_dpintf is set > - dpintf can and needs to choose between different clockdividers based > on the clockspeed. This is done by choosing a different clock parent. > - There are two additional clocks that need to be managed. These are > only set for dpintf and will be set to NULL if not supplied. The > clk_* calls handle these as normal clocks then. > - Some register contents differ slightly between the two components. To > work around this I added register bits/masks with a DPINTF_ prefix > and use them where different. > > Based on a separate driver for dpintf created by > Jason-JH.Lin <jason-jh.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>. > > Signed-off-by: Markus Schneider-Pargmann <msp-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > Signed-off-by: Guillaume Ranquet <granquet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 126 +++++++++++++++++--- > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 35 ++++++ > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 ++ > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +- > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > 6 files changed, 159 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > index eb969c5c5c2e..763bfb700135 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -71,6 +71,7 @@ struct mtk_dpi { > void __iomem *regs; > struct device *dev; > struct clk *engine_clk; > + struct clk *dpi_ck_cg; > struct clk *pixel_clk; > struct clk *tvd_clk; > int irq; > @@ -126,6 +127,7 @@ struct mtk_dpi_conf { > const u32 *output_fmts; > u32 num_output_fmts; > bool is_ck_de_pol; > + bool is_dpintf; > bool swap_input_support; > /* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH (no shift) */ > u32 dimension_mask; > @@ -438,6 +440,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi) > mtk_dpi_disable(dpi); > clk_disable_unprepare(dpi->pixel_clk); > clk_disable_unprepare(dpi->engine_clk); > + clk_disable_unprepare(dpi->dpi_ck_cg); > + clk_disable_unprepare(dpi->tvd_clk); > } > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > @@ -447,12 +451,24 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > if (++dpi->refcount != 1) > return 0; > Hi, belwo the error handling path looks odd. (both where we goto, and the order of the clk_disable_unprepare() in the error handling path. just my 2c, CJ > + ret = clk_prepare_enable(dpi->tvd_clk); > + if (ret) { > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", ret); > + goto err_pixel; > + } > + > ret = clk_prepare_enable(dpi->engine_clk); > if (ret) { > dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret); > goto err_refcount; > } > > + ret = clk_prepare_enable(dpi->dpi_ck_cg); > + if (ret) { > + dev_err(dpi->dev, "Failed to enable dpi_ck_cg clock: %d\n", ret); > + goto err_ck_cg; > + } > + > ret = clk_prepare_enable(dpi->pixel_clk); > if (ret) { > dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret); > @@ -466,6 +482,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > return 0; > > err_pixel: > + clk_disable_unprepare(dpi->dpi_ck_cg); > +err_ck_cg: > clk_disable_unprepare(dpi->engine_clk); > err_refcount: > dpi->refcount--; [...]
On Mon, 2022-05-30 at 16:38 +0800, CK Hu wrote: > Hi, Guillaume: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > Add flexibility by moving the swap shift value to SoC specific > > config > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 6eeda222a973..6d4d8c6ec47d 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -131,6 +131,7 @@ struct mtk_dpi_conf { > > u32 dimension_mask; > > /* HSIZE and VSIZE mask (no shift) */ > > u32 hvsize_mask; > > + u32 channel_swap_shift; > > const struct mtk_dpi_yc_limit *limit; > > }; > > > > @@ -349,7 +350,8 @@ static void mtk_dpi_config_channel_swap(struct > > mtk_dpi *dpi, > > break; > > } > > > > - mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << CH_SWAP, > > CH_SWAP_MASK); > > + mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf- > > > channel_swap_shift, > > > > + CH_SWAP_MASK); > > From the definiton: > > #define CH_SWAP 0 > +#define DPINTF_CH_SWAP BIT(1) > #define CH_SWAP_MASK (0x7 << 0) > +#define DPINTF_CH_SWAP_MASK (0x7 << 1) > > This statement should be: > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf- > > channel_swap_shift, CH_SWAP_MASK << dpi->conf->channel_swap_shift); > > dpi->conf->channel_swap_shift is 1 for MT8195-DP_INTF and 0 for > others. > And drop the definition of DPINTF_CH_SWAP and DPINTF_CH_SWAP_MASK, > > Regards, > CK > Hello CK, I think we should keep DPINTF_CH_SWAP? BIT(1) = 2 so the shift should be: dpi->conf->channel_swap_shift is 2 for MT8195-DP_INTF and 0 for others. BRs, Bo-Chen > > > } > > > > static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool > > enable) > > @@ -821,6 +823,7 @@ static const struct mtk_dpi_conf mt8173_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -835,6 +838,7 @@ static const struct mtk_dpi_conf mt2701_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -848,6 +852,7 @@ static const struct mtk_dpi_conf mt8183_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -861,6 +866,7 @@ static const struct mtk_dpi_conf mt8192_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2022-05-30 at 16:38 +0800, CK Hu wrote: > Hi, Guillaume: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > Add flexibility by moving the swap shift value to SoC specific > > config > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 6eeda222a973..6d4d8c6ec47d 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -131,6 +131,7 @@ struct mtk_dpi_conf { > > u32 dimension_mask; > > /* HSIZE and VSIZE mask (no shift) */ > > u32 hvsize_mask; > > + u32 channel_swap_shift; > > const struct mtk_dpi_yc_limit *limit; > > }; > > > > @@ -349,7 +350,8 @@ static void mtk_dpi_config_channel_swap(struct > > mtk_dpi *dpi, > > break; > > } > > > > - mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << CH_SWAP, > > CH_SWAP_MASK); > > + mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf- > > > channel_swap_shift, > > > > + CH_SWAP_MASK); > > From the definiton: > > #define CH_SWAP 0 > +#define DPINTF_CH_SWAP BIT(1) > #define CH_SWAP_MASK (0x7 << 0) > +#define DPINTF_CH_SWAP_MASK (0x7 << 1) > > This statement should be: > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf- > > channel_swap_shift, CH_SWAP_MASK << dpi->conf->channel_swap_shift); > > dpi->conf->channel_swap_shift is 1 for MT8195-DP_INTF and 0 for > others. > And drop the definition of DPINTF_CH_SWAP and DPINTF_CH_SWAP_MASK, > > Regards, > CK > Hello CK, I have checked this with Jitao, it's all shift 1 bit including mask and value. I will modify like this: #define DPINTF_CH_SWAP 1 mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, val << dpi->conf-> channel_swap_shift, CH_SWAP_MASK << dpi->conf->channel_swap_shift); BR, Bo-Chen > > > } > > > > static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool > > enable) > > @@ -821,6 +823,7 @@ static const struct mtk_dpi_conf mt8173_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -835,6 +838,7 @@ static const struct mtk_dpi_conf mt2701_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -848,6 +852,7 @@ static const struct mtk_dpi_conf mt8183_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -861,6 +866,7 @@ static const struct mtk_dpi_conf mt8192_conf = > > { > > .swap_input_support = true, > > .dimension_mask = HPW_MASK, > > .hvsize_mask = HSIZE_MASK, > > + .channel_swap_shift = CH_SWAP, > > .limit = &mtk_dpi_limit, > > }; > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2022-05-23 at 12:57 +0200, Matthias Brugger wrote: > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > We need a commit message here. > Hello Matthias, ok, I will add commit message in next version. Thanks. BRs, Bo-Chen > > --- > > include/drm/drm_edid.h | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > > index 144c495b99c4..37c420423625 100644 > > --- a/include/drm/drm_edid.h > > +++ b/include/drm/drm_edid.h > > @@ -359,12 +359,18 @@ struct edid { > > > > #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] > > << 8)) > > > > -/* Short Audio Descriptor */ > > +/** > > + * struct cea_sad - Short Audio Descriptor. > > + * @format: See HDMI_AUDIO_CODING_TYPE_*. > > + * @channels: max number of channels - 1. > > + * @freq: See CEA_SAD_FREQ_*. > > + * @byte2: meaning depends on format. > > + */ > > struct cea_sad { > > u8 format; > > - u8 channels; /* max number of channels - 1 */ > > + u8 channels; > > u8 freq; > > - u8 byte2; /* meaning depends on format */ > > + u8 byte2; > > }; > > > > struct drm_encoder; > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 2022-05-25 at 14:01 +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > include/drm/drm_edid.h | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > > index 144c495b99c4..37c420423625 100644 > > --- a/include/drm/drm_edid.h > > +++ b/include/drm/drm_edid.h > > @@ -359,12 +359,18 @@ struct edid { > > > > #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)- > > >prod_code[1] << 8)) > > > > -/* Short Audio Descriptor */ > > +/** > > + * struct cea_sad - Short Audio Descriptor. > > Perhaps.... > > * struct cea_sad - CEA Short Audio Descriptor > > ...but that's relative to personal liking and nothing else, it's also > fine as > it is, if you like it more as it is. The ball is yours :-P > > Regardless of any choice about changing the description or not: > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > > Cheers, > Angelo > Hello Angelo, ok, I will do this in next version. BRs, Bo-Chen > > + * @format: See HDMI_AUDIO_CODING_TYPE_*. > > + * @channels: max number of channels - 1. > > + * @freq: See CEA_SAD_FREQ_*. > > + * @byte2: meaning depends on format. > > + */ > > struct cea_sad { > > u8 format; > > - u8 channels; /* max number of channels - 1 */ > > + u8 channels; > > u8 freq; > > - u8 byte2; /* meaning depends on format */ > > + u8 byte2; > > }; > > > > struct drm_encoder; > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Mon, 2022-05-30 at 15:44 +0800, CK Hu wrote: > Hi, Guillaume: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > Adds a bit of flexibility to support SoCs without CK/DE pol support > > It seems that DP_INTF has no CK/DE pol function. If so, could you > explain why DP_INTF has this difference with DPI? > > Regards, > CK > Hello CK, Dp_intf does not support CK/DE polarity because the polarity information is not used for eDP and DP while dp_intf is only for eDP and DP. I will add this in commit message in next version. BRs, Bo-Chen > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 22 +++++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 4746eb342567..545a1337cc89 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -125,6 +125,7 @@ struct mtk_dpi_conf { > > bool edge_sel_en; > > const u32 *output_fmts; > > u32 num_output_fmts; > > + bool is_ck_de_pol; > > const struct mtk_dpi_yc_limit *limit; > > }; > > > > @@ -211,13 +212,20 @@ static void mtk_dpi_config_pol(struct mtk_dpi > > *dpi, > > struct mtk_dpi_polarities *dpi_pol) > > { > > unsigned int pol; > > + unsigned int mask; > > > > - pol = (dpi_pol->ck_pol == MTK_DPI_POLARITY_RISING ? 0 : CK_POL) > > > > > > > - (dpi_pol->de_pol == MTK_DPI_POLARITY_RISING ? 0 : DE_POL) > > > > > > > - (dpi_pol->hsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > > HSYNC_POL) | > > + mask = HSYNC_POL | VSYNC_POL; > > + pol = (dpi_pol->hsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > > HSYNC_POL) | > > (dpi_pol->vsync_pol == MTK_DPI_POLARITY_RISING ? 0 : > > VSYNC_POL); > > - mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, pol, > > - CK_POL | DE_POL | HSYNC_POL | VSYNC_POL); > > + if (dpi->conf->is_ck_de_pol) { > > + mask |= CK_POL | DE_POL; > > + pol |= (dpi_pol->ck_pol == MTK_DPI_POLARITY_RISING ? > > + 0 : CK_POL) | > > + (dpi_pol->de_pol == MTK_DPI_POLARITY_RISING ? > > + 0 : DE_POL); > > + } > > + > > + mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, pol, mask); > > } > > > > static void mtk_dpi_config_3d(struct mtk_dpi *dpi, bool en_3d) > > @@ -799,6 +807,7 @@ static const struct mtk_dpi_conf mt8173_conf = > > { > > .max_clock_khz = 300000, > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -809,6 +818,7 @@ static const struct mtk_dpi_conf mt2701_conf = > > { > > .max_clock_khz = 150000, > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -818,6 +828,7 @@ static const struct mtk_dpi_conf mt8183_conf = > > { > > .max_clock_khz = 100000, > > .output_fmts = mt8183_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -827,6 +838,7 @@ static const struct mtk_dpi_conf mt8192_conf = > > { > > .max_clock_khz = 150000, > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > + .is_ck_de_pol = true, > > .limit = &mtk_dpi_limit, > > }; > > > >
On Wed, 2022-05-25 at 14:32 +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 12:47, Guillaume Ranquet 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> > > --- > > drivers/video/hdmi.c | 82 ++++++++++++++++++++++++++--- > > ----- > > include/drm/dp/drm_dp_helper.h | 2 + > > this has been moved... again... this time it's > include/drm/display/drm_dp_helper.h > Hello Angelo, yes, I will rebase on 5.19-rc1 and fix this in next version. BRs, Bo-Chen > > include/linux/hdmi.h | 7 ++- > > 3 files changed, 71 insertions(+), 20 deletions(-) > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +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_disc > onnected; > + > + if (event < 0) event is always > 0, isn't it? > + return IRQ_HANDLED; > + > + if (mtk_dp->drm_dev) { > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); I think this ISR would come once. If bridge has not attached, the drm core would lost this event. Maybe you should enable eDP hardware after bridge attached or send this event when attached. > + } > + > + if (mtk_dp->train_info.cable_state_change) { Executing this thread imply cable_state_change = true, so drop cable_state_change. > + mtk_dp->train_info.cable_state_change = false; > + > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + > + if (!mtk_dp->train_info.cable_plugged_in || > + !mtk_dp_plug_state(mtk_dp)) { I do not like two variable to present one thing. If mtk_dp->train_info.cable_plugged_in = false and mtk_dp_plug_state(mtk_dp) = ture What does this mean? I think this mean 'now' is connected because cable_plugged_in is old information and mtk_dp_plug_state() is current information. But I would like to keep cable_plugged_in and drop mtk_dp_plug_state() because cable_plugged_in would be changed in isr and it would be the same as mtk_dp_plug_state(). Regards, CK > + 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) > + mtk_dp_fec_enable(mtk_dp, false); > + > + 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); > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + mtk_dp->train_info.link_rate = > + min_t(int, mtk_dp->max_linkrate, > + buf[mtk_dp->max_linkrate]); > + mtk_dp->train_info.lane_count = > + min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(buf)); > + } > + } > + > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > + mtk_dp_hpd_sink_event(mtk_dp); > + } > + > + return IRQ_HANDLED; > +} > +
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) > +{ > + bool training_done = false; > + short max_retry = 50; > + int ret = 0; > + > + do { > + switch (mtk_dp->train_state) { > + case MTK_DP_TRAIN_STATE_STARTUP: mtk_dp->train_state is initialized as MTK_DP_TRAIN_STATE_STARTUP even though HPD ISR does not exist. Does this mean HPD ISR is redundant? If HPD ISR is not redundant, create a new state MTK_DP_TRAIN_STATE_NONE for init state. > + mtk_dp_state_handler(mtk_dp); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_CHECKCAP; > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKCAP: > + if (mtk_dp_parse_capabilities(mtk_dp)) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_CHECKEDID; > + } else { > + mtk_dp->train_info.check_cap_count++; > + > + if (mtk_dp->train_info.check_cap_count > > > + MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT > ) { > + mtk_dp- > >train_info.check_cap_count = 0; > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_DPIDLE; > + ret = -ETIMEDOUT; > + } > + } > + break; > + > + case MTK_DP_TRAIN_STATE_CHECKEDID: > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_TRAINING_PRE; MTK_DP_TRAIN_STATE_CHECKEDID is a redundant state, drop it. > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > + mtk_dp_state_handler(mtk_dp); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_TRAINING; > + break; > + > + case MTK_DP_TRAIN_STATE_TRAINING: > + ret = mtk_dp_train_start(mtk_dp); > + if (ret == 0) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_NORMAL; > + mtk_dp_fec_enable(mtk_dp, mtk_dp- > >has_fec); > + } else if (ret != -EAGAIN) { > + mtk_dp->train_state = > MTK_DP_TRAIN_STATE_DPIDLE; > + } > + break; > + case MTK_DP_TRAIN_STATE_NORMAL: > + mtk_dp_state_handler(mtk_dp); > + training_done = true; > + break; > + case MTK_DP_TRAIN_STATE_DPIDLE: When would this case happen? Regards, CK > + break; > + default: > + break; > + } > + > + if (ret) { > + if (ret == -EAGAIN) > + continue; > + /* > + * If we get any other error number, it doesn't > + * make any sense to keep iterating. > + */ > + break; > + } > + } while (!training_done || --max_retry); > + > + return ret; > +}
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.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 || > + mtk_dp->train_info.irq_status & MTK_DP_HPD_DISCONNECT) { > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP; Changing state here has no any effect, so drop this. > + return -EAGAIN; > + } > + > + 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); > + return -EINVAL; > + } > + > + 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; > + } > + } > + > + 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, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static int mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > +{ > + ssize_t ret; > + u8 sink_count; > + bool locked; > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > + u32 link_status_reg = DP_LANE0_1_STATUS; > + > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, > &sink_count); > + if (ret < 0) { > + drm_err(mtk_dp->drm_dev, "Read sink count failed: > %ld\n", ret); > + return ret; > + } > + > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > link_status, > + sizeof(link_status)); > + if (!ret) { > + drm_err(mtk_dp->drm_dev, "Read link status failed: > %ld\n", > + ret); > + return ret; > + } > + > + locked = drm_dp_channel_eq_ok(link_status, > + mtk_dp->train_info.lane_count); > + if (!locked && mtk_dp->train_state > > MTK_DP_TRAIN_STATE_TRAINING_PRE) Before enter this function, mtk_dp->train_state is set to MTK_DP_TRAIN_STATE_STARTUP, so this never happen, drop this. > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE; > + > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > + drm_dp_dpcd_writeb(&mtk_dp->aux, > DP_DEVICE_SERVICE_IRQ_VECTOR, > + DP_REMOTE_CONTROL_COMMAND_PENDING); > + > + if (DP_GET_SINK_COUNT(sink_count) && > + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > + mtk_dp->train_info.check_cap_count = 0; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID; Why change state from MTK_DP_TRAIN_STATE_STARTUP to MTK_DP_TRAIN_STATE_CHECKEDID? In mtk_dp_train_handler(), mtk_dp_parse_capabilities() is true then change to MTK_DP_TRAIN_STATE_CHECKEDID. Give a reason why these two are different. Regards, CK > + msleep(20); > + } > + > + return 0; > +} > +
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static void mtk_dp_state_handler(struct mtk_dp *mtk_dp) > +{ > + switch (mtk_dp->state) { Does mtk_dp->state has any relation with mtk_dp->train_state. If yes, mix mtk_dp->state and mtk_dp->train_state into one state. If no, move calling mtk_dp_state_handler() out of mtk_dp_train_handler(). Regards, CK > + case MTK_DP_STATE_INITIAL: > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + break; > + > + case MTK_DP_STATE_IDLE: > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > + mtk_dp->state = MTK_DP_STATE_PREPARE; > + break; > + > + case MTK_DP_STATE_PREPARE: > + mtk_dp_video_config(mtk_dp); > + mtk_dp_video_enable(mtk_dp, true); > + > + mtk_dp->state = MTK_DP_STATE_NORMAL; > + break; > + > + case MTK_DP_STATE_NORMAL: > + if (mtk_dp->train_state != MTK_DP_TRAIN_STATE_NORMAL) { > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + } > + break; > + > + default: > + break; > + } > +}
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static int mtk_dp_train_start(struct mtk_dp *mtk_dp) > +{ > + int ret = 0; > + u8 lane_count; > + u8 link_rate; > + u8 train_limit; > + u8 max_link_rate; > + u8 plug_wait; > + > + for (plug_wait = 7; !mtk_dp_plug_state(mtk_dp) && plug_wait > > 0; > + --plug_wait) > + /* Avoid short pulses on the HPD isr */ > + usleep_range(1000, 5000); > + if (plug_wait == 0) { > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE; After return, mtk_dp->train_state would be set to MTK_DP_TRAIN_STATE_DPIDLE, so drop this. > + return -ENODEV; > + } > + > + link_rate = mtk_dp->rx_cap[1]; > + lane_count = mtk_dp->rx_cap[2] & 0x1F; > + > + mtk_dp->train_info.link_rate = min(mtk_dp->max_linkrate, > link_rate); > + mtk_dp->train_info.lane_count = min(mtk_dp->max_lanes, > lane_count); > + link_rate = mtk_dp->train_info.link_rate; > + lane_count = mtk_dp->train_info.lane_count; > + > + switch (link_rate) { > + case MTK_DP_LINKRATE_RBR: > + case MTK_DP_LINKRATE_HBR: > + case MTK_DP_LINKRATE_HBR2: > + case MTK_DP_LINKRATE_HBR25: > + case MTK_DP_LINKRATE_HBR3: > + break; > + default: > + mtk_dp->train_info.link_rate = MTK_DP_LINKRATE_HBR3; > + break; > + }; > + > + max_link_rate = link_rate; > + for (train_limit = 6; train_limit > 0; train_limit--) { > + mtk_dp->train_info.cr_done = false; > + mtk_dp->train_info.eq_done = false; > + > + mtk_dp_train_change_mode(mtk_dp); > + ret = mtk_dp_train_flow(mtk_dp, link_rate, lane_count); > + if (ret) > + return ret; > + > + if (!mtk_dp->train_info.cr_done) { > + switch (link_rate) { > + case MTK_DP_LINKRATE_RBR: > + lane_count = lane_count / 2; > + link_rate = max_link_rate; > + if (lane_count == 0) { > + mtk_dp->train_state = > + MTK_DP_TRAIN_STATE_DPID > LE; After return, mtk_dp->train_state would be set to MTK_DP_TRAIN_STATE_DPIDLE, so drop this. Regards, CK > + return -EIO; > + } > + break; > + case MTK_DP_LINKRATE_HBR: > + link_rate = MTK_DP_LINKRATE_RBR; > + break; > + case MTK_DP_LINKRATE_HBR2: > + link_rate = MTK_DP_LINKRATE_HBR; > + break; > + case MTK_DP_LINKRATE_HBR3: > + link_rate = MTK_DP_LINKRATE_HBR2; > + break; > + default: > + return -EINVAL; > + }; > + } else if (!mtk_dp->train_info.eq_done) { > + if (lane_count == 0) > + return -EIO; > + > + lane_count /= 2; > + } else { > + break; > + } > + } > + > + if (train_limit == 0) > + return -ETIMEDOUT; > + > + return 0; > +} > +
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +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_disc > onnected; > + > + if (event < 0) > + return IRQ_HANDLED; > + > + if (mtk_dp->drm_dev) { > + dev_info(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->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + > + if (!mtk_dp->train_info.cable_plugged_in || > + !mtk_dp_plug_state(mtk_dp)) { > + mtk_dp_video_mute(mtk_dp, true); For eDP, when does 'unplug' happen? Explain this or move unplug processing to DP patch. Regards, CK > + > + mtk_dp_initialize_priv_data(mtk_dp); > + mtk_dp_set_idle_pattern(mtk_dp, true); > + if (mtk_dp->has_fec) > + mtk_dp_fec_enable(mtk_dp, false); > + > + 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); > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + mtk_dp->train_info.link_rate = > + min_t(int, mtk_dp->max_linkrate, > + buf[mtk_dp->max_linkrate]); > + mtk_dp->train_info.lane_count = > + min_t(int, mtk_dp->max_lanes, > + drm_dp_max_lane_count(buf)); > + } > + } > + > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > + mtk_dp_hpd_sink_event(mtk_dp); > + } > + > + return IRQ_HANDLED; > +} > +
On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +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_disc > > onnected; > > + > > + if (event < 0) > > event is always > 0, isn't it? > Hello CK, ok, I will move this to dp patch. > > + return IRQ_HANDLED; > > + > > + if (mtk_dp->drm_dev) { > > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > I think this ISR would come once. If bridge has not attached, the drm > core would lost this event. Maybe you should enable eDP hardware > after > bridge attached or send this event when attached. > for edp patch, I will move it to (mtk_dp_bridge_attach). for dp patch, I will add it back. > > + } > > + > > + if (mtk_dp->train_info.cable_state_change) { > > Executing this thread imply cable_state_change = true, so drop > cable_state_change. > In mtk_dp_hpd_isr_handler(), there is another irq "MTK_DP_HPD_INTERRUPT" which means the sink devices give a interrupt to source device. it's not about connected status, so I think we still need this. > > + mtk_dp->train_info.cable_state_change = false; > > + > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > + > > + if (!mtk_dp->train_info.cable_plugged_in || > > + !mtk_dp_plug_state(mtk_dp)) { > > I do not like two variable to present one thing. If > > mtk_dp->train_info.cable_plugged_in = false > and > mtk_dp_plug_state(mtk_dp) = ture > > What does this mean? I think this mean 'now' is connected because > cable_plugged_in is old information and mtk_dp_plug_state() is > current > information. > > But I would like to keep cable_plugged_in and drop > mtk_dp_plug_state() > because cable_plugged_in would be changed in isr and it would be the > same as mtk_dp_plug_state(). > > Regards, > CK > ok, I will drop this. BRs, Rex > > + 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) > > + mtk_dp_fec_enable(mtk_dp, false); > > + > > + 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); > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > + mtk_dp->train_info.link_rate = > > + min_t(int, mtk_dp->max_linkrate, > > + buf[mtk_dp->max_linkrate]); > > + mtk_dp->train_info.lane_count = > > + min_t(int, mtk_dp->max_lanes, > > + drm_dp_max_lane_count(buf)); > > + } > > + } > > + > > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > > + mtk_dp_hpd_sink_event(mtk_dp); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > >
On Tue, 2022-06-07 at 14:44 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) > > +{ > > + bool training_done = false; > > + short max_retry = 50; > > + int ret = 0; > > + > > + do { > > + switch (mtk_dp->train_state) { > > + case MTK_DP_TRAIN_STATE_STARTUP: > > mtk_dp->train_state is initialized as MTK_DP_TRAIN_STATE_STARTUP even > though HPD ISR does not exist. Does this mean HPD ISR is redundant? > If > HPD ISR is not redundant, create a new state MTK_DP_TRAIN_STATE_NONE > for init state. > Hello CK, I think we don't need MTK_DP_TRAIN_STATE_NONE. Because it's "DP_TRAIN_STATE" not "DP_STATE", I think it's ok if we start this state machine with "MTK_DP_TRAIN_STATE_STARTUP". > > + mtk_dp_state_handler(mtk_dp); > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_CHECKCAP; > > + break; > > + > > + case MTK_DP_TRAIN_STATE_CHECKCAP: > > + if (mtk_dp_parse_capabilities(mtk_dp)) { > > + mtk_dp->train_info.check_cap_count = 0; > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_CHECKEDID; > > + } else { > > + mtk_dp->train_info.check_cap_count++; > > + > > + if (mtk_dp->train_info.check_cap_count > > > > > > > + MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT > > ) { > > + mtk_dp- > > > train_info.check_cap_count = 0; > > > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_DPIDLE; > > + ret = -ETIMEDOUT; > > + } > > + } > > + break; > > + > > + case MTK_DP_TRAIN_STATE_CHECKEDID: > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_TRAINING_PRE; > > MTK_DP_TRAIN_STATE_CHECKEDID is a redundant state, drop it. > > > + break; > > + > > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > > + mtk_dp_state_handler(mtk_dp); > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_TRAINING; > > + break; > > + > > + case MTK_DP_TRAIN_STATE_TRAINING: > > + ret = mtk_dp_train_start(mtk_dp); > > + if (ret == 0) { > > + mtk_dp_video_mute(mtk_dp, true); > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_NORMAL; > > + mtk_dp_fec_enable(mtk_dp, mtk_dp- > > > has_fec); > > > > + } else if (ret != -EAGAIN) { > > + mtk_dp->train_state = > > MTK_DP_TRAIN_STATE_DPIDLE; > > + } > > + break; > > + case MTK_DP_TRAIN_STATE_NORMAL: > > + mtk_dp_state_handler(mtk_dp); > > + training_done = true; > > + break; > > + case MTK_DP_TRAIN_STATE_DPIDLE: > > When would this case happen? > > Regards, > CK Yes, if it's disconnected if we are still training for dp. or failed to training min spec RBR. BRs, Rex > > > + break; > > + default: > > + break; > > + } > > + > > + if (ret) { > > + if (ret == -EAGAIN) > > + continue; > > + /* > > + * If we get any other error number, it doesn't > > + * make any sense to keep iterating. > > + */ > > + break; > > + } > > + } while (!training_done || --max_retry); > > + > > + return ret; > > +} > >
On Tue, 2022-06-07 at 15:30 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.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 || > > + mtk_dp->train_info.irq_status & MTK_DP_HPD_DISCONNECT) { > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP; > > Changing state here has no any effect, so drop this. > ok, I will drop it. > > + return -EAGAIN; > > + } > > + > > + 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); > > + return -EINVAL; > > + } > > + > > + 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; > > + } > > + } > > + > > + 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; > > +} > >
On Tue, 2022-06-07 at 16:12 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static int mtk_dp_train_start(struct mtk_dp *mtk_dp) > > +{ > > + int ret = 0; > > + u8 lane_count; > > + u8 link_rate; > > + u8 train_limit; > > + u8 max_link_rate; > > + u8 plug_wait; > > + > > + for (plug_wait = 7; !mtk_dp_plug_state(mtk_dp) && plug_wait > > > 0; > > + --plug_wait) > > + /* Avoid short pulses on the HPD isr */ > > + usleep_range(1000, 5000); > > + if (plug_wait == 0) { > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE; > > After return, mtk_dp->train_state would be set to > MTK_DP_TRAIN_STATE_DPIDLE, so drop this. > ok, I will do this. > > + return -ENODEV; > > + } > > + > > + link_rate = mtk_dp->rx_cap[1]; > > + lane_count = mtk_dp->rx_cap[2] & 0x1F; > > + > > + mtk_dp->train_info.link_rate = min(mtk_dp->max_linkrate, > > link_rate); > > + mtk_dp->train_info.lane_count = min(mtk_dp->max_lanes, > > lane_count); > > + link_rate = mtk_dp->train_info.link_rate; > > + lane_count = mtk_dp->train_info.lane_count; > > + > > + switch (link_rate) { > > + case MTK_DP_LINKRATE_RBR: > > + case MTK_DP_LINKRATE_HBR: > > + case MTK_DP_LINKRATE_HBR2: > > + case MTK_DP_LINKRATE_HBR25: > > + case MTK_DP_LINKRATE_HBR3: > > + break; > > + default: > > + mtk_dp->train_info.link_rate = MTK_DP_LINKRATE_HBR3; > > + break; > > + }; > > + > > + max_link_rate = link_rate; > > + for (train_limit = 6; train_limit > 0; train_limit--) { > > + mtk_dp->train_info.cr_done = false; > > + mtk_dp->train_info.eq_done = false; > > + > > + mtk_dp_train_change_mode(mtk_dp); > > + ret = mtk_dp_train_flow(mtk_dp, link_rate, lane_count); > > + if (ret) > > + return ret; > > + > > + if (!mtk_dp->train_info.cr_done) { > > + switch (link_rate) { > > + case MTK_DP_LINKRATE_RBR: > > + lane_count = lane_count / 2; > > + link_rate = max_link_rate; > > + if (lane_count == 0) { > > + mtk_dp->train_state = > > + MTK_DP_TRAIN_STATE_DPID > > LE; > > After return, mtk_dp->train_state would be set to > MTK_DP_TRAIN_STATE_DPIDLE, so drop this. > > Regards, > CK > ok. > > + return -EIO; > > + } > > + break; > > + case MTK_DP_LINKRATE_HBR: > > + link_rate = MTK_DP_LINKRATE_RBR; > > + break; > > + case MTK_DP_LINKRATE_HBR2: > > + link_rate = MTK_DP_LINKRATE_HBR; > > + break; > > + case MTK_DP_LINKRATE_HBR3: > > + link_rate = MTK_DP_LINKRATE_HBR2; > > + break; > > + default: > > + return -EINVAL; > > + }; > > + } else if (!mtk_dp->train_info.eq_done) { > > + if (lane_count == 0) > > + return -EIO; > > + > > + lane_count /= 2; > > + } else { > > + break; > > + } > > + } > > + > > + if (train_limit == 0) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > >
Hi, Rex: On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > > Hi, Rex: > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > This patch adds a 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 > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > --- > > > > [snip] > > > > > + > > > +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_disc > > > onnected; > > > + > > > + if (event < 0) > > > > event is always > 0, isn't it? > > > > Hello CK, > > ok, I will move this to dp patch. > > > > + return IRQ_HANDLED; > > > + > > > + if (mtk_dp->drm_dev) { > > > + dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n"); > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > > > I think this ISR would come once. If bridge has not attached, the > > drm > > core would lost this event. Maybe you should enable eDP hardware > > after > > bridge attached or send this event when attached. > > > > for edp patch, I will move it to (mtk_dp_bridge_attach). > for dp patch, I will add it back. I find out that mtk_dp_poweron() is in top of mtk_dp_bridge_attach(). If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(), mtk_dp- >drm_dev would not be NULL here. So we could drop this checking. > > > > + } > > > + > > > + if (mtk_dp->train_info.cable_state_change) { > > > > Executing this thread imply cable_state_change = true, so drop > > cable_state_change. > > > > In mtk_dp_hpd_isr_handler(), there is another irq > "MTK_DP_HPD_INTERRUPT" which means the sink devices give a interrupt > to > source device. it's not about connected status, so I think we still > need this. In bottom of mtk_dp_hpd_isr_handler(), the code is: + train_info->cable_state_change = true; + + return IRQ_WAKE_THREAD; This thread is called only when return IRQ_WAKE_THREAD, and before return IRQ_WAKE_THREAD, train_info->cable_state_change is always set to true. So in this thread, train_info->cable_state_change must be true. Regards, CK > > > > + mtk_dp->train_info.cable_state_change = false; > > > + > > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > > + > > > + if (!mtk_dp->train_info.cable_plugged_in || > > > + !mtk_dp_plug_state(mtk_dp)) { > > > > I do not like two variable to present one thing. If > > > > mtk_dp->train_info.cable_plugged_in = false > > and > > mtk_dp_plug_state(mtk_dp) = ture > > > > What does this mean? I think this mean 'now' is connected because > > cable_plugged_in is old information and mtk_dp_plug_state() is > > current > > information. > > > > But I would like to keep cable_plugged_in and drop > > mtk_dp_plug_state() > > because cable_plugged_in would be changed in isr and it would be > > the > > same as mtk_dp_plug_state(). > > > > Regards, > > CK > > > > ok, I will drop this. > > BRs, > Rex > > > > + 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) > > > + mtk_dp_fec_enable(mtk_dp, false); > > > + > > > + 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); > > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > > + mtk_dp->train_info.link_rate = > > > + min_t(int, mtk_dp->max_linkrate, > > > + buf[mtk_dp->max_linkrate]); > > > + mtk_dp->train_info.lane_count = > > > + min_t(int, mtk_dp->max_lanes, > > > + drm_dp_max_lane_count(buf)); > > > + } > > > + } > > > + > > > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > > > + mtk_dp_hpd_sink_event(mtk_dp); > > > + } > > > + > > > + return IRQ_HANDLED; > > > +} > > > + > > > > > >
Hi, Rex: On Tue, 2022-06-07 at 20:44 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-07 at 14:44 +0800, CK Hu wrote: > > Hi, Rex: > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > This patch adds a 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 > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > --- > > > > [snip] > > > > > + > > > +static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) > > > +{ > > > + bool training_done = false; > > > + short max_retry = 50; > > > + int ret = 0; > > > + > > > + do { > > > + switch (mtk_dp->train_state) { > > > + case MTK_DP_TRAIN_STATE_STARTUP: > > > > mtk_dp->train_state is initialized as MTK_DP_TRAIN_STATE_STARTUP > > even > > though HPD ISR does not exist. Does this mean HPD ISR is redundant? > > If > > HPD ISR is not redundant, create a new state > > MTK_DP_TRAIN_STATE_NONE > > for init state. > > > > Hello CK, > > I think we don't need MTK_DP_TRAIN_STATE_NONE. > Because it's "DP_TRAIN_STATE" not "DP_STATE", I think it's ok if we > start this state machine with "MTK_DP_TRAIN_STATE_STARTUP". The initial state is MTK_DP_TRAIN_STATE_STARTUP, and HPD thread would change state from MTK_DP_TRAIN_STATE_STARTUP to MTK_DP_TRAIN_STATE_STARTUP, this is redundant. So drop the state change in HPD thread. > > > > + mtk_dp_state_handler(mtk_dp); > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_CHECKCAP; > > > + break; > > > + > > > + case MTK_DP_TRAIN_STATE_CHECKCAP: > > > + if (mtk_dp_parse_capabilities(mtk_dp)) { > > > + mtk_dp->train_info.check_cap_count = 0; > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_CHECKEDID; > > > + } else { > > > + mtk_dp->train_info.check_cap_count++; > > > + > > > + if (mtk_dp->train_info.check_cap_count > > > > > > > > > > + MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT > > > ) { > > > + mtk_dp- > > > > train_info.check_cap_count = 0; > > > > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_DPIDLE; > > > + ret = -ETIMEDOUT; > > > + } > > > + } > > > + break; > > > + > > > + case MTK_DP_TRAIN_STATE_CHECKEDID: > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_TRAINING_PRE; > > > > MTK_DP_TRAIN_STATE_CHECKEDID is a redundant state, drop it. > > > > > + break; > > > + > > > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > > > + mtk_dp_state_handler(mtk_dp); > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_TRAINING; > > > + break; > > > + > > > + case MTK_DP_TRAIN_STATE_TRAINING: > > > + ret = mtk_dp_train_start(mtk_dp); > > > + if (ret == 0) { > > > + mtk_dp_video_mute(mtk_dp, true); > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_NORMAL; > > > + mtk_dp_fec_enable(mtk_dp, mtk_dp- > > > > has_fec); > > > > > > + } else if (ret != -EAGAIN) { > > > + mtk_dp->train_state = > > > MTK_DP_TRAIN_STATE_DPIDLE; > > > + } > > > + break; > > > + case MTK_DP_TRAIN_STATE_NORMAL: > > > + mtk_dp_state_handler(mtk_dp); > > > + training_done = true; > > > + break; > > > + case MTK_DP_TRAIN_STATE_DPIDLE: > > > > When would this case happen? > > > > Regards, > > CK > > Yes, if it's disconnected if we are still training for dp. > or failed to training min spec RBR. I mean, every time state change to MTK_DP_TRAIN_STATE_DPIDLE, it would jump out of this loop and would not get into this loop, so this case would never get in. This is redundant, so remove this. Regards, CK > > BRs, > Rex > > > > > + break; > > > + default: > > > + break; > > > + } > > > + > > > + if (ret) { > > > + if (ret == -EAGAIN) > > > + continue; > > > + /* > > > + * If we get any other error number, it doesn't > > > + * make any sense to keep iterating. > > > + */ > > > + break; > > > + } > > > + } while (!training_done || --max_retry); > > > + > > > + return ret; > > > +} > > > > > >
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static bool mtk_dp_parse_capabilities(struct mtk_dp *mtk_dp) > +{ > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > + u8 val; > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + > + if (!mtk_dp_plug_state(mtk_dp)) > + return false; > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, > DP_SET_POWER_D0); > + /* Wait for power on */ > + usleep_range(2000, 5000); > + > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > + > + memcpy(mtk_dp->rx_cap, buf, min(sizeof(mtk_dp->rx_cap), > sizeof(buf))); sizeof(mtk_dp->rx_cap) is identical to sizeof(buf), so drm_dp_read_dpcd_caps(&mtk_dp->aux, mtk_dp->rx_cap); > + mtk_dp->rx_cap[DP_TRAINING_AUX_RD_INTERVAL] &= > DP_TRAINING_AUX_RD_MASK; > + > + train_info->link_rate = > + min_t(int, mtk_dp->max_linkrate, buf[mtk_dp- > >max_linkrate]); > + train_info->lane_count = > + min_t(int, mtk_dp->max_lanes, > drm_dp_max_lane_count(buf)); > + > + train_info->tps3 = drm_dp_tps3_supported(buf); > + train_info->tps4 = drm_dp_tps4_supported(buf); > + > + train_info->sink_ssc = > + !!(buf[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5); > + > + train_info->sink_ssc = false; What does these two statement do? Regards, CK > + > + drm_dp_dpcd_readb(&mtk_dp->aux, DP_MSTM_CAP, &val); > + if (val & DP_MST_CAP) { > + /* Clear DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0 */ > + drm_dp_dpcd_readb(&mtk_dp->aux, > + DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0, > &val); > + if (val) > + drm_dp_dpcd_writeb(&mtk_dp->aux, > + DP_DEVICE_SERVICE_IRQ_VECTOR > _ESI0, > + val); > + } > + > + return true; > +} > +
On Wed, 2022-06-08 at 10:23 +0800, CK Hu wrote: > Hi, Rex: > > On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote: > > On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > > > Hi, Rex: > > > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > > > This patch adds a 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 > > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > --- > > > > > > [snip] > > > > > > > + > > > > +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_sta > > > > tus_disc > > > > onnected; > > > > + > > > > + if (event < 0) > > > > > > event is always > 0, isn't it? > > > > > > > Hello CK, > > > > ok, I will move this to dp patch. > > > > > > + return IRQ_HANDLED; > > > > + > > > > + if (mtk_dp->drm_dev) { > > > > + dev_info(mtk_dp->dev, > > > > "drm_helper_hpd_irq_event\n"); > > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > > > > > I think this ISR would come once. If bridge has not attached, the > > > drm > > > core would lost this event. Maybe you should enable eDP hardware > > > after > > > bridge attached or send this event when attached. > > > > > > > for edp patch, I will move it to (mtk_dp_bridge_attach). > > for dp patch, I will add it back. > > I find out that mtk_dp_poweron() is in top of mtk_dp_bridge_attach(). > If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(), mtk_dp- > > drm_dev would not be NULL here. So we could drop this checking. > > Hello CK, If we failed to setup phy(ret!=0), we alos need to deattach this bridge. I don't think it's a good idea just for remove this. > > > > + } > > > > + > > > > + if (mtk_dp->train_info.cable_state_change) { > > > > > > Executing this thread imply cable_state_change = true, so drop > > > cable_state_change. > > > > > > > In mtk_dp_hpd_isr_handler(), there is another irq > > "MTK_DP_HPD_INTERRUPT" which means the sink devices give a > > interrupt > > to > > source device. it's not about connected status, so I think we still > > need this. > > In bottom of mtk_dp_hpd_isr_handler(), the code is: > > + train_info->cable_state_change = true; > + > + return IRQ_WAKE_THREAD; > > This thread is called only when return IRQ_WAKE_THREAD, and before > return IRQ_WAKE_THREAD, train_info->cable_state_change is always set > to > true. So in this thread, train_info->cable_state_change must be true. > As mentioned, this irq handler function is not only for connected status. this could be return if this irq is interrupt from sink device. + if (!(train_info->irq_status & + (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT))) + return IRQ_HANDLED; BRs, Bo-Chen > Regards, > CK > > > > > > > + mtk_dp->train_info.cable_state_change = false; > > > > + > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_STARTUP; > > > > + > > > > + if (!mtk_dp->train_info.cable_plugged_in || > > > > + !mtk_dp_plug_state(mtk_dp)) { > > > > > > I do not like two variable to present one thing. If > > > > > > mtk_dp->train_info.cable_plugged_in = false > > > and > > > mtk_dp_plug_state(mtk_dp) = ture > > > > > > What does this mean? I think this mean 'now' is connected because > > > cable_plugged_in is old information and mtk_dp_plug_state() is > > > current > > > information. > > > > > > But I would like to keep cable_plugged_in and drop > > > mtk_dp_plug_state() > > > because cable_plugged_in would be changed in isr and it would be > > > the > > > same as mtk_dp_plug_state(). > > > > > > Regards, > > > CK > > > > > > > ok, I will drop this. > > > > BRs, > > Rex > > > > > > + 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) > > > > + mtk_dp_fec_enable(mtk_dp, > > > > false); > > > > + > > > > + 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); > > > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, > > > > buf); > > > > + mtk_dp->train_info.link_rate = > > > > + min_t(int, mtk_dp- > > > > >max_linkrate, > > > > + buf[mtk_dp- > > > > >max_linkrate]); > > > > + mtk_dp->train_info.lane_count = > > > > + min_t(int, mtk_dp->max_lanes, > > > > + drm_dp_max_lane_count(buf > > > > )); > > > > + } > > > > + } > > > > + > > > > + if (mtk_dp->train_info.irq_status & > > > > MTK_DP_HPD_INTERRUPT) { > > > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > > > + mtk_dp->train_info.irq_status &= > > > > ~MTK_DP_HPD_INTERRUPT; > > > > + mtk_dp_hpd_sink_event(mtk_dp); > > > > + } > > > > + > > > > + return IRQ_HANDLED; > > > > +} > > > > + > > > > > > > > > > > >
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static bool mtk_dp_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, int > lane_num, > + int swing_val, int > preemphasis) The return value is never processed, so let this function to be void. Regards, CK > +{ > + int ret; > + > + u32 lane_shift = lane_num * DP_TX1_VOLT_SWING_SHIFT; > + > + if (lane_num < 0 || lane_num > 3) lane_num < 0 would not happen. lane_num > 3 only if device tree max lane is wrong. So I would like to checkout max lane when parsing device tree instead of checking here. > + return false; > + > + dev_dbg(mtk_dp->dev, > + "link training swing_val= 0x%x, preemphasis = 0x%x\n", > + swing_val, preemphasis); > + > + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_SWING_EMP, > + swing_val << (DP_TX0_VOLT_SWING_SHIFT > + lane_shift), > + DP_TX0_VOLT_SWING_MASK << lane_shift); > + if (ret) > + return ret; > + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_SWING_EMP, > + preemphasis << (DP_TX0_PRE_EMPH_SHIFT > + lane_shift), > + DP_TX0_PRE_EMPH_MASK << lane_shift); > + > + return !ret; > +} > +
On Wed, 2022-06-08 at 16:45 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static bool mtk_dp_set_swing_pre_emphasis(struct mtk_dp *mtk_dp, > > int > > lane_num, > > + int swing_val, int > > preemphasis) > > The return value is never processed, so let this function to be void. > > Regards, > CK > Hello CK, ok, I will drop this. Actually, I change "mtk_dp_write", "mtk_dp_update_bits" and "mtk_dp_bulk_16bit_write" to return void. I don't think we need to handle the issue that we failed to set registers. If we failed to set register, it's because hw is not enable. Therefore, I drop this and we can reduce many lines of codes. BRs, Bo-Chen > > +{ > > + int ret; > > + > > + u32 lane_shift = lane_num * DP_TX1_VOLT_SWING_SHIFT; > > + > > + if (lane_num < 0 || lane_num > 3) > > lane_num < 0 would not happen. lane_num > 3 only if device tree max > lane is wrong. So I would like to checkout max lane when parsing > device > tree instead of checking here. > > + return false; > > + > > + dev_dbg(mtk_dp->dev, > > + "link training swing_val= 0x%x, preemphasis = 0x%x\n", > > + swing_val, preemphasis); > > + > > + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_SWING_EMP, > > + swing_val << (DP_TX0_VOLT_SWING_SHIFT > > + lane_shift), > > + DP_TX0_VOLT_SWING_MASK << lane_shift); > > + if (ret) > > + return ret; > > + ret = mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_SWING_EMP, > > + preemphasis << (DP_TX0_PRE_EMPH_SHIFT > > + lane_shift), > > + DP_TX0_PRE_EMPH_MASK << lane_shift); > > + > > + return !ret; > > +} > > + > >
Hi, Rex: On Wed, 2022-06-08 at 16:43 +0800, Rex-BC Chen wrote: > On Wed, 2022-06-08 at 10:23 +0800, CK Hu wrote: > > Hi, Rex: > > > > On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote: > > > On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > > > > Hi, Rex: > > > > > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > > > > > This patch adds a 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 > > > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > > --- > > > > > > > > [snip] > > > > > > > > > + > > > > > +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_sta > > > > > tus_disc > > > > > onnected; > > > > > + > > > > > + if (event < 0) > > > > > > > > event is always > 0, isn't it? > > > > > > > > > > Hello CK, > > > > > > ok, I will move this to dp patch. > > > > > > > > + return IRQ_HANDLED; > > > > > + > > > > > + if (mtk_dp->drm_dev) { > > > > > + dev_info(mtk_dp->dev, > > > > > "drm_helper_hpd_irq_event\n"); > > > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > > > > > > > I think this ISR would come once. If bridge has not attached, > > > > the > > > > drm > > > > core would lost this event. Maybe you should enable eDP > > > > hardware > > > > after > > > > bridge attached or send this event when attached. > > > > > > > > > > for edp patch, I will move it to (mtk_dp_bridge_attach). > > > for dp patch, I will add it back. > > > > I find out that mtk_dp_poweron() is in top of > > mtk_dp_bridge_attach(). > > If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(), > > mtk_dp- > > > drm_dev would not be NULL here. So we could drop this checking. > > > > > Hello CK, > > If we failed to setup phy(ret!=0), we alos need to deattach this > bridge. > I don't think it's a good idea just for remove this. OK, move mtk_dp_hwirq_enable() out of mtk_dp_poweron() and to the bottom of mtk_dp_bridge_attach(). irq is not part of power. > > > > > > + } > > > > > + > > > > > + if (mtk_dp->train_info.cable_state_change) { > > > > > > > > Executing this thread imply cable_state_change = true, so drop > > > > cable_state_change. > > > > > > > > > > In mtk_dp_hpd_isr_handler(), there is another irq > > > "MTK_DP_HPD_INTERRUPT" which means the sink devices give a > > > interrupt > > > to > > > source device. it's not about connected status, so I think we > > > still > > > need this. > > > > In bottom of mtk_dp_hpd_isr_handler(), the code is: > > > > + train_info->cable_state_change = true; > > + > > + return IRQ_WAKE_THREAD; > > > > This thread is called only when return IRQ_WAKE_THREAD, and before > > return IRQ_WAKE_THREAD, train_info->cable_state_change is always > > set > > to > > true. So in this thread, train_info->cable_state_change must be > > true. > > > > As mentioned, this irq handler function is not only for connected > status. > > this could be return if this irq is interrupt from sink device. > + if (!(train_info->irq_status & > + (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT))) > + return IRQ_HANDLED; According to [1], return IRQ_WAKE_THREAD to wake up thread. So return IRQ_HANDLED would not wake up thread. [1] https://www.kernel.org/doc/htmldocs/kernel-api/API-request-threaded-irq.html Regards, CK > > BRs, > Bo-Chen > > Regards, > > CK > > > > > > > > > > + mtk_dp->train_info.cable_state_change = false; > > > > > + > > > > > + mtk_dp->train_state = > > > > > MTK_DP_TRAIN_STATE_STARTUP; > > > > > + > > > > > + if (!mtk_dp->train_info.cable_plugged_in || > > > > > + !mtk_dp_plug_state(mtk_dp)) { > > > > > > > > I do not like two variable to present one thing. If > > > > > > > > mtk_dp->train_info.cable_plugged_in = false > > > > and > > > > mtk_dp_plug_state(mtk_dp) = ture > > > > > > > > What does this mean? I think this mean 'now' is connected > > > > because > > > > cable_plugged_in is old information and mtk_dp_plug_state() is > > > > current > > > > information. > > > > > > > > But I would like to keep cable_plugged_in and drop > > > > mtk_dp_plug_state() > > > > because cable_plugged_in would be changed in isr and it would > > > > be > > > > the > > > > same as mtk_dp_plug_state(). > > > > > > > > Regards, > > > > CK > > > > > > > > > > ok, I will drop this. > > > > > > BRs, > > > Rex > > > > > > > > + 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) > > > > > + mtk_dp_fec_enable(mtk_dp, > > > > > false); > > > > > + > > > > > + 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); > > > > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, > > > > > buf); > > > > > + mtk_dp->train_info.link_rate = > > > > > + min_t(int, mtk_dp- > > > > > > max_linkrate, > > > > > > > > > > + buf[mtk_dp- > > > > > > max_linkrate]); > > > > > > > > > > + mtk_dp->train_info.lane_count = > > > > > + min_t(int, mtk_dp->max_lanes, > > > > > + drm_dp_max_lane_count(buf > > > > > )); > > > > > + } > > > > > + } > > > > > + > > > > > + if (mtk_dp->train_info.irq_status & > > > > > MTK_DP_HPD_INTERRUPT) { > > > > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > > > > + mtk_dp->train_info.irq_status &= > > > > > ~MTK_DP_HPD_INTERRUPT; > > > > > + mtk_dp_hpd_sink_event(mtk_dp); > > > > > + } > > > > > + > > > > > + return IRQ_HANDLED; > > > > > +} > > > > > + > > > > > > > > > > > > > > > > > > > >
On Tue, 2022-06-07 at 15:47 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static int mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > > +{ > > + ssize_t ret; > > + u8 sink_count; > > + bool locked; > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > > + u32 link_status_reg = DP_LANE0_1_STATUS; > > + > > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, > > &sink_count); > > + if (ret < 0) { > > + drm_err(mtk_dp->drm_dev, "Read sink count failed: > > %ld\n", ret); > > + return ret; > > + } > > + > > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > > link_status, > > + sizeof(link_status)); > > + if (!ret) { > > + drm_err(mtk_dp->drm_dev, "Read link status failed: > > %ld\n", > > + ret); > > + return ret; > > + } > > + > > + locked = drm_dp_channel_eq_ok(link_status, > > + mtk_dp->train_info.lane_count); > > + if (!locked && mtk_dp->train_state > > > MTK_DP_TRAIN_STATE_TRAINING_PRE) > > Before enter this function, mtk_dp->train_state is set to > MTK_DP_TRAIN_STATE_STARTUP, so this never happen, drop this. > The interrupt from sink device could come any time. Why it's impossible? > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE; > > + > > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > + DP_REMOTE_CONTROL_COMMAND_PENDING); > > + > > + if (DP_GET_SINK_COUNT(sink_count) && > > + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > > + mtk_dp->train_info.check_cap_count = 0; > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID; > > Why change state from MTK_DP_TRAIN_STATE_STARTUP to > MTK_DP_TRAIN_STATE_CHECKEDID? In mtk_dp_train_handler(), > mtk_dp_parse_capabilities() is true then change to > MTK_DP_TRAIN_STATE_CHECKEDID. Give a reason why these two are > different. > > Regards, > CK > I will drop this and drop state of MTK_DP_TRAIN_STATE_CHECKEDID. MTK_DP_TRAIN_STATE_CHECKEDID is only used for audio. We can check enable status in another place. BRs, Bo-Chen > > + msleep(20); > > + } > > + > > + return 0; > > +} > > + > >
On Wed, 2022-06-08 at 17:15 +0800, CK Hu wrote: > Hi, Rex: > > On Wed, 2022-06-08 at 16:43 +0800, Rex-BC Chen wrote: > > On Wed, 2022-06-08 at 10:23 +0800, CK Hu wrote: > > > Hi, Rex: > > > > > > On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote: > > > > On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote: > > > > > Hi, Rex: > > > > > > > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > > > > > > > This patch adds a 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 > > > > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > > > --- > > > > > > > > > > [snip] > > > > > > > > > > > + > > > > > > +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_sta > > > > > > tus_disc > > > > > > onnected; > > > > > > + > > > > > > + if (event < 0) > > > > > > > > > > event is always > 0, isn't it? > > > > > > > > > > > > > Hello CK, > > > > > > > > ok, I will move this to dp patch. > > > > > > > > > > + return IRQ_HANDLED; > > > > > > + > > > > > > + if (mtk_dp->drm_dev) { > > > > > > + dev_info(mtk_dp->dev, > > > > > > "drm_helper_hpd_irq_event\n"); > > > > > > + drm_helper_hpd_irq_event(mtk_dp->bridge.dev); > > > > > > > > > > I think this ISR would come once. If bridge has not attached, > > > > > the > > > > > drm > > > > > core would lost this event. Maybe you should enable eDP > > > > > hardware > > > > > after > > > > > bridge attached or send this event when attached. > > > > > > > > > > > > > for edp patch, I will move it to (mtk_dp_bridge_attach). > > > > for dp patch, I will add it back. > > > > > > I find out that mtk_dp_poweron() is in top of > > > mtk_dp_bridge_attach(). > > > If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(), > > > mtk_dp- > > > > drm_dev would not be NULL here. So we could drop this checking. > > > > > > > > Hello CK, > > > > If we failed to setup phy(ret!=0), we alos need to deattach this > > bridge. > > I don't think it's a good idea just for remove this. > > OK, move mtk_dp_hwirq_enable() out of mtk_dp_poweron() and to the > bottom of mtk_dp_bridge_attach(). irq is not part of power. > I will do this and drop "if (mtk_dp->drm_dev)" > > > > > > > > + } > > > > > > + > > > > > > + if (mtk_dp->train_info.cable_state_change) { > > > > > > > > > > Executing this thread imply cable_state_change = true, so > > > > > drop > > > > > cable_state_change. > > > > > > > > > > > > > In mtk_dp_hpd_isr_handler(), there is another irq > > > > "MTK_DP_HPD_INTERRUPT" which means the sink devices give a > > > > interrupt > > > > to > > > > source device. it's not about connected status, so I think we > > > > still > > > > need this. > > > > > > In bottom of mtk_dp_hpd_isr_handler(), the code is: > > > > > > + train_info->cable_state_change = true; > > > + > > > + return IRQ_WAKE_THREAD; > > > > > > This thread is called only when return IRQ_WAKE_THREAD, and > > > before > > > return IRQ_WAKE_THREAD, train_info->cable_state_change is always > > > set > > > to > > > true. So in this thread, train_info->cable_state_change must be > > > true. > > > > > > > As mentioned, this irq handler function is not only for connected > > status. > > > > this could be return if this irq is interrupt from sink device. > > + if (!(train_info->irq_status & > > + (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT))) > > + return IRQ_HANDLED; > > According to [1], return IRQ_WAKE_THREAD to wake up thread. So return > IRQ_HANDLED would not wake up thread. > > [1] > https://www.kernel.org/doc/htmldocs/kernel-api/API-request-threaded-irq.html > > Regards, > CK > yes, you are right. I will return IRQ_WAKE_THREAD for handle sink interrupt. > > > > BRs, > > Bo-Chen > > > Regards, > > > CK > > > > > > > > > > > > > + mtk_dp->train_info.cable_state_change = false; > > > > > > + > > > > > > + mtk_dp->train_state = > > > > > > MTK_DP_TRAIN_STATE_STARTUP; > > > > > > + > > > > > > + if (!mtk_dp->train_info.cable_plugged_in || > > > > > > + !mtk_dp_plug_state(mtk_dp)) { > > > > > > > > > > I do not like two variable to present one thing. If > > > > > > > > > > mtk_dp->train_info.cable_plugged_in = false > > > > > and > > > > > mtk_dp_plug_state(mtk_dp) = ture > > > > > > > > > > What does this mean? I think this mean 'now' is connected > > > > > because > > > > > cable_plugged_in is old information and mtk_dp_plug_state() > > > > > is > > > > > current > > > > > information. > > > > > > > > > > But I would like to keep cable_plugged_in and drop > > > > > mtk_dp_plug_state() > > > > > because cable_plugged_in would be changed in isr and it would > > > > > be > > > > > the > > > > > same as mtk_dp_plug_state(). > > > > > > > > > > Regards, > > > > > CK > > > > > > > > > > > > > ok, I will drop this. > > > > > > > > BRs, > > > > Rex > > > > > > > > > > + 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) > > > > > > + mtk_dp_fec_enable(mtk_dp, > > > > > > false); > > > > > > + > > > > > > + 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); > > > > > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, > > > > > > buf); > > > > > > + mtk_dp->train_info.link_rate = > > > > > > + min_t(int, mtk_dp- > > > > > > > max_linkrate, > > > > > > > > > > > > + buf[mtk_dp- > > > > > > > max_linkrate]); > > > > > > > > > > > > + mtk_dp->train_info.lane_count = > > > > > > + min_t(int, mtk_dp->max_lanes, > > > > > > + drm_dp_max_lane_count(buf > > > > > > )); > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + if (mtk_dp->train_info.irq_status & > > > > > > MTK_DP_HPD_INTERRUPT) { > > > > > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > > > > > + mtk_dp->train_info.irq_status &= > > > > > > ~MTK_DP_HPD_INTERRUPT; > > > > > > + mtk_dp_hpd_sink_event(mtk_dp); > > > > > > + } > > > > > > + > > > > > > + return IRQ_HANDLED; > > > > > > +} > > > > > > + > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Wed, 2022-06-08 at 10:44 +0800, CK Hu wrote: > Hi, Rex: > > On Tue, 2022-06-07 at 20:44 +0800, Rex-BC Chen wrote: > > On Tue, 2022-06-07 at 14:44 +0800, CK Hu wrote: > > > Hi, Rex: > > > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > > > This patch adds a 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 > > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > --- > > > > > > [snip] > > > > > > > + > > > > +static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) > > > > +{ > > > > + bool training_done = false; > > > > + short max_retry = 50; > > > > + int ret = 0; > > > > + > > > > + do { > > > > + switch (mtk_dp->train_state) { > > > > + case MTK_DP_TRAIN_STATE_STARTUP: > > > > > > mtk_dp->train_state is initialized as MTK_DP_TRAIN_STATE_STARTUP > > > even > > > though HPD ISR does not exist. Does this mean HPD ISR is > > > redundant? > > > If > > > HPD ISR is not redundant, create a new state > > > MTK_DP_TRAIN_STATE_NONE > > > for init state. > > > > > > > Hello CK, > > > > I think we don't need MTK_DP_TRAIN_STATE_NONE. > > Because it's "DP_TRAIN_STATE" not "DP_STATE", I think it's ok if we > > start this state machine with "MTK_DP_TRAIN_STATE_STARTUP". > > The initial state is MTK_DP_TRAIN_STATE_STARTUP, and HPD thread would > change state from MTK_DP_TRAIN_STATE_STARTUP to > MTK_DP_TRAIN_STATE_STARTUP, this is redundant. So drop the state > change > in HPD thread. > ok, in edp it's redundant in mtk_dp_hpd_event_thread(), but I will add it back in dp patch. > > > > > > + mtk_dp_state_handler(mtk_dp); > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_CHECKCAP; > > > > + break; > > > > + > > > > + case MTK_DP_TRAIN_STATE_CHECKCAP: > > > > + if (mtk_dp_parse_capabilities(mtk_dp)) > > > > { > > > > + mtk_dp- > > > > >train_info.check_cap_count = 0; > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_CHECKEDID; > > > > + } else { > > > > + mtk_dp- > > > > >train_info.check_cap_count++; > > > > + > > > > + if (mtk_dp- > > > > >train_info.check_cap_count > > > > > > > > > > > > > + MTK_DP_CHECK_SINK_CAP_TIMEO > > > > UT_COUNT > > > > ) { > > > > + mtk_dp- > > > > > train_info.check_cap_count = 0; > > > > > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_DPIDLE; > > > > + ret = -ETIMEDOUT; > > > > + } > > > > + } > > > > + break; > > > > + > > > > + case MTK_DP_TRAIN_STATE_CHECKEDID: > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_TRAINING_PRE; > > > > > > MTK_DP_TRAIN_STATE_CHECKEDID is a redundant state, drop it. > > > > > > > + break; > > > > + > > > > + case MTK_DP_TRAIN_STATE_TRAINING_PRE: > > > > + mtk_dp_state_handler(mtk_dp); > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_TRAINING; > > > > + break; > > > > + > > > > + case MTK_DP_TRAIN_STATE_TRAINING: > > > > + ret = mtk_dp_train_start(mtk_dp); > > > > + if (ret == 0) { > > > > + mtk_dp_video_mute(mtk_dp, > > > > true); > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_NORMAL; > > > > + mtk_dp_fec_enable(mtk_dp, > > > > mtk_dp- > > > > > has_fec); > > > > > > > > + } else if (ret != -EAGAIN) { > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_DPIDLE; > > > > + } > > > > + break; > > > > + case MTK_DP_TRAIN_STATE_NORMAL: > > > > + mtk_dp_state_handler(mtk_dp); > > > > + training_done = true; > > > > + break; > > > > + case MTK_DP_TRAIN_STATE_DPIDLE: > > > > > > When would this case happen? > > > > > > Regards, > > > CK > > > > Yes, if it's disconnected if we are still training for dp. > > or failed to training min spec RBR. > > I mean, every time state change to MTK_DP_TRAIN_STATE_DPIDLE, it > would > jump out of this loop and would not get into this loop, so this case > would never get in. This is redundant, so remove this. > > Regards, > CK > ok, I will remove it BRs, Bo-Chen > > > > BRs, > > Rex > > > > > > > + break; > > > > + default: > > > > + break; > > > > + } > > > > + > > > > + if (ret) { > > > > + if (ret == -EAGAIN) > > > > + continue; > > > > + /* > > > > + * If we get any other error number, it > > > > doesn't > > > > + * make any sense to keep iterating. > > > > + */ > > > > + break; > > > > + } > > > > + } while (!training_done || --max_retry); > > > > + > > > > + return ret; > > > > +} > > > > > > > > > > > >
On 23-05-22, 12:47, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This is a new driver that supports the integrated DisplayPort phy for > mediatek SoCs, especially the mt8195. The phy is integrated into the > DisplayPort controller and will be created by the mtk-dp driver. This > driver expects a struct regmap to be able to work on the same registers > as the DisplayPort controller. It sets the device data to be the struct > phy so that the DisplayPort controller can easily work with it. > > The driver does not have any devicetree bindings because the datasheet > does not list the controller and the phy as distinct units. > > The interaction with the controller can be covered by the configure > callback of the phy framework and its displayport parameters. I must admit that I have missed previous iteration of this driver. This is a standalone phy driver, pls split and submit this and we can get this merged... > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- > MAINTAINERS | 1 + > drivers/phy/mediatek/Kconfig | 8 ++ > drivers/phy/mediatek/Makefile | 1 + > drivers/phy/mediatek/phy-mtk-dp.c | 200 ++++++++++++++++++++++++++++++ > 4 files changed, 210 insertions(+) > create mode 100644 drivers/phy/mediatek/phy-mtk-dp.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4cc47b2dbdc9..bfca96469d80 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6604,6 +6604,7 @@ L: linux-mediatek@lists.infradead.org (moderated for non-subscribers) > S: Supported > F: Documentation/devicetree/bindings/display/mediatek/ > F: drivers/gpu/drm/mediatek/ > +F: drivers/phy/mediatek/phy-mtk-dp.c > F: drivers/phy/mediatek/phy-mtk-hdmi* > F: drivers/phy/mediatek/phy-mtk-mipi* > > diff --git a/drivers/phy/mediatek/Kconfig b/drivers/phy/mediatek/Kconfig > index 55f8e6c048ab..f7ec86059049 100644 > --- a/drivers/phy/mediatek/Kconfig > +++ b/drivers/phy/mediatek/Kconfig > @@ -55,3 +55,11 @@ config PHY_MTK_MIPI_DSI > select GENERIC_PHY > help > Support MIPI DSI for Mediatek SoCs. > + > +config PHY_MTK_DP > + tristate "MediaTek DP-PHY Driver" > + depends on ARCH_MEDIATEK || COMPILE_TEST > + depends on OF > + select GENERIC_PHY > + help > + Support DisplayPort PHY for Mediatek SoCs. > diff --git a/drivers/phy/mediatek/Makefile b/drivers/phy/mediatek/Makefile > index ace660fbed3a..4ba1e0650434 100644 > --- a/drivers/phy/mediatek/Makefile > +++ b/drivers/phy/mediatek/Makefile > @@ -3,6 +3,7 @@ > # Makefile for the phy drivers. > # > > +obj-$(CONFIG_PHY_MTK_DP) += phy-mtk-dp.o > obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o > obj-$(CONFIG_PHY_MTK_UFS) += phy-mtk-ufs.o > obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o > diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c > new file mode 100644 > index 000000000000..6f29854f0c2f > --- /dev/null > +++ b/drivers/phy/mediatek/phy-mtk-dp.c > @@ -0,0 +1,200 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MediaTek DisplayPort PHY driver > + * > + * Copyright (c) 2021 BayLibre 2022 now > + * Author: Markus Schneider-Pargmann <msp@baylibre.com> use MODULE_AUTHOR() > + */ > + > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#define PHY_OFFSET 0x1000 > + > +#define MTK_DP_PHY_DIG_PLL_CTL_1 (PHY_OFFSET + 0x14) > +#define TPLL_SSC_EN BIT(3) > + > +#define MTK_DP_PHY_DIG_BIT_RATE (PHY_OFFSET + 0x3C) > +#define BIT_RATE_RBR 0 > +#define BIT_RATE_HBR 1 > +#define BIT_RATE_HBR2 2 > +#define BIT_RATE_HBR3 3 > + > +#define MTK_DP_PHY_DIG_SW_RST (PHY_OFFSET + 0x38) > +#define DP_GLB_SW_RST_PHYD BIT(0) > + > +#define MTK_DP_LANE0_DRIVING_PARAM_3 (PHY_OFFSET + 0x138) > +#define MTK_DP_LANE1_DRIVING_PARAM_3 (PHY_OFFSET + 0x238) > +#define MTK_DP_LANE2_DRIVING_PARAM_3 (PHY_OFFSET + 0x338) > +#define MTK_DP_LANE3_DRIVING_PARAM_3 (PHY_OFFSET + 0x438) > +#define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT BIT(4) > +#define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT ((BIT(2) | BIT(4)) << 8) Sound like BIT(10) and BIT (12), no? > +#define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT GENMASK(20, 19) > +#define XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT GENMASK(29, 29) > +#define DRIVING_PARAM_3_DEFAULT (XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT) > + > +#define XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT GENMASK(4, 3) > +#define XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT GENMASK(12, 9) > +#define XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT ((BIT(2) | BIT(5)) << 16) Here too > +#define XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT GENMASK(29, 29) > +#define DRIVING_PARAM_4_DEFAULT (XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT) > + > +#define XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT (BIT(3) | BIT(5)) > +#define XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT GENMASK(13, 12) > +#define DRIVING_PARAM_5_DEFAULT (XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT) > + > +#define XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT 0 > +#define XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT GENMASK(10, 10) > +#define XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT GENMASK(19, 19) > +#define XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT GENMASK(28, 28) > +#define DRIVING_PARAM_6_DEFAULT (XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT) > + > +#define XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT 0 > +#define XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT GENMASK(10, 9) > +#define XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT GENMASK(19, 18) > +#define XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT 0 > +#define DRIVING_PARAM_7_DEFAULT (XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT) > + > +#define XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT GENMASK(3, 3) > +#define XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT 0 > +#define DRIVING_PARAM_8_DEFAULT (XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \ > + XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT) > + > +struct mtk_dp_phy { > + struct regmap *regs; > +}; > + > +static int mtk_dp_phy_init(struct phy *phy) > +{ > + struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy); > + u32 driving_params[] = { > + DRIVING_PARAM_3_DEFAULT, > + DRIVING_PARAM_4_DEFAULT, > + DRIVING_PARAM_5_DEFAULT, > + DRIVING_PARAM_6_DEFAULT, > + DRIVING_PARAM_7_DEFAULT, > + DRIVING_PARAM_8_DEFAULT > + }; > + > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE0_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE1_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE2_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + regmap_bulk_write(dp_phy->regs, MTK_DP_LANE3_DRIVING_PARAM_3, > + driving_params, ARRAY_SIZE(driving_params)); > + > + return 0; > +} > + > +static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts) > +{ > + struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy); > + u32 val; > + > + if (opts->dp.set_rate) { > + switch (opts->dp.link_rate) { > + default: > + dev_err(&phy->dev, > + "Implementation error, unknown linkrate %x\n", > + opts->dp.link_rate); > + return -EINVAL; > + case 1620: > + val = BIT_RATE_RBR; > + break; > + case 2700: > + val = BIT_RATE_HBR; > + break; > + case 5400: > + val = BIT_RATE_HBR2; > + break; > + case 8100: > + val = BIT_RATE_HBR3; > + break; > + } > + regmap_write(dp_phy->regs, MTK_DP_PHY_DIG_BIT_RATE, val); > + } > + > + regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_PLL_CTL_1, > + TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0); > + > + return 0; > +} > + > +static int mtk_dp_phy_reset(struct phy *phy) > +{ > + struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy); > + > + regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_SW_RST, > + DP_GLB_SW_RST_PHYD, 0); > + usleep_range(50, 200); > + regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_SW_RST, > + DP_GLB_SW_RST_PHYD, 1); > + > + return 0; > +} > + > +static const struct phy_ops mtk_dp_phy_dev_ops = { > + .init = mtk_dp_phy_init, > + .configure = mtk_dp_phy_configure, > + .reset = mtk_dp_phy_reset, > + .owner = THIS_MODULE, > +}; > + > +static int mtk_dp_phy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_dp_phy *dp_phy; > + struct phy *phy; > + struct regmap *regs; > + > + regs = *(struct regmap **)dev->platform_data; > + if (!regs) > + return dev_err_probe(dev, EINVAL, "No data passed, requires struct regmap**\n"); > + > + dp_phy = devm_kzalloc(dev, sizeof(*dp_phy), GFP_KERNEL); > + if (!dp_phy) > + return -ENOMEM; > + > + dp_phy->regs = regs; > + phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops); > + if (IS_ERR(phy)) > + return dev_err_probe(dev, PTR_ERR(phy), "Failed to create DP PHY\n"); > + > + phy_set_drvdata(phy, dp_phy); > + if (!dev->of_node) > + phy_create_lookup(phy, "dp", dev_name(dev)); > + > + return 0; > +} > + > +struct platform_driver mtk_dp_phy_driver = { > + .probe = mtk_dp_phy_probe, > + .driver = { > + .name = "mediatek-dp-phy", > + }, > +}; > +module_platform_driver(mtk_dp_phy_driver); > + > +MODULE_AUTHOR("Markus Schneider-Pargmann <msp@baylibre.com>"); > +MODULE_DESCRIPTION("MediaTek DP PHY Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.35.1
Hi, Rex: On Wed, 2022-06-08 at 18:26 +0800, Rex-BC Chen wrote: > On Tue, 2022-06-07 at 15:47 +0800, CK Hu wrote: > > Hi, Rex: > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > This patch adds a 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 > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > --- > > > > [snip] > > > > > + > > > +static int mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > > > +{ > > > + ssize_t ret; > > > + u8 sink_count; > > > + bool locked; > > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > > > + u32 link_status_reg = DP_LANE0_1_STATUS; > > > + > > > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, > > > &sink_count); > > > + if (ret < 0) { > > > + drm_err(mtk_dp->drm_dev, "Read sink count failed: > > > %ld\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > > > link_status, > > > + sizeof(link_status)); > > > + if (!ret) { > > > + drm_err(mtk_dp->drm_dev, "Read link status failed: > > > %ld\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + locked = drm_dp_channel_eq_ok(link_status, > > > + mtk_dp->train_info.lane_count); > > > + if (!locked && mtk_dp->train_state > > > > MTK_DP_TRAIN_STATE_TRAINING_PRE) > > > > Before enter this function, mtk_dp->train_state is set to > > MTK_DP_TRAIN_STATE_STARTUP, so this never happen, drop this. > > > > The interrupt from sink device could come any time. Why it's > impossible? I still ask this question: "For eDP, when does disconnect happened?" If it indeed happen, I still do not like to change state here. When disconnect, the training flow would result in fail finally and need not to change state here, but the training flow would block for a while when disconnect. You could add some check point to check train_info- >cable_plugged_in to quickly break out the training flow. Regards, CK > > > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE; > > > + > > > + if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING) > > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > > DP_DEVICE_SERVICE_IRQ_VECTOR, > > > + DP_REMOTE_CONTROL_COMMAND_PENDING); > > > + > > > + if (DP_GET_SINK_COUNT(sink_count) && > > > + (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > > > + mtk_dp->train_info.check_cap_count = 0; > > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID; > > > > Why change state from MTK_DP_TRAIN_STATE_STARTUP to > > MTK_DP_TRAIN_STATE_CHECKEDID? In mtk_dp_train_handler(), > > mtk_dp_parse_capabilities() is true then change to > > MTK_DP_TRAIN_STATE_CHECKEDID. Give a reason why these two are > > different. > > > > Regards, > > CK > > > > I will drop this and drop state of MTK_DP_TRAIN_STATE_CHECKEDID. > MTK_DP_TRAIN_STATE_CHECKEDID is only used for audio. > We can check enable status in another place. > > BRs, > Bo-Chen > > > > + msleep(20); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > > > >
On Tue, 2022-06-07 at 16:01 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static void mtk_dp_state_handler(struct mtk_dp *mtk_dp) > > +{ > > + switch (mtk_dp->state) { > > Does mtk_dp->state has any relation with mtk_dp->train_state. If yes, > mix mtk_dp->state and mtk_dp->train_state into one state. If no, move > calling mtk_dp_state_handler() out of mtk_dp_train_handler(). > > Regards, > CK > Hello CK, OK, I will refine this flow. About the state machine of traning flow, we can review in v11. BRs, Bo-Chen > > + case MTK_DP_STATE_INITIAL: > > + mtk_dp_video_mute(mtk_dp, true); > > + mtk_dp->state = MTK_DP_STATE_IDLE; > > + break; > > + > > + case MTK_DP_STATE_IDLE: > > + if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL) > > + mtk_dp->state = MTK_DP_STATE_PREPARE; > > + break; > > + > > + case MTK_DP_STATE_PREPARE: > > + mtk_dp_video_config(mtk_dp); > > + mtk_dp_video_enable(mtk_dp, true); > > + > > + mtk_dp->state = MTK_DP_STATE_NORMAL; > > + break; > > + > > + case MTK_DP_STATE_NORMAL: > > + if (mtk_dp->train_state != MTK_DP_TRAIN_STATE_NORMAL) { > > + mtk_dp_video_mute(mtk_dp, true); > > + mtk_dp->state = MTK_DP_STATE_IDLE; > > + } > > + break; > > + > > + default: > > + break; > > + } > > +} > >
On Thu, 2022-06-09 at 10:30 +0800, CK Hu wrote: > Hi, Rex: > > On Wed, 2022-06-08 at 18:26 +0800, Rex-BC Chen wrote: > > On Tue, 2022-06-07 at 15:47 +0800, CK Hu wrote: > > > Hi, Rex: > > > > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > > > This patch adds a 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 > > > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > > > --- > > > > > > [snip] > > > > > > > + > > > > +static int mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) > > > > +{ > > > > + ssize_t ret; > > > > + u8 sink_count; > > > > + bool locked; > > > > + u8 link_status[DP_LINK_STATUS_SIZE] = {}; > > > > + u32 sink_count_reg = DP_SINK_COUNT_ESI; > > > > + u32 link_status_reg = DP_LANE0_1_STATUS; > > > > + > > > > + ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, > > > > &sink_count); > > > > + if (ret < 0) { > > > > + drm_err(mtk_dp->drm_dev, "Read sink count > > > > failed: > > > > %ld\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, > > > > link_status, > > > > + sizeof(link_status)); > > > > + if (!ret) { > > > > + drm_err(mtk_dp->drm_dev, "Read link status > > > > failed: > > > > %ld\n", > > > > + ret); > > > > + return ret; > > > > + } > > > > + > > > > + locked = drm_dp_channel_eq_ok(link_status, > > > > + mtk_dp- > > > > >train_info.lane_count); > > > > + if (!locked && mtk_dp->train_state > > > > > MTK_DP_TRAIN_STATE_TRAINING_PRE) > > > > > > Before enter this function, mtk_dp->train_state is set to > > > MTK_DP_TRAIN_STATE_STARTUP, so this never happen, drop this. > > > > > > > The interrupt from sink device could come any time. Why it's > > impossible? > > I still ask this question: "For eDP, when does disconnect happened?" > If > it indeed happen, I still do not like to change state here. When > disconnect, the training flow would result in fail finally and need > not > to change state here, but the training flow would block for a while > when disconnect. You could add some check point to check train_info- > > cable_plugged_in to quickly break out the training flow. > > Regards, > CK > Hello CK, This function is to handle "sink interrupt" and we need to read the status from sink device via aux channel. edp spec is just add some extension rule from dp spec, this is define in dp spec. The basic spec is still "dp", and these are all define in dp spec. Therefore, I think we should keep this. BRs, Bo-Chen > > > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_TRAINING_PRE; > > > > + > > > > + 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_PE > > > > NDING); > > > > + > > > > + if (DP_GET_SINK_COUNT(sink_count) && > > > > + (link_status[2] & > > > > DP_DOWNSTREAM_PORT_STATUS_CHANGED)) { > > > > + mtk_dp->train_info.check_cap_count = 0; > > > > + mtk_dp->train_state = > > > > MTK_DP_TRAIN_STATE_CHECKEDID; > > > > > > Why change state from MTK_DP_TRAIN_STATE_STARTUP to > > > MTK_DP_TRAIN_STATE_CHECKEDID? In mtk_dp_train_handler(), > > > mtk_dp_parse_capabilities() is true then change to > > > MTK_DP_TRAIN_STATE_CHECKEDID. Give a reason why these two are > > > different. > > > > > > Regards, > > > CK > > > > > > > I will drop this and drop state of MTK_DP_TRAIN_STATE_CHECKEDID. > > MTK_DP_TRAIN_STATE_CHECKEDID is only used for audio. > > We can check enable status in another place. > > > > BRs, > > Bo-Chen > > > > > > + msleep(20); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > > > > > > >
On Tue, 2022-06-07 at 17:04 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +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_disc > > onnected; > > + > > + if (event < 0) > > + return IRQ_HANDLED; > > + > > + if (mtk_dp->drm_dev) { > > + dev_info(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->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > + > > + if (!mtk_dp->train_info.cable_plugged_in || > > + !mtk_dp_plug_state(mtk_dp)) { > > + mtk_dp_video_mute(mtk_dp, true); > > For eDP, when does 'unplug' happen? Explain this or move unplug > processing to DP patch. > > Regards, > CK > Hello CK, ok, I move them to dp patch. BRs, Bo-Chen > > + > > + mtk_dp_initialize_priv_data(mtk_dp); > > + mtk_dp_set_idle_pattern(mtk_dp, true); > > + if (mtk_dp->has_fec) > > + mtk_dp_fec_enable(mtk_dp, false); > > + > > + 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); > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > + mtk_dp->train_info.link_rate = > > + min_t(int, mtk_dp->max_linkrate, > > + buf[mtk_dp->max_linkrate]); > > + mtk_dp->train_info.lane_count = > > + min_t(int, mtk_dp->max_lanes, > > + drm_dp_max_lane_count(buf)); > > + } > > + } > > + > > + if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) { > > + dev_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n"); > > + mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT; > > + mtk_dp_hpd_sink_event(mtk_dp); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > >
On Wed, 2022-06-08 at 16:30 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static bool mtk_dp_parse_capabilities(struct mtk_dp *mtk_dp) > > +{ > > + u8 buf[DP_RECEIVER_CAP_SIZE] = {}; > > + u8 val; > > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > > + > > + if (!mtk_dp_plug_state(mtk_dp)) > > + return false; > > + > > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, > > DP_SET_POWER_D0); > > + /* Wait for power on */ > > + usleep_range(2000, 5000); > > + > > + drm_dp_read_dpcd_caps(&mtk_dp->aux, buf); > > + > > + memcpy(mtk_dp->rx_cap, buf, min(sizeof(mtk_dp->rx_cap), > > sizeof(buf))); > > sizeof(mtk_dp->rx_cap) is identical to sizeof(buf), so > > drm_dp_read_dpcd_caps(&mtk_dp->aux, mtk_dp->rx_cap); > > Hello CK, I will drop buf[]. > > + mtk_dp->rx_cap[DP_TRAINING_AUX_RD_INTERVAL] &= > > DP_TRAINING_AUX_RD_MASK; > > + > > + train_info->link_rate = > > + min_t(int, mtk_dp->max_linkrate, buf[mtk_dp- > > > max_linkrate]); > > > > + train_info->lane_count = > > + min_t(int, mtk_dp->max_lanes, > > drm_dp_max_lane_count(buf)); > > + > > + train_info->tps3 = drm_dp_tps3_supported(buf); > > + train_info->tps4 = drm_dp_tps4_supported(buf); > > + > > + train_info->sink_ssc = > > + !!(buf[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5); > > + > > + train_info->sink_ssc = false; > > What does these two statement do? > ssc = Spread spectrum clock. it's for both edp and dp. BRs, Bo-Chen > Regards, > CK > > > + > > + drm_dp_dpcd_readb(&mtk_dp->aux, DP_MSTM_CAP, &val); > > + if (val & DP_MST_CAP) { > > + /* Clear DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0 */ > > + drm_dp_dpcd_readb(&mtk_dp->aux, > > + DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0, > > &val); > > + if (val) > > + drm_dp_dpcd_writeb(&mtk_dp->aux, > > + DP_DEVICE_SERVICE_IRQ_VECTOR > > _ESI0, > > + val); > > + } > > + > > + return true; > > +} > > + > >
Hi, Rex: On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a 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 > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > --- [snip] > + > +static int mtk_dp_power_disable(struct mtk_dp *mtk_dp) > +{ > + int ret; > + > + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0); > + > + if (ret) > + return ret; > + > + ret = mtk_dp_write(mtk_dp, MTK_DP_0034, MTK_DP_0034 is defined as: +#define MTK_DP_0034 (BIT(2) | BIT(4) | BIT(5)) I think this a weird address. > + 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); > + > + if (ret) > + return ret; > + > + /* Disable RX */ > + ret = mtk_dp_write(mtk_dp, MTK_DP_1040, 0); MTK_DP_1040 is defined as: +#define MTK_DP_1040 (BIT(6) | BIT(12)) I think this a weird address. Regards, CK > + > + if (ret) > + return ret; > + > + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD, > + 0x550 | BIT(FUSE_SEL_SHIFT) | > BIT(MEM_ISO_EN_SHIFT)); > + > + return ret; > +}
On Thu, 2022-06-09 at 17:37 +0800, CK Hu wrote: > Hi, Rex: > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a 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 > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > [snip] > > > + > > +static int mtk_dp_power_disable(struct mtk_dp *mtk_dp) > > +{ > > + int ret; > > + > > + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0); > > + > > + if (ret) > > + return ret; > > + > > + ret = mtk_dp_write(mtk_dp, MTK_DP_0034, > > MTK_DP_0034 is defined as: > > +#define MTK_DP_0034 (BIT(2) | BIT(4) | BIT(5)) > > I think this a weird address. > > > + 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); > > + > > + if (ret) > > + return ret; > > + > > + /* Disable RX */ > > + ret = mtk_dp_write(mtk_dp, MTK_DP_1040, 0); > > > MTK_DP_1040 is defined as: > > +#define MTK_DP_1040 (BIT(6) | BIT(12)) > > I think this a weird address. > > Regards, > CK > Hello CK, I will change them to 0x34 and 0x1040. BRs, Rex > > + > > + if (ret) > > + return ret; > > + > > + ret = mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD, > > + 0x550 | BIT(FUSE_SEL_SHIFT) | > > BIT(MEM_ISO_EN_SHIFT)); > > + > > + return ret; > > +} > >
On Wed, 2022-05-25 at 14:26 +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > > 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> > > --- > > drivers/gpu/drm/drm_edid.c | 74 > > ++++++++++++++++++++++++++++++++++++++ > > include/drm/drm_edid.h | 14 ++++++++ > > 2 files changed, 88 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_edid.c > > b/drivers/gpu/drm/drm_edid.c > > index 561f53831e29..61ef1b1c972c 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -4758,6 +4758,80 @@ int drm_edid_to_speaker_allocation(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_uncompressed(const struct cea_sad *sad) > > +{ > > + switch (sad->format) { > > + case HDMI_AUDIO_CODING_TYPE_STREAM: > > As far as I know, bit 0 is reserved, so HDMI_AUDIO_CODING_TYPE_STREAM > should > never occur here? > Hello Angelo, because we do not know whether "HDMI_AUDIO_CODING_TYPE_STREAM" is compressed, we decide to change this function name to drm_cea_sad_is_pcm to prevent misunderstanding. For this, I will drop case HDMI_AUDIO_CODING_TYPE_STREAM > > + case HDMI_AUDIO_CODING_TYPE_PCM: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > Also, I think that implementing a > drm_cea_sad_get_compressed_max_bitrate() > function should be pretty straightforward... the spec says that this > is > 8 bits, byte 3 (your byte2) contains the max bitrate divided by 8kHz, > so to extract it, you read byte2 and multiply it by 8000Hz. > > /** > * drm_cea_sad_get_compressed_max_bitrate - Extract maximum bitrate > * @sad: Pointer to the cea_sad structure > * > * Extracts the cea_sad byte2 field and returns the maximum bit rate > * of a compressed audio stream. > * > * Note: This function may only be called for compressed audio. > * > * Return: Maximum bitrate of compressed audio stream in bit/s or > * negative number for error > */ > int drm_cea_sad_get_compressed_max_bitrate(const struct cea_sad *sad) > { > if (drm_cea_sad_is_uncompressed(sad)) { > DRM_ERROR("Not supported: tried to get max bitrate for > uncompressed format: %u\n", > sad->format); > return -EINVAL; > } > > return sad->byte2 * 8000; > } > After sync with Jitao, we think this function is not correct. refer to table of "Short Audio Descriptor" in [1]. [1]:https://en.wikipedia.org/wiki/Extended_Display_Identification_Data BRs, Bo-Chen > > +/** > > + * drm_cea_sad_get_uncompressed_word_length - Extract word length > > + * @sad: Pointer to the cea_sad struct > > + * > > + * Extracts the cea_sad byte2 field and returns the word length > > for an > > + * uncompressed stream. > > + * > > + * Note: This function may only be called for uncompressed audio. > > + * > > + * Return: Word length in bits or a negative errno if parsing > > failed. > > + */ > > +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad > > *sad) > > +{ > > + if (!drm_cea_sad_is_uncompressed(sad)) { > > + DRM_WARN("Unable to get the uncompressed word length > > for a compressed format: %u\n", > > + sad->format); > > + return -EINVAL; > > + } > > + > > + switch (sad->byte2) { > > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT: > > + return 16; > > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT: > > + return 20; > > + case DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT: > > + return 24; > > + default: > > + return -EINVAL; > > + } > > +} > > +EXPORT_SYMBOL(drm_cea_sad_get_uncompressed_word_length); > > + > > /** > > * drm_av_sync_delay - compute the HDMI/DP sink audio-video sync > > delay > > * @connector: connector associated with the HDMI/DP sink > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > > index 37c420423625..7a939cb95b38 100644 > > --- a/include/drm/drm_edid.h > > +++ b/include/drm/drm_edid.h > > @@ -373,6 +373,18 @@ struct cea_sad { > > u8 byte2; > > }; > > > > +#define DRM_CEA_SAD_FREQ_32KHZ BIT(0) > > +#define DRM_CEA_SAD_FREQ_44KHZ BIT(1) > > +#define DRM_CEA_SAD_FREQ_48KHZ BIT(2) > > +#define DRM_CEA_SAD_FREQ_88KHZ BIT(3) > > +#define DRM_CEA_SAD_FREQ_96KHZ BIT(4) > > +#define DRM_CEA_SAD_FREQ_176KHZ BIT(5) > > +#define DRM_CEA_SAD_FREQ_192KHZ BIT(6) > > + > > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_16BIT BIT(0) > > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_20BIT BIT(1) > > +#define DRM_CEA_SAD_UNCOMPRESSED_WORD_24BIT BIT(2) > > + > > struct drm_encoder; > > struct drm_connector; > > struct drm_connector_state; > > @@ -380,6 +392,8 @@ struct drm_display_mode; > > > > int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads); > > int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb); > > +int drm_cea_sad_get_sample_rate(const struct cea_sad *sad); > > +int drm_cea_sad_get_uncompressed_word_length(const struct cea_sad > > *sad); > > int drm_av_sync_delay(struct drm_connector *connector, > > const struct drm_display_mode *mode); > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2022-05-30 at 15:50 +0800, CK Hu wrote: > Hi, Guillaume: > > > On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote: > > Adds a bit of flexibility to support SoCs without swap_input > > support > > > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Reviewed-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 545a1337cc89..454f8563efae 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -126,6 +126,7 @@ struct mtk_dpi_conf { > > const u32 *output_fmts; > > u32 num_output_fmts; > > bool is_ck_de_pol; > > + bool swap_input_support; > > const struct mtk_dpi_yc_limit *limit; > > }; > > > > @@ -378,18 +379,21 @@ static void > > mtk_dpi_config_color_format(struct > > mtk_dpi *dpi, > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > > mtk_dpi_config_yuv422_enable(dpi, false); > > mtk_dpi_config_csc_enable(dpi, true); > > - mtk_dpi_config_swap_input(dpi, false); > > + if (dpi->conf->swap_input_support) > > + mtk_dpi_config_swap_input(dpi, false); > > mtk_dpi_config_channel_swap(dpi, > > MTK_DPI_OUT_CHANNEL_SWAP_BGR); > > } else if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_422) || > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL)) { > > mtk_dpi_config_yuv422_enable(dpi, true); > > mtk_dpi_config_csc_enable(dpi, true); > > - mtk_dpi_config_swap_input(dpi, true); > > + if (dpi->conf->swap_input_support) > > + mtk_dpi_config_swap_input(dpi, true); > > As [1], please keep in touch with Mediatek engineer. > > Regards, > CK > > [1] > https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-8-granquet@baylibre.com/ > Hello CK, the reason is the hardware design of dp_intf does not support input swap. I will add this in commit message. BRs, Bo-Chen > > mtk_dpi_config_channel_swap(dpi, > > MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > } else { > > mtk_dpi_config_yuv422_enable(dpi, false); > > mtk_dpi_config_csc_enable(dpi, false); > > - mtk_dpi_config_swap_input(dpi, false); > > + if (dpi->conf->swap_input_support) > > + mtk_dpi_config_swap_input(dpi, false); > > mtk_dpi_config_channel_swap(dpi, > > MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > } > > } > > @@ -808,6 +812,7 @@ static const struct mtk_dpi_conf mt8173_conf = > > { > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -819,6 +824,7 @@ static const struct mtk_dpi_conf mt2701_conf = > > { > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -829,6 +835,7 @@ static const struct mtk_dpi_conf mt8183_conf = > > { > > .output_fmts = mt8183_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > > @@ -839,6 +846,7 @@ static const struct mtk_dpi_conf mt8192_conf = > > { > > .output_fmts = mt8173_output_fmts, > > .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > .is_ck_de_pol = true, > > + .swap_input_support = true, > > .limit = &mtk_dpi_limit, > > }; > > > >
On Thu, 2022-06-02 at 07:48 +0200, Christophe JAILLET wrote: > Le 23/05/2022 à 12:47, Guillaume Ranquet a écrit : > > dpintf is the displayport interface hardware unit. This unit is > > similar > > to dpi and can reuse most of the code. > > > > This patch adds support for mt8195-dpintf to this dpi driver. Main > > differences are: > > - Some features/functional components are not available for > > dpintf > > which are now excluded from code execution once is_dpintf is > > set > > - dpintf can and needs to choose between different clockdividers > > based > > on the clockspeed. This is done by choosing a different clock > > parent. > > - There are two additional clocks that need to be managed. These > > are > > only set for dpintf and will be set to NULL if not supplied. > > The > > clk_* calls handle these as normal clocks then. > > - Some register contents differ slightly between the two > > components. To > > work around this I added register bits/masks with a DPINTF_ > > prefix > > and use them where different. > > > > Based on a separate driver for dpintf created by > > Jason-JH.Lin <jason-jh.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> > > . > > > > Signed-off-by: Markus Schneider-Pargmann < > > msp-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > Signed-off-by: Guillaume Ranquet < > > granquet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 126 > > +++++++++++++++++--- > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 35 ++++++ > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 ++ > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +- > > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > > 6 files changed, 159 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index eb969c5c5c2e..763bfb700135 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -71,6 +71,7 @@ struct mtk_dpi { > > void __iomem *regs; > > struct device *dev; > > struct clk *engine_clk; > > + struct clk *dpi_ck_cg; > > struct clk *pixel_clk; > > struct clk *tvd_clk; > > int irq; > > @@ -126,6 +127,7 @@ struct mtk_dpi_conf { > > const u32 *output_fmts; > > u32 num_output_fmts; > > bool is_ck_de_pol; > > + bool is_dpintf; > > bool swap_input_support; > > /* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH > > (no shift) */ > > u32 dimension_mask; > > @@ -438,6 +440,8 @@ static void mtk_dpi_power_off(struct mtk_dpi > > *dpi) > > mtk_dpi_disable(dpi); > > clk_disable_unprepare(dpi->pixel_clk); > > clk_disable_unprepare(dpi->engine_clk); > > + clk_disable_unprepare(dpi->dpi_ck_cg); > > + clk_disable_unprepare(dpi->tvd_clk); > > } > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > @@ -447,12 +451,24 @@ static int mtk_dpi_power_on(struct mtk_dpi > > *dpi) > > if (++dpi->refcount != 1) > > return 0; > > > > Hi, > > belwo the error handling path looks odd. (both where we goto, and > the > order of the clk_disable_unprepare() in the error handling path. > > just my 2c, > > CJ > Hello Christophe, Thanks for your review. I will fix this order in enxt version. Thanks BRs, Bo-Chen > > + ret = clk_prepare_enable(dpi->tvd_clk); > > + if (ret) { > > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", > > ret); > > + goto err_pixel; > > + } > > + > > ret = clk_prepare_enable(dpi->engine_clk); > > if (ret) { > > dev_err(dpi->dev, "Failed to enable engine clock: > > %d\n", ret); > > goto err_refcount; > > } > > > > + ret = clk_prepare_enable(dpi->dpi_ck_cg); > > + if (ret) { > > + dev_err(dpi->dev, "Failed to enable dpi_ck_cg clock: > > %d\n", ret); > > + goto err_ck_cg; > > + } > > + > > ret = clk_prepare_enable(dpi->pixel_clk); > > if (ret) { > > dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", > > ret); > > @@ -466,6 +482,8 @@ static int mtk_dpi_power_on(struct mtk_dpi > > *dpi) > > return 0; > > > > err_pixel: > > + clk_disable_unprepare(dpi->dpi_ck_cg); > > +err_ck_cg: > > clk_disable_unprepare(dpi->engine_clk); > > err_refcount: > > dpi->refcount--; > > [...] > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 2022-05-25 at 14:58 +0200, AngeloGioacchino Del Regno wrote: > Il 23/05/22 12:47, Guillaume Ranquet ha scritto: > > dpintf is the displayport interface hardware unit. This unit is > > similar > > to dpi and can reuse most of the code. > > > > This patch adds support for mt8195-dpintf to this dpi driver. Main > > differences are: > > - Some features/functional components are not available for > > dpintf > > which are now excluded from code execution once is_dpintf is > > set > > - dpintf can and needs to choose between different clockdividers > > based > > on the clockspeed. This is done by choosing a different clock > > parent. > > - There are two additional clocks that need to be managed. These > > are > > only set for dpintf and will be set to NULL if not supplied. > > The > > clk_* calls handle these as normal clocks then. > > - Some register contents differ slightly between the two > > components. To > > work around this I added register bits/masks with a DPINTF_ > > prefix > > and use them where different. > > > > Based on a separate driver for dpintf created by > > Jason-JH.Lin <jason-jh.lin@mediatek.com>. > > > > Only after finishing the review, I've noticed that I just wrote the > same things > that I wrote in my review for version 8... and if I recall correctly, > this is not > the first time that something like that happens. > > Please pay attention to what reviewers say, as to not waste anyone's > time. > Hello Angelo, I will help to fix these issue in next version. > > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dpi.c | 126 > > +++++++++++++++++--- > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 35 ++++++ > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 8 ++ > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +- > > include/linux/soc/mediatek/mtk-mmsys.h | 4 +- > > 6 files changed, 159 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > > b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index eb969c5c5c2e..763bfb700135 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > @@ -71,6 +71,7 @@ struct mtk_dpi { > > void __iomem *regs; > > struct device *dev; > > struct clk *engine_clk; > > + struct clk *dpi_ck_cg; > > struct clk *pixel_clk; > > struct clk *tvd_clk; > > int irq; > > @@ -126,6 +127,7 @@ struct mtk_dpi_conf { > > const u32 *output_fmts; > > u32 num_output_fmts; > > bool is_ck_de_pol; > > + bool is_dpintf; > > bool swap_input_support; > > /* Mask used for HWIDTH, HPORCH, VSYNC_WIDTH and VSYNC_PORCH > > (no shift) */ > > u32 dimension_mask; > > @@ -438,6 +440,8 @@ static void mtk_dpi_power_off(struct mtk_dpi > > *dpi) > > mtk_dpi_disable(dpi); > > clk_disable_unprepare(dpi->pixel_clk); > > clk_disable_unprepare(dpi->engine_clk); > > + clk_disable_unprepare(dpi->dpi_ck_cg); > > + clk_disable_unprepare(dpi->tvd_clk); > > } > > > > static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > @@ -447,12 +451,24 @@ static int mtk_dpi_power_on(struct mtk_dpi > > *dpi) > > if (++dpi->refcount != 1) > > return 0; > > > > + ret = clk_prepare_enable(dpi->tvd_clk); > > + if (ret) { > > + dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", > > ret); > > + goto err_pixel; > > + } > > + > > ret = clk_prepare_enable(dpi->engine_clk); > > if (ret) { > > dev_err(dpi->dev, "Failed to enable engine clock: > > %d\n", ret); > > goto err_refcount; > > } > > > > + ret = clk_prepare_enable(dpi->dpi_ck_cg); > > + if (ret) { > > + dev_err(dpi->dev, "Failed to enable dpi_ck_cg clock: > > %d\n", ret); > > + goto err_ck_cg; > > + } > > + > > ret = clk_prepare_enable(dpi->pixel_clk); > > if (ret) { > > dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", > > ret); > > @@ -466,6 +482,8 @@ static int mtk_dpi_power_on(struct mtk_dpi > > *dpi) > > return 0; > > > > err_pixel: > > + clk_disable_unprepare(dpi->dpi_ck_cg); > > +err_ck_cg: > > clk_disable_unprepare(dpi->engine_clk); > > err_refcount: > > dpi->refcount--; > > @@ -498,11 +516,11 @@ static int mtk_dpi_set_display_mode(struct > > mtk_dpi *dpi, > > > > vm.pixelclock = pll_rate / factor; > > if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > - (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) > > + (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) { > > The indentation was perfect before that change... > > > clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2); > > - else > > + } else { > > clk_set_rate(dpi->pixel_clk, vm.pixelclock); > > - > > + } > > > > vm.pixelclock = clk_get_rate(dpi->pixel_clk); > > > > @@ -515,9 +533,15 @@ static int mtk_dpi_set_display_mode(struct > > mtk_dpi *dpi, > > MTK_DPI_POLARITY_FALLING : > > MTK_DPI_POLARITY_RISING; > > dpi_pol.vsync_pol = vm.flags & DISPLAY_FLAGS_VSYNC_HIGH ? > > MTK_DPI_POLARITY_FALLING : > > MTK_DPI_POLARITY_RISING; > > - hsync.sync_width = vm.hsync_len; > > - hsync.back_porch = vm.hback_porch; > > - hsync.front_porch = vm.hfront_porch; > > + if (dpi->conf->is_dpintf) { > > + hsync.sync_width = vm.hsync_len / 4; > > + hsync.back_porch = vm.hback_porch / 4; > > + hsync.front_porch = vm.hfront_porch / 4; > > + } else { > > + hsync.sync_width = vm.hsync_len; > > + hsync.back_porch = vm.hback_porch; > > + hsync.front_porch = vm.hfront_porch; > > + } > > This looks way better: > > hsync.sync_width = vm.hsync_len; > hsync.back_porch = vm.hback_porch; > hsync.front_porch = vm.hfront_porch; > > /* For DPINTF, we need to divide everything by 4 .. lanes? */ > if (dpi->conf->is_dpintf) { > hsync.sync_width /= 4; > hsync.back_porch /= 4; > hsync.front_porch /= 4; > } > ok. > > hsync.shift_half_line = false; > > vsync_lodd.sync_width = vm.vsync_len; > > vsync_lodd.back_porch = vm.vback_porch; > > @@ -559,13 +583,20 @@ static int mtk_dpi_set_display_mode(struct > > mtk_dpi *dpi, > > mtk_dpi_config_channel_limit(dpi); > > mtk_dpi_config_bit_num(dpi, dpi->bit_num); > > mtk_dpi_config_channel_swap(dpi, dpi->channel_swap); > > - mtk_dpi_config_yc_map(dpi, dpi->yc_map); > > mtk_dpi_config_color_format(dpi, dpi->color_format); > > - mtk_dpi_config_2n_h_fre(dpi); > > - mtk_dpi_dual_edge(dpi); > > - mtk_dpi_config_disable_edge(dpi); > > + if (dpi->conf->is_dpintf) { > > + mtk_dpi_mask(dpi, DPI_CON, DPINTF_INPUT_2P_EN, > > + DPINTF_INPUT_2P_EN); > > + } else { > > + mtk_dpi_config_yc_map(dpi, dpi->yc_map); > > + mtk_dpi_config_2n_h_fre(dpi); > > + mtk_dpi_dual_edge(dpi); > > + mtk_dpi_config_disable_edge(dpi); > > + } > > mtk_dpi_sw_reset(dpi, false); > > > > + mtk_dpi_enable(dpi); > > + > > return 0; > > } > > > > @@ -608,7 +639,6 @@ static u32 > > *mtk_dpi_bridge_atomic_get_input_bus_fmts(struct drm_bridge > > *bridge, > > u32 *input_fmts; > > > > *num_input_fmts = 0; > > - > > Removing this line is not part of adding dpintf support, so don't do > that. > ok. > > input_fmts = kcalloc(1, sizeof(*input_fmts), > > GFP_KERNEL); > > if (!input_fmts) > > @@ -634,15 +664,18 @@ static int mtk_dpi_bridge_atomic_check(struct > > drm_bridge *bridge, > > if (dpi->conf->num_output_fmts) > > out_bus_format = dpi->conf->output_fmts[0]; > > > > - dev_dbg(dpi->dev, "input format 0x%04x, output format > > 0x%04x\n", > > - bridge_state->input_bus_cfg.format, > > - bridge_state->output_bus_cfg.format); > > + dev_info(dpi->dev, "input format 0x%04x, output format > > 0x%04x\n", > > + bridge_state->input_bus_cfg.format, > > + bridge_state->output_bus_cfg.format); > > This message is not giving any constantly changing information, nor > any one that > is interesting for the user: keep this as dev_dbg(). > ok. > > > > dpi->output_fmt = out_bus_format; > > dpi->bit_num = MTK_DPI_OUT_BIT_NUM_8BITS; > > dpi->channel_swap = MTK_DPI_OUT_CHANNEL_SWAP_RGB; > > dpi->yc_map = MTK_DPI_OUT_YC_MAP_RGB; > > - dpi->color_format = MTK_DPI_COLOR_FORMAT_RGB; > > + if (out_bus_format == MEDIA_BUS_FMT_YUYV8_1X16) > > + dpi->color_format = > > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL; > > + else > > + dpi->color_format = MTK_DPI_COLOR_FORMAT_RGB; > > > > return 0; > > } > > @@ -687,7 +720,7 @@ mtk_dpi_bridge_mode_valid(struct drm_bridge > > *bridge, > > { > > struct mtk_dpi *dpi = bridge_to_dpi(bridge); > > > > - if (mode->clock > dpi->conf->max_clock_khz) > > + if (dpi->conf->max_clock_khz && mode->clock > dpi->conf- > > >max_clock_khz) > > return MODE_CLOCK_HIGH; > > > > return MODE_OK; > > @@ -801,6 +834,16 @@ static unsigned int > > mt8183_calculate_factor(int clock) > > return 2; > > } > > > > +static unsigned int mt8195_dpintf_calculate_factor(int clock) > > +{ > > + if (clock < 70000) > > is 70000 intentional? Or did you mean 700000? > 70000 is correct, the unit for this is KHz. > > + return 4; > > + else if (clock < 200000) > > + return 2; > > + else > > + return 1; > > +} > > + > > static const u32 mt8173_output_fmts[] = { > > MEDIA_BUS_FMT_RGB888_1X24, > > }; > > @@ -810,6 +853,12 @@ static const u32 mt8183_output_fmts[] = { > > MEDIA_BUS_FMT_RGB888_2X12_BE, > > }; > > > > +static const u32 mt8195_output_fmts[] = { > > + MEDIA_BUS_FMT_RGB888_1X24, > > + MEDIA_BUS_FMT_YUV8_1X24, > > + MEDIA_BUS_FMT_YUYV8_1X16, > > +}; > > + > > static const struct mtk_dpi_yc_limit mtk_dpi_limit = { > > .c_bottom = 0x0010, > > .c_top = 0x0FE0, > > @@ -817,6 +866,13 @@ static const struct mtk_dpi_yc_limit > > mtk_dpi_limit = { > > .y_top = 0x0FE0, > > }; > > > > +static const struct mtk_dpi_yc_limit mtk_dpintf_limit = { > > + .c_bottom = 0x0000, > > + .c_top = 0xFFF, > > + .y_bottom = 0x0000, > > + .y_top = 0xFFF, > > +}; > > + > > static const struct mtk_dpi_conf mt8173_conf = { > > .cal_factor = mt8173_calculate_factor, > > .reg_h_fre_con = 0xe0, > > @@ -882,6 +938,19 @@ static const struct mtk_dpi_conf mt8192_conf = > > { > > .limit = &mtk_dpi_limit, > > }; > > > > +static const struct mtk_dpi_conf mt8195_dpintf_conf = { > > + .cal_factor = mt8195_dpintf_calculate_factor, > > + .output_fmts = mt8195_output_fmts, > > + .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts), > > + .is_dpintf = true, > > + .dimension_mask = DPINTF_HPW_MASK, > > + .hvsize_mask = DPINTF_HSIZE_MASK, > > + .channel_swap_shift = DPINTF_CH_SWAP, > > + .yuv422_en_bit = DPINTF_YUV422_EN, > > + .csc_enable_bit = DPINTF_CSC_ENABLE, > > + .limit = &mtk_dpintf_limit, > > +}; > > + > > static int mtk_dpi_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -929,7 +998,27 @@ static int mtk_dpi_probe(struct > > platform_device *pdev) > > if (IS_ERR(dpi->engine_clk)) { > > ret = PTR_ERR(dpi->engine_clk); > > if (ret != -EPROBE_DEFER) > > - dev_err(dev, "Failed to get engine clock: > > %d\n", ret); > > + dev_err(dev, "Failed to get engine clock: > > %d\n", > > + ret); > > Why are you breaking this line? > I will fix this. > > + > > + return ret; > > + } > > + > > + dpi->dpi_ck_cg = devm_clk_get_optional(dev, "ck_cg"); > > + if (IS_ERR(dpi->dpi_ck_cg)) { > > + ret = PTR_ERR(dpi->dpi_ck_cg); > > + if (ret != -EPROBE_DEFER) > > + dev_err(dev, "Failed to get dpi ck cg clock: > > %d\n", > > + ret); > > + > > + return ret; > > + } > > + > > You're getting this clock twice, what happened here? > > P.S.: As I explained on the dt-bindings patch, you likely don't even > need this > clock at all. > I will remove this. For this clock gate, we stiil need to enable it. I reply in binding patch. I rename it as pll_gate. > > + dpi->dpi_ck_cg = devm_clk_get_optional(dev, "ck_cg"); > > + if (IS_ERR(dpi->dpi_ck_cg)) { > > + ret = PTR_ERR(dpi->dpi_ck_cg); > > + if (ret != -EPROBE_DEFER) > > + dev_err(dev, "Failed to get dpi ck cg clock: > > %d\n", ret); > > > > return ret; > > } > > @@ -1004,6 +1093,9 @@ static const struct of_device_id > > mtk_dpi_of_ids[] = { > > { .compatible = "mediatek,mt8192-dpi", > > .data = &mt8192_conf, > > }, > > + { .compatible = "mediatek,mt8195-dpintf", > > + .data = &mt8195_dpintf_conf, > > + }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, mtk_dpi_of_ids); > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > index 3a02fabe1662..dd47dd3f2e4f 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > @@ -40,10 +40,15 @@ > > #define FAKE_DE_LEVEN BIT(21) > > #define FAKE_DE_RODD BIT(22) > > #define FAKE_DE_REVEN BIT(23) > > +#define DPINTF_YUV422_EN BIT(24) > > +#define DPINTF_CSC_ENABLE BIT(26) > > +#define DPINTF_INPUT_2P_EN BIT(29) > > > > #define DPI_OUTPUT_SETTING 0x14 > > #define CH_SWAP 0 > > +#define DPINTF_CH_SWAP BIT(1) > > #define CH_SWAP_MASK (0x7 << 0) > > +#define DPINTF_CH_SWAP_MASK (0x7 << 1) > > #define SWAP_RGB 0x00 > > #define SWAP_GBR 0x01 > > #define SWAP_BRG 0x02 > > @@ -80,8 +85,10 @@ > > #define DPI_SIZE 0x18 > > #define HSIZE 0 > > #define HSIZE_MASK (0x1FFF << 0) > > +#define DPINTF_HSIZE_MASK (0xFFFF << 0) > > #define VSIZE 16 > > #define VSIZE_MASK (0x1FFF << 16) > > +#define DPINTF_VSIZE_MASK (0xFFFF << 16) > > > > #define DPI_DDR_SETTING 0x1C > > #define DDR_EN BIT(0) > > @@ -93,24 +100,30 @@ > > #define DPI_TGEN_HWIDTH 0x20 > > #define HPW 0 > > #define HPW_MASK (0xFFF << 0) > > +#define DPINTF_HPW_MASK (0xFFFF << 0) > > > > #define DPI_TGEN_HPORCH 0x24 > > #define HBP 0 > > #define HBP_MASK (0xFFF << 0) > > +#define DPINTF_HBP_MASK (0xFFFF << 0) > > #define HFP 16 > > #define HFP_MASK (0xFFF << 16) > > +#define DPINTF_HFP_MASK (0xFFFF << 16) > > > > #define DPI_TGEN_VWIDTH 0x28 > > #define DPI_TGEN_VPORCH 0x2C > > > > #define VSYNC_WIDTH_SHIFT 0 > > #define VSYNC_WIDTH_MASK (0xFFF << 0) > > +#define DPINTF_VSYNC_WIDTH_MASK (0xFFFF << 0) > > #define VSYNC_HALF_LINE_SHIFT 16 > > #define VSYNC_HALF_LINE_MASK BIT(16) > > #define VSYNC_BACK_PORCH_SHIFT 0 > > #define VSYNC_BACK_PORCH_MASK (0xFFF << 0) > > +#define DPINTF_VSYNC_BACK_PORCH_MASK (0xFFFF << 0) > > #define VSYNC_FRONT_PORCH_SHIFT 16 > > #define VSYNC_FRONT_PORCH_MASK (0xFFF << 16) > > +#define DPINTF_VSYNC_FRONT_PORCH_MASK (0xFFFF << 16) > > > > #define DPI_BG_HCNTL 0x30 > > #define BG_RIGHT (0x1FFF << 0) > > @@ -217,4 +230,26 @@ > > > > #define EDGE_SEL_EN BIT(5) > > #define H_FRE_2N BIT(25) > > + > > +#define RGB_TO_JPEG 0x00 > > This is 0x0 > > > +#define RGB_TO_FULL709 0x01 > > 0x1 > > > +#define RGB_TO_BT601 0x02 > > 0x2 > > ...remove the unnecessary leading zeros, but... you're not ever > using this set of definitions, so why are you even adding them? > ok, I will remove them. > > +#define RGB_TO_BT709 0x03 > > +#define JPEG_TO_RGB 0x04 > > +#define FULL709_TO_RGB 0x05 > > +#define BT601_TO_RGB 0x06 > > +#define BT709_TO_RGB 0x07 > > +#define JPEG_TO_BT601 0x08 > > +#define JPEG_TO_BT709 0x09 > > +#define BT601_TO_JPEG 0xA > > +#define BT709_TO_JPEG 0xB > > +#define BT709_TO_BT601 0xC > > +#define BT601_TO_BT709 0xD > > +#define JPEG_TO_CERGB 0x14 > > +#define FULL709_TO_CERGB 0x15 > > +#define BT601_TO_CERGB 0x16 > > +#define BT709_TO_CERGB 0x17 > > +#define RGB_TO_CERGB 0x1C > > +#define MATRIX_BIT BIT(8) > > +#define EXT_MATRIX_EN BIT(12) > > #endif /* __MTK_DPI_REGS_H */ > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > > index 245d0074e12d..3738665a712e 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > > @@ -419,6 +419,11 @@ static const char * const > > mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = { > > [MTK_DISP_WDMA] = "wdma", > > [MTK_DPI] = "dpi", > > [MTK_DSI] = "dsi", > > + [MTK_DP_INTF] = "dp-intf", > > + [MTK_DISP_PWM] = "pwm", > > + [MTK_DISP_MUTEX] = "mutex", > > + [MTK_DISP_OD] = "od", > > + [MTK_DISP_BLS] = "bls", > > Please keep alphabetic order. > Some of this are redundant, and I will remove them. > > }; > > > > struct mtk_ddp_comp_match { > > @@ -439,6 +444,8 @@ static const struct mtk_ddp_comp_match > > mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = { > > [DDP_COMPONENT_DPI1] = { MTK_DPI, > > 1, &ddp_dpi }, > > [DDP_COMPONENT_DSC0] = { MTK_DISP_DSC, > > 0, &ddp_dsc }, > > [DDP_COMPONENT_DSC1] = { MTK_DISP_DSC, > > 1, &ddp_dsc }, > > + [DDP_COMPONENT_DP_INTF0] = { MTK_DP_INTF, 0, &ddp_dpi > > }, > > + [DDP_COMPONENT_DP_INTF1] = { MTK_DP_INTF, 1, &ddp_dpi > > }, > > Indentation issue. Fix it. > Some of this are redundant, and I will remove them. > > [DDP_COMPONENT_DSI0] = { MTK_DSI, > > 0, &ddp_dsi }, > > [DDP_COMPONENT_DSI1] = { MTK_DSI, > > 1, &ddp_dsi }, > > [DDP_COMPONENT_DSI2] = { MTK_DSI, > > 2, &ddp_dsi }, > > @@ -565,6 +572,7 @@ int mtk_ddp_comp_init(struct device_node *node, > > struct mtk_ddp_comp *comp, > > type == MTK_DISP_PWM || > > type == MTK_DISP_RDMA || > > type == MTK_DPI || > > + type == MTK_DP_INTF || > > type == MTK_DSI) > > return 0; > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > index 825d763d2378..c4e683f46a95 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > > @@ -37,6 +37,7 @@ enum mtk_ddp_comp_type { > > MTK_DISP_UFOE, > > MTK_DISP_WDMA, > > MTK_DPI, > > + MTK_DP_INTF, > > MTK_DSI, > > MTK_DDP_COMP_TYPE_MAX, > > }; > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > index a2a783fc580e..e25ac61aac08 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > @@ -690,6 +690,8 @@ static const struct of_device_id > > mtk_ddp_comp_dt_ids[] = { > > .data = (void *)MTK_DPI }, > > { .compatible = "mediatek,mt8183-dpi", > > .data = (void *)MTK_DPI }, > > + { .compatible = "mediatek,mt8195-dpintf", > > + .data = (void *)MTK_DP_INTF }, > > { .compatible = "mediatek,mt2701-dsi", > > .data = (void *)MTK_DSI }, > > { .compatible = "mediatek,mt8173-dsi", > > @@ -801,8 +803,9 @@ static int mtk_drm_probe(struct platform_device > > *pdev) > > comp_type == MTK_DISP_OVL_2L || > > comp_type == MTK_DISP_OVL_ADAPTOR || > > comp_type == MTK_DISP_RDMA || > > + comp_type == MTK_DSI || > > comp_type == MTK_DPI || > > - comp_type == MTK_DSI) { > > + comp_type == MTK_DP_INTF) { > > These were nice and alphabetically ordered, why are you moving > MTK_DSI up there?! > > comp_type == MTK_DISP_RDMA || > comp_type == MTK_DP_INTF || > comp_type == MTK_DPI || > comp_type == MTK_DSI) { > > ... that's how it should look like. > Some of this are redundant, and I will remove them. > > dev_info(dev, "Adding component match for > > %pOF\n", > > node); > > drm_of_component_match_add(dev, &match, > > component_compare_of, > > diff --git a/include/linux/soc/mediatek/mtk-mmsys.h > > b/include/linux/soc/mediatek/mtk-mmsys.h > > index 3e998bfb795a..e4b84c347201 100644 > > --- a/include/linux/soc/mediatek/mtk-mmsys.h > > +++ b/include/linux/soc/mediatek/mtk-mmsys.h > > @@ -21,12 +21,12 @@ enum mtk_ddp_comp_id { > > DDP_COMPONENT_COLOR0, > > DDP_COMPONENT_COLOR1, > > DDP_COMPONENT_DITHER, > > - DDP_COMPONENT_DP_INTF0, > > - DDP_COMPONENT_DP_INTF1, > > DDP_COMPONENT_DPI0, > > DDP_COMPONENT_DPI1, > > DDP_COMPONENT_DSC0, > > DDP_COMPONENT_DSC1, > > + DDP_COMPONENT_DP_INTF0, > > + DDP_COMPONENT_DP_INTF1, > > Why are you moving this?! > I will fix this. BRs, Bo-Chen > > DDP_COMPONENT_DSI0, > > DDP_COMPONENT_DSI1, > > DDP_COMPONENT_DSI2, > >