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

[See, Chin Liang]

On Mon, 2019-04-29 at 10:34 +0200, Marek Vasut wrote:
> On 4/26/19 8:39 AM, Simon Goldschmidt wrote:
> > 
> > On Tue, Apr 23, 2019 at 6:14 PM Marek Vasut <marex at denx.de> wrote:
> > > 
> > > 
> > > The usage of socfpga_sdram_apply_static_cfg() seems rather
> > > dubious and
> > > is confirmed to lead to a rare system hang when enabling bridges.
> > > This
> > > patch removes the socfpga_sdram_apply_static_cfg() altogether,
> > > because
> > > it's use seems unjustified and problematic.
> > > 
> > > The socfpga_sdram_apply_static_cfg() triggers write to SDRAM
> > > staticcfg
> > > register to set the applycfg bit, which according to old vendor
> > > U-Boot
> > > sources can only be written when there is no traffic between the
> > > SDRAM
> > > controller and the rest of the system. Empirical measurements
> > > confirm
> > > this, setting the applycfg bit when there is traffic between the
> > > SDRAM
> > > controller and CPU leads to the SDRAM controller accesses being
> > > blocked
> > > shortly after.
> > > 
> > > Altera originally solved this by moving the entire code which
> > > sets the
> > > staticcfg register to OCRAM [1]. The commit message claims that
> > > the
> > > applycfg bit needs to be set after write to fpgaportrst register.
> > > This
> > > is however inverted by Altera shortly after in [2], where the
> > > order
> > > becomes the exact opposite of what commit message [1] claims to
> > > be the
> > > required order. The explanation points to a possible problem in
> > > AMP
> > > use-case, where the FPGA might be sending transactions through
> > > the F2S
> > > bridge.
> > > 
> > > However, the AMP is only the tip of the iceberg here. Any of the
> > > other
> > > L2, L3 or L4 masters can trigger transactions to the SDRAM. It
> > > becomes
> > > rather non-trivial to guarantee there are no transactions to the
> > > SDRAM
> > > controller.
> > > 
> > > The SoCFPGA SDRAM driver always writes the applycfg bit in SPL.
> > > Thus,
> > > writing the applycfg again in bridge enable code seems redundant
> > > and
> > > can presumably be dropped.
> > > 
> > > [1] https://github.com/altera-opensource/u-boot-socfpga/commit/75
> > > 905816ec95b0ccd515700b922628d7aa9036f8
> > > [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8b
> > > a6986b04a91d23c7adf529186b34c8d2967ad5
> > > 
> > > Signed-off-by: Marek Vasut <marex at denx.de>
> > > Cc: Chin Liang See <chin.liang.see at intel.com>
> > > Cc: Dinh Nguyen <dinguyen at kernel.org>
> > > Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> > > Cc: Tien Fong Chee <tien.fong.chee at intel.com>
> > Good catch!
> > 
> > Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> I am still hoping that Chin might jump in and explain the discrepancy
> between those two patches in Altera U-Boot fork I linked above.
> 

Hi Marek,

We still need the sdram_apply_static_cfg to ensure correct fpga2sdram
port is enabled, based on the new FPGA designs. https://www.intel.com/c
ontent/www/us/en/programmable/hps/cyclone-
v/hps.html#topic/sfo1411577376106.html

For the AMP, I checked back the fogbugz case and the correct term
should be multi-core. In our test scenario, we use a NIOS II on FPGA to
pump transaction to FPGA2SDRAM continuously. It failed with original
code when FPGA config take place and that's why patch [2] needed.

At same time, U-Boot run in serial manner. Hence we expect all L3 or L4
masters are idle during that time. As example, tftp or fatload from
SDMMC shall be complete before next U-Boot console instruction such as
"fpga load" can take place.

Hope this explains.

Thanks
Chin Liang