[PATCH] arm: dts: broadcom: Add missing required fields

[PATCH] arm: dts: broadcom: Add missing required fields

[Karan Sanghavi]

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.

Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
---
 arch/arm/boot/dts/broadcom/bcm2711.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/broadcom/bcm2711.dtsi b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
index e4e42af21ef3..313b1046d74f 100644
--- a/arch/arm/boot/dts/broadcom/bcm2711.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
@@ -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.43.0

Re: [PATCH] arm: dts: broadcom: Add missing required fields

[Stefan Wahren]

Hi Karan,

Am 25.09.24 um 18:14 schrieb Karan Sanghavi:
> 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/20240812200358.4061-5-wahrenst@gmx.net/
[2] -
https://lore.kernel.org/linux-arm-kernel/CA+G9fYuncv0fuBSC0A1z1G_UOv_XuMQz=DsrLZDK-Wc=10ghag@mail.gmail.com/
>
> Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
> ---
>   arch/arm/boot/dts/broadcom/bcm2711.dtsi | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/broadcom/bcm2711.dtsi b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
> index e4e42af21ef3..313b1046d74f 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm2711.dtsi
> @@ -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 {

Re: [PATCH] arm: dts: broadcom: Add missing required fields

[Krzysztof Kozlowski]

On 25/09/2024 18:39, Stefan Wahren wrote:
> Hi Karan,
> 
> Am 25.09.24 um 18:14 schrieb Karan Sanghavi:
>> 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.
> 

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

Re: [PATCH] arm: dts: broadcom: Add missing required fields

[Florian Fainelli]

On 9/25/24 09:39, Stefan Wahren wrote:
> Hi Karan,
> 
> Am 25.09.24 um 18:14 schrieb Karan Sanghavi:
>> 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.

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!
-- 
Florian

Re: [PATCH] arm: dts: broadcom: Add missing required fields

[Karan Sanghavi]

On Thu, 26 Sept 2024 at 02:08, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> On 9/25/24 09:39, Stefan Wahren wrote:
> > Hi Karan,
> >
> > Am 25.09.24 um 18:14 schrieb Karan Sanghavi:
> >> 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.
>
> 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!
> --
> Florian

Hello Florian,

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

Thank you.

Re: [PATCH] arm: dts: broadcom: Add missing required fields

[Stefan Wahren]

Hi Florian,

Am 25.09.24 um 22:38 schrieb Florian Fainelli:
> On 9/25/24 09:39, Stefan Wahren wrote:
>> Hi Karan,
>>
>> Am 25.09.24 um 18:14 schrieb Karan Sanghavi:
>>> 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.
>
> 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?
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
>
> Thanks!

Re: [PATCH] arm: dts: broadcom: Add missing required fields

[Dave Stevenson]

Hi Stefan and Florian

On Mon, 30 Sept 2024 at 19:36, Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Hi Florian,
>
> Am 25.09.24 um 22:38 schrieb Florian Fainelli:
> > On 9/25/24 09:39, Stefan Wahren wrote:
> >> Hi Karan,
> >>
> >> Am 25.09.24 um 18:14 schrieb Karan Sanghavi:
> >>> 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.
> >
> > 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?
> 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.

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