[U-Boot] [PATCH] spi: mxc_spi: Fix pre and post divider calculation
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);
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:
- 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].
- 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);
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:
- 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].
- 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);
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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
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
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:
- 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.
- 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
participants (3)
-
Dirk Behme -
Dirk Behme -
Troy Kisky