Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1564154 > unrolled thread

[PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration

Started byJohn Keeping <john@metanate.com>
First post2017-01-21 17:40 +0100
Last post2017-01-24 03:50 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration John Keeping <john@metanate.com> - 2017-01-21 17:40 +0100
    Re: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL  configuration Chris Zhong <zyw@rock-chips.com> - 2017-01-23 02:40 +0100
      Re: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL  configuration John Keeping <john@metanate.com> - 2017-01-23 14:10 +0100
        Re: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL  configuration Chris Zhong <zyw@rock-chips.com> - 2017-01-24 03:50 +0100

#1564154 — [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration

FromJohn Keeping <john@metanate.com>
Date2017-01-21 17:40 +0100
Subject[PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration
Message-ID<t26RP-6cn-7@gated-at.bofh.it>
The multiplication ratio for the PLL is required to be even due to the
use of a "by 2 pre-scaler".  Currently we are likely to end up with an
odd multiplier even though there is an equivalent set of parameters with
an even multiplier.

For example, using the 324MHz bit rate with a reference clock of 24MHz
we end up with M = 27, N = 2 whereas the example in the PHY databook
gives M = 54, N = 4 for this bit rate and reference clock.

By walking down through the available multiplier instead of up we are
more likely to hit an even multiplier.  With the above example we do now
get M = 54, N = 4 as given by the databook.

While doing this, change the loop limits to encode the actual limits on
the divisor, which are:

	40MHz >= (pllref / N) >= 5MHz

Signed-off-by: John Keeping <john@metanate.com>
---
Unchanged in v2
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
index 12432e41971b..f2320cf1366c 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
@@ -519,7 +519,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
 	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
 	tmp = pllref;
 
-	for (i = 1; i < 6; i++) {
+	for (i = pllref / 5; i > (pllref / 40); i--) {
 		pre = pllref / i;
 		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
 			tmp = target_mbps % pre;
-- 
2.11.0.197.gb556de5.dirty

[toc] | [next] | [standalone]

#1564630 — Re: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration

FromChris Zhong <zyw@rock-chips.com>
Date2017-01-23 02:40 +0100
SubjectRe: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration
Message-ID<t2BLX-8p1-3@gated-at.bofh.it>
In reply to#1564154
Hi John

On 01/22/2017 12:31 AM, John Keeping wrote:
> The multiplication ratio for the PLL is required to be even due to the
> use of a "by 2 pre-scaler".  Currently we are likely to end up with an
> odd multiplier even though there is an equivalent set of parameters with
> an even multiplier.
>
> For example, using the 324MHz bit rate with a reference clock of 24MHz
> we end up with M = 27, N = 2 whereas the example in the PHY databook
> gives M = 54, N = 4 for this bit rate and reference clock.
>
> By walking down through the available multiplier instead of up we are
> more likely to hit an even multiplier.  With the above example we do now
> get M = 54, N = 4 as given by the databook.
>
> While doing this, change the loop limits to encode the actual limits on
> the divisor, which are:
>
> 	40MHz >= (pllref / N) >= 5MHz

This formula is limit for N, but we still can not guarantee to get an 
even M.
Do you think we should do a check for M.
such as:
if (m % 2)
     continue;
...
     for (i = pllref / 5; i > (pllref / 40); i--) {
         pre = pllref / i;
         if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
             tmp = target_mbps % pre;
             n = i;
             m = target_mbps / pre;
             if (m % 2)
                 continue;
         }
         if (tmp == 0)
             break;
     }

if (m % 2)
     m++;

     dsi->lane_mbps = pllref / n * m;
     dsi->input_div = n;
     dsi->feedback_div = m;



>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
> Unchanged in v2
> ---
>   drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 12432e41971b..f2320cf1366c 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -519,7 +519,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>   	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
>   	tmp = pllref;
>   
> -	for (i = 1; i < 6; i++) {
> +	for (i = pllref / 5; i > (pllref / 40); i--) {
>   		pre = pllref / i;
>   		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
>   			tmp = target_mbps % pre;

[toc] | [prev] | [next] | [standalone]

#1564967 — Re: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration

FromJohn Keeping <john@metanate.com>
Date2017-01-23 14:10 +0100
SubjectRe: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration
Message-ID<t2MxH-6Rg-9@gated-at.bofh.it>
In reply to#1564630
Hi Chris,

On Mon, 23 Jan 2017 09:38:54 +0800, Chris Zhong wrote:
> On 01/22/2017 12:31 AM, John Keeping wrote:
> > The multiplication ratio for the PLL is required to be even due to the
> > use of a "by 2 pre-scaler".  Currently we are likely to end up with an
> > odd multiplier even though there is an equivalent set of parameters with
> > an even multiplier.
> >
> > For example, using the 324MHz bit rate with a reference clock of 24MHz
> > we end up with M = 27, N = 2 whereas the example in the PHY databook
> > gives M = 54, N = 4 for this bit rate and reference clock.
> >
> > By walking down through the available multiplier instead of up we are
> > more likely to hit an even multiplier.  With the above example we do now
> > get M = 54, N = 4 as given by the databook.
> >
> > While doing this, change the loop limits to encode the actual limits on
> > the divisor, which are:
> >
> > 	40MHz >= (pllref / N) >= 5MHz  
> 
> This formula is limit for N, but we still can not guarantee to get an 
> even M.
> Do you think we should do a check for M.
> such as:
> if (m % 2)
>      continue;
> ...
>      for (i = pllref / 5; i > (pllref / 40); i--) {
>          pre = pllref / i;
>          if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
>              tmp = target_mbps % pre;
>              n = i;
>              m = target_mbps / pre;
>              if (m % 2)
>                  continue;
>          }
>          if (tmp == 0)
>              break;
>      }
> 
> if (m % 2)
>      m++;
> 
>      dsi->lane_mbps = pllref / n * m;
>      dsi->input_div = n;
>      dsi->feedback_div = m;

Yes, I agree that there should be a check for M, but I'm not sure if
the version above is sufficient.  The "m % 2" check inside the loop
means that we don't break immediately when tmp=0 but then we are
guaranteed to break next time without having modified n, m because now
tmp=0 so "tmp > (target_mbps % pre)" is always false and we just hit the
"if (tmp == 0) break" case next time.

Given that the descending loop already means that if we can hit "tmp"
exactly we are more likely to do so with a bigger N and even M, I think
it might be better to just fix M after the loop like:

	if (m % 2) {
		if (m < 256 && (n * 2) <= (pllref / 5)) {
			n *= 2;
			m *= 2;
		} else {
			m++;
		}
	}

but I haven't thought about this too carefully.

For this series, I'd rather either keep this patch as it is or drop it
in favour of a more comprehensive solution.  I don't want to block the
other fixes waiting for a perfect fix here and we can always improve
this further with a follow-up patch.

> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> > Unchanged in v2
> > ---
> >   drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > index 12432e41971b..f2320cf1366c 100644
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > @@ -519,7 +519,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
> >   	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
> >   	tmp = pllref;
> >   
> > -	for (i = 1; i < 6; i++) {
> > +	for (i = pllref / 5; i > (pllref / 40); i--) {
> >   		pre = pllref / i;
> >   		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
> >   			tmp = target_mbps % pre;  
> 
>

[toc] | [prev] | [next] | [standalone]

#1565452 — Re: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration

FromChris Zhong <zyw@rock-chips.com>
Date2017-01-24 03:50 +0100
SubjectRe: [PATCH v2 19/26] drm/rockchip: dw-mipi-dsi: improve PLL configuration
Message-ID<t2Zlg-6Jz-9@gated-at.bofh.it>
In reply to#1564967

On 01/23/2017 08:49 PM, John Keeping wrote:
> Hi Chris,
>
> On Mon, 23 Jan 2017 09:38:54 +0800, Chris Zhong wrote:
>> On 01/22/2017 12:31 AM, John Keeping wrote:
>>> The multiplication ratio for the PLL is required to be even due to the
>>> use of a "by 2 pre-scaler".  Currently we are likely to end up with an
>>> odd multiplier even though there is an equivalent set of parameters with
>>> an even multiplier.
>>>
>>> For example, using the 324MHz bit rate with a reference clock of 24MHz
>>> we end up with M = 27, N = 2 whereas the example in the PHY databook
>>> gives M = 54, N = 4 for this bit rate and reference clock.
>>>
>>> By walking down through the available multiplier instead of up we are
>>> more likely to hit an even multiplier.  With the above example we do now
>>> get M = 54, N = 4 as given by the databook.
>>>
>>> While doing this, change the loop limits to encode the actual limits on
>>> the divisor, which are:
>>>
>>> 	40MHz >= (pllref / N) >= 5MHz
>> This formula is limit for N, but we still can not guarantee to get an
>> even M.
>> Do you think we should do a check for M.
>> such as:
>> if (m % 2)
>>       continue;
>> ...
>>       for (i = pllref / 5; i > (pllref / 40); i--) {
>>           pre = pllref / i;
>>           if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
>>               tmp = target_mbps % pre;
>>               n = i;
>>               m = target_mbps / pre;
>>               if (m % 2)
>>                   continue;
>>           }
>>           if (tmp == 0)
>>               break;
>>       }
>>
>> if (m % 2)
>>       m++;
>>
>>       dsi->lane_mbps = pllref / n * m;
>>       dsi->input_div = n;
>>       dsi->feedback_div = m;
> Yes, I agree that there should be a check for M, but I'm not sure if
> the version above is sufficient.  The "m % 2" check inside the loop
> means that we don't break immediately when tmp=0 but then we are
> guaranteed to break next time without having modified n, m because now
> tmp=0 so "tmp > (target_mbps % pre)" is always false and we just hit the
> "if (tmp == 0) break" case next time.
>
> Given that the descending loop already means that if we can hit "tmp"
> exactly we are more likely to do so with a bigger N and even M, I think
> it might be better to just fix M after the loop like:
>
> 	if (m % 2) {
> 		if (m < 256 && (n * 2) <= (pllref / 5)) {
> 			n *= 2;
> 			m *= 2;
> 		} else {
> 			m++;
> 		}
> 	}
>
> but I haven't thought about this too carefully.
>
> For this series, I'd rather either keep this patch as it is or drop it
> in favour of a more comprehensive solution.  I don't want to block the
> other fixes waiting for a perfect fix here and we can always improve
> this further with a follow-up patch.
Agree, We can improve the whole formula in the future.

>
>>> Signed-off-by: John Keeping <john@metanate.com>
>>> ---
>>> Unchanged in v2
>>> ---
>>>    drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>> index 12432e41971b..f2320cf1366c 100644
>>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>>> @@ -519,7 +519,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>>>    	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
>>>    	tmp = pllref;
>>>    
>>> -	for (i = 1; i < 6; i++) {
>>> +	for (i = pllref / 5; i > (pllref / 40); i--) {
>>>    		pre = pllref / i;
>>>    		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
>>>    			tmp = target_mbps % pre;
>>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web