Re: [PATCH v8 9/9] crypto: qce: core: Add new compatibles for qce crypto driver

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: neil.armstrong@linaro.org,
	Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Thara Gopinath <thara.gopinath@gmail.com>,
	Bhupesh Sharma <bhupesh.sharma@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Andy Gross <agross@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-crypto@vger.kernel.org, Rob Herring <robh@kernel.org>,
	Jordan Crouse <jorcrous@amazon.com>
Subject: Re: [PATCH v8 9/9] crypto: qce: core: Add new compatibles for qce crypto driver
Date: Fri, 3 Feb 2023 10:20:51 +0100	[thread overview]
Message-ID: <53af5fdd-149b-272e-902a-6961975f1071@linaro.org> (raw)
In-Reply-To: <261fb8cd-5043-3b2d-2c9a-2e602678517c@linaro.org>

On 03/02/2023 10:19, Neil Armstrong wrote:
> On 02/02/2023 15:20, Krzysztof Kozlowski wrote:
>> On 02/02/2023 15:15, Vladimir Zapolskiy wrote:
>>> Hi Krzysztof,
>>>
>>> On 2/2/23 16:01, Krzysztof Kozlowski wrote:
>>>> On 02/02/2023 14:50, Vladimir Zapolskiy wrote:
>>>>> From: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>>
>>>>> Since we decided to use soc specific compatibles for describing
>>>>> the qce crypto IP nodes in the device-trees, adapt the driver
>>>>> now to handle the same.
>>>>>
>>>>> Keep the old deprecated compatible strings still in the driver,
>>>>> to ensure backward compatibility.
>>>>>
>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>> Cc: herbert@gondor.apana.org.au
>>>>> Tested-by: Jordan Crouse <jorcrous@amazon.com>
>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>> [vladimir: added more SoC specfic compatibles]
>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>>>> ---
>>>>>    drivers/crypto/qce/core.c | 12 ++++++++++++
>>>>>    1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
>>>>> index 8e496fb2d5e2..2420a5ff44d1 100644
>>>>> --- a/drivers/crypto/qce/core.c
>>>>> +++ b/drivers/crypto/qce/core.c
>>>>> @@ -291,8 +291,20 @@ static int qce_crypto_remove(struct platform_device *pdev)
>>>>>    }
>>>>>    
>>>>>    static const struct of_device_id qce_crypto_of_match[] = {
>>>>> +	/* Following two entries are deprecated (kept only for backward compatibility) */
>>>>>    	{ .compatible = "qcom,crypto-v5.1", },
>>>>>    	{ .compatible = "qcom,crypto-v5.4", },
>>>>> +	/* Add compatible strings as per updated dt-bindings, here: */
>>>>> +	{ .compatible = "qcom,ipq4019-qce", },
>>>>> +	{ .compatible = "qcom,ipq6018-qce", },
>>>>> +	{ .compatible = "qcom,ipq8074-qce", },
>>>>> +	{ .compatible = "qcom,msm8996-qce", },
>>>>> +	{ .compatible = "qcom,sdm845-qce", },
>>>>> +	{ .compatible = "qcom,sm8150-qce", },
>>>>> +	{ .compatible = "qcom,sm8250-qce", },
>>>>> +	{ .compatible = "qcom,sm8350-qce", },
>>>>> +	{ .compatible = "qcom,sm8450-qce", },
>>>>> +	{ .compatible = "qcom,sm8550-qce", },
>>>> I did not agree with this at v7 and I still do not agree. We already did
>>>> some effort to clean this pattern in other drivers, so to make it clear
>>>> - driver does not need 10 compatibles because they are the same.
>>>
>>> Here is a misunderstanding, the compatibles are not the same and it shall
>>> not be assumed this way, only the current support of the IP on different SoCs
>>> in the driver is the same.
> 
> It seems the IP version is discoverable, in this case it's perfectly valid
> to have a generic compatible along a soc specific compatible.
> 
> It has been done and validated multiple times, like for the ARM Mali Bifrost [1]
> 
> I'll propose then to add a generic "qcom,crypto" as fallback to
> all of those new compatibles and clearly document that this is only
> for crypto IP cores versions that have the runtime version discoverable.

Yes, this is good idea.

> 
> We could even add a major version generic fallback compatible like "qcom,crypto-v5" or "qcom,crypto-v5.x"
> to differentiate from older crypto devices.

Since we have mapping of versions to SoC, it's also fine.

Best regards,
Krzysztof