[U-Boot,06/14] powerpc: Remove unneccessary #ifdefs in reginfo
diff mbox

Message ID a0ecab632c81feb7b40679076a6165ac7050d9b8.1499629706.git.christophe.leroy@c-s.fr
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Christophe Leroy July 12, 2017, 9:43 a.m. UTC
reginfo command is calling mpc8xx_reginfo(), mpc85xx_reginfo()
or mpc86xx_reginfo() based on CONFIG_ symbol.
As those 3 functions can't me defined at the same time, let's
rename them mpc8xxx_reginfo() to avoid the #ifdefs

Lets all remove the #ifdefs around the U_BOOT_CMD as this
file is only compiled when CONFIG_CMD_REGINFO is defined

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/cpu/mpc85xx/cpu.c    |  3 ++-
 arch/powerpc/cpu/mpc86xx/cpu.c    |  3 ++-
 arch/powerpc/cpu/mpc8xx/reginfo.c |  3 ++-
 arch/powerpc/include/asm/ppc.h    |  4 ++++
 cmd/reginfo.c                     | 20 ++------------------
 5 files changed, 12 insertions(+), 21 deletions(-)

Comments

Wolfgang Denk July 12, 2017, 1:07 p.m. UTC | #1
Dear Christophe Leroy,

In message <a0ecab632c81feb7b40679076a6165ac7050d9b8.1499629706.git.christophe.leroy@c-s.fr> you wrote:
> reginfo command is calling mpc8xx_reginfo(), mpc85xx_reginfo()
> or mpc86xx_reginfo() based on CONFIG_ symbol.
> As those 3 functions can't me defined at the same time, let's
> rename them mpc8xxx_reginfo() to avoid the #ifdefs

This is indeed a tempting idea, but MPC8xx is a totally different
thing than MPC8xxxx, so the chosen name is misleading and should be
avoided.

> Lets all remove the #ifdefs around the U_BOOT_CMD as this
> file is only compiled when CONFIG_CMD_REGINFO is defined

Has this change been tested / verified against all other boards /
architectures?

Best regards,

Wolfgang Denk
Christophe Leroy July 12, 2017, 2:28 p.m. UTC | #2
Dear Wolfgang,

Le 12/07/2017 à 15:07, Wolfgang Denk a écrit :
> Dear Christophe Leroy,
> 
> In message <a0ecab632c81feb7b40679076a6165ac7050d9b8.1499629706.git.christophe.leroy@c-s.fr> you wrote:
>> reginfo command is calling mpc8xx_reginfo(), mpc85xx_reginfo()
>> or mpc86xx_reginfo() based on CONFIG_ symbol.
>> As those 3 functions can't me defined at the same time, let's
>> rename them mpc8xxx_reginfo() to avoid the #ifdefs
> 
> This is indeed a tempting idea, but MPC8xx is a totally different
> thing than MPC8xxxx, so the chosen name is misleading and should be
> avoided.

Oh ? Ok. I thought it would be a possible name because for instance in 
the Linux Kernel, the watchdog driver is named that way and used also 
for the 8xx and so was also the SPI driver before its name was change to 
fsl_spi.
Isn't the 8xx an 81xx indeed ?

Any suggestion for a good name ? Would fsl_reginfo() be a good name ?


> 
>> Lets all remove the #ifdefs around the U_BOOT_CMD as this
>> file is only compiled when CONFIG_CMD_REGINFO is defined
> 
> Has this change been tested / verified against all other boards /
> architectures?

Travis-CI is not working at the time being, but what would be the issue ?

In the Makefile, it is defined as
obj-$(CONFIG_CMD_REGINFO) += reginfo.o, so if CONFIG_CMD_REGINFO is not 
defined, this file won't be compiled at all, will it ?

Regards
Christophe

> 
> Best regards,
> 
> Wolfgang Denk
>
Wolfgang Denk July 12, 2017, 2:53 p.m. UTC | #3
Dear Christophe,

In message <194b25e4-81fc-52cf-aeef-61ce6f4679a7@c-s.fr> you wrote:
> 
> Oh ? Ok. I thought it would be a possible name because for instance in 
> the Linux Kernel, the watchdog driver is named that way and used also 
> for the 8xx and so was also the SPI driver before its name was change to 
> fsl_spi.

There are always bad examples ;-)

> Isn't the 8xx an 81xx indeed ?

I have never seen this naming used; google does not know about it
either.

> Any suggestion for a good name ? Would fsl_reginfo() be a good name ?

FSL does not even exist any more...


Soory, I was never good in naming things...

Best regards,

Wolfgang Denk
Tom Rini July 12, 2017, 7:56 p.m. UTC | #4
On Wed, Jul 12, 2017 at 04:53:26PM +0200, Wolfgang Denk wrote:
> Dear Christophe,
> 
> In message <194b25e4-81fc-52cf-aeef-61ce6f4679a7@c-s.fr> you wrote:
> > 
> > Oh ? Ok. I thought it would be a possible name because for instance in 
> > the Linux Kernel, the watchdog driver is named that way and used also 
> > for the 8xx and so was also the SPI driver before its name was change to 
> > fsl_spi.
> 
> There are always bad examples ;-)
> 
> > Isn't the 8xx an 81xx indeed ?
> 
> I have never seen this naming used; google does not know about it
> either.
> 
> > Any suggestion for a good name ? Would fsl_reginfo() be a good name ?
> 
> FSL does not even exist any more...
> 
> 
> Soory, I was never good in naming things...

FSL doesn't exist, and I'm terrible at naming things as well, but I
think fsl_reginfo makes enough namespace sense, we have a lot of fsl_xxx
in arch/powerpc and qcom_ might start confusing folks more than helping
:)
Christophe Leroy July 13, 2017, 1:15 p.m. UTC | #5
Le 12/07/2017 à 21:56, Tom Rini a écrit :
> On Wed, Jul 12, 2017 at 04:53:26PM +0200, Wolfgang Denk wrote:
>> Dear Christophe,
>>
>> In message <194b25e4-81fc-52cf-aeef-61ce6f4679a7@c-s.fr> you wrote:
>>>
>>> Oh ? Ok. I thought it would be a possible name because for instance in
>>> the Linux Kernel, the watchdog driver is named that way and used also
>>> for the 8xx and so was also the SPI driver before its name was change to
>>> fsl_spi.
>>
>> There are always bad examples ;-)
>>
>>> Isn't the 8xx an 81xx indeed ?
>>
>> I have never seen this naming used; google does not know about it
>> either.
>>
>>> Any suggestion for a good name ? Would fsl_reginfo() be a good name ?
>>
>> FSL does not even exist any more...
>>
>>
>> Soory, I was never good in naming things...
> 
> FSL doesn't exist, and I'm terrible at naming things as well, but I
> think fsl_reginfo makes enough namespace sense, we have a lot of fsl_xxx
> in arch/powerpc and qcom_ might start confusing folks more than helping
> :)
> 


Finally I called in print_reginfo() as it has indeed no dependency with 
any target type or arch. Should someone want to implement it for another 
arch like an ARM CPU, he could do by just defining print_reginfo() and 
maybe moving the declaration from asm/ppc.h to a more generic place.

Christophe

Patch
diff mbox

diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
index e3ef4ae816..711354a868 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu.c
@@ -23,6 +23,7 @@ 
 #include <post.h>
 #include <asm/processor.h>
 #include <fsl_ddr_sdram.h>
+#include <asm/ppc.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -384,7 +385,7 @@  int cpu_mmc_init(bd_t *bis)
  * Currently prints out LAWs, BR0/OR0 for LBC, CSPR/CSOR/Timing
  * parameters for IFC and TLBs
  */
-void mpc85xx_reginfo(void)
+void mpc8xxx_reginfo(void)
 {
 	print_tlbcam();
 	print_laws();
diff --git a/arch/powerpc/cpu/mpc86xx/cpu.c b/arch/powerpc/cpu/mpc86xx/cpu.c
index 7a9570c8ec..c46cd965dd 100644
--- a/arch/powerpc/cpu/mpc86xx/cpu.c
+++ b/arch/powerpc/cpu/mpc86xx/cpu.c
@@ -13,6 +13,7 @@ 
 #include <asm/mmu.h>
 #include <mpc86xx.h>
 #include <asm/fsl_law.h>
+#include <asm/ppc.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -160,7 +161,7 @@  watchdog_reset(void)
  * Print out the state of various machine registers.
  * Currently prints out LAWs, BR0/OR0, and BATs
  */
-void mpc86xx_reginfo(void)
+void mpc8xxx_reginfo(void)
 {
 	print_bats();
 	print_laws();
diff --git a/arch/powerpc/cpu/mpc8xx/reginfo.c b/arch/powerpc/cpu/mpc8xx/reginfo.c
index 1ba4d22bdd..62fbf151ce 100644
--- a/arch/powerpc/cpu/mpc8xx/reginfo.c
+++ b/arch/powerpc/cpu/mpc8xx/reginfo.c
@@ -8,8 +8,9 @@ 
 #include <common.h>
 #include <mpc8xx.h>
 #include <asm/io.h>
+#include <asm/ppc.h>
 
-void mpc8xx_reginfo(void)
+void mpc8xxx_reginfo(void)
 {
 	immap_t __iomem     *immap  = (immap_t __iomem *)CONFIG_SYS_IMMR;
 	memctl8xx_t __iomem *memctl = &immap->im_memctl;
diff --git a/arch/powerpc/include/asm/ppc.h b/arch/powerpc/include/asm/ppc.h
index 89f08eccc7..4013ddaa74 100644
--- a/arch/powerpc/include/asm/ppc.h
+++ b/arch/powerpc/include/asm/ppc.h
@@ -110,6 +110,10 @@  static inline void set_msr(unsigned long msr)
 	asm volatile ("mtmsr %0" : : "r" (msr));
 }
 
+#ifdef CONFIG_CMD_REGINFO
+void mpc8xxx_reginfo(void);
+#endif
+
 #endif /* !__ASSEMBLY__ */
 
 #ifdef CONFIG_PPC
diff --git a/cmd/reginfo.c b/cmd/reginfo.c
index b364cc899a..5640b035dc 100644
--- a/cmd/reginfo.c
+++ b/cmd/reginfo.c
@@ -7,36 +7,20 @@ 
 
 #include <common.h>
 #include <command.h>
-#if defined(CONFIG_8xx)
-void mpc8xx_reginfo(void);
-#elif defined(CONFIG_MPC86xx)
-extern void mpc86xx_reginfo(void);
-#elif defined(CONFIG_MPC85xx)
-extern void mpc85xx_reginfo(void);
-#endif
+#include <asm/ppc.h>
 
 static int do_reginfo(cmd_tbl_t *cmdtp, int flag, int argc,
 		       char * const argv[])
 {
-#if defined(CONFIG_8xx)
-	mpc8xx_reginfo();
-
-#elif defined(CONFIG_MPC86xx)
-	mpc86xx_reginfo();
-
-#elif defined(CONFIG_MPC85xx)
-	mpc85xx_reginfo();
-#endif
+	mpc8xxx_reginfo();
 
 	return 0;
 }
 
  /**************************************************/
 
-#if defined(CONFIG_CMD_REGINFO)
 U_BOOT_CMD(
 	reginfo,	2,	1,	do_reginfo,
 	"print register information",
 	""
 );
-#endif