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

[Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>]

On 6/9/2026 12:24 PM, Vincent Mailhol wrote:
> 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?

Hello Vincent,

Thank you for taking time in reviewing this series.

This is not intentional. The driver currently supports up to 64 MBs
(bounded by the iflag1 + iflag2 register layout and the u64 mask).
I will update the comment accordingly in V5.

> 
>> +			 */
>> +			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);
> 

Sounds like a good simplification indeed. I will update it in V5.


Regards,
Ciprian

>>   		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
>