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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241126-capable-vagabond-tody-8b3717-mkl@pengutronix.de>
Date: Tue, 26 Nov 2024 16:02:11 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, 
	Vincent Mailhol <mailhol.vincent@...adoo.fr>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-can@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, NXP S32 Linux <s32@....com>, 
	imx@...ts.linux.dev, Christophe Lizzi <clizzi@...hat.com>, 
	Alberto Ruiz <aruizrui@...hat.com>, Enric Balletbo <eballetb@...hat.com>, 
	Frank Li <Frank.Li@....com>
Subject: Re: [PATCH v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3
 SoC support

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@...b4000 {
>     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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)