[PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

Packages that fails to be transmitted in one-shot mode are never the
less echoed back as sja1000 does not give any indication when a single
transmission has failed.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/net/can/sja1000/sja1000.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4c4f33d..cc771d9 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
 	struct sja1000_priv *priv = netdev_priv(dev);
 	unsigned char status = priv->read_reg(priv, REG_MOD);
 	int i;
+	u8 normal = priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY ? MOD_LOM : 0;
 
 	for (i = 0; i < 100; i++) {
 		/* check reset bit */
@@ -156,7 +157,7 @@ static void set_normal_mode(struct net_device *dev)
 		}
 
 		/* set chip to normal mode */
-		priv->write_reg(priv, REG_MOD, 0x00);
+		priv->write_reg(priv, REG_MOD, normal);
 		udelay(10);
 		status = priv->read_reg(priv, REG_MOD);
 	}
@@ -278,6 +279,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 	canid_t id;
 	uint8_t dreg;
 	int i;
+	u8 oneshot;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
@@ -310,7 +312,8 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 
 	can_put_echo_skb(skb, dev, 0);
 
-	sja1000_write_cmdreg(priv, CMD_TR);
+	oneshot = (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) ? CMD_AT : 0;
+	sja1000_write_cmdreg(priv, CMD_TR | oneshot);
 
 	return NETDEV_TX_OK;
 }
@@ -605,7 +608,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
 	priv->can.do_set_mode = sja1000_set_mode;
 	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
-		CAN_CTRLMODE_BERR_REPORTING;
+		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
+		CAN_CTRLMODE_ONE_SHOT;
 
 	spin_lock_init(&priv->cmdreg_lock);
 
-- 
1.7.0.4

Re: [PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[Marc Kleine-Budde]

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

On 09/17/2012 05:58 PM, Andreas Larsson wrote:
> Packages that fails to be transmitted in one-shot mode are never the
> less echoed back as sja1000 does not give any indication when a single
> transmission has failed.

Uhh... can any of the SJA1000 gurus comment to this?

Some write-more-readable-code comments inline :)

> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
>  drivers/net/can/sja1000/sja1000.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 4c4f33d..cc771d9 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
>  	struct sja1000_priv *priv = netdev_priv(dev);
>  	unsigned char status = priv->read_reg(priv, REG_MOD);
>  	int i;
> +	u8 normal = priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY ? MOD_LOM : 0;

What about a more readable:
	u8 normal = 0;

	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
		normal = MOD_LOM;

>  
>  	for (i = 0; i < 100; i++) {
>  		/* check reset bit */
> @@ -156,7 +157,7 @@ static void set_normal_mode(struct net_device *dev)
>  		}
>  
>  		/* set chip to normal mode */
> -		priv->write_reg(priv, REG_MOD, 0x00);
> +		priv->write_reg(priv, REG_MOD, normal);
>  		udelay(10);
>  		status = priv->read_reg(priv, REG_MOD);
>  	}
> @@ -278,6 +279,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  	canid_t id;
>  	uint8_t dreg;
>  	int i;
> +	u8 oneshot;
>  
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
> @@ -310,7 +312,8 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  
>  	can_put_echo_skb(skb, dev, 0);
>  
> -	sja1000_write_cmdreg(priv, CMD_TR);
> +	oneshot = (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) ? CMD_AT : 0;
> +	sja1000_write_cmdreg(priv, CMD_TR | oneshot);

something simmilar here, too

>  	return NETDEV_TX_OK;
>  }
> @@ -605,7 +608,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
>  	priv->can.do_set_mode = sja1000_set_mode;
>  	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
> -		CAN_CTRLMODE_BERR_REPORTING;
> +		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
> +		CAN_CTRLMODE_ONE_SHOT;
>  
>  	spin_lock_init(&priv->cmdreg_lock);
>  
> 

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

Re: [PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[Wolfgang Grandegger]

On 09/18/2012 03:46 PM, Marc Kleine-Budde wrote:
> On 09/17/2012 05:58 PM, Andreas Larsson wrote:
>> Packages that fails to be transmitted in one-shot mode are never the
>> less echoed back as sja1000 does not give any indication when a single
>> transmission has failed.
> 
> Uhh... can any of the SJA1000 gurus comment to this?

The commit message is incomplete and confusing, I agree. The patch
obviously implements listen-only and one-shot mode. From the data sheet:

LOM FUNCTION:
  1: listen only; in this mode the CAN controller would
  give no acknowledge to the CAN-bus, even if a message is received
  successfully; the error counters are stopped at the current value
  normal

Single-shot:
  Setting the command bits CMR.0 (TR) and CMR.1 (AT) simultaneously
  results in sending the transmit message once. No re-transmission will
  be performed in the event of an error or arbitration lost
  (single-shot transmission).


> Some write-more-readable-code comments inline :)
> 
>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>> ---
>>  drivers/net/can/sja1000/sja1000.c |   10 +++++++---
>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>> index 4c4f33d..cc771d9 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
>>  	struct sja1000_priv *priv = netdev_priv(dev);
>>  	unsigned char status = priv->read_reg(priv, REG_MOD);
>>  	int i;
>> +	u8 normal = priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY ? MOD_LOM : 0;

> What about a more readable:
> 	u8 normal = 0;
> 
> 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> 		normal = MOD_LOM;
> 
>>  
>>  	for (i = 0; i < 100; i++) {
>>  		/* check reset bit */
>> @@ -156,7 +157,7 @@ static void set_normal_mode(struct net_device *dev)
>>  		}
>>  
>>  		/* set chip to normal mode */
>> -		priv->write_reg(priv, REG_MOD, 0x00);
>> +		priv->write_reg(priv, REG_MOD, normal);
>>  		udelay(10);
>>  		status = priv->read_reg(priv, REG_MOD);
>>  	}
>> @@ -278,6 +279,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>>  	canid_t id;
>>  	uint8_t dreg;
>>  	int i;
>> +	u8 oneshot;
>>  
>>  	if (can_dropped_invalid_skb(dev, skb))
>>  		return NETDEV_TX_OK;
>> @@ -310,7 +312,8 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>>  
>>  	can_put_echo_skb(skb, dev, 0);
>>  
>> -	sja1000_write_cmdreg(priv, CMD_TR);
>> +	oneshot = (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) ? CMD_AT : 0;
>> +	sja1000_write_cmdreg(priv, CMD_TR | oneshot);
> 
> something simmilar here, too

I would prefer here:

	u8 reg = CMD_TR; (or cmd or cmdreg)

	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
		reg |= CMD_AT;

A cached value seem overkill. Similar above.

Andreas, I'm curious what you are using the one-shot mode for.

Wolfgang.

Re: [PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

On 09/18/2012 04:11 PM, Wolfgang Grandegger wrote:
> On 09/18/2012 03:46 PM, Marc Kleine-Budde wrote:
>> On 09/17/2012 05:58 PM, Andreas Larsson wrote:
>>> Packages that fails to be transmitted in one-shot mode are never the
>>> less echoed back as sja1000 does not give any indication when a single
>>> transmission has failed.
>>
>> Uhh... can any of the SJA1000 gurus comment to this?
>
> The commit message is incomplete and confusing, I agree. The patch
> obviously implements listen-only and one-shot mode. From the data sheet:
>
> LOM FUNCTION:
>    1: listen only; in this mode the CAN controller would
>    give no acknowledge to the CAN-bus, even if a message is received
>    successfully; the error counters are stopped at the current value
>    normal
>
> Single-shot:
>    Setting the command bits CMR.0 (TR) and CMR.1 (AT) simultaneously
>    results in sending the transmit message once. No re-transmission will
>    be performed in the event of an error or arbitration lost
>    (single-shot transmission).

The limitation is that when a one-shot message transmission fails, a 
Transmit Interrupt is generated and as a consequence can_get_echo_skb is 
called. Thus a can frame is echoed back even when the transmission fails 
(instead of being discarded). When one-shot mode is enabled no other 
interrupts are generated on a failed transmission. Thus, as far as I can 
see, there is no way for the failed frames not to be echoed back locally 
in this case.

I'll try to word it better in the next version of the patch.

>> Some write-more-readable-code comments inline :)
>>
>>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>>> ---
>>>   drivers/net/can/sja1000/sja1000.c |   10 +++++++---
>>>   1 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>>> index 4c4f33d..cc771d9 100644
>>> --- a/drivers/net/can/sja1000/sja1000.c
>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>> @@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
>>>   	struct sja1000_priv *priv = netdev_priv(dev);
>>>   	unsigned char status = priv->read_reg(priv, REG_MOD);
>>>   	int i;
>>> +	u8 normal = priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY ? MOD_LOM : 0;
>
>> What about a more readable:
>> 	u8 normal = 0;
>>
>> 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
>> 		normal = MOD_LOM;
>>
>>>
>>>   	for (i = 0; i < 100; i++) {
>>>   		/* check reset bit */
>>> @@ -156,7 +157,7 @@ static void set_normal_mode(struct net_device *dev)
>>>   		}
>>>
>>>   		/* set chip to normal mode */
>>> -		priv->write_reg(priv, REG_MOD, 0x00);
>>> +		priv->write_reg(priv, REG_MOD, normal);
>>>   		udelay(10);
>>>   		status = priv->read_reg(priv, REG_MOD);
>>>   	}
>>> @@ -278,6 +279,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>>>   	canid_t id;
>>>   	uint8_t dreg;
>>>   	int i;
>>> +	u8 oneshot;
>>>
>>>   	if (can_dropped_invalid_skb(dev, skb))
>>>   		return NETDEV_TX_OK;
>>> @@ -310,7 +312,8 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>>>
>>>   	can_put_echo_skb(skb, dev, 0);
>>>
>>> -	sja1000_write_cmdreg(priv, CMD_TR);
>>> +	oneshot = (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) ? CMD_AT : 0;
>>> +	sja1000_write_cmdreg(priv, CMD_TR | oneshot);
>>
>> something simmilar here, too
>
> I would prefer here:
>
> 	u8 reg = CMD_TR; (or cmd or cmdreg)
>
> 	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> 		reg |= CMD_AT;
>
> A cached value seem overkill. Similar above.

Thanks for the input from both of you! I'll take care of that.


> Andreas, I'm curious what you are using the one-shot mode for.

In situations when the current view of some information is repeatedly 
being sent there is no use in trying to resend an old view of the 
information. In such a case it would be nice to allow for discarding 
failed frames using one-shot mode.

Cheers,
Andreas

Re: [PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[Ira W. Snyder]

On Tue, Sep 18, 2012 at 05:54:54PM +0200, Andreas Larsson wrote:
> On 09/18/2012 04:11 PM, Wolfgang Grandegger wrote:
> > On 09/18/2012 03:46 PM, Marc Kleine-Budde wrote:
> >> On 09/17/2012 05:58 PM, Andreas Larsson wrote:
> >>> Packages that fails to be transmitted in one-shot mode are never the
> >>> less echoed back as sja1000 does not give any indication when a single
> >>> transmission has failed.
> >>
> >> Uhh... can any of the SJA1000 gurus comment to this?
> >
> > The commit message is incomplete and confusing, I agree. The patch
> > obviously implements listen-only and one-shot mode. From the data sheet:
> >
> > LOM FUNCTION:
> >    1: listen only; in this mode the CAN controller would
> >    give no acknowledge to the CAN-bus, even if a message is received
> >    successfully; the error counters are stopped at the current value
> >    normal
> >
> > Single-shot:
> >    Setting the command bits CMR.0 (TR) and CMR.1 (AT) simultaneously
> >    results in sending the transmit message once. No re-transmission will
> >    be performed in the event of an error or arbitration lost
> >    (single-shot transmission).
> 
> The limitation is that when a one-shot message transmission fails, a 
> Transmit Interrupt is generated and as a consequence can_get_echo_skb is 
> called. Thus a can frame is echoed back even when the transmission fails 
> (instead of being discarded). When one-shot mode is enabled no other 
> interrupts are generated on a failed transmission. Thus, as far as I can 
> see, there is no way for the failed frames not to be echoed back locally 
> in this case.
> 

I worked around this limitation in the janz-ican3 driver by writing my
own version of the can_put_echo_skb() and can_get_echo_skb() functions.
Also notice the skb_dequeue() in ican3_handle_cevtind(), which handles
the transmission failure case.

It is probably possible to modify the SJA1000 driver to do something
similar.

Hope it helps,
Ira

> I'll try to word it better in the next version of the patch.
> 
> >> Some write-more-readable-code comments inline :)
> >>
> >>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> >>> ---
> >>>   drivers/net/can/sja1000/sja1000.c |   10 +++++++---
> >>>   1 files changed, 7 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> >>> index 4c4f33d..cc771d9 100644
> >>> --- a/drivers/net/can/sja1000/sja1000.c
> >>> +++ b/drivers/net/can/sja1000/sja1000.c
> >>> @@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
> >>>   	struct sja1000_priv *priv = netdev_priv(dev);
> >>>   	unsigned char status = priv->read_reg(priv, REG_MOD);
> >>>   	int i;
> >>> +	u8 normal = priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY ? MOD_LOM : 0;
> >
> >> What about a more readable:
> >> 	u8 normal = 0;
> >>
> >> 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> >> 		normal = MOD_LOM;
> >>
> >>>
> >>>   	for (i = 0; i < 100; i++) {
> >>>   		/* check reset bit */
> >>> @@ -156,7 +157,7 @@ static void set_normal_mode(struct net_device *dev)
> >>>   		}
> >>>
> >>>   		/* set chip to normal mode */
> >>> -		priv->write_reg(priv, REG_MOD, 0x00);
> >>> +		priv->write_reg(priv, REG_MOD, normal);
> >>>   		udelay(10);
> >>>   		status = priv->read_reg(priv, REG_MOD);
> >>>   	}
> >>> @@ -278,6 +279,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
> >>>   	canid_t id;
> >>>   	uint8_t dreg;
> >>>   	int i;
> >>> +	u8 oneshot;
> >>>
> >>>   	if (can_dropped_invalid_skb(dev, skb))
> >>>   		return NETDEV_TX_OK;
> >>> @@ -310,7 +312,8 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
> >>>
> >>>   	can_put_echo_skb(skb, dev, 0);
> >>>
> >>> -	sja1000_write_cmdreg(priv, CMD_TR);
> >>> +	oneshot = (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT) ? CMD_AT : 0;
> >>> +	sja1000_write_cmdreg(priv, CMD_TR | oneshot);
> >>
> >> something simmilar here, too
> >
> > I would prefer here:
> >
> > 	u8 reg = CMD_TR; (or cmd or cmdreg)
> >
> > 	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> > 		reg |= CMD_AT;
> >
> > A cached value seem overkill. Similar above.
> 
> Thanks for the input from both of you! I'll take care of that.
> 
> 
> > Andreas, I'm curious what you are using the one-shot mode for.
> 
> In situations when the current view of some information is repeatedly 
> being sent there is no use in trying to resend an old view of the 
> information. In such a case it would be nice to allow for discarding 
> failed frames using one-shot mode.
> 
> Cheers,
> Andreas
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] can: sja1000: Add support for listen-only mode and one-shot mode

[Marc Kleine-Budde]

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

On 09/18/2012 06:16 PM, Ira W. Snyder wrote:
[...]

>>> The commit message is incomplete and confusing, I agree. The patch
>>> obviously implements listen-only and one-shot mode. From the data sheet:
>>>
>>> LOM FUNCTION:
>>>    1: listen only; in this mode the CAN controller would
>>>    give no acknowledge to the CAN-bus, even if a message is received
>>>    successfully; the error counters are stopped at the current value
>>>    normal
>>>
>>> Single-shot:
>>>    Setting the command bits CMR.0 (TR) and CMR.1 (AT) simultaneously
>>>    results in sending the transmit message once. No re-transmission will
>>>    be performed in the event of an error or arbitration lost
>>>    (single-shot transmission).
>>
>> The limitation is that when a one-shot message transmission fails, a 
>> Transmit Interrupt is generated and as a consequence can_get_echo_skb is 
>> called. Thus a can frame is echoed back even when the transmission fails 

The question is, can you identify somehow if the message has been
transmitted successfully or not.

>> (instead of being discarded). When one-shot mode is enabled no other 
>> interrupts are generated on a failed transmission. Thus, as far as I can 
>> see, there is no way for the failed frames not to be echoed back locally 
>> in this case.

> I worked around this limitation in the janz-ican3 driver by writing my
> own version of the can_put_echo_skb() and can_get_echo_skb() functions.
> Also notice the skb_dequeue() in ican3_handle_cevtind(), which handles
> the transmission failure case.
> 
> It is probably possible to modify the SJA1000 driver to do something
> similar.

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

[PATCH v2] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

One-shot mode correctly refrains from trying to resend frames that
fail. However, all can frames are echoed back by can_get_echo_skb
regardless of whether they were transmitted successfully or not. When
in one-shot mode, sja1000 generates a transmit interrupt both for a
successful and a failed transmission. It is not possible to
distinguish between the cases and therefore not possible to refrain
from echoing back a frame that was not transmitted successfully.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/net/can/sja1000/sja1000.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4c4f33d..1a2159e 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
 	struct sja1000_priv *priv = netdev_priv(dev);
 	unsigned char status = priv->read_reg(priv, REG_MOD);
 	int i;
+	u8 mod;
 
 	for (i = 0; i < 100; i++) {
 		/* check reset bit */
@@ -156,7 +157,10 @@ static void set_normal_mode(struct net_device *dev)
 		}
 
 		/* set chip to normal mode */
-		priv->write_reg(priv, REG_MOD, 0x00);
+		mod = 0;
+		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+			mod |= MOD_LOM;
+		priv->write_reg(priv, REG_MOD, mod);
 		udelay(10);
 		status = priv->read_reg(priv, REG_MOD);
 	}
@@ -278,6 +282,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 	canid_t id;
 	uint8_t dreg;
 	int i;
+	u8 cmd;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
@@ -310,7 +315,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 
 	can_put_echo_skb(skb, dev, 0);
 
-	sja1000_write_cmdreg(priv, CMD_TR);
+	cmd = CMD_TR;
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		cmd |= CMD_AT;
+	sja1000_write_cmdreg(priv, cmd);
 
 	return NETDEV_TX_OK;
 }
@@ -605,7 +613,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
 	priv->can.do_set_mode = sja1000_set_mode;
 	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
-		CAN_CTRLMODE_BERR_REPORTING;
+		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
+		CAN_CTRLMODE_ONE_SHOT;
 
 	spin_lock_init(&priv->cmdreg_lock);
 
-- 
1.7.0.4

Re: [PATCH v2] can: sja1000: Add support for listen-only mode and one-shot mode

[Marc Kleine-Budde]

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

On 09/18/2012 06:34 PM, Andreas Larsson wrote:
> One-shot mode correctly refrains from trying to resend frames that
> fail. However, all can frames are echoed back by can_get_echo_skb
> regardless of whether they were transmitted successfully or not. When
> in one-shot mode, sja1000 generates a transmit interrupt both for a
> successful and a failed transmission. It is not possible to
> distinguish between the cases and therefore not possible to refrain
> from echoing back a frame that was not transmitted successfully.

Stupid hardware :/

> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
>  drivers/net/can/sja1000/sja1000.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 4c4f33d..1a2159e 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -141,6 +141,7 @@ static void set_normal_mode(struct net_device *dev)
>  	struct sja1000_priv *priv = netdev_priv(dev);
>  	unsigned char status = priv->read_reg(priv, REG_MOD);
>  	int i;
> +	u8 mod;
	u8 mod = 0;

Why inside the loop, see below? Better do it here:

	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
		mod |= MOD_LOM;

(The compiler would probably move it out of the loop anyway)

>  
>  	for (i = 0; i < 100; i++) {
>  		/* check reset bit */
> @@ -156,7 +157,10 @@ static void set_normal_mode(struct net_device *dev)
>  		}
>  
>  		/* set chip to normal mode */
> -		priv->write_reg(priv, REG_MOD, 0x00);
> +		mod = 0;
> +		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +			mod |= MOD_LOM;
> +		priv->write_reg(priv, REG_MOD, mod);
>  		udelay(10);
>  		status = priv->read_reg(priv, REG_MOD);
>  	}
> @@ -278,6 +282,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  	canid_t id;
>  	uint8_t dreg;
>  	int i;
> +	u8 cmd;
>  
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
> @@ -310,7 +315,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  
>  	can_put_echo_skb(skb, dev, 0);
>  
> -	sja1000_write_cmdreg(priv, CMD_TR);
> +	cmd = CMD_TR;
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> +		cmd |= CMD_AT;
> +	sja1000_write_cmdreg(priv, cmd);
>  
>  	return NETDEV_TX_OK;
>  }
> @@ -605,7 +613,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
>  	priv->can.do_set_mode = sja1000_set_mode;
>  	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
> -		CAN_CTRLMODE_BERR_REPORTING;
> +		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
> +		CAN_CTRLMODE_ONE_SHOT;
>  
>  	spin_lock_init(&priv->cmdreg_lock);
>  
> 

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

Re: [PATCH v2] can: sja1000: Add support for listen-only mode and one-shot mode

[Oliver Hartkopp]

On 18.09.2012 19:17, Marc Kleine-Budde wrote:

> On 09/18/2012 06:34 PM, Andreas Larsson wrote:


>>  	unsigned char status = priv->read_reg(priv, REG_MOD);
>>  	int i;
>> +	u8 mod;
> 	u8 mod = 0;
> 
> Why inside the loop, see below? Better do it here:
> 
> 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> 		mod |= MOD_LOM;
> 
> (The compiler would probably move it out of the loop anyway)
> 
>>  
>>  	for (i = 0; i < 100; i++) {
>>  		/* check reset bit */
>> @@ -156,7 +157,10 @@ static void set_normal_mode(struct net_device *dev)
>>  		}
>>  
>>  		/* set chip to normal mode */
>> -		priv->write_reg(priv, REG_MOD, 0x00);
>> +		mod = 0;
>> +		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
>> +			mod |= MOD_LOM;
>> +		priv->write_reg(priv, REG_MOD, mod);
>>  		udelay(10);


What about using just the constants:

if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
	priv->write_reg(priv, REG_MOD, MOD_LOM);
else
	priv->write_reg(priv, REG_MOD, 0x00);

Which omits any experiments with extra variables ...

>>  		status = priv->read_reg(priv, REG_MOD);
>>  	}
>> @@ -278,6 +282,7 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>>  	canid_t id;
>>  	uint8_t dreg;
>>  	int i;
>> +	u8 cmd;
>>  
>>  	if (can_dropped_invalid_skb(dev, skb))
>>  		return NETDEV_TX_OK;
>> @@ -310,7 +315,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>>  
>>  	can_put_echo_skb(skb, dev, 0);
>>  
>> -	sja1000_write_cmdreg(priv, CMD_TR);
>> +	cmd = CMD_TR;
>> +	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
>> +		cmd |= CMD_AT;
>> +	sja1000_write_cmdreg(priv, cmd);


The same here:

if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
	sja1000_write_cmdreg(priv, CMD_TR|CMD_AT);
else
	sja1000_write_cmdreg(priv, CMD_TR);


>>  
>>  	return NETDEV_TX_OK;
>>  }
>> @@ -605,7 +613,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
>>  	priv->can.do_set_mode = sja1000_set_mode;
>>  	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
>>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>> -		CAN_CTRLMODE_BERR_REPORTING;
>> +		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
>> +		CAN_CTRLMODE_ONE_SHOT;
>>  
>>  	spin_lock_init(&priv->cmdreg_lock);
>>  


Regards,
Oliver

Re: [PATCH v2] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

On 09/18/2012 07:23 PM, Oliver Hartkopp wrote:
> On 18.09.2012 19:17, Marc Kleine-Budde wrote:
> What about using just the constants:
>
> if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> 	priv->write_reg(priv, REG_MOD, MOD_LOM);
> else
> 	priv->write_reg(priv, REG_MOD, 0x00);
>
> Which omits any experiments with extra variables ...

> The same here:
>
> if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> 	sja1000_write_cmdreg(priv, CMD_TR|CMD_AT);
> else
> 	sja1000_write_cmdreg(priv, CMD_TR);

Yes, that looks nice.

Cheers,
Andreas

Re: [PATCH v2] can: sja1000: Add support for listen-only mode and one-shot mode

[Wolfgang Grandegger]

On 09/18/2012 06:34 PM, Andreas Larsson wrote:
> One-shot mode correctly refrains from trying to resend frames that
> fail. However, all can frames are echoed back by can_get_echo_skb
> regardless of whether they were transmitted successfully or not. When
> in one-shot mode, sja1000 generates a transmit interrupt both for a
> successful and a failed transmission. It is not possible to
> distinguish between the cases and therefore not possible to refrain
> from echoing back a frame that was not transmitted successfully.

Did you check the transmission complete status bit (SR_TCS)? From the
data sheet it's not 100% clear if it should work with single-shot
messages as well.

Wolfgang.

Re: [PATCH v2] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

On 09/18/2012 08:27 PM, Wolfgang Grandegger wrote:
> On 09/18/2012 06:34 PM, Andreas Larsson wrote:
>> One-shot mode correctly refrains from trying to resend frames that
>> fail. However, all can frames are echoed back by can_get_echo_skb
>> regardless of whether they were transmitted successfully or not. When
>> in one-shot mode, sja1000 generates a transmit interrupt both for a
>> successful and a failed transmission. It is not possible to
>> distinguish between the cases and therefore not possible to refrain
>> from echoing back a frame that was not transmitted successfully.
>
> Did you check the transmission complete status bit (SR_TCS)? From the
> data sheet it's not 100% clear if it should work with single-shot
> messages as well.

Ah, excellent! This indeed indicates correctly in one-shot mode whether 
a message has been transmitted successfully or not. Thanks!

Cheers,
Andreas

[PATCH v3] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

One-shot mode uses the TCS bit of the status register to discern
whether the transmission was successful or not. On a failed
transmission frames are not echoed back.

When one-shot mode is not enabled, no transmit interrupt is generated
by a failed transmission. Therefore, the branch in the interrupt
handler that frees the echo skb is never triggered unless one-shot
mode is enabled.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/net/can/sja1000/sja1000.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4c4f33d..fb39461 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -156,7 +156,10 @@ static void set_normal_mode(struct net_device *dev)
 		}
 
 		/* set chip to normal mode */
-		priv->write_reg(priv, REG_MOD, 0x00);
+		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+			priv->write_reg(priv, REG_MOD, MOD_LOM);
+		else
+			priv->write_reg(priv, REG_MOD, 0x00);
 		udelay(10);
 		status = priv->read_reg(priv, REG_MOD);
 	}
@@ -310,7 +313,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 
 	can_put_echo_skb(skb, dev, 0);
 
-	sja1000_write_cmdreg(priv, CMD_TR);
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		sja1000_write_cmdreg(priv, CMD_TR | CMD_AT);
+	else
+		sja1000_write_cmdreg(priv, CMD_TR);
 
 	return NETDEV_TX_OK;
 }
@@ -505,10 +511,18 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 			netdev_warn(dev, "wakeup interrupt\n");
 
 		if (isrc & IRQ_TI) {
-			/* transmission complete interrupt */
-			stats->tx_bytes += priv->read_reg(priv, REG_FI) & 0xf;
-			stats->tx_packets++;
-			can_get_echo_skb(dev, 0);
+			/* transmission buffer released */
+			if (status & SR_TCS) {
+				/* transmission complete */
+				stats->tx_bytes +=
+					priv->read_reg(priv, REG_FI) & 0xf;
+				stats->tx_packets++;
+				can_get_echo_skb(dev, 0);
+			} else {
+				/* reachable only in one-shot mode */
+				stats->tx_errors++;
+				can_free_echo_skb(dev, 0);
+			}
 			netif_wake_queue(dev);
 		}
 		if (isrc & IRQ_RI) {
@@ -605,7 +619,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
 	priv->can.do_set_mode = sja1000_set_mode;
 	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
-		CAN_CTRLMODE_BERR_REPORTING;
+		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
+		CAN_CTRLMODE_ONE_SHOT;
 
 	spin_lock_init(&priv->cmdreg_lock);
 
-- 
1.7.0.4

Re: [PATCH v3] can: sja1000: Add support for listen-only mode and one-shot mode

[Wolfgang Grandegger]

Hi Andreas,

looks nice now. Just some final comments from my side...

On 09/19/2012 09:12 AM, Andreas Larsson wrote:
> One-shot mode uses the TCS bit of the status register to discern
> whether the transmission was successful or not. On a failed
> transmission frames are not echoed back.
> 
> When one-shot mode is not enabled, no transmit interrupt is generated
> by a failed transmission. Therefore, the branch in the interrupt
> handler that frees the echo skb is never triggered unless one-shot
> mode is enabled.
> 
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
>  drivers/net/can/sja1000/sja1000.c |   29 ++++++++++++++++++++++-------
>  1 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index 4c4f33d..fb39461 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -156,7 +156,10 @@ static void set_normal_mode(struct net_device *dev)
>  		}
>  
>  		/* set chip to normal mode */
> -		priv->write_reg(priv, REG_MOD, 0x00);
> +		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +			priv->write_reg(priv, REG_MOD, MOD_LOM);
> +		else
> +			priv->write_reg(priv, REG_MOD, 0x00);
>  		udelay(10);
>  		status = priv->read_reg(priv, REG_MOD);
>  	}
> @@ -310,7 +313,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
>  
>  	can_put_echo_skb(skb, dev, 0);
>  
> -	sja1000_write_cmdreg(priv, CMD_TR);
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> +		sja1000_write_cmdreg(priv, CMD_TR | CMD_AT);
> +	else
> +		sja1000_write_cmdreg(priv, CMD_TR);
>  
>  	return NETDEV_TX_OK;
>  }
> @@ -505,10 +511,18 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>  			netdev_warn(dev, "wakeup interrupt\n");
>  
>  		if (isrc & IRQ_TI) {
> -			/* transmission complete interrupt */
> -			stats->tx_bytes += priv->read_reg(priv, REG_FI) & 0xf;
> -			stats->tx_packets++;
> -			can_get_echo_skb(dev, 0);
> +			/* transmission buffer released */
> +			if (status & SR_TCS) {
> +				/* transmission complete */
> +				stats->tx_bytes +=
> +					priv->read_reg(priv, REG_FI) & 0xf;
> +				stats->tx_packets++;
> +				can_get_echo_skb(dev, 0);
> +			} else {
> +				/* reachable only in one-shot mode */
> +				stats->tx_errors++;
> +				can_free_echo_skb(dev, 0);
> +			}

I would prefer not touching the original path:

			if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
			    !(status & SR_TCS) {
				/* reachable only in one-shot mode */
				stats->tx_errors++;
				can_free_echo_skb(dev, 0);
			} else {
				/* transmission complete */
				stats->tx_bytes +=
					priv->read_reg(priv, REG_FI) & 0xf;
				stats->tx_packets++;
				can_get_echo_skb(dev, 0);
			}

With that change you can add my:

Acked-by: Wolfgang Grandegger <wg@grandegger.com>

Thanks,

Wolfgang.

[PATCH v4] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

One-shot mode uses the TCS bit of the status register to discern
whether a transmission was successful or not. On a failed
transmission, the frame is not echoed back.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/sja1000/sja1000.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4c4f33d..9ebfe65 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -156,7 +156,10 @@ static void set_normal_mode(struct net_device *dev)
 		}
 
 		/* set chip to normal mode */
-		priv->write_reg(priv, REG_MOD, 0x00);
+		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+			priv->write_reg(priv, REG_MOD, MOD_LOM);
+		else
+			priv->write_reg(priv, REG_MOD, 0x00);
 		udelay(10);
 		status = priv->read_reg(priv, REG_MOD);
 	}
@@ -310,7 +313,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 
 	can_put_echo_skb(skb, dev, 0);
 
-	sja1000_write_cmdreg(priv, CMD_TR);
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		sja1000_write_cmdreg(priv, CMD_TR | CMD_AT);
+	else
+		sja1000_write_cmdreg(priv, CMD_TR);
 
 	return NETDEV_TX_OK;
 }
@@ -505,10 +511,18 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 			netdev_warn(dev, "wakeup interrupt\n");
 
 		if (isrc & IRQ_TI) {
-			/* transmission complete interrupt */
-			stats->tx_bytes += priv->read_reg(priv, REG_FI) & 0xf;
-			stats->tx_packets++;
-			can_get_echo_skb(dev, 0);
+			/* transmission buffer released */
+			if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
+			    !(status & SR_TCS)) {
+				stats->tx_errors++;
+				can_free_echo_skb(dev, 0);
+			} else {
+				/* transmission complete */
+				stats->tx_bytes +=
+					priv->read_reg(priv, REG_FI) & 0xf;
+				stats->tx_packets++;
+				can_get_echo_skb(dev, 0);
+			}
 			netif_wake_queue(dev);
 		}
 		if (isrc & IRQ_RI) {
@@ -605,7 +619,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
 	priv->can.do_set_mode = sja1000_set_mode;
 	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
-		CAN_CTRLMODE_BERR_REPORTING;
+		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
+		CAN_CTRLMODE_ONE_SHOT;
 
 	spin_lock_init(&priv->cmdreg_lock);
 
-- 
1.7.0.4

Re: [PATCH v4] can: sja1000: Add support for listen-only mode and one-shot mode

[Oliver Hartkopp]

On 19.09.2012 12:24, Andreas Larsson wrote:

It's really a nitpick, but

>

>  		/* set chip to normal mode */
> -		priv->write_reg(priv, REG_MOD, 0x00);
> +		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +			priv->write_reg(priv, REG_MOD, MOD_LOM);
> +		else
> +			priv->write_reg(priv, REG_MOD, 0x00);
>  		udelay(10);
>  		status = priv->read_reg(priv, REG_MOD);
>  	}


i would prefer an empty line before (and probably also after) 'udelay(10)'.
For better readability.

Anyway you can add my

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

too.

Thanks,
Oliver

[PATCH v5] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

One-shot mode uses the TCS bit of the status register to discern
whether a transmission was successful or not. On a failed
transmission, the frame is not echoed back.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/sja1000/sja1000.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 4c4f33d..25011db 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -156,8 +156,13 @@ static void set_normal_mode(struct net_device *dev)
 		}
 
 		/* set chip to normal mode */
-		priv->write_reg(priv, REG_MOD, 0x00);
+		if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+			priv->write_reg(priv, REG_MOD, MOD_LOM);
+		else
+			priv->write_reg(priv, REG_MOD, 0x00);
+
 		udelay(10);
+
 		status = priv->read_reg(priv, REG_MOD);
 	}
 
@@ -310,7 +315,10 @@ static netdev_tx_t sja1000_start_xmit(struct sk_buff *skb,
 
 	can_put_echo_skb(skb, dev, 0);
 
-	sja1000_write_cmdreg(priv, CMD_TR);
+	if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		sja1000_write_cmdreg(priv, CMD_TR | CMD_AT);
+	else
+		sja1000_write_cmdreg(priv, CMD_TR);
 
 	return NETDEV_TX_OK;
 }
@@ -505,10 +513,18 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
 			netdev_warn(dev, "wakeup interrupt\n");
 
 		if (isrc & IRQ_TI) {
-			/* transmission complete interrupt */
-			stats->tx_bytes += priv->read_reg(priv, REG_FI) & 0xf;
-			stats->tx_packets++;
-			can_get_echo_skb(dev, 0);
+			/* transmission buffer released */
+			if (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT &&
+			    !(status & SR_TCS)) {
+				stats->tx_errors++;
+				can_free_echo_skb(dev, 0);
+			} else {
+				/* transmission complete */
+				stats->tx_bytes +=
+					priv->read_reg(priv, REG_FI) & 0xf;
+				stats->tx_packets++;
+				can_get_echo_skb(dev, 0);
+			}
 			netif_wake_queue(dev);
 		}
 		if (isrc & IRQ_RI) {
@@ -605,7 +621,8 @@ struct net_device *alloc_sja1000dev(int sizeof_priv)
 	priv->can.do_set_mode = sja1000_set_mode;
 	priv->can.do_get_berr_counter = sja1000_get_berr_counter;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
-		CAN_CTRLMODE_BERR_REPORTING;
+		CAN_CTRLMODE_BERR_REPORTING | CAN_CTRLMODE_LISTENONLY |
+		CAN_CTRLMODE_ONE_SHOT;
 
 	spin_lock_init(&priv->cmdreg_lock);
 
-- 
1.7.0.4

Re: [PATCH v5] can: sja1000: Add support for listen-only mode and one-shot mode

[Marc Kleine-Budde]

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

On 09/20/2012 09:50 AM, Andreas Larsson wrote:
> One-shot mode uses the TCS bit of the status register to discern
> whether a transmission was successful or not. On a failed
> transmission, the frame is not echoed back.
> 
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Applied to can-next.

Tnx, 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: 259 bytes --]

Re: [PATCH v5] can: sja1000: Add support for listen-only mode and one-shot mode

[Andreas Larsson]

On 09/21/2012 10:08 AM, Marc Kleine-Budde wrote:
> On 09/20/2012 09:50 AM, Andreas Larsson wrote:
>> One-shot mode uses the TCS bit of the status register to discern
>> whether a transmission was successful or not. On a failed
>> transmission, the frame is not echoed back.
>>
>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
>
> Applied to can-next.

Excellent! Thanks everyone for the feedback!

Cheers,
Andreas