Message ID | 1409067268-956-2-git-send-email-thierry.reding@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | Simon Glass |
Headers | show |
On 08/26/2014 09:33 AM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Provide a new modifier to vsprintf() to print phys_addr_t variables to > avoid having to cast or #ifdef when printing them out. The %pa modifier > is used for this purpose, so phys_addr_t variables need to be passed by > reference, like so: > > phys_addr_t start = 0; > > printf("start: %pa\n", &start); > > Depending on the size of phys_addr_t this will print out the address > with 8 or 16 hexadecimal digits following a 0x prefix. The series, Tested-by: Stephen Warren <swarren@nvidia.com> Note that I did see the following printed a couple of times when I executed "run bootcmd_pxe": pci_hose_bus_to_phys: invalid physical address ... but everything worked perfectly, so I guess we can track that down later.
Hi Thierry, On 26 August 2014 09:33, Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > Provide a new modifier to vsprintf() to print phys_addr_t variables to > avoid having to cast or #ifdef when printing them out. The %pa modifier > is used for this purpose, so phys_addr_t variables need to be passed by > reference, like so: > > phys_addr_t start = 0; > > printf("start: %pa\n", &start); > > Depending on the size of phys_addr_t this will print out the address > with 8 or 16 hexadecimal digits following a 0x prefix. Would it be better to use %#pa to get the 0x prefix so we have the option? Hex is the default in U-Boot. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > lib/vsprintf.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 7ec758e40fc5..044d5551bdd0 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -518,6 +518,8 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, > static char *pointer(const char *fmt, char *buf, char *end, void *ptr, > int field_width, int precision, int flags) > { > + u64 num = (uintptr_t)ptr; > + Will this impact code size much? I suppose it is vsprintf() so it doesn't matter too much? > /* > * Being a boot loader, we explicitly allow pointers to > * (physical) address null. > @@ -530,6 +532,17 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, > > #ifdef CONFIG_CMD_NET > switch (*fmt) { > + case 'a': > + flags |= SPECIAL | ZEROPAD; > + > + switch (fmt[1]) { > + case 'p': > + default: > + field_width = sizeof(phys_addr_t) * 2 + 2; > + num = *(phys_addr_t *)ptr; > + break; > + } > + break; > case 'm': > flags |= SPECIAL; > /* Fallthrough */ > @@ -555,8 +568,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, > field_width = 2*sizeof(void *); > flags |= ZEROPAD; > } > - return number(buf, end, (unsigned long)ptr, 16, field_width, > - precision, flags); > + return number(buf, end, num, 16, field_width, precision, flags); > } > > static int vsnprintf_internal(char *buf, size_t size, const char *fmt, > -- > 2.0.4 > Regards, Simon
On Tue, Aug 26, 2014 at 11:04:56AM -0600, Stephen Warren wrote: > On 08/26/2014 09:33 AM, Thierry Reding wrote: > >From: Thierry Reding <treding@nvidia.com> > > > >Provide a new modifier to vsprintf() to print phys_addr_t variables to > >avoid having to cast or #ifdef when printing them out. The %pa modifier > >is used for this purpose, so phys_addr_t variables need to be passed by > >reference, like so: > > > > phys_addr_t start = 0; > > > > printf("start: %pa\n", &start); > > > >Depending on the size of phys_addr_t this will print out the address > >with 8 or 16 hexadecimal digits following a 0x prefix. > > The series, > > Tested-by: Stephen Warren <swarren@nvidia.com> > > Note that I did see the following printed a couple of times when I executed > "run bootcmd_pxe": > > pci_hose_bus_to_phys: invalid physical address > > ... but everything worked perfectly, so I guess we can track that down > later. Yes, it should definitely be tracked down. I don't see that message on my setup. I've seen it for example when noncached_alloc() fails and returns 0, but in that case everything shouldn't be working perfectly. It would be helpful if that message showed what physical address was considered invalid. Thierry
On Tue, Aug 26, 2014 at 05:14:17PM -0600, Simon Glass wrote: > Hi Thierry, > > On 26 August 2014 09:33, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > Provide a new modifier to vsprintf() to print phys_addr_t variables to > > avoid having to cast or #ifdef when printing them out. The %pa modifier > > is used for this purpose, so phys_addr_t variables need to be passed by > > reference, like so: > > > > phys_addr_t start = 0; > > > > printf("start: %pa\n", &start); > > > > Depending on the size of phys_addr_t this will print out the address > > with 8 or 16 hexadecimal digits following a 0x prefix. > > Would it be better to use %#pa to get the 0x prefix so we have the > option? Hex is the default in U-Boot. Yeah, that's very confusing since there's no way to tell apart a hexadecimal number from a decimal one since there's no decimal equivalent of the 0x prefix. If you insist I can change that (it should be a simple matter of dropping the SPECIAL flag in the 'a' case). > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > lib/vsprintf.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 7ec758e40fc5..044d5551bdd0 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -518,6 +518,8 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, > > static char *pointer(const char *fmt, char *buf, char *end, void *ptr, > > int field_width, int precision, int flags) > > { > > + u64 num = (uintptr_t)ptr; > > + > > Will this impact code size much? I suppose it is vsprintf() so it > doesn't matter too much? Doing only this change and the corresponding change in the call to number() results in the exact sizes: $ size build/jetson-tk1.{before,after}/u-boot text data bss dec hex filename 310434 10872 309184 630490 99eda build/jetson-tk1.before/u-boot 310434 10872 309184 630490 99eda build/jetson-tk1.after/u-boot Given that the num parameter of the number() function is already a u64 the compiler will presumably automatically handle the ptr argument as a 64-bit value. Thierry
Hi Thierry, On 27 August 2014 01:37, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Aug 26, 2014 at 05:14:17PM -0600, Simon Glass wrote: >> Hi Thierry, >> >> On 26 August 2014 09:33, Thierry Reding <thierry.reding@gmail.com> wrote: >> > >> > From: Thierry Reding <treding@nvidia.com> >> > >> > Provide a new modifier to vsprintf() to print phys_addr_t variables to >> > avoid having to cast or #ifdef when printing them out. The %pa modifier >> > is used for this purpose, so phys_addr_t variables need to be passed by >> > reference, like so: >> > >> > phys_addr_t start = 0; >> > >> > printf("start: %pa\n", &start); >> > >> > Depending on the size of phys_addr_t this will print out the address >> > with 8 or 16 hexadecimal digits following a 0x prefix. >> >> Would it be better to use %#pa to get the 0x prefix so we have the >> option? Hex is the default in U-Boot. > > Yeah, that's very confusing since there's no way to tell apart a > hexadecimal number from a decimal one since there's no decimal > equivalent of the 0x prefix. > > If you insist I can change that (it should be a simple matter of > dropping the SPECIAL flag in the 'a' case). %p doesn't imply %#p, this seems like a unilateral change of behaviour. So I think you should drop the SPECIAL flag. It might be confusing in some cases but I feel it is quite expected that an 8- or 16-digit value will be hex in U-Boot. If you wish to change the default input/output base for U-Boot I feel that would require a larger patch. > >> > Signed-off-by: Thierry Reding <treding@nvidia.com> >> > --- >> > lib/vsprintf.c | 16 ++++++++++++++-- >> > 1 file changed, 14 insertions(+), 2 deletions(-) >> > >> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c >> > index 7ec758e40fc5..044d5551bdd0 100644 >> > --- a/lib/vsprintf.c >> > +++ b/lib/vsprintf.c >> > @@ -518,6 +518,8 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, >> > static char *pointer(const char *fmt, char *buf, char *end, void *ptr, >> > int field_width, int precision, int flags) >> > { >> > + u64 num = (uintptr_t)ptr; >> > + >> >> Will this impact code size much? I suppose it is vsprintf() so it >> doesn't matter too much? > > Doing only this change and the corresponding change in the call to > number() results in the exact sizes: > > $ size build/jetson-tk1.{before,after}/u-boot > text data bss dec hex filename > 310434 10872 309184 630490 99eda build/jetson-tk1.before/u-boot > 310434 10872 309184 630490 99eda build/jetson-tk1.after/u-boot > > Given that the num parameter of the number() function is already a u64 > the compiler will presumably automatically handle the ptr argument as a > 64-bit value. I hadn't picked that up - I though ulong was 32-bit. Still this code affects most boards. Regards, Simon > > Thierry
On 08/27/2014 01:01 AM, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 11:04:56AM -0600, Stephen Warren wrote: >> On 08/26/2014 09:33 AM, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> Provide a new modifier to vsprintf() to print phys_addr_t variables to >>> avoid having to cast or #ifdef when printing them out. The %pa modifier >>> is used for this purpose, so phys_addr_t variables need to be passed by >>> reference, like so: >>> >>> phys_addr_t start = 0; >>> >>> printf("start: %pa\n", &start); >>> >>> Depending on the size of phys_addr_t this will print out the address >>> with 8 or 16 hexadecimal digits following a 0x prefix. >> >> The series, >> >> Tested-by: Stephen Warren <swarren@nvidia.com> >> >> Note that I did see the following printed a couple of times when I executed >> "run bootcmd_pxe": >> >> pci_hose_bus_to_phys: invalid physical address >> >> ... but everything worked perfectly, so I guess we can track that down >> later. > > Yes, it should definitely be tracked down. I don't see that message on > my setup. I've seen it for example when noncached_alloc() fails and > returns 0, but in that case everything shouldn't be working perfectly. It looks like it happens when "dhcp" or "pxe get" attempt to download a file that doesn't exist on the server. > It would be helpful if that message showed what physical address was > considered invalid. The address is 0. I think this is because rtl_recv() is being called after rtl_halt(). rtl_halt() NULLs out all tpc->RxBufferRing[] entries, so calling rtl_recv() after that tries to convert address 0 to a PCI address. I haven't fully tracked this down, but it seems like a bug in the U-Boot networking or TFTP core, rather than anything to do with drivers for R8169 or the Tegra PCIe controller. I'm not planning on tracking this down any further at the moment.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 7ec758e40fc5..044d5551bdd0 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -518,6 +518,8 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field_width, int precision, int flags) { + u64 num = (uintptr_t)ptr; + /* * Being a boot loader, we explicitly allow pointers to * (physical) address null. @@ -530,6 +532,17 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, #ifdef CONFIG_CMD_NET switch (*fmt) { + case 'a': + flags |= SPECIAL | ZEROPAD; + + switch (fmt[1]) { + case 'p': + default: + field_width = sizeof(phys_addr_t) * 2 + 2; + num = *(phys_addr_t *)ptr; + break; + } + break; case 'm': flags |= SPECIAL; /* Fallthrough */ @@ -555,8 +568,7 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr, field_width = 2*sizeof(void *); flags |= ZEROPAD; } - return number(buf, end, (unsigned long)ptr, 16, field_width, - precision, flags); + return number(buf, end, num, 16, field_width, precision, flags); } static int vsnprintf_internal(char *buf, size_t size, const char *fmt,