* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() @ 2019-04-23 16:14 Marek Vasut 2019-04-26 6:39 ` Simon Goldschmidt 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2019-04-23 16:14 UTC (permalink / raw) To: u-boot 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@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); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2019-04-23 16:14 [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() Marek Vasut @ 2019-04-26 6:39 ` Simon Goldschmidt 2019-04-29 8:34 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2019-04-26 6:39 UTC (permalink / raw) To: u-boot 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/75905816ec95b0ccd515700b922628d7aa9036f8 > [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8ba6986b04a91d23c7adf529186b34c8d2967ad5 > > 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 > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2019-04-26 6:39 ` Simon Goldschmidt @ 2019-04-29 8:34 ` Marek Vasut 2019-04-29 13:02 ` See, Chin Liang 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2019-04-29 8:34 UTC (permalink / raw) To: u-boot 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/75905816ec95b0ccd515700b922628d7aa9036f8 >> [2] https://github.com/altera-opensource/u-boot-socfpga/commit/8ba6986b04a91d23c7adf529186b34c8d2967ad5 >> >> 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. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2019-04-29 8:34 ` Marek Vasut @ 2019-04-29 13:02 ` See, Chin Liang 2019-04-29 21:29 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: See, Chin Liang @ 2019-04-29 13:02 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2019-04-29 13:02 ` See, Chin Liang @ 2019-04-29 21:29 ` Marek Vasut 0 siblings, 0 replies; 39+ messages in thread From: Marek Vasut @ 2019-04-29 21:29 UTC (permalink / raw) To: u-boot 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. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() @ 2020-02-06 10:50 Nico Becker 2020-02-06 11:53 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Nico Becker @ 2020-02-06 10:50 UTC (permalink / raw) To: u-boot Hello, 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> -- Best regards Nico Becker ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-06 10:50 Nico Becker @ 2020-02-06 11:53 ` Marek Vasut 2020-02-06 12:57 ` Nico Becker 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-02-06 11:53 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-06 11:53 ` Marek Vasut @ 2020-02-06 12:57 ` Nico Becker 2020-02-06 13:00 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Nico Becker @ 2020-02-06 12:57 UTC (permalink / raw) To: u-boot 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. I attached my patch, sorry for the format, i am new in this. Thanks a lot --- arch/arm/mach-socfpga/misc_gen5.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/arch/arm/mach-socfpga/misc_gen5.c b/arch/arm/mach-socfpga/misc_gen5.c index 22042d0de0..19c6d24170 100644 --- a/arch/arm/mach-socfpga/misc_gen5.c +++ b/arch/arm/mach-socfpga/misc_gen5.c @@ -213,6 +213,35 @@ 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; @@ -227,6 +256,7 @@ 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); @@ -236,6 +266,7 @@ void do_bridge_reset(int enable, unsigned int mask) } else { writel(0, &sysmgr_regs->fpgaintfgrp_module); writel(0, &sdr_ctrl->fpgaport_rst); + socfpga_sdram_apply_static_cfg(); writel(0x7, &reset_manager_base->brg_mod_reset); writel(1, &nic301_regs->remap); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-06 12:57 ` Nico Becker @ 2020-02-06 13:00 ` Marek Vasut 2020-02-06 14:13 ` Nico Becker 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-02-06 13:00 UTC (permalink / raw) To: u-boot 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. [...] ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-06 13:00 ` Marek Vasut @ 2020-02-06 14:13 ` Nico Becker 2020-02-06 14:57 ` Simon Goldschmidt 0 siblings, 1 reply; 39+ messages in thread From: Nico Becker @ 2020-02-06 14:13 UTC (permalink / raw) To: u-boot 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. Thanks -- Nico Becker ic-automation GmbH Alexander-Diehl-Straße 2a D-55130 Mainz Tel: +49-(0)6131-62718-24 Fax: +49-(0)6131-62718-10 email: n.becker at ic-automation.de Web: http://www.ic-automation.de Geschäftsführer: Dr. Stefan Becker HRB 7283, Amtsgericht Mainz ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-06 14:13 ` Nico Becker @ 2020-02-06 14:57 ` Simon Goldschmidt 2020-02-07 7:55 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-02-06 14:57 UTC (permalink / raw) To: u-boot On Thu, Feb 6, 2020 at 3:41 PM Nico Becker <n.becker@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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-06 14:57 ` Simon Goldschmidt @ 2020-02-07 7:55 ` Marek Vasut 2020-02-07 12:09 ` Simon Goldschmidt 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-02-07 7:55 UTC (permalink / raw) To: u-boot On 2/6/20 3:57 PM, Simon Goldschmidt wrote: > On Thu, Feb 6, 2020 at 3:41 PM Nico Becker <n.becker@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. That's exactly what the problem was. I didn't manage to get further details from Altera on this. > 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. Maybe the apply config should only be used if the F2S bridge is being used ? > Oh, and by now I'm glad our own design connects to DDR via the main bus ;-) Right. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-07 7:55 ` Marek Vasut @ 2020-02-07 12:09 ` Simon Goldschmidt 2020-02-07 12:27 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-02-07 12:09 UTC (permalink / raw) To: u-boot On Fri, Feb 7, 2020 at 8:56 AM Marek Vasut <marex@denx.de> wrote: > > On 2/6/20 3:57 PM, Simon Goldschmidt wrote: > > On Thu, Feb 6, 2020 at 3:41 PM Nico Becker <n.becker@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. > > That's exactly what the problem was. I didn't manage to get further > details from Altera on this. Hrmpf :-( > > > 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. > > Maybe the apply config should only be used if the F2S bridge is being used ? Yes, well how do you know after programming an FPGA that this bridge is in use? This depends on the FPGA image, while currently the Altera work flow compiles it into the U-Boot binary.While I'm still working on moving this into the U-Boot dts (I still have size issues there), it's still not encoded with the FPGA. What we would need is some kind of meta info with the FPGA image that tells us how to configure the bridges (and maybe some clocks or hardware handed off into the FPGA) after programming the FPGA. Only then, we could write the apply config bit after programming the FPGA. As to the "SDRAM access must be idle", I think that should work from U-Boot. All peripherals should be idle, unless the FPGA accesses SDRAM right after being configured. Maybe we'd need to wait a bit in case the cache is filling a line or so... Regards, Simon > > > Oh, and by now I'm glad our own design connects to DDR via the main bus ;-) > > Right. > > -- > Best regards, > Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-07 12:09 ` Simon Goldschmidt @ 2020-02-07 12:27 ` Marek Vasut 2020-02-07 13:44 ` Simon Goldschmidt 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-02-07 12:27 UTC (permalink / raw) To: u-boot On 2/7/20 1:09 PM, Simon Goldschmidt wrote: > On Fri, Feb 7, 2020 at 8:56 AM Marek Vasut <marex@denx.de> wrote: >> >> On 2/6/20 3:57 PM, Simon Goldschmidt wrote: >>> On Thu, Feb 6, 2020 at 3:41 PM Nico Becker <n.becker@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. >> >> That's exactly what the problem was. I didn't manage to get further >> details from Altera on this. > > Hrmpf :-( > >> >>> 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. >> >> Maybe the apply config should only be used if the F2S bridge is being used ? > > Yes, well how do you know after programming an FPGA that this bridge is in use? From the handoff files ? > This depends on the FPGA image, while currently the Altera work flow compiles > it into the U-Boot binary.While I'm still working on moving this into the U-Boot > dts (I still have size issues there), it's still not encoded with the FPGA. Well you can dig this information out of the handoff files , so you can also make this somehow configurable via either DT or bridge command args. > What we would need is some kind of meta info with the FPGA image that tells us > how to configure the bridges (and maybe some clocks or hardware handed off into > the FPGA) after programming the FPGA. fitImage ? :) > Only then, we could write the apply config bit after programming the FPGA. > > As to the "SDRAM access must be idle", I think that should work from U-Boot. All > peripherals should be idle, unless the FPGA accesses SDRAM right after being > configured. Maybe we'd need to wait a bit in case the cache is filling a line > or so... ... or something is queued up in some buffer somewhere in the bridge. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-07 12:27 ` Marek Vasut @ 2020-02-07 13:44 ` Simon Goldschmidt 2020-02-07 13:49 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-02-07 13:44 UTC (permalink / raw) To: u-boot On Fri, Feb 7, 2020 at 1:27 PM Marek Vasut <marex@denx.de> wrote: > > On 2/7/20 1:09 PM, Simon Goldschmidt wrote: > > On Fri, Feb 7, 2020 at 8:56 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 2/6/20 3:57 PM, Simon Goldschmidt wrote: > >>> On Thu, Feb 6, 2020 at 3:41 PM Nico Becker <n.becker@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. > >> > >> That's exactly what the problem was. I didn't manage to get further > >> details from Altera on this. > > > > Hrmpf :-( > > > >> > >>> 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. > >> > >> Maybe the apply config should only be used if the F2S bridge is being used ? > > > > Yes, well how do you know after programming an FPGA that this bridge is in use? > > From the handoff files ? Yeah, well, I know that. I meant this is not available in U-Boot, see below. > > > This depends on the FPGA image, while currently the Altera work flow compiles > > it into the U-Boot binary.While I'm still working on moving this into the U-Boot > > dts (I still have size issues there), it's still not encoded with the FPGA. > > Well you can dig this information out of the handoff files , so you can > also make this somehow configurable via either DT or bridge command args. My point was this can differ between FPGA images. Once you load an FPGA image that doesn't match what you expected when building (or flashing) U-Boot, the system may lock up again. DT is not an option, as you hard-code it when flashing U-Boot. The loaded FPGA image can still change.Bridge command args is a way of how to get this info into the code that runs, but from where do these command args come? You need to somehow parse this info from an FPGA image file read from storage. > > > What we would need is some kind of meta info with the FPGA image that tells us > > how to configure the bridges (and maybe some clocks or hardware handed off into > > the FPGA) after programming the FPGA. > > fitImage ? :) I thought of that, too. Maybe a short dts (maybe overlay) beside the FPGA. But then it would again not work when loading a pure rbf. But then again, that might be ok... So I could imagine this to "just work" when someday I finally have finished my series of moving this handoff info into dts and then including an overlay dts into a fit image that gets applied after programming the FPGA (and by/after applying it, the code that applies bridge settings would run). Would that work? Regards, Simon > > > Only then, we could write the apply config bit after programming the FPGA. > > > > As to the "SDRAM access must be idle", I think that should work from U-Boot. All > > peripherals should be idle, unless the FPGA accesses SDRAM right after being > > configured. Maybe we'd need to wait a bit in case the cache is filling a line > > or so... > > ... or something is queued up in some buffer somewhere in the bridge. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-07 13:44 ` Simon Goldschmidt @ 2020-02-07 13:49 ` Marek Vasut 2020-02-12 18:52 ` Dalon L Westergreen 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-02-07 13:49 UTC (permalink / raw) To: u-boot On 2/7/20 2:44 PM, Simon Goldschmidt wrote: [...] >>> This depends on the FPGA image, while currently the Altera work flow compiles >>> it into the U-Boot binary.While I'm still working on moving this into the U-Boot >>> dts (I still have size issues there), it's still not encoded with the FPGA. >> >> Well you can dig this information out of the handoff files , so you can >> also make this somehow configurable via either DT or bridge command args. > > > My point was this can differ between FPGA images. Once you load an FPGA image > that doesn't match what you expected when building (or flashing) U-Boot, the > system may lock up again. > > DT is not an option, as you hard-code it when flashing U-Boot. You can apply a DTO on the U-Boot DT :-) > The loaded FPGA > image can still change.Bridge command args is a way of how to get this info into > the code that runs, but from where do these command args come? You need to > somehow parse this info from an FPGA image file read from storage. Maybe this could be part of a fitImage or DTO bundled with the FPGA image ? >>> What we would need is some kind of meta info with the FPGA image that tells us >>> how to configure the bridges (and maybe some clocks or hardware handed off into >>> the FPGA) after programming the FPGA. >> >> fitImage ? :) > I thought of that, too. Maybe a short dts (maybe overlay) beside the FPGA. But > then it would again not work when loading a pure rbf. But then again, that might > be ok... That's probably OK. If you're loading pure RBF, then you know what you're doing. > So I could imagine this to "just work" when someday I finally have finished my > series of moving this handoff info into dts and then including an overlay dts > into a fit image that gets applied after programming the FPGA (and by/after > applying it, the code that applies bridge settings would run). Would that work? I think so. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-07 13:49 ` Marek Vasut @ 2020-02-12 18:52 ` Dalon L Westergreen 2020-03-09 8:50 ` Ley Foon Tan 0 siblings, 1 reply; 39+ messages in thread From: Dalon L Westergreen @ 2020-02-12 18:52 UTC (permalink / raw) To: u-boot I am reading through this thread, and want to point out that it is not that the FPGA bridge need be actively used in the fpga, but rather that this port be configured in the FPGA configuration. This is an important distinction, ecery FPGA design that instantiates the HPS does configure the F2S Bridge. it drives select pins, control pins, data pins, etc, on the interface to known values, and so the apply can always be done as all SoC FPGA designs have the soc instantiated. If someone boots the arm, and uses an FPGA design without having the soc instantiated, it may then cause issues, but i would argue that is not a supported use in the first place. --dalon On Fri, 2020-02-07 at 14:49 +0100, Marek Vasut wrote: > On 2/7/20 2:44 PM, Simon Goldschmidt wrote: > > [...] > > > > > This depends on the FPGA image, while currently the Altera work flow compiles > > > > it into the U-Boot binary.While I'm still working on moving this into the U-Boot > > > > dts (I still have size issues there), it's still not encoded with the FPGA. > > > > > > Well you can dig this information out of the handoff files , so you can > > > also make this somehow configurable via either DT or bridge command args. > > > > > > My point was this can differ between FPGA images. Once you load an FPGA image > > that doesn't match what you expected when building (or flashing) U-Boot, the > > system may lock up again. > > > > DT is not an option, as you hard-code it when flashing U-Boot. > > You can apply a DTO on the U-Boot DT :-) > > > The loaded FPGA > > image can still change.Bridge command args is a way of how to get this info into > > the code that runs, but from where do these command args come? You need to > > somehow parse this info from an FPGA image file read from storage. > > Maybe this could be part of a fitImage or DTO bundled with the FPGA image ? > > > > > What we would need is some kind of meta info with the FPGA image that tells us > > > > how to configure the bridges (and maybe some clocks or hardware handed off into > > > > the FPGA) after programming the FPGA. > > > > > > fitImage ? :) > > I thought of that, too. Maybe a short dts (maybe overlay) beside the FPGA. But > > then it would again not work when loading a pure rbf. But then again, that might > > be ok... > > That's probably OK. If you're loading pure RBF, then you know what > you're doing. > > > So I could imagine this to "just work" when someday I finally have finished my > > series of moving this handoff info into dts and then including an overlay dts > > into a fit image that gets applied after programming the FPGA (and by/after > > applying it, the code that applies bridge settings would run). Would that work? > > I think so. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-02-12 18:52 ` Dalon L Westergreen @ 2020-03-09 8:50 ` Ley Foon Tan 2020-03-09 12:57 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Ley Foon Tan @ 2020-03-09 8:50 UTC (permalink / raw) To: u-boot On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen <dalon.westergreen@linux.intel.com> wrote: > > I am reading through this thread, and want to point out that it is not that the > FPGA bridge need be actively used in the fpga, but > rather that this port be configured in the FPGA configuration. This is an > important distinction, ecery FPGA design that > instantiates the HPS does configure the F2S Bridge. it drives select pins, > control pins, data pins, etc, on the interface to known values, > and so the apply can always be done as all SoC FPGA designs have the soc > instantiated. If someone boots the arm, and uses an > FPGA design without having the soc instantiated, it may then cause issues, but i > would argue that is not a supported use > in the first place. > > --dalon > I can reproduce the issue if without setting applycfg bit. Access to F2sdram interface will cause system hang. From the Cyclone 5 Soc datasheet: applycfg - Write with this bit set to apply all the settings loaded in SDR registers to the memory interface. This bit is write-only and always returns 0 if read. Software should set applycfg bit if change any register under SDR register range. SW is allowed to set this bit multiple times, the only condition is SDRAM need to be idle. My suggestion is we add back socfpga_sdram_apply_static_cfg(), but change the sequence to below: writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); socfpga_sdram_apply_static_cfg(); Marek and Simon, are you okay with this? If yes, I will submit patch for this. Thanks. Regards Ley Foon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 8:50 ` Ley Foon Tan @ 2020-03-09 12:57 ` Marek Vasut 2020-03-09 14:10 ` Simon Goldschmidt 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-03-09 12:57 UTC (permalink / raw) To: u-boot On 3/9/20 9:50 AM, Ley Foon Tan wrote: > On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen > <dalon.westergreen@linux.intel.com> wrote: >> >> I am reading through this thread, and want to point out that it is not that the >> FPGA bridge need be actively used in the fpga, but >> rather that this port be configured in the FPGA configuration. This is an >> important distinction, ecery FPGA design that >> instantiates the HPS does configure the F2S Bridge. it drives select pins, >> control pins, data pins, etc, on the interface to known values, >> and so the apply can always be done as all SoC FPGA designs have the soc >> instantiated. If someone boots the arm, and uses an >> FPGA design without having the soc instantiated, it may then cause issues, but i >> would argue that is not a supported use >> in the first place. >> >> --dalon >> > > I can reproduce the issue if without setting applycfg bit. Access to > F2sdram interface will cause system hang. > > From the Cyclone 5 Soc datasheet: > applycfg - Write with this bit set to apply all the settings loaded in > SDR registers to the memory interface. This bit is write-only and > always returns 0 if read. > > Software should set applycfg bit if change any register under SDR > register range. SW is allowed to set this bit multiple times, the only > condition is SDRAM need to be idle. > My suggestion is we add back socfpga_sdram_apply_static_cfg(), but > change the sequence to below: > writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); > socfpga_sdram_apply_static_cfg(); > > Marek and Simon, are you okay with this? If yes, I will submit patch for this. The problem was that this was causing weird sporadic hangs on Gen5, which is why it was removed. So until there is an explanation for those hangs, I'm not really OK with this. Maybe the application of static config should happen in SPL, before the DRAM is running, or something like that ? ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 12:57 ` Marek Vasut @ 2020-03-09 14:10 ` Simon Goldschmidt 2020-03-09 14:15 ` Marek Vasut 2020-03-11 9:54 ` Ley Foon Tan 0 siblings, 2 replies; 39+ messages in thread From: Simon Goldschmidt @ 2020-03-09 14:10 UTC (permalink / raw) To: u-boot On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: > > On 3/9/20 9:50 AM, Ley Foon Tan wrote: > > On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen > > <dalon.westergreen@linux.intel.com> wrote: > >> > >> I am reading through this thread, and want to point out that it is not that the > >> FPGA bridge need be actively used in the fpga, but > >> rather that this port be configured in the FPGA configuration. This is an > >> important distinction, ecery FPGA design that > >> instantiates the HPS does configure the F2S Bridge. it drives select pins, > >> control pins, data pins, etc, on the interface to known values, > >> and so the apply can always be done as all SoC FPGA designs have the soc > >> instantiated. If someone boots the arm, and uses an > >> FPGA design without having the soc instantiated, it may then cause issues, but i > >> would argue that is not a supported use > >> in the first place. > >> > >> --dalon > >> > > > > I can reproduce the issue if without setting applycfg bit. Access to > > F2sdram interface will cause system hang. > > > > From the Cyclone 5 Soc datasheet: > > applycfg - Write with this bit set to apply all the settings loaded in > > SDR registers to the memory interface. This bit is write-only and > > always returns 0 if read. > > > > Software should set applycfg bit if change any register under SDR > > register range. SW is allowed to set this bit multiple times, the only > > condition is SDRAM need to be idle. > > My suggestion is we add back socfpga_sdram_apply_static_cfg(), but > > change the sequence to below: > > writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); > > socfpga_sdram_apply_static_cfg(); > > > > Marek and Simon, are you okay with this? If yes, I will submit patch for this. > > The problem was that this was causing weird sporadic hangs on Gen5, > which is why it was removed. So until there is an explanation for those > hangs, I'm not really OK with this. These sporadic hangs you saw were on devices without an FPGA image directly accessing the SDRAM ports, right? > > Maybe the application of static config should happen in SPL, before the > DRAM is running, or something like that ? Thinking this further, limiting it to applying in SPL is not a good idea since that prevents us from implementing different FPGA images/configs by loading a config later in the boot (i.e. in U-Boot from a FIT-image). Would it work to make setting this optional, i.e. only write the bit if an FPGA config actually uses these ports? Then it couldn't lead to problems on other hardware... Regards, Simon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 14:10 ` Simon Goldschmidt @ 2020-03-09 14:15 ` Marek Vasut 2020-03-09 14:24 ` Simon Goldschmidt 2020-03-11 9:54 ` Ley Foon Tan 1 sibling, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-03-09 14:15 UTC (permalink / raw) To: u-boot On 3/9/20 3:10 PM, Simon Goldschmidt wrote: > On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: >> >> On 3/9/20 9:50 AM, Ley Foon Tan wrote: >>> On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen >>> <dalon.westergreen@linux.intel.com> wrote: >>>> >>>> I am reading through this thread, and want to point out that it is not that the >>>> FPGA bridge need be actively used in the fpga, but >>>> rather that this port be configured in the FPGA configuration. This is an >>>> important distinction, ecery FPGA design that >>>> instantiates the HPS does configure the F2S Bridge. it drives select pins, >>>> control pins, data pins, etc, on the interface to known values, >>>> and so the apply can always be done as all SoC FPGA designs have the soc >>>> instantiated. If someone boots the arm, and uses an >>>> FPGA design without having the soc instantiated, it may then cause issues, but i >>>> would argue that is not a supported use >>>> in the first place. >>>> >>>> --dalon >>>> >>> >>> I can reproduce the issue if without setting applycfg bit. Access to >>> F2sdram interface will cause system hang. >>> >>> From the Cyclone 5 Soc datasheet: >>> applycfg - Write with this bit set to apply all the settings loaded in >>> SDR registers to the memory interface. This bit is write-only and >>> always returns 0 if read. >>> >>> Software should set applycfg bit if change any register under SDR >>> register range. SW is allowed to set this bit multiple times, the only >>> condition is SDRAM need to be idle. >>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but >>> change the sequence to below: >>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); >>> socfpga_sdram_apply_static_cfg(); >>> >>> Marek and Simon, are you okay with this? If yes, I will submit patch for this. >> >> The problem was that this was causing weird sporadic hangs on Gen5, >> which is why it was removed. So until there is an explanation for those >> hangs, I'm not really OK with this. > > These sporadic hangs you saw were on devices without an FPGA image directly > accessing the SDRAM ports, right? Yes >> Maybe the application of static config should happen in SPL, before the >> DRAM is running, or something like that ? > > Thinking this further, limiting it to applying in SPL is not a good idea since > that prevents us from implementing different FPGA images/configs by loading a > config later in the boot (i.e. in U-Boot from a FIT-image). Well, but later we have SDRAM running and we cannot make it go idle, can we? > Would it work to make setting this optional, i.e. only write the bit if an FPGA > config actually uses these ports? Then it couldn't lead to problems on other > hardware... Can you make SDRAM bus really idle ? ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 14:15 ` Marek Vasut @ 2020-03-09 14:24 ` Simon Goldschmidt 2020-03-09 14:25 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-03-09 14:24 UTC (permalink / raw) To: u-boot On Mon, Mar 9, 2020 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > > On 3/9/20 3:10 PM, Simon Goldschmidt wrote: > > On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 3/9/20 9:50 AM, Ley Foon Tan wrote: > >>> On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen > >>> <dalon.westergreen@linux.intel.com> wrote: > >>>> > >>>> I am reading through this thread, and want to point out that it is not that the > >>>> FPGA bridge need be actively used in the fpga, but > >>>> rather that this port be configured in the FPGA configuration. This is an > >>>> important distinction, ecery FPGA design that > >>>> instantiates the HPS does configure the F2S Bridge. it drives select pins, > >>>> control pins, data pins, etc, on the interface to known values, > >>>> and so the apply can always be done as all SoC FPGA designs have the soc > >>>> instantiated. If someone boots the arm, and uses an > >>>> FPGA design without having the soc instantiated, it may then cause issues, but i > >>>> would argue that is not a supported use > >>>> in the first place. > >>>> > >>>> --dalon > >>>> > >>> > >>> I can reproduce the issue if without setting applycfg bit. Access to > >>> F2sdram interface will cause system hang. > >>> > >>> From the Cyclone 5 Soc datasheet: > >>> applycfg - Write with this bit set to apply all the settings loaded in > >>> SDR registers to the memory interface. This bit is write-only and > >>> always returns 0 if read. > >>> > >>> Software should set applycfg bit if change any register under SDR > >>> register range. SW is allowed to set this bit multiple times, the only > >>> condition is SDRAM need to be idle. > >>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but > >>> change the sequence to below: > >>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); > >>> socfpga_sdram_apply_static_cfg(); > >>> > >>> Marek and Simon, are you okay with this? If yes, I will submit patch for this. > >> > >> The problem was that this was causing weird sporadic hangs on Gen5, > >> which is why it was removed. So until there is an explanation for those > >> hangs, I'm not really OK with this. > > > > These sporadic hangs you saw were on devices without an FPGA image directly > > accessing the SDRAM ports, right? > > Yes > > >> Maybe the application of static config should happen in SPL, before the > >> DRAM is running, or something like that ? > > > > Thinking this further, limiting it to applying in SPL is not a good idea since > > that prevents us from implementing different FPGA images/configs by loading a > > config later in the boot (i.e. in U-Boot from a FIT-image). > > Well, but later we have SDRAM running and we cannot make it go idle, can we? > > > Would it work to make setting this optional, i.e. only write the bit if an FPGA > > config actually uses these ports? Then it couldn't lead to problems on other > > hardware... > > Can you make SDRAM bus really idle ? From the CPU side, that should work, no? Of course you have to make sure no other peripheraly (including FPGA!) are using the RAM. And if this would be an explicit command, people needing this could experiment with it - and hopefully give better hints as to what's going wrong if we *do* see problems again. Regards, Simon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 14:24 ` Simon Goldschmidt @ 2020-03-09 14:25 ` Marek Vasut 2020-03-12 15:54 ` Dalon L Westergreen 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-03-09 14:25 UTC (permalink / raw) To: u-boot On 3/9/20 3:24 PM, Simon Goldschmidt wrote: > On Mon, Mar 9, 2020 at 3:15 PM Marek Vasut <marex@denx.de> wrote: >> >> On 3/9/20 3:10 PM, Simon Goldschmidt wrote: >>> On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 3/9/20 9:50 AM, Ley Foon Tan wrote: >>>>> On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen >>>>> <dalon.westergreen@linux.intel.com> wrote: >>>>>> >>>>>> I am reading through this thread, and want to point out that it is not that the >>>>>> FPGA bridge need be actively used in the fpga, but >>>>>> rather that this port be configured in the FPGA configuration. This is an >>>>>> important distinction, ecery FPGA design that >>>>>> instantiates the HPS does configure the F2S Bridge. it drives select pins, >>>>>> control pins, data pins, etc, on the interface to known values, >>>>>> and so the apply can always be done as all SoC FPGA designs have the soc >>>>>> instantiated. If someone boots the arm, and uses an >>>>>> FPGA design without having the soc instantiated, it may then cause issues, but i >>>>>> would argue that is not a supported use >>>>>> in the first place. >>>>>> >>>>>> --dalon >>>>>> >>>>> >>>>> I can reproduce the issue if without setting applycfg bit. Access to >>>>> F2sdram interface will cause system hang. >>>>> >>>>> From the Cyclone 5 Soc datasheet: >>>>> applycfg - Write with this bit set to apply all the settings loaded in >>>>> SDR registers to the memory interface. This bit is write-only and >>>>> always returns 0 if read. >>>>> >>>>> Software should set applycfg bit if change any register under SDR >>>>> register range. SW is allowed to set this bit multiple times, the only >>>>> condition is SDRAM need to be idle. >>>>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but >>>>> change the sequence to below: >>>>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); >>>>> socfpga_sdram_apply_static_cfg(); >>>>> >>>>> Marek and Simon, are you okay with this? If yes, I will submit patch for this. >>>> >>>> The problem was that this was causing weird sporadic hangs on Gen5, >>>> which is why it was removed. So until there is an explanation for those >>>> hangs, I'm not really OK with this. >>> >>> These sporadic hangs you saw were on devices without an FPGA image directly >>> accessing the SDRAM ports, right? >> >> Yes >> >>>> Maybe the application of static config should happen in SPL, before the >>>> DRAM is running, or something like that ? >>> >>> Thinking this further, limiting it to applying in SPL is not a good idea since >>> that prevents us from implementing different FPGA images/configs by loading a >>> config later in the boot (i.e. in U-Boot from a FIT-image). >> >> Well, but later we have SDRAM running and we cannot make it go idle, can we? >> >>> Would it work to make setting this optional, i.e. only write the bit if an FPGA >>> config actually uses these ports? Then it couldn't lead to problems on other >>> hardware... >> >> Can you make SDRAM bus really idle ? > > From the CPU side, that should work, no? Of course you have to make sure no > other peripheraly (including FPGA!) are using the RAM. > > And if this would be an explicit command, people needing this could > experiment with it - and hopefully give better hints as to what's going wrong > if we *do* see problems again. Maybe altera has something hidden somewhere in the bus tunables ? :) ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 14:25 ` Marek Vasut @ 2020-03-12 15:54 ` Dalon L Westergreen 2020-03-12 15:57 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Dalon L Westergreen @ 2020-03-12 15:54 UTC (permalink / raw) To: u-boot On Mon, 2020-03-09 at 15:25 +0100, Marek Vasut wrote: > On 3/9/20 3:24 PM, Simon Goldschmidt wrote: > > On Mon, Mar 9, 2020 at 3:15 PM Marek Vasut <marex@denx.de> wrote: > > > On 3/9/20 3:10 PM, Simon Goldschmidt wrote: > > > > On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: > > > > > On 3/9/20 9:50 AM, Ley Foon Tan wrote: > > > > > > On Thu, Feb 13, 2020 at 2:52 AM Dalon L Westergreen > > > > > > <dalon.westergreen@linux.intel.com> wrote: > > > > > > > I am reading through this thread, and want to point out that it is > > > > > > > not that the > > > > > > > FPGA bridge need be actively used in the fpga, but > > > > > > > rather that this port be configured in the FPGA > > > > > > > configuration. This is an > > > > > > > important distinction, ecery FPGA design that > > > > > > > instantiates the HPS does configure the F2S Bridge. it drives > > > > > > > select pins, > > > > > > > control pins, data pins, etc, on the interface to known values, > > > > > > > and so the apply can always be done as all SoC FPGA designs have > > > > > > > the soc > > > > > > > instantiated. If someone boots the arm, and uses an > > > > > > > FPGA design without having the soc instantiated, it may then cause > > > > > > > issues, but i > > > > > > > would argue that is not a supported use > > > > > > > in the first place. > > > > > > > > > > > > > > --dalon > > > > > > > > > > > > > > > > > > > I can reproduce the issue if without setting applycfg bit. Access to > > > > > > F2sdram interface will cause system hang. > > > > > > > > > > > > From the Cyclone 5 Soc datasheet: > > > > > > applycfg - Write with this bit set to apply all the settings loaded > > > > > > in > > > > > > SDR registers to the memory interface. This bit is write-only and > > > > > > always returns 0 if read. > > > > > > > > > > > > Software should set applycfg bit if change any register under SDR > > > > > > register range. SW is allowed to set this bit multiple times, the > > > > > > only > > > > > > condition is SDRAM need to be idle. > > > > > > My suggestion is we add back socfpga_sdram_apply_static_cfg(), but > > > > > > change the sequence to below: > > > > > > writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); > > > > > > socfpga_sdram_apply_static_cfg(); > > > > > > > > > > > > Marek and Simon, are you okay with this? If yes, I will submit patch > > > > > > for this. > > > > > > > > > > The problem was that this was causing weird sporadic hangs on Gen5, > > > > > which is why it was removed. So until there is an explanation for > > > > > those > > > > > hangs, I'm not really OK with this. > > > > > > > > These sporadic hangs you saw were on devices without an FPGA image > > > > directly > > > > accessing the SDRAM ports, right? > > > > > > Yes > > > > > > > > Maybe the application of static config should happen in SPL, before > > > > > the > > > > > DRAM is running, or something like that ? > > > > > > > > Thinking this further, limiting it to applying in SPL is not a good idea > > > > since > > > > that prevents us from implementing different FPGA images/configs by > > > > loading a > > > > config later in the boot (i.e. in U-Boot from a FIT-image). > > > > > > Well, but later we have SDRAM running and we cannot make it go idle, can > > > we? > > > Unfortunately the sdram cfg apply must occur AFTER the fpga is 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. > > > > Would it work to make setting this optional, i.e. only write the bit if > > > > an FPGA > > > > config actually uses these ports? Then it couldn't lead to problems on > > > > other > > > > hardware... > > > > > > Can you make SDRAM bus really idle ? > > > > From the CPU side, that should work, no? Of course you have to make sure no > > other peripheraly (including FPGA!) are using the RAM. > > > > And if this would be an explicit command, people needing this could > > experiment with it - and hopefully give better hints as to what's going > > wrong > > if we *do* see problems again. > > Maybe altera has something hidden somewhere in the bus tunables ? :) The only trick i am aware of, and Ley Foon, please comment, is isolating relevant code to the caches before executing. --dalon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-12 15:54 ` Dalon L Westergreen @ 2020-03-12 15:57 ` Marek Vasut 2020-03-16 18:04 ` Dalon L Westergreen 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-03-12 15:57 UTC (permalink / raw) To: u-boot On 3/12/20 4:54 PM, Dalon L Westergreen wrote: [...] (thanks for fixing your mailer :)) >>>>>> >>>>>> The problem was that this was causing weird sporadic hangs on Gen5, >>>>>> which is why it was removed. So until there is an explanation for >>>>>> those >>>>>> hangs, I'm not really OK with this. >>>>> >>>>> These sporadic hangs you saw were on devices without an FPGA image >>>>> directly >>>>> accessing the SDRAM ports, right? >>>> >>>> Yes >>>> >>>>>> Maybe the application of static config should happen in SPL, before >>>>>> the >>>>>> DRAM is running, or something like that ? >>>>> >>>>> Thinking this further, limiting it to applying in SPL is not a good idea >>>>> since >>>>> that prevents us from implementing different FPGA images/configs by >>>>> loading a >>>>> config later in the boot (i.e. in U-Boot from a FIT-image). >>>> >>>> Well, but later we have SDRAM running and we cannot make it go idle, can >>>> we? >>>> > > Unfortunately the sdram cfg apply must occur AFTER the fpga is 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. >>>>> Would it work to make setting this optional, i.e. only write the bit if >>>>> an FPGA >>>>> config actually uses these ports? Then it couldn't lead to problems on >>>>> other >>>>> hardware... >>>> >>>> Can you make SDRAM bus really idle ? >>> >>> From the CPU side, that should work, no? Of course you have to make sure no >>> other peripheraly (including FPGA!) are using the RAM. >>> >>> And if this would be an explicit command, people needing this could >>> experiment with it - and hopefully give better hints as to what's going >>> wrong >>> if we *do* see problems again. >> >> Maybe altera has something hidden somewhere in the bus tunables ? :) > > The only trick i am aware of, and Ley Foon, please comment, is isolating > relevant code to the caches before executing. How do you make sure some DMA doesn't do something funny or some pending write doesn't trigger DRAM write ? There is more than the CPU that can access the DRAM and cause bus traffic. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-12 15:57 ` Marek Vasut @ 2020-03-16 18:04 ` Dalon L Westergreen 2020-03-16 19:04 ` Simon Goldschmidt 0 siblings, 1 reply; 39+ messages in thread From: Dalon L Westergreen @ 2020-03-16 18:04 UTC (permalink / raw) To: u-boot On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: > On 3/12/20 4:54 PM, Dalon L Westergreen wrote: > [...] > > (thanks for fixing your mailer :)) > > > > > > > > The problem was that this was causing weird sporadic hangs on > > > > > > > Gen5, > > > > > > > which is why it was removed. So until there is an explanation for > > > > > > > those > > > > > > > hangs, I'm not really OK with this. > > > > > > > > > > > > These sporadic hangs you saw were on devices without an FPGA image > > > > > > directly > > > > > > accessing the SDRAM ports, right? > > > > > > > > > > Yes > > > > > > > > > > > > Maybe the application of static config should happen in SPL, > > > > > > > before > > > > > > > the > > > > > > > DRAM is running, or something like that ? > > > > > > > > > > > > Thinking this further, limiting it to applying in SPL is not a good > > > > > > idea > > > > > > since > > > > > > that prevents us from implementing different FPGA images/configs by > > > > > > loading a > > > > > > config later in the boot (i.e. in U-Boot from a FIT-image). > > > > > > > > > > Well, but later we have SDRAM running and we cannot make it go idle, > > > > > can > > > > > we? > > > > > > > > > Unfortunately the sdram cfg apply must occur AFTER the fpga is > > 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. > > > > > > > Would it work to make setting this optional, i.e. only write the bit > > > > > > if > > > > > > an FPGA > > > > > > config actually uses these ports? Then it couldn't lead to problems > > > > > > on > > > > > > other > > > > > > hardware... > > > > > > > > > > Can you make SDRAM bus really idle ? > > > > > > > > From the CPU side, that should work, no? Of course you have to make sure > > > > no > > > > other peripheraly (including FPGA!) are using the RAM. > > > > > > > > And if this would be an explicit command, people needing this could > > > > experiment with it - and hopefully give better hints as to what's going > > > > wrong > > > > if we *do* see problems again. > > > > > > Maybe altera has something hidden somewhere in the bus tunables ? :) > > > > The only trick i am aware of, and Ley Foon, please comment, is isolating > > relevant code to the caches before executing. > > How do you make sure some DMA doesn't do something funny or some pending > write doesn't trigger DRAM write ? There is more than the CPU that can > access the DRAM and cause bus traffic. True, and it is unclear how we could ensure there is absolutely no traffic. --dalon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 18:04 ` Dalon L Westergreen @ 2020-03-16 19:04 ` Simon Goldschmidt 2020-03-16 19:06 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-03-16 19:04 UTC (permalink / raw) To: u-boot Am 16.03.2020 um 19:04 schrieb Dalon L Westergreen: > > > On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: >> On 3/12/20 4:54 PM, Dalon L Westergreen wrote: >> [...] >> >> (thanks for fixing your mailer :)) >> >>>>>>>> The problem was that this was causing weird sporadic hangs on >>>>>>>> Gen5, >>>>>>>> which is why it was removed. So until there is an explanation for >>>>>>>> those >>>>>>>> hangs, I'm not really OK with this. >>>>>>> >>>>>>> These sporadic hangs you saw were on devices without an FPGA image >>>>>>> directly >>>>>>> accessing the SDRAM ports, right? >>>>>> >>>>>> Yes >>>>>> >>>>>>>> Maybe the application of static config should happen in SPL, >>>>>>>> before >>>>>>>> the >>>>>>>> DRAM is running, or something like that ? >>>>>>> >>>>>>> Thinking this further, limiting it to applying in SPL is not a good >>>>>>> idea >>>>>>> since >>>>>>> that prevents us from implementing different FPGA images/configs by >>>>>>> loading a >>>>>>> config later in the boot (i.e. in U-Boot from a FIT-image). >>>>>> >>>>>> Well, but later we have SDRAM running and we cannot make it go idle, >>>>>> can >>>>>> we? >>>>>> >>> >>> Unfortunately the sdram cfg apply must occur AFTER the fpga is >>> 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. Today, too much configuration is applied at compile time (or when programming SPL to flash at latest) - there's currently no way to switch peripherals to FPGA for one image but not for another, for example. Worse: if you enable bridges but there's no slave attached, the CPU can be stuck. That would need to be described with the FPGA image as well. Regards, Simon > >> >>>>>>> Would it work to make setting this optional, i.e. only write the bit >>>>>>> if >>>>>>> an FPGA >>>>>>> config actually uses these ports? Then it couldn't lead to problems >>>>>>> on >>>>>>> other >>>>>>> hardware... >>>>>> >>>>>> Can you make SDRAM bus really idle ? >>>>> >>>>> From the CPU side, that should work, no? Of course you have to make sure >>>>> no >>>>> other peripheraly (including FPGA!) are using the RAM. >>>>> >>>>> And if this would be an explicit command, people needing this could >>>>> experiment with it - and hopefully give better hints as to what's going >>>>> wrong >>>>> if we *do* see problems again. >>>> >>>> Maybe altera has something hidden somewhere in the bus tunables ? :) >>> >>> The only trick i am aware of, and Ley Foon, please comment, is isolating >>> relevant code to the caches before executing. >> >> How do you make sure some DMA doesn't do something funny or some pending >> write doesn't trigger DRAM write ? There is more than the CPU that can >> access the DRAM and cause bus traffic. > > True, and it is unclear how we could ensure there is absolutely no traffic. > > --dalon > > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 19:04 ` Simon Goldschmidt @ 2020-03-16 19:06 ` Marek Vasut 2020-03-16 19:09 ` Simon Goldschmidt 2020-03-16 19:55 ` Dalon L Westergreen 0 siblings, 2 replies; 39+ messages in thread From: Marek Vasut @ 2020-03-16 19:06 UTC (permalink / raw) To: u-boot On 3/16/20 8:04 PM, Simon Goldschmidt wrote: > Am 16.03.2020 um 19:04 schrieb Dalon L Westergreen: >> >> >> On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: >>> On 3/12/20 4:54 PM, Dalon L Westergreen wrote: >>> [...] >>> >>> (thanks for fixing your mailer :)) >>> >>>>>>>>> The problem was that this was causing weird sporadic hangs on >>>>>>>>> Gen5, >>>>>>>>> which is why it was removed. So until there is an explanation for >>>>>>>>> those >>>>>>>>> hangs, I'm not really OK with this. >>>>>>>> >>>>>>>> These sporadic hangs you saw were on devices without an FPGA image >>>>>>>> directly >>>>>>>> accessing the SDRAM ports, right? >>>>>>> >>>>>>> Yes >>>>>>> >>>>>>>>> Maybe the application of static config should happen in SPL, >>>>>>>>> before >>>>>>>>> the >>>>>>>>> DRAM is running, or something like that ? >>>>>>>> >>>>>>>> Thinking this further, limiting it to applying in SPL is not a good >>>>>>>> idea >>>>>>>> since >>>>>>>> that prevents us from implementing different FPGA images/configs by >>>>>>>> loading a >>>>>>>> config later in the boot (i.e. in U-Boot from a FIT-image). >>>>>>> >>>>>>> Well, but later we have SDRAM running and we cannot make it go idle, >>>>>>> can >>>>>>> we? >>>>>>> >>>> >>>> Unfortunately the sdram cfg apply must occur AFTER the fpga is >>>> 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 ? > Today, too much > configuration is applied at compile time (or when programming SPL to > flash at latest) - there's currently no way to switch peripherals to > FPGA for one image but not for another, for example. Yes > Worse: if you enable bridges but there's no slave attached, the CPU can > be stuck. That would need to be described with the FPGA image as well. Can you propose a patch ? -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 19:06 ` Marek Vasut @ 2020-03-16 19:09 ` Simon Goldschmidt 2020-03-16 19:19 ` Marek Vasut 2020-03-16 19:55 ` Dalon L Westergreen 1 sibling, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-03-16 19:09 UTC (permalink / raw) To: u-boot Am 16.03.2020 um 20:06 schrieb Marek Vasut: > On 3/16/20 8:04 PM, Simon Goldschmidt wrote: >> Am 16.03.2020 um 19:04 schrieb Dalon L Westergreen: >>> >>> >>> On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: >>>> On 3/12/20 4:54 PM, Dalon L Westergreen wrote: >>>> [...] >>>> >>>> (thanks for fixing your mailer :)) >>>> >>>>>>>>>> The problem was that this was causing weird sporadic hangs on >>>>>>>>>> Gen5, >>>>>>>>>> which is why it was removed. So until there is an explanation for >>>>>>>>>> those >>>>>>>>>> hangs, I'm not really OK with this. >>>>>>>>> >>>>>>>>> These sporadic hangs you saw were on devices without an FPGA image >>>>>>>>> directly >>>>>>>>> accessing the SDRAM ports, right? >>>>>>>> >>>>>>>> Yes >>>>>>>> >>>>>>>>>> Maybe the application of static config should happen in SPL, >>>>>>>>>> before >>>>>>>>>> the >>>>>>>>>> DRAM is running, or something like that ? >>>>>>>>> >>>>>>>>> Thinking this further, limiting it to applying in SPL is not a good >>>>>>>>> idea >>>>>>>>> since >>>>>>>>> that prevents us from implementing different FPGA images/configs by >>>>>>>>> loading a >>>>>>>>> config later in the boot (i.e. in U-Boot from a FIT-image). >>>>>>>> >>>>>>>> Well, but later we have SDRAM running and we cannot make it go idle, >>>>>>>> can >>>>>>>> we? >>>>>>>> >>>>> >>>>> Unfortunately the sdram cfg apply must occur AFTER the fpga is >>>>> 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 ? > >> Today, too much >> configuration is applied at compile time (or when programming SPL to >> flash at latest) - there's currently no way to switch peripherals to >> FPGA for one image but not for another, for example. > > Yes > >> Worse: if you enable bridges but there's no slave attached, the CPU can >> be stuck. That would need to be described with the FPGA image as well. > > Can you propose a patch ? In corona times and with kids now at home, I don't know if I can soon :-( Regards, Simon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 19:09 ` Simon Goldschmidt @ 2020-03-16 19:19 ` Marek Vasut 0 siblings, 0 replies; 39+ messages in thread From: Marek Vasut @ 2020-03-16 19:19 UTC (permalink / raw) To: u-boot On 3/16/20 8:09 PM, Simon Goldschmidt wrote: [...] >>> Today, too much >>> configuration is applied at compile time (or when programming SPL to >>> flash at latest) - there's currently no way to switch peripherals to >>> FPGA for one image but not for another, for example. >> >> Yes >> >>> Worse: if you enable bridges but there's no slave attached, the CPU can >>> be stuck. That would need to be described with the FPGA image as well. >> >> Can you propose a patch ? > > In corona times and with kids now at home, I don't know if I can soon :-( The release is far away anyway :) We're already on full lockdown, hopefully it goes away soon. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 19:06 ` Marek Vasut 2020-03-16 19:09 ` Simon Goldschmidt @ 2020-03-16 19:55 ` Dalon L Westergreen 2020-03-16 20:01 ` Simon Goldschmidt 1 sibling, 1 reply; 39+ messages in thread From: Dalon L Westergreen @ 2020-03-16 19:55 UTC (permalink / raw) To: u-boot On Mon, 2020-03-16 at 20:06 +0100, Marek Vasut wrote: > On 3/16/20 8:04 PM, Simon Goldschmidt wrote: > > Am 16.03.2020 um 19:04 schrieb Dalon L Westergreen: > > > > > > On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: > > > > On 3/12/20 4:54 PM, Dalon L Westergreen wrote: > > > > [...] > > > > > > > > (thanks for fixing your mailer :)) > > > > > > > > > > > > > > The problem was that this was causing weird sporadic hangs > > > > > > > > > > on > > > > > > > > > > Gen5, > > > > > > > > > > which is why it was removed. So until there is an > > > > > > > > > > explanation for > > > > > > > > > > those > > > > > > > > > > hangs, I'm not really OK with this. > > > > > > > > > > > > > > > > > > These sporadic hangs you saw were on devices without an FPGA > > > > > > > > > image > > > > > > > > > directly > > > > > > > > > accessing the SDRAM ports, right? > > > > > > > > > > > > > > > > Yes > > > > > > > > > > > > > > > > > > Maybe the application of static config should happen in SPL, > > > > > > > > > > before > > > > > > > > > > the > > > > > > > > > > DRAM is running, or something like that ? > > > > > > > > > > > > > > > > > > Thinking this further, limiting it to applying in SPL is not a > > > > > > > > > good > > > > > > > > > idea > > > > > > > > > since > > > > > > > > > that prevents us from implementing different FPGA > > > > > > > > > images/configs by > > > > > > > > > loading a > > > > > > > > > config later in the boot (i.e. in U-Boot from a FIT-image). > > > > > > > > > > > > > > > > Well, but later we have SDRAM running and we cannot make it go > > > > > > > > idle, > > > > > > > > can > > > > > > > > we? > > > > > > > > > > > > > > > > > > Unfortunately the sdram cfg apply must occur AFTER the fpga is > > > > > 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. > > > Today, too much > > configuration is applied at compile time (or when programming SPL to > > flash at latest) - there's currently no way to switch peripherals to > > FPGA for one image but not for another, for example. > > Yes > > > Worse: if you enable bridges but there's no slave attached, the CPU can > > be stuck. That would need to be described with the FPGA image as well. > > Can you propose a patch ? > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 19:55 ` Dalon L Westergreen @ 2020-03-16 20:01 ` Simon Goldschmidt 2020-03-20 7:52 ` Ley Foon Tan 0 siblings, 1 reply; 39+ messages in thread From: Simon Goldschmidt @ 2020-03-16 20:01 UTC (permalink / raw) To: u-boot Am 16.03.2020 um 20:55 schrieb Dalon L Westergreen: > > > On Mon, 2020-03-16 at 20:06 +0100, Marek Vasut wrote: >> On 3/16/20 8:04 PM, Simon Goldschmidt wrote: >>> Am 16.03.2020 um 19:04 schrieb Dalon L Westergreen: >>>> >>>> On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: >>>>> On 3/12/20 4:54 PM, Dalon L Westergreen wrote: >>>>> [...] >>>>> >>>>> (thanks for fixing your mailer :)) >>>>> >>>>>>>>>>> The problem was that this was causing weird sporadic hangs >>>>>>>>>>> on >>>>>>>>>>> Gen5, >>>>>>>>>>> which is why it was removed. So until there is an >>>>>>>>>>> explanation for >>>>>>>>>>> those >>>>>>>>>>> hangs, I'm not really OK with this. >>>>>>>>>> >>>>>>>>>> These sporadic hangs you saw were on devices without an FPGA >>>>>>>>>> image >>>>>>>>>> directly >>>>>>>>>> accessing the SDRAM ports, right? >>>>>>>>> >>>>>>>>> Yes >>>>>>>>> >>>>>>>>>>> Maybe the application of static config should happen in SPL, >>>>>>>>>>> before >>>>>>>>>>> the >>>>>>>>>>> DRAM is running, or something like that ? >>>>>>>>>> >>>>>>>>>> Thinking this further, limiting it to applying in SPL is not a >>>>>>>>>> good >>>>>>>>>> idea >>>>>>>>>> since >>>>>>>>>> that prevents us from implementing different FPGA >>>>>>>>>> images/configs by >>>>>>>>>> loading a >>>>>>>>>> config later in the boot (i.e. in U-Boot from a FIT-image). >>>>>>>>> >>>>>>>>> Well, but later we have SDRAM running and we cannot make it go >>>>>>>>> idle, >>>>>>>>> can >>>>>>>>> we? >>>>>>>>> >>>>>> >>>>>> Unfortunately the sdram cfg apply must occur AFTER the fpga is >>>>>> 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... Regards, Simon > >> >>> Today, too much >>> configuration is applied at compile time (or when programming SPL to >>> flash at latest) - there's currently no way to switch peripherals to >>> FPGA for one image but not for another, for example. >> >> Yes >> >>> Worse: if you enable bridges but there's no slave attached, the CPU can >>> be stuck. That would need to be described with the FPGA image as well. >> >> Can you propose a patch ? >> > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-16 20:01 ` Simon Goldschmidt @ 2020-03-20 7:52 ` Ley Foon Tan 2020-03-31 8:13 ` Ley Foon Tan 0 siblings, 1 reply; 39+ messages in thread From: Ley Foon Tan @ 2020-03-20 7:52 UTC (permalink / raw) To: u-boot On Tue, Mar 17, 2020 at 4:01 AM Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > > Am 16.03.2020 um 20:55 schrieb Dalon L Westergreen: > > > > > > On Mon, 2020-03-16 at 20:06 +0100, Marek Vasut wrote: > >> On 3/16/20 8:04 PM, Simon Goldschmidt wrote: > >>> Am 16.03.2020 um 19:04 schrieb Dalon L Westergreen: > >>>> > >>>> On Thu, 2020-03-12 at 16:57 +0100, Marek Vasut wrote: > >>>>> On 3/12/20 4:54 PM, Dalon L Westergreen wrote: > >>>>> [...] > >>>>> > >>>>> (thanks for fixing your mailer :)) > >>>>> > >>>>>>>>>>> The problem was that this was causing weird sporadic hangs > >>>>>>>>>>> on > >>>>>>>>>>> Gen5, > >>>>>>>>>>> which is why it was removed. So until there is an > >>>>>>>>>>> explanation for > >>>>>>>>>>> those > >>>>>>>>>>> hangs, I'm not really OK with this. > >>>>>>>>>> > >>>>>>>>>> These sporadic hangs you saw were on devices without an FPGA > >>>>>>>>>> image > >>>>>>>>>> directly > >>>>>>>>>> accessing the SDRAM ports, right? > >>>>>>>>> > >>>>>>>>> Yes > >>>>>>>>> > >>>>>>>>>>> Maybe the application of static config should happen in SPL, > >>>>>>>>>>> before > >>>>>>>>>>> the > >>>>>>>>>>> DRAM is running, or something like that ? > >>>>>>>>>> > >>>>>>>>>> Thinking this further, limiting it to applying in SPL is not a > >>>>>>>>>> good > >>>>>>>>>> idea > >>>>>>>>>> since > >>>>>>>>>> that prevents us from implementing different FPGA > >>>>>>>>>> images/configs by > >>>>>>>>>> loading a > >>>>>>>>>> config later in the boot (i.e. in U-Boot from a FIT-image). > >>>>>>>>> > >>>>>>>>> Well, but later we have SDRAM running and we cannot make it go > >>>>>>>>> idle, > >>>>>>>>> can > >>>>>>>>> we? > >>>>>>>>> > >>>>>> > >>>>>> Unfortunately the sdram cfg apply must occur AFTER the fpga is > >>>>>> 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-20 7:52 ` Ley Foon Tan @ 2020-03-31 8:13 ` Ley Foon Tan 2020-03-31 11:27 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Ley Foon Tan @ 2020-03-31 8:13 UTC (permalink / raw) To: u-boot On Fri, Mar 20, 2020 at 3:52 PM Ley Foon Tan <lftan.linux@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. Thanks. Regards Ley Foon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-31 8:13 ` Ley Foon Tan @ 2020-03-31 11:27 ` Marek Vasut 0 siblings, 0 replies; 39+ messages in thread From: Marek Vasut @ 2020-03-31 11:27 UTC (permalink / raw) To: u-boot 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@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. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-09 14:10 ` Simon Goldschmidt 2020-03-09 14:15 ` Marek Vasut @ 2020-03-11 9:54 ` Ley Foon Tan 2020-03-11 9:58 ` Marek Vasut 1 sibling, 1 reply; 39+ messages in thread From: Ley Foon Tan @ 2020-03-11 9:54 UTC (permalink / raw) To: u-boot On Mon, Mar 9, 2020 at 10:10 PM Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > > On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: > > > > >> > > > > > > I can reproduce the issue if without setting applycfg bit. Access to > > > F2sdram interface will cause system hang. > > > > > > From the Cyclone 5 Soc datasheet: > > > applycfg - Write with this bit set to apply all the settings loaded in > > > SDR registers to the memory interface. This bit is write-only and > > > always returns 0 if read. > > > > > > Software should set applycfg bit if change any register under SDR > > > register range. SW is allowed to set this bit multiple times, the only > > > condition is SDRAM need to be idle. > > > My suggestion is we add back socfpga_sdram_apply_static_cfg(), but > > > change the sequence to below: > > > writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); > > > socfpga_sdram_apply_static_cfg(); > > > > > > Marek and Simon, are you okay with this? If yes, I will submit patch for this. > > > > The problem was that this was causing weird sporadic hangs on Gen5, > > which is why it was removed. So until there is an explanation for those > > hangs, I'm not really OK with this. > > These sporadic hangs you saw were on devices without an FPGA image directly > accessing the SDRAM ports, right? > > > > > Maybe the application of static config should happen in SPL, before the > > DRAM is running, or something like that ? > > Thinking this further, limiting it to applying in SPL is not a good idea since > that prevents us from implementing different FPGA images/configs by loading a > config later in the boot (i.e. in U-Boot from a FIT-image). > > Would it work to make setting this optional, i.e. only write the bit if an FPGA > config actually uses these ports? Then it couldn't lead to problems on other > hardware... If the sporadic hangs issue happen on FPGA image that doesn't connect to f2sdram interface, we can add the checking to only apply static cfg when it has f2sdram port enabled. It can send a patch with this checking for you to try if want. Regards Ley Foon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-11 9:54 ` Ley Foon Tan @ 2020-03-11 9:58 ` Marek Vasut 2020-03-12 9:31 ` Ley Foon Tan 0 siblings, 1 reply; 39+ messages in thread From: Marek Vasut @ 2020-03-11 9:58 UTC (permalink / raw) To: u-boot On 3/11/20 10:54 AM, Ley Foon Tan wrote: > On Mon, Mar 9, 2020 at 10:10 PM Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com> wrote: >> >> On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: >>> > >>>>> >>>> >>>> I can reproduce the issue if without setting applycfg bit. Access to >>>> F2sdram interface will cause system hang. >>>> >>>> From the Cyclone 5 Soc datasheet: >>>> applycfg - Write with this bit set to apply all the settings loaded in >>>> SDR registers to the memory interface. This bit is write-only and >>>> always returns 0 if read. >>>> >>>> Software should set applycfg bit if change any register under SDR >>>> register range. SW is allowed to set this bit multiple times, the only >>>> condition is SDRAM need to be idle. >>>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but >>>> change the sequence to below: >>>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); >>>> socfpga_sdram_apply_static_cfg(); >>>> >>>> Marek and Simon, are you okay with this? If yes, I will submit patch for this. >>> >>> The problem was that this was causing weird sporadic hangs on Gen5, >>> which is why it was removed. So until there is an explanation for those >>> hangs, I'm not really OK with this. >> >> These sporadic hangs you saw were on devices without an FPGA image directly >> accessing the SDRAM ports, right? >> >>> >>> Maybe the application of static config should happen in SPL, before the >>> DRAM is running, or something like that ? >> >> Thinking this further, limiting it to applying in SPL is not a good idea since >> that prevents us from implementing different FPGA images/configs by loading a >> config later in the boot (i.e. in U-Boot from a FIT-image). >> >> Would it work to make setting this optional, i.e. only write the bit if an FPGA >> config actually uses these ports? Then it couldn't lead to problems on other >> hardware... > > If the sporadic hangs issue happen on FPGA image that doesn't connect > to f2sdram interface, we can add the checking to only apply static cfg > when it has f2sdram port enabled. > It can send a patch with this checking for you to try if want. Well, what could cause that sporadic hang ? Yes, the hang happens on image which doesn't access the SDRAM IIRC. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-11 9:58 ` Marek Vasut @ 2020-03-12 9:31 ` Ley Foon Tan 2020-03-12 11:34 ` Marek Vasut 0 siblings, 1 reply; 39+ messages in thread From: Ley Foon Tan @ 2020-03-12 9:31 UTC (permalink / raw) To: u-boot On Wed, Mar 11, 2020 at 5:58 PM Marek Vasut <marex@denx.de> wrote: > > On 3/11/20 10:54 AM, Ley Foon Tan wrote: > > On Mon, Mar 9, 2020 at 10:10 PM Simon Goldschmidt > > <simon.k.r.goldschmidt@gmail.com> wrote: > >> > >> On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: > >>> > > > >>>>> > >>>> > >>>> I can reproduce the issue if without setting applycfg bit. Access to > >>>> F2sdram interface will cause system hang. > >>>> > >>>> From the Cyclone 5 Soc datasheet: > >>>> applycfg - Write with this bit set to apply all the settings loaded in > >>>> SDR registers to the memory interface. This bit is write-only and > >>>> always returns 0 if read. > >>>> > >>>> Software should set applycfg bit if change any register under SDR > >>>> register range. SW is allowed to set this bit multiple times, the only > >>>> condition is SDRAM need to be idle. > >>>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but > >>>> change the sequence to below: > >>>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); > >>>> socfpga_sdram_apply_static_cfg(); > >>>> > >>>> Marek and Simon, are you okay with this? If yes, I will submit patch for this. > >>> > >>> The problem was that this was causing weird sporadic hangs on Gen5, > >>> which is why it was removed. So until there is an explanation for those > >>> hangs, I'm not really OK with this. > >> > >> These sporadic hangs you saw were on devices without an FPGA image directly > >> accessing the SDRAM ports, right? > >> > >>> > >>> Maybe the application of static config should happen in SPL, before the > >>> DRAM is running, or something like that ? > >> > >> Thinking this further, limiting it to applying in SPL is not a good idea since > >> that prevents us from implementing different FPGA images/configs by loading a > >> config later in the boot (i.e. in U-Boot from a FIT-image). > >> > >> Would it work to make setting this optional, i.e. only write the bit if an FPGA > >> config actually uses these ports? Then it couldn't lead to problems on other > >> hardware... > > > > If the sporadic hangs issue happen on FPGA image that doesn't connect > > to f2sdram interface, we can add the checking to only apply static cfg > > when it has f2sdram port enabled. > > It can send a patch with this checking for you to try if want. > Well, what could cause that sporadic hang ? > Yes, the hang happens on image which doesn't access the SDRAM IIRC. > Do you remember if the hang happen when run bridge enable/disable command? Or other flow? I have tested the following sequence 500 times with FPGA image that doesn't contains fs2dram bridge, but didn't see the hang. - program fpga image - bridge enable - write to h2f bridge - read from h2f bridge - bridge disable - repeat step 1 Regards Ley Foon ^ permalink raw reply [flat|nested] 39+ messages in thread
* [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() 2020-03-12 9:31 ` Ley Foon Tan @ 2020-03-12 11:34 ` Marek Vasut 0 siblings, 0 replies; 39+ messages in thread From: Marek Vasut @ 2020-03-12 11:34 UTC (permalink / raw) To: u-boot On 3/12/20 10:31 AM, Ley Foon Tan wrote: > On Wed, Mar 11, 2020 at 5:58 PM Marek Vasut <marex@denx.de> wrote: >> >> On 3/11/20 10:54 AM, Ley Foon Tan wrote: >>> On Mon, Mar 9, 2020 at 10:10 PM Simon Goldschmidt >>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>> >>>> On Mon, Mar 9, 2020 at 1:57 PM Marek Vasut <marex@denx.de> wrote: >>>>> >>> >>>>>>> >>>>>> >>>>>> I can reproduce the issue if without setting applycfg bit. Access to >>>>>> F2sdram interface will cause system hang. >>>>>> >>>>>> From the Cyclone 5 Soc datasheet: >>>>>> applycfg - Write with this bit set to apply all the settings loaded in >>>>>> SDR registers to the memory interface. This bit is write-only and >>>>>> always returns 0 if read. >>>>>> >>>>>> Software should set applycfg bit if change any register under SDR >>>>>> register range. SW is allowed to set this bit multiple times, the only >>>>>> condition is SDRAM need to be idle. >>>>>> My suggestion is we add back socfpga_sdram_apply_static_cfg(), but >>>>>> change the sequence to below: >>>>>> writel(iswgrp_handoff[3], &sdr_ctrl->fpgaport_rst); >>>>>> socfpga_sdram_apply_static_cfg(); >>>>>> >>>>>> Marek and Simon, are you okay with this? If yes, I will submit patch for this. >>>>> >>>>> The problem was that this was causing weird sporadic hangs on Gen5, >>>>> which is why it was removed. So until there is an explanation for those >>>>> hangs, I'm not really OK with this. >>>> >>>> These sporadic hangs you saw were on devices without an FPGA image directly >>>> accessing the SDRAM ports, right? >>>> >>>>> >>>>> Maybe the application of static config should happen in SPL, before the >>>>> DRAM is running, or something like that ? >>>> >>>> Thinking this further, limiting it to applying in SPL is not a good idea since >>>> that prevents us from implementing different FPGA images/configs by loading a >>>> config later in the boot (i.e. in U-Boot from a FIT-image). >>>> >>>> Would it work to make setting this optional, i.e. only write the bit if an FPGA >>>> config actually uses these ports? Then it couldn't lead to problems on other >>>> hardware... >>> >>> If the sporadic hangs issue happen on FPGA image that doesn't connect >>> to f2sdram interface, we can add the checking to only apply static cfg >>> when it has f2sdram port enabled. >>> It can send a patch with this checking for you to try if want. >> Well, what could cause that sporadic hang ? >> Yes, the hang happens on image which doesn't access the SDRAM IIRC. >> > Do you remember if the hang happen when run bridge enable/disable > command? Or other flow? FPGA bitstream was loaded via the fpga command , and then during the bridge enable it got stuck. > I have tested the following sequence 500 times with FPGA image that > doesn't contains fs2dram bridge, but didn't see the hang. > - program fpga image > - bridge enable > - write to h2f bridge > - read from h2f bridge > - bridge disable > - repeat step 1 The test we did also contained booting Linux inbetween, and then reboot through U-Boot, which loaded the bitstream , enabled bridges , started Linux, reboot etc. -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2020-03-31 11:27 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-23 16:14 [U-Boot] [PATCH] ARM: socfpga: Remove socfpga_sdram_apply_static_cfg() Marek Vasut 2019-04-26 6:39 ` Simon Goldschmidt 2019-04-29 8:34 ` Marek Vasut 2019-04-29 13:02 ` See, Chin Liang 2019-04-29 21:29 ` Marek Vasut 2020-02-06 10:50 Nico Becker 2020-02-06 11:53 ` Marek Vasut 2020-02-06 12:57 ` Nico Becker 2020-02-06 13:00 ` Marek Vasut 2020-02-06 14:13 ` Nico Becker 2020-02-06 14:57 ` Simon Goldschmidt 2020-02-07 7:55 ` Marek Vasut 2020-02-07 12:09 ` Simon Goldschmidt 2020-02-07 12:27 ` Marek Vasut 2020-02-07 13:44 ` Simon Goldschmidt 2020-02-07 13:49 ` Marek Vasut 2020-02-12 18:52 ` Dalon L Westergreen 2020-03-09 8:50 ` Ley Foon Tan 2020-03-09 12:57 ` Marek Vasut 2020-03-09 14:10 ` Simon Goldschmidt 2020-03-09 14:15 ` Marek Vasut 2020-03-09 14:24 ` Simon Goldschmidt 2020-03-09 14:25 ` Marek Vasut 2020-03-12 15:54 ` Dalon L Westergreen 2020-03-12 15:57 ` Marek Vasut 2020-03-16 18:04 ` Dalon L Westergreen 2020-03-16 19:04 ` Simon Goldschmidt 2020-03-16 19:06 ` Marek Vasut 2020-03-16 19:09 ` Simon Goldschmidt 2020-03-16 19:19 ` Marek Vasut 2020-03-16 19:55 ` Dalon L Westergreen 2020-03-16 20:01 ` Simon Goldschmidt 2020-03-20 7:52 ` Ley Foon Tan 2020-03-31 8:13 ` Ley Foon Tan 2020-03-31 11:27 ` Marek Vasut 2020-03-11 9:54 ` Ley Foon Tan 2020-03-11 9:58 ` Marek Vasut 2020-03-12 9:31 ` Ley Foon Tan 2020-03-12 11:34 ` Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.