Re: [U-Boot] [PATCH 1/7] Add support for MX35 processor
Hi Wolfgang, What do these ! markers mean? They have no useful meaning and I must drop them. There are other such cryptic markers - eventually from some doc generating tool. Correct - I'm pretty sure this was intended for doxygen. Cheers Detlev -- Some people seem to think that C is a real programming language, but they are sadly mistaken. It really is about writing almost-portable assembly language [...] -- Linus Torvalds 10404265599082718160nore...@blogger.com -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/7] Add support for MX35 processor
On 01/19/2011 08:35 AM, Wolfgang Denk wrote:
Dear Stefano Babic,
+if (!reg) {
+reg = __REG(ROMPATCH_REV);
__REG()?
It should not be..
NAK. Please use I/O accessors. Please fix globally.
I supposed to have already replaced all of them, but I have missed some
of them.
+void imx_get_mac_from_fuse(unsigned char *mac)
+{
+int i;
+
+for (i = 0; i 6; i++)
+mac[i] = 0;
memset(mac, 0, 6);
?
I think your question mark is related to the fact that it is not so
clear why I set the mac address to all zero. At first glance, it makes
no sense.
However, the reason is that the i.MX35 does not have a MAC address
stored in its internal fuses as other i.MX processors
(i.MX27/i.MX25/i.MX51), but I want to expose the same interface for all
i.MX processors. In this way, I can avoid nasty #ifdef in code.
As I can see in u-boot code, the exception now in code is the i.MX31,
that was the first i.MX processor added to mainline. It has still a lot
of own functions (I mean, something like mx31_*, such as mx31_gpio.
mx31_iomux, and so on).
What do these ! markers mean?
They have no useful meaning and I must drop them.
+/*
+ * This function is used to configure a pin through the IOMUX module.
+ * FIXED ME: for backward compatible. Will be static function!
+ * @param pin a pin number as defined in \b #iomux_pin_name_t
+ * @param cfg an output function as defined in \b
#iomux_pin_cfg_t
+ *
+ * @return 0 if successful; Non-zero otherwise
When does the otherwise happen?
+ */
+static int iomux_config_mux(iomux_pin_name_t pin, iomux_pin_cfg_t cfg)
+{
+u32 mux_reg = PIN_TO_IOMUX_MUX(pin);
+
+if (mux_reg != NON_MUX_I) {
+mux_reg += IOMUXGPR;
+__REG(mux_reg) = cfg;
+}
+
+return 0;
+}
Should we make this function return void ?
iomux_config_mux is already void for mx5 processors. Of course, it must
return void for MX35, too. And mxc_request_mux as well.
I will check all these functions to remove inconsistencies.
+/* delay x useconds AND perserve advance timstamp value */
+/* GPTCNT is now supposed to tick 1 by 1 us. */
s/perserve/preserve/ ?
Incorrect multiline comment style.
Thanks, I will fix it.
diff --git a/arch/arm/include/asm/arch-mx35/imx-regs.h
b/arch/arm/include/asm/arch-mx35/imx-regs.h
new file mode 100644
index 000..f382960
--- /dev/null
+++ b/arch/arm/include/asm/arch-mx35/imx-regs.h
...
+#define __REG(x) (*((volatile u32 *)(x)))
+#define __REG16(x) (*((volatile u16 *)(x)))
+#define __REG8(x)(*((volatile u8 *)(x)))
NAK!! Please use I/O accessors. Please fix globally.
Of course - I will fix it.
+/* CCM */
+#define CLKCTL_CCMR 0x00
+#define CLKCTL_PDR0 0x04
+#define CLKCTL_PDR1 0x08
+#define CLKCTL_PDR2 0x0C
+#define CLKCTL_PDR3 0x10
+#define CLKCTL_PDR4 0x14
+#define CLKCTL_RCSR 0x18
+#define CLKCTL_MPCTL0x1C
+#define CLKCTL_PPCTL0x20
+#define CLKCTL_ACMR 0x24
+#define CLKCTL_COSR 0x28
+#define CLKCTL_CGR0 0x2C
+#define CLKCTL_CGR1 0x30
+#define CLKCTL_CGR2 0x34
+#define CLKCTL_CGR3 0x38
NAK!! Please use C struct. Please fix globally.
No, this not. I have already set a structure for the Clock Module, that
it is used in most part of code (ccm_regs). However, these offsets are
used in the assembly file (lowlevel_init.S), and I cannot use the
structure. It seems to me the only structure needed, and it seems to me
not necessary to add an asm-offsets.h only for it. For this reason. I
let it in imx-regs.h
+extern unsigned int get_board_rev(void);
+extern int is_soc_rev(int rev);
+extern int sdhc_init(void);
+
+#define fixup_before_linux \
+{ \
+volatile unsigned long *l2cc_ctl = (unsigned long *)0x3100;\
0x3100 ? Don't we have a #define for it?
I have already removed the usage of this macro, I forget to drop its
define. Thanks, I will remove it.
Please fix these license headers - all of these. They say where you
can _get_ License Version 2 or later, but they don't say which version
actually applies. Please make clear that License v2+ applies.
I will check this issue globally.
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 1/7] Add support for MX35 processor
Dear Stefano Babic,
In message 4d36b2af.9000...@denx.de you wrote:
+void imx_get_mac_from_fuse(unsigned char *mac)
+{
+ int i;
+
+ for (i = 0; i 6; i++)
+ mac[i] = 0;
memset(mac, 0, 6);
?
I think your question mark is related to the fact that it is not so
clear why I set the mac address to all zero. At first glance, it makes
no sense.
No, I wonder why code a loop when a simple memset() does the same.
What do these ! markers mean?
They have no useful meaning and I must drop them.
There are other such cryptic markers - eventually from some doc
generating tool.
+/* CCM */
+#define CLKCTL_CCMR 0x00
+#define CLKCTL_PDR0 0x04
+#define CLKCTL_PDR1 0x08
+#define CLKCTL_PDR2 0x0C
+#define CLKCTL_PDR3 0x10
+#define CLKCTL_PDR4 0x14
+#define CLKCTL_RCSR 0x18
+#define CLKCTL_MPCTL 0x1C
+#define CLKCTL_PPCTL 0x20
+#define CLKCTL_ACMR 0x24
+#define CLKCTL_COSR 0x28
+#define CLKCTL_CGR0 0x2C
+#define CLKCTL_CGR1 0x30
+#define CLKCTL_CGR2 0x34
+#define CLKCTL_CGR3 0x38
NAK!! Please use C struct. Please fix globally.
No, this not. I have already set a structure for the Clock Module, that
If you already have a struct, then drop these duplication.
it is used in most part of code (ccm_regs). However, these offsets are
used in the assembly file (lowlevel_init.S), and I cannot use the
structure. It seems to me the only structure needed, and it seems to me
not necessary to add an asm-offsets.h only for it. For this reason. I
let it in imx-regs.h
Such duplication of information is just a source of errors. And
asm-offsets.h has been provided exactly for the purpose to avoid such
#defines. Please use it.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Those who hate and fight must stop themselves -- otherwise it is not
stopped.
-- Spock, Day of the Dove, stardate unknown
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/7] Add support for MX35 processor
On 01/19/2011 12:37 PM, Wolfgang Denk wrote: No, I wonder why code a loop when a simple memset() does the same. Ok, I get the point - you are right, I will replace the code with memset. What do these ! markers mean? They have no useful meaning and I must drop them. There are other such cryptic markers - eventually from some doc generating tool. I will check in all patches, and I will remove them. Such duplication of information is just a source of errors. And asm-offsets.h has been provided exactly for the purpose to avoid such #defines. Please use it. Ok, understood. I will move these defines in asm-offsets.h 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 1/7] Add support for MX35 processor
Dear Stefano Babic,
In message 1295012124-15551-1-git-send-email-sba...@denx.de you wrote:
The patch adds basic support for the Freescale's i.MX35
(arm1136 based) processor.
Signed-off-by: Stefano Babic sba...@denx.de
...
+u32 get_cpu_rev(void)
+{
+ int reg;
+ struct iim_regs *iim =
+ (struct iim_regs *)IIM_BASE_ADDR;
+ reg = readl(iim-iim_srev);
+ if (!reg) {
+ reg = __REG(ROMPATCH_REV);
__REG()?
NAK. Please use I/O accessors. Please fix globally.
+void imx_get_mac_from_fuse(unsigned char *mac)
+{
+ int i;
+
+ for (i = 0; i 6; i++)
+ mac[i] = 0;
memset(mac, 0, 6);
?
+/*
+ * IOMUX register (base) addresses
+ */
+enum iomux_reg_addr {
+ IOMUXGPR = IOMUXC_BASE_ADDR,
+ /*! General purpose */
+ IOMUXSW_MUX_CTL = IOMUXC_BASE_ADDR + 4,
+ /*! MUX control */
+ IOMUXSW_MUX_END = IOMUXC_BASE_ADDR + 0x324,
+ /*! last MUX control register */
+ IOMUXSW_PAD_CTL = IOMUXC_BASE_ADDR + 0x328,
+ /*! Pad control */
+ IOMUXSW_PAD_END = IOMUXC_BASE_ADDR + 0x794,
+ /*! last Pad control register */
+ IOMUXSW_INPUT_CTL = IOMUXC_BASE_ADDR + 0x7AC,
+ /*! input select register */
+ IOMUXSW_INPUT_END = IOMUXC_BASE_ADDR + 0x9F4,
+ /*! last input select register */
+};
What do these ! markers mean?
+/*
+ * This function is used to configure a pin through the IOMUX module.
+ * FIXED ME: for backward compatible. Will be static function!
+ * @param pin a pin number as defined in \b #iomux_pin_name_t
+ * @param cfg an output function as defined in \b
#iomux_pin_cfg_t
+ *
+ * @return 0 if successful; Non-zero otherwise
When does the otherwise happen?
+ */
+static int iomux_config_mux(iomux_pin_name_t pin, iomux_pin_cfg_t cfg)
+{
+ u32 mux_reg = PIN_TO_IOMUX_MUX(pin);
+
+ if (mux_reg != NON_MUX_I) {
+ mux_reg += IOMUXGPR;
+ __REG(mux_reg) = cfg;
+ }
+
+ return 0;
+}
Should we make this function return void ?
+/* delay x useconds AND perserve advance timstamp value */
+/* GPTCNT is now supposed to tick 1 by 1 us. */
s/perserve/preserve/ ?
Incorrect multiline comment style.
diff --git a/arch/arm/include/asm/arch-mx35/imx-regs.h
b/arch/arm/include/asm/arch-mx35/imx-regs.h
new file mode 100644
index 000..f382960
--- /dev/null
+++ b/arch/arm/include/asm/arch-mx35/imx-regs.h
...
+#define __REG(x) (*((volatile u32 *)(x)))
+#define __REG16(x) (*((volatile u16 *)(x)))
+#define __REG8(x)(*((volatile u8 *)(x)))
NAK!! Please use I/O accessors. Please fix globally.
+/* CCM */
+#define CLKCTL_CCMR 0x00
+#define CLKCTL_PDR0 0x04
+#define CLKCTL_PDR1 0x08
+#define CLKCTL_PDR2 0x0C
+#define CLKCTL_PDR3 0x10
+#define CLKCTL_PDR4 0x14
+#define CLKCTL_RCSR 0x18
+#define CLKCTL_MPCTL 0x1C
+#define CLKCTL_PPCTL 0x20
+#define CLKCTL_ACMR 0x24
+#define CLKCTL_COSR 0x28
+#define CLKCTL_CGR0 0x2C
+#define CLKCTL_CGR1 0x30
+#define CLKCTL_CGR2 0x34
+#define CLKCTL_CGR3 0x38
NAK!! Please use C struct. Please fix globally.
+#if 0
+extern unsigned int mxc_get_clock(enum mxc_clock clk);
+#endif
Please remove such dead code. Please fix globally.
+extern unsigned int get_board_rev(void);
+extern int is_soc_rev(int rev);
+extern int sdhc_init(void);
+
+#define fixup_before_linux \
+ { \
+ volatile unsigned long *l2cc_ctl = (unsigned long *)0x3100;\
0x3100 ? Don't we have a #define for it?
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
Please fix these license headers - all of these. They say where you
can _get_ License Version 2 or later, but they don't say which version
actually applies. Please make clear that License v2+ applies.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
By the way, ALL software projects are done by iterative prototyping.
Some companies call their prototypes releases, that's all.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
