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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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@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.

>>> +	{ .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]?

>>> +	{ .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.

>>> +	{ .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.

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


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox