[U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg()

[Dalon L Westergreen <dalon.westergreen@linux.intel.com>]

On Mon, 2020-03-09 at 15:25 +0100, Marek Vasut wrote:
> On 3/9/20 3:24 PM, Simon Goldschmidt wrote:
> > On Mon, Mar 9, 2020 at 3:15 PM Marek Vasut <marex@denx.de> wrote:
> > > On 3/9/20 3:10 PM, Simon Goldschmidt wrote:
> > > > On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote:
> > > > > On 3/9/20 9:50 AM, Ley Foon Tan wrote:
> > > > > > On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen
> > > > > > <dalon.westergreen@linux.intel.com> wrote:
> > > > > > > I am reading through this thread, and want to point out that it is
> > > > > > > not that the
> > > > > > > FPGA bridge need be actively used in the fpga, but
> > > > > > > rather that this port be configured in the FPGA
> > > > > > > configuration.  This is an
> > > > > > > important distinction, ecery FPGA design that
> > > > > > > instantiates the HPS does configure the F2S Bridge.  it drives
> > > > > > > select pins,
> > > > > > > control pins, data pins, etc, on the interface to known values,
> > > > > > > and so the apply can always be done as all SoC FPGA designs have
> > > > > > > the soc
> > > > > > > instantiated.  If someone boots the arm, and uses an
> > > > > > > FPGA design without having the soc instantiated, it may then cause
> > > > > > > issues, but i
> > > > > > > would argue that is not a supported use
> > > > > > > in the first place.
> > > > > > > 
> > > > > > > --dalon
> > > > > > > 
> > > > > > 
> > > > > > I can reproduce the issue if without setting applycfg bit. Access to
> > > > > > F2sdram interface will cause system hang.
> > > > > > 
> > > > > > From the Cyclone 5 Soc datasheet:
> > > > > > applycfg - Write with this bit set to apply all the settings loaded
> > > > > > in
> > > > > > SDR registers to the memory interface. This bit is write-only and
> > > > > > always returns 0 if read.
> > > > > > 
> > > > > > Software should set applycfg bit if change any register under SDR
> > > > > > register range. SW is allowed to set this bit multiple times, the
> > > > > > only
> > > > > > condition is SDRAM need to be idle.
> > > > > > My suggestion is we add back socfpga_sdram_apply_static_cfg(), but
> > > > > > change the sequence to below:
> > > > > > writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
> > > > > > socfpga_sdram_apply_static_cfg();
> > > > > > 
> > > > > > Marek and Simon, are you okay with this? If yes, I will submit patch
> > > > > > for this.
> > > > > 
> > > > > The problem was that this was causing weird sporadic hangs on Gen5,
> > > > > which is why it was removed. So until there is an explanation for
> > > > > those
> > > > > hangs, I'm not really OK with this.
> > > > 
> > > > These sporadic hangs you saw were on devices without an FPGA image
> > > > directly
> > > > accessing the SDRAM ports, right?
> > > 
> > > Yes
> > > 
> > > > > Maybe the application of static config should happen in SPL, before
> > > > > the
> > > > > DRAM is running, or something like that ?
> > > > 
> > > > Thinking this further, limiting it to applying in SPL is not a good idea
> > > > since
> > > > that prevents us from implementing different FPGA images/configs by
> > > > loading a
> > > > config later in the boot (i.e. in U-Boot from a FIT-image).
> > > 
> > > Well, but later we have SDRAM running and we cannot make it go idle, can
> > > we?
> > > 

Unfortunately the sdram cfg apply must occur AFTER the fpga is configured.  This
is because the FPGA drives configuration bits, around the interfaces datawidth
for example, that are used in setting up the dram interface.  I believe the
intent is for the command to only run when the ridge enable function is called.


> > > > Would it work to make setting this optional, i.e. only write the bit if
> > > > an FPGA
> > > > config actually uses these ports? Then it couldn't lead to problems on
> > > > other
> > > > hardware...
> > > 
> > > Can you make SDRAM bus really idle ?
> > 
> > From the CPU side, that should work, no? Of course you have to make sure no
> > other peripheraly (including FPGA!) are using the RAM.
> > 
> > And if this would be an explicit command, people needing this could
> > experiment with it - and hopefully give better hints as to what's going
> > wrong
> > if we *do* see problems again.
> 
> Maybe altera has something hidden somewhere in the bus tunables ? :)

The only trick i am aware of, and Ley Foon, please comment, is isolating
relevant code to the caches before executing.

--dalon