Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9a0817c2-4101-5c21-977d-77ac0d83a067@baylibre.com>
Date:   Thu, 13 Jul 2023 15:22:41 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for
 top_dp/edp muxes
Content-Language: en-US
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>, sboyd@kernel.org
Cc:     mturquette@baylibre.com, matthias.bgg@gmail.com,
        wenst@chromium.org, msp@baylibre.com, yangyingliang@huawei.com,
        u.kleine-koenig@pengutronix.de, miles.chen@mediatek.com,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, kernel@collabora.com
References: <20230713072138.84117-1-angelogioacchino.delregno@collabora.com>
 <20230713072138.84117-3-angelogioacchino.delregno@collabora.com>
From:   Alexandre Mergnat <amergnat@baylibre.com>
In-Reply-To: <20230713072138.84117-3-angelogioacchino.delregno@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
> The top_dp and top_edp muxes can be both parented to either TVDPLL1
> or TVDPLL2, two identically specced PLLs for the specific purpose of
> giving out pixel clock: this becomes a problem when the MediaTek
> DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
> 
> In the usecase of two simultaneous outputs (using two controllers),
> it was seen that one of the displays would sometimes display garbled
> output (if any at all) and this was because:
>   - top_edp was set to TVDPLL1, outputting X GHz
>   - top_dp was set to TVDPLL2, outputting Y GHz
>     - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
>       - top_dp is switched to TVDPLL1
>       - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
>       - eDP display is garbled
> 
> To solve this issue, remove all TVDPLL1 parents from `top_dp` and
> all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
> clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
> able to use the right bit index for the new parents list.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> index 81daa24cadde..abb3721f6e1b 100644
> --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
> +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
> @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
>   
>   static const char * const dp_parents[] = {
>   	"clk26m",
> -	"tvdpll1_d2",
>   	"tvdpll2_d2",
> -	"tvdpll1_d4",
>   	"tvdpll2_d4",
> -	"tvdpll1_d8",
>   	"tvdpll2_d8",
> -	"tvdpll1_d16",
>   	"tvdpll2_d16"
>   };
> +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
> +
> +static const char * const edp_parents[] = {
> +	"clk26m",
> +	"tvdpll1_d2",
> +	"tvdpll1_d4",
> +	"tvdpll1_d8",
> +	"tvdpll1_d16"
> +};
> +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };

AFAII your solution is to force a specific TVDPLLX for each display, and 
it isn't dynamic.

Do you think it's possible to do that using the DTS ? I'm asking 
because, IMHO, this kind of setup is more friendly/readable/flexible in 
the DTS than hardcoded into the driver.

>   
>   static const char * const disp_pwm_parents[] = {
>   	"clk26m",
> @@ -957,11 +963,11 @@ static const struct mtk_mux top_mtk_muxes[] = {
>   	MUX_GATE_CLR_SET_UPD_FLAGS(CLK_TOP_PWRMCU, "top_pwrmcu",
>   		pwrmcu_parents, 0x08C, 0x090, 0x094, 16, 3, 23, 0x08, 6,
>   		CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> -	MUX_GATE_CLR_SET_UPD(CLK_TOP_DP, "top_dp",
> -		dp_parents, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
> +	MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_DP, "top_dp",
> +		dp_parents, dp_parents_idx, 0x08C, 0x090, 0x094, 24, 4, 31, 0x08, 7),
>   	/* CLK_CFG_10 */
> -	MUX_GATE_CLR_SET_UPD(CLK_TOP_EDP, "top_edp",
> -		dp_parents, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
> +	MUX_GATE_CLR_SET_UPD_INDEXED(CLK_TOP_EDP, "top_edp",
> +		edp_parents, edp_parents_idx, 0x098, 0x09C, 0x0A0, 0, 4, 7, 0x08, 8),
>   	MUX_GATE_CLR_SET_UPD(CLK_TOP_DPI, "top_dpi",
>   		dp_parents, 0x098, 0x09C, 0x0A0, 8, 4, 15, 0x08, 9),
>   	MUX_GATE_CLR_SET_UPD(CLK_TOP_DISP_PWM0, "top_disp_pwm0",

-- 
Regards,
Alexandre