| Message ID | 1365195056-20188-1-git-send-email-yorksun@freescale.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
-----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-----
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
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
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
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
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
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
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
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
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
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
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
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(-)