[U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg()
marex at denx.de
Mon Apr 29 21:29:10 UTC 2019
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 <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.
>>>> patch removes the socfpga_sdram_apply_static_cfg() altogether,
>>>> it's use seems unjustified and problematic.
>>>> The socfpga_sdram_apply_static_cfg() triggers write to SDRAM
>>>> register to set the applycfg bit, which according to old vendor
>>>> sources can only be written when there is no traffic between the
>>>> controller and the rest of the system. Empirical measurements
>>>> this, setting the applycfg bit when there is traffic between the
>>>> controller and CPU leads to the SDRAM controller accesses being
>>>> shortly after.
>>>> Altera originally solved this by moving the entire code which
>>>> sets the
>>>> staticcfg register to OCRAM . The commit message claims that
>>>> applycfg bit needs to be set after write to fpgaportrst register.
>>>> is however inverted by Altera shortly after in , where the
>>>> becomes the exact opposite of what commit message  claims to
>>>> be the
>>>> required order. The explanation points to a possible problem in
>>>> use-case, where the FPGA might be sending transactions through
>>>> the F2S
>>>> However, the AMP is only the tip of the iceberg here. Any of the
>>>> L2, L3 or L4 masters can trigger transactions to the SDRAM. It
>>>> rather non-trivial to guarantee there are no transactions to the
>>>> The SoCFPGA SDRAM driver always writes the applycfg bit in SPL.
>>>> writing the applycfg again in bridge enable code seems redundant
>>>> can presumably be dropped.
>>>>  https://github.com/altera-opensource/u-boot-socfpga/commit/75
>>>>  https://github.com/altera-opensource/u-boot-socfpga/commit/8b
>>>> 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
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
> 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  needed.
So  prevents traffic from F2S to reach the SDRAM controller by
enabling the F2S ports _after_ the applycfg bit was set in the SDRAM
But that clearly contradicts , 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.
More information about the U-Boot