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

[Simon Goldschmidt]

On Thu, Feb 6, 2020 at 3:41 PM Nico Becker <n.becker at ic-automation.de> wrote:
>
> Am 06.02.2020 um 14:00 schrieb Marek Vasut:
> > On 2/6/20 1:57 PM, Nico Becker wrote:
> >> Am 06.02.2020 um 12:53 schrieb Marek Vasut:
> >>> On 2/6/20 11:50 AM, Nico Becker wrote:
> >>>> Hello,
> >>>
> >>> Hi,
> >>>
> >>>> after removing the function socfpga_sdram_apply_static_cfg() in
> >>>> misc_gen5 we can not use the FPGA2SDRAM bridge.
> >>>>
> >>>> Without the apply static cfg the kernel crash every time,
> >>>> if we try to write @ the fpga2sdram bridge. After an soft reset
> >>>> everything is working.
> >>>>
> >>>> If we add the socfpga_sdram_apply_static_cfg() in the
> >>>> u-boot source code, everything is fine.
> >>>> Now we can use the fpga2sdram bridge after power on.
> >>>>
> >>>> Our setup:
> >>>> - u-boot v2020.01
> >>>>     - load and write fpga firmware
> >>>>     - enable bridges
> >>>> - boot linux
> >>>>
> >>>> I have found some information at
> >>>> <https://support.criticallink.com/redmine/projects/mityarm-5cs/wiki/Important_Note_about_FPGAHPS_SDRAM_Bridge>
> >>>>
> >>>>
> >>>> <http://u-boot.10912.n7.nabble.com/WG-Linux-hang-td314276.html>
> >>>
> >>> Can you send a patch which fixes this for you, with Fixes: tag ?
> >>> Then it would be clear what you changed.
> >>>
> >>> Thanks
> >>>
> >>
> >> Hello,
> >> the code was removed @ commit c5f4b805.
> >
> > Did you read the commit message of that commit and what problem that was
> > solving ? Clearly, reverting the commit isn't the way to go. We need to
> > find a way to unbreak this for you, while not break other platforms.
> >
> >> I attached my patch, sorry for the format, i am new in this.
> >
> > [...]
> >
> Hi,
> yes i read the commit message.
>
> but i found no other option to enable the sdram bridges,
> without crashes/hanging up linux, with the removed source code.
>
> i ve found some more information @intel
> <https://www.intel.com/content/www/us/en/programmable/support/support-resources/knowledge-base/embedded/2016/how-and-when-can-i-enable-the-fpga2sdram-bridge-on-cyclone-v-soc.html>
>
> Intel talk about an bridge_enable_handoff in my opionion
> the cmd set the sram aply cfg
>
> /* add signle command to enable all bridges based on handoff */
> setenv("bridge_enable_handoff",
>         "mw $fpgaintf ${fpgaintf_handoff}; "
>         "go $fpga2sdram_apply; "
>         "mw $fpga2sdram ${fpga2sdram_handoff}; "
>         "mw $axibridge ${axibridge_handoff}; "
>         "mw $l3remap ${l3remap_handoff} ");
>
> setenv_addr("fpga2sdram_apply", (void *)sdram_applycfg_uboot);
>
> /*
>   * Relocate the sdram_applycfg_ocram function to OCRAM and call it
>   */
> ENTRY(sdram_applycfg_uboot)
>         PUSH    {r4-r11, lr}            /* save registers per AAPCS */
>
>         ldr     r1, =sdram_applycfg_ocram
>         ldr     r2, =CONFIG_SYS_INIT_RAM_ADDR
>         mov     r3, r2
>         ldmia   r1!, {r4 - r11}
>         stmia   r3!, {r4 - r11}
>         ldmia   r1!, {r4 - r11}         /* copy more in case code added */
>         stmia   r3!, {r4 - r11}         /* in the future */
>         ldmia   r1!, {r4 - r11}         /* copy more in case code added */
>         stmia   r3!, {r4 - r11}         /* in the future */
>         dsb
>         isb
>         blx     r2                      /* jump to OCRAM */
>         POP     {r4-r11, pc}
> ENDPROC(sdram_applycfg_uboot)
>
>
> it could be an option to write the fpga firmware with u-boot,
> and do a soft reset.
> boot u-boot
> check fpga configuration state
> not configured write firmware reset
> if configured boot linux
>
> i ll check howto to determine the fpga configuration state
> and try this.

That doesn't look like a safe plan: what if you explicitly *want* a reboot
and want to reconfigure the FPGA then to ensure it is in a well-known state?

Marek, what were the problems why this has been removed? Aside from "is
confirmed to lead to a rare system hang when enabling bridges", the commit
message stays rather vague. Maybe that "apply config" bit should only be written
if explicitly configured so, but that would mean we need some kind of config
options included/coming with the FPGA image we program.

Oh, and by now I'm glad our own design connects to DDR via the main bus ;-)

Regards,
Simon