[PATCH v4 0/4] can: Support transceiver based bit rate limit

[PATCH v4 0/4] can: Support transceiver based bit rate limit

[Franklin S Cooper Jr]

Add a new generic binding that CAN drivers can be used to specify the max
bit rate supported by a transceiver. This is useful since in some instances
since 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 can-transceiver the user does not have to define it in their
device tree.

Version 4 changes:
Switch from fixed-transceiver to can-transceiver
Drop unit address that snuck back in again.
Indicate that can-transceiver is a subnode and not a property in
documentation

Version 3 changes:
Switch from having two "max bitrates" to one universal bitrate.

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
  dt-bindings: can: can-transceiver: Document new binding
  dt-bindings: can: m_can: Document new can transceiver binding
  can: m_can: Add call to of_can_transceiver

 .../bindings/net/can/can-transceiver.txt           | 24 +++++++++++
 .../devicetree/bindings/net/can/m_can.txt          |  9 ++++
 drivers/net/can/dev.c                              | 50 ++++++++++++++++++++++
 drivers/net/can/m_can/m_can.c                      |  2 +
 include/linux/can/dev.h                            |  5 +++
 5 files changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt

-- 
2.9.4.dirty

[PATCH v4 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 can-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 4 changes:
Used can-transceiver instead of fixed-transceiver.

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

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..372108f 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,39 @@ int open_candev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(open_candev);
 
+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver(struct net_device *dev)
+{
+	struct device_node *dn;
+	struct can_priv *priv = netdev_priv(dev);
+	struct device_node *np;
+	unsigned int max_bitrate;
+	int ret;
+
+	np = dev->dev.parent->of_node;
+
+	dn = of_get_child_by_name(np, "can-transceiver");
+	if (!dn)
+		return;
+
+	max_bitrate = 0;
+	ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
+
+	if (max_bitrate > 0) {
+		priv->max_bitrate = max_bitrate;
+		priv->is_bitrate_limited = true;
+	} else if (ret != -EINVAL) {
+		netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
+	}
+}
+EXPORT_SYMBOL(of_can_transceiver);
+#endif
+
 /*
  * Common close function for cleanup before the device gets closed.
  *
@@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 					priv->bitrate_const_cnt);
 		if (err)
 			return err;
+
+		if (priv->is_bitrate_limited &&
+		    bt.bitrate > priv->max_bitrate) {
+			netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
+				   priv->max_bitrate);
+			return -EINVAL;
+		}
+
 		memcpy(&priv->bittiming, &bt, sizeof(bt));
 
 		if (priv->do_set_bittiming) {
@@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 					priv->data_bitrate_const_cnt);
 		if (err)
 			return err;
+
+		if (priv->is_bitrate_limited &&
+		    dbt.bitrate > priv->max_bitrate) {
+			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
+				   priv->max_bitrate);
+			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..5519f59 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;
 
+	unsigned int max_bitrate;
+	bool is_bitrate_limited;
+
 	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(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.9.4.dirty

[PATCH v4 2/4] dt-bindings: can: can-transceiver: Document new binding

[Franklin S Cooper Jr]

Add documentation to describe usage of the new can-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 4 changes:
Drop unit address.
Switch from using fixed-transceiver to can-transceiver

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

diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 0000000..2c31dc0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN 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 "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate:	a positive non 0 value that determines the max
+		speed that CAN/CAN-FD can run. Any other value
+		will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+	....
+	can-transceiver@ {
+		max-bitrate = <5000000>;
+	};
+	...
+};
-- 
2.9.4.dirty

[PATCH v4 3/4] dt-bindings: can: m_can: Document new can transceiver binding

[Franklin S Cooper Jr]

Add information regarding can-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>
---
Drop unit address.
Switch from using fixed-transceiver to can-transceiver
Indicate that can-transceiver is an optional subnode not a property.

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

diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..ee90aac 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 Subnode:
+- can-transceiver	: Can-transceiver subnode describing maximum speed
+			  that can be used for CAN/CAN-FD modes. See
+			  Documentation/devicetree/bindings/net/can/can-transceiver.txt
+			  for details.
 Example:
 SoC dtsi:
 m_can1: can@020e8000 {
@@ -64,4 +69,8 @@ Board dts:
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_m_can1>;
 	status = "enabled";
+
+	can-transceiver@ {
+		max-bitrate = <5000000>;
+	};
 };
-- 
2.9.4.dirty

[PATCH v4 4/4] can: m_can: Add call to of_can_transceiver

[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..f72116e 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(dev);
+
 	dev_info(&pdev->dev, "%s device registered (irq=%d, version=%d)\n",
 		 KBUILD_MODNAME, dev->irq, priv->version);
 
-- 
2.9.4.dirty

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

[Sergei Shtylyov]

Hello!

On 8/10/2017 3:59 AM, Franklin S Cooper Jr wrote:

> 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 can-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 4 changes:
> Used can-transceiver instead of fixed-transceiver.
> 
>   drivers/net/can/dev.c   | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/can/dev.h |  5 +++++
>   2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 365a8cc..372108f 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
[...]
> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>   }
>   EXPORT_SYMBOL_GPL(open_candev);
>   
> +#ifdef CONFIG_OF
> +/*
> + * Common function that can be used to understand the limitation of
> + * a transceiver when it provides no means to determine these limitations
> + * at runtime.
> + */
> +void of_can_transceiver(struct net_device *dev)
> +{
> +	struct device_node *dn;
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct device_node *np;
> +	unsigned int max_bitrate;
> +	int ret;
> +
> +	np = dev->dev.parent->of_node;

    I'd do that as an initializer.

> +
> +	dn = of_get_child_by_name(np, "can-transceiver");
> +	if (!dn)
> +		return;
> +
> +	max_bitrate = 0;
> +	ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);

    I'd initialize max_bitrate to 0 as iff of_property_read_u32() fails, it'll 
leave the variable unset...

> +
> +	if (max_bitrate > 0) {

    You risk checking unset variable here.

> +		priv->max_bitrate = max_bitrate;
> +		priv->is_bitrate_limited = true;
> +	} else if (ret != -EINVAL) {
> +		netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
> +	}
> +}
> +EXPORT_SYMBOL(of_can_transceiver);
> +#endif
> +
>   /*
>    * Common close function for cleanup before the device gets closed.
>    *
[...]

MBR, Sergei

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

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 5015 bytes --]

Hello,

sorry for stepping in late....

On 08/10/2017 02:59 AM, Franklin S Cooper Jr wrote:
> 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 can-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 4 changes:
> Used can-transceiver instead of fixed-transceiver.
> 
>  drivers/net/can/dev.c   | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h |  5 +++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 365a8cc..372108f 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,39 @@ int open_candev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(open_candev);
>  
> +#ifdef CONFIG_OF
> +/*
> + * Common function that can be used to understand the limitation of
> + * a transceiver when it provides no means to determine these limitations
> + * at runtime.
> + */
> +void of_can_transceiver(struct net_device *dev)
> +{
> +	struct device_node *dn;
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct device_node *np;
> +	unsigned int max_bitrate;
> +	int ret;
> +
> +	np = dev->dev.parent->of_node;
> +
> +	dn = of_get_child_by_name(np, "can-transceiver");
> +	if (!dn)
> +		return;
> +
> +	max_bitrate = 0;
> +	ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);

IIRC the last argument is only modified in case of no error, so what about:
	ret = of_property_read_u32(dn, "max-bitrate",
			&priv->max_bitrate);

> +
> +	if (max_bitrate > 0) {
> +		priv->max_bitrate = max_bitrate;
> +		priv->is_bitrate_limited = true;

Do we need is_bitrate_limited...

> +	} else if (ret != -EINVAL) {
> +		netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
> +	}
> +}
> +EXPORT_SYMBOL(of_can_transceiver);
> +#endif
> +
>  /*
>   * Common close function for cleanup before the device gets closed.
>   *
> @@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  					priv->bitrate_const_cnt);
>  		if (err)
>  			return err;
> +
> +		if (priv->is_bitrate_limited &&
> +		    bt.bitrate > priv->max_bitrate) {

...can we just use priv->max_bitrate?

	if (priv->max_bitrate && bt.bitrate > priv->max_bitrate)

> +			netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
> +				   priv->max_bitrate);
> +			return -EINVAL;
> +		}
> +
>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
>  
>  		if (priv->do_set_bittiming) {
> @@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  					priv->data_bitrate_const_cnt);
>  		if (err)
>  			return err;
> +
> +		if (priv->is_bitrate_limited &&
> +		    dbt.bitrate > priv->max_bitrate) {
> +			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
> +				   priv->max_bitrate);
> +			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..5519f59 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;
>  
> +	unsigned int max_bitrate;
> +	bool is_bitrate_limited;
> +
>  	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(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);
> 

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

Re: [PATCH v4 2/4] dt-bindings: can: can-transceiver: Document new binding

[Rob Herring]

On Wed, Aug 09, 2017 at 07:59:14PM -0500, Franklin S Cooper Jr wrote:
> Add documentation to describe usage of the new can-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 4 changes:
> Drop unit address.
> Switch from using fixed-transceiver to can-transceiver
> 
>  .../bindings/net/can/can-transceiver.txt           | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
> new file mode 100644
> index 0000000..2c31dc0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
> @@ -0,0 +1,24 @@
> +Generic CAN 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 "can-transceiver" node can be used.
> +
> +Required Properties:
> + max-bitrate:	a positive non 0 value that determines the max
> +		speed that CAN/CAN-FD can run. Any other value
> +		will be ignored.
> +
> +Examples:
> +
> +Based on Texas Instrument's TCAN1042HGV CAN Transceiver
> +
> +m_can0 {
> +	....
> +	can-transceiver@ {

Need to drop the @ too. With that,

Acked-by: Rob Herring <robh@kernel.org>


> +		max-bitrate = <5000000>;
> +	};
> +	...
> +};
> -- 
> 2.9.4.dirty
> 

Re: [PATCH v4 3/4] dt-bindings: can: m_can: Document new can transceiver binding

[Rob Herring]

On Wed, Aug 09, 2017 at 07:59:15PM -0500, Franklin S Cooper Jr wrote:
> Add information regarding can-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>
> ---
> Drop unit address.
> Switch from using fixed-transceiver to can-transceiver
> Indicate that can-transceiver is an optional subnode not a property.
> 
>  Documentation/devicetree/bindings/net/can/m_can.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> index 9e33177..ee90aac 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 Subnode:
> +- can-transceiver	: Can-transceiver subnode describing maximum speed
> +			  that can be used for CAN/CAN-FD modes. See
> +			  Documentation/devicetree/bindings/net/can/can-transceiver.txt
> +			  for details.
>  Example:
>  SoC dtsi:
>  m_can1: can@020e8000 {
> @@ -64,4 +69,8 @@ Board dts:
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_m_can1>;
>  	status = "enabled";
> +
> +	can-transceiver@ {

ditto

Acked-by: Rob Herring <robh@kernel.org>

> +		max-bitrate = <5000000>;
> +	};
>  };
> -- 
> 2.9.4.dirty
> 

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

[Franklin S Cooper Jr]

On 08/10/2017 05:05 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 8/10/2017 3:59 AM, Franklin S Cooper Jr wrote:
> 
>> 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 can-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 4 changes:
>> Used can-transceiver instead of fixed-transceiver.
>>
>>   drivers/net/can/dev.c   | 50
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/can/dev.h |  5 +++++
>>   2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..372108f 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
> [...]
>> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(open_candev);
>>   +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these
>> limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> +    struct device_node *dn;
>> +    struct can_priv *priv = netdev_priv(dev);
>> +    struct device_node *np;
>> +    unsigned int max_bitrate;
>> +    int ret;
>> +
>> +    np = dev->dev.parent->of_node;
> 
>    I'd do that as an initializer.

Ok
> 
>> +
>> +    dn = of_get_child_by_name(np, "can-transceiver");
>> +    if (!dn)
>> +        return;
>> +
>> +    max_bitrate = 0;
>> +    ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
> 
>    I'd initialize max_bitrate to 0 as iff of_property_read_u32() fails,
> it'll leave the variable unset...>
>> +
>> +    if (max_bitrate > 0) {
> 
>    You risk checking unset variable here.

Four lines up I am already setting max_bitrate to a default value of 0.

> 
>> +        priv->max_bitrate = max_bitrate;
>> +        priv->is_bitrate_limited = true;
>> +    } else if (ret != -EINVAL) {
>> +        netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
>> +    }
>> +}
>> +EXPORT_SYMBOL(of_can_transceiver);
>> +#endif
>> +
>>   /*
>>    * Common close function for cleanup before the device gets closed.
>>    *
> [...]
> 
> MBR, Sergei

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

[Franklin S Cooper Jr]

Hi,
On 08/10/2017 10:29 AM, Marc Kleine-Budde wrote:
> Hello,
> 
> sorry for stepping in late....
> 
> On 08/10/2017 02:59 AM, Franklin S Cooper Jr wrote:
>> 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 can-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 4 changes:
>> Used can-transceiver instead of fixed-transceiver.
>>
>>  drivers/net/can/dev.c   | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/can/dev.h |  5 +++++
>>  2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..372108f 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,39 @@ int open_candev(struct net_device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(open_candev);
>>  
>> +#ifdef CONFIG_OF
>> +/*
>> + * Common function that can be used to understand the limitation of
>> + * a transceiver when it provides no means to determine these limitations
>> + * at runtime.
>> + */
>> +void of_can_transceiver(struct net_device *dev)
>> +{
>> +	struct device_node *dn;
>> +	struct can_priv *priv = netdev_priv(dev);
>> +	struct device_node *np;
>> +	unsigned int max_bitrate;
>> +	int ret;
>> +
>> +	np = dev->dev.parent->of_node;
>> +
>> +	dn = of_get_child_by_name(np, "can-transceiver");
>> +	if (!dn)
>> +		return;
>> +
>> +	max_bitrate = 0;
>> +	ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
> 
> IIRC the last argument is only modified in case of no error, so what about:
> 	ret = of_property_read_u32(dn, "max-bitrate",
> 			&priv->max_bitrate);

That works
> 
>> +
>> +	if (max_bitrate > 0) {
>> +		priv->max_bitrate = max_bitrate;
>> +		priv->is_bitrate_limited = true;
> 
> Do we need is_bitrate_limited...

No.
> 
>> +	} else if (ret != -EINVAL) {
>> +		netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
>> +	}
>> +}
>> +EXPORT_SYMBOL(of_can_transceiver);
>> +#endif
>> +
>>  /*
>>   * Common close function for cleanup before the device gets closed.
>>   *
>> @@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>>  					priv->bitrate_const_cnt);
>>  		if (err)
>>  			return err;
>> +
>> +		if (priv->is_bitrate_limited &&
>> +		    bt.bitrate > priv->max_bitrate) {
> 
> ...can we just use priv->max_bitrate?

Yes

Thanks for reviewing and for your feedback.
> 
> 	if (priv->max_bitrate && bt.bitrate > priv->max_bitrate)
> 
>> +			netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
>> +				   priv->max_bitrate);
>> +			return -EINVAL;
>> +		}
>> +
>>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
>>  
>>  		if (priv->do_set_bittiming) {
>> @@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>>  					priv->data_bitrate_const_cnt);
>>  		if (err)
>>  			return err;
>> +
>> +		if (priv->is_bitrate_limited &&
>> +		    dbt.bitrate > priv->max_bitrate) {
>> +			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
>> +				   priv->max_bitrate);
>> +			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..5519f59 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;
>>  
>> +	unsigned int max_bitrate;
>> +	bool is_bitrate_limited;
>> +
>>  	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(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);
>>
> 
> Marc
> 

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

[Sergei Shtylyov]

Hello!

On 08/17/2017 08:54 PM, Franklin S Cooper Jr wrote:

>>> 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 can-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 4 changes:
>>> Used can-transceiver instead of fixed-transceiver.
>>>
>>>    drivers/net/can/dev.c   | 50
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>    include/linux/can/dev.h |  5 +++++
>>>    2 files changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index 365a8cc..372108f 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>> [...]
>>> @@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
>>>    }
>>>    EXPORT_SYMBOL_GPL(open_candev);
>>>    +#ifdef CONFIG_OF
>>> +/*
>>> + * Common function that can be used to understand the limitation of
>>> + * a transceiver when it provides no means to determine these
>>> limitations
>>> + * at runtime.
>>> + */
>>> +void of_can_transceiver(struct net_device *dev)
>>> +{
>>> +    struct device_node *dn;
>>> +    struct can_priv *priv = netdev_priv(dev);
>>> +    struct device_node *np;
>>> +    unsigned int max_bitrate;
>>> +    int ret;
>>> +
>>> +    np = dev->dev.parent->of_node;
>>
>>     I'd do that as an initializer.
> 
> Ok
>>
>>> +
>>> +    dn = of_get_child_by_name(np, "can-transceiver");
>>> +    if (!dn)
>>> +        return;
>>> +
>>> +    max_bitrate = 0;
>>> +    ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
>>
>>     I'd initialize max_bitrate to 0 as iff of_property_read_u32() fails,
>> it'll leave the variable unset...>
>>> +
>>> +    if (max_bitrate > 0) {
>>
>>     You risk checking unset variable here.
> 
> Four lines up I am already setting max_bitrate to a default value of 0.

    Oops, sorry. Don't know how I missed that. Better done as initializer too 
though...

MBR, Sergei