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

[Marek Vasut]

On 3/31/20 10:13 AM, Ley Foon Tan wrote:
> On Fri, Mar 20, 2020 at 3:52 PM Ley Foon Tan <lftan.linux at gmail.com> wrote:
> 
>>>>>>>>> configured.  This
>>>>>>>>> is because the FPGA drives configuration bits, around the interfaces
>>>>>>>>> datawidth
>>>>>>>>> for example, that are used in setting up the dram interface.  I
>>>>>>>>> believe the
>>>>>>>>> intent is for the command to only run when the ridge enable function
>>>>>>>>> is
>>>>>>>>> called.
>>>>>>>>
>>>>>>>> So that's one part of the fix -- only run this apply_static_cfg if the
>>>>>>>> bitstream uses the F2S bridge.
>>>>>>>
>>>>>>> actually, the restriction is to apply this only if the FPGA is configured,
>>>>>>> whether the bridge is used is irrelevant.  you can further restrict this
>>>>>>> to only when the bridge is used, but to me that means devicetree entries
>>>>>>> or something to determine whether it is used.
>>>>>>
>>>>>> In my opinion, we need an FPGA-specific devicetree (or something
>>>>>> similar) to describe the FPGA image, anyway.
>>>>>
>>>>> Like a DTO ?
>>>>
>>>> DTOs are how i suggest solving this in linux, so i would assume a dto would
>>>> be best here too.
>>>
>>> Yes. That DTO should be beside the FPGA image, either in a FIT image or
>>> just loaded separately. It should contain pin settings, bridge settings etc.
>>>
>>> However, to ensure the bus is idle, we still would have to limit
>>> applying that config bit to before RAM is set up, so quite early in SPL,
>>> right? I cannot see how that would work, given the limit of 64K. Plus
>>> it's kind of a bad boot flow to configure the FPGA before even starting
>>> U-Boot...
>> There is usecase user may want to program fpga image in Uboot command
>> prompt. It will have problem too.
>>
>> socfpga_sdram_apply_static_cfg() is written in assembly code and the
>> code is fit to one cacheline size (32 bytes). This at least make sure
>> CPU no access to SDRAM when run apply static cfg code in uboot.
>> Frankly, this can't prevent external master access to SDRAM.
>>
>> Marek,
>> I found code in drivers/fpga/socfpga_gen5.c:socfpga_load() write to
>> SDR_CTRLGRP_FPGAPORTRST_ADDRESS register, but doesn't write to call to
>> socfpga_sdram_apply_static_cfg() function. This means FPGA port is not
>> put into reset before program fpga image. Suspect this might reason it
>> cause sporadic hangs.
>>
>> Stay safe everyone.
>>
>> Regards
>> Ley Foon
> 
> Hi
> 
> Cyclone5 F2sdram interface is totally broken in uboot now, can we
> revert back the socfpga_sdram_apply_static_cfg() but add checking if
> f2sdram bridge is enabled. Also add call to
> socfpga_sdram_apply_static_cfg() in socfpga_load().
> Simon's enhancement for DTO might not getting in soon.

Is it better if it's totally and visibly broken, or if every 500 or so
reboots the machine randomly and inobviously hangs when enabling bridges ?

Also, there is still the problem where we are not able to guarantee that
the DRAM bus will be completely idle before running the apply static
cfg, or did that get sorted out somehow ?

I think we shouldn't revert it, but rather make it somehow conditional,
maybe by adding argument to the 'bridge' command or an environment
variable (probably better), at least for this release.