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@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> >
> > 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