Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On Friday, May 29, 2015 at 05:24:19 PM, Dinh Nguyen wrote: [...] My concern is if this is actually true (and I asked this before, in an earlier round ov reviews). I cannot make heads or tails of this comment, as I don't understand what configuration time and reset are supposed to mean in U-Boot context. In my understanding, after a reset the memory content is uninitialized, i. e. random, and thus MUST always be properly initialized. Meh, since there's a pushback, I'll wait a bit with applying these until these remaining concerns settle :-/ That's fine. I'll send a v4 with further clean ups. OK This comment is related to the configuration where we have the NiOS cpu doing the ddr calibration and is not applicable for the Cyclone5/Arria5. So I think I can remove the comment for the the A5/C5 configuration. OK, then this should be removed. A10 support should then be added in a separate patch please. This situation will come into play for the Arria10 SoCFPGA, because that part will have a NiOS cpu that will do the DDR configuration. configuration time happens at power-up So configuration time happens if the FPGA loads itself from EPCQ or does it happen also if the FPGA is not loaded at all ? I think configuration time is only applicable when FPGA is programmed. So this is a timeframe between FPGA has been programmed (from EPCQ or whatever) and before HPS is released out of reset ? and reset is a warm reset. From what I was told, the situation where we might want to preserve a variable after a reset is to avoid reconfiguring the NiOS in the FPGA for DDR operations. Can't you synthesize a sticky register for this purpose in the FPGA instead? Or is it that this NIOS2 which configures the DRAM is actually a hardware, not a softcore ? On the Arria10, the NIOS2 is a hardened block and not a softcore. I think this is indeed a good idea. btw. I cooked yocto 1.8 (master) support for generating NIOS2 toolchain. I might as well work on it some more, but only once I'm back from LCJ. Also, what are external modules? I think these could be different FPGA instances that needs these variables that have survived a warm reset to the CPU. I _think_ I understand most of the above, but I kinda wonder why don't you cook a small IP block on the avalon bus in the FPGA which would contain a sticky register to hold all those configuration sticky bits. That'd look much more sensible to me. That does sound like a great idea. I think some of our customers are probably doing that already that. So uh, why aren't we doing it here ? Maybe because the sticky registers are not in the hardware the same way the NIOS2 core is ? ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On 05/28/2015 01:18 PM, Marek Vasut wrote: On Thursday, May 28, 2015 at 05:41:26 PM, Dinh Nguyen wrote: On 05/25/2015 08:23 AM, Wolfgang Denk wrote: Dear Pavel, In message 20150525123750.GD9943@amd you wrote: + ** All global variables that are explicitly initialized (including ** + ** explicitly initialized to zero), are only initialized once, during ** + ** configuration time, and not again on reset. This means that they** + ** preserve their current contents across resets, which is needed for some ** + ** special cases involving communication with external modules. In ** + ** addition, this avoids paying the price to have the memory initialized, ** + ** even for zeroed data, provided it is explicitly set to zero in the code, ** + ** and doesn't rely on implicit initialization. ** + ** + Is this sane thing to do? How does it work for variables in other sources? My concern is if this is actually true (and I asked this before, in an earlier round ov reviews). I cannot make heads or tails of this comment, as I don't understand what configuration time and reset are supposed to mean in U-Boot context. In my understanding, after a reset the memory content is uninitialized, i. e. random, and thus MUST always be properly initialized. Meh, since there's a pushback, I'll wait a bit with applying these until these remaining concerns settle :-/ That's fine. I'll send a v4 with further clean ups. This comment is related to the configuration where we have the NiOS cpu doing the ddr calibration and is not applicable for the Cyclone5/Arria5. So I think I can remove the comment for the the A5/C5 configuration. OK, then this should be removed. A10 support should then be added in a separate patch please. This situation will come into play for the Arria10 SoCFPGA, because that part will have a NiOS cpu that will do the DDR configuration. configuration time happens at power-up So configuration time happens if the FPGA loads itself from EPCQ or does it happen also if the FPGA is not loaded at all ? I think configuration time is only applicable when FPGA is programmed. and reset is a warm reset. From what I was told, the situation where we might want to preserve a variable after a reset is to avoid reconfiguring the NiOS in the FPGA for DDR operations. Can't you synthesize a sticky register for this purpose in the FPGA instead? Or is it that this NIOS2 which configures the DRAM is actually a hardware, not a softcore ? On the Arria10, the NIOS2 is a hardened block and not a softcore. Also, what are external modules? I think these could be different FPGA instances that needs these variables that have survived a warm reset to the CPU. I _think_ I understand most of the above, but I kinda wonder why don't you cook a small IP block on the avalon bus in the FPGA which would contain a sticky register to hold all those configuration sticky bits. That'd look much more sensible to me. That does sound like a great idea. I think some of our customers are probably doing that already that. Dinh ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On Thursday, May 28, 2015 at 05:41:26 PM, Dinh Nguyen wrote: On 05/25/2015 08:23 AM, Wolfgang Denk wrote: Dear Pavel, In message 20150525123750.GD9943@amd you wrote: + ** All global variables that are explicitly initialized (including ** + ** explicitly initialized to zero), are only initialized once, during ** + ** configuration time, and not again on reset. This means that they** + ** preserve their current contents across resets, which is needed for some ** + ** special cases involving communication with external modules. In ** + ** addition, this avoids paying the price to have the memory initialized, ** + ** even for zeroed data, provided it is explicitly set to zero in the code, ** + ** and doesn't rely on implicit initialization. ** + ** + Is this sane thing to do? How does it work for variables in other sources? My concern is if this is actually true (and I asked this before, in an earlier round ov reviews). I cannot make heads or tails of this comment, as I don't understand what configuration time and reset are supposed to mean in U-Boot context. In my understanding, after a reset the memory content is uninitialized, i. e. random, and thus MUST always be properly initialized. Meh, since there's a pushback, I'll wait a bit with applying these until these remaining concerns settle :-/ This comment is related to the configuration where we have the NiOS cpu doing the ddr calibration and is not applicable for the Cyclone5/Arria5. So I think I can remove the comment for the the A5/C5 configuration. OK, then this should be removed. A10 support should then be added in a separate patch please. This situation will come into play for the Arria10 SoCFPGA, because that part will have a NiOS cpu that will do the DDR configuration. configuration time happens at power-up So configuration time happens if the FPGA loads itself from EPCQ or does it happen also if the FPGA is not loaded at all ? and reset is a warm reset. From what I was told, the situation where we might want to preserve a variable after a reset is to avoid reconfiguring the NiOS in the FPGA for DDR operations. Can't you synthesize a sticky register for this purpose in the FPGA instead? Or is it that this NIOS2 which configures the DRAM is actually a hardware, not a softcore ? Also, what are external modules? I think these could be different FPGA instances that needs these variables that have survived a warm reset to the CPU. I _think_ I understand most of the above, but I kinda wonder why don't you cook a small IP block on the avalon bus in the FPGA which would contain a sticky register to hold all those configuration sticky bits. That'd look much more sensible to me. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On 05/25/2015 08:23 AM, Wolfgang Denk wrote: Dear Pavel, In message 20150525123750.GD9943@amd you wrote: + ** All global variables that are explicitly initialized (including ** + ** explicitly initialized to zero), are only initialized once, during ** + ** configuration time, and not again on reset. This means that they ** + ** preserve their current contents across resets, which is needed for some ** + ** special cases involving communication with external modules. In ** + ** addition, this avoids paying the price to have the memory initialized, ** + ** even for zeroed data, provided it is explicitly set to zero in the code, ** + ** and doesn't rely on implicit initialization. ** + ** + Is this sane thing to do? How does it work for variables in other sources? My concern is if this is actually true (and I asked this before, in an earlier round ov reviews). I cannot make heads or tails of this comment, as I don't understand what configuration time and reset are supposed to mean in U-Boot context. In my understanding, after a reset the memory content is uninitialized, i. e. random, and thus MUST always be properly initialized. This comment is related to the configuration where we have the NiOS cpu doing the ddr calibration and is not applicable for the Cyclone5/Arria5. So I think I can remove the comment for the the A5/C5 configuration. This situation will come into play for the Arria10 SoCFPGA, because that part will have a NiOS cpu that will do the DDR configuration. configuration time happens at power-up, and reset is a warm reset. From what I was told, the situation where we might want to preserve a variable after a reset is to avoid reconfiguring the NiOS in the FPGA for DDR operations. Also, what are external modules? I think these could be different FPGA instances that needs these variables that have survived a warm reset to the CPU. Dinh ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On 05/25/2015 08:23 AM, Wolfgang Denk wrote: Dear Pavel, In message 20150525123750.GD9943@amd you wrote: + ** All global variables that are explicitly initialized (including ** + ** explicitly initialized to zero), are only initialized once, during ** + ** configuration time, and not again on reset. This means that they ** + ** preserve their current contents across resets, which is needed for some ** + ** special cases involving communication with external modules. In ** + ** addition, this avoids paying the price to have the memory initialized, ** + ** even for zeroed data, provided it is explicitly set to zero in the code, ** + ** and doesn't rely on implicit initialization. ** + ** + Is this sane thing to do? How does it work for variables in other sources? My concern is if this is actually true (and I asked this before, in an earlier round ov reviews). I cannot make heads or tails of this comment, as I don't understand what configuration time and reset are supposed to mean in U-Boot context. In my understanding, after a reset the memory content is uninitialized, i. e. random, and thus MUST always be properly initialized. I've been able to get some more information on this comment. As it turns out, this comment is only applicable to configurations where the DDR calibration/configuration is being done by the FGPA(NiOS processor). So for Cyclone5/Arria5, this comment is not applicable and can be removed. configuration time in this comment refers to a power-up, and reset refers to a warm reset of the SoC, which may or may not involve reconfiguration of the FPGA(NiOS processor). This situation will come into play for the Arria10 platform, as it will have a NiOS processor that will do the DDR configuration/calibration. The reason why we may want to preserve variables from a warm reset is that we might not want to go through the FPGA configuration procedure on a warm reset. I hope this explains this comment a bit further. Thanks, Dinh ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
Hi! +/** + ** + ** NOTE: Special Rules for Globale Variables ** They are global, not globale. + ** All global variables that are explicitly initialized (including ** + ** explicitly initialized to zero), are only initialized once, during ** + ** configuration time, and not again on reset. This means that they ** + ** preserve their current contents across resets, which is needed for some ** + ** special cases involving communication with external modules. In ** + ** addition, this avoids paying the price to have the memory initialized, ** + ** even for zeroed data, provided it is explicitly set to zero in the code, ** + ** and doesn't rely on implicit initialization. ** + ** + Is this sane thing to do? How does it work for variables in other sources? What penalty is there for just redoing initialization from scratch each time? Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
Dear Pavel, In message 20150525123750.GD9943@amd you wrote: + ** All global variables that are explicitly initialized (including ** + ** explicitly initialized to zero), are only initialized once, during ** + ** configuration time, and not again on reset. This means that they ** + ** preserve their current contents across resets, which is needed for some ** + ** special cases involving communication with external modules. In ** + ** addition, this avoids paying the price to have the memory initialized, ** + ** even for zeroed data, provided it is explicitly set to zero in the code, ** + ** and doesn't rely on implicit initialization. ** + ** + Is this sane thing to do? How does it work for variables in other sources? My concern is if this is actually true (and I asked this before, in an earlier round ov reviews). I cannot make heads or tails of this comment, as I don't understand what configuration time and reset are supposed to mean in U-Boot context. In my understanding, after a reset the memory content is uninitialized, i. e. random, and thus MUST always be properly initialized. Also, what are external modules? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de It may be bad manners to talk with your mouth full, but it isn't too good either if you speak when your head is empty. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On 22.05.2015 04:43, Dinh Nguyen wrote: On 5/21/15 6:35 PM, Marek Vasut wrote: On Monday, May 18, 2015 at 09:36:48 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com This patch adds the DDR calibration portion of the Altera SDRAM driver. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Hi, DTTO here. The code is still ugly, but I'm afraid it's not gonna get any better if we keep it out of tree. It'd bet on getting this improved in-tree and under scrutiny from users. What do you think ? I agree. I really have tried my best to clean it up as best as I can. Yes, thanks. Really appreciated. I know that especially these kind of DDR calibration code tend to be quite complex and ugly. So I also vote for getting this applied now and perhaps improving it in-tree. For this patch series: Acked-by: Stefan Roese s...@denx.de Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On Friday, May 22, 2015 at 08:39:37 AM, Stefan Roese wrote: On 22.05.2015 04:43, Dinh Nguyen wrote: On 5/21/15 6:35 PM, Marek Vasut wrote: On Monday, May 18, 2015 at 09:36:48 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com This patch adds the DDR calibration portion of the Altera SDRAM driver. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Hi, DTTO here. The code is still ugly, but I'm afraid it's not gonna get any better if we keep it out of tree. It'd bet on getting this improved in-tree and under scrutiny from users. What do you think ? I agree. I really have tried my best to clean it up as best as I can. Yes, thanks. Really appreciated. I know that especially these kind of DDR calibration code tend to be quite complex and ugly. So I also vote for getting this applied now and perhaps improving it in-tree. I second that, I cannot say I fathom how huge the task of cleaning this up is. For this patch series: Acked-by: Stefan Roese s...@denx.de Thanks, Stefan Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On Monday, May 18, 2015 at 09:36:48 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com This patch adds the DDR calibration portion of the Altera SDRAM driver. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Hi, DTTO here. The code is still ugly, but I'm afraid it's not gonna get any better if we keep it out of tree. It'd bet on getting this improved in-tree and under scrutiny from users. What do you think ? Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion
On 5/21/15 6:35 PM, Marek Vasut wrote: On Monday, May 18, 2015 at 09:36:48 PM, dingu...@opensource.altera.com wrote: From: Dinh Nguyen dingu...@opensource.altera.com This patch adds the DDR calibration portion of the Altera SDRAM driver. Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com Hi, DTTO here. The code is still ugly, but I'm afraid it's not gonna get any better if we keep it out of tree. It'd bet on getting this improved in-tree and under scrutiny from users. What do you think ? I agree. I really have tried my best to clean it up as best as I can. Dinh ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
