Re: [PATCH 2/2] can: flexcan: Use last mailbox for Tx

[Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>]

On 10/11/2018 05:01 PM, Alexander Stein wrote:
> Essentially this patch moves the Tx mailbox to position 63, regardless of
> timestamp based offloading or Rx FIFO.

Can you get rid of priv->tx_mb and priv->tx_mb_idx, it's not needed if
it's a compile time constant.

> So mainly the iflag register usage regarding Tx has changed. The rest is
> consolidating Rx FIFO and timestamp offloading as they now use both the
> same Tx mailbox.
> The reason is a very annoying behavior regarding sending RTR frames when
> _not_ using Rx FIFO:
> If a Tx mailbox sent an RTR frame it becomes an Rx mailbox. For that
> reason flexcan_irq disables the Tx mailbox again. But if during the time
> the RTR was sent and the Tx mailbox is disabled a new CAN frames is
> received, it is lost without notice. The reason is that so-called "Move-in"
> process starts from the lowest mailbox which happen to be a Tx mailbox set
> to EMPTY.
> 
> Steps to reproduce (I used an imx7d):
> 1. generate regular bursts of messages
> 2. send an RTR from flexcan with higher priority than burst messages every
>    1ms, e.g. cangen -I 0x100 -L 0 -g 1 -R can0
> 3. notice a lost message without notification after some seconds
> 
> When running an iperf in parallel this problem is occurring even more
> frequently.
> Using filters is not possible as at least one single CAN-ID is allowed.
> Handling the Tx MB during Rx is also not possible as there is no race-free
> disable of Rx MB.
> 
> There is still a slight window when the described problem can occur. But
> for that all Rx MB must be in use which is essentially next to an overrun.
> Still there will be no indication if it ever occurs.
> 
> Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxxxxxxxx>

> ---
>  drivers/net/can/flexcan.c | 52 ++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index c9d30480aae9..83cc9e6d8ba1 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -135,13 +135,12 @@
>  
>  /* FLEXCAN interrupt flag register (IFLAG) bits */
>  /* Errata ERR005829 step7: Reserve first valid MB */
> -#define FLEXCAN_TX_MB_RESERVED_OFF_FIFO	8
> -#define FLEXCAN_TX_MB_OFF_FIFO		9
> +#define FLEXCAN_TX_MB_RESERVED_OFF_FIFO		8
>  #define FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP	0
> -#define FLEXCAN_TX_MB_OFF_TIMESTAMP		1
> -#define FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST	(FLEXCAN_TX_MB_OFF_TIMESTAMP + 1)
> -#define FLEXCAN_RX_MB_OFF_TIMESTAMP_LAST	63
> -#define FLEXCAN_IFLAG_MB(x)		BIT(x)
> +#define FLEXCAN_TX_MB				63
> +#define FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST	(FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP + 1)
> +#define FLEXCAN_RX_MB_OFF_TIMESTAMP_LAST	(FLEXCAN_TX_MB - 1)
> +#define FLEXCAN_IFLAG_MB(x)		(((x) < 32) ? BIT(x) : BIT((x) - 32))

What about using "BIT(x & 0x1f)" instead? This way we could get rid of
the conditional.

>  #define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW	BIT(7)
>  #define FLEXCAN_IFLAG_RX_FIFO_WARN	BIT(6)
>  #define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE	BIT(5)
> @@ -733,9 +732,9 @@ static inline u64 flexcan_read_reg_iflag_rx(struct flexcan_priv *priv)
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	u32 iflag1, iflag2;
>  
> -	iflag2 = priv->read(&regs->iflag2) & priv->reg_imask2_default;
> -	iflag1 = priv->read(&regs->iflag1) & priv->reg_imask1_default &
> +	iflag2 = priv->read(&regs->iflag2) & priv->reg_imask2_default &
>  		~FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> +	iflag1 = priv->read(&regs->iflag1) & priv->reg_imask1_default;
>  
>  	return (u64)iflag2 << 32 | iflag1;
>  }
> @@ -747,7 +746,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	struct flexcan_priv *priv = netdev_priv(dev);
>  	struct flexcan_regs __iomem *regs = priv->regs;
>  	irqreturn_t handled = IRQ_NONE;
> -	u32 reg_iflag1, reg_esr;
> +	u32 reg_iflag1, reg_iflag2, reg_esr;
>  	enum can_state last_state = priv->can.state;
>  
>  	reg_iflag1 = priv->read(&regs->iflag1);
> @@ -780,8 +779,10 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  		}
>  	}
>  
> +	reg_iflag2 = priv->read(&regs->iflag2);
> +
>  	/* transmission complete interrupt */
> -	if (reg_iflag1 & FLEXCAN_IFLAG_MB(priv->tx_mb_idx)) {
> +	if (reg_iflag2 & FLEXCAN_IFLAG_MB(priv->tx_mb_idx)) {
>  		handled = IRQ_HANDLED;
>  		stats->tx_bytes += can_get_echo_skb(dev, 0);
>  		stats->tx_packets++;
> @@ -790,7 +791,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  		/* after sending a RTR frame MB is in RX mode */
>  		priv->write(FLEXCAN_MB_CODE_TX_INACTIVE,
>  			    &priv->tx_mb->can_ctrl);
> -		priv->write(FLEXCAN_IFLAG_MB(priv->tx_mb_idx), &regs->iflag1);
> +		priv->write(FLEXCAN_IFLAG_MB(priv->tx_mb_idx), &regs->iflag2);
>  		netif_wake_queue(dev);
>  	}
>  
> @@ -936,11 +937,12 @@ static int flexcan_chip_start(struct net_device *dev)
>  
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
>  		reg_mcr &= ~FLEXCAN_MCR_FEN;
> -		reg_mcr |= FLEXCAN_MCR_MAXMB(priv->offload.mb_last);
>  	} else {
> -		reg_mcr |= FLEXCAN_MCR_FEN |
> -			FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
> +		reg_mcr |= FLEXCAN_MCR_FEN;
>  	}
> +	/* MAXMB must cover all used Rx & Tx mailboxes */
> +	reg_mcr |= FLEXCAN_MCR_MAXMB(priv->tx_mb_idx);
> +
>  	netdev_dbg(dev, "%s: writing mcr=0x%08x", __func__, reg_mcr);
>  	priv->write(reg_mcr, &regs->mcr);
>  
> @@ -983,16 +985,17 @@ static int flexcan_chip_start(struct net_device *dev)
>  		priv->write(reg_ctrl2, &regs->ctrl2);
>  	}
>  
> -	/* clear and invalidate all mailboxes first */
> -	for (i = priv->tx_mb_idx; i < ARRAY_SIZE(regs->mb); i++) {
> -		priv->write(FLEXCAN_MB_CODE_RX_INACTIVE,
> -			    &regs->mb[i].can_ctrl);
> -	}
> -
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
> -		for (i = priv->offload.mb_first; i <= priv->offload.mb_last; i++)
> +		for (i = priv->offload.mb_first; i <= priv->offload.mb_last; i++) {
>  			priv->write(FLEXCAN_MB_CODE_RX_EMPTY,
>  				    &regs->mb[i].can_ctrl);
> +		}
> +	} else {
> +		/* clear and invalidate unused mailboxes first */
> +		for (i = FLEXCAN_TX_MB_RESERVED_OFF_FIFO; i <= ARRAY_SIZE(regs->mb); i++) {
> +			priv->write(FLEXCAN_MB_CODE_RX_INACTIVE,
> +				    &regs->mb[i].can_ctrl);
> +		}
>  	}
>  
>  	/* Errata ERR005829: mark first TX mailbox as INACTIVE */
> @@ -1357,16 +1360,15 @@ static int flexcan_probe(struct platform_device *pdev)
>  	priv->reg_xceiver = reg_xceiver;
>  
>  	if (priv->devtype_data->quirks & FLEXCAN_QUIRK_USE_OFF_TIMESTAMP) {
> -		priv->tx_mb_idx = FLEXCAN_TX_MB_OFF_TIMESTAMP;
>  		priv->tx_mb_reserved = &regs->mb[FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP];
>  	} else {
> -		priv->tx_mb_idx = FLEXCAN_TX_MB_OFF_FIFO;
>  		priv->tx_mb_reserved = &regs->mb[FLEXCAN_TX_MB_RESERVED_OFF_FIFO];
>  	}
> +	priv->tx_mb_idx = FLEXCAN_TX_MB;
>  	priv->tx_mb = &regs->mb[priv->tx_mb_idx];
>  
> -	priv->reg_imask1_default = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> -	priv->reg_imask2_default = 0;
> +	priv->reg_imask1_default = 0;
> +	priv->reg_imask2_default = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
>  
>  	priv->offload.mailbox_read = flexcan_mailbox_read;
>  
> 

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: signature.asc
Description: OpenPGP digital signature