[U-Boot] [PATCH v2 07/10] powerpc: mpc8xx: get rid of get_immr()

[christophe leroy]

Le 03/03/2018 à 17:46, Wolfgang Denk a écrit :
> Dear Christophe,
> 
> In message <622b8aec162cc43e774bde5da990a61fc961b4d9.1519976944.git.christophe.leroy at 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