MCP251x bit rate calculations wrong

MCP251x bit rate calculations wrong

[Brennan Ashton]

The bit timing calculations in can-utils are certainly wrong, and
after a quick look at the ones that make up the kernel is appears that
they are not quite right as well.

From the datasheet:
TQ = 2*(BRP-1)/Fosc

Looking at the kernel:

can/dev.c defines best brp as:
182 >---v64 = (u64)best_brp * 1000000000UL;
183 >---do_div(v64, priv->clock.freq);
184 >---bt->tq = (u32)v64;
...
201 >---bt->brp = best_brp;$

so TQ[ns] = BRP/Fosc

in can/mcp251x.c

592 >---mcp251x_write_reg(spi, CNF1, ((bt->sjw - 1) << CNF1_SJW_SHIFT) |
593 >--->--->---  (bt->brp - 1));

This is missing the key /2 factor but does take into account the -1

Now looking at can-calc-bit-timing.c from can-utils:

195 >--->---cnf1 = ((bt->sjw - 1) << 6) | bt->brp;$

This is wrong as it is missing both the -1 and the /2
I have tested this corrected timing  on the MCP2515 at 20MHz Fosc and
20k bitrate.  It worked with the corrected calculations but not
before.

There is a little more work than just adjusting the calculation as the
limits and the best bpr calc must be adjusted for the /2.  I can write
some patches up unless someone thinks I am missing something.

--Brennan Ashton

Re: MCP251x bit rate calculations wrong

[Brennan Ashton]

I did not see this line:
1024 >---priv->can.clock.freq = pdata->oscillator_frequency / 2;$

So the kernel driver should be fine but in can utils this should still
be fixed for the -1

diff --git a/can-calc-bit-timing.c b/can-calc-bit-timing.c
index ecf5cb6..242e1ef 100644
--- a/can-calc-bit-timing.c
+++ b/can-calc-bit-timing.c
@@ -192,7 +192,7 @@ static void printf_btr_mcp251x(struct
can_bittiming *bt, int hd
        if (hdr) {
                printf("CNF1 CNF2 CNF3");
        } else {
-               cnf1 = ((bt->sjw - 1) << 6) | bt->brp;
+               cnf1 = ((bt->sjw - 1) << 6) | (bt->brp-1);
                cnf2 = 0x80 | ((bt->phase_seg1 - 1) << 3) | (bt->prop_seg - 1);
                cnf3 = bt->phase_seg2 - 1;
                printf("0x%02x 0x%02x 0x%02x", cnf1, cnf2, cnf3);


On Sat, Oct 26, 2013 at 10:08 AM, Brennan Ashton
<bashton@brennanashton.com> wrote:
> The bit timing calculations in can-utils are certainly wrong, and
> after a quick look at the ones that make up the kernel is appears that
> they are not quite right as well.
>
> From the datasheet:
> TQ = 2*(BRP-1)/Fosc
>
> Looking at the kernel:
>
> can/dev.c defines best brp as:
> 182 >---v64 = (u64)best_brp * 1000000000UL;
> 183 >---do_div(v64, priv->clock.freq);
> 184 >---bt->tq = (u32)v64;
> ...
> 201 >---bt->brp = best_brp;$
>
> so TQ[ns] = BRP/Fosc
>
> in can/mcp251x.c
>
> 592 >---mcp251x_write_reg(spi, CNF1, ((bt->sjw - 1) << CNF1_SJW_SHIFT) |
> 593 >--->--->---  (bt->brp - 1));
>
> This is missing the key /2 factor but does take into account the -1
>
> Now looking at can-calc-bit-timing.c from can-utils:
>
> 195 >--->---cnf1 = ((bt->sjw - 1) << 6) | bt->brp;$
>
> This is wrong as it is missing both the -1 and the /2
> I have tested this corrected timing  on the MCP2515 at 20MHz Fosc and
> 20k bitrate.  It worked with the corrected calculations but not
> before.
>
> There is a little more work than just adjusting the calculation as the
> limits and the best bpr calc must be adjusted for the /2.  I can write
> some patches up unless someone thinks I am missing something.
>
> --Brennan Ashton

Re: MCP251x bit rate calculations wrong

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On 10/26/2013 07:26 PM, Brennan Ashton wrote:
> I did not see this line:
> 1024 >---priv->can.clock.freq = pdata->oscillator_frequency / 2;$
> 
> So the kernel driver should be fine but in can utils this should still
> be fixed for the -1

Can I add your Signed-off-by to the patch?

Marc

> 
> diff --git a/can-calc-bit-timing.c b/can-calc-bit-timing.c
> index ecf5cb6..242e1ef 100644
> --- a/can-calc-bit-timing.c
> +++ b/can-calc-bit-timing.c
> @@ -192,7 +192,7 @@ static void printf_btr_mcp251x(struct
> can_bittiming *bt, int hd
>         if (hdr) {
>                 printf("CNF1 CNF2 CNF3");
>         } else {
> -               cnf1 = ((bt->sjw - 1) << 6) | bt->brp;
> +               cnf1 = ((bt->sjw - 1) << 6) | (bt->brp-1);
>                 cnf2 = 0x80 | ((bt->phase_seg1 - 1) << 3) | (bt->prop_seg - 1);
>                 cnf3 = bt->phase_seg2 - 1;
>                 printf("0x%02x 0x%02x 0x%02x", cnf1, cnf2, cnf3);

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: MCP251x bit rate calculations wrong

[Brennan Ashton]

On Mon, Oct 28, 2013 at 1:33 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 10/26/2013 07:26 PM, Brennan Ashton wrote:
>> I did not see this line:
>> 1024 >---priv->can.clock.freq = pdata->oscillator_frequency / 2;$
>>
>> So the kernel driver should be fine but in can utils this should still
>> be fixed for the -1
>
> Can I add your Signed-off-by to the patch?
>
> Marc

Sure,
Signed-off-by: Brennan Ashton <bashton@brennanashton.com>

>
>>
>> diff --git a/can-calc-bit-timing.c b/can-calc-bit-timing.c
>> index ecf5cb6..242e1ef 100644
>> --- a/can-calc-bit-timing.c
>> +++ b/can-calc-bit-timing.c
>> @@ -192,7 +192,7 @@ static void printf_btr_mcp251x(struct
>> can_bittiming *bt, int hd
>>         if (hdr) {
>>                 printf("CNF1 CNF2 CNF3");
>>         } else {
>> -               cnf1 = ((bt->sjw - 1) << 6) | bt->brp;
>> +               cnf1 = ((bt->sjw - 1) << 6) | (bt->brp-1);
>>                 cnf2 = 0x80 | ((bt->phase_seg1 - 1) << 3) | (bt->prop_seg - 1);
>>                 cnf3 = bt->phase_seg2 - 1;
>>                 printf("0x%02x 0x%02x 0x%02x", cnf1, cnf2, cnf3);
>
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>

Re: MCP251x bit rate calculations wrong

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On 10/28/2013 04:50 PM, Brennan Ashton wrote:
> On Mon, Oct 28, 2013 at 1:33 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 10/26/2013 07:26 PM, Brennan Ashton wrote:
>>> I did not see this line:
>>> 1024 >---priv->can.clock.freq = pdata->oscillator_frequency / 2;$
>>>
>>> So the kernel driver should be fine but in can utils this should still
>>> be fixed for the -1
>>
>> Can I add your Signed-off-by to the patch?
>>
>> Marc
> 
> Sure,
> Signed-off-by: Brennan Ashton <bashton@brennanashton.com>

Pushed to master.

tnx,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: MCP251x bit rate calculations wrong

[Brennan Ashton]

On Mon, Oct 28, 2013 at 9:05 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 10/28/2013 04:50 PM, Brennan Ashton wrote:
>> On Mon, Oct 28, 2013 at 1:33 AM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>> On 10/26/2013 07:26 PM, Brennan Ashton wrote:
>>>> I did not see this line:
>>>> 1024 >---priv->can.clock.freq = pdata->oscillator_frequency / 2;$
>>>>
>>>> So the kernel driver should be fine but in can utils this should still
>>>> be fixed for the -1
>>>
>>> Can I add your Signed-off-by to the patch?
>>>
>>> Marc
>>
>> Sure,
>> Signed-off-by: Brennan Ashton <bashton@brennanashton.com>
>
> Pushed to master.
>
> tnx,
> Marc

This never seemed to have made it into the master branch.

--Brennan

Re: MCP251x bit rate calculations wrong

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On 12/24/2013 09:08 PM, Brennan Ashton wrote:
>> Pushed to master.
>>
>> tnx,
>> Marc
> 
> This never seemed to have made it into the master branch.

Sorry, Done.

Marc


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]