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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

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