[PATCH RFC] can fd: Add separate bittiming infrastructure

[PATCH RFC] can fd: Add separate bittiming infrastructure

[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)

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 <socketcan@hartkopp.net>

---

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index fb0ab65..ce4d15c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -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;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index df944ed..bd0493a2 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -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
 };
 

Re: [PATCH RFC] can fd: Add separate bittiming infrastructure

[Marc Kleine-Budde]

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

On 01/15/2014 06:54 PM, Oliver Hartkopp wrote:
> 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

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

Re: [PATCH RFC] can fd: Add separate bittiming infrastructure

[Stephane Grosjean]

Hi,

Some questions:

Le 15/01/2014 18:54, Oliver Hartkopp a écrit :
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index df944ed..bd0493a2 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -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?

>   
>   /*
>    * 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, info@peak-system.com

Re: [PATCH RFC] can fd: Add separate bittiming infrastructure

[Oliver Hartkopp]

Hi Stephane,

On 17.01.2014 08:44, Stephane Grosjean wrote:

>> +#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?

> 
>>     /*
>>    * 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?

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

Re: [PATCH RFC] can fd: Add separate bittiming infrastructure

[Stephane Grosjean]

Le 18/01/2014 18:34, Oliver Hartkopp a écrit :
> Hi Stephane,
>
> On 17.01.2014 08:44, Stephane Grosjean wrote:
>
>>> +#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?

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?

> 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.

> 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".

> 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).

> (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?

> 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?
>
>>>      /*
>>>     * 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?
> 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

> Best regards,
> Oliver
>
> --
> 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, 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, info@peak-system.com

Re: [PATCH RFC] can fd: Add separate bittiming infrastructure

[Oliver Hartkopp]

On 21.01.2014 09:42, Stephane Grosjean wrote:
> Le 18/01/2014 18:34, Oliver Hartkopp a écrit :

>> 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 ...

>> 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".
> 
>> 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.

> 
>> (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

[RFC] can fd: backward compatibility for CANFD8

[Oliver Hartkopp]

On 21.01.2014 09:42, Stephane Grosjean wrote:
> Le 18/01/2014 18:34, Oliver Hartkopp a écrit :

>> 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?
> 
> 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.

> 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.


> 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;
> +

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

>          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?

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

> 
>> 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