| Message ID | 20231023-display-support-v1-14-5c860ed5c33b@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add display support for the MT8365-EVK board | expand |
Il 23/10/23 16:40, amergnat@baylibre.com ha scritto: > From: Fabien Parent <fparent@baylibre.com> > > MT8365 requires an additional clock for DPI. Add support for that > additional clock. > > Signed-off-by: Fabien Parent <fparent@baylibre.com> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> I'm not convinced that this is right... at all. From a fast check of the MT8365 DPI clocks, I can see that the DPI0 clock declares parent VPLL_DPIX (a fixed clock), but nothing ever has VPLL_DPIX_EN (which is the GATE clock, enabling output of DPIx VPLL?). But then, there's even more: no clock ever references the CLK_TOP_DPI0_SEL nor the CLK_TOP_DPI1_SEL gate, which is a PLL parent selector... in other platforms, that is muxing through the TVDPLL, but on MT8365 that is LVDSPLL?! I have many questions now: * Two PLLs are apparently brought up, but which one is the right one?! * Is the LVDS PLL really used for DisplayPort? (dpi0_sel) * Is the VPLL_DPIx PLL used for DisplayPort instead? (dpi0_dpi0) * Why is the LVDSTX_PXL clock using the same PLL as DPI0?! * Why is the VPLL_DPIx gate never enabled? * Are you sure that CLK_MM_DPI0_DPI0's parent shouldn't be dpi0_sel instead? * Where is DPI1 in this SoC? Why is there a dpi1_sel clock, but no MM clock for the DPI1 controller? Is there any DPI1 controller, even?! * Why is there a DPI1 MUX, if there's no DPI1 controller?! Answering all those questions will lead you to the right change, which I believe to be in the clock drivers, not here in mtk_dpi.c. Cheers! Angelo
On 24/10/2023 11:12, AngeloGioacchino Del Regno wrote: > Il 23/10/23 16:40, amergnat@baylibre.com ha scritto: >> From: Fabien Parent <fparent@baylibre.com> >> >> MT8365 requires an additional clock for DPI. Add support for that >> additional clock. >> >> Signed-off-by: Fabien Parent <fparent@baylibre.com> >> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > > I'm not convinced that this is right... at all. > > From a fast check of the MT8365 DPI clocks, I can see that the DPI0 > clock declares > parent VPLL_DPIX (a fixed clock), but nothing ever has VPLL_DPIX_EN > (which is the > GATE clock, enabling output of DPIx VPLL?). > > But then, there's even more: no clock ever references the > CLK_TOP_DPI0_SEL nor the > CLK_TOP_DPI1_SEL gate, which is a PLL parent selector... in other > platforms, that > is muxing through the TVDPLL, but on MT8365 that is LVDSPLL?! AFAI see into mt8365 documentation, there is no TVDPLL, only LVDSPLL > > I have many questions now: > * Two PLLs are apparently brought up, but which one is the right one?! > * Is the LVDS PLL really used for DisplayPort? (dpi0_sel) Seems to be LVDS enable prepare protect duty hardware clock count count count rate accuracy phase cycle enable ------------------------------------------------------------------------------------------------------- clk26m 18 19 1 26000000 0 0 Y vpll_dpix 1 1 0 75000000 0 0 50000 Y mm_flvdstx_pxl 0 0 0 75000000 0 0 50000 N mm_dpi0_dpi0 1 1 0 75000000 0 0 50000 Y vpll_dpix_en 0 0 0 75000000 0 0 50000 N lvdspll 1 1 0 283999497 0 0 50000 Y lvdspll_d16 0 0 0 17749968 0 0 50000 Y lvdspll_d8 0 0 0 35499937 0 0 50000 Y lvdspll_d4 0 0 0 70999874 0 0 50000 Y lvdspll_d2 1 1 0 141999748 0 0 50000 Y dpi0_sel 1 1 0 141999748 0 0 50000 Y dpi1_sel 0 0 0 141999748 0 0 50000 N mmpll 1 1 0 456999909 0 0 50000 Y mmpll_ck 1 1 0 456999909 0 0 50000 Y mm_sel 15 15 0 456999909 0 0 50000 Y mm_dpi0 1 1 0 456999909 0 0 50000 Y > * Are you sure that CLK_MM_DPI0_DPI0's parent shouldn't be dpi0_sel > instead? I'm agree with you. After few change, it works. - GATE_MM0(CLK_MM_DPI0_DPI0, "mm_dpi0_dpi0", "vpll_dpix", 20), + GATE_MM0(CLK_MM_DPI0_DPI0, "mm_dpi0_dpi0", "dpi0_sel", 20), - clocks = <&topckgen CLK_TOP_DPI0_SEL>, + clocks = <&mmsys CLK_MM_DPI0_DPI0>, enable prepare protect duty hardware clock count count count rate accuracy phase cycle enable ------------------------------------------------------------------------------------------------------- vpll_dpix 0 0 0 75000000 0 0 50000 Y mm_flvdstx_pxl 0 0 0 75000000 0 0 50000 N vpll_dpix_en 0 0 0 75000000 0 0 50000 N lvdspll 1 1 0 283999497 0 0 50000 Y lvdspll_d16 0 0 0 17749968 0 0 50000 Y lvdspll_d8 0 0 0 35499937 0 0 50000 Y lvdspll_d4 0 0 0 70999874 0 0 50000 Y lvdspll_d2 1 1 0 141999748 0 0 50000 Y dpi0_sel 1 1 0 141999748 0 0 50000 Y mm_dpi0_dpi0 1 1 0 141999748 0 0 50000 Y dpi1_sel 0 0 0 141999748 0 0 50000 N mmpll 1 1 0 456999909 0 0 50000 Y mmpll_d2 0 0 0 228499954 0 0 50000 Y mmpll_ck 1 1 0 456999909 0 0 50000 Y mm_sel 15 15 0 456999909 0 0 50000 Y mm_dpi0 1 1 0 456999909 0 0 50000 Y > * Where is DPI1 in this SoC? Why is there a dpi1_sel clock, but no MM clock > for the DPI1 controller? Is there any DPI1 controller, even?! DPI1 isn't documented. > * Why is there a DPI1 MUX, if there's no DPI1 controller?! Good question, I don't know. Legacy of the downstream code. That will be fixed for the next version.
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index 2f931e4e2b60..ddd7c54febe6 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -70,6 +70,7 @@ struct mtk_dpi { struct device *mmsys_dev; struct clk *engine_clk; struct clk *pixel_clk; + struct clk *dpi_clk; struct clk *tvd_clk; int irq; struct drm_display_mode mode; @@ -137,6 +138,7 @@ struct mtk_dpi_yc_limit { * @csc_enable_bit: Enable bit of CSC. * @pixels_per_iter: Quantity of transferred pixels per iteration. * @edge_cfg_in_mmsys: If the edge configuration for DPI's output needs to be set in MMSYS. + * @is_dpi_clk_req: Support the additionnal DPI clock. */ struct mtk_dpi_conf { unsigned int (*cal_factor)(int clock); @@ -156,6 +158,7 @@ struct mtk_dpi_conf { u32 csc_enable_bit; u32 pixels_per_iter; bool edge_cfg_in_mmsys; + bool is_dpi_clk_req; }; static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask) @@ -472,6 +475,7 @@ 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_clk); } static int mtk_dpi_power_on(struct mtk_dpi *dpi) @@ -481,10 +485,16 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) if (++dpi->refcount != 1) return 0; + ret = clk_prepare_enable(dpi->dpi_clk); + if (ret) { + dev_err(dpi->dev, "failed to enable dpi clock: %d\n", ret); + goto err_refcount; + } + ret = clk_prepare_enable(dpi->engine_clk); if (ret) { dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret); - goto err_refcount; + goto err_engine; } ret = clk_prepare_enable(dpi->pixel_clk); @@ -497,6 +507,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi) err_pixel: clk_disable_unprepare(dpi->engine_clk); +err_engine: + clk_disable_unprepare(dpi->dpi_clk); err_refcount: dpi->refcount--; return ret; @@ -902,6 +914,7 @@ static const struct mtk_dpi_conf mt8173_conf = { .channel_swap_shift = CH_SWAP, .yuv422_en_bit = YUV422_EN, .csc_enable_bit = CSC_ENABLE, + .is_dpi_clk_req = false, }; static const struct mtk_dpi_conf mt2701_conf = { @@ -920,6 +933,7 @@ static const struct mtk_dpi_conf mt2701_conf = { .channel_swap_shift = CH_SWAP, .yuv422_en_bit = YUV422_EN, .csc_enable_bit = CSC_ENABLE, + .is_dpi_clk_req = false, }; static const struct mtk_dpi_conf mt8183_conf = { @@ -937,6 +951,7 @@ static const struct mtk_dpi_conf mt8183_conf = { .channel_swap_shift = CH_SWAP, .yuv422_en_bit = YUV422_EN, .csc_enable_bit = CSC_ENABLE, + .is_dpi_clk_req = false, }; static const struct mtk_dpi_conf mt8186_conf = { @@ -969,6 +984,7 @@ static const struct mtk_dpi_conf mt8188_dpintf_conf = { .channel_swap_shift = DPINTF_CH_SWAP, .yuv422_en_bit = DPINTF_YUV422_EN, .csc_enable_bit = DPINTF_CSC_ENABLE, + .is_dpi_clk_req = false, }; static const struct mtk_dpi_conf mt8192_conf = { @@ -986,6 +1002,7 @@ static const struct mtk_dpi_conf mt8192_conf = { .channel_swap_shift = CH_SWAP, .yuv422_en_bit = YUV422_EN, .csc_enable_bit = CSC_ENABLE, + .is_dpi_clk_req = false, }; static const struct mtk_dpi_conf mt8195_dpintf_conf = { @@ -1000,6 +1017,25 @@ static const struct mtk_dpi_conf mt8195_dpintf_conf = { .channel_swap_shift = DPINTF_CH_SWAP, .yuv422_en_bit = DPINTF_YUV422_EN, .csc_enable_bit = DPINTF_CSC_ENABLE, + .is_dpi_clk_req = false, +}; + +static const struct mtk_dpi_conf mt8365_conf = { + .cal_factor = mt8183_calculate_factor, + .channel_swap_shift = CH_SWAP, + .csc_enable_bit = CSC_ENABLE, + .dimension_mask = HPW_MASK, + .hvsize_mask = HSIZE_MASK, + .is_ck_de_pol = true, + .is_dpi_clk_req = true, + .max_clock_khz = 150000, + .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), + .output_fmts = mt8183_output_fmts, + .pixels_per_iter = 1, + .reg_h_fre_con = 0xe0, + .support_direct_pin = true, + .swap_input_support = true, + .yuv422_en_bit = YUV422_EN, }; static int mtk_dpi_probe(struct platform_device *pdev) @@ -1056,6 +1092,17 @@ static int mtk_dpi_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(dpi->tvd_clk), "Failed to get tvdpll clock\n"); + if (dpi->conf->is_dpi_clk_req) { + dpi->dpi_clk = devm_clk_get(dev, "dpi"); + if (IS_ERR(dpi->dpi_clk)) { + ret = PTR_ERR(dpi->dpi_clk); + if (ret != -EPROBE_DEFER) + dev_err(dev, "Failed to get dpi clock: %d\n", ret); + + return ret; + } + } + dpi->irq = platform_get_irq(pdev, 0); if (dpi->irq < 0) return dpi->irq; @@ -1097,6 +1144,7 @@ static const struct of_device_id mtk_dpi_of_ids[] = { { .compatible = "mediatek,mt8188-dp-intf", .data = &mt8188_dpintf_conf }, { .compatible = "mediatek,mt8192-dpi", .data = &mt8192_conf }, { .compatible = "mediatek,mt8195-dp-intf", .data = &mt8195_dpintf_conf }, + { .compatible = "mediatek,mt8365-dpi", .data = &mt8365_conf }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, mtk_dpi_of_ids);