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

[Sean Anderson <sean.anderson@linux.dev>]

On 3/22/24 07:53, Michal Simek wrote:
> 
> 
> 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.simek@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;
> 
> 
> 

Reviewed-by: Sean Anderson <sean.anderson@linux.dev>