Re: [PATCH 1/2] dt-bindings: ufs: qcom: allow 'dma-coherent' property

[Krzysztof Kozlowski]

On 05/12/2022 14:37, Manivannan Sadhasivam wrote:
> On Mon, Dec 05, 2022 at 02:12:48PM +0100, Johan Hovold wrote:
>> On Mon, Dec 05, 2022 at 06:30:48PM +0530, Manivannan Sadhasivam wrote:
>>> On Mon, Dec 05, 2022 at 01:27:34PM +0100, Johan Hovold wrote:
>>>> On Mon, Dec 05, 2022 at 05:50:18PM +0530, Manivannan Sadhasivam wrote:
>>>>> On Mon, Dec 05, 2022 at 01:07:16PM +0100, Johan Hovold wrote:
>>>>>> On Mon, Dec 05, 2022 at 05:29:06PM +0530, Manivannan Sadhasivam wrote:
>>>>>>> On Mon, Dec 05, 2022 at 11:08:36AM +0100, Johan Hovold wrote:
>>>>>>>> UFS controllers may be cache coherent and must be marked as such in the
>>>>>>>> devicetree to avoid data corruption.
>>>>>>>>
>>>>>>>> This is specifically needed on recent Qualcomm platforms like SC8280XP.
>>>>>>>>
>>>>>>>> Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
>>
>>>>>> Yes, it would be a valid, but it will only be added to the DTs of SoCs
>>>>>> that actually require it. No need to re-encode the dtsi in the binding.
>>>>>>
>>>>>
>>>>> But if you make a property valid in the binding then it implies that anyone
>>>>> could add it to DTS which is wrong. You should make this property valid for
>>>>> SoCs that actually support it.
>>>>
>>>> No, it's not wrong.
>>>>
>>>> Note that the binding only requires 'compatible' and 'regs', all other
>>>> properties are optional, and you could, for example, add a
>>>> 'reset' property to a node for a device which does not have a reset
>>>> without the DT validation failing.

Optional properties are optional primarily looking at one variant. It
means that on different boards with the same SoC, things can be routed a
bit differently and some property can be skipped. E.g. sometimes
regulators come from PMIC and sometimes are wired to some VBATT, so we
do not have regulator in DTS for them. Or some interrupt/pin is not
connected.

Now between variants of devices - different SoCs: I don't think that
"optional" should be used in such context, except special cases or lack
of knowledge about hardware. For given SoC/variant, the property is either:
1. valid and possible (can be required or optional),
2. not valid, not possible.
And this we should express in constraints, if doable with reasonable
complexity.

Therefore the question is: is dma-coherent not valid for other SoCs?

If it is "not needed" for other SoCs, then I would leave it like this.
Consider also what Rob said, that otherwise we would create DTS from the
bindings.

Also, too many allOf:if:then: constraints in the bindings make them
trickier to read.

>>>>
>>>
>>> Then what is the point of devicetree validation using bindings?
>>
>> You're still making sure that no properties are added that are not
>> documented, number of clocks, names of clocks, etc.
>>
>>> There is also a comment from Krzysztof: https://lkml.org/lkml/2022/11/24/390
>>
>> Speaking of Krzysztof:
>>
>> 	https://lore.kernel.org/lkml/20221204094717.74016-5-krzysztof.kozlowski@xxxxxxxxxx/

That's not the best example, because I just do not know where
dma-coherent is applicable and where it is not, thus I added it as valid
for all variants. Also, I think that all variants are capable of using
IOMMU - it isn't restricted per variant. If they are capable of IOMMU,
then dma-coherent is a possible choice.


Best regards,
Krzysztof