Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion

[Marek Vasut]

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

[Dinh Nguyen]

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

[Marek Vasut]

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

[Dinh Nguyen]

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

[Dinh Nguyen]

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

[Wolfgang Denk]

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

[Pavel Machek]

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

[Marek Vasut]

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 
> >>> 
> >>> This patch adds the DDR calibration portion of the Altera SDRAM driver.
> >>> 
> >>> Signed-off-by: Dinh Nguyen 
> >> 
> >> 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 
> 
> 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

[Stefan Roese]

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 

This patch adds the DDR calibration portion of the Altera SDRAM driver.

Signed-off-by: Dinh Nguyen 


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 

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

[Dinh Nguyen]

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 
>>
>> This patch adds the DDR calibration portion of the Altera SDRAM driver.
>>
>> Signed-off-by: Dinh Nguyen 
> 
> 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

Re: [U-Boot] [PATCHv3 2/3] driver/ddr/altera/: Add the sdram calibration portion

[Marek Vasut]

On Monday, May 18, 2015 at 09:36:48 PM, dingu...@opensource.altera.com wrote:
> From: Dinh Nguyen 
> 
> This patch adds the DDR calibration portion of the Altera SDRAM driver.
> 
> Signed-off-by: Dinh Nguyen 

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