Re: [PATCH RESEND v4 3/8] can: flexcan: split rx/tx masks per mailbox IRQ line

[Vincent Mailhol <mailhol@kernel.org>]

On 03/06/2026 at 09:13, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> On S32G2, which has two mailbox IRQ lines (mb-0 for MBs 0-7, mb-1
> for MBs 8-127), both handlers currently process the full rx_mask/tx_mask
> range,
> 
> Introduce struct flexcan_mb_irq to hold per-IRQ-line rx and tx masks.
> 
> In flexcan_irq_mb(), the irq argument selects the correct mask set: the
> primary MB IRQ uses mb_irq[0] and the secondary uses mb_irq[1].
> 
> For single-IRQ platforms, mb_irq[0] holds the full combined masks with no
> functional change.
> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 61 +++++++++++++++++++-------
>  drivers/net/can/flexcan/flexcan.h      | 10 ++++-
>  2 files changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index 7dde2e623def..32e4d4da00a1 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -957,14 +957,16 @@ static inline void flexcan_write64(struct flexcan_priv *priv, u64 val, void __io
>  		priv->write(lower_32_bits(val), addr);
>  }
>  
> -static inline u64 flexcan_read_reg_iflag_rx(struct flexcan_priv *priv)
> +static inline u64 flexcan_read_reg_iflag_rx(struct flexcan_priv *priv,
> +					    u64 rx_mask)
>  {
> -	return flexcan_read64_mask(priv, &priv->regs->iflag1, priv->rx_mask);
> +	return flexcan_read64_mask(priv, &priv->regs->iflag1, rx_mask);
>  }
>  
> -static inline u64 flexcan_read_reg_iflag_tx(struct flexcan_priv *priv)
> +static inline u64 flexcan_read_reg_iflag_tx(struct flexcan_priv *priv,
> +					    u64 tx_mask)
>  {
> -	return flexcan_read64_mask(priv, &priv->regs->iflag1, priv->tx_mask);
> +	return flexcan_read64_mask(priv, &priv->regs->iflag1, tx_mask);
>  }
>  
>  static inline struct flexcan_priv *rx_offload_to_priv(struct can_rx_offload *offload)
> @@ -1071,7 +1073,8 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
>  }
>  
>  /* Process mailbox (RX + TX) events */
> -static irqreturn_t flexcan_do_mb(struct net_device *dev)
> +static irqreturn_t flexcan_do_mb(struct net_device *dev,
> +				 const struct flexcan_mb_irq *mb_irq)
>  {
>  	struct net_device_stats *stats = &dev->stats;
>  	struct flexcan_priv *priv = netdev_priv(dev);
> @@ -1084,7 +1087,8 @@ static irqreturn_t flexcan_do_mb(struct net_device *dev)
>  		u64 reg_iflag_rx;
>  		int ret;
>  
> -		while ((reg_iflag_rx = flexcan_read_reg_iflag_rx(priv))) {
> +		while ((reg_iflag_rx = flexcan_read_reg_iflag_rx(priv,
> +								 mb_irq->rx_mask))) {
>  			handled = IRQ_HANDLED;
>  			ret = can_rx_offload_irq_offload_timestamp(&priv->offload,
>  								   reg_iflag_rx);
> @@ -1110,10 +1114,10 @@ static irqreturn_t flexcan_do_mb(struct net_device *dev)
>  		}
>  	}
>  
> -	reg_iflag_tx = flexcan_read_reg_iflag_tx(priv);
> +	reg_iflag_tx = flexcan_read_reg_iflag_tx(priv, mb_irq->tx_mask);
>  
>  	/* transmission complete interrupt */
> -	if (reg_iflag_tx & priv->tx_mask) {
> +	if (reg_iflag_tx & mb_irq->tx_mask) {
>  		u32 reg_ctrl = priv->read(&priv->tx_mb->can_ctrl);
>  
>  		handled = IRQ_HANDLED;
> @@ -1125,7 +1129,7 @@ static irqreturn_t flexcan_do_mb(struct net_device *dev)
>  		/* after sending a RTR frame MB is in RX mode */
>  		priv->write(FLEXCAN_MB_CODE_TX_INACTIVE,
>  			    &priv->tx_mb->can_ctrl);
> -		flexcan_write64(priv, priv->tx_mask, &regs->iflag1);
> +		flexcan_write64(priv, mb_irq->tx_mask, &regs->iflag1);
>  		netif_wake_queue(dev);
>  	}
>  
> @@ -1228,7 +1232,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	struct flexcan_priv *priv = netdev_priv(dev);
>  	irqreturn_t handled;
>  
> -	handled = flexcan_do_mb(dev);
> +	handled = flexcan_do_mb(dev, &priv->mb_irq[0]);
>  	handled |= flexcan_do_state(dev);
>  	handled |= flexcan_do_berr(dev);
>  
> @@ -1243,9 +1247,15 @@ static irqreturn_t flexcan_irq_mb(int irq, void *dev_id)
>  {
>  	struct net_device *dev = dev_id;
>  	struct flexcan_priv *priv = netdev_priv(dev);
> +	const struct flexcan_mb_irq *mb_irq;
>  	irqreturn_t handled;
> +	int idx;
>  
> -	handled = flexcan_do_mb(dev);
> +	idx = (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ &&
> +	       irq == priv->irq_secondary_mb) ? 1 : 0;
> +	mb_irq = &priv->mb_irq[idx];
> +
> +	handled = flexcan_do_mb(dev, mb_irq);
>  
>  	if (handled)
>  		can_rx_offload_irq_finish(&priv->offload);
> @@ -1473,6 +1483,7 @@ static void flexcan_ram_init(struct net_device *dev)
>  static int flexcan_rx_offload_setup(struct net_device *dev)
>  {
>  	struct flexcan_priv *priv = netdev_priv(dev);
> +	u64 rx_mask, tx_mask;
>  	int err;
>  
>  	if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
> @@ -1494,20 +1505,35 @@ static int flexcan_rx_offload_setup(struct net_device *dev)
>  			flexcan_get_mb(priv, FLEXCAN_TX_MB_RESERVED_RX_FIFO);
>  	priv->tx_mb_idx = priv->mb_count - 1;
>  	priv->tx_mb = flexcan_get_mb(priv, priv->tx_mb_idx);
> -	priv->tx_mask = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> -
>  	priv->offload.mailbox_read = flexcan_mailbox_read;
>  
>  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_USE_RX_MAILBOX) {
>  		priv->offload.mb_first = FLEXCAN_RX_MB_RX_MAILBOX_FIRST;
>  		priv->offload.mb_last = priv->mb_count - 2;
>  
> -		priv->rx_mask = GENMASK_ULL(priv->offload.mb_last,
> -					    priv->offload.mb_first);
> +		rx_mask = GENMASK_ULL(priv->offload.mb_last,
> +				      priv->offload.mb_first);
> +		tx_mask = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> +
> +		if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +			/* S32G2 has two MB IRQ lines with the split at MB 8:
> +			 * mb-0 IRQ handles MBs 0-7,
> +			 * mb-1 IRQ handles MBs 8-127.
                                                  ^^^
Your comment says 8-127 but the code uses GENMASK_ULL(63, 8). Is this
intentional?

> +			 */
> +			priv->mb_irq[0].rx_mask = rx_mask & GENMASK_ULL(7, 0);
> +			priv->mb_irq[0].tx_mask = tx_mask & GENMASK_ULL(7, 0);
> +			priv->mb_irq[1].rx_mask = rx_mask & GENMASK_ULL(63, 8);
> +			priv->mb_irq[1].tx_mask = tx_mask & GENMASK_ULL(63, 8);
> +		} else {
> +			priv->mb_irq[0].rx_mask = rx_mask;
> +			priv->mb_irq[0].tx_mask = tx_mask;
> +		}
> +

The introduction of the struct flexcan_mb_irq seems a bit overkill.
Can't you just define two new masks and keep the existing struct
flexcan_stop_mode untouched:

	#define FLEXCAN_SECONDARY_MB_IRQ_MB0_MASK GENMASK_U64(7, 0)
	#define FLEXCAN_SECONDARY_MB_IRQ_MB1_MASK GENMASK_U64(63, 8)

and when you need to access the MB, just select the correct mask. For
example, flexcan_irq_mb() becomes something like this:

	u64 mb_mask;

	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ
	    && irq == priv->irq_secondary_mb)
		mb_mask = FLEXCAN_SECONDARY_MB_IRQ_MB0_MASK;
	else
		mb_mask = FLEXCAN_SECONDARY_MB_IRQ_MB1_MASK;

	handled = flexcan_do_mb(dev, mb_mask);

>  		err = can_rx_offload_add_timestamp(dev, &priv->offload);
>  	} else {
> -		priv->rx_mask = FLEXCAN_IFLAG_RX_FIFO_OVERFLOW |
> +		priv->mb_irq[0].rx_mask = FLEXCAN_IFLAG_RX_FIFO_OVERFLOW |
>  			FLEXCAN_IFLAG_RX_FIFO_AVAILABLE;
> +		priv->mb_irq[0].tx_mask = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
>  		err = can_rx_offload_add_fifo(dev, &priv->offload,
>  					      FLEXCAN_NAPI_WEIGHT);
>  	}
> @@ -1531,7 +1557,8 @@ static void flexcan_chip_interrupts_enable(const struct net_device *dev)
>  		disable_irq(priv->irq_secondary_mb);
>  
>  	priv->write(priv->reg_ctrl_default, &regs->ctrl);
> -	reg_imask = priv->rx_mask | priv->tx_mask;
> +	reg_imask = priv->mb_irq[0].rx_mask | priv->mb_irq[0].tx_mask |
> +		    priv->mb_irq[1].rx_mask | priv->mb_irq[1].tx_mask;
>  	priv->write(upper_32_bits(reg_imask), &regs->imask2);
>  	priv->write(lower_32_bits(reg_imask), &regs->imask1);
>  	enable_irq(dev->irq);
> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
> index 16692a2502eb..22aa097ec3c0 100644
> --- a/drivers/net/can/flexcan/flexcan.h
> +++ b/drivers/net/can/flexcan/flexcan.h
> @@ -75,10 +75,17 @@
>   */
>  #define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>  
> +#define FLEXCAN_NR_MB_IRQS	2
> +
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
>  };
>  
> +struct flexcan_mb_irq {
> +	u64 rx_mask;
> +	u64 tx_mask;
> +};
> +
>  struct flexcan_stop_mode {
>  	struct regmap *gpr;
>  	u8 req_gpr;
> @@ -99,8 +106,7 @@ struct flexcan_priv {
>  	u8 clk_src;	/* clock source of CAN Protocol Engine */
>  	u8 scu_idx;
>  
> -	u64 rx_mask;
> -	u64 tx_mask;
> +	struct flexcan_mb_irq mb_irq[FLEXCAN_NR_MB_IRQS];
>  	u32 reg_ctrl_default;
>  
>  	struct clk *clk_ipg;


Yours sincerely,
Vincent Mailhol