linux-kernel - Re: [PATCH v1,2/3] drm/mediatek: Add TOPCKGEN select mux control dpi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 2 Mar 2022 10:07:40 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     xinlei.lee@...iatek.com, chunkuang.hu@...nel.org,
        p.zabel@...gutronix.de, airlied@...ux.ie, daniel@...ll.ch,
        matthias.bgg@...il.com
Cc:     dri-devel@...ts.freedesktop.org,
        linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        jitao.shi@...iatek.com, allen-kh.cheng@...iatek.com,
        rex-bc.chen@...iatek.com
Subject: Re: [PATCH v1,2/3] drm/mediatek: Add TOPCKGEN select mux control
 dpi_clk

Il 25/02/22 10:53, xinlei.lee@...iatek.com ha scritto:
> From: Xinlei Lee <xinlei.lee@...iatek.com>
> 
> Dpi_clk is controlled by the mux selected
> by TOPCKGEN and APMIXEDSYS can support small resolution.
> 
> Signed-off-by: Xinlei Lee <xinlei.lee@...iatek.com>

Hello Xinlei,

as it was pointed out by reviewers in the MT8195 DisplayPort series, that is
adding the same logic that you are proposing in this patch, the clock parent
selection should be performed by the clock drivers, I'd say in the callback
.set_rate_and_parent(), and not by the mtk_dpi driver.

Please fix this in the proper drivers (clocks!) instead.

Thanks,
Angelo

> ---
>   drivers/gpu/drm/mediatek/mtk_dpi.c | 38 ++++++++++++++++++++++++++----
>   1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index 4554e2de1430..bad686817e29 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;
> @@ -73,6 +81,7 @@ struct mtk_dpi {
>   	struct clk *engine_clk;
>   	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;
> @@ -459,6 +468,7 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>   	struct videomode vm = { 0 };
>   	unsigned long pll_rate;
>   	unsigned int factor;
> +	struct clk *clksrc = NULL;
>   
>   	/* let pll_rate can fix the valid range of tvdpll (1G~2GHz) */
>   	factor = dpi->conf->cal_factor(mode->clock);
> @@ -473,11 +483,26 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>   
>   	vm.pixelclock = pll_rate / factor;
>   	if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) ||
> -	    (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE))
> -		clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2);
> -	else
> -		clk_set_rate(dpi->pixel_clk, vm.pixelclock);
> +	    (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE)) {
> +		if (factor == 8)
> +			clksrc = dpi->pclk_src[2];
> +		else if (factor == 4)
> +			clksrc = dpi->pclk_src[1];
> +		else
> +			clksrc = dpi->pclk_src[1];
> +		}
> +	else {
> +		if (factor == 8)
> +			clksrc = dpi->pclk_src[3];
> +		else if (factor == 4)
> +			clksrc = dpi->pclk_src[2];
> +		else
> +			clksrc = dpi->pclk_src[2];
> +	}
>   
> +	clk_prepare_enable(dpi->pixel_clk);
> +	clk_set_parent(dpi->pixel_clk, clksrc);
> +	clk_disable_unprepare(dpi->pixel_clk);
>   
>   	vm.pixelclock = clk_get_rate(dpi->pixel_clk);
>   
> @@ -893,6 +918,11 @@ static int mtk_dpi_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	dpi->pclk_src[1] = devm_clk_get_optional(dev, "tvdpll_d2");
> +	dpi->pclk_src[2] = devm_clk_get_optional(dev, "tvdpll_d4");
> +	dpi->pclk_src[3] = devm_clk_get_optional(dev, "tvdpll_d8");
> +	dpi->pclk_src[4] = devm_clk_get_optional(dev, "tvdpll_d16");
> +
>   	dpi->irq = platform_get_irq(pdev, 0);
>   	if (dpi->irq <= 0)
>   		return -EINVAL;