Why doesn't the socketcan prio qdisc work for first 10 can frames?

Why doesn't the socketcan prio qdisc work for first 10 can frames?

[Ruan TingQuan (ST-FIR/ENG1-Zhu)]

Hello Linux-can-team,

A prio qdisc is created with 8 bands for my different priority sending requirement.  
My test environment: ubuntu 12.04 (kernel 3.11) + PEAK-USB pro (can0 is connect to can1). 
The frame will be send on can0 and receive on can1 (candump)
Below is the setting on can0:
	> ip link set can0 type can bitrate 5000
	> ip link set can1 type can bitrate 5000
	> tc qdisc add dev can0 root handle 1: prio bands 8
	> tc filter add dev $1 parent 1:0 prio 1 basic match canid\(eff 0x1:0x7\) flowid 1:1
	> tc filter add dev $1 parent 1:0 prio 2 basic match canid\(eff 0x2:0x7\) flowid 1:2
	> tc filter add dev $1 parent 1:0 prio 3 basic match canid\(eff 0x3:0x7\) flowid 1:3
	> tc filter add dev $1 parent 1:0 prio 4 basic match canid\(eff 0x4:0x7\) flowid 1:4
	> tc filter add dev $1 parent 1:0 prio 5 basic match canid\(eff 0x5:0x7\) flowid 1:5
	> tc filter add dev $1 parent 1:0 prio 6 basic match canid\(eff 0x6:0x7\) flowid 1:6
	> tc filter add dev $1 parent 1:0 prio 7 basic match canid\(eff 0x7:0x7\) flowid 1:7
	> tc filter add dev $1 parent 1:0 prio 8 basic match canid\(sff 0x0:0x0 eff 0x0:0x0\) flowid 1:8

Then, I made a test program to send 10 frames on can0 socket quickly.
  	can0  81234560   [8]  00 00 00 00 50 50 50 50
  	can0  81234567   [8]  07 07 07 07 4F 4F 4F 4F
  	can0  81234566   [8]  06 06 06 06 4E 4E 4E 4E
  	can0  81234565   [8]  05 05 05 05 4D 4D 4D 4D
	can0  81234564   [8]  04 04 04 04 4C 4C 4C 4C
  	can0  81234563   [8]  03 03 03 03 4B 4B 4B 4B
  	can0  81234562   [8]  02 02 02 02 4A 4A 4A 4A
  	can0  81234561   [8]  01 01 01 01 49 49 49 49
  	can0  81234560   [8]  00 00 00 00 48 48 48 48
  	can0  81234567   [8]  07 07 07 07 47 47 47 47

I expect the frames from can1 with priority sequence high to low:
	can1  01234560   [8]  00 00 00 00 50 50 50 50
	can1  01234561   [8]  01 01 01 01 49 49 49 49  
	can1  01234562   [8]  02 02 02 02 4A 4A 4A 4A
	can1  01234563   [8]  03 03 03 03 4B 4B 4B 4B
	can1  01234564   [8]  04 04 04 04 4C 4C 4C 4C
	can1  01234565   [8]  05 05 05 05 4D 4D 4D 4D
	can1  01234566   [8]  06 06 06 06 4E 4E 4E 4E
	can1  01234567   [8]  07 07 07 07 4F 4F 4F 4F
	can1  01234560   [8]  00 00 00 00 48 48 48 48

But actually, I got the frames with same sequence
	can1  01234560   [8]  00 00 00 00 50 50 50 50
	can1  01234567   [8]  07 07 07 07 4F 4F 4F 4F
	can1  01234566   [8]  06 06 06 06 4E 4E 4E 4E
	can1  01234565   [8]  05 05 05 05 4D 4D 4D 4D
	can1  01234564   [8]  04 04 04 04 4C 4C 4C 4C
	can1  01234563   [8]  03 03 03 03 4B 4B 4B 4B
	can1  01234562   [8]  02 02 02 02 4A 4A 4A 4A
	can1  01234561   [8]  01 01 01 01 49 49 49 49
	can1  01234560   [8]  00 00 00 00 48 48 48 48
	can1  01234567   [8]  07 07 07 07 47 47 47 47
	can1  01234566   [8]  06 06 06 06 46 46 46 46

And I found that just happens for first 10 frames. After that, prio qdisc works fine.
Why? Or What am I missing?

Regards!
Leo Ruan

Re: Why doesn't the socketcan prio qdisc work for first 10 can frames?

[Oliver Hartkopp]

Please try to reduce the tx queue length of the CAN netdevice to 1

E.g.

ip link set can0 txqueuelen 1

Does that help?

Regards,
Oliver

On 27.11.2014 10:43, Ruan TingQuan (ST-FIR/ENG1-Zhu) wrote:
> Hello Linux-can-team,
>
> A prio qdisc is created with 8 bands for my different priority sending requirement.
> My test environment: ubuntu 12.04 (kernel 3.11) + PEAK-USB pro (can0 is connect to can1).
> The frame will be send on can0 and receive on can1 (candump)
> Below is the setting on can0:
> 	> ip link set can0 type can bitrate 5000
> 	> ip link set can1 type can bitrate 5000
> 	> tc qdisc add dev can0 root handle 1: prio bands 8
> 	> tc filter add dev $1 parent 1:0 prio 1 basic match canid\(eff 0x1:0x7\) flowid 1:1
> 	> tc filter add dev $1 parent 1:0 prio 2 basic match canid\(eff 0x2:0x7\) flowid 1:2
> 	> tc filter add dev $1 parent 1:0 prio 3 basic match canid\(eff 0x3:0x7\) flowid 1:3
> 	> tc filter add dev $1 parent 1:0 prio 4 basic match canid\(eff 0x4:0x7\) flowid 1:4
> 	> tc filter add dev $1 parent 1:0 prio 5 basic match canid\(eff 0x5:0x7\) flowid 1:5
> 	> tc filter add dev $1 parent 1:0 prio 6 basic match canid\(eff 0x6:0x7\) flowid 1:6
> 	> tc filter add dev $1 parent 1:0 prio 7 basic match canid\(eff 0x7:0x7\) flowid 1:7
> 	> tc filter add dev $1 parent 1:0 prio 8 basic match canid\(sff 0x0:0x0 eff 0x0:0x0\) flowid 1:8
>
> Then, I made a test program to send 10 frames on can0 socket quickly.
>    	can0  81234560   [8]  00 00 00 00 50 50 50 50
>    	can0  81234567   [8]  07 07 07 07 4F 4F 4F 4F
>    	can0  81234566   [8]  06 06 06 06 4E 4E 4E 4E
>    	can0  81234565   [8]  05 05 05 05 4D 4D 4D 4D
> 	can0  81234564   [8]  04 04 04 04 4C 4C 4C 4C
>    	can0  81234563   [8]  03 03 03 03 4B 4B 4B 4B
>    	can0  81234562   [8]  02 02 02 02 4A 4A 4A 4A
>    	can0  81234561   [8]  01 01 01 01 49 49 49 49
>    	can0  81234560   [8]  00 00 00 00 48 48 48 48
>    	can0  81234567   [8]  07 07 07 07 47 47 47 47
>
> I expect the frames from can1 with priority sequence high to low:
> 	can1  01234560   [8]  00 00 00 00 50 50 50 50
> 	can1  01234561   [8]  01 01 01 01 49 49 49 49
> 	can1  01234562   [8]  02 02 02 02 4A 4A 4A 4A
> 	can1  01234563   [8]  03 03 03 03 4B 4B 4B 4B
> 	can1  01234564   [8]  04 04 04 04 4C 4C 4C 4C
> 	can1  01234565   [8]  05 05 05 05 4D 4D 4D 4D
> 	can1  01234566   [8]  06 06 06 06 4E 4E 4E 4E
> 	can1  01234567   [8]  07 07 07 07 4F 4F 4F 4F
> 	can1  01234560   [8]  00 00 00 00 48 48 48 48
>
> But actually, I got the frames with same sequence
> 	can1  01234560   [8]  00 00 00 00 50 50 50 50
> 	can1  01234567   [8]  07 07 07 07 4F 4F 4F 4F
> 	can1  01234566   [8]  06 06 06 06 4E 4E 4E 4E
> 	can1  01234565   [8]  05 05 05 05 4D 4D 4D 4D
> 	can1  01234564   [8]  04 04 04 04 4C 4C 4C 4C
> 	can1  01234563   [8]  03 03 03 03 4B 4B 4B 4B
> 	can1  01234562   [8]  02 02 02 02 4A 4A 4A 4A
> 	can1  01234561   [8]  01 01 01 01 49 49 49 49
> 	can1  01234560   [8]  00 00 00 00 48 48 48 48
> 	can1  01234567   [8]  07 07 07 07 47 47 47 47
> 	can1  01234566   [8]  06 06 06 06 46 46 46 46
>
> And I found that just happens for first 10 frames. After that, prio qdisc works fine.
> Why? Or What am I missing?
>
> Regards!
> Leo Ruan
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

CAN implementation on A20 Allwinner

[Pankajkumar Misra (RBEI/EEA2)]

Hello All,

I am currently working on Olimex A20-Olinuxino Micro board.

I am trying to bring-up CAN device available on A20 Allwinner chip at base address 0x01c2BC00.

In initialization I am enabling clock, setting pins for Tx & Rx, IRQ & ISR are set. Interrupt,Tx & Rx enabled.
In transmit frame function, after adding required data into buffer I am setting command to 0x01, which means transmit request set.
But data is never transmitted from the buffer. Can status says "controller in the process of transmitting a msg". But it never transmits.
Interrupt is never generated for tx. ISR is never called. Nothing comes on candump.

One strange thing is that it is not responding to cansend again. i.e. possibly it is going in infinite loop somewhere.

What am I missing !!

Best Regards,
Pankaj

Re: CAN implementation on A20 Allwinner

[Uwe Bonnes]

>>>>> "Pankajkumar" == Pankajkumar Misra (RBEI/EEA2) <Pankaj.Kumar@in.bosch.com> writes:

    Pankajkumar> Hello All, I am currently working on Olimex A20-Olinuxino
    Pankajkumar> Micro board.

    Pankajkumar> I am trying to bring-up CAN device available on A20
    Pankajkumar> Allwinner chip at base address 0x01c2BC00.

Is there some open documentation about the CAN IP? Can you point me to that
documentation?  I looked hard with A10 some time ago, but didn't find
anything.

Thanks
-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

Re: CAN implementation on A20 Allwinner

[Gerhard Bertelsmann]

Hi again,

here are some updates:

https://github.com/btolfa/sunxi-can-driver

Regards
Gerd

Am Do, 27.11.2014, 20:32, schrieb Gerhard Bertelsmann:
> Hi,
>
> please have a look at:
>
> https://github.com/plaes/sunxi-can-driver
>
> http://forum.lemaker.org/thread-209-2-1-2.html
> http://forum.lemaker.org/forum.php?mod=attachment&aid=NTY2fDRiNjM1ZTAxfDE0MTcxMTYxNTh8MHwyMDk%3D
>
> Regards,
>
> Gerd
>
>
> Am Do, 27.11.2014, 19:28, schrieb Uwe Bonnes:
>>>>>>> "Pankajkumar" == Pankajkumar Misra (RBEI/EEA2) <Pankaj.Kumar@in.bosch.com> writes:
>>
>>     Pankajkumar> Hello All, I am currently working on Olimex A20-Olinuxino
>>     Pankajkumar> Micro board.
>>
>>     Pankajkumar> I am trying to bring-up CAN device available on A20
>>     Pankajkumar> Allwinner chip at base address 0x01c2BC00.
>>
>> Is there some open documentation about the CAN IP? Can you point me to that
>> documentation?  I looked hard with A10 some time ago, but didn't find
>> anything.
>>
>> Thanks
>> --
>> Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de
>>
>> Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
>> --------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: CAN implementation on A20 Allwinner

[Marc Kleine-Budde]

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

On 11/27/2014 08:53 PM, Gerhard Bertelsmann wrote:
> Hi again,
> 
> here are some updates:
> 
> https://github.com/btolfa/sunxi-can-driver

It it's sja1000 compatible, it should work out of the box with the
existing sja1000 device tree binding.

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: 819 bytes --]

Re: CAN implementation on A20 Allwinner

[Gerhard Bertelsmann]

Hi

Am 2014-12-02 13:37, schrieb Marc Kleine-Budde:
> On 11/27/2014 08:53 PM, Gerhard Bertelsmann wrote:
>> Hi again,
>> 
>> here are some updates:
>> 
>> https://github.com/btolfa/sunxi-can-driver
> 
> It it's sja1000 compatible, it should work out of the box with the
> existing sja1000 device tree binding.

the Allwinner CAN IP is very similar but not exactly a SJA1000, e g. the
registers differ and some are 32 bit wide and not 8 bit.
So the question is: change the sja1000 and the platform file for 
"quirks"
or write a new independent one ?
Which way do you prefer or better: which driver will be accepted for 
mainline ?

> 
> Marc

Regards

Gerd

P.S.: Allwinner A20 Manual - CAN beginning at page 811
https://github.com/allwinner-zh/documents/raw/master/A20/A20_User_Manual_v1.4_20150510.pdf

Re: CAN implementation on A20 Allwinner

[Marc Kleine-Budde]

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

On 08/27/2015 03:04 PM, Gerhard Bertelsmann wrote:
>> It it's sja1000 compatible, it should work out of the box with the
>> existing sja1000 device tree binding.
> 
> the Allwinner CAN IP is very similar but not exactly a SJA1000, e g. the
> registers differ and some are 32 bit wide and not 8 bit.

Which are the registers that have the upper 24 bit used?

> So the question is: change the sja1000 and the platform file for 
> "quirks" or write a new independent one ?

Depends on the actual differences, but I'd go for quirks first.

> Which way do you prefer or better: which driver will be accepted for 
> mainline ?

The github driver lacks a proper driver model binding, so that's not
mainline ready.

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: 455 bytes --]

Re: CAN implementation on A20 Allwinner

[Gerhard Bertelsmann]

Hi Marc,

many thx for your fast answer.

Am 2015-08-27 15:23, schrieb Marc Kleine-Budde:
> On 08/27/2015 03:04 PM, Gerhard Bertelsmann wrote:
>>> It it's sja1000 compatible, it should work out of the box with the
>>> existing sja1000 device tree binding.
>> 
>> the Allwinner CAN IP is very similar but not exactly a SJA1000, e g. 
>> the
>> registers differ and some are 32 bit wide and not 8 bit.
> 
> Which are the registers that have the upper 24 bit used?

CAN_STA_REG  status register
CAN_BUS_TIME bus timing
CAN_REC_REG  errot counter

Some regs also have a different address

> 
>> So the question is: change the sja1000 and the platform file for
>> "quirks" or write a new independent one ?
> 
> Depends on the actual differences, but I'd go for quirks first.
> 
>> Which way do you prefer or better: which driver will be accepted for
>> mainline ?
> 
> The github driver lacks a proper driver model binding, so that's not
> mainline ready.

That would be not a huge problem IMHO ...

> 
> Marc

Gerd

Re: CAN implementation on A20 Allwinner

[Marc Kleine-Budde]

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

On 08/27/2015 03:34 PM, Gerhard Bertelsmann wrote:
>> Which are the registers that have the upper 24 bit used?
> 
> CAN_STA_REG  status register
> CAN_BUS_TIME bus timing
> CAN_REC_REG  errot counter

That's ok.

> Some regs also have a different address

Which one? How may?

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: 455 bytes --]

Re: CAN implementation on A20 Allwinner

[Gerhard Bertelsmann]

Hi Marc,

Am 2015-08-27 15:49, schrieb Marc Kleine-Budde:
> On 08/27/2015 03:34 PM, Gerhard Bertelsmann wrote:
>>> Which are the registers that have the upper 24 bit used?
>> 
>> CAN_STA_REG  status register
>> CAN_BUS_TIME bus timing
>> CAN_REC_REG  errot counter
> 
> That's ok.
> 
>> Some regs also have a different address
> 
> Which one? How may?

here a quick overview (not a complete list):

   original    address    Allwinner
SJA1000_MOD    0x00   CAN_MOD_SEL    OK
SJA1000_CMR    0x01   CAN_CMD_REG    OK
SJA1000_SR     0x02   CAN_STA_REG    OK but 24 bit (more values)
SJA1000_IR     0x03   CAN_INT_REG    OK
SJA1000_IER    0x04   CAN_INTE_REG   OK
                0x05   CAN_BUS_TIME   NOK also 24 bit
SJA1000_BTR0   0x06   CAN_TEWL       NOK CAN TX error warn limit reg
SJA1000_BTR1   0x07   CAN_ERRC       NOK CAN error counter register
SJA1000_OCR    0x08   CAN_RMCNT      NOK CAN receive message count reg

SJA1000_ALC    0x0B                  NOK
SJA1000_ECC    0x0C                  NOK
SJA1000_EWL    0x0D                  NOK
SJA1000_RXERR  0x0E                  NOK
SJA1000_TXERR  0x0F                  NOK
SJA1000_ACCC0  0x10   CAN_AC0        OK  Reset mode: CAN_ACP_CODE but 32 
bit
SJA1000_ACCC1  0x11   CAN_AM0        NOK Reset mode: CAN_ACP_CODE 32 bit
SJA1000_ACCC2  0x12   CAN_TRBUF2     NOK CAN TX/RX mbuffer 2 reg
SJA1000_ACCC3  0x13   CAN_TRBUF3     NOK CAN TX/RX mbuffer 3 reg
SJA1000_ACCM0  0x14   CAN_TRBUF4     NOK CAN TX/RX mbuffer 4 reg
SJA1000_ACCM1  0x15   CAN_TRBUF5     NOK CAN TX/RX mbuffer 5 reg
SJA1000_ACCM2  0x16   CAN_TRBUF6     NOK CAN TX/RX mbuffer 6 reg
SJA1000_ACCM3  0x17   CAN_TRBUF7     NOK CAN TX/RX mbuffer 7 reg
SJA1000_RMC    0x1D   CAN_TRBUF8     NOK CAN TX/RX mbuffer 8 reg
SJA1000_RBSA   0x1E   CAN_TRBUF9     NOK CAN TX/RX mbuffer 9 reg

more differences than similarities ... A sja1000 module with "quirks" 
makes
no sense to me.

Gerd

Re: CAN implementation on A20 Allwinner

[Marc Kleine-Budde]

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

On 08/27/2015 04:29 PM, Gerhard Bertelsmann wrote:
>>> Some regs also have a different address
>>
>> Which one? How may?
> 
> here a quick overview (not a complete list):
> 
>    original    address    Allwinner
> SJA1000_MOD    0x00   CAN_MOD_SEL    OK
> SJA1000_CMR    0x01   CAN_CMD_REG    OK
> SJA1000_SR     0x02   CAN_STA_REG    OK but 24 bit (more values)
> SJA1000_IR     0x03   CAN_INT_REG    OK
> SJA1000_IER    0x04   CAN_INTE_REG   OK
>                 0x05   CAN_BUS_TIME   NOK also 24 bit
> SJA1000_BTR0   0x06   CAN_TEWL       NOK CAN TX error warn limit reg
> SJA1000_BTR1   0x07   CAN_ERRC       NOK CAN error counter register
> SJA1000_OCR    0x08   CAN_RMCNT      NOK CAN receive message count reg
> 
> SJA1000_ALC    0x0B                  NOK
> SJA1000_ECC    0x0C                  NOK
> SJA1000_EWL    0x0D                  NOK
> SJA1000_RXERR  0x0E                  NOK
> SJA1000_TXERR  0x0F                  NOK
> SJA1000_ACCC0  0x10   CAN_AC0        OK  Reset mode: CAN_ACP_CODE but 32 
> bit
> SJA1000_ACCC1  0x11   CAN_AM0        NOK Reset mode: CAN_ACP_CODE 32 bit
> SJA1000_ACCC2  0x12   CAN_TRBUF2     NOK CAN TX/RX mbuffer 2 reg
> SJA1000_ACCC3  0x13   CAN_TRBUF3     NOK CAN TX/RX mbuffer 3 reg
> SJA1000_ACCM0  0x14   CAN_TRBUF4     NOK CAN TX/RX mbuffer 4 reg
> SJA1000_ACCM1  0x15   CAN_TRBUF5     NOK CAN TX/RX mbuffer 5 reg
> SJA1000_ACCM2  0x16   CAN_TRBUF6     NOK CAN TX/RX mbuffer 6 reg
> SJA1000_ACCM3  0x17   CAN_TRBUF7     NOK CAN TX/RX mbuffer 7 reg
> SJA1000_RMC    0x1D   CAN_TRBUF8     NOK CAN TX/RX mbuffer 8 reg
> SJA1000_RBSA   0x1E   CAN_TRBUF9     NOK CAN TX/RX mbuffer 9 reg
> 
> more differences than similarities ... A sja1000 module with "quirks" 
> makes
> no sense to me.

ACK

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: 455 bytes --]

RE: CAN implementation on A20 Allwinner

[Misra Pankaj Kumar (RBEI/EEA2)]

Hello Marc,

I have used CAN4Linux package with A20 Allwinner. Its working fine for me.
Its char device implementation.

Best regards
Pankaj

-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de] 
Sent: Thursday, August 27, 2015 8:07 PM
To: Gerhard Bertelsmann <info@gerhard-bertelsmann.de>
Cc: Uwe Bonnes <bon@elektron.ikp.physik.tu-darmstadt.de>; Misra Pankaj Kumar (RBEI/EEA2) <Pankaj.Kumar@in.bosch.com>; linux-can@vger.kernel.org; linux-can-owner@vger.kernel.org
Subject: Re: CAN implementation on A20 Allwinner

On 08/27/2015 04:29 PM, Gerhard Bertelsmann wrote:
>>> Some regs also have a different address
>>
>> Which one? How may?
> 
> here a quick overview (not a complete list):
> 
>    original    address    Allwinner
> SJA1000_MOD    0x00   CAN_MOD_SEL    OK
> SJA1000_CMR    0x01   CAN_CMD_REG    OK
> SJA1000_SR     0x02   CAN_STA_REG    OK but 24 bit (more values)
> SJA1000_IR     0x03   CAN_INT_REG    OK
> SJA1000_IER    0x04   CAN_INTE_REG   OK
>                 0x05   CAN_BUS_TIME   NOK also 24 bit
> SJA1000_BTR0   0x06   CAN_TEWL       NOK CAN TX error warn limit reg
> SJA1000_BTR1   0x07   CAN_ERRC       NOK CAN error counter register
> SJA1000_OCR    0x08   CAN_RMCNT      NOK CAN receive message count reg
> 
> SJA1000_ALC    0x0B                  NOK
> SJA1000_ECC    0x0C                  NOK
> SJA1000_EWL    0x0D                  NOK
> SJA1000_RXERR  0x0E                  NOK
> SJA1000_TXERR  0x0F                  NOK
> SJA1000_ACCC0  0x10   CAN_AC0        OK  Reset mode: CAN_ACP_CODE but 32 
> bit
> SJA1000_ACCC1  0x11   CAN_AM0        NOK Reset mode: CAN_ACP_CODE 32 bit
> SJA1000_ACCC2  0x12   CAN_TRBUF2     NOK CAN TX/RX mbuffer 2 reg
> SJA1000_ACCC3  0x13   CAN_TRBUF3     NOK CAN TX/RX mbuffer 3 reg
> SJA1000_ACCM0  0x14   CAN_TRBUF4     NOK CAN TX/RX mbuffer 4 reg
> SJA1000_ACCM1  0x15   CAN_TRBUF5     NOK CAN TX/RX mbuffer 5 reg
> SJA1000_ACCM2  0x16   CAN_TRBUF6     NOK CAN TX/RX mbuffer 6 reg
> SJA1000_ACCM3  0x17   CAN_TRBUF7     NOK CAN TX/RX mbuffer 7 reg
> SJA1000_RMC    0x1D   CAN_TRBUF8     NOK CAN TX/RX mbuffer 8 reg
> SJA1000_RBSA   0x1E   CAN_TRBUF9     NOK CAN TX/RX mbuffer 9 reg
> 
> more differences than similarities ... A sja1000 module with "quirks" 
> makes
> no sense to me.

ACK

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   |

Re: CAN implementation on A20 Allwinner

[Marc Kleine-Budde]

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

On 08/28/2015 07:41 AM, Misra Pankaj Kumar (RBEI/EEA2) wrote:
> I have used CAN4Linux package with A20 Allwinner. Its working fine for me.
> Its char device implementation.

Good. Is the A20 supported bt a generic CAN4Linux SJA1000 driver or is
there a separate driver for it?

regards,
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: 455 bytes --]

Re: CAN implementation on A20 Allwinner

[Gerhard Bertelsmann]

Hi,
Am 2015-08-28 08:21, schrieb Marc Kleine-Budde:
> On 08/28/2015 07:41 AM, Misra Pankaj Kumar (RBEI/EEA2) wrote:
>> I have used CAN4Linux package with A20 Allwinner. Its working fine for 
>> me.
>> Its char device implementation.
> 
> Good. Is the A20 supported bt a generic CAN4Linux SJA1000 driver or is
> there a separate driver for it?

It's a different driver.

If nobody else has done it already I will refresh the existing sunxi-can 
driver
and fed it to the lions (SocketCAN team) :-)

> 
> regards,
> Marc

Regards

Gerd

Re: CAN implementation on A20 Allwinner

[Marc Kleine-Budde]

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

On 08/28/2015 11:22 AM, Gerhard Bertelsmann wrote:
> If nobody else has done it already I will refresh the existing
> sunxi-can driver and fed it to the lions (SocketCAN team) :-)

I'll give some comments first - may save some time. General remarks:

- I haven't looked closely at the reset_mode normal_mode switching, but
  this looks racy.
- there are quite some delays in the hot paths of the driver
- add proper driver model, i.e. DT bindings

> /*
> * sun7i_can.c - CAN bus controller driver for sun7i
> *
> * Copyright (c) 2013 Peter Chen
> *
> * Copyright (c) 2013 Inmotion Co,. LTD
> * All right reserved.
> *
> */

This driver is IMHO derived work from the existing sja1000 drivers,
removing the copyrights and the license not considered best practise.

> 
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <linux/types.h>
> #include <linux/fcntl.h>
> #include <linux/interrupt.h>
> #include <linux/ptrace.h>
> #include <linux/string.h>
> #include <linux/errno.h>
> #include <linux/netdevice.h>
> #include <linux/if_arp.h>
> #include <linux/if_ether.h>
> #include <linux/skbuff.h>
> #include <linux/delay.h>
> #include <linux/clk.h>
> 
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> 
> #include <mach/includes.h>
> #include <plat/sys_config.h>
> #include <mach/irqs.h>
> 
> #include "sun7i_can.h"
> 
> #define DRV_NAME "sun7i_can"
> 
> MODULE_AUTHOR("Peter Chen <xingkongcp@gmail.com>");
> MODULE_LICENSE("Dual BSD/GPL");
> MODULE_DESCRIPTION(DRV_NAME "CAN netdevice driver");

move to the end.

> static struct net_device *sun7ican_dev;

not globals please

> static struct can_bittiming_const sun7i_can_bittiming_const = {
>         .name = DRV_NAME,
>         .tseg1_min = 1,
>         .tseg1_max = 16,
>         .tseg2_min = 1,
>         .tseg2_max = 8,
>         .sjw_max = 4,
>         .brp_min = 1,
>         .brp_max = 64,
>         .brp_inc = 1,
> };
> 
> static void sun7i_can_write_cmdreg(struct sun7i_can_priv *priv, u8 val)
> {
>         unsigned long flags;
> 
>         /*
>          * The command register needs some locking and time to settle
>          * the write_reg() operation - especially on SMP systems.
>          */

is this true for the allwinner variant of the IP core?

>         spin_lock_irqsave(&priv->cmdreg_lock, flags);
>         writel(val, CAN_CMD_ADDR);
>         spin_unlock_irqrestore(&priv->cmdreg_lock, flags);
> }
> 
> static int sun7i_can_is_absent(struct sun7i_can_priv *priv)
> {
>         return ((readl(CAN_MSEL_ADDR) & 0xFF) == 0xFF);
> }

This can be removed for a SoC internal IP core.

> 
> static int sun7i_can_probe(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
> 
>         if (sun7i_can_is_absent(priv)) {
>                 printk(KERN_INFO "%s: probing @0x%lX failed\n",
>                  DRV_NAME, dev->base_addr);
>                 return 0;
>         }
>         return -1;

return proper error values

> }
> 
> static void set_reset_mode(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         uint32_t status = readl(CAN_MSEL_ADDR);
>         int i;
> 
>         for (i = 0; i < 100; i++) {
>                 /* check reset bit */
>                 if (status & RESET_MODE) {
>                         priv->can.state = CAN_STATE_STOPPED;
>                         return;
>                 }
> 
>                 writel(readl(CAN_MSEL_ADDR) | RESET_MODE, CAN_MSEL_ADDR);        /* select reset mode */
>                 //writel(RESET_MODE, CAN_MSEL_ADDR);        /* select reset mode */
>                 udelay(10);
>                 status = readl(CAN_MSEL_ADDR);
>         }
> 
>         netdev_err(dev, "setting SUN7I_CAN into reset mode failed!\n");
I think you can remove the SUN7I_CAN from the text, as netdev_* prints
all necessary information
> }
> 
> static void set_normal_mode(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         unsigned char status = readl(CAN_MSEL_ADDR);
>         int i;
> 
>         for (i = 0; i < 100; i++) {
>                 /* check reset bit */
>                 if ((status & RESET_MODE) == 0) {
>                         priv->can.state = CAN_STATE_ERROR_ACTIVE;												
> 						
> 						/* enable interrupts */
>                         if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
> 								writel(0xFFFF, CAN_INTEN_ADDR);
> 						} else {
> 								writel(0xFFFF & ~BERR_IRQ_EN, CAN_INTEN_ADDR);
> 						}
> 											
> 						if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> 							/* Put device into loopback mode */
> 							writel(readl(CAN_MSEL_ADDR) | LOOPBACK_MODE, CAN_MSEL_ADDR);
> 						} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
> 							/* Put device into listen-only mode */
> 							writel(readl(CAN_MSEL_ADDR) | LISTEN_ONLY_MODE, CAN_MSEL_ADDR);
> 						}
>                         return;
>                 }
Please use standard coding style.
> 
>                 /* set chip to normal mode */
>                 writel(readl(CAN_MSEL_ADDR) & (~RESET_MODE), CAN_MSEL_ADDR);
>                 udelay(10);
>                 status = readl(CAN_MSEL_ADDR);
>         }
> 
>         netdev_err(dev, "setting SUN7I_CAN into normal mode failed!\n");
> }
> 
> 
> static void sun7i_can_start(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
> 
>         /* leave reset mode */
>         if (priv->can.state != CAN_STATE_STOPPED)
>                 set_reset_mode(dev);
> 
>         /* Clear error counters and error code capture */
>         writel(0x0, CAN_ERRC_ADDR);
> 
>         /* leave reset mode */
>         set_normal_mode(dev);
> }
> 
> static int sun7i_can_set_mode(struct net_device *dev, enum can_mode mode)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
> 
>         if (!priv->open_time)
>                 return -EINVAL;
> 
>         switch (mode) {
>         case CAN_MODE_START:
>                 sun7i_can_start(dev);
>                 if (netif_queue_stopped(dev))
>                         netif_wake_queue(dev);
>                 break;
> 
>         default:
>                 return -EOPNOTSUPP;
>         }
> 
>         return 0;
> }
> 
> static int sun7i_can_set_bittiming(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         struct can_bittiming *bt = &priv->can.bittiming;
>         u32 cfg;
> 
>         cfg = ((bt->brp - 1) & 0x3FF)
>                 | (((bt->sjw - 1) & 0x3) << 14)
>                 | (((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) << 16)
>                 | (((bt->phase_seg2 - 1) & 0x7) << 20);
please use standard coding style - move | to the end of the line
>         if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>                 cfg |= 0x800000;
> 
>         netdev_info(dev, "setting BITTIMING=0x%08x\n", cfg);
> 
>         set_reset_mode(dev);                //CAN_BTIME_ADDR only writable in reset mode

no // comments
>         writel(cfg, CAN_BTIME_ADDR);
>         set_normal_mode(dev);
> 
>         return 0;
> }
> 
> static int sun7i_can_get_berr_counter(const struct net_device *dev,
>                                  struct can_berr_counter *bec)
> {
>         bec->txerr = readl(CAN_ERRC_ADDR) & 0x000F;
>         bec->rxerr = (readl(CAN_ERRC_ADDR) & 0x0F00) >> 16;

only read ERRC_ADDR once

> 
>         return 0;
> }
> 
> /*
> * initialize sun7i_can:
> * - reset chip
> * - set output mode
> * - set baudrate
> * - enable interrupts
> * - start operating mode
> */
> static void chipset_init(struct net_device *dev)
> {
>         u32 temp_irqen;
> 		
>         /* config pins
>          * PH20-TX, PH21-RX :4 */
> 
> 		if (gpio_request_ex("can_para", "can_tx") == 0 || gpio_request_ex("can_para", "can_rx") == 0 ) {
> 			printk(KERN_INFO "can request gpio fail!\n");
>         }

please fix indention
> 
>         //enable clock

>         writel(readl(0xF1C20000 + 0x6C) | (1 << 4), 0xF1C20000 + 0x6C);
please use the common clock framework
> 
>         //set can controller in reset mode
>         set_reset_mode(dev);
> 
>         //enable interrupt
>         temp_irqen = BERR_IRQ_EN | ERR_PASSIVE_IRQ_EN
>                         | OR_IRQ_EN | RX_IRQ_EN;
>         writel(readl(CAN_INTEN_ADDR) | temp_irqen, CAN_INTEN_ADDR);
> 
>         //return to transfer mode
>         set_normal_mode(dev);
> }
> 
> /*
> * transmit a CAN message
> * message layout in the sk_buff should be like this:
> * xx xx xx xx         ff         ll 00 11 22 33 44 55 66 77
> * [ can_id ] [flags] [len] [can data (up to 8 bytes]
> */
> static netdev_tx_t sun7i_can_start_xmit(struct sk_buff *skb,
>                                          struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         struct can_frame *cf = (struct can_frame *)skb->data;
>         uint8_t dlc;
>         canid_t id;
>         uint32_t temp = 0;
>         uint8_t i;
>         
>         //wait buff ready
>         while (!(readl(CAN_STA_ADDR) & TBUF_RDY));

What are we waiting for here?

> 
>         set_reset_mode(dev);
> 
>         writel(0xffffffff, CAN_ACPM_ADDR);
> 
>         //enter transfer mode
>         set_normal_mode(dev);
> 
>         if (can_dropped_invalid_skb(dev, skb))
>                 return NETDEV_TX_OK;
> 
>         netif_stop_queue(dev);
> 
>         dlc = cf->can_dlc;
>         id = cf->can_id;
> 
>         temp = ((id >> 30) << 6) | dlc;
>         writel(temp, CAN_BUF0_ADDR);
>         if (id & CAN_EFF_FLAG) {/* extern frame */
>                 writel(0xFF & (id >> 21), CAN_BUF1_ADDR);        //id28~21
>                 writel(0xFF & (id >> 13), CAN_BUF2_ADDR);         //id20~13
>                 writel(0xFF & (id >> 5), CAN_BUF3_ADDR);         //id12~5
>                 writel((id & 0x1F) << 3, CAN_BUF4_ADDR);         //id4~0
>                 
>                 for (i = 0; i < dlc; i++) {
>                         writel(cf->data[i], CAN_BUF5_ADDR + i * 4);
>                 }
>         } else {                /* standard frame*/        
>                 writel(0xFF & (id >> 3), CAN_BUF1_ADDR);         //id28~21
>                 writel((id & 0x7) << 5, CAN_BUF2_ADDR);                //id20~13
>                 
>                 for (i = 0; i < dlc; i++) {
>                         writel(cf->data[i], CAN_BUF3_ADDR + i * 4);
>                 }
>         }
> 
>         can_put_echo_skb(skb, dev, 0);
> 
>         while (!(readl(CAN_STA_ADDR) & TBUF_RDY));
What are we waiting for here?
>         sun7i_can_write_cmdreg(priv, TRANS_REQ);
> 
>         return NETDEV_TX_OK;
> }
> 
> static void sun7i_can_rx(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         struct net_device_stats *stats = &dev->stats;
>         struct can_frame *cf;
>         struct sk_buff *skb;
>         uint8_t fi;
>         canid_t id;
>         int i;
> 
>         /* create zero'ed CAN frame buffer */
>         skb = alloc_can_skb(dev, &cf);
>         if (skb == NULL)
>                 return;
> 
>         fi = readl(CAN_BUF0_ADDR);
>         cf->can_dlc = get_can_dlc(fi & 0x0F);
>         if (fi >> 7) {
>                 /* extended frame format (EFF) */
>                 id = (readl(CAN_BUF1_ADDR) << 21)        //id28~21
>                  | (readl(CAN_BUF2_ADDR) << 13)        //id20~13
>                  | (readl(CAN_BUF3_ADDR) << 5)        //id12~5
>                  | ((readl(CAN_BUF4_ADDR) >> 3) & 0x1f);        //id4~0
fix coding style
>                 id |= CAN_EFF_FLAG;
> 
>                 if ((fi >> 6) & 0x1) {        /* remote transmission request */
>                 id |= CAN_RTR_FLAG;
>                 } else {
>                         for (i = 0; i < cf->can_dlc; i++)
>                                 cf->data[i] = readl(CAN_BUF5_ADDR + i * 4);
>                 }
>         } else {
>                 /* standard frame format (SFF) */
>                 id = (readl(CAN_BUF1_ADDR) << 3)        //id28~21
>                  | ((readl(CAN_BUF2_ADDR) >> 5) & 0x7);        //id20~18
> 
>                 if ((fi >> 6) & 0x1) {        /* remote transmission request */
>                 id |= CAN_RTR_FLAG;
>                 } else {
>                         for (i = 0; i < cf->can_dlc; i++)
>                                 cf->data[i] = readl(CAN_BUF3_ADDR + i * 4);
>                 }
>         }
> 
>         cf->can_id = id;
> 
>         /* release receive buffer */
>         sun7i_can_write_cmdreg(priv, RELEASE_RBUF);
> 
>         netif_rx(skb);
> 
>         stats->rx_packets++;
>         stats->rx_bytes += cf->can_dlc;

don't touch skb after netif_rx

> }
> 
> static int sun7i_can_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         struct net_device_stats *stats = &dev->stats;
>         struct can_frame *cf;
>         struct sk_buff *skb;
>         enum can_state state = priv->can.state;
>         uint32_t ecc, alc;
> 
>         skb = alloc_can_err_skb(dev, &cf);
>         if (skb == NULL)
>                 return -ENOMEM;
> 
>         if (isrc & DATA_ORUNI) {
>                 /* data overrun interrupt */
>                 netdev_dbg(dev, "data overrun interrupt\n");
>                 cf->can_id |= CAN_ERR_CRTL;
>                 cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>                 stats->rx_over_errors++;
>                 stats->rx_errors++;
>                 sun7i_can_write_cmdreg(priv, CLEAR_DOVERRUN);        /* clear bit */
>         }
> 
>         if (isrc & ERR_WRN) {
>                 /* error warning interrupt */
>                 netdev_dbg(dev, "error warning interrupt\n");
> 
>                 if (status & BUS_OFF) {
>                         state = CAN_STATE_BUS_OFF;
>                         cf->can_id |= CAN_ERR_BUSOFF;
>                         can_bus_off(dev);
>                 } else if (status & ERR_STA) {
>                         state = CAN_STATE_ERROR_WARNING;
>                 } else
>                         state = CAN_STATE_ERROR_ACTIVE;
>         }
>         if (isrc & BUS_ERR) {
>                 /* bus error interrupt */
>                 priv->can.can_stats.bus_error++;
>                 stats->rx_errors++;
> 
>                 ecc = readl(CAN_STA_ADDR);
> 
>                 cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> 
>                 if(ecc & BIT_ERR)
>                         cf->data[2] |= CAN_ERR_PROT_BIT;
>                 else if (ecc & FORM_ERR)
>                         cf->data[2] |= CAN_ERR_PROT_FORM;
>                 else if (ecc & STUFF_ERR)
>                         cf->data[2] |= CAN_ERR_PROT_STUFF;
>                 else {
>                         cf->data[2] |= CAN_ERR_PROT_UNSPEC;
>                         cf->data[3] = (ecc & ERR_SEG_CODE) >> 16;
>                 }
>                 /* Error occurred during transmission? */
>                 if ((ecc & ERR_DIR) == 0)
>                         cf->data[2] |= CAN_ERR_PROT_TX;
>         }
>         if (isrc & ERR_PASSIVE) {
>                 /* error passive interrupt */
>                 netdev_dbg(dev, "error passive interrupt\n");
>                 if (status & ERR_STA)
>                         state = CAN_STATE_ERROR_PASSIVE;
>                 else
>                         state = CAN_STATE_ERROR_ACTIVE;
>         }
>         if (isrc & ARB_LOST) {
>                 /* arbitration lost interrupt */
>                 netdev_dbg(dev, "arbitration lost interrupt\n");
>                 alc = readl(CAN_STA_ADDR);
>                 priv->can.can_stats.arbitration_lost++;
>                 stats->tx_errors++;
>                 cf->can_id |= CAN_ERR_LOSTARB;
>                 cf->data[0] = (alc & 0x1f) >> 8;
>         }
> 
>         if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
>                                          state == CAN_STATE_ERROR_PASSIVE)) {
>                 uint8_t rxerr = (readl(CAN_ERRC_ADDR) >> 16) & 0xFF;
>                 uint8_t txerr = readl(CAN_ERRC_ADDR) & 0xFF;
>                 cf->can_id |= CAN_ERR_CRTL;
>                 if (state == CAN_STATE_ERROR_WARNING) {
>                         priv->can.can_stats.error_warning++;
>                         cf->data[1] = (txerr > rxerr) ?
>                                 CAN_ERR_CRTL_TX_WARNING :
>                                 CAN_ERR_CRTL_RX_WARNING;
>                 } else {
>                         priv->can.can_stats.error_passive++;
>                         cf->data[1] = (txerr > rxerr) ?
>                                 CAN_ERR_CRTL_TX_PASSIVE :
>                                 CAN_ERR_CRTL_RX_PASSIVE;
>                 }
>                 cf->data[6] = txerr;
>                 cf->data[7] = rxerr;
>         }
> 
>         priv->can.state = state;
> 
>         netif_rx(skb);
> 
>         stats->rx_packets++;
>         stats->rx_bytes += cf->can_dlc;
> 
>         return 0;
> }
> 
> irqreturn_t sun7i_can_interrupt(int irq, void *dev_id)
> {
>         struct net_device *dev = (struct net_device *)dev_id;
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         struct net_device_stats *stats = &dev->stats;
>         uint8_t isrc, status;
>         int n = 0;
> 
>         printk(KERN_INFO "sun7ican: capture a interrupt\n");

please remove this message
> 
>         /* Shared interrupts and IRQ off? */
>         if ((readl(CAN_INT_ADDR) & 0xF) == 0x0)
>                 return IRQ_NONE;
> 
>         while ((isrc = readl(CAN_INT_ADDR)) && (n < SUN7I_CAN_MAX_IRQ)) {
>                 n++;
>                 status = readl(CAN_STA_ADDR);
>                 /* check for absent controller due to hw unplug */

this is unlikely on a SoC internal core

>                 if (sun7i_can_is_absent(priv))
>                         return IRQ_NONE;
> 
>                 if (isrc & WAKEUP)
>                         netdev_warn(dev, "wakeup interrupt\n");
> 
>                 if (isrc & TBUF_VLD) {
>                         /* transmission complete interrupt */
>                         stats->tx_bytes += readl(CAN_RBUF_RBACK_START_ADDR) & 0xf;
>                         stats->tx_packets++;
>                         can_get_echo_skb(dev, 0);
>                         netif_wake_queue(dev);
>                 }
>                 if (isrc & RBUF_VLD) {
>                         /* receive interrupt */
>                         while (status & RBUF_RDY) {        //RX buffer is not empty
>                                 sun7i_can_rx(dev);
>                                 status = readl(CAN_STA_ADDR);
>                                 /* check for absent controller */
>                                 if (sun7i_can_is_absent(priv))
>                                         return IRQ_NONE;
>                         }
>                 }
>                 if (isrc & (DATA_ORUNI | ERR_WRN | BUS_ERR | ERR_PASSIVE | ARB_LOST)) {
>                         /* error interrupt */
>                         if (sun7i_can_err(dev, isrc, status))
>                                 break;
>                 }
> 
>                 //clear the interrupt
>                 writel(isrc, CAN_INT_ADDR);
>                 udelay(10);
Why the delay?
>         }
> 
>         if (n >= SUN7I_CAN_MAX_IRQ)
>                 netdev_dbg(dev, "%d messages handled in ISR", n);
> 
>         return (n) ? IRQ_HANDLED : IRQ_NONE;
> }
> EXPORT_SYMBOL_GPL(sun7i_can_interrupt);
why is this function exported?
> 
> static int sun7i_can_open(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
>         int err;
> 
>         /* set chip into reset mode */
>         set_reset_mode(dev);
> 
>         /* common open */
>         err = open_candev(dev);
>         if (err)
>                 return err;
> 
>         /* register interrupt handler, if not done by the device driver */
>         if (!(priv->flags & SUN7I_CAN_CUSTOM_IRQ_HANDLER)) {

As this should be a non modular drive the custon IRQ handler can be removed.

>                 err = request_irq(dev->irq, sun7i_can_interrupt, priv->irq_flags,
>                                  dev->name, (void *)dev);
>                 if (err) {
>                         close_candev(dev);
>                         printk(KERN_INFO "request_irq err:%d\n", err);
>                         return -EAGAIN;
>                 }
>         }
> 
>         /* init and start chi */
>         sun7i_can_start(dev);
>         priv->open_time = jiffies;

remove open_time, too
> 
>         netif_start_queue(dev);
> 
>         return 0;
> }
> 
> static int sun7i_can_close(struct net_device *dev)
> {
>         struct sun7i_can_priv *priv = netdev_priv(dev);
> 
>         netif_stop_queue(dev);
>         set_reset_mode(dev);
> 
>         if (!(priv->flags & SUN7I_CAN_CUSTOM_IRQ_HANDLER))
>                 free_irq(dev->irq, (void *)dev);
> 
>         close_candev(dev);
> 
>         priv->open_time = 0;
> 
>         return 0;
> }
> 
> struct net_device *alloc_sun7icandev(int sizeof_priv)
> {
>         struct net_device *dev;
>         struct sun7i_can_priv *priv;
> 
>         dev = alloc_candev(sizeof(struct sun7i_can_priv) + sizeof_priv,
>                 SUN7I_CAN_ECHO_SKB_MAX);
>         if (!dev)
>                 return NULL;
> 
>         priv = netdev_priv(dev);
> 
>         priv->dev = dev;
>         priv->can.bittiming_const = &sun7i_can_bittiming_const;
>         priv->can.do_set_bittiming = sun7i_can_set_bittiming;
>         priv->can.do_set_mode = sun7i_can_set_mode;
>         priv->can.do_get_berr_counter = sun7i_can_get_berr_counter;
>         priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>                 CAN_CTRLMODE_LISTENONLY |
>                 CAN_CTRLMODE_3_SAMPLES |
>                 CAN_CTRLMODE_BERR_REPORTING;
> 
>         spin_lock_init(&priv->cmdreg_lock);
> 
>         if (sizeof_priv)
>                 priv->priv = (void *)priv + sizeof(struct sun7i_can_priv);
> 
>         return dev;
> }
> EXPORT_SYMBOL_GPL(alloc_sun7icandev);
> 
> void free_sun7icandev(struct net_device *dev)
> {
>         free_candev(dev);
> }
> EXPORT_SYMBOL_GPL(free_sun7icandev);
> 
> static const struct net_device_ops sun7ican_netdev_ops = {
>        .ndo_open = sun7i_can_open,
>        .ndo_stop = sun7i_can_close,
>        .ndo_start_xmit = sun7i_can_start_xmit,
> };
> 
> int register_sun7icandev(struct net_device *dev)
> {
>         if (!sun7i_can_probe(dev))
>                 return -ENODEV;
> 
>         dev->flags |= IFF_ECHO;        /* support local echo */
>         dev->netdev_ops = &sun7ican_netdev_ops;
> 
>         set_reset_mode(dev);
>         
>         return register_candev(dev);
> }
> EXPORT_SYMBOL_GPL(register_sun7icandev);
> 
> void unregister_sun7icandev(struct net_device *dev)
> {
>         set_reset_mode(dev);
>         unregister_candev(dev);
> }
> EXPORT_SYMBOL_GPL(unregister_sun7icandev);
> 
> static __init int sun7i_can_init(void)
> {
>         struct sun7i_can_priv *priv;
>         int err = 0;
> 		int ret = 0;
> 		int used = 0;
> 		
>         sun7ican_dev = alloc_sun7icandev(0);
>         if(!sun7ican_dev) {
>                 printk(KERN_INFO "alloc sun7icandev fail\n");
>         }
> 	
> 		ret = script_parser_fetch("can_para", "can_used", &used, sizeof (used));
> 		if ( ret || used == 0) {
> 			printk(KERN_INFO "[sun7i-can] Cannot setup CANBus driver, maybe not configured in script.bin?");
> 			goto exit_free;
> 		}

Please add proper DT bindings for this driver.

> 		
>         priv = netdev_priv(sun7ican_dev);
>         sun7ican_dev->irq = SW_INT_IRQNO_CAN;
>         priv->irq_flags = 0;
>         priv->can.clock.freq = clk_get_rate(clk_get(NULL, CLK_MOD_CAN));
>         chipset_init(sun7ican_dev);
>         err = register_sun7icandev(sun7ican_dev);
>         if(err) {
>                 dev_err(&sun7ican_dev->dev, "registering %s failed (err=%d)\n", DRV_NAME, err);
>                 goto exit_free;
>         }
> 
>         dev_info(&sun7ican_dev->dev, "%s device registered (reg_base=0x%08x, irq=%d)\n",
>                  DRV_NAME, CAN_BASE0, sun7ican_dev->irq);
> 
>         printk(KERN_INFO "%s CAN netdevice driver\n", DRV_NAME);
> 
>         return 0;
> 
> exit_free:
>         free_sun7icandev(sun7ican_dev);
> 
>         return err;
> }
> module_init(sun7i_can_init);
> 
> static __exit void sun7i_can_exit(void)
> {
>         unregister_sun7icandev(sun7ican_dev);
>         free_sun7icandev(sun7ican_dev);
> 
>         printk(KERN_INFO "%s: driver removed\n", DRV_NAME);
> }
> module_exit(sun7i_can_exit);

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: 455 bytes --]