From: Vitaly W. <vw...@ru...> - 2006-10-26 06:43:29
|
Hello folks, we've observed the following build failure compiling the kernel w/ oprofile on 8540 (FSL_BOOKE): CC arch/powerpc/oprofile/op_model_7450.o arch/powerpc/oprofile/op_model_7450.c:62: error: conflicting types for 'pmc_start_ctrs' include/asm/pmc.h:41: error: previous declaration of 'pmc_start_ctrs' was here arch/powerpc/oprofile/op_model_7450.c:62: error: conflicting types for 'pmc_start_ctrs' include/asm/pmc.h:41: error: previous declaration of 'pmc_start_ctrs' was here arch/powerpc/oprofile/op_model_7450.c:73: warning: static declaration of 'pmc_stop_ctrs' follows non-static declaration include/asm/pmc.h:42: warning: previous declaration of 'pmc_stop_ctrs' was here arch/powerpc/oprofile/op_model_7450.c: In function `fsl7450_start': arch/powerpc/oprofile/op_model_7450.c:140: warning: implicit declaration of function `ctr_write' arch/powerpc/oprofile/op_model_7450.c: In function `fsl7450_handle_interrupt': arch/powerpc/oprofile/op_model_7450.c:182: warning: implicit declaration of function `ctr_read' make[1]: *** [arch/powerpc/oprofile/op_model_7450.o] Error 1 The thing is that op_model_7450.c is compiled when CONFIG_PPC32 is true which might be reasonable but results in conflicting functions' types when compiled w/ CONFIG_FSL_BOOKE=y. Effectively, op_model_fsl_booke.c introduces the functions with the same names as op_model_7450.c but with different declarations, and latter ones are static while former ones are not, and this can drive any compiler mad. This message will have two *alternative* follow-up patches which are probably more likely RFCs rather than complete solutions, but still I'm sure it's reasonable to look at those, b/c those represent two different approaches and we need to decide which way to go first. Vitaly |
From: Vitaly W. <vw...@ru...> - 2006-10-26 06:43:35
|
Below is the patch for the problem described in "[0/2] build failure for 8540 w/ CONFIG_OPROFLE=y" letter representing the first approach. This approach is based on (pretty reasonable) assumption that op_model_7450.c is applicable only to 6XX, so let's compile it only when CONFIG_6XX==y. This results in more #ifdef's in arch/powerpc/oprofile/common.c, which doesn't look good to me. I'd rather "switch (cur_cpu_spec->oprofile_type) ..." to SoC-dependent header files arranging that one as static inline func... Anyway, here's the patch. arch/powerpc/oprofile/Makefile | 2 +- arch/powerpc/oprofile/common.c | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) Signed-off-by: Vitaly Wool <vw...@ru...> Index: linux-2.6.18/arch/powerpc/oprofile/Makefile =================================================================== --- linux-2.6.18.orig/arch/powerpc/oprofile/Makefile +++ linux-2.6.18/arch/powerpc/oprofile/Makefile @@ -13,4 +13,4 @@ DRIVER_OBJS := $(addprefix ../../../driv oprofile-y := $(DRIVER_OBJS) common.o backtrace.o oprofile-$(CONFIG_PPC64) += op_model_rs64.o op_model_power4.o oprofile-$(CONFIG_FSL_BOOKE) += op_model_fsl_booke.o -oprofile-$(CONFIG_PPC32) += op_model_7450.o +oprofile-$(CONFIG_6XX) += op_model_7450.o Index: linux-2.6.18/arch/powerpc/oprofile/common.c =================================================================== --- linux-2.6.18.orig/arch/powerpc/oprofile/common.c +++ linux-2.6.18/arch/powerpc/oprofile/common.c @@ -135,19 +135,18 @@ int __init oprofile_arch_init(struct opr return -ENODEV; switch (cur_cpu_spec->oprofile_type) { -#ifdef CONFIG_PPC64 +#if defined(CONFIG_PPC64) case PPC_OPROFILE_RS64: model = &op_model_rs64; break; case PPC_OPROFILE_POWER4: model = &op_model_power4; break; -#else +#elif defined (CONFIG_6XX) case PPC_OPROFILE_G4: model = &op_model_7450; break; -#endif -#ifdef CONFIG_FSL_BOOKE +#elif defined (CONFIG_FSL_BOOKE) case PPC_OPROFILE_BOOKE: model = &op_model_fsl_booke; break; |
From: Kumar G. <ga...@ke...> - 2006-10-26 13:58:13
|
On Oct 26, 2006, at 1:43 AM, Vitaly Wool wrote: > Below is the patch for the problem described in "[0/2] build > failure for 8540 w/ CONFIG_OPROFLE=y" letter representing the first > approach. This approach is based on (pretty reasonable) assumption > that op_model_7450.c is applicable only to 6XX, so let's compile it > only when CONFIG_6XX==y. This results in more #ifdef's in arch/ > powerpc/oprofile/common.c, which doesn't look good to me. I'd > rather "switch (cur_cpu_spec->oprofile_type) ..." to SoC-dependent > header files arranging that one as static inline func... > > Anyway, here's the patch. > > arch/powerpc/oprofile/Makefile | 2 +- > arch/powerpc/oprofile/common.c | 7 +++---- > 2 files changed, 4 insertions(+), 5 deletions(-) > > Signed-off-by: Vitaly Wool <vw...@ru...> This makes sense to me since we are just increasing kernel code size for code we would never use for an FSL_BOOKE part if we do it the other way. I dont think its that much more messy with the ifdef's. If you want to do the other cleanup as well I've got no issue with that, but we really should NOT build in support for 7450 into a FSL_BOOKE kernel when reasonably avoidable. - kumar > > Index: linux-2.6.18/arch/powerpc/oprofile/Makefile > =================================================================== > --- linux-2.6.18.orig/arch/powerpc/oprofile/Makefile > +++ linux-2.6.18/arch/powerpc/oprofile/Makefile > @@ -13,4 +13,4 @@ DRIVER_OBJS := $(addprefix ../../../driv > oprofile-y := $(DRIVER_OBJS) common.o backtrace.o > oprofile-$(CONFIG_PPC64) += op_model_rs64.o op_model_power4.o > oprofile-$(CONFIG_FSL_BOOKE) += op_model_fsl_booke.o > -oprofile-$(CONFIG_PPC32) += op_model_7450.o > +oprofile-$(CONFIG_6XX) += op_model_7450.o > Index: linux-2.6.18/arch/powerpc/oprofile/common.c > =================================================================== > --- linux-2.6.18.orig/arch/powerpc/oprofile/common.c > +++ linux-2.6.18/arch/powerpc/oprofile/common.c > @@ -135,19 +135,18 @@ int __init oprofile_arch_init(struct opr > return -ENODEV; > > switch (cur_cpu_spec->oprofile_type) { > -#ifdef CONFIG_PPC64 > +#if defined(CONFIG_PPC64) > case PPC_OPROFILE_RS64: > model = &op_model_rs64; > break; > case PPC_OPROFILE_POWER4: > model = &op_model_power4; > break; > -#else > +#elif defined (CONFIG_6XX) > case PPC_OPROFILE_G4: > model = &op_model_7450; > break; > -#endif > -#ifdef CONFIG_FSL_BOOKE > +#elif defined (CONFIG_FSL_BOOKE) > case PPC_OPROFILE_BOOKE: > model = &op_model_fsl_booke; > break; > > _______________________________________________ > Linuxppc-embedded mailing list > Lin...@oz... > https://ozlabs.org/mailman/listinfo/linuxppc-embedded |
From: Vitaly W. <vw...@ru...> - 2006-10-26 20:26:46
|
Hello Kumar, > This makes sense to me since we are just increasing kernel code size > for code we would never use for an FSL_BOOKE part if we do it the > other way. I dont think its that much more messy with the ifdef's. Okay, if you're fine with this patch, is it possible that you include it into your tree? > If you want to do the other cleanup as well I've got no issue with > that, but we really should NOT build in support for 7450 into a > FSL_BOOKE kernel when reasonably avoidable. That's fine with me. As of the cleanups, well... looks to me some more patches will follow soon, kinda bugfixing ones rather than cleanups first :) Thanks, Vitaly |
From: Sergei S. <ssh...@ru...> - 2006-10-26 20:32:25
|
Hello. Vitaly Wool wrote: > Hello Kumar, >>This makes sense to me since we are just increasing kernel code size >>for code we would never use for an FSL_BOOKE part if we do it the >>other way. I dont think its that much more messy with the ifdef's. > Okay, if you're fine with this patch, is it possible that you include it > into your tree? >>If you want to do the other cleanup as well I've got no issue with >>that, but we really should NOT build in support for 7450 into a >>FSL_BOOKE kernel when reasonably avoidable. > That's fine with me. > As of the cleanups, well... looks to me some more patches will follow > soon, kinda bugfixing ones rather than cleanups first :) The most funny/stupd thing is that the patch existed since May but it never went in: http://patchwork.ozlabs.org/linuxppc/patch?id=5531 > Thanks, > Vitaly WBR, Sergei |
From: Andy F. <afl...@fr...> - 2006-10-27 20:03:57
|
On Oct 26, 2006, at 15:32, Sergei Shtylyov wrote: > Hello. > > Vitaly Wool wrote: >> Hello Kumar, > >>> This makes sense to me since we are just increasing kernel code size >>> for code we would never use for an FSL_BOOKE part if we do it the >>> other way. I dont think its that much more messy with the ifdef's. > >> Okay, if you're fine with this patch, is it possible that you >> include it >> into your tree? > >>> If you want to do the other cleanup as well I've got no issue with >>> that, but we really should NOT build in support for 7450 into a >>> FSL_BOOKE kernel when reasonably avoidable. > >> That's fine with me. >> As of the cleanups, well... looks to me some more patches will follow >> soon, kinda bugfixing ones rather than cleanups first :) > > The most funny/stupd thing is that the patch existed since May > but it never > went in: > > http://patchwork.ozlabs.org/linuxppc/patch?id=5531 Yeah, I guess it got lost. After looking at both patches, I see the different approaches. I think I'd vote for the older patch, since it also solves some SMP issues that will crop up when the dual-core 8572 comes out. In fact, it's similar to this patch: http:// patchwork.ozlabs.org/linuxppc/patch?id=4012 Oi. Ok, I'm going to update and resend that patch in just a second (Ok, this took longer than I thought, due to the lwsync patch I sent out being required). I like Vitaly's patch, but the one I sent cleans up some early design mistakes I made in the original ppc32 oprofile code. > >> Thanks, >> Vitaly > > WBR, Sergei > _______________________________________________ > Linuxppc-dev mailing list > Lin...@oz... > https://ozlabs.org/mailman/listinfo/linuxppc-dev |
From: Vitaly W. <vw...@ru...> - 2006-10-26 06:43:41
|
Below is the patch for the problem described in "[0/2] build failure for 8540 w/ CONFIG_OPROFLE=y" letter representing the second approach. This approach just fixes the mess with ctr_read/ctr_write functions. Pro: no need to mess with arch's and add more confusion to arch/powerpc/oprofile/common.c. Con: when compiled for FSL_BOOKE, op_model_7450 is not valid as it takes ctr_read/ctr_write from FSL_BOOKE variant, which is confusing but actually doesn't matter since it will never be used in this case. So, here's the second patch... arch/powerpc/oprofile/op_model_7450.c | 12 ++++----- arch/powerpc/oprofile/op_model_fsl_booke.c | 37 ----------------------------- include/asm-powerpc/oprofile_impl.h | 36 ++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 43 deletions(-) Signed-off-by: Vitaly Wool <vw...@ru...> Index: linux-2.6.18/arch/powerpc/oprofile/op_model_7450.c =================================================================== --- linux-2.6.18.orig/arch/powerpc/oprofile/op_model_7450.c +++ linux-2.6.18/arch/powerpc/oprofile/op_model_7450.c @@ -58,7 +58,7 @@ static u32 mmcr0_val, mmcr1_val, mmcr2_v * enables the counters to trigger the interrupt, and sets the * counters to only count when the mark bit is not set. */ -static void pmc_start_ctrs(void) +static void pmc_7450_start_ctrs(void) { u32 mmcr0 = mfspr(SPRN_MMCR0); @@ -69,7 +69,7 @@ static void pmc_start_ctrs(void) } /* Disables the counters on this CPU, and freezes them */ -static void pmc_stop_ctrs(void) +static void pmc_7450_stop_ctrs(void) { u32 mmcr0 = mfspr(SPRN_MMCR0); @@ -84,7 +84,7 @@ static void pmc_stop_ctrs(void) static void fsl7450_cpu_setup(void *unused) { /* freeze all counters */ - pmc_stop_ctrs(); + pmc_7450_stop_ctrs(); mtspr(SPRN_MMCR0, mmcr0_val); mtspr(SPRN_MMCR1, mmcr1_val); @@ -145,7 +145,7 @@ static void fsl7450_start(struct op_coun /* Clear the freeze bit, and enable the interrupt. * The counters won't actually start until the rfi clears * the PMM bit */ - pmc_start_ctrs(); + pmc_7450_start_ctrs(); oprofile_running = 1; } @@ -154,7 +154,7 @@ static void fsl7450_start(struct op_coun static void fsl7450_stop(void) { /* freeze counters */ - pmc_stop_ctrs(); + pmc_7450_stop_ctrs(); oprofile_running = 0; @@ -194,7 +194,7 @@ static void fsl7450_handle_interrupt(str /* Clear the freeze bit, and reenable the interrupt. * The counters won't actually start until the rfi clears * the PMM bit */ - pmc_start_ctrs(); + pmc_7450_start_ctrs(); } struct op_powerpc_model op_model_7450= { Index: linux-2.6.18/arch/powerpc/oprofile/op_model_fsl_booke.c =================================================================== --- linux-2.6.18.orig/arch/powerpc/oprofile/op_model_fsl_booke.c +++ linux-2.6.18/arch/powerpc/oprofile/op_model_fsl_booke.c @@ -32,43 +32,6 @@ static unsigned long reset_value[OP_MAX_ static int num_counters; static int oprofile_running; -static inline unsigned int ctr_read(unsigned int i) -{ - switch(i) { - case 0: - return mfpmr(PMRN_PMC0); - case 1: - return mfpmr(PMRN_PMC1); - case 2: - return mfpmr(PMRN_PMC2); - case 3: - return mfpmr(PMRN_PMC3); - default: - return 0; - } -} - -static inline void ctr_write(unsigned int i, unsigned int val) -{ - switch(i) { - case 0: - mtpmr(PMRN_PMC0, val); - break; - case 1: - mtpmr(PMRN_PMC1, val); - break; - case 2: - mtpmr(PMRN_PMC2, val); - break; - case 3: - mtpmr(PMRN_PMC3, val); - break; - default: - break; - } -} - - static void fsl_booke_reg_setup(struct op_counter_config *ctr, struct op_system_config *sys, int num_ctrs) Index: linux-2.6.18/include/asm-powerpc/oprofile_impl.h =================================================================== --- linux-2.6.18.orig/include/asm-powerpc/oprofile_impl.h +++ linux-2.6.18/include/asm-powerpc/oprofile_impl.h @@ -121,6 +121,42 @@ static inline void ctr_write(unsigned in break; } } +#else +static inline unsigned int ctr_read(unsigned int i) +{ + switch(i) { + case 0: + return mfpmr(PMRN_PMC0); + case 1: + return mfpmr(PMRN_PMC1); + case 2: + return mfpmr(PMRN_PMC2); + case 3: + return mfpmr(PMRN_PMC3); + default: + return 0; + } +} + +static inline void ctr_write(unsigned int i, unsigned int val) +{ + switch(i) { + case 0: + mtpmr(PMRN_PMC0, val); + break; + case 1: + mtpmr(PMRN_PMC1, val); + break; + case 2: + mtpmr(PMRN_PMC2, val); + break; + case 3: + mtpmr(PMRN_PMC3, val); + break; + default: + break; + } +} #endif /* !CONFIG_FSL_BOOKE */ extern void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth); |