Skip to contentSkip to conversation

arm: dts: broadcom: Add missing required fields

PATCH In review Karan Sanghavi 5 participants 7 messages Sep 25, 2024 – Oct 1, 2024

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm: dts: broadcom: Add missing required fields

Added below mentioned required fields
1. interrupt-controller
2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

---
 arch/arm/boot/dts/broadcom/bcm2711.dtsi | 2 ++
 1 file changed, 2 insertions(+)
arch/arm/boot/dts/broadcom/bcm2711.dtsi +2 -0
index e4e42af21ef3..313b1046d74f 100644
@@ -51,6 +51,8 @@ soc {
 		local_intc: interrupt-controller@40000000 {
 			compatible = "brcm,bcm2836-l1-intc";
 			reg = <0x40000000 0x100>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 		};
 
 		gicv2: interrupt-controller@40041000 {
Signature · 1 line
2.43.0
#1 lore raw

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:

Quoted 10 lines

Added below mentioned required fields
1. interrupt-controller
2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

looks like you made the same mistake like me [1]. This change breaks boot of Raspberry Pi 4 [2].

There are a lot of DT schema warnings to fix, but this doesn't belong to the trivial ones.

[1] -
https://lore.kernel.org/linux-arm-kernel/[email protected]/
[2] -
https://lore.kernel.org/linux-arm-kernel/CA+G9fYuncv0fuBSC0A1z1G_UOv_XuMQz=DsrLZDK-Wc=10ghag@mail.gmail.com/

Quoted 19 lines
---
  arch/arm/boot/dts/broadcom/bcm2711.dtsi | 2 ++
  1 file changed, 2 insertions(+)
arch/arm/boot/dts/broadcom/bcm2711.dtsi +2 -0
index e4e42af21ef3..313b1046d74f 100644
@@ -51,6 +51,8 @@ soc {
  		local_intc: interrupt-controller@40000000 {
  			compatible = "brcm,bcm2836-l1-intc";
  			reg = <0x40000000 0x100>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
  		};
 
  		gicv2: interrupt-controller@40041000 {
#2 lore raw
Quoted 13 lines · Stefan Wahren

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:

Quoted 10 lines

Added below mentioned required fields
1. interrupt-controller
2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

Quoted 6 lines

looks like you made the same mistake like me [1]. This change breaks boot of Raspberry Pi 4 [2].

There are a lot of DT schema warnings to fix, but this doesn't belong to the trivial ones.

Karan,

Entire commit msg lacks proper rationale for such significant change. Rationale is for example: "this is an interrupt controller", but here reason is rather "I want to fix error".

Important for every work focusing on fixing warnings/errors is to fix the cause, not the warning/error itself. Karan, you fixed the warning in a way it went away, but this did no fix the cause of the problem. You must find the real causes. Usually understanding the problem is necessary for that.

Best regards,
Krzysztof

maintainer
#3 lore raw
In reply to Stefan Wahren
Quoted 13 lines · Stefan Wahren

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:

Quoted 10 lines

Added below mentioned required fields
   1. interrupt-controller
   2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

Quoted 5 lines

looks like you made the same mistake like me [1]. This change breaks boot of Raspberry Pi 4 [2].

There are a lot of DT schema warnings to fix, but this doesn't belong to the trivial ones.

Including the #interrupt-cells would not have a functional impact however, and we ought to be able to do that.

The 'interrupt-controller' property presence means that the controller will be picked up by of_irq_init() and that is was causes problems for people testing this. Stefan, do you know if the VPU firmware removes/inserts that property to tell Linux which interrupt controller (bcm2836-l1-intc or ARM GIC) to use or does it make use of the "status" property which would be the canonical way about doing that?

Thanks!

Signature · 1 line
Florian
#4 lore raw
Quoted 31 lines · Florian Fainelli
Quoted 13 lines · Stefan Wahren

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:

Quoted 10 lines

Added below mentioned required fields
1. interrupt-controller
2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

Quoted 5 lines

looks like you made the same mistake like me [1]. This change breaks boot of Raspberry Pi 4 [2].

There are a lot of DT schema warnings to fix, but this doesn't belong to the trivial ones.

Including the #interrupt-cells would not have a functional impact however, and we ought to be able to do that.

The 'interrupt-controller' property presence means that the controller will be picked up by of_irq_init() and that is was causes problems for people testing this. Stefan, do you know if the VPU firmware removes/inserts that property to tell Linux which interrupt controller (bcm2836-l1-intc or ARM GIC) to use or does it make use of the "status" property which would be the canonical way about doing that?

Quoted 3 lines

Thanks!
--
Florian

Hello Florian,

So should I send a patch with only #interrupt-cells to solve one problem?

Thank you.

#5 lore raw
In reply to Florian Fainelli

Hi Florian,

Am 25.09.24 um 22:38 schrieb Florian Fainelli:

Quoted 23 lines · Stefan Wahren
Quoted 13 lines

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:

Quoted 10 lines

Added below mentioned required fields
   1. interrupt-controller
   2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

Quoted 5 lines

looks like you made the same mistake like me [1]. This change breaks boot of Raspberry Pi 4 [2].

There are a lot of DT schema warnings to fix, but this doesn't belong to the trivial ones.

Including the #interrupt-cells would not have a functional impact however, and we ought to be able to do that.

Quoted 6 lines

The 'interrupt-controller' property presence means that the controller will be picked up by of_irq_init() and that is was causes problems for people testing this. Stefan, do you know if the VPU firmware removes/inserts that property to tell Linux which interrupt controller (bcm2836-l1-intc or ARM GIC) to use or does it make use of the "status" property which would be the canonical way about doing that?

There is a config.txt parameter for this, which is called "enable_gic". But if i use this i couldn't see any difference to /proc/device-tree. Also i couldn't see any modifications by the firmware to the node in general:

interrupt-controller@40000000 {
        compatible = "brcm,bcm2836-l1-intc";
        reg = <0x40000000 0x100>;
        phandle = <0x8e>;
};

Except of this i don't have any clue about the VPU firmware.

Regards

Quoted 2 lines

Thanks!

#6 lore raw

Hi Stefan and Florian

Quoted 38 lines · Stefan Wahren

Hi Florian,

Am 25.09.24 um 22:38 schrieb Florian Fainelli:

Quoted 23 lines · Stefan Wahren
Quoted 13 lines

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:

Quoted 10 lines

Added below mentioned required fields
1. interrupt-controller
2. #interrupt-cells
in the bcm2711.dtsi file for the interrupt-controller@40000000 block as defined in the bindings/interrupt-controller/brcm,bcm2836-l1-intc.yaml. This issue was noticed while compiling the dtb file for broadcom/bcm2711-rpi-4-b.dts file.
After including the above fields in the dtsi file interrupt-conntroller error was resolved.

Quoted 5 lines

looks like you made the same mistake like me [1]. This change breaks boot of Raspberry Pi 4 [2].

There are a lot of DT schema warnings to fix, but this doesn't belong to the trivial ones.

Including the #interrupt-cells would not have a functional impact however, and we ought to be able to do that.

Quoted 6 lines

The 'interrupt-controller' property presence means that the controller will be picked up by of_irq_init() and that is was causes problems for people testing this. Stefan, do you know if the VPU firmware removes/inserts that property to tell Linux which interrupt controller (bcm2836-l1-intc or ARM GIC) to use or does it make use of the "status" property which would be the canonical way about doing that?

There is a config.txt parameter for this, which is called "enable_gic". But if i use this i couldn't see any difference to /proc/device-tree. Also i couldn't see any modifications by the firmware to the node in general:

Quoted 7 lines
interrupt-controller@40000000 {
         compatible = "brcm,bcm2836-l1-intc";
         reg = <0x40000000 0x100>;
         phandle = <0x8e>;
};

Except of this i don't have any clue about the VPU firmware.

cc Phil so he can correct me if I get this wrong.

The firmware looks at the DTB and automatically sets the enable_gic property if DT /interrupt-parent points at a node with compatible string "arm,gic-400". It doesn't modify DT around the interrupt controller nodes.

Manually setting enable_gic should only be necessary on a system which isn't using DT where they wish to control whether to use the GIC or bcm2836-l1-intc.

Dave