[PATCH 0/3] add FlexCAN support for S32G2/S32G3 SoCs

[PATCH 0/3] add FlexCAN support for S32G2/S32G3 SoCs

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

S32G2 and S32G3 SoCs share the FlexCAN module with i.MX SoCs, with some
hardware integration particularities.

Main difference covered by this patchset relates to interrupt management.
On S32G2/S32G3 SoC, there are separate interrupts for state change, bus
errors, MBs 0-7 and MBs 8-127 respectively.

Ciprian Marian Costea (3):
  dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support
  can: flexcan: add NXP S32G2/S32G3 SoC support
  can: flexcan: handle S32G2/S32G3 separate interrupt lines

 .../bindings/net/can/fsl,flexcan.yaml         | 25 +++++++++++++--
 drivers/net/can/flexcan/flexcan-core.c        | 31 +++++++++++++++++++
 drivers/net/can/flexcan/flexcan.h             |  3 ++
 3 files changed, 56 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Add S32G2/S32G3 SoCs compatible strings.

A particularity for these SoCs is the presence of separate interrupts for
state change, bus errors, MBs 0-7 and MBs 8-127 respectively.

Increase maxItems of 'interrupts' to 4 for S32G based SoCs and keep the
same restriction for other SoCs.

Also, as part of this commit, move the 'allOf' after the required
properties to make the documentation easier to read.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 .../bindings/net/can/fsl,flexcan.yaml         | 25 ++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
index 97dd1a7c5ed2..cb7204c06acf 100644
--- a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
+++ b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
@@ -10,9 +10,6 @@ title:
 maintainers:
   - Marc Kleine-Budde <mkl@pengutronix.de>
 
-allOf:
-  - $ref: can-controller.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -28,6 +25,7 @@ properties:
           - fsl,vf610-flexcan
           - fsl,ls1021ar2-flexcan
           - fsl,lx2160ar1-flexcan
+          - nxp,s32g2-flexcan
       - items:
           - enum:
               - fsl,imx53-flexcan
@@ -43,6 +41,10 @@ properties:
           - enum:
               - fsl,ls1028ar1-flexcan
           - const: fsl,lx2160ar1-flexcan
+      - items:
+          - enum:
+              - nxp,s32g3-flexcan
+          - const: nxp,s32g2-flexcan
 
   reg:
     maxItems: 1
@@ -136,6 +138,23 @@ required:
   - reg
   - interrupts
 
+allOf:
+  - $ref: can-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: nxp,s32g2-flexcan
+    then:
+      properties:
+        interrupts:
+          minItems: 4
+          maxItems: 4
+    else:
+      properties:
+        interrupts:
+          maxItems: 1
+
 additionalProperties: false
 
 examples:
-- 
2.45.2

Re: [PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Frank Li]

On Tue, Nov 19, 2024 at 10:10:51AM +0200, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>
> Add S32G2/S32G3 SoCs compatible strings.
>
> A particularity for these SoCs is the presence of separate interrupts for
> state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>
> Increase maxItems of 'interrupts' to 4 for S32G based SoCs and keep the
> same restriction for other SoCs.
>
> Also, as part of this commit, move the 'allOf' after the required
> properties to make the documentation easier to read.
>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  .../bindings/net/can/fsl,flexcan.yaml         | 25 ++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
> index 97dd1a7c5ed2..cb7204c06acf 100644
> --- a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
> +++ b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
> @@ -10,9 +10,6 @@ title:
>  maintainers:
>    - Marc Kleine-Budde <mkl@pengutronix.de>
>
> -allOf:
> -  - $ref: can-controller.yaml#
> -
>  properties:
>    compatible:
>      oneOf:
> @@ -28,6 +25,7 @@ properties:
>            - fsl,vf610-flexcan
>            - fsl,ls1021ar2-flexcan
>            - fsl,lx2160ar1-flexcan
> +          - nxp,s32g2-flexcan
>        - items:
>            - enum:
>                - fsl,imx53-flexcan
> @@ -43,6 +41,10 @@ properties:
>            - enum:
>                - fsl,ls1028ar1-flexcan
>            - const: fsl,lx2160ar1-flexcan
> +      - items:
> +          - enum:
> +              - nxp,s32g3-flexcan
> +          - const: nxp,s32g2-flexcan
>
>    reg:
>      maxItems: 1
> @@ -136,6 +138,23 @@ required:
>    - reg
>    - interrupts
>
> +allOf:
> +  - $ref: can-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: nxp,s32g2-flexcan
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 4
> +          maxItems: 4
> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +
>  additionalProperties: false
>
>  examples:
> --
> 2.45.2
>

Re: [PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Krzysztof Kozlowski]

On Tue, Nov 19, 2024 at 10:10:51AM +0200, Ciprian Costea wrote:
>    reg:
>      maxItems: 1
> @@ -136,6 +138,23 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - $ref: can-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: nxp,s32g2-flexcan
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 4
> +          maxItems: 4

Top level says max is 1. You need to keep there widest constraints.

> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Krzysztof Kozlowski]

On 20/11/2024 09:45, Krzysztof Kozlowski wrote:
> On Tue, Nov 19, 2024 at 10:10:51AM +0200, Ciprian Costea wrote:
>>    reg:
>>      maxItems: 1
>> @@ -136,6 +138,23 @@ required:
>>    - reg
>>    - interrupts
>>  
>> +allOf:
>> +  - $ref: can-controller.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: nxp,s32g2-flexcan
>> +    then:
>> +      properties:
>> +        interrupts:
>> +          minItems: 4
>> +          maxItems: 4
> 
> Top level says max is 1. You need to keep there widest constraints.
And list items here instead...

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/20/2024 11:12 AM, Krzysztof Kozlowski wrote:
> On 20/11/2024 09:45, Krzysztof Kozlowski wrote:
>> On Tue, Nov 19, 2024 at 10:10:51AM +0200, Ciprian Costea wrote:
>>>     reg:
>>>       maxItems: 1
>>> @@ -136,6 +138,23 @@ required:
>>>     - reg
>>>     - interrupts
>>>   
>>> +allOf:
>>> +  - $ref: can-controller.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: nxp,s32g2-flexcan
>>> +    then:
>>> +      properties:
>>> +        interrupts:
>>> +          minItems: 4
>>> +          maxItems: 4
>>
>> Top level says max is 1. You need to keep there widest constraints.
> And list items here instead...
> 
> Best regards,
> Krzysztof

Hello Krzysztof,

Just to confirm before making any changes:
Are you referring to directly change 'maxItems' to value 4 ? Instead of 
using this 'if' condition under 'allOf' ?

Best Regards,
Ciprian

Re: [PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Marc Kleine-Budde]

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

On 19.11.2024 10:10:51, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> Add S32G2/S32G3 SoCs compatible strings.
> 
> A particularity for these SoCs is the presence of separate interrupts for
> state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> 
> Increase maxItems of 'interrupts' to 4 for S32G based SoCs and keep the
> same restriction for other SoCs.

Can you add an "interrupt-names" property?

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/20/2024 10:49 AM, Marc Kleine-Budde wrote:
> On 19.11.2024 10:10:51, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add S32G2/S32G3 SoCs compatible strings.
>>
>> A particularity for these SoCs is the presence of separate interrupts for
>> state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>
>> Increase maxItems of 'interrupts' to 4 for S32G based SoCs and keep the
>> same restriction for other SoCs.
> 
> Can you add an "interrupt-names" property?
> 
> regards,
> Marc
> 

Yes, I will add "interrupt-names" property under the if condition from 
'allOf' in V2.

Best Regards,
Ciprian

[PATCH 2/3] can: flexcan: add NXP S32G2/S32G3 SoC support

[Ciprian Costea]

From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Add device type data for S32G2/S32G3 SoC.

FlexCAN module from S32G2/S32G3 is similar with i.MX SoCs, but interrupt
management is different. This initial S32G2/S32G3 SoC FlexCAN support
paves the road to address such differences.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/flexcan/flexcan-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index ac1a860986df..f0dee04800d3 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -386,6 +386,15 @@ static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
 		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
 };
 
+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_RX_MAILBOX |
+		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
+};
+
 static const struct can_bittiming_const flexcan_bittiming_const = {
 	.name = DRV_NAME,
 	.tseg1_min = 4,
@@ -2041,6 +2050,7 @@ static const struct of_device_id flexcan_of_match[] = {
 	{ .compatible = "fsl,vf610-flexcan", .data = &fsl_vf610_devtype_data, },
 	{ .compatible = "fsl,ls1021ar2-flexcan", .data = &fsl_ls1021a_r2_devtype_data, },
 	{ .compatible = "fsl,lx2160ar1-flexcan", .data = &fsl_lx2160a_r1_devtype_data, },
+	{ .compatible = "nxp,s32g2-flexcan", .data = &nxp_s32g2_devtype_data, },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, flexcan_of_match);
-- 
2.45.2

Re: [PATCH 2/3] can: flexcan: add NXP S32G2/S32G3 SoC support

[Frank Li]

On Tue, Nov 19, 2024 at 10:10:52AM +0200, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>
> Add device type data for S32G2/S32G3 SoC.
>
> FlexCAN module from S32G2/S32G3 is similar with i.MX SoCs, but interrupt
> management is different. This initial S32G2/S32G3 SoC FlexCAN support
> paves the road to address such differences.
>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/net/can/flexcan/flexcan-core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index ac1a860986df..f0dee04800d3 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -386,6 +386,15 @@ static const struct flexcan_devtype_data fsl_lx2160a_r1_devtype_data = {
>  		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
>  };
>
> +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_RX_MAILBOX |
> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
> +};
> +
>  static const struct can_bittiming_const flexcan_bittiming_const = {
>  	.name = DRV_NAME,
>  	.tseg1_min = 4,
> @@ -2041,6 +2050,7 @@ static const struct of_device_id flexcan_of_match[] = {
>  	{ .compatible = "fsl,vf610-flexcan", .data = &fsl_vf610_devtype_data, },
>  	{ .compatible = "fsl,ls1021ar2-flexcan", .data = &fsl_ls1021a_r2_devtype_data, },
>  	{ .compatible = "fsl,lx2160ar1-flexcan", .data = &fsl_lx2160a_r1_devtype_data, },
> +	{ .compatible = "nxp,s32g2-flexcan", .data = &nxp_s32g2_devtype_data, },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, flexcan_of_match);
> --
> 2.45.2
>

Re: [PATCH 2/3] can: flexcan: add NXP S32G2/S32G3 SoC support

[Marc Kleine-Budde]

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

On 19.11.2024 10:10:52, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> Add device type data for S32G2/S32G3 SoC.
> 
> FlexCAN module from S32G2/S32G3 is similar with i.MX SoCs, but interrupt
> management is different. This initial S32G2/S32G3 SoC FlexCAN support
> paves the road to address such differences.
> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

If this flexcan integration has separate IRQ lines for Bus-Off and Error
IRQs, please add the FLEXCAN_QUIRK_NR_IRQ_3 in this initial patch.

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] can: flexcan: add NXP S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/20/2024 11:01 AM, Marc Kleine-Budde wrote:
> On 19.11.2024 10:10:52, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> Add device type data for S32G2/S32G3 SoC.
>>
>> FlexCAN module from S32G2/S32G3 is similar with i.MX SoCs, but interrupt
>> management is different. This initial S32G2/S32G3 SoC FlexCAN support
>> paves the road to address such differences.
>>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> If this flexcan integration has separate IRQ lines for Bus-Off and Error
> IRQs, please add the FLEXCAN_QUIRK_NR_IRQ_3 in this initial patch.
> 
> regards,
> Marc
> 

Indeed the FlexCAN integration on S32G has separate IRQ lines for 
Bus-Off and Error. I will add 'FLEXCAN_QUIRK_NR_IRQ_3' quirk into the 
initial S32G FlexCAN support commit as suggested, in V2.

Best Regards,
Ciprian

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

[Ciprian Costea]

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

[Vincent Mailhol]

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

[Ciprian Marian Costea]

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

[Vincent Mailhol]

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

[Marc Kleine-Budde]

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

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vincent Mailhol]

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

[Ciprian Marian Costea]

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

[Marc Kleine-Budde]

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

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ciprian Marian Costea]

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

[Marc Kleine-Budde]

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

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ciprian Marian Costea]

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

[Marc Kleine-Budde]

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

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ciprian Marian Costea]

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

[Marc Kleine-Budde]

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

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ciprian Marian Costea]

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

[Marc Kleine-Budde]

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

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ciprian Marian Costea]

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
>