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

[Marek Vasut]

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.
>>>> 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,

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