Re: [PATCH v2 3/6] dt-bindings: crypto: Add Texas Instruments MCRC64

[Vignesh Raghavendra]

Hi Conor/Krzysztof

On 27/05/24 15:41, Kamlesh Gurudasani wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> writes:
> 
>> This message was sent from outside of Texas Instruments. 
>> Do not click links or open attachments unless you recognize the source of this email and know the content is safe. If you wish
>> to report this message to IT Security, please forward the message as an attachment to phishing@xxxxxxxxxxx 
>>  
>> On 27/05/2024 10:25, Kamlesh Gurudasani wrote:
>>> Conor Dooley <conor@xxxxxxxxxx> writes:
>>>
>>>> On Fri, Aug 11, 2023 at 04:34:33PM +0100, Conor Dooley wrote:
>>>>> On Fri, Aug 11, 2023 at 12:58:50AM +0530, Kamlesh Gurudasani wrote:
>>>>>> Add binding for Texas Instruments MCRC64
>>>>>>
>>>>>> MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
>>>>>> according to the ISO 3309 standard.
>>>>>>
>>>>>> The ISO 3309 64-bit CRC model parameters are as follows:
>>>>>>     Generator Polynomial: x^64 + x^4 + x^3 + x + 1
>>>>>>     Polynomial Value: 0x000000000000001B
>>>>>>     Initial value: 0x0000000000000000
>>>>>>     Reflected Input: False
>>>>>>     Reflected Output: False
>>>>>>     Xor Final: 0x0000000000000000
>>>>>>
>>>>>> Signed-off-by: Kamlesh Gurudasani <kamlesh@xxxxxx>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/crypto/ti,mcrc64.yaml | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  MAINTAINERS                                             |  5 +++++
>>>>>>  2 files changed, 52 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/crypto/ti,mcrc64.yaml b/Documentation/devicetree/bindings/crypto/ti,mcrc64.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..38bc7efebd68
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/crypto/ti,mcrc64.yaml
>>>>>> @@ -0,0 +1,47 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: https://urldefense.com/v3/__http://devicetree.org/schemas/crypto/ti,mcrc64.yaml*__;Iw!!G3vK!Qw75749h2ysFlROkyfLIUT9MGWlHfBEvPAbLVjScJXCPJ7vbwgxH-8hNWlJGBXGwz9Ny47eQi2mPS5R6La54vZo$
>>>>>> +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!G3vK!Qw75749h2ysFlROkyfLIUT9MGWlHfBEvPAbLVjScJXCPJ7vbwgxH-8hNWlJGBXGwz9Ny47eQi2mPS5R6P2LNJCQ$
>>>>>> +
>>>>>> +title: Texas Instruments MCRC64
>>>>>> +
>>>>>> +description: The MCRC64 engine calculates 64-bit cyclic redundancy checks
>>>>>
>>>>> A newline after "description" please.
>>>>>
>>>>>> +  (CRC) according to the ISO 3309 standard.
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Kamlesh Gurudasani <kamlesh@xxxxxx>
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: ti,am62-mcrc64
>>>>>
>>>>> Is the am62 an SoC or a family of SoCs? I googled a wee bit for am62 &
>>>>> there seems to be an am625 and an am623?
>>>>

Let me put this confusion to rest and clarify the ratioanle of namings
so far.

AM62 is a the name of the SoC variant. AM625/AM623/AM621/AM620 are the
Orderable Part Number (OPNs - full OPN is several digits long), the only
difference b/w them is number of CPU/GPU/PRU cores. At SoC level they
are all same with AM625 being superset.

Similarly AM62A is another SoC variant, with AM62A7 and AM62A3 are OPNs.
AM62P is yet another SoC variant with AM62P5 as OPN

Linux DT is written to support superset part numbers (AM625, AM62A7,
AM62P5) as TI EVMs always have superset parts. Board dts files are named
accordingly (eg.: k3-am625-sk.dts) Bootloader does the appropriate
fixups to disable components for subset devices based on eFUSE
indications when needed.


>>>> Or is it an am62p5, in which case the compatible should contain
>>>> ti,am62p5 I suppose. Sorry for my confusion here, its not really clear
>>>> me too since I've been seeing many different-but-similar product names
>>>> the last few days.
>>>>

MCRC64 is on all K3 platforms.

We have been using ti,am62-xxxx for all modules that were verified (or
first supported) on any of the AM625/3/1/0 SoCs. Last digit really is
represents CPU/GPU numbers, thus not relevant for peripherals and there
should be no change in HW

If peripheral is specific to AM62A (eg.: DSP) or AM62P, then
ti,am62a-xxxx and ti,am62p-xxxx is being used correspondingly.


>>>> Thanks,
>>>> Conor.
>>>>
>>> Hi Conor,
>>>
>>> Thanks for the review.
>>>
>>> am62 is family of SOCs.
>>>
>>> All devices under this family, like am623/5/p5 and etc, have MCRC64.
>>>
>>> I have kept the naming convention similar to SA2UL/SA3UL[0].
>>>
>>> [0] https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/crypto/ti,sa2ul.yaml*L18__;Iw!!G3vK!Qw75749h2ysFlROkyfLIUT9MGWlHfBEvPAbLVjScJXCPJ7vbwgxH-8hNWlJGBXGwz9Ny47eQi2mPS5R6afCEd8s$
>>
>> Usual answer is: no families. There are exceptions, though, so is this
>> case on the exception list?
> Okay, will use ti,am625-mcrc64 as compatible and as fallback compatible for
> other devices. I hope that is right.

As mentioned above ti,am62-mcrc64 would be better option here and would
be consistent with rest of the peripherals being supported on the SoC.

But if we really want be accurate to exact part number on which Kamlesh
has tested then, perhaps ti,am625-mcrc64 is fine with me.


> 
> Thanks.
> 
> Kamlesh
>>
>> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.10-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst*L42__;Iw!!G3vK!Qw75749h2ysFlROkyfLIUT9MGWlHfBEvPAbLVjScJXCPJ7vbwgxH-8hNWlJGBXGwz9Ny47eQi2mPS5R6WaRq1VM$
>>
>> P.S. Your email client added some weird subject prefix - please fix it.
> Thanks for bringing this to my notice, Will fix it.
>>
>>
>>
>> Best regards,
>> Krzysztof

-- 
Regards
Vignesh