[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

On Thu, Apr 16, 2020 at 03:39:19PM +0200, Marek Vasut wrote:
> On 4/16/20 3:21 PM, Tom Rini wrote:
> > On Thu, Apr 16, 2020 at 03:11:45PM +0200, Marek Vasut wrote:
> >> On 4/16/20 2:55 PM, Tom Rini wrote:
> >>> On Wed, Apr 15, 2020 at 08:06:45PM +0200, Marek Vasut wrote:
> >>>> On 4/15/20 7:44 PM, Tom Rini wrote:
> >>>>> On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
> >>>>>> On 4/15/20 5:14 PM, Tom Rini wrote:
> >>>>>>> On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
> >>>>>>>> On 4/15/20 4:56 PM, Tom Rini wrote:
> >>>>>>>>> On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
> >>>>>>>>>> On 4/15/20 11:00 AM, Ley Foon Tan wrote:
> >>>>>>>>>>> Tiny printf doesn't support %i, change to %u.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Ley Foon Tan <ley.foon.tan at intel.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  drivers/ddr/altera/sdram_arria10.c | 2 +-
> >>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c
> >>>>>>>>>>> index e3f11984a978..8acf324117af 100644
> >>>>>>>>>>> --- a/drivers/ddr/altera/sdram_arria10.c
> >>>>>>>>>>> +++ b/drivers/ddr/altera/sdram_arria10.c
> >>>>>>>>>>> @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
> >>>>>>>>>>>  
> >>>>>>>>>>>  	dcache_enable();
> >>>>>>>>>>>  
> >>>>>>>>>>> -	printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
> >>>>>>>>>>> +	printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20);
> >>>>>>>>>>>  	memset((void *)0x8000, 0, size - 0x8000);
> >>>>>>>>>>>  	flush_dcache_all();
> >>>>>>>>>>>  	printf("DDRCAL: Scrubbing ECC RAM done.\n");
> >>>>>>>>>>
> >>>>>>>>>> Yes, sadly, tiny printf is broken so we need to patch code to work
> >>>>>>>>>> around that breakage.
> >>>>>>>>>
> >>>>>>>>> Yes, limited by design, thanks for changing.
> >>>>>>>>
> >>>>>>>> This code could be used without tiny printf, so this change is unnecessary.
> >>>>>>>
> >>>>>>> You've got it backwards.  Code that could be used by tiny printf needs
> >>>>>>> to use the more limited set of formats.  But this should have been using
> >>>>>>> %u all along?  %i is for int, %u is unsigned int.
> >>>>>>
> >>>>>> That would mean most of U-Boot needs to be limited to the subset of
> >>>>>> formatting characters supported by tiny printf, which is unrealistic.
> >>>>>
> >>>>> Not at all.  Only the code that's used in SPL and in tiny printf
> >>>>> situations needs to be limited to reduced set.  Which is why we're at
> >>>>> 4.5 years in and just now "oh, %i doesn't work?".
> >>>>
> >>>> I keep running into "oh, %i, such a basic C printf() feature, doesn't
> >>>> work" again and again, and it makes my work with U-Boot real annoying.
> >>>> This should be fixed in the printf implementation, not all over the code
> >>>> base. Also, it prevents sane code reuse, unless we start adding #ifdef
> >>>> TINY_PRINTF all over the place, which is just ew ...
> >>>
> >>> I hear you saying "I type %i not %d without thinking about it", but I'm
> >>> telling you, think about it.
> >>
> >> No, it works everywhere else just fine, except U-Boot SPL is special.
> > 
> > It works fine in SPL when we use the full printf, which is still a good
> > percent of the boards.
> > 
> >> This is a tiny-printf bug, period.
> > 
> > It's a feature of the explicitly reduced functionality printf.  It's the
> > whole point of the feature, provide as much output with as little code
> > as we can.
> 
> Not supporting standard features is a bug, because it makes SPL behavior
> non-standard.

It's a non-standard function.  There's all sorts of standard printf
formats it does not support, on purpose.

> >>> I will not grow 200+ boards when there's
> >>> an easy way not to.
> >>
> >> By ~6 bytes, which happens with almost every DM patch.
> >> I am not buying the size argument.
> > 
> > Nope, not true.  Boards with tiny printf rarely grow their SPL size from
> > general changes (SoC trees only get scrutiny over growth in platforms
> > outside their area) because I get this annoying about why they grow in
> > size.
> 
> OK, so look at socfpga_cyclone5_defconfig for example, which grew by 32
> bytes between 2020.04 and u-boot/master thanks to:
> 
> commit 0486497e2b5f4d36fa968a1a60fea358cbf70b65
>     lib: Improve _parse_integer_fixup_radix base 16 detection
> 
> It uses tiny-printf, it grew from general change, and it came from SoC tree:
> https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze/-/commits/xilinx-for-v2020.07
> 
> It didn't take too long to find a counter-example ...

Yup, it grew for a bugfix.  The full growth is:
               spl-u-boot-spl: add: 0/0, grow: 3/0 bytes: 56/0 (56)
                 function                                   old     new   delta
                 _parse_integer_fixup_radix                  92     120     +28
                 ns16550_serial_ofdata_to_platdata          104     128     +24
                 ns16550_serial_probe                        76      80      +4

For other bugfixes too.  Any of those bugfixes that can be done with
zero size growth would be appreciated. 

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200416/42e83785/attachment.sig>