Patchwork [U-Boot,1/3] Revert "i.MX28: Enable additional DRAM address bits"

login
register
mail settings
Submitter Marek Vasut
Date May 2, 2012, 10:14 p.m.
Message ID <1335996878-2264-1-git-send-email-marex@denx.de>
Download mbox | patch
Permalink /patch/156566/
State Accepted
Commit d2f7ae14d387cf1882a3a38d27c8f237fa191a26
Headers show

Comments

Marek Vasut - May 2, 2012, 10:14 p.m.
This reverts commit 69d26d09de1cb93e0a09ca71d9f0d41a66f0756a.

Apparently, this commit got mainline only because of OOT port and causes
breakage on board that is mainline. Revert.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
---
 arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Detlev Zundel - May 3, 2012, 8:11 a.m.
Hi Marek,

> This reverts commit 69d26d09de1cb93e0a09ca71d9f0d41a66f0756a.
>
> Apparently, this commit got mainline only because of OOT port and causes
> breakage on board that is mainline. Revert.

To be honest, I don't understand what this patch or what the original
patch did, nor through what OOT port this hit mainline and what breakage
it causes on other boards.

So also I can read the sentence, I cannot make heads and tails of it.
Can you please write commit messages that people like me can make some
sense from?  I.e. answering the following questions would help me

- What does the original commit do? (its too late to change the original
  commit) 
- Why was the change made in the first place and for what OOT port?
- What breakage is caused on what boards?
- Why can we revert the change without any problems?

Thanks
  Detlev
Marek Vasut - May 3, 2012, 1:36 p.m.
Dear Detlev Zundel,

> Hi Marek,
> 
> > This reverts commit 69d26d09de1cb93e0a09ca71d9f0d41a66f0756a.
> > 
> > Apparently, this commit got mainline only because of OOT port and causes
> > breakage on board that is mainline. Revert.
> 
> To be honest, I don't understand what this patch or what the original
> patch did, nor through what OOT port this hit mainline and what breakage
> it causes on other boards.

It enabled some additional address bits on X board, allowing it to use full 
512MB of RAM. But because we don't have any other such configured board and X 
board isn't mainline, I reverted this patch. It caused trouble on new 256MB 
configuration of M28.

Once there'll be some module that needs different memory configuration (like X 
board) mainline, we'll add this here, but until then, I'd like to stick with 
common memory init.

> 
> So also I can read the sentence, I cannot make heads and tails of it.
> Can you please write commit messages that people like me can make some
> sense from?  I.e. answering the following questions would help me
> 
> - What does the original commit do? (its too late to change the original
>   commit)

Enabled additional address bits, to address full 512 MB of DRAM on the X board.

> - Why was the change made in the first place and for what OOT port?

Change of a DRAM configuration register that enabled additional address bit, at 
address 512MB of DRAM. Though this caused memory hole on our M28 module with 
256MB of DRAM, which _is_ mainline. X board is OOT and never will be mainlined I 
guess.

> - What breakage is caused on what boards?

See above.

> - Why can we revert the change without any problems?

Because we don't have any mainline port that used this feature.

> 
> Thanks
>   Detlev

Best regards,
Marek Vasut
Detlev Zundel - May 3, 2012, 2:30 p.m.
Hi Marek,

[...]

>> - Why was the change made in the first place and for what OOT port?
>
> Change of a DRAM configuration register that enabled additional
> address bit, at address 512MB of DRAM. Though this caused memory hole
> on our M28 module with 256MB of DRAM, which _is_ mainline. X board is
> OOT and never will be mainlined I guess.

I still do not understand this fully.  What exactly is this "memory
hole" and why is it fatal?  As far as I can remember, there are always
some holes in the adress map, so why is this special?

Apart from that, I think most of these answers should go into the commit
message to understand what is happening.

Thanks in advance
  Detlev
Marek Vasut - May 3, 2012, 2:41 p.m.
Dear Detlev Zundel,

> Hi Marek,
> 
> [...]
> 
> >> - Why was the change made in the first place and for what OOT port?
> > 
> > Change of a DRAM configuration register that enabled additional
> > address bit, at address 512MB of DRAM. Though this caused memory hole
> > on our M28 module with 256MB of DRAM, which _is_ mainline. X board is
> > OOT and never will be mainlined I guess.
> 
> I still do not understand this fully.  What exactly is this "memory
> hole" and why is it fatal?  As far as I can remember, there are always
> some holes in the adress map, so why is this special?

No, this one created this layout on our 256 MB module:

[chunk of memory][<- same thing][chunk of memory][<- same thing]

so get_ram_size() didn't work with it and it actually overwrote part of the U-
Boot etc.

> 
> Apart from that, I think most of these answers should go into the commit
> message to understand what is happening.

Agreed

> Thanks in advance
>   Detlev

Best regards,
Marek Vasut
Detlev Zundel - May 4, 2012, 9:15 a.m.
Hi Marek,

> Dear Detlev Zundel,
>
>> Hi Marek,
>> 
>> [...]
>> 
>> >> - Why was the change made in the first place and for what OOT port?
>> > 
>> > Change of a DRAM configuration register that enabled additional
>> > address bit, at address 512MB of DRAM. Though this caused memory hole
>> > on our M28 module with 256MB of DRAM, which _is_ mainline. X board is
>> > OOT and never will be mainlined I guess.
>> 
>> I still do not understand this fully.  What exactly is this "memory
>> hole" and why is it fatal?  As far as I can remember, there are always
>> some holes in the adress map, so why is this special?
>
> No, this one created this layout on our 256 MB module:
>
> [chunk of memory][<- same thing][chunk of memory][<- same thing]
>
> so get_ram_size() didn't work with it and it actually overwrote part
> of the U-
> Boot etc.

Ah, so it created what I would call a "memory mirroring" or "memory
aliasing", right?  Now I understand the problem, thanks.

Cheers
  Detlev

Patch

diff --git a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
index 4f62142..0d13537 100644
--- a/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
+++ b/arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c
@@ -39,7 +39,7 @@  uint32_t dram_vals[] = {
 	0x00000000, 0x00000100, 0x00000000, 0x00000000,
 	0x00000000, 0x00000000, 0x00000000, 0x00000000,
 	0x00000000, 0x00000000, 0x00010101, 0x01010101,
-	0x000f0f01, 0x0f02010a, 0x00000000, 0x00010101,
+	0x000f0f01, 0x0f02020a, 0x00000000, 0x00010101,
 	0x00000100, 0x00000100, 0x00000000, 0x00000002,
 	0x01010000, 0x05060302, 0x06005003, 0x0a0000c8,
 	0x02009c40, 0x0000030c, 0x0036a609, 0x031a0612,