mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Michael Riesch <michael.riesch@wolfvision.net>
To: Michael Tretter <m.tretter@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 3/3] arm: zynqmp: add boot source support
Date: Mon, 13 Sep 2021 12:53:58 +0200	[thread overview]
Message-ID: <18d58ffc-c93b-e350-24a1-b4338a2a2ee9@wolfvision.net> (raw)
In-Reply-To: <20210910135933.GA24395@pengutronix.de>

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[] = {
>> +	{ .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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>>

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


  reply	other threads:[~2021-09-13 10:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-10 13:43 [PATCH 0/3] arm: zynqmp: add support for zcu106 and boot sources Michael Riesch
2021-09-10 13:43 ` [PATCH 1/3] arm: zynqmp: add support for xilinx zcu106 board Michael Riesch
2021-09-10 13:43 ` [PATCH 2/3] Documentation: boards: zynqmp: fix broken links Michael Riesch
2021-09-10 13:43 ` [PATCH 3/3] arm: zynqmp: add boot source support Michael Riesch
2021-09-10 13:59   ` Michael Tretter
2021-09-13 10:53     ` Michael Riesch [this message]
2021-09-13 11:09       ` Ahmad Fatoum
2021-09-13 12:03         ` Michael Riesch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=18d58ffc-c93b-e350-24a1-b4338a2a2ee9@wolfvision.net \
    --to=michael.riesch@wolfvision.net \
    --cc=barebox@lists.infradead.org \
    --cc=m.tretter@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox