mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Michael Riesch <michael.riesch@wolfvision.net>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>,
	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 14:03:23 +0200	[thread overview]
Message-ID: <93665682-d3b1-03ce-b641-c4c6af526fae@wolfvision.net> (raw)
In-Reply-To: <3e891c60-2466-f7b5-55fd-6293dd25de31@pengutronix.de>

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@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@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/barebox
>>>>
>>
>> _______________________________________________
>> 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 12:05 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
2021-09-13 11:09       ` Ahmad Fatoum
2021-09-13 12:03         ` Michael Riesch [this message]

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=93665682-d3b1-03ce-b641-c4c6af526fae@wolfvision.net \
    --to=michael.riesch@wolfvision.net \
    --cc=a.fatoum@pengutronix.de \
    --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