| Message ID | 20241125163103.4166207-2-ciprianmarian.costea@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add FlexCAN support for S32G2/S32G3 SoCs | expand |
On Mon, Nov 25, 2024 at 06:31:00PM +0200, Ciprian Costea wrote: > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > 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@oss.nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> 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. > + - description: > + Interrupt indicating that the CAN bus went to Buss Off state s/Interrupt indicating that// Buss Off state status? > + - description: > + Interrupt indicating that errors were detected on the CAN bus Error detection? > + - description: > + Message Buffer interrupt for mailboxes 8-127 (ored) > + interrupt-names: > + items: > + - const: mb_0-7 Choose one: either underscores or hyphens. Keep it consistent in your bindings. > + - const: state > + - const: berr > + - 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? Best regards, Krzysztof
On 11/26/2024 9:19 AM, Krzysztof Kozlowski wrote: > On Mon, Nov 25, 2024 at 06:31:00PM +0200, Ciprian Costea wrote: >> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >> >> 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@oss.nxp.com> >> Reviewed-by: Frank Li <Frank.Li@nxp.com> > Hello Krzysztof, Thanks for your time in reviewing this patch. > You made multiple changes afterwards, which invalidated the review. See > submitting-patches which explain what to do in such case. > I will remove the tag in V3 and add this info in the changelog. >> --- >> .../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. I will update in V3. > >> + - description: >> + Interrupt indicating that the CAN bus went to Buss Off state > > s/Interrupt indicating that// > Buss Off state status? > >> + - description: >> + Interrupt indicating that errors were detected on the CAN bus > > Error detection? I will rephrase in V2. > >> + - description: >> + Message Buffer interrupt for mailboxes 8-127 (ored) >> + interrupt-names: >> + items: >> + - const: mb_0-7 > > Choose one: either underscores or hyphens. Keep it consistent in your > bindings. Makes sense. I will update in V3. > >> + - const: state >> + - const: berr >> + - const: mb_8-127 > > Choose one: either underscores or hyphens. Keep it consistent in your > bindings. > I will update in V3. >> + required: >> + - compatible >> + - reg >> + - interrupts >> + - interrupt-names > > What happened to "else:"? Why all other devices now have up to 4 interrupts? > > Best regards, > Krzysztof > I will add the 'else' branch back in V3. Best Regards, Ciprian
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@oss.nxp.com> > > > > 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@oss.nxp.com> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > 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. > > > + - 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? > > + - 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)". > > + 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. > > + - 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
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@oss.nxp.com> >>> >>> 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@oss.nxp.com> >>> Reviewed-by: Frank Li <Frank.Li@nxp.com> >> >> 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
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
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 >
On 26.11.2024 17:15:10, Ciprian Marian Costea wrote: > > > > > > + 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. Looks good to me! regards, Marc
On 26.11.2024 16:18:41, Marc Kleine-Budde wrote: > On 26.11.2024 17:15:10, Ciprian Marian Costea wrote: > > > > > > > + 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. > > Looks good to me! Or maybe start with "0", that makes it a bit easier to construct the names of the IRQ-names in a for loop. regards, Marc
On 11/26/2024 5:19 PM, Marc Kleine-Budde wrote: > On 26.11.2024 16:18:41, Marc Kleine-Budde wrote: >> On 26.11.2024 17:15:10, Ciprian Marian Costea wrote: >>>>>>>> + 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. >> >> Looks good to me! > > Or maybe start with "0", that makes it a bit easier to construct the > names of the IRQ-names in a for loop. > > regards, > Marc > That makes sense. Thanks for the suggestion. Best Regards, Ciprian
On 26.11.2024 17:21:14, Ciprian Marian Costea wrote: > On 11/26/2024 5:19 PM, Marc Kleine-Budde wrote: > > On 26.11.2024 16:18:41, Marc Kleine-Budde wrote: > > > On 26.11.2024 17:15:10, Ciprian Marian Costea wrote: > > > > > > > > > + 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. > > > > > > Looks good to me! > > > > Or maybe start with "0", that makes it a bit easier to construct the > > names of the IRQ-names in a for loop. > > > > regards, > > Marc > > > > That makes sense. Thanks for the suggestion. I think we're almost there. Now you can change patch 1 to platform_get_irq_byname(..., "mb-1");. regards, Marc P.S.: Actual support for the mailboxes 64..127 or the extended FIFO can be added in a later patch.
On 11/27/2024 9:23 AM, Marc Kleine-Budde wrote: > On 26.11.2024 17:21:14, Ciprian Marian Costea wrote: >> On 11/26/2024 5:19 PM, Marc Kleine-Budde wrote: >>> On 26.11.2024 16:18:41, Marc Kleine-Budde wrote: >>>> On 26.11.2024 17:15:10, Ciprian Marian Costea wrote: >>>>>>>>>> + 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. >>>> >>>> Looks good to me! >>> >>> Or maybe start with "0", that makes it a bit easier to construct the >>> names of the IRQ-names in a for loop. >>> >>> regards, >>> Marc >>> >> >> That makes sense. Thanks for the suggestion. > > I think we're almost there. Now you can change patch 1 to > platform_get_irq_byname(..., "mb-1");. > > regards, > Marc > Yes, I will also include this change in V3. Thanks for your suggestion. Best Regards, Ciprian > P.S.: Actual support for the mailboxes 64..127 or the extended FIFO can > be added in a later patch. >
diff --git a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml index 97dd1a7c5ed2..b2c16a7d864c 100644 --- a/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml +++ b/Documentation/devicetree/bindings/net/can/fsl,flexcan.yaml @@ -10,9 +10,6 @@ title: maintainers: - Marc Kleine-Budde <mkl@pengutronix.de> -allOf: - - $ref: can-controller.yaml# - properties: compatible: oneOf: @@ -28,6 +25,7 @@ properties: - fsl,vf610-flexcan - fsl,ls1021ar2-flexcan - fsl,lx2160ar1-flexcan + - nxp,s32g2-flexcan - items: - enum: - fsl,imx53-flexcan @@ -43,12 +41,21 @@ properties: - enum: - fsl,ls1028ar1-flexcan - const: fsl,lx2160ar1-flexcan + - items: + - enum: + - nxp,s32g3-flexcan + - const: nxp,s32g2-flexcan reg: maxItems: 1 interrupts: - maxItems: 1 + minItems: 1 + maxItems: 4 + + interrupt-names: + minItems: 1 + maxItems: 4 clocks: 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 + - description: + Interrupt indicating that the CAN bus went to Buss Off state + - description: + Interrupt indicating that errors were detected on the CAN bus + - description: + Message Buffer interrupt for mailboxes 8-127 (ored) + interrupt-names: + items: + - const: mb_0-7 + - const: state + - const: berr + - const: mb_8-127 + required: + - compatible + - reg + - interrupts + - interrupt-names + additionalProperties: false examples: