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: <01a7de95-24e2-4c75-a818-bbc363e89844@oss.nxp.com>
Date: Tue, 26 Nov 2024 15:48:15 +0200
From: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
To: Marc Kleine-Budde <mkl@...gutronix.de>,
 Krzysztof Kozlowski <krzk@...nel.org>
Cc: 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 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@....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@....nxp.com>
>>> Reviewed-by: Frank Li <Frank.Li@....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@...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";
};


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