Patchwork [U-Boot,v3] command/cache: Add flush command

login
register
mail settings
Submitter York Sun
Date April 5, 2013, 8:50 p.m.
Message ID <1365195056-20188-1-git-send-email-yorksun@freescale.com>
Download mbox | patch
Permalink /patch/234285/
State New
Delegated to: Tom Rini
Headers show

Comments

York Sun - April 5, 2013, 8:50 p.m.
When we copy code/data to the main memory, we may need to flush the
cache if required by architecture. It uses the existing function
flush_cache. Syntax is

flush <addr> <size>

The addr and size are given in hexadecimal. Like memory command, there is
no sanity check for the parameters.

Signed-off-by: York Sun <yorksun@freescale.com>
---
Change since v2: rename flush_cache to flush

 README             |    5 ++++-
 common/cmd_cache.c |   26 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
Tom Rini - April 5, 2013, 9 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/05/2013 04:50 PM, York Sun wrote:
> When we copy code/data to the main memory, we may need to flush
> the cache if required by architecture. It uses the existing
> function flush_cache. Syntax is
> 
> flush <addr> <size>
> 
> The addr and size are given in hexadecimal. Like memory command,
> there is no sanity check for the parameters.
> 
> Signed-off-by: York Sun <yorksun@freescale.com> --- Change since
> v2: rename flush_cache to flush

Wait, why?  flush_cache was clear as to what we were doing, flush less
so.  At least that's my first reaction.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRXzttAAoJENk4IS6UOR1W3B8QAJAM49Zwv7WP4s6B+77U88wd
sFqSoJIJM8djB/BllBYLZUKRc+o7ZfvA35VTzlaok3iqVec5v0Lk25ylDwebHr5f
Ucj2p/WV8wXlNDYq+6mNbHPQ5qiRihQxpwKHv3Dk9B5wtG18rxIxRvYVq/lqAsMp
AWzuFNyFqRmFIXAly5QcBgJLuFs9FcwL0mKQUuwGHlgkyrswnSbMuQiXwpvO9YkX
cy8zC4UhIgtGu+SQdUB5/CFB8Q7oZW3yXXeWRlMUdq7vEzsWIjslnz6qhIihGGcW
WnIiN7f9PY3q1UKFBZg2s+/xsUOQmxeVDbYqOi0/IuarinusWgKKcuyKfM6KgZGz
6kJ4wOSUg1xzSYB3brRRT8VchT2hCIJ9Iemm+M2kS6TP4LljZk/p93HrugPwtLXW
gJbBC8TquNY8xst0fWHgREcVS0M0QAmqy2xtYAx7L6yg8aZAVBzvZi+UOV5oN5k9
jIK/scnU+mTiNkBKAfwQoTIJ8XrBQWZ0cGYsZW694U5a45FnVFqFFcqWxHuRG7XV
Q3ZsySoC459g31sS5jHorYSBID6RQoebsXexxze3BmfNHZM5HN99y/IssBExkivG
KOMQUHrwFKOJUy2JAdZStBm02/P4uSm6AzTS9UYKdp09ze4IiqzbXh0WedvNsJui
WHBXfW68ts4nyTBhGkA3
=6vnO
-----END PGP SIGNATURE-----
York Sun - April 5, 2013, 9:09 p.m.
On 04/05/2013 02:00 PM, Tom Rini wrote:
> On 04/05/2013 04:50 PM, York Sun wrote:
>> When we copy code/data to the main memory, we may need to flush
>> the cache if required by architecture. It uses the existing
>> function flush_cache. Syntax is
> 
>> flush <addr> <size>
> 
>> The addr and size are given in hexadecimal. Like memory command,
>> there is no sanity check for the parameters.
> 
>> Signed-off-by: York Sun <yorksun@freescale.com> --- Change since
>> v2: rename flush_cache to flush
> 
> Wait, why?  flush_cache was clear as to what we were doing, flush less
> so.  At least that's my first reaction.
> 

Scott suggested the underscore is not recommended for the command name.
Shall I change it to cacheflush or flushcache?

York
Wolfgang Denk - April 5, 2013, 10:09 p.m.
Dear York Sun,

In message <1365195056-20188-1-git-send-email-yorksun@freescale.com> you wrote:
> When we copy code/data to the main memory, we may need to flush the
> cache if required by architecture. It uses the existing function
> flush_cache. Syntax is
> 
> flush <addr> <size>

Plain "flush" is way too generic a name.  I think we should make it
clear from the command invocation that we are dealing with caches
here.

Actually I think we should not even use a new command for this - we
already have the "dcahe" and "icache" commands for this purpose.

What do you think about implementiung this as a subcommand to
"dcache"?  Something like:

So far:

	dcache on
	dcache off

adding new:

	dcache flush			=> flush all
	dcache flush <addr> <size>	=> flush range

I think this makes more sense.  Comments?

Best regards,

Wolfgang Denk
York Sun - April 5, 2013, 11:02 p.m.
On 04/05/2013 03:09 PM, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1365195056-20188-1-git-send-email-yorksun@freescale.com> you wrote:
>> When we copy code/data to the main memory, we may need to flush the
>> cache if required by architecture. It uses the existing function
>> flush_cache. Syntax is
>>
>> flush <addr> <size>
> 
> Plain "flush" is way too generic a name.  I think we should make it
> clear from the command invocation that we are dealing with caches
> here.
> 
> Actually I think we should not even use a new command for this - we
> already have the "dcahe" and "icache" commands for this purpose.
> 
> What do you think about implementiung this as a subcommand to
> "dcache"?  Something like:
> 
> So far:
> 
> 	dcache on
> 	dcache off
> 
> adding new:
> 
> 	dcache flush			=> flush all
> 	dcache flush <addr> <size>	=> flush range
> 
> I think this makes more sense.  Comments?
> 
>

It would if the command only deals with dcache. This command flushes
dcache _and_ invalidates icache.

If "flush_cache" is acceptable, we can use v2. If not, please suggest
one. My candidates are "flushcache", "cacheflush".

York
Wolfgang Denk - April 6, 2013, 7:01 a.m.
Dear York Sun,

In message <515F5812.8030008@freescale.com> you wrote:
>
> > adding new:
> > 
> > 	dcache flush			=> flush all
> > 	dcache flush <addr> <size>	=> flush range
> > 
> > I think this makes more sense.  Comments?
> 
> It would if the command only deals with dcache. This command flushes
> dcache _and_ invalidates icache.

Then the name "flush" is even more a bad choice.

> If "flush_cache" is acceptable, we can use v2. If not, please suggest
> one. My candidates are "flushcache", "cacheflush".

Can we not split this into:

	dcache flush
	icache invalidate

?  This would make clear what's happening.

Best regards,

Wolfgang Denk
sun york-R58495 - April 7, 2013, 3:31 a.m.
On Apr 6, 2013, at 12:01 AM, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <515F5812.8030008@freescale.com> you wrote:
>> 
>>> adding new:
>>> 
>>> 	dcache flush			=> flush all
>>> 	dcache flush <addr> <size>	=> flush range
>>> 
>>> I think this makes more sense.  Comments?
>> 
>> It would if the command only deals with dcache. This command flushes
>> dcache _and_ invalidates icache.
> 
> Then the name "flush" is even more a bad choice.
> 
>> If "flush_cache" is acceptable, we can use v2. If not, please suggest
>> one. My candidates are "flushcache", "cacheflush".
> 
> Can we not split this into:
> 
> 	dcache flush
> 	icache invalidate
> 
> ?  This would make clear what's happening.


The idea is to reuse existing code with minimum addition. For the applications concerned, these two steps are both needed. Splitting them doesn't make things easier.
If I have to use existing command, I'd rather to put these two steps under icache invalide <addr> <size>.

York
Wolfgang Denk - April 7, 2013, 8:29 a.m.
Dear sun york-R58495,

In message <C707E9F4D8007146BF8DC1424B113AC70B3A435B@039-SN2MPN1-012.039d.mgd.msft.net> you wrote:
> 
> > Can we not split this into:
> > 
> > 	dcache flush
> > 	icache invalidate
> > 
> > ?  This would make clear what's happening.
> 
> 
> The idea is to reuse existing code with minimum addition. For the applicati
> ons concerned, these two steps are both needed. Splitting them doesn't make
>  things easier.

Reusing code is a Good Thing, but not when it comes at the cost of
obfucating what the code actually does.

> If I have to use existing command, I'd rather to put these two steps under
> icache invalide <addr> <size>.

No, this is not acceptable.  The "icache" command deals with the IC
only, it must not meddle with the data cache (like flushing it).

Best regards,

Wolfgang Denk
sun york-R58495 - April 8, 2013, 5:45 p.m.
On Apr 7, 2013, at 1:29 AM, Wolfgang Denk wrote:

> Dear sun york-R58495,
> 
> In message <C707E9F4D8007146BF8DC1424B113AC70B3A435B@039-SN2MPN1-012.039d.mgd.msft.net> you wrote:
>> 
>>> Can we not split this into:
>>> 
>>> 	dcache flush
>>> 	icache invalidate
>>> 
>>> ?  This would make clear what's happening.
>> 
>> 
>> The idea is to reuse existing code with minimum addition. For the applicati
>> ons concerned, these two steps are both needed. Splitting them doesn't make
>> things easier.
> 
> Reusing code is a Good Thing, but not when it comes at the cost of
> obfucating what the code actually does.
> 
>> If I have to use existing command, I'd rather to put these two steps under
>> icache invalide <addr> <size>.
> 
> No, this is not acceptable.  The "icache" command deals with the IC
> only, it must not meddle with the data cache (like flushing it).
> 

I think it is best to keep this patch as it and stick with the original flush_cache name. It uses the existing function flush_cache() which is available for most (if not all) architectures. Splitting the dcache and icache not only adds more code, but architecture-dependent code.

York
Wolfgang Denk - April 8, 2013, 6:35 p.m.
Dear sun york-R58495,

In message <C707E9F4D8007146BF8DC1424B113AC70B3A6A83@039-SN2MPN1-012.039d.mgd.msft.net> you wrote:
> 
> I think it is best to keep this patch as it and stick with the
> original flush_cache name. It uses the existing function
> flush_cache() which is available for most (if not all) architectures.
> Splitting the dcache and icache not only adds more code, but
> architecture-dependent code.

As mentioned before: reusing existing code is fine, but we already
have commands for cache handling, and adding arbitrary new ones to
implement combinations of functions makes does not scale.  Assume
tomorrow someone needs to add more ganular handling for example
regarding L2 caches, etc.  Would you suggest to add another set of new
commands, then?  This makes no sense.

Please implement IC related operations as subcommands to the "icache"
command, and DC releated ones as subcommands to "dcache".

[In the example of L2 cache above, it would be for example sufficient
to add a "-L2" option to the "icache" / "dcache" commands.]

Best regards,

Wolfgang Denk
Scott Wood - April 8, 2013, 7:50 p.m.
On 04/07/2013 03:29:31 AM, Wolfgang Denk wrote:
> Dear sun york-R58495,
> 
> In message  
> <C707E9F4D8007146BF8DC1424B113AC70B3A435B@039-SN2MPN1-012.039d.mgd.msft.net>  
> you wrote:
> >
> > > Can we not split this into:
> > >
> > > 	dcache flush
> > > 	icache invalidate
> > >
> > > ?  This would make clear what's happening.
> >
> >
> > The idea is to reuse existing code with minimum addition. For the  
> applicati
> > ons concerned, these two steps are both needed. Splitting them  
> doesn't make
> >  things easier.
> 
> Reusing code is a Good Thing, but not when it comes at the cost of
> obfucating what the code actually does.
> 
> > If I have to use existing command, I'd rather to put these two  
> steps under
> > icache invalide <addr> <size>.
> 
> No, this is not acceptable.  The "icache" command deals with the IC
> only, it must not meddle with the data cache (like flushing it).

I thought you said it was OK to flush more than the user asked for, if  
the implementation does not have separate icache/dcache flushes?  Why  
is it fundamentally different if it's a hardware limitation, or a  
limitation of the software layer whose functionality is being exposed?

-Scott
Wolfgang Denk - April 9, 2013, 5:48 p.m.
Dear Scott Wood,

In message <1365450620.28843.12@snotra> you wrote:
> >
> I thought you said it was OK to flush more than the user asked for, if  
> the implementation does not have separate icache/dcache flushes?  Why  
> is it fundamentally different if it's a hardware limitation, or a  
> limitation of the software layer whose functionality is being exposed?

I don't get what you are trying to prove.  Can you please point me to
the code (ideally in mainline U-Boot) which would cause problems with
the suggested separation of invalidating the IC and flushing the DC
into subcommands of the "icache" resp. "dcache" commands?

Best regards,

Wolfgang Denk

Patch

diff --git a/README b/README
index dc6b0b3..b2f80d6 100644
--- a/README
+++ b/README
@@ -809,6 +809,7 @@  The following options need to be configured:
 		CONFIG_CMD_BSP		* Board specific commands
 		CONFIG_CMD_BOOTD	  bootd
 		CONFIG_CMD_CACHE	* icache, dcache
+		CONFIG_CMD_CACHE_FLUSH	* flush cache by the address and range
 		CONFIG_CMD_CONSOLE	  coninfo
 		CONFIG_CMD_CRC32	* crc32
 		CONFIG_CMD_DATE		* support for RTC, date/time...
@@ -912,7 +913,9 @@  The following options need to be configured:
 		8260 (where accesses to the IMMR region must be
 		uncached), and it cannot be disabled on all other
 		systems where we (mis-) use the data cache to hold an
-		initial stack and some data.
+		initial stack and some data. The CONFIG_CMD_CACHE_FLUSH
+		macro enables flushing cache by the address and range to
+		maintain coherency if required by architecture.
 
 
 		XXX - this list needs to get updated!
diff --git a/common/cmd_cache.c b/common/cmd_cache.c
index 5512f92..c228e0c 100644
--- a/common/cmd_cache.c
+++ b/common/cmd_cache.c
@@ -120,3 +120,29 @@  U_BOOT_CMD(
 	"[on, off, flush]\n"
 	"    - enable, disable, or flush data (writethrough) cache"
 );
+
+#ifdef CONFIG_CMD_CACHE_FLUSH
+int do_flush_cache(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	ulong addr, size;
+
+	switch (argc) {
+	case 3:
+		addr = simple_strtoul(argv[1], NULL, 16);
+		size = simple_strtoul(argv[2], NULL, 16);
+		flush_cache(addr, size);
+		break;
+	default:
+		return cmd_usage(cmdtp);
+	}
+	return 0;
+
+}
+
+U_BOOT_CMD(
+	flush,   3,   0,     do_flush_cache,
+	"flush cache for a range",
+	"<addr> <size>\n"
+	"    - flush cache for specificed range"
+);
+#endif