On 4/29/19 3:02 PM, See, Chin Liang wrote: > 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 <ma...@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 <ma...@denx.de> >>>> Cc: Chin Liang See <chin.liang....@intel.com> >>>> Cc: Dinh Nguyen <dingu...@kernel.org> >>>> Cc: Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> >>>> Cc: Tien Fong Chee <tien.fong.c...@intel.com> >>> Good catch! >>> >>> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschm...@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,
Hi, > 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 I think the link might be broken, it leads to fpgaportrst description. Which "new FPGA designs" do you refer to ? Regarding sdram_apply_static_cfg, this only sets the applycfg bit, it has nothing to do with enabling or disabling the FPGA-to-SDRAM ports. Or does it ? The documentation is not clear what all the effects of this bit are. > 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. So [2] prevents traffic from F2S to reach the SDRAM controller by enabling the F2S ports _after_ the applycfg bit was set in the SDRAM controller. But that clearly contradicts [1], which claims: " To update the U-Boot FPGA2SDRAM enablement driver where applycfg bit need to be set after write to fpgaportrst. " > 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. Right, except you cannot guarantee that in any way in AMP setup (a CPU can access the SDRAM, or some rogue traffic from L3/L4). > Hope this explains. Well, not really. I think the main point that's unclear is what the applycfg bit really does and/or affects. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
