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

[Simon Goldschmidt]

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/75905816ec95b0ccd515700b922628d7aa9036f8
> [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8ba6986b04a91d23c7adf529186b34c8d2967ad5
>
> 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>

> ---
>  arch/arm/mach-socfpga/misc_gen5.c | 31 -------------------------------
>  1 file changed, 31 deletions(-)
>
> diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c
> index 7876953595..eeba199edc 100644
> --- a/arch/arm/mach-socfpga/misc_gen5.c
> +++ b/arch/arm/mach-socfpga/misc_gen5.c
> @@ -220,35 +220,6 @@ static struct socfpga_reset_manager *reset_manager_base =
>  static struct socfpga_sdr_ctrl *sdr_ctrl =
>         (struct socfpga_sdr_ctrl *)SDR_CTRLGRP_ADDRESS;
>
> -static void socfpga_sdram_apply_static_cfg(void)
> -{
> -       const u32 applymask = 0x8;
> -       u32 val = readl(&sdr_ctrl->static_cfg) | applymask;
> -
> -       /*
> -        * SDRAM staticcfg register specific:
> -        * When applying the register setting, the CPU must not access
> -        * SDRAM. Luckily for us, we can abuse i-cache here to help us
> -        * circumvent the SDRAM access issue. The idea is to make sure
> -        * that the code is in one full i-cache line by branching past
> -        * it and back. Once it is in the i-cache, we execute the core
> -        * of the code and apply the register settings.
> -        *
> -        * The code below uses 7 instructions, while the Cortex-A9 has
> -        * 32-byte cachelines, thus the limit is 8 instructions total.
> -        */
> -       asm volatile(
> -               ".align 5                       \n"
> -               "       b       2f              \n"
> -               "1:     str     %0,     [%1]    \n"
> -               "       dsb                     \n"
> -               "       isb                     \n"
> -               "       b       3f              \n"
> -               "2:     b       1b              \n"
> -               "3:     nop                     \n"
> -       : : "r"(val), "r"(&sdr_ctrl->static_cfg) : "memory", "cc");
> -}
> -
>  void do_bridge_reset(int enable, unsigned int mask)
>  {
>         int i;
> @@ -263,14 +234,12 @@ void do_bridge_reset(int enable, unsigned int mask)
>                 }
>
>                 writel(iswgrp_handoff[2], &sysmgr_regs->fpgaintfgrp_module);
> -               socfpga_sdram_apply_static_cfg();
>                 writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst);
>                 writel(iswgrp_handoff[0], &reset_manager_base->brg_mod_reset);
>                 writel(iswgrp_handoff[1], &nic301_regs->remap);
>         } else {
>                 writel(0, &sysmgr_regs->fpgaintfgrp_module);
>                 writel(0, &sdr_ctrl->fpgaport_rst);
> -               socfpga_sdram_apply_static_cfg();
>                 writel(0, &reset_manager_base->brg_mod_reset);
>                 writel(1, &nic301_regs->remap);
>         }
> --
> 2.20.1
>