Re: [PATCH] mcp251x: mcp2515 stops receiving

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

Hey Martin,

On 05/08/2014 08:33 AM, Rost, Martin wrote:
> The mcp2515 sometimes seems to trigger an interrupt with the
> corresponding register not being set yet. This makes the driver exit
> the interrupt because there is obviously nothing to do, but the
> interrupt line is kept low. Therefore the driver does not see any
> more interrupts until the chip is reset (via interface down/up).

Hmmmm, I think level triggered interrupt would help here. However not
all interrupt controller support level interrupts and
arch/arm/mach-pxa/icontrol.c sets the trigger to rising edge.

> This patch changes the IST to first check the IRQ registers, and wait
> up to 10 ms if an event really occurrs. If the IRQ register is still
> empty after 10ms, a kernel message gets issued.

What happens if the register is still empty after 10ms? Can we do
something better than the print, which will probably not fix the probem...

> The IST loop is rearranged to evaluate the IRQ register at the last
> moment before exiting, to not miss a late irq event.

Alexander, have you seen or heard of this problem before? What do you
think about this workaround?

> ---
> diff --git "a/mcp251x.c" "b/mcp251x.c"
> index ad58ac6..668ce63 100644
> --- "a/mcp251x.c"	
> +++ "b/mcp251x.c"	
> @@ -806,15 +806,29 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id)
>  	struct mcp251x_priv *priv = dev_id;
>  	struct spi_device *spi = priv->spi;
>  	struct net_device *net = priv->net;
> +	u8 intf, eflag;
> +	u8 retrycount = 10;
>  
>  	mutex_lock(&priv->mcp_lock);
> -	while (!priv->force_quit) {
> +	
> +	do {
> +	mcp251x_read_2regs(spi, CANINTF, &intf, &eflag);
> +		if (!intf) {
> +//			printk(KERN_CRIT "MCP251x: IRQ delaying.\r\n");

please remove that line

> +			mdelay(1);
> +		}
> +	} while (!intf && (retrycount--));
> +	
> +	if (!intf)
> +		printk(KERN_CRIT "MCP251x: IRQ without a cause.\r\n");

In Linux, we use \n only, also dev_LEVEL() is preferred over plain printk.

> +	
> +	while ((!priv->force_quit) && (intf)) {
>  		enum can_state new_state;
> -		u8 intf, eflag;
> +//		u8 intf, eflag;

please remove, not comment out.

>  		u8 clear_intf = 0;
>  		int can_id = 0, data1 = 0;
>  
> -		mcp251x_read_2regs(spi, CANINTF, &intf, &eflag);
> +//		mcp251x_read_2regs(spi, CANINTF, &intf, &eflag);

same here

>  
>  		/* mask out flags we don't care about */
>  		intf &= CANINTF_RX | CANINTF_TX | CANINTF_ERR;
> @@ -913,8 +927,8 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id)
>  			}
>  		}
>  
> -		if (intf == 0)
> -			break;
> +//		if (intf == 0)
> +//			break;

same here

>  
>  		if (intf & CANINTF_TX) {
>  			net->stats.tx_packets++;
> @@ -926,6 +940,7 @@ static irqreturn_t mcp251x_can_ist(int irq, void *dev_id)
>  			netif_wake_queue(net);
>  		}
>  
> +		mcp251x_read_2regs(spi, CANINTF, &intf, &eflag);
>  	}
>  	mutex_unlock(&priv->mcp_lock);
>  	return IRQ_HANDLED;

I'll send a cleaned version of you patch.

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