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

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

[Dirk Behme]

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;
 			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);
 
-- 
1.8.2

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

[Troy Kisky]

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);
>   

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

[Dirk Behme]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>


-- 
======================================================================
Dirk Behme                      Robert Bosch Car Multimedia GmbH
                                 CM-AI/PJ-CF32
Phone: +49 5121 49-3274         Dirk Behme
Fax:   +49 711 811 5053274      PO Box 77 77 77
mailto:dirk.behme at de.bosch.com  D-31132 Hildesheim - Germany

Bosch Group, Car Multimedia (CM)
              Automotive Navigation and Infotainment Systems (AI)
              ProJect - CoreFunctions (PJ-CF)

Robert Bosch Car Multimedia GmbH - Ein Unternehmen der Bosch Gruppe
Sitz: Hildesheim
Registergericht: Amtsgericht Hildesheim HRB 201334
Aufsichtsratsvorsitzender: Volkmar Denner
Gesch?ftsf?hrung: Uwe Thomas, Michael Bolle, Robby Drave, Egbert Hellwig
======================================================================

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

[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.
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;
_________________________
Checking values for largest divisor possible (16 << 15) = (1<<19) gives
post_div = 15;
pre_div = 16;

Checking 1st illegal divisor (0x80001) gives
post_div = 16;

Checking divisor 0xe0 gives
post_div = 4;
pre_div = 0xe;

Checking divisor 0xe1 gives
post_div = 4;
pre_div = 0xf;

Checking divisor 0x100 gives
post_div = 4;
pre_div = 0x10;

Checking divisor 0x101 gives
post_div = 5;
pre_div = 9;
_________________________________________
Code is simpler and more accurate

Troy

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

[Dirk Behme]

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

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

[Troy Kisky]

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