Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1692277 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] can: Add new binding to limit bit rate used Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-20 01:40 +0200
    [PATCH 3/4] can: m_can: Update documentation to mention new fixed transceiver binding Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-20 01:40 +0200
      Re: [PATCH 3/4] can: m_can: Update documentation to mention new  fixed transceiver binding Rob Herring <robh@kernel.org> - 2017-07-24 22:00 +0200
    [PATCH 1/4] can: dev: Add support for limiting configured bitrate Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-20 01:40 +0200
      Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate Oliver Hartkopp <socketcan@hartkopp.net> - 2017-07-20 09:50 +0200
        Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-20 18:00 +0200
          Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be> - 2017-07-26 22:30 +0200
            Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-27 03:50 +0200
      Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-07-20 12:00 +0200
        Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-20 18:10 +0200
    [PATCH 4/4] can: m_can: Add call to of_transceiver_is_fixed Franklin S Cooper Jr <fcooper@ti.com> - 2017-07-20 01:40 +0200

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

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.

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_transceiver_is_fixed

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

-- 
2.10.0

[toc] | [next] | [standalone]

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

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>
---
 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..4440e4b 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@0 {
+		max-arbitration-speed = <1000000>;
+		max-data-speed = <5000000>;
+	};
 };
-- 
2.10.0

[toc] | [prev] | [next] | [standalone]

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

On Wed, Jul 19, 2017 at 06:36:53PM -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>
> ---
>  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..4440e4b 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@0 {

unit-address should have corresponding reg prop or unit-address should 
be dropped.

> +		max-arbitration-speed = <1000000>;
> +		max-data-speed = <5000000>;
> +	};
>  };
> -- 
> 2.10.0
> 

[toc] | [prev] | [next] | [standalone]

#1692283 — [PATCH 1/4] can: dev: Add support for limiting configured bitrate

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>
---
 drivers/net/can/dev.c   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h |  5 +++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..fbab87d 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"
@@ -806,6 +807,21 @@ int open_candev(struct net_device *dev)
 		return -EINVAL;
 	}
 
+	if (priv->max_trans_arbitration_speed > 0 &&
+	    priv->bittiming.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;
+	}
+
+	if (priv->max_trans_data_speed  >= 0 &&
+	    (priv->ctrlmode & CAN_CTRLMODE_FD) &&
+	    (priv->data_bittiming.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;
+	}
+
 	/* Switch carrier on if device was stopped while in bus-off state */
 	if (!netif_carrier_ok(dev))
 		netif_carrier_on(dev);
@@ -814,6 +830,38 @@ int open_candev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(open_candev);
 
+#ifdef CONFIG_OF
+void of_transceiver_is_fixed(struct net_device *dev)
+{
+	struct device_node *dn;
+	struct can_priv *priv = netdev_priv(dev);
+	u32 max_frequency;
+	struct device_node *np;
+
+	np = dev->dev.parent->of_node;
+
+	/* New binding */
+	dn = of_get_child_by_name(np, "fixed-transceiver");
+	if (!dn)
+		return;
+
+	of_property_read_u32(dn, "max-arbitration-speed", &max_frequency);
+
+	if (max_frequency > 0)
+		priv->max_trans_arbitration_speed = max_frequency;
+	else
+		priv->max_trans_arbitration_speed = -1;
+
+	of_property_read_u32(dn, "max-data-speed", &max_frequency);
+
+	if (max_frequency >= 0)
+		priv->max_trans_data_speed = max_frequency;
+	else
+		priv->max_trans_data_speed = -1;
+}
+EXPORT_SYMBOL(of_transceiver_is_fixed);
+#endif
+
 /*
  * Common close function for cleanup before the device gets closed.
  *
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..aec72b5 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -69,6 +69,9 @@ struct can_priv {
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
 
+	unsigned int max_trans_arbitration_speed;
+	unsigned int max_trans_data_speed;
+
 #ifdef CONFIG_CAN_LEDS
 	struct led_trigger *tx_led_trig;
 	char tx_led_trig_name[CAN_LED_NAME_SZ];
@@ -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_transceiver_is_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

[toc] | [prev] | [next] | [standalone]

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

Hi Franklin,

On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:

> +#ifdef CONFIG_OF
> +void of_transceiver_is_fixed(struct net_device *dev)
> +{

(..)

> +}
> +EXPORT_SYMBOL(of_transceiver_is_fixed);
> +#endif

I'm not sure about the naming here.

As this is a CAN transceiver related option it should be named accordingly:

E.g.

	can_transceiver_is_fixed
	of_can_transceiver_is_fixed
	...

Especially as it is defined in include/linux/can/dev.h

Regards,
Oliver

[toc] | [prev] | [next] | [standalone]

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

Hi Oliver
On 07/20/2017 02:43 AM, Oliver Hartkopp wrote:
> Hi Franklin,
> 
> On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:
> 
>> +#ifdef CONFIG_OF
>> +void of_transceiver_is_fixed(struct net_device *dev)
>> +{
> 
> (..)
> 
>> +}
>> +EXPORT_SYMBOL(of_transceiver_is_fixed);
>> +#endif
> 
> I'm not sure about the naming here.
> 
> As this is a CAN transceiver related option it should be named accordingly:
> 
> E.g.
> 
>     can_transceiver_is_fixed
>     of_can_transceiver_is_fixed
>     ...
> 
> Especially as it is defined in include/linux/can/dev.h

Thanks for the feedback. I'll go with of_can_transceiver_is_fixed

> 
> Regards,
> Oliver
> 
> 

[toc] | [prev] | [next] | [standalone]

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

Hi,

I know my response is late ...

> Hi Oliver
> On 07/20/2017 02:43 AM, Oliver Hartkopp wrote:
> > Hi Franklin,
> > 
> > On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:
> > 
> >> +#ifdef CONFIG_OF
> >> +void of_transceiver_is_fixed(struct net_device *dev)
> >> +{
> > 
> > (..)
> > 
> >> +}
> >> +EXPORT_SYMBOL(of_transceiver_is_fixed);
> >> +#endif
> > 
> > I'm not sure about the naming here.
> > 
> > As this is a CAN transceiver related option it should be named accordingly:

I contest the the name too:
1) the can transceiver isn't fixed at all, it limited to the higher
bitrates.

2) of_can_transceiver_is_fixed suggests to test if a transceiver is
fixed, it does not suggest to load some properties from the device tree.
of_can_load_transceiver looks way more clear to me.

That's my opinion.
The important things, like the contents of the functions, look good.

Kind regards,
Kurt Van Dijck

[toc] | [prev] | [next] | [standalone]

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

Hi Kurt,
On 07/26/2017 03:04 PM, Kurt Van Dijck wrote:
> Hi,
> 
> I know my response is late ...
> 
>> Hi Oliver
>> On 07/20/2017 02:43 AM, Oliver Hartkopp wrote:
>>> Hi Franklin,
>>>
>>> On 07/20/2017 01:36 AM, Franklin S Cooper Jr wrote:
>>>
>>>> +#ifdef CONFIG_OF
>>>> +void of_transceiver_is_fixed(struct net_device *dev)
>>>> +{
>>>
>>> (..)
>>>
>>>> +}
>>>> +EXPORT_SYMBOL(of_transceiver_is_fixed);
>>>> +#endif
>>>
>>> I'm not sure about the naming here.
>>>
>>> As this is a CAN transceiver related option it should be named accordingly:
> 
> I contest the the name too:
> 1) the can transceiver isn't fixed at all, it limited to the higher
> bitrates.

Its "possible" that this subnode may have additional properties beyond
bitrates in the future. But your right as of now it is specifically
addressing max bit rates. The naming of this function and subnode is
based on "fixed-link". So "fixed" is just implying that certain
properties can't be changed.

> 
> 2) of_can_transceiver_is_fixed suggests to test if a transceiver is
> fixed, it does not suggest to load some properties from the device tree.
> of_can_load_transceiver looks way more clear to me.

I address this partially in my rev 2 that I've already sent. I'm now
using of_can_transceiver_fixed. The fact its of_ already implies it is
loading properties from device tree. So I don't think
of_can_load_transceiver really makes things clearer.

> 
> That's my opinion.
> The important things, like the contents of the functions, look good.

You mind throwing your two cents in the thread for my latest patch?
Specifically the conversation regarding naming the properties. A couple
of people prefer to not use "arbitration" in one of the property names.
Currently I believe there are two options on property names that can be
used and I'm open to a majority vote on which one to go with.

> 
> Kind regards,
> Kurt Van Dijck
> 

[toc] | [prev] | [next] | [standalone]

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

Hello!

On 7/20/2017 2:36 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 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>
> ---
>   drivers/net/can/dev.c   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/can/dev.h |  5 +++++
>   2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 365a8cc..fbab87d 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
[...]
> @@ -814,6 +830,38 @@ int open_candev(struct net_device *dev)
>   }
>   EXPORT_SYMBOL_GPL(open_candev);
>   
> +#ifdef CONFIG_OF
> +void of_transceiver_is_fixed(struct net_device *dev)

    Strange name for a *void* function...
    Also, I think 'struct net_device *' variables are typically called 'ndev'.

> +{
> +	struct device_node *dn;
> +	struct can_priv *priv = netdev_priv(dev);
> +	u32 max_frequency;
> +	struct device_node *np;
> +
> +	np = dev->dev.parent->of_node;
> +
> +	/* New binding */
> +	dn = of_get_child_by_name(np, "fixed-transceiver");
> +	if (!dn)
> +		return;
> +
> +	of_property_read_u32(dn, "max-arbitration-speed", &max_frequency);

    In case this function fails, 'max_frequency' will have no value -- you'd 
better initialize it...

> +
> +	if (max_frequency > 0)
> +		priv->max_trans_arbitration_speed = max_frequency;
> +	else
> +		priv->max_trans_arbitration_speed = -1;
> +
> +	of_property_read_u32(dn, "max-data-speed", &max_frequency);

    Again, when that function fails, the variable will keep the value from the 
previous call...

[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

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

On 07/20/2017 04:52 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 7/20/2017 2:36 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 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>
>> ---
>>   drivers/net/can/dev.c   | 48
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/can/dev.h |  5 +++++
>>   2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index 365a8cc..fbab87d 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
> [...]
>> @@ -814,6 +830,38 @@ int open_candev(struct net_device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(open_candev);
>>   +#ifdef CONFIG_OF
>> +void of_transceiver_is_fixed(struct net_device *dev)
> 
>    Strange name for a *void* function...

Ok I see what you mean since I'm not actually returning anything. I'll
go with of_can_transceiver_fixed based on your comment also Oliver's
suggestion.

>    Also, I think 'struct net_device *' variables are typically called
> 'ndev'.

All other functions within this file uses struct net_device *dev. So I'm
just following the style currently used.

> 
>> +{
>> +    struct device_node *dn;
>> +    struct can_priv *priv = netdev_priv(dev);
>> +    u32 max_frequency;
>> +    struct device_node *np;
>> +
>> +    np = dev->dev.parent->of_node;
>> +
>> +    /* New binding */
>> +    dn = of_get_child_by_name(np, "fixed-transceiver");
>> +    if (!dn)
>> +        return;
>> +
>> +    of_property_read_u32(dn, "max-arbitration-speed", &max_frequency);
> 
>    In case this function fails, 'max_frequency' will have no value --
> you'd better initialize it...

Thanks for catching this. Will fix.

> 
>> +
>> +    if (max_frequency > 0)
>> +        priv->max_trans_arbitration_speed = max_frequency;
>> +    else
>> +        priv->max_trans_arbitration_speed = -1;
>> +
>> +    of_property_read_u32(dn, "max-data-speed", &max_frequency);
> 
>    Again, when that function fails, the variable will keep the value
> from the previous call...

Will fix.

> 
> [...]
> 
> MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1692287 — [PATCH 4/4] can: m_can: Add call to of_transceiver_is_fixed

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web