| Message ID | 20200415090030.128489-7-ley.foon.tan@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ddr: altera: arria10: Convert SDRAM driver to DM | expand |
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.
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. Reviewed-by: Tom Rini <trini at konsulko.com>
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.
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.
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.
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?".
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 ...
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. I will not grow 200+ boards when there's an easy way not to.
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. This is a tiny-printf bug, period. > 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.
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. > > > 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.
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 ...
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.
On 4/16/20 8:02 PM, Tom Rini wrote: [...] >>>>> 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. So, how is this bugfix applicable, while a bugfix which fixes tiny printf to behave sane is not ? I don't really see a distinction here, sorry. And you should have rejected the above and asked for a more optimized version, based on 'I get this annoying about why they grow in size.'
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");
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(-)