From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Fri, 28 Jul 2017 05:16:28 +0000 Subject: [U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL In-Reply-To: <24754175-034a-5a7d-919c-d125a192e847@denx.de> References: <1501130208-2278-1-git-send-email-tien.fong.chee@intel.com> <1501130208-2278-2-git-send-email-tien.fong.chee@intel.com> <0f5f8bdc-9bd1-ec68-a3bd-13d8d6e6f5c4@denx.de> <1501147293.3184.9.camel@intel.com> <24754175-034a-5a7d-919c-d125a192e847@denx.de> Message-ID: <1501218987.3184.23.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > > > > > > > > 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 > > > > --- > > > >  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) > > > > >