* Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx [not found] <20100216151240.148f9413.eschwab@online.de> @ 2010-02-16 15:32 ` Grant Likely 2010-02-16 16:43 ` Ernst Schwab 0 siblings, 1 reply; 3+ messages in thread From: Grant Likely @ 2010-02-16 15:32 UTC (permalink / raw) To: Ernst Schwab, spi-devel-general, Kumar Gala, linuxppc-dev [cc'd spi-devel-general, linuxppc-dev & Kumar Gala] On Tue, Feb 16, 2010 at 7:12 AM, Ernst Schwab <eschwab@online.de> wrote: > From: Ernst Schwab <eschwab@online.de> > > Correct SPI clock frequency division factor rounding, preventing clock ra= tes > higher than the maximum specified clock frequency being used. For a patch like this, it helps to also cc the driver's specific maintainer (Kumar) and to explicitly state your rational so that I don't need to re-derive your calculations. I'm more likely to merge a brand new, if potentially broken, driver than to merge a change to an existing driver that I don't know the impact of. The change forces the division to always round up instead of down. Please describe (for me now, and for people looking at the commit in the future) the mathematical reason for the changes. Thanks, g. > > Signed-off-by: Ernst Schwab <eschwab@online.de> > --- > Tested on MPC8314. > > diff -up linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c linux-2.6.33-rc8= /drivers/spi/spi_mpc8xxx.c > --- linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c =A0 =A0 2010-02-12 20= :07:45.000000000 +0100 > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c =A02010-02-15 14:08:33.000= 000000 +0100 > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp > > =A0 =A0 =A0 =A0if ((mpc8xxx_spi->spibrg / hz) > 64) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cs->hw_mode |=3D SPMODE_DIV16; > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 64); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 64= ) + 1; > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0WARN_ONCE(pm > 16, "%s: Requested speed is= too low: %d Hz. " > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Will use %d Hz instea= d.\n", dev_name(&spi->dev), > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pm > 16) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm =3D 16; > =A0 =A0 =A0 =A0} else > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4)= + 1; > =A0 =A0 =A0 =A0if (pm) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm--; > > > > > -- > > --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx 2010-02-16 15:32 ` [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx Grant Likely @ 2010-02-16 16:43 ` Ernst Schwab 2010-02-16 17:39 ` Grant Likely 0 siblings, 1 reply; 3+ messages in thread From: Ernst Schwab @ 2010-02-16 16:43 UTC (permalink / raw) To: Grant Likely; +Cc: spi-devel-general, linuxppc-dev Grant Likely <grant.likely@secretlab.ca> wrote: > The change forces the division to always round up instead of down. > Please describe (for me now, and for people looking at the commit in > the future) the mathematical reason for the changes. Ok, here are some infos on the problem I observed, and the fix. Example: When specifying spi-max-frequency =3D <10000000> in the device tree, the resulting frequency was 11.1 MHz, with spibrg being 133333332.=20 According to the freescale datasheet [1], the spi clock rate is spiclk =3D spibrg / (4 * (pm+1)) The existing code calculated =20 pm =3D mpc8xxx_spi->spibrg / (hz * 4); pm--; resulting in pm =3D (int) (3.3333) - 1 =3D 2,=20 resulting in spiclk =3D 133333332/(4*(2+1)) =3D 11111111 With the fix,=20 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1; pm--; resulting in pm =3D (int) (4.3333) - 1 =3D 3, resulting in spiclk =3D 133333332/(4*(3+1)) =3D 8333333 Without the fix, for every desired SPI frequency that=20 is not exactly deriveable from spibrg, pm will be too=20 small due to rounding down, resulting in a too high SPI clock, so we need a pm which is one higher. For values that are exactly deriveable, spibrg will be divideable by (hz*4) without remainder, and=20 (int) ((spibrg-1)/(hz*4)) will be one lower than=20 (int) (spibrg)/(hz*4), which is compensated by adding 1. For these values, the fixed version calculates the same pm=20 as the unfixed version. For all values that are not exactly deriveable, spibrg will be not divideable by (hz*4) without=20 remainder, and (int) ((spibrg-1)/(hz*4)) will be=20 the same as (int) (spibrg)/(hz*4), and the calculated pm will be one higher than calculated by the unfixed version. Regards Ernst References: [1] http://www.freescale.com/files/32bit/doc/ref_manual/MPC8315ERM.pdf, page 22-10 -> 1398 " pm: Prescale modulus select. Specifies the divide ratio of the=20 prescale divider in the SPI clock generator. The SPI baud rate generator clock source (either system clock or system clock divided by 16,=20 depending on DIV16 bit) is divided by 4 * ([PM] + 1), a range from 4 to 64.= =20 The clock has a 50% duty cycle. For example, if the prescale modulus is set to PM =3D 0011 and DIV16 is set, the system/SPICLK clock=20 ratio will be 16 * (4 * (0011 + 1)) =3D 256. " > > > > Signed-off-by: Ernst Schwab <eschwab@online.de> > > --- > > Tested on MPC8314. > > > > diff -up linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c linux-2.6.33-r= c8/drivers/spi/spi_mpc8xxx.c > > --- linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c =A0 =A0 2010-02-12 = 20:07:45.000000000 +0100 > > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c =A02010-02-15 14:08:33.0= 00000000 +0100 > > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp > > > > =A0 =A0 =A0 =A0if ((mpc8xxx_spi->spibrg / hz) > 64) { > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cs->hw_mode |=3D SPMODE_DIV16; > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 64); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * = 64) + 1; > > > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0WARN_ONCE(pm > 16, "%s: Requested speed = is too low: %d Hz. " > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Will use %d Hz inst= ead.\n", dev_name(&spi->dev), > > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pm > 16) > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm =3D 16; > > =A0 =A0 =A0 =A0} else > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * = 4) + 1; > > =A0 =A0 =A0 =A0if (pm) > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm--; > > > > > > > > > > -- > > > > >=20 >=20 >=20 > --=20 > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. --=20 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx 2010-02-16 16:43 ` Ernst Schwab @ 2010-02-16 17:39 ` Grant Likely 0 siblings, 0 replies; 3+ messages in thread From: Grant Likely @ 2010-02-16 17:39 UTC (permalink / raw) To: Ernst Schwab; +Cc: spi-devel-general, linuxppc-dev On Tue, Feb 16, 2010 at 9:43 AM, Ernst Schwab <eschwab@online.de> wrote: > Grant Likely <grant.likely@secretlab.ca> wrote: > >> The change forces the division to always round up instead of down. >> Please describe (for me now, and for people looking at the commit in >> the future) the mathematical reason for the changes. > > Ok, here are some infos on the problem I observed, and the fix. Excellent description. Thank you. I'll add this to the commit text when I apply the patch. g. > > Example: > > When specifying spi-max-frequency =3D <10000000> in the device tree, > the resulting frequency was 11.1 MHz, with spibrg being 133333332. > > According to the freescale datasheet [1], the spi clock rate is > spiclk =3D spibrg / (4 * (pm+1)) > > The existing code calculated > =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4); pm--; > resulting in pm =3D (int) (3.3333) - 1 =3D 2, > resulting in spiclk =3D 133333332/(4*(2+1)) =3D 11111111 > > With the fix, > =A0pm =3D (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1; pm--; > resulting in pm =3D (int) (4.3333) - 1 =3D 3, > resulting in spiclk =3D 133333332/(4*(3+1)) =3D 8333333 > > Without the fix, for every desired SPI frequency that > is not exactly deriveable from spibrg, pm will be too > small due to rounding down, resulting in a too high SPI clock, > so we need a pm which is one higher. > > For values that are exactly deriveable, spibrg will > be divideable by (hz*4) without remainder, and > (int) ((spibrg-1)/(hz*4)) will be one lower than > (int) (spibrg)/(hz*4), which is compensated by adding 1. > For these values, the fixed version calculates the same pm > as the unfixed version. > > For all values that are not exactly deriveable, > spibrg will be not divideable by (hz*4) without > remainder, and (int) ((spibrg-1)/(hz*4)) will be > the same as (int) (spibrg)/(hz*4), and the calculated pm will > be one higher than calculated by the unfixed version. > > Regards > Ernst > > References: > [1] http://www.freescale.com/files/32bit/doc/ref_manual/MPC8315ERM.pdf, > =A0 =A0page 22-10 -> 1398 > " > pm: Prescale modulus select. Specifies the divide ratio of the > prescale divider in the SPI clock generator. The SPI baud rate generator > clock source (either system clock or system clock divided by 16, > depending on DIV16 bit) is divided by 4 * ([PM] + 1), a range from 4 to 6= 4. > The clock has a 50% duty cycle. For example, if the prescale > modulus is set to PM =3D 0011 and DIV16 is set, the system/SPICLK clock > ratio will be 16 * (4 * (0011 + 1)) =3D 256. > " > > >> > >> > Signed-off-by: Ernst Schwab <eschwab@online.de> >> > --- >> > Tested on MPC8314. >> > >> > diff -up linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c linux-2.6.33-= rc8/drivers/spi/spi_mpc8xxx.c >> > --- linux-2.6.33-rc8.orig/drivers/spi/spi_mpc8xxx.c =A0 =A0 2010-02-12= 20:07:45.000000000 +0100 >> > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c =A02010-02-15 14:08:33.= 000000000 +0100 >> > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp >> > >> > =A0 =A0 =A0 =A0if ((mpc8xxx_spi->spibrg / hz) > 64) { >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cs->hw_mode |=3D SPMODE_DIV16; >> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 64); >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz *= 64) + 1; >> > >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0WARN_ONCE(pm > 16, "%s: Requested speed= is too low: %d Hz. " >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Will use %d Hz ins= tead.\n", dev_name(&spi->dev), >> > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pm > 16) >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm =3D 16; >> > =A0 =A0 =A0 =A0} else >> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D mpc8xxx_spi->spibrg / (hz * 4); >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pm =3D (mpc8xxx_spi->spibrg - 1) / (hz *= 4) + 1; >> > =A0 =A0 =A0 =A0if (pm) >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pm--; >> > >> > >> > >> > >> > -- >> > >> > >> >> >> >> -- >> Grant Likely, B.Sc., P.Eng. >> Secret Lab Technologies Ltd. > > > -- > > --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-02-16 17:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20100216151240.148f9413.eschwab@online.de>
2010-02-16 15:32 ` [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx Grant Likely
2010-02-16 16:43 ` Ernst Schwab
2010-02-16 17:39 ` Grant Likely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).