Re: [RFC PATCH] arm64: broadcom: bcm2712: Rework rp1 overlay handling

[Andrea della Porta <andrea.porta@suse.com>]

Hi Rob,
sorry for the delay...

On 15:15 Mon 17 Nov     , Rob Herring (Arm) wrote:
> It's a requirement that DT overlays be applied at build time in order to
> validate them as overlays are not validated on their own.
> 
> Simply adding a build time target is possible, but it's also not desirable
> to have targets which include the overlay contents and apply the same
> overlay.
> 
> Rework the targets such that there are 2 overlays: the existing RP1 overlay
> and the board specific configuration of the RP1 (bcm2712-rpi-5-b.dtso).
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> The missing applying of rp1.dtbo will soon be a warning[1].
> 
> The comment in bcm2712-rpi-5-b.dts(o) needs updating or removing. The RP1
> driver or userspace handling of the overlay probably needs updating to
> handle the 2 overlays. IIRC, I think we can apply overlays to overlays
> which may be useful here. If not, adding that to the tools should be
> possible.
> 
> [1] https://lore.kernel.org/all/20251114222759.4181152-1-robh@kernel.org/
> ---
>  arch/arm64/boot/dts/broadcom/Makefile                 | 10 ++++++++--
>  ...m2712-d-rpi-5-b.dts => bcm2712-d-rpi-5-b-base.dts} |  2 +-
>  .../{bcm2712-rpi-5-b.dts => bcm2712-rpi-5-b.dtso}     | 11 +++++------
>  3 files changed, 14 insertions(+), 9 deletions(-)
>  rename arch/arm64/boot/dts/broadcom/{bcm2712-d-rpi-5-b.dts => bcm2712-d-rpi-5-b-base.dts} (95%)
>  rename arch/arm64/boot/dts/broadcom/{bcm2712-rpi-5-b.dts => bcm2712-rpi-5-b.dtso} (92%)
> 
> diff --git a/arch/arm64/boot/dts/broadcom/Makefile b/arch/arm64/boot/dts/broadcom/Makefile
> index 83d45afc6588..4957edec9d9f 100644
> --- a/arch/arm64/boot/dts/broadcom/Makefile
> +++ b/arch/arm64/boot/dts/broadcom/Makefile
> @@ -6,9 +6,9 @@ DTC_FLAGS := -@
>  dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \
>  			      bcm2711-rpi-4-b.dtb \
>  			      bcm2711-rpi-cm4-io.dtb \
> -			      bcm2712-rpi-5-b.dtb \
> +			      bcm2712-rpi-5-b.dtbo \
>  			      bcm2712-rpi-5-b-ovl-rp1.dtb \
> -			      bcm2712-d-rpi-5-b.dtb \
> +			      bcm2712-d-rpi-5-b-base.dtb \
>  			      bcm2837-rpi-2-b.dtb \
>  			      bcm2837-rpi-3-a-plus.dtb \
>  			      bcm2837-rpi-3-b.dtb \
> @@ -17,6 +17,12 @@ dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \
>  			      bcm2837-rpi-zero-2-w.dtb \
>  			      rp1.dtbo
>  
> +bcm2712-rpi-5-b-dtbs := bcm2712-rpi-5-b-ovl-rp1.dtb rp1.dtbo bcm2712-rpi-5-b.dtbo
> +dtb-$(CONFIG_ARCH_BCM2835) += bcm2712-rpi-5-b.dtb
> +
> +bcm2712-d-rpi-5-b-dtbs := bcm2712-d-rpi-5-b-base.dtb rp1.dtbo bcm2712-rpi-5-b.dtbo
> +dtb-$(CONFIG_ARCH_BCM2835) += bcm2712-d-rpi-5-b.dtb
> +
>  subdir-y	+= bcmbca
>  subdir-y	+= northstar2
>  subdir-y	+= stingray
> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-d-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-d-rpi-5-b-base.dts
> similarity index 95%
> rename from arch/arm64/boot/dts/broadcom/bcm2712-d-rpi-5-b.dts
> rename to arch/arm64/boot/dts/broadcom/bcm2712-d-rpi-5-b-base.dts
> index 7de24d60bcd1..48a0ae118839 100644
> --- a/arch/arm64/boot/dts/broadcom/bcm2712-d-rpi-5-b.dts
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712-d-rpi-5-b-base.dts
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: (GPL-2.0 OR MIT)
>  /dts-v1/;
>  
> -#include "bcm2712-rpi-5-b.dts"
> +#include "bcm2712-rpi-5-b-ovl-rp1.dts"
>  
>  &gio_aon {
>  	brcm,gpio-bank-widths = <15 6>;
> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtso
> similarity index 92%
> rename from arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
> rename to arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtso
> index b8f256545022..89d099c00035 100644
> --- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dtso
> @@ -15,19 +15,18 @@
>   */
>  
>  /dts-v1/;
> +/plugin/;
>  
> -#include "bcm2712-rpi-5-b-ovl-rp1.dts"
> -
> -&pcie2 {
> -	#include "rp1-nexus.dtsi"
> -};
> +#include <dt-bindings/gpio/gpio.h>

Sounds reasonable to me, please let me have a full round of tests to check for
any issue with the driver and I'll come back with feedback.

>  
>  &rp1_eth {
>  	status = "okay";
>  	phy-mode = "rgmii-id";
>  	phy-handle = <&phy1>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;

I think this should go in a separate patch.

>  
> -	mdio {
> +	mdio@1 {

Ditto.

Many thanks,
Andrea

>  		reg = <0x1>;
>  		reset-gpios = <&rp1_gpio 32 GPIO_ACTIVE_LOW>;
>  		reset-delay-us = <5000>;
> -- 
> 2.51.0
>