| Message ID | 20221205100837.29212-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: fix UFS DMA coherency | expand |
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 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, 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 >
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,