Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode

[Michal Simek]

On 3/21/24 17:20, Sean Anderson wrote:
On 3/20/24 07:18, Michal Simek wrote:
if (reg >> BOOT_MODE_ALT_SHIFT) condition rules out alternative jtag boot
mode which is 0. When 0 was used origin(HW) boot mode was used instead.
That's why directly fill reg variable with requested boot mode and don't
let code to read value back. "else" part of code remain unchanged.

Signed-off-by: Michal Simek <michal.si...@amd.com>
---

  arch/arm/mach-zynqmp/spl.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
index 5af735aa5cef..979ff3aef6c2 100644
--- a/arch/arm/mach-zynqmp/spl.c
+++ b/arch/arm/mach-zynqmp/spl.c
@@ -91,13 +91,14 @@ u32 spl_boot_device(void)
  
  #if defined(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)
        /* Change default boot mode at run-time */
+       reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
        writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
               &crlapb_base->boot_mode);
-#endif
-
+#else
        reg = readl(&crlapb_base->boot_mode);
        if (reg >> BOOT_MODE_ALT_SHIFT)
                reg >>= BOOT_MODE_ALT_SHIFT;
+#endif
  
  	bootmode = reg & BOOT_MODES_MASK;
  

Looks fine; can we change this to

if (IS_ENABLED(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)) {
        ...
} else {
        ...
}

Issue is that CONFIG_SPL_ZYNQMP_ALT_BOOTMODE symbol is not defined and depends 
on CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED. It means you get compilation error.
That symbol can be setup and then what you have above can work.
Is it worth? TBH I don't have preference. Please take a look at patch below.
(And if v1 is fine then at least there should be added depends on 
SPL_ZYNQMP_ALT_BOOTMODE_ENABLED to SPL_ZYNQMP_ALT_BOOTMODE which is missing 
there now).

Thanks,
Michal

diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig
index eee34380f0a0..75d3ec916a66 100644
--- a/arch/arm/mach-zynqmp/Kconfig
+++ b/arch/arm/mach-zynqmp/Kconfig
@@ -145,7 +145,7 @@ config ZYNQ_SDHCI_MAX_FREQ

 config SPL_ZYNQMP_ALT_BOOTMODE
        hex
-       default 0x0 if JTAG_MODE
+       default 0x0
        default 0x1 if QSPI_MODE_24BIT
        default 0x2 if QSPI_MODE_32BIT
        default 0x3 if SD_MODE
diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
index 979ff3aef6c2..bbbf684ae496 100644
--- a/arch/arm/mach-zynqmp/spl.c
+++ b/arch/arm/mach-zynqmp/spl.c
@@ -89,16 +89,16 @@ u32 spl_boot_device(void)
        u32 reg = 0;
        u8 bootmode;

-#if defined(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)
-       /* Change default boot mode at run-time */
-       reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
-       writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
-              &crlapb_base->boot_mode);
-#else
-       reg = readl(&crlapb_base->boot_mode);
-       if (reg >> BOOT_MODE_ALT_SHIFT)
-               reg >>= BOOT_MODE_ALT_SHIFT;
-#endif
+       if (IS_ENABLED(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)) {
+               /* Change default boot mode at run-time */
+               reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
+               writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
+                      &crlapb_base->boot_mode);
+       } else {
+               reg = readl(&crlapb_base->boot_mode);
+               if (reg >> BOOT_MODE_ALT_SHIFT)
+                       reg >>= BOOT_MODE_ALT_SHIFT;
+       }

        bootmode = reg & BOOT_MODES_MASK;