[PATCH] clk: qcom: Fix pre-divider usage for pixel RCG

[PATCH] clk: qcom: Fix pre-divider usage for pixel RCG

[Archit Taneja]

The clk_rcg_pixel_set_rate clk_op sets up the pre-divider by reading
its current value from the NS register.

Using the pre-divider wasn't really intended when creating these ops.
The pixel RCG was only intended to achieve fractional multiplication
provided in the pixel_table array. Leaving the pre-divider to the
existing register value results in a wrong pixel clock when the
bootloader sets up the display. This was left unidentified because
the IFC6410 Plus board on which this was verified didn't have a
bootloader that configured the display.

Don't set the RCG pre-divider in freq_tbl to the existing NS register
value. Instead, set it based on the fractional multiplication we want.
Prevent using M/N counter when we can just manage with using the
pre-divider.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Vinay Simha <vinaysimha@inforcecomputing.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
John, Vinay, could you please test this on N7 and confirm if it works?

 drivers/clk/qcom/clk-rcg.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
index bfbb28f..2c033f2 100644
--- a/drivers/clk/qcom/clk-rcg.c
+++ b/drivers/clk/qcom/clk-rcg.c
@@ -638,7 +638,6 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
 		return ret;
 
 	src = ns_to_src(&rcg->s, ns);
-	f.pre_div = ns_to_pre_div(&rcg->p, ns) + 1;
 
 	for (i = 0; i < num_parents; i++) {
 		if (src == rcg->s.parent_map[i].cfg) {
@@ -655,8 +654,13 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
 			(parent_rate > (request + delta)))
 			continue;
 
-		f.m = frac->num;
-		f.n = frac->den;
+		/* try to use only the pre-divider if we can */
+		if (frac->num == 1) {
+			f.pre_div = frac->den;
+		} else {
+			f.m = frac->num;
+			f.n = frac->den;
+		}
 
 		return __clk_rcg_set_rate(rcg, &f);
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] clk: qcom: Fix pre-divider usage for pixel RCG

[Stephen Boyd]

On 02/26, Archit Taneja wrote:
> diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
> index bfbb28f..2c033f2 100644
> --- a/drivers/clk/qcom/clk-rcg.c
> +++ b/drivers/clk/qcom/clk-rcg.c
> @@ -655,8 +654,13 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
>  			(parent_rate > (request + delta)))
>  			continue;
>  
> -		f.m = frac->num;
> -		f.n = frac->den;
> +		/* try to use only the pre-divider if we can */
> +		if (frac->num == 1) {
> +			f.pre_div = frac->den;

What if the pre divider can't support the frac->den value? Maybe
we should just force the pre divider to be in bypass so that we
can use the m/n all the time.

> +		} else {
> +			f.m = frac->num;
> +			f.n = frac->den;
> +		}
>  
>  		return __clk_rcg_set_rate(rcg, &f);
>  	}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: qcom: Fix pre-divider usage for pixel RCG

[John Stultz]

On Thu, Feb 25, 2016 at 9:30 PM, Archit Taneja <architt@codeaurora.org> wrote:
> The clk_rcg_pixel_set_rate clk_op sets up the pre-divider by reading
> its current value from the NS register.
>
> Using the pre-divider wasn't really intended when creating these ops.
> The pixel RCG was only intended to achieve fractional multiplication
> provided in the pixel_table array. Leaving the pre-divider to the
> existing register value results in a wrong pixel clock when the
> bootloader sets up the display. This was left unidentified because
> the IFC6410 Plus board on which this was verified didn't have a
> bootloader that configured the display.
>
> Don't set the RCG pre-divider in freq_tbl to the existing NS register
> value. Instead, set it based on the fractional multiplication we want.
> Prevent using M/N counter when we can just manage with using the
> pre-divider.
>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Vinay Simha <vinaysimha@inforcecomputing.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
> John, Vinay, could you please test this on N7 and confirm if it works?

Hey Archit,

So this did not work for me. I removed the earlier patch forcing
pre_div to 1, and added this, but that ended up with a mostly black
screen with a bit of blue slowly scrolling down the side.

With the f.pre_div=1 line re-added, I don't see anything. The screen
just goes black and doesn't come back.

Let me know if I can test anything else for you, or add any debug messages.

thanks
-john

Re: [PATCH] clk: qcom: Fix pre-divider usage for pixel RCG

[Archit Taneja]

On 2/26/2016 11:14 PM, Stephen Boyd wrote:
> On 02/26, Archit Taneja wrote:
>> diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
>> index bfbb28f..2c033f2 100644
>> --- a/drivers/clk/qcom/clk-rcg.c
>> +++ b/drivers/clk/qcom/clk-rcg.c
>> @@ -655,8 +654,13 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
>>   			(parent_rate > (request + delta)))
>>   			continue;
>>
>> -		f.m = frac->num;
>> -		f.n = frac->den;
>> +		/* try to use only the pre-divider if we can */
>> +		if (frac->num == 1) {
>> +			f.pre_div = frac->den;
>
> What if the pre divider can't support the frac->den value? Maybe
> we should just force the pre divider to be in bypass so that we
> can use the m/n all the time.

Yeah, for some strange reason it seems like having a pre-divider as 3
itself doesn't seem to work. As you suggested, I'll force the pre
divider to 1 and use m/n all the time.

Archit

>
>> +		} else {
>> +			f.m = frac->num;
>> +			f.n = frac->den;
>> +		}
>>
>>   		return __clk_rcg_set_rate(rcg, &f);
>>   	}
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: qcom: Fix pre-divider usage for pixel RCG

[Archit Taneja]

On 2/27/2016 2:10 AM, John Stultz wrote:
> On Thu, Feb 25, 2016 at 9:30 PM, Archit Taneja <architt@codeaurora.org> wrote:
>> The clk_rcg_pixel_set_rate clk_op sets up the pre-divider by reading
>> its current value from the NS register.
>>
>> Using the pre-divider wasn't really intended when creating these ops.
>> The pixel RCG was only intended to achieve fractional multiplication
>> provided in the pixel_table array. Leaving the pre-divider to the
>> existing register value results in a wrong pixel clock when the
>> bootloader sets up the display. This was left unidentified because
>> the IFC6410 Plus board on which this was verified didn't have a
>> bootloader that configured the display.
>>
>> Don't set the RCG pre-divider in freq_tbl to the existing NS register
>> value. Instead, set it based on the fractional multiplication we want.
>> Prevent using M/N counter when we can just manage with using the
>> pre-divider.
>>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Vinay Simha <vinaysimha@inforcecomputing.com>
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> ---
>> John, Vinay, could you please test this on N7 and confirm if it works?
>
> Hey Archit,
>
> So this did not work for me. I removed the earlier patch forcing
> pre_div to 1, and added this, but that ended up with a mostly black
> screen with a bit of blue slowly scrolling down the side.
>
> With the f.pre_div=1 line re-added, I don't see anything. The screen
> just goes black and doesn't come back.
>
> Let me know if I can test anything else for you, or add any debug messages.

Thanks for trying the patch. There was a small optimization which seems
to make things worse for us. I'm going to post out a v2 which does the
same thing as the earlier patch.

Archit

>
> thanks
> -john
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2] clk: qcom: Fix pre-divider usage for pixel RCG

[Archit Taneja]

The clk_rcg_pixel_set_rate clk_op sets up the pre-divider by reading
its current value from the NS register.

Using the pre-divider wasn't really intended when creating these ops.
The pixel RCG was only intended to achieve fractional multiplication
provided in the pixel_table array. Leaving the pre-divider to the
existing register value results in a wrong pixel clock when the
bootloader sets up the display. This was left unidentified because
the IFC6410 Plus board on which this was verified didn't have a
bootloader that configured the display.

Don't set the RCG pre-divider in freq_tbl to the existing NS register
value. Force it to 1 and only use the M/N counter to achieve the desired
fractional multiplication.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Vinay Simha <vinaysimha@inforcecomputing.com>
Signed-off-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/clk/qcom/clk-rcg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
index bfbb28f..67ce7c1 100644
--- a/drivers/clk/qcom/clk-rcg.c
+++ b/drivers/clk/qcom/clk-rcg.c
@@ -638,7 +638,6 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
 		return ret;
 
 	src = ns_to_src(&rcg->s, ns);
-	f.pre_div = ns_to_pre_div(&rcg->p, ns) + 1;
 
 	for (i = 0; i < num_parents; i++) {
 		if (src == rcg->s.parent_map[i].cfg) {
@@ -647,6 +646,9 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
 		}
 	}
 
+	/* bypass the pre divider */
+	f.pre_div = 1;
+
 	/* let us find appropriate m/n values for this */
 	for (; frac->num; frac++) {
 		request = (rate * frac->den) / frac->num;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] clk: qcom: Fix pre-divider usage for pixel RCG

[John Stultz]

On Sun, Feb 28, 2016 at 2:07 AM, Archit Taneja <architt@codeaurora.org> wrote:
> The clk_rcg_pixel_set_rate clk_op sets up the pre-divider by reading
> its current value from the NS register.
>
> Using the pre-divider wasn't really intended when creating these ops.
> The pixel RCG was only intended to achieve fractional multiplication
> provided in the pixel_table array. Leaving the pre-divider to the
> existing register value results in a wrong pixel clock when the
> bootloader sets up the display. This was left unidentified because
> the IFC6410 Plus board on which this was verified didn't have a
> bootloader that configured the display.
>
> Don't set the RCG pre-divider in freq_tbl to the existing NS register
> value. Force it to 1 and only use the M/N counter to achieve the desired
> fractional multiplication.
>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Vinay Simha <vinaysimha@inforcecomputing.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---
>  drivers/clk/qcom/clk-rcg.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/clk-rcg.c b/drivers/clk/qcom/clk-rcg.c
> index bfbb28f..67ce7c1 100644
> --- a/drivers/clk/qcom/clk-rcg.c
> +++ b/drivers/clk/qcom/clk-rcg.c
> @@ -638,7 +638,6 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
>                 return ret;
>
>         src = ns_to_src(&rcg->s, ns);
> -       f.pre_div = ns_to_pre_div(&rcg->p, ns) + 1;
>
>         for (i = 0; i < num_parents; i++) {
>                 if (src == rcg->s.parent_map[i].cfg) {
> @@ -647,6 +646,9 @@ static int clk_rcg_pixel_set_rate(struct clk_hw *hw, unsigned long rate,
>                 }
>         }
>
> +       /* bypass the pre divider */
> +       f.pre_div = 1;
> +
>         /* let us find appropriate m/n values for this */
>         for (; frac->num; frac++) {
>                 request = (rate * frac->den) / frac->num;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>

This one works for me on the nexus7

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john

Re: [PATCH v2] clk: qcom: Fix pre-divider usage for pixel RCG

[Stephen Boyd]

On 02/28, Archit Taneja wrote:
> The clk_rcg_pixel_set_rate clk_op sets up the pre-divider by reading
> its current value from the NS register.
> 
> Using the pre-divider wasn't really intended when creating these ops.
> The pixel RCG was only intended to achieve fractional multiplication
> provided in the pixel_table array. Leaving the pre-divider to the
> existing register value results in a wrong pixel clock when the
> bootloader sets up the display. This was left unidentified because
> the IFC6410 Plus board on which this was verified didn't have a
> bootloader that configured the display.
> 
> Don't set the RCG pre-divider in freq_tbl to the existing NS register
> value. Force it to 1 and only use the M/N counter to achieve the desired
> fractional multiplication.
> 
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Vinay Simha <vinaysimha@inforcecomputing.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project