Re: [U-Boot] [ PATCH] ARM: cache: runtime flag to suppress the "cache misaligned" message

[Marek Vasut]

On 03/07/2017 03:41 PM, Gary Bisson wrote:
> Hi Marek, All,
> 
> On Tue, Mar 07, 2017 at 03:52:23AM +0100, Marek Vasut wrote:
>> On 03/06/2017 11:21 PM, Steve Rae wrote:
>>> The "chunks" in the "fastboot sparse image" are not aligned,
>>> resulting in many "cached misaligned" messages from
>>> check_cache_range(). Implement a runtime flag to suppress this
>>> message, and use this flag when processing the "fastboot sparse
>>> image".
>>
>> Let me understand this, what we add here is a patch to silence a warning
>> which correctly indicates something totally wrong is happening instead
>> of fixing that problem ?
>>
>> From me, that is a NAK and if my understanding of this is correct, this
>> patch will go in only over my dead body.
>>
>>> Signed-off-by: Steve Rae  Reported-by: Gary
>>> Bisson 
>>>
>>> --- A little background: In the code that I am testing, the "sparse"
>>> image is 350,723,024 bytes and is downloaded into an aligned buffer
>>> on the device. This image/buffer contains a header ('a') and 13903
>>> "chunks" (b1/c1 through bx/cx):
>>
>> OT: Can you please fix your mailer to break lines at 80 chars ?
>> I reflowed the mail and the ascii art turned crap.
>>
>>> +---+++++ +++ | a | b1 | c1 | b2 | c2 |
>>> ... | bx | cx | +---+++++ +++ where: a  =
>>> the "sparse image" header (28 bytes) b* = the "chunk" header
>>> (12 bytes) c* = the "chunk" data  (which is always an exact
>>> multiple of CONFIG_SYS_CACHELINE_SIZE) Whenever the "chunk" type is
>>> CHUNK_TYPE_RAW, then the data in the current 'c' (the "chunk" data)
>>> needs to be written into the flash as is, using the pointer to the
>>> first byte of this 'c' as the starting address for the blk_dwrite().
>>> Because of the 28 byte image header, and the 12 byte "chunk"
>>> header(s), it is extremely unlikely that this starting address will
>>> be aligned - resulting in many "CACHE: Misaligned operation at range
>>> [, ]" messages while these 13093 "chunks" are processed. In
>>> the code that I am testing, this message is being generated by the
>>> call to flush_cache(start_addr, trans_bytes) in the
>>> sdhci_send_command() function.
>>
>> You can set this up such that the [a] part starts at offset +24 bytes, then
>> the [c] will be aligned.
> 
> This will align the first chunk but there's no guarantee the next one
> will be aligned.

OK, so the protocol is basically crappy ?

>>> Copying each "chuck data" to an aligned buffer before calling
>>> blk_dwrite() in order to avoid this message would be highly
>>> inefficient -- the desire is to download the code and flash it as
>>> fast as possible!
>>
>> Fast is great, but not if this makes the transfer unreliable, which will
>> happen if you cannot reliably flush/invalidate cache and you decide to
>> ignore this clear warning you're getting.
> 
> Actually fast isn't the problem, it is more than we can't afford to copy
> each chunk to an aligned buffer because of size constraints. Although we
> could break down "big" chunks to smaller one I guess.

Great

>>> Therefore, I propose this patch which provides a runtime flag to
>>> suppress this warning message, which is used when processing these
>>> fastboot "sparse" images.
>>
>> This is wrong.
> 
> Yes I agree that hiding the warning isn't solving anything. However I
> expected that disabling dcache would remove the warning but it doesn't.

Removing the warning if dcache is OFF is fine.

> Shouldn't the cache maintenance functions like flush_dcache_range()
> check the dcache_status() before doing anything?

They should.

> Regards,
> Gary
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [ PATCH] ARM: cache: runtime flag to suppress the "cache misaligned" message

[Gary Bisson]

Hi Marek, All,

On Tue, Mar 07, 2017 at 03:52:23AM +0100, Marek Vasut wrote:
> On 03/06/2017 11:21 PM, Steve Rae wrote:
> > The "chunks" in the "fastboot sparse image" are not aligned,
> > resulting in many "cached misaligned" messages from
> > check_cache_range(). Implement a runtime flag to suppress this
> > message, and use this flag when processing the "fastboot sparse
> > image".
> 
> Let me understand this, what we add here is a patch to silence a warning
> which correctly indicates something totally wrong is happening instead
> of fixing that problem ?
> 
> From me, that is a NAK and if my understanding of this is correct, this
> patch will go in only over my dead body.
> 
> > Signed-off-by: Steve Rae  Reported-by: Gary
> > Bisson 
> > 
> > --- A little background: In the code that I am testing, the "sparse"
> > image is 350,723,024 bytes and is downloaded into an aligned buffer
> > on the device. This image/buffer contains a header ('a') and 13903
> > "chunks" (b1/c1 through bx/cx):
> 
> OT: Can you please fix your mailer to break lines at 80 chars ?
> I reflowed the mail and the ascii art turned crap.
> 
> > +---+++++ +++ | a | b1 | c1 | b2 | c2 |
> > ... | bx | cx | +---+++++ +++ where: a  =
> > the "sparse image" header (28 bytes) b* = the "chunk" header
> > (12 bytes) c* = the "chunk" data  (which is always an exact
> > multiple of CONFIG_SYS_CACHELINE_SIZE) Whenever the "chunk" type is
> > CHUNK_TYPE_RAW, then the data in the current 'c' (the "chunk" data)
> > needs to be written into the flash as is, using the pointer to the
> > first byte of this 'c' as the starting address for the blk_dwrite().
> > Because of the 28 byte image header, and the 12 byte "chunk"
> > header(s), it is extremely unlikely that this starting address will
> > be aligned - resulting in many "CACHE: Misaligned operation at range
> > [, ]" messages while these 13093 "chunks" are processed. In
> > the code that I am testing, this message is being generated by the
> > call to flush_cache(start_addr, trans_bytes) in the
> > sdhci_send_command() function.
> 
> You can set this up such that the [a] part starts at offset +24 bytes, then
> the [c] will be aligned.

This will align the first chunk but there's no guarantee the next one
will be aligned.

> > Copying each "chuck data" to an aligned buffer before calling
> > blk_dwrite() in order to avoid this message would be highly
> > inefficient -- the desire is to download the code and flash it as
> > fast as possible!
> 
> Fast is great, but not if this makes the transfer unreliable, which will
> happen if you cannot reliably flush/invalidate cache and you decide to
> ignore this clear warning you're getting.

Actually fast isn't the problem, it is more than we can't afford to copy
each chunk to an aligned buffer because of size constraints. Although we
could break down "big" chunks to smaller one I guess.

> > Therefore, I propose this patch which provides a runtime flag to
> > suppress this warning message, which is used when processing these
> > fastboot "sparse" images.
> 
> This is wrong.

Yes I agree that hiding the warning isn't solving anything. However I
expected that disabling dcache would remove the warning but it doesn't.

Shouldn't the cache maintenance functions like flush_dcache_range()
check the dcache_status() before doing anything?

Regards,
Gary
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [ PATCH] ARM: cache: runtime flag to suppress the "cache misaligned" message

[Marek Vasut]

On 03/06/2017 11:21 PM, Steve Rae wrote:

The "chunks" in the "fastboot sparse image" are not aligned,
resulting in many "cached misaligned" messages from
check_cache_range(). Implement a runtime flag to suppress this
message, and use this flag when processing the "fastboot sparse
image".


Let me understand this, what we add here is a patch to silence a warning
which correctly indicates something totally wrong is happening instead
of fixing that problem ?

From me, that is a NAK and if my understanding of this is correct, this
patch will go in only over my dead body.


Signed-off-by: Steve Rae  Reported-by: Gary
Bisson 

--- A little background: In the code that I am testing, the "sparse"
image is 350,723,024 bytes and is downloaded into an aligned buffer
on the device. This image/buffer contains a header ('a') and 13903
"chunks" (b1/c1 through bx/cx):


OT: Can you please fix your mailer to break lines at 80 chars ?
I reflowed the mail and the ascii art turned crap.


+---+++++ +++ | a | b1 | c1 | b2 | c2 |
... | bx | cx | +---+++++ +++ where: a  =
the "sparse image" header (28 bytes) b* = the "chunk" header
(12 bytes) c* = the "chunk" data  (which is always an exact
multiple of CONFIG_SYS_CACHELINE_SIZE) Whenever the "chunk" type is
CHUNK_TYPE_RAW, then the data in the current 'c' (the "chunk" data)
needs to be written into the flash as is, using the pointer to the
first byte of this 'c' as the starting address for the blk_dwrite().
Because of the 28 byte image header, and the 12 byte "chunk"
header(s), it is extremely unlikely that this starting address will
be aligned - resulting in many "CACHE: Misaligned operation at range
[, ]" messages while these 13093 "chunks" are processed. In
the code that I am testing, this message is being generated by the
call to flush_cache(start_addr, trans_bytes) in the
sdhci_send_command() function.


You can set this up such that the [a] part starts at offset +24 bytes, 
then the [c] will be aligned.



Copying each "chuck data" to an aligned buffer before calling
blk_dwrite() in order to avoid this message would be highly
inefficient -- the desire is to download the code and flash it as
fast as possible!


Fast is great, but not if this makes the transfer unreliable, which will 
happen if you cannot reliably flush/invalidate cache and you decide to 
ignore this clear warning you're getting.



Therefore, I propose this patch which provides a runtime flag to
suppress this warning message, which is used when processing these
fastboot "sparse" images.


This is wrong.


arch/arm/include/asm/cache.h |  2 ++ arch/arm/lib/cache.c |
15 ++- common/fb_mmc.c  |  8 +++- 3 files
changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/cache.h
b/arch/arm/include/asm/cache.h index 5400cbe..1895b8a 100644 ---
a/arch/arm/include/asm/cache.h +++ b/arch/arm/include/asm/cache.h @@
-29,6 +29,8 @@ static inline void invalidate_l2_cache(void) } #endif

+void set_suppress_warning_flag(void); +void
clr_suppress_warning_flag(void); int check_cache_range(unsigned long
start, unsigned long stop);

void l2_cache_enable(void); diff --git a/arch/arm/lib/cache.c
b/arch/arm/lib/cache.c index 4f72f89..174a167 100644 ---
a/arch/arm/lib/cache.c +++ b/arch/arm/lib/cache.c @@ -46,6 +46,19 @@
__weak void flush_dcache_range(unsigned long start, unsigned long
stop) /* An empty stub, real implementation should be in platform
code */ }

+/* suppress "cache misaligned" warning message */ +static int
suppress_warning; + +void set_suppress_warning_flag(void) +{ +
suppress_warning = 1; +} + +void clr_suppress_warning_flag(void) +{ +
suppress_warning = 0; +} + int check_cache_range(unsigned long start,
unsigned long stop) { int ok = 1; @@ -56,7 +69,7 @@ int
check_cache_range(unsigned long start, unsigned long stop) if (stop &
(CONFIG_SYS_CACHELINE_SIZE - 1)) ok = 0;

-   if (!ok) { +if (!ok && !suppress_warning) { warn_non_spl("CACHE:
Misaligned operation at range [%08lx, %08lx]\n", start, stop); } diff
--git a/common/fb_mmc.c b/common/fb_mmc.c index 6cc113d..c8b1e17
100644 --- a/common/fb_mmc.c +++ b/common/fb_mmc.c @@ -58,8 +58,14 @@
static lbaint_t fb_mmc_sparse_write(struct sparse_storage *info, {
struct fb_mmc_sparse *sparse = info->priv; struct blk_desc *dev_desc
= sparse->dev_desc; +lbaint_t blks; + +  /* suppress "cache
misaligned" warning message during this write */ +
set_suppress_warning_flag(); +  blks = blk_dwrite(dev_desc, blk,
blkcnt, buffer); +  clr_suppress_warning_flag();

-   return blk_dwrite(dev_desc, blk, blkcnt, buffer); + return blks; }

static lbaint_t fb_mmc_sparse_reserve(struct sparse_storage *info,



___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [ PATCH] ARM: cache: runtime flag to suppress the "cache misaligned" message

[Steve Rae]

The "chunks" in the "fastboot sparse image" are not aligned, resulting
in many "cached misaligned" messages from check_cache_range().
Implement a runtime flag to suppress this message, and use this flag
when processing the "fastboot sparse image".

Signed-off-by: Steve Rae 
Reported-by: Gary Bisson 

---
A little background:
In the code that I am testing, the "sparse" image is 350,723,024 bytes and is 
downloaded into an aligned buffer on the device.
This image/buffer contains a header ('a') and 13903 "chunks" (b1/c1 through 
bx/cx):
+---+++++ +++
| a | b1 | c1 | b2 | c2 | ... | bx | cx |
+---+++++ +++
where:
  a  = the "sparse image" header (28 bytes)
  b* = the "chunk" header(12 bytes)
  c* = the "chunk" data  (which is always an exact multiple of 
CONFIG_SYS_CACHELINE_SIZE)
Whenever the "chunk" type is CHUNK_TYPE_RAW, then the data in the current 'c' 
(the "chunk" data) needs to be written into the flash as is, using the pointer 
to the first byte of this 'c' as the starting address for the blk_dwrite(). 
Because of the 28 byte image header, and the 12 byte "chunk" header(s), it is 
extremely unlikely that this starting address will be aligned - resulting in 
many "CACHE: Misaligned operation at range [, ]" messages while these 
13093 "chunks" are processed.
In the code that I am testing, this message is being generated by the call to
  flush_cache(start_addr, trans_bytes)
in the sdhci_send_command() function.

Copying each "chuck data" to an aligned buffer before calling blk_dwrite() in 
order to avoid this message would be highly inefficient -- the desire is to 
download the code and flash it as fast as possible!

Therefore, I propose this patch which provides a runtime flag to suppress this 
warning message, which is used when processing these fastboot "sparse" images.

 arch/arm/include/asm/cache.h |  2 ++
 arch/arm/lib/cache.c | 15 ++-
 common/fb_mmc.c  |  8 +++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/cache.h b/arch/arm/include/asm/cache.h
index 5400cbe..1895b8a 100644
--- a/arch/arm/include/asm/cache.h
+++ b/arch/arm/include/asm/cache.h
@@ -29,6 +29,8 @@ static inline void invalidate_l2_cache(void)
 }
 #endif
 
+void set_suppress_warning_flag(void);
+void clr_suppress_warning_flag(void);
 int check_cache_range(unsigned long start, unsigned long stop);
 
 void l2_cache_enable(void);
diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index 4f72f89..174a167 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -46,6 +46,19 @@ __weak void flush_dcache_range(unsigned long start, unsigned 
long stop)
/* An empty stub, real implementation should be in platform code */
 }
 
+/* suppress "cache misaligned" warning message */
+static int suppress_warning;
+
+void set_suppress_warning_flag(void)
+{
+   suppress_warning = 1;
+}
+
+void clr_suppress_warning_flag(void)
+{
+   suppress_warning = 0;
+}
+
 int check_cache_range(unsigned long start, unsigned long stop)
 {
int ok = 1;
@@ -56,7 +69,7 @@ int check_cache_range(unsigned long start, unsigned long stop)
if (stop & (CONFIG_SYS_CACHELINE_SIZE - 1))
ok = 0;
 
-   if (!ok) {
+   if (!ok && !suppress_warning) {
warn_non_spl("CACHE: Misaligned operation at range [%08lx, 
%08lx]\n",
 start, stop);
}
diff --git a/common/fb_mmc.c b/common/fb_mmc.c
index 6cc113d..c8b1e17 100644
--- a/common/fb_mmc.c
+++ b/common/fb_mmc.c
@@ -58,8 +58,14 @@ static lbaint_t fb_mmc_sparse_write(struct sparse_storage 
*info,
 {
struct fb_mmc_sparse *sparse = info->priv;
struct blk_desc *dev_desc = sparse->dev_desc;
+   lbaint_t blks;
+
+   /* suppress "cache misaligned" warning message during this write */
+   set_suppress_warning_flag();
+   blks = blk_dwrite(dev_desc, blk, blkcnt, buffer);
+   clr_suppress_warning_flag();
 
-   return blk_dwrite(dev_desc, blk, blkcnt, buffer);
+   return blks;
 }
 
 static lbaint_t fb_mmc_sparse_reserve(struct sparse_storage *info,
-- 
2.7.4

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot