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 <[email protected]> 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 <[email protected]> > > > Cc: Chin Liang See <[email protected]> > > > Cc: Dinh Nguyen <[email protected]> > > > Cc: Simon Goldschmidt <[email protected]> > > > Cc: Tien Fong Chee <[email protected]> > > Good catch! > > > > Reviewed-by: Simon Goldschmidt <[email protected]> > 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
