Re: [PATCH 2/2] iommu: arm-smmu-qcom: add sdm670 compatible

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

On 22/09/2022 04:38, Richard Acayan wrote:
>> On 21.09.2022 21:05, Krzysztof Kozlowski wrote:
>>> On 21/09/2022 20:48, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 21.09.2022 20:47, Konrad Dybcio wrote:
>>>>>
>>>>>
>>>>> On 21.09.2022 09:31, Krzysztof Kozlowski wrote:
>>>>>> On 21/09/2022 00:39, Richard Acayan wrote:
>>>>>>> The Snapdragon 670 needs the IOMMU for GENI I2C. Add a compatible string to
>>>>>>> support it.
>>>>>>>
>>>>>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> index b2708de25ea3..bf9653b9eb89 100644
>>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>>> @@ -431,6 +431,7 @@ static const struct of_device_id __maybe_unused qcom_smmu_impl_of_match[] = {
>>>>>>>  	{ .compatible = "qcom,sc8180x-smmu-500" },
>>>>>>>  	{ .compatible = "qcom,sc8280xp-smmu-500" },
>>>>>>>  	{ .compatible = "qcom,sdm630-smmu-v2" },
>>>>>>> +	{ .compatible = "qcom,sdm670-smmu-500" },
>>>>>>
>>>>>> Why do we keep adding compatibles to the driver for apparently
>>>>>> compatible devices?
>>>>>
>>>>> Because Linux has not funny run on bare Qualcomm hardware ever since at least msm8x60 times and
>>>> s/funny/fully
>>>>
>>>> unfortunate typo, this is not funny, quite the contrary..
>>>>
>>>> Konrad
>>>>> we are not interacting with real hardware, only with Qualcomm's flawed virtual implementation
>>>>> of it, that's abstracted to us through various generations of their saddening software stack. This
>>>>> is also the case for many more standard components, even as far as the GIC on recent boards..
>>>
>>> Unfortunately I don't get this explanation... you mean some other
>>> firmware requires Linux drivers to use specific compatibles instead of
>>> one fallback?
>> No, perhaps I misunderstood you.
>>
>>>
>>> All of these do not have driver data, so they are essentially compatible
>>> for Linux driver. Growing this list in the driver seems pointless. What
>>> is the benefit of growing driver with same entries, except more patches?
>> Compatible lists in smmu-impl files allow matching driver quirks for SMMUs themselves
>> and consumer devices (such as MDSS). The situation is more complicated, because some
>> qcom SMMUs also require more quirks than others (think 8974 vs 8994 vs 8996/pro&660&8998
>> vs 845+ vs adreno smmu in various flavours), so all qcom SMMUs need to use
>> `qcom_smmu_impl` and some others need even more quirks on top of that (that generally
>> hurt performance or functionality, so we don't want them when they're unnecessary).
>> If all generations of qcom SMMU implementation that bear the same name behaved anywhere
>> near consistent, there would be no need for keeping this around, instead requiring only
>> "qcom,broken-smmu" or something".
> 
> Hi, just stopping by to share my own thoughts.
> 
> First, I don't mind if this series doesn't get applied as-is. After seeing
> the eMMC driver in its current state:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-msm.c?h=v6.0-rc6#n2437
> 
> I can understand that the devicetree maintainers don't want to see new SoCs
> touching drivers unnecessarily. Second, I don't see enough quirks to
> justify needing all compatible strings in the driver (2 quirky SoCs
> compared to 16 total not counting adreno iommu):
> 
>     $ grep qcom, drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>     	if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2"))
>     	* All targets that use the qcom,adreno-smmu compatible string *should*
>     	{ .compatible = "qcom,adreno" },
>     	{ .compatible = "qcom,mdp4" },
>     	{ .compatible = "qcom,mdss" },
>     	{ .compatible = "qcom,sc7180-mdss" },
>     	{ .compatible = "qcom,sc7180-mss-pil" },
>     	{ .compatible = "qcom,sc7280-mdss" },
>     	{ .compatible = "qcom,sc7280-mss-pil" },
>     	{ .compatible = "qcom,sc8180x-mdss" },
>     	{ .compatible = "qcom,sm8250-mdss" },
>     	{ .compatible = "qcom,sdm845-mdss" },
>     	{ .compatible = "qcom,sdm845-mss-pil" },
>     	if (of_device_is_compatible(np, "qcom,sdm845-smmu-500"))
>     	{ .compatible = "qcom,msm8998-smmu-v2" },
>     	{ .compatible = "qcom,qcm2290-smmu-500" },
>     	{ .compatible = "qcom,sc7180-smmu-500" },
>     	{ .compatible = "qcom,sc7280-smmu-500" },
>     	{ .compatible = "qcom,sc8180x-smmu-500" },
>     	{ .compatible = "qcom,sc8280xp-smmu-500" },
>     	{ .compatible = "qcom,sdm630-smmu-v2" },
>     	{ .compatible = "qcom,sdm670-smmu-500" },
>     	{ .compatible = "qcom,sdm845-smmu-500" },
>     	{ .compatible = "qcom,sm6125-smmu-500" },
>     	{ .compatible = "qcom,sm6350-smmu-500" },
>     	{ .compatible = "qcom,sm6375-smmu-500" },
>     	{ .compatible = "qcom,sm8150-smmu-500" },
>     	{ .compatible = "qcom,sm8250-smmu-500" },
>     	{ .compatible = "qcom,sm8350-smmu-500" },
>     	{ .compatible = "qcom,sm8450-smmu-500" },
>     	if (of_device_is_compatible(np, "qcom,adreno-smmu"))
> 
> I don't know if it's better to get myself involved in fixing this, though.
> There is no fallback that encompasses qcom devices but not all arm devices.
> Either way, I'll have to add a new compatible string to the driver.
> 
> If something like this is fine for now, I'll format it properly tomorrow:

Please wait till we reach some conclusion otherwise your work might be
wasted.

> 
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -48,6 +48,13 @@ properties:
>                - qcom,sm8350-smmu-500
>                - qcom,sm8450-smmu-500
>            - const: arm,mmu-500
> +
> +      - description: Qcom SoCs implementing "qcom,smmu-500"
> +        items:
> +          - enum:
> +              - qcom,sdm670-smmu-500
> +          - const: qcom,smmu-500
> +

Someone would have to confirm that smmu-500 is a real device
spec/version. Otherwise this should be device-specific compatible (e.g.
earliest in family).

Best regards,
Krzysztof