Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx

Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx

[Grant Likely]

[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 rates
> 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     2010-02-12 20:07:45.000000000 +0100
> +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c  2010-02-15 14:08:33.000000000 +0100
> @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>
>        if ((mpc8xxx_spi->spibrg / hz) > 64) {
>                cs->hw_mode |= SPMODE_DIV16;
> -               pm = mpc8xxx_spi->spibrg / (hz * 64);
> +               pm = (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
>
>                WARN_ONCE(pm > 16, "%s: Requested speed is too low: %d Hz. "
>                          "Will use %d Hz instead.\n", dev_name(&spi->dev),
> @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>                if (pm > 16)
>                        pm = 16;
>        } else
> -               pm = mpc8xxx_spi->spibrg / (hz * 4);
> +               pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
>        if (pm)
>                pm--;
>
>
>
>
> --
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx

[Ernst Schwab]

Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> 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 = <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 = spibrg / (4 * (pm+1))

The existing code calculated  
   pm = mpc8xxx_spi->spibrg / (hz * 4); pm--;
resulting in pm = (int) (3.3333) - 1 = 2, 
resulting in spiclk = 133333332/(4*(2+1)) = 11111111

With the fix, 
  pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1; pm--;
resulting in pm = (int) (4.3333) - 1 = 3,
resulting in spiclk = 133333332/(4*(3+1)) = 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,
    page 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 64. 
The clock has a 50% duty cycle. For example, if the prescale
modulus is set to PM = 0011 and DIV16 is set, the system/SPICLK clock 
ratio will be 16 * (4 * (0011 + 1)) = 256.
"


> >
> > Signed-off-by: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
> > ---
> > 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     2010-02-12 20:07:45.000000000 +0100
> > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c  2010-02-15 14:08:33.000000000 +0100
> > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
> >
> >        if ((mpc8xxx_spi->spibrg / hz) > 64) {
> >                cs->hw_mode |= SPMODE_DIV16;
> > -               pm = mpc8xxx_spi->spibrg / (hz * 64);
> > +               pm = (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
> >
> >                WARN_ONCE(pm > 16, "%s: Requested speed is too low: %d Hz. "
> >                          "Will use %d Hz instead.\n", dev_name(&spi->dev),
> > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
> >                if (pm > 16)
> >                        pm = 16;
> >        } else
> > -               pm = mpc8xxx_spi->spibrg / (hz * 4);
> > +               pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
> >        if (pm)
> >                pm--;
> >
> >
> >
> >
> > --
> >
> >
> 
> 
> 
> -- 
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.


-- 


------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev

Re: [PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx

[Grant Likely]

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 = <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 = spibrg / (4 * (pm+1))
>
> The existing code calculated
>   pm = mpc8xxx_spi->spibrg / (hz * 4); pm--;
> resulting in pm = (int) (3.3333) - 1 = 2,
> resulting in spiclk = 133333332/(4*(2+1)) = 11111111
>
> With the fix,
>  pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1; pm--;
> resulting in pm = (int) (4.3333) - 1 = 3,
> resulting in spiclk = 133333332/(4*(3+1)) = 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,
>    page 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 64.
> The clock has a 50% duty cycle. For example, if the prescale
> modulus is set to PM = 0011 and DIV16 is set, the system/SPICLK clock
> ratio will be 16 * (4 * (0011 + 1)) = 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     2010-02-12 20:07:45.000000000 +0100
>> > +++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c  2010-02-15 14:08:33.000000000 +0100
>> > @@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>> >
>> >        if ((mpc8xxx_spi->spibrg / hz) > 64) {
>> >                cs->hw_mode |= SPMODE_DIV16;
>> > -               pm = mpc8xxx_spi->spibrg / (hz * 64);
>> > +               pm = (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
>> >
>> >                WARN_ONCE(pm > 16, "%s: Requested speed is too low: %d Hz. "
>> >                          "Will use %d Hz instead.\n", dev_name(&spi->dev),
>> > @@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
>> >                if (pm > 16)
>> >                        pm = 16;
>> >        } else
>> > -               pm = mpc8xxx_spi->spibrg / (hz * 4);
>> > +               pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
>> >        if (pm)
>> >                pm--;
>> >
>> >
>> >
>> >
>> > --
>> >
>> >
>>
>>
>>
>> --
>> Grant Likely, B.Sc., P.Eng.
>> Secret Lab Technologies Ltd.
>
>
> --
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH] spi: Correct SPI clock frequency setting in spi_mpc8xxx

[Ernst Schwab]

Correct SPI clock frequency division factor rounding, preventing clock rates 
higher than the maximum specified clock frequency being used. 

Signed-off-by: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
---
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	2010-02-12 20:07:45.000000000 +0100
+++ linux-2.6.33-rc8/drivers/spi/spi_mpc8xxx.c	2010-02-15 14:08:33.000000000 +0100
@@ -365,7 +365,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
 
 	if ((mpc8xxx_spi->spibrg / hz) > 64) {
 		cs->hw_mode |= SPMODE_DIV16;
-		pm = mpc8xxx_spi->spibrg / (hz * 64);
+		pm = (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
 
 		WARN_ONCE(pm > 16, "%s: Requested speed is too low: %d Hz. "
 			  "Will use %d Hz instead.\n", dev_name(&spi->dev),
@@ -373,7 +373,7 @@ int mpc8xxx_spi_setup_transfer(struct sp
 		if (pm > 16)
 			pm = 16;
 	} else
-		pm = mpc8xxx_spi->spibrg / (hz * 4);
+		pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
 	if (pm)
 		pm--;
 


------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev