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

[Marek Vasut <marex@denx.de>]

On 3/9/20 3:24 PM, Simon Goldschmidt wrote:
> On Mon, Mar 9, 2020 at 3:15 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/9/20 3:10 PM, Simon Goldschmidt wrote:
>>> On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/9/20 9:50 AM, Ley Foon Tan wrote:
>>>>> On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen
>>>>> <dalon.westergreen@linux.intel.com> wrote:
>>>>>>
>>>>>> I am reading through this thread, and want to point out that it is not that the
>>>>>> FPGA bridge need be actively used in the fpga, but
>>>>>> rather that this port be configured in the FPGA configuration.  This is an
>>>>>> important distinction, ecery FPGA design that
>>>>>> instantiates the HPS does configure the F2S Bridge.  it drives select pins,
>>>>>> control pins, data pins, etc, on the interface to known values,
>>>>>> and so the apply can always be done as all SoC FPGA designs have the soc
>>>>>> instantiated.  If someone boots the arm, and uses an
>>>>>> FPGA design without having the soc instantiated, it may then cause issues, but i
>>>>>> would argue that is not a supported use
>>>>>> in the first place.
>>>>>>
>>>>>> --dalon
>>>>>>
>>>>>
>>>>> I can reproduce the issue if without setting applycfg bit. Access to
>>>>> F2sdram interface will cause system hang.
>>>>>
>>>>> From the Cyclone 5 Soc datasheet:
>>>>> applycfg - Write with this bit set to apply all the settings loaded in
>>>>> SDR registers to the memory interface. This bit is write-only and
>>>>> always returns 0 if read.
>>>>>
>>>>> Software should set applycfg bit if change any register under SDR
>>>>> register range. SW is allowed to set this bit multiple times, the only
>>>>> condition is SDRAM need to be idle.
>>>>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but
>>>>> change the sequence to below:
>>>>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
>>>>> socfpga_sdram_apply_static_cfg();
>>>>>
>>>>> Marek and Simon, are you okay with this? If yes, I will submit patch for this.
>>>>
>>>> The problem was that this was causing weird sporadic hangs on Gen5,
>>>> which is why it was removed. So until there is an explanation for those
>>>> hangs, I'm not really OK with this.
>>>
>>> These sporadic hangs you saw were on devices without an FPGA image directly
>>> accessing the SDRAM ports, right?
>>
>> Yes
>>
>>>> Maybe the application of static config should happen in SPL, before the
>>>> DRAM is running, or something like that ?
>>>
>>> Thinking this further, limiting it to applying in SPL is not a good idea since
>>> that prevents us from implementing different FPGA images/configs by loading a
>>> config later in the boot (i.e. in U-Boot from a FIT-image).
>>
>> Well, but later we have SDRAM running and we cannot make it go idle, can we?
>>
>>> Would it work to make setting this optional, i.e. only write the bit if an FPGA
>>> config actually uses these ports? Then it couldn't lead to problems on other
>>> hardware...
>>
>> Can you make SDRAM bus really idle ?
> 
> From the CPU side, that should work, no? Of course you have to make sure no
> other peripheraly (including FPGA!) are using the RAM.
> 
> And if this would be an explicit command, people needing this could
> experiment with it - and hopefully give better hints as to what's going wrong
> if we *do* see problems again.

Maybe altera has something hidden somewhere in the bus tunables ? :)