Re: [PATCH v3 11/50] dt-bindings: crypto: add sam9x7 in Atmel TDES

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 21/08/2023 15:00, Tudor Ambarus wrote:
> 
> 
> On 8/21/23 13:10, Krzysztof Kozlowski wrote:
>> On 21/08/2023 11:54, Tudor Ambarus wrote:
>>>>>>> I am aware that there is no change in the crypto IP used. This patch is
>>>>
>>>> Actually, recent history showed us that it's not only the IP itself but
>>>> its integration into final product that could have an influence on the
>>>> behavior.
>>>>
>>>>>>> to add a SoC specific compatible as expected by writing-bindings
>>>>>>> guideline. Maybe a bit more explanation in the commit description might
>>>>>>> do the trick.
>>>>>>>
>>>>>>
>>>>>> So you add a compatible that will never be used just to comply with
>>>>>> the writing bindings guideline?
>>>>>
>>>>> How do you know that it is never going to be used? The guideline asks
>>>
>>> See
>>> https://git.kernel.org/pub/scm/linux/kernel/git/at91/linux.git/tree/drivers/crypto/atmel-tdes.c?h=at91-dt#n1120
>>
>> What's there? One compatible? How does it prove that it will not be
>> used? It only proves that currently it is not used... And anyway this is
> 
> Correct, as of now the compatible was not used to determine the hw caps,
> the capabilities were retrieved by checking at runtime a version
> register. I'm against adding a compatible that will not be used, in this
> particular case, defining "microchip,sam9x7-tdes" in the driver but
> still solely relying on the runtime version register interrogation.
> Unfortunately the commit message does not reveal any intention and from
> there these emails changed. Maybe it's just a matter of personal
> preference, so I'll stop commenting on this.

Hm, thanks for bringing this up, indeed it looks a bit redundant to have
additional compatible strings if HW can identify itself. In such case,
pretty often a generic compatible in the driver and DTS could do... but:

1. Adding specific compatibles for such case would have sense to be able
to validate differences between DTS (like clocks, resets and power domains).

2. Reading HW revision might require enabling clocks which might differ
per variant, thus driver still might need to match according to some
compatibles. After that matching, driver can get necessary clocks,
enable them and read the HW version/revision. There is an example for this:
Documentation/devicetree/bindings/cache/qcom,llcc.yaml
drivers/soc/qcom/llcc-qcom.c
We have there version of the block but clocks and register layout
differ, thus we must have device-specific compatibles.

> 
>> just one implementation in one system. How can you possibly know all
>> other possible implementations (other bootloaders/firmwares/systems)?
>> One cannot. The guideline is there for specific reason.
>>
> 
> I didn't say the guideline is wrong, I just tried to understand how this
> particular case is handled.
>>

Best regards,
Krzysztof