Re: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
On 09/08/2011 10:42 PM, Marek Vasut wrote:
This patch supports:
- Timers
- Debug UART
- Clock
Hi Marek,
a general remark. It seems to me that your patchset comes directly from
your development, as it looks like what I normally have when I develop a
new board ;-)
However, to put into mainline and to make review easier, because this is
a porting to a new SOC, I am expecting that all patches related to a new
file are squashed together. It makes no sense to have several patches
regarding, for example, m28evk.h, because this is a new file.
Another general remark here: we tend to have the same structure and the
same files for all IMX SOC. This means that all IMX SOC have a
imx-regs.h that contain the required register definitions (really a
subset what we have in kernel). Is it really necessary to split the
definitions in several small files ?
Signed-off-by: Marek Vasut marek.va...@gmail.com
Cc: Stefano Babic sba...@denx.de
Cc: Wolfgang Denk w...@denx.de
Cc: Detlev Zundel d...@denx.de
---
arch/arm/cpu/arm926ejs/mx28/Makefile | 46 +++
arch/arm/cpu/arm926ejs/mx28/clock.c | 359 ++
arch/arm/cpu/arm926ejs/mx28/mx28.c| 131
arch/arm/cpu/arm926ejs/mx28/timer.c | 143 +
arch/arm/include/asm/arch-mx28/clock.h| 48 +++
arch/arm/include/asm/arch-mx28/imx-regs.h | 33 ++
arch/arm/include/asm/arch-mx28/mx28.h | 30 ++
arch/arm/include/asm/arch-mx28/regs-base.h| 88 ++
arch/arm/include/asm/arch-mx28/regs-clkctrl.h | 308 +++
arch/arm/include/asm/arch-mx28/regs-common.h | 66
arch/arm/include/asm/arch-mx28/regs-power.h | 409
+
arch/arm/include/asm/arch-mx28/regs-ssp.h | 345 +
arch/arm/include/asm/arch-mx28/regs-timrot.h | 167 ++
arch/arm/include/asm/arch-mx28/regs-uartdbg.h | 182 +++
14 files changed, 2355 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/cpu/arm926ejs/mx28/Makefile
create mode 100644 arch/arm/cpu/arm926ejs/mx28/clock.c
create mode 100644 arch/arm/cpu/arm926ejs/mx28/mx28.c
create mode 100644 arch/arm/cpu/arm926ejs/mx28/timer.c
create mode 100644 arch/arm/include/asm/arch-mx28/clock.h
create mode 100644 arch/arm/include/asm/arch-mx28/imx-regs.h
create mode 100644 arch/arm/include/asm/arch-mx28/mx28.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-base.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-clkctrl.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-common.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-power.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-ssp.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-timrot.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-uartdbg.h
regs-uartdbg.h is used only by the driver, right ? The driver should be
in the driver directory, and also its header. See drivers/serial/
+
+#include common.h
+#include asm/errno.h
+#include asm/io.h
+#include asm/arch/clock.h
+#include asm/arch/regs-common.h
+#include asm/arch/regs-base.h
+#include asm/arch/regs-clkctrl.h
+#include asm/arch/regs-ssp.h
Again, regarding file splitting for register. If you prefer to have
several files, I think it is better that imx-regs.h include them. Then
we have still a common header imx-regs.h that contains all needed
register definitions. This is a better interface for a board maintainer.
+
+/* The PLL frequency is always 480MHz, see section 10.2 in iMX28 datasheet.
*/
+#define PLL_FREQ_KHZ48
+#define PLL_FREQ_COEF 18
+/* The XTAL frequency is always 24MHz, see section 10.2 in iMX28 datasheet.
*/
+#define XTAL_FREQ_KHZ 24000
+
+#define PLL_FREQ_MHZ(PLL_FREQ_KHZ / 1000)
+#define XTAL_FREQ_MHZ (XTAL_FREQ_KHZ / 1000)
+
+uint32_t mx28_get_pclk(void)
This function must be static.
+
+uint32_t mx28_get_hclk(void)
Ditto. All *_get_some_clk are exported with mxc_get_clk(index_number)
+
+uint32_t mx28_get_emiclk(void)
Ditto
+static inline void enable_gpmi_clk(void)
+{
Is there a reason to have inline ? And this is called only once. Do we
have a enable_ without a disable_ function ? Your mx28_get_gpmiclk()
enables without the caller knows the gpmi clock. Strange for a pure
get function.
+
+uint32_t mx28_get_gpmiclk(void)
Also static.
+/*
+ * Set IO clock frequency, in kHz
+ */
+void mx28_set_ioclk(unsigned int io, uint32_t freq)
+{
Please use an enum for io - it cannot assume any integer value,
something with GATE0 and GATE1 to be the call easier to understand.
+
+/*
+ * Get IO clock, returns IO clock in kHz
+ */
+uint32_t mx28_get_ioclk(unsigned int io)
Static function. Do you think to export this function ? In this case,
you need to add enums for mxc_get_clk().
+
+/*
+ * Configure SSP clock frequency, in kHz
+ */
+void mx28_set_sspclk(unsigned int ssp,
Re: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
Hello Stefano, Stefano Babic wrote: On 09/08/2011 10:42 PM, Marek Vasut wrote: This patch supports: - Timers - Debug UART - Clock Hi Marek, a general remark. It seems to me that your patchset comes directly from your development, as it looks like what I normally have when I develop a new board ;-) I also tend to do such things ;-) However, to put into mainline and to make review easier, because this is a porting to a new SOC, I am expecting that all patches related to a new file are squashed together. It makes no sense to have several patches regarding, for example, m28evk.h, because this is a new file. Yep, for new files, it makes sense to make only one patch. Another general remark here: we tend to have the same structure and the same files for all IMX SOC. This means that all IMX SOC have a imx-regs.h that contain the required register definitions (really a subset what we have in kernel). Is it really necessary to split the definitions in several small files ? Hmm... is this really a good thing? If I look in the statistic below, this results in a big (lines 1000) file ... I don;t like such big files, but this is just a personal taste ... Signed-off-by: Marek Vasut marek.va...@gmail.com Cc: Stefano Babic sba...@denx.de Cc: Wolfgang Denk w...@denx.de Cc: Detlev Zundel d...@denx.de --- arch/arm/cpu/arm926ejs/mx28/Makefile | 46 +++ arch/arm/cpu/arm926ejs/mx28/clock.c | 359 ++ arch/arm/cpu/arm926ejs/mx28/mx28.c| 131 arch/arm/cpu/arm926ejs/mx28/timer.c | 143 + arch/arm/include/asm/arch-mx28/clock.h| 48 +++ arch/arm/include/asm/arch-mx28/imx-regs.h | 33 ++ arch/arm/include/asm/arch-mx28/mx28.h | 30 ++ arch/arm/include/asm/arch-mx28/regs-base.h| 88 ++ arch/arm/include/asm/arch-mx28/regs-clkctrl.h | 308 +++ arch/arm/include/asm/arch-mx28/regs-common.h | 66 arch/arm/include/asm/arch-mx28/regs-power.h | 409 + arch/arm/include/asm/arch-mx28/regs-ssp.h | 345 + arch/arm/include/asm/arch-mx28/regs-timrot.h | 167 ++ arch/arm/include/asm/arch-mx28/regs-uartdbg.h | 182 +++ bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
On Friday, September 09, 2011 10:23:09 AM Stefano Babic wrote:
On 09/08/2011 10:42 PM, Marek Vasut wrote:
This patch supports:
- Timers
- Debug UART
- Clock
Hi Marek,
a general remark. It seems to me that your patchset comes directly from
your development, as it looks like what I normally have when I develop a
new board ;-)
However, to put into mainline and to make review easier, because this is
a porting to a new SOC, I am expecting that all patches related to a new
file are squashed together. It makes no sense to have several patches
regarding, for example, m28evk.h, because this is a new file.
Done, I'll retest and send V2 of the series
Another general remark here: we tend to have the same structure and the
same files for all IMX SOC. This means that all IMX SOC have a
imx-regs.h that contain the required register definitions (really a
subset what we have in kernel). Is it really necessary to split the
definitions in several small files ?
Just like Heiko said ... it'd produce terribly big and messy file. I'd prefer
to
avoid that.
Signed-off-by: Marek Vasut marek.va...@gmail.com
Cc: Stefano Babic sba...@denx.de
Cc: Wolfgang Denk w...@denx.de
Cc: Detlev Zundel d...@denx.de
---
arch/arm/cpu/arm926ejs/mx28/Makefile | 46 +++
arch/arm/cpu/arm926ejs/mx28/clock.c | 359
++ arch/arm/cpu/arm926ejs/mx28/mx28.c|
131
arch/arm/cpu/arm926ejs/mx28/timer.c | 143 +
arch/arm/include/asm/arch-mx28/clock.h| 48 +++
arch/arm/include/asm/arch-mx28/imx-regs.h | 33 ++
arch/arm/include/asm/arch-mx28/mx28.h | 30 ++
arch/arm/include/asm/arch-mx28/regs-base.h| 88 ++
arch/arm/include/asm/arch-mx28/regs-clkctrl.h | 308 +++
arch/arm/include/asm/arch-mx28/regs-common.h | 66
arch/arm/include/asm/arch-mx28/regs-power.h | 409
+ arch/arm/include/asm/arch-mx28/regs-ssp.h
| 345 +
arch/arm/include/asm/arch-mx28/regs-timrot.h | 167 ++
arch/arm/include/asm/arch-mx28/regs-uartdbg.h | 182 +++ 14
files changed, 2355 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/cpu/arm926ejs/mx28/Makefile
create mode 100644 arch/arm/cpu/arm926ejs/mx28/clock.c
create mode 100644 arch/arm/cpu/arm926ejs/mx28/mx28.c
create mode 100644 arch/arm/cpu/arm926ejs/mx28/timer.c
create mode 100644 arch/arm/include/asm/arch-mx28/clock.h
create mode 100644 arch/arm/include/asm/arch-mx28/imx-regs.h
create mode 100644 arch/arm/include/asm/arch-mx28/mx28.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-base.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-clkctrl.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-common.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-power.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-ssp.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-timrot.h
create mode 100644 arch/arm/include/asm/arch-mx28/regs-uartdbg.h
regs-uartdbg.h is used only by the driver, right ? The driver should be
in the driver directory, and also its header. See drivers/serial/
Honestly, I need to check if the file is used at all. We replaced the original
driver with pl011 driver.
+
+#include common.h
+#include asm/errno.h
+#include asm/io.h
+#include asm/arch/clock.h
+#include asm/arch/regs-common.h
+#include asm/arch/regs-base.h
+#include asm/arch/regs-clkctrl.h
+#include asm/arch/regs-ssp.h
Again, regarding file splitting for register. If you prefer to have
several files, I think it is better that imx-regs.h include them. Then
we have still a common header imx-regs.h that contains all needed
register definitions. This is a better interface for a board maintainer.
Won't it make the compiler slower at compile time if it has to go through all
of
the included files instead of a subset ?
+
+/* The PLL frequency is always 480MHz, see section 10.2 in iMX28
datasheet. */ +#define PLL_FREQ_KHZ48
+#definePLL_FREQ_COEF 18
+/* The XTAL frequency is always 24MHz, see section 10.2 in iMX28
datasheet. */ +#define XTAL_FREQ_KHZ 24000
+
+#definePLL_FREQ_MHZ(PLL_FREQ_KHZ / 1000)
+#defineXTAL_FREQ_MHZ (XTAL_FREQ_KHZ / 1000)
+
+uint32_t mx28_get_pclk(void)
This function must be static.
[...]
Fixed all the stuff
+/* Lowlevel init isn't used on i.MX28, so just have a dummy here */
+inline void lowlevel_init(void) {}
Ok.
+
+void reset_cpu(ulong ignored) __attribute__((noreturn));
+
+void reset_cpu(ulong ignored)
+{
+ struct mx28_clkctrl_regs *clkctrl_regs =
+ (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
+ struct mx28_power_regs *power_regs =
+ (struct mx28_power_regs *)MXS_POWER_BASE;
+
+ writel(0, power_regs-hw_power_charge);
Re: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
On 09/09/2011 12:12 PM, Marek Vasut wrote: Another general remark here: we tend to have the same structure and the same files for all IMX SOC. This means that all IMX SOC have a imx-regs.h that contain the required register definitions (really a subset what we have in kernel). Is it really necessary to split the definitions in several small files ? Just like Heiko said ... it'd produce terribly big and messy file. I'd prefer to avoid that. This is ok, and it is ok also that your imx-regs.h file contains only the reg-specific include files. What is not ok, is that you do not use imx-regs.h and you still includes each separate file. And as I see, they must be included in a specific order. This order must be defined only in imx-regs.h, as you have already done and then each driver/board requires to include only imx-regs.h, without having to care of the single files. Honestly, I need to check if the file is used at all. We replaced the original driver with pl011 driver. Ok Again, regarding file splitting for register. If you prefer to have several files, I think it is better that imx-regs.h include them. Then we have still a common header imx-regs.h that contains all needed register definitions. This is a better interface for a board maintainer. Won't it make the compiler slower at compile time if it has to go through all of the included files instead of a subset ? Well, I do not know if on our Intel-PC this makes a so noticeable difference ;-) What I remark here, it is to have a clear and identical interface among the several SOCs. This is not only easier for board developers, but also removes some nasty #ifdef CONFIG_MX from the drivers. To understand: does a reset mean on the i.MX28 a power off ? Do you turn off the power ? If this is the case, some features are not possible (as PRAM, for example) on this SOC. This should just reset the chip. No poweroff. Ok - thanks for clarification. I'll probably wait for someone to clearly say how this should be to avoid reworking it for the fourth time. Right. 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: off...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
On Friday, September 09, 2011 12:29:01 PM Stefano Babic wrote: On 09/09/2011 12:12 PM, Marek Vasut wrote: Another general remark here: we tend to have the same structure and the same files for all IMX SOC. This means that all IMX SOC have a imx-regs.h that contain the required register definitions (really a subset what we have in kernel). Is it really necessary to split the definitions in several small files ? Just like Heiko said ... it'd produce terribly big and messy file. I'd prefer to avoid that. This is ok, and it is ok also that your imx-regs.h file contains only the reg-specific include files. What is not ok, is that you do not use imx-regs.h and you still includes each separate file. And as I see, they must be included in a specific order. I patched the reg definition files. This should be solved. This order must be defined only in imx-regs.h, as you have already done and then each driver/board requires to include only imx-regs.h, without having to care of the single files. Ok Honestly, I need to check if the file is used at all. We replaced the original driver with pl011 driver. Ok Again, regarding file splitting for register. If you prefer to have several files, I think it is better that imx-regs.h include them. Then we have still a common header imx-regs.h that contains all needed register definitions. This is a better interface for a board maintainer. Won't it make the compiler slower at compile time if it has to go through all of the included files instead of a subset ? Well, I do not know if on our Intel-PC this makes a so noticeable difference ;-) It's a point-of-view question, really. But what about people compiling stuff on slower machines ? What I remark here, it is to have a clear and identical interface among the several SOCs. This is not only easier for board developers, but also removes some nasty #ifdef CONFIG_MX from the drivers. Yes, I understand. btw there's no CONFIG_MX stuff. To understand: does a reset mean on the i.MX28 a power off ? Do you turn off the power ? If this is the case, some features are not possible (as PRAM, for example) on this SOC. This should just reset the chip. No poweroff. Ok - thanks for clarification. I'll probably wait for someone to clearly say how this should be to avoid reworking it for the fourth time. Right. Best regards, Stefano Babic Cheers ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
On 09/09/2011 12:50 PM, Marek Vasut wrote: Well, I do not know if on our Intel-PC this makes a so noticeable difference ;-) It's a point-of-view question, really. But what about people compiling stuff on slower machines ? Yes, it is a point of view, and surely, compilation time is a drawback. However, if I compare the advantages between clear interface and compilation time, I choose the first one. What I remark here, it is to have a clear and identical interface among the several SOCs. This is not only easier for board developers, but also removes some nasty #ifdef CONFIG_MX from the drivers. Yes, I understand. btw there's no CONFIG_MX stuff. Not yet, but only because you are adding a new SOC without using IMX drivers. Things will be changed when the MX23 will be added. Or some newer SOC of the mxs family will be added by Freescale. Once we had only the MX31. Drivers were done only for this SOC. After using the same driver with different SOCs, a lot of CONFIG_MX was added, and most of code was really not readable - and it tooks time to remove most of them. 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: off...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
