* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
@ 2021-03-10 11:18 Stéphane Grosjean
2021-03-10 11:53 ` Marc Kleine-Budde
0 siblings, 1 reply; 9+ messages in thread
From: Stéphane Grosjean @ 2021-03-10 11:18 UTC (permalink / raw)
To: Marc Kleine-Budde, Oliver Hartkopp; +Cc: linux-can Mailing List
Hello,
To complete the open reflection on this subject: to be perfect, the echo skb should in fact not be released by the USB write completion routine but, like the PCI/PCIe version, on reception in the Rx queue of an echo frame to the one written...
This means :
1 - the core of the peak_usb driver has to be deeply modified
2 - the rx path of the USB interface will be much more loaded, resulting in a higher CPU load
2 - in the case of a one-shot frame, how to manage the fact that this echo frame is never received because the one-shot frame could not be written on the bus? Does this need a garbage collector on the echo skbs?
Is it worth it?
Regards,
--- Stéphane
From: Marc Kleine-Budde
Sent: Tuesday, March 9, 2021 15:58
To: Stéphane Grosjean
Cc: Oliver Hartkopp; linux-can Mailing List
Subject: Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
On 09.03.2021 14:53:36, Stéphane Grosjean wrote:
> In the usb drivers, the echo skb is always released by the USB write
> complete callback.
This means there will be an echo_skb_get() when the USB write completes,
which is usually before the CAN controller sends the CAN frame.
This means a TX complete or an TX abort due to one shot mode will not be
reported?
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-10 11:18 [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode Stéphane Grosjean
@ 2021-03-10 11:53 ` Marc Kleine-Budde
0 siblings, 0 replies; 9+ messages in thread
From: Marc Kleine-Budde @ 2021-03-10 11:53 UTC (permalink / raw)
To: Stéphane Grosjean; +Cc: Oliver Hartkopp, linux-can Mailing List
[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]
On 10.03.2021 11:18:15, Stéphane Grosjean wrote:
> To complete the open reflection on this subject: to be perfect, the
> echo skb should in fact not be released by the USB write completion
> routine but, like the PCI/PCIe version, on reception in the Rx queue
> of an echo frame to the one written...
If you don't have a TX complete notification, this would be a workaround
to let the TX echo better reflect what's going on on the bus.
> This means :
> 1 - the core of the peak_usb driver has to be deeply modified
> 2 - the rx path of the USB interface will be much more loaded,
> resulting in a higher CPU load
> 2 - in the case of a one-shot frame, how to manage the fact that this
> echo frame is never received because the one-shot frame could not
> be written on the bus? Does this need a garbage collector on the
> echo skbs?
Yes, you have to free the echo skb in case of a no-send due to one-shot
mode.
Does the HW offer a TX aborted notification?
> Is it worth it?
If you don't have TX complete or TX abort notifications, then it's not
worth it. Better spend time implementing these notifications on the
controller side :)
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-09 8:21 ` [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode Stephane Grosjean
2021-03-09 10:36 ` Marc Kleine-Budde
@ 2021-03-19 10:01 ` Marc Kleine-Budde
1 sibling, 0 replies; 9+ messages in thread
From: Marc Kleine-Budde @ 2021-03-19 10:01 UTC (permalink / raw)
To: Stephane Grosjean; +Cc: linux-can Mailing List
[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]
On 09.03.2021 09:21:28, Stephane Grosjean wrote:
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -39,6 +39,7 @@ MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
>
> #define PCAN_USBPRO_RTR 0x01
> #define PCAN_USBPRO_EXT 0x02
> +#define PCAN_USBPRO_SS 0x08
>
> #define PCAN_USBPRO_CMD_BUFFER_SIZE 512
>
> @@ -779,9 +780,13 @@ static int pcan_usb_pro_encode_msg(struct peak_usb_device *dev,
>
> flags = 0;
> if (cf->can_id & CAN_EFF_FLAG)
> - flags |= 0x02;
> + flags |= PCAN_USBPRO_EXT;
> if (cf->can_id & CAN_RTR_FLAG)
> - flags |= 0x01;
> + flags |= PCAN_USBPRO_RTR;
I've put this change in a separate patch and applied both to linux-can-next/testing.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-09 14:53 Stéphane Grosjean
@ 2021-03-09 14:58 ` Marc Kleine-Budde
0 siblings, 0 replies; 9+ messages in thread
From: Marc Kleine-Budde @ 2021-03-09 14:58 UTC (permalink / raw)
To: Stéphane Grosjean; +Cc: Oliver Hartkopp, linux-can Mailing List
[-- Attachment #1: Type: text/plain, Size: 662 bytes --]
On 09.03.2021 14:53:36, Stéphane Grosjean wrote:
> In the usb drivers, the echo skb is always released by the USB write
> complete callback.
This means there will be an echo_skb_get() when the USB write completes,
which is usually before the CAN controller sends the CAN frame.
This means a TX complete or an TX abort due to one shot mode will not be
reported?
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
@ 2021-03-09 14:53 Stéphane Grosjean
2021-03-09 14:58 ` Marc Kleine-Budde
0 siblings, 1 reply; 9+ messages in thread
From: Stéphane Grosjean @ 2021-03-09 14:53 UTC (permalink / raw)
To: Marc Kleine-Budde, Oliver Hartkopp, linux-can Mailing List
In the usb drivers, the echo skb is always released by the USB write complete callback.
--- Stéphane
From: Marc Kleine-Budde
Sent: Tuesday, March 9, 2021 11:58
To: Oliver Hartkopp; Stéphane Grosjean; linux-can Mailing List
Subject: Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
On 3/9/21 11:53 AM, Oliver Hartkopp wrote:
>> What happens if in one shot mode and the frame is not send? Who takes care of
>> the echo skb?
>
> ONE-SHOT only means that the CAN controller would not retry the
> transmission when e.g. loosing the arbitration or getting an error flag.
>
> The sja1000 does it this way, when the TX interrupt flag is set in the
> interrupt register:
>
> if (isrc & IRQ_TI) {
> /* transmission buffer released */
> if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
> !(status & SR_TCS)) {
> stats->tx_errors++;
> can_free_echo_skb(dev, 0);
> } else {
> /* transmission complete */
> stats->tx_bytes +=
> priv->read_reg(priv,
> SJA1000_FI) & 0xf;
> stats->tx_packets++;
> can_get_echo_skb(dev, 0, NULL);
> }
> netif_wake_queue(dev);
> can_led_event(dev, CAN_LED_EVENT_TX);
> }
>
>
> Do we need to check this for the other drivers?
Yes.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
PEAK-System Technik GmbH
Sitz der Gesellschaft Darmstadt - HRB 9183
Geschaeftsfuehrung: Alexander Gach / Uwe Wilhelm
Unsere Datenschutzerklaerung mit wichtigen Hinweisen
zur Behandlung personenbezogener Daten finden Sie unter
www.peak-system.com/Datenschutz.483.0.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-09 10:53 ` Oliver Hartkopp
@ 2021-03-09 10:58 ` Marc Kleine-Budde
0 siblings, 0 replies; 9+ messages in thread
From: Marc Kleine-Budde @ 2021-03-09 10:58 UTC (permalink / raw)
To: Oliver Hartkopp, Stephane Grosjean, linux-can Mailing List
[-- Attachment #1.1: Type: text/plain, Size: 1639 bytes --]
On 3/9/21 11:53 AM, Oliver Hartkopp wrote:
>> What happens if in one shot mode and the frame is not send? Who takes care of
>> the echo skb?
>
> ONE-SHOT only means that the CAN controller would not retry the
> transmission when e.g. loosing the arbitration or getting an error flag.
>
> The sja1000 does it this way, when the TX interrupt flag is set in the
> interrupt register:
>
> if (isrc & IRQ_TI) {
> /* transmission buffer released */
> if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
> !(status & SR_TCS)) {
> stats->tx_errors++;
> can_free_echo_skb(dev, 0);
> } else {
> /* transmission complete */
> stats->tx_bytes +=
> priv->read_reg(priv,
> SJA1000_FI) & 0xf;
> stats->tx_packets++;
> can_get_echo_skb(dev, 0, NULL);
> }
> netif_wake_queue(dev);
> can_led_event(dev, CAN_LED_EVENT_TX);
> }
>
>
> Do we need to check this for the other drivers?
Yes.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-09 10:36 ` Marc Kleine-Budde
@ 2021-03-09 10:53 ` Oliver Hartkopp
2021-03-09 10:58 ` Marc Kleine-Budde
0 siblings, 1 reply; 9+ messages in thread
From: Oliver Hartkopp @ 2021-03-09 10:53 UTC (permalink / raw)
To: Marc Kleine-Budde, Stephane Grosjean, linux-can Mailing List
On 09.03.21 11:36, Marc Kleine-Budde wrote:
> On 3/9/21 9:21 AM, Stephane Grosjean wrote:
>> This patch adds "ONE-SHOT" mode support to the following CAN-USB
>> PEAK-System GmbH interfaces:
>> - PCAN-USB X6
>> - PCAN-USB FD
>> - PCAN-USB Pro FD
>> - PCAN-Chip USB
>> - PCAN-USB Pro
>>
>> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
>
> What happens if in one shot mode and the frame is not send? Who takes care of
> the echo skb?
ONE-SHOT only means that the CAN controller would not retry the
transmission when e.g. loosing the arbitration or getting an error flag.
The sja1000 does it this way, when the TX interrupt flag is set in the
interrupt register:
if (isrc & IRQ_TI) {
/* transmission buffer released */
if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
!(status & SR_TCS)) {
stats->tx_errors++;
can_free_echo_skb(dev, 0);
} else {
/* transmission complete */
stats->tx_bytes +=
priv->read_reg(priv,
SJA1000_FI) & 0xf;
stats->tx_packets++;
can_get_echo_skb(dev, 0, NULL);
}
netif_wake_queue(dev);
can_led_event(dev, CAN_LED_EVENT_TX);
}
Do we need to check this for the other drivers?
Regards,
Oliver
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-09 8:21 ` [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode Stephane Grosjean
@ 2021-03-09 10:36 ` Marc Kleine-Budde
2021-03-09 10:53 ` Oliver Hartkopp
2021-03-19 10:01 ` Marc Kleine-Budde
1 sibling, 1 reply; 9+ messages in thread
From: Marc Kleine-Budde @ 2021-03-09 10:36 UTC (permalink / raw)
To: Stephane Grosjean, linux-can Mailing List
[-- Attachment #1.1: Type: text/plain, Size: 693 bytes --]
On 3/9/21 9:21 AM, Stephane Grosjean wrote:
> This patch adds "ONE-SHOT" mode support to the following CAN-USB
> PEAK-System GmbH interfaces:
> - PCAN-USB X6
> - PCAN-USB FD
> - PCAN-USB Pro FD
> - PCAN-Chip USB
> - PCAN-USB Pro
>
> Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
What happens if in one shot mode and the frame is not send? Who takes care of
the echo skb?
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode
2021-03-09 8:21 [PATCH 0/3] can/peak_usb: Added improvement to the peak_usb driver Stephane Grosjean
@ 2021-03-09 8:21 ` Stephane Grosjean
2021-03-09 10:36 ` Marc Kleine-Budde
2021-03-19 10:01 ` Marc Kleine-Budde
0 siblings, 2 replies; 9+ messages in thread
From: Stephane Grosjean @ 2021-03-09 8:21 UTC (permalink / raw)
To: linux-can Mailing List; +Cc: Stephane Grosjean
This patch adds "ONE-SHOT" mode support to the following CAN-USB
PEAK-System GmbH interfaces:
- PCAN-USB X6
- PCAN-USB FD
- PCAN-USB Pro FD
- PCAN-Chip USB
- PCAN-USB Pro
Signed-off-by: Stephane Grosjean <s.grosjean@peak-system.com>
---
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 12 ++++++++----
drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 12 +++++++++---
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 8e6250c4c417..0e3bb3a4945d 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -779,6 +779,10 @@ static int pcan_usb_fd_encode_msg(struct peak_usb_device *dev,
tx_msg_flags |= PUCAN_MSG_RTR;
}
+ /* Single-Shot frame */
+ if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+ tx_msg_flags |= PUCAN_MSG_SINGLE_SHOT;
+
tx_msg->flags = cpu_to_le16(tx_msg_flags);
tx_msg->channel_dlc = PUCAN_MSG_CHANNEL_DLC(dev->ctrl_idx, dlc);
memcpy(tx_msg->d, cfd->data, cfd->len);
@@ -1068,7 +1072,7 @@ const struct peak_usb_adapter pcan_usb_fd = {
.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
.ctrlmode_supported = CAN_CTRLMODE_FD |
CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_CC_LEN8_DLC,
+ CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
@@ -1143,7 +1147,7 @@ const struct peak_usb_adapter pcan_usb_chip = {
.ctrl_count = PCAN_USBFD_CHANNEL_COUNT,
.ctrlmode_supported = CAN_CTRLMODE_FD |
CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_CC_LEN8_DLC,
+ CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
@@ -1217,7 +1221,7 @@ const struct peak_usb_adapter pcan_usb_pro_fd = {
.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
.ctrlmode_supported = CAN_CTRLMODE_FD |
CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_CC_LEN8_DLC,
+ CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
@@ -1292,7 +1296,7 @@ const struct peak_usb_adapter pcan_usb_x6 = {
.ctrl_count = PCAN_USBPROFD_CHANNEL_COUNT,
.ctrlmode_supported = CAN_CTRLMODE_FD |
CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
- CAN_CTRLMODE_CC_LEN8_DLC,
+ CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_CC_LEN8_DLC,
.clock = {
.freq = PCAN_UFD_CRYSTAL_HZ,
},
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index ff740b4203fa..5b098d1e3746 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -39,6 +39,7 @@ MODULE_SUPPORTED_DEVICE("PEAK-System PCAN-USB Pro adapter");
#define PCAN_USBPRO_RTR 0x01
#define PCAN_USBPRO_EXT 0x02
+#define PCAN_USBPRO_SS 0x08
#define PCAN_USBPRO_CMD_BUFFER_SIZE 512
@@ -779,9 +780,13 @@ static int pcan_usb_pro_encode_msg(struct peak_usb_device *dev,
flags = 0;
if (cf->can_id & CAN_EFF_FLAG)
- flags |= 0x02;
+ flags |= PCAN_USBPRO_EXT;
if (cf->can_id & CAN_RTR_FLAG)
- flags |= 0x01;
+ flags |= PCAN_USBPRO_RTR;
+
+ /* Single-Shot frame */
+ if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+ flags |= PCAN_USBPRO_SS;
pcan_msg_add_rec(&usb_msg, data_type, 0, flags, len, cf->can_id,
cf->data);
@@ -1041,7 +1046,8 @@ const struct peak_usb_adapter pcan_usb_pro = {
.name = "PCAN-USB Pro",
.device_id = PCAN_USBPRO_PRODUCT_ID,
.ctrl_count = PCAN_USBPRO_CHANNEL_COUNT,
- .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY,
+ .ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY |
+ CAN_CTRLMODE_ONE_SHOT,
.clock = {
.freq = PCAN_USBPRO_CRYSTAL_HZ,
},
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-19 10:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 11:18 [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode Stéphane Grosjean
2021-03-10 11:53 ` Marc Kleine-Budde
-- strict thread matches above, loose matches on Subject: below --
2021-03-09 14:53 Stéphane Grosjean
2021-03-09 14:58 ` Marc Kleine-Budde
2021-03-09 8:21 [PATCH 0/3] can/peak_usb: Added improvement to the peak_usb driver Stephane Grosjean
2021-03-09 8:21 ` [PATCH 3/3] can/peak_usb: add support of ONE_SHOT mode Stephane Grosjean
2021-03-09 10:36 ` Marc Kleine-Budde
2021-03-09 10:53 ` Oliver Hartkopp
2021-03-09 10:58 ` Marc Kleine-Budde
2021-03-19 10:01 ` Marc Kleine-Budde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.