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