[PATCH v2 0/4] can: Add new binding to limit bit rate used

[PATCH v2 0/4] can: Add new binding to limit bit rate used

[Franklin S Cooper Jr]

Add a new generic binding that CAN drivers can use to specify the max
arbitration and data bit rate supported by a transceiver. This is
useful since in some instances the maximum speeds may be limited by
the transceiver used. However, transceivers may not provide a means
to determine this limitation at runtime. Therefore, create a new binding
that mimics "fixed-link" that allows a user to hardcode the max speeds
that can be used.

Also add support for this new binding in the MCAN driver.

Note this is an optional subnode so even if a driver adds support for
parsing fixed-transceiver the user does not have to define it in their
device tree.

Version 2 changes:
Rename function
Define proper variable default
Drop unit address
Move check to changelink function
Reword commit message
Reword documentation

Franklin S Cooper Jr (4):
  can: dev: Add support for limiting configured bitrate
  can: fixed-transceiver: Add documentation for CAN fixed transceiver
    bindings
  can: m_can: Update documentation to mention new fixed transceiver
    binding
  can: m_can: Add call to of_can_transceiver_fixed

 .../bindings/net/can/fixed-transceiver.txt         | 42 +++++++++++++++
 .../devicetree/bindings/net/can/m_can.txt          | 10 ++++
 drivers/net/can/dev.c                              | 59 ++++++++++++++++++++++
 drivers/net/can/m_can/m_can.c                      |  2 +
 include/linux/can/dev.h                            |  5 ++
 5 files changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt

-- 
2.10.0

[PATCH v2 1/4] can: dev: Add support for limiting configured bitrate

[Franklin S Cooper Jr]

Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a fixed-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Version 2 changes:
Rename new function to of_can_transceiver_fixed
Use version of of_property_read that supports signed/negative values
Return error when user tries to use CAN-FD if the transceiver doesn't
support it (max-data-speed = -1).

 drivers/net/can/dev.c   | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h |  5 +++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..c046631 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
 #include <linux/can/skb.h>
 #include <linux/can/netlink.h>
 #include <linux/can/led.h>
+#include <linux/of.h>
 #include <net/rtnetlink.h>
 
 #define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,41 @@ int open_candev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(open_candev);
 
+#ifdef CONFIG_OF
+void of_can_transceiver_fixed(struct net_device *dev)
+{
+	struct device_node *dn;
+	struct can_priv *priv = netdev_priv(dev);
+	int max_frequency;
+	struct device_node *np;
+
+	np = dev->dev.parent->of_node;
+
+	dn = of_get_child_by_name(np, "fixed-transceiver");
+	if (!dn)
+		return;
+
+	/* Value of 0 implies ignore max speed constraint */
+	max_frequency = 0;
+	of_property_read_s32(dn, "max-arbitration-speed", &max_frequency);
+
+	if (max_frequency >= 0)
+		priv->max_trans_arbitration_speed = max_frequency;
+	else
+		priv->max_trans_arbitration_speed = 0;
+
+	max_frequency = 0;
+
+	of_property_read_s32(dn, "max-data-speed", &max_frequency);
+
+	if (max_frequency >= -1)
+		priv->max_trans_data_speed = max_frequency;
+	else
+		priv->max_trans_data_speed = 0;
+}
+EXPORT_SYMBOL(of_can_transceiver_fixed);
+#endif
+
 /*
  * Common close function for cleanup before the device gets closed.
  *
@@ -913,6 +949,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 					priv->bitrate_const_cnt);
 		if (err)
 			return err;
+
+		if (priv->max_trans_arbitration_speed > 0 &&
+		    bt.bitrate > priv->max_trans_arbitration_speed) {
+			netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
+				   priv->max_trans_arbitration_speed);
+			return -EINVAL;
+		}
+
 		memcpy(&priv->bittiming, &bt, sizeof(bt));
 
 		if (priv->do_set_bittiming) {
@@ -989,6 +1033,12 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		if (!priv->data_bittiming_const && !priv->do_set_data_bittiming)
 			return -EOPNOTSUPP;
 
+		if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
+		    priv->max_trans_data_speed == -1) {
+			netdev_err(dev, "canfd mode is not supported by transceiver\n");
+			return -EINVAL;
+		}
+
 		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
 		       sizeof(dbt));
 		err = can_get_bittiming(dev, &dbt,
@@ -997,6 +1047,15 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 					priv->data_bitrate_const_cnt);
 		if (err)
 			return err;
+
+		if (priv->max_trans_data_speed  > 0 &&
+		    (priv->ctrlmode & CAN_CTRLMODE_FD) &&
+		    (dbt.bitrate > priv->max_trans_data_speed)) {
+			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
+				   priv->max_trans_data_speed);
+			return -EINVAL;
+		}
+
 		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
 
 		if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..926fc7e 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -47,6 +47,9 @@ struct can_priv {
 	unsigned int data_bitrate_const_cnt;
 	struct can_clock clock;
 
+	int max_trans_arbitration_speed;
+	int max_trans_data_speed;
+
 	enum can_state state;
 
 	/* CAN controller features - see include/uapi/linux/can/netlink.h */
@@ -165,6 +168,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
 void can_free_echo_skb(struct net_device *dev, unsigned int idx);
 
+void of_can_transceiver_fixed(struct net_device *dev);
+
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
 struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 				struct canfd_frame **cfd);
-- 
2.10.0

[PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Franklin S Cooper Jr]

Add documentation to describe usage of the new fixed transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Version 2:
Tweak commit message working
Tweak wording for properties
Drop unit addressing
Give example if CAN-FD isn't supported

 .../bindings/net/can/fixed-transceiver.txt         | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt

diff --git a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
new file mode 100644
index 0000000..dc7e3eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
@@ -0,0 +1,42 @@
+Fixed transceiver Device Tree binding
+------------------------------
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "fixed-transceiver" node can be used.
+
+Properties:
+
+Optional:
+ max-arbitration-speed: a positive non 0 value that determines the max
+			speed that CAN can run in non CAN-FD mode or during the
+			arbitration phase in CAN-FD mode.
+
+ max-data-speed:	a positive non 0 value that determines the max data rate
+			that can be used in CAN-FD mode. A value of -1 implies
+			CAN-FD is not supported by the transceiver.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+	....
+	fixed-transceiver {
+		max-arbitration-speed = <1000000>;
+		max-data-speed = <5000000>;
+	};
+	...
+};
+
+Based on a CAN Transceiver that doesn't support CAN-FD. Only needed if the
+CAN IP supports CAN-FD but is using a transceiver that doesn't.
+
+m_can0 {
+	....
+	fixed-transceiver {
+		max-data-speed = <(-1)>;
+	};
+	...
+};
-- 
2.10.0

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

> + max-data-speed:	a positive non 0 value that determines the max data rate
> +			that can be used in CAN-FD mode. A value of -1 implies
> +			CAN-FD is not supported by the transceiver.
> +
> +Examples:

(..)

> +	fixed-transceiver {
> +		max-data-speed = <(-1)>;

Looks ugly IMHO.

Why didn't you stay on '0' for 'not supported'??

Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Franklin S Cooper Jr]

On 07/25/2017 11:32 AM, Oliver Hartkopp wrote:
> 
>> + max-data-speed:    a positive non 0 value that determines the max
>> data rate
>> +            that can be used in CAN-FD mode. A value of -1 implies
>> +            CAN-FD is not supported by the transceiver.
>> +
>> +Examples:
> 
> (..)
> 
>> +    fixed-transceiver {
>> +        max-data-speed = <(-1)>;
> 
> Looks ugly IMHO.
> 
> Why didn't you stay on '0' for 'not supported'??

Unless a driver specifically calls of_can_transceiver_fixed
priv->max_trans_data_speed will be by default 0. Therefore, all drivers
that support CAN-FD will claim that the transceiver indicates that it
isn't supported. So one option was to update every single driver to set
this property by default which I started to do but it end up becoming a
massive patch and it was risky in case I missed a driver which would of
resulted in major regressions. Its also problematic for new drivers that
miss this property or the many out of tree CAN drivers. The other option
was to create another variable to track to see if
of_can_transceiver_fixed was called but I didn't think that was the
better solution. So using signed values in DT is a bit ugly due to
syntax but was valid and I made sure I documented it so its clear.
> 
> Regards,
> Oliver
> 

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Andrew Lunn]

On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote:
> Add documentation to describe usage of the new fixed transceiver binding.
> This new binding is applicable for any CAN device therefore it exists as
> its own document.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> Version 2:
> Tweak commit message working
> Tweak wording for properties
> Drop unit addressing
> Give example if CAN-FD isn't supported
> 
>  .../bindings/net/can/fixed-transceiver.txt         | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
> new file mode 100644
> index 0000000..dc7e3eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
> @@ -0,0 +1,42 @@
> +Fixed transceiver Device Tree binding
> +------------------------------
> +
> +CAN transceiver typically limits the max speed in standard CAN and CAN FD
> +modes. Typically these limitations are static and the transceivers themselves
> +provide no way to detect this limitation at runtime. For this situation,
> +the "fixed-transceiver" node can be used.
> +
> +Properties:
> +
> +Optional:
> + max-arbitration-speed: a positive non 0 value that determines the max
> +			speed that CAN can run in non CAN-FD mode or during the
> +			arbitration phase in CAN-FD mode.

Hi Franklin

Since this and the next property are optional, it is good to document
what happens when they are not in the DT blob. Also document what 0
means.

> +
> + max-data-speed:	a positive non 0 value that determines the max data rate
> +			that can be used in CAN-FD mode. A value of -1 implies
> +			CAN-FD is not supported by the transceiver.

-1 is ugly. I think it would be better to have a missing
max-data-speed property indicate that CAN-FD is not supported. And
maybe put 'fd' into the property name.

      Andrew

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

On 07/26/2017 06:41 PM, Andrew Lunn wrote:
> On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote:

>> +
>> +Optional:
>> + max-arbitration-speed: a positive non 0 value that determines the max
>> +			speed that CAN can run in non CAN-FD mode or during the
>> +			arbitration phase in CAN-FD mode.
>
> Hi Franklin
>
> Since this and the next property are optional, it is good to document
> what happens when they are not in the DT blob. Also document what 0
> means.
>
>> +
>> + max-data-speed:	a positive non 0 value that determines the max data rate
>> +			that can be used in CAN-FD mode. A value of -1 implies
>> +			CAN-FD is not supported by the transceiver.
>
> -1 is ugly. I think it would be better to have a missing
> max-data-speed property indicate that CAN-FD is not supported.

Thanks Andrew! I had the same feeling about '-1' :-)

> And
> maybe put 'fd' into the property name.

Good point. In fact the common naming to set bitrates for CAN(FD) 
controllers are 'bitrate' and 'data bitrate'.

'speed' is not really a good word for that.

Finally, @Franklin:

A CAN transceiver is limited in bandwidth. But you only have one RX and 
one TX line between the CAN controller and the CAN transceiver. The 
transceiver does not know about CAN FD - it has just a physical(!) layer 
with a limited bandwidth. This is ONE limitation.

So I tend to specify only ONE 'max-bitrate' property for the 
fixed-transceiver binding.

The fact whether the CAN controller is CAN FD capable or not is provided 
by the netlink configuration interface for CAN controllers.

Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Franklin S Cooper Jr]

On 07/26/2017 12:05 PM, Oliver Hartkopp wrote:
> On 07/26/2017 06:41 PM, Andrew Lunn wrote:
>> On Mon, Jul 24, 2017 at 06:05:19PM -0500, Franklin S Cooper Jr wrote:
> 
>>> +
>>> +Optional:
>>> + max-arbitration-speed: a positive non 0 value that determines the max
>>> +            speed that CAN can run in non CAN-FD mode or during the
>>> +            arbitration phase in CAN-FD mode.
>>
>> Hi Franklin
>>
>> Since this and the next property are optional, it is good to document
>> what happens when they are not in the DT blob. Also document what 0
>> means.

The driver ignores values less than 0 with the exception being
max-data-speed which supports a value of -1. Not sure what I'm
documenting when the binding specifically says to use a positive non
zero value. Its the same reason I don't document what happens if you
give it a negative value.

>>
>>> +
>>> + max-data-speed:    a positive non 0 value that determines the max
>>> data rate
>>> +            that can be used in CAN-FD mode. A value of -1 implies
>>> +            CAN-FD is not supported by the transceiver.
>>
>> -1 is ugly. I think it would be better to have a missing
>> max-data-speed property indicate that CAN-FD is not supported.
> 

Although this leads to your later point I don't think this is the right
approach. Its an optional property. Not including the property should
not assume it isn't supported.

> Thanks Andrew! I had the same feeling about '-1' :-)

Ok I'll go back to having 0 = not supported. Which will handle the
documentation comment above.

> 
>> And
>> maybe put 'fd' into the property name.
> 
> Good point. In fact the common naming to set bitrates for CAN(FD)
> controllers are 'bitrate' and 'data bitrate'.
> 
> 'speed' is not really a good word for that.

I'm fine with switching to using bitrate instead of speed. Kurk was
originally the one that suggested to use the term arbitration and data
since thats how the spec refers to it. Which I do agree with. But your
right that in the drivers (struct can_priv) we just use bittiming and
data_bittiming (CAN-FD timings). I don't think adding "fd" into the
property name makes sense unless we are calling it something like
"max-canfd-bitrate" which I would agree is the easiest to understand.

So what is the preference if we end up sticking with two properties?
Option 1 or 2?

1)
max-bitrate
max-data-bitrate

2)
max-bitrate
max-canfd-bitrate



> 
> Finally, @Franklin:
> 
> A CAN transceiver is limited in bandwidth. But you only have one RX and
> one TX line between the CAN controller and the CAN transceiver. The
> transceiver does not know about CAN FD - it has just a physical(!) layer
> with a limited bandwidth. This is ONE limitation.
> 
> So I tend to specify only ONE 'max-bitrate' property for the
> fixed-transceiver binding.
> 
> The fact whether the CAN controller is CAN FD capable or not is provided
> by the netlink configuration interface for CAN controllers.

Part of the reasoning to have two properties is to indicate that you
don't support CAN FD while limiting the "arbitration" bit rate. With one
property you can not determine this and end up having to make some
assumptions that can quickly end up biting people.



> 
> Regards,
> Oliver
> 

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote:
>

> I'm fine with switching to using bitrate instead of speed. Kurk was
> originally the one that suggested to use the term arbitration and data
> since thats how the spec refers to it. Which I do agree with. But your
> right that in the drivers (struct can_priv) we just use bittiming and
> data_bittiming (CAN-FD timings). I don't think adding "fd" into the
> property name makes sense unless we are calling it something like
> "max-canfd-bitrate" which I would agree is the easiest to understand.
>
> So what is the preference if we end up sticking with two properties?
> Option 1 or 2?
>
> 1)
> max-bitrate
> max-data-bitrate
>
> 2)
> max-bitrate
> max-canfd-bitrate
>
>

1

>> A CAN transceiver is limited in bandwidth. But you only have one RX and
>> one TX line between the CAN controller and the CAN transceiver. The
>> transceiver does not know about CAN FD - it has just a physical(!) layer
>> with a limited bandwidth. This is ONE limitation.
>>
>> So I tend to specify only ONE 'max-bitrate' property for the
>> fixed-transceiver binding.
>>
>> The fact whether the CAN controller is CAN FD capable or not is provided
>> by the netlink configuration interface for CAN controllers.
>
> Part of the reasoning to have two properties is to indicate that you
> don't support CAN FD while limiting the "arbitration" bit rate.

??

It's a physical layer device which only has a bandwidth limitation.
The transceiver does not know about CAN FD.

> With one
> property you can not determine this and end up having to make some
> assumptions that can quickly end up biting people.

Despite the fact that the transceiver does not know anything about ISO 
layer 2 (CAN/CAN FD) the properties should look like

	max-bitrate
	canfd-capable

then.

But when the tranceiver is 'canfd-capable' agnostic, why provide a 
property for it?

Maybe I'm wrong but I still can't follow your argumentation ideas.

Regards,
Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Franklin S Cooper Jr]

On 07/27/2017 01:47 PM, Oliver Hartkopp wrote:
> On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote:
>>
> 
>> I'm fine with switching to using bitrate instead of speed. Kurk was
>> originally the one that suggested to use the term arbitration and data
>> since thats how the spec refers to it. Which I do agree with. But your
>> right that in the drivers (struct can_priv) we just use bittiming and
>> data_bittiming (CAN-FD timings). I don't think adding "fd" into the
>> property name makes sense unless we are calling it something like
>> "max-canfd-bitrate" which I would agree is the easiest to understand.
>>
>> So what is the preference if we end up sticking with two properties?
>> Option 1 or 2?
>>
>> 1)
>> max-bitrate
>> max-data-bitrate
>>
>> 2)
>> max-bitrate
>> max-canfd-bitrate
>>
>>
> 
> 1
> 
>>> A CAN transceiver is limited in bandwidth. But you only have one RX and
>>> one TX line between the CAN controller and the CAN transceiver. The
>>> transceiver does not know about CAN FD - it has just a physical(!) layer
>>> with a limited bandwidth. This is ONE limitation.
>>>
>>> So I tend to specify only ONE 'max-bitrate' property for the
>>> fixed-transceiver binding.
>>>
>>> The fact whether the CAN controller is CAN FD capable or not is provided
>>> by the netlink configuration interface for CAN controllers.
>>
>> Part of the reasoning to have two properties is to indicate that you
>> don't support CAN FD while limiting the "arbitration" bit rate.
> 
> ??
> 
> It's a physical layer device which only has a bandwidth limitation.
> The transceiver does not know about CAN FD.
> 
>> With one
>> property you can not determine this and end up having to make some
>> assumptions that can quickly end up biting people.
> 
> Despite the fact that the transceiver does not know anything about ISO
> layer 2 (CAN/CAN FD) the properties should look like
> 
>     max-bitrate
>     canfd-capable
> 
> then.
> 
> But when the tranceiver is 'canfd-capable' agnostic, why provide a
> property for it?
> 
> Maybe I'm wrong but I still can't follow your argumentation ideas.

Your right. I spoke to our CAN transceiver team and I finally get your
points.

So yes using "max-bitrate" alone is all we need. Sorry for the confusion
and I'll create a new rev using this approach.
> 
> Regards,
> Oliver

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Kurt Van Dijck]

> 
> On 07/27/2017 01:47 PM, Oliver Hartkopp wrote:
> > On 07/26/2017 08:29 PM, Franklin S Cooper Jr wrote:
> >>
> > 
> >> I'm fine with switching to using bitrate instead of speed. Kurk was
> >> originally the one that suggested to use the term arbitration and data
> >> since thats how the spec refers to it. Which I do agree with. But your
> >> right that in the drivers (struct can_priv) we just use bittiming and
> >> data_bittiming (CAN-FD timings). I don't think adding "fd" into the
> >> property name makes sense unless we are calling it something like
> >> "max-canfd-bitrate" which I would agree is the easiest to understand.
> >>
> >> So what is the preference if we end up sticking with two properties?
> >> Option 1 or 2?
> >>
> >> 1)
> >> max-bitrate
> >> max-data-bitrate
> >>
> >> 2)
> >> max-bitrate
> >> max-canfd-bitrate
> >>
> >>
> > 
> > 1
> > 
> >>> A CAN transceiver is limited in bandwidth. But you only have one RX and
> >>> one TX line between the CAN controller and the CAN transceiver. The
> >>> transceiver does not know about CAN FD - it has just a physical(!) layer
> >>> with a limited bandwidth. This is ONE limitation.
> >>>
> >>> So I tend to specify only ONE 'max-bitrate' property for the
> >>> fixed-transceiver binding.
> >>>
> >>> The fact whether the CAN controller is CAN FD capable or not is provided
> >>> by the netlink configuration interface for CAN controllers.
> >>
> >> Part of the reasoning to have two properties is to indicate that you
> >> don't support CAN FD while limiting the "arbitration" bit rate.
> > 
> > ??
> > 
> > It's a physical layer device which only has a bandwidth limitation.
> > The transceiver does not know about CAN FD.
> > 
> >> With one
> >> property you can not determine this and end up having to make some
> >> assumptions that can quickly end up biting people.
> > 
> > Despite the fact that the transceiver does not know anything about ISO
> > layer 2 (CAN/CAN FD) the properties should look like
> > 
> >     max-bitrate
> >     canfd-capable
> > 
> > then.
> > 
> > But when the tranceiver is 'canfd-capable' agnostic, why provide a
> > property for it?
> > 
> > Maybe I'm wrong but I still can't follow your argumentation ideas.
> 

The transceiver does not know about CAN FD, but CAN FD uses
the different restrictions of the arbitration & data phase in the CAN
frame, i.e. during arbitration, the RX must indicate the wire
(dominant/recessive) within 1 bit time, during data in CAN FD, this is
not necessary.

So while _a_ transceiver may be spec'd to 1MBit during arbitration,
CAN FD packets may IMHO exceed that speed during data phase.
That was the whole point of CAN FD: exceed the limits required for
correct arbitration on transceiver & wire.

So I do not agree on the single bandwidth limitation.

The word 'max-arbitration-bitrate' makes the difference very clear.

> Your right. I spoke to our CAN transceiver team and I finally get your
> points.
> 
> So yes using "max-bitrate" alone is all we need. Sorry for the confusion
> and I'll create a new rev using this approach.
> > 
> > Regards,
> > Oliver

Kind regards,
Kurt

Re: [PATCH v2 2/4] can: fixed-transceiver: Add documentation for CAN fixed transceiver bindings

[Oliver Hartkopp]

On 07/28/2017 06:57 AM, Kurt Van Dijck wrote:

> So while _a_ transceiver may be spec'd to 1MBit during arbitration,
> CAN FD packets may IMHO exceed that speed during data phase.

When the bitrate is limited to 1Mbit/s you are ONLY allowed to use 
1Mbit/s in the data section too (either with CAN or CAN FD).

> That was the whole point of CAN FD: exceed the limits required for
> correct arbitration on transceiver & wire.

No. CAN FD is about a different frame format with up to 64 bytes AND the 
possibility to increase the bitrate in the data section of the frame.

> So I do not agree on the single bandwidth limitation.

The transceiver provides a single maximum bandwidth. It's an ISO Layer 1 
device.

> The word 'max-arbitration-bitrate' makes the difference very clear.

I think you are mixing up ISO layer 1 and ISO layer 2.

Regards,
Oliver

[PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding

[Franklin S Cooper Jr]

Add information regarding fixed transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Version 2 changes:
Drop unit address

 Documentation/devicetree/bindings/net/can/m_can.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..e4abd2c 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
 			  Please refer to 2.4.1 Message RAM Configuration in
 			  Bosch M_CAN user manual for details.
 
+Optional properties:
+- fixed-transceiver	: Fixed-transceiver subnode describing maximum speed
+			  that can be used for CAN and/or CAN-FD modes.  See
+			  Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
+			  for details.
 Example:
 SoC dtsi:
 m_can1: can@020e8000 {
@@ -64,4 +69,9 @@ Board dts:
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_m_can1>;
 	status = "enabled";
+
+	fixed-transceiver {
+		max-arbitration-speed = <1000000>;
+		max-data-speed = <5000000>;
+	};
 };
-- 
2.10.0

Re: [PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding

[Rob Herring]

On Mon, Jul 24, 2017 at 06:05:20PM -0500, Franklin S Cooper Jr wrote:
> Add information regarding fixed transceiver binding. This is especially
> important for MCAN since the IP allows CAN FD mode to run significantly
> faster than what most transceivers are capable of.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> Version 2 changes:
> Drop unit address
> 
>  Documentation/devicetree/bindings/net/can/m_can.txt | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> index 9e33177..e4abd2c 100644
> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -43,6 +43,11 @@ Required properties:
>  			  Please refer to 2.4.1 Message RAM Configuration in
>  			  Bosch M_CAN user manual for details.
>  
> +Optional properties:
> +- fixed-transceiver	: Fixed-transceiver subnode describing maximum speed

This is a node, not a property. Sub nodes should have their own section.

> +			  that can be used for CAN and/or CAN-FD modes.  See
> +			  Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
> +			  for details.
>  Example:
>  SoC dtsi:
>  m_can1: can@020e8000 {
> @@ -64,4 +69,9 @@ Board dts:
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_m_can1>;
>  	status = "enabled";
> +
> +	fixed-transceiver {
> +		max-arbitration-speed = <1000000>;
> +		max-data-speed = <5000000>;
> +	};
>  };
> -- 
> 2.10.0
> 

Re: [PATCH v2 3/4] can: m_can: Update documentation to mention new fixed transceiver binding

[Franklin S Cooper Jr]

Hi Rob,
On 08/03/2017 12:07 PM, Rob Herring wrote:
> On Mon, Jul 24, 2017 at 06:05:20PM -0500, Franklin S Cooper Jr wrote:
>> Add information regarding fixed transceiver binding. This is especially
>> important for MCAN since the IP allows CAN FD mode to run significantly
>> faster than what most transceivers are capable of.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>> Version 2 changes:
>> Drop unit address
>>
>>  Documentation/devicetree/bindings/net/can/m_can.txt | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
>> index 9e33177..e4abd2c 100644
>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>> @@ -43,6 +43,11 @@ Required properties:
>>  			  Please refer to 2.4.1 Message RAM Configuration in
>>  			  Bosch M_CAN user manual for details.
>>  
>> +Optional properties:
>> +- fixed-transceiver	: Fixed-transceiver subnode describing maximum speed
> 
> This is a node, not a property. Sub nodes should have their own section.

Fixed in my v4 that I just sent.
> 
>> +			  that can be used for CAN and/or CAN-FD modes.  See
>> +			  Documentation/devicetree/bindings/net/can/fixed-transceiver.txt
>> +			  for details.
>>  Example:
>>  SoC dtsi:
>>  m_can1: can@020e8000 {
>> @@ -64,4 +69,9 @@ Board dts:
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&pinctrl_m_can1>;
>>  	status = "enabled";
>> +
>> +	fixed-transceiver {
>> +		max-arbitration-speed = <1000000>;
>> +		max-data-speed = <5000000>;
>> +	};
>>  };
>> -- 
>> 2.10.0
>>

[PATCH v2 4/4] can: m_can: Add call to of_can_transceiver_fixed

[Franklin S Cooper Jr]

Add call to new generic functions that provides support via a binding
to limit the arbitration rate and/or data rate imposed by the physical
transceiver connected to the MCAN peripheral.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/net/can/m_can/m_can.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..bd75df1 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1649,6 +1649,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
 
 	devm_can_led_init(dev);
 
+	of_can_transceiver_fixed(dev);
+
 	dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n",
 		 KBUILD_MODNAME, dev->irq, priv->version);
 
-- 
2.10.0