* Re: [PATCH 0/3] Fixes for kvaser_usb driver
From: Marc Kleine-Budde @ 2017-11-21 13:14 UTC (permalink / raw)
To: Jimmy Assarsson, linux-can
In-Reply-To: <20171121072228.7570-1-jimmyassarsson@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 737 bytes --]
On 11/21/2017 08:22 AM, Jimmy Assarsson wrote:
> The first patch fix two missing free calls in error paths. The second patch
> fix a bad comparison between signed and unsigned integer, causing undesired
> behaviour. The third patch will ratelimit error prinouts, if incomplete USB
> messages are received. My kernel log got flooded with these error messages,
> when I encountered the bug described in the previous patch.
Applied to can.
Tnx,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A
From: Marc Kleine-Budde @ 2017-11-21 12:43 UTC (permalink / raw)
To: ZHU Yi (ST-FIR/ENG1-Zhu), Pankaj Bansal, Wolfgang Grandegger,
linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal, Bhupesh Sharma
In-Reply-To: <cf35f42c513946918031b4c1cf6d8778@SGPMBX1017.APAC.bosch.com>
[-- Attachment #1.1: Type: text/plain, Size: 1784 bytes --]
On 11/21/2017 03:13 AM, ZHU Yi (ST-FIR/ENG1-Zhu) wrote:
> Hello Pankaj,
>
>> From: Pankaj Bansal [mailto:pankaj.bansal@nxp.com]
>> Sent: Monday, November 20, 2017 7:11 PM
>> [...]
>> Thank you Zhu Yi for this explanation. This helped me in understanding the quirks.
>> Is there any flexcan core which generates passive state interrupt ?
> I'm afraid so far no known flexcan core supports it.
Should we update the table and add a "IRQ Err Passive" no for all cores?
> * Below is some version info we got:
> * SOC Version IP-Version Glitch- [TR]WRN_INT IRQ Err Memory err RTR re-
> * Filter? connected? Passive detection ception in MB
> * MX25 FlexCAN2 03.00.00.00 no no ? no no
> * MX28 FlexCAN2 03.00.04.00 yes yes no no no
> * MX35 FlexCAN2 03.00.00.00 no no ? no no
> * MX53 FlexCAN2 03.00.00.00 yes no no no no
> * MX6s FlexCAN3 10.00.12.00 yes yes no no yes
> * VF610 FlexCAN3 ? no yes ? yes yes?
And does it make sense to add "FLEXCAN_QUIRK_BROKEN_PERR_STATE" to the
vf610, too?
> static const struct flexcan_devtype_data fsl_vf610_devtype_data = {
> .quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
> FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_USE_OFF_TIMESTAMP,
> };
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.
From: Marc Kleine-Budde @ 2017-11-21 12:38 UTC (permalink / raw)
To: Pankaj Bansal, wg@grandegger.com, linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal
In-Reply-To: <AM0PR0402MB3940428AF84544CD28F613B4F1230@AM0PR0402MB3940.eurprd04.prod.outlook.com>
[-- Attachment #1.1: Type: text/plain, Size: 1417 bytes --]
On 11/21/2017 01:18 PM, Pankaj Bansal wrote:
>> Looks better now. Please add note to
>> "Documentation/devicetree/bindings/net/can/fsl-flexcan.txt" that we now
>> support endianess and state the default endianess.
>>
>> What about:
>>
>> On a "fsl,p1010-flexcan" device BE is default, on all other devices LE is.
>>
>> Please remove the existing "fsl,p1010-flexcan" from "arch/arm/boot/dts"
>> and add fsl,imx25-flexcan, fsl,imx35-flexcan and fsl,imx53-flexcan support to
>> the driver.
>
> @Marc Kleine-Budde :
> Should we keep
> .compatible = "fsl,imx25-flexcan", .data = &fsl_p1010_devtype_data,
> or
> .compatible = "fsl,imx25-flexcan", .data = &fsl_imx25_devtype_data,
>
> in flexcan.c ? right now imx25, imx35, imx53 use same dev_data as p1010.
Yes, keep it this way.
> So If any quirk is added to p1010, it's also applicable for other 3
> platforms.
Probably... I've never had a p1010 in my hands.
> Once we remove p1010 compatible from their device-tree, should we
> keep The dev_data as p1010 or make it separate too ?
Keep it until there is a change makes separating them necessary.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH v2 1/2] can: flexcan: Remodel FlexCAN register r/w APIs for big endian FlexCAN controllers.
From: Pankaj Bansal @ 2017-11-21 12:18 UTC (permalink / raw)
To: Marc Kleine-Budde, wg@grandegger.com, linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal
In-Reply-To: <26cf5587-3100-e681-b477-0f87ed58b872@pengutronix.de>
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Tuesday, November 14, 2017 8:54 PM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; wg@grandegger.com; linux-
> can@vger.kernel.org
> Cc: Varun Sethi <V.Sethi@nxp.com>; Poonam Aggrwal
> <poonam.aggrwal@nxp.com>; Bhupesh Sharma
> <bhupesh.sharma@freescale.com>; Sakar Arora
> <Sakar.Arora@freescale.com>
> Subject: Re: [PATCH v2 1/2] can: flexcan: Remodel FlexCAN register r/w APIs
> for big endian FlexCAN controllers.
>
> On 11/14/2017 12:56 PM, Pankaj Bansal wrote:
> > The FlexCAN driver assumed that FlexCAN controller is big endian for
> > powerpc architecture and little endian for other architectures.
> >
> > But this may not be the case. FlexCAN controller can be little or big
> > endian on any architecture. For e.g. NXP LS1021A ARM based SOC has big
> > endian FlexCAN controller.
> >
> > Therefore, the driver has been modified to add a provision for both
> > types of controllers using an additional device tree property. Big
> > Endian controllers should have "big-endian" set in the device tree.
> >
> > This is the standard practice followed in linux. for more info check:
> > Documentation/devicetree/bindings/common-properties.txt
>
> Looks better now. Please add note to
> "Documentation/devicetree/bindings/net/can/fsl-flexcan.txt" that we now
> support endianess and state the default endianess.
>
> What about:
>
> On a "fsl,p1010-flexcan" device BE is default, on all other devices LE is.
>
> Please remove the existing "fsl,p1010-flexcan" from "arch/arm/boot/dts"
> and add fsl,imx25-flexcan, fsl,imx35-flexcan and fsl,imx53-flexcan support to
> the driver.
>
@Marc Kleine-Budde :
Should we keep
.compatible = "fsl,imx25-flexcan", .data = &fsl_p1010_devtype_data,
or
.compatible = "fsl,imx25-flexcan", .data = &fsl_imx25_devtype_data,
in flexcan.c ? right now imx25, imx35, imx53 use same dev_data as p1010.
So If any quirk is added to p1010, it's also applicable for other 3 platforms.
Once we remove p1010 compatible from their device-tree, should we keep
The dev_data as p1010 or make it separate too ?
^ permalink raw reply
* RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A
From: Pankaj Bansal @ 2017-11-21 10:01 UTC (permalink / raw)
To: ZHU Yi (ST-FIR/ENG1-Zhu), Wolfgang Grandegger, Marc Kleine-Budde,
linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal
In-Reply-To: <63a0798d1f5b4fa6ada730c6ba03480c@SGPMBX1017.APAC.bosch.com>
> -----Original Message-----
> From: ZHU Yi (ST-FIR/ENG1-Zhu) [mailto:Yi.Zhu5@cn.bosch.com]
> Sent: Tuesday, November 21, 2017 9:01 AM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Wolfgang Grandegger
> <wg@grandegger.com>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-
> can@vger.kernel.org
> Cc: Varun Sethi <V.Sethi@nxp.com>; Poonam Aggrwal
> <poonam.aggrwal@nxp.com>; Bhupesh Sharma
> <bhupesh.sharma@freescale.com>
> Subject: RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for
> LS1021A
>
> Hello Pankaj,
>
> >From: Pankaj Bansal [mailto:pankaj.bansal@nxp.com]
> >Sent: Tuesday, November 21, 2017 10:38 AM [...]
> >
> >Thanks Zhu Yi. In that case do we even need to put these changes in a
> quirk?
> >Shouldn't this code be generic flexcan code, irrespective of the platform?
> If in the future version of flexcan supports the complete set of state
> interrupts, then the new core which don't need the workaround can switch it
> off easily via not specifying these quirks.
>
> Make it irrespective of the platform will add the unnecessary overhead to the
> new core (although the overhead is little).
I understand. It's nice that you have thought of future hardware enhancements.
>
> >
> > [...]
> >
> >I will test this after PERR_STATE quirk. But as I mentioned above, in
> >my opinion we should remove FLEXCAN_QUIRK_BROKEN_PERR_STATE, and
> make
> >these changes common for all platforms.
> Thanks, I'm looking forward to the test results. As explained above, we are
> still in the hope that FLEXCAN might improve in the future. :)
>
Without PERR_STATE :
root@TinyDistro:~# ip link set can0 up type can bitrate 125000 restart-ms 100 berr-reporting off
[ 22.507345] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@TinyDistro:~# ip link set can1 up type can bitrate 125000 restart-ms 100 berr-reporting off
[ 30.934669] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready
root@TinyDistro:~# candump -td -e any,0:0,#FFFFFFFF &
[1] 617
root@TinyDistro:~# cangen can0 -D 11223344DEADBEEF -L 8
[ 48.684839] flexcan_irq 804 reg_esr=0x00062242, reg_ecr=0x00000060, reg_ctrl=0x4a31ac55
(000.000000) can0 20000004 [8] 00 08 00 00 00 00 00 00 ERRORFRAME
controller-problem{tx-error-warning}
write: No buffer space available
********** Short L and H signals of CAN ************
root@TinyDistro:~# [ 228.338038] flexcan_irq 804 reg_esr=0x00006036, reg_ecr=0x00000000, reg_ctrl=0x4a31ac55
(179.653204) can0 20000040 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
bus-off
[ 228.438287] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
(000.092122) can0 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
restarted-after-bus-off
With PERR_STATE :
root@TinyDistro:~# ip link set can0 up type can bitrate 125000 restart-ms 100 berr-reporting off
[ 31.997286] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@TinyDistro:~# ip link set can1 up type can bitrate 125000 restart-ms 100 berr-reporting off
[ 40.934824] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready
root@TinyDistro:~# candump -td -e any,0:0,#FFFFFFFF &
[1] 620
root@TinyDistro:~# cangen can0 -D 11223344DEADBEEF -L 8
[ 59.314920] flexcan_irq 805 reg_esr=0x00062242, reg_ecr=0x00000060, reg_ctrl=0x4a31ac55
(000.000000) [ 59.323862] flexcan_irq 805 reg_esr=0x00042252, reg_ecr=0x00000080, reg_ctrl=0x4a31ec55
can0 20000004 [8] 00 08 00 00 00 00 00 00 ERRORFRAME
controller-problem{tx-error-warning}
(000.009673) can0 20000004 [8] 00 20 00 00 00 00 00 00 ERRORFRAME
controller-problem{tx-error-passive}
write: No buffer space available
********** Short L and H signals of CAN ************
root@TinyDistro:~# [ 87.995878] flexcan_irq 805 reg_esr=0x00006036, reg_ecr=0x00000000, reg_ctrl=0x4a31ac55
(028.671291) can0 20000040 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
bus-off
(000.094305) [ 88.098464] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
can0 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
restarted-after-bus-off
> Best regards
> Yi
^ permalink raw reply
* [PATCH 3/3] can: kvaser_usb: ratelimit errors if incomplete messages are received
From: Jimmy Assarsson @ 2017-11-21 7:22 UTC (permalink / raw)
To: linux-can; +Cc: Jimmy Assarsson
In-Reply-To: <20171121072228.7570-1-jimmyassarsson@gmail.com>
Avoid flooding the kernel log with "Formate error", if incomplete message
are received.
Signed-off-by: Jimmy Assarsson <jimmyassarsson@gmail.com>
---
| 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index d87e330a20b3..f95945915d20 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -609,8 +609,8 @@ static int kvaser_usb_wait_msg(const struct kvaser_usb *dev, u8 id,
}
if (pos + tmp->len > actual_len) {
- dev_err(dev->udev->dev.parent,
- "Format error\n");
+ dev_err_ratelimited(dev->udev->dev.parent,
+ "Format error\n");
break;
}
@@ -1353,7 +1353,8 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
}
if (pos + msg->len > urb->actual_length) {
- dev_err(dev->udev->dev.parent, "Format error\n");
+ dev_err_ratelimited(dev->udev->dev.parent,
+ "Format error\n");
break;
}
--
2.15.0
^ permalink raw reply related
* [PATCH 2/3] can: kvaser_usb: Fix comparison bug in kvaser_usb_read_bulk_callback()
From: Jimmy Assarsson @ 2017-11-21 7:22 UTC (permalink / raw)
To: linux-can; +Cc: Jimmy Assarsson
In-Reply-To: <20171121072228.7570-1-jimmyassarsson@gmail.com>
The conditon in the while-loop becomes true when actual_length is less than
2 (MSG_HEADER_LEN). In best case we end up with a former, already
dispatched msg, that got msg->len greater than actual_length. This will
result in a "Format error" error printout.
Problem seen when unplugging a Kvaser USB device connected to a vbox guest.
warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]
Signed-off-by: Jimmy Assarsson <jimmyassarsson@gmail.com>
---
| 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 075644591498..d87e330a20b3 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1334,7 +1334,7 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
goto resubmit_urb;
}
- while (pos <= urb->actual_length - MSG_HEADER_LEN) {
+ while (pos <= (int)(urb->actual_length - MSG_HEADER_LEN)) {
msg = urb->transfer_buffer + pos;
/* The Kvaser firmware can only read and write messages that
--
2.15.0
^ permalink raw reply related
* [PATCH 1/3] can: kvaser_usb: free buf in error paths
From: Jimmy Assarsson @ 2017-11-21 7:22 UTC (permalink / raw)
To: linux-can; +Cc: Jimmy Assarsson
In-Reply-To: <20171121072228.7570-1-jimmyassarsson@gmail.com>
The allocated buffer was not freed if usb_submit_urb() failed.
Signed-off-by: Jimmy Assarsson <jimmyassarsson@gmail.com>
---
| 2 ++
1 file changed, 2 insertions(+)
--git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 9b18d96ef526..075644591498 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -813,6 +813,7 @@ static int kvaser_usb_simple_msg_async(struct kvaser_usb_net_priv *priv,
if (err) {
netdev_err(netdev, "Error transmitting URB\n");
usb_unanchor_urb(urb);
+ kfree(buf);
usb_free_urb(urb);
return err;
}
@@ -1768,6 +1769,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
spin_unlock_irqrestore(&priv->tx_contexts_lock, flags);
usb_unanchor_urb(urb);
+ kfree(buf);
stats->tx_dropped++;
--
2.15.0
^ permalink raw reply related
* [PATCH 0/3] Fixes for kvaser_usb driver
From: Jimmy Assarsson @ 2017-11-21 7:22 UTC (permalink / raw)
To: linux-can; +Cc: Jimmy Assarsson
The first patch fix two missing free calls in error paths. The second patch
fix a bad comparison between signed and unsigned integer, causing undesired
behaviour. The third patch will ratelimit error prinouts, if incomplete USB
messages are received. My kernel log got flooded with these error messages,
when I encountered the bug described in the previous patch.
Jimmy Assarsson (3):
can: kvaser_usb: free buf in error paths
can: kvaser_usb: Fix comparison bug in kvaser_usb_read_bulk_callback()
can: kvaser_usb: ratelimit errors if incomplete messages are received
drivers/net/can/usb/kvaser_usb.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
--
2.15.0
^ permalink raw reply
* RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A
From: ZHU Yi (ST-FIR/ENG1-Zhu) @ 2017-11-21 3:31 UTC (permalink / raw)
To: Pankaj Bansal, Wolfgang Grandegger, Marc Kleine-Budde,
linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal, Bhupesh Sharma
In-Reply-To: <AM0PR0402MB3940F2C5AEB0734DC8B79608F1230@AM0PR0402MB3940.eurprd04.prod.outlook.com>
Hello Pankaj,
>From: Pankaj Bansal [mailto:pankaj.bansal@nxp.com]
>Sent: Tuesday, November 21, 2017 10:38 AM
> [...]
>
>Thanks Zhu Yi. In that case do we even need to put these changes in a quirk?
>Shouldn't this code be generic flexcan code, irrespective of the platform?
If in the future version of flexcan supports the complete set of
state interrupts, then the new core which don't need the workaround
can switch it off easily via not specifying these quirks.
Make it irrespective of the platform will add the unnecessary overhead
to the new core (although the overhead is little).
>
> [...]
>
>I will test this after PERR_STATE quirk. But as I mentioned above, in my opinion
>we should remove FLEXCAN_QUIRK_BROKEN_PERR_STATE, and make these changes
>common for all platforms.
Thanks, I'm looking forward to the test results. As explained above,
we are still in the hope that FLEXCAN might improve in the future. :)
Best regards
Yi
^ permalink raw reply
* RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A
From: Pankaj Bansal @ 2017-11-21 2:37 UTC (permalink / raw)
To: ZHU Yi (ST-FIR/ENG1-Zhu), Wolfgang Grandegger, Marc Kleine-Budde,
linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal, Bhupesh Sharma
In-Reply-To: <cf35f42c513946918031b4c1cf6d8778@SGPMBX1017.APAC.bosch.com>
> -----Original Message-----
> From: ZHU Yi (ST-FIR/ENG1-Zhu) [mailto:Yi.Zhu5@cn.bosch.com]
> Sent: Tuesday, November 21, 2017 7:43 AM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Wolfgang Grandegger
> <wg@grandegger.com>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-
> can@vger.kernel.org
> Cc: Varun Sethi <V.Sethi@nxp.com>; Poonam Aggrwal
> <poonam.aggrwal@nxp.com>; Bhupesh Sharma
> <bhupesh.sharma@freescale.com>
> Subject: RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for
> LS1021A
>
> Hello Pankaj,
>
> >From: Pankaj Bansal [mailto:pankaj.bansal@nxp.com]
> >Sent: Monday, November 20, 2017 7:11 PM [...] Thank you Zhu Yi for
> >this explanation. This helped me in understanding the quirks.
> >Is there any flexcan core which generates passive state interrupt ?
> I'm afraid so far no known flexcan core supports it.
>
> >The reason I ask this is because I see in flexcan doc :
> >
> >The module has many interrupt sources: interrupts due to message
> >buffers and interrupts due to the ORed interrupts from MBs, Bus Off, Error,
> Tx Warning, and Rx Warning.
> >
> >Also in flexcan.c :
> >#define FLEXCAN_ESR_ALL_INT \
> > (FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> > FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> >
> >I don't see (in document or in code), if passive state interrupts are even
> generated ?
> >I see that we can detect passive state using ESR[FLTCONF] = 01.
> >But I think we need to rely on ERROR interrupts to do that (even if the
> BERR_REPORTING is off).
> >
> >Please correct me if I am wrong in my understanding.
> Yes, you're right. We use the error interrupts to report active -> warning ->
> passive, and use the tx/rx interrupts to report passive -> warning -> active.
>
Thanks Zhu Yi. In that case do we even need to put these changes in a quirk?
Shouldn't this code be generic flexcan code, irrespective of the platform?
> >
> > [...]
> >Thank you Wolfgang. With the steps you mentioned and some debug logs I
> >see that LS1021A supports [TR]WRN_INT.
> >
> >root@TinyDistro:~# ip link set can0 up type can bitrate 125000
> >restart-ms 100 berr-reporting off [ 596.117066] IPv6:
> >ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root@TinyDistro:~# ip
> >link set can1 up type can bitrate 125000 restart-ms 100 berr-reporting
> >off [ 600.724547] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes
> >ready root@TinyDistro:~# candump -td -e any,0:0,#FFFFFFFF & [1] 632
> >root@TinyDistro:~# cansend can0 5A1#123412341234 root@TinyDistro:~# [
> >647.522941] flexcan_irq 804 reg_esr=0x00062242, reg_ecr=0x00000060,
> reg_ctrl=0x4a31ac55
> > (000.000000) can0 20000004 [8] 00 08 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{tx-error-warning}
> Seems it needs the FLEXCAN_QUIRK_BROKEN_PERR_STATE. After that, above
> steps should be able to report the correct state - error passive, and all the
> scenarios Wolfgang mentioned should also works fine.
>
> Please be aware that you need to send multiple can frames (manually with
> cansend or automatically with cangen) to get the state back to warning and
> active if removed the error condition on the bus.
>
I will test this after PERR_STATE quirk. But as I mentioned above, in my opinion
we should remove FLEXCAN_QUIRK_BROKEN_PERR_STATE, and make these changes
common for all platforms.
> Best regards
> Yi
^ permalink raw reply
* RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A
From: ZHU Yi (ST-FIR/ENG1-Zhu) @ 2017-11-21 2:13 UTC (permalink / raw)
To: Pankaj Bansal, Wolfgang Grandegger, Marc Kleine-Budde,
linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal, Bhupesh Sharma
In-Reply-To: <AM0PR0402MB394058F204ECA509D1ADB6AEF1220@AM0PR0402MB3940.eurprd04.prod.outlook.com>
Hello Pankaj,
>From: Pankaj Bansal [mailto:pankaj.bansal@nxp.com]
>Sent: Monday, November 20, 2017 7:11 PM
> [...]
>Thank you Zhu Yi for this explanation. This helped me in understanding the quirks.
>Is there any flexcan core which generates passive state interrupt ?
I'm afraid so far no known flexcan core supports it.
>The reason I ask this is because I see in flexcan doc :
>
>The module has many interrupt sources: interrupts due to message buffers and interrupts
>due to the ORed interrupts from MBs, Bus Off, Error, Tx Warning, and Rx Warning.
>
>Also in flexcan.c :
>#define FLEXCAN_ESR_ALL_INT \
> (FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
>
>I don't see (in document or in code), if passive state interrupts are even generated ?
>I see that we can detect passive state using ESR[FLTCONF] = 01.
>But I think we need to rely on ERROR interrupts to do that (even if the BERR_REPORTING is off).
>
>Please correct me if I am wrong in my understanding.
Yes, you're right. We use the error interrupts to report active -> warning -> passive,
and use the tx/rx interrupts to report passive -> warning -> active.
>
> [...]
>Thank you Wolfgang. With the steps you mentioned and some debug logs I see that LS1021A
>supports [TR]WRN_INT.
>
>root@TinyDistro:~# ip link set can0 up type can bitrate 125000 restart-ms 100 berr-reporting off
>[ 596.117066] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
>root@TinyDistro:~# ip link set can1 up type can bitrate 125000 restart-ms 100 berr-reporting off
>[ 600.724547] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready
>root@TinyDistro:~# candump -td -e any,0:0,#FFFFFFFF &
>[1] 632
>root@TinyDistro:~# cansend can0 5A1#123412341234
>root@TinyDistro:~# [ 647.522941] flexcan_irq 804 reg_esr=0x00062242, reg_ecr=0x00000060, reg_ctrl=0x4a31ac55
> (000.000000) can0 20000004 [8] 00 08 00 00 00 00 00 00 ERRORFRAME
> controller-problem{tx-error-warning}
Seems it needs the FLEXCAN_QUIRK_BROKEN_PERR_STATE. After that, above
steps should be able to report the correct state - error passive, and
all the scenarios Wolfgang mentioned should also works fine.
Please be aware that you need to send multiple can frames (manually
with cansend or automatically with cangen) to get the state back to
warning and active if removed the error condition on the bus.
Best regards
Yi
^ permalink raw reply
* Re: [PATCH 7/7] can: rcar_canfd: document r8a77995 (R-Car D3) compatibility strings
From: Rob Herring @ 2017-11-20 20:43 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc, linux-can, devicetree, wsa, horms, geert,
magnus.damm, chris.paterson2, ramesh.shanmugasundaram
In-Reply-To: <1510915289-15059-8-git-send-email-ulrich.hecht+renesas@gmail.com>
On Fri, Nov 17, 2017 at 11:41:29AM +0100, Ulrich Hecht wrote:
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
> Documentation/devicetree/bindings/net/can/rcar_canfd.txt | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 6/7] can: rcar_can: document r8a77995 (R-Car D3) compatibility strings
From: Rob Herring @ 2017-11-20 20:42 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc, linux-can, devicetree, wsa, horms, geert,
magnus.damm, chris.paterson2, ramesh.shanmugasundaram
In-Reply-To: <1510915289-15059-7-git-send-email-ulrich.hecht+renesas@gmail.com>
On Fri, Nov 17, 2017 at 11:41:28AM +0100, Ulrich Hecht wrote:
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
> Documentation/devicetree/bindings/net/can/rcar_can.txt | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH] can: ti_hecc: Fix napi poll return value for repoll
From: Oliver Stäbler @ 2017-11-20 13:45 UTC (permalink / raw)
To: Wolfgang Grandegger, Marc Kleine-Budde, linux-can, netdev
Cc: Oliver Stäbler
After commit d75b1ade567f ("net: less interrupt masking in NAPI") napi
repoll is done only when work_done == budget.
So we need to return budget if there are still packets to receive.
Signed-off-by: Oliver Stäbler <oliver.staebler@bytesatwork.ch>
---
drivers/net/can/ti_hecc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
index 4d4941469cfc..db6ea936dc3f 100644
--- a/drivers/net/can/ti_hecc.c
+++ b/drivers/net/can/ti_hecc.c
@@ -637,6 +637,9 @@ static int ti_hecc_rx_poll(struct napi_struct *napi, int quota)
mbx_mask = hecc_read(priv, HECC_CANMIM);
mbx_mask |= HECC_TX_MBOX_MASK;
hecc_write(priv, HECC_CANMIM, mbx_mask);
+ } else {
+ /* repoll is done only if whole budget is used */
+ num_pkts = quota;
}
return num_pkts;
--
2.13.6
^ permalink raw reply related
* RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A
From: Pankaj Bansal @ 2017-11-20 11:11 UTC (permalink / raw)
To: ZHU Yi (ST-FIR/ENG1-Zhu), Wolfgang Grandegger, Marc Kleine-Budde,
linux-can@vger.kernel.org
Cc: Varun Sethi, Poonam Aggrwal, Bhupesh Sharma
In-Reply-To: <ff7b32a8bc694c92b10967bfd2de24fa@SGPMBX1017.APAC.bosch.com>
> -----Original Message-----
> From: ZHU Yi (ST-FIR/ENG1-Zhu) [mailto:Yi.Zhu5@cn.bosch.com]
> Sent: Thursday, November 16, 2017 12:53 PM
> To: Wolfgang Grandegger <wg@grandegger.com>; Pankaj Bansal
> <pankaj.bansal@nxp.com>; Marc Kleine-Budde <mkl@pengutronix.de>;
> linux-can@vger.kernel.org
> Cc: Varun Sethi <V.Sethi@nxp.com>; Poonam Aggrwal
> <poonam.aggrwal@nxp.com>; Bhupesh Sharma
> <bhupesh.sharma@freescale.com>
> Subject: RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for
> LS1021A
>
> Hello Pankaj,
>
> >From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> >Sent: Thursday, November 16, 2017 3:06 PM
> >
> >Am 16.11.2017 um 06:34 schrieb Pankaj Bansal:
> >> [...]
> >>> Please add your IP core to the "FLEXCAN hardware feature flags"
> >>> table [1] in the driver. Also please test the transition interrupts
> >>> for changes in the CAN error state, see the commits of ZHU Yi
> >>> (ST-FIR/ENG1-Zhu) <Yi.Zhu5@cn.bosch.com> on the driver. [2]
> >>>
> >>
> >> I did not understand this quirk (FLEXCAN_QUIRK_BROKEN_PERR_STATE).
> >> What does this quirk do ? how do I check if my IP supports it or not ?
> >> How do I test this ?
> >>
> >> Can you please help me to understand ?
> >
> >It's about reporting CAN error state changes "error-active" -> "warning"
> >-> "error-passive" -> "bus-off" and back. The quirk might be required
> >-> to
> >trigger the state-change to "error-passive".
>
> The FLEXCAN_QUIRK_BROKEN_PERR_STATE in conjunction with the
> FLEXCAN_QUIRK_BROKEN_WERR_STATE were added for:
> 1. solve error state transition problems found in some core 2. throttle error
> interrupt flooding when applicable due to some core cannot generate state
> interrupt for error warning (that's the WERR_STATE stands for) and/or error
> passive (that's the PERR_STATE stands for), thus the user space cannot
> receive correct state transitions via listening to the SocketCAN socket.
>
> The workaround overcomes the interrupt support shortage by:
> 1. derive correct state from error counters upon any interrupt.
> 2. disable/enable error interrupt to minimize the potential
> performance impact caused by error interrupt flooding.
>
> These two quirks need to set according to hardware features, e.g., the
> i.MX28 and i.MX6 set the PERR_STATE quirk because they have the
> [TR]WRN_INT connected but no interrupt for error passive. If they don't have
> the [TR]WRN_INT too, then both quirks are required.
> (PS: As we understand so far, there is no flexcan core lack of error warning
> interrupt but have error passive interrupt, so the WERR_STATE should never
> been set alone).
>
Thank you Zhu Yi for this explanation. This helped me in understanding the quirks.
Is there any flexcan core which generates passive state interrupt ?
The reason I ask this is because I see in flexcan doc :
The module has many interrupt sources: interrupts due to message buffers and interrupts
due to the ORed interrupts from MBs, Bus Off, Error, Tx Warning, and Rx Warning.
Also in flexcan.c :
#define FLEXCAN_ESR_ALL_INT \
(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
I don't see (in document or in code), if passive state interrupts are even generated ?
I see that we can detect passive state using ESR[FLTCONF] = 01.
But I think we need to rely on ERROR interrupts to do that (even if the BERR_REPORTING is off).
Please correct me if I am wrong in my understanding.
> >
> >You can test such state changes by doing:
> >
> >1. Send messages without CAN cable connected
> >
> >2. Short-circuit the CAN low and high signals
> >
> >Then connect the cable and remove the short-circuit and send messages
> >using "cansend" or "cangen". Please also use "ip set link can0 ...
> >restart-ms 100" to recover from bus-off automatically.
> >
> >While testing watch the error messages with the command below and
> >report the results to the list.
> >
> ># candump -td -e any,0:0,#FFFFFFFF
>
> The fast way to check whether your IP needs the quirk or not is look into the
> reference and find whether it supports the aforementioned interrupts, if
> anyone is missing, then the quirk is needed. Please consult suggestion from
> Wolfgang about how to test.
>
Thank you Wolfgang. With the steps you mentioned and some debug logs I see that LS1021A
supports [TR]WRN_INT.
root@TinyDistro:~# ip link set can0 up type can bitrate 125000 restart-ms 100 berr-reporting off
[ 596.117066] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@TinyDistro:~# ip link set can1 up type can bitrate 125000 restart-ms 100 berr-reporting off
[ 600.724547] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready
root@TinyDistro:~# candump -td -e any,0:0,#FFFFFFFF &
[1] 632
root@TinyDistro:~# cansend can0 5A1#123412341234
root@TinyDistro:~# [ 647.522941] flexcan_irq 804 reg_esr=0x00062242, reg_ecr=0x00000060, reg_ctrl=0x4a31ac55
(000.000000) can0 20000004 [8] 00 08 00 00 00 00 00 00 ERRORFRAME
controller-problem{tx-error-warning}
> Best regards
> Yi
^ permalink raw reply
* Re: [PATCH 0/8] Add CAN support to iwg2[02]d
From: Simon Horman @ 2017-11-20 10:07 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
Russell King, Laurent Pinchart, Geert Uytterhoeven, Linus Walleij,
Magnus Damm, linux-can, netdev, devicetree, linux-arm-kernel,
linux-renesas-soc, linux-gpio, Chris Paterson, Biju Das
In-Reply-To: <1510067449-17017-1-git-send-email-fabrizio.castro@bp.renesas.com>
On Tue, Nov 07, 2017 at 03:10:41PM +0000, Fabrizio Castro wrote:
> Hello,
>
> this series delivers all of the changes necessary to add CAN bus
> support to the:
> * iW-RainboW-G22D SODIMM, and
> * iW-RainboW-G20M-Qseven-RZG1M
> development platforms, including documentation, pinctrl driver, SoC
> specific device trees, and board specific device trees.
>
> This work has been based and tested on top of:
> renesas-devel-20171106-v4.14-rc8
>
> Best regards,
>
> Fabrizio Castro (8):
> dt-bindings: can: rcar_can: document r8a774[35] can support
> pinctrl: sh-pfc: r8a7745: Add CAN[01] support
> ARM: dts: r8a7745: Add CAN[01] SoC support
> ARM: dts: iwg22d-sodimm: Add can0 support to carrier board
> ARM: dts: iwg22d-sodimm-dbhd-ca: Add can1 support to HDMI DB
> ARM: dts: r8a7743: Add CAN[01] SoC support
> ARM: dts: iwg20d-q7-common: Add can0 support to carrier board
> ARM: dts: iwg20d-q7-dbcm-ca: Add can1 support to camera DB
Thanks, I have applied the "ARM: dts" patches for inclusion in v4.16.
^ permalink raw reply
* Re: [PATCH 1/8] dt-bindings: can: rcar_can: document r8a774[35] can support
From: Simon Horman @ 2017-11-20 10:05 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Wolfgang Grandegger, Marc Kleine-Budde, Rob Herring, Mark Rutland,
Geert Uytterhoeven, linux-can, netdev, devicetree,
linux-renesas-soc, Chris Paterson, Biju Das
In-Reply-To: <1510067449-17017-2-git-send-email-fabrizio.castro@bp.renesas.com>
On Tue, Nov 07, 2017 at 03:10:42PM +0000, Fabrizio Castro wrote:
> Document "renesas,can-r8a7743" and "renesas,can-r8a7745" compatible
> strings. Since the fallback compatible string ("renesas,rcar-gen2-can")
> activates the right code in the driver, no driver change is needed.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
^ permalink raw reply
* Re: Request-for-testing: j1939
From: Kurt Van Dijck @ 2017-11-19 19:04 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Elenita Hinds, linux-can, David Jander
In-Reply-To: <20171116084746.GA6974@airbook.vandijck-laurijssen.be>
Marc,
I pushed another commit to my github repository, that fixes the
transport protocol behaviour when the local j1939 node is not
participating.
Kurt
> > I pulled in commit a9ca675 and re-tested -- works!
>
> Great. I'm glad we found this.
>
> It seems that the problem existed for quite some time.
> Since we passed an ISOBUS certification with an early prototype of this
> stack, I'm sure it went wrong afterwards some time ... but its getting
> too complicated to retrieve due to the out-of-tree existance of the
> stack :-(
>
> > I was going to post
> > in the linux-can mail about when the timeframe of merging J1939 in the
> > linux mainline.
>
> Yep, please share your findings on the linux-can mailing list.
>
> >
> > I have started to re-test address claiming and transmissions next.
> >
> > One question - what is the significance of J1939_IDLE_ADDR (0xfe)? Is
> > this address specified in the J1939 spec (I can't find it).
>
> 0xfe is to be used when an ecu was not able to claim an address.
> It can be anybody, and it is used by multiple ecu's, just to participate
> in the necessary network management communication.
>
> >
> > Thanks!
> >
> > --elenita
> >
> > On Tue, Nov 14, 2017 at 2:59 AM, Kurt Van Dijck
> > <dev.kurt@vandijck-laurijssen.be> wrote:
> > > Bravo,
> > >
> > > I think you pointed the right spot.
> > > I'm still looking why it got wrong like that,
> > > in the meanwhile, I pushed a fix in my github repository.
> > >
> > > I found a second similar problem.
> > >
> > > Kurt
> > >
> > >>
> > >> Replies below ...
> > >>
> > >> >I'm curious about the result.
> > >>
> > >> So adding more logging, "priv->ents[cb->addr.da].nusers" is always 0
> > >> (not set at all), as expected since the user space program is not
> > >> claiming any address. But in j1939_can_recv() of main.c,
> > >> skcb->dst_flags is being set to 1 for ECU_LOCAL. See log excerpt
> > >> below:
> > >>
> > >> [ 103.744765] can_j1939: j1939sk_bind: j1939_name_local_get name=0
> > >> <== No name, nusers not set
> > >> [ 103.744827] can_j1939: j1939sk_bind: j1939_addr_local_get sa=255
> > >> <== No addr claimed, nusers not set
> > >> [ 104.135692] j1939_can_recv: da=132 nusers=0 prev_dstflags=0
> > >> curr_dstflags=1 <== Multiframe, curr_dstflags should be 0, not 1
> > >> [ 104.136966] j1939_can_recv: da=28 nusers=0 prev_dstflags=0
> > >> curr_dstflags=1 <== Multiframe, curr_dstflags should be 0, not 1
> > >> [ 104.174357] j1939_can_recv: da=255 nusers=0 prev_dstflags=0
> > >> curr_dstflags=0 <== Multiframe, curr_dstflags=0 is good
> > >>
> > >> (Note: 'da' contains 'skcb->addr.da' value, 'nusers' =>
> > >> 'priv->ents[skcb->addr.da].nusers', '*_dstflags' => 'skcb->dst_flags'
> > >> before and after.)
> > >>
> > >> So a response transmission is attempted even though the destination
> > >> address is not claimed locally because j1939tp_im_receiver() was
> > >> returning 1.
> > >>
> > >> The problem could be the 'if' condition in lines 105-107 of main.c,
> > >> j1939_can_recv():
> > >> if (j1939_address_is_valid(skcb->addr.da) ||
> > >> (j1939_address_is_unicast(skcb->addr.da) &&
> > >> priv->ents[skcb->addr.da].nusers))
> > >>
> > >> As long as skcb->addr.da is not 255, the above logic will always
> > >> return true since j1939_address_is_valid() returns True and the rest
> > >> of the check is skipped because of the OR.
> > >>
> > >> Should the check be something like:
> > >> if ( (j1939_address_is_valid(skcb->addr.da) ||
> > >> j1939_address_is_unicast(skcb->addr.da)) &&
> > >> (priv->ents[skcb->addr.da].nusers) )
> > >>
> > >> or
> > >>
> > >> if ( j1939_address_is_valid(skcb->addr.da) &&
> > >> j1939_address_is_unicast(skcb->addr.da) &&
> > >> priv->ents[skcb->addr.da].nusers )
> > >> ??
> > >>
> > >> Actually, is there a need to call j1939_address_is_valid()? DA=255 is
> > >> not allowed for RTS/CTS data transfers anyway and
> > >> j1939tp_im_receiver() is only called for RTS/CTS transmissions as far
> > >> as I can tell.
> > >>
> > >> >I figured now that I the can-j1939 node tries to participate in the
> > >> >replayed transport sessions, then 2 nodes on the CAN bus will try to
> > >> >send the same CAN id's, on a similar moment in time.
> > >> >That is not good at all, do you have bus-off recovery in place?
> > >>
> > >> Currently, only to configure the CAN interfaces to automatically
> > >> restart in case of a bus-off condition (log the error as well).
> > >>
> > >> Looking forward to more feedback.
> > >>
> > >> Thanks,
> > >> Elenita
> > >>
> > >> On Sat, Nov 11, 2017 at 6:17 AM, Kurt Van Dijck
> > >> <dev.kurt@vandijck-laurijssen.be> wrote:
> > >> >> > Do you replay the log from the same node?
> > >> >>
> > >> >> No, not the same node. The CAN frames are sent by a different node.
> > >> >> Are you familiar with PCAN devices, for example
> > >> >> https://gridconnect.com/can-usb.html? So a script running on a PC
> > >> >> sends the CAN frames through the PCAN adapter which is connected to
> > >> >> the hardware running Linux.
> > >> >
> > >> > I'm aware, I think gridconnect delivers rebranded peak can devices, see
> > >> > https://www.peak-system.com/PCAN-USB.199.0.html?L=1
> > >> >
> > >> >>
> > >> >> > Am I correct that you don't to intend to send a single j1939 frame since
> > >> >> > you replay all via a CAN_RAW socket?
> > >> >>
> > >> >> Yes. Currently, the way I'm able to test is using the test setup I
> > >> >> described above, so many frames will be sent and are captured by the
> > >> >> Linux-based hardware.
> > >> >
> > >> > I figured now that I the can-j1939 node tries to participate in the
> > >> > replayed transport sessions, then 2 nodes on the CAN bus will try to
> > >> > send the same CAN id's, on a similar moment in time.
> > >> > That is not good at all, do you have bus-off recovery in place?
> > >> >
> > >> >>
> > >> >> > You are now testing for dynamically addressed endpoints, not static ones.
> > >> >> > I think that what you try to test is already covered by 'j1939tp_im_receiver()', or it should have been covered.
> > >> >> > So the tests in j1939_can_recv (net/can/j1939/main.c) that add ECU_LOCAL are not accurate in your case,
> > >> >> > but I fail to see why.
> > >> >>
> > >> >> I looked at that part but was not quite sure about the intention of
> > >> >> ECU_LOCAL. But now, I understand -- so if no process within the device
> > >> >> claimed an address, the check "priv->ents[cb->addr.da].nusers" should
> > >> >> be 0, right?
> > >> >
> > >> > Well, ECU_LOCAL is cached copy of priv->ents[cb->addr.da].nusers, at the
> > >> > time of arrival, which is in this case not a problem.
> > >> >
> > >> >> I'll check why it is getting set.
> > >> >
> > >> > I'm curious about the result.
> > >> >
> > >> > Kurt
> > >> >
> > >> >>
> > >> >> > Maybe it helps to look to j1939_can_recv to solve your problem.
> > >> >>
> > >> >> Will do.
> > >> >>
> > >> >> Thanks, Kurt.
> > >> >>
> > >> >> --elenita
> > >> >>
> > >> >>
> > >> >> On Fri, Nov 10, 2017 at 8:42 AM, Kurt Van Dijck
> > >> >> <dev.kurt@vandijck-laurijssen.be> wrote:
> > >> >> > Hey,
> > >> >> >
> > >> >> > I remain puzzled with your setup.
> > >> >> > Do you replay the log from the same node?
> > >> >> >
> > >> >> > Am I correct that you don't to intend to send a single j1939 frame since
> > >> >> > you replay all via a CAN_RAW socket?
> > >> >> >
> > >> >> > Please also find some remarks on your hack below:
> > >> >> >
> > >> >> > --- Original message ---
> > >> >> >> Date: Mon, 6 Nov 2017 16:23:01 -0600
> > >> >> >> From: Elenita Hinds <ecathinds@gmail.com>
> > >> >> >> To: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>
> > >> >> >> Subject: Re: Request-for-testing: j1939
> > >> >> >>
> > >> >> >> Hi Kurt,
> > >> >> >>
> > >> >> >> My temporary hack for the addressing issue is to create a new function
> > >> >> >> in bus.c called j1939_address_is_claimed():
> > >> >> >>
> > >> >> >> bool j1939_address_is_claimed(u8 addr, int ifindex)
> > >> >> >> {
> > >> >> >> struct j1939_ecu *ecu = NULL;
> > >> >> >> struct j1939_priv *priv;
> > >> >> >> bool result = false;
> > >> >> >>
> > >> >> >> if (!j1939_address_is_unicast(addr))
> > >> >> >> return false;
> > >> >> >>
> > >> >> >> priv = j1939_priv_get_by_ifindex(ifindex);
> > >> >> >> if (!priv)
> > >> >> >> return false;
> > >> >> >>
> > >> >> >> read_lock_bh(&priv->lock);
> > >> >> >> list_for_each_entry(ecu, &priv->ecus, list) {
> > >> >> >> if (ecu->sa == addr) {
> > >> >> >> /* Destination address claimed here */
> > >> >> >> result = true;
> > >> >> >> break;
> > >> >> >> }
> > >> >> >> }
> > >> >> >> read_unlock_bh(&priv->lock);
> > >> >> >>
> > >> >> >> return result;
> > >> >> >> }
> > >> >> >>
> > >> >> >>
> > >> >> >> This function is then called in transport.c, functions j1939xtp_rx_rts():
> > >> >> >>
> > >> >> >> if (j1939tp_im_receiver(session->skb)) {
> > >> >> >> // TEST: Check if claimed by this device
> > >> >> >> bool isClaimed = j1939_address_is_claimed(cb->addr.da,
> > >> >> >> session->skb->dev->ifindex);
> > >> >> >> if ( (extd || (tp_cmd_bam != dat[0])) && (isClaimed == true) )
> > >> >> >> j1939_session_schedule_txnow(session);
> > >> >> >> }
> > >> >> >
> > >> >> > You are now testing for dynamically addressed endpoints, not static
> > >> >> > ones.
> > >> >> > I think that what you try to test is already covered by
> > >> >> > 'j1939tp_im_receiver()', or it should have been covered.
> > >> >> > So the tests in j1939_can_recv (net/can/j1939/main.c) that add ECU_LOCAL
> > >> >> > are not accurate in your case, but I fail to see why.
> > >> >> > I bet it originates from 'replaying', of which I'm sure I never tested
> > >> >> > like that :-(
> > >> >> >
> > >> >> > Maybe it helps to look to j1939_can_recv to solve your problem.
> > >> >> >
> > >> >> > Kind regards,
> > >> >> > Kurt
^ permalink raw reply
* Re: PCAN-USB on 3v3 CAN
From: Kurt Van Dijck @ 2017-11-17 18:03 UTC (permalink / raw)
To: linux-can@vger.kernel.org
In-Reply-To: <01dc3e8c-9d59-3211-843a-fc2afe1672c3@posteo.de>
Thanks for your answers. I think have gathered enough argument
to change bitrate ...
923kBit must have been a mistake in the first place :-)
Kurt
--- Original message ---
> Date: Fri, 17 Nov 2017 18:29:47 +0100
> From: Patrick Menschel <menschel.p@posteo.de>
> To: "linux-can@vger.kernel.org" <linux-can@vger.kernel.org>
> Subject: Re: PCAN-USB on 3v3 CAN
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
> Thunderbird/52.4.0
>
> Hi,
>
> I can also confirm that mixing 3,3V and 5V transceivers has minimal
> effect on signal quality.
> The only measureable effect is the slightly reduced differential voltage.
> I have multiple SN65HVD230, MCP2551 and TJA1050 on the same CAN bus.
>
> The only problem I know of are bias networks where CAN Hi is pulled to
> VCC and obviously you should not have a 5V and a 3,3V bias on the same wire.
>
> Regards,
> Patrick
>
>
>
> Am 17.11.2017 um 15:35 schrieb Rost, Martin:
> > Hi Kurt,
> >
> > We're using both 5V transceivers (like TJA1040) and 3.3V tranceivers (like SN65HVD231) on the same busses for years, and I'm not having problems with them.
> > According to the datasheet the bus voltage adheres to the same standard ISO11898.
> > The 3.3V devices are more sensitive to voltage spikes, so shorting the bus lines to e.g. 24V kills your 3.3V transceiver for sure, while most 5V ones should survive.
> >
> > There is a different CAN standard though, thas carries 12V on the bus lines. It's hard to find, and I have never seen anyone using it.
> >
> > Regards
> > Martin
>
> Am 17.11.2017 um 15:41 schrieb Stéphane Grosjean:
> > Hi,
> >
> > The 3V3 is not the problem but this weird bitrate is not supported by the PCAN-USB.
> >
> > Regards,
> >
> > Stephane
> >
> > -----Original Message-----
> > From: linux-can-owner@vger.kernel.org [mailto:linux-can-owner@vger.kernel.org] On Behalf Of Kurt Van Dijck
> > Sent: vendredi 17 novembre 2017 14:30
> > To: linux-can@vger.kernel.org
> > Subject: PCAN-USB on 3v3 CAN
> >
> > Hello,
> >
> > I have difficulty connecting my PCAN-USB to a CAN network where all nodes have a 3v3 transceiver, and the obscure bitrate of 923kBit.
> >
> > Since this is the first time I encountered a '3v3' CAN bus, I was a bit supprised. Will this work with my (I think) 5v PCAN-USB?
> > Is this transceiver voltage interchangeable?
> >
> > Kind regards,
> > Kurt
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> > PEAK-System Technik GmbH
> > Sitz der Gesellschaft Darmstadt - HRB 9183
> > Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
> > --
> > N�����r��y���b�X��ǧv�^�){.n�+����{�q���^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> >
>
^ permalink raw reply
* Re: PCAN-USB on 3v3 CAN
From: Patrick Menschel @ 2017-11-17 17:29 UTC (permalink / raw)
To: linux-can@vger.kernel.org
In-Reply-To: <VI1PR0302MB3325CE41756C19E9042B791BD62F0@VI1PR0302MB3325.eurprd03.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]
Hi,
I can also confirm that mixing 3,3V and 5V transceivers has minimal
effect on signal quality.
The only measureable effect is the slightly reduced differential voltage.
I have multiple SN65HVD230, MCP2551 and TJA1050 on the same CAN bus.
The only problem I know of are bias networks where CAN Hi is pulled to
VCC and obviously you should not have a 5V and a 3,3V bias on the same wire.
Regards,
Patrick
Am 17.11.2017 um 15:35 schrieb Rost, Martin:
> Hi Kurt,
>
> We're using both 5V transceivers (like TJA1040) and 3.3V tranceivers (like SN65HVD231) on the same busses for years, and I'm not having problems with them.
> According to the datasheet the bus voltage adheres to the same standard ISO11898.
> The 3.3V devices are more sensitive to voltage spikes, so shorting the bus lines to e.g. 24V kills your 3.3V transceiver for sure, while most 5V ones should survive.
>
> There is a different CAN standard though, thas carries 12V on the bus lines. It's hard to find, and I have never seen anyone using it.
>
> Regards
> Martin
Am 17.11.2017 um 15:41 schrieb Stéphane Grosjean:
> Hi,
>
> The 3V3 is not the problem but this weird bitrate is not supported by the PCAN-USB.
>
> Regards,
>
> Stephane
>
> -----Original Message-----
> From: linux-can-owner@vger.kernel.org [mailto:linux-can-owner@vger.kernel.org] On Behalf Of Kurt Van Dijck
> Sent: vendredi 17 novembre 2017 14:30
> To: linux-can@vger.kernel.org
> Subject: PCAN-USB on 3v3 CAN
>
> Hello,
>
> I have difficulty connecting my PCAN-USB to a CAN network where all nodes have a 3v3 transceiver, and the obscure bitrate of 923kBit.
>
> Since this is the first time I encountered a '3v3' CAN bus, I was a bit supprised. Will this work with my (I think) 5v PCAN-USB?
> Is this transceiver voltage interchangeable?
>
> Kind regards,
> Kurt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> PEAK-System Technik GmbH
> Sitz der Gesellschaft Darmstadt - HRB 9183
> Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
> --
> N�����r��y���b�X��ǧv�^�){.n�+����{�q���^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3992 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] R-Car D3 (r8a77995) CAN support
From: Wolfram Sang @ 2017-11-17 16:17 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
linux-can-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, horms-/R6kz+dDXgpPR4JQBCEnsQ,
geert-Td1EMuHUCqxL1ZNQvxDV9g, magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
chris.paterson2-zM6kxYcvzFBBDgjK7y7TUQ,
ramesh.shanmugasundaram-kTT6dE0pTRh9uiUsa/gSgQ
In-Reply-To: <1510915289-15059-1-git-send-email-ulrich.hecht+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 361 bytes --]
> Here's CAN and CAN FD support for the R-Car D3. This is a by-the-datasheet
> implementation, with the datasheet missing some bits, namely the pin map.
> I filled in the gaps with frog DNA^W^W^Wby deducing the information from
> pin numbers already in the PFC driver, so careful scrutiny is advised.
Did you have a chance to test one of the configurations?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 6/7] can: rcar_can: document r8a77995 (R-Car D3) compatibility strings
From: Simon Horman @ 2017-11-17 15:39 UTC (permalink / raw)
To: Ulrich Hecht
Cc: linux-renesas-soc, linux-can, devicetree, wsa, geert, magnus.damm,
chris.paterson2, ramesh.shanmugasundaram
In-Reply-To: <1510915289-15059-7-git-send-email-ulrich.hecht+renesas@gmail.com>
On Fri, Nov 17, 2017 at 11:41:28AM +0100, Ulrich Hecht wrote:
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
> Documentation/devicetree/bindings/net/can/rcar_can.txt | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/rcar_can.txt b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> index 06bb7cc..f7d0358 100644
> --- a/Documentation/devicetree/bindings/net/can/rcar_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/rcar_can.txt
> @@ -11,6 +11,7 @@ Required properties:
> "renesas,can-r8a7794" if CAN controller is a part of R8A7794 SoC.
> "renesas,can-r8a7795" if CAN controller is a part of R8A7795 SoC.
> "renesas,can-r8a7796" if CAN controller is a part of R8A7796 SoC.
> + "renesas,can-r8a77995" if CAN controller is a part of R8A77995 SoC.
> "renesas,rcar-gen1-can" for a generic R-Car Gen1 compatible device.
> "renesas,rcar-gen2-can" for a generic R-Car Gen2 compatible device.
> "renesas,rcar-gen3-can" for a generic R-Car Gen3 compatible device.
> @@ -25,12 +26,12 @@ Required properties:
> - pinctrl-0: pin control group to be used for this controller.
> - pinctrl-names: must be "default".
>
> -Required properties for "renesas,can-r8a7795" and "renesas,can-r8a7796"
> -compatible:
> -In R8A7795 and R8A7796 SoCs, "clkp2" can be CANFD clock. This is a div6 clock
> -and can be used by both CAN and CAN FD controller at the same time. It needs to
> -be scaled to maximum frequency if any of these controllers use it. This is done
> -using the below properties:
> +Required properties for "renesas,can-r8a7795", "renesas,can-r8a7796" and
> +"renesas,can-r8a77995" compatible:
Not strictly related to this patch, but aren't these requirements also
applicable to "renesas,rcar-gen3-can"?
> +In R8A7795, R8A7796 and R8A77995 SoCs, "clkp2" can be CANFD clock. This is a
> +div6 clock and can be used by both CAN and CAN FD controller at the same
> +time. It needs to be scaled to maximum frequency if any of these
> +controllers use it. This is done using the below properties:
>
> - assigned-clocks: phandle of clkp2(CANFD) clock.
> - assigned-clock-rates: maximum frequency of this clock.
> --
> 2.7.4
>
^ permalink raw reply
* RE: PCAN-USB on 3v3 CAN
From: Rost, Martin @ 2017-11-17 14:35 UTC (permalink / raw)
To: linux-can@vger.kernel.org
In-Reply-To: <20171117132954.GA7355@airbook.vandijck-laurijssen.be>
Hi Kurt,
We're using both 5V transceivers (like TJA1040) and 3.3V tranceivers (like SN65HVD231) on the same busses for years, and I'm not having problems with them.
According to the datasheet the bus voltage adheres to the same standard ISO11898.
The 3.3V devices are more sensitive to voltage spikes, so shorting the bus lines to e.g. 24V kills your 3.3V transceiver for sure, while most 5V ones should survive.
There is a different CAN standard though, thas carries 12V on the bus lines. It's hard to find, and I have never seen anyone using it.
Regards
Martin
> -----Original Message-----
> From: linux-can-owner@vger.kernel.org [mailto:linux-can-
> owner@vger.kernel.org] On Behalf Of Kurt Van Dijck
> Sent: Friday, November 17, 2017 2:30 PM
> To: linux-can@vger.kernel.org
> Subject: PCAN-USB on 3v3 CAN
>
> Hello,
>
> I have difficulty connecting my PCAN-USB to a CAN network
> where all nodes have a 3v3 transceiver,
> and the obscure bitrate of 923kBit.
>
> Since this is the first time I encountered a '3v3' CAN bus,
> I was a bit supprised. Will this work with my (I think) 5v PCAN-USB?
> Is this transceiver voltage interchangeable?
>
> Kind regards,
> Kurt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: PCAN-USB on 3v3 CAN
From: Stéphane Grosjean @ 2017-11-17 14:41 UTC (permalink / raw)
To: Kurt Van Dijck; +Cc: linux-can@vger.kernel.org
In-Reply-To: <20171117132954.GA7355@airbook.vandijck-laurijssen.be>
Hi,
The 3V3 is not the problem but this weird bitrate is not supported by the PCAN-USB.
Regards,
Stephane
-----Original Message-----
From: linux-can-owner@vger.kernel.org [mailto:linux-can-owner@vger.kernel.org] On Behalf Of Kurt Van Dijck
Sent: vendredi 17 novembre 2017 14:30
To: linux-can@vger.kernel.org
Subject: PCAN-USB on 3v3 CAN
Hello,
I have difficulty connecting my PCAN-USB to a CAN network where all nodes have a 3v3 transceiver, and the obscure bitrate of 923kBit.
Since this is the first time I encountered a '3v3' CAN bus, I was a bit supprised. Will this work with my (I think) 5v PCAN-USB?
Is this transceiver voltage interchangeable?
Kind regards,
Kurt
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
--
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox