Patchwork [U-Boot,v2,2/3] avr32: Use uncached() macro to get an address for SDRAM init

login
register
mail settings
Submitter Haavard Skinnemoen
Date Aug. 12, 2010, 6:52 a.m.
Message ID <1281595974-32279-3-git-send-email-haavard.skinnemoen@atmel.com>
Download mbox | patch
Permalink /patch/71811/
State Accepted
Commit 9cec2fc209a000655af77256a39ede7c7d441e56
Delegated to: Reinhard Meyer
Headers show

Comments

Haavard Skinnemoen - Aug. 12, 2010, 6:52 a.m.
The paging system which is required to set up caching properties has not
yet been initialized when the SDRAM is initialized. So when the
map_physmem() function is converted to return the physical address
unchanged, the SDRAM initialization will break on some boards.

The avr32-specific uncached() macro will return an address which will
always cause uncached accessed to be made. Since this happens in the
board code, using avr32-specific features should be ok, and will allow
the SDRAM initialization to keep working.

Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 board/atmel/atngw100/atngw100.c              |    4 +---
 board/atmel/atstk1000/atstk1000.c            |    4 +---
 board/earthlcd/favr-32-ezkit/favr-32-ezkit.c |    4 +---
 board/mimc/mimc200/mimc200.c                 |    4 +---
 board/miromico/hammerhead/hammerhead.c       |    4 +---
 5 files changed, 5 insertions(+), 15 deletions(-)
Detlev Zundel - Aug. 12, 2010, 3:04 p.m.
Hi Haavard,

> The paging system which is required to set up caching properties has not
> yet been initialized when the SDRAM is initialized. So when the
> map_physmem() function is converted to return the physical address
> unchanged, the SDRAM initialization will break on some boards.
>
> The avr32-specific uncached() macro will return an address which will
> always cause uncached accessed to be made. Since this happens in the
> board code, using avr32-specific features should be ok, and will allow
> the SDRAM initialization to keep working.
>
> Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> ---
>  board/atmel/atngw100/atngw100.c              |    4 +---
>  board/atmel/atstk1000/atstk1000.c            |    4 +---
>  board/earthlcd/favr-32-ezkit/favr-32-ezkit.c |    4 +---
>  board/mimc/mimc200/mimc200.c                 |    4 +---
>  board/miromico/hammerhead/hammerhead.c       |    4 +---
>  5 files changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/board/atmel/atngw100/atngw100.c b/board/atmel/atngw100/atngw100.c
> index 004d8da..4580f55 100644
> --- a/board/atmel/atngw100/atngw100.c
> +++ b/board/atmel/atngw100/atngw100.c
> @@ -75,13 +75,11 @@ phys_size_t initdram(int board_type)
>  	unsigned long actual_size;
>  	void *sdram_base;
>  
> -	sdram_base = map_physmem(EBI_SDRAM_BASE, EBI_SDRAM_SIZE, MAP_NOCACHE);
> +	sdram_base = uncached(EBI_SDRAM_BASE);
>  
>  	expected_size = sdram_init(sdram_base, &sdram_config);
>  	actual_size = get_ram_size(sdram_base, expected_size);
>  
> -	unmap_physmem(sdram_base, EBI_SDRAM_SIZE);
> -

So this patch replaces a construct which seems to be valid over all
architectures by a construct which is only used in avr32, right?  It
also deletes the explicit statement that such a mapping is not needed
any further.

Isn't this a step backward?  Can't you put the functionality inside the
map function and leave the unmap a noop?

Cheers
  Detlev
Haavard Skinnemoen - Aug. 13, 2010, 3:56 a.m.
Detlev Zundel <dzu@denx.de> wrote:
> So this patch replaces a construct which seems to be valid over all
> architectures by a construct which is only used in avr32, right?  It
> also deletes the explicit statement that such a mapping is not needed
> any further.

Problem is that in order to make the CFI driver work on avr32, we need
to change the map_physmem() macro to return the physical address
unchanged.

> Isn't this a step backward?  Can't you put the functionality inside the
> map function and leave the unmap a noop?

I agree it's a step backward, but since the previous flamewar didn't
get us anywhere, I decided to go for a compromise this time. At least
this small architecture-specific kludge is localized to
architecture-specific code.

The map_physmem() macro currently does exactly the same thing as the
uncached() macro, and the unmap is a noop, but the next patch changes
it in order to fix the CFI driver. If the next patch is applied without
this patch being applied first, the SDRAM driver will do cached
accesses during initialization, and that may cause the initialization
to fail.

Haavard
Detlev Zundel - Aug. 13, 2010, 11:01 a.m.
Hi Haavard,

> Detlev Zundel <dzu@denx.de> wrote:
>> So this patch replaces a construct which seems to be valid over all
>> architectures by a construct which is only used in avr32, right?  It
>> also deletes the explicit statement that such a mapping is not needed
>> any further.
>
> Problem is that in order to make the CFI driver work on avr32, we need
> to change the map_physmem() macro to return the physical address
> unchanged.

I see.  And I presume you cannot tell the situation apart inside
map_physmem?

>> Isn't this a step backward?  Can't you put the functionality inside the
>> map function and leave the unmap a noop?
>
> I agree it's a step backward, but since the previous flamewar didn't
> get us anywhere, I decided to go for a compromise this time. At least
> this small architecture-specific kludge is localized to
> architecture-specific code.
>
> The map_physmem() macro currently does exactly the same thing as the
> uncached() macro, and the unmap is a noop, but the next patch changes
> it in order to fix the CFI driver. If the next patch is applied without
> this patch being applied first, the SDRAM driver will do cached
> accesses during initialization, and that may cause the initialization
> to fail.

Why not include a note to this extent into the git commit message?  This
would be a great help for other people to later understand why this
change has been done the "backward way" that it was.

Cheers
  Detlev
Haavard Skinnemoen - Aug. 13, 2010, 12:12 p.m.
Detlev Zundel <dzu@denx.de> wrote:
> > Problem is that in order to make the CFI driver work on avr32, we need
> > to change the map_physmem() macro to return the physical address
> > unchanged.
> 
> I see.  And I presume you cannot tell the situation apart inside
> map_physmem?

I don't think so. How do you propose we do that?

> > The map_physmem() macro currently does exactly the same thing as the
> > uncached() macro, and the unmap is a noop, but the next patch changes
> > it in order to fix the CFI driver. If the next patch is applied without
> > this patch being applied first, the SDRAM driver will do cached
> > accesses during initialization, and that may cause the initialization
> > to fail.
> 
> Why not include a note to this extent into the git commit message?  This
> would be a great help for other people to later understand why this
> change has been done the "backward way" that it was.

The commit message already contains this:

The paging system which is required to set up caching properties has not
yet been initialized when the SDRAM is initialized. So when the
map_physmem() function is converted to return the physical address
unchanged, the SDRAM initialization will break on some boards.

which is essentially the same thing, isn't it?

Haavard
Detlev Zundel - Aug. 13, 2010, 1:01 p.m.
Hi Haavard,

> Detlev Zundel <dzu@denx.de> wrote:
>> > Problem is that in order to make the CFI driver work on avr32, we need
>> > to change the map_physmem() macro to return the physical address
>> > unchanged.
>> 
>> I see.  And I presume you cannot tell the situation apart inside
>> map_physmem?
>
> I don't think so. How do you propose we do that?

I don't know, that's why I'm asking.  

Let's take a step back and please excuse my ignorant question - why
exactly does the CFI driver need the physical address unchanged?  Isn't
the real constraint that the address requested by CFI is uncached?  Why
can't this be done by map_physmem()?

>> > The map_physmem() macro currently does exactly the same thing as the
>> > uncached() macro, and the unmap is a noop, but the next patch changes
>> > it in order to fix the CFI driver. If the next patch is applied without
>> > this patch being applied first, the SDRAM driver will do cached
>> > accesses during initialization, and that may cause the initialization
>> > to fail.
>> 
>> Why not include a note to this extent into the git commit message?  This
>> would be a great help for other people to later understand why this
>> change has been done the "backward way" that it was.
>
> The commit message already contains this:
>
> The paging system which is required to set up caching properties has not
> yet been initialized when the SDRAM is initialized. So when the
> map_physmem() function is converted to return the physical address
> unchanged, the SDRAM initialization will break on some boards.
>
> which is essentially the same thing, isn't it?

For me this is not the same - it does not include a word why the change
which you agree "looks backward" is something that we want to do.  For
me something like this would give me more information:

  Unfortunately we cannot make "map_physmem()/unmap_physmem()" on the
  AVR32 platform work with the CFI driver as it works on other
  platforms. [I don't understand why this is the case, so your
  explanation would go here ;) ] To still fix the issue we deliberately
  replace these generic routines by AVR32 specific routines.  If someone
  can fix this using the generic patterns, patches are welcome.

I believe that good docmumentation should include such pro- and con
reasoning so that code changes can be comprehended even after the fact.

Cheers
  Detlev
Andreas Bießmann - Sept. 3, 2010, 11:16 a.m.
Dear Haavard Skinnemoen,

Am 12.08.2010 08:52, schrieb Haavard Skinnemoen:
> The paging system which is required to set up caching properties has not
> yet been initialized when the SDRAM is initialized. So when the
> map_physmem() function is converted to return the physical address
> unchanged, the SDRAM initialization will break on some boards.
> 
> The avr32-specific uncached() macro will return an address which will
> always cause uncached accessed to be made. Since this happens in the
> board code, using avr32-specific features should be ok, and will allow
> the SDRAM initialization to keep working.
> 
> Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>

Tested-by: Andreas Bießmann <biessmann@corscience.de>

one colleague reported similar problems initialising SDRAM on our own
board. I could not reproduce this issue here, however this patch fixed
the problem. Thanks for that, it came the right time.

This patch should be applied too.

regards

Andreas Bießmann
Reinhard Meyer - Sept. 3, 2010, 2:37 p.m.
Haavard Skinnemoen schrieb:
> The paging system which is required to set up caching properties has not
> yet been initialized when the SDRAM is initialized. So when the
> map_physmem() function is converted to return the physical address
> unchanged, the SDRAM initialization will break on some boards.
> 
> The avr32-specific uncached() macro will return an address which will
> always cause uncached accessed to be made. Since this happens in the
> board code, using avr32-specific features should be ok, and will allow
> the SDRAM initialization to keep working.
> 
> Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> ---
>  board/atmel/atngw100/atngw100.c              |    4 +---
>  board/atmel/atstk1000/atstk1000.c            |    4 +---
>  board/earthlcd/favr-32-ezkit/favr-32-ezkit.c |    4 +---
>  board/mimc/mimc200/mimc200.c                 |    4 +---
>  board/miromico/hammerhead/hammerhead.c       |    4 +---
>  5 files changed, 5 insertions(+), 15 deletions(-)
Applied to u-boot-atmel/avr32
Thanks,
Reinhard

Patch

diff --git a/board/atmel/atngw100/atngw100.c b/board/atmel/atngw100/atngw100.c
index 004d8da..4580f55 100644
--- a/board/atmel/atngw100/atngw100.c
+++ b/board/atmel/atngw100/atngw100.c
@@ -75,13 +75,11 @@  phys_size_t initdram(int board_type)
 	unsigned long actual_size;
 	void *sdram_base;
 
-	sdram_base = map_physmem(EBI_SDRAM_BASE, EBI_SDRAM_SIZE, MAP_NOCACHE);
+	sdram_base = uncached(EBI_SDRAM_BASE);
 
 	expected_size = sdram_init(sdram_base, &sdram_config);
 	actual_size = get_ram_size(sdram_base, expected_size);
 
-	unmap_physmem(sdram_base, EBI_SDRAM_SIZE);
-
 	if (expected_size != actual_size)
 		printf("Warning: Only %lu of %lu MiB SDRAM is working\n",
 				actual_size >> 20, expected_size >> 20);
diff --git a/board/atmel/atstk1000/atstk1000.c b/board/atmel/atstk1000/atstk1000.c
index c36cb57..d91d594 100644
--- a/board/atmel/atstk1000/atstk1000.c
+++ b/board/atmel/atstk1000/atstk1000.c
@@ -97,13 +97,11 @@  phys_size_t initdram(int board_type)
 	unsigned long actual_size;
 	void *sdram_base;
 
-	sdram_base = map_physmem(EBI_SDRAM_BASE, EBI_SDRAM_SIZE, MAP_NOCACHE);
+	sdram_base = uncached(EBI_SDRAM_BASE);
 
 	expected_size = sdram_init(sdram_base, &sdram_config);
 	actual_size = get_ram_size(sdram_base, expected_size);
 
-	unmap_physmem(sdram_base, EBI_SDRAM_SIZE);
-
 	if (expected_size != actual_size)
 		printf("Warning: Only %lu of %lu MiB SDRAM is working\n",
 				actual_size >> 20, expected_size >> 20);
diff --git a/board/earthlcd/favr-32-ezkit/favr-32-ezkit.c b/board/earthlcd/favr-32-ezkit/favr-32-ezkit.c
index 8af680f..d2843c9 100644
--- a/board/earthlcd/favr-32-ezkit/favr-32-ezkit.c
+++ b/board/earthlcd/favr-32-ezkit/favr-32-ezkit.c
@@ -68,13 +68,11 @@  phys_size_t initdram(int board_type)
 	unsigned long actual_size;
 	void *sdram_base;
 
-	sdram_base = map_physmem(EBI_SDRAM_BASE, EBI_SDRAM_SIZE, MAP_NOCACHE);
+	sdram_base = uncached(EBI_SDRAM_BASE);
 
 	expected_size = sdram_init(sdram_base, &sdram_config);
 	actual_size = get_ram_size(sdram_base, expected_size);
 
-	unmap_physmem(sdram_base, EBI_SDRAM_SIZE);
-
 	if (expected_size != actual_size)
 		printf("Warning: Only %lu of %lu MiB SDRAM is working\n",
 				actual_size >> 20, expected_size >> 20);
diff --git a/board/mimc/mimc200/mimc200.c b/board/mimc/mimc200/mimc200.c
index cc0f137..9940669 100644
--- a/board/mimc/mimc200/mimc200.c
+++ b/board/mimc/mimc200/mimc200.c
@@ -153,13 +153,11 @@  phys_size_t initdram(int board_type)
 	unsigned long actual_size;
 	void *sdram_base;
 
-	sdram_base = map_physmem(EBI_SDRAM_BASE, EBI_SDRAM_SIZE, MAP_NOCACHE);
+	sdram_base = uncached(EBI_SDRAM_BASE);
 
 	expected_size = sdram_init(sdram_base, &sdram_config);
 	actual_size = get_ram_size(sdram_base, expected_size);
 
-	unmap_physmem(sdram_base, EBI_SDRAM_SIZE);
-
 	if (expected_size != actual_size)
 		printf("Warning: Only %lu of %lu MiB SDRAM is working\n",
 				actual_size >> 20, expected_size >> 20);
diff --git a/board/miromico/hammerhead/hammerhead.c b/board/miromico/hammerhead/hammerhead.c
index 8b3e22c..5ab999e 100644
--- a/board/miromico/hammerhead/hammerhead.c
+++ b/board/miromico/hammerhead/hammerhead.c
@@ -80,13 +80,11 @@  phys_size_t initdram(int board_type)
 	unsigned long actual_size;
 	void *sdram_base;
 
-	sdram_base = map_physmem(EBI_SDRAM_BASE, EBI_SDRAM_SIZE, MAP_NOCACHE);
+	sdram_base = uncached(EBI_SDRAM_BASE);
 
 	expected_size = sdram_init(sdram_base, &sdram_config);
 	actual_size = get_ram_size(sdram_base, expected_size);
 
-	unmap_physmem(sdram_base, EBI_SDRAM_SIZE);
-
 	if (expected_size != actual_size)
 		printf("Warning: Only %lu of %lu MiB SDRAM is working\n",
 		       actual_size >> 20, expected_size >> 20);