[U-Boot,v2,07/10] powerpc: mpc8xx: get rid of get_immr()
diff mbox series

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
Related show

Commit Message

Christophe Leroy March 2, 2018, 9:32 a.m. UTC
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(-)

Comments

Wolfgang Denk March 3, 2018, 4:46 p.m. UTC | #1
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
Christophe Leroy March 4, 2018, 9:01 a.m. UTC | #2
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
Wolfgang Denk March 4, 2018, 2:51 p.m. UTC | #3
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
Christophe Leroy March 4, 2018, 4:39 p.m. UTC | #4
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

Patch
diff mbox series

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);