[U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg()
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/75905816ec95b0ccd... [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8ba6986b04a91d23c...
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See chin.liang.see@intel.com Cc: Dinh Nguyen dinguyen@kernel.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tien Fong Chee tien.fong.chee@intel.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); }
On Tue, Apr 23, 2019 at 6:14 PM Marek Vasut marex@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/75905816ec95b0ccd... [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8ba6986b04a91d23c...
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See chin.liang.see@intel.com Cc: Dinh Nguyen dinguyen@kernel.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tien Fong Chee tien.fong.chee@intel.com
Good catch!
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@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
On 4/26/19 8:39 AM, Simon Goldschmidt wrote:
On Tue, Apr 23, 2019 at 6:14 PM Marek Vasut marex@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/75905816ec95b0ccd... [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8ba6986b04a91d23c...
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See chin.liang.see@intel.com Cc: Dinh Nguyen dinguyen@kernel.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tien Fong Chee tien.fong.chee@intel.com
Good catch!
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
I am still hoping that Chin might jump in and explain the discrepancy between those two patches in Altera U-Boot fork I linked above.
On Mon, 2019-04-29 at 10:34 +0200, Marek Vasut wrote:
On 4/26/19 8:39 AM, Simon Goldschmidt wrote:
On Tue, Apr 23, 2019 at 6:14 PM Marek Vasut marex@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/75 905816ec95b0ccd515700b922628d7aa9036f8 [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8b a6986b04a91d23c7adf529186b34c8d2967ad5
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See chin.liang.see@intel.com Cc: Dinh Nguyen dinguyen@kernel.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tien Fong Chee tien.fong.chee@intel.com
Good catch!
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
I am still hoping that Chin might jump in and explain the discrepancy between those two patches in Altera U-Boot fork I linked above.
Hi Marek,
We still need the sdram_apply_static_cfg to ensure correct fpga2sdram port is enabled, based on the new FPGA designs. https://www.intel.com/c ontent/www/us/en/programmable/hps/cyclone- v/hps.html#topic/sfo1411577376106.html
For the AMP, I checked back the fogbugz case and the correct term should be multi-core. In our test scenario, we use a NIOS II on FPGA to pump transaction to FPGA2SDRAM continuously. It failed with original code when FPGA config take place and that's why patch [2] needed.
At same time, U-Boot run in serial manner. Hence we expect all L3 or L4 masters are idle during that time. As example, tftp or fatload from SDMMC shall be complete before next U-Boot console instruction such as "fpga load" can take place.
Hope this explains.
Thanks Chin Liang
On 4/29/19 3:02 PM, See, Chin Liang wrote:
On Mon, 2019-04-29 at 10:34 +0200, Marek Vasut wrote:
On 4/26/19 8:39 AM, Simon Goldschmidt wrote:
On Tue, Apr 23, 2019 at 6:14 PM Marek Vasut marex@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/75 905816ec95b0ccd515700b922628d7aa9036f8 [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8b a6986b04a91d23c7adf529186b34c8d2967ad5
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See chin.liang.see@intel.com Cc: Dinh Nguyen dinguyen@kernel.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tien Fong Chee tien.fong.chee@intel.com
Good catch!
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
I am still hoping that Chin might jump in and explain the discrepancy between those two patches in Altera U-Boot fork I linked above.
Hi Marek,
Hi,
We still need the sdram_apply_static_cfg to ensure correct fpga2sdram port is enabled, based on the new FPGA designs. https://www.intel.com/c ontent/www/us/en/programmable/hps/cyclone- v/hps.html#topic/sfo1411577376106.html
I think the link might be broken, it leads to fpgaportrst description.
Which "new FPGA designs" do you refer to ?
Regarding sdram_apply_static_cfg, this only sets the applycfg bit, it has nothing to do with enabling or disabling the FPGA-to-SDRAM ports. Or does it ? The documentation is not clear what all the effects of this bit are.
For the AMP, I checked back the fogbugz case and the correct term should be multi-core. In our test scenario, we use a NIOS II on FPGA to pump transaction to FPGA2SDRAM continuously. It failed with original code when FPGA config take place and that's why patch [2] needed.
So [2] prevents traffic from F2S to reach the SDRAM controller by enabling the F2S ports _after_ the applycfg bit was set in the SDRAM controller.
But that clearly contradicts [1], which claims: " To update the U-Boot FPGA2SDRAM enablement driver where applycfg bit need to be set after write to fpgaportrst. "
At same time, U-Boot run in serial manner. Hence we expect all L3 or L4 masters are idle during that time. As example, tftp or fatload from SDMMC shall be complete before next U-Boot console instruction such as "fpga load" can take place.
Right, except you cannot guarantee that in any way in AMP setup (a CPU can access the SDRAM, or some rogue traffic from L3/L4).
Hope this explains.
Well, not really. I think the main point that's unclear is what the applycfg bit really does and/or affects.
participants (3)
-
Marek Vasut -
See, Chin Liang -
Simon Goldschmidt