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.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;
>
>
>
Reviewed-by: Sean Anderson <sean.ander...@linux.dev>
