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

[Richard Acayan <mailingradian@gmail.com>]

> On 22/09/2022 12:48, Konrad Dybcio wrote:
>> 
>> 
>> On 22.09.2022 08:41, Krzysztof Kozlowski wrote:
>>> 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).
>> In my view it's hard to name it, downstream uses bool properties for enabling/disabling
>> certain quirks and on different generations there are different combinations of that.
>> Interestingly enough, I vaguely remember that the same quirk names can mean different
>> things on different msm-X.Y versions.. Add to that, different msm-X.Y versions can have
>> different assumptions on what's the default (aka without taking the bool properties into
>> account) behaviour for a given compatible. 
>
> Downstream does not care about ABI, coding style, reasonable approach,
> so it should not wonder that they code things inconsistent.
>
>
>> So I suppose "first in the family" would be
>> the best way to go for mainline, though there are still quite a few families:
>> 
>> <earlier ones used qcom_iommu>
>> - 8996 with quirks that are already accounted for (or one may also say it works by miracle,
>> just like msm8916 - downstream adds more special handling, but looks like the fw is not as
>> restrictive)
>> 
>> - 8996pro + 660 + 8998 with serious unmerged ones [1]
>> 
>> - 845 which seems to be aok
>> 
>> - special case of chromebooks where they only have qcom TZ/XPUs and not the hypervisor to
>> fight with, so ma-a-aybe (no downstream reference & I don't have the hw to confirm) they
>> can get away with less things
>> 
>> - 8[1234]50 which seem to be a mix-and-match of less serious (read: not accounting for them
>> may hurt performance but will not make your device sepuku at SMMU probe) minor quirks
>> [2][3][4][5] (big warning: these may be overriden somewhere in other device tree fragments,
>> the surest option would be to take a compiled dtb and decompile it to be sure about it)
>> 
>> - 4xxx/6xxx series that mostly align with "whatever was there on the flagship soc released
>> a year before"
>
> If the devices are really different, even though it is not visible in
> Linux driver, then indeed there is no point for fake compatibility. I
> raised the question only because the driver does not customize the
> variants, but that might be not enough.

If there are no problems with the original patch, could you please indicate
that? I think this thread is getting a little lengthy for the other
maintainers.

> Linked DTSI use different quirks (assuming quirks would have same
> meaning...), plus they have sometimes different amount of clocks, so in
> total maybe it is not reasonable to make them compatible. On the other
> hand, maybe the programming model is very, very similar thus Linux could
> bind to one fallback and few different bits would be customized with
> specific compatible.

Not to rush you or anything. If you think this should be decided before the
series goes in, it's not urgent.