Patchwork [U-Boot] spi: mxc_spi: Fix pre and post divider calculation

login
register
mail settings
Submitter Dirk Behme
Date May 2, 2013, 10:59 a.m.
Message ID <1367492386-20464-1-git-send-email-dirk.behme@de.bosch.com>
Download mbox | patch
Permalink /patch/240944/
State Superseded
Delegated to: Stefano Babic
Headers show

Comments

Dirk Behme - May 2, 2013, 10:59 a.m.
From: Dirk Behme <dirk.behme@gmail.com>

Fix two issues with the calculation of pre_div and post_div:

1. pre_div: While the calculation of pre_div looks correct, to set the
CONREG[15-12] bits pre_div needs to be decremented by 1:

The i.MX 6Dual/6Quad Applications Processor Reference Manual (IMX6DQRM
Rev. 0, 11/2012) states:

CONREG[15-12]: PRE_DIVIDER
0000 Divide by 1
0001 Divide by 2
0010 Divide by 3
...
1101 Divide by 14
1110 Divide by 15
1111 Divide by 16

I.e. if we want to divide by 2, we have to write 1 to CONREG[15-12].

2. In case the post divider becomes necessary, pre_div will be set to 15. To
calculate the post divider, divide by 15, too. And not 16.

Both issues above are tested using the following examples:

clk_src = 60000000 (60MHz, default i.MX6 ECSPI clock)

a) max_hz == 23000000 (23MHz, max i.MX6 ECSPI read clock)

-> pre_div =  3 (divide by 3 => CONREG[15-12] == 2)
-> post_div = 0 (divide by 1 => CONREG[11- 8] == 0)
               => 60MHz / 3 = 20MHz SPI clock

b) max_hz == 2000000 (2MHz)

-> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
-> post_div = 1  (divide by  2 => CONREG[11- 8] == 1)
               => 60MHz / 30 = 2MHz SPI clock

c) max_hz == 1000000 (1MHz)

-> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
-> post_div = 2  (divide by  4 => CONREG[11- 8] == 2)
               => 60MHz / 60 = 1MHz SPI clock

d) max_hz == 500000 (500kHz)

-> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
-> post_div = 3  (divide by  8 => CONREG[11- 8] == 3)
               => 60MHz / 120 = 500kHz SPI clock

Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
---
 drivers/spi/mxc_spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Troy Kisky - May 2, 2013, 6:38 p.m.
On 5/2/2013 3:59 AM, Dirk Behme wrote:
> From: Dirk Behme <dirk.behme@gmail.com>
>
> Fix two issues with the calculation of pre_div and post_div:
>
> 1. pre_div: While the calculation of pre_div looks correct, to set the
> CONREG[15-12] bits pre_div needs to be decremented by 1:
>
> The i.MX 6Dual/6Quad Applications Processor Reference Manual (IMX6DQRM
> Rev. 0, 11/2012) states:
>
> CONREG[15-12]: PRE_DIVIDER
> 0000 Divide by 1
> 0001 Divide by 2
> 0010 Divide by 3
> ...
> 1101 Divide by 14
> 1110 Divide by 15
> 1111 Divide by 16
>
> I.e. if we want to divide by 2, we have to write 1 to CONREG[15-12].
>
> 2. In case the post divider becomes necessary, pre_div will be set to 15. To
> calculate the post divider, divide by 15, too. And not 16.
>
> Both issues above are tested using the following examples:
>
> clk_src = 60000000 (60MHz, default i.MX6 ECSPI clock)
>
> a) max_hz == 23000000 (23MHz, max i.MX6 ECSPI read clock)
>
> -> pre_div =  3 (divide by 3 => CONREG[15-12] == 2)
> -> post_div = 0 (divide by 1 => CONREG[11- 8] == 0)
>                 => 60MHz / 3 = 20MHz SPI clock
>
> b) max_hz == 2000000 (2MHz)
>
> -> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
> -> post_div = 1  (divide by  2 => CONREG[11- 8] == 1)
>                 => 60MHz / 30 = 2MHz SPI clock
>
> c) max_hz == 1000000 (1MHz)
>
> -> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
> -> post_div = 2  (divide by  4 => CONREG[11- 8] == 2)
>                 => 60MHz / 60 = 1MHz SPI clock
>
> d) max_hz == 500000 (500kHz)
>
> -> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
> -> post_div = 3  (divide by  8 => CONREG[11- 8] == 3)
>                 => 60MHz / 120 = 500kHz SPI clock
>
> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
> ---
>   drivers/spi/mxc_spi.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> index 5bed858..8630bbd 100644
> --- a/drivers/spi/mxc_spi.c
> +++ b/drivers/spi/mxc_spi.c
> @@ -128,7 +128,7 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
>   		unsigned int max_hz, unsigned int mode)
>   {
>   	u32 clk_src = mxc_get_clock(MXC_CSPI_CLK);
> -	s32 pre_div = 0, post_div = 0, i, reg_ctrl, reg_config;
> +	s32 pre_div = 1, post_div = 0, i, reg_ctrl, reg_config;
>   	u32 ss_pol = 0, sclkpol = 0, sclkpha = 0;
>   	struct cspi_regs *regs = (struct cspi_regs *)mxcs->base;
>   
> @@ -153,7 +153,7 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
>   	if (clk_src > max_hz) {
>   		pre_div = DIV_ROUND_UP(clk_src, max_hz);
>   		if (pre_div > 16) {
> -			post_div = pre_div / 16;
> +			post_div = pre_div / 15;
I think this should not change
>   			pre_div = 15;
and this should be = 16, because you now subtract 1 below
>   		}
>   		if (post_div != 0) {
> @@ -174,7 +174,7 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
>   	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_SELCHAN(3)) |
>   		MXC_CSPICTRL_SELCHAN(cs);
>   	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_PREDIV(0x0F)) |
> -		MXC_CSPICTRL_PREDIV(pre_div);
> +		MXC_CSPICTRL_PREDIV(pre_div - 1);
>   	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) |
>   		MXC_CSPICTRL_POSTDIV(post_div);
>
Dirk Behme - May 3, 2013, 5:58 a.m.
On 02.05.2013 20:38, Troy Kisky wrote:
> On 5/2/2013 3:59 AM, Dirk Behme wrote:
>> From: Dirk Behme <dirk.behme@gmail.com>
>>
>> Fix two issues with the calculation of pre_div and post_div:
>>
>> 1. pre_div: While the calculation of pre_div looks correct, to set the
>> CONREG[15-12] bits pre_div needs to be decremented by 1:
>>
>> The i.MX 6Dual/6Quad Applications Processor Reference Manual (IMX6DQRM
>> Rev. 0, 11/2012) states:
>>
>> CONREG[15-12]: PRE_DIVIDER
>> 0000 Divide by 1
>> 0001 Divide by 2
>> 0010 Divide by 3
>> ...
>> 1101 Divide by 14
>> 1110 Divide by 15
>> 1111 Divide by 16
>>
>> I.e. if we want to divide by 2, we have to write 1 to CONREG[15-12].
>>
>> 2. In case the post divider becomes necessary, pre_div will be set to 15. To
>> calculate the post divider, divide by 15, too. And not 16.
>>
>> Both issues above are tested using the following examples:
>>
>> clk_src = 60000000 (60MHz, default i.MX6 ECSPI clock)
>>
>> a) max_hz == 23000000 (23MHz, max i.MX6 ECSPI read clock)
>>
>> -> pre_div =  3 (divide by 3 => CONREG[15-12] == 2)
>> -> post_div = 0 (divide by 1 => CONREG[11- 8] == 0)
>>                  => 60MHz / 3 = 20MHz SPI clock
>>
>> b) max_hz == 2000000 (2MHz)
>>
>> -> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
>> -> post_div = 1  (divide by  2 => CONREG[11- 8] == 1)
>>                  => 60MHz / 30 = 2MHz SPI clock
>>
>> c) max_hz == 1000000 (1MHz)
>>
>> -> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
>> -> post_div = 2  (divide by  4 => CONREG[11- 8] == 2)
>>                  => 60MHz / 60 = 1MHz SPI clock
>>
>> d) max_hz == 500000 (500kHz)
>>
>> -> pre_div =  15 (divide by 15 => CONREG[15-12] == 14)
>> -> post_div = 3  (divide by  8 => CONREG[11- 8] == 3)
>>                  => 60MHz / 120 = 500kHz SPI clock
>>
>> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
>> ---
>>    drivers/spi/mxc_spi.c | 6 +++---
>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
>> index 5bed858..8630bbd 100644
>> --- a/drivers/spi/mxc_spi.c
>> +++ b/drivers/spi/mxc_spi.c
>> @@ -128,7 +128,7 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
>>    		unsigned int max_hz, unsigned int mode)
>>    {
>>    	u32 clk_src = mxc_get_clock(MXC_CSPI_CLK);
>> -	s32 pre_div = 0, post_div = 0, i, reg_ctrl, reg_config;
>> +	s32 pre_div = 1, post_div = 0, i, reg_ctrl, reg_config;
>>    	u32 ss_pol = 0, sclkpol = 0, sclkpha = 0;
>>    	struct cspi_regs *regs = (struct cspi_regs *)mxcs->base;
>>
>> @@ -153,7 +153,7 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
>>    	if (clk_src > max_hz) {
>>    		pre_div = DIV_ROUND_UP(clk_src, max_hz);
>>    		if (pre_div > 16) {
>> -			post_div = pre_div / 16;
>> +			post_div = pre_div / 15;
> I think this should not change

Hmm, why? You get wrong post_div dividers if you run with 'pre_div = 15' 
and '/16'. Hmm, reading your below comment do you want to say

"I agree that both lines

post_div = pre_div / 16;
pre_div = 15;

have to use the same value, but instead of using 15, use 16
"

?

>>    			pre_div = 15;
> and this should be = 16, because you now subtract 1 below

Do you want to say you propose

post_div = pre_div / 16;
pre_div = 16;

?

If so:

First, I agree that we have to use the same dividers in both lines.

But, second, this would mean that you use /16 as max pre_div. For the 
i.MX6 case where clk_src is 60MHz this would result in a pre-divided 
clock of 3.75Mhz (instead of 4MHz with /15).

So using /15 or /16 is just a decision of which end clocks most probably 
are needed.

If you want to be able to configure 4MHz, 2MHz, 1MHz, 500kHz etc then 
/15 is the better choice.

If you want to be able to configure 3.75Mhz, 1.875MHz, 937.5kHz, 
468.75kHz etc then /16 is the better choice.

I vote for /15 as done by my patch.

Best regards

Dirk

>>    		}
>>    		if (post_div != 0) {
>> @@ -174,7 +174,7 @@ static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
>>    	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_SELCHAN(3)) |
>>    		MXC_CSPICTRL_SELCHAN(cs);
>>    	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_PREDIV(0x0F)) |
>> -		MXC_CSPICTRL_PREDIV(pre_div);
>> +		MXC_CSPICTRL_PREDIV(pre_div - 1);
>>    	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) |
>>    		MXC_CSPICTRL_POSTDIV(post_div);
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Troy Kisky - May 3, 2013, 8:47 p.m.
On 5/2/2013 10:58 PM, Dirk Behme wrote:
> Do you want to say you propose
>
> post_div = pre_div / 16;
> pre_div = 16;
>
> ?
yes, that's what I said
>
> If so:
>
> First, I agree that we have to use the same dividers in both lines.
>
> But, second, this would mean that you use /16 as max pre_div. For the 
> i.MX6 case where clk_src is 60MHz this would result in a pre-divided 
> clock of 3.75Mhz (instead of 4MHz with /15).

That does sound better for i.MX6, what about other processors using this 
file?

>
> So using /15 or /16 is just a decision of which end clocks most 
> probably are needed.
>
> If you want to be able to configure 4MHz, 2MHz, 1MHz, 500kHz etc then 
> /15 is the better choice.
>
> If you want to be able to configure 3.75Mhz, 1.875MHz, 937.5kHz, 
> 468.75kHz etc then /16 is the better choice.
>
> I vote for /15 as done by my patch.

Thanks for explaining. The downside of using /15 is that you can't get 
the slowest clock possible.
How about restructuring the code to improve both. Calculate post_div first.

pre_div = DIV_ROUND_UP(clk_src, max_hz);
/* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
post_div = fls(pre_div - 1);
if (post_div > 4)
     post_div -= 4;
else
     post_div = 0;

if (post_div >= 16) {
            printf("Error: no divider for the freq: %d\n",
                                         max_hz);
            return -1;
}
pre_div = (pre_div + (1 << post_div) - 1) >> post_div;
Dirk Behme - May 4, 2013, 10:06 a.m.
Am 03.05.2013 22:47, schrieb Troy Kisky:
> On 5/2/2013 10:58 PM, Dirk Behme wrote:
>> Do you want to say you propose
>>
>> post_div = pre_div / 16;
>> pre_div = 16;
>>
>> ?
> yes, that's what I said
>>
>> If so:
>>
>> First, I agree that we have to use the same dividers in both lines.
>>
>> But, second, this would mean that you use /16 as max pre_div. For
>> the i.MX6 case where clk_src is 60MHz this would result in a
>> pre-divided clock of 3.75Mhz (instead of 4MHz with /15).
>
> That does sound better for i.MX6, what about other processors using
> this file?
>
>>
>> So using /15 or /16 is just a decision of which end clocks most
>> probably are needed.
>>
>> If you want to be able to configure 4MHz, 2MHz, 1MHz, 500kHz etc
>> then /15 is the better choice.
>>
>> If you want to be able to configure 3.75Mhz, 1.875MHz, 937.5kHz,
>> 468.75kHz etc then /16 is the better choice.
>>
>> I vote for /15 as done by my patch.
>
> Thanks for explaining. The downside of using /15 is that you can't get
> the slowest clock possible.

Yes. I was looking for the _highest_ clock possible, though ;) And 
this isn't correctly done by the recent code. This is why I was 
looking into it ...

> How about restructuring the code to improve both. Calculate post_div
> first.
>
> pre_div = DIV_ROUND_UP(clk_src, max_hz);
> /* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
> post_div = fls(pre_div - 1);
> if (post_div > 4)
>      post_div -= 4;
> else
>      post_div = 0;
>
> if (post_div >= 16) {
>             printf("Error: no divider for the freq: %d\n",
>                                          max_hz);
>             return -1;
> }
> pre_div = (pre_div + (1 << post_div) - 1) >> post_div;

Using my test code gives the correct values using this algorithm. So 
yes, sounds good.

Just a small note: Wouldn't it be better to put the printf and the 
last line with the pre_div calculation into the if(post_div > 4) part? 
I.e.

pre_div = DIV_ROUND_UP(clk_src, max_hz);
/* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
post_div = fls(pre_div - 1);
if (post_div > 4) {
     post_div -= 4;

     if (post_div >= 16) {
            printf("Error: no divider for the freq: %d\n",
                                         max_hz);
            return -1;
     }
     pre_div = (pre_div + (1 << post_div) - 1) >> post_div;

} else
     post_div = 0;

?

In case we agree on this, I'm thinking about doing 2 patches to make 
clear what we are doing:

1. Re-doing my initial patch with

post_div = pre_div / 16;
pre_div = 16;

This would be the "fix the issues in the existing (non-optimal) 
algorithm but keep that" patch.

2. Replace the existing algorithm with your above version. This would 
be the "improve the algorithm" patch.

What do you think?

Best regards

Dirk
Troy Kisky - May 6, 2013, 6:26 p.m.
On 5/4/2013 3:06 AM, Dirk Behme wrote:
> Am 03.05.2013 22:47, schrieb Troy Kisky:
>> On 5/2/2013 10:58 PM, Dirk Behme wrote:
>>> Do you want to say you propose
>>>
>>> post_div = pre_div / 16;
>>> pre_div = 16;
>>>
>>> ?
>> yes, that's what I said
>>>
>>> If so:
>>>
>>> First, I agree that we have to use the same dividers in both lines.
>>>
>>> But, second, this would mean that you use /16 as max pre_div. For
>>> the i.MX6 case where clk_src is 60MHz this would result in a
>>> pre-divided clock of 3.75Mhz (instead of 4MHz with /15).
>>
>> That does sound better for i.MX6, what about other processors using
>> this file?
>>
>>>
>>> So using /15 or /16 is just a decision of which end clocks most
>>> probably are needed.
>>>
>>> If you want to be able to configure 4MHz, 2MHz, 1MHz, 500kHz etc
>>> then /15 is the better choice.
>>>
>>> If you want to be able to configure 3.75Mhz, 1.875MHz, 937.5kHz,
>>> 468.75kHz etc then /16 is the better choice.
>>>
>>> I vote for /15 as done by my patch.
>>
>> Thanks for explaining. The downside of using /15 is that you can't get
>> the slowest clock possible.
>
> Yes. I was looking for the _highest_ clock possible, though ;) And 
> this isn't correctly done by the recent code. This is why I was 
> looking into it ...
>
>> How about restructuring the code to improve both. Calculate post_div
>> first.
>>
>> pre_div = DIV_ROUND_UP(clk_src, max_hz);
>> /* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
>> post_div = fls(pre_div - 1);
>> if (post_div > 4)
>>      post_div -= 4;
>> else
>>      post_div = 0;
>>
>> if (post_div >= 16) {
>>             printf("Error: no divider for the freq: %d\n",
>>                                          max_hz);
>>             return -1;
>> }
>> pre_div = (pre_div + (1 << post_div) - 1) >> post_div;
>
> Using my test code gives the correct values using this algorithm. So 
> yes, sounds good.
>
> Just a small note: Wouldn't it be better to put the printf and the 
> last line with the pre_div calculation into the if(post_div > 4) part? 
> I.e.
>
> pre_div = DIV_ROUND_UP(clk_src, max_hz);
> /* fls(1) = 1, fls(0x80000000) = 32, fls(16) = 5 */
> post_div = fls(pre_div - 1);
> if (post_div > 4) {
>     post_div -= 4;
>
>     if (post_div >= 16) {
>            printf("Error: no divider for the freq: %d\n",
>                                         max_hz);
>            return -1;
>     }
>     pre_div = (pre_div + (1 << post_div) - 1) >> post_div;
>
> } else
>     post_div = 0;
>
> ?

Looks good to me. (but {} for else too)

>
> In case we agree on this, I'm thinking about doing 2 patches to make 
> clear what we are doing:
>
> 1. Re-doing my initial patch with
>
> post_div = pre_div / 16;
> pre_div = 16;
>
> This would be the "fix the issues in the existing (non-optimal) 
> algorithm but keep that" patch.
>
> 2. Replace the existing algorithm with your above version. This would 
> be the "improve the algorithm" patch.
>
> What do you think?
>
> Best regards
>
> Dirk

Sounds like a plan.

Troy

Patch

diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
index 5bed858..8630bbd 100644
--- a/drivers/spi/mxc_spi.c
+++ b/drivers/spi/mxc_spi.c
@@ -128,7 +128,7 @@  static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
 		unsigned int max_hz, unsigned int mode)
 {
 	u32 clk_src = mxc_get_clock(MXC_CSPI_CLK);
-	s32 pre_div = 0, post_div = 0, i, reg_ctrl, reg_config;
+	s32 pre_div = 1, post_div = 0, i, reg_ctrl, reg_config;
 	u32 ss_pol = 0, sclkpol = 0, sclkpha = 0;
 	struct cspi_regs *regs = (struct cspi_regs *)mxcs->base;
 
@@ -153,7 +153,7 @@  static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
 	if (clk_src > max_hz) {
 		pre_div = DIV_ROUND_UP(clk_src, max_hz);
 		if (pre_div > 16) {
-			post_div = pre_div / 16;
+			post_div = pre_div / 15;
 			pre_div = 15;
 		}
 		if (post_div != 0) {
@@ -174,7 +174,7 @@  static s32 spi_cfg_mxc(struct mxc_spi_slave *mxcs, unsigned int cs,
 	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_SELCHAN(3)) |
 		MXC_CSPICTRL_SELCHAN(cs);
 	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_PREDIV(0x0F)) |
-		MXC_CSPICTRL_PREDIV(pre_div);
+		MXC_CSPICTRL_PREDIV(pre_div - 1);
 	reg_ctrl = (reg_ctrl & ~MXC_CSPICTRL_POSTDIV(0x0F)) |
 		MXC_CSPICTRL_POSTDIV(post_div);