mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/3] arm: zynqmp: add support for zcu106 and boot sources
@ 2021-09-10 13:43 Michael Riesch
  2021-09-10 13:43 ` [PATCH 1/3] arm: zynqmp: add support for xilinx zcu106 board Michael Riesch
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Michael Riesch @ 2021-09-10 13:43 UTC (permalink / raw)
  To: barebox; +Cc: Michael Riesch

Hi all,

This series features some mixed additions to and improvements of
the Xilinx Zynq UltraScale+ MPSoC support in barebox.

The first patch introduces support for the Xilinx Zynq UltraScale+
MPSoC ZCU106 evaluation board (based on the board support for the
ZCU104). Tested the basic features (SD card, Ethernet, ...) on a
ZCU106 board (well, obviously...).

The second patch fixes two links in the Zynq UltraScale+ part
of the documentation.

Finally, the third patch introduces support for the boot source
detection on the Zynq UltraScale+ architecture in general. Tested
on the ZCU106 and a custom Zynq UltraScale+ board.

Best regards,
Michael

Michael Riesch (3):
  arm: zynqmp: add support for xilinx zcu106 board
  Documentation: boards: zynqmp: fix broken links
  arm: zynqmp: add boot source support

 Documentation/boards/zynqmp.rst               |  4 +-
 arch/arm/boards/Makefile                      |  1 +
 arch/arm/boards/xilinx-zcu106/Makefile        |  3 ++
 arch/arm/boards/xilinx-zcu106/board.c         | 21 +++++++++
 arch/arm/boards/xilinx-zcu106/lowlevel.c      | 24 +++++++++++
 arch/arm/boards/xilinx-zcu106/lowlevel_init.S | 12 ++++++
 arch/arm/dts/Makefile                         |  1 +
 arch/arm/dts/zynqmp-zcu106-revA.dts           | 21 +++++++++
 arch/arm/mach-zynqmp/Kconfig                  |  6 +++
 arch/arm/mach-zynqmp/zynqmp.c                 | 43 +++++++++++++++++++
 images/Makefile.zynqmp                        |  4 ++
 11 files changed, 138 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/boards/xilinx-zcu106/Makefile
 create mode 100644 arch/arm/boards/xilinx-zcu106/board.c
 create mode 100644 arch/arm/boards/xilinx-zcu106/lowlevel.c
 create mode 100644 arch/arm/boards/xilinx-zcu106/lowlevel_init.S
 create mode 100644 arch/arm/dts/zynqmp-zcu106-revA.dts

-- 
2.17.1


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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] arm: zynqmp: add support for xilinx zcu106 board
  2021-09-10 13:43 [PATCH 0/3] arm: zynqmp: add support for zcu106 and boot sources Michael Riesch
@ 2021-09-10 13:43 ` 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
  2 siblings, 0 replies; 8+ messages in thread
From: Michael Riesch @ 2021-09-10 13:43 UTC (permalink / raw)
  To: barebox; +Cc: Michael Riesch

Add support for the Xilinx Zynq UltraScale+ MPSoC ZCU106 evaluation
board.

The changes are derived from the ZCU104 board support by applying
s/104/106/g (more or less).

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 arch/arm/boards/Makefile                      |  1 +
 arch/arm/boards/xilinx-zcu106/Makefile        |  3 +++
 arch/arm/boards/xilinx-zcu106/board.c         | 21 ++++++++++++++++
 arch/arm/boards/xilinx-zcu106/lowlevel.c      | 24 +++++++++++++++++++
 arch/arm/boards/xilinx-zcu106/lowlevel_init.S | 12 ++++++++++
 arch/arm/dts/Makefile                         |  1 +
 arch/arm/dts/zynqmp-zcu106-revA.dts           | 21 ++++++++++++++++
 arch/arm/mach-zynqmp/Kconfig                  |  6 +++++
 images/Makefile.zynqmp                        |  4 ++++
 9 files changed, 93 insertions(+)
 create mode 100644 arch/arm/boards/xilinx-zcu106/Makefile
 create mode 100644 arch/arm/boards/xilinx-zcu106/board.c
 create mode 100644 arch/arm/boards/xilinx-zcu106/lowlevel.c
 create mode 100644 arch/arm/boards/xilinx-zcu106/lowlevel_init.S
 create mode 100644 arch/arm/dts/zynqmp-zcu106-revA.dts

diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
index 5aac64fce..22dc9fe3d 100644
--- a/arch/arm/boards/Makefile
+++ b/arch/arm/boards/Makefile
@@ -170,6 +170,7 @@ obj-$(CONFIG_MACH_WARP7)			+= element14-warp7/
 obj-$(CONFIG_MACH_WEBASTO_CCBV2)		+= webasto-ccbv2/
 obj-$(CONFIG_MACH_VF610_TWR)			+= freescale-vf610-twr/
 obj-$(CONFIG_MACH_XILINX_ZCU104)		+= xilinx-zcu104/
+obj-$(CONFIG_MACH_XILINX_ZCU106)		+= xilinx-zcu106/
 obj-$(CONFIG_MACH_ZII_COMMON)			+= zii-common/
 obj-$(CONFIG_MACH_ZII_RDU1)			+= zii-imx51-rdu1/
 obj-$(CONFIG_MACH_ZII_RDU2)			+= zii-imx6q-rdu2/
diff --git a/arch/arm/boards/xilinx-zcu106/Makefile b/arch/arm/boards/xilinx-zcu106/Makefile
new file mode 100644
index 000000000..297f77d57
--- /dev/null
+++ b/arch/arm/boards/xilinx-zcu106/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+obj-y += board.o
+lwl-y += lowlevel.o lowlevel_init.o
diff --git a/arch/arm/boards/xilinx-zcu106/board.c b/arch/arm/boards/xilinx-zcu106/board.c
new file mode 100644
index 000000000..0cb5ce86e
--- /dev/null
+++ b/arch/arm/boards/xilinx-zcu106/board.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021, WolfVision GmbH
+ * Author: Michael Riesch <michael.riesch@wolfvision.net>
+ *
+ * Based on the barebox ZCU104 board support code.
+ */
+
+#include <common.h>
+#include <init.h>
+#include <mach/zynqmp-bbu.h>
+
+static int zcu106_register_update_handler(void)
+{
+	if (!of_machine_is_compatible("xlnx,zynqmp-zcu106"))
+		return 0;
+
+	return zynqmp_bbu_register_handler("SD", "/boot/BOOT.BIN",
+					   BBU_HANDLER_FLAG_DEFAULT);
+}
+device_initcall(zcu106_register_update_handler);
diff --git a/arch/arm/boards/xilinx-zcu106/lowlevel.c b/arch/arm/boards/xilinx-zcu106/lowlevel.c
new file mode 100644
index 000000000..ccc8d6141
--- /dev/null
+++ b/arch/arm/boards/xilinx-zcu106/lowlevel.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021, WolfVision GmbH
+ * Author: Michael Riesch <michael.riesch@wolfvision.net>
+ *
+ * Based on the barebox ZCU104 board support code.
+ */
+
+#include <common.h>
+#include <debug_ll.h>
+#include <asm/barebox-arm.h>
+
+extern char __dtb_zynqmp_zcu106_revA_start[];
+
+void zynqmp_zcu106_start(uint32_t, uint32_t, uint32_t);
+
+void noinline zynqmp_zcu106_start(uint32_t r0, uint32_t r1, uint32_t r2)
+{
+	/* Assume that the first stage boot loader configured the UART */
+	putc_ll('>');
+
+	barebox_arm_entry(0, SZ_2G,
+			  __dtb_zynqmp_zcu106_revA_start + global_variable_offset());
+}
diff --git a/arch/arm/boards/xilinx-zcu106/lowlevel_init.S b/arch/arm/boards/xilinx-zcu106/lowlevel_init.S
new file mode 100644
index 000000000..f3d55dcef
--- /dev/null
+++ b/arch/arm/boards/xilinx-zcu106/lowlevel_init.S
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <linux/linkage.h>
+#include <asm/barebox-arm64.h>
+
+/* The DRAM is already setup */
+#define STACK_TOP 0x80000000
+
+ENTRY_PROC(start_zynqmp_zcu106)
+	mov x0, #STACK_TOP
+	mov sp, x0
+	b zynqmp_zcu106_start
+ENTRY_PROC_END(start_zynqmp_zcu106)
diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index ffa9fe88c..14ca6c38e 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -170,6 +170,7 @@ lwl-$(CONFIG_MACH_SAMA5D27_SOM1) += at91-sama5d27_som1_ek.dtb.o
 lwl-$(CONFIG_MACH_SAMA5D27_GIANTBOARD) += at91-sama5d27_giantboard.dtb.o
 lwl-$(CONFIG_MACH_AT91SAM9X5EK) += at91sam9x5ek.dtb.o
 lwl-$(CONFIG_MACH_XILINX_ZCU104) += zynqmp-zcu104-revA.dtb.o
+lwl-$(CONFIG_MACH_XILINX_ZCU106) += zynqmp-zcu106-revA.dtb.o
 
 lwl-$(CONFIG_MACH_ZII_IMX7D_DEV) += imx7d-zii-rpu2.dtb.o imx7d-zii-rmu2.dtb.o
 lwl-$(CONFIG_MACH_WAGO_PFC_AM35XX) += am35xx-pfc-750_820x.dtb.o
diff --git a/arch/arm/dts/zynqmp-zcu106-revA.dts b/arch/arm/dts/zynqmp-zcu106-revA.dts
new file mode 100644
index 000000000..7c5058826
--- /dev/null
+++ b/arch/arm/dts/zynqmp-zcu106-revA.dts
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * dts file for Xilinx ZynqMP ZCU106
+ *
+ * Copyright (C) 2021, WolfVision GmbH
+ * Author: Michael Riesch <michael.riesch@wolfvision.net>
+ *
+ * Based on the dts for the Xilinx ZynqMP ZCU104.
+ */
+
+#include <arm64/xilinx/zynqmp-zcu106-revA.dts>
+
+/ {
+	chosen {
+		environment {
+			compatible = "barebox,environment";
+			device-path = &sdhci1, "partname:0";
+			file-path = "barebox.env";
+		};
+	};
+};
diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig
index c9dc71c9e..78cb90165 100644
--- a/arch/arm/mach-zynqmp/Kconfig
+++ b/arch/arm/mach-zynqmp/Kconfig
@@ -7,4 +7,10 @@ config MACH_XILINX_ZCU104
 	  Say Y here if you are using the Xilinx Zynq UltraScale+ MPSoC ZCU104
 	  evaluation board.
 
+config MACH_XILINX_ZCU106
+	bool "Xilinx Zynq UltraScale+ MPSoC ZCU106"
+	help
+	  Say Y here if you are using the Xilinx Zynq UltraScale+ MPSoC ZCU106
+	  evaluation board.
+
 endif
diff --git a/images/Makefile.zynqmp b/images/Makefile.zynqmp
index da3e90b84..872f39988 100644
--- a/images/Makefile.zynqmp
+++ b/images/Makefile.zynqmp
@@ -6,3 +6,7 @@
 pblb-$(CONFIG_MACH_XILINX_ZCU104) += start_zynqmp_zcu104
 FILE_barebox-zynqmp-zcu104.img = start_zynqmp_zcu104.pblb
 image-$(CONFIG_MACH_XILINX_ZCU104) += barebox-zynqmp-zcu104.img
+
+pblb-$(CONFIG_MACH_XILINX_ZCU106) += start_zynqmp_zcu106
+FILE_barebox-zynqmp-zcu106.img = start_zynqmp_zcu106.pblb
+image-$(CONFIG_MACH_XILINX_ZCU106) += barebox-zynqmp-zcu106.img
-- 
2.17.1


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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] Documentation: boards: zynqmp: fix broken links
  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 ` Michael Riesch
  2021-09-10 13:43 ` [PATCH 3/3] arm: zynqmp: add boot source support Michael Riesch
  2 siblings, 0 replies; 8+ messages in thread
From: Michael Riesch @ 2021-09-10 13:43 UTC (permalink / raw)
  To: barebox; +Cc: Michael Riesch

The external links are not properly recognized. Remove quotation marks
as a fix.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 Documentation/boards/zynqmp.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/boards/zynqmp.rst b/Documentation/boards/zynqmp.rst
index 05d41c401..98fcac017 100644
--- a/Documentation/boards/zynqmp.rst
+++ b/Documentation/boards/zynqmp.rst
@@ -30,8 +30,8 @@ instructions for these tools how to prepare the BOOT.BIN image.
 Create a FAT partition as the first partition of the SD card and copy the
 produced BOOT.BIN into this partition.
 
-.. _FSBL: `https://github.com/Xilinx/embeddedsw/`
-.. _bootgen: `https://github.com/Xilinx/bootgen`
+.. _FSBL: https://github.com/Xilinx/embeddedsw/
+.. _bootgen: https://github.com/Xilinx/bootgen
 
 Booting Barebox
 ---------------
-- 
2.17.1


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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] arm: zynqmp: add boot source support
  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 ` Michael Riesch
  2021-09-10 13:59   ` Michael Tretter
  2 siblings, 1 reply; 8+ messages in thread
From: Michael Riesch @ 2021-09-10 13:43 UTC (permalink / raw)
  To: barebox; +Cc: Michael Riesch

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 },
+};
+
+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);
+
+	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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] arm: zynqmp: add boot source support
  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
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Tretter @ 2021-09-10 13:59 UTC (permalink / raw)
  To: Michael Riesch; +Cc: barebox

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.

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

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] arm: zynqmp: add boot source support
  2021-09-10 13:59   ` Michael Tretter
@ 2021-09-13 10:53     ` Michael Riesch
  2021-09-13 11:09       ` Ahmad Fatoum
  0 siblings, 1 reply; 8+ messages in thread
From: Michael Riesch @ 2021-09-13 10:53 UTC (permalink / raw)
  To: Michael Tretter; +Cc: barebox

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] arm: zynqmp: add boot source support
  2021-09-13 10:53     ` Michael Riesch
@ 2021-09-13 11:09       ` Ahmad Fatoum
  2021-09-13 12:03         ` Michael Riesch
  0 siblings, 1 reply; 8+ messages in thread
From: Ahmad Fatoum @ 2021-09-13 11:09 UTC (permalink / raw)
  To: Michael Riesch, Michael Tretter; +Cc: barebox

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] arm: zynqmp: add boot source support
  2021-09-13 11:09       ` Ahmad Fatoum
@ 2021-09-13 12:03         ` Michael Riesch
  0 siblings, 0 replies; 8+ messages in thread
From: Michael Riesch @ 2021-09-13 12:03 UTC (permalink / raw)
  To: Ahmad Fatoum, Michael Tretter; +Cc: barebox

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-13 12:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox