Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode
On 3/20/24 12: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 --- 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; Applied. M
Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode
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
>>> ---
>>>
>>> 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
Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode
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
---
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;
Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode
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
> ---
>
> 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 {
...
}
?
--Sean
[PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode
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 --- 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; -- 2.36.1
