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

[PATCH v2 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.

Changes in V2:
- Separated 'FLEXCAN_QUIRK_NR_IRQ_3' quirk addition from S32G SoC Flexcan
  support.
- Provided more information in dt-bindings documentation with respect to
  FlexCAN module integration on S32G SoCs.
- Fixed and irq resource freeing management issue.

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

 .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
 drivers/net/can/flexcan/flexcan-core.c        | 35 +++++++++++++-
 drivers/net/can/flexcan/flexcan.h             |  5 ++
 3 files changed, 81 insertions(+), 5 deletions(-)

-- 
2.45.2

[PATCH v2 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>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml
index 97dd1a7c5ed2..b2c16a7d864c 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,12 +41,21 @@ properties:
           - enum:
               - fsl,ls1028ar1-flexcan
           - const: fsl,lx2160ar1-flexcan
+      - items:
+          - enum:
+              - nxp,s32g3-flexcan
+          - const: nxp,s32g2-flexcan
 
   reg:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 4
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 4
 
   clocks:
     maxItems: 2
@@ -136,6 +143,37 @@ required:
   - reg
   - interrupts
 
+allOf:
+  - $ref: can-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: nxp,s32g2-flexcan
+    then:
+      properties:
+        interrupts:
+          items:
+            - description:
+                Message Buffer interrupt for mailboxes 0-7
+            - description:
+                Interrupt indicating that the CAN bus went to Buss Off state
+            - description:
+                Interrupt indicating that errors were detected on the CAN bus
+            - description:
+                Message Buffer interrupt for mailboxes 8-127 (ored)
+        interrupt-names:
+          items:
+            - const: mb_0-7
+            - const: state
+            - const: berr
+            - const: mb_8-127
+      required:
+        - compatible
+        - reg
+        - interrupts
+        - interrupt-names
+
 additionalProperties: false
 
 examples:
-- 
2.45.2

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

[Krzysztof Kozlowski]

On Mon, Nov 25, 2024 at 06:31:00PM +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>

You made multiple changes afterwards, which invalidated the review. See
submitting-patches which explain what to do in such case.

> ---
>  .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
>  1 file changed, 42 insertions(+), 4 deletions(-)

...

>      maxItems: 2
> @@ -136,6 +143,37 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - $ref: can-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: nxp,s32g2-flexcan
> +    then:
> +      properties:
> +        interrupts:
> +          items:
> +            - description:
> +                Message Buffer interrupt for mailboxes 0-7

Keep it in one line.

> +            - description:
> +                Interrupt indicating that the CAN bus went to Buss Off state

s/Interrupt indicating that//
Buss Off state status?

> +            - description:
> +                Interrupt indicating that errors were detected on the CAN bus

Error detection?

> +            - description:
> +                Message Buffer interrupt for mailboxes 8-127 (ored)
> +        interrupt-names:
> +          items:
> +            - const: mb_0-7

Choose one: either underscores or hyphens. Keep it consistent in your
bindings.

> +            - const: state
> +            - const: berr
> +            - const: mb_8-127

Choose one: either underscores or hyphens. Keep it consistent in your
bindings.

> +      required:
> +        - compatible
> +        - reg
> +        - interrupts
> +        - interrupt-names

What happened to "else:"? Why all other devices now have up to 4 interrupts?

Best regards,
Krzysztof

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

[Ciprian Marian Costea]

On 11/26/2024 9:19 AM, Krzysztof Kozlowski wrote:
> On Mon, Nov 25, 2024 at 06:31:00PM +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>
> 

Hello Krzysztof,

Thanks for your time in reviewing this patch.

> You made multiple changes afterwards, which invalidated the review. See
> submitting-patches which explain what to do in such case.
> 

I will remove the tag in V3 and add this info in the changelog.

>> ---
>>   .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
>>   1 file changed, 42 insertions(+), 4 deletions(-)
> 
> ...
> 
>>       maxItems: 2
>> @@ -136,6 +143,37 @@ required:
>>     - reg
>>     - interrupts
>>   
>> +allOf:
>> +  - $ref: can-controller.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: nxp,s32g2-flexcan
>> +    then:
>> +      properties:
>> +        interrupts:
>> +          items:
>> +            - description:
>> +                Message Buffer interrupt for mailboxes 0-7
> 
> Keep it in one line.

I will update in V3.

> 
>> +            - description:
>> +                Interrupt indicating that the CAN bus went to Buss Off state
> 
> s/Interrupt indicating that//
> Buss Off state status?
> 
>> +            - description:
>> +                Interrupt indicating that errors were detected on the CAN bus
> 
> Error detection?

I will rephrase in V2.

> 
>> +            - description:
>> +                Message Buffer interrupt for mailboxes 8-127 (ored)
>> +        interrupt-names:
>> +          items:
>> +            - const: mb_0-7
> 
> Choose one: either underscores or hyphens. Keep it consistent in your
> bindings.

Makes sense. I will update in V3.

> 
>> +            - const: state
>> +            - const: berr
>> +            - const: mb_8-127
> 
> Choose one: either underscores or hyphens. Keep it consistent in your
> bindings.
> 

I will update in V3.

>> +      required:
>> +        - compatible
>> +        - reg
>> +        - interrupts
>> +        - interrupt-names
> 
> What happened to "else:"? Why all other devices now have up to 4 interrupts?
> 
> Best regards,
> Krzysztof
> 

I will add the 'else' branch back in V3.

Best Regards,
Ciprian

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

[Marc Kleine-Budde]

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

On 26.11.2024 08:19:04, Krzysztof Kozlowski wrote:
> On Mon, Nov 25, 2024 at 06:31:00PM +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>
> 
> You made multiple changes afterwards, which invalidated the review. See
> submitting-patches which explain what to do in such case.
> 
> > ---
> >  .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
> >  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> ...
> 
> >      maxItems: 2
> > @@ -136,6 +143,37 @@ required:
> >    - reg
> >    - interrupts
> >  
> > +allOf:
> > +  - $ref: can-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: nxp,s32g2-flexcan
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          items:
> > +            - description:
> > +                Message Buffer interrupt for mailboxes 0-7
> 
> Keep it in one line.

According to the excel sheet the IRQ is also for the enhanced RX FIFO.

> 
> > +            - description:
> > +                Interrupt indicating that the CAN bus went to Buss Off state
> 
> s/Interrupt indicating that//
> Buss Off state status?

What about: "Device went into Bus Off state"

However from the excel sheet I read it as a device changes state, to Bus
Off, finished Bus Off or transition from error counters from < 96 to >= 96.

So "Device state change" would be a more complete description?

> > +            - description:
> > +                Interrupt indicating that errors were detected on the CAN bus
> 
> Error detection?
> 
> > +            - description:
> > +                Message Buffer interrupt for mailboxes 8-127 (ored)

nitpick: all these different events for the other interrupts are ored,
so IMHO you can omit the "(ored)".

> > +        interrupt-names:
> > +          items:
> > +            - const: mb_0-7
> 
> Choose one: either underscores or hyphens. Keep it consistent in your
> bindings.

> > +            - const: state
> > +            - const: berr

The order of IRQ names is not consistent with the description.

> > +            - const: mb_8-127
> 
> Choose one: either underscores or hyphens. Keep it consistent in your
> bindings.
> 
> > +      required:
> > +        - compatible
> > +        - reg
> > +        - interrupts
> > +        - interrupt-names
> 
> What happened to "else:"? Why all other devices now have up to 4 interrupts?

Do you already have a dtsi snippet for the flexcan nodes? Please make
sure that the interrupts are correctly mapped.

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 v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/26/2024 12:59 PM, Marc Kleine-Budde wrote:
> On 26.11.2024 08:19:04, Krzysztof Kozlowski wrote:
>> On Mon, Nov 25, 2024 at 06:31:00PM +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>
>>
>> You made multiple changes afterwards, which invalidated the review. See
>> submitting-patches which explain what to do in such case.
>>
>>> ---
>>>   .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
>>>   1 file changed, 42 insertions(+), 4 deletions(-)
>>
>> ...
>>
>>>       maxItems: 2
>>> @@ -136,6 +143,37 @@ required:
>>>     - reg
>>>     - interrupts
>>>   
>>> +allOf:
>>> +  - $ref: can-controller.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: nxp,s32g2-flexcan
>>> +    then:
>>> +      properties:
>>> +        interrupts:
>>> +          items:
>>> +            - description:
>>> +                Message Buffer interrupt for mailboxes 0-7
>>
>> Keep it in one line.
> 
> According to the excel sheet the IRQ is also for the enhanced RX FIFO.
> 

Hello Marc,

Thank you for taking time in reviewing this patchset.

I will update description for the first irq as:
'Message Buffer interrupt for mailboxes 0-7 and Enhanced RX FIFO'

>>
>>> +            - description:
>>> +                Interrupt indicating that the CAN bus went to Buss Off state
>>
>> s/Interrupt indicating that//
>> Buss Off state status?
> 
> What about: "Device went into Bus Off state"
> 
> However from the excel sheet I read it as a device changes state, to Bus
> Off, finished Bus Off or transition from error counters from < 96 to >= 96.
> 
> So "Device state change" would be a more complete description?
> 

I agree "Device state change" would be a more suitable description. I 
will update accordingly in V3.

>>> +            - description:
>>> +                Interrupt indicating that errors were detected on the CAN bus
>>
>> Error detection?
>>
>>> +            - description:
>>> +                Message Buffer interrupt for mailboxes 8-127 (ored)
> 
> nitpick: all these different events for the other interrupts are ored,
> so IMHO you can omit the "(ored)".
> 

True. I will update.

>>> +        interrupt-names:
>>> +          items:
>>> +            - const: mb_0-7
>>
>> Choose one: either underscores or hyphens. Keep it consistent in your
>> bindings.
> 
>>> +            - const: state
>>> +            - const: berr
> 
> The order of IRQ names is not consistent with the description.
> 

Good point. Indeed the order which is in the S32G3 interrupt map excel 
is not consistent with the bindings.

The reason is that in the flexcan driver, reusing the 
'FLEXCAN_QUIRK_NR_IRQ_3' quirk forces the probing of irqs to be done in 
the following order:
mailbox (irq) -> state (irq_boff) -> berr (irq_err)

Hence in order to maintain ABI compatibility I am proposing the 
following order for irqs in case of S32G2/S32G3 SoCs:
mb-0-7 -> state -> berr -> mb-8-127

>>> +            - const: mb_8-127
>>
>> Choose one: either underscores or hyphens. Keep it consistent in your
>> bindings.
>>
>>> +      required:
>>> +        - compatible
>>> +        - reg
>>> +        - interrupts
>>> +        - interrupt-names
>>
>> What happened to "else:"? Why all other devices now have up to 4 interrupts?
> 
> Do you already have a dtsi snippet for the flexcan nodes? Please make
> sure that the interrupts are correctly mapped.
> 
> regards,
> Marc
> 

Yes, I am testing using the following dtsi snippet:

can0: can@401b4000 {
     compatible = "nxp,s32g3-flexcan",
                  "nxp,s32g2-flexcan";
     reg = <0x401b4000 0xa000>;
     interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
                  <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
                  <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
                  <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
     interrupt-names = "mb-0-7", "state", "berr", "mb-8-127";
     clocks = <&clks 9>, <&clks 11>;
     clock-names = "ipg", "per";
};


And checking with:
$ make ARCH=arm64 CHECK_DTBS=y W=1 freescale/s32g274a-evb.dtb 
freescale/s32g274a-rdb2.dtb freescale/s32g399a-rdb3.dtb


Best Regards,
Ciprian

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

[Marc Kleine-Budde]

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

On 26.11.2024 15:48:15, Ciprian Marian Costea wrote:
> Thank you for taking time in reviewing this patchset.
> 
> I will update description for the first irq as:
> 'Message Buffer interrupt for mailboxes 0-7 and Enhanced RX FIFO'
> 
> > > 
> > > > +            - description:
> > > > +                Interrupt indicating that the CAN bus went to Buss Off state
> > > 
> > > s/Interrupt indicating that//
> > > Buss Off state status?
> > 
> > What about: "Device went into Bus Off state"
> > 
> > However from the excel sheet I read it as a device changes state, to Bus
> > Off, finished Bus Off or transition from error counters from < 96 to >= 96.
> > 
> > So "Device state change" would be a more complete description?
> > 
> 
> I agree "Device state change" would be a more suitable description. I will
> update accordingly in V3.

Thanks.

> > > > +            - description:
> > > > +                Interrupt indicating that errors were detected on the CAN bus
> > > 
> > > Error detection?
> > > 
> > > > +            - description:
> > > > +                Message Buffer interrupt for mailboxes 8-127 (ored)
> > 
> > nitpick: all these different events for the other interrupts are ored,
> > so IMHO you can omit the "(ored)".
> > 
> 
> True. I will update.

Thanks

> > > > +        interrupt-names:
> > > > +          items:
> > > > +            - const: mb_0-7

I was wondering if it makes sense to have an interrupt name not
mentioning the exact mailbox numbers, so that the same interrupt name
can be used for a different IP core, too. On the coldfire SoC the 1st
IRQ handles mailboxes 0...15.

> > > Choose one: either underscores or hyphens. Keep it consistent in your
> > > bindings.
> > 
> > > > +            - const: state
> > > > +            - const: berr
> > 
> > The order of IRQ names is not consistent with the description.

Sorry, I misread the interrupt names and was under the misconception
that the interrupt names have a different order than the interrupt
descriptions.

> Good point. Indeed the order which is in the S32G3 interrupt map excel is
> not consistent with the bindings.
> 
> The reason is that in the flexcan driver, reusing the
> 'FLEXCAN_QUIRK_NR_IRQ_3' quirk forces the probing of irqs to be done in the
> following order:
> mailbox (irq) -> state (irq_boff) -> berr (irq_err)
> 
> Hence in order to maintain ABI compatibility I am proposing the following
> order for irqs in case of S32G2/S32G3 SoCs:
> mb-0-7 -> state -> berr -> mb-8-127

That makes totally sense!

> 
> > > > +            - const: mb_8-127

same here

> > > Choose one: either underscores or hyphens. Keep it consistent in your
> > > bindings.
> > > 
> > > > +      required:
> > > > +        - compatible
> > > > +        - reg
> > > > +        - interrupts
> > > > +        - interrupt-names
> > > 
> > > What happened to "else:"? Why all other devices now have up to 4 interrupts?
> > 
> > Do you already have a dtsi snippet for the flexcan nodes? Please make
> > sure that the interrupts are correctly mapped.
> 
> Yes, I am testing using the following dtsi snippet:
> 
> can0: can@401b4000 {
>     compatible = "nxp,s32g3-flexcan",
>                  "nxp,s32g2-flexcan";
>     reg = <0x401b4000 0xa000>;
>     interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
>                  <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
>                  <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
>                  <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>     interrupt-names = "mb-0-7", "state", "berr", "mb-8-127";
>     clocks = <&clks 9>, <&clks 11>;
>     clock-names = "ipg", "per";
> };

looks good to me!

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 v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/26/2024 5:02 PM, Marc Kleine-Budde wrote:
> On 26.11.2024 15:48:15, Ciprian Marian Costea wrote:
>> Thank you for taking time in reviewing this patchset.
>>
>> I will update description for the first irq as:
>> 'Message Buffer interrupt for mailboxes 0-7 and Enhanced RX FIFO'
>>
>>>>
>>>>> +            - description:
>>>>> +                Interrupt indicating that the CAN bus went to Buss Off state
>>>>
>>>> s/Interrupt indicating that//
>>>> Buss Off state status?
>>>
>>> What about: "Device went into Bus Off state"
>>>
>>> However from the excel sheet I read it as a device changes state, to Bus
>>> Off, finished Bus Off or transition from error counters from < 96 to >= 96.
>>>
>>> So "Device state change" would be a more complete description?
>>>
>>
>> I agree "Device state change" would be a more suitable description. I will
>> update accordingly in V3.
> 
> Thanks.
> 
>>>>> +            - description:
>>>>> +                Interrupt indicating that errors were detected on the CAN bus
>>>>
>>>> Error detection?
>>>>
>>>>> +            - description:
>>>>> +                Message Buffer interrupt for mailboxes 8-127 (ored)
>>>
>>> nitpick: all these different events for the other interrupts are ored,
>>> so IMHO you can omit the "(ored)".
>>>
>>
>> True. I will update.
> 
> Thanks
> 
>>>>> +        interrupt-names:
>>>>> +          items:
>>>>> +            - const: mb_0-7
> 
> I was wondering if it makes sense to have an interrupt name not
> mentioning the exact mailbox numbers, so that the same interrupt name
> can be used for a different IP core, too. On the coldfire SoC the 1st
> IRQ handles mailboxes 0...15.
> 

I am ok with proposing a more generic name for mailboxes in order to 
increase reusability among FlexCAN enabled SoCs.
Further specific mailbox numbers could be mentioned in the actual 
S32G2/S32G3 dtsi flexcan node.

One proposal could be:
- mb-1: First Range of Mailboxes
- mb-2: Second Range of Mailboxes

Let me know if you agree to update as proposed in V3.

Best Regards,
Ciprian

>>>> Choose one: either underscores or hyphens. Keep it consistent in your
>>>> bindings.
>>>
>>>>> +            - const: state
>>>>> +            - const: berr
>>>
>>> The order of IRQ names is not consistent with the description.
> 
> Sorry, I misread the interrupt names and was under the misconception
> that the interrupt names have a different order than the interrupt
> descriptions.
> 
>> Good point. Indeed the order which is in the S32G3 interrupt map excel is
>> not consistent with the bindings.
>>
>> The reason is that in the flexcan driver, reusing the
>> 'FLEXCAN_QUIRK_NR_IRQ_3' quirk forces the probing of irqs to be done in the
>> following order:
>> mailbox (irq) -> state (irq_boff) -> berr (irq_err)
>>
>> Hence in order to maintain ABI compatibility I am proposing the following
>> order for irqs in case of S32G2/S32G3 SoCs:
>> mb-0-7 -> state -> berr -> mb-8-127
> 
> That makes totally sense!
> 
>>
>>>>> +            - const: mb_8-127
> 
> same here
> 
>>>> Choose one: either underscores or hyphens. Keep it consistent in your
>>>> bindings.
>>>>
>>>>> +      required:
>>>>> +        - compatible
>>>>> +        - reg
>>>>> +        - interrupts
>>>>> +        - interrupt-names
>>>>
>>>> What happened to "else:"? Why all other devices now have up to 4 interrupts?
>>>
>>> Do you already have a dtsi snippet for the flexcan nodes? Please make
>>> sure that the interrupts are correctly mapped.
>>
>> Yes, I am testing using the following dtsi snippet:
>>
>> can0: can@401b4000 {
>>      compatible = "nxp,s32g3-flexcan",
>>                   "nxp,s32g2-flexcan";
>>      reg = <0x401b4000 0xa000>;
>>      interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
>>                   <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
>>                   <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
>>                   <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>>      interrupt-names = "mb-0-7", "state", "berr", "mb-8-127";
>>      clocks = <&clks 9>, <&clks 11>;
>>      clock-names = "ipg", "per";
>> };
> 
> looks good to me!
> 
> regards,
> Marc
> 

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

[Marc Kleine-Budde]

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

On 26.11.2024 17:15:10, Ciprian Marian Costea wrote:
> > > > > > +        interrupt-names:
> > > > > > +          items:
> > > > > > +            - const: mb_0-7
> > 
> > I was wondering if it makes sense to have an interrupt name not
> > mentioning the exact mailbox numbers, so that the same interrupt name
> > can be used for a different IP core, too. On the coldfire SoC the 1st
> > IRQ handles mailboxes 0...15.
> > 
> 
> I am ok with proposing a more generic name for mailboxes in order to
> increase reusability among FlexCAN enabled SoCs.
> Further specific mailbox numbers could be mentioned in the actual
> S32G2/S32G3 dtsi flexcan node.
> 
> One proposal could be:
> - mb-1: First Range of Mailboxes
> - mb-2: Second Range of Mailboxes
> 
> Let me know if you agree to update as proposed in V3.

Looks good to me!

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 v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Marc Kleine-Budde]

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

On 26.11.2024 16:18:41, Marc Kleine-Budde wrote:
> On 26.11.2024 17:15:10, Ciprian Marian Costea wrote:
> > > > > > > +        interrupt-names:
> > > > > > > +          items:
> > > > > > > +            - const: mb_0-7
> > > 
> > > I was wondering if it makes sense to have an interrupt name not
> > > mentioning the exact mailbox numbers, so that the same interrupt name
> > > can be used for a different IP core, too. On the coldfire SoC the 1st
> > > IRQ handles mailboxes 0...15.
> > > 
> > 
> > I am ok with proposing a more generic name for mailboxes in order to
> > increase reusability among FlexCAN enabled SoCs.
> > Further specific mailbox numbers could be mentioned in the actual
> > S32G2/S32G3 dtsi flexcan node.
> > 
> > One proposal could be:
> > - mb-1: First Range of Mailboxes
> > - mb-2: Second Range of Mailboxes
> > 
> > Let me know if you agree to update as proposed in V3.
> 
> Looks good to me!

Or maybe start with "0", that makes it a bit easier to construct the
names of the IRQ-names in a for loop.

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 v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/26/2024 5:19 PM, Marc Kleine-Budde wrote:
> On 26.11.2024 16:18:41, Marc Kleine-Budde wrote:
>> On 26.11.2024 17:15:10, Ciprian Marian Costea wrote:
>>>>>>>> +        interrupt-names:
>>>>>>>> +          items:
>>>>>>>> +            - const: mb_0-7
>>>>
>>>> I was wondering if it makes sense to have an interrupt name not
>>>> mentioning the exact mailbox numbers, so that the same interrupt name
>>>> can be used for a different IP core, too. On the coldfire SoC the 1st
>>>> IRQ handles mailboxes 0...15.
>>>>
>>>
>>> I am ok with proposing a more generic name for mailboxes in order to
>>> increase reusability among FlexCAN enabled SoCs.
>>> Further specific mailbox numbers could be mentioned in the actual
>>> S32G2/S32G3 dtsi flexcan node.
>>>
>>> One proposal could be:
>>> - mb-1: First Range of Mailboxes
>>> - mb-2: Second Range of Mailboxes
>>>
>>> Let me know if you agree to update as proposed in V3.
>>
>> Looks good to me!
> 
> Or maybe start with "0", that makes it a bit easier to construct the
> names of the IRQ-names in a for loop.
> 
> regards,
> Marc
> 

That makes sense. Thanks for the suggestion.

Best Regards,
Ciprian

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

[Marc Kleine-Budde]

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

On 26.11.2024 17:21:14, Ciprian Marian Costea wrote:
> On 11/26/2024 5:19 PM, Marc Kleine-Budde wrote:
> > On 26.11.2024 16:18:41, Marc Kleine-Budde wrote:
> > > On 26.11.2024 17:15:10, Ciprian Marian Costea wrote:
> > > > > > > > > +        interrupt-names:
> > > > > > > > > +          items:
> > > > > > > > > +            - const: mb_0-7
> > > > > 
> > > > > I was wondering if it makes sense to have an interrupt name not
> > > > > mentioning the exact mailbox numbers, so that the same interrupt name
> > > > > can be used for a different IP core, too. On the coldfire SoC the 1st
> > > > > IRQ handles mailboxes 0...15.
> > > > > 
> > > > 
> > > > I am ok with proposing a more generic name for mailboxes in order to
> > > > increase reusability among FlexCAN enabled SoCs.
> > > > Further specific mailbox numbers could be mentioned in the actual
> > > > S32G2/S32G3 dtsi flexcan node.
> > > > 
> > > > One proposal could be:
> > > > - mb-1: First Range of Mailboxes
> > > > - mb-2: Second Range of Mailboxes
> > > > 
> > > > Let me know if you agree to update as proposed in V3.
> > > 
> > > Looks good to me!
> > 
> > Or maybe start with "0", that makes it a bit easier to construct the
> > names of the IRQ-names in a for loop.
> > 
> > regards,
> > Marc
> > 
> 
> That makes sense. Thanks for the suggestion.

I think we're almost there. Now you can change patch 1 to
platform_get_irq_byname(..., "mb-1");.

regards,
Marc

P.S.: Actual support for the mailboxes 64..127 or the extended FIFO can
be added in a later patch.

-- 
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 v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Ciprian Marian Costea]

On 11/27/2024 9:23 AM, Marc Kleine-Budde wrote:
> On 26.11.2024 17:21:14, Ciprian Marian Costea wrote:
>> On 11/26/2024 5:19 PM, Marc Kleine-Budde wrote:
>>> On 26.11.2024 16:18:41, Marc Kleine-Budde wrote:
>>>> On 26.11.2024 17:15:10, Ciprian Marian Costea wrote:
>>>>>>>>>> +        interrupt-names:
>>>>>>>>>> +          items:
>>>>>>>>>> +            - const: mb_0-7
>>>>>>
>>>>>> I was wondering if it makes sense to have an interrupt name not
>>>>>> mentioning the exact mailbox numbers, so that the same interrupt name
>>>>>> can be used for a different IP core, too. On the coldfire SoC the 1st
>>>>>> IRQ handles mailboxes 0...15.
>>>>>>
>>>>>
>>>>> I am ok with proposing a more generic name for mailboxes in order to
>>>>> increase reusability among FlexCAN enabled SoCs.
>>>>> Further specific mailbox numbers could be mentioned in the actual
>>>>> S32G2/S32G3 dtsi flexcan node.
>>>>>
>>>>> One proposal could be:
>>>>> - mb-1: First Range of Mailboxes
>>>>> - mb-2: Second Range of Mailboxes
>>>>>
>>>>> Let me know if you agree to update as proposed in V3.
>>>>
>>>> Looks good to me!
>>>
>>> Or maybe start with "0", that makes it a bit easier to construct the
>>> names of the IRQ-names in a for loop.
>>>
>>> regards,
>>> Marc
>>>
>>
>> That makes sense. Thanks for the suggestion.
> 
> I think we're almost there. Now you can change patch 1 to
> platform_get_irq_byname(..., "mb-1");.
> 
> regards,
> Marc
> 

Yes, I will also include this change in V3. Thanks for your suggestion.

Best Regards,
Ciprian

> P.S.: Actual support for the mailboxes 64..127 or the extended FIFO can
> be added in a later patch.
> 

[PATCH v2 2/3] can: flexcan: Add quirk to handle separate interrupt lines for mailboxes

[Ciprian Costea]

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

Introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk to handle a FlexCAN
hardware module integration particularity where two ranges of mailboxes
are controlled by separate hardware interrupt lines.
The same 'flexcan_irq' handler is used for both separate mailbox interrupt
lines, with no other changes.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/flexcan/flexcan-core.c | 24 +++++++++++++++++++++++-
 drivers/net/can/flexcan/flexcan.h      |  5 +++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index ac1a860986df..7d7f3fa1d67b 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -1762,14 +1762,25 @@ 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:
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
+		free_irq(priv->irq_err, dev);
  out_free_irq_boff:
-	free_irq(priv->irq_boff, dev);
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
+		free_irq(priv->irq_boff, dev);
  out_free_irq:
 	free_irq(dev->irq, dev);
  out_can_rx_offload_disable:
@@ -1799,6 +1810,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);
@@ -2187,6 +2201,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..2cf886618c96 100644
--- a/drivers/net/can/flexcan/flexcan.h
+++ b/drivers/net/can/flexcan/flexcan.h
@@ -70,6 +70,10 @@
 #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)
+/* Device has two separate interrupt lines for two mailbox ranges, which
+ * both need to have an interrupt handler registered.
+ */
+#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
 
 struct flexcan_devtype_data {
 	u32 quirks;		/* quirks needed for different IP cores */
@@ -107,6 +111,7 @@ struct flexcan_priv {
 
 	int irq_boff;
 	int irq_err;
+	int irq_secondary_mb;
 
 	/* IPC handle when setup stop mode by System Controller firmware(scfw) */
 	struct imx_sc_ipc *sc_ipc_handle;
-- 
2.45.2

Re: [PATCH v2 2/3] can: flexcan: Add quirk to handle separate interrupt lines for mailboxes

[Vincent Mailhol]

Hi Ciprian,

Thanks for the patch.

On 26/11/2024 at 01:31, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> Introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk to handle a FlexCAN
> hardware module integration particularity where two ranges of mailboxes
> are controlled by separate hardware interrupt lines.
> The same 'flexcan_irq' handler is used for both separate mailbox interrupt
> lines, with no other changes.
> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Yours sincerely,
Vincent Mailhol

[PATCH v2 3/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.

On S32G2/S32G3 SoC, there are separate interrupts for state change, bus
errors, Mailboxes 0-7 and Mailboxes 8-127 respectively.
In order to handle this FlexCAN hardware particularity, first reuse the
'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq handling
support. Secondly, use the newly introduced
'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk which handles the case where two
separate mailbox ranges are controlled by independent hardware interrupt
lines.

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index 7d7f3fa1d67b..bb24ae6195f9 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -386,6 +386,16 @@ 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_NR_IRQ_3 |
+		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
+		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
+		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
+};
+
 static const struct can_bittiming_const flexcan_bittiming_const = {
 	.name = DRV_NAME,
 	.tseg1_min = 4,
@@ -2055,6 +2065,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