On Tue, Apr 23, 2019 at 6:14 PM Marek Vasut <[email protected]> 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 <[email protected]> > Cc: Chin Liang See <[email protected]> > Cc: Dinh Nguyen <[email protected]> > Cc: Simon Goldschmidt <[email protected]> > Cc: Tien Fong Chee <[email protected]>
Good catch! Reviewed-by: Simon Goldschmidt <[email protected]> > --- > 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 > _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
