[PATCH 3/3] arm: zynqmp: add boot source support

[Michael Riesch]

Hi Ahmad,

On 9/13/21 1:09 PM, Ahmad Fatoum wrote:
> On 13.09.21 12:53, Michael Riesch wrote:
>> 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 at wolfvision.net>
>>>> ---
>>>>  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[] = {
> 
> This could be marked const.

Thanks for your comments. In v2 I got rid of the table as there are some
redundancies...

> 
>>>> +	{ .src = BOOTSOURCE_JTAG, .instance = 0 },
>>>> +	{ .src = BOOTSOURCE_SPI, .instance = 0 },
>>>> +	{ .src = BOOTSOURCE_SPI, .instance = 0 },
> 
> What difference is between boot_modes[1] and boot_modes[2]?

... for example the two SPI modes, which differ by the SPI addressing
mode (24 vs. 32 bit). I reckon this is not relevant for barebox and the
two modes can be grouped in the switch statement.

>>>> +	{ .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 },
> 
> There's BOOTSOURCE_INSTANCE_UNKNOWN, which you may want to use here.

Thanks for pointing it out, I'll use it in the default case.

>>>> +	{ .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.
> 
> I like the array, you can do
> 
>   [0x0] = { .src = BOOTSOURCE_JTAG, .instance = 0 }
>   [0x1] = { .src = BOOTSOURCE_SPI, .instance = 0 }
> 
> A switch is too verbose IMO.

Since many cases can be grouped it does not seem too verbose to me, but
feel free of course to comment on the v2.

Best regards,
Michael

> 
>>
>>>> +
>>>> +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 at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/barebox
>>>>
>>
>> _______________________________________________
>> barebox mailing list
>> barebox at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>>
> 
>