[PATCH 0/5] Clock fixes for DSI 10nm PLL

[PATCH 0/5] Clock fixes for DSI 10nm PLL

[AngeloGioacchino Del Regno]

The DSI 10nm PLL driver was apparently ported from downstream, but some
of its "features" were not ported over, for a good reason.
Pity is that the removal of the downstream dependencies broke the clock
calculation logic for this driver and that made it impossible to use any
DSI display on at least MSM8998.

This patch series fixes the calculation issues and also solves some TODOs
that I've found in this driver.

Tested on:
- Sony Xperia XZ Premium (MSM8998) dual-dsi command-mode LCD display
- F(x)Tec Pro1 (MSM8998) single dsi, video-mode OLED display

AngeloGioacchino Del Regno (5):
  drm/msm/dsi_pll_10nm: Fix dividing the same numbers twice
  drm/msm/dsi_pll_10nm: Solve TODO for multiplier frac_bits assignment
  drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler
  drm/msm/dsi_pll_10nm: Fix variable usage for pll_lockdet_rate
  drm/msm/dsi_pll_10nm: Convert pr_err prints to DRM_DEV_ERROR

 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 43 ++++++++++------------
 1 file changed, 20 insertions(+), 23 deletions(-)

-- 
2.29.2

[PATCH 1/5] drm/msm/dsi_pll_10nm: Fix dividing the same numbers twice

[AngeloGioacchino Del Regno]

In function dsi_pll_calc_dec_frac we are calculating the decimal
div start parameter by dividing the decimal multiple by the
fractional multiplier: the remainder of that operation is stored
to then get programmed to the fractional divider registers of
the PLL.

It's useless to call div_u64_rem to get the remainder and *then*
call div_u64 to get the division result, as the first is already
giving that result: let's fix it by just caring about the result
of div_u64_rem.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index 6ac04fc303f5..2c1fcf092ab8 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -172,9 +172,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
 
 	multiplier = 1 << config->frac_bits;
 	dec_multiple = div_u64(pll_freq * multiplier, divider);
-	div_u64_rem(dec_multiple, multiplier, &frac);
-
-	dec = div_u64(dec_multiple, multiplier);
+	dec = div_u64_rem(dec_multiple, multiplier, &frac);
 
 	if (pll_freq <= 1900000000UL)
 		regs->pll_prop_gain_rate = 8;
-- 
2.29.2

[PATCH 2/5] drm/msm/dsi_pll_10nm: Solve TODO for multiplier frac_bits assignment

[AngeloGioacchino Del Regno]

The number of fractional registers bits is known and already set in
the frac_bits variable of the dsi_pll_config struct here in 10nm:
remove the TODO by simply using that variable.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index 2c1fcf092ab8..8b66e852eb36 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -481,6 +481,7 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
 {
 	struct msm_dsi_pll *pll = hw_clk_to_pll(hw);
 	struct dsi_pll_10nm *pll_10nm = to_pll_10nm(pll);
+	struct dsi_pll_config *config = &pll_10nm->pll_configuration;
 	void __iomem *base = pll_10nm->mmio;
 	u64 ref_clk = pll_10nm->vco_ref_clk_rate;
 	u64 vco_rate = 0x0;
@@ -501,9 +502,8 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
 	/*
 	 * TODO:
 	 *	1. Assumes prescaler is disabled
-	 *	2. Multiplier is 2^18. it should be 2^(num_of_frac_bits)
 	 */
-	multiplier = 1 << 18;
+	multiplier = 1 << config->frac_bits;
 	pll_freq = dec * (ref_clk * 2);
 	tmp64 = (ref_clk * 2 * frac);
 	pll_freq += div_u64(tmp64, multiplier);
-- 
2.29.2

[PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[AngeloGioacchino Del Regno]

The VCO rate was being miscalculated due to a big overlook during
the process of porting this driver from downstream to upstream:
here we are really recalculating the rate of the VCO by reading
the appropriate registers and returning a real frequency, while
downstream the driver was doing something entirely different.

In our case here, the recalculated rate was wrong, as it was then
given back to the set_rate function, which was erroneously doing
a division on the fractional value, based on the prescaler being
either enabled or disabled: this was actually producing a bug for
which the final VCO rate was being doubled, causing very obvious
issues when trying to drive a DSI panel because the actual divider
value was multiplied by two!

To make things work properly, remove the multiplication of the
reference clock by two from function dsi_pll_calc_dec_frac and
account for the prescaler enablement in the vco_recalc_rate (if
the prescaler is enabled, then the hardware will divide the rate
by two).

This will make the vco_recalc_rate function to pass the right
frequency to the (clock framework) set_rate function when called,
which will - in turn - program the right values in both the
DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
registers, finally making the PLL to output the right clock.

Also, while at it, remove the prescaler TODO by also adding the
possibility of disabling the prescaler on the PLL (it is in the
PLL_ANALOG_CONTROLS_ONE register).
Of course, both prescaler-ON and OFF cases were tested.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index 8b66e852eb36..5be562dfbf06 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
 
 	pll_freq = pll->vco_current_rate;
 
-	if (config->disable_prescaler)
-		divider = fref;
-	else
-		divider = fref * 2;
-
+	divider = fref;
 	multiplier = 1 << config->frac_bits;
 	dec_multiple = div_u64(pll_freq * multiplier, divider);
 	dec = div_u64_rem(dec_multiple, multiplier, &frac);
@@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
 
 static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
 {
+	struct dsi_pll_config *config = &pll->pll_configuration;
 	void __iomem *base = pll->mmio;
+	u32 val = config->disable_prescaler ? 0x0 : 0x80;
 
-	pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
+	pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
@@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
 	frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
 		  0x3) << 16);
 
-	/*
-	 * TODO:
-	 *	1. Assumes prescaler is disabled
-	 */
 	multiplier = 1 << config->frac_bits;
-	pll_freq = dec * (ref_clk * 2);
-	tmp64 = (ref_clk * 2 * frac);
+	pll_freq = dec * ref_clk;
+	tmp64 = ref_clk * frac;
 	pll_freq += div_u64(tmp64, multiplier);
-
 	vco_rate = pll_freq;
 
+	if (config->disable_prescaler)
+		vco_rate = div_u64(vco_rate, 2);
+
 	DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
 	    pll_10nm->id, (unsigned long)vco_rate, dec, frac);
 
-- 
2.29.2

[PATCH 4/5] drm/msm/dsi_pll_10nm: Fix variable usage for pll_lockdet_rate

[AngeloGioacchino Del Regno]

The PLL_LOCKDET_RATE_1 was being programmed with a hardcoded value
directly, but the same value was also being specified in the
dsi_pll_regs struct pll_lockdet_rate variable: let's use it!

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index 5be562dfbf06..df3e4584dfd7 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -302,7 +302,8 @@ static void dsi_pll_commit(struct dsi_pll_10nm *pll)
 		  reg->frac_div_start_mid);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1,
 		  reg->frac_div_start_high);
-	pll_write(base + REG_DSI_10nm_PHY_PLL_PLL_LOCKDET_RATE_1, 0x40);
+	pll_write(base + REG_DSI_10nm_PHY_PLL_PLL_LOCKDET_RATE_1,
+		  reg->pll_lockdet_rate);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_PLL_LOCK_DELAY, 0x06);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_CMODE, 0x10);
 	pll_write(base + REG_DSI_10nm_PHY_PLL_CLOCK_INVERTERS,
-- 
2.29.2

[PATCH 5/5] drm/msm/dsi_pll_10nm: Convert pr_err prints to DRM_DEV_ERROR

[AngeloGioacchino Del Regno]

DRM_DEV_ERROR should be used across this entire source: convert the
pr_err prints to the first as a cleanup.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
index df3e4584dfd7..f1091c023836 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
@@ -342,6 +342,7 @@ static int dsi_pll_10nm_vco_set_rate(struct clk_hw *hw, unsigned long rate,
 
 static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
 {
+	struct device *dev = &pll->pdev->dev;
 	int rc;
 	u32 status = 0;
 	u32 const delay_us = 100;
@@ -354,8 +355,8 @@ static int dsi_pll_10nm_lock_status(struct dsi_pll_10nm *pll)
 				       delay_us,
 				       timeout_us);
 	if (rc)
-		pr_err("DSI PLL(%d) lock failed, status=0x%08x\n",
-		       pll->id, status);
+		DRM_DEV_ERROR(dev, "DSI PLL(%d) lock failed, status=0x%08x\n",
+			      pll->id, status);
 
 	return rc;
 }
@@ -402,6 +403,7 @@ static int dsi_pll_10nm_vco_prepare(struct clk_hw *hw)
 {
 	struct msm_dsi_pll *pll = hw_clk_to_pll(hw);
 	struct dsi_pll_10nm *pll_10nm = to_pll_10nm(pll);
+	struct device *dev = &pll_10nm->pdev->dev;
 	int rc;
 
 	dsi_pll_enable_pll_bias(pll_10nm);
@@ -410,7 +412,7 @@ static int dsi_pll_10nm_vco_prepare(struct clk_hw *hw)
 
 	rc = dsi_pll_10nm_vco_set_rate(hw,pll_10nm->vco_current_rate, 0);
 	if (rc) {
-		pr_err("vco_set_rate failed, rc=%d\n", rc);
+		DRM_DEV_ERROR(dev, "vco_set_rate failed, rc=%d\n", rc);
 		return rc;
 	}
 
@@ -427,7 +429,7 @@ static int dsi_pll_10nm_vco_prepare(struct clk_hw *hw)
 	/* Check for PLL lock */
 	rc = dsi_pll_10nm_lock_status(pll_10nm);
 	if (rc) {
-		pr_err("PLL(%d) lock failed\n", pll_10nm->id);
+		DRM_DEV_ERROR(dev, "PLL(%d) lock failed\n", pll_10nm->id);
 		goto error;
 	}
 
-- 
2.29.2

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> The VCO rate was being miscalculated due to a big overlook during
> the process of porting this driver from downstream to upstream:
> here we are really recalculating the rate of the VCO by reading
> the appropriate registers and returning a real frequency, while
> downstream the driver was doing something entirely different.
>
> In our case here, the recalculated rate was wrong, as it was then
> given back to the set_rate function, which was erroneously doing
> a division on the fractional value, based on the prescaler being
> either enabled or disabled: this was actually producing a bug for
> which the final VCO rate was being doubled, causing very obvious
> issues when trying to drive a DSI panel because the actual divider
> value was multiplied by two!
>
> To make things work properly, remove the multiplication of the
> reference clock by two from function dsi_pll_calc_dec_frac and
> account for the prescaler enablement in the vco_recalc_rate (if
> the prescaler is enabled, then the hardware will divide the rate
> by two).
>
> This will make the vco_recalc_rate function to pass the right
> frequency to the (clock framework) set_rate function when called,
> which will - in turn - program the right values in both the
> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> registers, finally making the PLL to output the right clock.
>
> Also, while at it, remove the prescaler TODO by also adding the
> possibility of disabling the prescaler on the PLL (it is in the
> PLL_ANALOG_CONTROLS_ONE register).
> Of course, both prescaler-ON and OFF cases were tested.

This somehow breaks things on sc7180 (display gets stuck at first
frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
bridge)

Also, something (I assume DSI related) that I was testing on
msm-next-staging seems to have effected the colors on the panel (ie.
they are more muted).. which seems to persist across reboots (ie. when
switching back to a good kernel), and interestingly if I reboot from a
good kernel I see part of the login prompt (or whatever was previously
on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
the display to think it is in PSR mode??)

Not sure if that is caused by these patches, but if I can figure out
how to get the panel back to normal I can bisect.  I think for now
I'll drop this series.  Possibly it could be a
two-wrongs-makes-a-right situation that had things working before, but
I think someone from qcom who knows the DSI IP should take a look.

BR,
-R


> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> index 8b66e852eb36..5be562dfbf06 100644
> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
>
>         pll_freq = pll->vco_current_rate;
>
> -       if (config->disable_prescaler)
> -               divider = fref;
> -       else
> -               divider = fref * 2;
> -
> +       divider = fref;
>         multiplier = 1 << config->frac_bits;
>         dec_multiple = div_u64(pll_freq * multiplier, divider);
>         dec = div_u64_rem(dec_multiple, multiplier, &frac);
> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
>
>  static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
>  {
> +       struct dsi_pll_config *config = &pll->pll_configuration;
>         void __iomem *base = pll->mmio;
> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
>
> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
>         pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
>         pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
>         pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
>         frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
>                   0x3) << 16);
>
> -       /*
> -        * TODO:
> -        *      1. Assumes prescaler is disabled
> -        */
>         multiplier = 1 << config->frac_bits;
> -       pll_freq = dec * (ref_clk * 2);
> -       tmp64 = (ref_clk * 2 * frac);
> +       pll_freq = dec * ref_clk;
> +       tmp64 = ref_clk * frac;
>         pll_freq += div_u64(tmp64, multiplier);
> -
>         vco_rate = pll_freq;
>
> +       if (config->disable_prescaler)
> +               vco_rate = div_u64(vco_rate, 2);
> +
>         DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
>             pll_10nm->id, (unsigned long)vco_rate, dec, frac);
>
> --
> 2.29.2
>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[AngeloGioacchino Del Regno]

Il 31/01/21 20:50, Rob Clark ha scritto:
> On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>>
>> The VCO rate was being miscalculated due to a big overlook during
>> the process of porting this driver from downstream to upstream:
>> here we are really recalculating the rate of the VCO by reading
>> the appropriate registers and returning a real frequency, while
>> downstream the driver was doing something entirely different.
>>
>> In our case here, the recalculated rate was wrong, as it was then
>> given back to the set_rate function, which was erroneously doing
>> a division on the fractional value, based on the prescaler being
>> either enabled or disabled: this was actually producing a bug for
>> which the final VCO rate was being doubled, causing very obvious
>> issues when trying to drive a DSI panel because the actual divider
>> value was multiplied by two!
>>
>> To make things work properly, remove the multiplication of the
>> reference clock by two from function dsi_pll_calc_dec_frac and
>> account for the prescaler enablement in the vco_recalc_rate (if
>> the prescaler is enabled, then the hardware will divide the rate
>> by two).
>>
>> This will make the vco_recalc_rate function to pass the right
>> frequency to the (clock framework) set_rate function when called,
>> which will - in turn - program the right values in both the
>> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
>> registers, finally making the PLL to output the right clock.
>>
>> Also, while at it, remove the prescaler TODO by also adding the
>> possibility of disabling the prescaler on the PLL (it is in the
>> PLL_ANALOG_CONTROLS_ONE register).
>> Of course, both prescaler-ON and OFF cases were tested.
> 
> This somehow breaks things on sc7180 (display gets stuck at first
> frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> bridge)
> 

First frame of the splash means that something is "a bit" wrong...
...like the DSI clock is a little off.

I don't have such hardware, otherwise I would've tried... but what you
describe is a bit strange.
Is there any other older qcom platform using this chip? Any other
non-qcom platform? Is the driver for the SN65DSI86 surely fine?
Anyway, as you know, I would never propose untested patches nor
partially working ones for any reason: I'm sorry that this happened.

In any case, just to be perfectly transparent, while being here waiting
for review, this patch series got tested on more smartphones, even ones
that I don't personally own, with different displays.

For your reference, here's a list (all MSM8998..):
- OnePlus 5               (1920x1080)
- F(x)Tec Pro 1           (2160x1080)
- Sony Xperia XZ1 Compact (1280x720)
- Sony Xperia XZ1         (1920x1080)
- Sony Xperia XZ Premium  (3840x2160)


> Also, something (I assume DSI related) that I was testing on
> msm-next-staging seems to have effected the colors on the panel (ie.
> they are more muted).. which seems to persist across reboots (ie. when

So much "fun". This makes me think something about the PCC block doing
the wrong thing (getting misconfigured).

> switching back to a good kernel), and interestingly if I reboot from a
> good kernel I see part of the login prompt (or whatever was previously
> on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> the display to think it is in PSR mode??)
> 

 From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
cannot produce (logically, at least) this, so I say that it is very
unlikely for this to be a consequence of the 10nm pll fixes...

...unless the bootloader is not configuring the DSI rates, but that's
also veeeeery unlikely (it always does, or it always does not).

> Not sure if that is caused by these patches, but if I can figure out
> how to get the panel back to normal I can bisect.  I think for now
> I'll drop this series.  Possibly it could be a
> two-wrongs-makes-a-right situation that had things working before, but
> I think someone from qcom who knows the DSI IP should take a look.
> 

I would be happy if someone from Qualcomm takes a look: after all, there
is no documentation and they're the only ones that can verify this kind
of stuff. Please, Qualcomm.

Besides that, if there's anything I can help with to solve this riddle,
I'm here for you.

Yours,
-- Angelo

> BR,
> -R
> 
> 
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
>>   1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>> index 8b66e852eb36..5be562dfbf06 100644
>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
>>
>>          pll_freq = pll->vco_current_rate;
>>
>> -       if (config->disable_prescaler)
>> -               divider = fref;
>> -       else
>> -               divider = fref * 2;
>> -
>> +       divider = fref;
>>          multiplier = 1 << config->frac_bits;
>>          dec_multiple = div_u64(pll_freq * multiplier, divider);
>>          dec = div_u64_rem(dec_multiple, multiplier, &frac);
>> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
>>
>>   static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
>>   {
>> +       struct dsi_pll_config *config = &pll->pll_configuration;
>>          void __iomem *base = pll->mmio;
>> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
>>
>> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
>> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
>>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
>>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
>>          pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
>> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
>>          frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
>>                    0x3) << 16);
>>
>> -       /*
>> -        * TODO:
>> -        *      1. Assumes prescaler is disabled
>> -        */
>>          multiplier = 1 << config->frac_bits;
>> -       pll_freq = dec * (ref_clk * 2);
>> -       tmp64 = (ref_clk * 2 * frac);
>> +       pll_freq = dec * ref_clk;
>> +       tmp64 = ref_clk * frac;
>>          pll_freq += div_u64(tmp64, multiplier);
>> -
>>          vco_rate = pll_freq;
>>
>> +       if (config->disable_prescaler)
>> +               vco_rate = div_u64(vco_rate, 2);
>> +
>>          DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
>>              pll_10nm->id, (unsigned long)vco_rate, dec, frac);
>>
>> --
>> 2.29.2
>>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 31/01/21 20:50, Rob Clark ha scritto:
> > On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@somainline.org> wrote:
> >>
> >> The VCO rate was being miscalculated due to a big overlook during
> >> the process of porting this driver from downstream to upstream:
> >> here we are really recalculating the rate of the VCO by reading
> >> the appropriate registers and returning a real frequency, while
> >> downstream the driver was doing something entirely different.
> >>
> >> In our case here, the recalculated rate was wrong, as it was then
> >> given back to the set_rate function, which was erroneously doing
> >> a division on the fractional value, based on the prescaler being
> >> either enabled or disabled: this was actually producing a bug for
> >> which the final VCO rate was being doubled, causing very obvious
> >> issues when trying to drive a DSI panel because the actual divider
> >> value was multiplied by two!
> >>
> >> To make things work properly, remove the multiplication of the
> >> reference clock by two from function dsi_pll_calc_dec_frac and
> >> account for the prescaler enablement in the vco_recalc_rate (if
> >> the prescaler is enabled, then the hardware will divide the rate
> >> by two).
> >>
> >> This will make the vco_recalc_rate function to pass the right
> >> frequency to the (clock framework) set_rate function when called,
> >> which will - in turn - program the right values in both the
> >> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> >> registers, finally making the PLL to output the right clock.
> >>
> >> Also, while at it, remove the prescaler TODO by also adding the
> >> possibility of disabling the prescaler on the PLL (it is in the
> >> PLL_ANALOG_CONTROLS_ONE register).
> >> Of course, both prescaler-ON and OFF cases were tested.
> >
> > This somehow breaks things on sc7180 (display gets stuck at first
> > frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> > bridge)
> >
>
> First frame of the splash means that something is "a bit" wrong...
> ...like the DSI clock is a little off.
>
> I don't have such hardware, otherwise I would've tried... but what you
> describe is a bit strange.
> Is there any other older qcom platform using this chip? Any other
> non-qcom platform? Is the driver for the SN65DSI86 surely fine?
> Anyway, as you know, I would never propose untested patches nor
> partially working ones for any reason: I'm sorry that this happened.

I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)

The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
the snapdragon windows laptops).. and I think also the older 835
laptops.. ofc that doesn't mean that there isn't some bug, but I'd
guess maybe more likely that there is some small difference in DSI vs
older devices, or some cmd vs video mode difference.

Anyways, seems like the screen did eventually recover so that gives me
a bit of confidence to bisect this series, which I'll do a bit later
today.

> In any case, just to be perfectly transparent, while being here waiting
> for review, this patch series got tested on more smartphones, even ones
> that I don't personally own, with different displays.
>
> For your reference, here's a list (all MSM8998..):
> - OnePlus 5               (1920x1080)
> - F(x)Tec Pro 1           (2160x1080)
> - Sony Xperia XZ1 Compact (1280x720)
> - Sony Xperia XZ1         (1920x1080)
> - Sony Xperia XZ Premium  (3840x2160)
>

Yeah, no worries, I wasn't trying to imply that the patch was untested.

Out of curiosity, are any of those video mode panels?

>
> > Also, something (I assume DSI related) that I was testing on
> > msm-next-staging seems to have effected the colors on the panel (ie.
> > they are more muted).. which seems to persist across reboots (ie. when
>
> So much "fun". This makes me think something about the PCC block doing
> the wrong thing (getting misconfigured).
>
> > switching back to a good kernel), and interestingly if I reboot from a
> > good kernel I see part of the login prompt (or whatever was previously
> > on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> > the display to think it is in PSR mode??)
> >
>
>  From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
> cannot produce (logically, at least) this, so I say that it is very
> unlikely for this to be a consequence of the 10nm pll fixes...
>

Note that the bridge can also be programmed via dsi cmd mode packets,
which I believe is the case on the 835 laptops (or at least one of
them).. but all the things I have are using i2c as the control path.

> ...unless the bootloader is not configuring the DSI rates, but that's
> also veeeeery unlikely (it always does, or it always does not).

I haven't looked at the bootloader display code, but booting back to
an old/good kernel didn't change anything..  even powering off didn't.
But the ghost image seemed to fade after some time, and by the next
morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
who works on display only when necessary.. ie. a gpu without a display
isn't so much fun ;-))

> > Not sure if that is caused by these patches, but if I can figure out
> > how to get the panel back to normal I can bisect.  I think for now
> > I'll drop this series.  Possibly it could be a
> > two-wrongs-makes-a-right situation that had things working before, but
> > I think someone from qcom who knows the DSI IP should take a look.
> >
>
> I would be happy if someone from Qualcomm takes a look: after all, there
> is no documentation and they're the only ones that can verify this kind
> of stuff. Please, Qualcomm.

Hopefully someone can take a look.

> Besides that, if there's anything I can help with to solve this riddle,
> I'm here for you.

Thanks, like I said I'll try applying the patches one by one and see
if I can narrow down what made the panel go funny, and we can go from
there

BR,
-R

> Yours,
> -- Angelo
>
> > BR,
> > -R
> >
> >
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> >>   1 file changed, 9 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >> index 8b66e852eb36..5be562dfbf06 100644
> >> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
> >>
> >>          pll_freq = pll->vco_current_rate;
> >>
> >> -       if (config->disable_prescaler)
> >> -               divider = fref;
> >> -       else
> >> -               divider = fref * 2;
> >> -
> >> +       divider = fref;
> >>          multiplier = 1 << config->frac_bits;
> >>          dec_multiple = div_u64(pll_freq * multiplier, divider);
> >>          dec = div_u64_rem(dec_multiple, multiplier, &frac);
> >> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
> >>
> >>   static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> >>   {
> >> +       struct dsi_pll_config *config = &pll->pll_configuration;
> >>          void __iomem *base = pll->mmio;
> >> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
> >>
> >> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> >> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> >>          pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> >> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> >>          frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> >>                    0x3) << 16);
> >>
> >> -       /*
> >> -        * TODO:
> >> -        *      1. Assumes prescaler is disabled
> >> -        */
> >>          multiplier = 1 << config->frac_bits;
> >> -       pll_freq = dec * (ref_clk * 2);
> >> -       tmp64 = (ref_clk * 2 * frac);
> >> +       pll_freq = dec * ref_clk;
> >> +       tmp64 = ref_clk * frac;
> >>          pll_freq += div_u64(tmp64, multiplier);
> >> -
> >>          vco_rate = pll_freq;
> >>
> >> +       if (config->disable_prescaler)
> >> +               vco_rate = div_u64(vco_rate, 2);
> >> +
> >>          DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> >>              pll_10nm->id, (unsigned long)vco_rate, dec, frac);
> >>
> >> --
> >> 2.29.2
> >>
>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
> >
> > Il 31/01/21 20:50, Rob Clark ha scritto:
> > > On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@somainline.org> wrote:
> > >>
> > >> The VCO rate was being miscalculated due to a big overlook during
> > >> the process of porting this driver from downstream to upstream:
> > >> here we are really recalculating the rate of the VCO by reading
> > >> the appropriate registers and returning a real frequency, while
> > >> downstream the driver was doing something entirely different.
> > >>
> > >> In our case here, the recalculated rate was wrong, as it was then
> > >> given back to the set_rate function, which was erroneously doing
> > >> a division on the fractional value, based on the prescaler being
> > >> either enabled or disabled: this was actually producing a bug for
> > >> which the final VCO rate was being doubled, causing very obvious
> > >> issues when trying to drive a DSI panel because the actual divider
> > >> value was multiplied by two!
> > >>
> > >> To make things work properly, remove the multiplication of the
> > >> reference clock by two from function dsi_pll_calc_dec_frac and
> > >> account for the prescaler enablement in the vco_recalc_rate (if
> > >> the prescaler is enabled, then the hardware will divide the rate
> > >> by two).
> > >>
> > >> This will make the vco_recalc_rate function to pass the right
> > >> frequency to the (clock framework) set_rate function when called,
> > >> which will - in turn - program the right values in both the
> > >> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> > >> registers, finally making the PLL to output the right clock.
> > >>
> > >> Also, while at it, remove the prescaler TODO by also adding the
> > >> possibility of disabling the prescaler on the PLL (it is in the
> > >> PLL_ANALOG_CONTROLS_ONE register).
> > >> Of course, both prescaler-ON and OFF cases were tested.
> > >
> > > This somehow breaks things on sc7180 (display gets stuck at first
> > > frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> > > bridge)
> > >
> >
> > First frame of the splash means that something is "a bit" wrong...
> > ...like the DSI clock is a little off.
> >
> > I don't have such hardware, otherwise I would've tried... but what you
> > describe is a bit strange.
> > Is there any other older qcom platform using this chip? Any other
> > non-qcom platform? Is the driver for the SN65DSI86 surely fine?
> > Anyway, as you know, I would never propose untested patches nor
> > partially working ones for any reason: I'm sorry that this happened.
>
> I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
>
> The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
> the snapdragon windows laptops).. and I think also the older 835
> laptops.. ofc that doesn't mean that there isn't some bug, but I'd
> guess maybe more likely that there is some small difference in DSI vs
> older devices, or some cmd vs video mode difference.
>
> Anyways, seems like the screen did eventually recover so that gives me
> a bit of confidence to bisect this series, which I'll do a bit later
> today.

fwiw, this series minus this patch, and everything looks ok.. let me
take a closer look at what changes with this patch

BR,
-R

> > In any case, just to be perfectly transparent, while being here waiting
> > for review, this patch series got tested on more smartphones, even ones
> > that I don't personally own, with different displays.
> >
> > For your reference, here's a list (all MSM8998..):
> > - OnePlus 5               (1920x1080)
> > - F(x)Tec Pro 1           (2160x1080)
> > - Sony Xperia XZ1 Compact (1280x720)
> > - Sony Xperia XZ1         (1920x1080)
> > - Sony Xperia XZ Premium  (3840x2160)
> >
>
> Yeah, no worries, I wasn't trying to imply that the patch was untested.
>
> Out of curiosity, are any of those video mode panels?
>
> >
> > > Also, something (I assume DSI related) that I was testing on
> > > msm-next-staging seems to have effected the colors on the panel (ie.
> > > they are more muted).. which seems to persist across reboots (ie. when
> >
> > So much "fun". This makes me think something about the PCC block doing
> > the wrong thing (getting misconfigured).
> >
> > > switching back to a good kernel), and interestingly if I reboot from a
> > > good kernel I see part of the login prompt (or whatever was previously
> > > on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> > > the display to think it is in PSR mode??)
> > >
> >
> >  From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
> > cannot produce (logically, at least) this, so I say that it is very
> > unlikely for this to be a consequence of the 10nm pll fixes...
> >
>
> Note that the bridge can also be programmed via dsi cmd mode packets,
> which I believe is the case on the 835 laptops (or at least one of
> them).. but all the things I have are using i2c as the control path.
>
> > ...unless the bootloader is not configuring the DSI rates, but that's
> > also veeeeery unlikely (it always does, or it always does not).
>
> I haven't looked at the bootloader display code, but booting back to
> an old/good kernel didn't change anything..  even powering off didn't.
> But the ghost image seemed to fade after some time, and by the next
> morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
> who works on display only when necessary.. ie. a gpu without a display
> isn't so much fun ;-))
>
> > > Not sure if that is caused by these patches, but if I can figure out
> > > how to get the panel back to normal I can bisect.  I think for now
> > > I'll drop this series.  Possibly it could be a
> > > two-wrongs-makes-a-right situation that had things working before, but
> > > I think someone from qcom who knows the DSI IP should take a look.
> > >
> >
> > I would be happy if someone from Qualcomm takes a look: after all, there
> > is no documentation and they're the only ones that can verify this kind
> > of stuff. Please, Qualcomm.
>
> Hopefully someone can take a look.
>
> > Besides that, if there's anything I can help with to solve this riddle,
> > I'm here for you.
>
> Thanks, like I said I'll try applying the patches one by one and see
> if I can narrow down what made the panel go funny, and we can go from
> there
>
> BR,
> -R
>
> > Yours,
> > -- Angelo
> >
> > > BR,
> > > -R
> > >
> > >
> > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > >> ---
> > >>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> > >>   1 file changed, 9 insertions(+), 13 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > >> index 8b66e852eb36..5be562dfbf06 100644
> > >> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > >> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > >> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
> > >>
> > >>          pll_freq = pll->vco_current_rate;
> > >>
> > >> -       if (config->disable_prescaler)
> > >> -               divider = fref;
> > >> -       else
> > >> -               divider = fref * 2;
> > >> -
> > >> +       divider = fref;
> > >>          multiplier = 1 << config->frac_bits;
> > >>          dec_multiple = div_u64(pll_freq * multiplier, divider);
> > >>          dec = div_u64_rem(dec_multiple, multiplier, &frac);
> > >> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
> > >>
> > >>   static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> > >>   {
> > >> +       struct dsi_pll_config *config = &pll->pll_configuration;
> > >>          void __iomem *base = pll->mmio;
> > >> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
> > >>
> > >> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> > >> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> > >> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> > >>          frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> > >>                    0x3) << 16);
> > >>
> > >> -       /*
> > >> -        * TODO:
> > >> -        *      1. Assumes prescaler is disabled
> > >> -        */
> > >>          multiplier = 1 << config->frac_bits;
> > >> -       pll_freq = dec * (ref_clk * 2);
> > >> -       tmp64 = (ref_clk * 2 * frac);
> > >> +       pll_freq = dec * ref_clk;
> > >> +       tmp64 = ref_clk * frac;
> > >>          pll_freq += div_u64(tmp64, multiplier);
> > >> -
> > >>          vco_rate = pll_freq;
> > >>
> > >> +       if (config->disable_prescaler)
> > >> +               vco_rate = div_u64(vco_rate, 2);
> > >> +
> > >>          DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> > >>              pll_10nm->id, (unsigned long)vco_rate, dec, frac);
> > >>
> > >> --
> > >> 2.29.2
> > >>
> >

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@somainline.org> wrote:
> > >
> > > Il 31/01/21 20:50, Rob Clark ha scritto:
> > > > On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> > > > <angelogioacchino.delregno@somainline.org> wrote:
> > > >>
> > > >> The VCO rate was being miscalculated due to a big overlook during
> > > >> the process of porting this driver from downstream to upstream:
> > > >> here we are really recalculating the rate of the VCO by reading
> > > >> the appropriate registers and returning a real frequency, while
> > > >> downstream the driver was doing something entirely different.
> > > >>
> > > >> In our case here, the recalculated rate was wrong, as it was then
> > > >> given back to the set_rate function, which was erroneously doing
> > > >> a division on the fractional value, based on the prescaler being
> > > >> either enabled or disabled: this was actually producing a bug for
> > > >> which the final VCO rate was being doubled, causing very obvious
> > > >> issues when trying to drive a DSI panel because the actual divider
> > > >> value was multiplied by two!
> > > >>
> > > >> To make things work properly, remove the multiplication of the
> > > >> reference clock by two from function dsi_pll_calc_dec_frac and
> > > >> account for the prescaler enablement in the vco_recalc_rate (if
> > > >> the prescaler is enabled, then the hardware will divide the rate
> > > >> by two).
> > > >>
> > > >> This will make the vco_recalc_rate function to pass the right
> > > >> frequency to the (clock framework) set_rate function when called,
> > > >> which will - in turn - program the right values in both the
> > > >> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> > > >> registers, finally making the PLL to output the right clock.
> > > >>
> > > >> Also, while at it, remove the prescaler TODO by also adding the
> > > >> possibility of disabling the prescaler on the PLL (it is in the
> > > >> PLL_ANALOG_CONTROLS_ONE register).
> > > >> Of course, both prescaler-ON and OFF cases were tested.
> > > >
> > > > This somehow breaks things on sc7180 (display gets stuck at first
> > > > frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> > > > bridge)
> > > >
> > >
> > > First frame of the splash means that something is "a bit" wrong...
> > > ...like the DSI clock is a little off.
> > >
> > > I don't have such hardware, otherwise I would've tried... but what you
> > > describe is a bit strange.
> > > Is there any other older qcom platform using this chip? Any other
> > > non-qcom platform? Is the driver for the SN65DSI86 surely fine?
> > > Anyway, as you know, I would never propose untested patches nor
> > > partially working ones for any reason: I'm sorry that this happened.
> >
> > I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
> >
> > The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
> > the snapdragon windows laptops).. and I think also the older 835
> > laptops.. ofc that doesn't mean that there isn't some bug, but I'd
> > guess maybe more likely that there is some small difference in DSI vs
> > older devices, or some cmd vs video mode difference.
> >
> > Anyways, seems like the screen did eventually recover so that gives me
> > a bit of confidence to bisect this series, which I'll do a bit later
> > today.
>
> fwiw, this series minus this patch, and everything looks ok.. let me
> take a closer look at what changes with this patch

Btw, it looks like upstream, config->disable_prescaler is always
false.. I don't suppose you have anything WIP that changes this?

BR,
-R

>
> > > In any case, just to be perfectly transparent, while being here waiting
> > > for review, this patch series got tested on more smartphones, even ones
> > > that I don't personally own, with different displays.
> > >
> > > For your reference, here's a list (all MSM8998..):
> > > - OnePlus 5               (1920x1080)
> > > - F(x)Tec Pro 1           (2160x1080)
> > > - Sony Xperia XZ1 Compact (1280x720)
> > > - Sony Xperia XZ1         (1920x1080)
> > > - Sony Xperia XZ Premium  (3840x2160)
> > >
> >
> > Yeah, no worries, I wasn't trying to imply that the patch was untested.
> >
> > Out of curiosity, are any of those video mode panels?
> >
> > >
> > > > Also, something (I assume DSI related) that I was testing on
> > > > msm-next-staging seems to have effected the colors on the panel (ie.
> > > > they are more muted).. which seems to persist across reboots (ie. when
> > >
> > > So much "fun". This makes me think something about the PCC block doing
> > > the wrong thing (getting misconfigured).
> > >
> > > > switching back to a good kernel), and interestingly if I reboot from a
> > > > good kernel I see part of the login prompt (or whatever was previously
> > > > on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> > > > the display to think it is in PSR mode??)
> > > >
> > >
> > >  From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
> > > cannot produce (logically, at least) this, so I say that it is very
> > > unlikely for this to be a consequence of the 10nm pll fixes...
> > >
> >
> > Note that the bridge can also be programmed via dsi cmd mode packets,
> > which I believe is the case on the 835 laptops (or at least one of
> > them).. but all the things I have are using i2c as the control path.
> >
> > > ...unless the bootloader is not configuring the DSI rates, but that's
> > > also veeeeery unlikely (it always does, or it always does not).
> >
> > I haven't looked at the bootloader display code, but booting back to
> > an old/good kernel didn't change anything..  even powering off didn't.
> > But the ghost image seemed to fade after some time, and by the next
> > morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
> > who works on display only when necessary.. ie. a gpu without a display
> > isn't so much fun ;-))
> >
> > > > Not sure if that is caused by these patches, but if I can figure out
> > > > how to get the panel back to normal I can bisect.  I think for now
> > > > I'll drop this series.  Possibly it could be a
> > > > two-wrongs-makes-a-right situation that had things working before, but
> > > > I think someone from qcom who knows the DSI IP should take a look.
> > > >
> > >
> > > I would be happy if someone from Qualcomm takes a look: after all, there
> > > is no documentation and they're the only ones that can verify this kind
> > > of stuff. Please, Qualcomm.
> >
> > Hopefully someone can take a look.
> >
> > > Besides that, if there's anything I can help with to solve this riddle,
> > > I'm here for you.
> >
> > Thanks, like I said I'll try applying the patches one by one and see
> > if I can narrow down what made the panel go funny, and we can go from
> > there
> >
> > BR,
> > -R
> >
> > > Yours,
> > > -- Angelo
> > >
> > > > BR,
> > > > -R
> > > >
> > > >
> > > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > >> ---
> > > >>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> > > >>   1 file changed, 9 insertions(+), 13 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > > >> index 8b66e852eb36..5be562dfbf06 100644
> > > >> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > > >> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > > >> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
> > > >>
> > > >>          pll_freq = pll->vco_current_rate;
> > > >>
> > > >> -       if (config->disable_prescaler)
> > > >> -               divider = fref;
> > > >> -       else
> > > >> -               divider = fref * 2;
> > > >> -
> > > >> +       divider = fref;
> > > >>          multiplier = 1 << config->frac_bits;
> > > >>          dec_multiple = div_u64(pll_freq * multiplier, divider);
> > > >>          dec = div_u64_rem(dec_multiple, multiplier, &frac);
> > > >> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
> > > >>
> > > >>   static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> > > >>   {
> > > >> +       struct dsi_pll_config *config = &pll->pll_configuration;
> > > >>          void __iomem *base = pll->mmio;
> > > >> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
> > > >>
> > > >> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> > > >> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> > > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> > > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> > > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> > > >> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> > > >>          frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> > > >>                    0x3) << 16);
> > > >>
> > > >> -       /*
> > > >> -        * TODO:
> > > >> -        *      1. Assumes prescaler is disabled
> > > >> -        */
> > > >>          multiplier = 1 << config->frac_bits;
> > > >> -       pll_freq = dec * (ref_clk * 2);
> > > >> -       tmp64 = (ref_clk * 2 * frac);
> > > >> +       pll_freq = dec * ref_clk;
> > > >> +       tmp64 = ref_clk * frac;
> > > >>          pll_freq += div_u64(tmp64, multiplier);
> > > >> -
> > > >>          vco_rate = pll_freq;
> > > >>
> > > >> +       if (config->disable_prescaler)
> > > >> +               vco_rate = div_u64(vco_rate, 2);
> > > >> +
> > > >>          DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> > > >>              pll_10nm->id, (unsigned long)vco_rate, dec, frac);
> > > >>
> > > >> --
> > > >> 2.29.2
> > > >>
> > >

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Mon, Feb 1, 2021 at 9:18 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@somainline.org> wrote:
> > > >
> > > > Il 31/01/21 20:50, Rob Clark ha scritto:
> > > > > On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> > > > > <angelogioacchino.delregno@somainline.org> wrote:
> > > > >>
> > > > >> The VCO rate was being miscalculated due to a big overlook during
> > > > >> the process of porting this driver from downstream to upstream:
> > > > >> here we are really recalculating the rate of the VCO by reading
> > > > >> the appropriate registers and returning a real frequency, while
> > > > >> downstream the driver was doing something entirely different.
> > > > >>
> > > > >> In our case here, the recalculated rate was wrong, as it was then
> > > > >> given back to the set_rate function, which was erroneously doing
> > > > >> a division on the fractional value, based on the prescaler being
> > > > >> either enabled or disabled: this was actually producing a bug for
> > > > >> which the final VCO rate was being doubled, causing very obvious
> > > > >> issues when trying to drive a DSI panel because the actual divider
> > > > >> value was multiplied by two!
> > > > >>
> > > > >> To make things work properly, remove the multiplication of the
> > > > >> reference clock by two from function dsi_pll_calc_dec_frac and
> > > > >> account for the prescaler enablement in the vco_recalc_rate (if
> > > > >> the prescaler is enabled, then the hardware will divide the rate
> > > > >> by two).
> > > > >>
> > > > >> This will make the vco_recalc_rate function to pass the right
> > > > >> frequency to the (clock framework) set_rate function when called,
> > > > >> which will - in turn - program the right values in both the
> > > > >> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> > > > >> registers, finally making the PLL to output the right clock.
> > > > >>
> > > > >> Also, while at it, remove the prescaler TODO by also adding the
> > > > >> possibility of disabling the prescaler on the PLL (it is in the
> > > > >> PLL_ANALOG_CONTROLS_ONE register).
> > > > >> Of course, both prescaler-ON and OFF cases were tested.
> > > > >
> > > > > This somehow breaks things on sc7180 (display gets stuck at first
> > > > > frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> > > > > bridge)
> > > > >
> > > >
> > > > First frame of the splash means that something is "a bit" wrong...
> > > > ...like the DSI clock is a little off.
> > > >
> > > > I don't have such hardware, otherwise I would've tried... but what you
> > > > describe is a bit strange.
> > > > Is there any other older qcom platform using this chip? Any other
> > > > non-qcom platform? Is the driver for the SN65DSI86 surely fine?
> > > > Anyway, as you know, I would never propose untested patches nor
> > > > partially working ones for any reason: I'm sorry that this happened.
> > >
> > > I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
> > >
> > > The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
> > > the snapdragon windows laptops).. and I think also the older 835
> > > laptops.. ofc that doesn't mean that there isn't some bug, but I'd
> > > guess maybe more likely that there is some small difference in DSI vs
> > > older devices, or some cmd vs video mode difference.
> > >
> > > Anyways, seems like the screen did eventually recover so that gives me
> > > a bit of confidence to bisect this series, which I'll do a bit later
> > > today.
> >
> > fwiw, this series minus this patch, and everything looks ok.. let me
> > take a closer look at what changes with this patch
>
> Btw, it looks like upstream, config->disable_prescaler is always
> false.. I don't suppose you have anything WIP that changes this?

fwiw, this is the clk_summary diff with and without this patch:

------------------
270,282c270,282
<     dsi0_pll_out_div_clk              1        1        0
887039941          0     0  50000         Y
<        dsi0_pll_post_out_div_clk       0        0        0
221759985          0     0  50000         Y
<        dsi0_pll_bit_clk               2        2        0
887039941          0     0  50000         Y
<           dsi0_pclk_mux               1        1        0
887039941          0     0  50000         Y
<              dsi0_phy_pll_out_dsiclk       1        1        0
147839991          0     0  50000         Y
<                 disp_cc_mdss_pclk0_clk_src       1        1        0
  147839991          0     0  50000         Y
<                    disp_cc_mdss_pclk0_clk       1        1        0
 147839991          0     0  50000         Y
<           dsi0_pll_by_2_bit_clk       0        0        0
443519970          0     0  50000         Y
<           dsi0_phy_pll_out_byteclk       1        1        0
110879992          0     0  50000         Y
<              disp_cc_mdss_byte0_clk_src       2        2        0
110879992          0     0  50000         Y
<                 disp_cc_mdss_byte0_div_clk_src       1        1
  0    55439996          0     0  50000         Y
<                    disp_cc_mdss_byte0_intf_clk       1        1
  0    55439996          0     0  50000         Y
<                 disp_cc_mdss_byte0_clk       1        1        0
110879992          0     0  50000         Y
---
>     dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
>        dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
>        dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
>           dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
>              dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
>                 disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
>                    disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
>           dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
>           dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
>              disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
>                 disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
>                    disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
>                 disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
------------------


> >
> > > > In any case, just to be perfectly transparent, while being here waiting
> > > > for review, this patch series got tested on more smartphones, even ones
> > > > that I don't personally own, with different displays.
> > > >
> > > > For your reference, here's a list (all MSM8998..):
> > > > - OnePlus 5               (1920x1080)
> > > > - F(x)Tec Pro 1           (2160x1080)
> > > > - Sony Xperia XZ1 Compact (1280x720)
> > > > - Sony Xperia XZ1         (1920x1080)
> > > > - Sony Xperia XZ Premium  (3840x2160)
> > > >
> > >
> > > Yeah, no worries, I wasn't trying to imply that the patch was untested.
> > >
> > > Out of curiosity, are any of those video mode panels?
> > >
> > > >
> > > > > Also, something (I assume DSI related) that I was testing on
> > > > > msm-next-staging seems to have effected the colors on the panel (ie.
> > > > > they are more muted).. which seems to persist across reboots (ie. when
> > > >
> > > > So much "fun". This makes me think something about the PCC block doing
> > > > the wrong thing (getting misconfigured).
> > > >
> > > > > switching back to a good kernel), and interestingly if I reboot from a
> > > > > good kernel I see part of the login prompt (or whatever was previously
> > > > > on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> > > > > the display to think it is in PSR mode??)
> > > > >
> > > >
> > > >  From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
> > > > cannot produce (logically, at least) this, so I say that it is very
> > > > unlikely for this to be a consequence of the 10nm pll fixes...
> > > >
> > >
> > > Note that the bridge can also be programmed via dsi cmd mode packets,
> > > which I believe is the case on the 835 laptops (or at least one of
> > > them).. but all the things I have are using i2c as the control path.
> > >
> > > > ...unless the bootloader is not configuring the DSI rates, but that's
> > > > also veeeeery unlikely (it always does, or it always does not).
> > >
> > > I haven't looked at the bootloader display code, but booting back to
> > > an old/good kernel didn't change anything..  even powering off didn't.
> > > But the ghost image seemed to fade after some time, and by the next
> > > morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
> > > who works on display only when necessary.. ie. a gpu without a display
> > > isn't so much fun ;-))
> > >
> > > > > Not sure if that is caused by these patches, but if I can figure out
> > > > > how to get the panel back to normal I can bisect.  I think for now
> > > > > I'll drop this series.  Possibly it could be a
> > > > > two-wrongs-makes-a-right situation that had things working before, but
> > > > > I think someone from qcom who knows the DSI IP should take a look.
> > > > >
> > > >
> > > > I would be happy if someone from Qualcomm takes a look: after all, there
> > > > is no documentation and they're the only ones that can verify this kind
> > > > of stuff. Please, Qualcomm.
> > >
> > > Hopefully someone can take a look.
> > >
> > > > Besides that, if there's anything I can help with to solve this riddle,
> > > > I'm here for you.
> > >
> > > Thanks, like I said I'll try applying the patches one by one and see
> > > if I can narrow down what made the panel go funny, and we can go from
> > > there
> > >
> > > BR,
> > > -R
> > >
> > > > Yours,
> > > > -- Angelo
> > > >
> > > > > BR,
> > > > > -R
> > > > >
> > > > >
> > > > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > > >> ---
> > > > >>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> > > > >>   1 file changed, 9 insertions(+), 13 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > > > >> index 8b66e852eb36..5be562dfbf06 100644
> > > > >> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > > > >> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> > > > >> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
> > > > >>
> > > > >>          pll_freq = pll->vco_current_rate;
> > > > >>
> > > > >> -       if (config->disable_prescaler)
> > > > >> -               divider = fref;
> > > > >> -       else
> > > > >> -               divider = fref * 2;
> > > > >> -
> > > > >> +       divider = fref;
> > > > >>          multiplier = 1 << config->frac_bits;
> > > > >>          dec_multiple = div_u64(pll_freq * multiplier, divider);
> > > > >>          dec = div_u64_rem(dec_multiple, multiplier, &frac);
> > > > >> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
> > > > >>
> > > > >>   static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> > > > >>   {
> > > > >> +       struct dsi_pll_config *config = &pll->pll_configuration;
> > > > >>          void __iomem *base = pll->mmio;
> > > > >> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
> > > > >>
> > > > >> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> > > > >> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> > > > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> > > > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> > > > >>          pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> > > > >> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> > > > >>          frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> > > > >>                    0x3) << 16);
> > > > >>
> > > > >> -       /*
> > > > >> -        * TODO:
> > > > >> -        *      1. Assumes prescaler is disabled
> > > > >> -        */
> > > > >>          multiplier = 1 << config->frac_bits;
> > > > >> -       pll_freq = dec * (ref_clk * 2);
> > > > >> -       tmp64 = (ref_clk * 2 * frac);
> > > > >> +       pll_freq = dec * ref_clk;
> > > > >> +       tmp64 = ref_clk * frac;
> > > > >>          pll_freq += div_u64(tmp64, multiplier);
> > > > >> -
> > > > >>          vco_rate = pll_freq;
> > > > >>
> > > > >> +       if (config->disable_prescaler)
> > > > >> +               vco_rate = div_u64(vco_rate, 2);
> > > > >> +
> > > > >>          DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> > > > >>              pll_10nm->id, (unsigned long)vco_rate, dec, frac);
> > > > >>
> > > > >> --
> > > > >> 2.29.2
> > > > >>
> > > >

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[AngeloGioacchino Del Regno]

Il 01/02/21 18:31, Rob Clark ha scritto:
> On Mon, Feb 1, 2021 at 9:18 AM Rob Clark <robdclark@gmail.com> wrote:
>>
>> On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark@gmail.com> wrote:
>>>
>>> On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>
>>>> On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
>>>> <angelogioacchino.delregno@somainline.org> wrote:
>>>>>
>>>>> Il 31/01/21 20:50, Rob Clark ha scritto:
>>>>>> On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
>>>>>> <angelogioacchino.delregno@somainline.org> wrote:
>>>>>>>
>>>>>>> The VCO rate was being miscalculated due to a big overlook during
>>>>>>> the process of porting this driver from downstream to upstream:
>>>>>>> here we are really recalculating the rate of the VCO by reading
>>>>>>> the appropriate registers and returning a real frequency, while
>>>>>>> downstream the driver was doing something entirely different.
>>>>>>>
>>>>>>> In our case here, the recalculated rate was wrong, as it was then
>>>>>>> given back to the set_rate function, which was erroneously doing
>>>>>>> a division on the fractional value, based on the prescaler being
>>>>>>> either enabled or disabled: this was actually producing a bug for
>>>>>>> which the final VCO rate was being doubled, causing very obvious
>>>>>>> issues when trying to drive a DSI panel because the actual divider
>>>>>>> value was multiplied by two!
>>>>>>>
>>>>>>> To make things work properly, remove the multiplication of the
>>>>>>> reference clock by two from function dsi_pll_calc_dec_frac and
>>>>>>> account for the prescaler enablement in the vco_recalc_rate (if
>>>>>>> the prescaler is enabled, then the hardware will divide the rate
>>>>>>> by two).
>>>>>>>
>>>>>>> This will make the vco_recalc_rate function to pass the right
>>>>>>> frequency to the (clock framework) set_rate function when called,
>>>>>>> which will - in turn - program the right values in both the
>>>>>>> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
>>>>>>> registers, finally making the PLL to output the right clock.
>>>>>>>
>>>>>>> Also, while at it, remove the prescaler TODO by also adding the
>>>>>>> possibility of disabling the prescaler on the PLL (it is in the
>>>>>>> PLL_ANALOG_CONTROLS_ONE register).
>>>>>>> Of course, both prescaler-ON and OFF cases were tested.
>>>>>>
>>>>>> This somehow breaks things on sc7180 (display gets stuck at first
>>>>>> frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
>>>>>> bridge)
>>>>>>
>>>>>
>>>>> First frame of the splash means that something is "a bit" wrong...
>>>>> ...like the DSI clock is a little off.
>>>>>
>>>>> I don't have such hardware, otherwise I would've tried... but what you
>>>>> describe is a bit strange.
>>>>> Is there any other older qcom platform using this chip? Any other
>>>>> non-qcom platform? Is the driver for the SN65DSI86 surely fine?
>>>>> Anyway, as you know, I would never propose untested patches nor
>>>>> partially working ones for any reason: I'm sorry that this happened.
>>>>
>>>> I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
>>>>
>>>> The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
>>>> the snapdragon windows laptops).. and I think also the older 835
>>>> laptops.. ofc that doesn't mean that there isn't some bug, but I'd
>>>> guess maybe more likely that there is some small difference in DSI vs
>>>> older devices, or some cmd vs video mode difference.
>>>>
>>>> Anyways, seems like the screen did eventually recover so that gives me
>>>> a bit of confidence to bisect this series, which I'll do a bit later
>>>> today.
>>>
>>> fwiw, this series minus this patch, and everything looks ok.. let me
>>> take a closer look at what changes with this patch
>>
>> Btw, it looks like upstream, config->disable_prescaler is always
>> false.. I don't suppose you have anything WIP that changes this?
> 

Regarding that one, I have tested the driver in both cases, with
and without prescaler enabled (both worked fine), then I have decided
to leave the prescaler option exactly as the previous default.

My plan about this was/still is:
1. Wait until this one gets merged (gives me time to also look
    at the other billion patches that I've sent);
2. Add the prescaler option DT property and explain that it has
    to be used only with "puny" displays (low resolution, low
    clocks) as with "good ones", enabling the prescaler gives less
    clock jitter (and some microamps more power consumption);
3. Add the Spread Spectrum Clock (SSC) functionality with related
    DT properties.

Point 2 and 3 would go in the same series, unless someone does
N.2 before I do... and N.3 requires a bit of extensive testing,
which I have already partially started on the FxTec phone.

> fwiw, this is the clk_summary diff with and without this patch:
> 
> ------------------
> 270,282c270,282
> <     dsi0_pll_out_div_clk              1        1        0
> 887039941          0     0  50000         Y
> <        dsi0_pll_post_out_div_clk       0        0        0
> 221759985          0     0  50000         Y
> <        dsi0_pll_bit_clk               2        2        0
> 887039941          0     0  50000         Y
> <           dsi0_pclk_mux               1        1        0
> 887039941          0     0  50000         Y
> <              dsi0_phy_pll_out_dsiclk       1        1        0
> 147839991          0     0  50000         Y
> <                 disp_cc_mdss_pclk0_clk_src       1        1        0
>    147839991          0     0  50000         Y
> <                    disp_cc_mdss_pclk0_clk       1        1        0
>   147839991          0     0  50000         Y
> <           dsi0_pll_by_2_bit_clk       0        0        0
> 443519970          0     0  50000         Y
> <           dsi0_phy_pll_out_byteclk       1        1        0
> 110879992          0     0  50000         Y
> <              disp_cc_mdss_byte0_clk_src       2        2        0
> 110879992          0     0  50000         Y
> <                 disp_cc_mdss_byte0_div_clk_src       1        1
>    0    55439996          0     0  50000         Y
> <                    disp_cc_mdss_byte0_intf_clk       1        1
>    0    55439996          0     0  50000         Y
> <                 disp_cc_mdss_byte0_clk       1        1        0
> 110879992          0     0  50000         Y
> ---
>>      dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
>>         dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
>>         dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
>>            dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
>>               dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
>>                  disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
>>                     disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
>>            dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
>>            dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
>>               disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
>>                  disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
>>                     disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
>>                  disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
> ------------------
> 
> 

This is almost exactly what I saw on my devices as well, you get a
difference of "just some Hz" (which can be totally ignored), because
of how the calculation is done now.

Thing is, what you see as PIXEL and BYTE clocks *before* the change is
Linux thinking that your DSI is at that frequency, while the PLL will
output *half* the rate, which is exactly what the patch fixes.

"Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same
display... by lowering the DSI rate on the MSM8996 phone by half, I
get the same *identical* issues as the 8998 one without this patch.
The clocks all match between one and another, because.. it's.. the same
display, after all.

It is because of the aforementioned test that I have raised doubts about
the TI chip driver (or anything else really).. but then, anything is
possible.

>>>
>>>>> In any case, just to be perfectly transparent, while being here waiting
>>>>> for review, this patch series got tested on more smartphones, even ones
>>>>> that I don't personally own, with different displays.
>>>>>
>>>>> For your reference, here's a list (all MSM8998..):
>>>>> - OnePlus 5               (1920x1080)
>>>>> - F(x)Tec Pro 1           (2160x1080)
>>>>> - Sony Xperia XZ1 Compact (1280x720)
>>>>> - Sony Xperia XZ1         (1920x1080)
>>>>> - Sony Xperia XZ Premium  (3840x2160)
>>>>>
>>>>
>>>> Yeah, no worries, I wasn't trying to imply that the patch was untested.
>>>>

I know, of course!

>>>> Out of curiosity, are any of those video mode panels?

Yes and "also":
The FxTec Pro1 has a video mode panel, for which I'm trying to upstream
the driver...look here: https://lore.kernel.org/patchwork/patch/1365228/

The Xperia XZ Premium has a Sharp LS055D1SX04 panel under NT35950, which
can be configured as command or as video mode... I tried both modes, but
there is some issue with the DPU1/DSI drivers and *DUAL DSI*, as cmd
does work with some tearing, but video doesn't even start (downstream it
works).

So the only video mode panel that I could test is that BOE panel on the
FxTec phone (single dsi), which works just great.

>>>>
>>>>>
>>>>>> Also, something (I assume DSI related) that I was testing on
>>>>>> msm-next-staging seems to have effected the colors on the panel (ie.
>>>>>> they are more muted).. which seems to persist across reboots (ie. when
>>>>>
>>>>> So much "fun". This makes me think something about the PCC block doing
>>>>> the wrong thing (getting misconfigured).
>>>>>
>>>>>> switching back to a good kernel), and interestingly if I reboot from a
>>>>>> good kernel I see part of the login prompt (or whatever was previously
>>>>>> on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
>>>>>> the display to think it is in PSR mode??)
>>>>>>
>>>>>
>>>>>   From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
>>>>> cannot produce (logically, at least) this, so I say that it is very
>>>>> unlikely for this to be a consequence of the 10nm pll fixes...
>>>>>
>>>>
>>>> Note that the bridge can also be programmed via dsi cmd mode packets,
>>>> which I believe is the case on the 835 laptops (or at least one of
>>>> them).. but all the things I have are using i2c as the control path.
>>>>
>>>>> ...unless the bootloader is not configuring the DSI rates, but that's
>>>>> also veeeeery unlikely (it always does, or it always does not).
>>>>
>>>> I haven't looked at the bootloader display code, but booting back to
>>>> an old/good kernel didn't change anything..  even powering off didn't.
>>>> But the ghost image seemed to fade after some time, and by the next
>>>> morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
>>>> who works on display only when necessary.. ie. a gpu without a display
>>>> isn't so much fun ;-))
>>>>

OpenCL all the way! lol :D

On Qualcomm platforms, the first thing that I've ever done was to bring
up displays on 8974 Sony platforms... (we're talking about years ago).

I'm a lil more on the display side of things (but growing a beard while
waiting between a frame and another due to no GPU isn't so much fun
either!).

>>>>>> Not sure if that is caused by these patches, but if I can figure out
>>>>>> how to get the panel back to normal I can bisect.  I think for now
>>>>>> I'll drop this series.  Possibly it could be a
>>>>>> two-wrongs-makes-a-right situation that had things working before, but
>>>>>> I think someone from qcom who knows the DSI IP should take a look.
>>>>>>
>>>>>
>>>>> I would be happy if someone from Qualcomm takes a look: after all, there
>>>>> is no documentation and they're the only ones that can verify this kind
>>>>> of stuff. Please, Qualcomm.
>>>>
>>>> Hopefully someone can take a look.
>>>>
>>>>> Besides that, if there's anything I can help with to solve this riddle,
>>>>> I'm here for you.
>>>>
>>>> Thanks, like I said I'll try applying the patches one by one and see
>>>> if I can narrow down what made the panel go funny, and we can go from
>>>> there
>>>>
>>>> BR,
>>>> -R
>>>>
>>>>> Yours,
>>>>> -- Angelo
>>>>>
>>>>>> BR,
>>>>>> -R
>>>>>>
>>>>>>
>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
>>>>>>>    1 file changed, 9 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>> index 8b66e852eb36..5be562dfbf06 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
>>>>>>>
>>>>>>>           pll_freq = pll->vco_current_rate;
>>>>>>>
>>>>>>> -       if (config->disable_prescaler)
>>>>>>> -               divider = fref;
>>>>>>> -       else
>>>>>>> -               divider = fref * 2;
>>>>>>> -
>>>>>>> +       divider = fref;
>>>>>>>           multiplier = 1 << config->frac_bits;
>>>>>>>           dec_multiple = div_u64(pll_freq * multiplier, divider);
>>>>>>>           dec = div_u64_rem(dec_multiple, multiplier, &frac);
>>>>>>> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
>>>>>>>
>>>>>>>    static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
>>>>>>>    {
>>>>>>> +       struct dsi_pll_config *config = &pll->pll_configuration;
>>>>>>>           void __iomem *base = pll->mmio;
>>>>>>> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
>>>>>>>
>>>>>>> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
>>>>>>> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
>>>>>>>           pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
>>>>>>>           pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
>>>>>>>           pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
>>>>>>> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
>>>>>>>           frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
>>>>>>>                     0x3) << 16);
>>>>>>>
>>>>>>> -       /*
>>>>>>> -        * TODO:
>>>>>>> -        *      1. Assumes prescaler is disabled
>>>>>>> -        */
>>>>>>>           multiplier = 1 << config->frac_bits;
>>>>>>> -       pll_freq = dec * (ref_clk * 2);
>>>>>>> -       tmp64 = (ref_clk * 2 * frac);
>>>>>>> +       pll_freq = dec * ref_clk;
>>>>>>> +       tmp64 = ref_clk * frac;
>>>>>>>           pll_freq += div_u64(tmp64, multiplier);
>>>>>>> -
>>>>>>>           vco_rate = pll_freq;
>>>>>>>
>>>>>>> +       if (config->disable_prescaler)
>>>>>>> +               vco_rate = div_u64(vco_rate, 2);
>>>>>>> +
>>>>>>>           DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
>>>>>>>               pll_10nm->id, (unsigned long)vco_rate, dec, frac);
>>>>>>>
>>>>>>> --
>>>>>>> 2.29.2
>>>>>>>
>>>>>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 01/02/21 18:31, Rob Clark ha scritto:
> > On Mon, Feb 1, 2021 at 9:18 AM Rob Clark <robdclark@gmail.com> wrote:
> >>
> >> On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>
> >>> On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>>
> >>>> On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
> >>>> <angelogioacchino.delregno@somainline.org> wrote:
> >>>>>
> >>>>> Il 31/01/21 20:50, Rob Clark ha scritto:
> >>>>>> On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> >>>>>> <angelogioacchino.delregno@somainline.org> wrote:
> >>>>>>>
> >>>>>>> The VCO rate was being miscalculated due to a big overlook during
> >>>>>>> the process of porting this driver from downstream to upstream:
> >>>>>>> here we are really recalculating the rate of the VCO by reading
> >>>>>>> the appropriate registers and returning a real frequency, while
> >>>>>>> downstream the driver was doing something entirely different.
> >>>>>>>
> >>>>>>> In our case here, the recalculated rate was wrong, as it was then
> >>>>>>> given back to the set_rate function, which was erroneously doing
> >>>>>>> a division on the fractional value, based on the prescaler being
> >>>>>>> either enabled or disabled: this was actually producing a bug for
> >>>>>>> which the final VCO rate was being doubled, causing very obvious
> >>>>>>> issues when trying to drive a DSI panel because the actual divider
> >>>>>>> value was multiplied by two!
> >>>>>>>
> >>>>>>> To make things work properly, remove the multiplication of the
> >>>>>>> reference clock by two from function dsi_pll_calc_dec_frac and
> >>>>>>> account for the prescaler enablement in the vco_recalc_rate (if
> >>>>>>> the prescaler is enabled, then the hardware will divide the rate
> >>>>>>> by two).
> >>>>>>>
> >>>>>>> This will make the vco_recalc_rate function to pass the right
> >>>>>>> frequency to the (clock framework) set_rate function when called,
> >>>>>>> which will - in turn - program the right values in both the
> >>>>>>> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> >>>>>>> registers, finally making the PLL to output the right clock.
> >>>>>>>
> >>>>>>> Also, while at it, remove the prescaler TODO by also adding the
> >>>>>>> possibility of disabling the prescaler on the PLL (it is in the
> >>>>>>> PLL_ANALOG_CONTROLS_ONE register).
> >>>>>>> Of course, both prescaler-ON and OFF cases were tested.
> >>>>>>
> >>>>>> This somehow breaks things on sc7180 (display gets stuck at first
> >>>>>> frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> >>>>>> bridge)
> >>>>>>
> >>>>>
> >>>>> First frame of the splash means that something is "a bit" wrong...
> >>>>> ...like the DSI clock is a little off.
> >>>>>
> >>>>> I don't have such hardware, otherwise I would've tried... but what you
> >>>>> describe is a bit strange.
> >>>>> Is there any other older qcom platform using this chip? Any other
> >>>>> non-qcom platform? Is the driver for the SN65DSI86 surely fine?
> >>>>> Anyway, as you know, I would never propose untested patches nor
> >>>>> partially working ones for any reason: I'm sorry that this happened.
> >>>>
> >>>> I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
> >>>>
> >>>> The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
> >>>> the snapdragon windows laptops).. and I think also the older 835
> >>>> laptops.. ofc that doesn't mean that there isn't some bug, but I'd
> >>>> guess maybe more likely that there is some small difference in DSI vs
> >>>> older devices, or some cmd vs video mode difference.
> >>>>
> >>>> Anyways, seems like the screen did eventually recover so that gives me
> >>>> a bit of confidence to bisect this series, which I'll do a bit later
> >>>> today.
> >>>
> >>> fwiw, this series minus this patch, and everything looks ok.. let me
> >>> take a closer look at what changes with this patch
> >>
> >> Btw, it looks like upstream, config->disable_prescaler is always
> >> false.. I don't suppose you have anything WIP that changes this?
> >
>
> Regarding that one, I have tested the driver in both cases, with
> and without prescaler enabled (both worked fine), then I have decided
> to leave the prescaler option exactly as the previous default.
>
> My plan about this was/still is:
> 1. Wait until this one gets merged (gives me time to also look
>     at the other billion patches that I've sent);
> 2. Add the prescaler option DT property and explain that it has
>     to be used only with "puny" displays (low resolution, low
>     clocks) as with "good ones", enabling the prescaler gives less
>     clock jitter (and some microamps more power consumption);
> 3. Add the Spread Spectrum Clock (SSC) functionality with related
>     DT properties.
>
> Point 2 and 3 would go in the same series, unless someone does
> N.2 before I do... and N.3 requires a bit of extensive testing,
> which I have already partially started on the FxTec phone.
>
> > fwiw, this is the clk_summary diff with and without this patch:
> >
> > ------------------
> > 270,282c270,282
> > <     dsi0_pll_out_div_clk              1        1        0
> > 887039941          0     0  50000         Y
> > <        dsi0_pll_post_out_div_clk       0        0        0
> > 221759985          0     0  50000         Y
> > <        dsi0_pll_bit_clk               2        2        0
> > 887039941          0     0  50000         Y
> > <           dsi0_pclk_mux               1        1        0
> > 887039941          0     0  50000         Y
> > <              dsi0_phy_pll_out_dsiclk       1        1        0
> > 147839991          0     0  50000         Y
> > <                 disp_cc_mdss_pclk0_clk_src       1        1        0
> >    147839991          0     0  50000         Y
> > <                    disp_cc_mdss_pclk0_clk       1        1        0
> >   147839991          0     0  50000         Y
> > <           dsi0_pll_by_2_bit_clk       0        0        0
> > 443519970          0     0  50000         Y
> > <           dsi0_phy_pll_out_byteclk       1        1        0
> > 110879992          0     0  50000         Y
> > <              disp_cc_mdss_byte0_clk_src       2        2        0
> > 110879992          0     0  50000         Y
> > <                 disp_cc_mdss_byte0_div_clk_src       1        1
> >    0    55439996          0     0  50000         Y
> > <                    disp_cc_mdss_byte0_intf_clk       1        1
> >    0    55439996          0     0  50000         Y
> > <                 disp_cc_mdss_byte0_clk       1        1        0
> > 110879992          0     0  50000         Y
> > ---
> >>      dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
> >>         dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
> >>         dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
> >>            dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
> >>               dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
> >>                  disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
> >>                     disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
> >>            dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
> >>            dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
> >>               disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
> >>                  disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
> >>                     disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
> >>                  disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
> > ------------------
> >
> >
>
> This is almost exactly what I saw on my devices as well, you get a
> difference of "just some Hz" (which can be totally ignored), because
> of how the calculation is done now.
>
> Thing is, what you see as PIXEL and BYTE clocks *before* the change is
> Linux thinking that your DSI is at that frequency, while the PLL will
> output *half* the rate, which is exactly what the patch fixes.
>
> "Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same
> display... by lowering the DSI rate on the MSM8996 phone by half, I
> get the same *identical* issues as the 8998 one without this patch.
> The clocks all match between one and another, because.. it's.. the same
> display, after all.
>
> It is because of the aforementioned test that I have raised doubts about
> the TI chip driver (or anything else really).. but then, anything is
> possible.

It does look like, *so far* the TI bridge chip is only used on qc
platforms (according to grep'ing dts), so I suppose I can't rule out
bugs which cancel each other out.  Although there are various other
bridges used (for ex, the sdm845 rb3 board has some dsi->hdmi bridge)

I guess it would be useful if we could measure the clk somehow to
confirm that it is running at the rate we think it is..

BR,
-R


> >>>
> >>>>> In any case, just to be perfectly transparent, while being here waiting
> >>>>> for review, this patch series got tested on more smartphones, even ones
> >>>>> that I don't personally own, with different displays.
> >>>>>
> >>>>> For your reference, here's a list (all MSM8998..):
> >>>>> - OnePlus 5               (1920x1080)
> >>>>> - F(x)Tec Pro 1           (2160x1080)
> >>>>> - Sony Xperia XZ1 Compact (1280x720)
> >>>>> - Sony Xperia XZ1         (1920x1080)
> >>>>> - Sony Xperia XZ Premium  (3840x2160)
> >>>>>
> >>>>
> >>>> Yeah, no worries, I wasn't trying to imply that the patch was untested.
> >>>>
>
> I know, of course!
>
> >>>> Out of curiosity, are any of those video mode panels?
>
> Yes and "also":
> The FxTec Pro1 has a video mode panel, for which I'm trying to upstream
> the driver...look here: https://lore.kernel.org/patchwork/patch/1365228/
>
> The Xperia XZ Premium has a Sharp LS055D1SX04 panel under NT35950, which
> can be configured as command or as video mode... I tried both modes, but
> there is some issue with the DPU1/DSI drivers and *DUAL DSI*, as cmd
> does work with some tearing, but video doesn't even start (downstream it
> works).
>
> So the only video mode panel that I could test is that BOE panel on the
> FxTec phone (single dsi), which works just great.
>
> >>>>
> >>>>>
> >>>>>> Also, something (I assume DSI related) that I was testing on
> >>>>>> msm-next-staging seems to have effected the colors on the panel (ie.
> >>>>>> they are more muted).. which seems to persist across reboots (ie. when
> >>>>>
> >>>>> So much "fun". This makes me think something about the PCC block doing
> >>>>> the wrong thing (getting misconfigured).
> >>>>>
> >>>>>> switching back to a good kernel), and interestingly if I reboot from a
> >>>>>> good kernel I see part of the login prompt (or whatever was previously
> >>>>>> on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> >>>>>> the display to think it is in PSR mode??)
> >>>>>>
> >>>>>
> >>>>>   From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
> >>>>> cannot produce (logically, at least) this, so I say that it is very
> >>>>> unlikely for this to be a consequence of the 10nm pll fixes...
> >>>>>
> >>>>
> >>>> Note that the bridge can also be programmed via dsi cmd mode packets,
> >>>> which I believe is the case on the 835 laptops (or at least one of
> >>>> them).. but all the things I have are using i2c as the control path.
> >>>>
> >>>>> ...unless the bootloader is not configuring the DSI rates, but that's
> >>>>> also veeeeery unlikely (it always does, or it always does not).
> >>>>
> >>>> I haven't looked at the bootloader display code, but booting back to
> >>>> an old/good kernel didn't change anything..  even powering off didn't.
> >>>> But the ghost image seemed to fade after some time, and by the next
> >>>> morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
> >>>> who works on display only when necessary.. ie. a gpu without a display
> >>>> isn't so much fun ;-))
> >>>>
>
> OpenCL all the way! lol :D
>
> On Qualcomm platforms, the first thing that I've ever done was to bring
> up displays on 8974 Sony platforms... (we're talking about years ago).
>
> I'm a lil more on the display side of things (but growing a beard while
> waiting between a frame and another due to no GPU isn't so much fun
> either!).
>
> >>>>>> Not sure if that is caused by these patches, but if I can figure out
> >>>>>> how to get the panel back to normal I can bisect.  I think for now
> >>>>>> I'll drop this series.  Possibly it could be a
> >>>>>> two-wrongs-makes-a-right situation that had things working before, but
> >>>>>> I think someone from qcom who knows the DSI IP should take a look.
> >>>>>>
> >>>>>
> >>>>> I would be happy if someone from Qualcomm takes a look: after all, there
> >>>>> is no documentation and they're the only ones that can verify this kind
> >>>>> of stuff. Please, Qualcomm.
> >>>>
> >>>> Hopefully someone can take a look.
> >>>>
> >>>>> Besides that, if there's anything I can help with to solve this riddle,
> >>>>> I'm here for you.
> >>>>
> >>>> Thanks, like I said I'll try applying the patches one by one and see
> >>>> if I can narrow down what made the panel go funny, and we can go from
> >>>> there
> >>>>
> >>>> BR,
> >>>> -R
> >>>>
> >>>>> Yours,
> >>>>> -- Angelo
> >>>>>
> >>>>>> BR,
> >>>>>> -R
> >>>>>>
> >>>>>>
> >>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >>>>>>> ---
> >>>>>>>    drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> >>>>>>>    1 file changed, 9 insertions(+), 13 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>>>>>> index 8b66e852eb36..5be562dfbf06 100644
> >>>>>>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>>>>>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>>>>>> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
> >>>>>>>
> >>>>>>>           pll_freq = pll->vco_current_rate;
> >>>>>>>
> >>>>>>> -       if (config->disable_prescaler)
> >>>>>>> -               divider = fref;
> >>>>>>> -       else
> >>>>>>> -               divider = fref * 2;
> >>>>>>> -
> >>>>>>> +       divider = fref;
> >>>>>>>           multiplier = 1 << config->frac_bits;
> >>>>>>>           dec_multiple = div_u64(pll_freq * multiplier, divider);
> >>>>>>>           dec = div_u64_rem(dec_multiple, multiplier, &frac);
> >>>>>>> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
> >>>>>>>
> >>>>>>>    static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> >>>>>>>    {
> >>>>>>> +       struct dsi_pll_config *config = &pll->pll_configuration;
> >>>>>>>           void __iomem *base = pll->mmio;
> >>>>>>> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
> >>>>>>>
> >>>>>>> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> >>>>>>> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> >>>>>>>           pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> >>>>>>>           pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> >>>>>>>           pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> >>>>>>> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> >>>>>>>           frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> >>>>>>>                     0x3) << 16);
> >>>>>>>
> >>>>>>> -       /*
> >>>>>>> -        * TODO:
> >>>>>>> -        *      1. Assumes prescaler is disabled
> >>>>>>> -        */
> >>>>>>>           multiplier = 1 << config->frac_bits;
> >>>>>>> -       pll_freq = dec * (ref_clk * 2);
> >>>>>>> -       tmp64 = (ref_clk * 2 * frac);
> >>>>>>> +       pll_freq = dec * ref_clk;
> >>>>>>> +       tmp64 = ref_clk * frac;
> >>>>>>>           pll_freq += div_u64(tmp64, multiplier);
> >>>>>>> -
> >>>>>>>           vco_rate = pll_freq;
> >>>>>>>
> >>>>>>> +       if (config->disable_prescaler)
> >>>>>>> +               vco_rate = div_u64(vco_rate, 2);
> >>>>>>> +
> >>>>>>>           DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> >>>>>>>               pll_10nm->id, (unsigned long)vco_rate, dec, frac);
> >>>>>>>
> >>>>>>> --
> >>>>>>> 2.29.2
> >>>>>>>
> >>>>>
>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[AngeloGioacchino Del Regno]

Il 02/02/21 19:45, Rob Clark ha scritto:
> On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>>
>> Il 01/02/21 18:31, Rob Clark ha scritto:
>>> On Mon, Feb 1, 2021 at 9:18 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>
>>>> On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>>
>>>>> On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
>>>>>>
>>>>>> On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
>>>>>> <angelogioacchino.delregno@somainline.org> wrote:
>>>>>>>
>>>>>>> Il 31/01/21 20:50, Rob Clark ha scritto:
>>>>>>>> On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
>>>>>>>> <angelogioacchino.delregno@somainline.org> wrote:
>>>>>>>>>
>>>>>>>>> The VCO rate was being miscalculated due to a big overlook during
>>>>>>>>> the process of porting this driver from downstream to upstream:
>>>>>>>>> here we are really recalculating the rate of the VCO by reading
>>>>>>>>> the appropriate registers and returning a real frequency, while
>>>>>>>>> downstream the driver was doing something entirely different.
>>>>>>>>>
>>>>>>>>> In our case here, the recalculated rate was wrong, as it was then
>>>>>>>>> given back to the set_rate function, which was erroneously doing
>>>>>>>>> a division on the fractional value, based on the prescaler being
>>>>>>>>> either enabled or disabled: this was actually producing a bug for
>>>>>>>>> which the final VCO rate was being doubled, causing very obvious
>>>>>>>>> issues when trying to drive a DSI panel because the actual divider
>>>>>>>>> value was multiplied by two!
>>>>>>>>>
>>>>>>>>> To make things work properly, remove the multiplication of the
>>>>>>>>> reference clock by two from function dsi_pll_calc_dec_frac and
>>>>>>>>> account for the prescaler enablement in the vco_recalc_rate (if
>>>>>>>>> the prescaler is enabled, then the hardware will divide the rate
>>>>>>>>> by two).
>>>>>>>>>
>>>>>>>>> This will make the vco_recalc_rate function to pass the right
>>>>>>>>> frequency to the (clock framework) set_rate function when called,
>>>>>>>>> which will - in turn - program the right values in both the
>>>>>>>>> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
>>>>>>>>> registers, finally making the PLL to output the right clock.
>>>>>>>>>
>>>>>>>>> Also, while at it, remove the prescaler TODO by also adding the
>>>>>>>>> possibility of disabling the prescaler on the PLL (it is in the
>>>>>>>>> PLL_ANALOG_CONTROLS_ONE register).
>>>>>>>>> Of course, both prescaler-ON and OFF cases were tested.
>>>>>>>>
>>>>>>>> This somehow breaks things on sc7180 (display gets stuck at first
>>>>>>>> frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
>>>>>>>> bridge)
>>>>>>>>
>>>>>>>
>>>>>>> First frame of the splash means that something is "a bit" wrong...
>>>>>>> ...like the DSI clock is a little off.
>>>>>>>
>>>>>>> I don't have such hardware, otherwise I would've tried... but what you
>>>>>>> describe is a bit strange.
>>>>>>> Is there any other older qcom platform using this chip? Any other
>>>>>>> non-qcom platform? Is the driver for the SN65DSI86 surely fine?
>>>>>>> Anyway, as you know, I would never propose untested patches nor
>>>>>>> partially working ones for any reason: I'm sorry that this happened.
>>>>>>
>>>>>> I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
>>>>>>
>>>>>> The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
>>>>>> the snapdragon windows laptops).. and I think also the older 835
>>>>>> laptops.. ofc that doesn't mean that there isn't some bug, but I'd
>>>>>> guess maybe more likely that there is some small difference in DSI vs
>>>>>> older devices, or some cmd vs video mode difference.
>>>>>>
>>>>>> Anyways, seems like the screen did eventually recover so that gives me
>>>>>> a bit of confidence to bisect this series, which I'll do a bit later
>>>>>> today.
>>>>>
>>>>> fwiw, this series minus this patch, and everything looks ok.. let me
>>>>> take a closer look at what changes with this patch
>>>>
>>>> Btw, it looks like upstream, config->disable_prescaler is always
>>>> false.. I don't suppose you have anything WIP that changes this?
>>>
>>
>> Regarding that one, I have tested the driver in both cases, with
>> and without prescaler enabled (both worked fine), then I have decided
>> to leave the prescaler option exactly as the previous default.
>>
>> My plan about this was/still is:
>> 1. Wait until this one gets merged (gives me time to also look
>>      at the other billion patches that I've sent);
>> 2. Add the prescaler option DT property and explain that it has
>>      to be used only with "puny" displays (low resolution, low
>>      clocks) as with "good ones", enabling the prescaler gives less
>>      clock jitter (and some microamps more power consumption);
>> 3. Add the Spread Spectrum Clock (SSC) functionality with related
>>      DT properties.
>>
>> Point 2 and 3 would go in the same series, unless someone does
>> N.2 before I do... and N.3 requires a bit of extensive testing,
>> which I have already partially started on the FxTec phone.
>>
>>> fwiw, this is the clk_summary diff with and without this patch:
>>>
>>> ------------------
>>> 270,282c270,282
>>> <     dsi0_pll_out_div_clk              1        1        0
>>> 887039941          0     0  50000         Y
>>> <        dsi0_pll_post_out_div_clk       0        0        0
>>> 221759985          0     0  50000         Y
>>> <        dsi0_pll_bit_clk               2        2        0
>>> 887039941          0     0  50000         Y
>>> <           dsi0_pclk_mux               1        1        0
>>> 887039941          0     0  50000         Y
>>> <              dsi0_phy_pll_out_dsiclk       1        1        0
>>> 147839991          0     0  50000         Y
>>> <                 disp_cc_mdss_pclk0_clk_src       1        1        0
>>>     147839991          0     0  50000         Y
>>> <                    disp_cc_mdss_pclk0_clk       1        1        0
>>>    147839991          0     0  50000         Y
>>> <           dsi0_pll_by_2_bit_clk       0        0        0
>>> 443519970          0     0  50000         Y
>>> <           dsi0_phy_pll_out_byteclk       1        1        0
>>> 110879992          0     0  50000         Y
>>> <              disp_cc_mdss_byte0_clk_src       2        2        0
>>> 110879992          0     0  50000         Y
>>> <                 disp_cc_mdss_byte0_div_clk_src       1        1
>>>     0    55439996          0     0  50000         Y
>>> <                    disp_cc_mdss_byte0_intf_clk       1        1
>>>     0    55439996          0     0  50000         Y
>>> <                 disp_cc_mdss_byte0_clk       1        1        0
>>> 110879992          0     0  50000         Y
>>> ---
>>>>       dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
>>>>          dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
>>>>          dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
>>>>             dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
>>>>                dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
>>>>                   disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
>>>>                      disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
>>>>             dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
>>>>             dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
>>>>                disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
>>>>                   disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
>>>>                      disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
>>>>                   disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
>>> ------------------
>>>
>>>
>>
>> This is almost exactly what I saw on my devices as well, you get a
>> difference of "just some Hz" (which can be totally ignored), because
>> of how the calculation is done now.
>>
>> Thing is, what you see as PIXEL and BYTE clocks *before* the change is
>> Linux thinking that your DSI is at that frequency, while the PLL will
>> output *half* the rate, which is exactly what the patch fixes.
>>
>> "Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same
>> display... by lowering the DSI rate on the MSM8996 phone by half, I
>> get the same *identical* issues as the 8998 one without this patch.
>> The clocks all match between one and another, because.. it's.. the same
>> display, after all.
>>
>> It is because of the aforementioned test that I have raised doubts about
>> the TI chip driver (or anything else really).. but then, anything is
>> possible.
> 
> It does look like, *so far* the TI bridge chip is only used on qc
> platforms (according to grep'ing dts), so I suppose I can't rule out
> bugs which cancel each other out.  Although there are various other
> bridges used (for ex, the sdm845 rb3 board has some dsi->hdmi bridge)
> 

Argh...

> I guess it would be useful if we could measure the clk somehow to
> confirm that it is running at the rate we think it is..
> 

I totally agree with you on this, I actually wanted to do it the proper
way, but then these clocks are really too high for my cheap oscilloscope
and I couldn't... :(

> BR,
> -R
> 
> 
>>>>>
>>>>>>> In any case, just to be perfectly transparent, while being here waiting
>>>>>>> for review, this patch series got tested on more smartphones, even ones
>>>>>>> that I don't personally own, with different displays.
>>>>>>>
>>>>>>> For your reference, here's a list (all MSM8998..):
>>>>>>> - OnePlus 5               (1920x1080)
>>>>>>> - F(x)Tec Pro 1           (2160x1080)
>>>>>>> - Sony Xperia XZ1 Compact (1280x720)
>>>>>>> - Sony Xperia XZ1         (1920x1080)
>>>>>>> - Sony Xperia XZ Premium  (3840x2160)
>>>>>>>
>>>>>>
>>>>>> Yeah, no worries, I wasn't trying to imply that the patch was untested.
>>>>>>
>>
>> I know, of course!
>>
>>>>>> Out of curiosity, are any of those video mode panels?
>>
>> Yes and "also":
>> The FxTec Pro1 has a video mode panel, for which I'm trying to upstream
>> the driver...look here: https://lore.kernel.org/patchwork/patch/1365228/
>>
>> The Xperia XZ Premium has a Sharp LS055D1SX04 panel under NT35950, which
>> can be configured as command or as video mode... I tried both modes, but
>> there is some issue with the DPU1/DSI drivers and *DUAL DSI*, as cmd
>> does work with some tearing, but video doesn't even start (downstream it
>> works).
>>
>> So the only video mode panel that I could test is that BOE panel on the
>> FxTec phone (single dsi), which works just great.
>>
>>>>>>
>>>>>>>
>>>>>>>> Also, something (I assume DSI related) that I was testing on
>>>>>>>> msm-next-staging seems to have effected the colors on the panel (ie.
>>>>>>>> they are more muted).. which seems to persist across reboots (ie. when
>>>>>>>
>>>>>>> So much "fun". This makes me think something about the PCC block doing
>>>>>>> the wrong thing (getting misconfigured).
>>>>>>>
>>>>>>>> switching back to a good kernel), and interestingly if I reboot from a
>>>>>>>> good kernel I see part of the login prompt (or whatever was previously
>>>>>>>> on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
>>>>>>>> the display to think it is in PSR mode??)
>>>>>>>>
>>>>>>>
>>>>>>>    From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
>>>>>>> cannot produce (logically, at least) this, so I say that it is very
>>>>>>> unlikely for this to be a consequence of the 10nm pll fixes...
>>>>>>>
>>>>>>
>>>>>> Note that the bridge can also be programmed via dsi cmd mode packets,
>>>>>> which I believe is the case on the 835 laptops (or at least one of
>>>>>> them).. but all the things I have are using i2c as the control path.
>>>>>>
>>>>>>> ...unless the bootloader is not configuring the DSI rates, but that's
>>>>>>> also veeeeery unlikely (it always does, or it always does not).
>>>>>>
>>>>>> I haven't looked at the bootloader display code, but booting back to
>>>>>> an old/good kernel didn't change anything..  even powering off didn't.
>>>>>> But the ghost image seemed to fade after some time, and by the next
>>>>>> morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
>>>>>> who works on display only when necessary.. ie. a gpu without a display
>>>>>> isn't so much fun ;-))
>>>>>>
>>
>> OpenCL all the way! lol :D
>>
>> On Qualcomm platforms, the first thing that I've ever done was to bring
>> up displays on 8974 Sony platforms... (we're talking about years ago).
>>
>> I'm a lil more on the display side of things (but growing a beard while
>> waiting between a frame and another due to no GPU isn't so much fun
>> either!).
>>
>>>>>>>> Not sure if that is caused by these patches, but if I can figure out
>>>>>>>> how to get the panel back to normal I can bisect.  I think for now
>>>>>>>> I'll drop this series.  Possibly it could be a
>>>>>>>> two-wrongs-makes-a-right situation that had things working before, but
>>>>>>>> I think someone from qcom who knows the DSI IP should take a look.
>>>>>>>>
>>>>>>>
>>>>>>> I would be happy if someone from Qualcomm takes a look: after all, there
>>>>>>> is no documentation and they're the only ones that can verify this kind
>>>>>>> of stuff. Please, Qualcomm.
>>>>>>
>>>>>> Hopefully someone can take a look.
>>>>>>
>>>>>>> Besides that, if there's anything I can help with to solve this riddle,
>>>>>>> I'm here for you.
>>>>>>
>>>>>> Thanks, like I said I'll try applying the patches one by one and see
>>>>>> if I can narrow down what made the panel go funny, and we can go from
>>>>>> there
>>>>>>
>>>>>> BR,
>>>>>> -R
>>>>>>
>>>>>>> Yours,
>>>>>>> -- Angelo
>>>>>>>
>>>>>>>> BR,
>>>>>>>> -R
>>>>>>>>
>>>>>>>>
>>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>>>>>>>>> ---
>>>>>>>>>     drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
>>>>>>>>>     1 file changed, 9 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>>>> index 8b66e852eb36..5be562dfbf06 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
>>>>>>>>> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
>>>>>>>>>
>>>>>>>>>            pll_freq = pll->vco_current_rate;
>>>>>>>>>
>>>>>>>>> -       if (config->disable_prescaler)
>>>>>>>>> -               divider = fref;
>>>>>>>>> -       else
>>>>>>>>> -               divider = fref * 2;
>>>>>>>>> -
>>>>>>>>> +       divider = fref;
>>>>>>>>>            multiplier = 1 << config->frac_bits;
>>>>>>>>>            dec_multiple = div_u64(pll_freq * multiplier, divider);
>>>>>>>>>            dec = div_u64_rem(dec_multiple, multiplier, &frac);
>>>>>>>>> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
>>>>>>>>>
>>>>>>>>>     static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
>>>>>>>>>     {
>>>>>>>>> +       struct dsi_pll_config *config = &pll->pll_configuration;
>>>>>>>>>            void __iomem *base = pll->mmio;
>>>>>>>>> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
>>>>>>>>>
>>>>>>>>> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
>>>>>>>>> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
>>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
>>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
>>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
>>>>>>>>> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
>>>>>>>>>            frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
>>>>>>>>>                      0x3) << 16);
>>>>>>>>>
>>>>>>>>> -       /*
>>>>>>>>> -        * TODO:
>>>>>>>>> -        *      1. Assumes prescaler is disabled
>>>>>>>>> -        */
>>>>>>>>>            multiplier = 1 << config->frac_bits;
>>>>>>>>> -       pll_freq = dec * (ref_clk * 2);
>>>>>>>>> -       tmp64 = (ref_clk * 2 * frac);
>>>>>>>>> +       pll_freq = dec * ref_clk;
>>>>>>>>> +       tmp64 = ref_clk * frac;
>>>>>>>>>            pll_freq += div_u64(tmp64, multiplier);
>>>>>>>>> -
>>>>>>>>>            vco_rate = pll_freq;
>>>>>>>>>
>>>>>>>>> +       if (config->disable_prescaler)
>>>>>>>>> +               vco_rate = div_u64(vco_rate, 2);
>>>>>>>>> +
>>>>>>>>>            DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
>>>>>>>>>                pll_10nm->id, (unsigned long)vco_rate, dec, frac);
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 2.29.2
>>>>>>>>>
>>>>>>>
>>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Tue, Feb 2, 2021 at 10:46 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 02/02/21 19:45, Rob Clark ha scritto:
> > On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@somainline.org> wrote:
> >>
> >> Il 01/02/21 18:31, Rob Clark ha scritto:
> >>> On Mon, Feb 1, 2021 at 9:18 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>>
> >>>> On Mon, Feb 1, 2021 at 9:05 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>>>
> >>>>> On Mon, Feb 1, 2021 at 7:47 AM Rob Clark <robdclark@gmail.com> wrote:
> >>>>>>
> >>>>>> On Mon, Feb 1, 2021 at 2:11 AM AngeloGioacchino Del Regno
> >>>>>> <angelogioacchino.delregno@somainline.org> wrote:
> >>>>>>>
> >>>>>>> Il 31/01/21 20:50, Rob Clark ha scritto:
> >>>>>>>> On Sat, Jan 9, 2021 at 5:51 AM AngeloGioacchino Del Regno
> >>>>>>>> <angelogioacchino.delregno@somainline.org> wrote:
> >>>>>>>>>
> >>>>>>>>> The VCO rate was being miscalculated due to a big overlook during
> >>>>>>>>> the process of porting this driver from downstream to upstream:
> >>>>>>>>> here we are really recalculating the rate of the VCO by reading
> >>>>>>>>> the appropriate registers and returning a real frequency, while
> >>>>>>>>> downstream the driver was doing something entirely different.
> >>>>>>>>>
> >>>>>>>>> In our case here, the recalculated rate was wrong, as it was then
> >>>>>>>>> given back to the set_rate function, which was erroneously doing
> >>>>>>>>> a division on the fractional value, based on the prescaler being
> >>>>>>>>> either enabled or disabled: this was actually producing a bug for
> >>>>>>>>> which the final VCO rate was being doubled, causing very obvious
> >>>>>>>>> issues when trying to drive a DSI panel because the actual divider
> >>>>>>>>> value was multiplied by two!
> >>>>>>>>>
> >>>>>>>>> To make things work properly, remove the multiplication of the
> >>>>>>>>> reference clock by two from function dsi_pll_calc_dec_frac and
> >>>>>>>>> account for the prescaler enablement in the vco_recalc_rate (if
> >>>>>>>>> the prescaler is enabled, then the hardware will divide the rate
> >>>>>>>>> by two).
> >>>>>>>>>
> >>>>>>>>> This will make the vco_recalc_rate function to pass the right
> >>>>>>>>> frequency to the (clock framework) set_rate function when called,
> >>>>>>>>> which will - in turn - program the right values in both the
> >>>>>>>>> DECIMAL_DIV_START_1 and the FRAC_DIV_START_{LOW/MID/HIGH}_1
> >>>>>>>>> registers, finally making the PLL to output the right clock.
> >>>>>>>>>
> >>>>>>>>> Also, while at it, remove the prescaler TODO by also adding the
> >>>>>>>>> possibility of disabling the prescaler on the PLL (it is in the
> >>>>>>>>> PLL_ANALOG_CONTROLS_ONE register).
> >>>>>>>>> Of course, both prescaler-ON and OFF cases were tested.
> >>>>>>>>
> >>>>>>>> This somehow breaks things on sc7180 (display gets stuck at first
> >>>>>>>> frame of splash screen).  (This is a setup w/ an ti-sn65dsi86 dsi->eDP
> >>>>>>>> bridge)
> >>>>>>>>
> >>>>>>>
> >>>>>>> First frame of the splash means that something is "a bit" wrong...
> >>>>>>> ...like the DSI clock is a little off.
> >>>>>>>
> >>>>>>> I don't have such hardware, otherwise I would've tried... but what you
> >>>>>>> describe is a bit strange.
> >>>>>>> Is there any other older qcom platform using this chip? Any other
> >>>>>>> non-qcom platform? Is the driver for the SN65DSI86 surely fine?
> >>>>>>> Anyway, as you know, I would never propose untested patches nor
> >>>>>>> partially working ones for any reason: I'm sorry that this happened.
> >>>>>>
> >>>>>> I don't think there is anything publicly avail w/ sc7180 (yet.. but very soon)
> >>>>>>
> >>>>>> The ti-sn65dsi86 bridge is used on a bunch of 845/850 devices (like
> >>>>>> the snapdragon windows laptops).. and I think also the older 835
> >>>>>> laptops.. ofc that doesn't mean that there isn't some bug, but I'd
> >>>>>> guess maybe more likely that there is some small difference in DSI vs
> >>>>>> older devices, or some cmd vs video mode difference.
> >>>>>>
> >>>>>> Anyways, seems like the screen did eventually recover so that gives me
> >>>>>> a bit of confidence to bisect this series, which I'll do a bit later
> >>>>>> today.
> >>>>>
> >>>>> fwiw, this series minus this patch, and everything looks ok.. let me
> >>>>> take a closer look at what changes with this patch
> >>>>
> >>>> Btw, it looks like upstream, config->disable_prescaler is always
> >>>> false.. I don't suppose you have anything WIP that changes this?
> >>>
> >>
> >> Regarding that one, I have tested the driver in both cases, with
> >> and without prescaler enabled (both worked fine), then I have decided
> >> to leave the prescaler option exactly as the previous default.
> >>
> >> My plan about this was/still is:
> >> 1. Wait until this one gets merged (gives me time to also look
> >>      at the other billion patches that I've sent);
> >> 2. Add the prescaler option DT property and explain that it has
> >>      to be used only with "puny" displays (low resolution, low
> >>      clocks) as with "good ones", enabling the prescaler gives less
> >>      clock jitter (and some microamps more power consumption);
> >> 3. Add the Spread Spectrum Clock (SSC) functionality with related
> >>      DT properties.
> >>
> >> Point 2 and 3 would go in the same series, unless someone does
> >> N.2 before I do... and N.3 requires a bit of extensive testing,
> >> which I have already partially started on the FxTec phone.
> >>
> >>> fwiw, this is the clk_summary diff with and without this patch:
> >>>
> >>> ------------------
> >>> 270,282c270,282
> >>> <     dsi0_pll_out_div_clk              1        1        0
> >>> 887039941          0     0  50000         Y
> >>> <        dsi0_pll_post_out_div_clk       0        0        0
> >>> 221759985          0     0  50000         Y
> >>> <        dsi0_pll_bit_clk               2        2        0
> >>> 887039941          0     0  50000         Y
> >>> <           dsi0_pclk_mux               1        1        0
> >>> 887039941          0     0  50000         Y
> >>> <              dsi0_phy_pll_out_dsiclk       1        1        0
> >>> 147839991          0     0  50000         Y
> >>> <                 disp_cc_mdss_pclk0_clk_src       1        1        0
> >>>     147839991          0     0  50000         Y
> >>> <                    disp_cc_mdss_pclk0_clk       1        1        0
> >>>    147839991          0     0  50000         Y
> >>> <           dsi0_pll_by_2_bit_clk       0        0        0
> >>> 443519970          0     0  50000         Y
> >>> <           dsi0_phy_pll_out_byteclk       1        1        0
> >>> 110879992          0     0  50000         Y
> >>> <              disp_cc_mdss_byte0_clk_src       2        2        0
> >>> 110879992          0     0  50000         Y
> >>> <                 disp_cc_mdss_byte0_div_clk_src       1        1
> >>>     0    55439996          0     0  50000         Y
> >>> <                    disp_cc_mdss_byte0_intf_clk       1        1
> >>>     0    55439996          0     0  50000         Y
> >>> <                 disp_cc_mdss_byte0_clk       1        1        0
> >>> 110879992          0     0  50000         Y
> >>> ---
> >>>>       dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
> >>>>          dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
> >>>>          dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
> >>>>             dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
> >>>>                dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
> >>>>                   disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
> >>>>                      disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
> >>>>             dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
> >>>>             dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
> >>>>                disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
> >>>>                   disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
> >>>>                      disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
> >>>>                   disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
> >>> ------------------
> >>>
> >>>
> >>
> >> This is almost exactly what I saw on my devices as well, you get a
> >> difference of "just some Hz" (which can be totally ignored), because
> >> of how the calculation is done now.
> >>
> >> Thing is, what you see as PIXEL and BYTE clocks *before* the change is
> >> Linux thinking that your DSI is at that frequency, while the PLL will
> >> output *half* the rate, which is exactly what the patch fixes.
> >>
> >> "Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same
> >> display... by lowering the DSI rate on the MSM8996 phone by half, I
> >> get the same *identical* issues as the 8998 one without this patch.
> >> The clocks all match between one and another, because.. it's.. the same
> >> display, after all.
> >>
> >> It is because of the aforementioned test that I have raised doubts about
> >> the TI chip driver (or anything else really).. but then, anything is
> >> possible.
> >
> > It does look like, *so far* the TI bridge chip is only used on qc
> > platforms (according to grep'ing dts), so I suppose I can't rule out
> > bugs which cancel each other out.  Although there are various other
> > bridges used (for ex, the sdm845 rb3 board has some dsi->hdmi bridge)
> >
>
> Argh...
>
> > I guess it would be useful if we could measure the clk somehow to
> > confirm that it is running at the rate we think it is..
> >
>
> I totally agree with you on this, I actually wanted to do it the proper
> way, but then these clocks are really too high for my cheap oscilloscope
> and I couldn't... :(

Ok, there might actually be a way to do this.. there is a testclock
utility (added sboyd who wrote it in CC):

https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/refs/heads/main/chipset-qc845/dev-util/testclock/files/testclock.py

there is a similar thing for sc7180.. for other devices, I guess it
would require some porting..

Looking at disp_cc_mdss_byte0_clk and disp_cc_mdss_pclk0_clk on
sc7180, the rates returned by testclock roughly match what is in
clk_summary, ie. within rounding error

BR,
-R

> >
> >
> >>>>>
> >>>>>>> In any case, just to be perfectly transparent, while being here waiting
> >>>>>>> for review, this patch series got tested on more smartphones, even ones
> >>>>>>> that I don't personally own, with different displays.
> >>>>>>>
> >>>>>>> For your reference, here's a list (all MSM8998..):
> >>>>>>> - OnePlus 5               (1920x1080)
> >>>>>>> - F(x)Tec Pro 1           (2160x1080)
> >>>>>>> - Sony Xperia XZ1 Compact (1280x720)
> >>>>>>> - Sony Xperia XZ1         (1920x1080)
> >>>>>>> - Sony Xperia XZ Premium  (3840x2160)
> >>>>>>>
> >>>>>>
> >>>>>> Yeah, no worries, I wasn't trying to imply that the patch was untested.
> >>>>>>
> >>
> >> I know, of course!
> >>
> >>>>>> Out of curiosity, are any of those video mode panels?
> >>
> >> Yes and "also":
> >> The FxTec Pro1 has a video mode panel, for which I'm trying to upstream
> >> the driver...look here: https://lore.kernel.org/patchwork/patch/1365228/
> >>
> >> The Xperia XZ Premium has a Sharp LS055D1SX04 panel under NT35950, which
> >> can be configured as command or as video mode... I tried both modes, but
> >> there is some issue with the DPU1/DSI drivers and *DUAL DSI*, as cmd
> >> does work with some tearing, but video doesn't even start (downstream it
> >> works).
> >>
> >> So the only video mode panel that I could test is that BOE panel on the
> >> FxTec phone (single dsi), which works just great.
> >>
> >>>>>>
> >>>>>>>
> >>>>>>>> Also, something (I assume DSI related) that I was testing on
> >>>>>>>> msm-next-staging seems to have effected the colors on the panel (ie.
> >>>>>>>> they are more muted).. which seems to persist across reboots (ie. when
> >>>>>>>
> >>>>>>> So much "fun". This makes me think something about the PCC block doing
> >>>>>>> the wrong thing (getting misconfigured).
> >>>>>>>
> >>>>>>>> switching back to a good kernel), and interestingly if I reboot from a
> >>>>>>>> good kernel I see part of the login prompt (or whatever was previously
> >>>>>>>> on-screen) in the firmware ui screen !?!  (so maybe somehow triggered
> >>>>>>>> the display to think it is in PSR mode??)
> >>>>>>>>
> >>>>>>>
> >>>>>>>    From a fast read, the SN65DSI86 is on I2C.. giving it a wrong dsi clock
> >>>>>>> cannot produce (logically, at least) this, so I say that it is very
> >>>>>>> unlikely for this to be a consequence of the 10nm pll fixes...
> >>>>>>>
> >>>>>>
> >>>>>> Note that the bridge can also be programmed via dsi cmd mode packets,
> >>>>>> which I believe is the case on the 835 laptops (or at least one of
> >>>>>> them).. but all the things I have are using i2c as the control path.
> >>>>>>
> >>>>>>> ...unless the bootloader is not configuring the DSI rates, but that's
> >>>>>>> also veeeeery unlikely (it always does, or it always does not).
> >>>>>>
> >>>>>> I haven't looked at the bootloader display code, but booting back to
> >>>>>> an old/good kernel didn't change anything..  even powering off didn't.
> >>>>>> But the ghost image seemed to fade after some time, and by the next
> >>>>>> morning it was fine.  Which is strange. (But tbf, I'm more a gpu guy
> >>>>>> who works on display only when necessary.. ie. a gpu without a display
> >>>>>> isn't so much fun ;-))
> >>>>>>
> >>
> >> OpenCL all the way! lol :D
> >>
> >> On Qualcomm platforms, the first thing that I've ever done was to bring
> >> up displays on 8974 Sony platforms... (we're talking about years ago).
> >>
> >> I'm a lil more on the display side of things (but growing a beard while
> >> waiting between a frame and another due to no GPU isn't so much fun
> >> either!).
> >>
> >>>>>>>> Not sure if that is caused by these patches, but if I can figure out
> >>>>>>>> how to get the panel back to normal I can bisect.  I think for now
> >>>>>>>> I'll drop this series.  Possibly it could be a
> >>>>>>>> two-wrongs-makes-a-right situation that had things working before, but
> >>>>>>>> I think someone from qcom who knows the DSI IP should take a look.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I would be happy if someone from Qualcomm takes a look: after all, there
> >>>>>>> is no documentation and they're the only ones that can verify this kind
> >>>>>>> of stuff. Please, Qualcomm.
> >>>>>>
> >>>>>> Hopefully someone can take a look.
> >>>>>>
> >>>>>>> Besides that, if there's anything I can help with to solve this riddle,
> >>>>>>> I'm here for you.
> >>>>>>
> >>>>>> Thanks, like I said I'll try applying the patches one by one and see
> >>>>>> if I can narrow down what made the panel go funny, and we can go from
> >>>>>> there
> >>>>>>
> >>>>>> BR,
> >>>>>> -R
> >>>>>>
> >>>>>>> Yours,
> >>>>>>> -- Angelo
> >>>>>>>
> >>>>>>>> BR,
> >>>>>>>> -R
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> >>>>>>>>> ---
> >>>>>>>>>     drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c | 22 +++++++++-------------
> >>>>>>>>>     1 file changed, 9 insertions(+), 13 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>>>>>>>> index 8b66e852eb36..5be562dfbf06 100644
> >>>>>>>>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_10nm.c
> >>>>>>>>> @@ -165,11 +165,7 @@ static void dsi_pll_calc_dec_frac(struct dsi_pll_10nm *pll)
> >>>>>>>>>
> >>>>>>>>>            pll_freq = pll->vco_current_rate;
> >>>>>>>>>
> >>>>>>>>> -       if (config->disable_prescaler)
> >>>>>>>>> -               divider = fref;
> >>>>>>>>> -       else
> >>>>>>>>> -               divider = fref * 2;
> >>>>>>>>> -
> >>>>>>>>> +       divider = fref;
> >>>>>>>>>            multiplier = 1 << config->frac_bits;
> >>>>>>>>>            dec_multiple = div_u64(pll_freq * multiplier, divider);
> >>>>>>>>>            dec = div_u64_rem(dec_multiple, multiplier, &frac);
> >>>>>>>>> @@ -266,9 +262,11 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll)
> >>>>>>>>>
> >>>>>>>>>     static void dsi_pll_config_hzindep_reg(struct dsi_pll_10nm *pll)
> >>>>>>>>>     {
> >>>>>>>>> +       struct dsi_pll_config *config = &pll->pll_configuration;
> >>>>>>>>>            void __iomem *base = pll->mmio;
> >>>>>>>>> +       u32 val = config->disable_prescaler ? 0x0 : 0x80;
> >>>>>>>>>
> >>>>>>>>> -       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
> >>>>>>>>> +       pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, val);
> >>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
> >>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
> >>>>>>>>>            pll_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
> >>>>>>>>> @@ -499,17 +497,15 @@ static unsigned long dsi_pll_10nm_vco_recalc_rate(struct clk_hw *hw,
> >>>>>>>>>            frac |= ((pll_read(base + REG_DSI_10nm_PHY_PLL_FRAC_DIV_START_HIGH_1) &
> >>>>>>>>>                      0x3) << 16);
> >>>>>>>>>
> >>>>>>>>> -       /*
> >>>>>>>>> -        * TODO:
> >>>>>>>>> -        *      1. Assumes prescaler is disabled
> >>>>>>>>> -        */
> >>>>>>>>>            multiplier = 1 << config->frac_bits;
> >>>>>>>>> -       pll_freq = dec * (ref_clk * 2);
> >>>>>>>>> -       tmp64 = (ref_clk * 2 * frac);
> >>>>>>>>> +       pll_freq = dec * ref_clk;
> >>>>>>>>> +       tmp64 = ref_clk * frac;
> >>>>>>>>>            pll_freq += div_u64(tmp64, multiplier);
> >>>>>>>>> -
> >>>>>>>>>            vco_rate = pll_freq;
> >>>>>>>>>
> >>>>>>>>> +       if (config->disable_prescaler)
> >>>>>>>>> +               vco_rate = div_u64(vco_rate, 2);
> >>>>>>>>> +
> >>>>>>>>>            DBG("DSI PLL%d returning vco rate = %lu, dec = %x, frac = %x",
> >>>>>>>>>                pll_10nm->id, (unsigned long)vco_rate, dec, frac);
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> 2.29.2
> >>>>>>>>>
> >>>>>>>
> >>
>

Re: [PATCH 3/5] drm/msm/dsi_pll_10nm: Fix bad VCO rate calculation and prescaler

[Rob Clark]

On Tue, Feb 2, 2021 at 11:08 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Feb 2, 2021 at 10:46 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
> >
> > Il 02/02/21 19:45, Rob Clark ha scritto:
> > > On Tue, Feb 2, 2021 at 6:32 AM AngeloGioacchino Del Regno
> > > <angelogioacchino.delregno@somainline.org> wrote:
> > >>
> > >> Il 01/02/21 18:31, Rob Clark ha scritto:
> > >>> fwiw, this is the clk_summary diff with and without this patch:
> > >>>
> > >>> ------------------
> > >>> 270,282c270,282
> > >>> <     dsi0_pll_out_div_clk              1        1        0
> > >>> 887039941          0     0  50000         Y
> > >>> <        dsi0_pll_post_out_div_clk       0        0        0
> > >>> 221759985          0     0  50000         Y
> > >>> <        dsi0_pll_bit_clk               2        2        0
> > >>> 887039941          0     0  50000         Y
> > >>> <           dsi0_pclk_mux               1        1        0
> > >>> 887039941          0     0  50000         Y
> > >>> <              dsi0_phy_pll_out_dsiclk       1        1        0
> > >>> 147839991          0     0  50000         Y
> > >>> <                 disp_cc_mdss_pclk0_clk_src       1        1        0
> > >>>     147839991          0     0  50000         Y
> > >>> <                    disp_cc_mdss_pclk0_clk       1        1        0
> > >>>    147839991          0     0  50000         Y
> > >>> <           dsi0_pll_by_2_bit_clk       0        0        0
> > >>> 443519970          0     0  50000         Y
> > >>> <           dsi0_phy_pll_out_byteclk       1        1        0
> > >>> 110879992          0     0  50000         Y
> > >>> <              disp_cc_mdss_byte0_clk_src       2        2        0
> > >>> 110879992          0     0  50000         Y
> > >>> <                 disp_cc_mdss_byte0_div_clk_src       1        1
> > >>>     0    55439996          0     0  50000         Y
> > >>> <                    disp_cc_mdss_byte0_intf_clk       1        1
> > >>>     0    55439996          0     0  50000         Y
> > >>> <                 disp_cc_mdss_byte0_clk       1        1        0
> > >>> 110879992          0     0  50000         Y
> > >>> ---
> > >>>>       dsi0_pll_out_div_clk              1        1        0   887039978          0     0  50000         Y
> > >>>>          dsi0_pll_post_out_div_clk       0        0        0   221759994          0     0  50000         Y
> > >>>>          dsi0_pll_bit_clk               2        2        0   887039978          0     0  50000         Y
> > >>>>             dsi0_pclk_mux               1        1        0   887039978          0     0  50000         Y
> > >>>>                dsi0_phy_pll_out_dsiclk       1        1        0   147839997          0     0  50000         Y
> > >>>>                   disp_cc_mdss_pclk0_clk_src       1        1        0   147839997          0     0  50000         Y
> > >>>>                      disp_cc_mdss_pclk0_clk       1        1        0   147839997          0     0  50000         Y
> > >>>>             dsi0_pll_by_2_bit_clk       0        0        0   443519989          0     0  50000         Y
> > >>>>             dsi0_phy_pll_out_byteclk       1        1        0   110879997          0     0  50000         Y
> > >>>>                disp_cc_mdss_byte0_clk_src       2        2        0   110879997          0     0  50000         Y
> > >>>>                   disp_cc_mdss_byte0_div_clk_src       1        1        0    55439999          0     0  50000         Y
> > >>>>                      disp_cc_mdss_byte0_intf_clk       1        1        0    55439999          0     0  50000         Y
> > >>>>                   disp_cc_mdss_byte0_clk       1        1        0   110879997          0     0  50000         Y
> > >>> ------------------
> > >>>
> > >>>
> > >>
> > >> This is almost exactly what I saw on my devices as well, you get a
> > >> difference of "just some Hz" (which can be totally ignored), because
> > >> of how the calculation is done now.
> > >>
> > >> Thing is, what you see as PIXEL and BYTE clocks *before* the change is
> > >> Linux thinking that your DSI is at that frequency, while the PLL will
> > >> output *half* the rate, which is exactly what the patch fixes.
> > >>
> > >> "Fun" story is: the Xperia XZ1 (8998) and XZ (8996) have got the same
> > >> display... by lowering the DSI rate on the MSM8996 phone by half, I
> > >> get the same *identical* issues as the 8998 one without this patch.
> > >> The clocks all match between one and another, because.. it's.. the same
> > >> display, after all.
> > >>
> > >> It is because of the aforementioned test that I have raised doubts about
> > >> the TI chip driver (or anything else really).. but then, anything is
> > >> possible.
> > >
> > > It does look like, *so far* the TI bridge chip is only used on qc
> > > platforms (according to grep'ing dts), so I suppose I can't rule out
> > > bugs which cancel each other out.  Although there are various other
> > > bridges used (for ex, the sdm845 rb3 board has some dsi->hdmi bridge)
> > >
> >
> > Argh...
> >
> > > I guess it would be useful if we could measure the clk somehow to
> > > confirm that it is running at the rate we think it is..
> > >
> >
> > I totally agree with you on this, I actually wanted to do it the proper
> > way, but then these clocks are really too high for my cheap oscilloscope
> > and I couldn't... :(
>
> Ok, there might actually be a way to do this.. there is a testclock
> utility (added sboyd who wrote it in CC):
>
> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/refs/heads/main/chipset-qc845/dev-util/testclock/files/testclock.py
>
> there is a similar thing for sc7180.. for other devices, I guess it
> would require some porting..
>
> Looking at disp_cc_mdss_byte0_clk and disp_cc_mdss_pclk0_clk on
> sc7180, the rates returned by testclock roughly match what is in
> clk_summary, ie. within rounding error
>

So Doug Anderson pointed out that we can actually read the DSI clock
from the bridge, if we put it in "automatic" mode, from him:

1. Boot up.
2. i2cget -f -y 2 0x2d 0x12 # reads calculated rate that we programmed
3. i2cset -f -y 2 0x2d 0x12 0 # switch to automatic mode
4. i2cget -f -y 2 0x2d 0x12 # reads measured rate; repeat this a bunch
and it should go up/down by 1 since measurement isn't perfect.

What I see without this patch:

localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cset -f -y 2 0x2d 0x12 0
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x59
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ #

And with this patch:

localhost ~ # i2cget -f -y 2 0x2d 0x12
0x58
localhost ~ # i2cset -f -y 2 0x2d 0x12 0
localhost ~ # i2cget -f -y 2 0x2d 0x12
0xb1
localhost ~ # i2cget -f -y 2 0x2d 0x12
0xb2
localhost ~ # i2cget -f -y 2 0x2d 0x12
0xb2
localhost ~ #

So this patch is doubling the rate

BR,
-R