[PATCH 0/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

[PATCH 0/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

[Johan Hovold]

The SC8280XP UFS controllers are cache coherent and must be marked as
such in the devicetree to avoid potential data corruption.

Johan


Johan Hovold (2):
  dt-bindings: ufs: qcom: allow 'dma-coherent' property
  arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

 Documentation/devicetree/bindings/ufs/qcom,ufs.yaml | 2 ++
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi              | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.37.4

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

[Johan Hovold]

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
+
   interconnects:
     minItems: 2
     maxItems: 2
-- 
2.37.4

[PATCH 2/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

[Johan Hovold]

The SC8280XP UFS controllers are cache coherent and must be marked as
such in the devicetree to avoid potential data corruption.

Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
Cc: stable@vger.kernel.org      # 6.0
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index c4947c563099..23d1f51527aa 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1430,6 +1430,7 @@ ufs_mem_hc: ufs@1d84000 {
 			required-opps = <&rpmhpd_opp_nom>;
 
 			iommus = <&apps_smmu 0xe0 0x0>;
+			dma-coherent;
 
 			clocks = <&gcc GCC_UFS_PHY_AXI_CLK>,
 				 <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
@@ -1491,6 +1492,7 @@ ufs_card_hc: ufs@1da4000 {
 			power-domains = <&gcc UFS_CARD_GDSC>;
 
 			iommus = <&apps_smmu 0x4a0 0x0>;
+			dma-coherent;
 
 			clocks = <&gcc GCC_UFS_CARD_AXI_CLK>,
 				 <&gcc GCC_AGGRE_UFS_CARD_AXI_CLK>,
-- 
2.37.4

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

[Konrad Dybcio]

On 05/12/2022 11:08, Johan Hovold wrote:
> The SC8280XP UFS controllers are cache coherent and must be marked as
> such in the devicetree to avoid potential data corruption.
> 
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Cc: stable@vger.kernel.org      # 6.0
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index c4947c563099..23d1f51527aa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1430,6 +1430,7 @@ ufs_mem_hc: ufs@1d84000 {
>   			required-opps = <&rpmhpd_opp_nom>;
>   
>   			iommus = <&apps_smmu 0xe0 0x0>;
> +			dma-coherent;
>   
>   			clocks = <&gcc GCC_UFS_PHY_AXI_CLK>,
>   				 <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
> @@ -1491,6 +1492,7 @@ ufs_card_hc: ufs@1da4000 {
>   			power-domains = <&gcc UFS_CARD_GDSC>;
>   
>   			iommus = <&apps_smmu 0x4a0 0x0>;
> +			dma-coherent;
>   
>   			clocks = <&gcc GCC_UFS_CARD_AXI_CLK>,
>   				 <&gcc GCC_AGGRE_UFS_CARD_AXI_CLK>,

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

[Manivannan Sadhasivam]

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
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

[Manivannan Sadhasivam]

On Mon, Dec 05, 2022 at 11:08:37AM +0100, Johan Hovold wrote:
> The SC8280XP UFS controllers are cache coherent and must be marked as
> such in the devicetree to avoid potential data corruption.
> 
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Cc: stable@vger.kernel.org      # 6.0
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index c4947c563099..23d1f51527aa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1430,6 +1430,7 @@ ufs_mem_hc: ufs@1d84000 {
>  			required-opps = <&rpmhpd_opp_nom>;
>  
>  			iommus = <&apps_smmu 0xe0 0x0>;
> +			dma-coherent;
>  
>  			clocks = <&gcc GCC_UFS_PHY_AXI_CLK>,
>  				 <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
> @@ -1491,6 +1492,7 @@ ufs_card_hc: ufs@1da4000 {
>  			power-domains = <&gcc UFS_CARD_GDSC>;
>  
>  			iommus = <&apps_smmu 0x4a0 0x0>;
> +			dma-coherent;
>  
>  			clocks = <&gcc GCC_UFS_CARD_AXI_CLK>,
>  				 <&gcc GCC_AGGRE_UFS_CARD_AXI_CLK>,
> -- 
> 2.37.4
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

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

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

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

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

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

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Johan Hovold]

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

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

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்

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

[Rob Herring]

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

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

[Rob Herring]

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>

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

[Bjorn Andersson]

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
> 

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

[Johan Hovold]

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

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

[Johan Hovold]

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

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@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

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

[Manivannan Sadhasivam]

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
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: (subset) [PATCH 0/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency

[Bjorn Andersson]

On Mon, 5 Dec 2022 11:08:35 +0100, Johan Hovold wrote:
> The SC8280XP UFS controllers are cache coherent and must be marked as
> such in the devicetree to avoid potential data corruption.
> 
> Johan
> 
> 
> Johan Hovold (2):
>   dt-bindings: ufs: qcom: allow 'dma-coherent' property
>   arm64: dts: qcom: sc8280xp: fix UFS DMA coherency
> 
> [...]

Applied, thanks!

[2/2] arm64: dts: qcom: sc8280xp: fix UFS DMA coherency
      commit: 0953777640354dc459a22369eea488603d225dd9

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>