Hi Michael,
Thanks for your comments.
On 9/10/21 3:59 PM, Michael Tretter wrote:
> On Fri, 10 Sep 2021 15:43:23 +0200, Michael Riesch wrote:
>> The ZynqMP reports the mode pins sampled at POR via the register
>> ZYNQMP_CRL_APB_BOOT_MODE_USER. This commit adds a function that reads
>> the register and populates the boot source.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx>
>> ---
>> arch/arm/mach-zynqmp/zynqmp.c | 43 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>>
>> diff --git a/arch/arm/mach-zynqmp/zynqmp.c b/arch/arm/mach-zynqmp/zynqmp.c
>> index 5871c145b..262bc0dd4 100644
>> --- a/arch/arm/mach-zynqmp/zynqmp.c
>> +++ b/arch/arm/mach-zynqmp/zynqmp.c
>> @@ -6,9 +6,11 @@
>> #include <common.h>
>> #include <init.h>
>> #include <linux/types.h>
>> +#include <bootsource.h>
>> #include <reset_source.h>
>>
>> #define ZYNQMP_CRL_APB_BASE 0xff5e0000
>> +#define ZYNQMP_CRL_APB_BOOT_MODE_USER (ZYNQMP_CRL_APB_BASE + 0x200)
>> #define ZYNQMP_CRL_APB_RESET_REASON (ZYNQMP_CRL_APB_BASE + 0x220)
>>
>> /* External POR: The PS_POR_B reset signal pin was asserted. */
>> @@ -26,6 +28,46 @@
>> /* Software debugger reset: Write to BLOCKONLY_RST [debug_only]. */
>> #define ZYNQMP_CRL_APB_RESET_REASON_DEBUG_SYS BIT(6)
>>
>> +struct zynqmp_bootsource {
>> + enum bootsource src;
>> + int instance;
>> +};
>> +
>> +/* cf. Table 11-1 "Boot Modes" in UG1085 Zynq UltraScale+ Device TRM */
>> +static struct zynqmp_bootsource boot_modes[] = {
>> + { .src = BOOTSOURCE_JTAG, .instance = 0 },
>> + { .src = BOOTSOURCE_SPI, .instance = 0 },
>> + { .src = BOOTSOURCE_SPI, .instance = 0 },
>> + { .src = BOOTSOURCE_MMC, .instance = 0 },
>> + { .src = BOOTSOURCE_SPI_NAND, .instance = 0 },
>> + { .src = BOOTSOURCE_MMC, .instance = 1 },
>> + { .src = BOOTSOURCE_MMC, .instance = 0 },
>> + { .src = BOOTSOURCE_USB, .instance = 0 },
>> + { .src = BOOTSOURCE_JTAG, .instance = 0 },
>> + { .src = BOOTSOURCE_JTAG, .instance = 0 },
>> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
>> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
>> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
>> + { .src = BOOTSOURCE_UNKNOWN, .instance = 0 },
>> + { .src = BOOTSOURCE_MMC, .instance = 1 },
>> +};
>
> Thanks for the patch.
>
> Please make the mapping of the Boot Mode value to the BOOTSOURCE explicit
> instead of hiding in as the index into this array.
OK. This approach is used in mach-rockchip/rk3568.c and I took it from
there. But if it serves readability I can rewrite it. I think I'll drop
the table altogether, though, and use a switch instead.
>> +
>> +static enum bootsource zynqmp_bootsource(void)
>> +{
>> + u32 v;
>> +
>> + v = readl(ZYNQMP_CRL_APB_BOOT_MODE_USER);
>> + v &= 0x0F;
>> +
>> + if (v >= ARRAY_SIZE(boot_modes))
>> + return BOOTSOURCE_UNKNOWN;
>> +
>> + bootsource_set(boot_modes[v].src);
>> + bootsource_set_instance(boot_modes[v].instance);
>
> Don't set the bootsource as a side effect of this function. This function
> should only lookup of the boot mode and zynqmp_init should actually set it.
Again, this is pretty much taken from rk3568.c and I didn't see any harm
in it. But the way you suggested is fine to me as well. I'll prepare a v2.
Best regards,
Michael
>
> Michael
>
>> +
>> + return boot_modes[v].src;
>> +}
>> +
>> struct zynqmp_reset_reason {
>> u32 mask;
>> enum reset_src_type type;
>> @@ -65,6 +107,7 @@ static enum reset_src_type zynqmp_get_reset_src(void)
>>
>> static int zynqmp_init(void)
>> {
>> + zynqmp_bootsource();
>> reset_source_set(zynqmp_get_reset_src());
>>
>> return 0;
>> --
>> 2.17.1
>>
>>
>> _______________________________________________
>> barebox mailing list
>> barebox@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/barebox
>>
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
