From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Thu, 16 Apr 2020 14:02:39 -0400 Subject: [PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf In-Reply-To: <64d0baf5-3aa3-79f7-40fd-81131fc02996@denx.de> References: <20200415145647.GA12111@bill-the-cat> <189570cc-0a06-b6a7-ccba-445674e836cb@denx.de> <20200415151454.GB12111@bill-the-cat> <20200415174443.GE12111@bill-the-cat> <3bba1f9d-35a6-1795-16fe-aa7c262af106@denx.de> <20200416125525.GP12111@bill-the-cat> <155caf0b-ea50-f5d2-0405-1e692064383e@denx.de> <20200416132126.GQ12111@bill-the-cat> <64d0baf5-3aa3-79f7-40fd-81131fc02996@denx.de> Message-ID: <20200416180239.GY12111@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >>>>>>>>>>> --- > >>>>>>>>>>> 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: