Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 17.03.2017 um 19:28 schrieb Akshay Bhat: Hi Wolfgang, On 03/17/2017 01:04 PM, Wolfgang Grandegger wrote: Hm, that's unusual. Cable disconnected and then send a message: $ grep /proc/interrupts; sleep 10; /proc/interrupts should make things clear. But maybe it's a clever chip and it does stop sending error messages if the error counter does not change any more. After bus-off, the chip is quiet, of course. Should have a closer look to the CAN standard. The interrupt count does not increment after device reaches tx-error-passive (with cable disconnected). # while true; do grep -i hi3110 /proc/interrupts; sleep 10; done & [1] 793 # 111: 0 0 gpio-mxc 12 Edge hi3110 # candump -t d -e any,0:0,#FFF & [2] 798 # cansend can0 123# # (000.00) can0 2004 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} error-counter-tx-rx{{96}{0}} (000.002122) can0 2004 [8] 00 20 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} error-counter-tx-rx{{128}{0}} 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 OK, then there is no good reason connecting the STAT interrupt pin. Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 17.03.2017 um 19:28 schrieb Akshay Bhat: Hi Wolfgang, On 03/17/2017 01:04 PM, Wolfgang Grandegger wrote: Hm, that's unusual. Cable disconnected and then send a message: $ grep /proc/interrupts; sleep 10; /proc/interrupts should make things clear. But maybe it's a clever chip and it does stop sending error messages if the error counter does not change any more. After bus-off, the chip is quiet, of course. Should have a closer look to the CAN standard. The interrupt count does not increment after device reaches tx-error-passive (with cable disconnected). # while true; do grep -i hi3110 /proc/interrupts; sleep 10; done & [1] 793 # 111: 0 0 gpio-mxc 12 Edge hi3110 # candump -t d -e any,0:0,#FFF & [2] 798 # cansend can0 123# # (000.00) can0 2004 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} error-counter-tx-rx{{96}{0}} (000.002122) can0 2004 [8] 00 20 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} error-counter-tx-rx{{128}{0}} 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 OK, then there is no good reason connecting the STAT interrupt pin. Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/17/2017 01:04 PM, Wolfgang Grandegger wrote: > > Hm, that's unusual. Cable disconnected and then send a message: > > $ grep /proc/interrupts; sleep 10; /proc/interrupts > > should make things clear. But maybe it's a clever chip and it does stop > sending error messages if the error counter does not change any more. > After bus-off, the chip is quiet, of course. Should have a closer look > to the CAN standard. > The interrupt count does not increment after device reaches tx-error-passive (with cable disconnected). # while true; do grep -i hi3110 /proc/interrupts; sleep 10; done & [1] 793 # 111: 0 0 gpio-mxc 12 Edge hi3110 # candump -t d -e any,0:0,#FFF & [2] 798 # cansend can0 123# # (000.00) can0 2004 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} error-counter-tx-rx{{96}{0}} (000.002122) can0 2004 [8] 00 20 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} error-counter-tx-rx{{128}{0}} 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 >>> So far using NAPI was mandatory. There is the problem of out-of-order >>> message reception if handled in the isr on multi processor systems. >>> Marc, what is the current policy? >>> >> >> Since this is a SPI based CAN, I am wary for any additional latencies >> NAPI might introduce. The RX handling is being done at the very >> beginning of the ISR for this reason. >> >> Can we go ahead with the existing implementation and re-visit this at a >> later time? > > Likely yes, as Marc has already reviewed the driver once. > Thanks, I will go ahead and submit v4 patch. > BTW: what system board/processor are you using? > It is a custom board using Phytec phyFLEX-i.MX6 Dual SOM.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/17/2017 01:04 PM, Wolfgang Grandegger wrote: > > Hm, that's unusual. Cable disconnected and then send a message: > > $ grep /proc/interrupts; sleep 10; /proc/interrupts > > should make things clear. But maybe it's a clever chip and it does stop > sending error messages if the error counter does not change any more. > After bus-off, the chip is quiet, of course. Should have a closer look > to the CAN standard. > The interrupt count does not increment after device reaches tx-error-passive (with cable disconnected). # while true; do grep -i hi3110 /proc/interrupts; sleep 10; done & [1] 793 # 111: 0 0 gpio-mxc 12 Edge hi3110 # candump -t d -e any,0:0,#FFF & [2] 798 # cansend can0 123# # (000.00) can0 2004 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} error-counter-tx-rx{{96}{0}} (000.002122) can0 2004 [8] 00 20 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} error-counter-tx-rx{{128}{0}} 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 111: 10 0 gpio-mxc 12 Edge hi3110 >>> So far using NAPI was mandatory. There is the problem of out-of-order >>> message reception if handled in the isr on multi processor systems. >>> Marc, what is the current policy? >>> >> >> Since this is a SPI based CAN, I am wary for any additional latencies >> NAPI might introduce. The RX handling is being done at the very >> beginning of the ISR for this reason. >> >> Can we go ahead with the existing implementation and re-visit this at a >> later time? > > Likely yes, as Marc has already reviewed the driver once. > Thanks, I will go ahead and submit v4 patch. > BTW: what system board/processor are you using? > It is a custom board using Phytec phyFLEX-i.MX6 Dual SOM.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Akshay, Am 17.03.2017 um 17:00 schrieb Akshay Bhat: Hi Wolfgang, On 03/17/2017 03:39 AM, Wolfgang Grandegger wrote: Hello Akshay, Am 16.03.2017 um 23:29 schrieb Akshay Bhat: Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Hm, yes. The raw data looks correct. You could download and build a recent version from "https://github.com/linux-can/can-utils; to check. It could also be a bug. Turned out to be a old version of can-utils. Using the above git tree reports the flag. (000.200308) can0 2004 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME controller-problem{back-to-error-active} error-counter-tx-rx{{95}{0}} Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts on the INT pin. Should we make it a requirement that both INT and STAT pins need to be connected in hardware for the driver to do the error reporting? As I said, it's the better solution, especially if interrupt flooding does harm. How does your system behave when bus errors come in due to no cable connected? I did not see any issues on the system with the cable disconnected. In my particular setup with the cable disconnected the system goes to tx-error-passive and does not get any further interrupts until a state change occurs. Hm, that's unusual. Cable disconnected and then send a message: $ grep /proc/interrupts; sleep 10; /proc/interrupts should make things clear. But maybe it's a clever chip and it does stop sending error messages if the error counter does not change any more. After bus-off, the chip is quiet, of course. Should have a closer look to the CAN standard. So far using NAPI was mandatory. There is the problem of out-of-order message reception if handled in the isr on multi processor systems. Marc, what is the current policy? Since this is a SPI based CAN, I am wary for any additional latencies NAPI might introduce. The RX handling is being done at the very beginning of the ISR for this reason. Can we go ahead with the existing implementation and re-visit this at a later time? Likely yes, as Marc has already reviewed the driver once. BTW: what system board/processor are you using? Thanks again for all your help in reviewing/improving the driver :) You are welcome! Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Akshay, Am 17.03.2017 um 17:00 schrieb Akshay Bhat: Hi Wolfgang, On 03/17/2017 03:39 AM, Wolfgang Grandegger wrote: Hello Akshay, Am 16.03.2017 um 23:29 schrieb Akshay Bhat: Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Hm, yes. The raw data looks correct. You could download and build a recent version from "https://github.com/linux-can/can-utils; to check. It could also be a bug. Turned out to be a old version of can-utils. Using the above git tree reports the flag. (000.200308) can0 2004 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME controller-problem{back-to-error-active} error-counter-tx-rx{{95}{0}} Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts on the INT pin. Should we make it a requirement that both INT and STAT pins need to be connected in hardware for the driver to do the error reporting? As I said, it's the better solution, especially if interrupt flooding does harm. How does your system behave when bus errors come in due to no cable connected? I did not see any issues on the system with the cable disconnected. In my particular setup with the cable disconnected the system goes to tx-error-passive and does not get any further interrupts until a state change occurs. Hm, that's unusual. Cable disconnected and then send a message: $ grep /proc/interrupts; sleep 10; /proc/interrupts should make things clear. But maybe it's a clever chip and it does stop sending error messages if the error counter does not change any more. After bus-off, the chip is quiet, of course. Should have a closer look to the CAN standard. So far using NAPI was mandatory. There is the problem of out-of-order message reception if handled in the isr on multi processor systems. Marc, what is the current policy? Since this is a SPI based CAN, I am wary for any additional latencies NAPI might introduce. The RX handling is being done at the very beginning of the ISR for this reason. Can we go ahead with the existing implementation and re-visit this at a later time? Likely yes, as Marc has already reviewed the driver once. BTW: what system board/processor are you using? Thanks again for all your help in reviewing/improving the driver :) You are welcome! Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/17/2017 03:39 AM, Wolfgang Grandegger wrote: > Hello Akshay, > > Am 16.03.2017 um 23:29 schrieb Akshay Bhat: >> Hi Wolfgang, >> >> On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: >>> >>> Looks much better now! There are message for state changes to error >>> warning and passive. Just the following message is not correct: >>> >>> (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 >>> ERRORFRAME >>> controller-problem{} >>> error-counter-tx-rx{{95}{25}} >>> >>> Sorry, forgot to mention... the function can_change_state() [1] >>> should be used for that purpose, if possible. It fixes the issue >>> above as well. >>> >> >> The updated driver (the one used to create the above log) is using >> can_change_state() function. data[1] 40 corresponds to >> CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I >> am using is old and not reporting state change? > > Hm, yes. The raw data looks correct. You could download and build a > recent version from "https://github.com/linux-can/can-utils; to check. > It could also be a bug. > Turned out to be a old version of can-utils. Using the above git tree reports the flag. (000.200308) can0 2004 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME controller-problem{back-to-error-active} error-counter-tx-rx{{95}{0}} >> Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts >> on the INT pin. Should we make it a requirement that both INT and STAT >> pins need to be connected in hardware for the driver to do the error >> reporting? > > As I said, it's the better solution, especially if interrupt flooding > does harm. How does your system behave when bus errors come in due to no > cable connected? > I did not see any issues on the system with the cable disconnected. In my particular setup with the cable disconnected the system goes to tx-error-passive and does not get any further interrupts until a state change occurs. > So far using NAPI was mandatory. There is the problem of out-of-order > message reception if handled in the isr on multi processor systems. > Marc, what is the current policy? > Since this is a SPI based CAN, I am wary for any additional latencies NAPI might introduce. The RX handling is being done at the very beginning of the ISR for this reason. Can we go ahead with the existing implementation and re-visit this at a later time? Thanks again for all your help in reviewing/improving the driver :) Akshay > Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/17/2017 03:39 AM, Wolfgang Grandegger wrote: > Hello Akshay, > > Am 16.03.2017 um 23:29 schrieb Akshay Bhat: >> Hi Wolfgang, >> >> On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: >>> >>> Looks much better now! There are message for state changes to error >>> warning and passive. Just the following message is not correct: >>> >>> (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 >>> ERRORFRAME >>> controller-problem{} >>> error-counter-tx-rx{{95}{25}} >>> >>> Sorry, forgot to mention... the function can_change_state() [1] >>> should be used for that purpose, if possible. It fixes the issue >>> above as well. >>> >> >> The updated driver (the one used to create the above log) is using >> can_change_state() function. data[1] 40 corresponds to >> CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I >> am using is old and not reporting state change? > > Hm, yes. The raw data looks correct. You could download and build a > recent version from "https://github.com/linux-can/can-utils; to check. > It could also be a bug. > Turned out to be a old version of can-utils. Using the above git tree reports the flag. (000.200308) can0 2004 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME controller-problem{back-to-error-active} error-counter-tx-rx{{95}{0}} >> Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts >> on the INT pin. Should we make it a requirement that both INT and STAT >> pins need to be connected in hardware for the driver to do the error >> reporting? > > As I said, it's the better solution, especially if interrupt flooding > does harm. How does your system behave when bus errors come in due to no > cable connected? > I did not see any issues on the system with the cable disconnected. In my particular setup with the cable disconnected the system goes to tx-error-passive and does not get any further interrupts until a state change occurs. > So far using NAPI was mandatory. There is the problem of out-of-order > message reception if handled in the isr on multi processor systems. > Marc, what is the current policy? > Since this is a SPI based CAN, I am wary for any additional latencies NAPI might introduce. The RX handling is being done at the very beginning of the ISR for this reason. Can we go ahead with the existing implementation and re-visit this at a later time? Thanks again for all your help in reviewing/improving the driver :) Akshay > Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 17.03.2017 um 08:39 schrieb Wolfgang Grandegger: Hello Akshay, Am 16.03.2017 um 23:29 schrieb Akshay Bhat: Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Hm, yes. The raw data looks correct. You could download and build a recent version from "https://github.com/linux-can/can-utils; to check. It could also be a bug. Support for that flags has been added in December 2014. Wolfgang-
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 17.03.2017 um 08:39 schrieb Wolfgang Grandegger: Hello Akshay, Am 16.03.2017 um 23:29 schrieb Akshay Bhat: Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Hm, yes. The raw data looks correct. You could download and build a recent version from "https://github.com/linux-can/can-utils; to check. It could also be a bug. Support for that flags has been added in December 2014. Wolfgang-
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 16.03.2017 um 23:29 schrieb Akshay Bhat: Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Hm, yes. The raw data looks correct. You could download and build a recent version from "https://github.com/linux-can/can-utils; to check. It could also be a bug. Tentative v4 driver for reference: http://pastebin.com/4xFVL1Sj berr-reporting off case: http://pastebin.com/fUn3j7qU Ditto. I just had another look to the manual and there is this undocumented STATFE register at offset 0x1E. It's mentioned in some other parts of the doc as interrupt enable register for STATF events. I would assume the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP) and 4 (ERRW), you may get interrupts. It's worth a try, I think. If it works, it's the much better solution. The HI-311x has a INT pin and a STAT pin. The hardware I have has only the INT pin connected to the processor. If the STAT pin was also connected, then like you mentioned it could be a much better solution to use STAT for state changes. OK, I understand. Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts on the INT pin. Should we make it a requirement that both INT and STAT pins need to be connected in hardware for the driver to do the error reporting? As I said, it's the better solution, especially if interrupt flooding does harm. How does your system behave when bus errors come in due to no cable connected? So far using NAPI was mandatory. There is the problem of out-of-order message reception if handled in the isr on multi processor systems. Marc, what is the current policy? Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 16.03.2017 um 23:29 schrieb Akshay Bhat: Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Hm, yes. The raw data looks correct. You could download and build a recent version from "https://github.com/linux-can/can-utils; to check. It could also be a bug. Tentative v4 driver for reference: http://pastebin.com/4xFVL1Sj berr-reporting off case: http://pastebin.com/fUn3j7qU Ditto. I just had another look to the manual and there is this undocumented STATFE register at offset 0x1E. It's mentioned in some other parts of the doc as interrupt enable register for STATF events. I would assume the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP) and 4 (ERRW), you may get interrupts. It's worth a try, I think. If it works, it's the much better solution. The HI-311x has a INT pin and a STAT pin. The hardware I have has only the INT pin connected to the processor. If the STAT pin was also connected, then like you mentioned it could be a much better solution to use STAT for state changes. OK, I understand. Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts on the INT pin. Should we make it a requirement that both INT and STAT pins need to be connected in hardware for the driver to do the error reporting? As I said, it's the better solution, especially if interrupt flooding does harm. How does your system behave when bus errors come in due to no cable connected? So far using NAPI was mandatory. There is the problem of out-of-order message reception if handled in the isr on multi processor systems. Marc, what is the current policy? Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: > > Looks much better now! There are message for state changes to error > warning and passive. Just the following message is not correct: > > (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME > controller-problem{} > error-counter-tx-rx{{95}{25}} > > Sorry, forgot to mention... the function can_change_state() [1] > should be used for that purpose, if possible. It fixes the issue > above as well. > The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Tentative v4 driver for reference: http://pastebin.com/4xFVL1Sj >> berr-reporting off case: >> http://pastebin.com/fUn3j7qU > > Ditto. > > I just had another look to the manual and there is this undocumented > STATFE register at offset 0x1E. It's mentioned in some other parts of > the doc as interrupt enable register for STATF events. I would assume > the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP) > and 4 (ERRW), you may get interrupts. It's worth a try, I think. If > it works, it's the much better solution. > The HI-311x has a INT pin and a STAT pin. The hardware I have has only the INT pin connected to the processor. If the STAT pin was also connected, then like you mentioned it could be a much better solution to use STAT for state changes. Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts on the INT pin. Should we make it a requirement that both INT and STAT pins need to be connected in hardware for the driver to do the error reporting? Thanks, Akshay > Wolfgang. > > [1] http://lxr.free-electrons.com/ident?i=can_change_state > > Wolfgang. >
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote: > > Looks much better now! There are message for state changes to error > warning and passive. Just the following message is not correct: > > (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME > controller-problem{} > error-counter-tx-rx{{95}{25}} > > Sorry, forgot to mention... the function can_change_state() [1] > should be used for that purpose, if possible. It fixes the issue > above as well. > The updated driver (the one used to create the above log) is using can_change_state() function. data[1] 40 corresponds to CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I am using is old and not reporting state change? Tentative v4 driver for reference: http://pastebin.com/4xFVL1Sj >> berr-reporting off case: >> http://pastebin.com/fUn3j7qU > > Ditto. > > I just had another look to the manual and there is this undocumented > STATFE register at offset 0x1E. It's mentioned in some other parts of > the doc as interrupt enable register for STATF events. I would assume > the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP) > and 4 (ERRW), you may get interrupts. It's worth a try, I think. If > it works, it's the much better solution. > The HI-311x has a INT pin and a STAT pin. The hardware I have has only the INT pin connected to the processor. If the STAT pin was also connected, then like you mentioned it could be a much better solution to use STAT for state changes. Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts on the INT pin. Should we make it a requirement that both INT and STAT pins need to be connected in hardware for the driver to do the error reporting? Thanks, Akshay > Wolfgang. > > [1] http://lxr.free-electrons.com/ident?i=can_change_state > > Wolfgang. >
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 16.03.2017 um 18:06 schrieb Akshay Bhat: > Hi Wolfgang, > > On 03/15/2017 05:42 AM, Wolfgang Grandegger wrote: >> Hello Akshay, >> > ..snip.. >>> >>> So here is my plan: >>> - Have the bus error interrupt always enabled >>> - If berr-reporting off, then have the isr checks/reports state changes >> >> Error state change messages should always be there. These are the >> important one. >> >>> - if berr-reporting on, then have the isr checks/reports bus errors >>> and state changes (Does it make sense packing the error message, if >>> the ISR finds both bus and state changes?) >> >> If berr-reporting is off, simply do not create an error message for bus >> errors, and only if the state changed. If it's "on" create an additional >> bus error message. >> >> http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334 >> >> > > I have fixed the driver to handle the error reporting. Also thanks for > your tip for generating bus-off by setting the host device at a > different CAN bit rate! Below are logs with the updated driver. Let me > know if you have any concerns, if not I will submit the v4 patch. > > berr-reporting on case: > http://pastebin.com/qDRLERmW Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. > berr-reporting off case: > http://pastebin.com/fUn3j7qU Ditto. I just had another look to the manual and there is this undocumented STATFE register at offset 0x1E. It's mentioned in some other parts of the doc as interrupt enable register for STATF events. I would assume the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP) and 4 (ERRW), you may get interrupts. It's worth a try, I think. If it works, it's the much better solution. Wolfgang. [1] http://lxr.free-electrons.com/ident?i=can_change_state Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 16.03.2017 um 18:06 schrieb Akshay Bhat: > Hi Wolfgang, > > On 03/15/2017 05:42 AM, Wolfgang Grandegger wrote: >> Hello Akshay, >> > ..snip.. >>> >>> So here is my plan: >>> - Have the bus error interrupt always enabled >>> - If berr-reporting off, then have the isr checks/reports state changes >> >> Error state change messages should always be there. These are the >> important one. >> >>> - if berr-reporting on, then have the isr checks/reports bus errors >>> and state changes (Does it make sense packing the error message, if >>> the ISR finds both bus and state changes?) >> >> If berr-reporting is off, simply do not create an error message for bus >> errors, and only if the state changed. If it's "on" create an additional >> bus error message. >> >> http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334 >> >> > > I have fixed the driver to handle the error reporting. Also thanks for > your tip for generating bus-off by setting the host device at a > different CAN bit rate! Below are logs with the updated driver. Let me > know if you have any concerns, if not I will submit the v4 patch. > > berr-reporting on case: > http://pastebin.com/qDRLERmW Looks much better now! There are message for state changes to error warning and passive. Just the following message is not correct: (000.200824) can0 2004 [8] 00 40 00 00 00 00 5F 19 ERRORFRAME controller-problem{} error-counter-tx-rx{{95}{25}} Sorry, forgot to mention... the function can_change_state() [1] should be used for that purpose, if possible. It fixes the issue above as well. > berr-reporting off case: > http://pastebin.com/fUn3j7qU Ditto. I just had another look to the manual and there is this undocumented STATFE register at offset 0x1E. It's mentioned in some other parts of the doc as interrupt enable register for STATF events. I would assume the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP) and 4 (ERRW), you may get interrupts. It's worth a try, I think. If it works, it's the much better solution. Wolfgang. [1] http://lxr.free-electrons.com/ident?i=can_change_state Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/15/2017 05:42 AM, Wolfgang Grandegger wrote: > Hello Akshay, > ..snip.. >> >> So here is my plan: >> - Have the bus error interrupt always enabled >> - If berr-reporting off, then have the isr checks/reports state changes > > Error state change messages should always be there. These are the > important one. > >> - if berr-reporting on, then have the isr checks/reports bus errors >> and state changes (Does it make sense packing the error message, if >> the ISR finds both bus and state changes?) > > If berr-reporting is off, simply do not create an error message for bus > errors, and only if the state changed. If it's "on" create an additional > bus error message. > > http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334 > > I have fixed the driver to handle the error reporting. Also thanks for your tip for generating bus-off by setting the host device at a different CAN bit rate! Below are logs with the updated driver. Let me know if you have any concerns, if not I will submit the v4 patch. berr-reporting on case: http://pastebin.com/qDRLERmW berr-reporting off case: http://pastebin.com/fUn3j7qU Thanks, Akshay
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/15/2017 05:42 AM, Wolfgang Grandegger wrote: > Hello Akshay, > ..snip.. >> >> So here is my plan: >> - Have the bus error interrupt always enabled >> - If berr-reporting off, then have the isr checks/reports state changes > > Error state change messages should always be there. These are the > important one. > >> - if berr-reporting on, then have the isr checks/reports bus errors >> and state changes (Does it make sense packing the error message, if >> the ISR finds both bus and state changes?) > > If berr-reporting is off, simply do not create an error message for bus > errors, and only if the state changed. If it's "on" create an additional > bus error message. > > http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334 > > I have fixed the driver to handle the error reporting. Also thanks for your tip for generating bus-off by setting the host device at a different CAN bit rate! Below are logs with the updated driver. Let me know if you have any concerns, if not I will submit the v4 patch. berr-reporting on case: http://pastebin.com/qDRLERmW berr-reporting off case: http://pastebin.com/fUn3j7qU Thanks, Akshay
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 15.03.2017 um 05:44 schrieb Akshay Bhat: Hi Wolfgang, On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandeggerwrote: ...snip /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. This support was missing in the driver, added in V4 patch. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". HI-3110 has only 1 bus error interrupt. There is no dedicated state change interrupts like other controllers. I have little hope! Some revision of the flexcan controller have the same problem So here is my plan: - Have the bus error interrupt always enabled - If berr-reporting off, then have the isr checks/reports state changes Error state change messages should always be there. These are the important one. - if berr-reporting on, then have the isr checks/reports bus errors and state changes (Does it make sense packing the error message, if the ISR finds both bus and state changes?) If berr-reporting is off, simply do not create an error message for bus errors, and only if the state changed. If it's "on" create an additional bus error message. http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334 write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 The error warning and passive counter increased , though. Also the bus error should come in at a rather hight rate. Looking to the code, maybe you need to test STATF to check for state changes (and not ERR). Apologize, just realized In the above case some error packets were lost, because I forgot to set the CPU frequency to max. Will resend the log. ..snip... After some more messages there should be also: can0 2200 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME state-change{back-to-error-active} error-counter-tx-rx{{95}{0}} For each message sent, the error counter decreases by 8. The HI-3110 controller decrements the error counter by 1 for every message sent. The error count increments by 8 when there is an error. Seems OK according to: http://electronics.stackexchange.com/questions/220596/can-error-counters-behaviour root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 1 0 0 0 0 Strange, some counters got lost. This was a bug introduced when adding berr-reporting, have fixed in v4 patch. I have not been able to check the bus-off condition by (short-circuiting CAN low and high). The tec error count remains at 128 when I short the CAN low and high pins and the status never goes BUSOFF. You also need to send a message and the short-circuit should be at the connector of the sending host. What tranceiver is used? Do you know? ADM3054 transceiver is used with HI-3111. I connected the
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 15.03.2017 um 05:44 schrieb Akshay Bhat: Hi Wolfgang, On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandegger wrote: ...snip /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. This support was missing in the driver, added in V4 patch. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". HI-3110 has only 1 bus error interrupt. There is no dedicated state change interrupts like other controllers. I have little hope! Some revision of the flexcan controller have the same problem So here is my plan: - Have the bus error interrupt always enabled - If berr-reporting off, then have the isr checks/reports state changes Error state change messages should always be there. These are the important one. - if berr-reporting on, then have the isr checks/reports bus errors and state changes (Does it make sense packing the error message, if the ISR finds both bus and state changes?) If berr-reporting is off, simply do not create an error message for bus errors, and only if the state changed. If it's "on" create an additional bus error message. http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334 write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 The error warning and passive counter increased , though. Also the bus error should come in at a rather hight rate. Looking to the code, maybe you need to test STATF to check for state changes (and not ERR). Apologize, just realized In the above case some error packets were lost, because I forgot to set the CPU frequency to max. Will resend the log. ..snip... After some more messages there should be also: can0 2200 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME state-change{back-to-error-active} error-counter-tx-rx{{95}{0}} For each message sent, the error counter decreases by 8. The HI-3110 controller decrements the error counter by 1 for every message sent. The error count increments by 8 when there is an error. Seems OK according to: http://electronics.stackexchange.com/questions/220596/can-error-counters-behaviour root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 1 0 0 0 0 Strange, some counters got lost. This was a bug introduced when adding berr-reporting, have fixed in v4 patch. I have not been able to check the bus-off condition by (short-circuiting CAN low and high). The tec error count remains at 128 when I short the CAN low and high pins and the status never goes BUSOFF. You also need to send a message and the short-circuit should be at the connector of the sending host. What tranceiver is used? Do you know? ADM3054 transceiver is used with HI-3111. I connected the HI-3111/ADM3054 board to kvaser leaf and ran "cangen -i can0" and "candump -e
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 15.03.2017 um 05:44 schrieb Akshay Bhat: Hi Wolfgang, On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandeggerwrote: ...snip /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. This support was missing in the driver, added in V4 patch. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". HI-3110 has only 1 bus error interrupt. There is no dedicated state change interrupts like other controllers. To double check: Could you please read INTF, ERR and STATF at the beginning of the ISR and print it out (using dev_dbg and fiends). Then run a test with no cable connected and bus error reporting off. Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 15.03.2017 um 05:44 schrieb Akshay Bhat: Hi Wolfgang, On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandegger wrote: ...snip /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. This support was missing in the driver, added in V4 patch. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". HI-3110 has only 1 bus error interrupt. There is no dedicated state change interrupts like other controllers. To double check: Could you please read INTF, ERR and STATF at the beginning of the ISR and print it out (using dev_dbg and fiends). Then run a test with no cable connected and bus error reporting off. Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandeggerwrote: ...snip >> /disconnect cable >> can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{40}{0}} >> can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{88}{0}} >> can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{128}{0}} > > > TX error warning is missing. > This support was missing in the driver, added in V4 patch. >> can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME >> controller-problem{tx-error-passive} >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{128}{0}} > > > Here "tx-error-passiv" is packed with a bus error. What I'm looking for are > state change messages similar to: > >can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME > controller-problem{tx-error-warning} > state-change{tx-error-warning} > error-counter-tx-rx{{96}{0}} >can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME > controller-problem{tx-error-passive} > state-change{tx-error-passive} > error-counter-tx-rx{{128}{0} > > They should always come, even with "berr-reporting off". > HI-3110 has only 1 bus error interrupt. There is no dedicated state change interrupts like other controllers. So here is my plan: - Have the bus error interrupt always enabled - If berr-reporting off, then have the isr checks/reports state changes - if berr-reporting on, then have the isr checks/reports bus errors and state changes (Does it make sense packing the error message, if the ISR finds both bus and state changes?) >> write: No buffer space available >> root@imx6qrom5420b1:~# ip -s -d link show can0 >> 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN >> mode DEFAULT group default qlen 10 >> link/can promiscuity 0 >> can state ERROR-PASSIVE (berr-counter tx 128 rx 0) >> restart-ms 0 >> bitrate 100 sample-point 0.750 >> tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 >> hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 >> clock 1600 >> re-started bus-errors arbit-lost error-warn error-pass bus-off >> 0 6 0 1 1 0 > > > The error warning and passive counter increased , though. Also the bus error > should come in at a rather hight rate. Looking to the code, maybe > you need to test STATF to check for state changes (and not ERR). > Apologize, just realized In the above case some error packets were lost, because I forgot to set the CPU frequency to max. Will resend the log. ..snip... > > After some more messages there should be also: > > can0 2200 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME > state-change{back-to-error-active} > error-counter-tx-rx{{95}{0}} > > For each message sent, the error counter decreases by 8. > The HI-3110 controller decrements the error counter by 1 for every message sent. The error count increments by 8 when there is an error. > >> >> root@imx6qrom5420b1:~# ip -s -d link show can0 >> 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN >> mode DEFAULT group default qlen 10 >> link/can promiscuity 0 >> can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 >> bitrate 100 sample-point 0.750 >> tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 >> hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 >> clock 1600 >> re-started bus-errors arbit-lost error-warn error-pass bus-off >> 0 1 0 0 0 0 > > > Strange, some counters got lost. > This was a bug introduced when adding berr-reporting, have fixed in v4 patch. >> >> I have not been able to check the bus-off condition by (short-circuiting >> CAN low and high). The tec error count remains at 128 when I short the >> CAN low and high pins and the status never goes BUSOFF. > > > You also need to send a message and the short-circuit should be at the > connector of the sending host. What tranceiver is used? Do you know? > ADM3054 transceiver is used with HI-3111. I connected the HI-3111/ADM3054 board to kvaser leaf and ran "cangen -i can0" and "candump -e any,0:0,#FFF" on the board. Removed the cable and shorted the CAN_H/L pins coming out of ADM3054. I will try your suggestion of using a different bit-rate on the Kvaser leaf instead. I appreciate your continued feedback, it has helped significantly improve the error handling of the driver. Looking back I should have based it
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandegger wrote: ...snip >> /disconnect cable >> can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{40}{0}} >> can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{88}{0}} >> can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{128}{0}} > > > TX error warning is missing. > This support was missing in the driver, added in V4 patch. >> can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME >> controller-problem{tx-error-passive} >> protocol-violation{{}{acknowledge-slot}} >> bus-error >> error-counter-tx-rx{{128}{0}} > > > Here "tx-error-passiv" is packed with a bus error. What I'm looking for are > state change messages similar to: > >can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME > controller-problem{tx-error-warning} > state-change{tx-error-warning} > error-counter-tx-rx{{96}{0}} >can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME > controller-problem{tx-error-passive} > state-change{tx-error-passive} > error-counter-tx-rx{{128}{0} > > They should always come, even with "berr-reporting off". > HI-3110 has only 1 bus error interrupt. There is no dedicated state change interrupts like other controllers. So here is my plan: - Have the bus error interrupt always enabled - If berr-reporting off, then have the isr checks/reports state changes - if berr-reporting on, then have the isr checks/reports bus errors and state changes (Does it make sense packing the error message, if the ISR finds both bus and state changes?) >> write: No buffer space available >> root@imx6qrom5420b1:~# ip -s -d link show can0 >> 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN >> mode DEFAULT group default qlen 10 >> link/can promiscuity 0 >> can state ERROR-PASSIVE (berr-counter tx 128 rx 0) >> restart-ms 0 >> bitrate 100 sample-point 0.750 >> tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 >> hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 >> clock 1600 >> re-started bus-errors arbit-lost error-warn error-pass bus-off >> 0 6 0 1 1 0 > > > The error warning and passive counter increased , though. Also the bus error > should come in at a rather hight rate. Looking to the code, maybe > you need to test STATF to check for state changes (and not ERR). > Apologize, just realized In the above case some error packets were lost, because I forgot to set the CPU frequency to max. Will resend the log. ..snip... > > After some more messages there should be also: > > can0 2200 [8] 00 40 00 00 00 00 5F 00 ERRORFRAME > state-change{back-to-error-active} > error-counter-tx-rx{{95}{0}} > > For each message sent, the error counter decreases by 8. > The HI-3110 controller decrements the error counter by 1 for every message sent. The error count increments by 8 when there is an error. > >> >> root@imx6qrom5420b1:~# ip -s -d link show can0 >> 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN >> mode DEFAULT group default qlen 10 >> link/can promiscuity 0 >> can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 >> bitrate 100 sample-point 0.750 >> tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 >> hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 >> clock 1600 >> re-started bus-errors arbit-lost error-warn error-pass bus-off >> 0 1 0 0 0 0 > > > Strange, some counters got lost. > This was a bug introduced when adding berr-reporting, have fixed in v4 patch. >> >> I have not been able to check the bus-off condition by (short-circuiting >> CAN low and high). The tec error count remains at 128 when I short the >> CAN low and high pins and the status never goes BUSOFF. > > > You also need to send a message and the short-circuit should be at the > connector of the sending host. What tranceiver is used? Do you know? > ADM3054 transceiver is used with HI-3111. I connected the HI-3111/ADM3054 board to kvaser leaf and ran "cangen -i can0" and "candump -e any,0:0,#FFF" on the board. Removed the cable and shorted the CAN_H/L pins coming out of ADM3054. I will try your suggestion of using a different bit-rate on the Kvaser leaf instead. I appreciate your continued feedback, it has helped significantly improve the error handling of the driver. Looking back I should have based it on sja1000 or flexcan driver. Thanks, Akshay
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Am 14.03.2017 um 19:08 schrieb Wolfgang Grandegger: Hello Akshay, Am 14.03.2017 um 17:20 schrieb Akshay Bhat: Hi Wolfgang, On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote: ... snip ... A few other things to check: Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". Then 1) disconnect the cable or 2) short-circuit CAN low and high at the connector. You should see error messages. After reconnection or removing the short-circuit (and bus-off recovery) the state should go back to "active". With the above sequence, candump reports "ERRORFRAME" with protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting the cable the can state goes back to ACTIVE and I see the messages that were in the queue being sent. Do you get the ACK error also with berr-reporting off? Would be nice if you could show a candump log here. Below is a log for disconnecting and re-connecting CAN cable scenario: (Note this is on a 4.1.18 kernel with RT patch) root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting on root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & Please add "-td" ... [1] 768 root@imx6qrom5420b1:~# cangen can0 and "-i" here. can0 21C [8] 35 98 C0 7A 95 03 E6 2A can0 6E6 [1] F2 can0 5C7 [2] 42 50 can0 57C [8] 83 7A E4 0C 03 8B 90 45 can0 55C [8] B9 74 87 52 D8 F4 64 04 can0 014 [8] 28 CB 96 57 3B 80 67 4F can0 6AF [1] 35 can0 51E [8] B6 C8 6C 1D 3A 87 ED 2E can0 527 [8] D0 8A D3 59 0E 34 40 78 can0 30C [2] 6A 12 can0 145 [8] CB 6E FF 55 C1 BE C3 22 can0 5A5 [8] C4 49 54 68 02 63 F9 35 can0 0BA [8] DA 57 5E 3A CE 88 20 1C can0 516 [2] 09 09 can0 743 [8] 7C 4D 25 47 61 4C 56 3D can0 31D [2] 9C D3 can0 71E [8] 53 7C 97 2A 2A F2 9F 56 can0 52E [8] FE DA 2D 51 73 96 DF 79 /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0:mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 The error warning and passive counter increased , though. Also the bus error should come in at a rather hight rate. Looking to the code, maybe you need to test STATF to check for state changes (and not ERR). Likely the ERR bits are only valid if the BUSERR bit in INTF is set. RX: bytes packets errors dropped overrun mcast 0 06 0 0 0 TX: bytes packets errors dropped carrier collsns 10618 0 0 0 0 root@imx6qrom5420b1:~# /re-connect cable can0 169 [8] 35 55 A3 1C 0F 47 2E 5B can0 318 [8] 11 AA 27 11 D2 1B CE 34 can0 577 [8] A0 A4 EE 50 8D A2 E1 3E can0 4ED [8] 52 96 17 7E 31 FC 7D 7C can0 2E7 [8] 92 48 D4 39 05 1E 9F 50 can0 200 [8] 4A 66 F6 02 1E 71 8E 26 can0 29A [8] 49 63 2E 7D C9 77 85 7A can0 15A [7] 3C 0E 65 74 C3 62 80 can0 011 [1] D2 can0 26B [3] FC D6 68 can0 5CE [8] 6F 02 B5 14 BC 7A D7 02 root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Am 14.03.2017 um 19:08 schrieb Wolfgang Grandegger: Hello Akshay, Am 14.03.2017 um 17:20 schrieb Akshay Bhat: Hi Wolfgang, On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote: ... snip ... A few other things to check: Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". Then 1) disconnect the cable or 2) short-circuit CAN low and high at the connector. You should see error messages. After reconnection or removing the short-circuit (and bus-off recovery) the state should go back to "active". With the above sequence, candump reports "ERRORFRAME" with protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting the cable the can state goes back to ACTIVE and I see the messages that were in the queue being sent. Do you get the ACK error also with berr-reporting off? Would be nice if you could show a candump log here. Below is a log for disconnecting and re-connecting CAN cable scenario: (Note this is on a 4.1.18 kernel with RT patch) root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting on root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & Please add "-td" ... [1] 768 root@imx6qrom5420b1:~# cangen can0 and "-i" here. can0 21C [8] 35 98 C0 7A 95 03 E6 2A can0 6E6 [1] F2 can0 5C7 [2] 42 50 can0 57C [8] 83 7A E4 0C 03 8B 90 45 can0 55C [8] B9 74 87 52 D8 F4 64 04 can0 014 [8] 28 CB 96 57 3B 80 67 4F can0 6AF [1] 35 can0 51E [8] B6 C8 6C 1D 3A 87 ED 2E can0 527 [8] D0 8A D3 59 0E 34 40 78 can0 30C [2] 6A 12 can0 145 [8] CB 6E FF 55 C1 BE C3 22 can0 5A5 [8] C4 49 54 68 02 63 F9 35 can0 0BA [8] DA 57 5E 3A CE 88 20 1C can0 516 [2] 09 09 can0 743 [8] 7C 4D 25 47 61 4C 56 3D can0 31D [2] 9C D3 can0 71E [8] 53 7C 97 2A 2A F2 9F 56 can0 52E [8] FE DA 2D 51 73 96 DF 79 /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 The error warning and passive counter increased , though. Also the bus error should come in at a rather hight rate. Looking to the code, maybe you need to test STATF to check for state changes (and not ERR). Likely the ERR bits are only valid if the BUSERR bit in INTF is set. RX: bytes packets errors dropped overrun mcast 0 06 0 0 0 TX: bytes packets errors dropped carrier collsns 10618 0 0 0 0 root@imx6qrom5420b1:~# /re-connect cable can0 169 [8] 35 55 A3 1C 0F 47 2E 5B can0 318 [8] 11 AA 27 11 D2 1B CE 34 can0 577 [8] A0 A4 EE 50 8D A2 E1 3E can0 4ED [8] 52 96 17 7E 31 FC 7D 7C can0 2E7 [8] 92 48 D4 39 05 1E 9F 50 can0 200 [8] 4A 66 F6 02 1E 71 8E 26 can0 29A [8] 49 63 2E 7D C9 77 85 7A can0 15A [7] 3C 0E 65 74 C3 62 80 can0 011 [1] D2 can0 26B [3] FC D6 68 can0 5CE [8] 6F 02 B5 14 BC 7A D7 02 root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 14.03.2017 um 17:20 schrieb Akshay Bhat: Hi Wolfgang, On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote: ... snip ... A few other things to check: Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". Then 1) disconnect the cable or 2) short-circuit CAN low and high at the connector. You should see error messages. After reconnection or removing the short-circuit (and bus-off recovery) the state should go back to "active". With the above sequence, candump reports "ERRORFRAME" with protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting the cable the can state goes back to ACTIVE and I see the messages that were in the queue being sent. Do you get the ACK error also with berr-reporting off? Would be nice if you could show a candump log here. Below is a log for disconnecting and re-connecting CAN cable scenario: (Note this is on a 4.1.18 kernel with RT patch) root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting on root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & Please add "-td" ... [1] 768 root@imx6qrom5420b1:~# cangen can0 and "-i" here. can0 21C [8] 35 98 C0 7A 95 03 E6 2A can0 6E6 [1] F2 can0 5C7 [2] 42 50 can0 57C [8] 83 7A E4 0C 03 8B 90 45 can0 55C [8] B9 74 87 52 D8 F4 64 04 can0 014 [8] 28 CB 96 57 3B 80 67 4F can0 6AF [1] 35 can0 51E [8] B6 C8 6C 1D 3A 87 ED 2E can0 527 [8] D0 8A D3 59 0E 34 40 78 can0 30C [2] 6A 12 can0 145 [8] CB 6E FF 55 C1 BE C3 22 can0 5A5 [8] C4 49 54 68 02 63 F9 35 can0 0BA [8] DA 57 5E 3A CE 88 20 1C can0 516 [2] 09 09 can0 743 [8] 7C 4D 25 47 61 4C 56 3D can0 31D [2] 9C D3 can0 71E [8] 53 7C 97 2A 2A F2 9F 56 can0 52E [8] FE DA 2D 51 73 96 DF 79 /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0:mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 The error warning and passive counter increased , though. Also the bus error should come in at a rather hight rate. Looking to the code, maybe you need to test STATF to check for state changes (and not ERR). RX: bytes packets errors dropped overrun mcast 0 06 0 0 0 TX: bytes packets errors dropped carrier collsns 10618 0 0 0 0 root@imx6qrom5420b1:~# /re-connect cable can0 169 [8] 35 55 A3 1C 0F 47 2E 5B can0 318 [8] 11 AA 27 11 D2 1B CE 34 can0 577 [8] A0 A4 EE 50 8D A2 E1 3E can0 4ED [8] 52 96 17 7E 31 FC 7D 7C can0 2E7 [8] 92 48 D4 39 05 1E 9F 50 can0 200 [8] 4A 66 F6 02 1E 71 8E 26 can0 29A [8] 49 63 2E 7D C9 77 85 7A can0 15A [7] 3C 0E 65 74 C3 62 80 can0 011 [1] D2 can0 26B [3] FC D6 68 can0 5CE [8] 6F 02 B5 14 BC 7A D7 02 root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 14.03.2017 um 17:20 schrieb Akshay Bhat: Hi Wolfgang, On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote: ... snip ... A few other things to check: Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". Then 1) disconnect the cable or 2) short-circuit CAN low and high at the connector. You should see error messages. After reconnection or removing the short-circuit (and bus-off recovery) the state should go back to "active". With the above sequence, candump reports "ERRORFRAME" with protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting the cable the can state goes back to ACTIVE and I see the messages that were in the queue being sent. Do you get the ACK error also with berr-reporting off? Would be nice if you could show a candump log here. Below is a log for disconnecting and re-connecting CAN cable scenario: (Note this is on a 4.1.18 kernel with RT patch) root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting on root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & Please add "-td" ... [1] 768 root@imx6qrom5420b1:~# cangen can0 and "-i" here. can0 21C [8] 35 98 C0 7A 95 03 E6 2A can0 6E6 [1] F2 can0 5C7 [2] 42 50 can0 57C [8] 83 7A E4 0C 03 8B 90 45 can0 55C [8] B9 74 87 52 D8 F4 64 04 can0 014 [8] 28 CB 96 57 3B 80 67 4F can0 6AF [1] 35 can0 51E [8] B6 C8 6C 1D 3A 87 ED 2E can0 527 [8] D0 8A D3 59 0E 34 40 78 can0 30C [2] 6A 12 can0 145 [8] CB 6E FF 55 C1 BE C3 22 can0 5A5 [8] C4 49 54 68 02 63 F9 35 can0 0BA [8] DA 57 5E 3A CE 88 20 1C can0 516 [2] 09 09 can0 743 [8] 7C 4D 25 47 61 4C 56 3D can0 31D [2] 9C D3 can0 71E [8] 53 7C 97 2A 2A F2 9F 56 can0 52E [8] FE DA 2D 51 73 96 DF 79 /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} TX error warning is missing. can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} Here "tx-error-passiv" is packed with a bus error. What I'm looking for are state change messages similar to: can0 2204 [8] 00 08 00 00 00 00 60 00 ERRORFRAME controller-problem{tx-error-warning} state-change{tx-error-warning} error-counter-tx-rx{{96}{0}} can0 2204 [8] 00 30 00 00 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} state-change{tx-error-passive} error-counter-tx-rx{{128}{0} They should always come, even with "berr-reporting off". write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 The error warning and passive counter increased , though. Also the bus error should come in at a rather hight rate. Looking to the code, maybe you need to test STATF to check for state changes (and not ERR). RX: bytes packets errors dropped overrun mcast 0 06 0 0 0 TX: bytes packets errors dropped carrier collsns 10618 0 0 0 0 root@imx6qrom5420b1:~# /re-connect cable can0 169 [8] 35 55 A3 1C 0F 47 2E 5B can0 318 [8] 11 AA 27 11 D2 1B CE 34 can0 577 [8] A0 A4 EE 50 8D A2 E1 3E can0 4ED [8] 52 96 17 7E 31 FC 7D 7C can0 2E7 [8] 92 48 D4 39 05 1E 9F 50 can0 200 [8] 4A 66 F6 02 1E 71 8E 26 can0 29A [8] 49 63 2E 7D C9 77 85 7A can0 15A [7] 3C 0E 65 74 C3 62 80 can0 011 [1] D2 can0 26B [3] FC D6 68 can0 5CE [8] 6F 02 B5 14 BC 7A D7 02 root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote: > ... snip ... >>> A few other things to check: >>> >>> Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". >>> Then 1) disconnect the cable or 2) short-circuit CAN low and high at the >>> connector. You should see error messages. After reconnection or removing >>> the short-circuit (and bus-off recovery) the state should go back to >>> "active". >>> >> >> With the above sequence, candump reports "ERRORFRAME" with >> protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting >> the cable the can state goes back to ACTIVE and I see the messages that >> were in the queue being sent. > > Do you get the ACK error also with berr-reporting off? Would be nice if > you could show a candump log here. > Below is a log for disconnecting and re-connecting CAN cable scenario: (Note this is on a 4.1.18 kernel with RT patch) root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting on root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & [1] 768 root@imx6qrom5420b1:~# cangen can0 can0 21C [8] 35 98 C0 7A 95 03 E6 2A can0 6E6 [1] F2 can0 5C7 [2] 42 50 can0 57C [8] 83 7A E4 0C 03 8B 90 45 can0 55C [8] B9 74 87 52 D8 F4 64 04 can0 014 [8] 28 CB 96 57 3B 80 67 4F can0 6AF [1] 35 can0 51E [8] B6 C8 6C 1D 3A 87 ED 2E can0 527 [8] D0 8A D3 59 0E 34 40 78 can0 30C [2] 6A 12 can0 145 [8] CB 6E FF 55 C1 BE C3 22 can0 5A5 [8] C4 49 54 68 02 63 F9 35 can0 0BA [8] DA 57 5E 3A CE 88 20 1C can0 516 [2] 09 09 can0 743 [8] 7C 4D 25 47 61 4C 56 3D can0 31D [2] 9C D3 can0 71E [8] 53 7C 97 2A 2A F2 9F 56 can0 52E [8] FE DA 2D 51 73 96 DF 79 /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0:mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 RX: bytes packets errors dropped overrun mcast 0 06 0 0 0 TX: bytes packets errors dropped carrier collsns 10618 0 0 0 0 root@imx6qrom5420b1:~# /re-connect cable can0 169 [8] 35 55 A3 1C 0F 47 2E 5B can0 318 [8] 11 AA 27 11 D2 1B CE 34 can0 577 [8] A0 A4 EE 50 8D A2 E1 3E can0 4ED [8] 52 96 17 7E 31 FC 7D 7C can0 2E7 [8] 92 48 D4 39 05 1E 9F 50 can0 200 [8] 4A 66 F6 02 1E 71 8E 26 can0 29A [8] 49 63 2E 7D C9 77 85 7A can0 15A [7] 3C 0E 65 74 C3 62 80 can0 011 [1] D2 can0 26B [3] FC D6 68 can0 5CE [8] 6F 02 B5 14 BC 7A D7 02 root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 7 0 1 1 0 RX: bytes packets errors dropped overrun mcast 0 07 0 0 0 TX: bytes packets errors dropped carrier collsns 18129 0 0 0 0 //Reboot the board and test with bus error reporting off root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting off root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & [1] 782 root@imx6qrom5420b1:~# cangen can0 can0 1FA [3] C9 FE C2 can0 3E2 [5] 85 37 03 5B 6F can0 289 [8] A4 F6 BF 4A 3F 70 65 1B can0 12D [8] B2 72 10 33 AB B4 68 64
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote: > ... snip ... >>> A few other things to check: >>> >>> Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". >>> Then 1) disconnect the cable or 2) short-circuit CAN low and high at the >>> connector. You should see error messages. After reconnection or removing >>> the short-circuit (and bus-off recovery) the state should go back to >>> "active". >>> >> >> With the above sequence, candump reports "ERRORFRAME" with >> protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting >> the cable the can state goes back to ACTIVE and I see the messages that >> were in the queue being sent. > > Do you get the ACK error also with berr-reporting off? Would be nice if > you could show a candump log here. > Below is a log for disconnecting and re-connecting CAN cable scenario: (Note this is on a 4.1.18 kernel with RT patch) root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting on root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & [1] 768 root@imx6qrom5420b1:~# cangen can0 can0 21C [8] 35 98 C0 7A 95 03 E6 2A can0 6E6 [1] F2 can0 5C7 [2] 42 50 can0 57C [8] 83 7A E4 0C 03 8B 90 45 can0 55C [8] B9 74 87 52 D8 F4 64 04 can0 014 [8] 28 CB 96 57 3B 80 67 4F can0 6AF [1] 35 can0 51E [8] B6 C8 6C 1D 3A 87 ED 2E can0 527 [8] D0 8A D3 59 0E 34 40 78 can0 30C [2] 6A 12 can0 145 [8] CB 6E FF 55 C1 BE C3 22 can0 5A5 [8] C4 49 54 68 02 63 F9 35 can0 0BA [8] DA 57 5E 3A CE 88 20 1C can0 516 [2] 09 09 can0 743 [8] 7C 4D 25 47 61 4C 56 3D can0 31D [2] 9C D3 can0 71E [8] 53 7C 97 2A 2A F2 9F 56 can0 52E [8] FE DA 2D 51 73 96 DF 79 /disconnect cable can0 2088 [8] 00 00 00 19 00 00 28 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{40}{0}} can0 2088 [8] 00 00 00 19 00 00 58 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{88}{0}} can0 2088 [8] 00 00 00 19 00 00 80 00 ERRORFRAME protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} can0 208C [8] 00 20 00 19 00 00 80 00 ERRORFRAME controller-problem{tx-error-passive} protocol-violation{{}{acknowledge-slot}} bus-error error-counter-tx-rx{{128}{0}} write: No buffer space available root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 6 0 1 1 0 RX: bytes packets errors dropped overrun mcast 0 06 0 0 0 TX: bytes packets errors dropped carrier collsns 10618 0 0 0 0 root@imx6qrom5420b1:~# /re-connect cable can0 169 [8] 35 55 A3 1C 0F 47 2E 5B can0 318 [8] 11 AA 27 11 D2 1B CE 34 can0 577 [8] A0 A4 EE 50 8D A2 E1 3E can0 4ED [8] 52 96 17 7E 31 FC 7D 7C can0 2E7 [8] 92 48 D4 39 05 1E 9F 50 can0 200 [8] 4A 66 F6 02 1E 71 8E 26 can0 29A [8] 49 63 2E 7D C9 77 85 7A can0 15A [7] 3C 0E 65 74 C3 62 80 can0 011 [1] D2 can0 26B [3] FC D6 68 can0 5CE [8] 6F 02 B5 14 BC 7A D7 02 root@imx6qrom5420b1:~# ip -s -d link show can0 4: can0: mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT group default qlen 10 link/can promiscuity 0 can state ERROR-ACTIVE (berr-counter tx 117 rx 0) restart-ms 0 bitrate 100 sample-point 0.750 tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1 hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1 clock 1600 re-started bus-errors arbit-lost error-warn error-pass bus-off 0 7 0 1 1 0 RX: bytes packets errors dropped overrun mcast 0 07 0 0 0 TX: bytes packets errors dropped carrier collsns 18129 0 0 0 0 //Reboot the board and test with bus error reporting off root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100 berr-reporting off root@imx6qrom5420b1:~# candump -e any,0:0,#FFF & [1] 782 root@imx6qrom5420b1:~# cangen can0 can0 1FA [3] C9 FE C2 can0 3E2 [5] 85 37 03 5B 6F can0 289 [8] A4 F6 BF 4A 3F 70 65 1B can0 12D [8] B2 72 10 33 AB B4 68 64 can0 054 [2] 01 D7 can0 4A6 [8] 29
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hallo Akshay, Am 13.03.2017 um 16:38 schrieb Akshay Bhat: Hi Wolfgang, On 03/09/2017 12:36 PM, Wolfgang Grandegger wrote: Hello, doing a quick review... I realized a few issues... Am 17.01.2017 um 20:22 schrieb Akshay Bhat: ... snip ... A few other things to check: Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". Then 1) disconnect the cable or 2) short-circuit CAN low and high at the connector. You should see error messages. After reconnection or removing the short-circuit (and bus-off recovery) the state should go back to "active". With the above sequence, candump reports "ERRORFRAME" with protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting the cable the can state goes back to ACTIVE and I see the messages that were in the queue being sent. Do you get the ACK error also with berr-reporting off? Would be nice if you could show a candump log here. Also, any error message should show the bus error counts in data[7,8]: http://lxr.free-electrons.com/source/drivers/net/can/sja1000/sja1000.c#L408 And please check bus-off as well (short-circuiting CAN low and high). Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hallo Akshay, Am 13.03.2017 um 16:38 schrieb Akshay Bhat: Hi Wolfgang, On 03/09/2017 12:36 PM, Wolfgang Grandegger wrote: Hello, doing a quick review... I realized a few issues... Am 17.01.2017 um 20:22 schrieb Akshay Bhat: ... snip ... A few other things to check: Run "cangen" and monitor the message with "candump -e any,0:0,#FFF". Then 1) disconnect the cable or 2) short-circuit CAN low and high at the connector. You should see error messages. After reconnection or removing the short-circuit (and bus-off recovery) the state should go back to "active". With the above sequence, candump reports "ERRORFRAME" with protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting the cable the can state goes back to ACTIVE and I see the messages that were in the queue being sent. Do you get the ACK error also with berr-reporting off? Would be nice if you could show a candump log here. Also, any error message should show the bus error counts in data[7,8]: http://lxr.free-electrons.com/source/drivers/net/can/sja1000/sja1000.c#L408 And please check bus-off as well (short-circuiting CAN low and high). Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/09/2017 12:36 PM, Wolfgang Grandegger wrote: > Hello, > > doing a quick review... I realized a few issues... > > Am 17.01.2017 um 20:22 schrieb Akshay Bhat: >> +static u8 hi3110_read(struct spi_device *spi, u8 command) >> +{ >> +struct hi3110_priv *priv = spi_get_drvdata(spi); >> +u8 val = 0; >> + >> +priv->spi_tx_buf[0] = command; >> +hi3110_spi_trans(spi, 2); >> +val = priv->spi_rx_buf[1]; >> +dev_dbg(>dev, "hi3110_read: %02X, %02X\n", command, val); > > This produces a lot of output which is not useful for the normal user. > Fixed in v3 patch. >> +static void hi3110_write(struct spi_device *spi, u8 reg, u8 val) >> +{ >> +struct hi3110_priv *priv = spi_get_drvdata(spi); >> + >> +priv->spi_tx_buf[0] = reg; >> +priv->spi_tx_buf[1] = val; >> +dev_dbg(>dev, "hi3110_write: %02X, %02X\n", reg, val); > > Ditto. > Fixed in v3 patch. >> + >> +static void hi3110_hw_rx(struct spi_device *spi) >> +{ >> +struct hi3110_priv *priv = spi_get_drvdata(spi); >> +struct sk_buff *skb; >> +struct can_frame *frame; >> +u8 buf[HI3110_RX_BUF_LEN - 1]; >> + >> +skb = alloc_can_skb(priv->net, ); >> +if (!skb) { >> +dev_err(>dev, "cannot allocate RX skb\n"); > > Please return silenty! Otherwise it will make the situation worse. > Fixed in v3 patch. >> + >> +/* Data length */ >> +frame->can_dlc = get_can_dlc(buf[HI3110_FIFO_WOTIME_DLC_OFF] & >> 0x0F); >> +memcpy(frame->data, buf + HI3110_FIFO_WOTIME_DAT_OFF, >> frame->can_dlc); > > No data bytes should not be copied for RTR messages. > Fixed in v3 patch. >> + >> +if (priv->tx_skb || priv->tx_len) { >> +dev_warn(>dev, "hard_xmit called while tx busy\n"); > > s/warn/err/? This should never happen; IIUC. > Fixed in v3 patch. >> +return NETDEV_TX_BUSY; >> +} >> + >> +if (can_dropped_invalid_skb(net, skb)) >> +return NETDEV_TX_OK; >> + >> +netif_stop_queue(net); > > The driver transfers the packets in sequence. Any chance to queue them? > At least there is a TX FIFO for 8 messages. That's bad for RT but would > increase the throughput. > I initially did not use the TX FIFO for the reason you mentioned above. Queuing should be possible but since it requires lot more additional logic, I can work on it a later time. >> + >> +if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { >> +/* Put device into loopback mode */ >> +hi3110_write(spi, HI3110_WRITE_CTRL0, >> + HI3110_CTRL0_LOOPBACK_MODE); >> +} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) { >> +/* Put device into listen-only mode */ >> +hi3110_write(spi, HI3110_WRITE_CTRL0, >> + HI3110_CTRL0_MONITOR_MODE); >> +} else { >> +/* Put device into normal mode */ >> +hi3110_write(spi, HI3110_WRITE_CTRL0, >> + HI3110_CTRL0_NORMAL_MODE); > > "mode = x" and just one write is more compact. > Fixed in v3 patch. >> + >> +/* Wait for the device to enter normal mode */ >> +mdelay(HI3110_OST_DELAY_MS); >> +reg = hi3110_read(spi, HI3110_READ_CTRL0); >> +if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_NORMAL_MODE) >> +return -EBUSY; > > Is this not necesary for listen or loopbcak only mode? > It is necessary, fixed in v3 patch. >> + >> +static void hi3110_error_skb(struct net_device *net, int can_id, >> + int data1, int data2) >> +{ >> +struct sk_buff *skb; >> +struct can_frame *frame; >> + >> +skb = alloc_can_err_skb(net, ); >> +if (skb) { >> +frame->can_id |= can_id; >> +frame->data[1] = data1; >> +frame->data[2] = data2; >> +netif_rx_ni(skb); >> +} else { >> +netdev_err(net, "cannot allocate error skb\n"); > > Please remove the error message. Not a good at low memory situations. > Fixed in v3 patch. >> +/* Check for protocol errors */ >> +if (eflag & HI3110_ERR_PROTOCOL_MASK) { >> +can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; >> +priv->can.can_stats.bus_error++; >> +priv->net->stats.rx_errors++; >> +if (eflag & HI3110_ERR_BITERR) >> +data2 |= CAN_ERR_PROT_BIT; >> +else if (eflag & HI3110_ERR_FRMERR) >> +data2 |= CAN_ERR_PROT_FORM; >> +else if (eflag & HI3110_ERR_STUFERR) >> +data2 |= CAN_ERR_PROT_STUFF; >> +else >> +data2 |= CAN_ERR_PROT_UNSPEC; > > And what about the ACK and CRC error defines at the beginning? > It's also comon to use netdev_dbg() on error interrupts. > Good catch, I missed it. Fixed in v3 patch. >> +} > > Bus error reporting can flood the system with interrupts. Any chance to > implement CAN_CTRLMODE_BERR_REPORTING. I think the bus error interrupt > can be enabled/disabled. > Thanks, was not aware of this feature. Added it in v3 patch. >> +
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/09/2017 12:36 PM, Wolfgang Grandegger wrote: > Hello, > > doing a quick review... I realized a few issues... > > Am 17.01.2017 um 20:22 schrieb Akshay Bhat: >> +static u8 hi3110_read(struct spi_device *spi, u8 command) >> +{ >> +struct hi3110_priv *priv = spi_get_drvdata(spi); >> +u8 val = 0; >> + >> +priv->spi_tx_buf[0] = command; >> +hi3110_spi_trans(spi, 2); >> +val = priv->spi_rx_buf[1]; >> +dev_dbg(>dev, "hi3110_read: %02X, %02X\n", command, val); > > This produces a lot of output which is not useful for the normal user. > Fixed in v3 patch. >> +static void hi3110_write(struct spi_device *spi, u8 reg, u8 val) >> +{ >> +struct hi3110_priv *priv = spi_get_drvdata(spi); >> + >> +priv->spi_tx_buf[0] = reg; >> +priv->spi_tx_buf[1] = val; >> +dev_dbg(>dev, "hi3110_write: %02X, %02X\n", reg, val); > > Ditto. > Fixed in v3 patch. >> + >> +static void hi3110_hw_rx(struct spi_device *spi) >> +{ >> +struct hi3110_priv *priv = spi_get_drvdata(spi); >> +struct sk_buff *skb; >> +struct can_frame *frame; >> +u8 buf[HI3110_RX_BUF_LEN - 1]; >> + >> +skb = alloc_can_skb(priv->net, ); >> +if (!skb) { >> +dev_err(>dev, "cannot allocate RX skb\n"); > > Please return silenty! Otherwise it will make the situation worse. > Fixed in v3 patch. >> + >> +/* Data length */ >> +frame->can_dlc = get_can_dlc(buf[HI3110_FIFO_WOTIME_DLC_OFF] & >> 0x0F); >> +memcpy(frame->data, buf + HI3110_FIFO_WOTIME_DAT_OFF, >> frame->can_dlc); > > No data bytes should not be copied for RTR messages. > Fixed in v3 patch. >> + >> +if (priv->tx_skb || priv->tx_len) { >> +dev_warn(>dev, "hard_xmit called while tx busy\n"); > > s/warn/err/? This should never happen; IIUC. > Fixed in v3 patch. >> +return NETDEV_TX_BUSY; >> +} >> + >> +if (can_dropped_invalid_skb(net, skb)) >> +return NETDEV_TX_OK; >> + >> +netif_stop_queue(net); > > The driver transfers the packets in sequence. Any chance to queue them? > At least there is a TX FIFO for 8 messages. That's bad for RT but would > increase the throughput. > I initially did not use the TX FIFO for the reason you mentioned above. Queuing should be possible but since it requires lot more additional logic, I can work on it a later time. >> + >> +if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { >> +/* Put device into loopback mode */ >> +hi3110_write(spi, HI3110_WRITE_CTRL0, >> + HI3110_CTRL0_LOOPBACK_MODE); >> +} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) { >> +/* Put device into listen-only mode */ >> +hi3110_write(spi, HI3110_WRITE_CTRL0, >> + HI3110_CTRL0_MONITOR_MODE); >> +} else { >> +/* Put device into normal mode */ >> +hi3110_write(spi, HI3110_WRITE_CTRL0, >> + HI3110_CTRL0_NORMAL_MODE); > > "mode = x" and just one write is more compact. > Fixed in v3 patch. >> + >> +/* Wait for the device to enter normal mode */ >> +mdelay(HI3110_OST_DELAY_MS); >> +reg = hi3110_read(spi, HI3110_READ_CTRL0); >> +if ((reg & HI3110_CTRL0_MODE_MASK) != HI3110_CTRL0_NORMAL_MODE) >> +return -EBUSY; > > Is this not necesary for listen or loopbcak only mode? > It is necessary, fixed in v3 patch. >> + >> +static void hi3110_error_skb(struct net_device *net, int can_id, >> + int data1, int data2) >> +{ >> +struct sk_buff *skb; >> +struct can_frame *frame; >> + >> +skb = alloc_can_err_skb(net, ); >> +if (skb) { >> +frame->can_id |= can_id; >> +frame->data[1] = data1; >> +frame->data[2] = data2; >> +netif_rx_ni(skb); >> +} else { >> +netdev_err(net, "cannot allocate error skb\n"); > > Please remove the error message. Not a good at low memory situations. > Fixed in v3 patch. >> +/* Check for protocol errors */ >> +if (eflag & HI3110_ERR_PROTOCOL_MASK) { >> +can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; >> +priv->can.can_stats.bus_error++; >> +priv->net->stats.rx_errors++; >> +if (eflag & HI3110_ERR_BITERR) >> +data2 |= CAN_ERR_PROT_BIT; >> +else if (eflag & HI3110_ERR_FRMERR) >> +data2 |= CAN_ERR_PROT_FORM; >> +else if (eflag & HI3110_ERR_STUFERR) >> +data2 |= CAN_ERR_PROT_STUFF; >> +else >> +data2 |= CAN_ERR_PROT_UNSPEC; > > And what about the ACK and CRC error defines at the beginning? > It's also comon to use netdev_dbg() on error interrupts. > Good catch, I missed it. Fixed in v3 patch. >> +} > > Bus error reporting can flood the system with interrupts. Any chance to > implement CAN_CTRLMODE_BERR_REPORTING. I think the bus error interrupt > can be enabled/disabled. > Thanks, was not aware of this feature. Added it in v3 patch. >> +
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello, doing a quick review... I realized a few issues... Am 17.01.2017 um 20:22 schrieb Akshay Bhat: This patch adds support for the Holt HI-311x CAN controller. The HI311x CAN controller is capable of transmitting and receiving standard data frames, extended data frames and remote frames. The HI311x interfaces with the host over SPI. Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do Signed-off-by: Akshay Bhat--- v1 -> v2: Address comments from Marc Kleine-Budde: - use u8 instead of uint8_t - alphabetically sort Makefile and Kconfig - copy over copyright information from mcp251x - use single space after each marco - add missing HI3110_ namespace in defines - remove magic number for IDE & SRR bits - simplify logic to populate extended CAN ID - remove unused parameters in hi3110_setup function - remove redundant frame->can_dlc length check - simplify error handling in hi3110_open function Address comments from Julia Lawall: - remove unnecessary semicolon after while loop in hi3110_can_ist drivers/net/can/spi/Kconfig |6 + drivers/net/can/spi/Makefile |1 + drivers/net/can/spi/hi311x.c | 1069 ++ 3 files changed, 1076 insertions(+) create mode 100644 drivers/net/can/spi/hi311x.c diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig index 148cae5..8f2e0dd 100644 --- a/drivers/net/can/spi/Kconfig +++ b/drivers/net/can/spi/Kconfig @@ -1,6 +1,12 @@ menu "CAN SPI interfaces" depends on SPI +config CAN_HI311X + tristate "Holt HI311x SPI CAN controllers" + depends on CAN_DEV && SPI && HAS_DMA + ---help--- + Driver for the Holt HI311x SPI CAN controllers. + config CAN_MCP251X tristate "Microchip MCP251x SPI CAN controllers" depends on HAS_DMA diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile index 0e86040..f59fa37 100644 --- a/drivers/net/can/spi/Makefile +++ b/drivers/net/can/spi/Makefile @@ -3,4 +3,5 @@ # +obj-$(CONFIG_CAN_HI311X) += hi311x.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c new file mode 100644 index 000..cccfe2d --- /dev/null +++ b/drivers/net/can/spi/hi311x.c @@ -0,0 +1,1069 @@ +/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface + * + * Copyright(C) Timesys Corporation 2016 + * + * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver + * Copyright 2009 Christian Pellegrin EVOL S.r.l. + * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved. + * Copyright 2006 Arcom Control Systems Ltd. + * + * Based on CAN bus driver for the CCAN controller written by + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix + * - Simon Kallweit, intefo AG + * Copyright 2007 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HI3110_MASTER_RESET 0x56 +#define HI3110_READ_CTRL0 0xD2 +#define HI3110_READ_CTRL1 0xD4 +#define HI3110_READ_STATF 0xE2 +#define HI3110_WRITE_CTRL0 0x14 +#define HI3110_WRITE_CTRL1 0x16 +#define HI3110_WRITE_INTE 0x1C +#define HI3110_WRITE_BTR0 0x18 +#define HI3110_WRITE_BTR1 0x1A +#define HI3110_READ_BTR0 0xD6 +#define HI3110_READ_BTR1 0xD8 +#define HI3110_READ_INTF 0xDE +#define HI3110_READ_ERR 0xDC +#define HI3110_READ_FIFO_WOTIME 0x48 +#define HI3110_WRITE_FIFO 0x12 +#define HI3110_READ_MESSTAT 0xDA +#define HI3110_READ_TEC 0xEC + +#define HI3110_CTRL0_MODE_MASK (7 << 5) +#define HI3110_CTRL0_NORMAL_MODE (0 << 5) +#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5) +#define HI3110_CTRL0_MONITOR_MODE (2 << 5) +#define HI3110_CTRL0_SLEEP_MODE (3 << 5) +#define HI3110_CTRL0_INIT_MODE (4 << 5) + +#define HI3110_CTRL1_TXEN BIT(7) + +#define HI3110_INT_RXTMP BIT(7) +#define HI3110_INT_RXFIFO BIT(6) +#define HI3110_INT_TXCPLT BIT(5) +#define HI3110_INT_BUSERR BIT(4) +#define HI3110_INT_MCHG BIT(3) +#define HI3110_INT_WAKEUP BIT(2) +#define HI3110_INT_F1MESS BIT(1) +#define HI3110_INT_F0MESS BIT(0) + +#define HI3110_ERR_BUSOFF BIT(7) +#define HI3110_ERR_TXERRP BIT(6) +#define HI3110_ERR_RXERRP BIT(5) +#define HI3110_ERR_BITERR BIT(4) +#define HI3110_ERR_FRMERR BIT(3) +#define HI3110_ERR_CRCERR BIT(2) +#define HI3110_ERR_ACKERR BIT(1) +#define HI3110_ERR_STUFERR BIT(0) +#define HI3110_ERR_PROTOCOL_MASK (0x1F) + +#define HI3110_STAT_RXFMTY BIT(1) + +#define HI3110_BTR0_SJW_SHIFT 6 +#define HI3110_BTR0_BRP_SHIFT 0 + +#define HI3110_BTR1_SAMP_3PERBIT (1 << 7) +#define HI3110_BTR1_SAMP_1PERBIT (0 << 7) +#define HI3110_BTR1_TSEG2_SHIFT 4 +#define HI3110_BTR1_TSEG1_SHIFT 0 + +#define HI3110_FIFO_WOTIME_TAG_OFF 0 +#define
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello, doing a quick review... I realized a few issues... Am 17.01.2017 um 20:22 schrieb Akshay Bhat: This patch adds support for the Holt HI-311x CAN controller. The HI311x CAN controller is capable of transmitting and receiving standard data frames, extended data frames and remote frames. The HI311x interfaces with the host over SPI. Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do Signed-off-by: Akshay Bhat --- v1 -> v2: Address comments from Marc Kleine-Budde: - use u8 instead of uint8_t - alphabetically sort Makefile and Kconfig - copy over copyright information from mcp251x - use single space after each marco - add missing HI3110_ namespace in defines - remove magic number for IDE & SRR bits - simplify logic to populate extended CAN ID - remove unused parameters in hi3110_setup function - remove redundant frame->can_dlc length check - simplify error handling in hi3110_open function Address comments from Julia Lawall: - remove unnecessary semicolon after while loop in hi3110_can_ist drivers/net/can/spi/Kconfig |6 + drivers/net/can/spi/Makefile |1 + drivers/net/can/spi/hi311x.c | 1069 ++ 3 files changed, 1076 insertions(+) create mode 100644 drivers/net/can/spi/hi311x.c diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig index 148cae5..8f2e0dd 100644 --- a/drivers/net/can/spi/Kconfig +++ b/drivers/net/can/spi/Kconfig @@ -1,6 +1,12 @@ menu "CAN SPI interfaces" depends on SPI +config CAN_HI311X + tristate "Holt HI311x SPI CAN controllers" + depends on CAN_DEV && SPI && HAS_DMA + ---help--- + Driver for the Holt HI311x SPI CAN controllers. + config CAN_MCP251X tristate "Microchip MCP251x SPI CAN controllers" depends on HAS_DMA diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile index 0e86040..f59fa37 100644 --- a/drivers/net/can/spi/Makefile +++ b/drivers/net/can/spi/Makefile @@ -3,4 +3,5 @@ # +obj-$(CONFIG_CAN_HI311X) += hi311x.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c new file mode 100644 index 000..cccfe2d --- /dev/null +++ b/drivers/net/can/spi/hi311x.c @@ -0,0 +1,1069 @@ +/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface + * + * Copyright(C) Timesys Corporation 2016 + * + * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver + * Copyright 2009 Christian Pellegrin EVOL S.r.l. + * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved. + * Copyright 2006 Arcom Control Systems Ltd. + * + * Based on CAN bus driver for the CCAN controller written by + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix + * - Simon Kallweit, intefo AG + * Copyright 2007 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HI3110_MASTER_RESET 0x56 +#define HI3110_READ_CTRL0 0xD2 +#define HI3110_READ_CTRL1 0xD4 +#define HI3110_READ_STATF 0xE2 +#define HI3110_WRITE_CTRL0 0x14 +#define HI3110_WRITE_CTRL1 0x16 +#define HI3110_WRITE_INTE 0x1C +#define HI3110_WRITE_BTR0 0x18 +#define HI3110_WRITE_BTR1 0x1A +#define HI3110_READ_BTR0 0xD6 +#define HI3110_READ_BTR1 0xD8 +#define HI3110_READ_INTF 0xDE +#define HI3110_READ_ERR 0xDC +#define HI3110_READ_FIFO_WOTIME 0x48 +#define HI3110_WRITE_FIFO 0x12 +#define HI3110_READ_MESSTAT 0xDA +#define HI3110_READ_TEC 0xEC + +#define HI3110_CTRL0_MODE_MASK (7 << 5) +#define HI3110_CTRL0_NORMAL_MODE (0 << 5) +#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5) +#define HI3110_CTRL0_MONITOR_MODE (2 << 5) +#define HI3110_CTRL0_SLEEP_MODE (3 << 5) +#define HI3110_CTRL0_INIT_MODE (4 << 5) + +#define HI3110_CTRL1_TXEN BIT(7) + +#define HI3110_INT_RXTMP BIT(7) +#define HI3110_INT_RXFIFO BIT(6) +#define HI3110_INT_TXCPLT BIT(5) +#define HI3110_INT_BUSERR BIT(4) +#define HI3110_INT_MCHG BIT(3) +#define HI3110_INT_WAKEUP BIT(2) +#define HI3110_INT_F1MESS BIT(1) +#define HI3110_INT_F0MESS BIT(0) + +#define HI3110_ERR_BUSOFF BIT(7) +#define HI3110_ERR_TXERRP BIT(6) +#define HI3110_ERR_RXERRP BIT(5) +#define HI3110_ERR_BITERR BIT(4) +#define HI3110_ERR_FRMERR BIT(3) +#define HI3110_ERR_CRCERR BIT(2) +#define HI3110_ERR_ACKERR BIT(1) +#define HI3110_ERR_STUFERR BIT(0) +#define HI3110_ERR_PROTOCOL_MASK (0x1F) + +#define HI3110_STAT_RXFMTY BIT(1) + +#define HI3110_BTR0_SJW_SHIFT 6 +#define HI3110_BTR0_BRP_SHIFT 0 + +#define HI3110_BTR1_SAMP_3PERBIT (1 << 7) +#define HI3110_BTR1_SAMP_1PERBIT (0 << 7) +#define HI3110_BTR1_TSEG2_SHIFT 4 +#define HI3110_BTR1_TSEG1_SHIFT 0 + +#define HI3110_FIFO_WOTIME_TAG_OFF 0 +#define HI3110_FIFO_WOTIME_ID_OFF 1
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/09/2017 09:45 AM, Wolfgang Grandegger wrote: > Hello Akshay, > > Am 09.03.2017 um 13:34 schrieb Akshay Bhat: >> >> Hi Wolfgang, >> >> Good question. I have not worked with MPC251x but the HI-311x performs >> much better because HI-3110 features: >> 8 message FIFO (as opposed to 2 buffers on MPC2510) >> 20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510) >> >> As for the real world test results: >> >> With RT patch applied to the kernel running on a i.MX6 Dual processor >> (worst case interrupt latency of 50us as reported by cyclictest), there >> are ZERO packet drops. >> Tested with Kvaser Leaf sending 100 burst messages (back to back) every >> 40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser >> leaf and received successfully by the HI-311x driver. > > This corresponds to a bus load of approx. 50%, I think? The usecase I tested above was more like ~30% (data payload length was randomized) but I did test with 50% bus load as well without any dropped messages. > >> Even without the RT patch, I was able to get the packet drop to zero but >> this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0. > > Vanilla Linux is more critical here due to higher latencies. With 2500 > Messages per sec the RX FIFO (8 Messages) fills up within 3.2 ms... and > in a burst even quicker. That's already heavy load. > I agree with Vanilla Linux is more critical due to higher latencies, however if the frequency governor is set to performance and CAN/SPI threads are moved to another CPU (if that is an option) with SCHED_FIFO setting, the driver is able to keep up even at high bit rates / bursts. I understand SPI based CAN controllers are not ideal choice but for users that do not have any other option, I feel this controller/driver meets their need :) Thanks, Akshay
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hi Wolfgang, On 03/09/2017 09:45 AM, Wolfgang Grandegger wrote: > Hello Akshay, > > Am 09.03.2017 um 13:34 schrieb Akshay Bhat: >> >> Hi Wolfgang, >> >> Good question. I have not worked with MPC251x but the HI-311x performs >> much better because HI-3110 features: >> 8 message FIFO (as opposed to 2 buffers on MPC2510) >> 20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510) >> >> As for the real world test results: >> >> With RT patch applied to the kernel running on a i.MX6 Dual processor >> (worst case interrupt latency of 50us as reported by cyclictest), there >> are ZERO packet drops. >> Tested with Kvaser Leaf sending 100 burst messages (back to back) every >> 40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser >> leaf and received successfully by the HI-311x driver. > > This corresponds to a bus load of approx. 50%, I think? The usecase I tested above was more like ~30% (data payload length was randomized) but I did test with 50% bus load as well without any dropped messages. > >> Even without the RT patch, I was able to get the packet drop to zero but >> this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0. > > Vanilla Linux is more critical here due to higher latencies. With 2500 > Messages per sec the RX FIFO (8 Messages) fills up within 3.2 ms... and > in a burst even quicker. That's already heavy load. > I agree with Vanilla Linux is more critical due to higher latencies, however if the frequency governor is set to performance and CAN/SPI threads are moved to another CPU (if that is an option) with SCHED_FIFO setting, the driver is able to keep up even at high bit rates / bursts. I understand SPI based CAN controllers are not ideal choice but for users that do not have any other option, I feel this controller/driver meets their need :) Thanks, Akshay
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 09.03.2017 um 13:34 schrieb Akshay Bhat: On 03/09/2017 04:59 AM, Wolfgang Grandegger wrote: Hello Akshay, unfortunately there are not many CAN controllers for the SPI bus. I just know the MPC251x, which behaves badly (message losses) under Linux, especially at hight bit-rates due to insufficient RX buffering. What is your experience with that driver for the HI-311x? Hi Wolfgang, Good question. I have not worked with MPC251x but the HI-311x performs much better because HI-3110 features: 8 message FIFO (as opposed to 2 buffers on MPC2510) 20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510) As for the real world test results: With RT patch applied to the kernel running on a i.MX6 Dual processor (worst case interrupt latency of 50us as reported by cyclictest), there are ZERO packet drops. Tested with Kvaser Leaf sending 100 burst messages (back to back) every 40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser leaf and received successfully by the HI-311x driver. This corresponds to a bus load of approx. 50%, I think? Even without the RT patch, I was able to get the packet drop to zero but this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0. Vanilla Linux is more critical here due to higher latencies. With 2500 Messages per sec the RX FIFO (8 Messages) fills up within 3.2 ms... and in a burst even quicker. That's already heavy load. Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, Am 09.03.2017 um 13:34 schrieb Akshay Bhat: On 03/09/2017 04:59 AM, Wolfgang Grandegger wrote: Hello Akshay, unfortunately there are not many CAN controllers for the SPI bus. I just know the MPC251x, which behaves badly (message losses) under Linux, especially at hight bit-rates due to insufficient RX buffering. What is your experience with that driver for the HI-311x? Hi Wolfgang, Good question. I have not worked with MPC251x but the HI-311x performs much better because HI-3110 features: 8 message FIFO (as opposed to 2 buffers on MPC2510) 20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510) As for the real world test results: With RT patch applied to the kernel running on a i.MX6 Dual processor (worst case interrupt latency of 50us as reported by cyclictest), there are ZERO packet drops. Tested with Kvaser Leaf sending 100 burst messages (back to back) every 40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser leaf and received successfully by the HI-311x driver. This corresponds to a bus load of approx. 50%, I think? Even without the RT patch, I was able to get the packet drop to zero but this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0. Vanilla Linux is more critical here due to higher latencies. With 2500 Messages per sec the RX FIFO (8 Messages) fills up within 3.2 ms... and in a burst even quicker. That's already heavy load. Wolfgang.
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
On 03/09/2017 04:59 AM, Wolfgang Grandegger wrote: > Hello Akshay, > > unfortunately there are not many CAN controllers for the SPI bus. I just > know the MPC251x, which behaves badly (message losses) under Linux, > especially at hight bit-rates due to insufficient RX buffering. What is > your experience with that driver for the HI-311x? > Hi Wolfgang, Good question. I have not worked with MPC251x but the HI-311x performs much better because HI-3110 features: 8 message FIFO (as opposed to 2 buffers on MPC2510) 20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510) As for the real world test results: With RT patch applied to the kernel running on a i.MX6 Dual processor (worst case interrupt latency of 50us as reported by cyclictest), there are ZERO packet drops. Tested with Kvaser Leaf sending 100 burst messages (back to back) every 40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser leaf and received successfully by the HI-311x driver. Even without the RT patch, I was able to get the packet drop to zero but this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0. Hence I feel the driver is a good candidate to be included in the Linux kernel. Below are detailed test results if you like: https://goo.gl/VWgzp7 Ones of particular interest: 10M-msgs-1M-bitrate-100burst-40ms-interval-leaf.png 10M-msgs-1M-bitrate-100burst-40ms-interval-pwc.txt Thanks, Akshay > > Am 07.03.2017 um 16:31 schrieb Akshay Bhat: >> >> >> On 01/17/2017 02:22 PM, Akshay Bhat wrote: >>> This patch adds support for the Holt HI-311x CAN controller. The HI311x >>> CAN controller is capable of transmitting and receiving standard data >>> frames, extended data frames and remote frames. The HI311x interfaces >>> with the host over SPI. >>> >>> Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do >>> >>> Signed-off-by: Akshay Bhat>>> --- >>> >> >> >> Hi Marc, >> >> Wanted to check if this patch can be included in the next kernel release >> (4.12). >> >> Thanks, >> Akshay >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-can" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >>
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
On 03/09/2017 04:59 AM, Wolfgang Grandegger wrote: > Hello Akshay, > > unfortunately there are not many CAN controllers for the SPI bus. I just > know the MPC251x, which behaves badly (message losses) under Linux, > especially at hight bit-rates due to insufficient RX buffering. What is > your experience with that driver for the HI-311x? > Hi Wolfgang, Good question. I have not worked with MPC251x but the HI-311x performs much better because HI-3110 features: 8 message FIFO (as opposed to 2 buffers on MPC2510) 20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510) As for the real world test results: With RT patch applied to the kernel running on a i.MX6 Dual processor (worst case interrupt latency of 50us as reported by cyclictest), there are ZERO packet drops. Tested with Kvaser Leaf sending 100 burst messages (back to back) every 40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser leaf and received successfully by the HI-311x driver. Even without the RT patch, I was able to get the packet drop to zero but this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0. Hence I feel the driver is a good candidate to be included in the Linux kernel. Below are detailed test results if you like: https://goo.gl/VWgzp7 Ones of particular interest: 10M-msgs-1M-bitrate-100burst-40ms-interval-leaf.png 10M-msgs-1M-bitrate-100burst-40ms-interval-pwc.txt Thanks, Akshay > > Am 07.03.2017 um 16:31 schrieb Akshay Bhat: >> >> >> On 01/17/2017 02:22 PM, Akshay Bhat wrote: >>> This patch adds support for the Holt HI-311x CAN controller. The HI311x >>> CAN controller is capable of transmitting and receiving standard data >>> frames, extended data frames and remote frames. The HI311x interfaces >>> with the host over SPI. >>> >>> Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do >>> >>> Signed-off-by: Akshay Bhat >>> --- >>> >> >> >> Hi Marc, >> >> Wanted to check if this patch can be included in the next kernel release >> (4.12). >> >> Thanks, >> Akshay >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-can" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >>
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, unfortunately there are not many CAN controllers for the SPI bus. I just know the MPC251x, which behaves badly (message losses) under Linux, especially at hight bit-rates due to insufficient RX buffering. What is your experience with that driver for the HI-311x? Thanks, Wolfgang. Am 07.03.2017 um 16:31 schrieb Akshay Bhat: On 01/17/2017 02:22 PM, Akshay Bhat wrote: This patch adds support for the Holt HI-311x CAN controller. The HI311x CAN controller is capable of transmitting and receiving standard data frames, extended data frames and remote frames. The HI311x interfaces with the host over SPI. Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do Signed-off-by: Akshay Bhat--- Hi Marc, Wanted to check if this patch can be included in the next kernel release (4.12). Thanks, Akshay -- To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
Hello Akshay, unfortunately there are not many CAN controllers for the SPI bus. I just know the MPC251x, which behaves badly (message losses) under Linux, especially at hight bit-rates due to insufficient RX buffering. What is your experience with that driver for the HI-311x? Thanks, Wolfgang. Am 07.03.2017 um 16:31 schrieb Akshay Bhat: On 01/17/2017 02:22 PM, Akshay Bhat wrote: This patch adds support for the Holt HI-311x CAN controller. The HI311x CAN controller is capable of transmitting and receiving standard data frames, extended data frames and remote frames. The HI311x interfaces with the host over SPI. Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do Signed-off-by: Akshay Bhat --- Hi Marc, Wanted to check if this patch can be included in the next kernel release (4.12). Thanks, Akshay -- To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
On 01/17/2017 02:22 PM, Akshay Bhat wrote: > This patch adds support for the Holt HI-311x CAN controller. The HI311x > CAN controller is capable of transmitting and receiving standard data > frames, extended data frames and remote frames. The HI311x interfaces > with the host over SPI. > > Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do > > Signed-off-by: Akshay Bhat> --- > Hi Marc, Wanted to check if this patch can be included in the next kernel release (4.12). Thanks, Akshay
Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
On 01/17/2017 02:22 PM, Akshay Bhat wrote: > This patch adds support for the Holt HI-311x CAN controller. The HI311x > CAN controller is capable of transmitting and receiving standard data > frames, extended data frames and remote frames. The HI311x interfaces > with the host over SPI. > > Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do > > Signed-off-by: Akshay Bhat > --- > Hi Marc, Wanted to check if this patch can be included in the next kernel release (4.12). Thanks, Akshay
[PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
This patch adds support for the Holt HI-311x CAN controller. The HI311x CAN controller is capable of transmitting and receiving standard data frames, extended data frames and remote frames. The HI311x interfaces with the host over SPI. Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do Signed-off-by: Akshay Bhat--- v1 -> v2: Address comments from Marc Kleine-Budde: - use u8 instead of uint8_t - alphabetically sort Makefile and Kconfig - copy over copyright information from mcp251x - use single space after each marco - add missing HI3110_ namespace in defines - remove magic number for IDE & SRR bits - simplify logic to populate extended CAN ID - remove unused parameters in hi3110_setup function - remove redundant frame->can_dlc length check - simplify error handling in hi3110_open function Address comments from Julia Lawall: - remove unnecessary semicolon after while loop in hi3110_can_ist drivers/net/can/spi/Kconfig |6 + drivers/net/can/spi/Makefile |1 + drivers/net/can/spi/hi311x.c | 1069 ++ 3 files changed, 1076 insertions(+) create mode 100644 drivers/net/can/spi/hi311x.c diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig index 148cae5..8f2e0dd 100644 --- a/drivers/net/can/spi/Kconfig +++ b/drivers/net/can/spi/Kconfig @@ -1,6 +1,12 @@ menu "CAN SPI interfaces" depends on SPI +config CAN_HI311X + tristate "Holt HI311x SPI CAN controllers" + depends on CAN_DEV && SPI && HAS_DMA + ---help--- + Driver for the Holt HI311x SPI CAN controllers. + config CAN_MCP251X tristate "Microchip MCP251x SPI CAN controllers" depends on HAS_DMA diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile index 0e86040..f59fa37 100644 --- a/drivers/net/can/spi/Makefile +++ b/drivers/net/can/spi/Makefile @@ -3,4 +3,5 @@ # +obj-$(CONFIG_CAN_HI311X) += hi311x.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c new file mode 100644 index 000..cccfe2d --- /dev/null +++ b/drivers/net/can/spi/hi311x.c @@ -0,0 +1,1069 @@ +/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface + * + * Copyright(C) Timesys Corporation 2016 + * + * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver + * Copyright 2009 Christian Pellegrin EVOL S.r.l. + * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved. + * Copyright 2006 Arcom Control Systems Ltd. + * + * Based on CAN bus driver for the CCAN controller written by + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix + * - Simon Kallweit, intefo AG + * Copyright 2007 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HI3110_MASTER_RESET 0x56 +#define HI3110_READ_CTRL0 0xD2 +#define HI3110_READ_CTRL1 0xD4 +#define HI3110_READ_STATF 0xE2 +#define HI3110_WRITE_CTRL0 0x14 +#define HI3110_WRITE_CTRL1 0x16 +#define HI3110_WRITE_INTE 0x1C +#define HI3110_WRITE_BTR0 0x18 +#define HI3110_WRITE_BTR1 0x1A +#define HI3110_READ_BTR0 0xD6 +#define HI3110_READ_BTR1 0xD8 +#define HI3110_READ_INTF 0xDE +#define HI3110_READ_ERR 0xDC +#define HI3110_READ_FIFO_WOTIME 0x48 +#define HI3110_WRITE_FIFO 0x12 +#define HI3110_READ_MESSTAT 0xDA +#define HI3110_READ_TEC 0xEC + +#define HI3110_CTRL0_MODE_MASK (7 << 5) +#define HI3110_CTRL0_NORMAL_MODE (0 << 5) +#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5) +#define HI3110_CTRL0_MONITOR_MODE (2 << 5) +#define HI3110_CTRL0_SLEEP_MODE (3 << 5) +#define HI3110_CTRL0_INIT_MODE (4 << 5) + +#define HI3110_CTRL1_TXEN BIT(7) + +#define HI3110_INT_RXTMP BIT(7) +#define HI3110_INT_RXFIFO BIT(6) +#define HI3110_INT_TXCPLT BIT(5) +#define HI3110_INT_BUSERR BIT(4) +#define HI3110_INT_MCHG BIT(3) +#define HI3110_INT_WAKEUP BIT(2) +#define HI3110_INT_F1MESS BIT(1) +#define HI3110_INT_F0MESS BIT(0) + +#define HI3110_ERR_BUSOFF BIT(7) +#define HI3110_ERR_TXERRP BIT(6) +#define HI3110_ERR_RXERRP BIT(5) +#define HI3110_ERR_BITERR BIT(4) +#define HI3110_ERR_FRMERR BIT(3) +#define HI3110_ERR_CRCERR BIT(2) +#define HI3110_ERR_ACKERR BIT(1) +#define HI3110_ERR_STUFERR BIT(0) +#define HI3110_ERR_PROTOCOL_MASK (0x1F) + +#define HI3110_STAT_RXFMTY BIT(1) + +#define HI3110_BTR0_SJW_SHIFT 6 +#define HI3110_BTR0_BRP_SHIFT 0 + +#define HI3110_BTR1_SAMP_3PERBIT (1 << 7) +#define HI3110_BTR1_SAMP_1PERBIT (0 << 7) +#define HI3110_BTR1_TSEG2_SHIFT 4 +#define HI3110_BTR1_TSEG1_SHIFT 0 + +#define HI3110_FIFO_WOTIME_TAG_OFF 0 +#define HI3110_FIFO_WOTIME_ID_OFF 1 +#define HI3110_FIFO_WOTIME_DLC_OFF 5 +#define HI3110_FIFO_WOTIME_DAT_OFF 6 + +#define
[PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver
This patch adds support for the Holt HI-311x CAN controller. The HI311x CAN controller is capable of transmitting and receiving standard data frames, extended data frames and remote frames. The HI311x interfaces with the host over SPI. Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do Signed-off-by: Akshay Bhat --- v1 -> v2: Address comments from Marc Kleine-Budde: - use u8 instead of uint8_t - alphabetically sort Makefile and Kconfig - copy over copyright information from mcp251x - use single space after each marco - add missing HI3110_ namespace in defines - remove magic number for IDE & SRR bits - simplify logic to populate extended CAN ID - remove unused parameters in hi3110_setup function - remove redundant frame->can_dlc length check - simplify error handling in hi3110_open function Address comments from Julia Lawall: - remove unnecessary semicolon after while loop in hi3110_can_ist drivers/net/can/spi/Kconfig |6 + drivers/net/can/spi/Makefile |1 + drivers/net/can/spi/hi311x.c | 1069 ++ 3 files changed, 1076 insertions(+) create mode 100644 drivers/net/can/spi/hi311x.c diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig index 148cae5..8f2e0dd 100644 --- a/drivers/net/can/spi/Kconfig +++ b/drivers/net/can/spi/Kconfig @@ -1,6 +1,12 @@ menu "CAN SPI interfaces" depends on SPI +config CAN_HI311X + tristate "Holt HI311x SPI CAN controllers" + depends on CAN_DEV && SPI && HAS_DMA + ---help--- + Driver for the Holt HI311x SPI CAN controllers. + config CAN_MCP251X tristate "Microchip MCP251x SPI CAN controllers" depends on HAS_DMA diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile index 0e86040..f59fa37 100644 --- a/drivers/net/can/spi/Makefile +++ b/drivers/net/can/spi/Makefile @@ -3,4 +3,5 @@ # +obj-$(CONFIG_CAN_HI311X) += hi311x.o obj-$(CONFIG_CAN_MCP251X) += mcp251x.o diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c new file mode 100644 index 000..cccfe2d --- /dev/null +++ b/drivers/net/can/spi/hi311x.c @@ -0,0 +1,1069 @@ +/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface + * + * Copyright(C) Timesys Corporation 2016 + * + * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver + * Copyright 2009 Christian Pellegrin EVOL S.r.l. + * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved. + * Copyright 2006 Arcom Control Systems Ltd. + * + * Based on CAN bus driver for the CCAN controller written by + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix + * - Simon Kallweit, intefo AG + * Copyright 2007 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HI3110_MASTER_RESET 0x56 +#define HI3110_READ_CTRL0 0xD2 +#define HI3110_READ_CTRL1 0xD4 +#define HI3110_READ_STATF 0xE2 +#define HI3110_WRITE_CTRL0 0x14 +#define HI3110_WRITE_CTRL1 0x16 +#define HI3110_WRITE_INTE 0x1C +#define HI3110_WRITE_BTR0 0x18 +#define HI3110_WRITE_BTR1 0x1A +#define HI3110_READ_BTR0 0xD6 +#define HI3110_READ_BTR1 0xD8 +#define HI3110_READ_INTF 0xDE +#define HI3110_READ_ERR 0xDC +#define HI3110_READ_FIFO_WOTIME 0x48 +#define HI3110_WRITE_FIFO 0x12 +#define HI3110_READ_MESSTAT 0xDA +#define HI3110_READ_TEC 0xEC + +#define HI3110_CTRL0_MODE_MASK (7 << 5) +#define HI3110_CTRL0_NORMAL_MODE (0 << 5) +#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5) +#define HI3110_CTRL0_MONITOR_MODE (2 << 5) +#define HI3110_CTRL0_SLEEP_MODE (3 << 5) +#define HI3110_CTRL0_INIT_MODE (4 << 5) + +#define HI3110_CTRL1_TXEN BIT(7) + +#define HI3110_INT_RXTMP BIT(7) +#define HI3110_INT_RXFIFO BIT(6) +#define HI3110_INT_TXCPLT BIT(5) +#define HI3110_INT_BUSERR BIT(4) +#define HI3110_INT_MCHG BIT(3) +#define HI3110_INT_WAKEUP BIT(2) +#define HI3110_INT_F1MESS BIT(1) +#define HI3110_INT_F0MESS BIT(0) + +#define HI3110_ERR_BUSOFF BIT(7) +#define HI3110_ERR_TXERRP BIT(6) +#define HI3110_ERR_RXERRP BIT(5) +#define HI3110_ERR_BITERR BIT(4) +#define HI3110_ERR_FRMERR BIT(3) +#define HI3110_ERR_CRCERR BIT(2) +#define HI3110_ERR_ACKERR BIT(1) +#define HI3110_ERR_STUFERR BIT(0) +#define HI3110_ERR_PROTOCOL_MASK (0x1F) + +#define HI3110_STAT_RXFMTY BIT(1) + +#define HI3110_BTR0_SJW_SHIFT 6 +#define HI3110_BTR0_BRP_SHIFT 0 + +#define HI3110_BTR1_SAMP_3PERBIT (1 << 7) +#define HI3110_BTR1_SAMP_1PERBIT (0 << 7) +#define HI3110_BTR1_TSEG2_SHIFT 4 +#define HI3110_BTR1_TSEG1_SHIFT 0 + +#define HI3110_FIFO_WOTIME_TAG_OFF 0 +#define HI3110_FIFO_WOTIME_ID_OFF 1 +#define HI3110_FIFO_WOTIME_DLC_OFF 5 +#define HI3110_FIFO_WOTIME_DAT_OFF 6 + +#define