diff mbox

[U-Boot,1/6] i.mx: i.mx5: Move some files to imx-common folder

Message ID 1321094190-8108-2-git-send-email-jason.hui@linaro.org
State Changes Requested
Delegated to: Stefano Babic
Headers show

Commit Message

Jason Liu Nov. 12, 2011, 10:36 a.m. UTC
In order to support the coming MX6 platform and to reducde
the duplicated code, we had better move some common files
to the imx-common folder for sharing.

Signed-off-by: Jason Liu <jason.hui@linaro.org>
---
 Makefile                                       |    7 ++
 arch/arm/cpu/armv7/imx-common/Makefile         |   47 ++++++++++
 arch/arm/cpu/armv7/imx-common/cpu_info.c       |  108 ++++++++++++++++++++++++
 arch/arm/cpu/armv7/{mx5 => imx-common}/speed.c |    0
 arch/arm/cpu/armv7/{mx5 => imx-common}/timer.c |   17 ++--
 arch/arm/cpu/armv7/mx5/Makefile                |    2 +-
 arch/arm/cpu/armv7/mx5/soc.c                   |   77 -----------------
 7 files changed, 172 insertions(+), 86 deletions(-)

Comments

Stefano Babic Nov. 14, 2011, 8:34 a.m. UTC | #1
On 11/12/2011 11:36 AM, Jason Liu wrote:
> In order to support the coming MX6 platform and to reducde
> the duplicated code, we had better move some common files
> to the imx-common folder for sharing.
> 
> Signed-off-by: Jason Liu <jason.hui@linaro.org>
> ---

Hi jason,

>  Makefile                                       |    7 ++
>  arch/arm/cpu/armv7/imx-common/Makefile         |   47 ++++++++++
>  arch/arm/cpu/armv7/imx-common/cpu_info.c       |  108 ++++++++++++++++++++++++
>  arch/arm/cpu/armv7/{mx5 => imx-common}/speed.c |    0
>  arch/arm/cpu/armv7/{mx5 => imx-common}/timer.c |   17 ++--
>  arch/arm/cpu/armv7/mx5/Makefile                |    2 +-
>  arch/arm/cpu/armv7/mx5/soc.c                   |   77 -----------------
>  7 files changed, 172 insertions(+), 86 deletions(-)

I agree completely to add a common directory to share code between MX5
and MX6. However, your patch mixes new code with code moved from MX5.
This is at least not coherent with the commit message, where you say you
are only moving files. I understand what you did, I think it is enough
to extend the commit message to better explain it.


> +#########################################################################
> diff --git a/arch/arm/cpu/armv7/imx-common/cpu_info.c b/arch/arm/cpu/armv7/imx-common/cpu_info.c

The name of the file is quite confusing - I am expecting to see only
function to retrieve information about the revision, but there are other
common functions. It is better to use another name.

> +++ b/arch/arm/cpu/armv7/imx-common/cpu_info.c
> @@ -0,0 +1,108 @@
> +/*
> + * (C) Copyright 2007
> + * Sascha Hauer, Pengutronix
> + *
> + * (C) Copyright 2009 Freescale Semiconductor, Inc.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +

Ok - all these functions are taken from the original soc.c.

> diff --git a/arch/arm/cpu/armv7/mx5/speed.c b/arch/arm/cpu/armv7/imx-common/speed.c
> similarity index 100%
> rename from arch/arm/cpu/armv7/mx5/speed.c
> rename to arch/arm/cpu/armv7/imx-common/speed.c
> diff --git a/arch/arm/cpu/armv7/mx5/timer.c b/arch/arm/cpu/armv7/imx-common/timer.c
> old mode 100644
> new mode 100755
> similarity index 84%
> rename from arch/arm/cpu/armv7/mx5/timer.c
> rename to arch/arm/cpu/armv7/imx-common/timer.c
> index 2544b08..98e9f4a
> --- a/arch/arm/cpu/armv7/mx5/timer.c
> +++ b/arch/arm/cpu/armv7/imx-common/timer.c

You mix here two things - you move the files and you change it adding
new features. Split into two patches.

Best regards,
Stefano Babic
Jason Liu Nov. 14, 2011, 8:57 a.m. UTC | #2
On Mon, Nov 14, 2011 at 4:34 PM, Stefano Babic <sbabic@denx.de> wrote:
> On 11/12/2011 11:36 AM, Jason Liu wrote:
>> In order to support the coming MX6 platform and to reducde
>> the duplicated code, we had better move some common files
>> to the imx-common folder for sharing.
>>
>> Signed-off-by: Jason Liu <jason.hui@linaro.org>
>> ---
>
> Hi jason,
>
>>  Makefile                                       |    7 ++
>>  arch/arm/cpu/armv7/imx-common/Makefile         |   47 ++++++++++
>>  arch/arm/cpu/armv7/imx-common/cpu_info.c       |  108 ++++++++++++++++++++++++
>>  arch/arm/cpu/armv7/{mx5 => imx-common}/speed.c |    0
>>  arch/arm/cpu/armv7/{mx5 => imx-common}/timer.c |   17 ++--
>>  arch/arm/cpu/armv7/mx5/Makefile                |    2 +-
>>  arch/arm/cpu/armv7/mx5/soc.c                   |   77 -----------------
>>  7 files changed, 172 insertions(+), 86 deletions(-)
>
> I agree completely to add a common directory to share code between MX5
> and MX6. However, your patch mixes new code with code moved from MX5.
> This is at least not coherent with the commit message, where you say you
> are only moving files. I understand what you did, I think it is enough
> to extend the commit message to better explain it.

Thanks for review. I will extend the commit message to reflect other changes.

>
>
>> +#########################################################################
>> diff --git a/arch/arm/cpu/armv7/imx-common/cpu_info.c b/arch/arm/cpu/armv7/imx-common/cpu_info.c
>
> The name of the file is quite confusing - I am expecting to see only
> function to retrieve information about the revision, but there are other
> common functions. It is better to use another name.

Yes, agree. what about cpu.c? if you dislike, could you do me a favor
to name it. :)

>
>> +++ b/arch/arm/cpu/armv7/imx-common/cpu_info.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * (C) Copyright 2007
>> + * Sascha Hauer, Pengutronix
>> + *
>> + * (C) Copyright 2009 Freescale Semiconductor, Inc.
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>
> Ok - all these functions are taken from the original soc.c.
>
>> diff --git a/arch/arm/cpu/armv7/mx5/speed.c b/arch/arm/cpu/armv7/imx-common/speed.c
>> similarity index 100%
>> rename from arch/arm/cpu/armv7/mx5/speed.c
>> rename to arch/arm/cpu/armv7/imx-common/speed.c
>> diff --git a/arch/arm/cpu/armv7/mx5/timer.c b/arch/arm/cpu/armv7/imx-common/timer.c
>> old mode 100644
>> new mode 100755
>> similarity index 84%
>> rename from arch/arm/cpu/armv7/mx5/timer.c
>> rename to arch/arm/cpu/armv7/imx-common/timer.c
>> index 2544b08..98e9f4a
>> --- a/arch/arm/cpu/armv7/mx5/timer.c
>> +++ b/arch/arm/cpu/armv7/imx-common/timer.c
>
> You mix here two things - you move the files and you change it adding
> new features. Split into two patches.

In fact, I did not do any function change. I just do the followings two changes:

- fix the checkpatch warnings with the original timer.c file in mx5 folder.
- change the CONFIG_SYS_MX5_CLK32 to CLK_32KHZ

I can add the changes to the commit message. Do I still need split
into two patches?

Jason Liu
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
> =====================================================================
>
Stefano Babic Nov. 14, 2011, 11:28 a.m. UTC | #3
On 11/14/2011 09:57 AM, Jason Hui wrote:
> 
> Yes, agree. what about cpu.c? if you dislike, could you do me a favor
> to name it. :)

Agree - let's change it into cpu.c !

>>
>> You mix here two things - you move the files and you change it adding
>> new features. Split into two patches.
> 
> In fact, I did not do any function change. I just do the followings two changes:
> 
> - fix the checkpatch warnings with the original timer.c file in mx5 folder.
> - change the CONFIG_SYS_MX5_CLK32 to CLK_32KHZ
> 
> I can add the changes to the commit message. Do I still need split
> into two patches?

No, you have better explained what you did - it is ok for me if you
extend the commit message.

Best regards,
Stefano Babic
diff mbox

Patch

diff --git a/Makefile b/Makefile
index 294c762..9672a10 100644
--- a/Makefile
+++ b/Makefile
@@ -300,6 +300,13 @@  ifeq ($(SOC),omap4)
 LIBS += $(CPUDIR)/omap-common/libomap-common.o
 endif
 
+ifeq ($(SOC),mx5)
+LIBS += $(CPUDIR)/imx-common/libimx-common.o
+endif
+ifeq ($(SOC),mx6)
+LIBS += $(CPUDIR)/imx-common/libimx-common.o
+endif
+
 ifeq ($(SOC),s5pc1xx)
 LIBS += $(CPUDIR)/s5p-common/libs5p-common.o
 endif
diff --git a/arch/arm/cpu/armv7/imx-common/Makefile b/arch/arm/cpu/armv7/imx-common/Makefile
new file mode 100644
index 0000000..9fe8f02
--- /dev/null
+++ b/arch/arm/cpu/armv7/imx-common/Makefile
@@ -0,0 +1,47 @@ 
+#
+# (C) Copyright 2000-2006
+# Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+#
+# (C) Copyright 2011 Freescale Semiconductor, Inc.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+#
+
+include $(TOPDIR)/config.mk
+
+LIB     = $(obj)libimx-common.o
+
+COBJS	= timer.o cpu_info.o speed.o
+
+SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
+OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
+
+all:	$(obj).depend $(LIB)
+
+$(LIB):	$(OBJS)
+	$(call cmd_link_o_target, $(OBJS))
+
+#########################################################################
+
+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk
+
+sinclude $(obj).depend
+
+#########################################################################
diff --git a/arch/arm/cpu/armv7/imx-common/cpu_info.c b/arch/arm/cpu/armv7/imx-common/cpu_info.c
new file mode 100644
index 0000000..1e30ae5
--- /dev/null
+++ b/arch/arm/cpu/armv7/imx-common/cpu_info.c
@@ -0,0 +1,108 @@ 
+/*
+ * (C) Copyright 2007
+ * Sascha Hauer, Pengutronix
+ *
+ * (C) Copyright 2009 Freescale Semiconductor, Inc.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <asm/errno.h>
+#include <asm/io.h>
+#include <asm/arch/imx-regs.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/sys_proto.h>
+
+#ifdef CONFIG_FSL_ESDHC
+#include <fsl_esdhc.h>
+#endif
+
+static char *get_reset_cause(void)
+{
+	u32 cause;
+	struct src *src_regs = (struct src *)SRC_BASE_ADDR;
+
+	cause = readl(&src_regs->srsr);
+	writel(cause, &src_regs->srsr);
+
+	switch (cause) {
+	case 0x00001:
+		return "POR";
+	case 0x00004:
+		return "CSU";
+	case 0x00008:
+		return "IPP USER";
+	case 0x00010:
+		return "WDOG";
+	case 0x00020:
+		return "JTAG HIGH-Z";
+	case 0x00040:
+		return "JTAG SW";
+	case 0x10000:
+		return "WARM BOOT";
+	default:
+		return "unknown reset";
+	}
+}
+
+#if defined(CONFIG_DISPLAY_CPUINFO)
+int print_cpuinfo(void)
+{
+	u32 cpurev;
+
+	cpurev = get_cpu_rev();
+	printf("CPU:   Freescale i.MX%x family rev%d.%d at %d MHz\n",
+		(cpurev & 0xFF000) >> 12,
+		(cpurev & 0x000F0) >> 4,
+		(cpurev & 0x0000F) >> 0,
+		mxc_get_clock(MXC_ARM_CLK) / 1000000);
+	printf("Reset cause: %s\n", get_reset_cause());
+	return 0;
+}
+#endif
+
+int cpu_eth_init(bd_t *bis)
+{
+	int rc = -ENODEV;
+
+#if defined(CONFIG_FEC_MXC)
+	rc = fecmxc_initialize(bis);
+#endif
+
+	return rc;
+}
+
+/*
+ * Initializes on-chip MMC controllers.
+ * to override, implement board_mmc_init()
+ */
+int cpu_mmc_init(bd_t *bis)
+{
+#ifdef CONFIG_FSL_ESDHC
+	return fsl_esdhc_mmc_init(bis);
+#else
+	return 0;
+#endif
+}
+
+void reset_cpu(ulong addr)
+{
+	__raw_writew(4, WDOG1_BASE_ADDR);
+}
diff --git a/arch/arm/cpu/armv7/mx5/speed.c b/arch/arm/cpu/armv7/imx-common/speed.c
similarity index 100%
rename from arch/arm/cpu/armv7/mx5/speed.c
rename to arch/arm/cpu/armv7/imx-common/speed.c
diff --git a/arch/arm/cpu/armv7/mx5/timer.c b/arch/arm/cpu/armv7/imx-common/timer.c
old mode 100644
new mode 100755
similarity index 84%
rename from arch/arm/cpu/armv7/mx5/timer.c
rename to arch/arm/cpu/armv7/imx-common/timer.c
index 2544b08..98e9f4a
--- a/arch/arm/cpu/armv7/mx5/timer.c
+++ b/arch/arm/cpu/armv7/imx-common/timer.c
@@ -39,10 +39,11 @@  struct mxc_gpt {
 static struct mxc_gpt *cur_gpt = (struct mxc_gpt *)GPT1_BASE_ADDR;
 
 /* General purpose timers bitfields */
-#define GPTCR_SWR       (1<<15)	/* Software reset */
-#define GPTCR_FRR       (1<<9)	/* Freerun / restart */
-#define GPTCR_CLKSOURCE_32 (4<<6)	/* Clock source */
-#define GPTCR_TEN       (1)	/* Timer enable */
+#define GPTCR_SWR		(1 << 15)	/* Software reset */
+#define GPTCR_FRR		(1 << 9)	/* Freerun / restart */
+#define GPTCR_CLKSOURCE_32	(4 << 6)	/* Clock source */
+#define GPTCR_TEN		1		/* Timer enable */
+#define CLK_32KHZ		32768		/* 32Khz input */
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -68,7 +69,7 @@  int timer_init(void)
 	__raw_writel(i | GPTCR_CLKSOURCE_32 | GPTCR_TEN, &cur_gpt->control);
 
 	val = __raw_readl(&cur_gpt->counter);
-	lastinc = val / (CONFIG_SYS_MX5_CLK32 / CONFIG_SYS_HZ);
+	lastinc = val / (CLK_32KHZ / CONFIG_SYS_HZ);
 	timestamp = 0;
 
 	return 0;
@@ -77,11 +78,11 @@  int timer_init(void)
 ulong get_timer_masked(void)
 {
 	ulong val = __raw_readl(&cur_gpt->counter);
-	val /= (CONFIG_SYS_MX5_CLK32 / CONFIG_SYS_HZ);
+	val /= (CLK_32KHZ / CONFIG_SYS_HZ);
 	if (val >= lastinc)
 		timestamp += (val - lastinc);
 	else
-		timestamp += ((0xFFFFFFFF / (CONFIG_SYS_MX5_CLK32 / CONFIG_SYS_HZ))
+		timestamp += ((0xFFFFFFFF / (CLK_32KHZ / CONFIG_SYS_HZ))
 				- lastinc) + val;
 	lastinc = val;
 	return timestamp;
@@ -96,7 +97,7 @@  ulong get_timer(ulong base)
 void __udelay(unsigned long usec)
 {
 	unsigned long now, start, tmo;
-	tmo = usec * (CONFIG_SYS_MX5_CLK32 / 1000) / 1000;
+	tmo = usec * (CLK_32KHZ / 1000) / 1000;
 
 	if (!tmo)
 		tmo = 1;
diff --git a/arch/arm/cpu/armv7/mx5/Makefile b/arch/arm/cpu/armv7/mx5/Makefile
index e8be9c9..ecd1184 100644
--- a/arch/arm/cpu/armv7/mx5/Makefile
+++ b/arch/arm/cpu/armv7/mx5/Makefile
@@ -27,7 +27,7 @@  include $(TOPDIR)/config.mk
 
 LIB	= $(obj)lib$(SOC).o
 
-COBJS	= soc.o clock.o iomux.o timer.o speed.o
+COBJS	= soc.o clock.o iomux.o
 SOBJS = lowlevel_init.o
 
 SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
diff --git a/arch/arm/cpu/armv7/mx5/soc.c b/arch/arm/cpu/armv7/mx5/soc.c
index cf12ba8..1533dd8 100644
--- a/arch/arm/cpu/armv7/mx5/soc.c
+++ b/arch/arm/cpu/armv7/mx5/soc.c
@@ -31,10 +31,6 @@ 
 #include <asm/errno.h>
 #include <asm/io.h>
 
-#ifdef CONFIG_FSL_ESDHC
-#include <fsl_esdhc.h>
-#endif
-
 #if !(defined(CONFIG_MX51) || defined(CONFIG_MX53))
 #error "CPU_TYPE not defined"
 #endif
@@ -75,61 +71,6 @@  u32 get_cpu_rev(void)
 	return system_rev;
 }
 
-static char *get_reset_cause(void)
-{
-	u32 cause;
-	struct src *src_regs = (struct src *)SRC_BASE_ADDR;
-
-	cause = readl(&src_regs->srsr);
-	writel(cause, &src_regs->srsr);
-
-	switch (cause) {
-	case 0x00001:
-		return "POR";
-	case 0x00004:
-		return "CSU";
-	case 0x00008:
-		return "IPP USER";
-	case 0x00010:
-		return "WDOG";
-	case 0x00020:
-		return "JTAG HIGH-Z";
-	case 0x00040:
-		return "JTAG SW";
-	case 0x10000:
-		return "WARM BOOT";
-	default:
-		return "unknown reset";
-	}
-}
-
-#if defined(CONFIG_DISPLAY_CPUINFO)
-int print_cpuinfo(void)
-{
-	u32 cpurev;
-
-	cpurev = get_cpu_rev();
-	printf("CPU:   Freescale i.MX%x family rev%d.%d at %d MHz\n",
-		(cpurev & 0xFF000) >> 12,
-		(cpurev & 0x000F0) >> 4,
-		(cpurev & 0x0000F) >> 0,
-		mxc_get_clock(MXC_ARM_CLK) / 1000000);
-	printf("Reset cause: %s\n", get_reset_cause());
-	return 0;
-}
-#endif
-
-int cpu_eth_init(bd_t *bis)
-{
-	int rc = -ENODEV;
-
-#if defined(CONFIG_FEC_MXC)
-	rc = fecmxc_initialize(bis);
-#endif
-
-	return rc;
-}
-
 #if defined(CONFIG_FEC_MXC)
 void imx_get_mac_from_fuse(unsigned char *mac)
 {
@@ -144,19 +85,6 @@  void imx_get_mac_from_fuse(unsigned char *mac)
 }
 #endif
 
-/*
- * Initializes on-chip MMC controllers.
- * to override, implement board_mmc_init()
- */
-int cpu_mmc_init(bd_t *bis)
-{
-#ifdef CONFIG_FSL_ESDHC
-	return fsl_esdhc_mmc_init(bis);
-#else
-	return 0;
-#endif
-}
-
 void set_chipselect_size(int const cs_size)
 {
 	unsigned int reg;
@@ -187,8 +115,3 @@  void set_chipselect_size(int const cs_size)
 
 	writel(reg, &iomuxc_regs->gpr1);
 }
-
-void reset_cpu(ulong addr)
-{
-	__raw_writew(4, WDOG1_BASE_ADDR);
-}