[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 at 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.