[PATCH] drm/msm/dsi: save PLL registers across first PHY reset

[PATCH] drm/msm/dsi: save PLL registers across first PHY reset

[benl-kernelpatches]

From: Benjamin Li <benl@squareup.com>

Take advantage of previously-added support for persisting PLL
registers across DSI PHY disable/enable cycles (see 328e1a6
'drm/msm/dsi: Save/Restore PLL status across PHY reset') to
support persisting across the very first DSI PHY enable at
boot.

The bootloader may have left the PLL registers in a non-default
state. For example, for dsi_pll_28nm.c on 8x16/8x39, the byte
clock mux's power-on reset configuration is to bypass DIV1, but
depending on bandwidth requirements[1] the bootloader may have
set the DIV1 path.

When the byte clock mux is registered with the generic clock
framework at probe time, the framework reads & caches the value
of the mux bit field (the initial clock parent). After PHY enable,
when clk_set_rate is called on the byte clock, the framework
assumes there is no need to reparent, and doesn't re-write the
mux bit field. But PHY enable resets PLL registers, so the mux
bit field actually silently reverted to the DIV1 bypass path.
This causes the byte clock to be off by a factor of e.g. 2 for
our tested WXGA panel.

The above issue manifests as the display not working and a
constant stream of FIFO/LP0 contention errors.

[1] The specific requirement for triggering the DIV1 path (and
thus this issue) on 28nm is a panel with pixel clock <116.7MHz
(one-third the minimum VCO setting). FHD/1080p (~145MHz) is fine,
WXGA/1280x800 (~75MHz) is not.

Signed-off-by: Benjamin Li <benl@squareup.com>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 009f5b843dd1..139b4a5aaf86 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -621,6 +621,22 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 		phy->pll = NULL;
 	}
 
+	/*
+	 * As explained in msm_dsi_phy_enable, resetting the DSI PHY (as done
+	 * in dsi_mgr_phy_enable) silently changes its PLL registers to power-on
+	 * defaults, but the generic clock framework manages and caches several
+	 * of the PLL registers. It initializes these caches at registration
+	 * time via register read.
+	 *
+	 * As a result, we need to save DSI PLL registers once at probe in order
+	 * for the first call to msm_dsi_phy_enable to successfully bring PLL
+	 * registers back in line with what the generic clock framework expects.
+	 *
+	 * Subsequent PLL restores during msm_dsi_phy_enable will always be
+	 * paired with PLL saves in msm_dsi_phy_disable.
+	 */
+	msm_dsi_pll_save_state(phy->pll);
+
 	dsi_phy_disable_resource(phy);
 
 	platform_set_drvdata(pdev, phy);
-- 
2.17.1

Re: [PATCH] drm/msm/dsi: save PLL registers across first PHY reset

[Dmitry Baryshkov]

Hello,

On 07/10/2020 03:10, benl-kernelpatches@squareup.com wrote:
> From: Benjamin Li <benl@squareup.com>
> 
> Take advantage of previously-added support for persisting PLL
> registers across DSI PHY disable/enable cycles (see 328e1a6
> 'drm/msm/dsi: Save/Restore PLL status across PHY reset') to
> support persisting across the very first DSI PHY enable at
> boot.

Interesting enough, this breaks exactly on 8016. On DB410c with latest 
bootloader and w/o splash screen this patch causes boot freeze. Without 
this patch the board would successfully boot with display routed to HDMI.

> The bootloader may have left the PLL registers in a non-default
> state. For example, for dsi_pll_28nm.c on 8x16/8x39, the byte
> clock mux's power-on reset configuration is to bypass DIV1, but
> depending on bandwidth requirements[1] the bootloader may have
> set the DIV1 path.
> 
> When the byte clock mux is registered with the generic clock
> framework at probe time, the framework reads & caches the value
> of the mux bit field (the initial clock parent). After PHY enable,
> when clk_set_rate is called on the byte clock, the framework
> assumes there is no need to reparent, and doesn't re-write the
> mux bit field. But PHY enable resets PLL registers, so the mux
> bit field actually silently reverted to the DIV1 bypass path.
> This causes the byte clock to be off by a factor of e.g. 2 for
> our tested WXGA panel.
> 
> The above issue manifests as the display not working and a
> constant stream of FIFO/LP0 contention errors.
> 
> [1] The specific requirement for triggering the DIV1 path (and
> thus this issue) on 28nm is a panel with pixel clock <116.7MHz
> (one-third the minimum VCO setting). FHD/1080p (~145MHz) is fine,
> WXGA/1280x800 (~75MHz) is not.
> 
> Signed-off-by: Benjamin Li <benl@squareup.com>
> ---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 009f5b843dd1..139b4a5aaf86 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -621,6 +621,22 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   		phy->pll = NULL;
>   	}
>   
> +	/*
> +	 * As explained in msm_dsi_phy_enable, resetting the DSI PHY (as done
> +	 * in dsi_mgr_phy_enable) silently changes its PLL registers to power-on
> +	 * defaults, but the generic clock framework manages and caches several
> +	 * of the PLL registers. It initializes these caches at registration
> +	 * time via register read.
> +	 *
> +	 * As a result, we need to save DSI PLL registers once at probe in order
> +	 * for the first call to msm_dsi_phy_enable to successfully bring PLL
> +	 * registers back in line with what the generic clock framework expects.
> +	 *
> +	 * Subsequent PLL restores during msm_dsi_phy_enable will always be
> +	 * paired with PLL saves in msm_dsi_phy_disable.
> +	 */
> +	msm_dsi_pll_save_state(phy->pll);
> +
>   	dsi_phy_disable_resource(phy);
>   
>   	platform_set_drvdata(pdev, phy);
> 


-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm/dsi: save PLL registers across first PHY reset

[Benjamin Li]

On 10/30/20 6:55 AM, Dmitry Baryshkov wrote:
> Hello,
> 
> On 07/10/2020 03:10, benl-kernelpatches@squareup.com wrote:
>> From: Benjamin Li <benl@squareup.com>
>>
>> Take advantage of previously-added support for persisting PLL
>> registers across DSI PHY disable/enable cycles (see 328e1a6
>> 'drm/msm/dsi: Save/Restore PLL status across PHY reset') to
>> support persisting across the very first DSI PHY enable at
>> boot.
> 
> Interesting enough, this breaks exactly on 8016. On DB410c with latest bootloader and w/o splash screen this patch causes boot freeze. Without this patch the board would successfully boot with display routed to HDMI.

Hi Dimtry,

Thanks for your fix for the DB410c breakage ("drm/msm/dsi: do not
try reading 28nm vco rate if it's not enabled") that this patch
causes.

I re-tested my patch on top of qcom/linux for-next (3e6a8ce094759)
which now has your fix, on a DB410c with HDMI display and no splash
(which seems to be the default using the Linaro SD card image's LK),
and indeed it is fixed.

I assume you already also did the same & are okay with this going in.
Appreciate the testing!

Ben

> 
>> The bootloader may have left the PLL registers in a non-default
>> state. For example, for dsi_pll_28nm.c on 8x16/8x39, the byte
>> clock mux's power-on reset configuration is to bypass DIV1, but
>> depending on bandwidth requirements[1] the bootloader may have
>> set the DIV1 path.
>>
>> When the byte clock mux is registered with the generic clock
>> framework at probe time, the framework reads & caches the value
>> of the mux bit field (the initial clock parent). After PHY enable,
>> when clk_set_rate is called on the byte clock, the framework
>> assumes there is no need to reparent, and doesn't re-write the
>> mux bit field. But PHY enable resets PLL registers, so the mux
>> bit field actually silently reverted to the DIV1 bypass path.
>> This causes the byte clock to be off by a factor of e.g. 2 for
>> our tested WXGA panel.
>>
>> The above issue manifests as the display not working and a
>> constant stream of FIFO/LP0 contention errors.
>>
>> [1] The specific requirement for triggering the DIV1 path (and
>> thus this issue) on 28nm is a panel with pixel clock <116.7MHz
>> (one-third the minimum VCO setting). FHD/1080p (~145MHz) is fine,
>> WXGA/1280x800 (~75MHz) is not.
>>
>> Signed-off-by: Benjamin Li <benl@squareup.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
>> index 009f5b843dd1..139b4a5aaf86 100644
>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
>> @@ -621,6 +621,22 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>>           phy->pll = NULL;
>>       }
>>   +    /*
>> +     * As explained in msm_dsi_phy_enable, resetting the DSI PHY (as done
>> +     * in dsi_mgr_phy_enable) silently changes its PLL registers to power-on
>> +     * defaults, but the generic clock framework manages and caches several
>> +     * of the PLL registers. It initializes these caches at registration
>> +     * time via register read.
>> +     *
>> +     * As a result, we need to save DSI PLL registers once at probe in order
>> +     * for the first call to msm_dsi_phy_enable to successfully bring PLL
>> +     * registers back in line with what the generic clock framework expects.
>> +     *
>> +     * Subsequent PLL restores during msm_dsi_phy_enable will always be
>> +     * paired with PLL saves in msm_dsi_phy_disable.
>> +     */
>> +    msm_dsi_pll_save_state(phy->pll);
>> +
>>       dsi_phy_disable_resource(phy);
>>         platform_set_drvdata(pdev, phy);
>>
> 
> 

Re: [PATCH] drm/msm/dsi: save PLL registers across first PHY reset

[Dmitry Baryshkov]

On Sat, 30 Jan 2021 at 05:00, Benjamin Li <benl@squareup.com> wrote:
>
>
> On 10/30/20 6:55 AM, Dmitry Baryshkov wrote:
> > Hello,
> >
> > On 07/10/2020 03:10, benl-kernelpatches@squareup.com wrote:
> >> From: Benjamin Li <benl@squareup.com>
> >>
> >> Take advantage of previously-added support for persisting PLL
> >> registers across DSI PHY disable/enable cycles (see 328e1a6
> >> 'drm/msm/dsi: Save/Restore PLL status across PHY reset') to
> >> support persisting across the very first DSI PHY enable at
> >> boot.
> >
> > Interesting enough, this breaks exactly on 8016. On DB410c with latest bootloader and w/o splash screen this patch causes boot freeze. Without this patch the board would successfully boot with display routed to HDMI.
>
> Hi Dimtry,
>
> Thanks for your fix for the DB410c breakage ("drm/msm/dsi: do not
> try reading 28nm vco rate if it's not enabled") that this patch
> causes.
>
> I re-tested my patch on top of qcom/linux for-next (3e6a8ce094759)
> which now has your fix, on a DB410c with HDMI display and no splash
> (which seems to be the default using the Linaro SD card image's LK),
> and indeed it is fixed.
>
> I assume you already also did the same & are okay with this going in.
> Appreciate the testing!

Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested on RB5 and Dragonboard 845c (RB3).




-- 
With best wishes
Dmitry

[PATCH] drm/msm/dsi: do not try reading 28nm vco rate if it's not enabled

[Dmitry Baryshkov]

Reading VCO rate for this PLL can cause boot stalls, if it is not
enabled. Guard clk_hw_get_rate with a call to
dsi_pll_28nm_clk_is_enabled().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c
index 6dffd7f4a99b..37a1f996a588 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c
@@ -447,7 +447,10 @@ static void dsi_pll_28nm_save_state(struct msm_dsi_pll *pll)
 	cached_state->postdiv1 =
 			pll_read(base + REG_DSI_28nm_PHY_PLL_POSTDIV1_CFG);
 	cached_state->byte_mux = pll_read(base + REG_DSI_28nm_PHY_PLL_VREG_CFG);
-	cached_state->vco_rate = clk_hw_get_rate(&pll->clk_hw);
+	if (dsi_pll_28nm_clk_is_enabled(&pll->clk_hw))
+		cached_state->vco_rate = clk_hw_get_rate(&pll->clk_hw);
+	else
+		cached_state->vco_rate = 0;
 }
 
 static int dsi_pll_28nm_restore_state(struct msm_dsi_pll *pll)
-- 
2.28.0

Re: [PATCH] drm/msm/dsi: do not try reading 28nm vco rate if it's not enabled

[patchwork-bot+linux-arm-msm]

Hello:

This patch was applied to qcom/linux.git (refs/heads/for-next):

On Fri, 30 Oct 2020 18:12:07 +0300 you wrote:
> Reading VCO rate for this PLL can cause boot stalls, if it is not
> enabled. Guard clk_hw_get_rate with a call to
> dsi_pll_28nm_clk_is_enabled().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Here is the summary with links:
  - drm/msm/dsi: do not try reading 28nm vco rate if it's not enabled
    https://git.kernel.org/qcom/c/0e720ea8de07

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] drm/msm/dsi: save PLL registers across first PHY reset

[Dmitry Baryshkov]

Rob,

On 07/10/2020 03:10, benl-kernelpatches@squareup.com wrote:
> From: Benjamin Li <benl@squareup.com>
> 
> Take advantage of previously-added support for persisting PLL
> registers across DSI PHY disable/enable cycles (see 328e1a6
> 'drm/msm/dsi: Save/Restore PLL status across PHY reset') to
> support persisting across the very first DSI PHY enable at
> boot.
> 
> The bootloader may have left the PLL registers in a non-default
> state. For example, for dsi_pll_28nm.c on 8x16/8x39, the byte
> clock mux's power-on reset configuration is to bypass DIV1, but
> depending on bandwidth requirements[1] the bootloader may have
> set the DIV1 path.
> 
> When the byte clock mux is registered with the generic clock
> framework at probe time, the framework reads & caches the value
> of the mux bit field (the initial clock parent). After PHY enable,
> when clk_set_rate is called on the byte clock, the framework
> assumes there is no need to reparent, and doesn't re-write the
> mux bit field. But PHY enable resets PLL registers, so the mux
> bit field actually silently reverted to the DIV1 bypass path.
> This causes the byte clock to be off by a factor of e.g. 2 for
> our tested WXGA panel.
> 
> The above issue manifests as the display not working and a
> constant stream of FIFO/LP0 contention errors.
> 
> [1] The specific requirement for triggering the DIV1 path (and
> thus this issue) on 28nm is a panel with pixel clock <116.7MHz
> (one-third the minimum VCO setting). FHD/1080p (~145MHz) is fine,
> WXGA/1280x800 (~75MHz) is not.

This patch seems to be still relevant. Applying it would allow us to 
drop respective save_state calls from 10nm and 7nm drivers.

I'd suggest applying it.

> 
> Signed-off-by: Benjamin Li <benl@squareup.com>
> ---
>   drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> index 009f5b843dd1..139b4a5aaf86 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> @@ -621,6 +621,22 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
>   		phy->pll = NULL;
>   	}
>   
> +	/*
> +	 * As explained in msm_dsi_phy_enable, resetting the DSI PHY (as done
> +	 * in dsi_mgr_phy_enable) silently changes its PLL registers to power-on
> +	 * defaults, but the generic clock framework manages and caches several
> +	 * of the PLL registers. It initializes these caches at registration
> +	 * time via register read.
> +	 *
> +	 * As a result, we need to save DSI PLL registers once at probe in order
> +	 * for the first call to msm_dsi_phy_enable to successfully bring PLL
> +	 * registers back in line with what the generic clock framework expects.
> +	 *
> +	 * Subsequent PLL restores during msm_dsi_phy_enable will always be
> +	 * paired with PLL saves in msm_dsi_phy_disable.
> +	 */
> +	msm_dsi_pll_save_state(phy->pll);
> +
>   	dsi_phy_disable_resource(phy);
>   
>   	platform_set_drvdata(pdev, phy);
> 


-- 
With best wishes
Dmitry