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

  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@kernel.org>
---
 Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
 1 file changed, 2 insertions(+)
  

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@kernel.org>
> ---
>  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> index f2d6298d926c..1f1d286749c0 100644
> --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> @@ -44,6 +44,8 @@ properties:
>      minItems: 8
>      maxItems: 11
>  
> +  dma-coherent: true
> +

This property is not applicable to all SoCs. So setting true here will make it
valid for all.

Thanks,
Mani

>    interconnects:
>      minItems: 2
>      maxItems: 2
> -- 
> 2.37.4
>
  

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@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > index f2d6298d926c..1f1d286749c0 100644
> > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > @@ -44,6 +44,8 @@ properties:
> >      minItems: 8
> >      maxItems: 11
> >  
> > +  dma-coherent: true
> > +
> 
> This property is not applicable to all SoCs. So setting true here will make it
> valid for all.

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.

Johan
  

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@kernel.org>
> > > ---
> > >  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > index f2d6298d926c..1f1d286749c0 100644
> > > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > @@ -44,6 +44,8 @@ properties:
> > >      minItems: 8
> > >      maxItems: 11
> > >  
> > > +  dma-coherent: true
> > > +
> > 
> > This property is not applicable to all SoCs. So setting true here will make it
> > valid for all.
> 
> 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.

Thanks,
Mani

> Johan
  

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@kernel.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > > index f2d6298d926c..1f1d286749c0 100644
> > > > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > > @@ -44,6 +44,8 @@ properties:
> > > >      minItems: 8
> > > >      maxItems: 11
> > > >  
> > > > +  dma-coherent: true
> > > > +
> > > 
> > > This property is not applicable to all SoCs. So setting true here will make it
> > > valid for all.
> > 
> > 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.

It's the devicetree which is supposed to describe hardware, you don't
have to encode it also in the binding.

Johan
  

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@kernel.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > > > index f2d6298d926c..1f1d286749c0 100644
> > > > > --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > > > +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> > > > > @@ -44,6 +44,8 @@ properties:
> > > > >      minItems: 8
> > > > >      maxItems: 11
> > > > >  
> > > > > +  dma-coherent: true
> > > > > +
> > > > 
> > > > This property is not applicable to all SoCs. So setting true here will make it
> > > > valid for all.
> > > 
> > > 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.
> 

Then what is the point of devicetree validation using bindings?

There is also a comment from Krzysztof: https://lkml.org/lkml/2022/11/24/390

Thanks,
Mani

> It's the devicetree which is supposed to describe hardware, you don't
> have to encode it also in the binding.
> 
> Johan
  

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@kernel.org>

> > > > 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.
> > 
> 
> 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@linaro.org/

Johan
  

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@kernel.org>
> 
> > > > > 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.
> > > 
> > 
> > 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@linaro.org/
> 

Heh... I will wait for him to chime in then.

Thanks,
Mani

> Johan
  

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.

Typically, you'd only be doing unnecessary cache flushes without it 
rather than getting data corruption. However, it is possible this 
property triggers other system setup or something that would cause 
problems if not setup right.

> > > > > > > 
> > > > > > > This is specifically needed on recent Qualcomm platforms like SC8280XP.
> > > > > > > 
> > > > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> > > > > 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.
> > > 
> > 
> > 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.

The schema can never be 100%. If it was, then we could practically just 
generate the DT.

IMO, 'dma-coherent' is bit special. I'd say it is valid on any DMA 
capable device node, but there's not any generic way to determine that. 
So it has to be explicit in a device binding.

Rob
  

On Mon, 05 Dec 2022 11:08:36 +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@kernel.org>
> ---
>  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
  

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@kernel.org>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

But I think this should be picked up by James or Martin. Added them as
explicit recipients, but perhaps they would like you to resubmit this?

I'm picking the dts change for now.

Thanks,
Bjorn

> ---
>  Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> index f2d6298d926c..1f1d286749c0 100644
> --- a/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> +++ b/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml
> @@ -44,6 +44,8 @@ properties:
>      minItems: 8
>      maxItems: 11
>  
> +  dma-coherent: true
> +
>    interconnects:
>      minItems: 2
>      maxItems: 2
> -- 
> 2.37.4
>
  

On Mon, Dec 05, 2022 at 04:35:22PM -0600, Rob Herring 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.
> 
> Typically, you'd only be doing unnecessary cache flushes without it 
> rather than getting data corruption. However, it is possible this 
> property triggers other system setup or something that would cause 
> problems if not setup right.

You can end up with data corruption, for example, if the kernel remaps
a consistent buffer and writes data through the non-cacheable alias
while the coherent device snoops stale data from the caches.

Johan
  

On Mon, Dec 05, 2022 at 05:07:51PM -0600, Bjorn Andersson 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@kernel.org>
> 
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> 
> But I think this should be picked up by James or Martin. Added them as
> explicit recipients, but perhaps they would like you to resubmit this?

They were not included in the get_maintainer.pl output so I'm guessing
they prefer to pick UFS binding patches from the scsi list.

> I'm picking the dts change for now.

Thanks.

Johan
  

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@kernel.org>
>>
>>>>>> 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@linaro.org/

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
  

On Tue, Dec 06, 2022 at 09:14:30AM +0100, Krzysztof Kozlowski wrote:
> 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@kernel.org>
> >>
> >>>>>> 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?
> 

Yes, it is not valid on older SoCs because they don't support I/O coherency.
So setting this property on those un-supported SoCs may lead to wierd behavior.
This was the concern I had for setting this property valid for all SoCs.

So far we only know that SC8280XP and newer SoCs support I/O coherency.

Thanks,
Mani

> 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@linaro.org/
> 
> 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
>