| Message ID | 1367492386-20464-1-git-send-email-dirk.behme@de.bosch.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
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); >
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 >
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;
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: > > 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
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);