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
