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