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