| Message ID | 20220323175920.93155-1-singh.kuldeep87k@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] ARM: dts: spear13xx: Update SPI dma properties | expand |
On 23-03-22, 23:29, Kuldeep Singh wrote: > Reorder dmas and dma-names property for spi controller node to make it > compliant with bindings. > > Fixes: 6e8887f60f60 ("ARM: SPEAr13xx: Pass generic DW DMAC platform data from DT") > Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com> > --- > v2: > - Add fixes tag > - Remove 1/2 patch from series as it's not required > - v1 discussion link: > https://lore.kernel.org/linux-devicetree/20220312180615.68929-2-singh.kuldeep87k@gmail.com/ > > arch/arm/boot/dts/spear13xx.dtsi | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi > index c87b881b2c8b..45f0b2a33e02 100644 > --- a/arch/arm/boot/dts/spear13xx.dtsi > +++ b/arch/arm/boot/dts/spear13xx.dtsi > @@ -284,9 +284,8 @@ spi0: spi@e0100000 { > #size-cells = <0>; > interrupts = <0 31 0x4>; > status = "disabled"; > - dmas = <&dwdma0 4 0 0>, > - <&dwdma0 5 0 0>; > - dma-names = "tx", "rx"; > + dmas = <&dwdma0 5 0 0>, <&dwdma0 4 0 0>; > + dma-names = "rx", "tx"; > }; > > rtc@e0580000 { Rob, I tried to ask this at V1 as well [1]. Why do we need a patch like this ? Isn't this a DT tooling issue, where it is asking for a fixed order of values ?
On Thu, Mar 24, 2022 at 08:09:04AM +0530, Viresh Kumar wrote: > On 23-03-22, 23:29, Kuldeep Singh wrote: > > Reorder dmas and dma-names property for spi controller node to make it > > compliant with bindings. > > > > Fixes: 6e8887f60f60 ("ARM: SPEAr13xx: Pass generic DW DMAC platform data from DT") > > Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com> > > --- > > v2: > > - Add fixes tag > > - Remove 1/2 patch from series as it's not required > > - v1 discussion link: > > https://lore.kernel.org/linux-devicetree/20220312180615.68929-2-singh.kuldeep87k@gmail.com/ > > > > arch/arm/boot/dts/spear13xx.dtsi | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi > > index c87b881b2c8b..45f0b2a33e02 100644 > > --- a/arch/arm/boot/dts/spear13xx.dtsi > > +++ b/arch/arm/boot/dts/spear13xx.dtsi > > @@ -284,9 +284,8 @@ spi0: spi@e0100000 { > > #size-cells = <0>; > > interrupts = <0 31 0x4>; > > status = "disabled"; > > - dmas = <&dwdma0 4 0 0>, > > - <&dwdma0 5 0 0>; > > - dma-names = "tx", "rx"; > > + dmas = <&dwdma0 5 0 0>, <&dwdma0 4 0 0>; > > + dma-names = "rx", "tx"; > > }; > > > > rtc@e0580000 { > > Rob, > > I tried to ask this at V1 as well [1]. Why do we need a patch like > this ? Isn't this a DT tooling issue, where it is asking for a fixed > order of values ? Fixed order of values is important in case of properties like compatibles etc. In case of dma-names, yes order shouldn't matter here. This patch is more of appeasing dtbs_check warning rather than fixing something. It's safe to go with this patch. I am not sure if there's a provision to exclude dma-names from fix ordering checks. Rob can help here in providing better insights. - Kuldeep
On 24-03-22, 11:55, Kuldeep Singh wrote: > Fixed order of values is important in case of properties like > compatibles etc. In case of dma-names, yes order shouldn't matter here. > > This patch is more of appeasing dtbs_check warning rather than fixing > something. Exactly my point. We have seen similar type of issues with other tools, like coccinelle, earlier and such patches were rejected as the kernel was just fine and tooling needs to be fixed. > It's safe to go with this patch. > I am not sure if there's a provision to exclude dma-names from fix > ordering checks. Rob can help here in providing better insights.
On Fri, Mar 25, 2022 at 2:58 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 24-03-22, 11:55, Kuldeep Singh wrote: > > Fixed order of values is important in case of properties like > > compatibles etc. In case of dma-names, yes order shouldn't matter here. > > > > This patch is more of appeasing dtbs_check warning rather than fixing > > something. > > Exactly my point. We have seen similar type of issues with other tools, like > coccinelle, earlier and such patches were rejected as the kernel was just fine > and tooling needs to be fixed. > > > It's safe to go with this patch. > > I am not sure if there's a provision to exclude dma-names from fix > > ordering checks. Rob can help here in providing better insights. I think it's a question of the scale of the warnings: my understanding is that there are only a handful of dts files that trigger the warning at all, and it would be rather hard to change the tooling around this. Since the proposed dts change is clearly harmless, I don't mind applying it. Kuldeep, you have probably looked at all dts files in the kernel, can you say how many of them are affected by the dma property reordering? Arnd
On 25-03-22, 10:11, Arnd Bergmann wrote: > On Fri, Mar 25, 2022 at 2:58 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 24-03-22, 11:55, Kuldeep Singh wrote: > > > Fixed order of values is important in case of properties like > > > compatibles etc. In case of dma-names, yes order shouldn't matter here. > > > > > > This patch is more of appeasing dtbs_check warning rather than fixing > > > something. > > > > Exactly my point. We have seen similar type of issues with other tools, like > > coccinelle, earlier and such patches were rejected as the kernel was just fine > > and tooling needs to be fixed. > > > > > It's safe to go with this patch. > > > I am not sure if there's a provision to exclude dma-names from fix > > > ordering checks. Rob can help here in providing better insights. > > I think it's a question of the scale of the warnings: my understanding is that > there are only a handful of dts files that trigger the warning at all, and it > would be rather hard to change the tooling around this. Since the proposed > dts change is clearly harmless, I don't mind applying it. I am fine if we want to apply this patch as is. Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Fri, Mar 25, 2022 at 10:11:41AM +0100, Arnd Bergmann wrote: > On Fri, Mar 25, 2022 at 2:58 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 24-03-22, 11:55, Kuldeep Singh wrote: > > > Fixed order of values is important in case of properties like > > > compatibles etc. In case of dma-names, yes order shouldn't matter here. > > > > > > This patch is more of appeasing dtbs_check warning rather than fixing > > > something. > > > > Exactly my point. We have seen similar type of issues with other tools, like > > coccinelle, earlier and such patches were rejected as the kernel was just fine > > and tooling needs to be fixed. > > > > > It's safe to go with this patch. > > > I am not sure if there's a provision to exclude dma-names from fix > > > ordering checks. Rob can help here in providing better insights. > > I think it's a question of the scale of the warnings: my understanding is that > there are only a handful of dts files that trigger the warning at all, and it > would be rather hard to change the tooling around this. Since the proposed > dts change is clearly harmless, I don't mind applying it. > > Kuldeep, you have probably looked at all dts files in the kernel, can you > say how many of them are affected by the dma property reordering? I have checked spi-pl022.yaml as of now and this was the only one which was affected with dma ordering. For all dts files, I can definitely give a try and will come up with some numbers. Please note, there are still bindings left to be converted to yaml format, so won't be able to catch those cases. - Kuldeep
On Fri, Mar 25, 2022 at 11:08:26PM +0530, Kuldeep Singh wrote: > On Fri, Mar 25, 2022 at 10:11:41AM +0100, Arnd Bergmann wrote: > > On Fri, Mar 25, 2022 at 2:58 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > On 24-03-22, 11:55, Kuldeep Singh wrote: > > > > Fixed order of values is important in case of properties like > > > > compatibles etc. In case of dma-names, yes order shouldn't matter here. > > > > > > > > This patch is more of appeasing dtbs_check warning rather than fixing > > > > something. > > > > > > Exactly my point. We have seen similar type of issues with other tools, like > > > coccinelle, earlier and such patches were rejected as the kernel was just fine > > > and tooling needs to be fixed. > > > > > > > It's safe to go with this patch. > > > > I am not sure if there's a provision to exclude dma-names from fix > > > > ordering checks. Rob can help here in providing better insights. > > > > I think it's a question of the scale of the warnings: my understanding is that > > there are only a handful of dts files that trigger the warning at all, and it > > would be rather hard to change the tooling around this. Since the proposed > > dts change is clearly harmless, I don't mind applying it. > > > > Kuldeep, you have probably looked at all dts files in the kernel, can you > > say how many of them are affected by the dma property reordering? > > I have checked spi-pl022.yaml as of now and this was the only one which > was affected with dma ordering. > > For all dts files, I can definitely give a try and will come up with > some numbers. Please note, there are still bindings left to be converted > to yaml format, so won't be able to catch those cases. I checked and found below dts with dma-names warnings[1]. spear1340, spear1310, spear13xx, fsl-ls1043a, fsl-ls1046a Yes Arnd, you were right. Very few dts are affected by dma ordering right now. There might be cases where nodes don't define generic names and thus not running required checks, still I assume the numbers will be very less. So, probably we can go ahead with the change. - Kuldeep [1] root@9a2d8922b8f1:~/linux/torvalds# grep "'rx' was expected" output.txt arch/arm/boot/dts/spear1340-evb.dt.yaml: spi@e0100000: dma-names:0: 'rx' was expected arch/arm/boot/dts/spear1310-evb.dt.yaml: spi@e0100000: dma-names:0: 'rx' was expected arch/arm/boot/dts/spear1340-evb.dt.yaml: serial@b4100000: dma-names:0: 'rx' was expected root@9a2d8922b8f1:~/linux/torvalds# grep "'rx' was expected" arm64_all arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dt.yaml: i2c@2180000: dma-names:0: 'rx' was expected arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dt.yaml: i2c@2180000: dma-names:0: 'rx' was expected arch/arm64/boot/dts/freescale/fsl-ls1046a-frwy.dt.yaml: i2c@2180000: dma-names:0: 'rx' was expected arch/arm64/boot/dts/freescale/fsl-ls1046a-qds.dt.yaml: i2c@2180000: dma-names:0: 'rx' was expected arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dt.yaml: i2c@2180000: dma-names:0: 'rx' was expected
diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi index c87b881b2c8b..45f0b2a33e02 100644 --- a/arch/arm/boot/dts/spear13xx.dtsi +++ b/arch/arm/boot/dts/spear13xx.dtsi @@ -284,9 +284,8 @@ spi0: spi@e0100000 { #size-cells = <0>; interrupts = <0 31 0x4>; status = "disabled"; - dmas = <&dwdma0 4 0 0>, - <&dwdma0 5 0 0>; - dma-names = "tx", "rx"; + dmas = <&dwdma0 5 0 0>, <&dwdma0 4 0 0>; + dma-names = "rx", "tx"; }; rtc@e0580000 {
Reorder dmas and dma-names property for spi controller node to make it compliant with bindings. Fixes: 6e8887f60f60 ("ARM: SPEAr13xx: Pass generic DW DMAC platform data from DT") Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com> --- v2: - Add fixes tag - Remove 1/2 patch from series as it's not required - v1 discussion link: https://lore.kernel.org/linux-devicetree/20220312180615.68929-2-singh.kuldeep87k@gmail.com/ arch/arm/boot/dts/spear13xx.dtsi | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)