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

[Marc Kleine-Budde]

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: signature.asc
Description: PGP signature