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 <[email protected]>
> > ---
> > v2:
> > - Add fixes tag
> > - Remove 1/2 patch from series as it's not required
> > - v1 discussion link:
> > https://lore.kernel.org/linux-devicetree/[email protected]/
> >
> > 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.
--
viresh
On Fri, Mar 25, 2022 at 2:58 AM Viresh Kumar <[email protected]> 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 Fri, Mar 25, 2022 at 10:11:41AM +0100, Arnd Bergmann wrote:
> On Fri, Mar 25, 2022 at 2:58 AM Viresh Kumar <[email protected]> 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 <[email protected]> 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