| Message ID | 1438253358-25223-2-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 30 July 2015 at 04:49, Bin Meng <bmeng.cn@gmail.com> wrote: > bd->bi_dram[] has both start address and size defined as 32-bit, > which is not the case on some platforms where >=4GiB memory bank > is used. Change them to support such memory banks. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > common/board_f.c | 2 +- > include/asm-generic/u-boot.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/common/board_f.c b/common/board_f.c > index 21be26f..a8ed6c8 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -206,7 +206,7 @@ static int show_dram_config(void) > debug("\nRAM Configuration:\n"); > for (i = size = 0; i < CONFIG_NR_DRAM_BANKS; i++) { > size += gd->bd->bi_dram[i].size; > - debug("Bank #%d: %08lx ", i, gd->bd->bi_dram[i].start); > + debug("Bank #%d: %016llx ", i, gd->bd->bi_dram[i].start); This makes all boards use an unreadable 16-character hex string. Can I suggest we only use the larger value when the size demands it? A bit messy but few things have >4GB memory so far. > #ifdef DEBUG > print_size(gd->bd->bi_dram[i].size, "\n"); > #endif > diff --git a/include/asm-generic/u-boot.h b/include/asm-generic/u-boot.h > index c918049..4e8b481 100644 > --- a/include/asm-generic/u-boot.h > +++ b/include/asm-generic/u-boot.h > @@ -130,8 +130,8 @@ typedef struct bd_info { > ulong bi_boot_params; /* where this board expects params */ > #ifdef CONFIG_NR_DRAM_BANKS > struct { /* RAM configuration */ > - ulong start; > - ulong size; > + unsigned long long start; Probably should add a comment why we can't use phys_addr_t? > + phys_size_t size; Should this be ULL too? > } bi_dram[CONFIG_NR_DRAM_BANKS]; > #endif /* CONFIG_NR_DRAM_BANKS */ > } bd_t; > -- > 1.8.2.1 > Regards, Simon
Hi Simon, On Fri, Jul 31, 2015 at 3:53 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 30 July 2015 at 04:49, Bin Meng <bmeng.cn@gmail.com> wrote: >> bd->bi_dram[] has both start address and size defined as 32-bit, >> which is not the case on some platforms where >=4GiB memory bank >> is used. Change them to support such memory banks. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> common/board_f.c | 2 +- >> include/asm-generic/u-boot.h | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/common/board_f.c b/common/board_f.c >> index 21be26f..a8ed6c8 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -206,7 +206,7 @@ static int show_dram_config(void) >> debug("\nRAM Configuration:\n"); >> for (i = size = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >> size += gd->bd->bi_dram[i].size; >> - debug("Bank #%d: %08lx ", i, gd->bd->bi_dram[i].start); >> + debug("Bank #%d: %016llx ", i, gd->bd->bi_dram[i].start); > > This makes all boards use an unreadable 16-character hex string. Can I > suggest we only use the larger value when the size demands it? A bit > messy but few things have >4GB memory so far. OK, we can do: if (gd->bd->bi_dram[i].start >= 0x100000000ULL) debug("Bank #%d: %016llx ", i, (unsigned long long)(gd->bd->bi_dram[i].start)); else debug("Bank #%d: %08lx ", i, (unsigned long)(gd->bd->bi_dram[i].start)); But this is debug output anyway, does that really matter? > >> #ifdef DEBUG >> print_size(gd->bd->bi_dram[i].size, "\n"); >> #endif >> diff --git a/include/asm-generic/u-boot.h b/include/asm-generic/u-boot.h >> index c918049..4e8b481 100644 >> --- a/include/asm-generic/u-boot.h >> +++ b/include/asm-generic/u-boot.h >> @@ -130,8 +130,8 @@ typedef struct bd_info { >> ulong bi_boot_params; /* where this board expects params */ >> #ifdef CONFIG_NR_DRAM_BANKS >> struct { /* RAM configuration */ >> - ulong start; >> - ulong size; >> + unsigned long long start; > > Probably should add a comment why we can't use phys_addr_t? I think we can use phys_addr_t. > >> + phys_size_t size; > > Should this be ULL too? OK, we can change them to: phys_addr_t start; phys_size_t size; It this OK? > >> } bi_dram[CONFIG_NR_DRAM_BANKS]; >> #endif /* CONFIG_NR_DRAM_BANKS */ >> } bd_t; >> -- Regards, Bin
Hi Bin, On 30 July 2015 at 21:44, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Fri, Jul 31, 2015 at 3:53 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 30 July 2015 at 04:49, Bin Meng <bmeng.cn@gmail.com> wrote: >>> bd->bi_dram[] has both start address and size defined as 32-bit, >>> which is not the case on some platforms where >=4GiB memory bank >>> is used. Change them to support such memory banks. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> common/board_f.c | 2 +- >>> include/asm-generic/u-boot.h | 4 ++-- >>> 2 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/common/board_f.c b/common/board_f.c >>> index 21be26f..a8ed6c8 100644 >>> --- a/common/board_f.c >>> +++ b/common/board_f.c >>> @@ -206,7 +206,7 @@ static int show_dram_config(void) >>> debug("\nRAM Configuration:\n"); >>> for (i = size = 0; i < CONFIG_NR_DRAM_BANKS; i++) { >>> size += gd->bd->bi_dram[i].size; >>> - debug("Bank #%d: %08lx ", i, gd->bd->bi_dram[i].start); >>> + debug("Bank #%d: %016llx ", i, gd->bd->bi_dram[i].start); >> >> This makes all boards use an unreadable 16-character hex string. Can I >> suggest we only use the larger value when the size demands it? A bit >> messy but few things have >4GB memory so far. > > OK, we can do: > > if (gd->bd->bi_dram[i].start >= 0x100000000ULL) > debug("Bank #%d: %016llx ", i, (unsigned long > long)(gd->bd->bi_dram[i].start)); > else > debug("Bank #%d: %08lx ", i, (unsigned long)(gd->bd->bi_dram[i].start)); > > But this is debug output anyway, does that really matter? > I know, but I think it is worth it. Most platforms don't want this mess. >> >>> #ifdef DEBUG >>> print_size(gd->bd->bi_dram[i].size, "\n"); >>> #endif >>> diff --git a/include/asm-generic/u-boot.h b/include/asm-generic/u-boot.h >>> index c918049..4e8b481 100644 >>> --- a/include/asm-generic/u-boot.h >>> +++ b/include/asm-generic/u-boot.h >>> @@ -130,8 +130,8 @@ typedef struct bd_info { >>> ulong bi_boot_params; /* where this board expects params */ >>> #ifdef CONFIG_NR_DRAM_BANKS >>> struct { /* RAM configuration */ >>> - ulong start; >>> - ulong size; >>> + unsigned long long start; >> >> Probably should add a comment why we can't use phys_addr_t? > > I think we can use phys_addr_t. On a 32-bit platform we might face an address beyond 4GB. But in that case we would not be able to boot anyway. So yes I think you are right. > >> >>> + phys_size_t size; >> >> Should this be ULL too? > > OK, we can change them to: > > phys_addr_t start; > phys_size_t size; > > It this OK? Yes. > >> >>> } bi_dram[CONFIG_NR_DRAM_BANKS]; >>> #endif /* CONFIG_NR_DRAM_BANKS */ >>> } bd_t; >>> -- > > Regards, > Bin Regards, Simon
diff --git a/common/board_f.c b/common/board_f.c index 21be26f..a8ed6c8 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -206,7 +206,7 @@ static int show_dram_config(void) debug("\nRAM Configuration:\n"); for (i = size = 0; i < CONFIG_NR_DRAM_BANKS; i++) { size += gd->bd->bi_dram[i].size; - debug("Bank #%d: %08lx ", i, gd->bd->bi_dram[i].start); + debug("Bank #%d: %016llx ", i, gd->bd->bi_dram[i].start); #ifdef DEBUG print_size(gd->bd->bi_dram[i].size, "\n"); #endif diff --git a/include/asm-generic/u-boot.h b/include/asm-generic/u-boot.h index c918049..4e8b481 100644 --- a/include/asm-generic/u-boot.h +++ b/include/asm-generic/u-boot.h @@ -130,8 +130,8 @@ typedef struct bd_info { ulong bi_boot_params; /* where this board expects params */ #ifdef CONFIG_NR_DRAM_BANKS struct { /* RAM configuration */ - ulong start; - ulong size; + unsigned long long start; + phys_size_t size; } bi_dram[CONFIG_NR_DRAM_BANKS]; #endif /* CONFIG_NR_DRAM_BANKS */ } bd_t;
bd->bi_dram[] has both start address and size defined as 32-bit, which is not the case on some platforms where >=4GiB memory bank is used. Change them to support such memory banks. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- common/board_f.c | 2 +- include/asm-generic/u-boot.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)