| Message ID | 20191021232652.132164-4-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 831c1611195961bf79ac55a8deaac9034112ef5f |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | bootstage: TPL and SPL improvements | expand |
With a bit of code reordering we can support %p using the existing code
for ulong.
Move the %p code up and adjust the logic accordingly.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add a new patch to support %p without DEBUG
lib/tiny-printf.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
Applied to u-boot-dm, thanks!
Hi Simon, On 22/10/19 4:56 am, Simon Glass wrote: > With a bit of code reordering we can support %p using the existing code > for ulong. > > Move the %p code up and adjust the logic accordingly. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > This patch seems to have broken Ethernet boot in am335x-evm for me. It seems to be caused by SPL not being able to set ethaddr variable but its not obvious to me why this would cause it to happen. Here's a log: Trying to boot from USB eth ## Error: flags type check failure for "ethaddr" <= "40309f20M" (type: m) ## Error inserting "ethaddr" variable, errno=1 Warning: eth_cpsw using MAC address from ROM eth0: eth_cpsw## Error: flags type check failure for "eth1addr" <= "81f01098M" (type: m) ## Error inserting "eth1addr" variable, errno=1 Warning: usb_ether using MAC address from ROM , eth1: usb_ether eth_cpsw Waiting for PHY auto negotiation to complete...... done link up on port 0, speed 1000, full duplex BOOTP broadcast 1 BOOTP broadcast 2 BOOTP broadcast 3 BOOTP broadcast 4 BOOTP broadcast 5 BOOTP broadcast 6 BOOTP broadcast 7 BOOTP broadcast 8 BOOTP broadcast 9 BOOTP broadcast 10 BOOTP broadcast 11 BOOTP broadcast 12 BOOTP broadcast 13 BOOTP broadcast 14 BOOTP broadcast 15 BOOTP broadcast 16 BOOTP broadcast 17 Retry time exceeded; starting again Problem booting with BOOTP SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ### Reverting this patch on the latest U-boot master fixes the issue for me. I'll look into this more deeply tomorrow. Let me know if you see something obviously wrong with the patch. Thanks, Faiz
Hi Faiz, On Thu, 30 Jan 2020 at 08:22, Faiz Abbas <faiz_abbas@ti.com> wrote: > > Hi Simon, > > On 22/10/19 4:56 am, Simon Glass wrote: > > With a bit of code reordering we can support %p using the existing code > > for ulong. > > > > Move the %p code up and adjust the logic accordingly. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > This patch seems to have broken Ethernet boot in am335x-evm for me. It > seems to be caused by SPL not being able to set ethaddr variable but its > not obvious to me why this would cause it to happen. Here's a log: > > Trying to boot from USB eth > ## Error: flags type check failure for "ethaddr" <= "40309f20M" (type: m) > ## Error inserting "ethaddr" variable, errno=1 > > Warning: eth_cpsw using MAC address from ROM > eth0: eth_cpsw## Error: flags type check failure for "eth1addr" <= > "81f01098M" (type: m) > ## Error inserting "eth1addr" variable, errno=1 > > Warning: usb_ether using MAC address from ROM > , eth1: usb_ether > eth_cpsw Waiting for PHY auto negotiation to complete...... done > link up on port 0, speed 1000, full duplex > BOOTP broadcast 1 > BOOTP broadcast 2 > BOOTP broadcast 3 > BOOTP broadcast 4 > BOOTP broadcast 5 > BOOTP broadcast 6 > BOOTP broadcast 7 > BOOTP broadcast 8 > BOOTP broadcast 9 > BOOTP broadcast 10 > BOOTP broadcast 11 > BOOTP broadcast 12 > BOOTP broadcast 13 > BOOTP broadcast 14 > BOOTP broadcast 15 > BOOTP broadcast 16 > BOOTP broadcast 17 > > Retry time exceeded; starting again > Problem booting with BOOTP > SPL: failed to boot from all boot devices > ### ERROR ### Please RESET the board ### > > Reverting this patch on the latest U-boot master fixes the issue for me. > > I'll look into this more deeply tomorrow. Let me know if you see > something obviously wrong with the patch. Well one thing is that eth_env_set_enetaddr() called from the board's board.c has this: sprintf(buf, "%pM", enetaddr); which is not supported with tiny-printf. Before my patch it would probably produce an empty string, but now it will produce garbage. Perhaps need an SPL check there. Regards, Simon
Hi Simon, On 31/01/20 7:57 am, Simon Glass wrote: > Hi Faiz, > > On Thu, 30 Jan 2020 at 08:22, Faiz Abbas <faiz_abbas@ti.com> wrote: >> >> Hi Simon, >> >> On 22/10/19 4:56 am, Simon Glass wrote: >>> With a bit of code reordering we can support %p using the existing code >>> for ulong. >>> >>> Move the %p code up and adjust the logic accordingly. >>> [...] >> >> Retry time exceeded; starting again >> Problem booting with BOOTP >> SPL: failed to boot from all boot devices >> ### ERROR ### Please RESET the board ### >> >> Reverting this patch on the latest U-boot master fixes the issue for me. >> >> I'll look into this more deeply tomorrow. Let me know if you see >> something obviously wrong with the patch. > > Well one thing is that eth_env_set_enetaddr() called from the board's > board.c has this: > > sprintf(buf, "%pM", enetaddr); > > which is not supported with tiny-printf. That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See: https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183 I added this specifically to support Ethernet Boot usecases on TI platforms But above commit seems to move pointer() function that formats the output under #ifdef DEBUG which definitely breaks %pM > > Before my patch it would probably produce an empty string, but now it > will produce garbage. Perhaps need an SPL check there. > > Regards, > Simon >
Hi Vignesh, On Thu, 30 Jan 2020 at 22:12, Vignesh Raghavendra <vigneshr@ti.com> wrote: > > Hi Simon, > > On 31/01/20 7:57 am, Simon Glass wrote: > > Hi Faiz, > > > > On Thu, 30 Jan 2020 at 08:22, Faiz Abbas <faiz_abbas@ti.com> wrote: > >> > >> Hi Simon, > >> > >> On 22/10/19 4:56 am, Simon Glass wrote: > >>> With a bit of code reordering we can support %p using the existing code > >>> for ulong. > >>> > >>> Move the %p code up and adjust the logic accordingly. > >>> > > [...] > >> > >> Retry time exceeded; starting again > >> Problem booting with BOOTP > >> SPL: failed to boot from all boot devices > >> ### ERROR ### Please RESET the board ### > >> > >> Reverting this patch on the latest U-boot master fixes the issue for me. > >> > >> I'll look into this more deeply tomorrow. Let me know if you see > >> something obviously wrong with the patch. > > > > Well one thing is that eth_env_set_enetaddr() called from the board's > > board.c has this: > > > > sprintf(buf, "%pM", enetaddr); > > > > which is not supported with tiny-printf. > > That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See: > > https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183 > > I added this specifically to support Ethernet Boot usecases on TI platforms > > But above commit seems to move pointer() function that formats the > output under #ifdef DEBUG which definitely breaks %pM OK I see. I think it is too confusing to use #ifdef DEBUG in this code. One fix would be to change pointer() to return true if it actually does something. I'll take a look. This code needs tests also. Vignesh, do you feel like writing something? > > > > > Before my patch it would probably produce an empty string, but now it > > will produce garbage. Perhaps need an SPL check there. Regards, Simon
Faiz, On 31/01/20 11:44 pm, Simon Glass wrote: > Hi Vignesh, > > On Thu, 30 Jan 2020 at 22:12, Vignesh Raghavendra <vigneshr@ti.com> wrote: >> >> Hi Simon, >> >> On 31/01/20 7:57 am, Simon Glass wrote: >>> Hi Faiz, >>> >>> On Thu, 30 Jan 2020 at 08:22, Faiz Abbas <faiz_abbas@ti.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On 22/10/19 4:56 am, Simon Glass wrote: >>>>> With a bit of code reordering we can support %p using the existing code >>>>> for ulong. >>>>> >>>>> Move the %p code up and adjust the logic accordingly. >>>>> >> >> [...] >>>> >>>> Retry time exceeded; starting again >>>> Problem booting with BOOTP >>>> SPL: failed to boot from all boot devices >>>> ### ERROR ### Please RESET the board ### >>>> >>>> Reverting this patch on the latest U-boot master fixes the issue for me. >>>> >>>> I'll look into this more deeply tomorrow. Let me know if you see >>>> something obviously wrong with the patch. >>> >>> Well one thing is that eth_env_set_enetaddr() called from the board's >>> board.c has this: >>> >>> sprintf(buf, "%pM", enetaddr); >>> >>> which is not supported with tiny-printf. >> >> That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See: >> >> https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183 >> >> I added this specifically to support Ethernet Boot usecases on TI platforms >> >> But above commit seems to move pointer() function that formats the >> output under #ifdef DEBUG which definitely breaks %pM > > OK I see. I think it is too confusing to use #ifdef DEBUG in this code. > > One fix would be to change pointer() to return true if it actually > does something. I'll take a look. > > This code needs tests also. Vignesh, do you feel like writing something? > Is there a testcase for full printf()? I am not sure where to look for. Is test/print_ut.c the right place to add new test?
Hi Vignesh, On Tue, 4 Feb 2020 at 00:58, Vignesh Raghavendra <vigneshr@ti.com> wrote: > > Faiz, > > On 31/01/20 11:44 pm, Simon Glass wrote: > > Hi Vignesh, > > > > On Thu, 30 Jan 2020 at 22:12, Vignesh Raghavendra <vigneshr@ti.com> wrote: > >> > >> Hi Simon, > >> > >> On 31/01/20 7:57 am, Simon Glass wrote: > >>> Hi Faiz, > >>> > >>> On Thu, 30 Jan 2020 at 08:22, Faiz Abbas <faiz_abbas@ti.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On 22/10/19 4:56 am, Simon Glass wrote: > >>>>> With a bit of code reordering we can support %p using the existing code > >>>>> for ulong. > >>>>> > >>>>> Move the %p code up and adjust the logic accordingly. > >>>>> > >> > >> [...] > >>>> > >>>> Retry time exceeded; starting again > >>>> Problem booting with BOOTP > >>>> SPL: failed to boot from all boot devices > >>>> ### ERROR ### Please RESET the board ### > >>>> > >>>> Reverting this patch on the latest U-boot master fixes the issue for me. > >>>> > >>>> I'll look into this more deeply tomorrow. Let me know if you see > >>>> something obviously wrong with the patch. > >>> > >>> Well one thing is that eth_env_set_enetaddr() called from the board's > >>> board.c has this: > >>> > >>> sprintf(buf, "%pM", enetaddr); > >>> > >>> which is not supported with tiny-printf. > >> > >> That is not true. %pM is supported when SPL_NET_SUPPORT is enabled. See: > >> > >> https://gitlab.denx.de/u-boot/u-boot/blob/master/lib/tiny-printf.c#L183 > >> > >> I added this specifically to support Ethernet Boot usecases on TI platforms > >> > >> But above commit seems to move pointer() function that formats the > >> output under #ifdef DEBUG which definitely breaks %pM > > > > OK I see. I think it is too confusing to use #ifdef DEBUG in this code. > > > > One fix would be to change pointer() to return true if it actually > > does something. I'll take a look. > > > > This code needs tests also. Vignesh, do you feel like writing something? > > > > Is there a testcase for full printf()? I am not sure where to look for. > Is test/print_ut.c the right place to add new test? Yes. See also this commit in u-boot-dm/testing test: Add a way to check each line of console output Regards, Simon
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 430c3255bc5..c80c4f95ae0 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -157,7 +157,8 @@ static void ip4_addr_string(struct printf_info *info, u8 *addr) * decimal). */ -static void pointer(struct printf_info *info, const char *fmt, void *ptr) +static void __maybe_unused pointer(struct printf_info *info, const char *fmt, + void *ptr) { #ifdef DEBUG unsigned long num = (uintptr_t)ptr; @@ -266,6 +267,21 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) div_out(info, &num, div); } break; + case 'p': +#ifdef DEBUG + pointer(info, fmt, va_arg(va, void *)); + /* + * Skip this because it pulls in _ctype which is + * 256 bytes, and we don't generally implement + * pointer anyway + */ + while (isalnum(fmt[0])) + fmt++; + break; +#else + islong = true; + /* no break */ +#endif case 'x': if (islong) { num = va_arg(va, unsigned long); @@ -287,18 +303,6 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) case 's': p = va_arg(va, char*); break; - case 'p': - pointer(info, fmt, va_arg(va, void *)); - /* - * Skip this because it pulls in _ctype which is - * 256 bytes, and we don't generally implement - * pointer anyway - */ -#ifdef DEBUG - while (isalnum(fmt[0])) - fmt++; -#endif - break; case '%': out(info, '%'); default:
With a bit of code reordering we can support %p using the existing code for ulong. Move the %p code up and adjust the logic accordingly. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: - Add a new patch to support %p without DEBUG lib/tiny-printf.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)