[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf
Marek Vasut
marex at denx.de
Thu Apr 16 15:39:19 CEST 2020
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.
>>> 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 ...
More information about the U-Boot
mailing list