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

[Grant Likely <grant.likely@secretlab.ca>]

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.