[U-Boot] [PATCH v2] net: eth_common: Don't use %pM when USE_TINY_PRINTF is enabled
Tiny printf doesn't support %pM, so when CONFIG_USE_TINY_PRINTF is enabled use %x to manually print MAC address.
Signed-off-by: Vignesh R vigneshr@ti.com --- v2: use positive logic.
net/eth_common.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/net/eth_common.c b/net/eth_common.c index 58fa29577102..419b85bf5d8a 100644 --- a/net/eth_common.c +++ b/net/eth_common.c @@ -37,7 +37,14 @@ int eth_setenv_enetaddr(const char *name, const uchar *enetaddr) if (eth_getenv_enetaddr(name, (uchar *)buf)) return -EEXIST;
+#ifdef CONFIG_USE_TINY_PRINTF + sprintf(buf, "%x:%x:%x:%x:%x:%x", (unsigned int)enetaddr[0], + (unsigned int)enetaddr[1], (unsigned int)enetaddr[2], + (unsigned int)enetaddr[3], (unsigned int)enetaddr[4], + (unsigned int)enetaddr[5]); +#else sprintf(buf, "%pM", enetaddr); +#endif
return setenv(name, buf); }
Dear Vignesh,
In message 20170403054659.32553-1-vigneshr@ti.com you wrote:
Tiny printf doesn't support %pM, so when CONFIG_USE_TINY_PRINTF is enabled use %x to manually print MAC address.
...
net/eth_common.c | 7 +++++++ 1 file changed, 7 insertions(+)
I wonder if this is sufficient. There is a ton of other files which use the %pM format:
arch/arm/mach-davinci/misc.c board/BuR/common/common.c board/bct-brettl2/bct-brettl2.c board/keymile/common/ivm.c board/pdm360ng/pdm360ng.c board/spear/common/spr_misc.c drivers/net/dm9000x.c drivers/net/fec_mxc.c drivers/net/lan91c96.c drivers/net/ne2000_base.c drivers/net/sandbox.c drivers/net/smc91111.c drivers/net/smc911x.c drivers/usb/eth/asix.c drivers/usb/eth/asix88179.c drivers/usb/eth/mcs7830.c drivers/usb/eth/r8152.c drivers/usb/eth/smsc95xx.c drivers/usb/gadget/ether.c net/arp.c net/eth-uclass.c net/eth_common.c net/eth_legacy.c net/link_local.c net/net.c
Adding similar #ifdef's to all of these seems to be a really bad idea to me.
Also, it would result in a lot of code duplication which is exactly what the introduction of the %p? formats intends to avoid.
So hat we need here is a way to determin if the code for a given configuration uses a specific %p? format, and then enable this even with CONFIG_USE_TINY_PRINTF.
OTOH - what is the use case for CONFIG_USE_TINY_PRINTF in combination with network related code? I would expect to see this config option in memory restricted environments like SPL, but there you don't include support for networking - or do you?
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
On Monday 03 April 2017 12:03 PM, Wolfgang Denk wrote:
Dear Vignesh,
In message 20170403054659.32553-1-vigneshr@ti.com you wrote:
Tiny printf doesn't support %pM, so when CONFIG_USE_TINY_PRINTF is enabled use %x to manually print MAC address.
...
net/eth_common.c | 7 +++++++ 1 file changed, 7 insertions(+)
I wonder if this is sufficient. There is a ton of other files which use the %pM format:
arch/arm/mach-davinci/misc.c board/BuR/common/common.c board/bct-brettl2/bct-brettl2.c board/keymile/common/ivm.c board/pdm360ng/pdm360ng.c board/spear/common/spr_misc.c drivers/net/dm9000x.c drivers/net/fec_mxc.c drivers/net/lan91c96.c drivers/net/ne2000_base.c drivers/net/sandbox.c drivers/net/smc91111.c drivers/net/smc911x.c drivers/usb/eth/asix.c drivers/usb/eth/asix88179.c drivers/usb/eth/mcs7830.c drivers/usb/eth/r8152.c drivers/usb/eth/smsc95xx.c drivers/usb/gadget/ether.c net/arp.c net/eth-uclass.c net/eth_common.c net/eth_legacy.c net/link_local.c net/net.c
Adding similar #ifdef's to all of these seems to be a really bad idea to me.
Also, it would result in a lot of code duplication which is exactly what the introduction of the %p? formats intends to avoid.
So hat we need here is a way to determin if the code for a given configuration uses a specific %p? format, and then enable this even with CONFIG_USE_TINY_PRINTF.
I agree, networking widely uses %pM for printing mac addresses. Are you suggesting to port %pM support from lib/vsprintf.c to lib/tiny-printf.c and make it available based on CONFIG_CMD_NET?
OTOH - what is the use case for CONFIG_USE_TINY_PRINTF in combination with network related code? I would expect to see this config option in memory restricted environments like SPL, but there you don't include support for networking - or do you?
I am trying to use RNDIS boot on am437x/am335x boards with CONFIG_USE_TINY_PRINTF(to reduce code size due to internal SRAM size constraint) and therefore need networking support in SPL.
Dear Vignesh,
In message 2c453144-8aba-3e66-8f1f-fe468bcdc382@ti.com you wrote:
I agree, networking widely uses %pM for printing mac addresses. Are you suggesting to port %pM support from lib/vsprintf.c to lib/tiny-printf.c and make it available based on CONFIG_CMD_NET?
I don't think we should add another #ifdef for this. How much (in terms of number of bytes) does it hurt to add it without an #ifdef?
I guess we cannot handle this in some intelligent automatic way similar to dead code elimination?
I am trying to use RNDIS boot on am437x/am335x boards with CONFIG_USE_TINY_PRINTF(to reduce code size due to internal SRAM size constraint) and therefore need networking support in SPL.
So you need networking _and_ USB and still care about the printf() size? Ouch...
Best regards,
Wolfgang Denk
On Tuesday 04 April 2017 01:38 AM, Wolfgang Denk wrote:
Dear Vignesh,
In message 2c453144-8aba-3e66-8f1f-fe468bcdc382@ti.com you wrote:
I agree, networking widely uses %pM for printing mac addresses. Are you suggesting to port %pM support from lib/vsprintf.c to lib/tiny-printf.c and make it available based on CONFIG_CMD_NET?
I don't think we should add another #ifdef for this. How much (in terms of number of bytes) does it hurt to add it without an #ifdef?
I ported minimal support for %p? to tiny-printf and here are the stats:
With changes to support %p?(%p %pa[p], %pI[46], %pm, %pM) format in tiny printf SPL size increases by 509 bytes.
With changes to support just %p, %pa[p] formats in tiny-printf delta is ~75 bytes wrt plain SPL
Therefore, if %pM and other net related printf formats are disabled based on CONFIG_CMD_NET savings is ~434 bytes
Compiler used is: arm-linux-gnueabihf-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
I guess we cannot handle this in some intelligent automatic way similar to dead code elimination?
I am trying to use RNDIS boot on am437x/am335x boards with CONFIG_USE_TINY_PRINTF(to reduce code size due to internal SRAM size constraint) and therefore need networking support in SPL.
So you need networking _and_ USB and still care about the printf() size? Ouch...
USB RNDIS boot has been supported on am335x platform in mainline u-boot for quite some time. But the SPL size is steadily increasing due to which CONFIG_USE_TINY_PRINTF needs to be enabled to avoid SRAM overflow.
Best regards,
Wolfgang Denk
participants (2)
-
Vignesh R -
Wolfgang Denk