Patchwork [U-Boot,v3,19/21] Use uintptr_t for 32/64-bit compatibility

login
register
mail settings
Submitter Simon Glass
Date Sept. 27, 2011, 12:10 a.m.
Message ID <1317082255-24247-20-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/116515/
State New, archived
Headers show

Comments

Simon Glass - Sept. 27, 2011, 12:10 a.m.
This fixes a few problems when building on 64-bit machines.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_mem.c     |    2 +-
 common/fdt_support.c |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)
Mike Frysinger - Oct. 3, 2011, 6:57 p.m.
On Monday, September 26, 2011 20:10:53 Simon Glass wrote:
> --- a/common/cmd_mem.c
> +++ b/common/cmd_mem.c
>
>  				printf ("\nMem error @ 0x%08X: "
>  					"found %08lX, expected %08lX\n",
> -					(uint)addr, readback, val);
> +					(uint)(uintptr_t)addr, readback, val);

could you change the printf format to %#p instead (and drop the casts 
completely for "addr") ?
-mike
Simon Glass - Oct. 4, 2011, 5:24 a.m.
Hi Mike,

On Mon, Oct 3, 2011 at 11:57 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday, September 26, 2011 20:10:53 Simon Glass wrote:
>> --- a/common/cmd_mem.c
>> +++ b/common/cmd_mem.c
>>
>>                               printf ("\nMem error @ 0x%08X: "
>>                                       "found %08lX, expected %08lX\n",
>> -                                     (uint)addr, readback, val);
>> +                                     (uint)(uintptr_t)addr, readback, val);
>
> could you change the printf format to %#p instead (and drop the casts
> completely for "addr") ?
> -mike
>

Well it's not complete clear where we want to go with this. My current
thinking is that the emulated memory will be small (in 2011 terms) -
maybe 128MB. The real memory of the machine is not accessible since it
doesn't make sense. So perhaps just because the host happens to be a
64-bit machine we don't suddenly want to be printing 64-bit addresses.

So the answer is 'yes' but for now I'm not sure that's the right thing
to do. I have another patch which enables 'md', etc. but of course
only within the 128MB 'emulated' memory area.

It actually brings up a question - does U-Boot run on 64-machines?

Regards,
Simon
Mike Frysinger - Oct. 10, 2011, 2:54 a.m.
On Tuesday 04 October 2011 01:24:56 Simon Glass wrote:
> On Mon, Oct 3, 2011 at 11:57 AM, Mike Frysinger wrote:
> > On Monday, September 26, 2011 20:10:53 Simon Glass wrote:
> >> --- a/common/cmd_mem.c
> >> +++ b/common/cmd_mem.c
> >> 
> >>                               printf ("\nMem error @ 0x%08X: "
> >>                                       "found %08lX, expected %08lX\n",
> >> -                                     (uint)addr, readback, val);
> >> +                                     (uint)(uintptr_t)addr, readback,
> >> val);
> > 
> > could you change the printf format to %#p instead (and drop the casts
> > completely for "addr") ?
> 
> Well it's not complete clear where we want to go with this. My current
> thinking is that the emulated memory will be small (in 2011 terms) -
> maybe 128MB. The real memory of the machine is not accessible since it
> doesn't make sense. So perhaps just because the host happens to be a
> 64-bit machine we don't suddenly want to be printing 64-bit addresses.
> 
> So the answer is 'yes' but for now I'm not sure that's the right thing
> to do. I have another patch which enables 'md', etc. but of course
> only within the 128MB 'emulated' memory area.
> 
> It actually brings up a question - does U-Boot run on 64-machines?

i don't believe u-boot runs on any 64bit system (or at least, in 64bit mode).  
there's quite a lot of random assumptions all over the tree where pointers are 
stored in 32bits, so for making sandbox work on 64bit stuff, i would focus on 
making stuff work with minimal intrusion for 32bit systems.

although thinking about this a bit more, %p will not zero pad its output (i 
think even if you specify like "08" it'll get ignored).  but maybe people 
don't care as normally this output shouldn't occur ?
-mike
Simon Glass - Oct. 10, 2011, 3:49 a.m.
Hi Mike,

On Sun, Oct 9, 2011 at 7:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 04 October 2011 01:24:56 Simon Glass wrote:
>> On Mon, Oct 3, 2011 at 11:57 AM, Mike Frysinger wrote:
>> > On Monday, September 26, 2011 20:10:53 Simon Glass wrote:
>> >> --- a/common/cmd_mem.c
>> >> +++ b/common/cmd_mem.c
>> >>
>> >>                               printf ("\nMem error @ 0x%08X: "
>> >>                                       "found %08lX, expected %08lX\n",
>> >> -                                     (uint)addr, readback, val);
>> >> +                                     (uint)(uintptr_t)addr, readback,
>> >> val);
>> >
>> > could you change the printf format to %#p instead (and drop the casts
>> > completely for "addr") ?
>>
>> Well it's not complete clear where we want to go with this. My current
>> thinking is that the emulated memory will be small (in 2011 terms) -
>> maybe 128MB. The real memory of the machine is not accessible since it
>> doesn't make sense. So perhaps just because the host happens to be a
>> 64-bit machine we don't suddenly want to be printing 64-bit addresses.
>>
>> So the answer is 'yes' but for now I'm not sure that's the right thing
>> to do. I have another patch which enables 'md', etc. but of course
>> only within the 128MB 'emulated' memory area.
>>
>> It actually brings up a question - does U-Boot run on 64-machines?
>
> i don't believe u-boot runs on any 64bit system (or at least, in 64bit mode).
> there's quite a lot of random assumptions all over the tree where pointers are
> stored in 32bits, so for making sandbox work on 64bit stuff, i would focus on
> making stuff work with minimal intrusion for 32bit systems.

Yes - I don't seem a lot of use in having massive amounts of RAM for
testing U-Boot, so 32-bit addresses should be fine for now.

>
> although thinking about this a bit more, %p will not zero pad its output (i
> think even if you specify like "08" it'll get ignored).  but maybe people
> don't care as normally this output shouldn't occur ?
> -mike
>

Yes %p is not quite what we want, but as you say this is an error condition.

Regards,
Simon
Aaron Williams - Oct. 11, 2011, 2:46 a.m.
Our OCTEON platform is a 64-bit SOC and we run it in the MIPS N32 ABI mode 
(64-bit registers, 32-bit address space). In our case we use virtual memory so 
we can move U-Boot to the top of memory which is often above 4GB. The only 
changes we had to make to U-Boot was that drivers need to use the proper 
mapping functions and support 64-bit physical addresses. Generally the 
hardware devices support this and the driver changes have been minimal. Some 
of our boards have over 8GB of RAM installed and 4GB is common for us. With 
the hole we create for our boot bus (flash, etc.) that means that in many 
cases U-Boot is loaded above the 32-bit boundary. By using a couple of TLB 
entries on MIPS we always map U-Boot to 0xC0000000 no matter where it is 
executing from. While a 64-bit U-Boot would be nice, we don't really need it 
since the TLB solves all of our issues. I would rather see drivers better make 
use of the proper mapping functions and support 64-bit physical addresses as 
well as use wrappers for accessing the PCI register space. (in our case all of 
our SOC registers require 64-bit addresses).

-Aaron

On Sunday, October 09, 2011 07:54:50 PM Mike Frysinger wrote:
> On Tuesday 04 October 2011 01:24:56 Simon Glass wrote:
> > On Mon, Oct 3, 2011 at 11:57 AM, Mike Frysinger wrote:
> > > On Monday, September 26, 2011 20:10:53 Simon Glass wrote:
> > >> --- a/common/cmd_mem.c
> > >> +++ b/common/cmd_mem.c
> > >> 
> > >>                               printf ("\nMem error @ 0x%08X: "
> > >>                               
> > >>                                       "found %08lX, expected %08lX\n",
> > >> 
> > >> -                                     (uint)addr, readback, val);
> > >> +                                     (uint)(uintptr_t)addr, readback,
> > >> val);
> > > 
> > > could you change the printf format to %#p instead (and drop the casts
> > > completely for "addr") ?
> > 
> > Well it's not complete clear where we want to go with this. My current
> > thinking is that the emulated memory will be small (in 2011 terms) -
> > maybe 128MB. The real memory of the machine is not accessible since it
> > doesn't make sense. So perhaps just because the host happens to be a
> > 64-bit machine we don't suddenly want to be printing 64-bit addresses.
> > 
> > So the answer is 'yes' but for now I'm not sure that's the right thing
> > to do. I have another patch which enables 'md', etc. but of course
> > only within the 128MB 'emulated' memory area.
> > 
> > It actually brings up a question - does U-Boot run on 64-machines?
> 
> i don't believe u-boot runs on any 64bit system (or at least, in 64bit
> mode). there's quite a lot of random assumptions all over the tree where
> pointers are stored in 32bits, so for making sandbox work on 64bit stuff,
> i would focus on making stuff work with minimal intrusion for 32bit
> systems.
> 
> although thinking about this a bit more, %p will not zero pad its output (i
> think even if you specify like "08" it'll get ignored).  but maybe people
> don't care as normally this output shouldn't occur ?
> -mike
Mike Frysinger - Oct. 11, 2011, 4:52 a.m.
On Monday 10 October 2011 22:46:10 Aaron Williams wrote:
> Our OCTEON platform is a 64-bit SOC and we run it in the MIPS N32 ABI mode
> (64-bit registers, 32-bit address space).

right, n32 == 32bit pointers, so i don't consider that a port with 64bit 
pointer issues

> rather see drivers better make use of the proper mapping functions and
> support 64-bit physical addresses as well as use wrappers for accessing
> the PCI register space. (in our case all of our SOC registers require
> 64-bit addresses).

what exactly are "proper mapping functions" ?  you mean things like the 
kernel's ioremap() ?

also, please don't top post
-mike
Aaron Williams - Oct. 11, 2011, 5:41 a.m.
On Monday, October 10, 2011 09:52:59 PM Mike Frysinger wrote:
> On Monday 10 October 2011 22:46:10 Aaron Williams wrote:
> > Our OCTEON platform is a 64-bit SOC and we run it in the MIPS N32 ABI
> > mode (64-bit registers, 32-bit address space).
> 
> right, n32 == 32bit pointers, so i don't consider that a port with 64bit
> pointer issues
> 
> > rather see drivers better make use of the proper mapping functions and
> > support 64-bit physical addresses as well as use wrappers for accessing
> > the PCI register space. (in our case all of our SOC registers require
> > 64-bit addresses).
> 
> what exactly are "proper mapping functions" ?  you mean things like the
> kernel's ioremap() ?
> 
> also, please don't top post
> -mike

I mean using functions like pci_virt_to_mem(). I have fixed a couple of 
drivers such as the USB EHCI driver and Intel E1000 driver to make use of this 
on our platform. We also can optionally perform I/O remapping so that PCI 
devices that only support 32-bit addressing can still access the memory where 
U-Boot is located. I have found that numerous drivers assume that an address 
in a pointer can be used directly for DMA which is not the case on MIPS since 
U-Boot typically runs in KSEG0 (0x8000000) which is aliased to physical memory 
address 0.

The other area I have seen issues is where drivers do not use 
readl/writel/etc. to access the registers. It is even easier if they use a 
macro per-driver since in our case we might use different functions to access 
the underlying hardware registers depending on if they're part of our SOC or 
on the PCIe bus since they map to different 64-bit I/O areas.

Now none of these actually require 64-bit pointers, just 64-bit physical 
addresses. We even support 64-bit ELF executables loaded from U-Boot as well 
as the 64-bit Linux kernel (we don't support a 32-bit kernel any longer) and 
basically make use of a 64-bit version of memcpy.

64-bit support would be useful for the various memory functions, i.e. md, mw, 
mm, md5sum, etc. It can also be useful for loading images to higher addresses. 
It's not essential though and currently we added separate commands for 
accessing 64-bit addresses.

One of these daya I need to work on getting some of our patches back upstream 
since some of them are quite useful for other platforms as well.

Even though we use virtual memory for U-Boot the number of changes to U-Boot 
has been very minimal except for drivers and even then the changes can be 
portable (i.e. using pci_virt_to_mem(). It actually simplifies things in that 
we don't have to deal with remapping. Some drivers like the proposed one for 
the Silicon Image SATA controller work as-is. The virtual memory allows the 
same U-Boot image to run from anywhere in memory. We use a single image 
whether we're booting out of NOR flash or over PCI. We also support multiple 
flash images (standard and failsafe). We perform the mapping before any C code 
is executed so we don't have to worry about relocation when U-Boot copies 
itself to the top of RAM and begins executing there. We do have a very 
different lib/board.c however, in order to deal with our SOC.

I think moving U-Boot to be fully 64-bit will be a lot more difficult compared 
to what we are doing since a lot of code assumes addresses are 32-bit. Our VM 
change allows U-Boot to remain 32-bit while still being able to support a 64-
bit platform.

-Aaron
Mike Frysinger - Oct. 14, 2011, 4:18 a.m.
On Tuesday 11 October 2011 01:41:07 Aaron Williams wrote:
> One of these daya I need to work on getting some of our patches back
> upstream since some of them are quite useful for other platforms as well.

yeah, for things like changing pci drivers to use pci_virt_to_mem() or 
changing pointer derefs to asm/io.h helpers, that sounds like things we all 
want.
-mike

Patch

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index e84cc4e..28476d7 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -937,7 +937,7 @@  int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			if (readback != val) {
 				printf ("\nMem error @ 0x%08X: "
 					"found %08lX, expected %08lX\n",
-					(uint)addr, readback, val);
+					(uint)(uintptr_t)addr, readback, val);
 				errs++;
 				if (ctrlc()) {
 					putc ('\n');
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 19b2ef6..9bed3ce 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -495,7 +495,7 @@  int fdt_resize(void *blob)
 	total = fdt_num_mem_rsv(blob);
 	for (i = 0; i < total; i++) {
 		fdt_get_mem_rsv(blob, i, &addr, &size);
-		if (addr == (uint64_t)(u32)blob) {
+		if (addr == (uintptr_t)blob) {
 			fdt_del_mem_rsv(blob, i);
 			break;
 		}
@@ -511,14 +511,14 @@  int fdt_resize(void *blob)
 		fdt_size_dt_strings(blob) + 5 * sizeof(struct fdt_reserve_entry);
 
 	/* Make it so the fdt ends on a page boundary */
-	actualsize = ALIGN(actualsize + ((uint)blob & 0xfff), 0x1000);
-	actualsize = actualsize - ((uint)blob & 0xfff);
+	actualsize = ALIGN(actualsize + ((uintptr_t)blob & 0xfff), 0x1000);
+	actualsize = actualsize - ((uintptr_t)blob & 0xfff);
 
 	/* Change the fdt header to reflect the correct size */
 	fdt_set_totalsize(blob, actualsize);
 
 	/* Add the new reservation */
-	ret = fdt_add_mem_rsv(blob, (uint)blob, actualsize);
+	ret = fdt_add_mem_rsv(blob, (uintptr_t)blob, actualsize);
 	if (ret < 0)
 		return ret;