| Message ID | 20191212113540.7.Ia9bd3fca24ad34a5faaf1c3e58095c74b38abca1@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: sc7180: Make dtbs_check mostly happy | expand |
Quoting Douglas Anderson (2019-12-12 11:35:43) > Running `make dtbs_check` yells: > > arch/arm64/boot/dts/qcom/sc7180-idp.dt.yaml: timer@17c20000: #size-cells:0:0: 1 was expected > > It appears that someone was trying to assert the fact that sub-nodes > describing frames would never have a size that's more than 32-bits > big. That's certainly true in the case of sc7180. > > I guess this is a hint that it's time to do the thing that nobody > seems to do but that "writing-bindings.txt" says we should all do. > Specifically it says: "DO use non-empty 'ranges' to limit the size of > child buses/devices". That means we should probably limit the It got cut off here. I'm waiting to find out what it is!! > > I believe that this patch is the way to do it and there should be no > bad side effects here. I believe that since we're far enough down > (not trying to describe an actual device, just some sub-pieces) that > this won't cause us to run into the problems that caused us to > increase the soc-level #address-cells and #size-cells to 2 in sdm845 > in commit bede7d2dc8f3 ("arm64: dts: qcom: sdm845: Increase address > and size cells for soc"). > > I can at least confirm that "arch_mem_timer" seems to keep getting > interrupts in "/proc/interrupts" after this change. > > Fixes: 90db71e48070 ("arm64: dts: sc7180: Add minimal dts/dtsi files for SC7180 soc") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- This pattern exists in most of the qcom dts files. Can you fix all the arm,armv7-timer-mem nodes. Maybe the binding has the same problem too in the example. > > arch/arm64/boot/dts/qcom/sc7180.dtsi | 36 ++++++++++++++-------------- > 1 file changed, 18 insertions(+), 18 deletions(-) >
Hi, On Thu, Dec 12, 2019 at 1:55 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Douglas Anderson (2019-12-12 11:35:43) > > Running `make dtbs_check` yells: > > > > arch/arm64/boot/dts/qcom/sc7180-idp.dt.yaml: timer@17c20000: #size-cells:0:0: 1 was expected > > > > It appears that someone was trying to assert the fact that sub-nodes > > describing frames would never have a size that's more than 32-bits > > big. That's certainly true in the case of sc7180. > > > > I guess this is a hint that it's time to do the thing that nobody > > seems to do but that "writing-bindings.txt" says we should all do. > > Specifically it says: "DO use non-empty 'ranges' to limit the size of > > child buses/devices". That means we should probably limit the > > It got cut off here. I'm waiting to find out what it is!! I was going to say that I should use ranges to limit the address cells in addition to the size cells, but then I think I must have got distracted and forgot to finish my > > I believe that this patch is the way to do it and there should be no > > bad side effects here. I believe that since we're far enough down > > (not trying to describe an actual device, just some sub-pieces) that > > this won't cause us to run into the problems that caused us to > > increase the soc-level #address-cells and #size-cells to 2 in sdm845 > > in commit bede7d2dc8f3 ("arm64: dts: qcom: sdm845: Increase address > > and size cells for soc"). > > > > I can at least confirm that "arch_mem_timer" seems to keep getting > > interrupts in "/proc/interrupts" after this change. > > > > Fixes: 90db71e48070 ("arm64: dts: sc7180: Add minimal dts/dtsi files for SC7180 soc") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > This pattern exists in most of the qcom dts files. Can you fix all the > arm,armv7-timer-mem nodes. Maybe the binding has the same problem too in > the example. Yeah. I'm a little scared to go and do this for every qcom device tree file since I have no good way to test them, but I suppose I can give it a shot. I was kinda thinking that, in general, it would make sense for folks to tackle one SoC at a time and make that SoC clean and test it. In any case, your idea about updating the example seemed wise to me, so I sent out: https://lore.kernel.org/r/20191216220512.1.I7dbd712cfe0bdf7b53d9ef9791072b7e9c6d3c33@changeid I'll put this patch on hold until Rob gives his thoughts on that one so we can really make sure we're supposed to be using ranges in this way. -Doug
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index c671f0719d42..336cf65bdcc2 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -1206,59 +1206,59 @@ watchdog@17c10000 { }; timer@17c20000{ - #address-cells = <2>; - #size-cells = <2>; - ranges; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0 0x17c21000 0x10000>; compatible = "arm,armv7-timer-mem"; reg = <0 0x17c20000 0 0x1000>; - frame@17c21000 { + frame@0 { frame-number = <0>; interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c21000 0 0x1000>, - <0 0x17c22000 0 0x1000>; + reg = <0x0000 0x1000>, + <0x1000 0x1000>; }; - frame@17c23000 { + frame@2000 { frame-number = <1>; interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c23000 0 0x1000>; + reg = <0x2000 0x1000>; status = "disabled"; }; - frame@17c25000 { + frame@4000 { frame-number = <2>; interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c25000 0 0x1000>; + reg = <0x4000 0x1000>; status = "disabled"; }; - frame@17c27000 { + frame@6000 { frame-number = <3>; interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c27000 0 0x1000>; + reg = <0x6000 0x1000>; status = "disabled"; }; - frame@17c29000 { + frame@8000 { frame-number = <4>; interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c29000 0 0x1000>; + reg = <0x8000 0x1000>; status = "disabled"; }; - frame@17c2b000 { + frame@a000 { frame-number = <5>; interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c2b000 0 0x1000>; + reg = <0xa000 0x1000>; status = "disabled"; }; - frame@17c2d000 { + frame@c000 { frame-number = <6>; interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; - reg = <0 0x17c2d000 0 0x1000>; + reg = <0xc000 0x1000>; status = "disabled"; }; };
Running `make dtbs_check` yells: arch/arm64/boot/dts/qcom/sc7180-idp.dt.yaml: timer@17c20000: #size-cells:0:0: 1 was expected It appears that someone was trying to assert the fact that sub-nodes describing frames would never have a size that's more than 32-bits big. That's certainly true in the case of sc7180. I guess this is a hint that it's time to do the thing that nobody seems to do but that "writing-bindings.txt" says we should all do. Specifically it says: "DO use non-empty 'ranges' to limit the size of child buses/devices". That means we should probably limit the I believe that this patch is the way to do it and there should be no bad side effects here. I believe that since we're far enough down (not trying to describe an actual device, just some sub-pieces) that this won't cause us to run into the problems that caused us to increase the soc-level #address-cells and #size-cells to 2 in sdm845 in commit bede7d2dc8f3 ("arm64: dts: qcom: sdm845: Increase address and size cells for soc"). I can at least confirm that "arch_mem_timer" seems to keep getting interrupts in "/proc/interrupts" after this change. Fixes: 90db71e48070 ("arm64: dts: sc7180: Add minimal dts/dtsi files for SC7180 soc") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- arch/arm64/boot/dts/qcom/sc7180.dtsi | 36 ++++++++++++++-------------- 1 file changed, 18 insertions(+), 18 deletions(-)