1. Patchew
  2. linux
  3. [PATCH 0/3] add FlexCAN support for S32G2/S32G3 SoCs
  4. View patch

[PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines

Ciprian Costea posted 3 patches 4 days, 6 hours ago
There is a newer version of this series
[PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Costea 4 days, 6 hours ago 
From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

On S32G2/S32G3 SoC, there are separate interrupts
for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.

In order to handle this FlexCAN hardware particularity, reuse
the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
handling support.

Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
which can be used in case there are two separate mailbox ranges
controlled by independent hardware interrupt lines, as it is
the case on S32G2/S32G3 SoC.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
 drivers/net/can/flexcan/flexcan.h      |  3 +++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index f0dee04800d3..dc56d4a7d30b 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
 		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
-		FLEXCAN_QUIRK_SUPPORT_ECC |
+		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
 		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
-		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
+		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
+		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
 };
 
 static const struct can_bittiming_const flexcan_bittiming_const = {
@@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
 			goto out_free_irq_boff;
 	}
 
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
+		err = request_irq(priv->irq_secondary_mb,
+				  flexcan_irq, IRQF_SHARED, dev->name, dev);
+		if (err)
+			goto out_free_irq_err;
+	}
+
 	flexcan_chip_interrupts_enable(dev);
 
 	netif_start_queue(dev);
 
 	return 0;
 
+ out_free_irq_err:
+	free_irq(priv->irq_err, dev);
  out_free_irq_boff:
 	free_irq(priv->irq_boff, dev);
  out_free_irq:
@@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
 		free_irq(priv->irq_boff, dev);
 	}
 
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
+		free_irq(priv->irq_secondary_mb, dev);
+
 	free_irq(dev->irq, dev);
 	can_rx_offload_disable(&priv->offload);
 	flexcan_chip_stop_disable_on_error(dev);
@@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
+		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
+		if (priv->irq_secondary_mb < 0) {
+			err = priv->irq_secondary_mb;
+			goto failed_platform_get_irq;
+		}
+	}
+
 	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
 		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_FD_NON_ISO;
diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
index 4933d8c7439e..d4b1a954c538 100644
--- a/drivers/net/can/flexcan/flexcan.h
+++ b/drivers/net/can/flexcan/flexcan.h
@@ -70,6 +70,8 @@
 #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
 /* Setup stop mode with ATF SCMI protocol to support wakeup */
 #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
+/* Setup secondary mailbox interrupt */
+#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
 
 struct flexcan_devtype_data {
 	u32 quirks;		/* quirks needed for different IP cores */
@@ -105,6 +107,7 @@ struct flexcan_priv {
 	struct regulator *reg_xceiver;
 	struct flexcan_stop_mode stm;
 
+	int irq_secondary_mb;
 	int irq_boff;
 	int irq_err;
 
-- 
2.45.2
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 2 hours ago 
On 19.11.2024 10:10:53, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> On S32G2/S32G3 SoC, there are separate interrupts
> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> 
> In order to handle this FlexCAN hardware particularity, reuse
> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> handling support.
> 
> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> which can be used in case there are two separate mailbox ranges
> controlled by independent hardware interrupt lines, as it is
> the case on S32G2/S32G3 SoC.

Please move the quirk and quirk handling to the 2nd patch. The 3rd patch
should only add the nxp,s32g2-flexcan compatible and the struct
flexcan_devtype_data nxp_s32g2_devtype_data.

> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>  drivers/net/can/flexcan/flexcan.h      |  3 +++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f0dee04800d3..dc56d4a7d30b 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>  		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
> -		FLEXCAN_QUIRK_SUPPORT_ECC |
> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>  		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>  };
>  
>  static const struct can_bittiming_const flexcan_bittiming_const = {
> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>  			goto out_free_irq_boff;
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		err = request_irq(priv->irq_secondary_mb,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_free_irq_err;
> +	}
> +
>  	flexcan_chip_interrupts_enable(dev);
>  
>  	netif_start_queue(dev);
>  
>  	return 0;
>  
> + out_free_irq_err:
> +	free_irq(priv->irq_err, dev);
>   out_free_irq_boff:
>  	free_irq(priv->irq_boff, dev);
>   out_free_irq:
> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>  		free_irq(priv->irq_boff, dev);
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> +		free_irq(priv->irq_secondary_mb, dev);
> +
>  	free_irq(dev->irq, dev);
>  	can_rx_offload_disable(&priv->offload);
>  	flexcan_chip_stop_disable_on_error(dev);
> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
> +		if (priv->irq_secondary_mb < 0) {
> +			err = priv->irq_secondary_mb;
> +			goto failed_platform_get_irq;
> +		}
> +	}
> +
>  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>  			CAN_CTRLMODE_FD_NON_ISO;
> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
> index 4933d8c7439e..d4b1a954c538 100644
> --- a/drivers/net/can/flexcan/flexcan.h
> +++ b/drivers/net/can/flexcan/flexcan.h
> @@ -70,6 +70,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>  /* Setup stop mode with ATF SCMI protocol to support wakeup */
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
> +/* Setup secondary mailbox interrupt */

Describe why this quirk is needed. If you have a proper description in
the commit message, you can copy it here.

> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>  
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
> @@ -105,6 +107,7 @@ struct flexcan_priv {
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
>  
> +	int irq_secondary_mb;

Please place it after the irq_err, this way it's in order with the
spread sheet.

>  	int irq_boff;
>  	int irq_err;
>  

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 3 days, 1 hour ago 
On 11/20/2024 2:20 PM, Marc Kleine-Budde wrote:
> On 19.11.2024 10:10:53, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> On S32G2/S32G3 SoC, there are separate interrupts
>> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>
>> In order to handle this FlexCAN hardware particularity, reuse
>> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
>> handling support.
>>
>> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
>> which can be used in case there are two separate mailbox ranges
>> controlled by independent hardware interrupt lines, as it is
>> the case on S32G2/S32G3 SoC.
> 
> Please move the quirk and quirk handling to the 2nd patch. The 3rd patch
> should only add the nxp,s32g2-flexcan compatible and the struct
> flexcan_devtype_data nxp_s32g2_devtype_data.
> 

I will refactor accordingly in V2.

>>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>> ---
>>   drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>>   drivers/net/can/flexcan/flexcan.h      |  3 +++
>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
>> index f0dee04800d3..dc56d4a7d30b 100644
>> --- a/drivers/net/can/flexcan/flexcan-core.c
>> +++ b/drivers/net/can/flexcan/flexcan-core.c
>> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>>   	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>>   		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>>   		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
>> -		FLEXCAN_QUIRK_SUPPORT_ECC |
>> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>>   		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
>> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
>> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
>> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>>   };
>>   
>>   static const struct can_bittiming_const flexcan_bittiming_const = {
>> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>>   			goto out_free_irq_boff;
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		err = request_irq(priv->irq_secondary_mb,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_free_irq_err;
>> +	}
>> +
>>   	flexcan_chip_interrupts_enable(dev);
>>   
>>   	netif_start_queue(dev);
>>   
>>   	return 0;
>>   
>> + out_free_irq_err:
>> +	free_irq(priv->irq_err, dev);
>>    out_free_irq_boff:
>>   	free_irq(priv->irq_boff, dev);
>>    out_free_irq:
>> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>>   		free_irq(priv->irq_boff, dev);
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
>> +		free_irq(priv->irq_secondary_mb, dev);
>> +
>>   	free_irq(dev->irq, dev);
>>   	can_rx_offload_disable(&priv->offload);
>>   	flexcan_chip_stop_disable_on_error(dev);
>> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
>> +		if (priv->irq_secondary_mb < 0) {
>> +			err = priv->irq_secondary_mb;
>> +			goto failed_platform_get_irq;
>> +		}
>> +	}
>> +
>>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>>   		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>>   			CAN_CTRLMODE_FD_NON_ISO;
>> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
>> index 4933d8c7439e..d4b1a954c538 100644
>> --- a/drivers/net/can/flexcan/flexcan.h
>> +++ b/drivers/net/can/flexcan/flexcan.h
>> @@ -70,6 +70,8 @@
>>   #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>>   /* Setup stop mode with ATF SCMI protocol to support wakeup */
>>   #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
>> +/* Setup secondary mailbox interrupt */
> 
> Describe why this quirk is needed. If you have a proper description in
> the commit message, you can copy it here.
> 

I will add more motivation in the associated comment in V2.

>> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>>   
>>   struct flexcan_devtype_data {
>>   	u32 quirks;		/* quirks needed for different IP cores */
>> @@ -105,6 +107,7 @@ struct flexcan_priv {
>>   	struct regulator *reg_xceiver;
>>   	struct flexcan_stop_mode stm;
>>   
>> +	int irq_secondary_mb;
> 
> Please place it after the irq_err, this way it's in order with the
> spread sheet.
> 

Will refactor.


Best Regards,
Ciprian

>>   	int irq_boff;
>>   	int irq_err;
>>   
> 
> regards,
> Marc
>
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 5 hours ago 
On 19.11.2024 10:10:53, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> On S32G2/S32G3 SoC, there are separate interrupts
> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> 
> In order to handle this FlexCAN hardware particularity, reuse
> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> handling support.
> 
> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> which can be used in case there are two separate mailbox ranges
> controlled by independent hardware interrupt lines, as it is
> the case on S32G2/S32G3 SoC.

Does the mainline driver already handle the 2nd mailbox range? Is there
any downstream code yet?

Marc

> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>  drivers/net/can/flexcan/flexcan.h      |  3 +++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f0dee04800d3..dc56d4a7d30b 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>  		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
> -		FLEXCAN_QUIRK_SUPPORT_ECC |
> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>  		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>  };
>  
>  static const struct can_bittiming_const flexcan_bittiming_const = {
> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>  			goto out_free_irq_boff;
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		err = request_irq(priv->irq_secondary_mb,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_free_irq_err;
> +	}
> +
>  	flexcan_chip_interrupts_enable(dev);
>  
>  	netif_start_queue(dev);
>  
>  	return 0;
>  
> + out_free_irq_err:
> +	free_irq(priv->irq_err, dev);
>   out_free_irq_boff:
>  	free_irq(priv->irq_boff, dev);
>   out_free_irq:
> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>  		free_irq(priv->irq_boff, dev);
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> +		free_irq(priv->irq_secondary_mb, dev);
> +
>  	free_irq(dev->irq, dev);
>  	can_rx_offload_disable(&priv->offload);
>  	flexcan_chip_stop_disable_on_error(dev);
> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
> +		if (priv->irq_secondary_mb < 0) {
> +			err = priv->irq_secondary_mb;
> +			goto failed_platform_get_irq;
> +		}
> +	}
> +
>  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>  			CAN_CTRLMODE_FD_NON_ISO;
> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
> index 4933d8c7439e..d4b1a954c538 100644
> --- a/drivers/net/can/flexcan/flexcan.h
> +++ b/drivers/net/can/flexcan/flexcan.h
> @@ -70,6 +70,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>  /* Setup stop mode with ATF SCMI protocol to support wakeup */
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
> +/* Setup secondary mailbox interrupt */
> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>  
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
> @@ -105,6 +107,7 @@ struct flexcan_priv {
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
>  
> +	int irq_secondary_mb;
>  	int irq_boff;
>  	int irq_err;
>  
> -- 
> 2.45.2
> 
> 
> 

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 3 days, 5 hours ago 
On 11/20/2024 10:52 AM, Marc Kleine-Budde wrote:
> On 19.11.2024 10:10:53, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> On S32G2/S32G3 SoC, there are separate interrupts
>> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>
>> In order to handle this FlexCAN hardware particularity, reuse
>> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
>> handling support.
>>
>> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
>> which can be used in case there are two separate mailbox ranges
>> controlled by independent hardware interrupt lines, as it is
>> the case on S32G2/S32G3 SoC.
> 
> Does the mainline driver already handle the 2nd mailbox range? Is there
> any downstream code yet?
> 
> Marc
> 

Hello Marc,

The mainline driver already handles the 2nd mailbox range (same 
'flexcan_irq') is used. The only difference is that for the 2nd mailbox 
range a separate interrupt line is used.

I do plan to upstream more patches to the flexcan driver but they relate 
to Power Management (Suspend and Resume routines) and I plan to do this 
in a separate patchset.

Best Regards,
Ciprian

>>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>> ---
>>   drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>>   drivers/net/can/flexcan/flexcan.h      |  3 +++
>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
>> index f0dee04800d3..dc56d4a7d30b 100644
>> --- a/drivers/net/can/flexcan/flexcan-core.c
>> +++ b/drivers/net/can/flexcan/flexcan-core.c
>> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>>   	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>>   		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>>   		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
>> -		FLEXCAN_QUIRK_SUPPORT_ECC |
>> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>>   		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
>> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
>> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
>> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>>   };
>>   
>>   static const struct can_bittiming_const flexcan_bittiming_const = {
>> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>>   			goto out_free_irq_boff;
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		err = request_irq(priv->irq_secondary_mb,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_free_irq_err;
>> +	}
>> +
>>   	flexcan_chip_interrupts_enable(dev);
>>   
>>   	netif_start_queue(dev);
>>   
>>   	return 0;
>>   
>> + out_free_irq_err:
>> +	free_irq(priv->irq_err, dev);
>>    out_free_irq_boff:
>>   	free_irq(priv->irq_boff, dev);
>>    out_free_irq:
>> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>>   		free_irq(priv->irq_boff, dev);
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
>> +		free_irq(priv->irq_secondary_mb, dev);
>> +
>>   	free_irq(dev->irq, dev);
>>   	can_rx_offload_disable(&priv->offload);
>>   	flexcan_chip_stop_disable_on_error(dev);
>> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
>> +		if (priv->irq_secondary_mb < 0) {
>> +			err = priv->irq_secondary_mb;
>> +			goto failed_platform_get_irq;
>> +		}
>> +	}
>> +
>>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>>   		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>>   			CAN_CTRLMODE_FD_NON_ISO;
>> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
>> index 4933d8c7439e..d4b1a954c538 100644
>> --- a/drivers/net/can/flexcan/flexcan.h
>> +++ b/drivers/net/can/flexcan/flexcan.h
>> @@ -70,6 +70,8 @@
>>   #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>>   /* Setup stop mode with ATF SCMI protocol to support wakeup */
>>   #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
>> +/* Setup secondary mailbox interrupt */
>> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>>   
>>   struct flexcan_devtype_data {
>>   	u32 quirks;		/* quirks needed for different IP cores */
>> @@ -105,6 +107,7 @@ struct flexcan_priv {
>>   	struct regulator *reg_xceiver;
>>   	struct flexcan_stop_mode stm;
>>   
>> +	int irq_secondary_mb;
>>   	int irq_boff;
>>   	int irq_err;
>>   
>> -- 
>> 2.45.2
>>
>>
>>
>
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 4 hours ago 
On 20.11.2024 11:01:25, Ciprian Marian Costea wrote:
> On 11/20/2024 10:52 AM, Marc Kleine-Budde wrote:
> > On 19.11.2024 10:10:53, Ciprian Costea wrote:
> > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > 
> > > On S32G2/S32G3 SoC, there are separate interrupts
> > > for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> > > 
> > > In order to handle this FlexCAN hardware particularity, reuse
> > > the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> > > handling support.
> > > 
> > > Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> > > which can be used in case there are two separate mailbox ranges
> > > controlled by independent hardware interrupt lines, as it is
> > > the case on S32G2/S32G3 SoC.
> > 
> > Does the mainline driver already handle the 2nd mailbox range? Is there
> > any downstream code yet?
> > 
> > Marc
> > 
> 
> Hello Marc,
> 
> The mainline driver already handles the 2nd mailbox range (same
> 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
> range a separate interrupt line is used.

AFAICS the IP core supports up to 128 mailboxes, though the driver only
supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
range"? What about mailboxes 64..127, which IRQ will them?

> I do plan to upstream more patches to the flexcan driver but they relate to
> Power Management (Suspend and Resume routines) and I plan to do this in a
> separate patchset.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 3 days, 4 hours ago 
On 11/20/2024 12:01 PM, Marc Kleine-Budde wrote:
> On 20.11.2024 11:01:25, Ciprian Marian Costea wrote:
>> On 11/20/2024 10:52 AM, Marc Kleine-Budde wrote:
>>> On 19.11.2024 10:10:53, Ciprian Costea wrote:
>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>
>>>> On S32G2/S32G3 SoC, there are separate interrupts
>>>> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>>>
>>>> In order to handle this FlexCAN hardware particularity, reuse
>>>> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
>>>> handling support.
>>>>
>>>> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
>>>> which can be used in case there are two separate mailbox ranges
>>>> controlled by independent hardware interrupt lines, as it is
>>>> the case on S32G2/S32G3 SoC.
>>>
>>> Does the mainline driver already handle the 2nd mailbox range? Is there
>>> any downstream code yet?
>>>
>>> Marc
>>>
>>
>> Hello Marc,
>>
>> The mainline driver already handles the 2nd mailbox range (same
>> 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
>> range a separate interrupt line is used.
> 
> AFAICS the IP core supports up to 128 mailboxes, though the driver only
> supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
> range"? What about mailboxes 64..127, which IRQ will them?
> 

On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
- IRQ line X -> Mailboxes 0-7
- IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt 
lines 127 to 8)

By 2nd range, I was refering to Mailboxes 8-127.



>> I do plan to upstream more patches to the flexcan driver but they relate to
>> Power Management (Suspend and Resume routines) and I plan to do this in a
>> separate patchset.
> 
> regards,
> Marc
>
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 4 hours ago 
On 20.11.2024 12:18:03, Ciprian Marian Costea wrote:
> On 11/20/2024 12:01 PM, Marc Kleine-Budde wrote:
> > On 20.11.2024 11:01:25, Ciprian Marian Costea wrote:
> > > On 11/20/2024 10:52 AM, Marc Kleine-Budde wrote:
> > > > On 19.11.2024 10:10:53, Ciprian Costea wrote:
> > > > > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> > > > > 
> > > > > On S32G2/S32G3 SoC, there are separate interrupts
> > > > > for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> > > > > 
> > > > > In order to handle this FlexCAN hardware particularity, reuse
> > > > > the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> > > > > handling support.
> > > > > 
> > > > > Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> > > > > which can be used in case there are two separate mailbox ranges
> > > > > controlled by independent hardware interrupt lines, as it is
> > > > > the case on S32G2/S32G3 SoC.
> > > > 
> > > > Does the mainline driver already handle the 2nd mailbox range? Is there
> > > > any downstream code yet?
> > > > 
> > > > Marc
> > > > 
> > > 
> > > Hello Marc,
> > > 
> > > The mainline driver already handles the 2nd mailbox range (same
> > > 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
> > > range a separate interrupt line is used.
> > 
> > AFAICS the IP core supports up to 128 mailboxes, though the driver only
> > supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
> > range"? What about mailboxes 64..127, which IRQ will them?
> 
> On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
> - IRQ line X -> Mailboxes 0-7
> - IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt
> lines 127 to 8)
> 
> By 2nd range, I was refering to Mailboxes 8-127.

Interesting, do you know why it's not symmetrical (0...63, 64...127)?
Can you point me to the documentation.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 3 days, 4 hours ago 
On 11/20/2024 12:29 PM, Marc Kleine-Budde wrote:
> On 20.11.2024 12:18:03, Ciprian Marian Costea wrote:
>> On 11/20/2024 12:01 PM, Marc Kleine-Budde wrote:
>>> On 20.11.2024 11:01:25, Ciprian Marian Costea wrote:
>>>> On 11/20/2024 10:52 AM, Marc Kleine-Budde wrote:
>>>>> On 19.11.2024 10:10:53, Ciprian Costea wrote:
>>>>>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>>>>>
>>>>>> On S32G2/S32G3 SoC, there are separate interrupts
>>>>>> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>>>>>
>>>>>> In order to handle this FlexCAN hardware particularity, reuse
>>>>>> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
>>>>>> handling support.
>>>>>>
>>>>>> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
>>>>>> which can be used in case there are two separate mailbox ranges
>>>>>> controlled by independent hardware interrupt lines, as it is
>>>>>> the case on S32G2/S32G3 SoC.
>>>>>
>>>>> Does the mainline driver already handle the 2nd mailbox range? Is there
>>>>> any downstream code yet?
>>>>>
>>>>> Marc
>>>>>
>>>>
>>>> Hello Marc,
>>>>
>>>> The mainline driver already handles the 2nd mailbox range (same
>>>> 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
>>>> range a separate interrupt line is used.
>>>
>>> AFAICS the IP core supports up to 128 mailboxes, though the driver only
>>> supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
>>> range"? What about mailboxes 64..127, which IRQ will them?
>>
>> On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
>> - IRQ line X -> Mailboxes 0-7
>> - IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt
>> lines 127 to 8)
>>
>> By 2nd range, I was refering to Mailboxes 8-127.
> 
> Interesting, do you know why it's not symmetrical (0...63, 64...127)?
> Can you point me to the documentation.
> 
> Thanks,
> Marc
> 


Unfortunately I do not know why such hardware integration decisions have 
been made.

Documentation for S32G3 SoC can be found on the official NXP website, 
here: 
https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32g-vehicle-network-processors/s32g3-processors-for-vehicle-networking:S32G3

But please note that you need to setup an account beforehand.

Best Regards,
Ciprian
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 3 hours ago 
On 20.11.2024 12:47:02, Ciprian Marian Costea wrote:
> > > > > The mainline driver already handles the 2nd mailbox range (same
> > > > > 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
> > > > > range a separate interrupt line is used.
> > > > 
> > > > AFAICS the IP core supports up to 128 mailboxes, though the driver only
> > > > supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
> > > > range"? What about mailboxes 64..127, which IRQ will them?
> > > 
> > > On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
> > > - IRQ line X -> Mailboxes 0-7
> > > - IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt
> > > lines 127 to 8)
> > > 
> > > By 2nd range, I was refering to Mailboxes 8-127.
> > 
> > Interesting, do you know why it's not symmetrical (0...63, 64...127)?
> > Can you point me to the documentation.
> 
> Unfortunately I do not know why such hardware integration decisions have
> been made.
> 
> Documentation for S32G3 SoC can be found on the official NXP website,
> here:
> https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32g-vehicle-network-processors/s32g3-processors-for-vehicle-networking:S32G3
> 
> But please note that you need to setup an account beforehand.

I have that already, where is the mailbox to IRQ mapping described?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 3 days, 3 hours ago 
On 11/20/2024 12:54 PM, Marc Kleine-Budde wrote:
> On 20.11.2024 12:47:02, Ciprian Marian Costea wrote:
>>>>>> The mainline driver already handles the 2nd mailbox range (same
>>>>>> 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
>>>>>> range a separate interrupt line is used.
>>>>>
>>>>> AFAICS the IP core supports up to 128 mailboxes, though the driver only
>>>>> supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
>>>>> range"? What about mailboxes 64..127, which IRQ will them?
>>>>
>>>> On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
>>>> - IRQ line X -> Mailboxes 0-7
>>>> - IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt
>>>> lines 127 to 8)
>>>>
>>>> By 2nd range, I was refering to Mailboxes 8-127.
>>>
>>> Interesting, do you know why it's not symmetrical (0...63, 64...127)?
>>> Can you point me to the documentation.
>>
>> Unfortunately I do not know why such hardware integration decisions have
>> been made.
>>
>> Documentation for S32G3 SoC can be found on the official NXP website,
>> here:
>> https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32g-vehicle-network-processors/s32g3-processors-for-vehicle-networking:S32G3
>>
>> But please note that you need to setup an account beforehand.
> 
> I have that already, where is the mailbox to IRQ mapping described?
> 
> regards,
> Marc
> 

If you have successfully downloaded the Reference Manual for S32G2 or 
S32G3 SoC, it should have attached an excel file describing all the 
interrupt mappings.

In the excel file, if you search for 'FlexCAN_0' for example, you should 
be able to find IRQ lines 39 and 40 which correspond to Maiboxes 0-7 and 
8-129 (ored) previously discussed.

Best Regards,
Ciprian
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 3 hours ago 
On 20.11.2024 13:02:56, Ciprian Marian Costea wrote:
> On 11/20/2024 12:54 PM, Marc Kleine-Budde wrote:
> > On 20.11.2024 12:47:02, Ciprian Marian Costea wrote:
> > > > > > > The mainline driver already handles the 2nd mailbox range (same
> > > > > > > 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
> > > > > > > range a separate interrupt line is used.
> > > > > > 
> > > > > > AFAICS the IP core supports up to 128 mailboxes, though the driver only
> > > > > > supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
> > > > > > range"? What about mailboxes 64..127, which IRQ will them?
> > > > > 
> > > > > On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
> > > > > - IRQ line X -> Mailboxes 0-7
> > > > > - IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt
> > > > > lines 127 to 8)
> > > > > 
> > > > > By 2nd range, I was refering to Mailboxes 8-127.
> > > > 
> > > > Interesting, do you know why it's not symmetrical (0...63, 64...127)?
> > > > Can you point me to the documentation.
> > > 
> > > Unfortunately I do not know why such hardware integration decisions have
> > > been made.
> > > 
> > > Documentation for S32G3 SoC can be found on the official NXP website,
> > > here:
> > > https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32g-vehicle-network-processors/s32g3-processors-for-vehicle-networking:S32G3
> > > 
> > > But please note that you need to setup an account beforehand.
> > 
> > I have that already, where is the mailbox to IRQ mapping described?
> > 
> > regards,
> > Marc
> > 
> 
> If you have successfully downloaded the Reference Manual for S32G2 or S32G3
> SoC, it should have attached an excel file describing all the interrupt
> mappings.

I downloaded the S32G3 Reference Manual:

| https://www.nxp.com/webapp/Download?colCode=RMS32G3

It's a pdf. Where can I find the execl file?

> In the excel file, if you search for 'FlexCAN_0' for example, you should be
> able to find IRQ lines 39 and 40 which correspond to Maiboxes 0-7 and 8-129
> (ored) previously discussed.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 3 days, 3 hours ago 
On 11/20/2024 1:33 PM, Marc Kleine-Budde wrote:
> On 20.11.2024 13:02:56, Ciprian Marian Costea wrote:
>> On 11/20/2024 12:54 PM, Marc Kleine-Budde wrote:
>>> On 20.11.2024 12:47:02, Ciprian Marian Costea wrote:
>>>>>>>> The mainline driver already handles the 2nd mailbox range (same
>>>>>>>> 'flexcan_irq') is used. The only difference is that for the 2nd mailbox
>>>>>>>> range a separate interrupt line is used.
>>>>>>>
>>>>>>> AFAICS the IP core supports up to 128 mailboxes, though the driver only
>>>>>>> supports 64 mailboxes. Which mailboxes do you mean by the "2nd mailbox
>>>>>>> range"? What about mailboxes 64..127, which IRQ will them?
>>>>>>
>>>>>> On S32G the following is the mapping between FlexCAN IRQs and mailboxes:
>>>>>> - IRQ line X -> Mailboxes 0-7
>>>>>> - IRQ line Y -> Mailboxes 8-127 (Logical OR of Message Buffer Interrupt
>>>>>> lines 127 to 8)
>>>>>>
>>>>>> By 2nd range, I was refering to Mailboxes 8-127.
>>>>>
>>>>> Interesting, do you know why it's not symmetrical (0...63, 64...127)?
>>>>> Can you point me to the documentation.
>>>>
>>>> Unfortunately I do not know why such hardware integration decisions have
>>>> been made.
>>>>
>>>> Documentation for S32G3 SoC can be found on the official NXP website,
>>>> here:
>>>> https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32g-vehicle-network-processors/s32g3-processors-for-vehicle-networking:S32G3
>>>>
>>>> But please note that you need to setup an account beforehand.
>>>
>>> I have that already, where is the mailbox to IRQ mapping described?
>>>
>>> regards,
>>> Marc
>>>
>>
>> If you have successfully downloaded the Reference Manual for S32G2 or S32G3
>> SoC, it should have attached an excel file describing all the interrupt
>> mappings.
> 
> I downloaded the S32G3 Reference Manual:
> 
> | https://www.nxp.com/webapp/Download?colCode=RMS32G3
> 
> It's a pdf. Where can I find the execl file?

Correct, and in the software used after opening the pdf file (Adobe 
Acrobat Reader, Foxit PDF Reader, etc.) you should be able to find some 
excel files attached to it.

Regards,
Ciprian

> 
>> In the excel file, if you search for 'FlexCAN_0' for example, you should be
>> able to find IRQ lines 39 and 40 which correspond to Maiboxes 0-7 and 8-129
>> (ored) previously discussed.
> 
> regards,
> Marc
>
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 3 days, 2 hours ago 
On 20.11.2024 13:42:18, Ciprian Marian Costea wrote:
> > > If you have successfully downloaded the Reference Manual for S32G2 or S32G3
> > > SoC, it should have attached an excel file describing all the interrupt
> > > mappings.
> > 
> > I downloaded the S32G3 Reference Manual:
> > 
> > | https://www.nxp.com/webapp/Download?colCode=RMS32G3
> > 
> > It's a pdf. Where can I find the execl file?
> 
> Correct, and in the software used after opening the pdf file (Adobe Acrobat
> Reader, Foxit PDF Reader, etc.) you should be able to find some excel files
> attached to it.

These are not available under Linux (Adobe), or you have to pay (foxit).
Can you recommend a Linux reader?

I've found zathura can extract attached files, use the "export" command
for this:

| export Export attachments. First argument specifies the attachment
| identifier (use completion with Tab), second argument gives the target
| filename (relative to current working directory).

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Vincent Mailhol 4 days, 5 hours ago 
On 19/11/2024 at 17:10, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> On S32G2/S32G3 SoC, there are separate interrupts
> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> 
> In order to handle this FlexCAN hardware particularity, reuse
> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> handling support.
> 
> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> which can be used in case there are two separate mailbox ranges
> controlled by independent hardware interrupt lines, as it is
> the case on S32G2/S32G3 SoC.
> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>  drivers/net/can/flexcan/flexcan.h      |  3 +++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f0dee04800d3..dc56d4a7d30b 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>  		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
> -		FLEXCAN_QUIRK_SUPPORT_ECC |
> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>  		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>  };
>  
>  static const struct can_bittiming_const flexcan_bittiming_const = {
> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>  			goto out_free_irq_boff;
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		err = request_irq(priv->irq_secondary_mb,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_free_irq_err;
> +	}

Is the logic here correct?

  request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);

is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.

So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
not initialized.

Did you confirm if it is safe to call free_irq() on an uninitialized irq?

(and I can see that currently there is no such device with
FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
who knows if such device will be introduced in the future?)

>  	flexcan_chip_interrupts_enable(dev);
>  
>  	netif_start_queue(dev);
>  
>  	return 0;
>  
> + out_free_irq_err:
> +	free_irq(priv->irq_err, dev);
>   out_free_irq_boff:
>  	free_irq(priv->irq_boff, dev);
>   out_free_irq:
> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>  		free_irq(priv->irq_boff, dev);
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> +		free_irq(priv->irq_secondary_mb, dev);
> +
>  	free_irq(dev->irq, dev);
>  	can_rx_offload_disable(&priv->offload);
>  	flexcan_chip_stop_disable_on_error(dev);
> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
> +		if (priv->irq_secondary_mb < 0) {
> +			err = priv->irq_secondary_mb;
> +			goto failed_platform_get_irq;
> +		}
> +	}
> +
>  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>  			CAN_CTRLMODE_FD_NON_ISO;
> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
> index 4933d8c7439e..d4b1a954c538 100644
> --- a/drivers/net/can/flexcan/flexcan.h
> +++ b/drivers/net/can/flexcan/flexcan.h
> @@ -70,6 +70,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>  /* Setup stop mode with ATF SCMI protocol to support wakeup */
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
> +/* Setup secondary mailbox interrupt */
> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>  
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
> @@ -105,6 +107,7 @@ struct flexcan_priv {
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
>  
> +	int irq_secondary_mb;
>  	int irq_boff;
>  	int irq_err;
>  

Yours sincerely,
Vincent Mailhol
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 4 days, 4 hours ago 
On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
> On 19/11/2024 at 17:10, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> On S32G2/S32G3 SoC, there are separate interrupts
>> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>
>> In order to handle this FlexCAN hardware particularity, reuse
>> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
>> handling support.
>>
>> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
>> which can be used in case there are two separate mailbox ranges
>> controlled by independent hardware interrupt lines, as it is
>> the case on S32G2/S32G3 SoC.
>>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>> ---
>>   drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>>   drivers/net/can/flexcan/flexcan.h      |  3 +++
>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
>> index f0dee04800d3..dc56d4a7d30b 100644
>> --- a/drivers/net/can/flexcan/flexcan-core.c
>> +++ b/drivers/net/can/flexcan/flexcan-core.c
>> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>>   	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>>   		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>>   		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
>> -		FLEXCAN_QUIRK_SUPPORT_ECC |
>> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>>   		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
>> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
>> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
>> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>>   };
>>   
>>   static const struct can_bittiming_const flexcan_bittiming_const = {
>> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>>   			goto out_free_irq_boff;
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		err = request_irq(priv->irq_secondary_mb,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_free_irq_err;
>> +	}
> 
> Is the logic here correct?
> 
>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
> 
> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
> 
> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
> not initialized.
> 
> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
> 
> (and I can see that currently there is no such device with
> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
> who knows if such device will be introduced in the future?)
> 

Hello Vincent,

Thanks for your review. Indeed this seems to be an incorrect logic since 
I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3' 
and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.

I will change the impacted section to:
	if (err) {
		if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
			goto out_free_irq_err;
		else
			goto out_free_irq;
	}

Best Regards,
Ciprian

>>   	flexcan_chip_interrupts_enable(dev);
>>   
>>   	netif_start_queue(dev);
>>   
>>   	return 0;
>>   
>> + out_free_irq_err:
>> +	free_irq(priv->irq_err, dev);
>>    out_free_irq_boff:
>>   	free_irq(priv->irq_boff, dev);
>>    out_free_irq:
>> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>>   		free_irq(priv->irq_boff, dev);
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
>> +		free_irq(priv->irq_secondary_mb, dev);
>> +
>>   	free_irq(dev->irq, dev);
>>   	can_rx_offload_disable(&priv->offload);
>>   	flexcan_chip_stop_disable_on_error(dev);
>> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
>> +		if (priv->irq_secondary_mb < 0) {
>> +			err = priv->irq_secondary_mb;
>> +			goto failed_platform_get_irq;
>> +		}
>> +	}
>> +
>>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>>   		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>>   			CAN_CTRLMODE_FD_NON_ISO;
>> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
>> index 4933d8c7439e..d4b1a954c538 100644
>> --- a/drivers/net/can/flexcan/flexcan.h
>> +++ b/drivers/net/can/flexcan/flexcan.h
>> @@ -70,6 +70,8 @@
>>   #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>>   /* Setup stop mode with ATF SCMI protocol to support wakeup */
>>   #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
>> +/* Setup secondary mailbox interrupt */
>> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>>   
>>   struct flexcan_devtype_data {
>>   	u32 quirks;		/* quirks needed for different IP cores */
>> @@ -105,6 +107,7 @@ struct flexcan_priv {
>>   	struct regulator *reg_xceiver;
>>   	struct flexcan_stop_mode stm;
>>   
>> +	int irq_secondary_mb;
>>   	int irq_boff;
>>   	int irq_err;
>>   
> 
> Yours sincerely,
> Vincent Mailhol
>
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Vincent Mailhol 4 days, 3 hours ago 
On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
> On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
>> On 19/11/2024 at 17:10, Ciprian Costea wrote:

(...)

>>>   +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>>> +        err = request_irq(priv->irq_secondary_mb,
>>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
>>> +        if (err)
>>> +            goto out_free_irq_err;
>>> +    }
>>
>> Is the logic here correct?
>>
>>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>
>> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
>>
>> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
>> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
>> not initialized.
>>
>> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
>>
>> (and I can see that currently there is no such device with
>> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
>> who knows if such device will be introduced in the future?)
>>
> 
> Hello Vincent,
> 
> Thanks for your review. Indeed this seems to be an incorrect logic since
> I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
> and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
> 
> I will change the impacted section to:
>     if (err) {
>         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>             goto out_free_irq_err;
>         else
>             goto out_free_irq;
>     }

This is better. Alternatively, you could move the check into the label:

  out_free_irq_err:
  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
  		free_irq(priv->irq_err, dev);

But this is not a strong preference, I let you pick the one which you
prefer.

>>>       flexcan_chip_interrupts_enable(dev);
>>>         netif_start_queue(dev);
>>>         return 0;
>>>   + out_free_irq_err:
>>> +    free_irq(priv->irq_err, dev);
>>>    out_free_irq_boff:
>>>       free_irq(priv->irq_boff, dev);
>>>    out_free_irq:

(...)


Yours sincerely,
Vincent Mailhol
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Vincent Mailhol 4 days, 3 hours ago 
On 19/11/2024 at 20:26, Vincent Mailhol wrote:
> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
>> On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
>>> On 19/11/2024 at 17:10, Ciprian Costea wrote:
> 
> (...)
> 
>>>>   +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>>>> +        err = request_irq(priv->irq_secondary_mb,
>>>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>> +        if (err)
>>>> +            goto out_free_irq_err;
>>>> +    }
>>>
>>> Is the logic here correct?
>>>
>>>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>
>>> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
>>>
>>> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
>>> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
>>> not initialized.
>>>
>>> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
>>>
>>> (and I can see that currently there is no such device with
>>> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
>>> who knows if such device will be introduced in the future?)
>>>
>>
>> Hello Vincent,
>>
>> Thanks for your review. Indeed this seems to be an incorrect logic since
>> I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
>> and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
>>
>> I will change the impacted section to:
>>     if (err) {
>>         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>>             goto out_free_irq_err;
>>         else
>>             goto out_free_irq;
>>     }
> 
> This is better. Alternatively, you could move the check into the label:
> 
>   out_free_irq_err:
>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>   		free_irq(priv->irq_err, dev);
> 
> But this is not a strong preference, I let you pick the one which you
> prefer.

On second thought, it is a strong preference. If you keep the

	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
		goto out_free_irq_err;
	else
		goto out_free_irq;

then what if more code with a clean-up label is added to flexcan_open()?
I am thinking of this:

  out_free_foo:
  	free(foo);
  out_free_irq_err:
  	free_irq(priv-irq_err, dev);
  out_free_irq_boff:
  	free_irq(priv->irq_boff, dev);

Jumping to out_free_foo would now be incorrect because the
out_free_irq_err label would also be visited.

>>>>       flexcan_chip_interrupts_enable(dev);
>>>>         netif_start_queue(dev);
>>>>         return 0;
>>>>   + out_free_irq_err:
>>>> +    free_irq(priv->irq_err, dev);
>>>>    out_free_irq_boff:
>>>>       free_irq(priv->irq_boff, dev);
>>>>    out_free_irq:
> 
> (...)

Yours sincerely,
Vincent Mailhol
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Ciprian Marian Costea 4 days, 3 hours ago 
On 11/19/2024 1:36 PM, Vincent Mailhol wrote:
> On 19/11/2024 at 20:26, Vincent Mailhol wrote:
>> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
>>> On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
>>>> On 19/11/2024 at 17:10, Ciprian Costea wrote:
>>
>> (...)
>>
>>>>>    +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>>>>> +        err = request_irq(priv->irq_secondary_mb,
>>>>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>>> +        if (err)
>>>>> +            goto out_free_irq_err;
>>>>> +    }
>>>>
>>>> Is the logic here correct?
>>>>
>>>>     request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>>
>>>> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
>>>>
>>>> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
>>>> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
>>>> not initialized.
>>>>
>>>> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
>>>>
>>>> (and I can see that currently there is no such device with
>>>> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
>>>> who knows if such device will be introduced in the future?)
>>>>
>>>
>>> Hello Vincent,
>>>
>>> Thanks for your review. Indeed this seems to be an incorrect logic since
>>> I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
>>> and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
>>>
>>> I will change the impacted section to:
>>>      if (err) {
>>>          if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>>>              goto out_free_irq_err;
>>>          else
>>>              goto out_free_irq;
>>>      }
>>
>> This is better. Alternatively, you could move the check into the label:
>>
>>    out_free_irq_err:
>>    	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>>    		free_irq(priv->irq_err, dev);
>>
>> But this is not a strong preference, I let you pick the one which you
>> prefer.
> 
> On second thought, it is a strong preference. If you keep the
> 
> 	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> 		goto out_free_irq_err;
> 	else
> 		goto out_free_irq;
> 
> then what if more code with a clean-up label is added to flexcan_open()?
> I am thinking of this:
> 
>    out_free_foo:
>    	free(foo);
>    out_free_irq_err:
>    	free_irq(priv-irq_err, dev);
>    out_free_irq_boff:
>    	free_irq(priv->irq_boff, dev);
> 
> Jumping to out_free_foo would now be incorrect because the
> out_free_irq_err label would also be visited.
> 

Correct, moving the check under the label would be better. Thanks.
I will change accordingly in V2.


Best Regards,
Ciprian

>>>>>        flexcan_chip_interrupts_enable(dev);
>>>>>          netif_start_queue(dev);
>>>>>          return 0;
>>>>>    + out_free_irq_err:
>>>>> +    free_irq(priv->irq_err, dev);
>>>>>     out_free_irq_boff:
>>>>>        free_irq(priv->irq_boff, dev);
>>>>>     out_free_irq:
>>
>> (...)
> 
> Yours sincerely,
> Vincent Mailhol
>
Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines
Posted by Marc Kleine-Budde 4 days, 3 hours ago 
On 19.11.2024 20:26:26, Vincent Mailhol wrote:
> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
> > On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
> >> On 19/11/2024 at 17:10, Ciprian Costea wrote:
> 
> (...)
> 
> >>>   +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> >>> +        err = request_irq(priv->irq_secondary_mb,
> >>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
> >>> +        if (err)
> >>> +            goto out_free_irq_err;
> >>> +    }
> >>
> >> Is the logic here correct?
> >>
> >>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
> >>
> >> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
> >>
> >> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
> >> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
> >> not initialized.
> >>
> >> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
> >>
> >> (and I can see that currently there is no such device with
> >> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
> >> who knows if such device will be introduced in the future?)
> >>
> > 
> > Hello Vincent,
> > 
> > Thanks for your review. Indeed this seems to be an incorrect logic since
> > I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
> > and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
> > 
> > I will change the impacted section to:
> >     if (err) {
> >         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> >             goto out_free_irq_err;
> >         else
> >             goto out_free_irq;
> >     }
> 
> This is better. Alternatively, you could move the check into the label:

+1

>   out_free_irq_err:
>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>   		free_irq(priv->irq_err, dev);
> 
> But this is not a strong preference, I let you pick the one which you
> prefer.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.