| Message ID | 20211110130623.20553-1-granquet@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/mediatek: Add mt8195 DisplayPort driver | expand |
Hi, On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > 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. > > > > If the PHY is in the same register space than the DP controller, why do > > you need a separate PHY driver in the first place? > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ It's a bit of a circular argument though :) It's a separate phy driver because it needs to go through another maintainer's tree, but it needs to go through another maintainer's tree because it's a separate phy driver. It doesn't explain why it needs to be a separate phy driver? Why can't the phy setup be done directly in the DP driver, if it's essentially a single device? That being said, usually what those kind of questions mean is that you're missing a comment or something in the commit log to provide that context in the first place, so it would be great to add that context here. And it will avoid the situation we're now in where multiple reviewers ask the same questions over and over again :) > > > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > > > + struct drm_bridge_state *old_state) > > > +{ > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > + struct drm_connector *conn; > > > + struct drm_connector_state *conn_state; > > > + struct drm_crtc *crtc; > > > + struct drm_crtc_state *crtc_state; > > > + int ret = 0; > > > + int i; > > > + > > > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > > + bridge->encoder); > > > + if (!conn) { > > > + drm_err(mtk_dp->drm_dev, > > > + "Can't enable bridge as connector is missing\n"); > > > + return; > > > + } > > > + > > > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > > > > This should be protected by a mutex (just like any resource shared > > between KMS and ALSA) > > Ok. I forgot to ask (even though checkpatch does mention it iirc), but since you have multiple mutex it would be nice to have a comment for each mutex stating exactly what it protects, and against what. It's hard otherwise to remember or figure out what the locks are here for. > > > + ret = mtk_dp_dt_parse_pdata(mtk_dp, pdev); > > > + if (ret) > > > + return ret; > > > > pdata? > > > can you elaborate? Sorry, yeah, pdata is usually the abbreviation used in linux for the platform_data mechanism, but you're using the DT to retrieve your resources (and platform_data usually don't involve any parsing), so the name is odd. > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > index 384074f69111b..e6e88e3cd811d 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > @@ -63,6 +63,14 @@ enum mtk_dpi_out_color_format { > > > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > > > }; > > > > > > +enum TVDPLL_CLK { > > > + TVDPLL_PLL = 0, > > > + TVDPLL_D2 = 2, > > > + TVDPLL_D4 = 4, > > > + TVDPLL_D8 = 8, > > > + TVDPLL_D16 = 16, > > > +}; > > > + > > > struct mtk_dpi { > > > struct drm_encoder encoder; > > > struct drm_bridge bridge; > > > @@ -71,8 +79,10 @@ 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; > > > + struct clk *pclk_src[5]; > > > int irq; > > > struct drm_display_mode mode; > > > const struct mtk_dpi_conf *conf; > > > @@ -135,6 +145,7 @@ struct mtk_dpi_conf { > > > u32 hvsize_mask; > > > u32 channel_swap_shift; > > > u32 yuv422_en_bit; > > > + u32 csc_enable_bit; > > > const struct mtk_dpi_yc_limit *limit; > > > }; > > > > > > @@ -365,7 +376,7 @@ static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool enable) > > > > > > static void mtk_dpi_config_csc_enable(struct mtk_dpi *dpi, bool enable) > > > { > > > - mtk_dpi_mask(dpi, DPI_CON, enable ? CSC_ENABLE : 0, CSC_ENABLE); > > > + mtk_dpi_mask(dpi, DPI_CON, enable ? dpi->conf->csc_enable_bit : 0, dpi->conf->csc_enable_bit); > > > } > > > > > > static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable) > > > @@ -384,22 +395,45 @@ static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi) > > > mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN); > > > } > > > > > > +static void mtk_dpi_matrix_sel(struct mtk_dpi *dpi, enum mtk_dpi_out_color_format format) > > > +{ > > > + u32 matrix_sel = 0; > > > + > > > + switch (format) { > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422: > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL: > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444: > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL: > > > + case MTK_DPI_COLOR_FORMAT_XV_YCC: > > > + if (dpi->mode.hdisplay <= 720) > > > + matrix_sel = 0x2; > > > + break; > > > + default: > > > + break; > > > + } > > > + mtk_dpi_mask(dpi, DPI_MATRIX_SET, matrix_sel, INT_MATRIX_SEL_MASK); > > > +} > > > + > > > static void mtk_dpi_config_color_format(struct mtk_dpi *dpi, > > > enum mtk_dpi_out_color_format format) > > > { > > > if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_444) || > > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > > > mtk_dpi_config_yuv422_enable(dpi, false); > > > - if (dpi->conf->csc_support) > > > + if (dpi->conf->csc_support) { > > > mtk_dpi_config_csc_enable(dpi, true); > > > + mtk_dpi_matrix_sel(dpi, format); > > > + } > > > 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); > > > - if (dpi->conf->csc_support) > > > + if (dpi->conf->csc_support) { > > > mtk_dpi_config_csc_enable(dpi, true); > > > + mtk_dpi_matrix_sel(dpi, format); > > > + } > > > if (dpi->conf->swap_input_support) > > > mtk_dpi_config_swap_input(dpi, true); > > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > > @@ -441,6 +475,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) > > > @@ -450,12 +486,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); > > > @@ -465,10 +513,11 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > if (dpi->pinctrl && dpi->pins_dpi) > > > pinctrl_select_state(dpi->pinctrl, dpi->pins_dpi); > > > > > > - mtk_dpi_enable(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--; > > > @@ -500,9 +549,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > pll_rate = clk_get_rate(dpi->tvd_clk); > > > > > > vm.pixelclock = pll_rate / factor; > > > - if (dpi->conf->is_dpintf) > > > - clk_set_rate(dpi->pixel_clk, vm.pixelclock / 4); > > > - else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > + if (dpi->conf->is_dpintf) { > > > + if (factor == 1) > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > + else if (factor == 2) > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[3]); > > > + else if (factor == 4) > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[4]); > > > + else > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > + } else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) > > > clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2); > > > else > > > @@ -581,6 +637,8 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > } > > > mtk_dpi_sw_reset(dpi, false); > > > > > > + mtk_dpi_enable(dpi); > > > + > > > return 0; > > > } > > > > > > @@ -623,7 +681,6 @@ static u32 *mtk_dpi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > > > u32 *input_fmts; > > > > > > *num_input_fmts = 0; > > > - > > > input_fmts = kcalloc(1, sizeof(*input_fmts), > > > GFP_KERNEL); > > > if (!input_fmts) > > > @@ -649,7 +706,7 @@ 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", > > > + 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); > > > > > > @@ -657,7 +714,10 @@ static int mtk_dpi_bridge_atomic_check(struct drm_bridge *bridge, > > > 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; > > > } > > > @@ -835,6 +895,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, > > > @@ -862,6 +928,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > > > .hvsize_mask = HSIZE_MASK, > > > .channel_swap_shift = CH_SWAP, > > > .yuv422_en_bit = YUV422_EN, > > > + .csc_enable_bit = CSC_ENABLE, > > > .limit = &mtk_dpi_limit, > > > }; > > > > > > @@ -879,6 +946,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > > > .hvsize_mask = HSIZE_MASK, > > > .channel_swap_shift = CH_SWAP, > > > .yuv422_en_bit = YUV422_EN, > > > + .csc_enable_bit = CSC_ENABLE, > > > .limit = &mtk_dpi_limit, > > > }; > > > > > > @@ -895,6 +963,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > > > .hvsize_mask = HSIZE_MASK, > > > .channel_swap_shift = CH_SWAP, > > > .yuv422_en_bit = YUV422_EN, > > > + .csc_enable_bit = CSC_ENABLE, > > > .limit = &mtk_dpi_limit, > > > }; > > > > > > @@ -911,18 +980,21 @@ static const struct mtk_dpi_conf mt8192_conf = { > > > .hvsize_mask = HSIZE_MASK, > > > .channel_swap_shift = CH_SWAP, > > > .yuv422_en_bit = YUV422_EN, > > > + .csc_enable_bit = CSC_ENABLE, > > > .limit = &mtk_dpi_limit, > > > }; > > > > > > static const struct mtk_dpi_conf mt8195_dpintf_conf = { > > > .cal_factor = mt8195_dpintf_calculate_factor, > > > - .output_fmts = mt8173_output_fmts, > > > - .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > > + .output_fmts = mt8195_output_fmts, > > > + .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts), > > > .is_dpintf = true, > > > + .csc_support = 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, > > > }; > > > > > > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > + dpi->dpi_ck_cg = devm_clk_get(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; > > > + } > > > + > > > dpi->pixel_clk = devm_clk_get(dev, "pixel"); > > > if (IS_ERR(dpi->pixel_clk)) { > > > ret = PTR_ERR(dpi->pixel_clk); > > > @@ -997,6 +1079,11 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > + dpi->pclk_src[1] = devm_clk_get(dev, "TVDPLL_D2"); > > > + dpi->pclk_src[2] = devm_clk_get(dev, "TVDPLL_D4"); > > > + dpi->pclk_src[3] = devm_clk_get(dev, "TVDPLL_D8"); > > > + dpi->pclk_src[4] = devm_clk_get(dev, "TVDPLL_D16"); > > > + > > > dpi->irq = platform_get_irq(pdev, 0); > > > if (dpi->irq <= 0) > > > return -EINVAL; > > > > All those changes look unrelated as well? > > > Do you mean all the changes in mtk_dpi.c ? They are in support of > enabling the mt8195 dpintf driver... so, not sure they are unlreated? I don't know that driver at all, so it's probably related if you say so :) The DPI interface seems to be a panel interface, so it's a bit weird to me why you would need to change things in the panel interface while claiming to support a DP output. Especially since it looks like you have some support for CSC, and support for additional output formats? I guess the DPI interface feeds the DP that essentially acts as a RGB -> DP bridge? If so, then the part adding support for CSC and additional formats should be split into a separate patch, and it should be mentioned in the commit log Maxime
Hi, Thanks for all your input, really appreciated. Quoting Maxime Ripard (2021-11-16 15:51:12) > Hi, > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > 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. > > > > > > If the PHY is in the same register space than the DP controller, why do > > > you need a separate PHY driver in the first place? > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > It's a bit of a circular argument though :) > > It's a separate phy driver because it needs to go through another > maintainer's tree, but it needs to go through another maintainer's tree > because it's a separate phy driver. > > It doesn't explain why it needs to be a separate phy driver? Why can't > the phy setup be done directly in the DP driver, if it's essentially a > single device? > > That being said, usually what those kind of questions mean is that > you're missing a comment or something in the commit log to provide that > context in the first place, so it would be great to add that context > here. > > And it will avoid the situation we're now in where multiple reviewers > ask the same questions over and over again :) > At first I didn't understand your reply, then I realized I gave you the wrong link... my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. The driver and phy were a single driver until v2 of this patch series and the phy setup was done directly in the driver (single driver, single C file). Here's the relevant link to the discussion between Chun-Kuang and Markus https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t I'll try to find a way to make it clearer for v7. > > > > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > > > > + struct drm_bridge_state *old_state) > > > > +{ > > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > > + struct drm_connector *conn; > > > > + struct drm_connector_state *conn_state; > > > > + struct drm_crtc *crtc; > > > > + struct drm_crtc_state *crtc_state; > > > > + int ret = 0; > > > > + int i; > > > > + > > > > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > > > + bridge->encoder); > > > > + if (!conn) { > > > > + drm_err(mtk_dp->drm_dev, > > > > + "Can't enable bridge as connector is missing\n"); > > > > + return; > > > > + } > > > > + > > > > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > > > > > > This should be protected by a mutex (just like any resource shared > > > between KMS and ALSA) > > > > Ok. > > I forgot to ask (even though checkpatch does mention it iirc), but since > you have multiple mutex it would be nice to have a comment for each > mutex stating exactly what it protects, and against what. > > It's hard otherwise to remember or figure out what the locks are here > for. > > > > > + ret = mtk_dp_dt_parse_pdata(mtk_dp, pdev); > > > > + if (ret) > > > > + return ret; > > > > > > pdata? > > > > > can you elaborate? > > Sorry, yeah, pdata is usually the abbreviation used in linux for the > platform_data mechanism, but you're using the DT to retrieve your > resources (and platform_data usually don't involve any parsing), so the > name is odd. > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > index 384074f69111b..e6e88e3cd811d 100644 > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > @@ -63,6 +63,14 @@ enum mtk_dpi_out_color_format { > > > > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > > > > }; > > > > > > > > +enum TVDPLL_CLK { > > > > + TVDPLL_PLL = 0, > > > > + TVDPLL_D2 = 2, > > > > + TVDPLL_D4 = 4, > > > > + TVDPLL_D8 = 8, > > > > + TVDPLL_D16 = 16, > > > > +}; > > > > + > > > > struct mtk_dpi { > > > > struct drm_encoder encoder; > > > > struct drm_bridge bridge; > > > > @@ -71,8 +79,10 @@ 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; > > > > + struct clk *pclk_src[5]; > > > > int irq; > > > > struct drm_display_mode mode; > > > > const struct mtk_dpi_conf *conf; > > > > @@ -135,6 +145,7 @@ struct mtk_dpi_conf { > > > > u32 hvsize_mask; > > > > u32 channel_swap_shift; > > > > u32 yuv422_en_bit; > > > > + u32 csc_enable_bit; > > > > const struct mtk_dpi_yc_limit *limit; > > > > }; > > > > > > > > @@ -365,7 +376,7 @@ static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool enable) > > > > > > > > static void mtk_dpi_config_csc_enable(struct mtk_dpi *dpi, bool enable) > > > > { > > > > - mtk_dpi_mask(dpi, DPI_CON, enable ? CSC_ENABLE : 0, CSC_ENABLE); > > > > + mtk_dpi_mask(dpi, DPI_CON, enable ? dpi->conf->csc_enable_bit : 0, dpi->conf->csc_enable_bit); > > > > } > > > > > > > > static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable) > > > > @@ -384,22 +395,45 @@ static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi) > > > > mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN); > > > > } > > > > > > > > +static void mtk_dpi_matrix_sel(struct mtk_dpi *dpi, enum mtk_dpi_out_color_format format) > > > > +{ > > > > + u32 matrix_sel = 0; > > > > + > > > > + switch (format) { > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422: > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL: > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444: > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL: > > > > + case MTK_DPI_COLOR_FORMAT_XV_YCC: > > > > + if (dpi->mode.hdisplay <= 720) > > > > + matrix_sel = 0x2; > > > > + break; > > > > + default: > > > > + break; > > > > + } > > > > + mtk_dpi_mask(dpi, DPI_MATRIX_SET, matrix_sel, INT_MATRIX_SEL_MASK); > > > > +} > > > > + > > > > static void mtk_dpi_config_color_format(struct mtk_dpi *dpi, > > > > enum mtk_dpi_out_color_format format) > > > > { > > > > if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_444) || > > > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > > > > mtk_dpi_config_yuv422_enable(dpi, false); > > > > - if (dpi->conf->csc_support) > > > > + if (dpi->conf->csc_support) { > > > > mtk_dpi_config_csc_enable(dpi, true); > > > > + mtk_dpi_matrix_sel(dpi, format); > > > > + } > > > > 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); > > > > - if (dpi->conf->csc_support) > > > > + if (dpi->conf->csc_support) { > > > > mtk_dpi_config_csc_enable(dpi, true); > > > > + mtk_dpi_matrix_sel(dpi, format); > > > > + } > > > > if (dpi->conf->swap_input_support) > > > > mtk_dpi_config_swap_input(dpi, true); > > > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > > > @@ -441,6 +475,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) > > > > @@ -450,12 +486,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); > > > > @@ -465,10 +513,11 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > if (dpi->pinctrl && dpi->pins_dpi) > > > > pinctrl_select_state(dpi->pinctrl, dpi->pins_dpi); > > > > > > > > - mtk_dpi_enable(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--; > > > > @@ -500,9 +549,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > > pll_rate = clk_get_rate(dpi->tvd_clk); > > > > > > > > vm.pixelclock = pll_rate / factor; > > > > - if (dpi->conf->is_dpintf) > > > > - clk_set_rate(dpi->pixel_clk, vm.pixelclock / 4); > > > > - else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > > + if (dpi->conf->is_dpintf) { > > > > + if (factor == 1) > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > > + else if (factor == 2) > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[3]); > > > > + else if (factor == 4) > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[4]); > > > > + else > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > > + } else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > > (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) > > > > clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2); > > > > else > > > > @@ -581,6 +637,8 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > > } > > > > mtk_dpi_sw_reset(dpi, false); > > > > > > > > + mtk_dpi_enable(dpi); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -623,7 +681,6 @@ static u32 *mtk_dpi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > > > > u32 *input_fmts; > > > > > > > > *num_input_fmts = 0; > > > > - > > > > input_fmts = kcalloc(1, sizeof(*input_fmts), > > > > GFP_KERNEL); > > > > if (!input_fmts) > > > > @@ -649,7 +706,7 @@ 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", > > > > + 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); > > > > > > > > @@ -657,7 +714,10 @@ static int mtk_dpi_bridge_atomic_check(struct drm_bridge *bridge, > > > > 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; > > > > } > > > > @@ -835,6 +895,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, > > > > @@ -862,6 +928,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > > > > .hvsize_mask = HSIZE_MASK, > > > > .channel_swap_shift = CH_SWAP, > > > > .yuv422_en_bit = YUV422_EN, > > > > + .csc_enable_bit = CSC_ENABLE, > > > > .limit = &mtk_dpi_limit, > > > > }; > > > > > > > > @@ -879,6 +946,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > > > > .hvsize_mask = HSIZE_MASK, > > > > .channel_swap_shift = CH_SWAP, > > > > .yuv422_en_bit = YUV422_EN, > > > > + .csc_enable_bit = CSC_ENABLE, > > > > .limit = &mtk_dpi_limit, > > > > }; > > > > > > > > @@ -895,6 +963,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > > > > .hvsize_mask = HSIZE_MASK, > > > > .channel_swap_shift = CH_SWAP, > > > > .yuv422_en_bit = YUV422_EN, > > > > + .csc_enable_bit = CSC_ENABLE, > > > > .limit = &mtk_dpi_limit, > > > > }; > > > > > > > > @@ -911,18 +980,21 @@ static const struct mtk_dpi_conf mt8192_conf = { > > > > .hvsize_mask = HSIZE_MASK, > > > > .channel_swap_shift = CH_SWAP, > > > > .yuv422_en_bit = YUV422_EN, > > > > + .csc_enable_bit = CSC_ENABLE, > > > > .limit = &mtk_dpi_limit, > > > > }; > > > > > > > > static const struct mtk_dpi_conf mt8195_dpintf_conf = { > > > > .cal_factor = mt8195_dpintf_calculate_factor, > > > > - .output_fmts = mt8173_output_fmts, > > > > - .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > > > + .output_fmts = mt8195_output_fmts, > > > > + .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts), > > > > .is_dpintf = true, > > > > + .csc_support = 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, > > > > }; > > > > > > > > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > > return ret; > > > > } > > > > > > > > + dpi->dpi_ck_cg = devm_clk_get(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; > > > > + } > > > > + > > > > dpi->pixel_clk = devm_clk_get(dev, "pixel"); > > > > if (IS_ERR(dpi->pixel_clk)) { > > > > ret = PTR_ERR(dpi->pixel_clk); > > > > @@ -997,6 +1079,11 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > > return ret; > > > > } > > > > > > > > + dpi->pclk_src[1] = devm_clk_get(dev, "TVDPLL_D2"); > > > > + dpi->pclk_src[2] = devm_clk_get(dev, "TVDPLL_D4"); > > > > + dpi->pclk_src[3] = devm_clk_get(dev, "TVDPLL_D8"); > > > > + dpi->pclk_src[4] = devm_clk_get(dev, "TVDPLL_D16"); > > > > + > > > > dpi->irq = platform_get_irq(pdev, 0); > > > > if (dpi->irq <= 0) > > > > return -EINVAL; > > > > > > All those changes look unrelated as well? > > > > > Do you mean all the changes in mtk_dpi.c ? They are in support of > > enabling the mt8195 dpintf driver... so, not sure they are unlreated? > > I don't know that driver at all, so it's probably related if you say so > :) > > The DPI interface seems to be a panel interface, so it's a bit weird to > me why you would need to change things in the panel interface while > claiming to support a DP output. > > Especially since it looks like you have some support for CSC, and > support for additional output formats? > > I guess the DPI interface feeds the DP that essentially acts as a RGB -> > DP bridge? > > If so, then the part adding support for CSC and additional formats > should be split into a separate patch, and it should be mentioned in the > commit log > > Maxime I see what you mean, I'll split the patches functionally... I'm discovering the gpu/drm tree, so It may take me a while to get it right :D Thx, Guillaume.
On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > Hi, > Thanks for all your input, really appreciated. > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > Hi, > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > 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. > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > you need a separate PHY driver in the first place? > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > It's a bit of a circular argument though :) > > > > It's a separate phy driver because it needs to go through another > > maintainer's tree, but it needs to go through another maintainer's tree > > because it's a separate phy driver. > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > the phy setup be done directly in the DP driver, if it's essentially a > > single device? > > > > That being said, usually what those kind of questions mean is that > > you're missing a comment or something in the commit log to provide that > > context in the first place, so it would be great to add that context > > here. > > > > And it will avoid the situation we're now in where multiple reviewers > > ask the same questions over and over again :) > > > At first I didn't understand your reply, then I realized I gave you > the wrong link... > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > The driver and phy were a single driver until v2 of this patch series > and the phy setup > was done directly in the driver (single driver, single C file). > Here's the relevant link to the discussion between Chun-Kuang and Markus > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > I'll try to find a way to make it clearer for v7. OK, it makes sense then :) There's something weird though: the devices definitely look like they're in a separate register range, yet you mention a regmap to handle the shared register range. That range doesn't seem described anywhere in the device tree though? What is it for? > > > > > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > > > > > + struct drm_bridge_state *old_state) > > > > > +{ > > > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > > > + struct drm_connector *conn; > > > > > + struct drm_connector_state *conn_state; > > > > > + struct drm_crtc *crtc; > > > > > + struct drm_crtc_state *crtc_state; > > > > > + int ret = 0; > > > > > + int i; > > > > > + > > > > > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > > > > + bridge->encoder); > > > > > + if (!conn) { > > > > > + drm_err(mtk_dp->drm_dev, > > > > > + "Can't enable bridge as connector is missing\n"); > > > > > + return; > > > > > + } > > > > > + > > > > > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > > > > > > > > This should be protected by a mutex (just like any resource shared > > > > between KMS and ALSA) > > > > > > Ok. > > > > I forgot to ask (even though checkpatch does mention it iirc), but since > > you have multiple mutex it would be nice to have a comment for each > > mutex stating exactly what it protects, and against what. > > > > It's hard otherwise to remember or figure out what the locks are here > > for. > > > > > > > + ret = mtk_dp_dt_parse_pdata(mtk_dp, pdev); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > pdata? > > > > > > > can you elaborate? > > > > Sorry, yeah, pdata is usually the abbreviation used in linux for the > > platform_data mechanism, but you're using the DT to retrieve your > > resources (and platform_data usually don't involve any parsing), so the > > name is odd. > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > index 384074f69111b..e6e88e3cd811d 100644 > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > @@ -63,6 +63,14 @@ enum mtk_dpi_out_color_format { > > > > > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > > > > > }; > > > > > > > > > > +enum TVDPLL_CLK { > > > > > + TVDPLL_PLL = 0, > > > > > + TVDPLL_D2 = 2, > > > > > + TVDPLL_D4 = 4, > > > > > + TVDPLL_D8 = 8, > > > > > + TVDPLL_D16 = 16, > > > > > +}; > > > > > + > > > > > struct mtk_dpi { > > > > > struct drm_encoder encoder; > > > > > struct drm_bridge bridge; > > > > > @@ -71,8 +79,10 @@ 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; > > > > > + struct clk *pclk_src[5]; > > > > > int irq; > > > > > struct drm_display_mode mode; > > > > > const struct mtk_dpi_conf *conf; > > > > > @@ -135,6 +145,7 @@ struct mtk_dpi_conf { > > > > > u32 hvsize_mask; > > > > > u32 channel_swap_shift; > > > > > u32 yuv422_en_bit; > > > > > + u32 csc_enable_bit; > > > > > const struct mtk_dpi_yc_limit *limit; > > > > > }; > > > > > > > > > > @@ -365,7 +376,7 @@ static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool enable) > > > > > > > > > > static void mtk_dpi_config_csc_enable(struct mtk_dpi *dpi, bool enable) > > > > > { > > > > > - mtk_dpi_mask(dpi, DPI_CON, enable ? CSC_ENABLE : 0, CSC_ENABLE); > > > > > + mtk_dpi_mask(dpi, DPI_CON, enable ? dpi->conf->csc_enable_bit : 0, dpi->conf->csc_enable_bit); > > > > > } > > > > > > > > > > static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable) > > > > > @@ -384,22 +395,45 @@ static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi) > > > > > mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN); > > > > > } > > > > > > > > > > +static void mtk_dpi_matrix_sel(struct mtk_dpi *dpi, enum mtk_dpi_out_color_format format) > > > > > +{ > > > > > + u32 matrix_sel = 0; > > > > > + > > > > > + switch (format) { > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422: > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL: > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444: > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL: > > > > > + case MTK_DPI_COLOR_FORMAT_XV_YCC: > > > > > + if (dpi->mode.hdisplay <= 720) > > > > > + matrix_sel = 0x2; > > > > > + break; > > > > > + default: > > > > > + break; > > > > > + } > > > > > + mtk_dpi_mask(dpi, DPI_MATRIX_SET, matrix_sel, INT_MATRIX_SEL_MASK); > > > > > +} > > > > > + > > > > > static void mtk_dpi_config_color_format(struct mtk_dpi *dpi, > > > > > enum mtk_dpi_out_color_format format) > > > > > { > > > > > if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_444) || > > > > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > > > > > mtk_dpi_config_yuv422_enable(dpi, false); > > > > > - if (dpi->conf->csc_support) > > > > > + if (dpi->conf->csc_support) { > > > > > mtk_dpi_config_csc_enable(dpi, true); > > > > > + mtk_dpi_matrix_sel(dpi, format); > > > > > + } > > > > > 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); > > > > > - if (dpi->conf->csc_support) > > > > > + if (dpi->conf->csc_support) { > > > > > mtk_dpi_config_csc_enable(dpi, true); > > > > > + mtk_dpi_matrix_sel(dpi, format); > > > > > + } > > > > > if (dpi->conf->swap_input_support) > > > > > mtk_dpi_config_swap_input(dpi, true); > > > > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > > > > @@ -441,6 +475,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) > > > > > @@ -450,12 +486,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); > > > > > @@ -465,10 +513,11 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > > if (dpi->pinctrl && dpi->pins_dpi) > > > > > pinctrl_select_state(dpi->pinctrl, dpi->pins_dpi); > > > > > > > > > > - mtk_dpi_enable(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--; > > > > > @@ -500,9 +549,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > > > pll_rate = clk_get_rate(dpi->tvd_clk); > > > > > > > > > > vm.pixelclock = pll_rate / factor; > > > > > - if (dpi->conf->is_dpintf) > > > > > - clk_set_rate(dpi->pixel_clk, vm.pixelclock / 4); > > > > > - else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > > > + if (dpi->conf->is_dpintf) { > > > > > + if (factor == 1) > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > > > + else if (factor == 2) > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[3]); > > > > > + else if (factor == 4) > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[4]); > > > > > + else > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > > > + } else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > > > (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) > > > > > clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2); > > > > > else > > > > > @@ -581,6 +637,8 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > > > } > > > > > mtk_dpi_sw_reset(dpi, false); > > > > > > > > > > + mtk_dpi_enable(dpi); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -623,7 +681,6 @@ static u32 *mtk_dpi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > > > > > u32 *input_fmts; > > > > > > > > > > *num_input_fmts = 0; > > > > > - > > > > > input_fmts = kcalloc(1, sizeof(*input_fmts), > > > > > GFP_KERNEL); > > > > > if (!input_fmts) > > > > > @@ -649,7 +706,7 @@ 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", > > > > > + 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); > > > > > > > > > > @@ -657,7 +714,10 @@ static int mtk_dpi_bridge_atomic_check(struct drm_bridge *bridge, > > > > > 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; > > > > > } > > > > > @@ -835,6 +895,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, > > > > > @@ -862,6 +928,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > > > > > .hvsize_mask = HSIZE_MASK, > > > > > .channel_swap_shift = CH_SWAP, > > > > > .yuv422_en_bit = YUV422_EN, > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > .limit = &mtk_dpi_limit, > > > > > }; > > > > > > > > > > @@ -879,6 +946,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > > > > > .hvsize_mask = HSIZE_MASK, > > > > > .channel_swap_shift = CH_SWAP, > > > > > .yuv422_en_bit = YUV422_EN, > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > .limit = &mtk_dpi_limit, > > > > > }; > > > > > > > > > > @@ -895,6 +963,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > > > > > .hvsize_mask = HSIZE_MASK, > > > > > .channel_swap_shift = CH_SWAP, > > > > > .yuv422_en_bit = YUV422_EN, > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > .limit = &mtk_dpi_limit, > > > > > }; > > > > > > > > > > @@ -911,18 +980,21 @@ static const struct mtk_dpi_conf mt8192_conf = { > > > > > .hvsize_mask = HSIZE_MASK, > > > > > .channel_swap_shift = CH_SWAP, > > > > > .yuv422_en_bit = YUV422_EN, > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > .limit = &mtk_dpi_limit, > > > > > }; > > > > > > > > > > static const struct mtk_dpi_conf mt8195_dpintf_conf = { > > > > > .cal_factor = mt8195_dpintf_calculate_factor, > > > > > - .output_fmts = mt8173_output_fmts, > > > > > - .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > > > > + .output_fmts = mt8195_output_fmts, > > > > > + .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts), > > > > > .is_dpintf = true, > > > > > + .csc_support = 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, > > > > > }; > > > > > > > > > > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > > > return ret; > > > > > } > > > > > > > > > > + dpi->dpi_ck_cg = devm_clk_get(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; > > > > > + } > > > > > + > > > > > dpi->pixel_clk = devm_clk_get(dev, "pixel"); > > > > > if (IS_ERR(dpi->pixel_clk)) { > > > > > ret = PTR_ERR(dpi->pixel_clk); > > > > > @@ -997,6 +1079,11 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > > > return ret; > > > > > } > > > > > > > > > > + dpi->pclk_src[1] = devm_clk_get(dev, "TVDPLL_D2"); > > > > > + dpi->pclk_src[2] = devm_clk_get(dev, "TVDPLL_D4"); > > > > > + dpi->pclk_src[3] = devm_clk_get(dev, "TVDPLL_D8"); > > > > > + dpi->pclk_src[4] = devm_clk_get(dev, "TVDPLL_D16"); > > > > > + > > > > > dpi->irq = platform_get_irq(pdev, 0); > > > > > if (dpi->irq <= 0) > > > > > return -EINVAL; > > > > > > > > All those changes look unrelated as well? > > > > > > > Do you mean all the changes in mtk_dpi.c ? They are in support of > > > enabling the mt8195 dpintf driver... so, not sure they are unlreated? > > > > I don't know that driver at all, so it's probably related if you say so > > :) > > > > The DPI interface seems to be a panel interface, so it's a bit weird to > > me why you would need to change things in the panel interface while > > claiming to support a DP output. > > > > Especially since it looks like you have some support for CSC, and > > support for additional output formats? > > > > I guess the DPI interface feeds the DP that essentially acts as a RGB -> > > DP bridge? > > > > If so, then the part adding support for CSC and additional formats > > should be split into a separate patch, and it should be mentioned in the > > commit log > > > > Maxime > > I see what you mean, I'll split the patches functionally... > I'm discovering the gpu/drm tree, so It may take me a while to get it right :D It's not really yet another weird drm rule :) Splitting your patches is valuable for a bunch of reasons that apply to basically every tree: - It's easier to review (and maintainers like that). It's also easier to justify in a commit log, so it leads to a better history over the long term. - It's also easier to merge since you can merge smaller chunks when they are ready, even if the entire set is not. Smaller chunks also mean it's easier to apply (for the maintainer) and rebase (for you). - Finally, and possibly the strongest technical argument, it's much more helpful when you're bisecting. Taking the above patch as an example, let's assume you introduced a bug in some RGB panel with the colors being off due to the CSC changes. If you run git bisect, you'll end up with that big patch affecting multiple things and it would still be difficult to figure out what's wrong exactly. If it was split, you would end up only on the changeset that introduces the regression, which is very likely to be much easier to analyze. Maxime
Hi, Guillaume: This is a big patch, so I give some comment first. Guillaume Ranquet <granquet@baylibre.com> 於 2021年11月10日 週三 下午9:06寫道： > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > according phy driver mediatek-dp-phy. > > It supports both functional units on the mt8195, the embedded > DisplayPort as well as the external DisplayPort units. It offers > hot-plug-detection, audio up to 8 channels, and 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> > Reported-by: kernel test robot <lkp@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 2 +- Separate this to another patch. > drivers/gpu/drm/mediatek/Kconfig | 7 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 3094 +++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 +++++ > drivers/gpu/drm/mediatek/mtk_dpi.c | 111 +- Ditto. > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 26 + Ditto. > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + Ditto > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + Ditto > 9 files changed, 3799 insertions(+), 13 deletions(-) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 500279a82167a..bfd98b50ceb5b 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -5183,7 +5183,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector, > * modes and forbids YCRCB422 support for all video modes per > * HDMI 1.3 spec. > */ > - info->color_formats = DRM_COLOR_FORMAT_RGB444; > + info->color_formats |= DRM_COLOR_FORMAT_RGB444; > > /* YCRCB444 is optional according to spec. */ > if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) { > diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig > index 2976d21e9a34a..029b94c716131 100644 > --- a/drivers/gpu/drm/mediatek/Kconfig > +++ b/drivers/gpu/drm/mediatek/Kconfig > @@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI > select PHY_MTK_HDMI > help > DRM/KMS HDMI driver for Mediatek SoCs > + > +config MTK_DPTX_SUPPORT > + tristate "DRM DPTX Support for Mediatek SoCs" > + depends on DRM_MEDIATEK > + select PHY_MTK_DP > + help > + DRM/KMS Display Port driver for Mediatek SoCs. > diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile > index 29098d7c8307c..d86a6406055e6 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \ > mtk_hdmi_ddc.o > > obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o > + > +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > new file mode 100644 > index 0000000000000..83087219d5a5e > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -0,0 +1,3094 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019 MediaTek Inc. > + * Copyright (c) 2021 BayLibre > + */ > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_dp_helper.h> > +#include <drm/drm_edid.h> > +#include <drm/drm_of.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_print.h> > +#include <drm/drm_probe_helper.h> > +#include <linux/arm-smccc.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/kernel.h> > +#include <linux/nvmem-consumer.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <sound/hdmi-codec.h> > +#include <video/videomode.h> > + > +#include "mtk_dp_reg.h" > + > +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 > +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3 > + > +#define MTK_DP_MAX_LANES 4 > +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3 > + > +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 > + > +#define MTK_DP_TRAIN_RETRY_LIMIT 8 > +#define MTK_DP_TRAIN_MAX_ITERATIONS 5 > + > +#define MTK_DP_AUX_WRITE_READ_WAIT_TIME_US 20 > + > +#define MTK_DP_DP_VERSION_11 0x11 > + > +enum mtk_dp_state { > + MTK_DP_STATE_INITIAL, > + MTK_DP_STATE_IDLE, > + MTK_DP_STATE_PREPARE, > + MTK_DP_STATE_NORMAL, > +}; > + > +enum mtk_dp_train_state { > + MTK_DP_TRAIN_STATE_STARTUP = 0, > + MTK_DP_TRAIN_STATE_CHECKCAP, > + MTK_DP_TRAIN_STATE_CHECKEDID, > + MTK_DP_TRAIN_STATE_TRAINING_PRE, > + MTK_DP_TRAIN_STATE_TRAINING, > + MTK_DP_TRAIN_STATE_CHECKTIMING, > + MTK_DP_TRAIN_STATE_NORMAL, > + MTK_DP_TRAIN_STATE_POWERSAVE, > + MTK_DP_TRAIN_STATE_DPIDLE, > +}; > + > +struct mtk_dp_timings { > + struct videomode vm; > + > + u16 htotal; > + u16 vtotal; > + u8 frame_rate; > + u32 pix_rate_khz; > +}; > + > +struct mtk_dp_train_info { > + bool tps3; > + bool tps4; > + bool sink_ssc; > + bool cable_plugged_in; > + bool cable_state_change; > + bool cr_done; > + bool eq_done; > + > + // link_rate is in multiple of 0.27Gbps > + int link_rate; > + int lane_count; > + > + int irq_status; > + int check_cap_count; > +}; > + > +// Same values as used by the DP Spec for MISC0 bits 1 and 2 > +enum mtk_dp_color_format { > + MTK_DP_COLOR_FORMAT_RGB_444 = 0, > + MTK_DP_COLOR_FORMAT_YUV_422 = 1, > + MTK_DP_COLOR_FORMAT_YUV_444 = 2, > + MTK_DP_COLOR_FORMAT_YUV_420 = 3, > + MTK_DP_COLOR_FORMAT_YONLY = 4, > + MTK_DP_COLOR_FORMAT_RAW = 5, > + MTK_DP_COLOR_FORMAT_RESERVED = 6, > + MTK_DP_COLOR_FORMAT_DEFAULT = MTK_DP_COLOR_FORMAT_RGB_444, > + MTK_DP_COLOR_FORMAT_UNKNOWN = 15, > +}; > + > +// Multiple of 0.27Gbps > +enum mtk_dp_linkrate { > + MTK_DP_LINKRATE_RBR = 0x6, > + MTK_DP_LINKRATE_HBR = 0xA, > + MTK_DP_LINKRATE_HBR2 = 0x14, > + MTK_DP_LINKRATE_HBR25 = 0x19, > + MTK_DP_LINKRATE_HBR3 = 0x1E, > +}; > + > +// Same values as used for DP Spec MISC0 bits 5,6,7 > +enum mtk_dp_color_depth { > + MTK_DP_COLOR_DEPTH_6BIT = 0, > + MTK_DP_COLOR_DEPTH_8BIT = 1, > + MTK_DP_COLOR_DEPTH_10BIT = 2, > + MTK_DP_COLOR_DEPTH_12BIT = 3, > + MTK_DP_COLOR_DEPTH_16BIT = 4, > + MTK_DP_COLOR_DEPTH_UNKNOWN = 5, > +}; > + > +struct mtk_dp_audio_cfg { > + int sample_rate; > + int word_length_bits; > + int channels; > +}; > + > +struct mtk_dp_info { > + enum mtk_dp_color_depth depth; > + enum mtk_dp_color_format format; > + struct mtk_dp_audio_cfg audio_caps; > + struct mtk_dp_timings timings; > +}; > + > +struct dp_cal_data { > + unsigned int glb_bias_trim; > + unsigned int clktx_impse; > + > + unsigned int ln0_tx_impsel_pmos; > + unsigned int ln0_tx_impsel_nmos; > + unsigned int ln1_tx_impsel_pmos; > + unsigned int ln1_tx_impsel_nmos; > + unsigned int ln2_tx_impsel_pmos; > + unsigned int ln2_tx_impsel_nmos; > + unsigned int ln3_tx_impsel_pmos; > + unsigned int ln3_tx_impsel_nmos; > +}; > + > +struct mtk_dp { > + struct device *dev; > + struct platform_device *phy_dev; > + struct phy *phy; > + struct dp_cal_data cal_data; > + > + struct drm_device *drm_dev; > + struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > + struct drm_dp_aux aux; > + > + struct mutex edid_lock; > + struct edid *edid; > + > + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; > + > + struct mtk_dp_info info; > + enum mtk_dp_state state; > + > + struct mtk_dp_train_info train_info; > + enum mtk_dp_train_state train_state; > + unsigned int input_fmt; > + > + struct regmap *regs; > + struct clk *dp_tx_clk; > + > + bool enabled; > + bool audio_enable; > + > + bool has_fec; > + struct mutex dp_lock; > + > + struct mutex update_plugged_status_lock; > + > + hdmi_codec_plugged_cb plugged_cb; > + struct device *codec_dev; > + u8 connector_eld[MAX_ELD_BYTES]; > +}; > + > +enum mtk_dp_sdp_type { > + MTK_DP_SDP_NONE = 0x00, > + MTK_DP_SDP_ACM = 0x01, > + MTK_DP_SDP_ISRC = 0x02, > + MTK_DP_SDP_AVI = 0x03, > + MTK_DP_SDP_AUI = 0x04, > + MTK_DP_SDP_SPD = 0x05, > + MTK_DP_SDP_MPEG = 0x06, > + MTK_DP_SDP_NTSC = 0x07, > + MTK_DP_SDP_VSP = 0x08, > + MTK_DP_SDP_VSC = 0x09, > + MTK_DP_SDP_EXT = 0x0A, > + MTK_DP_SDP_PPS0 = 0x0B, > + MTK_DP_SDP_PPS1 = 0x0C, > + MTK_DP_SDP_PPS2 = 0x0D, > + MTK_DP_SDP_PPS3 = 0x0E, > + MTK_DP_SDP_DRM = 0x10, > + MTK_DP_SDP_MAX_NUM > +}; > + > +struct mtk_dp_sdp_packet { > + enum mtk_dp_sdp_type type; > + struct dp_sdp sdp; > +}; > + > +#define MTK_DP_IEC_CHANNEL_STATUS_LEN 5 > +union mtk_dp_audio_channel_status { > + struct { > + u8 rev : 1; > + u8 is_lpcm : 1; > + u8 copy_right : 1; > + u8 additional_format_info : 3; > + u8 channel_status_mode : 2; > + u8 category_code; > + u8 src_num : 4; > + u8 channel_num : 4; > + u8 sampling_freq : 4; > + u8 clk_accuracy : 2; > + u8 rev2 : 2; > + u8 word_len : 4; > + u8 original_sampling_freq : 4; > + } iec; > + > + u8 buf[MTK_DP_IEC_CHANNEL_STATUS_LEN]; > +}; > + > +static struct regmap_config mtk_dp_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = SEC_OFFSET + 0x90, > + .name = "mtk-dp-registers", > +}; > + > +static bool mtk_dp_is_edp(struct mtk_dp *mtk_dp) Separate edp and displayport into two patches. For example, the first patch support edp only, and the second patch add displayport function. > +{ > + return mtk_dp->next_bridge != NULL; > +} > + [snip] > + > +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge, > + struct drm_connector *connector) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + bool enabled = mtk_dp->enabled; > + struct edid *new_edid = NULL; > + > + if (!enabled) > + drm_bridge_chain_pre_enable(bridge); In mtk_hdmi_bridge_get_edid(), there does not check the power. Why in this function need this? Does mtk hdmi driver has a bug? > + > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0); > + usleep_range(2000, 5000); > + > + if (mtk_dp_plug_state(mtk_dp)) > + new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc); > + > + if (!enabled) > + drm_bridge_chain_post_disable(bridge); > + > + mutex_lock(&mtk_dp->edid_lock); > + kfree(mtk_dp->edid); > + mtk_dp->edid = NULL; > + > + if (!new_edid) { > + mutex_unlock(&mtk_dp->edid_lock); > + return NULL; > + } > + > + mtk_dp->edid = drm_edid_duplicate(new_edid); > + mutex_unlock(&mtk_dp->edid_lock); > + > + return new_edid; > +} > + > +static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux, > + struct drm_dp_aux_msg *msg) > +{ > + ssize_t err = -EAGAIN; > + struct mtk_dp *mtk_dp; > + bool is_read; > + u8 request; > + size_t accessed_bytes = 0; > + int retry = 3, 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; > + err = -EAGAIN; > + goto err; > + } > + > + switch (msg->request) { > + case DP_AUX_I2C_MOT: > + case DP_AUX_I2C_WRITE: > + case DP_AUX_NATIVE_WRITE: > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > + case DP_AUX_I2C_WRITE_STATUS_UPDATE | DP_AUX_I2C_MOT: > + request = msg->request & ~DP_AUX_I2C_WRITE_STATUS_UPDATE; > + is_read = false; > + break; > + case DP_AUX_I2C_READ: > + case DP_AUX_NATIVE_READ: > + case DP_AUX_I2C_READ | DP_AUX_I2C_MOT: > + request = msg->request; > + is_read = true; > + break; > + default: > + drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n", > + msg->request); > + err = -EINVAL; > + goto err; Directly return here. > + } > + > + if (msg->size == 0) { > + 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); > + retry = 3; > + while (retry--) { > + ret = mtk_dp_aux_do_transfer( > + mtk_dp, is_read, request, > + msg->address + accessed_bytes, > + msg->buffer + accessed_bytes, > + to_access); > + if (ret == 0) > + break; > + udelay(50); > + } > + if (!retry && ret) { > + drm_info(mtk_dp->drm_dev, > + "Failed to do AUX transfer: %d\n", > + ret); > + break; > + } > + accessed_bytes += to_access; > + } > + } > +err: > + if (!ret) { > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > + ret = msg->size; > + } else { > + msg->reply = DP_AUX_NATIVE_REPLY_NACK | DP_AUX_I2C_REPLY_NACK; > + return err; > + } > + > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > + return msg->size; > +} > + > +static void mtk_dp_aux_init(struct mtk_dp *mtk_dp) > +{ > + drm_dp_aux_init(&mtk_dp->aux); > + mtk_dp->aux.name = "aux_mtk_dp"; > + mtk_dp->aux.transfer = mtk_dp_aux_transfer; > +} > + > +static void mtk_dp_poweroff(struct mtk_dp *mtk_dp) > +{ > + mutex_lock(&mtk_dp->dp_lock); > + > + mtk_dp_hwirq_enable(mtk_dp, false); > + mtk_dp_power_disable(mtk_dp); > + phy_exit(mtk_dp->phy); > + clk_disable_unprepare(mtk_dp->dp_tx_clk); > + > + mutex_unlock(&mtk_dp->dp_lock); > +} > + > +static int mtk_dp_poweron(struct mtk_dp *mtk_dp) > +{ > + int ret = 0; > + > + mutex_lock(&mtk_dp->dp_lock); > + > + ret = clk_prepare_enable(mtk_dp->dp_tx_clk); > + if (ret < 0) { > + dev_err(mtk_dp->dev, "Fail to enable clock: %d\n", ret); > + goto err; > + } > + ret = phy_init(mtk_dp->phy); > + if (ret) { > + dev_err(mtk_dp->dev, "Failed to initialize phy: %d\n", ret); > + goto err_phy_init; > + } > + ret = mtk_dp_phy_configure(mtk_dp, MTK_DP_LINKRATE_RBR, 1); > + if (ret) { > + dev_err(mtk_dp->dev, "Failed to configure phy: %d\n", ret); > + goto err_phy_config; > + } > + > + mtk_dp_init_port(mtk_dp); > + mtk_dp_power_enable(mtk_dp); > + mtk_dp_hwirq_enable(mtk_dp, true); > + > +err_phy_config: > + phy_exit(mtk_dp->phy); > +err_phy_init: > + clk_disable_unprepare(mtk_dp->dp_tx_clk); > +err: > + mutex_unlock(&mtk_dp->dp_lock); > + return ret; > +} > + > +static int mtk_dp_bridge_attach(struct drm_bridge *bridge, > + enum drm_bridge_attach_flags flags) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + int ret; > + > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > + dev_err(mtk_dp->dev, "Driver does not provide a connector!"); > + return -EINVAL; > + } > + > + ret = mtk_dp_poweron(mtk_dp); Move the power on to mtk_dp_bridge_atomic_enable(). > + if (ret) > + return ret; > + > + if (mtk_dp->next_bridge) { > + ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge, > + &mtk_dp->bridge, flags); > + if (ret) { > + drm_warn(mtk_dp->drm_dev, > + "Failed to attach external bridge: %d\n", ret); > + goto err_bridge_attach; > + } > + } > + > + mtk_dp->drm_dev = bridge->dev; > + > + return 0; > + > +err_bridge_attach: > + mtk_dp_poweroff(mtk_dp); > + return ret; > +} > + > +static void mtk_dp_bridge_detach(struct drm_bridge *bridge) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + > + mtk_dp->drm_dev = NULL; > + > + mtk_dp_poweroff(mtk_dp); > +} > + > +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_state) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + > + mtk_dp_video_mute(mtk_dp, true); > + mtk_dp_audio_mute(mtk_dp, true); > + mtk_dp->state = MTK_DP_STATE_IDLE; > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > + > + mtk_dp->enabled = false; > + msleep(100); > +} > + > +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp, > + struct drm_display_mode *mode) > +{ > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > + > + drm_display_mode_to_videomode(mode, &timings->vm); > + timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal; > + timings->htotal = mode->htotal; > + timings->vtotal = mode->vtotal; > +} > + > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *old_state) > +{ > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > + struct drm_connector *conn; > + struct drm_connector_state *conn_state; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + int ret = 0; > + int i; > + > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > + bridge->encoder); mtk_dp->conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, bridge->encoder); > + if (!conn) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector is missing\n"); > + return; > + } > + > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > + > + conn_state = > + drm_atomic_get_new_connector_state(old_state->base.state, conn); > + if (!conn_state) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state is missing\n"); > + return; > + } > + > + crtc = conn_state->crtc; > + if (!crtc) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as connector state doesn't have a crtc\n"); > + return; > + } > + > + crtc_state = drm_atomic_get_new_crtc_state(old_state->base.state, crtc); > + if (!crtc_state) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as crtc state is missing\n"); > + return; > + } > + > + mtk_dp_parse_drm_mode_timings(mtk_dp, &crtc_state->adjusted_mode); Refer to mtk_hdmi_bridge_atomic_enable() for getting the mode. > + if (!mtk_dp_parse_capabilities(mtk_dp)) { > + drm_err(mtk_dp->drm_dev, > + "Can't enable bridge as nothing is plugged in\n"); > + return; > + } > + > + //training Run checkpatch first. Regards, Chun-Kuang. > + for (i = 0; i < 50; i++) { > + ret = mtk_dp_train_handler(mtk_dp); > + if (ret) { > + drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", > + ret); > + return; > + } > + > + ret = mtk_dp_state_handler(mtk_dp); > + if (ret) { > + drm_err(mtk_dp->drm_dev, "State handler failed %d\n", > + ret); > + return; > + } > + } > + > + mtk_dp->enabled = true; > + mtk_dp_update_plugged_status(mtk_dp); > +} > + > -- > 2.32.0 >
Hi, Quoting Maxime Ripard (2021-11-25 15:30:34) > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > > Hi, > > Thanks for all your input, really appreciated. > > > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > > Hi, > > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > > 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. > > > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > > you need a separate PHY driver in the first place? > > > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > > > It's a bit of a circular argument though :) > > > > > > It's a separate phy driver because it needs to go through another > > > maintainer's tree, but it needs to go through another maintainer's tree > > > because it's a separate phy driver. > > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > > the phy setup be done directly in the DP driver, if it's essentially a > > > single device? > > > > > > That being said, usually what those kind of questions mean is that > > > you're missing a comment or something in the commit log to provide that > > > context in the first place, so it would be great to add that context > > > here. > > > > > > And it will avoid the situation we're now in where multiple reviewers > > > ask the same questions over and over again :) > > > > > At first I didn't understand your reply, then I realized I gave you > > the wrong link... > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > > > The driver and phy were a single driver until v2 of this patch series > > and the phy setup > > was done directly in the driver (single driver, single C file). > > Here's the relevant link to the discussion between Chun-Kuang and Markus > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > > > I'll try to find a way to make it clearer for v7. > > OK, it makes sense then :) > > There's something weird though: the devices definitely look like they're > in a separate register range, yet you mention a regmap to handle the > shared register range. That range doesn't seem described anywhere in the > device tree though? What is it for? > My understanding is that 0x1000 to 0x1fff controls the phy functionalities and 0x2000 to 0x4fff controls "non-phy" functionalities. And you are right, there's no description of that in the device tree whatsoever. The ranges are in the same actual device and thus it has been decided to not have dt-bindings for the phy device. The phy driver is a child of the DP driver that we register using platform_device_register_data() and we pass along the same regmap as the DP driver in its platform data. I'm not sure I've understood and fully replied to the question... > > > > > > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > > > > > > + struct drm_bridge_state *old_state) > > > > > > +{ > > > > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > > > > + struct drm_connector *conn; > > > > > > + struct drm_connector_state *conn_state; > > > > > > + struct drm_crtc *crtc; > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > + int ret = 0; > > > > > > + int i; > > > > > > + > > > > > > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > > > > > + bridge->encoder); > > > > > > + if (!conn) { > > > > > > + drm_err(mtk_dp->drm_dev, > > > > > > + "Can't enable bridge as connector is missing\n"); > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > > > > > > > > > > This should be protected by a mutex (just like any resource shared > > > > > between KMS and ALSA) > > > > > > > > Ok. > > > > > > I forgot to ask (even though checkpatch does mention it iirc), but since > > > you have multiple mutex it would be nice to have a comment for each > > > mutex stating exactly what it protects, and against what. > > > > > > It's hard otherwise to remember or figure out what the locks are here > > > for. > > > > > > > > > + ret = mtk_dp_dt_parse_pdata(mtk_dp, pdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > > > > > pdata? > > > > > > > > > can you elaborate? > > > > > > Sorry, yeah, pdata is usually the abbreviation used in linux for the > > > platform_data mechanism, but you're using the DT to retrieve your > > > resources (and platform_data usually don't involve any parsing), so the > > > name is odd. > > > > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > index 384074f69111b..e6e88e3cd811d 100644 > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > @@ -63,6 +63,14 @@ enum mtk_dpi_out_color_format { > > > > > > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > > > > > > }; > > > > > > > > > > > > +enum TVDPLL_CLK { > > > > > > + TVDPLL_PLL = 0, > > > > > > + TVDPLL_D2 = 2, > > > > > > + TVDPLL_D4 = 4, > > > > > > + TVDPLL_D8 = 8, > > > > > > + TVDPLL_D16 = 16, > > > > > > +}; > > > > > > + > > > > > > struct mtk_dpi { > > > > > > struct drm_encoder encoder; > > > > > > struct drm_bridge bridge; > > > > > > @@ -71,8 +79,10 @@ 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; > > > > > > + struct clk *pclk_src[5]; > > > > > > int irq; > > > > > > struct drm_display_mode mode; > > > > > > const struct mtk_dpi_conf *conf; > > > > > > @@ -135,6 +145,7 @@ struct mtk_dpi_conf { > > > > > > u32 hvsize_mask; > > > > > > u32 channel_swap_shift; > > > > > > u32 yuv422_en_bit; > > > > > > + u32 csc_enable_bit; > > > > > > const struct mtk_dpi_yc_limit *limit; > > > > > > }; > > > > > > > > > > > > @@ -365,7 +376,7 @@ static void mtk_dpi_config_yuv422_enable(struct mtk_dpi *dpi, bool enable) > > > > > > > > > > > > static void mtk_dpi_config_csc_enable(struct mtk_dpi *dpi, bool enable) > > > > > > { > > > > > > - mtk_dpi_mask(dpi, DPI_CON, enable ? CSC_ENABLE : 0, CSC_ENABLE); > > > > > > + mtk_dpi_mask(dpi, DPI_CON, enable ? dpi->conf->csc_enable_bit : 0, dpi->conf->csc_enable_bit); > > > > > > } > > > > > > > > > > > > static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable) > > > > > > @@ -384,22 +395,45 @@ static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi) > > > > > > mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN); > > > > > > } > > > > > > > > > > > > +static void mtk_dpi_matrix_sel(struct mtk_dpi *dpi, enum mtk_dpi_out_color_format format) > > > > > > +{ > > > > > > + u32 matrix_sel = 0; > > > > > > + > > > > > > + switch (format) { > > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422: > > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL: > > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444: > > > > > > + case MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL: > > > > > > + case MTK_DPI_COLOR_FORMAT_XV_YCC: > > > > > > + if (dpi->mode.hdisplay <= 720) > > > > > > + matrix_sel = 0x2; > > > > > > + break; > > > > > > + default: > > > > > > + break; > > > > > > + } > > > > > > + mtk_dpi_mask(dpi, DPI_MATRIX_SET, matrix_sel, INT_MATRIX_SEL_MASK); > > > > > > +} > > > > > > + > > > > > > static void mtk_dpi_config_color_format(struct mtk_dpi *dpi, > > > > > > enum mtk_dpi_out_color_format format) > > > > > > { > > > > > > if ((format == MTK_DPI_COLOR_FORMAT_YCBCR_444) || > > > > > > (format == MTK_DPI_COLOR_FORMAT_YCBCR_444_FULL)) { > > > > > > mtk_dpi_config_yuv422_enable(dpi, false); > > > > > > - if (dpi->conf->csc_support) > > > > > > + if (dpi->conf->csc_support) { > > > > > > mtk_dpi_config_csc_enable(dpi, true); > > > > > > + mtk_dpi_matrix_sel(dpi, format); > > > > > > + } > > > > > > 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); > > > > > > - if (dpi->conf->csc_support) > > > > > > + if (dpi->conf->csc_support) { > > > > > > mtk_dpi_config_csc_enable(dpi, true); > > > > > > + mtk_dpi_matrix_sel(dpi, format); > > > > > > + } > > > > > > if (dpi->conf->swap_input_support) > > > > > > mtk_dpi_config_swap_input(dpi, true); > > > > > > mtk_dpi_config_channel_swap(dpi, MTK_DPI_OUT_CHANNEL_SWAP_RGB); > > > > > > @@ -441,6 +475,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) > > > > > > @@ -450,12 +486,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); > > > > > > @@ -465,10 +513,11 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) > > > > > > if (dpi->pinctrl && dpi->pins_dpi) > > > > > > pinctrl_select_state(dpi->pinctrl, dpi->pins_dpi); > > > > > > > > > > > > - mtk_dpi_enable(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--; > > > > > > @@ -500,9 +549,16 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > > > > pll_rate = clk_get_rate(dpi->tvd_clk); > > > > > > > > > > > > vm.pixelclock = pll_rate / factor; > > > > > > - if (dpi->conf->is_dpintf) > > > > > > - clk_set_rate(dpi->pixel_clk, vm.pixelclock / 4); > > > > > > - else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > > > > + if (dpi->conf->is_dpintf) { > > > > > > + if (factor == 1) > > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > > > > + else if (factor == 2) > > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[3]); > > > > > > + else if (factor == 4) > > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[4]); > > > > > > + else > > > > > > + clk_set_parent(dpi->pixel_clk, dpi->pclk_src[2]); > > > > > > + } else if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) || > > > > > > (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) > > > > > > clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2); > > > > > > else > > > > > > @@ -581,6 +637,8 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, > > > > > > } > > > > > > mtk_dpi_sw_reset(dpi, false); > > > > > > > > > > > > + mtk_dpi_enable(dpi); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -623,7 +681,6 @@ static u32 *mtk_dpi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > > > > > > u32 *input_fmts; > > > > > > > > > > > > *num_input_fmts = 0; > > > > > > - > > > > > > input_fmts = kcalloc(1, sizeof(*input_fmts), > > > > > > GFP_KERNEL); > > > > > > if (!input_fmts) > > > > > > @@ -649,7 +706,7 @@ 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", > > > > > > + 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); > > > > > > > > > > > > @@ -657,7 +714,10 @@ static int mtk_dpi_bridge_atomic_check(struct drm_bridge *bridge, > > > > > > 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; > > > > > > } > > > > > > @@ -835,6 +895,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, > > > > > > @@ -862,6 +928,7 @@ static const struct mtk_dpi_conf mt8173_conf = { > > > > > > .hvsize_mask = HSIZE_MASK, > > > > > > .channel_swap_shift = CH_SWAP, > > > > > > .yuv422_en_bit = YUV422_EN, > > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > > .limit = &mtk_dpi_limit, > > > > > > }; > > > > > > > > > > > > @@ -879,6 +946,7 @@ static const struct mtk_dpi_conf mt2701_conf = { > > > > > > .hvsize_mask = HSIZE_MASK, > > > > > > .channel_swap_shift = CH_SWAP, > > > > > > .yuv422_en_bit = YUV422_EN, > > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > > .limit = &mtk_dpi_limit, > > > > > > }; > > > > > > > > > > > > @@ -895,6 +963,7 @@ static const struct mtk_dpi_conf mt8183_conf = { > > > > > > .hvsize_mask = HSIZE_MASK, > > > > > > .channel_swap_shift = CH_SWAP, > > > > > > .yuv422_en_bit = YUV422_EN, > > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > > .limit = &mtk_dpi_limit, > > > > > > }; > > > > > > > > > > > > @@ -911,18 +980,21 @@ static const struct mtk_dpi_conf mt8192_conf = { > > > > > > .hvsize_mask = HSIZE_MASK, > > > > > > .channel_swap_shift = CH_SWAP, > > > > > > .yuv422_en_bit = YUV422_EN, > > > > > > + .csc_enable_bit = CSC_ENABLE, > > > > > > .limit = &mtk_dpi_limit, > > > > > > }; > > > > > > > > > > > > static const struct mtk_dpi_conf mt8195_dpintf_conf = { > > > > > > .cal_factor = mt8195_dpintf_calculate_factor, > > > > > > - .output_fmts = mt8173_output_fmts, > > > > > > - .num_output_fmts = ARRAY_SIZE(mt8173_output_fmts), > > > > > > + .output_fmts = mt8195_output_fmts, > > > > > > + .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts), > > > > > > .is_dpintf = true, > > > > > > + .csc_support = 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, > > > > > > }; > > > > > > > > > > > > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > > > > return ret; > > > > > > } > > > > > > > > > > > > + dpi->dpi_ck_cg = devm_clk_get(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; > > > > > > + } > > > > > > + > > > > > > dpi->pixel_clk = devm_clk_get(dev, "pixel"); > > > > > > if (IS_ERR(dpi->pixel_clk)) { > > > > > > ret = PTR_ERR(dpi->pixel_clk); > > > > > > @@ -997,6 +1079,11 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > > > > > return ret; > > > > > > } > > > > > > > > > > > > + dpi->pclk_src[1] = devm_clk_get(dev, "TVDPLL_D2"); > > > > > > + dpi->pclk_src[2] = devm_clk_get(dev, "TVDPLL_D4"); > > > > > > + dpi->pclk_src[3] = devm_clk_get(dev, "TVDPLL_D8"); > > > > > > + dpi->pclk_src[4] = devm_clk_get(dev, "TVDPLL_D16"); > > > > > > + > > > > > > dpi->irq = platform_get_irq(pdev, 0); > > > > > > if (dpi->irq <= 0) > > > > > > return -EINVAL; > > > > > > > > > > All those changes look unrelated as well? > > > > > > > > > Do you mean all the changes in mtk_dpi.c ? They are in support of > > > > enabling the mt8195 dpintf driver... so, not sure they are unlreated? > > > > > > I don't know that driver at all, so it's probably related if you say so > > > :) > > > > > > The DPI interface seems to be a panel interface, so it's a bit weird to > > > me why you would need to change things in the panel interface while > > > claiming to support a DP output. > > > > > > Especially since it looks like you have some support for CSC, and > > > support for additional output formats? > > > > > > I guess the DPI interface feeds the DP that essentially acts as a RGB -> > > > DP bridge? > > > > > > If so, then the part adding support for CSC and additional formats > > > should be split into a separate patch, and it should be mentioned in the > > > commit log > > > > > > Maxime > > > > I see what you mean, I'll split the patches functionally... > > I'm discovering the gpu/drm tree, so It may take me a while to get it right :D > > It's not really yet another weird drm rule :) > > Splitting your patches is valuable for a bunch of reasons that apply to > basically every tree: > > - It's easier to review (and maintainers like that). It's also easier > to justify in a commit log, so it leads to a better history over the > long term. > > - It's also easier to merge since you can merge smaller chunks when > they are ready, even if the entire set is not. Smaller chunks also > mean it's easier to apply (for the maintainer) and rebase (for you). > > - Finally, and possibly the strongest technical argument, it's much > more helpful when you're bisecting. Taking the above patch as an > example, let's assume you introduced a bug in some RGB panel with > the colors being off due to the CSC changes. If you run git bisect, > you'll end up with that big patch affecting multiple things and it > would still be difficult to figure out what's wrong exactly. If it > was split, you would end up only on the changeset that introduces > the regression, which is very likely to be much easier to analyze. > > Maxime Thx, Guillaume.
Hi Chun-Kuang. Quoting Chun-Kuang Hu (2021-11-25 16:27:45) > Hi, Guillaume: > > This is a big patch, so I give some comment first. > > Guillaume Ranquet <granquet@baylibre.com> 於 2021年11月10日 週三 下午9:06寫道： > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > > according phy driver mediatek-dp-phy. > > > > It supports both functional units on the mt8195, the embedded > > DisplayPort as well as the external DisplayPort units. It offers > > hot-plug-detection, audio up to 8 channels, and 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> > > Reported-by: kernel test robot <lkp@intel.com> > > --- > > drivers/gpu/drm/drm_edid.c | 2 +- > > Separate this to another patch. > > > drivers/gpu/drm/mediatek/Kconfig | 7 + > > drivers/gpu/drm/mediatek/Makefile | 2 + > > drivers/gpu/drm/mediatek/mtk_dp.c | 3094 +++++++++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 +++++ > > drivers/gpu/drm/mediatek/mtk_dpi.c | 111 +- > > Ditto. > > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 26 + > > Ditto. > > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > > Ditto > > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > > Ditto > yes my bad, I've made a bunch of fixup which ended up in the wrong place. It will be fixed for the next version. > > 9 files changed, 3799 insertions(+), 13 deletions(-) > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > index 500279a82167a..bfd98b50ceb5b 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -5183,7 +5183,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector, > > * modes and forbids YCRCB422 support for all video modes per > > * HDMI 1.3 spec. > > */ > > - info->color_formats = DRM_COLOR_FORMAT_RGB444; > > + info->color_formats |= DRM_COLOR_FORMAT_RGB444; > > > > /* YCRCB444 is optional according to spec. */ > > if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) { > > diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig > > index 2976d21e9a34a..029b94c716131 100644 > > --- a/drivers/gpu/drm/mediatek/Kconfig > > +++ b/drivers/gpu/drm/mediatek/Kconfig > > @@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI > > select PHY_MTK_HDMI > > help > > DRM/KMS HDMI driver for Mediatek SoCs > > + > > +config MTK_DPTX_SUPPORT > > + tristate "DRM DPTX Support for Mediatek SoCs" > > + depends on DRM_MEDIATEK > > + select PHY_MTK_DP > > + help > > + DRM/KMS Display Port driver for Mediatek SoCs. > > diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile > > index 29098d7c8307c..d86a6406055e6 100644 > > --- a/drivers/gpu/drm/mediatek/Makefile > > +++ b/drivers/gpu/drm/mediatek/Makefile > > @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \ > > mtk_hdmi_ddc.o > > > > obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o > > + > > +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > > new file mode 100644 > > index 0000000000000..83087219d5a5e > > --- /dev/null > > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > > @@ -0,0 +1,3094 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2019 MediaTek Inc. > > + * Copyright (c) 2021 BayLibre > > + */ > > + > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_bridge.h> > > +#include <drm/drm_crtc.h> > > +#include <drm/drm_dp_helper.h> > > +#include <drm/drm_edid.h> > > +#include <drm/drm_of.h> > > +#include <drm/drm_panel.h> > > +#include <drm/drm_print.h> > > +#include <drm/drm_probe_helper.h> > > +#include <linux/arm-smccc.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/errno.h> > > +#include <linux/kernel.h> > > +#include <linux/nvmem-consumer.h> > > +#include <linux/of.h> > > +#include <linux/of_irq.h> > > +#include <linux/of_platform.h> > > +#include <linux/phy/phy.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/regmap.h> > > +#include <sound/hdmi-codec.h> > > +#include <video/videomode.h> > > + > > +#include "mtk_dp_reg.h" > > + > > +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 > > +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3 > > + > > +#define MTK_DP_MAX_LANES 4 > > +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3 > > + > > +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 > > + > > +#define MTK_DP_TRAIN_RETRY_LIMIT 8 > > +#define MTK_DP_TRAIN_MAX_ITERATIONS 5 > > + > > +#define MTK_DP_AUX_WRITE_READ_WAIT_TIME_US 20 > > + > > +#define MTK_DP_DP_VERSION_11 0x11 > > + > > +enum mtk_dp_state { > > + MTK_DP_STATE_INITIAL, > > + MTK_DP_STATE_IDLE, > > + MTK_DP_STATE_PREPARE, > > + MTK_DP_STATE_NORMAL, > > +}; > > + > > +enum mtk_dp_train_state { > > + MTK_DP_TRAIN_STATE_STARTUP = 0, > > + MTK_DP_TRAIN_STATE_CHECKCAP, > > + MTK_DP_TRAIN_STATE_CHECKEDID, > > + MTK_DP_TRAIN_STATE_TRAINING_PRE, > > + MTK_DP_TRAIN_STATE_TRAINING, > > + MTK_DP_TRAIN_STATE_CHECKTIMING, > > + MTK_DP_TRAIN_STATE_NORMAL, > > + MTK_DP_TRAIN_STATE_POWERSAVE, > > + MTK_DP_TRAIN_STATE_DPIDLE, > > +}; > > + > > +struct mtk_dp_timings { > > + struct videomode vm; > > + > > + u16 htotal; > > + u16 vtotal; > > + u8 frame_rate; > > + u32 pix_rate_khz; > > +}; > > + > > +struct mtk_dp_train_info { > > + bool tps3; > > + bool tps4; > > + bool sink_ssc; > > + bool cable_plugged_in; > > + bool cable_state_change; > > + bool cr_done; > > + bool eq_done; > > + > > + // link_rate is in multiple of 0.27Gbps > > + int link_rate; > > + int lane_count; > > + > > + int irq_status; > > + int check_cap_count; > > +}; > > + > > +// Same values as used by the DP Spec for MISC0 bits 1 and 2 > > +enum mtk_dp_color_format { > > + MTK_DP_COLOR_FORMAT_RGB_444 = 0, > > + MTK_DP_COLOR_FORMAT_YUV_422 = 1, > > + MTK_DP_COLOR_FORMAT_YUV_444 = 2, > > + MTK_DP_COLOR_FORMAT_YUV_420 = 3, > > + MTK_DP_COLOR_FORMAT_YONLY = 4, > > + MTK_DP_COLOR_FORMAT_RAW = 5, > > + MTK_DP_COLOR_FORMAT_RESERVED = 6, > > + MTK_DP_COLOR_FORMAT_DEFAULT = MTK_DP_COLOR_FORMAT_RGB_444, > > + MTK_DP_COLOR_FORMAT_UNKNOWN = 15, > > +}; > > + > > +// Multiple of 0.27Gbps > > +enum mtk_dp_linkrate { > > + MTK_DP_LINKRATE_RBR = 0x6, > > + MTK_DP_LINKRATE_HBR = 0xA, > > + MTK_DP_LINKRATE_HBR2 = 0x14, > > + MTK_DP_LINKRATE_HBR25 = 0x19, > > + MTK_DP_LINKRATE_HBR3 = 0x1E, > > +}; > > + > > +// Same values as used for DP Spec MISC0 bits 5,6,7 > > +enum mtk_dp_color_depth { > > + MTK_DP_COLOR_DEPTH_6BIT = 0, > > + MTK_DP_COLOR_DEPTH_8BIT = 1, > > + MTK_DP_COLOR_DEPTH_10BIT = 2, > > + MTK_DP_COLOR_DEPTH_12BIT = 3, > > + MTK_DP_COLOR_DEPTH_16BIT = 4, > > + MTK_DP_COLOR_DEPTH_UNKNOWN = 5, > > +}; > > + > > +struct mtk_dp_audio_cfg { > > + int sample_rate; > > + int word_length_bits; > > + int channels; > > +}; > > + > > +struct mtk_dp_info { > > + enum mtk_dp_color_depth depth; > > + enum mtk_dp_color_format format; > > + struct mtk_dp_audio_cfg audio_caps; > > + struct mtk_dp_timings timings; > > +}; > > + > > +struct dp_cal_data { > > + unsigned int glb_bias_trim; > > + unsigned int clktx_impse; > > + > > + unsigned int ln0_tx_impsel_pmos; > > + unsigned int ln0_tx_impsel_nmos; > > + unsigned int ln1_tx_impsel_pmos; > > + unsigned int ln1_tx_impsel_nmos; > > + unsigned int ln2_tx_impsel_pmos; > > + unsigned int ln2_tx_impsel_nmos; > > + unsigned int ln3_tx_impsel_pmos; > > + unsigned int ln3_tx_impsel_nmos; > > +}; > > + > > +struct mtk_dp { > > + struct device *dev; > > + struct platform_device *phy_dev; > > + struct phy *phy; > > + struct dp_cal_data cal_data; > > + > > + struct drm_device *drm_dev; > > + struct drm_bridge bridge; > > + struct drm_bridge *next_bridge; > > + struct drm_dp_aux aux; > > + > > + struct mutex edid_lock; > > + struct edid *edid; > > + > > + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; > > + > > + struct mtk_dp_info info; > > + enum mtk_dp_state state; > > + > > + struct mtk_dp_train_info train_info; > > + enum mtk_dp_train_state train_state; > > + unsigned int input_fmt; > > + > > + struct regmap *regs; > > + struct clk *dp_tx_clk; > > + > > + bool enabled; > > + bool audio_enable; > > + > > + bool has_fec; > > + struct mutex dp_lock; > > + > > + struct mutex update_plugged_status_lock; > > + > > + hdmi_codec_plugged_cb plugged_cb; > > + struct device *codec_dev; > > + u8 connector_eld[MAX_ELD_BYTES]; > > +}; > > + > > +enum mtk_dp_sdp_type { > > + MTK_DP_SDP_NONE = 0x00, > > + MTK_DP_SDP_ACM = 0x01, > > + MTK_DP_SDP_ISRC = 0x02, > > + MTK_DP_SDP_AVI = 0x03, > > + MTK_DP_SDP_AUI = 0x04, > > + MTK_DP_SDP_SPD = 0x05, > > + MTK_DP_SDP_MPEG = 0x06, > > + MTK_DP_SDP_NTSC = 0x07, > > + MTK_DP_SDP_VSP = 0x08, > > + MTK_DP_SDP_VSC = 0x09, > > + MTK_DP_SDP_EXT = 0x0A, > > + MTK_DP_SDP_PPS0 = 0x0B, > > + MTK_DP_SDP_PPS1 = 0x0C, > > + MTK_DP_SDP_PPS2 = 0x0D, > > + MTK_DP_SDP_PPS3 = 0x0E, > > + MTK_DP_SDP_DRM = 0x10, > > + MTK_DP_SDP_MAX_NUM > > +}; > > + > > +struct mtk_dp_sdp_packet { > > + enum mtk_dp_sdp_type type; > > + struct dp_sdp sdp; > > +}; > > + > > +#define MTK_DP_IEC_CHANNEL_STATUS_LEN 5 > > +union mtk_dp_audio_channel_status { > > + struct { > > + u8 rev : 1; > > + u8 is_lpcm : 1; > > + u8 copy_right : 1; > > + u8 additional_format_info : 3; > > + u8 channel_status_mode : 2; > > + u8 category_code; > > + u8 src_num : 4; > > + u8 channel_num : 4; > > + u8 sampling_freq : 4; > > + u8 clk_accuracy : 2; > > + u8 rev2 : 2; > > + u8 word_len : 4; > > + u8 original_sampling_freq : 4; > > + } iec; > > + > > + u8 buf[MTK_DP_IEC_CHANNEL_STATUS_LEN]; > > +}; > > + > > +static struct regmap_config mtk_dp_regmap_config = { > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > + .max_register = SEC_OFFSET + 0x90, > > + .name = "mtk-dp-registers", > > +}; > > + > > +static bool mtk_dp_is_edp(struct mtk_dp *mtk_dp) > > Separate edp and displayport into two patches. For example, the first > patch support edp only, and the second patch add displayport function. > I have made the opposite as it seemed a bit more logical from how it's coded now. I've made a single patch with DP and a new one adding eDP support on top of it. The change is quite small, adding the eDP on top of DP: drivers/gpu/drm/mediatek/mtk_dp.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 74 insertions(+), 29 deletions(-) > > +{ > > + return mtk_dp->next_bridge != NULL; > > +} > > + > > [snip] > > > + > > +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge, > > + struct drm_connector *connector) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + bool enabled = mtk_dp->enabled; > > + struct edid *new_edid = NULL; > > + > > + if (!enabled) > > + drm_bridge_chain_pre_enable(bridge); > > In mtk_hdmi_bridge_get_edid(), there does not check the power. Why in > this function need this? > Does mtk hdmi driver has a bug? > I don't know, but I will be asking around. > > + > > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0); > > + usleep_range(2000, 5000); > > + > > + if (mtk_dp_plug_state(mtk_dp)) > > + new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc); > > + > > + if (!enabled) > > + drm_bridge_chain_post_disable(bridge); > > + > > + mutex_lock(&mtk_dp->edid_lock); > > + kfree(mtk_dp->edid); > > + mtk_dp->edid = NULL; > > + > > + if (!new_edid) { > > + mutex_unlock(&mtk_dp->edid_lock); > > + return NULL; > > + } > > + > > + mtk_dp->edid = drm_edid_duplicate(new_edid); > > + mutex_unlock(&mtk_dp->edid_lock); > > + > > + return new_edid; > > +} > > + > > +static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux, > > + struct drm_dp_aux_msg *msg) > > +{ > > + ssize_t err = -EAGAIN; > > + struct mtk_dp *mtk_dp; > > + bool is_read; > > + u8 request; > > + size_t accessed_bytes = 0; > > + int retry = 3, 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; > > + err = -EAGAIN; > > + goto err; > > + } > > + > > + switch (msg->request) { > > + case DP_AUX_I2C_MOT: > > + case DP_AUX_I2C_WRITE: > > + case DP_AUX_NATIVE_WRITE: > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE | DP_AUX_I2C_MOT: > > + request = msg->request & ~DP_AUX_I2C_WRITE_STATUS_UPDATE; > > + is_read = false; > > + break; > > + case DP_AUX_I2C_READ: > > + case DP_AUX_NATIVE_READ: > > + case DP_AUX_I2C_READ | DP_AUX_I2C_MOT: > > + request = msg->request; > > + is_read = true; > > + break; > > + default: > > + drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n", > > + msg->request); > > + err = -EINVAL; > > + goto err; > > Directly return here. > Hmm, not sure yet, it seems I still need to prime msg->reply anyway. Though the error handling can be simplified a bit. > > + } > > + > > + if (msg->size == 0) { > > + 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); > > + retry = 3; > > + while (retry--) { > > + ret = mtk_dp_aux_do_transfer( > > + mtk_dp, is_read, request, > > + msg->address + accessed_bytes, > > + msg->buffer + accessed_bytes, > > + to_access); > > + if (ret == 0) > > + break; > > + udelay(50); > > + } > > + if (!retry && ret) { > > + drm_info(mtk_dp->drm_dev, > > + "Failed to do AUX transfer: %d\n", > > + ret); > > + break; > > + } > > + accessed_bytes += to_access; > > + } > > + } > > +err: > > + if (!ret) { > > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > > + ret = msg->size; > > + } else { > > + msg->reply = DP_AUX_NATIVE_REPLY_NACK | DP_AUX_I2C_REPLY_NACK; > > + return err; > > + } > > + > > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > > + return msg->size; > > +} > > + > > +static void mtk_dp_aux_init(struct mtk_dp *mtk_dp) > > +{ > > + drm_dp_aux_init(&mtk_dp->aux); > > + mtk_dp->aux.name = "aux_mtk_dp"; > > + mtk_dp->aux.transfer = mtk_dp_aux_transfer; > > +} > > + > > +static void mtk_dp_poweroff(struct mtk_dp *mtk_dp) > > +{ > > + mutex_lock(&mtk_dp->dp_lock); > > + > > + mtk_dp_hwirq_enable(mtk_dp, false); > > + mtk_dp_power_disable(mtk_dp); > > + phy_exit(mtk_dp->phy); > > + clk_disable_unprepare(mtk_dp->dp_tx_clk); > > + > > + mutex_unlock(&mtk_dp->dp_lock); > > +} > > + > > +static int mtk_dp_poweron(struct mtk_dp *mtk_dp) > > +{ > > + int ret = 0; > > + > > + mutex_lock(&mtk_dp->dp_lock); > > + > > + ret = clk_prepare_enable(mtk_dp->dp_tx_clk); > > + if (ret < 0) { > > + dev_err(mtk_dp->dev, "Fail to enable clock: %d\n", ret); > > + goto err; > > + } > > + ret = phy_init(mtk_dp->phy); > > + if (ret) { > > + dev_err(mtk_dp->dev, "Failed to initialize phy: %d\n", ret); > > + goto err_phy_init; > > + } > > + ret = mtk_dp_phy_configure(mtk_dp, MTK_DP_LINKRATE_RBR, 1); > > + if (ret) { > > + dev_err(mtk_dp->dev, "Failed to configure phy: %d\n", ret); > > + goto err_phy_config; > > + } > > + > > + mtk_dp_init_port(mtk_dp); > > + mtk_dp_power_enable(mtk_dp); > > + mtk_dp_hwirq_enable(mtk_dp, true); > > + > > +err_phy_config: > > + phy_exit(mtk_dp->phy); > > +err_phy_init: > > + clk_disable_unprepare(mtk_dp->dp_tx_clk); > > +err: > > + mutex_unlock(&mtk_dp->dp_lock); > > + return ret; > > +} > > + > > +static int mtk_dp_bridge_attach(struct drm_bridge *bridge, > > + enum drm_bridge_attach_flags flags) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + int ret; > > + > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > + dev_err(mtk_dp->dev, "Driver does not provide a connector!"); > > + return -EINVAL; > > + } > > + > > + ret = mtk_dp_poweron(mtk_dp); > > Move the power on to mtk_dp_bridge_atomic_enable(). > > > + if (ret) > > + return ret; > > + > > + if (mtk_dp->next_bridge) { > > + ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge, > > + &mtk_dp->bridge, flags); > > + if (ret) { > > + drm_warn(mtk_dp->drm_dev, > > + "Failed to attach external bridge: %d\n", ret); > > + goto err_bridge_attach; > > + } > > + } > > + > > + mtk_dp->drm_dev = bridge->dev; > > + > > + return 0; > > + > > +err_bridge_attach: > > + mtk_dp_poweroff(mtk_dp); > > + return ret; > > +} > > + > > +static void mtk_dp_bridge_detach(struct drm_bridge *bridge) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + > > + mtk_dp->drm_dev = NULL; > > + > > + mtk_dp_poweroff(mtk_dp); > > +} > > + > > +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge, > > + struct drm_bridge_state *old_state) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + > > + mtk_dp_video_mute(mtk_dp, true); > > + mtk_dp_audio_mute(mtk_dp, true); > > + mtk_dp->state = MTK_DP_STATE_IDLE; > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > + > > + mtk_dp->enabled = false; > > + msleep(100); > > +} > > + > > +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp, > > + struct drm_display_mode *mode) > > +{ > > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > > + > > + drm_display_mode_to_videomode(mode, &timings->vm); > > + timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal; > > + timings->htotal = mode->htotal; > > + timings->vtotal = mode->vtotal; > > +} > > + > > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > > + struct drm_bridge_state *old_state) > > +{ > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > + struct drm_connector *conn; > > + struct drm_connector_state *conn_state; > > + struct drm_crtc *crtc; > > + struct drm_crtc_state *crtc_state; > > + int ret = 0; > > + int i; > > + > > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > + bridge->encoder); > > mtk_dp->conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > bridge->encoder); > The connector doesn't seem to be used anywhere else in the driver. Though I can add a drm_connector in the mtk_dp struct. > > + if (!conn) { > > + drm_err(mtk_dp->drm_dev, > > + "Can't enable bridge as connector is missing\n"); > > + return; > > + } > > + > > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > > + > > + conn_state = > > + drm_atomic_get_new_connector_state(old_state->base.state, conn); > > + if (!conn_state) { > > + drm_err(mtk_dp->drm_dev, > > + "Can't enable bridge as connector state is missing\n"); > > + return; > > + } > > + > > + crtc = conn_state->crtc; > > + if (!crtc) { > > + drm_err(mtk_dp->drm_dev, > > + "Can't enable bridge as connector state doesn't have a crtc\n"); > > + return; > > + } > > + > > + crtc_state = drm_atomic_get_new_crtc_state(old_state->base.state, crtc); > > + if (!crtc_state) { > > + drm_err(mtk_dp->drm_dev, > > + "Can't enable bridge as crtc state is missing\n"); > > + return; > > + } > > + > > + mtk_dp_parse_drm_mode_timings(mtk_dp, &crtc_state->adjusted_mode); > > Refer to mtk_hdmi_bridge_atomic_enable() for getting the mode. > I'm sorry, I don't understand what I'm supposed to do here. Could you elaborate? > > + if (!mtk_dp_parse_capabilities(mtk_dp)) { > > + drm_err(mtk_dp->drm_dev, > > + "Can't enable bridge as nothing is plugged in\n"); > > + return; > > + } > > + > > + //training > > Run checkpatch first. > > Regards, > Chun-Kuang. > > > + for (i = 0; i < 50; i++) { > > + ret = mtk_dp_train_handler(mtk_dp); > > + if (ret) { > > + drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", > > + ret); > > + return; > > + } > > + > > + ret = mtk_dp_state_handler(mtk_dp); > > + if (ret) { > > + drm_err(mtk_dp->drm_dev, "State handler failed %d\n", > > + ret); > > + return; > > + } > > + } > > + > > + mtk_dp->enabled = true; > > + mtk_dp_update_plugged_status(mtk_dp); > > +} > > + > > > > > -- > > 2.32.0 > >
On Wed, Nov 10, 2021 at 9:08 PM Guillaume Ranquet <granquet@baylibre.com> wrote: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > according phy driver mediatek-dp-phy. > > It supports both functional units on the mt8195, the embedded > DisplayPort as well as the external DisplayPort units. It offers > hot-plug-detection, audio up to 8 channels, and 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> > Reported-by: kernel test robot <lkp@intel.com> > --- <snip> > +static int mtk_dp_probe(struct platform_device *pdev) > +{ > + struct mtk_dp *mtk_dp; > + struct device *dev = &pdev->dev; > + int ret; > + int irq_num = 0; > + struct drm_panel *panel = NULL; > + > + mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL); > + if (!mtk_dp) > + return -ENOMEM; > + > + mtk_dp->dev = dev; > + > + irq_num = platform_get_irq(pdev, 0); > + if (irq_num < 0) { > + dev_err(dev, "failed to request dp irq resource\n"); > + return -EPROBE_DEFER; > + } > + > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, > + &mtk_dp->next_bridge); > + if (ret == -ENODEV) { > + dev_info( > + dev, > + "No panel connected in devicetree, continuing as external DP\n"); > + mtk_dp->next_bridge = NULL; > + } else if (ret) { > + dev_err(dev, "Failed to find panel or bridge: %d\n", ret); Hi, We're seeing [ 0.424599] mediatek-drm-dp 1c500000.edp_tx: Failed to find panel or bridge: -517 It's probably better to use dev_err_probe here. Thanks > + return ret; > + } > + > + if (panel) { > + mtk_dp->next_bridge = devm_drm_panel_bridge_add(dev, panel); > + if (IS_ERR(mtk_dp->next_bridge)) { > + ret = PTR_ERR(mtk_dp->next_bridge); > + dev_err(dev, "Failed to create bridge: %d\n", ret); > + return -EPROBE_DEFER; > + } > + } <snip> > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Il 10/11/21 14:06, Guillaume Ranquet ha scritto: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > according phy driver mediatek-dp-phy. > > It supports both functional units on the mt8195, the embedded > DisplayPort as well as the external DisplayPort units. It offers > hot-plug-detection, audio up to 8 channels, and 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> > Reported-by: kernel test robot <lkp@intel.com> Hello Markus, Guillaume, there is a critical issue with this patch. Please check below. > --- > drivers/gpu/drm/drm_edid.c | 2 +- > drivers/gpu/drm/mediatek/Kconfig | 7 + > drivers/gpu/drm/mediatek/Makefile | 2 + > drivers/gpu/drm/mediatek/mtk_dp.c | 3094 +++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 +++++ > drivers/gpu/drm/mediatek/mtk_dpi.c | 111 +- > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 26 + > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > 9 files changed, 3799 insertions(+), 13 deletions(-) > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > <snip> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 384074f69111b..e6e88e3cd811d 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c <snip> > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev) > return ret; > } > > + dpi->dpi_ck_cg = devm_clk_get(dev, "ck_cg"); mtk_dpi is used on MT2701, MT7183, MT8183, MT8192, but these platforms haven't got any "ck_cg" clock defined in their device-trees (regardless of whether these can support adding this clock or not, any code change shall be retro-compatible hence not breaking compatibility/functionality with older device-trees). Reminding that: - mediatek-drm uses the component framework - mtk_drm_drv is the component master - mtk_drm_drv bind() won't be called unless all of the components added with match aren't calling component_add() ... this change not only breaks DisplayPort support for *all* of the aforementioned SoCs, but also makes the entire mediatek-drm to not finish probing, producing a global breakage that also includes DSI and the entire stack of components of that master (so, no display on all of them). To avoid breaking any SoC that's not MT8195, please use devm_clk_get_optional() here in the next version. Thanks, - Angelo
On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote: > Hi, > > Quoting Maxime Ripard (2021-11-25 15:30:34) > > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > > > Hi, > > > Thanks for all your input, really appreciated. > > > > > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > > > Hi, > > > > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > > > 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. > > > > > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > > > you need a separate PHY driver in the first place? > > > > > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > > > > > It's a bit of a circular argument though :) > > > > > > > > It's a separate phy driver because it needs to go through another > > > > maintainer's tree, but it needs to go through another maintainer's tree > > > > because it's a separate phy driver. > > > > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > > > the phy setup be done directly in the DP driver, if it's essentially a > > > > single device? > > > > > > > > That being said, usually what those kind of questions mean is that > > > > you're missing a comment or something in the commit log to provide that > > > > context in the first place, so it would be great to add that context > > > > here. > > > > > > > > And it will avoid the situation we're now in where multiple reviewers > > > > ask the same questions over and over again :) > > > > > > > At first I didn't understand your reply, then I realized I gave you > > > the wrong link... > > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > > > > > The driver and phy were a single driver until v2 of this patch series > > > and the phy setup > > > was done directly in the driver (single driver, single C file). > > > Here's the relevant link to the discussion between Chun-Kuang and Markus > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > > > > > I'll try to find a way to make it clearer for v7. > > > > OK, it makes sense then :) > > > > There's something weird though: the devices definitely look like they're > > in a separate register range, yet you mention a regmap to handle the > > shared register range. That range doesn't seem described anywhere in the > > device tree though? What is it for? > > My understanding is that 0x1000 to 0x1fff controls the phy > functionalities and 0x2000 to 0x4fff controls "non-phy" > functionalities. And you are right, there's no description of that in > the device tree whatsoever. The ranges are in the same actual device > and thus it has been decided to not have dt-bindings for the phy > device. Sure, that last part makes sense, but then I'm not sure why you don't have the full register range in the device node you have in the DT? > The phy driver is a child of the DP driver that we register using > platform_device_register_data() and we pass along the same regmap as > the DP driver in its platform data. Especially if it's used by something, it should be described in the DT somewhere. Maxime
Hi Angelo, Quoting AngeloGioacchino Del Regno (2021-12-10 11:17:44) > Il 10/11/21 14:06, Guillaume Ranquet ha scritto: > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > > according phy driver mediatek-dp-phy. > > > > It supports both functional units on the mt8195, the embedded > > DisplayPort as well as the external DisplayPort units. It offers > > hot-plug-detection, audio up to 8 channels, and 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> > > Reported-by: kernel test robot <lkp@intel.com> > > Hello Markus, Guillaume, > > there is a critical issue with this patch. Please check below. > > > --- > > drivers/gpu/drm/drm_edid.c | 2 +- > > drivers/gpu/drm/mediatek/Kconfig | 7 + > > drivers/gpu/drm/mediatek/Makefile | 2 + > > drivers/gpu/drm/mediatek/mtk_dp.c | 3094 +++++++++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 +++++ > > drivers/gpu/drm/mediatek/mtk_dpi.c | 111 +- > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 26 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > > 9 files changed, 3799 insertions(+), 13 deletions(-) > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > > > <snip> > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > > index 384074f69111b..e6e88e3cd811d 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > <snip> > > > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev) > > return ret; > > } > > > > + dpi->dpi_ck_cg = devm_clk_get(dev, "ck_cg"); > > mtk_dpi is used on MT2701, MT7183, MT8183, MT8192, but these platforms haven't > got any "ck_cg" clock defined in their device-trees (regardless of whether these > can support adding this clock or not, any code change shall be retro-compatible > hence not breaking compatibility/functionality with older device-trees). > > Reminding that: > - mediatek-drm uses the component framework > - mtk_drm_drv is the component master > - mtk_drm_drv bind() won't be called unless all of the components added with > match aren't calling component_add() > > ... this change not only breaks DisplayPort support for *all* of the > aforementioned SoCs, but also makes the entire mediatek-drm to not finish > probing, producing a global breakage that also includes DSI and the entire > stack of components of that master (so, no display on all of them). > > To avoid breaking any SoC that's not MT8195, please use devm_clk_get_optional() > here in the next version. > > Thanks, > - Angelo > This is a good catch, I will update for v7. Thank you very much for your review. Thx, Guillaume.
Hi Hsin-Yi, Quoting Hsin-Yi Wang (2021-12-09 07:29:30) > On Wed, Nov 10, 2021 at 9:08 PM Guillaume Ranquet <granquet@baylibre.com> wrote: > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > > according phy driver mediatek-dp-phy. > > > > It supports both functional units on the mt8195, the embedded > > DisplayPort as well as the external DisplayPort units. It offers > > hot-plug-detection, audio up to 8 channels, and 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> > > Reported-by: kernel test robot <lkp@intel.com> > > --- > > > <snip> > > > +static int mtk_dp_probe(struct platform_device *pdev) > > +{ > > + struct mtk_dp *mtk_dp; > > + struct device *dev = &pdev->dev; > > + int ret; > > + int irq_num = 0; > > + struct drm_panel *panel = NULL; > > + > > + mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL); > > + if (!mtk_dp) > > + return -ENOMEM; > > + > > + mtk_dp->dev = dev; > > + > > + irq_num = platform_get_irq(pdev, 0); > > + if (irq_num < 0) { > > + dev_err(dev, "failed to request dp irq resource\n"); > > + return -EPROBE_DEFER; > > + } > > + > > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, > > + &mtk_dp->next_bridge); > > + if (ret == -ENODEV) { > > + dev_info( > > + dev, > > + "No panel connected in devicetree, continuing as external DP\n"); > > + mtk_dp->next_bridge = NULL; > > + } else if (ret) { > > + dev_err(dev, "Failed to find panel or bridge: %d\n", ret); > > Hi, > > We're seeing > [ 0.424599] mediatek-drm-dp 1c500000.edp_tx: Failed to find panel > or bridge: -517 > > It's probably better to use dev_err_probe here. > > Thanks Good suggestion, it will be in v7. Thanks for your review, Guillaume. > > > + return ret; > > + } > > + > > + if (panel) { > > + mtk_dp->next_bridge = devm_drm_panel_bridge_add(dev, panel); > > + if (IS_ERR(mtk_dp->next_bridge)) { > > + ret = PTR_ERR(mtk_dp->next_bridge); > > + dev_err(dev, "Failed to create bridge: %d\n", ret); > > + return -EPROBE_DEFER; > > + } > > + } > > > <snip> > > > _______________________________________________ > > Linux-mediatek mailing list > > Linux-mediatek@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi Chun-Kuang. Quoting Guillaume Ranquet (2021-12-02 16:31:03) > Hi Chun-Kuang. > > Quoting Chun-Kuang Hu (2021-11-25 16:27:45) > > Hi, Guillaume: > > > > This is a big patch, so I give some comment first. > > > > Guillaume Ranquet <granquet@baylibre.com> 於 2021年11月10日 週三 下午9:06寫道： > > > > > > From: Markus Schneider-Pargmann <msp@baylibre.com> > > > > > > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a > > > according phy driver mediatek-dp-phy. > > > > > > It supports both functional units on the mt8195, the embedded > > > DisplayPort as well as the external DisplayPort units. It offers > > > hot-plug-detection, audio up to 8 channels, and 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> > > > Reported-by: kernel test robot <lkp@intel.com> > > > --- > > > drivers/gpu/drm/drm_edid.c | 2 +- > > > > Separate this to another patch. > > > > > drivers/gpu/drm/mediatek/Kconfig | 7 + > > > drivers/gpu/drm/mediatek/Makefile | 2 + > > > drivers/gpu/drm/mediatek/mtk_dp.c | 3094 +++++++++++++++++++++++ > > > drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 +++++ > > > drivers/gpu/drm/mediatek/mtk_dpi.c | 111 +- > > > > Ditto. > > > > > drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 26 + > > > > Ditto. > > > > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > > > > Ditto > > > > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + > > > > Ditto > > > yes my bad, I've made a bunch of fixup which ended up in the wrong place. > It will be fixed for the next version. > > > > 9 files changed, 3799 insertions(+), 13 deletions(-) > > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c > > > create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > index 500279a82167a..bfd98b50ceb5b 100644 > > > --- a/drivers/gpu/drm/drm_edid.c > > > +++ b/drivers/gpu/drm/drm_edid.c > > > @@ -5183,7 +5183,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector, > > > * modes and forbids YCRCB422 support for all video modes per > > > * HDMI 1.3 spec. > > > */ > > > - info->color_formats = DRM_COLOR_FORMAT_RGB444; > > > + info->color_formats |= DRM_COLOR_FORMAT_RGB444; > > > > > > /* YCRCB444 is optional according to spec. */ > > > if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) { > > > diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig > > > index 2976d21e9a34a..029b94c716131 100644 > > > --- a/drivers/gpu/drm/mediatek/Kconfig > > > +++ b/drivers/gpu/drm/mediatek/Kconfig > > > @@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI > > > select PHY_MTK_HDMI > > > help > > > DRM/KMS HDMI driver for Mediatek SoCs > > > + > > > +config MTK_DPTX_SUPPORT > > > + tristate "DRM DPTX Support for Mediatek SoCs" > > > + depends on DRM_MEDIATEK > > > + select PHY_MTK_DP > > > + help > > > + DRM/KMS Display Port driver for Mediatek SoCs. > > > diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile > > > index 29098d7c8307c..d86a6406055e6 100644 > > > --- a/drivers/gpu/drm/mediatek/Makefile > > > +++ b/drivers/gpu/drm/mediatek/Makefile > > > @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \ > > > mtk_hdmi_ddc.o > > > > > > obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o > > > + > > > +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c > > > new file mode 100644 > > > index 0000000000000..83087219d5a5e > > > --- /dev/null > > > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > > > @@ -0,0 +1,3094 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (c) 2019 MediaTek Inc. > > > + * Copyright (c) 2021 BayLibre > > > + */ > > > + > > > +#include <drm/drm_atomic_helper.h> > > > +#include <drm/drm_bridge.h> > > > +#include <drm/drm_crtc.h> > > > +#include <drm/drm_dp_helper.h> > > > +#include <drm/drm_edid.h> > > > +#include <drm/drm_of.h> > > > +#include <drm/drm_panel.h> > > > +#include <drm/drm_print.h> > > > +#include <drm/drm_probe_helper.h> > > > +#include <linux/arm-smccc.h> > > > +#include <linux/clk.h> > > > +#include <linux/delay.h> > > > +#include <linux/errno.h> > > > +#include <linux/kernel.h> > > > +#include <linux/nvmem-consumer.h> > > > +#include <linux/of.h> > > > +#include <linux/of_irq.h> > > > +#include <linux/of_platform.h> > > > +#include <linux/phy/phy.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/pm_runtime.h> > > > +#include <linux/regmap.h> > > > +#include <sound/hdmi-codec.h> > > > +#include <video/videomode.h> > > > + > > > +#include "mtk_dp_reg.h" > > > + > > > +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 > > > +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3 > > > + > > > +#define MTK_DP_MAX_LANES 4 > > > +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3 > > > + > > > +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08 > > > + > > > +#define MTK_DP_TRAIN_RETRY_LIMIT 8 > > > +#define MTK_DP_TRAIN_MAX_ITERATIONS 5 > > > + > > > +#define MTK_DP_AUX_WRITE_READ_WAIT_TIME_US 20 > > > + > > > +#define MTK_DP_DP_VERSION_11 0x11 > > > + > > > +enum mtk_dp_state { > > > + MTK_DP_STATE_INITIAL, > > > + MTK_DP_STATE_IDLE, > > > + MTK_DP_STATE_PREPARE, > > > + MTK_DP_STATE_NORMAL, > > > +}; > > > + > > > +enum mtk_dp_train_state { > > > + MTK_DP_TRAIN_STATE_STARTUP = 0, > > > + MTK_DP_TRAIN_STATE_CHECKCAP, > > > + MTK_DP_TRAIN_STATE_CHECKEDID, > > > + MTK_DP_TRAIN_STATE_TRAINING_PRE, > > > + MTK_DP_TRAIN_STATE_TRAINING, > > > + MTK_DP_TRAIN_STATE_CHECKTIMING, > > > + MTK_DP_TRAIN_STATE_NORMAL, > > > + MTK_DP_TRAIN_STATE_POWERSAVE, > > > + MTK_DP_TRAIN_STATE_DPIDLE, > > > +}; > > > + > > > +struct mtk_dp_timings { > > > + struct videomode vm; > > > + > > > + u16 htotal; > > > + u16 vtotal; > > > + u8 frame_rate; > > > + u32 pix_rate_khz; > > > +}; > > > + > > > +struct mtk_dp_train_info { > > > + bool tps3; > > > + bool tps4; > > > + bool sink_ssc; > > > + bool cable_plugged_in; > > > + bool cable_state_change; > > > + bool cr_done; > > > + bool eq_done; > > > + > > > + // link_rate is in multiple of 0.27Gbps > > > + int link_rate; > > > + int lane_count; > > > + > > > + int irq_status; > > > + int check_cap_count; > > > +}; > > > + > > > +// Same values as used by the DP Spec for MISC0 bits 1 and 2 > > > +enum mtk_dp_color_format { > > > + MTK_DP_COLOR_FORMAT_RGB_444 = 0, > > > + MTK_DP_COLOR_FORMAT_YUV_422 = 1, > > > + MTK_DP_COLOR_FORMAT_YUV_444 = 2, > > > + MTK_DP_COLOR_FORMAT_YUV_420 = 3, > > > + MTK_DP_COLOR_FORMAT_YONLY = 4, > > > + MTK_DP_COLOR_FORMAT_RAW = 5, > > > + MTK_DP_COLOR_FORMAT_RESERVED = 6, > > > + MTK_DP_COLOR_FORMAT_DEFAULT = MTK_DP_COLOR_FORMAT_RGB_444, > > > + MTK_DP_COLOR_FORMAT_UNKNOWN = 15, > > > +}; > > > + > > > +// Multiple of 0.27Gbps > > > +enum mtk_dp_linkrate { > > > + MTK_DP_LINKRATE_RBR = 0x6, > > > + MTK_DP_LINKRATE_HBR = 0xA, > > > + MTK_DP_LINKRATE_HBR2 = 0x14, > > > + MTK_DP_LINKRATE_HBR25 = 0x19, > > > + MTK_DP_LINKRATE_HBR3 = 0x1E, > > > +}; > > > + > > > +// Same values as used for DP Spec MISC0 bits 5,6,7 > > > +enum mtk_dp_color_depth { > > > + MTK_DP_COLOR_DEPTH_6BIT = 0, > > > + MTK_DP_COLOR_DEPTH_8BIT = 1, > > > + MTK_DP_COLOR_DEPTH_10BIT = 2, > > > + MTK_DP_COLOR_DEPTH_12BIT = 3, > > > + MTK_DP_COLOR_DEPTH_16BIT = 4, > > > + MTK_DP_COLOR_DEPTH_UNKNOWN = 5, > > > +}; > > > + > > > +struct mtk_dp_audio_cfg { > > > + int sample_rate; > > > + int word_length_bits; > > > + int channels; > > > +}; > > > + > > > +struct mtk_dp_info { > > > + enum mtk_dp_color_depth depth; > > > + enum mtk_dp_color_format format; > > > + struct mtk_dp_audio_cfg audio_caps; > > > + struct mtk_dp_timings timings; > > > +}; > > > + > > > +struct dp_cal_data { > > > + unsigned int glb_bias_trim; > > > + unsigned int clktx_impse; > > > + > > > + unsigned int ln0_tx_impsel_pmos; > > > + unsigned int ln0_tx_impsel_nmos; > > > + unsigned int ln1_tx_impsel_pmos; > > > + unsigned int ln1_tx_impsel_nmos; > > > + unsigned int ln2_tx_impsel_pmos; > > > + unsigned int ln2_tx_impsel_nmos; > > > + unsigned int ln3_tx_impsel_pmos; > > > + unsigned int ln3_tx_impsel_nmos; > > > +}; > > > + > > > +struct mtk_dp { > > > + struct device *dev; > > > + struct platform_device *phy_dev; > > > + struct phy *phy; > > > + struct dp_cal_data cal_data; > > > + > > > + struct drm_device *drm_dev; > > > + struct drm_bridge bridge; > > > + struct drm_bridge *next_bridge; > > > + struct drm_dp_aux aux; > > > + > > > + struct mutex edid_lock; > > > + struct edid *edid; > > > + > > > + u8 rx_cap[DP_RECEIVER_CAP_SIZE]; > > > + > > > + struct mtk_dp_info info; > > > + enum mtk_dp_state state; > > > + > > > + struct mtk_dp_train_info train_info; > > > + enum mtk_dp_train_state train_state; > > > + unsigned int input_fmt; > > > + > > > + struct regmap *regs; > > > + struct clk *dp_tx_clk; > > > + > > > + bool enabled; > > > + bool audio_enable; > > > + > > > + bool has_fec; > > > + struct mutex dp_lock; > > > + > > > + struct mutex update_plugged_status_lock; > > > + > > > + hdmi_codec_plugged_cb plugged_cb; > > > + struct device *codec_dev; > > > + u8 connector_eld[MAX_ELD_BYTES]; > > > +}; > > > + > > > +enum mtk_dp_sdp_type { > > > + MTK_DP_SDP_NONE = 0x00, > > > + MTK_DP_SDP_ACM = 0x01, > > > + MTK_DP_SDP_ISRC = 0x02, > > > + MTK_DP_SDP_AVI = 0x03, > > > + MTK_DP_SDP_AUI = 0x04, > > > + MTK_DP_SDP_SPD = 0x05, > > > + MTK_DP_SDP_MPEG = 0x06, > > > + MTK_DP_SDP_NTSC = 0x07, > > > + MTK_DP_SDP_VSP = 0x08, > > > + MTK_DP_SDP_VSC = 0x09, > > > + MTK_DP_SDP_EXT = 0x0A, > > > + MTK_DP_SDP_PPS0 = 0x0B, > > > + MTK_DP_SDP_PPS1 = 0x0C, > > > + MTK_DP_SDP_PPS2 = 0x0D, > > > + MTK_DP_SDP_PPS3 = 0x0E, > > > + MTK_DP_SDP_DRM = 0x10, > > > + MTK_DP_SDP_MAX_NUM > > > +}; > > > + > > > +struct mtk_dp_sdp_packet { > > > + enum mtk_dp_sdp_type type; > > > + struct dp_sdp sdp; > > > +}; > > > + > > > +#define MTK_DP_IEC_CHANNEL_STATUS_LEN 5 > > > +union mtk_dp_audio_channel_status { > > > + struct { > > > + u8 rev : 1; > > > + u8 is_lpcm : 1; > > > + u8 copy_right : 1; > > > + u8 additional_format_info : 3; > > > + u8 channel_status_mode : 2; > > > + u8 category_code; > > > + u8 src_num : 4; > > > + u8 channel_num : 4; > > > + u8 sampling_freq : 4; > > > + u8 clk_accuracy : 2; > > > + u8 rev2 : 2; > > > + u8 word_len : 4; > > > + u8 original_sampling_freq : 4; > > > + } iec; > > > + > > > + u8 buf[MTK_DP_IEC_CHANNEL_STATUS_LEN]; > > > +}; > > > + > > > +static struct regmap_config mtk_dp_regmap_config = { > > > + .reg_bits = 32, > > > + .val_bits = 32, > > > + .reg_stride = 4, > > > + .max_register = SEC_OFFSET + 0x90, > > > + .name = "mtk-dp-registers", > > > +}; > > > + > > > +static bool mtk_dp_is_edp(struct mtk_dp *mtk_dp) > > > > Separate edp and displayport into two patches. For example, the first > > patch support edp only, and the second patch add displayport function. > > > I have made the opposite as it seemed a bit more logical from how it's coded > now. > I've made a single patch with DP and a new one adding eDP support on top of it. > The change is quite small, adding the eDP on top of DP: > drivers/gpu/drm/mediatek/mtk_dp.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 74 insertions(+), 29 deletions(-) > > > +{ > > > + return mtk_dp->next_bridge != NULL; > > > +} > > > + > > > > [snip] > > > > > + > > > +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge, > > > + struct drm_connector *connector) > > > +{ > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > + bool enabled = mtk_dp->enabled; > > > + struct edid *new_edid = NULL; > > > + > > > + if (!enabled) > > > + drm_bridge_chain_pre_enable(bridge); > > > > In mtk_hdmi_bridge_get_edid(), there does not check the power. Why in > > this function need this? > > Does mtk hdmi driver has a bug? > > > I don't know, but I will be asking around. In short, no, the hdmi driver does not have a bug. We have to check power here as this driver handles both a DP and an eDP panel. As the power is controlled by the panel driver in the case of eDP, we have to make sure the bridge is enabled before trying to read the edid. Hope this answers the question. > > > + > > > + drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0); > > > + usleep_range(2000, 5000); > > > + > > > + if (mtk_dp_plug_state(mtk_dp)) > > > + new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc); > > > + > > > + if (!enabled) > > > + drm_bridge_chain_post_disable(bridge); > > > + > > > + mutex_lock(&mtk_dp->edid_lock); > > > + kfree(mtk_dp->edid); > > > + mtk_dp->edid = NULL; > > > + > > > + if (!new_edid) { > > > + mutex_unlock(&mtk_dp->edid_lock); > > > + return NULL; > > > + } > > > + > > > + mtk_dp->edid = drm_edid_duplicate(new_edid); > > > + mutex_unlock(&mtk_dp->edid_lock); > > > + > > > + return new_edid; > > > +} > > > + > > > +static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux, > > > + struct drm_dp_aux_msg *msg) > > > +{ > > > + ssize_t err = -EAGAIN; > > > + struct mtk_dp *mtk_dp; > > > + bool is_read; > > > + u8 request; > > > + size_t accessed_bytes = 0; > > > + int retry = 3, 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; > > > + err = -EAGAIN; > > > + goto err; > > > + } > > > + > > > + switch (msg->request) { > > > + case DP_AUX_I2C_MOT: > > > + case DP_AUX_I2C_WRITE: > > > + case DP_AUX_NATIVE_WRITE: > > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE: > > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE | DP_AUX_I2C_MOT: > > > + request = msg->request & ~DP_AUX_I2C_WRITE_STATUS_UPDATE; > > > + is_read = false; > > > + break; > > > + case DP_AUX_I2C_READ: > > > + case DP_AUX_NATIVE_READ: > > > + case DP_AUX_I2C_READ | DP_AUX_I2C_MOT: > > > + request = msg->request; > > > + is_read = true; > > > + break; > > > + default: > > > + drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n", > > > + msg->request); > > > + err = -EINVAL; > > > + goto err; > > > > Directly return here. > > > Hmm, not sure yet, it seems I still need to prime msg->reply anyway. > Though the error handling can be simplified a bit. > > > > + } > > > + > > > + if (msg->size == 0) { > > > + 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); > > > + retry = 3; > > > + while (retry--) { > > > + ret = mtk_dp_aux_do_transfer( > > > + mtk_dp, is_read, request, > > > + msg->address + accessed_bytes, > > > + msg->buffer + accessed_bytes, > > > + to_access); > > > + if (ret == 0) > > > + break; > > > + udelay(50); > > > + } > > > + if (!retry && ret) { > > > + drm_info(mtk_dp->drm_dev, > > > + "Failed to do AUX transfer: %d\n", > > > + ret); > > > + break; > > > + } > > > + accessed_bytes += to_access; > > > + } > > > + } > > > +err: > > > + if (!ret) { > > > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > > > + ret = msg->size; > > > + } else { > > > + msg->reply = DP_AUX_NATIVE_REPLY_NACK | DP_AUX_I2C_REPLY_NACK; > > > + return err; > > > + } > > > + > > > + msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK; > > > + return msg->size; > > > +} > > > + > > > +static void mtk_dp_aux_init(struct mtk_dp *mtk_dp) > > > +{ > > > + drm_dp_aux_init(&mtk_dp->aux); > > > + mtk_dp->aux.name = "aux_mtk_dp"; > > > + mtk_dp->aux.transfer = mtk_dp_aux_transfer; > > > +} > > > + > > > +static void mtk_dp_poweroff(struct mtk_dp *mtk_dp) > > > +{ > > > + mutex_lock(&mtk_dp->dp_lock); > > > + > > > + mtk_dp_hwirq_enable(mtk_dp, false); > > > + mtk_dp_power_disable(mtk_dp); > > > + phy_exit(mtk_dp->phy); > > > + clk_disable_unprepare(mtk_dp->dp_tx_clk); > > > + > > > + mutex_unlock(&mtk_dp->dp_lock); > > > +} > > > + > > > +static int mtk_dp_poweron(struct mtk_dp *mtk_dp) > > > +{ > > > + int ret = 0; > > > + > > > + mutex_lock(&mtk_dp->dp_lock); > > > + > > > + ret = clk_prepare_enable(mtk_dp->dp_tx_clk); > > > + if (ret < 0) { > > > + dev_err(mtk_dp->dev, "Fail to enable clock: %d\n", ret); > > > + goto err; > > > + } > > > + ret = phy_init(mtk_dp->phy); > > > + if (ret) { > > > + dev_err(mtk_dp->dev, "Failed to initialize phy: %d\n", ret); > > > + goto err_phy_init; > > > + } > > > + ret = mtk_dp_phy_configure(mtk_dp, MTK_DP_LINKRATE_RBR, 1); > > > + if (ret) { > > > + dev_err(mtk_dp->dev, "Failed to configure phy: %d\n", ret); > > > + goto err_phy_config; > > > + } > > > + > > > + mtk_dp_init_port(mtk_dp); > > > + mtk_dp_power_enable(mtk_dp); > > > + mtk_dp_hwirq_enable(mtk_dp, true); > > > + > > > +err_phy_config: > > > + phy_exit(mtk_dp->phy); > > > +err_phy_init: > > > + clk_disable_unprepare(mtk_dp->dp_tx_clk); > > > +err: > > > + mutex_unlock(&mtk_dp->dp_lock); > > > + return ret; > > > +} > > > + > > > +static int mtk_dp_bridge_attach(struct drm_bridge *bridge, > > > + enum drm_bridge_attach_flags flags) > > > +{ > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > + int ret; > > > + > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) { > > > + dev_err(mtk_dp->dev, "Driver does not provide a connector!"); > > > + return -EINVAL; > > > + } > > > + > > > + ret = mtk_dp_poweron(mtk_dp); > > > > Move the power on to mtk_dp_bridge_atomic_enable(). > > Moving the power-on sequence to the mtk_dp_bridge_atoimc_enable() breaks the eDP functionality. For the time being (ie: v7), the power on will stay in the bridge_attach() callback, sorry. > > > + if (ret) > > > + return ret; > > > + > > > + if (mtk_dp->next_bridge) { > > > + ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge, > > > + &mtk_dp->bridge, flags); > > > + if (ret) { > > > + drm_warn(mtk_dp->drm_dev, > > > + "Failed to attach external bridge: %d\n", ret); > > > + goto err_bridge_attach; > > > + } > > > + } > > > + > > > + mtk_dp->drm_dev = bridge->dev; > > > + > > > + return 0; > > > + > > > +err_bridge_attach: > > > + mtk_dp_poweroff(mtk_dp); > > > + return ret; > > > +} > > > + > > > +static void mtk_dp_bridge_detach(struct drm_bridge *bridge) > > > +{ > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > + > > > + mtk_dp->drm_dev = NULL; > > > + > > > + mtk_dp_poweroff(mtk_dp); > > > +} > > > + > > > +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge, > > > + struct drm_bridge_state *old_state) > > > +{ > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > + > > > + mtk_dp_video_mute(mtk_dp, true); > > > + mtk_dp_audio_mute(mtk_dp, true); > > > + mtk_dp->state = MTK_DP_STATE_IDLE; > > > + mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP; > > > + > > > + mtk_dp->enabled = false; > > > + msleep(100); > > > +} > > > + > > > +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp, > > > + struct drm_display_mode *mode) > > > +{ > > > + struct mtk_dp_timings *timings = &mtk_dp->info.timings; > > > + > > > + drm_display_mode_to_videomode(mode, &timings->vm); > > > + timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal; > > > + timings->htotal = mode->htotal; > > > + timings->vtotal = mode->vtotal; > > > +} > > > + > > > +static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge, > > > + struct drm_bridge_state *old_state) > > > +{ > > > + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > > + struct drm_connector *conn; > > > + struct drm_connector_state *conn_state; > > > + struct drm_crtc *crtc; > > > + struct drm_crtc_state *crtc_state; > > > + int ret = 0; > > > + int i; > > > + > > > + conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > > + bridge->encoder); > > > > mtk_dp->conn = drm_atomic_get_new_connector_for_encoder(old_state->base.state, > > bridge->encoder); > > > The connector doesn't seem to be used anywhere else in the driver. > Though I can add a drm_connector in the mtk_dp struct. > > > > + if (!conn) { > > > + drm_err(mtk_dp->drm_dev, > > > + "Can't enable bridge as connector is missing\n"); > > > + return; > > > + } > > > + > > > + memcpy(mtk_dp->connector_eld, conn->eld, MAX_ELD_BYTES); > > > + > > > + conn_state = > > > + drm_atomic_get_new_connector_state(old_state->base.state, conn); > > > + if (!conn_state) { > > > + drm_err(mtk_dp->drm_dev, > > > + "Can't enable bridge as connector state is missing\n"); > > > + return; > > > + } > > > + > > > + crtc = conn_state->crtc; > > > + if (!crtc) { > > > + drm_err(mtk_dp->drm_dev, > > > + "Can't enable bridge as connector state doesn't have a crtc\n"); > > > + return; > > > + } > > > + > > > + crtc_state = drm_atomic_get_new_crtc_state(old_state->base.state, crtc); > > > + if (!crtc_state) { > > > + drm_err(mtk_dp->drm_dev, > > > + "Can't enable bridge as crtc state is missing\n"); > > > + return; > > > + } > > > + > > > + mtk_dp_parse_drm_mode_timings(mtk_dp, &crtc_state->adjusted_mode); > > > > Refer to mtk_hdmi_bridge_atomic_enable() for getting the mode. > > > I'm sorry, I don't understand what I'm supposed to do here. > Could you elaborate? > > > > + if (!mtk_dp_parse_capabilities(mtk_dp)) { > > > + drm_err(mtk_dp->drm_dev, > > > + "Can't enable bridge as nothing is plugged in\n"); > > > + return; > > > + } > > > + > > > + //training > > > > Run checkpatch first. > > > > Regards, > > Chun-Kuang. > > > > > + for (i = 0; i < 50; i++) { > > > + ret = mtk_dp_train_handler(mtk_dp); > > > + if (ret) { > > > + drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", > > > + ret); > > > + return; > > > + } > > > + > > > + ret = mtk_dp_state_handler(mtk_dp); > > > + if (ret) { > > > + drm_err(mtk_dp->drm_dev, "State handler failed %d\n", > > > + ret); > > > + return; > > > + } > > > + } > > > + > > > + mtk_dp->enabled = true; > > > + mtk_dp_update_plugged_status(mtk_dp); > > > +} > > > + > > > > > > > > > -- > > > 2.32.0 > > > Thx, Guillaume.
Hi Maxime, Quoting Maxime Ripard (2021-12-13 17:54:22) > On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote: > > Hi, > > > > Quoting Maxime Ripard (2021-11-25 15:30:34) > > > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > > > > Hi, > > > > Thanks for all your input, really appreciated. > > > > > > > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > > > > Hi, > > > > > > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > > > > 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. > > > > > > > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > > > > you need a separate PHY driver in the first place? > > > > > > > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > > > > > > > It's a bit of a circular argument though :) > > > > > > > > > > It's a separate phy driver because it needs to go through another > > > > > maintainer's tree, but it needs to go through another maintainer's tree > > > > > because it's a separate phy driver. > > > > > > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > > > > the phy setup be done directly in the DP driver, if it's essentially a > > > > > single device? > > > > > > > > > > That being said, usually what those kind of questions mean is that > > > > > you're missing a comment or something in the commit log to provide that > > > > > context in the first place, so it would be great to add that context > > > > > here. > > > > > > > > > > And it will avoid the situation we're now in where multiple reviewers > > > > > ask the same questions over and over again :) > > > > > > > > > At first I didn't understand your reply, then I realized I gave you > > > > the wrong link... > > > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > > > > > > > The driver and phy were a single driver until v2 of this patch series > > > > and the phy setup > > > > was done directly in the driver (single driver, single C file). > > > > Here's the relevant link to the discussion between Chun-Kuang and Markus > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > > > > > > > I'll try to find a way to make it clearer for v7. > > > > > > OK, it makes sense then :) > > > > > > There's something weird though: the devices definitely look like they're > > > in a separate register range, yet you mention a regmap to handle the > > > shared register range. That range doesn't seem described anywhere in the > > > device tree though? What is it for? > > > > My understanding is that 0x1000 to 0x1fff controls the phy > > functionalities and 0x2000 to 0x4fff controls "non-phy" > > functionalities. And you are right, there's no description of that in > > the device tree whatsoever. The ranges are in the same actual device > > and thus it has been decided to not have dt-bindings for the phy > > device. > > Sure, that last part makes sense, but then I'm not sure why you don't > have the full register range in the device node you have in the DT? > > > The phy driver is a child of the DP driver that we register using > > platform_device_register_data() and we pass along the same regmap as > > the DP driver in its platform data. > > Especially if it's used by something, it should be described in the DT > somewhere. > > Maxime So, to make things crystal clear to a newbie (like me). Would you describe it like this: compatible = "mediatek,mt8195-dp-tx"; reg = <0 0x1c501000 0 0x0fff>, <0 0x1c502000 0 0x2fff>; instead of the current description: compatible = "mediatek,mt8195-dp-tx"; reg = <0 0x1c500000 0 0x8000>; I haven't checked what the rest of the 0x8000 range is used for though... Thx, Guillaume.
Hi, On Wed, Dec 15, 2021 at 09:03:01AM -0600, Guillaume Ranquet wrote: > Quoting Maxime Ripard (2021-12-13 17:54:22) > > On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote: > > > Hi, > > > > > > Quoting Maxime Ripard (2021-11-25 15:30:34) > > > > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > > > > > Hi, > > > > > Thanks for all your input, really appreciated. > > > > > > > > > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > > > > > Hi, > > > > > > > > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > > > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > > > > > 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. > > > > > > > > > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > > > > > you need a separate PHY driver in the first place? > > > > > > > > > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > > > > > > > > > It's a bit of a circular argument though :) > > > > > > > > > > > > It's a separate phy driver because it needs to go through another > > > > > > maintainer's tree, but it needs to go through another maintainer's tree > > > > > > because it's a separate phy driver. > > > > > > > > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > > > > > the phy setup be done directly in the DP driver, if it's essentially a > > > > > > single device? > > > > > > > > > > > > That being said, usually what those kind of questions mean is that > > > > > > you're missing a comment or something in the commit log to provide that > > > > > > context in the first place, so it would be great to add that context > > > > > > here. > > > > > > > > > > > > And it will avoid the situation we're now in where multiple reviewers > > > > > > ask the same questions over and over again :) > > > > > > > > > > > At first I didn't understand your reply, then I realized I gave you > > > > > the wrong link... > > > > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > > > > > > > > > The driver and phy were a single driver until v2 of this patch series > > > > > and the phy setup > > > > > was done directly in the driver (single driver, single C file). > > > > > Here's the relevant link to the discussion between Chun-Kuang and Markus > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > > > > > > > > > I'll try to find a way to make it clearer for v7. > > > > > > > > OK, it makes sense then :) > > > > > > > > There's something weird though: the devices definitely look like they're > > > > in a separate register range, yet you mention a regmap to handle the > > > > shared register range. That range doesn't seem described anywhere in the > > > > device tree though? What is it for? > > > > > > My understanding is that 0x1000 to 0x1fff controls the phy > > > functionalities and 0x2000 to 0x4fff controls "non-phy" > > > functionalities. And you are right, there's no description of that in > > > the device tree whatsoever. The ranges are in the same actual device > > > and thus it has been decided to not have dt-bindings for the phy > > > device. > > > > Sure, that last part makes sense, but then I'm not sure why you don't > > have the full register range in the device node you have in the DT? > > > > > The phy driver is a child of the DP driver that we register using > > > platform_device_register_data() and we pass along the same regmap as > > > the DP driver in its platform data. > > > > Especially if it's used by something, it should be described in the DT > > somewhere. > > > > Maxime > > > So, to make things crystal clear to a newbie (like me). > Would you describe it like this: > compatible = "mediatek,mt8195-dp-tx"; > reg = <0 0x1c501000 0 0x0fff>, > <0 0x1c502000 0 0x2fff>; > > instead of the current description: > compatible = "mediatek,mt8195-dp-tx"; > reg = <0 0x1c500000 0 0x8000>; > > I haven't checked what the rest of the 0x8000 range is used for though... I'm confused, is that what you had before? I recall you had a DTSI somewhere where you have two devices, and the dp-tx device not having the phy range? If the latter is what you have, and there's no overlapping ranges over multiple nodes, then it's fine already. Maxime
Quoting Maxime Ripard (2021-12-15 16:15:08) > Hi, > > On Wed, Dec 15, 2021 at 09:03:01AM -0600, Guillaume Ranquet wrote: > > Quoting Maxime Ripard (2021-12-13 17:54:22) > > > On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote: > > > > Hi, > > > > > > > > Quoting Maxime Ripard (2021-11-25 15:30:34) > > > > > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > > > > > > Hi, > > > > > > Thanks for all your input, really appreciated. > > > > > > > > > > > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > > > > > > Hi, > > > > > > > > > > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > > > > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > > > > > > you need a separate PHY driver in the first place? > > > > > > > > > > > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > > > > > > > > > > > It's a bit of a circular argument though :) > > > > > > > > > > > > > > It's a separate phy driver because it needs to go through another > > > > > > > maintainer's tree, but it needs to go through another maintainer's tree > > > > > > > because it's a separate phy driver. > > > > > > > > > > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > > > > > > the phy setup be done directly in the DP driver, if it's essentially a > > > > > > > single device? > > > > > > > > > > > > > > That being said, usually what those kind of questions mean is that > > > > > > > you're missing a comment or something in the commit log to provide that > > > > > > > context in the first place, so it would be great to add that context > > > > > > > here. > > > > > > > > > > > > > > And it will avoid the situation we're now in where multiple reviewers > > > > > > > ask the same questions over and over again :) > > > > > > > > > > > > > At first I didn't understand your reply, then I realized I gave you > > > > > > the wrong link... > > > > > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > > > > > > > > > > > The driver and phy were a single driver until v2 of this patch series > > > > > > and the phy setup > > > > > > was done directly in the driver (single driver, single C file). > > > > > > Here's the relevant link to the discussion between Chun-Kuang and Markus > > > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > > > > > > > > > > > I'll try to find a way to make it clearer for v7. > > > > > > > > > > OK, it makes sense then :) > > > > > > > > > > There's something weird though: the devices definitely look like they're > > > > > in a separate register range, yet you mention a regmap to handle the > > > > > shared register range. That range doesn't seem described anywhere in the > > > > > device tree though? What is it for? > > > > > > > > My understanding is that 0x1000 to 0x1fff controls the phy > > > > functionalities and 0x2000 to 0x4fff controls "non-phy" > > > > functionalities. And you are right, there's no description of that in > > > > the device tree whatsoever. The ranges are in the same actual device > > > > and thus it has been decided to not have dt-bindings for the phy > > > > device. > > > > > > Sure, that last part makes sense, but then I'm not sure why you don't > > > have the full register range in the device node you have in the DT? > > > > > > > The phy driver is a child of the DP driver that we register using > > > > platform_device_register_data() and we pass along the same regmap as > > > > the DP driver in its platform data. > > > > > > Especially if it's used by something, it should be described in the DT > > > somewhere. > > > > > > Maxime > > > > > > So, to make things crystal clear to a newbie (like me). > > Would you describe it like this: > > compatible = "mediatek,mt8195-dp-tx"; > > reg = <0 0x1c501000 0 0x0fff>, > > <0 0x1c502000 0 0x2fff>; > > > > instead of the current description: > > compatible = "mediatek,mt8195-dp-tx"; > > reg = <0 0x1c500000 0 0x8000>; > > > > I haven't checked what the rest of the 0x8000 range is used for though... > > I'm confused, is that what you had before? > > I recall you had a DTSI somewhere where you have two devices, and the > dp-tx device not having the phy range? > > If the latter is what you have, and there's no overlapping ranges over > multiple nodes, then it's fine already. > > Maxime This is what I have today in the mt8195.dtsi I'm using for testing purpose. Provided by mediatek on their vendor branch, not sure if it has been submitted to the list yet: edp_tx: edp_tx@1c500000 { status = "disabled"; compatible = "mediatek,mt8195-dp-tx"; reg = <0 0x1c500000 0 0x8000>; nvmem-cells = <&dp_calibration>; nvmem-cell-names = "dp_calibration_data"; power-domains = <&spm MT8195_POWER_DOMAIN_EPD_TX>; interrupts = <GIC_SPI 676 IRQ_TYPE_LEVEL_HIGH 0>; }; dp_tx: dp_tx@1c600000 { compatible = "mediatek,mt8195-dp-tx"; reg = <0 0x1c600000 0 0x8000>; nvmem-cells = <&dp_calibration>; nvmem-cell-names = "dp_calibration_data"; power-domains = <&spm MT8195_POWER_DOMAIN_DP_TX>; interrupts = <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH 0>; status = "disabled"; }; With no device tree node for the dp-phy. The phy range is included in the reg range of the dp-tx device.
On Wed, Dec 15, 2021 at 10:02:22AM -0600, Guillaume Ranquet wrote: > Quoting Maxime Ripard (2021-12-15 16:15:08) > > Hi, > > > > On Wed, Dec 15, 2021 at 09:03:01AM -0600, Guillaume Ranquet wrote: > > > Quoting Maxime Ripard (2021-12-13 17:54:22) > > > > On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote: > > > > > Hi, > > > > > > > > > > Quoting Maxime Ripard (2021-11-25 15:30:34) > > > > > > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote: > > > > > > > Hi, > > > > > > > Thanks for all your input, really appreciated. > > > > > > > > > > > > > > Quoting Maxime Ripard (2021-11-16 15:51:12) > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote: > > > > > > > > > Quoting Maxime Ripard (2021-11-15 11:11:29) > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > If the PHY is in the same register space than the DP controller, why do > > > > > > > > > > you need a separate PHY driver in the first place? > > > > > > > > > > > > > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series: > > > > > > > > > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/ > > > > > > > > > > > > > > > > It's a bit of a circular argument though :) > > > > > > > > > > > > > > > > It's a separate phy driver because it needs to go through another > > > > > > > > maintainer's tree, but it needs to go through another maintainer's tree > > > > > > > > because it's a separate phy driver. > > > > > > > > > > > > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't > > > > > > > > the phy setup be done directly in the DP driver, if it's essentially a > > > > > > > > single device? > > > > > > > > > > > > > > > > That being said, usually what those kind of questions mean is that > > > > > > > > you're missing a comment or something in the commit log to provide that > > > > > > > > context in the first place, so it would be great to add that context > > > > > > > > here. > > > > > > > > > > > > > > > > And it will avoid the situation we're now in where multiple reviewers > > > > > > > > ask the same questions over and over again :) > > > > > > > > > > > > > > > At first I didn't understand your reply, then I realized I gave you > > > > > > > the wrong link... > > > > > > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually. > > > > > > > > > > > > > > The driver and phy were a single driver until v2 of this patch series > > > > > > > and the phy setup > > > > > > > was done directly in the driver (single driver, single C file). > > > > > > > Here's the relevant link to the discussion between Chun-Kuang and Markus > > > > > > > > > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t > > > > > > > > > > > > > > I'll try to find a way to make it clearer for v7. > > > > > > > > > > > > OK, it makes sense then :) > > > > > > > > > > > > There's something weird though: the devices definitely look like they're > > > > > > in a separate register range, yet you mention a regmap to handle the > > > > > > shared register range. That range doesn't seem described anywhere in the > > > > > > device tree though? What is it for? > > > > > > > > > > My understanding is that 0x1000 to 0x1fff controls the phy > > > > > functionalities and 0x2000 to 0x4fff controls "non-phy" > > > > > functionalities. And you are right, there's no description of that in > > > > > the device tree whatsoever. The ranges are in the same actual device > > > > > and thus it has been decided to not have dt-bindings for the phy > > > > > device. > > > > > > > > Sure, that last part makes sense, but then I'm not sure why you don't > > > > have the full register range in the device node you have in the DT? > > > > > > > > > The phy driver is a child of the DP driver that we register using > > > > > platform_device_register_data() and we pass along the same regmap as > > > > > the DP driver in its platform data. > > > > > > > > Especially if it's used by something, it should be described in the DT > > > > somewhere. > > > > > > > > Maxime > > > > > > > > > So, to make things crystal clear to a newbie (like me). > > > Would you describe it like this: > > > compatible = "mediatek,mt8195-dp-tx"; > > > reg = <0 0x1c501000 0 0x0fff>, > > > <0 0x1c502000 0 0x2fff>; > > > > > > instead of the current description: > > > compatible = "mediatek,mt8195-dp-tx"; > > > reg = <0 0x1c500000 0 0x8000>; > > > > > > I haven't checked what the rest of the 0x8000 range is used for though... > > > > I'm confused, is that what you had before? > > > > I recall you had a DTSI somewhere where you have two devices, and the > > dp-tx device not having the phy range? > > > > If the latter is what you have, and there's no overlapping ranges over > > multiple nodes, then it's fine already. > > > > Maxime > > This is what I have today in the mt8195.dtsi I'm using for testing purpose. > Provided by mediatek on their vendor branch, not sure if it has been submitted > to the list yet: > > edp_tx: edp_tx@1c500000 { > status = "disabled"; > compatible = "mediatek,mt8195-dp-tx"; > reg = <0 0x1c500000 0 0x8000>; > nvmem-cells = <&dp_calibration>; > nvmem-cell-names = "dp_calibration_data"; > power-domains = <&spm MT8195_POWER_DOMAIN_EPD_TX>; > interrupts = <GIC_SPI 676 IRQ_TYPE_LEVEL_HIGH 0>; > }; > > dp_tx: dp_tx@1c600000 { > compatible = "mediatek,mt8195-dp-tx"; > reg = <0 0x1c600000 0 0x8000>; > nvmem-cells = <&dp_calibration>; > nvmem-cell-names = "dp_calibration_data"; > power-domains = <&spm MT8195_POWER_DOMAIN_DP_TX>; > interrupts = <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH 0>; > status = "disabled"; > }; > > With no device tree node for the dp-phy. > The phy range is included in the reg range of the dp-tx device. It's all good then, sorry for the noise Maxime