[PATCH] Enabling rcar_can changelink support

[PATCH] Enabling rcar_can changelink support

[Bogdan Mirea]

Added rcar_can_set_bittiming as can.do_set_bittiming callback that will
be called from can_changelink generic callback when link state is
changed.

This enables set bittiming support:
	ip link set can0 type can bitrate 500000 triple-sampling on

Signed-off-by: Bogdan Mirea <Bogdan-Stefan_mirea@mentor.com>
Signed-off-by: Eugen Hristev <Eugen_hristev@mentor.com>
---
 drivers/net/can/rcar_can.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 788459f..cfabb95 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -419,7 +419,7 @@ static irqreturn_t rcar_can_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void rcar_can_set_bittiming(struct net_device *dev)
+static int rcar_can_set_bittiming(struct net_device *dev)
 {
 	struct rcar_can_priv *priv = netdev_priv(dev);
 	struct can_bittiming *bt = &priv->can.bittiming;
@@ -433,6 +433,8 @@ static void rcar_can_set_bittiming(struct net_device *dev)
 	 * read/write (but not on 8-bit, contrary to the manuals)...
 	 */
 	writel((bcr << 8) | priv->clock_select, &priv->regs->bcr);
+
+	return 0;
 }
 
 static void rcar_can_start(struct net_device *ndev)
@@ -809,6 +811,7 @@ static int rcar_can_probe(struct platform_device *pdev)
 	priv->clock_select = clock_select;
 	priv->can.clock.freq = clk_get_rate(priv->can_clk);
 	priv->can.bittiming_const = &rcar_can_bittiming_const;
+	priv->can.do_set_bittiming = &rcar_can_set_bittiming;
 	priv->can.do_set_mode = rcar_can_do_set_mode;
 	priv->can.do_get_berr_counter = rcar_can_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
-- 
1.9.1

[PATCH] Enabling rcar_can changelink support

[Bogdan Mirea]

Added rcar_can_set_bittiming as can.do_set_bittiming callback that will
be called from can_changelink generic callback when link state is
changed.

This enables set bittiming support:
	ip link set can0 type can bitrate 500000 triple-sampling on

Signed-off-by: Bogdan Mirea <Bogdan-Stefan_mirea@mentor.com>
Signed-off-by: Eugen Hristev <Eugen_hristev@mentor.com>
---
 drivers/net/can/rcar_can.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
index 788459f..cfabb95 100644
--- a/drivers/net/can/rcar/rcar_can.c
+++ b/drivers/net/can/rcar/rcar_can.c
@@ -419,7 +419,7 @@ static irqreturn_t rcar_can_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void rcar_can_set_bittiming(struct net_device *dev)
+static int rcar_can_set_bittiming(struct net_device *dev)
 {
 	struct rcar_can_priv *priv = netdev_priv(dev);
 	struct can_bittiming *bt = &priv->can.bittiming;
@@ -433,6 +433,8 @@ static void rcar_can_set_bittiming(struct net_device *dev)
 	 * read/write (but not on 8-bit, contrary to the manuals)...
 	 */
 	writel((bcr << 8) | priv->clock_select, &priv->regs->bcr);
+
+	return 0;
 }
 
 static void rcar_can_start(struct net_device *ndev)
@@ -809,6 +811,7 @@ static int rcar_can_probe(struct platform_device *pdev)
 	priv->clock_select = clock_select;
 	priv->can.clock.freq = clk_get_rate(priv->can_clk);
 	priv->can.bittiming_const = &rcar_can_bittiming_const;
+	priv->can.do_set_bittiming = &rcar_can_set_bittiming;
 	priv->can.do_set_mode = rcar_can_do_set_mode;
 	priv->can.do_get_berr_counter = rcar_can_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
-- 
1.9.1

Re: [PATCH] Enabling rcar_can changelink support

[Marc Kleine-Budde]

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

On 04/29/2017 09:48 PM, Bogdan Mirea wrote:
> Added rcar_can_set_bittiming as can.do_set_bittiming callback that will
> be called from can_changelink generic callback when link state is
> changed.
> 
> This enables set bittiming support:
> 	ip link set can0 type can bitrate 500000 triple-sampling on

Why is this needed? rcar_can_set_bittiming() is called during
rcar_can_open().

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] Enabling rcar_can changelink support

[Mirea, Bogdan-Stefan]

Hello Marc,

> On 04/29/2017 09:48 PM, Bogdan Mirea wrote:
> > Added rcar_can_set_bittiming as can.do_set_bittiming callback that
> > will be called from can_changelink generic callback when link state
> > is 
> > changed.
> > 
> > This enables set bittiming support:
> >     ip link set can0 type can bitrate 500000 triple-sampling on
> 
> Why is this needed? rcar_can_set_bittiming() is called during
> rcar_can_open().

I know that rcar_can_set_bittiming() is called during rcar_can_open()
and setting bittiming works fine when setting can up with a command
like:
(1st approah)
    $ip link set can0 up type can bitrate 500000

But most of tutorials online[1] are presenting as a TODO the following
pair of commands:
(2nd approah)
    $ip link set can0 type can bitrate 500000
    $ip link set up can0

And in this 2nd approah, the first command will fail since it will call
can_changelink which on its turn will try calling .do_set_bittiming
callback and the former callback is not defined for rcar_can.

I don't say this is a must-have feature, since calling iproute2 with
"set bitrate" and "set can up"  as in 1st approach will work just fine,
will work just fine, but it is a nice to have feature for the second
approach of setting bitrate.
Also peak_usb pcan[2] uses this second approach (the changelink one) and
this patch updates rcar_can for it.

Best Regards,
Bogdan

[1] http://elinux.org/Bringing_CAN_interface_up
[2]
http://lxr.free-electrons.com/source/drivers/net/can/usb/peak_usb/pcan_usb.c#L900

Re: [PATCH] Enabling rcar_can changelink support

[Marc Kleine-Budde]

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

On 04/30/2017 01:33 PM, Mirea, Bogdan-Stefan wrote:
> Hello Marc,
> 
>> On 04/29/2017 09:48 PM, Bogdan Mirea wrote:
>>> Added rcar_can_set_bittiming as can.do_set_bittiming callback that
>>> will be called from can_changelink generic callback when link state
>>> is 
>>> changed.
>>>
>>> This enables set bittiming support:
>>>     ip link set can0 type can bitrate 500000 triple-sampling on
>>
>> Why is this needed? rcar_can_set_bittiming() is called during
>> rcar_can_open().
> 
> I know that rcar_can_set_bittiming() is called during rcar_can_open()
> and setting bittiming works fine when setting can up with a command
> like:
> (1st approah)
>     $ip link set can0 up type can bitrate 500000
> 
> But most of tutorials online[1] are presenting as a TODO the following
> pair of commands:
> (2nd approah)
>     $ip link set can0 type can bitrate 500000
>     $ip link set up can0
> 
> And in this 2nd approah, the first command will fail since it will call
> can_changelink which on its turn will try calling .do_set_bittiming
> callback and the former callback is not defined for rcar_can.

Why should it fail?

>                 memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
>                 err = can_get_bittiming(dev, &bt,
>                                         priv->bittiming_const,
>                                         priv->bitrate_const,
>                                         priv->bitrate_const_cnt);
>                 if (err)
>                         return err;
>                 memcpy(&priv->bittiming, &bt, sizeof(bt));

Here the bittiming settings are copied to priv->bittiming
> 
>                 if (priv->do_set_bittiming) {
>                         /* Finally, set the bit-timing registers */
>                         err = priv->do_set_bittiming(dev);
>                         if (err)
>                                 return err;
>                 }

and it will only call the set_bittiming if it's defined.

> I don't say this is a must-have feature, since calling iproute2 with
> "set bitrate" and "set can up"  as in 1st approach will work just fine,
> will work just fine, but it is a nice to have feature for the second
> approach of setting bitrate.
> Also peak_usb pcan[2] uses this second approach (the changelink one) and
> this patch updates rcar_can for it.

Sorry I still don't get why this is necessary.

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] Enabling rcar_can changelink support

[Mirea, Bogdan-Stefan]

On April 30, 2017 10:31 PM Marc Kleine-Budde wrote:
> >                 memcpy(&priv->bittiming, &bt, sizeof(bt));
>
> Here the bittiming settings are copied to priv->bittiming

Yes Marc, you are right, I don't know how did I noticed an error which it isn't
since the bittiming structure is filled here, in can_changelink function...
My bad. 

Best Regards,
Bogdan