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
