Remove unused FPGA feature from gen5 SPL

From: Tien Fong Chee <[email protected]>

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

Signed-off-by Tien Fong Chee <[email protected]>

---
 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(-)

arch/arm/mach-socfpga/reset_manager_gen5.c +8 -0

index aa88adb..c954063 100644
@@ -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

arch/arm/mach-socfpga/reset_manager_gen5.c +1 -0

+#endif
 	}
 	return;
 }

arch/arm/mach-socfpga/system_manager_gen5.c +0 -6

index 3588a57..ee25496 100644
@@ -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);
-	}
 }
 
 /*

drivers/ddr/altera/sdram.c +7 -1

index e74c5b0..df16102 100644
@@ -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)

Quoted 4 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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?

Quoted 31 lines

Signed-off-by Tien Fong Chee <[email protected]>

---
 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(-)

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

index aa88adb..c954063 100644
@@ -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. */

Quoted 7 lines

  if (!fpgamgr_test_fpga_ready()) {
  /* FPGA not ready, do nothing. We allow system to boot

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

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

fpgaintfgrp_module);

brg_mod_reset);

Quoted 82 lines

+#endif
  }
  return;
 }

arch/arm/mach-socfpga/system_manager_gen5.c +0 -0

index 3588a57..ee25496 100644
@@ -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);

arch/arm/mach-socfpga/system_manager_gen5.c +0 -5

-
-	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
-	if (fpgamgr_test_fpga_ready()) {
-		/* Enable the required signals only */
-		writel(handoff_val, &sysmgr_regs-

Quoted 1 line

fpgaintfgrp_module);

arch/arm/mach-socfpga/system_manager_gen5.c +0 -1

-	}

 }

 /*

drivers/ddr/altera/sdram.c +0 -0

index e74c5b0..df16102 100644
@@ -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

drivers/ddr/altera/sdram.c +1 -0

@@ -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

drivers/ddr/altera/sdram.c +0 -0

@@ -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;

drivers/ddr/altera/sdram.c +0 -1

-	int ret;

  writel(rows, &sysmgr_regs->iswgrp_handoff[4]);

drivers/ddr/altera/sdram.c +0 -0

@@ -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-

Quoted 1 line

iswgrp_handoff[3]);

drivers/ddr/altera/sdram.c +1 -0

+	/* 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 */

drivers/ddr/altera/sdram.c +2 -0

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

Quoted 8 lines · Marek Vasut

Quoted 4 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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.

Quoted 36 lines

Quoted 36 lines

Signed-off-by Tien Fong Chee <[email protected]>

---
 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(-)

arch/arm/mach-socfpga/reset_manager_gen5.c +2 -0

index aa88adb..c954063 100644
@@ -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
+ */

arch/arm/mach-socfpga/reset_manager_gen5.c +1 -0

+#ifndef CONFIG_SPL_BUILD
 		/* Check signal from FPGA. */
 		if (!fpgamgr_test_fpga_ready()) {
 			/* FPGA not ready, do nothing. We allow

system to boot

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

@@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
 

Quoted 4 lines

arch/arm/mach-socfpga/reset_manager_gen5.c void socfpga_bridges_reset(int enable) · line 120

Quoted 2 lines

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

 		/* 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-

Quoted 1 line

fpgaintfgrp_module);

		socfpga_sdram_apply_static_cfg();
		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
		writel(iswgrp_handoff[0], &reset_manager_base-

Quoted 1 line

brg_mod_reset);

		writel(iswgrp_handoff[1], &nic301_regs->remap);
		break;

Quoted 82 lines

Quoted 82 lines

arch/arm/mach-socfpga/reset_manager_gen5.c +1 -0

+#endif
 	}
 	return;
 }

arch/arm/mach-socfpga/system_manager_gen5.c +0 -0

index 3588a57..ee25496 100644
@@ -43,12 +43,6 @@ static void
 populate_sysmgr_fpgaintf_module(void)
 	/* populate (not writing) the value for

SYSMGR.FPGAINTF.MODULE

arch/arm/mach-socfpga/system_manager_gen5.c +0 -0

 	based on pinmux setting */
 	setbits_le32(&sysmgr_regs->iswgrp_handoff[2],

handoff_val);

arch/arm/mach-socfpga/system_manager_gen5.c +0 -5

-
-	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
-	if (fpgamgr_test_fpga_ready()) {
-		/* Enable the required signals only */
-		writel(handoff_val, &sysmgr_regs-

Quoted 1 line

fpgaintfgrp_module);

arch/arm/mach-socfpga/system_manager_gen5.c +0 -1

-	}
 }
 
 /*

drivers/ddr/altera/sdram.c +2 -0

index e74c5b0..df16102 100644
@@ -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

drivers/ddr/altera/sdram.c +0 -0

@@ -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) >>

drivers/ddr/altera/sdram.c +1 -1

 			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 */

drivers/ddr/altera/sdram.c +4 -0

+#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)

Quoted 8 lines · Tien Fong

Quoted 6 lines · Marek Vasut

Quoted 4 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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 ?

Quoted 5 lines

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.

Quoted 49 lines

Quoted 49 lines

Quoted 44 lines

Signed-off-by Tien Fong Chee <[email protected]>

---
 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(-)

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

index aa88adb..c954063 100644
@@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);

  writel(l3mask, &sysmgr_regs-

Quoted 1 line

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

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

@@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)

Quoted 3 lines

  /* Remap the bridges into memory map */ writel(l3mask, SOCFPGA_L3REGS_ADDRESS);

I believe the L3REGS needs to be programmed on Gen5.

Quoted 3 lines

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.

Quoted 115 lines

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-

Quoted 2 lines

fpgaintfgrp_module);

		socfpga_sdram_apply_static_cfg();
		writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
		writel(iswgrp_handoff[0], &reset_manager_base-

Quoted 2 lines

brg_mod_reset);

		writel(iswgrp_handoff[1], &nic301_regs->remap);
		break;

Quoted 92 lines

Quoted 92 lines

+#endif
  }
  return;
 }

arch/arm/mach-socfpga/system_manager_gen5.c +0 -0

index 3588a57..ee25496 100644
@@ -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);

arch/arm/mach-socfpga/system_manager_gen5.c +0 -5

-
-	handoff_val = readl(&sysmgr_regs->iswgrp_handoff[2]);
-	if (fpgamgr_test_fpga_ready()) {
-		/* Enable the required signals only */
-		writel(handoff_val, &sysmgr_regs-

Quoted 2 lines

fpgaintfgrp_module);

arch/arm/mach-socfpga/system_manager_gen5.c +0 -1

-	}

 }

 /*

drivers/ddr/altera/sdram.c +0 -0

index e74c5b0..df16102 100644
@@ -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

drivers/ddr/altera/sdram.c +0 -0

@@ -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

drivers/ddr/altera/sdram.c +0 -0

@@ -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;

drivers/ddr/altera/sdram.c +0 -1

-	int ret;

  writel(rows, &sysmgr_regs->iswgrp_handoff[4]);

drivers/ddr/altera/sdram.c +0 -0

@@ -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-

Quoted 2 lines

iswgrp_handoff[3]);

drivers/ddr/altera/sdram.c +1 -0

+	/* 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 */

drivers/ddr/altera/sdram.c +2 -0

+	int ret;
+

  if (fpgamgr_test_fpga_ready()) { ret = sdram_write_verify(&sdr_ctrl-

Quoted 1 line

fpgaport_rst,

   cfg->fpgaport_rst); if (ret)
  return ret;
  }
+#endif

  /* Restore the SDR PHY Register if valid */ if (sdr_phy_reg != 0xffffffff)

Quoted 13 lines · Marek Vasut

Quoted 12 lines · Tien Fong

Quoted 10 lines · Marek Vasut

Quoted 5 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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?

Quoted 4 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

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/... ?

Quoted 52 lines

Quoted 52 lines

Quoted 45 lines

Quoted 45 lines

Signed-off-by Tien Fong Chee <[email protected]>

---
 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(-)

arch/arm/mach-socfpga/reset_manager_gen5.c +2 -0

index aa88adb..c954063 100644
@@ -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
+ */

arch/arm/mach-socfpga/reset_manager_gen5.c +1 -0

+#ifndef CONFIG_SPL_BUILD
 		/* Check signal from FPGA. */
 		if (!fpgamgr_test_fpga_ready()) {
 			/* FPGA not ready, do nothing. We

allow
system to boot

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

@@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
 

Quoted 4 lines

arch/arm/mach-socfpga/reset_manager_gen5.c void socfpga_bridges_reset(int enable) · line 120

Quoted 2 lines

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

 		/* 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.

Quoted 4 lines

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.

Quoted 23 lines · Tien Fong

Quoted 21 lines · Marek Vasut

Quoted 19 lines · Tien Fong

Quoted 6 lines · Marek Vasut

Quoted 3 lines · tien.fong.chee at intel.com

Quoted 7 lines

Quoted 4 lines

From: Tien Fong Chee <[email protected]>

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 11 lines

But, i will keep minimal ifdefery on socfpga_bridges_reset to indicate the fpga bridges should not be released in SPL.

Quoted 8 lines

Quoted 8 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 68 lines

Quoted 59 lines

Quoted 59 lines

Quoted 59 lines

Quoted 54 lines

Signed-off-by Tien Fong Chee <[email protected]>

---
 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(-)

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

index aa88adb..c954063 100644
@@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)

  writel(0, &sysmgr_regs-

Quoted 1 line

iswgrp_handoff[0]);

  writel(l3mask, &sysmgr_regs-

Quoted 2 lines

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

arch/arm/mach-socfpga/reset_manager_gen5.c +0 -0

@@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)

Quoted 3 lines

  /* Remap the bridges into memory map */ writel(l3mask, SOCFPGA_L3REGS_ADDRESS);

I believe the L3REGS needs to be programmed on Gen5.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

[...]

Quoted 29 lines · Marek Vasut

Quoted 28 lines · Tien Fong

Quoted 15 lines · Marek Vasut

Quoted 13 lines · Tien Fong

Quoted 10 lines · Marek Vasut

Quoted 7 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 13 lines

Quoted 13 lines

But, i will keep minimal ifdefery on socfpga_bridges_reset to indicate the fpga bridges should not be released in SPL.

Quoted 10 lines

Quoted 8 lines

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.

Quoted 5 lines

Quoted 3 lines

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?

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

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?

Quoted 1 line

[...]

Quoted 43 lines · Tien Fong

Quoted 41 lines · Marek Vasut

Quoted 31 lines · Tien Fong

Quoted 28 lines · Marek Vasut

Quoted 25 lines · Tien Fong

Quoted 8 lines · Marek Vasut

Quoted 4 lines · tien.fong.chee at intel.com

Quoted 9 lines

Quoted 5 lines

From: Tien Fong Chee <[email protected]>

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 7 lines

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 ?

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

But, i will keep minimal ifdefery on socfpga_bridges_reset to indicate the fpga bridges should not be released in SPL.

Quoted 5 lines

Quoted 5 lines

Quoted 8 lines

Quoted 6 lines

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/... ?

Quoted 5 lines

Quoted 2 lines

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.

Quoted 28 lines

Quoted 10 lines

Quoted 10 lines

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.

Quoted 10 lines

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

Quoted 3 lines

Chin Liang, Could you help to advice on these?

Quoted 2 lines

[...]

From: Tien Fong Chee <[email protected]>

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 <[email protected]>

---
 include/configs/socfpga_common.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

include/configs/socfpga_common.h +0 -0

index 9be9e79..f5b3277 100644

include/configs/socfpga_common.h +0 -0

 /*
  * Cache
  */

Quoted 13 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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 <[email protected]>

---
 include/configs/socfpga_common.h |    4 ++++

 1 files changed, 4 insertions(+), 0 deletions(-)

include/configs/socfpga_common.h line 75

Quoted 12 lines

include/configs/socfpga_common.h +0 -0

index 9be9e79..f5b3277 100644
@@ -70,6 +70,10 @@
  #define CONFIG_CMD_PXE
  #define CONFIG_MENU

include/configs/socfpga_common.h +3 -0

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

Quoted 5 lines

 /*
  * Cache
  */

Quoted 23 lines · Marek Vasut

Quoted 22 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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 <[email protected]>

---
 include/configs/socfpga_common.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

include/configs/socfpga_common.h +0 -0

index 9be9e79..f5b3277 100644
@@ -70,6 +70,10 @@
 #define CONFIG_CMD_PXE
 #define CONFIG_MENU
 

Quoted 6 lines

include/configs/socfpga_common.h line 75

Quoted 3 lines

include/configs/socfpga_common.h +3 -0

+#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?