[U-Boot] [PATCH 0/3] Remove unused FPGA feature from gen5 SPL

[U-Boot] [PATCH 0/3] Remove unused FPGA feature from gen5 SPL

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

This patchset would remove the FPGA enable support feature from SPL, and moving
all FPGA related functions into one location drivers/fpga/. After this, FPGA
enable would be supported in U-boot instead of SPL. This change can help to save
some space in SPL.

This series is working on top of u-boot.git -
http://git.denx.de/u-boot.git

Tien Fong Chee (3):
  arm: socfpga: Remove unused FPGA feature from gen5 SPL
  arm: socfpga: Move Gen5 FPGA manager driver to FPGA driver
  arm: socfpga: Enable FPGA bridge in gen5 U-boot

 arch/arm/mach-socfpga/Makefile              |    1 -
 arch/arm/mach-socfpga/fpga_manager.c        |   78 ---------------------------
 arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++
 arch/arm/mach-socfpga/system_manager_gen5.c |    6 --
 drivers/ddr/altera/sdram.c                  |    8 +++-
 drivers/fpga/socfpga_gen5.c                 |   54 ++++++++++++++++++
 include/configs/socfpga_common.h            |    4 ++
 7 files changed, 74 insertions(+), 86 deletions(-)
 delete mode 100644 arch/arm/mach-socfpga/fpga_manager.c

-- 
1.7.7.4

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Remove unused FPGA feature for saving some space in gen5 SPL.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
 arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
 drivers/ddr/altera/sdram.c                  |    8 +++++++-
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c
index aa88adb..c954063 100644
--- a/arch/arm/mach-socfpga/reset_manager_gen5.c
+++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
@@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
 		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
 		writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
 
+/*
+ * FPGA feature is not required in SPL, so FPGA bridges release from reset
+ * should not be supported in SPL, but some FPGA releated setting can be stored
+ * in handoff registers as SPL to U-boot/OS handoff information. These
+ * handoff information can be used in later phase such as feeding handoff
+ * information to bridge command when user want to enable FPGA feature in U-boot
+ */
+#ifndef CONFIG_SPL_BUILD
 		/* Check signal from FPGA. */
 		if (!fpgamgr_test_fpga_ready()) {
 			/* FPGA not ready, do nothing. We allow system to boot
@@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
 
 		/* Remap the bridges into memory map */
 		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
+#endif
 	}
 	return;
 }
diff --git a/arch/arm/mach-socfpga/system_manager_gen5.c b/arch/arm/mach-socfpga/system_manager_gen5.c
index 3588a57..ee25496 100644
--- a/arch/arm/mach-socfpga/system_manager_gen5.c
+++ b/arch/arm/mach-socfpga/system_manager_gen5.c
@@ -43,12 +43,6 @@ static void populate_sysmgr_fpgaintf_module(void)
 	/* populate (not writing) the value for SYSMGR.FPGAINTF.MODULE
 	based on pinmux setting */
 	setbits_le32(&sysmgr_regs->iswgrp_handoff[2], handoff_val);
-
-	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
-	if (fpgamgr_test_fpga_ready()) {
-		/* Enable the required signals only */
-		writel(handoff_val, &sysmgr_regs->fpgaintfgrp_module);
-	}
 }
 
 /*
diff --git a/drivers/ddr/altera/sdram.c b/drivers/ddr/altera/sdram.c
index e74c5b0..df16102 100644
--- a/drivers/ddr/altera/sdram.c
+++ b/drivers/ddr/altera/sdram.c
@@ -233,6 +233,7 @@ static void sdram_dump_protection_config(void)
 	}
 }
 
+#ifndef CONFIG_SPL_BUILD
 /**
  * sdram_write_verify() - write to register and verify the write.
  * @addr:	Register address
@@ -259,6 +260,7 @@ static unsigned sdram_write_verify(const u32 *addr, const u32 val)
 	debug("correct!\n");
 	return 0;
 }
+#endif
 
 /**
  * sdr_get_ctrlcfg() - Get the value of DRAM CTRLCFG register
@@ -435,7 +437,6 @@ int sdram_mmr_init_full(unsigned int sdr_phy_reg)
 	const unsigned int rows =
 		(cfg->dram_addrw & SDR_CTRLGRP_DRAMADDRW_ROWBITS_MASK) >>
 			SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB;
-	int ret;
 
 	writel(rows, &sysmgr_regs->iswgrp_handoff[4]);
 
@@ -444,13 +445,18 @@ int sdram_mmr_init_full(unsigned int sdr_phy_reg)
 	/* saving this value to SYSMGR.ISWGRP.HANDOFF.FPGA2SDR */
 	writel(cfg->fpgaport_rst, &sysmgr_regs->iswgrp_handoff[3]);
 
+	/* FPGA feature is not required in SPL, so no test on FPGA reset in SPL */
+#ifndef CONFIG_SPL_BUILD
 	/* only enable if the FPGA is programmed */
+	int ret;
+
 	if (fpgamgr_test_fpga_ready()) {
 		ret = sdram_write_verify(&sdr_ctrl->fpgaport_rst,
 					 cfg->fpgaport_rst);
 		if (ret)
 			return ret;
 	}
+#endif
 
 	/* Restore the SDR PHY Register if valid */
 	if (sdr_phy_reg != 0xffffffff)
-- 
1.7.7.4

[U-Boot] [PATCH 2/3] arm: socfpga: Move Gen5 FPGA manager driver to FPGA driver

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Move FPGA manager driver which is Gen5 specific code from arch/arm/
into FPGA driver at driver/fpga/. No functional change.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 arch/arm/mach-socfpga/Makefile       |    1 -
 arch/arm/mach-socfpga/fpga_manager.c |   78 ----------------------------------
 drivers/fpga/socfpga_gen5.c          |   54 +++++++++++++++++++++++
 3 files changed, 54 insertions(+), 79 deletions(-)
 delete mode 100644 arch/arm/mach-socfpga/fpga_manager.c

diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 286bfef..824cd8e 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -20,7 +20,6 @@ obj-y	+= reset_manager_gen5.o
 obj-y	+= scan_manager.o
 obj-y	+= system_manager_gen5.o
 obj-y	+= wrap_pll_config.o
-obj-y	+= fpga_manager.o
 endif
 
 ifdef CONFIG_TARGET_SOCFPGA_ARRIA10
diff --git a/arch/arm/mach-socfpga/fpga_manager.c b/arch/arm/mach-socfpga/fpga_manager.c
deleted file mode 100644
index f909573..0000000
--- a/arch/arm/mach-socfpga/fpga_manager.c
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
- * Copyright (C) 2012 Altera Corporation <www.altera.com>
- * All rights reserved.
- *
- * This file contains only support functions used also by the SoCFPGA
- * platform code, the real meat is located in drivers/fpga/socfpga.c .
- *
- * SPDX-License-Identifier:	BSD-3-Clause
- */
-
-#include <common.h>
-#include <asm/io.h>
-#include <linux/errno.h>
-#include <asm/arch/fpga_manager.h>
-#include <asm/arch/reset_manager.h>
-#include <asm/arch/system_manager.h>
-
-DECLARE_GLOBAL_DATA_PTR;
-
-/* Timeout count */
-#define FPGA_TIMEOUT_CNT		0x1000000
-
-static struct socfpga_fpga_manager *fpgamgr_regs =
-	(struct socfpga_fpga_manager *)SOCFPGA_FPGAMGRREGS_ADDRESS;
-
-/* Check whether FPGA Init_Done signal is high */
-static int is_fpgamgr_initdone_high(void)
-{
-	unsigned long val;
-
-	val = readl(&fpgamgr_regs->gpio_ext_porta);
-	return val & FPGAMGRREGS_MON_GPIO_EXT_PORTA_ID_MASK;
-}
-
-/* Get the FPGA mode */
-int fpgamgr_get_mode(void)
-{
-	unsigned long val;
-
-	val = readl(&fpgamgr_regs->stat);
-	return val & FPGAMGRREGS_STAT_MODE_MASK;
-}
-
-/* Check whether FPGA is ready to be accessed */
-int fpgamgr_test_fpga_ready(void)
-{
-	/* Check for init done signal */
-	if (!is_fpgamgr_initdone_high())
-		return 0;
-
-	/* Check again to avoid false glitches */
-	if (!is_fpgamgr_initdone_high())
-		return 0;
-
-	if (fpgamgr_get_mode() != FPGAMGRREGS_MODE_USERMODE)
-		return 0;
-
-	return 1;
-}
-
-/* Poll until FPGA is ready to be accessed or timeout occurred */
-int fpgamgr_poll_fpga_ready(void)
-{
-	unsigned long i;
-
-	/* If FPGA is blank, wait till WD invoke warm reset */
-	for (i = 0; i < FPGA_TIMEOUT_CNT; i++) {
-		/* check for init done signal */
-		if (!is_fpgamgr_initdone_high())
-			continue;
-		/* check again to avoid false glitches */
-		if (!is_fpgamgr_initdone_high())
-			continue;
-		return 1;
-	}
-
-	return 0;
-}
diff --git a/drivers/fpga/socfpga_gen5.c b/drivers/fpga/socfpga_gen5.c
index 3dfb030..d8f222a 100644
--- a/drivers/fpga/socfpga_gen5.c
+++ b/drivers/fpga/socfpga_gen5.c
@@ -21,6 +21,60 @@ static struct socfpga_fpga_manager *fpgamgr_regs =
 static struct socfpga_system_manager *sysmgr_regs =
 	(struct socfpga_system_manager *)SOCFPGA_SYSMGR_ADDRESS;
 
+/* Check whether FPGA Init_Done signal is high */
+static int is_fpgamgr_initdone_high(void)
+{
+	unsigned long val;
+
+	val = readl(&fpgamgr_regs->gpio_ext_porta);
+	return val & FPGAMGRREGS_MON_GPIO_EXT_PORTA_ID_MASK;
+}
+
+/* Get the FPGA mode */
+int fpgamgr_get_mode(void)
+{
+	unsigned long val;
+
+	val = readl(&fpgamgr_regs->stat);
+	return val & FPGAMGRREGS_STAT_MODE_MASK;
+}
+
+/* Check whether FPGA is ready to be accessed */
+int fpgamgr_test_fpga_ready(void)
+{
+	/* Check for init done signal */
+	if (!is_fpgamgr_initdone_high())
+		return 0;
+
+	/* Check again to avoid false glitches */
+	if (!is_fpgamgr_initdone_high())
+		return 0;
+
+	if (fpgamgr_get_mode() != FPGAMGRREGS_MODE_USERMODE)
+		return 0;
+
+	return 1;
+}
+
+/* Poll until FPGA is ready to be accessed or timeout occurred */
+int fpgamgr_poll_fpga_ready(void)
+{
+	unsigned long i;
+
+	/* If FPGA is blank, wait till WD invoke warm reset */
+	for (i = 0; i < FPGA_TIMEOUT_CNT; i++) {
+		/* check for init done signal */
+		if (!is_fpgamgr_initdone_high())
+			continue;
+		/* check again to avoid false glitches */
+		if (!is_fpgamgr_initdone_high())
+			continue;
+		return 1;
+	}
+
+	return 0;
+}
+
 /* Set CD ratio */
 static void fpgamgr_set_cd_ratio(unsigned long ratio)
 {
-- 
1.7.7.4

[U-Boot] [PATCH 3/3] arm: socfpga: Enable FPGA bridge in gen5 U-boot

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Enable FPGA bridge in gen5 U-boot instead of gen5 SPL because FPGA feature is not
required in SPL. Remove FPGA feature in SPL can help to save some space.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 include/configs/socfpga_common.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index 9be9e79..f5b3277 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -70,6 +70,10 @@
 #define CONFIG_CMD_PXE
 #define CONFIG_MENU
 
+#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
+#define CONFIG_PREBOOT "bridge enable; echo bridge enable"
+#endif
+
 /*
  * Cache
  */
-- 
1.7.7.4

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Marek Vasut]

On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Remove unused FPGA feature for saving some space in gen5 SPL.

I really dislike the ifdefery. Why is this patch even needed?
I thought we had no space problems in the Gen5 SPL?

> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
>  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
>  drivers/ddr/altera/sdram.c                  |    8 +++++++-
>  3 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c b/arch/arm/mach-socfpga/reset_manager_gen5.c
> index aa88adb..c954063 100644
> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
> @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
>  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
>  		writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
>  
> +/*
> + * FPGA feature is not required in SPL, so FPGA bridges release from reset
> + * should not be supported in SPL, but some FPGA releated setting can be stored
> + * in handoff registers as SPL to U-boot/OS handoff information. These
> + * handoff information can be used in later phase such as feeding handoff
> + * information to bridge command when user want to enable FPGA feature in U-boot
> + */
> +#ifndef CONFIG_SPL_BUILD
>  		/* Check signal from FPGA. */
>  		if (!fpgamgr_test_fpga_ready()) {
>  			/* FPGA not ready, do nothing. We allow system to boot
> @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
>  
>  		/* Remap the bridges into memory map */
>  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);

I believe the L3REGS needs to be programmed on Gen5.

> +#endif
>  	}
>  	return;
>  }
> diff --git a/arch/arm/mach-socfpga/system_manager_gen5.c b/arch/arm/mach-socfpga/system_manager_gen5.c
> index 3588a57..ee25496 100644
> --- a/arch/arm/mach-socfpga/system_manager_gen5.c
> +++ b/arch/arm/mach-socfpga/system_manager_gen5.c
> @@ -43,12 +43,6 @@ static void populate_sysmgr_fpgaintf_module(void)
>  	/* populate (not writing) the value for SYSMGR.FPGAINTF.MODULE
>  	based on pinmux setting */
>  	setbits_le32(&sysmgr_regs->iswgrp_handoff[2], handoff_val);
> -
> -	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
> -	if (fpgamgr_test_fpga_ready()) {
> -		/* Enable the required signals only */
> -		writel(handoff_val, &sysmgr_regs->fpgaintfgrp_module);
> -	}
>  }
>  
>  /*
> diff --git a/drivers/ddr/altera/sdram.c b/drivers/ddr/altera/sdram.c
> index e74c5b0..df16102 100644
> --- a/drivers/ddr/altera/sdram.c
> +++ b/drivers/ddr/altera/sdram.c
> @@ -233,6 +233,7 @@ static void sdram_dump_protection_config(void)
>  	}
>  }
>  
> +#ifndef CONFIG_SPL_BUILD
>  /**
>   * sdram_write_verify() - write to register and verify the write.
>   * @addr:	Register address
> @@ -259,6 +260,7 @@ static unsigned sdram_write_verify(const u32 *addr, const u32 val)
>  	debug("correct!\n");
>  	return 0;
>  }
> +#endif
>  
>  /**
>   * sdr_get_ctrlcfg() - Get the value of DRAM CTRLCFG register
> @@ -435,7 +437,6 @@ int sdram_mmr_init_full(unsigned int sdr_phy_reg)
>  	const unsigned int rows =
>  		(cfg->dram_addrw & SDR_CTRLGRP_DRAMADDRW_ROWBITS_MASK) >>
>  			SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB;
> -	int ret;
>  
>  	writel(rows, &sysmgr_regs->iswgrp_handoff[4]);
>  
> @@ -444,13 +445,18 @@ int sdram_mmr_init_full(unsigned int sdr_phy_reg)
>  	/* saving this value to SYSMGR.ISWGRP.HANDOFF.FPGA2SDR */
>  	writel(cfg->fpgaport_rst, &sysmgr_regs->iswgrp_handoff[3]);
>  
> +	/* FPGA feature is not required in SPL, so no test on FPGA reset in SPL */
> +#ifndef CONFIG_SPL_BUILD
>  	/* only enable if the FPGA is programmed */
> +	int ret;
> +
>  	if (fpgamgr_test_fpga_ready()) {
>  		ret = sdram_write_verify(&sdr_ctrl->fpgaport_rst,
>  					 cfg->fpgaport_rst);
>  		if (ret)
>  			return ret;
>  	}
> +#endif
>  
>  	/* Restore the SDR PHY Register if valid */
>  	if (sdr_phy_reg != 0xffffffff)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] arm: socfpga: Enable FPGA bridge in gen5 U-boot

[Marek Vasut]

On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Enable FPGA bridge in gen5 U-boot instead of gen5 SPL because FPGA feature is not
> required in SPL. Remove FPGA feature in SPL can help to save some space.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>  include/configs/socfpga_common.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
> index 9be9e79..f5b3277 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -70,6 +70,10 @@
>  #define CONFIG_CMD_PXE
>  #define CONFIG_MENU
>  
> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> +#define CONFIG_PREBOOT "bridge enable; echo bridge enable"
> +#endif

If someone needs to define their own preboot env, this will break.
If the FPGA is not programmed, this will also break.

>  /*
>   * Cache
>   */
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Chee, Tien Fong]

On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > Remove unused FPGA feature for saving some space in gen5 SPL.
> I really dislike the ifdefery. Why is this patch even needed?
> I thought we had no space problems in the Gen5 SPL?
> 
I can remove those codes within ifdefery because they are no longer
required. I keep them here just for one day we can use if we need to.
Do you remember we have consent to clean up those useless codes for
SPL, all fpga related drivers will be moved into one driver location.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > ---
> >  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
> >  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
> >  drivers/ddr/altera/sdram.c                  |    8 +++++++-
> >  3 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
> > b/arch/arm/mach-socfpga/reset_manager_gen5.c
> > index aa88adb..c954063 100644
> > --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
> > +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
> > @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
> >  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
> >  		writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
> >  
> > +/*
> > + * FPGA feature is not required in SPL, so FPGA bridges release
> > from reset
> > + * should not be supported in SPL, but some FPGA releated setting
> > can be stored
> > + * in handoff registers as SPL to U-boot/OS handoff information.
> > These
> > + * handoff information can be used in later phase such as feeding
> > handoff
> > + * information to bridge command when user want to enable FPGA
> > feature in U-boot
> > + */
> > +#ifndef CONFIG_SPL_BUILD
> >  		/* Check signal from FPGA. */
> >  		if (!fpgamgr_test_fpga_ready()) {
> >  			/* FPGA not ready, do nothing. We allow
> > system to boot
> > @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
> >  
> >  		/* Remap the bridges into memory map */
> >  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
> I believe the L3REGS needs to be programmed on Gen5.
> 
Yes, this is done when calling command "bridge
enable". iswgrp_handoff[1] contains l3mask, which will be written
into SOCFPGA_L3REGS_ADDRESS.
Snippet from misc_gen5.c: 
int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const
argv[])
{
	if (argc != 2)
		return CMD_RET_USAGE;

	argv++;

	switch (*argv[0]) {
	case 'e':	/* Enable */
		writel(iswgrp_handoff[2], &sysmgr_regs-
>fpgaintfgrp_module);
		socfpga_sdram_apply_static_cfg();
		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
		writel(iswgrp_handoff[0], &reset_manager_base-
>brg_mod_reset);
		writel(iswgrp_handoff[1], &nic301_regs->remap);
		break;

> > 
> > +#endif
> >  	}
> >  	return;
> >  }
> > diff --git a/arch/arm/mach-socfpga/system_manager_gen5.c
> > b/arch/arm/mach-socfpga/system_manager_gen5.c
> > index 3588a57..ee25496 100644
> > --- a/arch/arm/mach-socfpga/system_manager_gen5.c
> > +++ b/arch/arm/mach-socfpga/system_manager_gen5.c
> > @@ -43,12 +43,6 @@ static void
> > populate_sysmgr_fpgaintf_module(void)
> >  	/* populate (not writing) the value for
> > SYSMGR.FPGAINTF.MODULE
> >  	based on pinmux setting */
> >  	setbits_le32(&sysmgr_regs->iswgrp_handoff[2],
> > handoff_val);
> > -
> > -	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
> > -	if (fpgamgr_test_fpga_ready()) {
> > -		/* Enable the required signals only */
> > -		writel(handoff_val, &sysmgr_regs-
> > >fpgaintfgrp_module);
> > -	}
> >  }
> >  
> >  /*
> > diff --git a/drivers/ddr/altera/sdram.c
> > b/drivers/ddr/altera/sdram.c
> > index e74c5b0..df16102 100644
> > --- a/drivers/ddr/altera/sdram.c
> > +++ b/drivers/ddr/altera/sdram.c
> > @@ -233,6 +233,7 @@ static void sdram_dump_protection_config(void)
> >  	}
> >  }
> >  
> > +#ifndef CONFIG_SPL_BUILD
> >  /**
> >   * sdram_write_verify() - write to register and verify the write.
> >   * @addr:	Register address
> > @@ -259,6 +260,7 @@ static unsigned sdram_write_verify(const u32
> > *addr, const u32 val)
> >  	debug("correct!\n");
> >  	return 0;
> >  }
> > +#endif
> >  
> >  /**
> >   * sdr_get_ctrlcfg() - Get the value of DRAM CTRLCFG register
> > @@ -435,7 +437,6 @@ int sdram_mmr_init_full(unsigned int
> > sdr_phy_reg)
> >  	const unsigned int rows =
> >  		(cfg->dram_addrw &
> > SDR_CTRLGRP_DRAMADDRW_ROWBITS_MASK) >>
> >  			SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB;
> > -	int ret;
> >  
> >  	writel(rows, &sysmgr_regs->iswgrp_handoff[4]);
> >  
> > @@ -444,13 +445,18 @@ int sdram_mmr_init_full(unsigned int
> > sdr_phy_reg)
> >  	/* saving this value to SYSMGR.ISWGRP.HANDOFF.FPGA2SDR */
> >  	writel(cfg->fpgaport_rst, &sysmgr_regs-
> > >iswgrp_handoff[3]);
> >  
> > +	/* FPGA feature is not required in SPL, so no test on FPGA
> > reset in SPL */
> > +#ifndef CONFIG_SPL_BUILD
> >  	/* only enable if the FPGA is programmed */
> > +	int ret;
> > +
> >  	if (fpgamgr_test_fpga_ready()) {
> >  		ret = sdram_write_verify(&sdr_ctrl->fpgaport_rst,
> >  					 cfg->fpgaport_rst);
> >  		if (ret)
> >  			return ret;
> >  	}
> > +#endif
> >  
> >  	/* Restore the SDR PHY Register if valid */
> >  	if (sdr_phy_reg != 0xffffffff)
> > 
> 

[U-Boot] [PATCH 3/3] arm: socfpga: Enable FPGA bridge in gen5 U-boot

[Chee, Tien Fong]

On Kha, 2017-07-27 at 10:24 +0200, Marek Vasut wrote:
> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > Enable FPGA bridge in gen5 U-boot instead of gen5 SPL because FPGA
> > feature is not
> > required in SPL. Remove FPGA feature in SPL can help to save some
> > space.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > ---
> >  include/configs/socfpga_common.h |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/configs/socfpga_common.h
> > b/include/configs/socfpga_common.h
> > index 9be9e79..f5b3277 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -70,6 +70,10 @@
> >  #define CONFIG_CMD_PXE
> >  #define CONFIG_MENU
> >  
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > +#define CONFIG_PREBOOT "bridge enable; echo bridge enable"
> > +#endif
> If someone needs to define their own preboot env, this will break.
> If the FPGA is not programmed, this will also break.
> 
I think we can add one new config for FPGA is programmed or not.
However, i think this should be as default since most intel fpga
devkits have this feature, but user can edit the environment in code,
or in boot console. I prefer not to hard code this in the code like
what previously doing in gen5. DO you have any better idea?
> > 
> >  /*
> >   * Cache
> >   */
> > 
> 

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Marek Vasut]

On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
> On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
>> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> Remove unused FPGA feature for saving some space in gen5 SPL.
>> I really dislike the ifdefery. Why is this patch even needed?
>> I thought we had no space problems in the Gen5 SPL?
>>
> I can remove those codes within ifdefery because they are no longer
> required.

Why ?

> I keep them here just for one day we can use if we need to.
> Do you remember we have consent to clean up those useless codes for
> SPL, all fpga related drivers will be moved into one driver location.

No, sorry.

>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>> ---
>>>  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
>>>  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
>>>  drivers/ddr/altera/sdram.c                  |    8 +++++++-
>>>  3 files changed, 16 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> index aa88adb..c954063 100644
>>> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>> @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
>>>  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
>>>  		writel(l3mask, &sysmgr_regs->iswgrp_handoff[1]);
>>>  
>>> +/*
>>> + * FPGA feature is not required in SPL, so FPGA bridges release
>>> from reset
>>> + * should not be supported in SPL, but some FPGA releated setting
>>> can be stored
>>> + * in handoff registers as SPL to U-boot/OS handoff information.
>>> These
>>> + * handoff information can be used in later phase such as feeding
>>> handoff
>>> + * information to bridge command when user want to enable FPGA
>>> feature in U-boot
>>> + */
>>> +#ifndef CONFIG_SPL_BUILD
>>>  		/* Check signal from FPGA. */
>>>  		if (!fpgamgr_test_fpga_ready()) {
>>>  			/* FPGA not ready, do nothing. We allow
>>> system to boot
>>> @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
>>>  
>>>  		/* Remap the bridges into memory map */
>>>  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
>> I believe the L3REGS needs to be programmed on Gen5.
>>
> Yes, this is done when calling command "bridge
> enable". iswgrp_handoff[1] contains l3mask, which will be written
> into SOCFPGA_L3REGS_ADDRESS.

I think this needs to be done earlier, otherwise the first 1 MiB or so
of DRAM isn't accessible.

> Snippet from misc_gen5.c: 
> int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> {
> 	if (argc != 2)
> 		return CMD_RET_USAGE;
> 
> 	argv++;
> 
> 	switch (*argv[0]) {
> 	case 'e':	/* Enable */
> 		writel(iswgrp_handoff[2], &sysmgr_regs-
>> fpgaintfgrp_module);
> 		socfpga_sdram_apply_static_cfg();
> 		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
> 		writel(iswgrp_handoff[0], &reset_manager_base-
>> brg_mod_reset);
> 		writel(iswgrp_handoff[1], &nic301_regs->remap);
> 		break;
> 
>>>
>>> +#endif
>>>  	}
>>>  	return;
>>>  }
>>> diff --git a/arch/arm/mach-socfpga/system_manager_gen5.c
>>> b/arch/arm/mach-socfpga/system_manager_gen5.c
>>> index 3588a57..ee25496 100644
>>> --- a/arch/arm/mach-socfpga/system_manager_gen5.c
>>> +++ b/arch/arm/mach-socfpga/system_manager_gen5.c
>>> @@ -43,12 +43,6 @@ static void
>>> populate_sysmgr_fpgaintf_module(void)
>>>  	/* populate (not writing) the value for
>>> SYSMGR.FPGAINTF.MODULE
>>>  	based on pinmux setting */
>>>  	setbits_le32(&sysmgr_regs->iswgrp_handoff[2],
>>> handoff_val);
>>> -
>>> -	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
>>> -	if (fpgamgr_test_fpga_ready()) {
>>> -		/* Enable the required signals only */
>>> -		writel(handoff_val, &sysmgr_regs-
>>>> fpgaintfgrp_module);
>>> -	}
>>>  }
>>>  
>>>  /*
>>> diff --git a/drivers/ddr/altera/sdram.c
>>> b/drivers/ddr/altera/sdram.c
>>> index e74c5b0..df16102 100644
>>> --- a/drivers/ddr/altera/sdram.c
>>> +++ b/drivers/ddr/altera/sdram.c
>>> @@ -233,6 +233,7 @@ static void sdram_dump_protection_config(void)
>>>  	}
>>>  }
>>>  
>>> +#ifndef CONFIG_SPL_BUILD
>>>  /**
>>>   * sdram_write_verify() - write to register and verify the write.
>>>   * @addr:	Register address
>>> @@ -259,6 +260,7 @@ static unsigned sdram_write_verify(const u32
>>> *addr, const u32 val)
>>>  	debug("correct!\n");
>>>  	return 0;
>>>  }
>>> +#endif
>>>  
>>>  /**
>>>   * sdr_get_ctrlcfg() - Get the value of DRAM CTRLCFG register
>>> @@ -435,7 +437,6 @@ int sdram_mmr_init_full(unsigned int
>>> sdr_phy_reg)
>>>  	const unsigned int rows =
>>>  		(cfg->dram_addrw &
>>> SDR_CTRLGRP_DRAMADDRW_ROWBITS_MASK) >>
>>>  			SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB;
>>> -	int ret;
>>>  
>>>  	writel(rows, &sysmgr_regs->iswgrp_handoff[4]);
>>>  
>>> @@ -444,13 +445,18 @@ int sdram_mmr_init_full(unsigned int
>>> sdr_phy_reg)
>>>  	/* saving this value to SYSMGR.ISWGRP.HANDOFF.FPGA2SDR */
>>>  	writel(cfg->fpgaport_rst, &sysmgr_regs-
>>>> iswgrp_handoff[3]);
>>>  
>>> +	/* FPGA feature is not required in SPL, so no test on FPGA
>>> reset in SPL */
>>> +#ifndef CONFIG_SPL_BUILD
>>>  	/* only enable if the FPGA is programmed */
>>> +	int ret;
>>> +
>>>  	if (fpgamgr_test_fpga_ready()) {
>>>  		ret = sdram_write_verify(&sdr_ctrl->fpgaport_rst,
>>>  					 cfg->fpgaport_rst);
>>>  		if (ret)
>>>  			return ret;
>>>  	}
>>> +#endif
>>>  
>>>  	/* Restore the SDR PHY Register if valid */
>>>  	if (sdr_phy_reg != 0xffffffff)
>>>


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] arm: socfpga: Enable FPGA bridge in gen5 U-boot

[Marek Vasut]

On 07/27/2017 11:26 AM, Chee, Tien Fong wrote:
> On Kha, 2017-07-27 at 10:24 +0200, Marek Vasut wrote:
>> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> Enable FPGA bridge in gen5 U-boot instead of gen5 SPL because FPGA
>>> feature is not
>>> required in SPL. Remove FPGA feature in SPL can help to save some
>>> space.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>> ---
>>>  include/configs/socfpga_common.h |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/configs/socfpga_common.h
>>> b/include/configs/socfpga_common.h
>>> index 9be9e79..f5b3277 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -70,6 +70,10 @@
>>>  #define CONFIG_CMD_PXE
>>>  #define CONFIG_MENU
>>>  
>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>> +#define CONFIG_PREBOOT "bridge enable; echo bridge enable"
>>> +#endif
>> If someone needs to define their own preboot env, this will break.
>> If the FPGA is not programmed, this will also break.
>>
> I think we can add one new config for FPGA is programmed or not.

No

> However, i think this should be as default since most intel fpga
> devkits have this feature, but user can edit the environment in code,
> or in boot console.

Well if this hangs the kit, he cant. Most users of socfpga don't use the
devkits, but rather their own device.

> I prefer not to hard code this in the code like
> what previously doing in gen5. DO you have any better idea?
I think there's a reason for that code, see my reply to 1/3.
It has to do with the remapping of first 1 MiB of RAM on the Gen5.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Chee, Tien Fong]

On Kha, 2017-07-27 at 11:37 +0200, Marek Vasut wrote:
> On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
> > 
> > On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
> > > 
> > > On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > Remove unused FPGA feature for saving some space in gen5 SPL.
> > > I really dislike the ifdefery. Why is this patch even needed?
> > > I thought we had no space problems in the Gen5 SPL?
> > > 
> > I can remove those codes within ifdefery because they are no longer
> > required.
> Why ?
> 
because those functions are testing on FPGA when the bridge is enabled
in SPL. But, i will keep minimal ifdefery on socfpga_bridges_reset to
indicate the fpga bridges should not be released in SPL.
> > 
> > I keep them here just for one day we can use if we need to.
> > Do you remember we have consent to clean up those useless codes for
> > SPL, all fpga related drivers will be moved into one driver
> > location.
> No, sorry.
> 
Are you disagree on keeping the ifdefery codes, or disagree on moving
all FPGA related functions into drivers/fpga/... ?
> > 
> > > 
> > > > 
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > ---
> > > >  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
> > > >  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
> > > >  drivers/ddr/altera/sdram.c                  |    8 +++++++-
> > > >  3 files changed, 16 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > b/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > index aa88adb..c954063 100644
> > > > --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
> > > >  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
> > > >  		writel(l3mask, &sysmgr_regs-
> > > > >iswgrp_handoff[1]);
> > > >  
> > > > +/*
> > > > + * FPGA feature is not required in SPL, so FPGA bridges
> > > > release
> > > > from reset
> > > > + * should not be supported in SPL, but some FPGA releated
> > > > setting
> > > > can be stored
> > > > + * in handoff registers as SPL to U-boot/OS handoff
> > > > information.
> > > > These
> > > > + * handoff information can be used in later phase such as
> > > > feeding
> > > > handoff
> > > > + * information to bridge command when user want to enable FPGA
> > > > feature in U-boot
> > > > + */
> > > > +#ifndef CONFIG_SPL_BUILD
> > > >  		/* Check signal from FPGA. */
> > > >  		if (!fpgamgr_test_fpga_ready()) {
> > > >  			/* FPGA not ready, do nothing. We
> > > > allow
> > > > system to boot
> > > > @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
> > > >  
> > > >  		/* Remap the bridges into memory map */
> > > >  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
> > > I believe the L3REGS needs to be programmed on Gen5.
> > > 
> > Yes, this is done when calling command "bridge
> > enable". iswgrp_handoff[1] contains l3mask, which will be written
> > into SOCFPGA_L3REGS_ADDRESS.
> I think this needs to be done earlier, otherwise the first 1 MiB or
> so
> of DRAM isn't accessible.
> 
Yeah, you are right....this is what i missing, the OCRAM should be
remap to lowest memory region 1 MiB. So i propose just to remap OCRAM,
and FPGA related bridges can be remaped in U-boot by calling the
"enable bridge" command.
> > 
> > Snippet from misc_gen5.c: 
> > int do_bridge(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > argv[])
> > {
> > 	if (argc != 2)
> > 		return CMD_RET_USAGE;
> > 
> > 	argv++;
> > 
> > 	switch (*argv[0]) {
> > 	case 'e':	/* Enable */
> > 		writel(iswgrp_handoff[2], &sysmgr_regs-
> > > 
> > > fpgaintfgrp_module);
> > 		socfpga_sdram_apply_static_cfg();
> > 		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
> > 		writel(iswgrp_handoff[0], &reset_manager_base-
> > > 
> > > brg_mod_reset);
> > 		writel(iswgrp_handoff[1], &nic301_regs->remap);
> > 		break;
> > 
> > > 
> > > > 
> > > > 
> > > > +#endif
> > > >  	}
> > > >  	return;
> > > >  }
> > > > diff --git a/arch/arm/mach-socfpga/system_manager_gen5.c
> > > > b/arch/arm/mach-socfpga/system_manager_gen5.c
> > > > index 3588a57..ee25496 100644
> > > > --- a/arch/arm/mach-socfpga/system_manager_gen5.c
> > > > +++ b/arch/arm/mach-socfpga/system_manager_gen5.c
> > > > @@ -43,12 +43,6 @@ static void
> > > > populate_sysmgr_fpgaintf_module(void)
> > > >  	/* populate (not writing) the value for
> > > > SYSMGR.FPGAINTF.MODULE
> > > >  	based on pinmux setting */
> > > >  	setbits_le32(&sysmgr_regs->iswgrp_handoff[2],
> > > > handoff_val);
> > > > -
> > > > -	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
> > > > -	if (fpgamgr_test_fpga_ready()) {
> > > > -		/* Enable the required signals only */
> > > > -		writel(handoff_val, &sysmgr_regs-
> > > > > 
> > > > > fpgaintfgrp_module);
> > > > -	}
> > > >  }
> > > >  
> > > >  /*
> > > > diff --git a/drivers/ddr/altera/sdram.c
> > > > b/drivers/ddr/altera/sdram.c
> > > > index e74c5b0..df16102 100644
> > > > --- a/drivers/ddr/altera/sdram.c
> > > > +++ b/drivers/ddr/altera/sdram.c
> > > > @@ -233,6 +233,7 @@ static void
> > > > sdram_dump_protection_config(void)
> > > >  	}
> > > >  }
> > > >  
> > > > +#ifndef CONFIG_SPL_BUILD
> > > >  /**
> > > >   * sdram_write_verify() - write to register and verify the
> > > > write.
> > > >   * @addr:	Register address
> > > > @@ -259,6 +260,7 @@ static unsigned sdram_write_verify(const
> > > > u32
> > > > *addr, const u32 val)
> > > >  	debug("correct!\n");
> > > >  	return 0;
> > > >  }
> > > > +#endif
> > > >  
> > > >  /**
> > > >   * sdr_get_ctrlcfg() - Get the value of DRAM CTRLCFG register
> > > > @@ -435,7 +437,6 @@ int sdram_mmr_init_full(unsigned int
> > > > sdr_phy_reg)
> > > >  	const unsigned int rows =
> > > >  		(cfg->dram_addrw &
> > > > SDR_CTRLGRP_DRAMADDRW_ROWBITS_MASK) >>
> > > >  			SDR_CTRLGRP_DRAMADDRW_ROWBITS_LSB;
> > > > -	int ret;
> > > >  
> > > >  	writel(rows, &sysmgr_regs->iswgrp_handoff[4]);
> > > >  
> > > > @@ -444,13 +445,18 @@ int sdram_mmr_init_full(unsigned int
> > > > sdr_phy_reg)
> > > >  	/* saving this value to SYSMGR.ISWGRP.HANDOFF.FPGA2SDR
> > > > */
> > > >  	writel(cfg->fpgaport_rst, &sysmgr_regs-
> > > > > 
> > > > > iswgrp_handoff[3]);
> > > >  
> > > > +	/* FPGA feature is not required in SPL, so no test on
> > > > FPGA
> > > > reset in SPL */
> > > > +#ifndef CONFIG_SPL_BUILD
> > > >  	/* only enable if the FPGA is programmed */
> > > > +	int ret;
> > > > +
> > > >  	if (fpgamgr_test_fpga_ready()) {
> > > >  		ret = sdram_write_verify(&sdr_ctrl-
> > > > >fpgaport_rst,
> > > >  					 cfg->fpgaport_rst);
> > > >  		if (ret)
> > > >  			return ret;
> > > >  	}
> > > > +#endif
> > > >  
> > > >  	/* Restore the SDR PHY Register if valid */
> > > >  	if (sdr_phy_reg != 0xffffffff)
> > > > 
> 

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Marek Vasut]

On 07/28/2017 07:16 AM, Chee, Tien Fong wrote:
> On Kha, 2017-07-27 at 11:37 +0200, Marek Vasut wrote:
>> On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
>>>
>>> On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
>>>>
>>>> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
>>>>>
>>>>>
>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>
>>>>> Remove unused FPGA feature for saving some space in gen5 SPL.
>>>> I really dislike the ifdefery. Why is this patch even needed?
>>>> I thought we had no space problems in the Gen5 SPL?
>>>>
>>> I can remove those codes within ifdefery because they are no longer
>>> required.
>> Why ?
>>
> because those functions are testing on FPGA when the bridge is enabled
> in SPL.

That's what those functions do, it doesn't answer my question why
they're no longer needed.

> But, i will keep minimal ifdefery on socfpga_bridges_reset to
> indicate the fpga bridges should not be released in SPL.
>>>
>>> I keep them here just for one day we can use if we need to.
>>> Do you remember we have consent to clean up those useless codes for
>>> SPL, all fpga related drivers will be moved into one driver
>>> location.
>> No, sorry.
>>
> Are you disagree on keeping the ifdefery codes, or disagree on moving
> all FPGA related functions into drivers/fpga/... ?

I dislike the ifdeffery.

>>>
>>>>
>>>>>
>>>>>
>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>> ---
>>>>>  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
>>>>>  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
>>>>>  drivers/ddr/altera/sdram.c                  |    8 +++++++-
>>>>>  3 files changed, 16 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> index aa88adb..c954063 100644
>>>>> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
>>>>>  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
>>>>>  		writel(l3mask, &sysmgr_regs-
>>>>>> iswgrp_handoff[1]);
>>>>>  
>>>>> +/*
>>>>> + * FPGA feature is not required in SPL, so FPGA bridges
>>>>> release
>>>>> from reset
>>>>> + * should not be supported in SPL, but some FPGA releated
>>>>> setting
>>>>> can be stored
>>>>> + * in handoff registers as SPL to U-boot/OS handoff
>>>>> information.
>>>>> These
>>>>> + * handoff information can be used in later phase such as
>>>>> feeding
>>>>> handoff
>>>>> + * information to bridge command when user want to enable FPGA
>>>>> feature in U-boot
>>>>> + */
>>>>> +#ifndef CONFIG_SPL_BUILD
>>>>>  		/* Check signal from FPGA. */
>>>>>  		if (!fpgamgr_test_fpga_ready()) {
>>>>>  			/* FPGA not ready, do nothing. We
>>>>> allow
>>>>> system to boot
>>>>> @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
>>>>>  
>>>>>  		/* Remap the bridges into memory map */
>>>>>  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
>>>> I believe the L3REGS needs to be programmed on Gen5.
>>>>
>>> Yes, this is done when calling command "bridge
>>> enable". iswgrp_handoff[1] contains l3mask, which will be written
>>> into SOCFPGA_L3REGS_ADDRESS.
>> I think this needs to be done earlier, otherwise the first 1 MiB or
>> so
>> of DRAM isn't accessible.
>>
> Yeah, you are right....this is what i missing, the OCRAM should be
> remap to lowest memory region 1 MiB. So i propose just to remap OCRAM,
> and FPGA related bridges can be remaped in U-boot by calling the
> "enable bridge" command.

So basically the user would need to run this random command to fix his
obscure memory issues with the first 1 MiB ? Unacceptable, U-Boot does
this and this behavior will be retained. There's no way such a change of
behavior can be let in.

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Chee, Tien Fong]

On Jum, 2017-07-28 at 09:52 +0200, Marek Vasut wrote:
> On 07/28/2017 07:16 AM, Chee, Tien Fong wrote:
> > 
> > On Kha, 2017-07-27 at 11:37 +0200, Marek Vasut wrote:
> > > 
> > > On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > 
> > > > > > Remove unused FPGA feature for saving some space in gen5
> > > > > > SPL.
> > > > > I really dislike the ifdefery. Why is this patch even needed?
> > > > > I thought we had no space problems in the Gen5 SPL?
> > > > > 
> > > > I can remove those codes within ifdefery because they are no
> > > > longer
> > > > required.
> > > Why ?
> > > 
> > because those functions are testing on FPGA when the bridge is
> > enabled
> > in SPL.
> That's what those functions do, it doesn't answer my question why
> they're no longer needed.
> 
It is because fpga is disabled in SPL, hence those fpga checking
routine can be removed. For example init sdram mmr running before
calling command "enable fpga", then we can safely
remove pgamgr_test_fpga_ready(). But, drivers are shared between SPL
and U-boot, we still need minimal ifdefery in socfpga_bridges_reset.
> > 
> > But, i will keep minimal ifdefery on socfpga_bridges_reset to
> > indicate the fpga bridges should not be released in SPL.
> > > 
> > > > 
> > > > 
> > > > I keep them here just for one day we can use if we need to.
> > > > Do you remember we have consent to clean up those useless codes
> > > > for
> > > > SPL, all fpga related drivers will be moved into one driver
> > > > location.
> > > No, sorry.
> > > 
> > Are you disagree on keeping the ifdefery codes, or disagree on
> > moving
> > all FPGA related functions into drivers/fpga/... ?
> I dislike the ifdeffery.
> 
DO you have any better idea on this common function shared between SPL
and U-boot?
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > ---
> > > > > >  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9
> > > > > > +++++++++
> > > > > >  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
> > > > > >  drivers/ddr/altera/sdram.c                  |    8
> > > > > > +++++++-
> > > > > >  3 files changed, 16 insertions(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > > > b/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > > > index aa88adb..c954063 100644
> > > > > > --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > > > +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
> > > > > > @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
> > > > > >  		writel(0, &sysmgr_regs-
> > > > > > >iswgrp_handoff[0]);
> > > > > >  		writel(l3mask, &sysmgr_regs-
> > > > > > > 
> > > > > > > iswgrp_handoff[1]);
> > > > > >  
> > > > > > +/*
> > > > > > + * FPGA feature is not required in SPL, so FPGA bridges
> > > > > > release
> > > > > > from reset
> > > > > > + * should not be supported in SPL, but some FPGA releated
> > > > > > setting
> > > > > > can be stored
> > > > > > + * in handoff registers as SPL to U-boot/OS handoff
> > > > > > information.
> > > > > > These
> > > > > > + * handoff information can be used in later phase such as
> > > > > > feeding
> > > > > > handoff
> > > > > > + * information to bridge command when user want to enable
> > > > > > FPGA
> > > > > > feature in U-boot
> > > > > > + */
> > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > >  		/* Check signal from FPGA. */
> > > > > >  		if (!fpgamgr_test_fpga_ready()) {
> > > > > >  			/* FPGA not ready, do nothing. We
> > > > > > allow
> > > > > > system to boot
> > > > > > @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
> > > > > >  
> > > > > >  		/* Remap the bridges into memory map */
> > > > > >  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
> > > > > I believe the L3REGS needs to be programmed on Gen5.
> > > > > 
> > > > Yes, this is done when calling command "bridge
> > > > enable". iswgrp_handoff[1] contains l3mask, which will be
> > > > written
> > > > into SOCFPGA_L3REGS_ADDRESS.
> > > I think this needs to be done earlier, otherwise the first 1 MiB
> > > or
> > > so
> > > of DRAM isn't accessible.
> > > 
> > Yeah, you are right....this is what i missing, the OCRAM should be
> > remap to lowest memory region 1 MiB. So i propose just to remap
> > OCRAM,
> > and FPGA related bridges can be remaped in U-boot by calling the
> > "enable bridge" command.
> So basically the user would need to run this random command to fix
> his
> obscure memory issues with the first 1 MiB ? Unacceptable, U-Boot
> does
> this and this behavior will be retained. There's no way such a change
> of
> behavior can be let in.
> 
The only remap first 1 MiB required to L3 is OCRAM. Hence, i propose we
only keep OCRAM remap at here. Any FPGA related bridges visibility to
L3 master can be set visible on L3 memory map when we require accessing
to FPGA or calling "enable bridge".

Chin Liang, Could you help to advice on these?
> [...]
> 

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Marek Vasut]

On 07/28/2017 01:35 PM, Chee, Tien Fong wrote:
> On Jum, 2017-07-28 at 09:52 +0200, Marek Vasut wrote:
>> On 07/28/2017 07:16 AM, Chee, Tien Fong wrote:
>>>
>>> On Kha, 2017-07-27 at 11:37 +0200, Marek Vasut wrote:
>>>>
>>>> On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>
>>>>>>> Remove unused FPGA feature for saving some space in gen5
>>>>>>> SPL.
>>>>>> I really dislike the ifdefery. Why is this patch even needed?
>>>>>> I thought we had no space problems in the Gen5 SPL?
>>>>>>
>>>>> I can remove those codes within ifdefery because they are no
>>>>> longer
>>>>> required.
>>>> Why ?
>>>>
>>> because those functions are testing on FPGA when the bridge is
>>> enabled
>>> in SPL.
>> That's what those functions do, it doesn't answer my question why
>> they're no longer needed.
>>
> It is because fpga is disabled in SPL, hence those fpga checking
> routine can be removed. For example init sdram mmr running before
> calling command "enable fpga", then we can safely
> remove pgamgr_test_fpga_ready(). But, drivers are shared between SPL
> and U-boot, we still need minimal ifdefery in socfpga_bridges_reset.

What if the FPGA starts before the HPS and is loaded from ie. EPCQ ?

>>>
>>> But, i will keep minimal ifdefery on socfpga_bridges_reset to
>>> indicate the fpga bridges should not be released in SPL.
>>>>
>>>>>
>>>>>
>>>>> I keep them here just for one day we can use if we need to.
>>>>> Do you remember we have consent to clean up those useless codes
>>>>> for
>>>>> SPL, all fpga related drivers will be moved into one driver
>>>>> location.
>>>> No, sorry.
>>>>
>>> Are you disagree on keeping the ifdefery codes, or disagree on
>>> moving
>>> all FPGA related functions into drivers/fpga/... ?
>> I dislike the ifdeffery.
>>
> DO you have any better idea on this common function shared between SPL
> and U-boot?

[...]

I'm not really sure you can remove them from the SPL, see above.

>>> Yeah, you are right....this is what i missing, the OCRAM should be
>>> remap to lowest memory region 1 MiB. So i propose just to remap
>>> OCRAM,
>>> and FPGA related bridges can be remaped in U-boot by calling the
>>> "enable bridge" command.
>> So basically the user would need to run this random command to fix
>> his
>> obscure memory issues with the first 1 MiB ? Unacceptable, U-Boot
>> does
>> this and this behavior will be retained. There's no way such a change
>> of
>> behavior can be let in.
>>
> The only remap first 1 MiB required to L3 is OCRAM. Hence, i propose we
> only keep OCRAM remap at here. Any FPGA related bridges visibility to
> L3 master can be set visible on L3 memory map when we require accessing
> to FPGA or calling "enable bridge".
> 
> Chin Liang, Could you help to advice on these?
>> [...]


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Chee, Tien Fong]

On Jum, 2017-07-28 at 13:40 +0200, Marek Vasut wrote:
> On 07/28/2017 01:35 PM, Chee, Tien Fong wrote:
> > 
> > On Jum, 2017-07-28 at 09:52 +0200, Marek Vasut wrote:
> > > 
> > > On 07/28/2017 07:16 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Kha, 2017-07-27 at 11:37 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > > 
> > > > > > > > Remove unused FPGA feature for saving some space in
> > > > > > > > gen5
> > > > > > > > SPL.
> > > > > > > I really dislike the ifdefery. Why is this patch even
> > > > > > > needed?
> > > > > > > I thought we had no space problems in the Gen5 SPL?
> > > > > > > 
> > > > > > I can remove those codes within ifdefery because they are
> > > > > > no
> > > > > > longer
> > > > > > required.
> > > > > Why ?
> > > > > 
> > > > because those functions are testing on FPGA when the bridge is
> > > > enabled
> > > > in SPL.
> > > That's what those functions do, it doesn't answer my question why
> > > they're no longer needed.
> > > 
> > It is because fpga is disabled in SPL, hence those fpga checking
> > routine can be removed. For example init sdram mmr running before
> > calling command "enable fpga", then we can safely
> > remove pgamgr_test_fpga_ready(). But, drivers are shared between
> > SPL
> > and U-boot, we still need minimal ifdefery in
> > socfpga_bridges_reset.
> What if the FPGA starts before the HPS and is loaded from ie. EPCQ ?
> 
On an FPGA Boot the BootROM checks to see that the INIT_DONE bit from
the FPGA is set. It reads the bit twice and the bit must be set for
both reads. Once the INIT_DONE bit has been checked the BootROM maps
the FPGA into the HPS address space. The BootROM code will then take
the H2F AXI Bridge out of reset. So, the impact of changes i made are
no pinmux of fpga interface update from handoff and also no functions
checking on FPGA interface in SPL pahse. Because no functions checking
on FPGA interface in SPL, which are located in arch/arm/mach-
socpfga/fpga_manager.c, so i can merge fpga_manager.c into
drivers/fpga/socfpga_gen5.
> > 
> > > 
> > > > 
> > > > 
> > > > But, i will keep minimal ifdefery on socfpga_bridges_reset to
> > > > indicate the fpga bridges should not be released in SPL.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > I keep them here just for one day we can use if we need to.
> > > > > > Do you remember we have consent to clean up those useless
> > > > > > codes
> > > > > > for
> > > > > > SPL, all fpga related drivers will be moved into one driver
> > > > > > location.
> > > > > No, sorry.
> > > > > 
> > > > Are you disagree on keeping the ifdefery codes, or disagree on
> > > > moving
> > > > all FPGA related functions into drivers/fpga/... ?
> > > I dislike the ifdeffery.
> > > 
> > DO you have any better idea on this common function shared between
> > SPL
> > and U-boot?
> [...]
> 
> I'm not really sure you can remove them from the SPL, see above.
> 
I found out OCRAM is already remap and FPGA intefaces is set visible on
L3 memory map by function socfpga_nic301_slave_ns in very early of
spl.c . So, it is safe for above changes.
> > 
> > > 
> > > > 
> > > > Yeah, you are right....this is what i missing, the OCRAM should
> > > > be
> > > > remap to lowest memory region 1 MiB. So i propose just to remap
> > > > OCRAM,
> > > > and FPGA related bridges can be remaped in U-boot by calling
> > > > the
> > > > "enable bridge" command.
> > > So basically the user would need to run this random command to
> > > fix
> > > his
> > > obscure memory issues with the first 1 MiB ? Unacceptable, U-Boot
> > > does
> > > this and this behavior will be retained. There's no way such a
> > > change
> > > of
> > > behavior can be let in.
> > > 
> > The only remap first 1 MiB required to L3 is OCRAM. Hence, i
> > propose we
> > only keep OCRAM remap at here. Any FPGA related bridges visibility
> > to
> > L3 master can be set visible on L3 memory map when we require
> > accessing
> > to FPGA or calling "enable bridge".
> > 
> > Chin Liang, Could you help to advice on these?
> > > 
> > > [...]
> 
I propose to keep this patchset as open discussion, with more people
included especially input from chin liang(gen5 owener). The purpose of
this patch set is to keep SPL on HW critial intialization and sdram
init setting only, remove useless feature(no FPGA pinmux update and
function checking on FPGA in SPL), cleaning up FPGA driver
code(fpga_manager.c) for gen5 by moving them into one location
/drivers/fpga. All FPGA driver should be enabled by one control
CONFIG_SPL_FPGA_SUPPORT.

What do you guys think?

Thanks.