diff mbox

[U-Boot,11/14] sunxi: Fill memory before comparing it when doing dram init on sun6i

Message ID 1418761900-14035-11-git-send-email-hdegoede@redhat.com
State Superseded
Delegated to: Ian Campbell
Headers show

Commit Message

Hans de Goede Dec. 16, 2014, 8:31 p.m. UTC
The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
it at different offsets to do columns, etc. detection. The sun6i boot0 code
does not do it, but it seems like a good idea to do this regardless.

The new mctl_mem_fill function this introduces is added as an inline helper
in dram.h, so that it can be shared with the sun8i dram code.

While at it move mctl_mem_matches to dram.h for re-use in sun8i too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/cpu/armv7/sunxi/dram_sun6i.c  | 15 +--------------
 arch/arm/include/asm/arch-sunxi/dram.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 14 deletions(-)

Comments

Ian Campbell Dec. 18, 2014, 7:12 p.m. UTC | #1
On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
> it at different offsets to do columns, etc. detection. The sun6i boot0 code
> does not do it, but it seems like a good idea to do this regardless.

Is this the right way round? The existing sun6i code (which you are
moving here) seems to _rely_ on something having written a useful
pattern, which I would have assumed to have been boot0. Or else how does
it work now? Chance?

> The new mctl_mem_fill function this introduces is added as an inline helper
> in dram.h, so that it can be shared with the sun8i dram code.
> 
> While at it move mctl_mem_matches to dram.h for re-use in sun8i too.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/arm/cpu/armv7/sunxi/dram_sun6i.c  | 15 +--------------
>  arch/arm/include/asm/arch-sunxi/dram.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> index e1670e5..4675c48 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> @@ -326,20 +326,6 @@ static void mctl_port_cfg(void)
>  	writel(0x00000307, &mctl_com->mbagcr[5]);
>  }
>  
> -static bool mctl_mem_matches(u32 offset)
> -{
> -	const int match_count = 64;
> -	int i, matches = 0;
> -
> -	for (i = 0; i < match_count; i++) {
> -		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
> -		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
> -			matches++;
> -	}
> -
> -	return matches == match_count;
> -}
> -
>  unsigned long sunxi_dram_init(void)
>  {
>  	struct sunxi_mctl_com_reg * const mctl_com =
> @@ -391,6 +377,7 @@ unsigned long sunxi_dram_init(void)
>  		    MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
>  
>  	/* Detect and set page size */
> +	mctl_mem_fill();
>  	for (columns = 7; columns < 20; columns++) {
>  		if (mctl_mem_matches(1 << columns))
>  			break;
> diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h
> index 18924f5..0bf718c 100644
> --- a/arch/arm/include/asm/arch-sunxi/dram.h
> +++ b/arch/arm/include/asm/arch-sunxi/dram.h
> @@ -22,6 +22,8 @@
>  #include <asm/arch/dram_sun4i.h>
>  #endif
>  
> +#define MCTL_MEM_FILL_MATCH_COUNT 64
> +
>  unsigned long sunxi_dram_init(void);
>  
>  /*
> @@ -37,4 +39,31 @@ static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val)
>  	}
>  }
>  
> +/*
> + * Fill beginning of DRAM with "random" data for mctl_mem_matches()
> + */
> +static inline void mctl_mem_fill(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++)
> +		writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4);
> +}
> +
> +/*
> + * Test if memory at offset offset matches memory at begin of DRAM
> + */
> +static inline bool mctl_mem_matches(u32 offset)
> +{
> +	int i, matches = 0;
> +
> +	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++) {
> +		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
> +		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
> +			matches++;
> +	}
> +
> +	return matches == MCTL_MEM_FILL_MATCH_COUNT;
> +}
> +
>  #endif /* _SUNXI_DRAM_H */
Siarhei Siamashka Dec. 19, 2014, 10:08 a.m. UTC | #2
On Thu, 18 Dec 2014 19:12:13 +0000
Ian Campbell <ijc@hellion.org.uk> wrote:

> On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
> > The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
> > it at different offsets to do columns, etc. detection. The sun6i boot0 code
> > does not do it, but it seems like a good idea to do this regardless.
> 
> Is this the right way round? The existing sun6i code (which you are
> moving here) seems to _rely_ on something having written a useful
> pattern, which I would have assumed to have been boot0. Or else how does
> it work now? Chance?

It appears that this code is just trying to find the first address line,
which is not connected anywhere. If the address line is not connected,
then having the corresponding bit set or clear in the memory address
does not matter and we are effectively accessing the same location.

Both sun6i and sun8i code is incorrect and only works because of relying
on luck.

The sun6i code is incorrect because it is just reading memory without
initializing it at all. Probably relying on having some unique garbage
there in a natural way.

The newer sun8i code is still incorrect because it is not safeguarding
against accidentally encountering the same test pattern at an unrelated
memory location. Yes, the probability is extremely low, but still not
zero. 

It's a good idea to have a look at the 'get_ram_size' function for
the inspiration.


> > The new mctl_mem_fill function this introduces is added as an inline helper
> > in dram.h, so that it can be shared with the sun8i dram code.
> > 
> > While at it move mctl_mem_matches to dram.h for re-use in sun8i too.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  arch/arm/cpu/armv7/sunxi/dram_sun6i.c  | 15 +--------------
> >  arch/arm/include/asm/arch-sunxi/dram.h | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> > index e1670e5..4675c48 100644
> > --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> > +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> > @@ -326,20 +326,6 @@ static void mctl_port_cfg(void)
> >  	writel(0x00000307, &mctl_com->mbagcr[5]);
> >  }
> >  
> > -static bool mctl_mem_matches(u32 offset)
> > -{
> > -	const int match_count = 64;
> > -	int i, matches = 0;
> > -
> > -	for (i = 0; i < match_count; i++) {
> > -		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
> > -		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
> > -			matches++;
> > -	}
> > -
> > -	return matches == match_count;
> > -}
> > -
> >  unsigned long sunxi_dram_init(void)
> >  {
> >  	struct sunxi_mctl_com_reg * const mctl_com =
> > @@ -391,6 +377,7 @@ unsigned long sunxi_dram_init(void)
> >  		    MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
> >  
> >  	/* Detect and set page size */
> > +	mctl_mem_fill();
> >  	for (columns = 7; columns < 20; columns++) {
> >  		if (mctl_mem_matches(1 << columns))
> >  			break;
> > diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h
> > index 18924f5..0bf718c 100644
> > --- a/arch/arm/include/asm/arch-sunxi/dram.h
> > +++ b/arch/arm/include/asm/arch-sunxi/dram.h
> > @@ -22,6 +22,8 @@
> >  #include <asm/arch/dram_sun4i.h>
> >  #endif
> >  
> > +#define MCTL_MEM_FILL_MATCH_COUNT 64
> > +
> >  unsigned long sunxi_dram_init(void);
> >  
> >  /*
> > @@ -37,4 +39,31 @@ static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val)
> >  	}
> >  }
> >  
> > +/*
> > + * Fill beginning of DRAM with "random" data for mctl_mem_matches()
> > + */
> > +static inline void mctl_mem_fill(void)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++)
> > +		writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4);
> > +}
> > +
> > +/*
> > + * Test if memory at offset offset matches memory at begin of DRAM
> > + */
> > +static inline bool mctl_mem_matches(u32 offset)
> > +{
> > +	int i, matches = 0;
> > +
> > +	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++) {
> > +		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
> > +		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
> > +			matches++;
> > +	}
> > +
> > +	return matches == MCTL_MEM_FILL_MATCH_COUNT;
> > +}
> > +
> >  #endif /* _SUNXI_DRAM_H */
Hans de Goede Dec. 19, 2014, 4:55 p.m. UTC | #3
Hi,

On 18-12-14 20:12, Ian Campbell wrote:
> On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
>> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
>> it at different offsets to do columns, etc. detection. The sun6i boot0 code
>> does not do it, but it seems like a good idea to do this regardless.
>
> Is this the right way round?

Yes, while working on the sun8i code I noticed that it was prefilling memory
before doing wrap-around checks like the sun6i code is doing too, and then I
realized that the sun6i code was relying on whatever is in DRAM being random
enough for its wrap-around tests to work, without ensuring it is random
enough.

> The existing sun6i code (which you are
> moving here) seems to _rely_ on something having written a useful
> pattern, which I would have assumed to have been boot0. Or else how does
> it work now? Chance?

Yep, it purely works by chance so far.

Regards,

Hans


>
>> The new mctl_mem_fill function this introduces is added as an inline helper
>> in dram.h, so that it can be shared with the sun8i dram code.
>>
>> While at it move mctl_mem_matches to dram.h for re-use in sun8i too.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   arch/arm/cpu/armv7/sunxi/dram_sun6i.c  | 15 +--------------
>>   arch/arm/include/asm/arch-sunxi/dram.h | 29 +++++++++++++++++++++++++++++
>>   2 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
>> index e1670e5..4675c48 100644
>> --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
>> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
>> @@ -326,20 +326,6 @@ static void mctl_port_cfg(void)
>>   	writel(0x00000307, &mctl_com->mbagcr[5]);
>>   }
>>
>> -static bool mctl_mem_matches(u32 offset)
>> -{
>> -	const int match_count = 64;
>> -	int i, matches = 0;
>> -
>> -	for (i = 0; i < match_count; i++) {
>> -		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
>> -		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
>> -			matches++;
>> -	}
>> -
>> -	return matches == match_count;
>> -}
>> -
>>   unsigned long sunxi_dram_init(void)
>>   {
>>   	struct sunxi_mctl_com_reg * const mctl_com =
>> @@ -391,6 +377,7 @@ unsigned long sunxi_dram_init(void)
>>   		    MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
>>
>>   	/* Detect and set page size */
>> +	mctl_mem_fill();
>>   	for (columns = 7; columns < 20; columns++) {
>>   		if (mctl_mem_matches(1 << columns))
>>   			break;
>> diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h
>> index 18924f5..0bf718c 100644
>> --- a/arch/arm/include/asm/arch-sunxi/dram.h
>> +++ b/arch/arm/include/asm/arch-sunxi/dram.h
>> @@ -22,6 +22,8 @@
>>   #include <asm/arch/dram_sun4i.h>
>>   #endif
>>
>> +#define MCTL_MEM_FILL_MATCH_COUNT 64
>> +
>>   unsigned long sunxi_dram_init(void);
>>
>>   /*
>> @@ -37,4 +39,31 @@ static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val)
>>   	}
>>   }
>>
>> +/*
>> + * Fill beginning of DRAM with "random" data for mctl_mem_matches()
>> + */
>> +static inline void mctl_mem_fill(void)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++)
>> +		writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4);
>> +}
>> +
>> +/*
>> + * Test if memory at offset offset matches memory at begin of DRAM
>> + */
>> +static inline bool mctl_mem_matches(u32 offset)
>> +{
>> +	int i, matches = 0;
>> +
>> +	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++) {
>> +		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
>> +		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
>> +			matches++;
>> +	}
>> +
>> +	return matches == MCTL_MEM_FILL_MATCH_COUNT;
>> +}
>> +
>>   #endif /* _SUNXI_DRAM_H */
>
>
Hans de Goede Dec. 19, 2014, 4:56 p.m. UTC | #4
Hi,

On 19-12-14 11:08, Siarhei Siamashka wrote:
> On Thu, 18 Dec 2014 19:12:13 +0000
> Ian Campbell <ijc@hellion.org.uk> wrote:
>
>> On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
>>> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
>>> it at different offsets to do columns, etc. detection. The sun6i boot0 code
>>> does not do it, but it seems like a good idea to do this regardless.
>>
>> Is this the right way round? The existing sun6i code (which you are
>> moving here) seems to _rely_ on something having written a useful
>> pattern, which I would have assumed to have been boot0. Or else how does
>> it work now? Chance?
>
> It appears that this code is just trying to find the first address line,
> which is not connected anywhere. If the address line is not connected,
> then having the corresponding bit set or clear in the memory address
> does not matter and we are effectively accessing the same location.

Correct.

> Both sun6i and sun8i code is incorrect and only works because of relying
> on luck.
>
> The sun6i code is incorrect because it is just reading memory without
> initializing it at all. Probably relying on having some unique garbage
> there in a natural way.
>
> The newer sun8i code is still incorrect because it is not safeguarding
> against accidentally encountering the same test pattern at an unrelated
> memory location. Yes, the probability is extremely low, but still not
> zero.

The probability is so low that it really does not matter, so I've no intention
to fix this.

Regards,

Hans
Siarhei Siamashka Dec. 22, 2014, 7:39 a.m. UTC | #5
On Fri, 19 Dec 2014 17:56:40 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 19-12-14 11:08, Siarhei Siamashka wrote:
> > On Thu, 18 Dec 2014 19:12:13 +0000
> > Ian Campbell <ijc@hellion.org.uk> wrote:
> >
> >> On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
> >>> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
> >>> it at different offsets to do columns, etc. detection. The sun6i boot0 code
> >>> does not do it, but it seems like a good idea to do this regardless.
> >>
> >> Is this the right way round? The existing sun6i code (which you are
> >> moving here) seems to _rely_ on something having written a useful
> >> pattern, which I would have assumed to have been boot0. Or else how does
> >> it work now? Chance?
> >
> > It appears that this code is just trying to find the first address line,
> > which is not connected anywhere. If the address line is not connected,
> > then having the corresponding bit set or clear in the memory address
> > does not matter and we are effectively accessing the same location.
> 
> Correct.
> 
> > Both sun6i and sun8i code is incorrect and only works because of relying
> > on luck.
> >
> > The sun6i code is incorrect because it is just reading memory without
> > initializing it at all. Probably relying on having some unique garbage
> > there in a natural way.
> >
> > The newer sun8i code is still incorrect because it is not safeguarding
> > against accidentally encountering the same test pattern at an unrelated
> > memory location. Yes, the probability is extremely low, but still not
> > zero.
> 
> The probability is so low that it really does not matter, so I've no intention
> to fix this.

"If anything bad can happen, it probably will" :-)

There are many real life examples, when something supposedly almost
impossible has in fact happened. Like the following story:
    http://ixbtlabs.com/articles2/magia-chisel/
In a nutshell, the designers of some old CD/CD-RW drives just did not
bother to ensure that the written user data can be never confused
with the service information (sync headers) on disk. The probability
of encountering something that resembles a sync header was considered
to be way too low. Well, until somebody has encountered a hardware
glitch and a sync header fragment got mistakenly read from the drive
as part of some file. Now this corrupted file can wreak havoc if
written to a CD disk, because the false sync header in it is getting
interpreted as an actual sync header. Oops.

Anyway, this is a bug in the sun6i/sun8i dram code. And the submitted
patch appears to be only a partial/incomplete fix. A strong NAK from
me.
Ian Campbell Dec. 22, 2014, 2:19 p.m. UTC | #6
On Fri, 2014-12-19 at 17:55 +0100, Hans de Goede wrote:
> Hi,
> 
> On 18-12-14 20:12, Ian Campbell wrote:
> > On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
> >> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
> >> it at different offsets to do columns, etc. detection. The sun6i boot0 code
> >> does not do it, but it seems like a good idea to do this regardless.
> >
> > Is this the right way round?
> 
> Yes, while working on the sun8i code I noticed that it was prefilling memory
> before doing wrap-around checks like the sun6i code is doing too, and then I
> realized that the sun6i code was relying on whatever is in DRAM being random
> enough for its wrap-around tests to work, without ensuring it is random
> enough.
> 
> > The existing sun6i code (which you are
> > moving here) seems to _rely_ on something having written a useful
> > pattern, which I would have assumed to have been boot0. Or else how does
> > it work now? Chance?
> 
> Yep, it purely works by chance so far.

"excellent". Can you mention that in the commit message please. With
that: Acked-by: Ian Campbell <ijc@hellion.org.uk>

Ian.
Siarhei Siamashka Dec. 22, 2014, 2:32 p.m. UTC | #7
On Mon, 22 Dec 2014 14:19:20 +0000
Ian Campbell <ijc@hellion.org.uk> wrote:

> On Fri, 2014-12-19 at 17:55 +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 18-12-14 20:12, Ian Campbell wrote:
> > > On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
> > >> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
> > >> it at different offsets to do columns, etc. detection. The sun6i boot0 code
> > >> does not do it, but it seems like a good idea to do this regardless.
> > >
> > > Is this the right way round?
> > 
> > Yes, while working on the sun8i code I noticed that it was prefilling memory
> > before doing wrap-around checks like the sun6i code is doing too, and then I
> > realized that the sun6i code was relying on whatever is in DRAM being random
> > enough for its wrap-around tests to work, without ensuring it is random
> > enough.
> > 
> > > The existing sun6i code (which you are
> > > moving here) seems to _rely_ on something having written a useful
> > > pattern, which I would have assumed to have been boot0. Or else how does
> > > it work now? Chance?
> > 
> > Yep, it purely works by chance so far.
> 
> "excellent". Can you mention that in the commit message please. With
> that: Acked-by: Ian Campbell <ijc@hellion.org.uk>

Ian, are you fine with the fact that it still works by chance
even after this "fix"? Albeit with a much higher chance of not
running into troubles.
Ian Campbell Dec. 22, 2014, 2:34 p.m. UTC | #8
On Mon, 2014-12-22 at 16:32 +0200, Siarhei Siamashka wrote:
> On Mon, 22 Dec 2014 14:19:20 +0000
> Ian Campbell <ijc@hellion.org.uk> wrote:
> 
> > On Fri, 2014-12-19 at 17:55 +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 18-12-14 20:12, Ian Campbell wrote:
> > > > On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
> > > >> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
> > > >> it at different offsets to do columns, etc. detection. The sun6i boot0 code
> > > >> does not do it, but it seems like a good idea to do this regardless.
> > > >
> > > > Is this the right way round?
> > > 
> > > Yes, while working on the sun8i code I noticed that it was prefilling memory
> > > before doing wrap-around checks like the sun6i code is doing too, and then I
> > > realized that the sun6i code was relying on whatever is in DRAM being random
> > > enough for its wrap-around tests to work, without ensuring it is random
> > > enough.
> > > 
> > > > The existing sun6i code (which you are
> > > > moving here) seems to _rely_ on something having written a useful
> > > > pattern, which I would have assumed to have been boot0. Or else how does
> > > > it work now? Chance?
> > > 
> > > Yep, it purely works by chance so far.
> > 
> > "excellent". Can you mention that in the commit message please. With
> > that: Acked-by: Ian Campbell <ijc@hellion.org.uk>
> 
> Ian, are you fine with the fact that it still works by chance
> even after this "fix"? Albeit with a much higher chance of not
> running into troubles.

Yes, it's clearly an improvement even if it isn't perfect.

Ian.
Hans de Goede Dec. 22, 2014, 3:47 p.m. UTC | #9
Hi,

On 22-12-14 15:19, Ian Campbell wrote:
> On Fri, 2014-12-19 at 17:55 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 18-12-14 20:12, Ian Campbell wrote:
>>> On Tue, 2014-12-16 at 21:31 +0100, Hans de Goede wrote:
>>>> The sun8i boot0 code fills the DRAM with a "random" pattern before comparing
>>>> it at different offsets to do columns, etc. detection. The sun6i boot0 code
>>>> does not do it, but it seems like a good idea to do this regardless.
>>>
>>> Is this the right way round?
>>
>> Yes, while working on the sun8i code I noticed that it was prefilling memory
>> before doing wrap-around checks like the sun6i code is doing too, and then I
>> realized that the sun6i code was relying on whatever is in DRAM being random
>> enough for its wrap-around tests to work, without ensuring it is random
>> enough.
>>
>>> The existing sun6i code (which you are
>>> moving here) seems to _rely_ on something having written a useful
>>> pattern, which I would have assumed to have been boot0. Or else how does
>>> it work now? Chance?
>>
>> Yep, it purely works by chance so far.
>
> "excellent". Can you mention that in the commit message please.

Done.

> With that: Acked-by: Ian Campbell <ijc@hellion.org.uk>

Thanks, the now fully acked sun8i support patch-set has been pushed to
u-boot-sunxi/next .

Thanks for all the reviews!

Regards,

Hans
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
index e1670e5..4675c48 100644
--- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
+++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
@@ -326,20 +326,6 @@  static void mctl_port_cfg(void)
 	writel(0x00000307, &mctl_com->mbagcr[5]);
 }
 
-static bool mctl_mem_matches(u32 offset)
-{
-	const int match_count = 64;
-	int i, matches = 0;
-
-	for (i = 0; i < match_count; i++) {
-		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
-		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
-			matches++;
-	}
-
-	return matches == match_count;
-}
-
 unsigned long sunxi_dram_init(void)
 {
 	struct sunxi_mctl_com_reg * const mctl_com =
@@ -391,6 +377,7 @@  unsigned long sunxi_dram_init(void)
 		    MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
 
 	/* Detect and set page size */
+	mctl_mem_fill();
 	for (columns = 7; columns < 20; columns++) {
 		if (mctl_mem_matches(1 << columns))
 			break;
diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h
index 18924f5..0bf718c 100644
--- a/arch/arm/include/asm/arch-sunxi/dram.h
+++ b/arch/arm/include/asm/arch-sunxi/dram.h
@@ -22,6 +22,8 @@ 
 #include <asm/arch/dram_sun4i.h>
 #endif
 
+#define MCTL_MEM_FILL_MATCH_COUNT 64
+
 unsigned long sunxi_dram_init(void);
 
 /*
@@ -37,4 +39,31 @@  static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val)
 	}
 }
 
+/*
+ * Fill beginning of DRAM with "random" data for mctl_mem_matches()
+ */
+static inline void mctl_mem_fill(void)
+{
+	int i;
+
+	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++)
+		writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4);
+}
+
+/*
+ * Test if memory at offset offset matches memory at begin of DRAM
+ */
+static inline bool mctl_mem_matches(u32 offset)
+{
+	int i, matches = 0;
+
+	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++) {
+		if (readl(CONFIG_SYS_SDRAM_BASE + i * 4) ==
+		    readl(CONFIG_SYS_SDRAM_BASE + offset + i * 4))
+			matches++;
+	}
+
+	return matches == MCTL_MEM_FILL_MATCH_COUNT;
+}
+
 #endif /* _SUNXI_DRAM_H */