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

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

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