can fd: Add separate bittiming infrastructure

RFC PATCH In review Oliver Hartkopp 3 participants 7 messages Jan 15, 2014 – Jan 21, 2014 CAN network drivers

Share & tools

Oliver Hartkopp

author

2014-01-15 17:54 UTC #0 lore raw

RFC PATCH 1/1 can fd: Add separate bittiming infrastructure

Introduce a second set of bitrate configuration for the (potential different) data bitrate. The CAN FD arbitration bitrate is stored in the existing bittiming configuration space.

TODO:
The functions can_calc_bittiming(), can_fixup_bittiming() should change their parameters from

(struct net_device *dev, struct can_bittiming *bt)
to
(struct can_bittiming *bt, const struct can_bittiming_const *btc, const struct can_clock *clk)

to be able to pass the two sets of bittiming parameters (CAN / CAN FD).

And some more changes in dev.c taking care of the two bitrates (when FD is enabled).

Btw. starting with the first idea of a separate bittiming setup. Any comments?

Regards,
Oliver

Signed-off-by Oliver Hartkopp <[email protected]>

---

include/linux/can/dev.h +4 -3

index fb0ab65..ce4d15c 100644
@@ -33,9 +33,10 @@ enum can_mode {
 struct can_priv {
 	struct can_device_stats can_stats;
 
-	struct can_bittiming bittiming;
-	const struct can_bittiming_const *bittiming_const;
-	struct can_clock clock;
+	struct can_bittiming bittiming, data_bittiming;
+	const struct can_bittiming_const *bittiming_const,
+		*data_bittiming_const;
+	struct can_clock clock, data_clock;
 
 	enum can_state state;
 	u32 ctrlmode;

include/uapi/linux/can/netlink.h +4 -0

index df944ed..bd0493a2 100644
@@ -96,6 +96,7 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
 #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
 #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
+#define CAN_CTRLMODE_FD			0x20	/* CAN FD mode */
 
 /*
  * CAN device statistics

@@ -122,6 +123,9 @@ enum {
 	IFLA_CAN_RESTART_MS,
 	IFLA_CAN_RESTART,
 	IFLA_CAN_BERR_COUNTER,
+	IFLA_CAN_DATA_BITTIMING,
+	IFLA_CAN_DATA_BITTIMING_CONST,
+	IFLA_CAN_DATA_CLOCK,
 	__IFLA_CAN_MAX
 };

Marc Kleine-Budde

maintainer

2014-01-16 16:30 UTC #1 lore raw

Quoted 11 lines · Oliver Hartkopp

Introduce a second set of bitrate configuration for the (potential different) data bitrate. The CAN FD arbitration bitrate is stored in the existing bittiming configuration space.
TODO:
The functions can_calc_bittiming(), can_fixup_bittiming() should change their parameters from
(struct net_device *dev, struct can_bittiming *bt)
to
(struct can_bittiming *bt, const struct can_bittiming_const *btc, const struct can_clock *clk)

Are there any netdev_printk()? I don't want to loose the explicit device information.

Otherwise looks good so far.

Marc

Signature · 4 lines

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   |

Stephane Grosjean

2014-01-17 07:44 UTC #2 lore raw

In reply to Oliver Hartkopp

Hi,

Some questions:

include/uapi/linux/can/netlink.h struct can_ctrlmode { · line 99

Quoted 9 lines · Oliver Hartkopp

include/uapi/linux/can/netlink.h +1 -0

index df944ed..bd0493a2 100644
@@ -96,6 +96,7 @@ struct can_ctrlmode {
  #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
  #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
  #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
+#define CAN_CTRLMODE_FD			0x20	/* CAN FD mode */

- What is exactly the goal of this new CAN_CTRLMODE_FD please? Did you define it to allow user to enable the CANFD function into the hardware, for example? If yes, isn't it redundant with setting the MTU to 72 bytes?

- Moreover, how a CANFD -able driver has to handle a CANFD frame read from the CANFD controller, when its network device MTU *ISNOT* == 72 ??? Should it discard the CANFD frame?

include/uapi/linux/can/netlink.h enum { · line 130

Quoted 12 lines

include/uapi/linux/can/netlink.h +3 -0

  /*
   * CAN device statistics

@@ -122,6 +123,9 @@ enum {
  	IFLA_CAN_RESTART_MS,
  	IFLA_CAN_RESTART,
  	IFLA_CAN_BERR_COUNTER,
+	IFLA_CAN_DATA_BITTIMING,
+	IFLA_CAN_DATA_BITTIMING_CONST,
+	IFLA_CAN_DATA_CLOCK,
  	__IFLA_CAN_MAX
  };

- by defining another clock for the data bitrate, do you suppose that some hardwares could use different clocks for both arbitration and data bitrates?

Regards,

Stéphane

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, [email protected]

Oliver Hartkopp

author

2014-01-18 17:34 UTC #3 lore raw

Hi Stephane,

Quoted 9 lines · Stephane Grosjean

Quoted 1 line
+#define CAN_CTRLMODE_FD 0x20 /* CAN FD mode */
- What is exactly the goal of this new CAN_CTRLMODE_FD please? Did you define it to allow user to enable the CANFD function into the hardware, for example? If yes, isn't it redundant with setting the MTU to 72 bytes?
- Moreover, how a CANFD -able driver has to handle a CANFD frame read from the CANFD controller, when its network device MTU *ISNOT* == 72 ??? Should it discard the CANFD frame?

The stuff in linux/net/can/ takes care that only applications that know (and enable) CAN FD get CAN frames with a length > 8 byte.

E.g. today a CAN FD frame with only 8 bytes (aka CANFD8) can be passed to applications that are not CAN FD capable. In this case the size (MTU) is reduced from CANFD_MTU to CAN_MTU, see:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/can/raw.c?id=e2d265d3b587f5f6f8febc0222aace93302ff0be

and

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/can/raw.c#n753

This leads IMO to two questions:

1. Is it a good idea to provide CANFD8 frames to legacy applications?
2. If yes, should the CAN FD controller driver be able to make legacy frames CANDF8 frames to support the higher data bitrate for legacy applications?

Maybe even the first question has to be denied (and the adaption in can/raw.c should be removed then) ?!?

My idea behind CAN_CTRLMODE_FD is to enable/disable the CAN FD mode in the CAN FD capable controller chip.

Depending on this CAN_CTRLMODE_FD the CAN FD functionality is enabled when this feature is supported(!) by the driver, see priv->ctrlmode_supported:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/dev.c#n669

Depending on CAN_CTRLMODE_FD the MTU is then set to CAN_MTU or CANFD_MTU (note that the interface has to be 'down' when configuring this feature - that's just the same as with configuring the bitrate).

When CAN_CTRLMODE_FD was set but the second bitrate was *not* set before the interface is requested to be set to 'up' this leads to the same error as if the (single and only) bitrate is missing today.

I hope this would be a reasonable solution and I was able to describe it well.

Comming back to question 2 from above, we would then need an additional controlmode:

#define CAN_CTRLMODE_TX_FD8 0x40 /* send legacy CAN frames in FD mode */

But I'm not really sure if these two 'compatibility' tweaks for legacy applications to use CAN FD8 is a wanted/needed feature.

Any thoughts?

Quoted 15 lines

include/uapi/linux/can/netlink.h enum { · line 130
Quoted 12 lines
    /*
* CAN device statistics
include/uapi/linux/can/netlink.h +3 -0
@@ -122,6 +123,9 @@ enum {
      IFLA_CAN_RESTART_MS,
      IFLA_CAN_RESTART,
      IFLA_CAN_BERR_COUNTER,
+    IFLA_CAN_DATA_BITTIMING,
+    IFLA_CAN_DATA_BITTIMING_CONST,
+    IFLA_CAN_DATA_CLOCK,
      __IFLA_CAN_MAX
  };
- by defining another clock for the data bitrate, do you suppose that some hardwares could use different clocks for both arbitration and data bitrates?

Yes. But this is probably not necessary. I just looked into the M_CAN specification. They have *one* CAN clock and two different baud rate prescalers. So I would tend to remove IFLA_CAN_DATA_CLOCK in the next RFC until a real requirement emerges from some real hardware.

Best regards,
Oliver

Stephane Grosjean

2014-01-21 08:42 UTC #4 lore raw

Quoted 21 lines · Oliver Hartkopp

Hi Stephane,
Quoted 8 lines · Stephane Grosjean
Quoted 1 line
+#define CAN_CTRLMODE_FD 0x20 /* CAN FD mode */
- What is exactly the goal of this new CAN_CTRLMODE_FD please? Did you define it to allow user to enable the CANFD function into the hardware, for example? If yes, isn't it redundant with setting the MTU to 72 bytes?
- Moreover, how a CANFD -able driver has to handle a CANFD frame read from the CANFD controller, when its network device MTU *ISNOT* == 72 ??? Should it discard the CANFD frame?
The stuff in linux/net/can/ takes care that only applications that know (and enable) CAN FD get CAN frames with a length > 8 byte.
E.g. today a CAN FD frame with only 8 bytes (aka CANFD8) can be passed to applications that are not CAN FD capable. In this case the size (MTU) is reduced from CANFD_MTU to CAN_MTU, see:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/can/raw.c?id=e2d265d3b587f5f6f8febc0222aace93302ff0be

Quoted 7 lines

and
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/can/raw.c#n753
This leads IMO to two questions:
1. Is it a good idea to provide CANFD8 frames to legacy applications?

Well, at first, I didn't very agree with this idea ... But I'm not able to find an example against, so ... My first option was to define a new socket option, or using the "dev->mtu" field value:

If the application sets this option to "on" (or the MTU to 72) so it tells the CAN core that it is able to receive ANY CANFD frames.
If the application doesn't set one or the other option, as all existing "legacy" CAN applications does not, well, the CAN core won't give it ANY incoming CANFD frame, that is, any CAN frame with "skb->len != CAN_MTU" if "dev->mtu != 72". Something like

static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)

 {
         struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

         if (unlikely(!net_eq(dev_net(dev), &init_net)))
                 goto drop;

+ if (dev->mtu != CANFD_MTU)
+ goto drop;
+

         if (WARN_ONCE(dev->type != ARPHRD_CAN ||
                       skb->len != CANFD_MTU ||
                       cfd->len > CANFD_MAX_DLEN,
                       "PF_CAN: dropped non conform CAN FD skbuf: "
                       "dev type %d, len %d, datalen %d\n",
                       dev->type, skb->len, cfd->len))
                 goto drop;

         can_receive(skb, dev);
         return NET_RX_SUCCESS;

drop:

         kfree_skb(skb);
         return NET_RX_DROP;

}

Why trying to be backward compatible at any price?

Quoted 2 lines

2. If yes, should the CAN FD controller driver be able to make legacy frames CANDF8 frames to support the higher data bitrate for legacy applications?

Same as above, I have no real arguments against this.

Quoted 7 lines

Maybe even the first question has to be denied (and the adaption in can/raw.c should be removed then) ?!?
My idea behind CAN_CTRLMODE_FD is to enable/disable the CAN FD mode in the CAN FD capable controller chip.
Depending on this CAN_CTRLMODE_FD the CAN FD functionality is enabled when this feature is supported(!) by the driver, see priv->ctrlmode_supported:

Ok I agree with the usage of this new flag in "ctrmode_supported".

Quoted 3 lines

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/dev.c#n669
Depending on CAN_CTRLMODE_FD the MTU is then set to CAN_MTU or CANFD_MTU

So? Do you mean that the *CAN core* will set the MTU to CAN_MTU or CANFD_MTU according to "priv->ctrlmode_supported & CAN_CTRLMODE_FD" ? (Atm, the MTU is changed from user space only).

Quoted 6 lines

(note that the interface has to be 'down' when configuring this feature - that's just the same as with configuring the bitrate).
When CAN_CTRLMODE_FD was set but the second bitrate was *not* set before the interface is requested to be set to 'up' this leads to the same error as if the (single and only) bitrate is missing today.

So, this means that the arbitration bitrate could not be the default data bitrate, doesn't it?

Quoted 6 lines

I hope this would be a reasonable solution and I was able to describe it well.
Comming back to question 2 from above, we would then need an additional controlmode:
#define CAN_CTRLMODE_TX_FD8 0x40 /* send legacy CAN frames in FD mode */

...

Quoted 18 lines

But I'm not really sure if these two 'compatibility' tweaks for legacy applications to use CAN FD8 is a wanted/needed feature.
Any thoughts?
Quoted 5 lines
Quoted 5 lines
/*
* CAN device statistics
include/uapi/linux/can/netlink.h +0 -0
@@ -122,6 +123,9 @@ enum {
       IFLA_CAN_RESTART_MS,
       IFLA_CAN_RESTART,
Quoted 8 lines
include/uapi/linux/can/netlink.h enum { · line 130
Quoted 6 lines
include/uapi/linux/can/netlink.h +3 -0
       IFLA_CAN_BERR_COUNTER,
+    IFLA_CAN_DATA_BITTIMING,
+    IFLA_CAN_DATA_BITTIMING_CONST,
+    IFLA_CAN_DATA_CLOCK,
       __IFLA_CAN_MAX
   };
- by defining another clock for the data bitrate, do you suppose that some hardwares could use different clocks for both arbitration and data bitrates?

Quoted 4 lines

Yes. But this is probably not necessary. I just looked into the M_CAN specification. They have *one* CAN clock and two different baud rate prescalers. So I would tend to remove IFLA_CAN_DATA_CLOCK in the next RFC until a real requirement emerges from some real hardware.

Ack

Quoted 7 lines

Best regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Oliver Hartkopp

author

2014-01-21 09:56 UTC #5 lore raw

Quoted 6 lines · Stephane Grosjean

Quoted 1 line · Oliver Hartkopp
1. Is it a good idea to provide CANFD8 frames to legacy applications?
Well, at first, I didn't very agree with this idea ... But I'm not able to find an example against, so ...

Ok. Me too :-)

I'll better create a new thread for this compatibility question ...

Quoted 8 lines

Quoted 5 lines
My idea behind CAN_CTRLMODE_FD is to enable/disable the CAN FD mode in the CAN FD capable controller chip.
Depending on this CAN_CTRLMODE_FD the CAN FD functionality is enabled when this feature is supported(!) by the driver, see priv->ctrlmode_supported:
Ok I agree with the usage of this new flag in "ctrmode_supported".

Quoted 8 lines

Quoted 4 lines
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/can/dev.c#n669
Depending on CAN_CTRLMODE_FD the MTU is then set to CAN_MTU or CANFD_MTU
So? Do you mean that the *CAN core* will set the MTU to CAN_MTU or CANFD_MTU according to "priv->ctrlmode_supported & CAN_CTRLMODE_FD" ? (Atm, the MTU is changed from user space only).

Not really. Not the CAN core but the driver does so.

What you see is, that the MTU for the VCAN is set at configuration(!) time. But this is an exception to me, as the VCAN does not use the stuff from dev.c as it has no bitrate setting, etc.

Therefore the MTU for the VCAN is set by root - and not by any application.

Usually AFAIK the MTU is not set by root but from the driver side.

Here: Setting CAN_CTRLMODE_FD at configuration time leads to CAN_MTU or CANFD_MTU set by the driver when the interface is set to 'up'.

The mode of the driver can be checked by *reading* dev->mtu then.

Quoted 11 lines

Quoted 7 lines
(note that the interface has to be 'down' when configuring this feature - that's just the same as with configuring the bitrate).
When CAN_CTRLMODE_FD was set but the second bitrate was *not* set before the interface is requested to be set to 'up' this leads to the same error as if the (single and only) bitrate is missing today.
So, this means that the arbitration bitrate could not be the default data bitrate, doesn't it?

You think about copying the arbitration bitrate into the data bitrate when the data bitrate was not set (with CAN_CTRLMODE_FD), right?

Yes, this could be an idea too.

The question is if this convenience functionality is confusing people or not. IMO your idea is nice.

When you set a different bitrate this will be used. If not, the arbitration bitrate is used for data bitrate too. No objections.

Regards,
Oliver

Oliver Hartkopp

author

2014-01-21 10:22 UTC #6 lore raw

[RFC] can fd: backward compatibility for CANFD8

In reply to Stephane Grosjean

Quoted 21 lines · Stephane Grosjean

Quoted 10 lines · Oliver Hartkopp
The stuff in linux/net/can/ takes care that only applications that know (and enable) CAN FD get CAN frames with a length > 8 byte.
E.g. today a CAN FD frame with only 8 bytes (aka CANFD8) can be passed to applications that are not CAN FD capable. In this case the size (MTU) is reduced from CANFD_MTU to CAN_MTU, see:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/net/can/raw.c?id=e2d265d3b587f5f6f8febc0222aace93302ff0be
Quoted 8 lines
and
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/can/raw.c#n753
This leads IMO to two questions:
1. Is it a good idea to provide CANFD8 frames to legacy applications?

Quoted 7 lines

Well, at first, I didn't very agree with this idea ... But I'm not able to find an example against, so ...
My first option was to define a new socket option, or using the "dev->mtu" field value:
If the application sets this option to "on" (or the MTU to 72) so it tells the CAN core that it is able to receive ANY CANFD frames.

Please distinguish application and configuration.

At configuration time CAN_CTRLMODE_FD can be set by 'root' which leads to CAN_MTU or CANFD_MTU in dev->mtu when the interface is brought 'up'.

The application (e.g. at the CAN_RAW socket) can enable the socket option

CAN_RAW_FD_FRAMES, /* allow CAN FD frames (default:off) */

which means that it is able to cope with CAN frames *and* CAN FD frames.

A CAN controller which is CAN FD enabled is always able to receive 'legacy' CAN frames too.

Quoted 4 lines

If the application doesn't set one or the other option, as all existing "legacy" CAN applications does not, well, the CAN core won't give it ANY incoming CANFD frame, that is, any CAN frame with "skb->len != CAN_MTU" if "dev->mtu != 72".

When CAN_RAW_FD_FRAMES sockopt is not set, the application does not see CAN FD frames (CAN_MTU == 72).

But currently it might see CANFD8 frames, where the size which is read from the socket is reduced from CANFD_MTU to CAN_MTU.

Review

Quoted 13 lines

Something like
static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
 {
         struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

         if (unlikely(!net_eq(dev_net(dev), &init_net)))
                 goto drop;
(quoted diff) +3 -0
+        if (dev->mtu != CANFD_MTU)
+               goto drop;
+

IMO that's not relevant fo application, when we test for skb->len != CANFD_MTU some line later.

Quoted 11 lines

         if (WARN_ONCE(dev->type != ARPHRD_CAN ||
                       skb->len != CANFD_MTU ||
                       cfd->len > CANFD_MAX_DLEN,
                       "PF_CAN: dropped non conform CAN FD skbuf: "
                       "dev type %d, len %d, datalen %d\n",
                       dev->type, skb->len, cfd->len))
                 goto drop;

         can_receive(skb, dev);
         return NET_RX_SUCCESS;

Quoted 6 lines

drop:
         kfree_skb(skb);
         return NET_RX_DROP;
}
Why trying to be backward compatible at any price?

Today you can enable CAN FD on a per socket basis, depending on your ability to cope with CAN FD.

Quoted 6 lines

Quoted 3 lines
2. If yes, should the CAN FD controller driver be able to make legacy frames CANDF8 frames to support the higher data bitrate for legacy applications?
Same as above, I have no real arguments against this.

I'm just not sure, if someone will ever need this ...

Regards,
Oliver