| Message ID | 1308851448-25139-2-git-send-email-dave.martin@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jun 23, 2011 at 06:50:09PM +0100, Dave Martin wrote: > +.macro define_cache_functions name:req, default:req, \ > + flush_kern_dcache_area, dma_map_area, dma_flush_range > + .align 2 > + .type \name\()_cache_fns, #object > +ENTRY(\name\()_cache_fns) > + .long \default\()_flush_icache_all > + .long \default\()_flush_kern_cache_all > + .long \default\()_flush_user_cache_all > + .long \default\()_flush_user_cache_range > + .long \default\()_coherent_kern_range > + .long \default\()_coherent_user_range > + .ifb \flush_kern_dcache_area > + .long \default\()_flush_kern_dcache_area > + .else > + .long \flush_kern_dcache_area > + .endif > + .ifb \dma_map_area > + .long \default\()_dma_map_area > + .else > + .long \dma_map_area > + .endif > + .long \default\()_dma_unmap_area > + .ifb \dma_flush_range > + .long \default\()_dma_flush_range > + .else > + .long \dma_flush_range > + .endif > + .size \name\()_cache_fns, . - \name\()_cache_fns > +.endm This looks wrong. The flush_kern_dcache_area and DMA stuff can't be any different: #define __cpuc_flush_icache_all __glue(_CACHE,_flush_icache_all) #define __cpuc_flush_kern_all __glue(_CACHE,_flush_kern_cache_all) #define __cpuc_flush_user_all __glue(_CACHE,_flush_user_cache_all) #define __cpuc_flush_user_range __glue(_CACHE,_flush_user_cache_range) #define __cpuc_coherent_kern_range __glue(_CACHE,_coherent_kern_range) #define __cpuc_coherent_user_range __glue(_CACHE,_coherent_user_range) #define __cpuc_flush_dcache_area __glue(_CACHE,_flush_kern_dcache_area) #define dmac_map_area __glue(_CACHE,_dma_map_area) #define dmac_unmap_area __glue(_CACHE,_dma_unmap_area) #define dmac_flush_range __glue(_CACHE,_dma_flush_range) If they don't conform to the "<cachename>_<functionname>" pattern then single-cache model kernels will not compile. As these macros are supposed to be about correctness, they should be enforcing that too, and not allowing certain functions to be overriden by different names.
Russell, On Thu, Jul 07, 2011 at 09:22:51AM +0100, Russell King - ARM Linux wrote: > On Thu, Jun 23, 2011 at 06:50:09PM +0100, Dave Martin wrote: > > +.macro define_cache_functions name:req, default:req, \ > > + flush_kern_dcache_area, dma_map_area, dma_flush_range > > + .align 2 > > + .type \name\()_cache_fns, #object > > +ENTRY(\name\()_cache_fns) > > + .long \default\()_flush_icache_all > > + .long \default\()_flush_kern_cache_all > > + .long \default\()_flush_user_cache_all > > + .long \default\()_flush_user_cache_range > > + .long \default\()_coherent_kern_range > > + .long \default\()_coherent_user_range > > + .ifb \flush_kern_dcache_area > > + .long \default\()_flush_kern_dcache_area > > + .else > > + .long \flush_kern_dcache_area > > + .endif > > + .ifb \dma_map_area > > + .long \default\()_dma_map_area > > + .else > > + .long \dma_map_area > > + .endif > > + .long \default\()_dma_unmap_area > > + .ifb \dma_flush_range > > + .long \default\()_dma_flush_range > > + .else > > + .long \dma_flush_range > > + .endif > > + .size \name\()_cache_fns, . - \name\()_cache_fns > > +.endm > > This looks wrong. The flush_kern_dcache_area and DMA stuff can't be > any different: [...] Would you be happy to pull from my tree if Dave fixes these issues this week, or would you rather I drop this series for the coming merge window? Cheers, Will
On Thu, Jul 07, 2011 at 09:22:51AM +0100, Russell King - ARM Linux wrote: > On Thu, Jun 23, 2011 at 06:50:09PM +0100, Dave Martin wrote: > > +.macro define_cache_functions name:req, default:req, \ > > + flush_kern_dcache_area, dma_map_area, dma_flush_range > > + .align 2 > > + .type \name\()_cache_fns, #object > > +ENTRY(\name\()_cache_fns) > > + .long \default\()_flush_icache_all > > + .long \default\()_flush_kern_cache_all > > + .long \default\()_flush_user_cache_all > > + .long \default\()_flush_user_cache_range > > + .long \default\()_coherent_kern_range > > + .long \default\()_coherent_user_range > > + .ifb \flush_kern_dcache_area > > + .long \default\()_flush_kern_dcache_area > > + .else > > + .long \flush_kern_dcache_area > > + .endif > > + .ifb \dma_map_area > > + .long \default\()_dma_map_area > > + .else > > + .long \dma_map_area > > + .endif > > + .long \default\()_dma_unmap_area > > + .ifb \dma_flush_range > > + .long \default\()_dma_flush_range > > + .else > > + .long \dma_flush_range > > + .endif > > + .size \name\()_cache_fns, . - \name\()_cache_fns > > +.endm > > This looks wrong. The flush_kern_dcache_area and DMA stuff can't be > any different: > > #define __cpuc_flush_icache_all __glue(_CACHE,_flush_icache_all) > #define __cpuc_flush_kern_all __glue(_CACHE,_flush_kern_cache_all) > #define __cpuc_flush_user_all __glue(_CACHE,_flush_user_cache_all) > #define __cpuc_flush_user_range __glue(_CACHE,_flush_user_cache_range) > #define __cpuc_coherent_kern_range __glue(_CACHE,_coherent_kern_range) > #define __cpuc_coherent_user_range __glue(_CACHE,_coherent_user_range) > #define __cpuc_flush_dcache_area __glue(_CACHE,_flush_kern_dcache_area) > > #define dmac_map_area __glue(_CACHE,_dma_map_area) > #define dmac_unmap_area __glue(_CACHE,_dma_unmap_area) > #define dmac_flush_range __glue(_CACHE,_dma_flush_range) > > If they don't conform to the "<cachename>_<functionname>" pattern then > single-cache model kernels will not compile. I added this override capability purely because some proc-*.S define multiple sets of cache functions which are mostly common but have some differences. This facility is actually just used by feroceon and xscale, so you're probably right -- it may be better to fix those than encourage deviation elsewhere. If this makes any CPU broken, it must by definition be broken already since I was very careful to ensure there was no change to symbols or generated code as a result of these patches. IIUC, these processors may already not work for a single cache model kernel. In asm/glue-cache.h, this seems to be acknowledged for feroceon (which defines MULTI_CACHE unconditionally if CONFIG_CPU_FEROCEON is defined). For xscale, there is no unconditional MULTI_CACHE, so it looks as though if a single cache model kernel is built, xscale_dma_map_area may get used unconditionally instead of xscale_dma_a0_map_area even for the buggy A0/A1 processor revisions. That won't result in a compile failure, just some incorrect cache maintenance at runtime -- if so, it looks like a bug. > As these macros are supposed to be about correctness, they should be > enforcing that too, and not allowing certain functions to be overriden > by different names. I guess I agree with that -- making it too easy to override individual functions may encourage people to do the wrong thing. So yes, taking away that generic override capability the forcing people to define appropriate aliases in the proc-*.S files sounds like a better approach. However, we also have the problem that any kenel containing multiple sets of cache fns needs to have MULTI_CACHE defined; otherwise, a faulty kernel may be built silently. I think this only applies to xscale right now, if I've understood correctly. The easiest solution to that particular problem would be to define MULTI_CACHE for xscale unconditionally, as is done for feroceon. Do you think we need a generic check for such situations? Cheers ---Dave
On Thu, Jul 07, 2011 at 11:07:15AM +0100, Dave Martin wrote: > On Thu, Jul 07, 2011 at 09:22:51AM +0100, Russell King - ARM Linux wrote: > > If they don't conform to the "<cachename>_<functionname>" pattern then > > single-cache model kernels will not compile. > > I added this override capability purely because some proc-*.S define multiple > sets of cache functions which are mostly common but have some differences. > This facility is actually just used by feroceon and xscale, so you're probably > right -- it may be better to fix those than encourage deviation elsewhere. I think so. > For xscale, there is no unconditional MULTI_CACHE, so it looks as though if > a single cache model kernel is built, xscale_dma_map_area may get used > unconditionally instead of xscale_dma_a0_map_area even for the buggy A0/A1 > processor revisions. That won't result in a compile failure, just some > incorrect cache maintenance at runtime -- if so, it looks like a bug. That sounds like there needs to be a separate patch preceding this series which sets MULTI_CACHE unconditionally for Xscale. > Do you think we need a generic check for such situations? No, I think that would be incredibly hard, and for two processors out of the number we already support, I don't think its worth it.
On Thu, Jul 07, 2011 at 11:16:43AM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 07, 2011 at 11:07:15AM +0100, Dave Martin wrote: > > On Thu, Jul 07, 2011 at 09:22:51AM +0100, Russell King - ARM Linux wrote: > > > If they don't conform to the "<cachename>_<functionname>" pattern then > > > single-cache model kernels will not compile. > > > > I added this override capability purely because some proc-*.S define multiple > > sets of cache functions which are mostly common but have some differences. > > This facility is actually just used by feroceon and xscale, so you're probably > > right -- it may be better to fix those than encourage deviation elsewhere. > > I think so. > > > For xscale, there is no unconditional MULTI_CACHE, so it looks as though if > > a single cache model kernel is built, xscale_dma_map_area may get used > > unconditionally instead of xscale_dma_a0_map_area even for the buggy A0/A1 > > processor revisions. That won't result in a compile failure, just some > > incorrect cache maintenance at runtime -- if so, it looks like a bug. > > That sounds like there needs to be a separate patch preceding this series > which sets MULTI_CACHE unconditionally for Xscale. OK, I'll submit a separate patch for that. It's an orthogonal issue really. > > Do you think we need a generic check for such situations? > > No, I think that would be incredibly hard, and for two processors out of > the number we already support, I don't think its worth it. OK -- that was my view too. I'll rework the series and repost. The changes shouldn't be invasive. Cheers ---Dave
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S index 34261f9..fa28d6b 100644 --- a/arch/arm/mm/proc-macros.S +++ b/arch/arm/mm/proc-macros.S @@ -254,3 +254,79 @@ mcr p15, 0, r0, c7, c10, 1 @ clean L1 D line mcr p15, 0, ip, c7, c10, 4 @ data write barrier .endm + +.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0 + .type \name\()_processor_functions, #object + .align 2 +ENTRY(\name\()_processor_functions) + .word \dabort + .word \pabort + .word cpu_\name\()_proc_init + .word cpu_\name\()_proc_fin + .word cpu_\name\()_reset + .word cpu_\name\()_do_idle + .word cpu_\name\()_dcache_clean_area + .word cpu_\name\()_switch_mm + + .if \nommu + .word 0 + .else + .word cpu_\name\()_set_pte_ext + .endif + + .if \suspend + .word cpu_\name\()_suspend_size + .word cpu_\name\()_do_suspend + .word cpu_\name\()_do_resume + .else + .word 0 + .word 0 + .word 0 + .endif + + .size \name\()_processor_functions, . - \name\()_processor_functions +.endm + +.macro define_cache_functions name:req, default:req, \ + flush_kern_dcache_area, dma_map_area, dma_flush_range + .align 2 + .type \name\()_cache_fns, #object +ENTRY(\name\()_cache_fns) + .long \default\()_flush_icache_all + .long \default\()_flush_kern_cache_all + .long \default\()_flush_user_cache_all + .long \default\()_flush_user_cache_range + .long \default\()_coherent_kern_range + .long \default\()_coherent_user_range + .ifb \flush_kern_dcache_area + .long \default\()_flush_kern_dcache_area + .else + .long \flush_kern_dcache_area + .endif + .ifb \dma_map_area + .long \default\()_dma_map_area + .else + .long \dma_map_area + .endif + .long \default\()_dma_unmap_area + .ifb \dma_flush_range + .long \default\()_dma_flush_range + .else + .long \dma_flush_range + .endif + .size \name\()_cache_fns, . - \name\()_cache_fns +.endm + +.macro define_tlb_functions name:req, flags_up:req, flags_smp + .type \name\()_tlb_fns, #object +ENTRY(\name\()_tlb_fns) + .long \name\()_flush_user_tlb_range + .long \name\()_flush_kern_tlb_range + .ifnb \flags_smp + ALT_SMP(.long \flags_smp ) + ALT_UP(.long \flags_up ) + .else + .long \flags_up + .endif + .size \name\()_tlb_fns, . - \name\()_tlb_fns +.endm
This patch adds some generic macros to reduce boilerplate when declaring certain common structures in arch/arm/mm/*.S Thanks to Russell King for outlining what the define_processor_functions macro could look like. Currently, only a few things can be overriden when invoking the macros, based on the overrides actually required by the existing CPUs. More overrides arguments can be added in the future, if needed. Signed-off-by: Dave Martin <dave.martin@linaro.org> --- arch/arm/mm/proc-macros.S | 76 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 76 insertions(+), 0 deletions(-)