[PATCH 1/7] mmc: msm_sdhci: Match clocks through "clocks" property

[PATCH 1/7] mmc: msm_sdhci: Match clocks through "clocks" property

[Konrad Dybcio]

"clocks" is the standard property used in Linux, "clock" seems to be
an U-Boot invention. Use the one that's more standardized.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 drivers/mmc/msm_sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
index 604f9c3ff99c..174435f01f68 100644
--- a/drivers/mmc/msm_sdhci.c
+++ b/drivers/mmc/msm_sdhci.c
@@ -63,7 +63,7 @@ static int msm_sdc_clk_init(struct udevice *dev)
 	struct clk clk;
 	int ret;
 
-	ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2);
+	ret = fdtdec_get_int_array(gd->fdt_blob, node, "clocks", clkd, 2);
 	if (ret)
 		return ret;
 
-- 
2.40.0

[PATCH 2/7] serial: msm: Match clocks through "clocks" property

[Konrad Dybcio]

"clocks" is the standard property used in Linux, "clock" seems to be
an U-Boot invention. Use the one that's more standardized.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 arch/arm/dts/qcom-ipq4019.dtsi | 2 +-
 drivers/serial/serial_msm.c    | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi
index 6edc69da6747..dee3159e5893 100644
--- a/arch/arm/dts/qcom-ipq4019.dtsi
+++ b/arch/arm/dts/qcom-ipq4019.dtsi
@@ -87,7 +87,7 @@
 		blsp1_uart1: serial@78af000 {
 			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
 			reg = <0x78af000 0x200>;
-			clock = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
+			clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
 			bit-rate = <0xFF>;
 			status = "disabled";
 			u-boot,dm-pre-reloc;
diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
index a22623c316ed..9c370cac323f 100644
--- a/drivers/serial/serial_msm.c
+++ b/drivers/serial/serial_msm.c
@@ -166,8 +166,7 @@ static int msm_uart_clk_init(struct udevice *dev)
 	struct clk clk;
 	int ret;
 
-	ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clock",
-				   clkd, 2);
+	ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clocks", clkd, 2);
 	if (ret)
 		return ret;
 
-- 
2.40.0

[PATCH 3/7] serial: msm: Always bind before relocation

[Konrad Dybcio]

In preparation for supporting upstream Linux device trees on Qualcomm
platforms, make this the default behavior.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 drivers/serial/serial_msm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
index 9c370cac323f..59a2cf27aaf1 100644
--- a/drivers/serial/serial_msm.c
+++ b/drivers/serial/serial_msm.c
@@ -251,4 +251,5 @@ U_BOOT_DRIVER(serial_msm) = {
 	.priv_auto	= sizeof(struct msm_serial_data),
 	.probe = msm_serial_probe,
 	.ops	= &msm_serial_ops,
+	.flags		= DM_FLAG_PRE_RELOC,
 };
-- 
2.40.0

[PATCH 4/7] arch: snapdragon: clock: Always bind before relocation

[Konrad Dybcio]

In preparation for supporting upstream Linux device trees on Qualcomm
platforms, make this the default behavior.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 arch/arm/mach-snapdragon/clock-snapdragon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c
index 0ac45dce9a92..d1af5d1fec7d 100644
--- a/arch/arm/mach-snapdragon/clock-snapdragon.c
+++ b/arch/arm/mach-snapdragon/clock-snapdragon.c
@@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = {
 	.ops		= &msm_clk_ops,
 	.priv_auto	= sizeof(struct msm_clk_priv),
 	.probe		= msm_clk_probe,
+	.flags		= DM_FLAG_PRE_RELOC,
 };
-- 
2.40.0

[PATCH 5/7] arm: snapdragon: pinctrl: Always bind before relocation

[Konrad Dybcio]

In preparation for supporting upstream Linux device trees on Qualcomm
platforms, make this the default behavior.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 arch/arm/mach-snapdragon/pinctrl-snapdragon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c
index 826dc5148661..9f261d70e4d3 100644
--- a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c
+++ b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c
@@ -163,4 +163,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = {
 	.ops		= &msm_pinctrl_ops,
 	.probe		= msm_pinctrl_probe,
 	.bind		= msm_pinctrl_bind,
+	.flags		= DM_FLAG_PRE_RELOC,
 };
-- 
2.40.0

[PATCH 6/7] serial: msm_geni: Use upstream Linux bindings

[Konrad Dybcio]

The name "se" is used in upstream Linux device trees and has been for
ages, long before this U-Boot-ism was introduced. Same goes for the
existing compatible. Get rid of that.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 arch/arm/dts/sdm845.dtsi         | 4 ++--
 drivers/serial/serial_msm_geni.c | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi
index 607af277f8be..92bdc82177d6 100644
--- a/arch/arm/dts/sdm845.dtsi
+++ b/arch/arm/dts/sdm845.dtsi
@@ -52,10 +52,10 @@
 		};
 
 		debug_uart: serial@a84000 {
-			compatible = "qcom,msm-geni-uart";
+			compatible = "qcom,geni-debug-uart";
 			reg = <0xa84000 0x4000>;
 			reg-names = "se_phys";
-			clock-names = "se-clk";
+			clock-names = "se";
 			clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>;
 			pinctrl-names = "default";
 			pinctrl-0 = <&qup_uart9>;
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c
index 3943ca43e49e..6c9371c4e69d 100644
--- a/drivers/serial/serial_msm_geni.c
+++ b/drivers/serial/serial_msm_geni.c
@@ -189,7 +189,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate)
 	struct clk *clk;
 	int ret;
 
-	clk = devm_clk_get(dev, "se-clk");
+	clk = devm_clk_get(dev, "se");
 	if (!clk)
 		return -EINVAL;
 
@@ -554,7 +554,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev)
 }
 
 static const struct udevice_id msm_serial_ids[] = {
-	{.compatible = "qcom,msm-geni-uart"}, {}};
+	{ .compatible = "qcom,geni-debug-uart" },
+	{ }
+};
 
 U_BOOT_DRIVER(serial_msm_geni) = {
 	.name = "serial_msm_geni",
-- 
2.40.0

[PATCH 7/7] arm: Migrate Apple M1 to save_prev_bl_data

[Konrad Dybcio]

Mark's and Dzmitry's approaches come down to the same thing.. Let's
unify them by first removing the static keyword from the common file
to allow the variable to be reused, then renaming "reg0" to the more
sensible fw_dtb_pointer coming from the Apple file and finally remove
the mach-apple implementation of this very thing and enable the common
approach in the respective defconfig.

Only build-tested.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---

 arch/arm/lib/save_prev_bl_data.c    | 14 +++++++-------
 arch/arm/mach-apple/Makefile        |  1 -
 arch/arm/mach-apple/lowlevel_init.S | 17 -----------------
 configs/apple_m1_defconfig          |  1 +
 4 files changed, 8 insertions(+), 25 deletions(-)
 delete mode 100644 arch/arm/mach-apple/lowlevel_init.S

diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c
index f7b23faf0d66..a127fec1f149 100644
--- a/arch/arm/lib/save_prev_bl_data.c
+++ b/arch/arm/lib/save_prev_bl_data.c
@@ -15,7 +15,7 @@
 #include <asm/system.h>
 #include <asm/armv8/mmu.h>
 
-static ulong reg0 __section(".data");
+ulong fw_dtb_pointer __section(".data");
 
 /**
  * Save x0 register value, assuming previous bootloader set it to
@@ -23,7 +23,7 @@ static ulong reg0 __section(".data");
  */
 void save_boot_params(ulong r0)
 {
-	reg0 = r0;
+	fw_dtb_pointer = r0;
 	save_boot_params_ret();
 }
 
@@ -51,24 +51,24 @@ int save_prev_bl_data(void)
 	int node;
 	u64 initrd_start_prop;
 
-	if (!is_addr_accessible((phys_addr_t)reg0))
+	if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer))
 		return -ENODATA;
 
-	fdt_blob = (struct fdt_header *)reg0;
+	fdt_blob = (struct fdt_header *)fw_dtb_pointer;
 	if (!fdt_valid(&fdt_blob)) {
-		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
+		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);
 		return -ENODATA;
 	}
 
 	if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
-		env_set_addr("prevbl_fdt_addr", (void *)reg0);
+		env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);
 	if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR))
 		return 0;
 
 	node = fdt_path_offset(fdt_blob, "/chosen");
 	if (!node) {
 		pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
-					__func__, reg0);
+					__func__, fw_dtb_pointer);
 		return -ENODATA;
 	}
 	/*
diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile
index 50b465b9473f..a775d8866ad4 100644
--- a/arch/arm/mach-apple/Makefile
+++ b/arch/arm/mach-apple/Makefile
@@ -1,6 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0+
 
 obj-y += board.o
-obj-y += lowlevel_init.o
 obj-y += rtkit.o
 obj-$(CONFIG_NVME_APPLE) += sart.o
diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S
deleted file mode 100644
index e1c0d91cef2c..000000000000
--- a/arch/arm/mach-apple/lowlevel_init.S
+++ /dev/null
@@ -1,17 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
-/*
- * (C) Copyright 2021 Mark Kettenis <kettenis@openbsd.org>
- */
-
-.align 8
-.global fw_dtb_pointer
-fw_dtb_pointer:
-	.quad	0
-
-.global save_boot_params
-save_boot_params:
-	/* Stash DTB pointer passed by m1n1 */
-	adr	x1, fw_dtb_pointer
-	str	x0, [x1]
-
-	b	save_boot_params_ret
diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig
index b4ecf73cbc78..eb0addb741c5 100644
--- a/configs/apple_m1_defconfig
+++ b/configs/apple_m1_defconfig
@@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y
 CONFIG_DEFAULT_DEVICE_TREE="t8103-j274"
 CONFIG_SYS_LOAD_ADDR=0x0
 CONFIG_USE_PREBOOT=y
+CONFIG_SAVE_PREV_BL_FDT_ADDR=y
 # CONFIG_DISPLAY_CPUINFO is not set
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_BOARD_LATE_INIT=y
-- 
2.40.0

Re: [PATCH 1/7] mmc: msm_sdhci: Match clocks through "clocks" property

[Sumit Garg]

Hi,

On Fri, 24 Mar 2023 at 07:26, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> "clocks" is the standard property used in Linux, "clock" seems to be
> an U-Boot invention. Use the one that's more standardized.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  drivers/mmc/msm_sdhci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
> index 604f9c3ff99c..174435f01f68 100644
> --- a/drivers/mmc/msm_sdhci.c
> +++ b/drivers/mmc/msm_sdhci.c
> @@ -63,7 +63,7 @@ static int msm_sdc_clk_init(struct udevice *dev)
>         struct clk clk;
>         int ret;
>
> -       ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2);
> +       ret = fdtdec_get_int_array(gd->fdt_blob, node, "clocks", clkd, 2);

You have to update corresponding dts files as well to avoid breaking platforms.

-Sumit

>         if (ret)
>                 return ret;
>
> --
> 2.40.0
>

Re: [PATCH 2/7] serial: msm: Match clocks through "clocks" property

[Sumit Garg]

On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> "clocks" is the standard property used in Linux, "clock" seems to be
> an U-Boot invention. Use the one that's more standardized.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  arch/arm/dts/qcom-ipq4019.dtsi | 2 +-
>  drivers/serial/serial_msm.c    | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi
> index 6edc69da6747..dee3159e5893 100644
> --- a/arch/arm/dts/qcom-ipq4019.dtsi
> +++ b/arch/arm/dts/qcom-ipq4019.dtsi
> @@ -87,7 +87,7 @@
>                 blsp1_uart1: serial@78af000 {
>                         compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>                         reg = <0x78af000 0x200>;
> -                       clock = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
> +                       clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>;

You have to update other platform dts files as well to avoid breaking them.

-Sumit

>                         bit-rate = <0xFF>;
>                         status = "disabled";
>                         u-boot,dm-pre-reloc;
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index a22623c316ed..9c370cac323f 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -166,8 +166,7 @@ static int msm_uart_clk_init(struct udevice *dev)
>         struct clk clk;
>         int ret;
>
> -       ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clock",
> -                                  clkd, 2);
> +       ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clocks", clkd, 2);
>         if (ret)
>                 return ret;
>
> --
> 2.40.0
>

Re: [PATCH 3/7] serial: msm: Always bind before relocation

[Sumit Garg]

On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> In preparation for supporting upstream Linux device trees on Qualcomm
> platforms, make this the default behavior.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  drivers/serial/serial_msm.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
> index 9c370cac323f..59a2cf27aaf1 100644
> --- a/drivers/serial/serial_msm.c
> +++ b/drivers/serial/serial_msm.c
> @@ -251,4 +251,5 @@ U_BOOT_DRIVER(serial_msm) = {
>         .priv_auto      = sizeof(struct msm_serial_data),
>         .probe = msm_serial_probe,
>         .ops    = &msm_serial_ops,
> +       .flags          = DM_FLAG_PRE_RELOC,

"u-boot,dm-pre-reloc" serves the same purpose but this looks even
better as we would like the serial driver to be enabled by default
prior to relocation. So you need to get rid of redundant
"u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.

-Sumit

>  };
> --
> 2.40.0
>

Re: [PATCH 4/7] arch: snapdragon: clock: Always bind before relocation

[Sumit Garg]

On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> In preparation for supporting upstream Linux device trees on Qualcomm
> platforms, make this the default behavior.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  arch/arm/mach-snapdragon/clock-snapdragon.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c
> index 0ac45dce9a92..d1af5d1fec7d 100644
> --- a/arch/arm/mach-snapdragon/clock-snapdragon.c
> +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c
> @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = {
>         .ops            = &msm_clk_ops,
>         .priv_auto      = sizeof(struct msm_clk_priv),
>         .probe          = msm_clk_probe,
> +       .flags          = DM_FLAG_PRE_RELOC,

"u-boot,dm-pre-reloc" serves the same purpose but this looks even
better as we would like the serial driver to be enabled by default
prior to relocation. So you need to get rid of redundant
"u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.

-Sumit

>  };
> --
> 2.40.0
>

Re: [PATCH 5/7] arm: snapdragon: pinctrl: Always bind before relocation

[Sumit Garg]

On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> In preparation for supporting upstream Linux device trees on Qualcomm
> platforms, make this the default behavior.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  arch/arm/mach-snapdragon/pinctrl-snapdragon.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c
> index 826dc5148661..9f261d70e4d3 100644
> --- a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c
> +++ b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c
> @@ -163,4 +163,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = {
>         .ops            = &msm_pinctrl_ops,
>         .probe          = msm_pinctrl_probe,
>         .bind           = msm_pinctrl_bind,
> +       .flags          = DM_FLAG_PRE_RELOC,

"u-boot,dm-pre-reloc" serves the same purpose but this looks even
better as we would like the serial driver to be enabled by default
prior to relocation. So you need to get rid of redundant
"u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.

-Sumit

>  };
> --
> 2.40.0
>

Re: [PATCH 6/7] serial: msm_geni: Use upstream Linux bindings

[Sumit Garg]

On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> The name "se" is used in upstream Linux device trees and has been for
> ages, long before this U-Boot-ism was introduced. Same goes for the
> existing compatible. Get rid of that.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  arch/arm/dts/sdm845.dtsi         | 4 ++--
>  drivers/serial/serial_msm_geni.c | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi
> index 607af277f8be..92bdc82177d6 100644
> --- a/arch/arm/dts/sdm845.dtsi
> +++ b/arch/arm/dts/sdm845.dtsi
> @@ -52,10 +52,10 @@
>                 };
>
>                 debug_uart: serial@a84000 {
> -                       compatible = "qcom,msm-geni-uart";
> +                       compatible = "qcom,geni-debug-uart";

With this the custom u-boot DT bindings [1] becomes redundant, we
should remove that as well.

[1] doc/device-tree-bindings/serial/msm-geni-serial.txt

-Sumit

>                         reg = <0xa84000 0x4000>;
>                         reg-names = "se_phys";
> -                       clock-names = "se-clk";
> +                       clock-names = "se";
>                         clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>;
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&qup_uart9>;
> diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c
> index 3943ca43e49e..6c9371c4e69d 100644
> --- a/drivers/serial/serial_msm_geni.c
> +++ b/drivers/serial/serial_msm_geni.c
> @@ -189,7 +189,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate)
>         struct clk *clk;
>         int ret;
>
> -       clk = devm_clk_get(dev, "se-clk");
> +       clk = devm_clk_get(dev, "se");
>         if (!clk)
>                 return -EINVAL;
>
> @@ -554,7 +554,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev)
>  }
>
>  static const struct udevice_id msm_serial_ids[] = {
> -       {.compatible = "qcom,msm-geni-uart"}, {}};
> +       { .compatible = "qcom,geni-debug-uart" },
> +       { }
> +};
>
>  U_BOOT_DRIVER(serial_msm_geni) = {
>         .name = "serial_msm_geni",
> --
> 2.40.0
>

Re: [PATCH 1/7] mmc: msm_sdhci: Match clocks through "clocks" property

[Bhupesh Sharma]

Hi Konrad,

On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> "clocks" is the standard property used in Linux, "clock" seems to be
> an U-Boot invention. Use the one that's more standardized.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>
>  drivers/mmc/msm_sdhci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/msm_sdhci.c b/drivers/mmc/msm_sdhci.c
> index 604f9c3ff99c..174435f01f68 100644
> --- a/drivers/mmc/msm_sdhci.c
> +++ b/drivers/mmc/msm_sdhci.c
> @@ -63,7 +63,7 @@ static int msm_sdc_clk_init(struct udevice *dev)
>         struct clk clk;
>         int ret;
>
> -       ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2);
> +       ret = fdtdec_get_int_array(gd->fdt_blob, node, "clocks", clkd, 2);
>         if (ret)
>                 return ret;

This would break existing msm platforms like qcs404 and rb3. So,
please send the changed dts for those platforms with the driver
changes. We would also need to test the changes on those platforms
first.

Same is valid for other driver patches which alter the dts properties
without changing the actual dtsi/dts first.

Thanks,
Bhupesh

Re: [PATCH 2/7] serial: msm: Match clocks through "clocks" property

[Konrad Dybcio]

On 24.03.2023 07:00, Sumit Garg wrote:
> On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> "clocks" is the standard property used in Linux, "clock" seems to be
>> an U-Boot invention. Use the one that's more standardized.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>
>>  arch/arm/dts/qcom-ipq4019.dtsi | 2 +-
>>  drivers/serial/serial_msm.c    | 3 +--
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi
>> index 6edc69da6747..dee3159e5893 100644
>> --- a/arch/arm/dts/qcom-ipq4019.dtsi
>> +++ b/arch/arm/dts/qcom-ipq4019.dtsi
>> @@ -87,7 +87,7 @@
>>                 blsp1_uart1: serial@78af000 {
>>                         compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>>                         reg = <0x78af000 0x200>;
>> -                       clock = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
>> +                       clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>;
> 
> You have to update other platform dts files as well to avoid breaking them.
Right, my grep pattern didn't include dragonboard* (which I should probably
update to include the SoC name too, anyway..).

Konrad
> 
> -Sumit
> 
>>                         bit-rate = <0xFF>;
>>                         status = "disabled";
>>                         u-boot,dm-pre-reloc;
>> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
>> index a22623c316ed..9c370cac323f 100644
>> --- a/drivers/serial/serial_msm.c
>> +++ b/drivers/serial/serial_msm.c
>> @@ -166,8 +166,7 @@ static int msm_uart_clk_init(struct udevice *dev)
>>         struct clk clk;
>>         int ret;
>>
>> -       ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clock",
>> -                                  clkd, 2);
>> +       ret = fdtdec_get_int_array(gd->fdt_blob, dev_of_offset(dev), "clocks", clkd, 2);
>>         if (ret)
>>                 return ret;
>>
>> --
>> 2.40.0
>>

Re: [PATCH 3/7] serial: msm: Always bind before relocation

[Konrad Dybcio]

On 24.03.2023 07:04, Sumit Garg wrote:
> On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> In preparation for supporting upstream Linux device trees on Qualcomm
>> platforms, make this the default behavior.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>
>>  drivers/serial/serial_msm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c
>> index 9c370cac323f..59a2cf27aaf1 100644
>> --- a/drivers/serial/serial_msm.c
>> +++ b/drivers/serial/serial_msm.c
>> @@ -251,4 +251,5 @@ U_BOOT_DRIVER(serial_msm) = {
>>         .priv_auto      = sizeof(struct msm_serial_data),
>>         .probe = msm_serial_probe,
>>         .ops    = &msm_serial_ops,
>> +       .flags          = DM_FLAG_PRE_RELOC,
> 
> "u-boot,dm-pre-reloc" serves the same purpose but this looks even
> better as we would like the serial driver to be enabled by default
> prior to relocation.
Yep, this way you can take a dtb you built with Linux and use u-boot
with that!

So you need to get rid of redundant
> "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
Will do!

Konrad
> 
> -Sumit
> 
>>  };
>> --
>> 2.40.0
>>

Re: [PATCH 6/7] serial: msm_geni: Use upstream Linux bindings

[Konrad Dybcio]

On 24.03.2023 07:13, Sumit Garg wrote:
> On Fri, 24 Mar 2023 at 06:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> The name "se" is used in upstream Linux device trees and has been for
>> ages, long before this U-Boot-ism was introduced. Same goes for the
>> existing compatible. Get rid of that.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>
>>  arch/arm/dts/sdm845.dtsi         | 4 ++--
>>  drivers/serial/serial_msm_geni.c | 6 ++++--
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi
>> index 607af277f8be..92bdc82177d6 100644
>> --- a/arch/arm/dts/sdm845.dtsi
>> +++ b/arch/arm/dts/sdm845.dtsi
>> @@ -52,10 +52,10 @@
>>                 };
>>
>>                 debug_uart: serial@a84000 {
>> -                       compatible = "qcom,msm-geni-uart";
>> +                       compatible = "qcom,geni-debug-uart";
> 
> With this the custom u-boot DT bindings [1] becomes redundant, we
> should remove that as well.
> 
> [1] doc/device-tree-bindings/serial/msm-geni-serial.txt
Ack!

Konrad
> 
> -Sumit
> 
>>                         reg = <0xa84000 0x4000>;
>>                         reg-names = "se_phys";
>> -                       clock-names = "se-clk";
>> +                       clock-names = "se";
>>                         clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>;
>>                         pinctrl-names = "default";
>>                         pinctrl-0 = <&qup_uart9>;
>> diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c
>> index 3943ca43e49e..6c9371c4e69d 100644
>> --- a/drivers/serial/serial_msm_geni.c
>> +++ b/drivers/serial/serial_msm_geni.c
>> @@ -189,7 +189,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate)
>>         struct clk *clk;
>>         int ret;
>>
>> -       clk = devm_clk_get(dev, "se-clk");
>> +       clk = devm_clk_get(dev, "se");
>>         if (!clk)
>>                 return -EINVAL;
>>
>> @@ -554,7 +554,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev)
>>  }
>>
>>  static const struct udevice_id msm_serial_ids[] = {
>> -       {.compatible = "qcom,msm-geni-uart"}, {}};
>> +       { .compatible = "qcom,geni-debug-uart" },
>> +       { }
>> +};
>>
>>  U_BOOT_DRIVER(serial_msm_geni) = {
>>         .name = "serial_msm_geni",
>> --
>> 2.40.0
>>

Re: [PATCH 4/7] arch: snapdragon: clock: Always bind before relocation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Fri, Mar 24, 2023 at 11:36:24AM +0530, Sumit Garg wrote:
> On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >
> > In preparation for supporting upstream Linux device trees on Qualcomm
> > platforms, make this the default behavior.
> >
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > ---
> >
> >  arch/arm/mach-snapdragon/clock-snapdragon.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c
> > index 0ac45dce9a92..d1af5d1fec7d 100644
> > --- a/arch/arm/mach-snapdragon/clock-snapdragon.c
> > +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c
> > @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = {
> >         .ops            = &msm_clk_ops,
> >         .priv_auto      = sizeof(struct msm_clk_priv),
> >         .probe          = msm_clk_probe,
> > +       .flags          = DM_FLAG_PRE_RELOC,
> 
> "u-boot,dm-pre-reloc" serves the same purpose but this looks even
> better as we would like the serial driver to be enabled by default
> prior to relocation. So you need to get rid of redundant
> "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.

And in turn u-boot,dm-* are now bootph-* flags which can (should!) be
part of the upstream dts file.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 4/7] arch: snapdragon: clock: Always bind before relocation

[Sumit Garg]

On Fri, 24 Mar 2023 at 20:54, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 24, 2023 at 11:36:24AM +0530, Sumit Garg wrote:
> > On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > >
> > > In preparation for supporting upstream Linux device trees on Qualcomm
> > > platforms, make this the default behavior.
> > >
> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > ---
> > >
> > >  arch/arm/mach-snapdragon/clock-snapdragon.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c
> > > index 0ac45dce9a92..d1af5d1fec7d 100644
> > > --- a/arch/arm/mach-snapdragon/clock-snapdragon.c
> > > +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c
> > > @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = {
> > >         .ops            = &msm_clk_ops,
> > >         .priv_auto      = sizeof(struct msm_clk_priv),
> > >         .probe          = msm_clk_probe,
> > > +       .flags          = DM_FLAG_PRE_RELOC,
> >
> > "u-boot,dm-pre-reloc" serves the same purpose but this looks even
> > better as we would like the serial driver to be enabled by default
> > prior to relocation. So you need to get rid of redundant
> > "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
>
> And in turn u-boot,dm-* are now bootph-* flags which can (should!) be
> part of the upstream dts file.
>

That's good to know. Can you share a reference to the patch set adding
support for bootph-* flags?

-Sumit

> --
> Tom

Re: [PATCH 4/7] arch: snapdragon: clock: Always bind before relocation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On Mon, Mar 27, 2023 at 11:07:06AM +0530, Sumit Garg wrote:
> On Fri, 24 Mar 2023 at 20:54, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Mar 24, 2023 at 11:36:24AM +0530, Sumit Garg wrote:
> > > On Fri, 24 Mar 2023 at 07:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > >
> > > > In preparation for supporting upstream Linux device trees on Qualcomm
> > > > platforms, make this the default behavior.
> > > >
> > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > > ---
> > > >
> > > >  arch/arm/mach-snapdragon/clock-snapdragon.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/arm/mach-snapdragon/clock-snapdragon.c b/arch/arm/mach-snapdragon/clock-snapdragon.c
> > > > index 0ac45dce9a92..d1af5d1fec7d 100644
> > > > --- a/arch/arm/mach-snapdragon/clock-snapdragon.c
> > > > +++ b/arch/arm/mach-snapdragon/clock-snapdragon.c
> > > > @@ -178,4 +178,5 @@ U_BOOT_DRIVER(clk_msm) = {
> > > >         .ops            = &msm_clk_ops,
> > > >         .priv_auto      = sizeof(struct msm_clk_priv),
> > > >         .probe          = msm_clk_probe,
> > > > +       .flags          = DM_FLAG_PRE_RELOC,
> > >
> > > "u-boot,dm-pre-reloc" serves the same purpose but this looks even
> > > better as we would like the serial driver to be enabled by default
> > > prior to relocation. So you need to get rid of redundant
> > > "u-boot,dm-pre-reloc" from corresponding <platform>-uboot.dtsi files.
> >
> > And in turn u-boot,dm-* are now bootph-* flags which can (should!) be
> > part of the upstream dts file.
> >
> 
> That's good to know. Can you share a reference to the patch set adding
> support for bootph-* flags?

https://github.com/devicetree-org/dt-schema/commit/63bd8472dfc6951eae1c95b27ac8cbc88f4cb3b0

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 7/7] arm: Migrate Apple M1 to save_prev_bl_data

[Mark Kettenis]

> From: Konrad Dybcio <konrad.dybcio@linaro.org>
> Date: Fri, 24 Mar 2023 01:40:40 +0100

Hi Konrad,

> Mark's and Dzmitry's approaches come down to the same thing.. Let's
> unify them by first removing the static keyword from the common file
> to allow the variable to be reused, then renaming "reg0" to the more
> sensible fw_dtb_pointer coming from the Apple file and finally remove
> the mach-apple implementation of this very thing and enable the common
> approach in the respective defconfig.
> 
> Only build-tested.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> 
>  arch/arm/lib/save_prev_bl_data.c    | 14 +++++++-------
>  arch/arm/mach-apple/Makefile        |  1 -
>  arch/arm/mach-apple/lowlevel_init.S | 17 -----------------
>  configs/apple_m1_defconfig          |  1 +
>  4 files changed, 8 insertions(+), 25 deletions(-)
>  delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
> 
> diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c
> index f7b23faf0d66..a127fec1f149 100644
> --- a/arch/arm/lib/save_prev_bl_data.c
> +++ b/arch/arm/lib/save_prev_bl_data.c
> @@ -15,7 +15,7 @@
>  #include <asm/system.h>
>  #include <asm/armv8/mmu.h>
>  
> -static ulong reg0 __section(".data");
> +ulong fw_dtb_pointer __section(".data");
>  
>  /**
>   * Save x0 register value, assuming previous bootloader set it to
> @@ -23,7 +23,7 @@ static ulong reg0 __section(".data");
>   */
>  void save_boot_params(ulong r0)
>  {
> -	reg0 = r0;
> +	fw_dtb_pointer = r0;
>  	save_boot_params_ret();

I suppose this works, but it is a bit strange sice
save_boot_params_ret is just a label that we're supposed to jump back
to, not a function.

>  }
>  
> @@ -51,24 +51,24 @@ int save_prev_bl_data(void)
>  	int node;
>  	u64 initrd_start_prop;
>  
> -	if (!is_addr_accessible((phys_addr_t)reg0))
> +	if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer))
>  		return -ENODATA;
>  
> -	fdt_blob = (struct fdt_header *)reg0;
> +	fdt_blob = (struct fdt_header *)fw_dtb_pointer;
>  	if (!fdt_valid(&fdt_blob)) {
> -		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
> +		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);
>  		return -ENODATA;
>  	}
>  
>  	if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
> -		env_set_addr("prevbl_fdt_addr", (void *)reg0);
> +		env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);
>  	if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR))
>  		return 0;
>  
>  	node = fdt_path_offset(fdt_blob, "/chosen");
>  	if (!node) {
>  		pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
> -					__func__, reg0);
> +					__func__, fw_dtb_pointer);
>  		return -ENODATA;
>  	}
>  	/*

And we have no use for these additional environment variables and I'd
prefer not to add them.

> diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile
> index 50b465b9473f..a775d8866ad4 100644
> --- a/arch/arm/mach-apple/Makefile
> +++ b/arch/arm/mach-apple/Makefile
> @@ -1,6 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  
>  obj-y += board.o
> -obj-y += lowlevel_init.o
>  obj-y += rtkit.o
>  obj-$(CONFIG_NVME_APPLE) += sart.o
> diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S
> deleted file mode 100644
> index e1c0d91cef2c..000000000000
> --- a/arch/arm/mach-apple/lowlevel_init.S
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0+ */
> -/*
> - * (C) Copyright 2021 Mark Kettenis <kettenis@openbsd.org>
> - */
> -
> -.align 8
> -.global fw_dtb_pointer
> -fw_dtb_pointer:
> -	.quad	0
> -
> -.global save_boot_params
> -save_boot_params:
> -	/* Stash DTB pointer passed by m1n1 */
> -	adr	x1, fw_dtb_pointer
> -	str	x0, [x1]
> -
> -	b	save_boot_params_ret
> diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig
> index b4ecf73cbc78..eb0addb741c5 100644
> --- a/configs/apple_m1_defconfig
> +++ b/configs/apple_m1_defconfig
> @@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y
>  CONFIG_DEFAULT_DEVICE_TREE="t8103-j274"
>  CONFIG_SYS_LOAD_ADDR=0x0
>  CONFIG_USE_PREBOOT=y
> +CONFIG_SAVE_PREV_BL_FDT_ADDR=y
>  # CONFIG_DISPLAY_CPUINFO is not set
>  CONFIG_DISPLAY_BOARDINFO_LATE=y
>  CONFIG_BOARD_LATE_INIT=y
> -- 
> 2.40.0
> 
> 

Re: [PATCH 7/7] arm: Migrate Apple M1 to save_prev_bl_data

[Konrad Dybcio]

On 27.03.2023 21:31, Mark Kettenis wrote:
>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Date: Fri, 24 Mar 2023 01:40:40 +0100
> 
> Hi Konrad,
> 
>> Mark's and Dzmitry's approaches come down to the same thing.. Let's
>> unify them by first removing the static keyword from the common file
>> to allow the variable to be reused, then renaming "reg0" to the more
>> sensible fw_dtb_pointer coming from the Apple file and finally remove
>> the mach-apple implementation of this very thing and enable the common
>> approach in the respective defconfig.
>>
>> Only build-tested.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>
>>  arch/arm/lib/save_prev_bl_data.c    | 14 +++++++-------
>>  arch/arm/mach-apple/Makefile        |  1 -
>>  arch/arm/mach-apple/lowlevel_init.S | 17 -----------------
>>  configs/apple_m1_defconfig          |  1 +
>>  4 files changed, 8 insertions(+), 25 deletions(-)
>>  delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
>>
>> diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c
>> index f7b23faf0d66..a127fec1f149 100644
>> --- a/arch/arm/lib/save_prev_bl_data.c
>> +++ b/arch/arm/lib/save_prev_bl_data.c
>> @@ -15,7 +15,7 @@
>>  #include <asm/system.h>
>>  #include <asm/armv8/mmu.h>
>>  
>> -static ulong reg0 __section(".data");
>> +ulong fw_dtb_pointer __section(".data");
>>  
>>  /**
>>   * Save x0 register value, assuming previous bootloader set it to
>> @@ -23,7 +23,7 @@ static ulong reg0 __section(".data");
>>   */
>>  void save_boot_params(ulong r0)
>>  {
>> -	reg0 = r0;
>> +	fw_dtb_pointer = r0;
>>  	save_boot_params_ret();
> 
> I suppose this works, but it is a bit strange sice
> save_boot_params_ret is just a label that we're supposed to jump back
> to, not a function.
> 
>>  }
>>  
>> @@ -51,24 +51,24 @@ int save_prev_bl_data(void)
>>  	int node;
>>  	u64 initrd_start_prop;
>>  
>> -	if (!is_addr_accessible((phys_addr_t)reg0))
>> +	if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer))
>>  		return -ENODATA;
>>  
>> -	fdt_blob = (struct fdt_header *)reg0;
>> +	fdt_blob = (struct fdt_header *)fw_dtb_pointer;
>>  	if (!fdt_valid(&fdt_blob)) {
>> -		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
>> +		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);
>>  		return -ENODATA;
>>  	}
>>  
>>  	if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
>> -		env_set_addr("prevbl_fdt_addr", (void *)reg0);
>> +		env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);
>>  	if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR))
>>  		return 0;
>>  
>>  	node = fdt_path_offset(fdt_blob, "/chosen");
>>  	if (!node) {
>>  		pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
>> -					__func__, reg0);
>> +					__func__, fw_dtb_pointer);
>>  		return -ENODATA;
>>  	}
>>  	/*
> 
> And we have no use for these additional environment variables and I'd
> prefer not to add them.
Okay, let's forget about it then.

Would you (or anybody responsible, I don't know who picks things up)
be willing to take this patch without the Apple part then, a.k.a.
renaming r0 to fw_dtb_pointer?

Konrad
> 
>> diff --git a/arch/arm/mach-apple/Makefile b/arch/arm/mach-apple/Makefile
>> index 50b465b9473f..a775d8866ad4 100644
>> --- a/arch/arm/mach-apple/Makefile
>> +++ b/arch/arm/mach-apple/Makefile
>> @@ -1,6 +1,5 @@
>>  # SPDX-License-Identifier: GPL-2.0+
>>  
>>  obj-y += board.o
>> -obj-y += lowlevel_init.o
>>  obj-y += rtkit.o
>>  obj-$(CONFIG_NVME_APPLE) += sart.o
>> diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S
>> deleted file mode 100644
>> index e1c0d91cef2c..000000000000
>> --- a/arch/arm/mach-apple/lowlevel_init.S
>> +++ /dev/null
>> @@ -1,17 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0+ */
>> -/*
>> - * (C) Copyright 2021 Mark Kettenis <kettenis@openbsd.org>
>> - */
>> -
>> -.align 8
>> -.global fw_dtb_pointer
>> -fw_dtb_pointer:
>> -	.quad	0
>> -
>> -.global save_boot_params
>> -save_boot_params:
>> -	/* Stash DTB pointer passed by m1n1 */
>> -	adr	x1, fw_dtb_pointer
>> -	str	x0, [x1]
>> -
>> -	b	save_boot_params_ret
>> diff --git a/configs/apple_m1_defconfig b/configs/apple_m1_defconfig
>> index b4ecf73cbc78..eb0addb741c5 100644
>> --- a/configs/apple_m1_defconfig
>> +++ b/configs/apple_m1_defconfig
>> @@ -3,6 +3,7 @@ CONFIG_ARCH_APPLE=y
>>  CONFIG_DEFAULT_DEVICE_TREE="t8103-j274"
>>  CONFIG_SYS_LOAD_ADDR=0x0
>>  CONFIG_USE_PREBOOT=y
>> +CONFIG_SAVE_PREV_BL_FDT_ADDR=y
>>  # CONFIG_DISPLAY_CPUINFO is not set
>>  CONFIG_DISPLAY_BOARDINFO_LATE=y
>>  CONFIG_BOARD_LATE_INIT=y
>> -- 
>> 2.40.0
>>
>>

Re: [PATCH 7/7] arm: Migrate Apple M1 to save_prev_bl_data

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4173 bytes --]

On Tue, Mar 28, 2023 at 10:17:44AM +0200, Konrad Dybcio wrote:
> 
> 
> On 27.03.2023 21:31, Mark Kettenis wrote:
> >> From: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> Date: Fri, 24 Mar 2023 01:40:40 +0100
> > 
> > Hi Konrad,
> > 
> >> Mark's and Dzmitry's approaches come down to the same thing.. Let's
> >> unify them by first removing the static keyword from the common file
> >> to allow the variable to be reused, then renaming "reg0" to the more
> >> sensible fw_dtb_pointer coming from the Apple file and finally remove
> >> the mach-apple implementation of this very thing and enable the common
> >> approach in the respective defconfig.
> >>
> >> Only build-tested.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >>
> >>  arch/arm/lib/save_prev_bl_data.c    | 14 +++++++-------
> >>  arch/arm/mach-apple/Makefile        |  1 -
> >>  arch/arm/mach-apple/lowlevel_init.S | 17 -----------------
> >>  configs/apple_m1_defconfig          |  1 +
> >>  4 files changed, 8 insertions(+), 25 deletions(-)
> >>  delete mode 100644 arch/arm/mach-apple/lowlevel_init.S
> >>
> >> diff --git a/arch/arm/lib/save_prev_bl_data.c b/arch/arm/lib/save_prev_bl_data.c
> >> index f7b23faf0d66..a127fec1f149 100644
> >> --- a/arch/arm/lib/save_prev_bl_data.c
> >> +++ b/arch/arm/lib/save_prev_bl_data.c
> >> @@ -15,7 +15,7 @@
> >>  #include <asm/system.h>
> >>  #include <asm/armv8/mmu.h>
> >>  
> >> -static ulong reg0 __section(".data");
> >> +ulong fw_dtb_pointer __section(".data");
> >>  
> >>  /**
> >>   * Save x0 register value, assuming previous bootloader set it to
> >> @@ -23,7 +23,7 @@ static ulong reg0 __section(".data");
> >>   */
> >>  void save_boot_params(ulong r0)
> >>  {
> >> -	reg0 = r0;
> >> +	fw_dtb_pointer = r0;
> >>  	save_boot_params_ret();
> > 
> > I suppose this works, but it is a bit strange sice
> > save_boot_params_ret is just a label that we're supposed to jump back
> > to, not a function.

I think maybe a problem is save_boot_params_ret is poorly defined, or
too specifically defined as a point in the early boot code.

We're in a funny spot here since arch/arm/lib/save_prev_bl_data.c isn't
exactly generic (it might be previous BL is little kernel specific? not
100% sure), as well. It would be good, now that everything is in
Kconfig, to tighten the dependencies (this code is arm64 only, and
initramfs without fdt option doesn't make sense, so you can cleanup the
Makefile and caller logic) and preface is_addr_accessible with static as
it's only called there.

> >>  }
> >>  
> >> @@ -51,24 +51,24 @@ int save_prev_bl_data(void)
> >>  	int node;
> >>  	u64 initrd_start_prop;
> >>  
> >> -	if (!is_addr_accessible((phys_addr_t)reg0))
> >> +	if (!is_addr_accessible((phys_addr_t)fw_dtb_pointer))
> >>  		return -ENODATA;
> >>  
> >> -	fdt_blob = (struct fdt_header *)reg0;
> >> +	fdt_blob = (struct fdt_header *)fw_dtb_pointer;
> >>  	if (!fdt_valid(&fdt_blob)) {
> >> -		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, reg0);
> >> +		pr_warn("%s: address 0x%lx is not a valid fdt\n", __func__, fw_dtb_pointer);
> >>  		return -ENODATA;
> >>  	}
> >>  
> >>  	if (IS_ENABLED(CONFIG_SAVE_PREV_BL_FDT_ADDR))
> >> -		env_set_addr("prevbl_fdt_addr", (void *)reg0);
> >> +		env_set_addr("prevbl_fdt_addr", (void *)fw_dtb_pointer);
> >>  	if (!IS_ENABLED(CONFIG_SAVE_PREV_BL_INITRAMFS_START_ADDR))
> >>  		return 0;
> >>  
> >>  	node = fdt_path_offset(fdt_blob, "/chosen");
> >>  	if (!node) {
> >>  		pr_warn("%s: chosen node not found in device tree at addr: 0x%lx\n",
> >> -					__func__, reg0);
> >> +					__func__, fw_dtb_pointer);
> >>  		return -ENODATA;
> >>  	}
> >>  	/*
> > 
> > And we have no use for these additional environment variables and I'd
> > prefer not to add them.
> Okay, let's forget about it then.
> 
> Would you (or anybody responsible, I don't know who picks things up)
> be willing to take this patch without the Apple part then, a.k.a.
> renaming r0 to fw_dtb_pointer?

Well, in the next iteration of this series just drop the apple m1
related changes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]