| Message ID | 622b8aec162cc43e774bde5da990a61fc961b4d9.1519976944.git.christophe.leroy@c-s.fr |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
| Series | Powerpc: mpc8xx: cleanup before migration to DM model | expand |
Dear Christophe, In message <622b8aec162cc43e774bde5da990a61fc961b4d9.1519976944.git.christophe.leroy@c-s.fr> you wrote: > Function get_immr() is almost not used and doesn't bring much > added value. Just replace it with mfspr(SPRN_IMMR) at the two > needed places. ... > static int check_CPU(long clock, uint pvr, uint immr) > { > - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); > + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; This is a totally unrelated change, which additionally changes the behaviour of the code - the content of the argument "immr" is now not longer used here. If this is necessary / intentional, it should be a separate commit with proper explanation. > - uint immr = get_immr(0); /* Return full IMMR contents */ > - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); > + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; Ditto here. > - uint immr = get_immr(0); /* Return full IMMR contents */ > - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); > + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; And here again. > -#if defined(CONFIG_8xx) > -static inline uint get_immr(uint mask) > -{ > - uint immr = mfspr(SPRN_IMMR); > - > - return mask ? (immr & mask) : immr; > -} > -#endif Actually, I do not see what you win by this "cleanup". In my opinion the function serves a good purpose; your code just becomes more difficult to understand. Best regards, Wolfgang Denk
Le 03/03/2018 à 17:46, Wolfgang Denk a écrit : > Dear Christophe, > > In message <622b8aec162cc43e774bde5da990a61fc961b4d9.1519976944.git.christophe.leroy@c-s.fr> you wrote: >> Function get_immr() is almost not used and doesn't bring much >> added value. Just replace it with mfspr(SPRN_IMMR) at the two >> needed places. > ... >> static int check_CPU(long clock, uint pvr, uint immr) >> { >> - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); >> + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; > > This is a totally unrelated change, which additionally changes the > behaviour of the code - the content of the argument "immr" is now > not longer used here. In many other places (eg. checkdcache(), do_reset(), etc.), immap is defined as this. Why not doing the same everywhere ? The lower part of immr is still used later in the function. > > If this is necessary / intentional, it should be a separate commit > with proper explanation. Ok > > >> - uint immr = get_immr(0); /* Return full IMMR contents */ >> - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); >> + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; > > Ditto here. > >> - uint immr = get_immr(0); /* Return full IMMR contents */ >> - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); >> + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; > > And here again. > >> -#if defined(CONFIG_8xx) >> -static inline uint get_immr(uint mask) >> -{ >> - uint immr = mfspr(SPRN_IMMR); >> - >> - return mask ? (immr & mask) : immr; >> -} >> -#endif > > Actually, I do not see what you win by this "cleanup". In my > opinion the function serves a good purpose; your code just becomes > more difficult to understand. The main idea was to get rid of as much as possible specific 8xx stuff in common header files. Since SPRN_IMMR is defined regardless of the subarch, maybe I should have just remove the #ifdef around get_immr() Regarding the understandability of the code, I thought it would be clearer to define immap the same way in all functions. immr being set with CONFIG_SYS_IMMR early in start.S, and not changing anywhere after that, I don't see any point in reading it from SPRN_IMMR everytime we need it, especially as many other functions already set it from CONFIG_SYS_IMMR. 83xx, 86xx etc... do set immap ptr from CONFIG_SYS_IMMR too. Regards Christophe > > Best regards, > > Wolfgang Denk > --- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus
Dear Christophe, In message <71a3900b-f61e-e9f8-c12a-5ec5aa1420bc@c-s.fr> you wrote: > > >> - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); > >> + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; > > > > This is a totally unrelated change, which additionally changes the > > behaviour of the code - the content of the argument "immr" is now > > not longer used here. > > In many other places (eg. checkdcache(), do_reset(), etc.), immap is > defined as this. Why not doing the same everywhere ? Firwt, it is an unrelated and undocumented change - you don't mention it in the commit message. This _must_ be fixed. Actually I think this should be a separate patch, both for documentation and bisectability. The other question is if there really is a guarantee that IMMR ist set to the value of CONFIG_SYS_IMMR. If that was the case, the whole code would make little sense, and I don't think it was written like that just for fun. So please double-check. > The lower part of immr is still used later in the function. Now if that is the case,, then your modification makes even less sense. If we need the value anyway, why implement two different sources of information? This looks bogus to me. > Since SPRN_IMMR is defined regardless of the subarch, maybe I should > have just remove the #ifdef around get_immr() Indeed that would make more sense, IMO. Best regards, Wolfgang Denk
Le 04/03/2018 à 15:51, Wolfgang Denk a écrit : > Dear Christophe, > > In message <71a3900b-f61e-e9f8-c12a-5ec5aa1420bc@c-s.fr> you wrote: >> >>>> - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); >>>> + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; >>> >>> This is a totally unrelated change, which additionally changes the >>> behaviour of the code - the content of the argument "immr" is now >>> not longer used here. >> >> In many other places (eg. checkdcache(), do_reset(), etc.), immap is >> defined as this. Why not doing the same everywhere ? > > Firwt, it is an unrelated and undocumented change - you don't > mention it in the commit message. This _must_ be fixed. Actually I > think this should be a separate patch, both for documentation and > bisectability. I agree, my mistake. > > The other question is if there really is a guarantee that IMMR ist > set to the value of CONFIG_SYS_IMMR. If that was the case, the > whole code would make little sense, and I don't think it was written > like that just for fun. So please double-check. In start.S: .globl _start _start: lis r3, CONFIG_SYS_IMMR@h /* position IMMR */ mtspr 638, r3 In many fonctions, you have (in the same file cpu.c): int checkicache(void) { immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; int checkdcache(void) { immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; void upmconfig(uint upm, uint *table, uint size) { uint i; uint addr = 0; immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { ulong msr, addr; immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; In other files too (eg: 5 times in immap.c, 7 times in interrupts.c, 6 times in mpc8xx_fec.c , etc.) > >> The lower part of immr is still used later in the function. > > Now if that is the case,, then your modification makes even less > sense. If we need the value anyway, why implement two different > sources of information? This looks bogus to me. What makes little sense to me is to define the immap pointer differently in three places, allthough it is equivalent. It is not the same information we need later in the function. What this function (and only this one) needs in addition is the lower part of SPRN_IMMR while the immap pointer is the higher part of SPRN_IMMR (that we set earlier in start.S) > >> Since SPRN_IMMR is defined regardless of the subarch, maybe I should >> have just remove the #ifdef around get_immr() > > Indeed that would make more sense, IMO. I'll do that, and eventually make another patch for unifying the way the immap pointer is set. Best regards Christophe --- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus
diff --git a/arch/powerpc/cpu/mpc8xx/cpu.c b/arch/powerpc/cpu/mpc8xx/cpu.c index 1883440db34..0eabb714d31 100644 --- a/arch/powerpc/cpu/mpc8xx/cpu.c +++ b/arch/powerpc/cpu/mpc8xx/cpu.c @@ -36,7 +36,7 @@ DECLARE_GLOBAL_DATA_PTR; static int check_CPU(long clock, uint pvr, uint immr) { - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; uint k; char buf[32]; @@ -90,7 +90,7 @@ static int check_CPU(long clock, uint pvr, uint immr) int checkcpu(void) { ulong clock = gd->cpu_clk; - uint immr = get_immr(0); /* Return full IMMR contents */ + uint immr = mfspr(SPRN_IMMR); /* Return full IMMR contents */ uint pvr = get_pvr(); puts("CPU: "); @@ -237,8 +237,7 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) */ unsigned long get_tbclk(void) { - uint immr = get_immr(0); /* Return full IMMR contents */ - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; ulong oscclk, factor, pll; if (in_be32(&immap->im_clkrst.car_sccr) & SCCR_TBS) diff --git a/arch/powerpc/cpu/mpc8xx/reginfo.c b/arch/powerpc/cpu/mpc8xx/reginfo.c index 277d2753b25..1f6e606e52a 100644 --- a/arch/powerpc/cpu/mpc8xx/reginfo.c +++ b/arch/powerpc/cpu/mpc8xx/reginfo.c @@ -22,7 +22,7 @@ void print_reginfo(void) */ printf("\nSystem Configuration registers\n" - "\tIMMR\t0x%08X\n", get_immr(0)); + "\tIMMR\t0x%08X\n", mfspr(SPRN_IMMR)); printf("\tSIUMCR\t0x%08X", in_be32(&sysconf->sc_siumcr)); printf("\tSYPCR\t0x%08X\n", in_be32(&sysconf->sc_sypcr)); diff --git a/arch/powerpc/cpu/mpc8xx/speed.c b/arch/powerpc/cpu/mpc8xx/speed.c index fa8f87cbc5e..f8eb4a13eaf 100644 --- a/arch/powerpc/cpu/mpc8xx/speed.c +++ b/arch/powerpc/cpu/mpc8xx/speed.c @@ -17,8 +17,7 @@ DECLARE_GLOBAL_DATA_PTR; */ int get_clocks(void) { - uint immr = get_immr(0); /* Return full IMMR contents */ - immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000); + immap_t __iomem *immap = (immap_t __iomem *)CONFIG_SYS_IMMR; uint sccr = in_be32(&immap->im_clkrst.car_sccr); uint divider = 1 << (((sccr & SCCR_DFBRG11) >> 11) * 2); diff --git a/arch/powerpc/include/asm/ppc.h b/arch/powerpc/include/asm/ppc.h index 5e0aa08be93..db289ed7072 100644 --- a/arch/powerpc/include/asm/ppc.h +++ b/arch/powerpc/include/asm/ppc.h @@ -40,14 +40,6 @@ #include <asm/processor.h> -#if defined(CONFIG_8xx) -static inline uint get_immr(uint mask) -{ - uint immr = mfspr(SPRN_IMMR); - - return mask ? (immr & mask) : immr; -} -#endif static inline uint get_pvr(void) { return mfspr(PVR);
Function get_immr() is almost not used and doesn't bring much added value. Just replace it with mfspr(SPRN_IMMR) at the two needed places. Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- arch/powerpc/cpu/mpc8xx/cpu.c | 7 +++---- arch/powerpc/cpu/mpc8xx/reginfo.c | 2 +- arch/powerpc/cpu/mpc8xx/speed.c | 3 +-- arch/powerpc/include/asm/ppc.h | 8 -------- 4 files changed, 5 insertions(+), 15 deletions(-)