| Message ID | 1463256198-3829-8-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
On 05/14/2016 10:02 PM, Simon Glass wrote: > Move these to debug() like the one in check_cache range(), to save SPL space. This hides cache problems, which were visibly reported so far. I am opposed to this patch. Wouldn't it make more sense to completely disable printf() and co. in SPL if you're after saving space? > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > arch/arm/cpu/armv7/cache_v7.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c > index dc309da..68cf62e 100644 > --- a/arch/arm/cpu/armv7/cache_v7.c > +++ b/arch/arm/cpu/armv7/cache_v7.c > @@ -66,8 +66,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) > * invalidate the first cache-line > */ > if (start & (line_len - 1)) { > - printf("ERROR: %s - start address is not aligned - 0x%08x\n", > - __func__, start); > + debug("ERROR: %s - start address is not aligned - 0x%08x\n", > + __func__, start); > /* move to next cache line */ > start = (start + line_len - 1) & ~(line_len - 1); > } > @@ -77,8 +77,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) > * invalidate the last cache-line > */ > if (stop & (line_len - 1)) { > - printf("ERROR: %s - stop address is not aligned - 0x%08x\n", > - __func__, stop); > + debug("ERROR: %s - stop address is not aligned - 0x%08x\n", > + __func__, stop); > /* align to the beginning of this cache line */ > stop &= ~(line_len - 1); > } >
Hi Marek, On 14 May 2016 at 14:23, Marek Vasut <marex@denx.de> wrote: > On 05/14/2016 10:02 PM, Simon Glass wrote: >> Move these to debug() like the one in check_cache range(), to save SPL space. > > This hides cache problems, which were visibly reported so far. > I am opposed to this patch. Sure, but see check_cache_range(). It uses debug(). In fact I found the at91 cache problem only after trying #define DEBUG in the code there. > > Wouldn't it make more sense to completely disable printf() and co. > in SPL if you're after saving space? Or maybe we need something that prints a message in U-Boot proper, but not SPL? I'll take a look. > >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> arch/arm/cpu/armv7/cache_v7.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c >> index dc309da..68cf62e 100644 >> --- a/arch/arm/cpu/armv7/cache_v7.c >> +++ b/arch/arm/cpu/armv7/cache_v7.c >> @@ -66,8 +66,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >> * invalidate the first cache-line >> */ >> if (start & (line_len - 1)) { >> - printf("ERROR: %s - start address is not aligned - 0x%08x\n", >> - __func__, start); >> + debug("ERROR: %s - start address is not aligned - 0x%08x\n", >> + __func__, start); >> /* move to next cache line */ >> start = (start + line_len - 1) & ~(line_len - 1); >> } >> @@ -77,8 +77,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >> * invalidate the last cache-line >> */ >> if (stop & (line_len - 1)) { >> - printf("ERROR: %s - stop address is not aligned - 0x%08x\n", >> - __func__, stop); >> + debug("ERROR: %s - stop address is not aligned - 0x%08x\n", >> + __func__, stop); >> /* align to the beginning of this cache line */ >> stop &= ~(line_len - 1); >> } >> > > > -- > Best regards, > Marek Vasut Regards, Simon
On 05/14/2016 11:22 PM, Simon Glass wrote: > Hi Marek, Hi! > On 14 May 2016 at 14:23, Marek Vasut <marex@denx.de> wrote: >> On 05/14/2016 10:02 PM, Simon Glass wrote: >>> Move these to debug() like the one in check_cache range(), to save SPL space. >> >> This hides cache problems, which were visibly reported so far. >> I am opposed to this patch. > > Sure, but see check_cache_range(). It uses debug(). In fact I found > the at91 cache problem only after trying #define DEBUG in the code > there. Which is the reason we should really be vocal about such cache misuse. I had a few of such cache problems bite me too, which is why I would like to avoid silencing this warning with debug() by default. I think check_cache_range() should also be fixed and should use printf() by default. >> >> Wouldn't it make more sense to completely disable printf() and co. >> in SPL if you're after saving space? > > Or maybe we need something that prints a message in U-Boot proper, but > not SPL? I'll take a look. But what if you trigger the issue only in SPL ? >> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> arch/arm/cpu/armv7/cache_v7.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c >>> index dc309da..68cf62e 100644 >>> --- a/arch/arm/cpu/armv7/cache_v7.c >>> +++ b/arch/arm/cpu/armv7/cache_v7.c >>> @@ -66,8 +66,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >>> * invalidate the first cache-line >>> */ >>> if (start & (line_len - 1)) { >>> - printf("ERROR: %s - start address is not aligned - 0x%08x\n", >>> - __func__, start); >>> + debug("ERROR: %s - start address is not aligned - 0x%08x\n", >>> + __func__, start); >>> /* move to next cache line */ >>> start = (start + line_len - 1) & ~(line_len - 1); >>> } >>> @@ -77,8 +77,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >>> * invalidate the last cache-line >>> */ >>> if (stop & (line_len - 1)) { >>> - printf("ERROR: %s - stop address is not aligned - 0x%08x\n", >>> - __func__, stop); >>> + debug("ERROR: %s - stop address is not aligned - 0x%08x\n", >>> + __func__, stop); >>> /* align to the beginning of this cache line */ >>> stop &= ~(line_len - 1); >>> } >>> >> >> >> -- >> Best regards, >> Marek Vasut > > > Regards, > Simon >
Hi Marek, On 14 May 2016 at 15:41, Marek Vasut <marex@denx.de> wrote: > On 05/14/2016 11:22 PM, Simon Glass wrote: >> Hi Marek, > > Hi! > >> On 14 May 2016 at 14:23, Marek Vasut <marex@denx.de> wrote: >>> On 05/14/2016 10:02 PM, Simon Glass wrote: >>>> Move these to debug() like the one in check_cache range(), to save SPL space. >>> >>> This hides cache problems, which were visibly reported so far. >>> I am opposed to this patch. >> >> Sure, but see check_cache_range(). It uses debug(). In fact I found >> the at91 cache problem only after trying #define DEBUG in the code >> there. > > Which is the reason we should really be vocal about such cache misuse. > I had a few of such cache problems bite me too, which is why I would > like to avoid silencing this warning with debug() by default. > > I think check_cache_range() should also be fixed and should use printf() > by default. > >>> >>> Wouldn't it make more sense to completely disable printf() and co. >>> in SPL if you're after saving space? >> >> Or maybe we need something that prints a message in U-Boot proper, but >> not SPL? I'll take a look. > > But what if you trigger the issue only in SPL ? Yes, but is that likely? So far I don't think the cache is enabled in SPL... > >>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> arch/arm/cpu/armv7/cache_v7.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c >>>> index dc309da..68cf62e 100644 >>>> --- a/arch/arm/cpu/armv7/cache_v7.c >>>> +++ b/arch/arm/cpu/armv7/cache_v7.c >>>> @@ -66,8 +66,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >>>> * invalidate the first cache-line >>>> */ >>>> if (start & (line_len - 1)) { >>>> - printf("ERROR: %s - start address is not aligned - 0x%08x\n", >>>> - __func__, start); >>>> + debug("ERROR: %s - start address is not aligned - 0x%08x\n", >>>> + __func__, start); >>>> /* move to next cache line */ >>>> start = (start + line_len - 1) & ~(line_len - 1); >>>> } >>>> @@ -77,8 +77,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >>>> * invalidate the last cache-line >>>> */ >>>> if (stop & (line_len - 1)) { >>>> - printf("ERROR: %s - stop address is not aligned - 0x%08x\n", >>>> - __func__, stop); >>>> + debug("ERROR: %s - stop address is not aligned - 0x%08x\n", >>>> + __func__, stop); >>>> /* align to the beginning of this cache line */ >>>> stop &= ~(line_len - 1); >>>> } Regards, Simon
On 05/19/2016 06:02 AM, Simon Glass wrote: > Hi Marek, > > On 14 May 2016 at 15:41, Marek Vasut <marex@denx.de> wrote: >> On 05/14/2016 11:22 PM, Simon Glass wrote: >>> Hi Marek, >> >> Hi! >> >>> On 14 May 2016 at 14:23, Marek Vasut <marex@denx.de> wrote: >>>> On 05/14/2016 10:02 PM, Simon Glass wrote: >>>>> Move these to debug() like the one in check_cache range(), to save SPL space. >>>> >>>> This hides cache problems, which were visibly reported so far. >>>> I am opposed to this patch. >>> >>> Sure, but see check_cache_range(). It uses debug(). In fact I found >>> the at91 cache problem only after trying #define DEBUG in the code >>> there. >> >> Which is the reason we should really be vocal about such cache misuse. >> I had a few of such cache problems bite me too, which is why I would >> like to avoid silencing this warning with debug() by default. >> >> I think check_cache_range() should also be fixed and should use printf() >> by default. >> >>>> >>>> Wouldn't it make more sense to completely disable printf() and co. >>>> in SPL if you're after saving space? >>> >>> Or maybe we need something that prints a message in U-Boot proper, but >>> not SPL? I'll take a look. >> >> But what if you trigger the issue only in SPL ? > > Yes, but is that likely? So far I don't think the cache is enabled in SPL... Yeah, it's probably unlikely. btw have you tried patching away all console IO support in SPL? Does it save space? >> >>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>> --- >>>>> >>>>> arch/arm/cpu/armv7/cache_v7.c | 8 ++++---- >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c >>>>> index dc309da..68cf62e 100644 >>>>> --- a/arch/arm/cpu/armv7/cache_v7.c >>>>> +++ b/arch/arm/cpu/armv7/cache_v7.c >>>>> @@ -66,8 +66,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >>>>> * invalidate the first cache-line >>>>> */ >>>>> if (start & (line_len - 1)) { >>>>> - printf("ERROR: %s - start address is not aligned - 0x%08x\n", >>>>> - __func__, start); >>>>> + debug("ERROR: %s - start address is not aligned - 0x%08x\n", >>>>> + __func__, start); >>>>> /* move to next cache line */ >>>>> start = (start + line_len - 1) & ~(line_len - 1); >>>>> } >>>>> @@ -77,8 +77,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) >>>>> * invalidate the last cache-line >>>>> */ >>>>> if (stop & (line_len - 1)) { >>>>> - printf("ERROR: %s - stop address is not aligned - 0x%08x\n", >>>>> - __func__, stop); >>>>> + debug("ERROR: %s - stop address is not aligned - 0x%08x\n", >>>>> + __func__, stop); >>>>> /* align to the beginning of this cache line */ >>>>> stop &= ~(line_len - 1); >>>>> } > > Regards, > Simon >
Hi Marek, On 19 May 2016 at 08:22, Marek Vasut <marex@denx.de> wrote: > On 05/19/2016 06:02 AM, Simon Glass wrote: >> Hi Marek, >> >> On 14 May 2016 at 15:41, Marek Vasut <marex@denx.de> wrote: >>> On 05/14/2016 11:22 PM, Simon Glass wrote: >>>> Hi Marek, >>> >>> Hi! >>> >>>> On 14 May 2016 at 14:23, Marek Vasut <marex@denx.de> wrote: >>>>> On 05/14/2016 10:02 PM, Simon Glass wrote: >>>>>> Move these to debug() like the one in check_cache range(), to save SPL space. >>>>> >>>>> This hides cache problems, which were visibly reported so far. >>>>> I am opposed to this patch. >>>> >>>> Sure, but see check_cache_range(). It uses debug(). In fact I found >>>> the at91 cache problem only after trying #define DEBUG in the code >>>> there. >>> >>> Which is the reason we should really be vocal about such cache misuse. >>> I had a few of such cache problems bite me too, which is why I would >>> like to avoid silencing this warning with debug() by default. >>> >>> I think check_cache_range() should also be fixed and should use printf() >>> by default. >>> >>>>> >>>>> Wouldn't it make more sense to completely disable printf() and co. >>>>> in SPL if you're after saving space? >>>> >>>> Or maybe we need something that prints a message in U-Boot proper, but >>>> not SPL? I'll take a look. >>> >>> But what if you trigger the issue only in SPL ? >> >> Yes, but is that likely? So far I don't think the cache is enabled in SPL... > > Yeah, it's probably unlikely. > > btw have you tried patching away all console IO support in SPL? Does it > save space? No I have not. I imagine it would, though. There is also the option now of using the debug UART, which avoids the small amount of serial/console overhead. [snip] Regards, Simon
diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c index dc309da..68cf62e 100644 --- a/arch/arm/cpu/armv7/cache_v7.c +++ b/arch/arm/cpu/armv7/cache_v7.c @@ -66,8 +66,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) * invalidate the first cache-line */ if (start & (line_len - 1)) { - printf("ERROR: %s - start address is not aligned - 0x%08x\n", - __func__, start); + debug("ERROR: %s - start address is not aligned - 0x%08x\n", + __func__, start); /* move to next cache line */ start = (start + line_len - 1) & ~(line_len - 1); } @@ -77,8 +77,8 @@ static void v7_dcache_inval_range(u32 start, u32 stop, u32 line_len) * invalidate the last cache-line */ if (stop & (line_len - 1)) { - printf("ERROR: %s - stop address is not aligned - 0x%08x\n", - __func__, stop); + debug("ERROR: %s - stop address is not aligned - 0x%08x\n", + __func__, stop); /* align to the beginning of this cache line */ stop &= ~(line_len - 1); }
Move these to debug() like the one in check_cache range(), to save SPL space. Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/arm/cpu/armv7/cache_v7.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)