[U-Boot] [PATCH 1/4] ARM: socfpga: Factor out handoff register configuration

[U-Boot] [PATCH 1/4] ARM: socfpga: Factor out handoff register configuration

[Marek Vasut]

Factor out the code for programming preloader handoff register values,
the ISWGRP Handoff 0 and 1. These registers later control which bridges
are enabled by the "bridge" command on Gen5 devices.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <chin.liang.see@intel.com>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Cc: Tien Fong Chee <tien.fong.chee@intel.com>
---
 .../include/mach/reset_manager_gen5.h         |  1 +
 arch/arm/mach-socfpga/reset_manager_gen5.c    | 25 +++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
index dd58922cec..5e490d182e 100644
--- a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
+++ b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
@@ -9,6 +9,7 @@
 #include <dt-bindings/reset/altr,rst-mgr.h>
 
 void reset_deassert_peripherals_handoff(void);
+void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h);
 void socfpga_bridges_reset(int enable);
 
 struct socfpga_reset_manager {
diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c
index 25baef79bc..66af924485 100644
--- a/arch/arm/mach-socfpga/reset_manager_gen5.c
+++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
@@ -73,6 +73,28 @@ void reset_deassert_peripherals_handoff(void)
 #define L3REGS_REMAP_HPS2FPGA_MASK	0x08
 #define L3REGS_REMAP_OCRAM_MASK		0x01
 
+void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h)
+{
+	u32 brgmask = 0x0;
+	u32 l3rmask = L3REGS_REMAP_OCRAM_MASK;
+
+	if (h2f)
+		brgmask |= BIT(0);
+	else
+		l3rmask |= L3REGS_REMAP_HPS2FPGA_MASK;
+
+	if (lwh2f)
+		brgmask |= BIT(1);
+	else
+		l3rmask |= L3REGS_REMAP_LWHPS2FPGA_MASK;
+
+	if (f2h)
+		brgmask |= BIT(2);
+
+	writel(brgmask, &sysmgr_regs->iswgrp_handoff[0]);
+	writel(l3rmask, &sysmgr_regs->iswgrp_handoff[1]);
+}
+
 void socfpga_bridges_reset(int enable)
 {
 	const u32 l3mask = L3REGS_REMAP_LWHPS2FPGA_MASK |
@@ -83,8 +105,7 @@ void socfpga_bridges_reset(int enable)
 		/* brdmodrst */
 		writel(0xffffffff, &reset_manager_base->brg_mod_reset);
 	} else {
-		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
-		writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
+		socfpga_bridges_set_handoff_regs(false, false, false);
 
 		/* Check signal from FPGA. */
 		if (!fpgamgr_test_fpga_ready()) {
-- 
2.20.1

[U-Boot] [PATCH 2/4] ARM: socfpga: Disable bridges in SPL unless booting from FPGA

[Marek Vasut]

Disable bridges between L3 Main switch and FPGA unless booting
from FPGA and keep them disabled to prevent glitches and possible
hangs of the L3 Main switch.

The current version of the code could have enabled the bridges
between the L3 Main switch and FPGA for a short period of time
in board_init_f() in case the FPGA was programmed and then again
disable them at the end of board_init_f(). Replace this with a
code which only sets up the handoff registers and let the user
enable the bridges later on.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <chin.liang.see@intel.com>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Cc: Tien Fong Chee <tien.fong.chee@intel.com>
---
 arch/arm/mach-socfpga/spl_gen5.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c
index 45382b549a..aa88f2cf3e 100644
--- a/arch/arm/mach-socfpga/spl_gen5.c
+++ b/arch/arm/mach-socfpga/spl_gen5.c
@@ -188,7 +188,7 @@ void board_init_f(ulong dummy)
 
 	/* De-assert reset for peripherals and bridges based on handoff */
 	reset_deassert_peripherals_handoff();
-	socfpga_bridges_reset(0);
+	socfpga_bridges_set_handoff_regs(true, true, true);
 
 	debug("Unfreezing/Thaw all I/O banks\n");
 	/* unfreeze / thaw all IO banks */
@@ -228,7 +228,4 @@ void board_init_f(ulong dummy)
 		puts("SDRAM size check failed!\n");
 		hang();
 	}
-
-	if (!socfpga_is_booting_from_fpga())
-		socfpga_bridges_reset(1);
 }
-- 
2.20.1

[U-Boot] [PATCH 3/4] ARM: socfpga: Fully unmap the FPGA bridges from L3 space

[Marek Vasut]

Instead of just putting the bridges into reset, fully remove the bridges
from the L3 main bridge space when disabling them by clearing bits in
NIC-301 remap register. Moreover, only touch the 3 LSbits in brgmodrst
register as the rest of the bits are undefined.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <chin.liang.see@intel.com>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Cc: Tien Fong Chee <tien.fong.chee@intel.com>
---
 arch/arm/mach-socfpga/reset_manager_gen5.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c
index 66af924485..89a384b59c 100644
--- a/arch/arm/mach-socfpga/reset_manager_gen5.c
+++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
@@ -103,7 +103,8 @@ void socfpga_bridges_reset(int enable)
 
 	if (enable) {
 		/* brdmodrst */
-		writel(0xffffffff, &reset_manager_base->brg_mod_reset);
+		writel(0x7, &reset_manager_base->brg_mod_reset);
+		writel(L3REGS_REMAP_OCRAM_MASK, SOCFPGA_L3REGS_ADDRESS);
 	} else {
 		socfpga_bridges_set_handoff_regs(false, false, false);
 
-- 
2.20.1

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Marek Vasut]

Add optional "mask" argument to the SoCFPGA bridge command, to select
which bridges should be enabled/disabled. This allows the user to avoid
enabling bridges which are not connected into the FPGA fabric. Default
behavior is to enable/disable all bridges.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <chin.liang.see@intel.com>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Cc: Tien Fong Chee <tien.fong.chee@intel.com>
---
 arch/arm/mach-socfpga/include/mach/misc.h |  2 +-
 arch/arm/mach-socfpga/misc.c              | 17 +++++++++++------
 arch/arm/mach-socfpga/misc_arria10.c      |  2 +-
 arch/arm/mach-socfpga/misc_gen5.c         | 12 +++++++++++-
 arch/arm/mach-socfpga/misc_s10.c          |  2 +-
 5 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h
index 86d5d2b62b..c3ca8cdf3b 100644
--- a/arch/arm/mach-socfpga/include/mach/misc.h
+++ b/arch/arm/mach-socfpga/include/mach/misc.h
@@ -39,6 +39,6 @@ void socfpga_init_security_policies(void);
 void socfpga_sdram_remap_zero(void);
 #endif
 
-void do_bridge_reset(int enable);
+void do_bridge_reset(int enable, unsigned int mask);
 
 #endif /* _MISC_H_ */
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
index ec8339e045..e1ea8eb73e 100644
--- a/arch/arm/mach-socfpga/misc.c
+++ b/arch/arm/mach-socfpga/misc.c
@@ -126,17 +126,22 @@ int arch_cpu_init(void)
 #ifndef CONFIG_SPL_BUILD
 static int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	if (argc != 2)
+	unsigned int mask = ~0;
+
+	if (argc < 2 || argc > 3)
 		return CMD_RET_USAGE;
 
 	argv++;
 
+	if (argc == 3)
+		mask = simple_strtoul(argv[1], NULL, 16);
+
 	switch (*argv[0]) {
 	case 'e':	/* Enable */
-		do_bridge_reset(1);
+		do_bridge_reset(1, mask);
 		break;
 	case 'd':	/* Disable */
-		do_bridge_reset(0);
+		do_bridge_reset(0, mask);
 		break;
 	default:
 		return CMD_RET_USAGE;
@@ -145,10 +150,10 @@ static int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	return 0;
 }
 
-U_BOOT_CMD(bridge, 2, 1, do_bridge,
+U_BOOT_CMD(bridge, 3, 1, do_bridge,
 	   "SoCFPGA HPS FPGA bridge control",
-	   "enable  - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
-	   "bridge disable - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
+	   "enable [mask] - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
+	   "bridge disable [mask] - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
 	   ""
 );
 
diff --git a/arch/arm/mach-socfpga/misc_arria10.c b/arch/arm/mach-socfpga/misc_arria10.c
index 63b8c75d31..2e2a40b65d 100644
--- a/arch/arm/mach-socfpga/misc_arria10.c
+++ b/arch/arm/mach-socfpga/misc_arria10.c
@@ -115,7 +115,7 @@ int print_cpuinfo(void)
 }
 #endif
 
-void do_bridge_reset(int enable)
+void do_bridge_reset(int enable, unsigned int mask)
 {
 	if (enable)
 		socfpga_reset_deassert_bridges_handoff();
diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
index 6e11ba6cb2..7876953595 100644
--- a/arch/arm/mach-socfpga/misc_gen5.c
+++ b/arch/arm/mach-socfpga/misc_gen5.c
@@ -249,9 +249,19 @@ static void socfpga_sdram_apply_static_cfg(void)
 	: : "r"(val), "r"(&sdr_ctrl->static_cfg) : "memory", "cc");
 }
 
-void do_bridge_reset(int enable)
+void do_bridge_reset(int enable, unsigned int mask)
 {
+	int i;
+
 	if (enable) {
+		socfpga_bridges_set_handoff_regs(!(mask & BIT(0)),
+						 !(mask & BIT(1)),
+						 !(mask & BIT(2)));
+		for (i = 0; i < 2; i++) {	/* Reload SW setting cache */
+			iswgrp_handoff[i] =
+				readl(&sysmgr_regs->iswgrp_handoff[i]);
+		}
+
 		writel(iswgrp_handoff[2], &sysmgr_regs->fpgaintfgrp_module);
 		socfpga_sdram_apply_static_cfg();
 		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach-socfpga/misc_s10.c
index 113eace650..60c96090ce 100644
--- a/arch/arm/mach-socfpga/misc_s10.c
+++ b/arch/arm/mach-socfpga/misc_s10.c
@@ -150,7 +150,7 @@ int arch_early_init_r(void)
 	return 0;
 }
 
-void do_bridge_reset(int enable)
+void do_bridge_reset(int enable, unsigned int mask)
 {
 	socfpga_bridges_reset(enable);
 }
-- 
2.20.1

[U-Boot] [PATCH 1/4] ARM: socfpga: Factor out handoff register configuration

[Simon Goldschmidt]

On 17.04.19 22:15, Marek Vasut wrote:
> Factor out the code for programming preloader handoff register values,
> the ISWGRP Handoff 0 and 1. These registers later control which bridges
> are enabled by the "bridge" command on Gen5 devices.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <chin.liang.see@intel.com>
> Cc: Dinh Nguyen <dinguyen@kernel.org>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Cc: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>   .../include/mach/reset_manager_gen5.h         |  1 +
>   arch/arm/mach-socfpga/reset_manager_gen5.c    | 25 +++++++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
> index dd58922cec..5e490d182e 100644
> --- a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
> @@ -9,6 +9,7 @@
>   #include <dt-bindings/reset/altr,rst-mgr.h>
>   
>   void reset_deassert_peripherals_handoff(void);
> +void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h);
>   void socfpga_bridges_reset(int enable);
>   
>   struct socfpga_reset_manager {
> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c
> index 25baef79bc..66af924485 100644
> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
> @@ -73,6 +73,28 @@ void reset_deassert_peripherals_handoff(void)
>   #define L3REGS_REMAP_HPS2FPGA_MASK	0x08
>   #define L3REGS_REMAP_OCRAM_MASK		0x01
>   
> +void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h)
> +{
> +	u32 brgmask = 0x0;
> +	u32 l3rmask = L3REGS_REMAP_OCRAM_MASK;
> +
> +	if (h2f)
> +		brgmask |= BIT(0);
> +	else
> +		l3rmask |= L3REGS_REMAP_HPS2FPGA_MASK;
> +
> +	if (lwh2f)
> +		brgmask |= BIT(1);
> +	else
> +		l3rmask |= L3REGS_REMAP_LWHPS2FPGA_MASK;
> +
> +	if (f2h)
> +		brgmask |= BIT(2);
> +
> +	writel(brgmask, &sysmgr_regs->iswgrp_handoff[0]);
> +	writel(l3rmask, &sysmgr_regs->iswgrp_handoff[1]);
> +}
> +
>   void socfpga_bridges_reset(int enable)
>   {
>   	const u32 l3mask = L3REGS_REMAP_LWHPS2FPGA_MASK |

'l3mask' seems unused after this change, no?

Other than that:
Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> @@ -83,8 +105,7 @@ void socfpga_bridges_reset(int enable)
>   		/* brdmodrst */
>   		writel(0xffffffff, &reset_manager_base->brg_mod_reset);
>   	} else {
> -		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
> -		writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
> +		socfpga_bridges_set_handoff_regs(false, false, false);
>   
>   		/* Check signal from FPGA. */
>   		if (!fpgamgr_test_fpga_ready()) {
> 

[U-Boot] [PATCH 2/4] ARM: socfpga: Disable bridges in SPL unless booting from FPGA

[Simon Goldschmidt]

On 17.04.19 22:15, Marek Vasut wrote:
> Disable bridges between L3 Main switch and FPGA unless booting
> from FPGA and keep them disabled to prevent glitches and possible
> hangs of the L3 Main switch.
> 
> The current version of the code could have enabled the bridges
> between the L3 Main switch and FPGA for a short period of time
> in board_init_f() in case the FPGA was programmed and then again
> disable them at the end of board_init_f(). Replace this with a
> code which only sets up the handoff registers and let the user
> enable the bridges later on.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <chin.liang.see@intel.com>
> Cc: Dinh Nguyen <dinguyen@kernel.org>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Cc: Tien Fong Chee <tien.fong.chee@intel.com>

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> ---
>   arch/arm/mach-socfpga/spl_gen5.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c
> index 45382b549a..aa88f2cf3e 100644
> --- a/arch/arm/mach-socfpga/spl_gen5.c
> +++ b/arch/arm/mach-socfpga/spl_gen5.c
> @@ -188,7 +188,7 @@ void board_init_f(ulong dummy)
>   
>   	/* De-assert reset for peripherals and bridges based on handoff */
>   	reset_deassert_peripherals_handoff();
> -	socfpga_bridges_reset(0);
> +	socfpga_bridges_set_handoff_regs(true, true, true);
>   
>   	debug("Unfreezing/Thaw all I/O banks\n");
>   	/* unfreeze / thaw all IO banks */
> @@ -228,7 +228,4 @@ void board_init_f(ulong dummy)
>   		puts("SDRAM size check failed!\n");
>   		hang();
>   	}
> -
> -	if (!socfpga_is_booting_from_fpga())
> -		socfpga_bridges_reset(1);
>   }
> 

[U-Boot] [PATCH 3/4] ARM: socfpga: Fully unmap the FPGA bridges from L3 space

[Simon Goldschmidt]

On 17.04.19 22:15, Marek Vasut wrote:
> Instead of just putting the bridges into reset, fully remove the bridges
> from the L3 main bridge space when disabling them by clearing bits in
> NIC-301 remap register. Moreover, only touch the 3 LSbits in brgmodrst
> register as the rest of the bits are undefined.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <chin.liang.see@intel.com>
> Cc: Dinh Nguyen <dinguyen@kernel.org>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Cc: Tien Fong Chee <tien.fong.chee@intel.com>

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> ---
>   arch/arm/mach-socfpga/reset_manager_gen5.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c
> index 66af924485..89a384b59c 100644
> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
> @@ -103,7 +103,8 @@ void socfpga_bridges_reset(int enable)
>   
>   	if (enable) {
>   		/* brdmodrst */
> -		writel(0xffffffff, &reset_manager_base->brg_mod_reset);
> +		writel(0x7, &reset_manager_base->brg_mod_reset);
> +		writel(L3REGS_REMAP_OCRAM_MASK, SOCFPGA_L3REGS_ADDRESS);
>   	} else {
>   		socfpga_bridges_set_handoff_regs(false, false, false);
>   
> 

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Simon Goldschmidt]

On 17.04.19 22:15, Marek Vasut wrote:
> Add optional "mask" argument to the SoCFPGA bridge command, to select
> which bridges should be enabled/disabled. This allows the user to avoid
> enabling bridges which are not connected into the FPGA fabric. Default
> behavior is to enable/disable all bridges.

So does this change the command? Seems like leaving away the new 'mask' 
argument would now lead to enabling all bridges by overwriting whatever 
the handoff values were before?

Regards,
Simon

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <chin.liang.see@intel.com>
> Cc: Dinh Nguyen <dinguyen@kernel.org>
> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Cc: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>   arch/arm/mach-socfpga/include/mach/misc.h |  2 +-
>   arch/arm/mach-socfpga/misc.c              | 17 +++++++++++------
>   arch/arm/mach-socfpga/misc_arria10.c      |  2 +-
>   arch/arm/mach-socfpga/misc_gen5.c         | 12 +++++++++++-
>   arch/arm/mach-socfpga/misc_s10.c          |  2 +-
>   5 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h
> index 86d5d2b62b..c3ca8cdf3b 100644
> --- a/arch/arm/mach-socfpga/include/mach/misc.h
> +++ b/arch/arm/mach-socfpga/include/mach/misc.h
> @@ -39,6 +39,6 @@ void socfpga_init_security_policies(void);
>   void socfpga_sdram_remap_zero(void);
>   #endif
>   
> -void do_bridge_reset(int enable);
> +void do_bridge_reset(int enable, unsigned int mask);
>   
>   #endif /* _MISC_H_ */
> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> index ec8339e045..e1ea8eb73e 100644
> --- a/arch/arm/mach-socfpga/misc.c
> +++ b/arch/arm/mach-socfpga/misc.c
> @@ -126,17 +126,22 @@ int arch_cpu_init(void)
>   #ifndef CONFIG_SPL_BUILD
>   static int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   {
> -	if (argc != 2)
> +	unsigned int mask = ~0;
> +
> +	if (argc < 2 || argc > 3)
>   		return CMD_RET_USAGE;
>   
>   	argv++;
>   
> +	if (argc == 3)
> +		mask = simple_strtoul(argv[1], NULL, 16);
> +
>   	switch (*argv[0]) {
>   	case 'e':	/* Enable */
> -		do_bridge_reset(1);
> +		do_bridge_reset(1, mask);
>   		break;
>   	case 'd':	/* Disable */
> -		do_bridge_reset(0);
> +		do_bridge_reset(0, mask);
>   		break;
>   	default:
>   		return CMD_RET_USAGE;
> @@ -145,10 +150,10 @@ static int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	return 0;
>   }
>   
> -U_BOOT_CMD(bridge, 2, 1, do_bridge,
> +U_BOOT_CMD(bridge, 3, 1, do_bridge,
>   	   "SoCFPGA HPS FPGA bridge control",
> -	   "enable  - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
> -	   "bridge disable - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
> +	   "enable [mask] - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
> +	   "bridge disable [mask] - Enable HPS-to-FPGA, FPGA-to-HPS, LWHPS-to-FPGA bridges\n"
>   	   ""
>   );
>   
> diff --git a/arch/arm/mach-socfpga/misc_arria10.c b/arch/arm/mach-socfpga/misc_arria10.c
> index 63b8c75d31..2e2a40b65d 100644
> --- a/arch/arm/mach-socfpga/misc_arria10.c
> +++ b/arch/arm/mach-socfpga/misc_arria10.c
> @@ -115,7 +115,7 @@ int print_cpuinfo(void)
>   }
>   #endif
>   
> -void do_bridge_reset(int enable)
> +void do_bridge_reset(int enable, unsigned int mask)
>   {
>   	if (enable)
>   		socfpga_reset_deassert_bridges_handoff();
> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
> index 6e11ba6cb2..7876953595 100644
> --- a/arch/arm/mach-socfpga/misc_gen5.c
> +++ b/arch/arm/mach-socfpga/misc_gen5.c
> @@ -249,9 +249,19 @@ static void socfpga_sdram_apply_static_cfg(void)
>   	: : "r"(val), "r"(&sdr_ctrl->static_cfg) : "memory", "cc");
>   }
>   
> -void do_bridge_reset(int enable)
> +void do_bridge_reset(int enable, unsigned int mask)
>   {
> +	int i;
> +
>   	if (enable) {
> +		socfpga_bridges_set_handoff_regs(!(mask & BIT(0)),
> +						 !(mask & BIT(1)),
> +						 !(mask & BIT(2)));
> +		for (i = 0; i < 2; i++) {	/* Reload SW setting cache */
> +			iswgrp_handoff[i] =
> +				readl(&sysmgr_regs->iswgrp_handoff[i]);
> +		}
> +
>   		writel(iswgrp_handoff[2], &sysmgr_regs->fpgaintfgrp_module);
>   		socfpga_sdram_apply_static_cfg();
>   		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
> diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach-socfpga/misc_s10.c
> index 113eace650..60c96090ce 100644
> --- a/arch/arm/mach-socfpga/misc_s10.c
> +++ b/arch/arm/mach-socfpga/misc_s10.c
> @@ -150,7 +150,7 @@ int arch_early_init_r(void)
>   	return 0;
>   }
>   
> -void do_bridge_reset(int enable)
> +void do_bridge_reset(int enable, unsigned int mask)
>   {
>   	socfpga_bridges_reset(enable);
>   }
> 

[U-Boot] [PATCH 1/4] ARM: socfpga: Factor out handoff register configuration

[Marek Vasut]

On 4/19/19 9:47 PM, Simon Goldschmidt wrote:
> 
> 
> On 17.04.19 22:15, Marek Vasut wrote:
>> Factor out the code for programming preloader handoff register values,
>> the ISWGRP Handoff 0 and 1. These registers later control which bridges
>> are enabled by the "bridge" command on Gen5 devices.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Chin Liang See <chin.liang.see@intel.com>
>> Cc: Dinh Nguyen <dinguyen@kernel.org>
>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>> Cc: Tien Fong Chee <tien.fong.chee@intel.com>
>> ---
>>   .../include/mach/reset_manager_gen5.h         |  1 +
>>   arch/arm/mach-socfpga/reset_manager_gen5.c    | 25 +++++++++++++++++--
>>   2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>> b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>> index dd58922cec..5e490d182e 100644
>> --- a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>> @@ -9,6 +9,7 @@
>>   #include <dt-bindings/reset/altr,rst-mgr.h>
>>     void reset_deassert_peripherals_handoff(void);
>> +void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h);
>>   void socfpga_bridges_reset(int enable);
>>     struct socfpga_reset_manager {
>> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
>> b/arch/arm/mach-socfpga/reset_manager_gen5.c
>> index 25baef79bc..66af924485 100644
>> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
>> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
>> @@ -73,6 +73,28 @@ void reset_deassert_peripherals_handoff(void)
>>   #define L3REGS_REMAP_HPS2FPGA_MASK    0x08
>>   #define L3REGS_REMAP_OCRAM_MASK        0x01
>>   +void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h)
>> +{
>> +    u32 brgmask = 0x0;
>> +    u32 l3rmask = L3REGS_REMAP_OCRAM_MASK;
>> +
>> +    if (h2f)
>> +        brgmask |= BIT(0);
>> +    else
>> +        l3rmask |= L3REGS_REMAP_HPS2FPGA_MASK;
>> +
>> +    if (lwh2f)
>> +        brgmask |= BIT(1);
>> +    else
>> +        l3rmask |= L3REGS_REMAP_LWHPS2FPGA_MASK;
>> +
>> +    if (f2h)
>> +        brgmask |= BIT(2);
>> +
>> +    writel(brgmask, &sysmgr_regs->iswgrp_handoff[0]);
>> +    writel(l3rmask, &sysmgr_regs->iswgrp_handoff[1]);
>> +}
>> +
>>   void socfpga_bridges_reset(int enable)
>>   {
>>       const u32 l3mask = L3REGS_REMAP_LWHPS2FPGA_MASK |
> 
> 'l3mask' seems unused after this change, no?

Nope, it's still used in the else {} branch of the conditional below.

[...]

>> @@ -83,8 +105,7 @@ void socfpga_bridges_reset(int enable)
>>           /* brdmodrst */
>>           writel(0xffffffff, &reset_manager_base->brg_mod_reset);
>>       } else {
>> -        writel(0, &sysmgr_regs->iswgrp_handoff[0]);
>> -        writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
>> +        socfpga_bridges_set_handoff_regs(false, false, false);
>>             /* Check signal from FPGA. */
>>           if (!fpgamgr_test_fpga_ready()) {
>>


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Marek Vasut]

On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
> 
> 
> On 17.04.19 22:15, Marek Vasut wrote:
>> Add optional "mask" argument to the SoCFPGA bridge command, to select
>> which bridges should be enabled/disabled. This allows the user to avoid
>> enabling bridges which are not connected into the FPGA fabric. Default
>> behavior is to enable/disable all bridges.
> 
> So does this change the command? Seems like leaving away the new 'mask'
> argument would now lead to enabling all bridges by overwriting whatever
> the handoff values were before?

That's how it behaved before though -- all the bridges were enabled.
Now it's possible to explicitly select which bridges to enable/disable.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/4] ARM: socfpga: Factor out handoff register configuration

[Simon Goldschmidt]

Am 22.04.2019 um 19:59 schrieb Marek Vasut:
> On 4/19/19 9:47 PM, Simon Goldschmidt wrote:
>>
>>
>> On 17.04.19 22:15, Marek Vasut wrote:
>>> Factor out the code for programming preloader handoff register values,
>>> the ISWGRP Handoff 0 and 1. These registers later control which bridges
>>> are enabled by the "bridge" command on Gen5 devices.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Chin Liang See <chin.liang.see@intel.com>
>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
>>> Cc: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>> Cc: Tien Fong Chee <tien.fong.chee@intel.com>
>>> ---
>>>    .../include/mach/reset_manager_gen5.h         |  1 +
>>>    arch/arm/mach-socfpga/reset_manager_gen5.c    | 25 +++++++++++++++++--
>>>    2 files changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>>> b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>>> index dd58922cec..5e490d182e 100644
>>> --- a/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>>> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_gen5.h
>>> @@ -9,6 +9,7 @@
>>>    #include <dt-bindings/reset/altr,rst-mgr.h>
>>>      void reset_deassert_peripherals_handoff(void);
>>> +void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h);
>>>    void socfpga_bridges_reset(int enable);
>>>      struct socfpga_reset_manager {
>>> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> index 25baef79bc..66af924485 100644
>>> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> @@ -73,6 +73,28 @@ void reset_deassert_peripherals_handoff(void)
>>>    #define L3REGS_REMAP_HPS2FPGA_MASK    0x08
>>>    #define L3REGS_REMAP_OCRAM_MASK        0x01
>>>    +void socfpga_bridges_set_handoff_regs(bool h2f, bool lwh2f, bool f2h)
>>> +{
>>> +    u32 brgmask = 0x0;
>>> +    u32 l3rmask = L3REGS_REMAP_OCRAM_MASK;
>>> +
>>> +    if (h2f)
>>> +        brgmask |= BIT(0);
>>> +    else
>>> +        l3rmask |= L3REGS_REMAP_HPS2FPGA_MASK;
>>> +
>>> +    if (lwh2f)
>>> +        brgmask |= BIT(1);
>>> +    else
>>> +        l3rmask |= L3REGS_REMAP_LWHPS2FPGA_MASK;
>>> +
>>> +    if (f2h)
>>> +        brgmask |= BIT(2);
>>> +
>>> +    writel(brgmask, &sysmgr_regs->iswgrp_handoff[0]);
>>> +    writel(l3rmask, &sysmgr_regs->iswgrp_handoff[1]);
>>> +}
>>> +
>>>    void socfpga_bridges_reset(int enable)
>>>    {
>>>        const u32 l3mask = L3REGS_REMAP_LWHPS2FPGA_MASK |
>>
>> 'l3mask' seems unused after this change, no?
> 
> Nope, it's still used in the else {} branch of the conditional below.

Oops, missed that. In that case:

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> 
> [...]
> 
>>> @@ -83,8 +105,7 @@ void socfpga_bridges_reset(int enable)
>>>            /* brdmodrst */
>>>            writel(0xffffffff, &reset_manager_base->brg_mod_reset);
>>>        } else {
>>> -        writel(0, &sysmgr_regs->iswgrp_handoff[0]);
>>> -        writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
>>> +        socfpga_bridges_set_handoff_regs(false, false, false);
>>>              /* Check signal from FPGA. */
>>>            if (!fpgamgr_test_fpga_ready()) {
>>>
> 
> 

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Simon Goldschmidt]

Am 22.04.2019 um 20:01 schrieb Marek Vasut:
> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
>>
>>
>> On 17.04.19 22:15, Marek Vasut wrote:
>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
>>> which bridges should be enabled/disabled. This allows the user to avoid
>>> enabling bridges which are not connected into the FPGA fabric. Default
>>> behavior is to enable/disable all bridges.
>>
>> So does this change the command? Seems like leaving away the new 'mask'
>> argument would now lead to enabling all bridges by overwriting whatever
>> the handoff values were before?
> 
> That's how it behaved before though -- all the bridges were enabled.
> Now it's possible to explicitly select which bridges to enable/disable.

As I read the code, before it wrote iswgrp_handoff[x] to the registers. 
The question is what is iswgrp_handoff[x]. It's not the bridges status 
from Quartus (as the "handoff" suffix might suggest). Instead (if I 
remember correctly), it's either "all bridges" or "no bridges", 
depending on the FPGA configuration status at SPL runtime.

In this case, we're probably better off with leaving it to the command 
line scripts to say which bridges shall be enabled...

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Marek Vasut]

On 4/22/19 8:22 PM, Simon Goldschmidt wrote:
> Am 22.04.2019 um 20:01 schrieb Marek Vasut:
>> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
>>>
>>>
>>> On 17.04.19 22:15, Marek Vasut wrote:
>>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
>>>> which bridges should be enabled/disabled. This allows the user to avoid
>>>> enabling bridges which are not connected into the FPGA fabric. Default
>>>> behavior is to enable/disable all bridges.
>>>
>>> So does this change the command? Seems like leaving away the new 'mask'
>>> argument would now lead to enabling all bridges by overwriting whatever
>>> the handoff values were before?
>>
>> That's how it behaved before though -- all the bridges were enabled.
>> Now it's possible to explicitly select which bridges to enable/disable.
> 
> As I read the code, before it wrote iswgrp_handoff[x] to the registers.

Nope, before it was the SPL that wrote the iswgrp_handoff registers
(iswgrp_handoff[0] to 0x0 and iswgrp_handoff[1] to 0x7)

> The question is what is iswgrp_handoff[x].

Just regular scratch registers with special name. The [0] is used to
indicate to the software how the brg_mod_reset register is supposed to
be configured. The [1] is used to indicate how the l3remap register is
supposed to be configured.

The SPL currently sets these registers as -- all bridges released out of
reset ; all bridges mapped into the L3 space. I believe this patch does
not change that behavior.

> It's not the bridges status
> from Quartus (as the "handoff" suffix might suggest). Instead (if I
> remember correctly), it's either "all bridges" or "no bridges",
> depending on the FPGA configuration status at SPL runtime.
> 
> In this case, we're probably better off with leaving it to the command
> line scripts to say which bridges shall be enabled...

This patch only changes things in that it updates the handoff registers
when the user selects a new mask, so that any software which runs after
U-Boot would be aware of which bridges U-Boot configured.

But maybe I'm missing something obvious, this stuff is so convoluted
that I'd really appreciate if you could review thoroughly if there's
something that doesn't seem right.

> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> 
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Simon Goldschmidt]

On 22.04.19 20:41, Marek Vasut wrote:
> On 4/22/19 8:22 PM, Simon Goldschmidt wrote:
>> Am 22.04.2019 um 20:01 schrieb Marek Vasut:
>>> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
>>>>
>>>>
>>>> On 17.04.19 22:15, Marek Vasut wrote:
>>>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
>>>>> which bridges should be enabled/disabled. This allows the user to avoid
>>>>> enabling bridges which are not connected into the FPGA fabric. Default
>>>>> behavior is to enable/disable all bridges.
>>>>
>>>> So does this change the command? Seems like leaving away the new 'mask'
>>>> argument would now lead to enabling all bridges by overwriting whatever
>>>> the handoff values were before?
>>>
>>> That's how it behaved before though -- all the bridges were enabled.
>>> Now it's possible to explicitly select which bridges to enable/disable.
>>
>> As I read the code, before it wrote iswgrp_handoff[x] to the registers.
> 
> Nope, before it was the SPL that wrote the iswgrp_handoff registers
> (iswgrp_handoff[0] to 0x0 and iswgrp_handoff[1] to 0x7)
> 
>> The question is what is iswgrp_handoff[x].
> 
> Just regular scratch registers with special name. The [0] is used to
> indicate to the software how the brg_mod_reset register is supposed to
> be configured. The [1] is used to indicate how the l3remap register is
> supposed to be configured.
> 
> The SPL currently sets these registers as -- all bridges released out of
> reset ; all bridges mapped into the L3 space. I believe this patch does
> not change that behavior.
> 
>> It's not the bridges status
>> from Quartus (as the "handoff" suffix might suggest). Instead (if I
>> remember correctly), it's either "all bridges" or "no bridges",
>> depending on the FPGA configuration status at SPL runtime.
>>
>> In this case, we're probably better off with leaving it to the command
>> line scripts to say which bridges shall be enabled...
> 
> This patch only changes things in that it updates the handoff registers
> when the user selects a new mask, so that any software which runs after
> U-Boot would be aware of which bridges U-Boot configured.
> 
> But maybe I'm missing something obvious, this stuff is so convoluted
> that I'd really appreciate if you could review thoroughly if there's
> something that doesn't seem right.

Hmm, if you're not 100% sure yourself, let me take the time to check 
again. What made me stumble accross this is that "bridge enable" did not 
work when no FPGA had been configured during SPL.

And the duplication of names where U-Boot caches the handoff registers 
doesn't really help to make it easy to follow...

Regards,
Simon

> 
>> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>
>>
> 
> 

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Marek Vasut]

On 4/22/19 9:18 PM, Simon Goldschmidt wrote:
> 
> 
> On 22.04.19 20:41, Marek Vasut wrote:
>> On 4/22/19 8:22 PM, Simon Goldschmidt wrote:
>>> Am 22.04.2019 um 20:01 schrieb Marek Vasut:
>>>> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
>>>>>
>>>>>
>>>>> On 17.04.19 22:15, Marek Vasut wrote:
>>>>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
>>>>>> which bridges should be enabled/disabled. This allows the user to
>>>>>> avoid
>>>>>> enabling bridges which are not connected into the FPGA fabric.
>>>>>> Default
>>>>>> behavior is to enable/disable all bridges.
>>>>>
>>>>> So does this change the command? Seems like leaving away the new
>>>>> 'mask'
>>>>> argument would now lead to enabling all bridges by overwriting
>>>>> whatever
>>>>> the handoff values were before?
>>>>
>>>> That's how it behaved before though -- all the bridges were enabled.
>>>> Now it's possible to explicitly select which bridges to enable/disable.
>>>
>>> As I read the code, before it wrote iswgrp_handoff[x] to the registers.
>>
>> Nope, before it was the SPL that wrote the iswgrp_handoff registers
>> (iswgrp_handoff[0] to 0x0 and iswgrp_handoff[1] to 0x7)
>>
>>> The question is what is iswgrp_handoff[x].
>>
>> Just regular scratch registers with special name. The [0] is used to
>> indicate to the software how the brg_mod_reset register is supposed to
>> be configured. The [1] is used to indicate how the l3remap register is
>> supposed to be configured.
>>
>> The SPL currently sets these registers as -- all bridges released out of
>> reset ; all bridges mapped into the L3 space. I believe this patch does
>> not change that behavior.
>>
>>> It's not the bridges status
>>> from Quartus (as the "handoff" suffix might suggest). Instead (if I
>>> remember correctly), it's either "all bridges" or "no bridges",
>>> depending on the FPGA configuration status at SPL runtime.
>>>
>>> In this case, we're probably better off with leaving it to the command
>>> line scripts to say which bridges shall be enabled...
>>
>> This patch only changes things in that it updates the handoff registers
>> when the user selects a new mask, so that any software which runs after
>> U-Boot would be aware of which bridges U-Boot configured.
>>
>> But maybe I'm missing something obvious, this stuff is so convoluted
>> that I'd really appreciate if you could review thoroughly if there's
>> something that doesn't seem right.
> 
> Hmm, if you're not 100% sure yourself, let me take the time to check
> again. What made me stumble accross this is that "bridge enable" did not
> work when no FPGA had been configured during SPL.

I'm reasonably sure it's OK, but another pair of eyeballs and all that ...

If the FPGA isn't configured, you shouldn't even run bridge enable, the
result of that is undefined.

> And the duplication of names where U-Boot caches the handoff registers
> doesn't really help to make it easy to follow...

Look at arch/arm/mach-socfpga/misc_gen5.c do_bridge_reset() , that
should clarify how the handoff registers are used. Keep in mind, they
are only scratch registers , they have no real impact on the HW (except
some are also read by BootROM during warm reset).

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Simon Goldschmidt]

On Mon, Apr 22, 2019 at 9:24 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/22/19 9:18 PM, Simon Goldschmidt wrote:
> >
> >
> > On 22.04.19 20:41, Marek Vasut wrote:
> >> On 4/22/19 8:22 PM, Simon Goldschmidt wrote:
> >>> Am 22.04.2019 um 20:01 schrieb Marek Vasut:
> >>>> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
> >>>>>
> >>>>>
> >>>>> On 17.04.19 22:15, Marek Vasut wrote:
> >>>>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
> >>>>>> which bridges should be enabled/disabled. This allows the user to
> >>>>>> avoid
> >>>>>> enabling bridges which are not connected into the FPGA fabric.
> >>>>>> Default
> >>>>>> behavior is to enable/disable all bridges.
> >>>>>
> >>>>> So does this change the command? Seems like leaving away the new
> >>>>> 'mask'
> >>>>> argument would now lead to enabling all bridges by overwriting
> >>>>> whatever
> >>>>> the handoff values were before?
> >>>>
> >>>> That's how it behaved before though -- all the bridges were enabled.
> >>>> Now it's possible to explicitly select which bridges to enable/disable.
> >>>
> >>> As I read the code, before it wrote iswgrp_handoff[x] to the registers.
> >>
> >> Nope, before it was the SPL that wrote the iswgrp_handoff registers
> >> (iswgrp_handoff[0] to 0x0 and iswgrp_handoff[1] to 0x7)
> >>
> >>> The question is what is iswgrp_handoff[x].
> >>
> >> Just regular scratch registers with special name. The [0] is used to
> >> indicate to the software how the brg_mod_reset register is supposed to
> >> be configured. The [1] is used to indicate how the l3remap register is
> >> supposed to be configured.
> >>
> >> The SPL currently sets these registers as -- all bridges released out of
> >> reset ; all bridges mapped into the L3 space. I believe this patch does
> >> not change that behavior.
> >>
> >>> It's not the bridges status
> >>> from Quartus (as the "handoff" suffix might suggest). Instead (if I
> >>> remember correctly), it's either "all bridges" or "no bridges",
> >>> depending on the FPGA configuration status at SPL runtime.
> >>>
> >>> In this case, we're probably better off with leaving it to the command
> >>> line scripts to say which bridges shall be enabled...
> >>
> >> This patch only changes things in that it updates the handoff registers
> >> when the user selects a new mask, so that any software which runs after
> >> U-Boot would be aware of which bridges U-Boot configured.
> >>
> >> But maybe I'm missing something obvious, this stuff is so convoluted
> >> that I'd really appreciate if you could review thoroughly if there's
> >> something that doesn't seem right.
> >
> > Hmm, if you're not 100% sure yourself, let me take the time to check
> > again. What made me stumble accross this is that "bridge enable" did not
> > work when no FPGA had been configured during SPL.
>
> I'm reasonably sure it's OK, but another pair of eyeballs and all that ...
>
> If the FPGA isn't configured, you shouldn't even run bridge enable, the
> result of that is undefined.

Well, what I meant is FPGA is configured later. Not in SPL but before the
bridge command is run. But nevermind, I got it now...

>
> > And the duplication of names where U-Boot caches the handoff registers
> > doesn't really help to make it easy to follow...
>
> Look at arch/arm/mach-socfpga/misc_gen5.c do_bridge_reset() , that
> should clarify how the handoff registers are used. Keep in mind, they
> are only scratch registers , they have no real impact on the HW (except
> some are also read by BootROM during warm reset).

I know. What I was missing is that iswgrp_handoff[3] is never written and
keeps its reset value of 0, so yes, your patch shouldn't change the current
behaviour. So:

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Regards,
Simon

[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Marek Vasut]

On 4/26/19 8:36 AM, Simon Goldschmidt wrote:
> On Mon, Apr 22, 2019 at 9:24 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/22/19 9:18 PM, Simon Goldschmidt wrote:
>>>
>>>
>>> On 22.04.19 20:41, Marek Vasut wrote:
>>>> On 4/22/19 8:22 PM, Simon Goldschmidt wrote:
>>>>> Am 22.04.2019 um 20:01 schrieb Marek Vasut:
>>>>>> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 17.04.19 22:15, Marek Vasut wrote:
>>>>>>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
>>>>>>>> which bridges should be enabled/disabled. This allows the user to
>>>>>>>> avoid
>>>>>>>> enabling bridges which are not connected into the FPGA fabric.
>>>>>>>> Default
>>>>>>>> behavior is to enable/disable all bridges.
>>>>>>>
>>>>>>> So does this change the command? Seems like leaving away the new
>>>>>>> 'mask'
>>>>>>> argument would now lead to enabling all bridges by overwriting
>>>>>>> whatever
>>>>>>> the handoff values were before?
>>>>>>
>>>>>> That's how it behaved before though -- all the bridges were enabled.
>>>>>> Now it's possible to explicitly select which bridges to enable/disable.
>>>>>
>>>>> As I read the code, before it wrote iswgrp_handoff[x] to the registers.
>>>>
>>>> Nope, before it was the SPL that wrote the iswgrp_handoff registers
>>>> (iswgrp_handoff[0] to 0x0 and iswgrp_handoff[1] to 0x7)
>>>>
>>>>> The question is what is iswgrp_handoff[x].
>>>>
>>>> Just regular scratch registers with special name. The [0] is used to
>>>> indicate to the software how the brg_mod_reset register is supposed to
>>>> be configured. The [1] is used to indicate how the l3remap register is
>>>> supposed to be configured.
>>>>
>>>> The SPL currently sets these registers as -- all bridges released out of
>>>> reset ; all bridges mapped into the L3 space. I believe this patch does
>>>> not change that behavior.
>>>>
>>>>> It's not the bridges status
>>>>> from Quartus (as the "handoff" suffix might suggest). Instead (if I
>>>>> remember correctly), it's either "all bridges" or "no bridges",
>>>>> depending on the FPGA configuration status at SPL runtime.
>>>>>
>>>>> In this case, we're probably better off with leaving it to the command
>>>>> line scripts to say which bridges shall be enabled...
>>>>
>>>> This patch only changes things in that it updates the handoff registers
>>>> when the user selects a new mask, so that any software which runs after
>>>> U-Boot would be aware of which bridges U-Boot configured.
>>>>
>>>> But maybe I'm missing something obvious, this stuff is so convoluted
>>>> that I'd really appreciate if you could review thoroughly if there's
>>>> something that doesn't seem right.
>>>
>>> Hmm, if you're not 100% sure yourself, let me take the time to check
>>> again. What made me stumble accross this is that "bridge enable" did not
>>> work when no FPGA had been configured during SPL.
>>
>> I'm reasonably sure it's OK, but another pair of eyeballs and all that ...
>>
>> If the FPGA isn't configured, you shouldn't even run bridge enable, the
>> result of that is undefined.
> 
> Well, what I meant is FPGA is configured later. Not in SPL but before the
> bridge command is run. But nevermind, I got it now...
> 
>>
>>> And the duplication of names where U-Boot caches the handoff registers
>>> doesn't really help to make it easy to follow...
>>
>> Look at arch/arm/mach-socfpga/misc_gen5.c do_bridge_reset() , that
>> should clarify how the handoff registers are used. Keep in mind, they
>> are only scratch registers , they have no real impact on the HW (except
>> some are also read by BootROM during warm reset).
> 
> I know. What I was missing is that iswgrp_handoff[3] is never written and
> keeps its reset value of 0, so yes, your patch shouldn't change the current
> behaviour. So:
> 
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

All right, in that case, let me enqueue the current bulk for 2019.07.

-- 
Best regards,
Marek Vasut