* [U-Boot] [PATCH] mx25: Clean up lowlevel_init
@ 2012-08-14 21:25 Benoît Thébaudeau
2012-08-15 6:27 ` Matthias Weißer
0 siblings, 1 reply; 10+ messages in thread
From: Benoît Thébaudeau @ 2012-08-14 21:25 UTC (permalink / raw)
To: u-boot
Clean up mx25 lowlevel_init:
- Add comments.
- Do not use write32 repeatedly with the same value in order no to increase
code size.
- Make register values configurable.
- Use defined values instead of literal constants.
- Use defined macros instead of duplicating code.
Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
Cc: Stefano Babic <sbabic@denx.de>
---
.../arch/arm/include/asm/arch-mx25/macro.h | 57 +++++++++++++-------
.../board/karo/tx25/lowlevel_init.S | 37 +++----------
.../board/syteco/zmx25/lowlevel_init.S | 6 +++
3 files changed, 51 insertions(+), 49 deletions(-)
diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
index 3b694da..9550ade 100644
--- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
+++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
@@ -32,32 +32,49 @@
#include <asm/arch/imx-regs.h>
#include <generated/asm-offsets.h>
+#include <asm/macro.h>
+/*
+ * AIPS setup - Only setup MPROTx registers.
+ * The PACR default values are good.
+ */
.macro init_aips
- write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
- write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
- write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
- write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
+ ldr r0, =IMX_AIPS1_BASE
+ ldr r1, =AIPS_MPR_CONFIG
+ str r1, [r0, #AIPS_MPR_0_7]
+ str r1, [r0, #AIPS_MPR_8_15]
+ ldr r2, =IMX_AIPS2_BASE
+ str r1, [r2, #AIPS_MPR_0_7]
+ str r1, [r2, #AIPS_MPR_8_15]
.endm
+/* MAX (Multi-Layer AHB Crossbar Switch) setup */
.macro init_max
- write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
-
- write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
+ ldr r0, =IMX_MAX_BASE
+ ldr r1, =MAX_MPR_CONFIG
+ str r1, [r0, #MAX_MPR0] /* for S0 */
+ str r1, [r0, #MAX_MPR1] /* for S1 */
+ str r1, [r0, #MAX_MPR2] /* for S2 */
+ str r1, [r0, #MAX_MPR3] /* for S3 */
+ str r1, [r0, #MAX_MPR4] /* for S4 */
+ ldr r1, =MAX_SGPCR_CONFIG
+ str r1, [r0, #MAX_SGPCR0] /* for S0 */
+ str r1, [r0, #MAX_SGPCR1] /* for S1 */
+ str r1, [r0, #MAX_SGPCR2] /* for S2 */
+ str r1, [r0, #MAX_SGPCR3] /* for S3 */
+ str r1, [r0, #MAX_SGPCR4] /* for S4 */
+ ldr r1, =MAX_MGPCR_CONFIG
+ str r1, [r0, #MAX_MGPCR0] /* for M0 */
+ str r1, [r0, #MAX_MGPCR1] /* for M1 */
+ str r1, [r0, #MAX_MGPCR2] /* for M2 */
+ str r1, [r0, #MAX_MGPCR3] /* for M3 */
+ str r1, [r0, #MAX_MGPCR4] /* for M4 */
+.endm
- write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
+/* M3IF setup */
+.macro init_m3if
+ /* M3IF Control Register (M3IFCTL) */
+ write32 IMX_M3IF_CTRL_BASE, M3IFCTL_CONFIG
.endm
#endif /* __ASSEMBLY__ */
diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
index 823df10..cdd9bda 100644
--- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
+++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
@@ -23,36 +23,13 @@
#include <asm/macro.h>
-.macro init_aips
- write32 0x43f00000, 0x77777777
- write32 0x43f00004, 0x77777777
- write32 0x43f00000, 0x77777777
- write32 0x53f00004, 0x77777777
-.endm
+#define AIPS_MPR_CONFIG 0x77777777
+#define MAX_MPR_CONFIG 0x43210
+#define MAX_SGPCR_CONFIG 0x10
+#define MAX_MGPCR_CONFIG 0x0
+#define M3IFCTL_CONFIG 0x1
-.macro init_max
- write32 0x43f04000, 0x43210
- write32 0x43f04100, 0x43210
- write32 0x43f04200, 0x43210
- write32 0x43f04300, 0x43210
- write32 0x43f04400, 0x43210
-
- write32 0x43f04010, 0x10
- write32 0x43f04110, 0x10
- write32 0x43f04210, 0x10
- write32 0x43f04310, 0x10
- write32 0x43f04410, 0x10
-
- write32 0x43f04800, 0x0
- write32 0x43f04900, 0x0
- write32 0x43f04a00, 0x0
- write32 0x43f04b00, 0x0
- write32 0x43f04c00, 0x0
-.endm
-
-.macro init_m3if
- write32 0xb8003000, 0x1
-.endm
+#include <asm/arch/macro.h>
.macro init_clocks
/*
@@ -64,6 +41,8 @@
* 0x00600000 makes CLKO parent clk the USB clk
*/
write32 0x53f80064, 0x45600000
+
+ /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
write32 0x53f80008, 0x20034000
/*
diff --git u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
index 1002674..575755c 100644
--- u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S
+++ u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
@@ -25,6 +25,12 @@
*/
#include <asm/macro.h>
+
+#define AIPS_MPR_CONFIG 0x77777777
+#define MAX_MPR_CONFIG 0x43210
+#define MAX_SGPCR_CONFIG 0x10
+#define MAX_MGPCR_CONFIG 0x0
+
#include <asm/arch/macro.h>
#include <asm/arch/imx-regs.h>
#include <generated/asm-offsets.h>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] mx25: Clean up lowlevel_init
2012-08-14 21:25 [U-Boot] [PATCH] mx25: Clean up lowlevel_init Benoît Thébaudeau
@ 2012-08-15 6:27 ` Matthias Weißer
2012-08-15 13:47 ` Benoît Thébaudeau
0 siblings, 1 reply; 10+ messages in thread
From: Matthias Weißer @ 2012-08-15 6:27 UTC (permalink / raw)
To: u-boot
Dear Beno?t
Am 14.08.2012 23:25, schrieb Beno?t Th?baudeau:
> Clean up mx25 lowlevel_init:
> - Add comments.
> - Do not use write32 repeatedly with the same value in order no to increase
> code size.
> - Make register values configurable.
> - Use defined values instead of literal constants.
> - Use defined macros instead of duplicating code.
>
> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
> .../arch/arm/include/asm/arch-mx25/macro.h | 57 +++++++++++++-------
> .../board/karo/tx25/lowlevel_init.S | 37 +++----------
> .../board/syteco/zmx25/lowlevel_init.S | 6 +++
> 3 files changed, 51 insertions(+), 49 deletions(-)
>
> diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> index 3b694da..9550ade 100644
> --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> @@ -32,32 +32,49 @@
>
> #include <asm/arch/imx-regs.h>
> #include <generated/asm-offsets.h>
> +#include <asm/macro.h>
>
> +/*
> + * AIPS setup - Only setup MPROTx registers.
> + * The PACR default values are good.
> + */
> .macro init_aips
> - write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
> - write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
> - write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
> - write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
> + ldr r0, =IMX_AIPS1_BASE
> + ldr r1, =AIPS_MPR_CONFIG
> + str r1, [r0, #AIPS_MPR_0_7]
> + str r1, [r0, #AIPS_MPR_8_15]
> + ldr r2, =IMX_AIPS2_BASE
> + str r1, [r2, #AIPS_MPR_0_7]
> + str r1, [r2, #AIPS_MPR_8_15]
> .endm
>
> +/* MAX (Multi-Layer AHB Crossbar Switch) setup */
> .macro init_max
> - write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
> -
> - write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
> + ldr r0, =IMX_MAX_BASE
> + ldr r1, =MAX_MPR_CONFIG
> + str r1, [r0, #MAX_MPR0] /* for S0 */
> + str r1, [r0, #MAX_MPR1] /* for S1 */
> + str r1, [r0, #MAX_MPR2] /* for S2 */
> + str r1, [r0, #MAX_MPR3] /* for S3 */
> + str r1, [r0, #MAX_MPR4] /* for S4 */
> + ldr r1, =MAX_SGPCR_CONFIG
> + str r1, [r0, #MAX_SGPCR0] /* for S0 */
> + str r1, [r0, #MAX_SGPCR1] /* for S1 */
> + str r1, [r0, #MAX_SGPCR2] /* for S2 */
> + str r1, [r0, #MAX_SGPCR3] /* for S3 */
> + str r1, [r0, #MAX_SGPCR4] /* for S4 */
> + ldr r1, =MAX_MGPCR_CONFIG
> + str r1, [r0, #MAX_MGPCR0] /* for M0 */
> + str r1, [r0, #MAX_MGPCR1] /* for M1 */
> + str r1, [r0, #MAX_MGPCR2] /* for M2 */
> + str r1, [r0, #MAX_MGPCR3] /* for M3 */
> + str r1, [r0, #MAX_MGPCR4] /* for M4 */
> +.endm
>
> - write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
> +/* M3IF setup */
> +.macro init_m3if
> + /* M3IF Control Register (M3IFCTL) */
> + write32 IMX_M3IF_CTRL_BASE, M3IFCTL_CONFIG
> .endm
>
> #endif /* __ASSEMBLY__ */
> diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> index 823df10..cdd9bda 100644
> --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> @@ -23,36 +23,13 @@
>
> #include <asm/macro.h>
>
> -.macro init_aips
> - write32 0x43f00000, 0x77777777
> - write32 0x43f00004, 0x77777777
> - write32 0x43f00000, 0x77777777
> - write32 0x53f00004, 0x77777777
> -.endm
> +#define AIPS_MPR_CONFIG 0x77777777
> +#define MAX_MPR_CONFIG 0x43210
> +#define MAX_SGPCR_CONFIG 0x10
> +#define MAX_MGPCR_CONFIG 0x0
> +#define M3IFCTL_CONFIG 0x1
I think it would be nicer if the init_max macro can get these values as
parameters and not as define which has to be there before including the
header file. You could even use default arguments so that the macro can
be used without any argument in the default case. What do you think?
> -.macro init_max
> - write32 0x43f04000, 0x43210
> - write32 0x43f04100, 0x43210
> - write32 0x43f04200, 0x43210
> - write32 0x43f04300, 0x43210
> - write32 0x43f04400, 0x43210
> -
> - write32 0x43f04010, 0x10
> - write32 0x43f04110, 0x10
> - write32 0x43f04210, 0x10
> - write32 0x43f04310, 0x10
> - write32 0x43f04410, 0x10
> -
> - write32 0x43f04800, 0x0
> - write32 0x43f04900, 0x0
> - write32 0x43f04a00, 0x0
> - write32 0x43f04b00, 0x0
> - write32 0x43f04c00, 0x0
> -.endm
> -
> -.macro init_m3if
> - write32 0xb8003000, 0x1
> -.endm
> +#include <asm/arch/macro.h>
>
> .macro init_clocks
> /*
> @@ -64,6 +41,8 @@
> * 0x00600000 makes CLKO parent clk the USB clk
> */
> write32 0x53f80064, 0x45600000
> +
> + /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
> write32 0x53f80008, 0x20034000
>
> /*
> diff --git u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
> index 1002674..575755c 100644
> --- u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S
> +++ u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
> @@ -25,6 +25,12 @@
> */
>
> #include <asm/macro.h>
> +
> +#define AIPS_MPR_CONFIG 0x77777777
> +#define MAX_MPR_CONFIG 0x43210
> +#define MAX_SGPCR_CONFIG 0x10
> +#define MAX_MGPCR_CONFIG 0x0
> +
> #include <asm/arch/macro.h>
> #include <asm/arch/imx-regs.h>
> #include <generated/asm-offsets.h>
I didn't build and run this but from a functional point it looks good.
BTW:
It would be nice if you put the maintainer of a changed file on CC. I
didn't check the ML all the time and maybe missed this change.
Regards
Matthias
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] mx25: Clean up lowlevel_init
2012-08-15 6:27 ` Matthias Weißer
@ 2012-08-15 13:47 ` Benoît Thébaudeau
2012-08-17 22:04 ` [U-Boot] [PATCH v2] " Benoît Thébaudeau
0 siblings, 1 reply; 10+ messages in thread
From: Benoît Thébaudeau @ 2012-08-15 13:47 UTC (permalink / raw)
To: u-boot
Dear Matthias,
> Dear Beno?t
>
> Am 14.08.2012 23:25, schrieb Beno?t Th?baudeau:
> > Clean up mx25 lowlevel_init:
> > - Add comments.
> > - Do not use write32 repeatedly with the same value in order no
> > to increase
> > code size.
> > - Make register values configurable.
> > - Use defined values instead of literal constants.
> > - Use defined macros instead of duplicating code.
> >
> > Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> > .../arch/arm/include/asm/arch-mx25/macro.h | 57
> > +++++++++++++-------
> > .../board/karo/tx25/lowlevel_init.S | 37
> > +++----------
> > .../board/syteco/zmx25/lowlevel_init.S | 6 +++
> > 3 files changed, 51 insertions(+), 49 deletions(-)
> >
> > diff --git
> > u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> > u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> > index 3b694da..9550ade 100644
> > --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> > +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> > @@ -32,32 +32,49 @@
> >
> > #include <asm/arch/imx-regs.h>
> > #include <generated/asm-offsets.h>
> > +#include <asm/macro.h>
> >
> > +/*
> > + * AIPS setup - Only setup MPROTx registers.
> > + * The PACR default values are good.
> > + */
> > .macro init_aips
> > - write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
> > - write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
> > - write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
> > - write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
> > + ldr r0, =IMX_AIPS1_BASE
> > + ldr r1, =AIPS_MPR_CONFIG
> > + str r1, [r0, #AIPS_MPR_0_7]
> > + str r1, [r0, #AIPS_MPR_8_15]
> > + ldr r2, =IMX_AIPS2_BASE
> > + str r1, [r2, #AIPS_MPR_0_7]
> > + str r1, [r2, #AIPS_MPR_8_15]
> > .endm
> >
> > +/* MAX (Multi-Layer AHB Crossbar Switch) setup */
> > .macro init_max
> > - write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
> > -
> > - write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
> > + ldr r0, =IMX_MAX_BASE
> > + ldr r1, =MAX_MPR_CONFIG
> > + str r1, [r0, #MAX_MPR0] /* for S0 */
> > + str r1, [r0, #MAX_MPR1] /* for S1 */
> > + str r1, [r0, #MAX_MPR2] /* for S2 */
> > + str r1, [r0, #MAX_MPR3] /* for S3 */
> > + str r1, [r0, #MAX_MPR4] /* for S4 */
> > + ldr r1, =MAX_SGPCR_CONFIG
> > + str r1, [r0, #MAX_SGPCR0] /* for S0 */
> > + str r1, [r0, #MAX_SGPCR1] /* for S1 */
> > + str r1, [r0, #MAX_SGPCR2] /* for S2 */
> > + str r1, [r0, #MAX_SGPCR3] /* for S3 */
> > + str r1, [r0, #MAX_SGPCR4] /* for S4 */
> > + ldr r1, =MAX_MGPCR_CONFIG
> > + str r1, [r0, #MAX_MGPCR0] /* for M0 */
> > + str r1, [r0, #MAX_MGPCR1] /* for M1 */
> > + str r1, [r0, #MAX_MGPCR2] /* for M2 */
> > + str r1, [r0, #MAX_MGPCR3] /* for M3 */
> > + str r1, [r0, #MAX_MGPCR4] /* for M4 */
> > +.endm
> >
> > - write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
> > +/* M3IF setup */
> > +.macro init_m3if
> > + /* M3IF Control Register (M3IFCTL) */
> > + write32 IMX_M3IF_CTRL_BASE, M3IFCTL_CONFIG
> > .endm
> >
> > #endif /* __ASSEMBLY__ */
> > diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> > u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> > index 823df10..cdd9bda 100644
> > --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> > +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> > @@ -23,36 +23,13 @@
> >
> > #include <asm/macro.h>
> >
> > -.macro init_aips
> > - write32 0x43f00000, 0x77777777
> > - write32 0x43f00004, 0x77777777
> > - write32 0x43f00000, 0x77777777
> > - write32 0x53f00004, 0x77777777
> > -.endm
> > +#define AIPS_MPR_CONFIG 0x77777777
> > +#define MAX_MPR_CONFIG 0x43210
> > +#define MAX_SGPCR_CONFIG 0x10
> > +#define MAX_MGPCR_CONFIG 0x0
> > +#define M3IFCTL_CONFIG 0x1
>
> I think it would be nicer if the init_max macro can get these values
> as
> parameters and not as define which has to be there before including
> the
> header file. You could even use default arguments so that the macro
> can
> be used without any argument in the default case. What do you think?
The solution that I applied here comes from the i.MX35 lowlevel_init code. I
like your suggestion. I think I will do that in my i.MX35 ll series too.
> > -.macro init_max
> > - write32 0x43f04000, 0x43210
> > - write32 0x43f04100, 0x43210
> > - write32 0x43f04200, 0x43210
> > - write32 0x43f04300, 0x43210
> > - write32 0x43f04400, 0x43210
> > -
> > - write32 0x43f04010, 0x10
> > - write32 0x43f04110, 0x10
> > - write32 0x43f04210, 0x10
> > - write32 0x43f04310, 0x10
> > - write32 0x43f04410, 0x10
> > -
> > - write32 0x43f04800, 0x0
> > - write32 0x43f04900, 0x0
> > - write32 0x43f04a00, 0x0
> > - write32 0x43f04b00, 0x0
> > - write32 0x43f04c00, 0x0
> > -.endm
> > -
> > -.macro init_m3if
> > - write32 0xb8003000, 0x1
> > -.endm
> > +#include <asm/arch/macro.h>
> >
> > .macro init_clocks
> > /*
> > @@ -64,6 +41,8 @@
> > * 0x00600000 makes CLKO parent clk the USB clk
> > */
> > write32 0x53f80064, 0x45600000
> > +
> > + /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
> > write32 0x53f80008, 0x20034000
> >
> > /*
> > diff --git u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S
> > u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
> > index 1002674..575755c 100644
> > --- u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S
> > +++ u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
> > @@ -25,6 +25,12 @@
> > */
> >
> > #include <asm/macro.h>
> > +
> > +#define AIPS_MPR_CONFIG 0x77777777
> > +#define MAX_MPR_CONFIG 0x43210
> > +#define MAX_SGPCR_CONFIG 0x10
> > +#define MAX_MGPCR_CONFIG 0x0
> > +
> > #include <asm/arch/macro.h>
> > #include <asm/arch/imx-regs.h>
> > #include <generated/asm-offsets.h>
>
> I didn't build and run this but from a functional point it looks
> good.
Great.
> BTW:
> It would be nice if you put the maintainer of a changed file on CC. I
> didn't check the ML all the time and maybe missed this change.
Sure, sorry. I Cc'ed the custodian, but forgot the maintainers. I add the other
board maintainer to Cc.
Best regards,
Beno?t
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] mx25: Clean up lowlevel_init
2012-08-15 13:47 ` Benoît Thébaudeau
@ 2012-08-17 22:04 ` Benoît Thébaudeau
2012-08-20 19:00 ` [U-Boot] [PATCH v3] " Benoît Thébaudeau
0 siblings, 1 reply; 10+ messages in thread
From: Benoît Thébaudeau @ 2012-08-17 22:04 UTC (permalink / raw)
To: u-boot
Clean up mx25 lowlevel_init:
- Add comments.
- Do not use write32 repeatedly with the same value in order not to increase
code size.
- Make register values configurable.
- Use macro parameters with default values instead of literal constants.
- Use defined macros instead of duplicating code.
Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: John Rigby <jcrigby@gmail.com>
Cc: Matthias Weisser <weisserm@arcor.de>
---
Changes for v2:
- Use macro arguments with default values instead of #define's.
.../arch/arm/include/asm/arch-mx25/macro.h | 87 +++++++++++++++-----
.../board/karo/tx25/lowlevel_init.S | 34 +-------
2 files changed, 68 insertions(+), 53 deletions(-)
diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
index 3b694da..7061cd1 100644
--- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
+++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
@@ -32,32 +32,75 @@
#include <asm/arch/imx-regs.h>
#include <generated/asm-offsets.h>
+#include <asm/macro.h>
-.macro init_aips
- write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
- write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
- write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
- write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
+/*
+ * AIPS setup - Only setup MPROTx registers.
+ * The PACR default values are good.
+ *
+ * Default argument values:
+ * - MPR: Set all MPROTx to be non-bufferable, trusted for R/W, not forced to
+ * user-mode.
+ */
+.macro init_aips mpr=0x77777777
+ ldr r0, =IMX_AIPS1_BASE
+ ldr r1, =\mpr
+ str r1, [r0, #AIPS_MPR_0_7]
+ str r1, [r0, #AIPS_MPR_8_15]
+ ldr r2, =IMX_AIPS2_BASE
+ str r1, [r2, #AIPS_MPR_0_7]
+ str r1, [r2, #AIPS_MPR_8_15]
.endm
-.macro init_max
- write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
-
- write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
+/*
+ * MAX (Multi-Layer AHB Crossbar Switch) setup
+ *
+ * Default argument values:
+ * - MPR: priority is IAHB > RTIC > DAHB > USBOTG > eSDHC2/SDMA
+ * - SGPCR: always park on last master
+ * - MGPCR: restore default values
+ */
+.macro init_max mpr=0x00043210, sgpcr=0x00000010, mgpcr=0x00000000
+ ldr r0, =IMX_MAX_BASE
+ ldr r1, =\mpr
+ str r1, [r0, #MAX_MPR0] /* for S0 */
+ str r1, [r0, #MAX_MPR1] /* for S1 */
+ str r1, [r0, #MAX_MPR2] /* for S2 */
+ str r1, [r0, #MAX_MPR3] /* for S3 */
+ str r1, [r0, #MAX_MPR4] /* for S4 */
+ ldr r1, =\sgpcr
+ str r1, [r0, #MAX_SGPCR0] /* for S0 */
+ str r1, [r0, #MAX_SGPCR1] /* for S1 */
+ str r1, [r0, #MAX_SGPCR2] /* for S2 */
+ str r1, [r0, #MAX_SGPCR3] /* for S3 */
+ str r1, [r0, #MAX_SGPCR4] /* for S4 */
+ ldr r1, =\mgpcr
+ str r1, [r0, #MAX_MGPCR0] /* for M0 */
+ str r1, [r0, #MAX_MGPCR1] /* for M1 */
+ str r1, [r0, #MAX_MGPCR2] /* for M2 */
+ str r1, [r0, #MAX_MGPCR3] /* for M3 */
+ str r1, [r0, #MAX_MGPCR4] /* for M4 */
+.endm
- write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
+/*
+ * M3IF setup
+ *
+ * Default argument values:
+ * - CTL:
+ * MRRP[0] = LCDC on priority list (1 << 0) = 0x00000001
+ * MRRP[1] = MAX1 not on priority list (0 << 1) = 0x00000000
+ * MRRP[2] = MAX0 not on priority list (0 << 2) = 0x00000000
+ * MRRP[3] = USBH not on priority list (0 << 3) = 0x00000000
+ * MRRP[4] = SDMA not on priority list (0 << 4) = 0x00000000
+ * MRRP[5] = eSDHC1/ATA/FEC not on priority list (0 << 5) = 0x00000000
+ * MRRP[6] = LCDC/SLCDC/MAX2 not on priority list (0 << 6) = 0x00000000
+ * MRRP[7] = CSI not on priority list (0 << 7) = 0x00000000
+ * ------------
+ * 0x00000001
+ */
+.macro init_m3if ctl=0x00000001
+ /* M3IF Control Register (M3IFCTL) */
+ write32 IMX_M3IF_CTRL_BASE, \ctl
.endm
#endif /* __ASSEMBLY__ */
diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
index 823df10..0421237 100644
--- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
+++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
@@ -22,37 +22,7 @@
*/
#include <asm/macro.h>
-
-.macro init_aips
- write32 0x43f00000, 0x77777777
- write32 0x43f00004, 0x77777777
- write32 0x43f00000, 0x77777777
- write32 0x53f00004, 0x77777777
-.endm
-
-.macro init_max
- write32 0x43f04000, 0x43210
- write32 0x43f04100, 0x43210
- write32 0x43f04200, 0x43210
- write32 0x43f04300, 0x43210
- write32 0x43f04400, 0x43210
-
- write32 0x43f04010, 0x10
- write32 0x43f04110, 0x10
- write32 0x43f04210, 0x10
- write32 0x43f04310, 0x10
- write32 0x43f04410, 0x10
-
- write32 0x43f04800, 0x0
- write32 0x43f04900, 0x0
- write32 0x43f04a00, 0x0
- write32 0x43f04b00, 0x0
- write32 0x43f04c00, 0x0
-.endm
-
-.macro init_m3if
- write32 0xb8003000, 0x1
-.endm
+#include <asm/arch/macro.h>
.macro init_clocks
/*
@@ -64,6 +34,8 @@
* 0x00600000 makes CLKO parent clk the USB clk
*/
write32 0x53f80064, 0x45600000
+
+ /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
write32 0x53f80008, 0x20034000
/*
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v3] mx25: Clean up lowlevel_init
2012-08-17 22:04 ` [U-Boot] [PATCH v2] " Benoît Thébaudeau
@ 2012-08-20 19:00 ` Benoît Thébaudeau
2012-09-30 13:04 ` Stefano Babic
2012-10-02 8:31 ` Stefano Babic
0 siblings, 2 replies; 10+ messages in thread
From: Benoît Thébaudeau @ 2012-08-20 19:00 UTC (permalink / raw)
To: u-boot
Clean up mx25 lowlevel_init:
- Add comments.
- Do not use write32 repeatedly with the same value in order not to increase
code size.
- Make register values configurable.
- Use macro parameters with default values instead of literal constants.
- Use defined macros instead of duplicating code.
Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: John Rigby <jcrigby@gmail.com>
Cc: Matthias Weisser <weisserm@arcor.de>
---
Changes for v2:
- Use macro arguments with default values instead of #define's.
Changes for v3:
- Fix comment for default MAX MPR value.
.../arch/arm/include/asm/arch-mx25/macro.h | 87 +++++++++++++++-----
.../board/karo/tx25/lowlevel_init.S | 34 +-------
2 files changed, 68 insertions(+), 53 deletions(-)
diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
index 3b694da..56cae36 100644
--- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
+++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
@@ -32,32 +32,75 @@
#include <asm/arch/imx-regs.h>
#include <generated/asm-offsets.h>
+#include <asm/macro.h>
-.macro init_aips
- write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
- write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
- write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
- write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
+/*
+ * AIPS setup - Only setup MPROTx registers.
+ * The PACR default values are good.
+ *
+ * Default argument values:
+ * - MPR: Set all MPROTx to be non-bufferable, trusted for R/W, not forced to
+ * user-mode.
+ */
+.macro init_aips mpr=0x77777777
+ ldr r0, =IMX_AIPS1_BASE
+ ldr r1, =\mpr
+ str r1, [r0, #AIPS_MPR_0_7]
+ str r1, [r0, #AIPS_MPR_8_15]
+ ldr r2, =IMX_AIPS2_BASE
+ str r1, [r2, #AIPS_MPR_0_7]
+ str r1, [r2, #AIPS_MPR_8_15]
.endm
-.macro init_max
- write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
- write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
-
- write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
- write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
+/*
+ * MAX (Multi-Layer AHB Crossbar Switch) setup
+ *
+ * Default argument values:
+ * - MPR: priority is IAHB > DAHB > USBOTG > RTIC > eSDHC2/SDMA
+ * - SGPCR: always park on last master
+ * - MGPCR: restore default values
+ */
+.macro init_max mpr=0x00043210, sgpcr=0x00000010, mgpcr=0x00000000
+ ldr r0, =IMX_MAX_BASE
+ ldr r1, =\mpr
+ str r1, [r0, #MAX_MPR0] /* for S0 */
+ str r1, [r0, #MAX_MPR1] /* for S1 */
+ str r1, [r0, #MAX_MPR2] /* for S2 */
+ str r1, [r0, #MAX_MPR3] /* for S3 */
+ str r1, [r0, #MAX_MPR4] /* for S4 */
+ ldr r1, =\sgpcr
+ str r1, [r0, #MAX_SGPCR0] /* for S0 */
+ str r1, [r0, #MAX_SGPCR1] /* for S1 */
+ str r1, [r0, #MAX_SGPCR2] /* for S2 */
+ str r1, [r0, #MAX_SGPCR3] /* for S3 */
+ str r1, [r0, #MAX_SGPCR4] /* for S4 */
+ ldr r1, =\mgpcr
+ str r1, [r0, #MAX_MGPCR0] /* for M0 */
+ str r1, [r0, #MAX_MGPCR1] /* for M1 */
+ str r1, [r0, #MAX_MGPCR2] /* for M2 */
+ str r1, [r0, #MAX_MGPCR3] /* for M3 */
+ str r1, [r0, #MAX_MGPCR4] /* for M4 */
+.endm
- write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
- write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
+/*
+ * M3IF setup
+ *
+ * Default argument values:
+ * - CTL:
+ * MRRP[0] = LCDC on priority list (1 << 0) = 0x00000001
+ * MRRP[1] = MAX1 not on priority list (0 << 1) = 0x00000000
+ * MRRP[2] = MAX0 not on priority list (0 << 2) = 0x00000000
+ * MRRP[3] = USBH not on priority list (0 << 3) = 0x00000000
+ * MRRP[4] = SDMA not on priority list (0 << 4) = 0x00000000
+ * MRRP[5] = eSDHC1/ATA/FEC not on priority list (0 << 5) = 0x00000000
+ * MRRP[6] = LCDC/SLCDC/MAX2 not on priority list (0 << 6) = 0x00000000
+ * MRRP[7] = CSI not on priority list (0 << 7) = 0x00000000
+ * ------------
+ * 0x00000001
+ */
+.macro init_m3if ctl=0x00000001
+ /* M3IF Control Register (M3IFCTL) */
+ write32 IMX_M3IF_CTRL_BASE, \ctl
.endm
#endif /* __ASSEMBLY__ */
diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
index 823df10..0421237 100644
--- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
+++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
@@ -22,37 +22,7 @@
*/
#include <asm/macro.h>
-
-.macro init_aips
- write32 0x43f00000, 0x77777777
- write32 0x43f00004, 0x77777777
- write32 0x43f00000, 0x77777777
- write32 0x53f00004, 0x77777777
-.endm
-
-.macro init_max
- write32 0x43f04000, 0x43210
- write32 0x43f04100, 0x43210
- write32 0x43f04200, 0x43210
- write32 0x43f04300, 0x43210
- write32 0x43f04400, 0x43210
-
- write32 0x43f04010, 0x10
- write32 0x43f04110, 0x10
- write32 0x43f04210, 0x10
- write32 0x43f04310, 0x10
- write32 0x43f04410, 0x10
-
- write32 0x43f04800, 0x0
- write32 0x43f04900, 0x0
- write32 0x43f04a00, 0x0
- write32 0x43f04b00, 0x0
- write32 0x43f04c00, 0x0
-.endm
-
-.macro init_m3if
- write32 0xb8003000, 0x1
-.endm
+#include <asm/arch/macro.h>
.macro init_clocks
/*
@@ -64,6 +34,8 @@
* 0x00600000 makes CLKO parent clk the USB clk
*/
write32 0x53f80064, 0x45600000
+
+ /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
write32 0x53f80008, 0x20034000
/*
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v3] mx25: Clean up lowlevel_init
2012-08-20 19:00 ` [U-Boot] [PATCH v3] " Benoît Thébaudeau
@ 2012-09-30 13:04 ` Stefano Babic
2012-09-30 13:47 ` Benoît Thébaudeau
2012-10-02 8:31 ` Stefano Babic
1 sibling, 1 reply; 10+ messages in thread
From: Stefano Babic @ 2012-09-30 13:04 UTC (permalink / raw)
To: u-boot
On 20/08/2012 21:00, Beno?t Th?baudeau wrote:
> Clean up mx25 lowlevel_init:
> - Add comments.
> - Do not use write32 repeatedly with the same value in order not to increase
> code size.
> - Make register values configurable.
> - Use macro parameters with default values instead of literal constants.
> - Use defined macros instead of duplicating code.
>
> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: John Rigby <jcrigby@gmail.com>
> Cc: Matthias Weisser <weisserm@arcor.de>
> ---
Hi Beno?t,
> Changes for v2:
> - Use macro arguments with default values instead of #define's.
> Changes for v3:
> - Fix comment for default MAX MPR value.
>
> .../arch/arm/include/asm/arch-mx25/macro.h | 87 +++++++++++++++-----
> .../board/karo/tx25/lowlevel_init.S | 34 +-------
> 2 files changed, 68 insertions(+), 53 deletions(-)
>
> diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> index 3b694da..56cae36 100644
> --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> @@ -32,32 +32,75 @@
>
> #include <asm/arch/imx-regs.h>
> #include <generated/asm-offsets.h>
> +#include <asm/macro.h>
>
> -.macro init_aips
> - write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
> - write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
> - write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
> - write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
> +/*
> + * AIPS setup - Only setup MPROTx registers.
> + * The PACR default values are good.
> + *
> + * Default argument values:
> + * - MPR: Set all MPROTx to be non-bufferable, trusted for R/W, not forced to
> + * user-mode.
> + */
> +.macro init_aips mpr=0x77777777
> + ldr r0, =IMX_AIPS1_BASE
> + ldr r1, =\mpr
> + str r1, [r0, #AIPS_MPR_0_7]
> + str r1, [r0, #AIPS_MPR_8_15]
> + ldr r2, =IMX_AIPS2_BASE
> + str r1, [r2, #AIPS_MPR_0_7]
> + str r1, [r2, #AIPS_MPR_8_15]
> .endm
>
> -.macro init_max
> - write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
> - write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
> -
> - write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
> - write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
> +/*
> + * MAX (Multi-Layer AHB Crossbar Switch) setup
> + *
> + * Default argument values:
> + * - MPR: priority is IAHB > DAHB > USBOTG > RTIC > eSDHC2/SDMA
> + * - SGPCR: always park on last master
> + * - MGPCR: restore default values
> + */
> +.macro init_max mpr=0x00043210, sgpcr=0x00000010, mgpcr=0x00000000
> + ldr r0, =IMX_MAX_BASE
> + ldr r1, =\mpr
> + str r1, [r0, #MAX_MPR0] /* for S0 */
> + str r1, [r0, #MAX_MPR1] /* for S1 */
> + str r1, [r0, #MAX_MPR2] /* for S2 */
> + str r1, [r0, #MAX_MPR3] /* for S3 */
> + str r1, [r0, #MAX_MPR4] /* for S4 */
> + ldr r1, =\sgpcr
> + str r1, [r0, #MAX_SGPCR0] /* for S0 */
> + str r1, [r0, #MAX_SGPCR1] /* for S1 */
> + str r1, [r0, #MAX_SGPCR2] /* for S2 */
> + str r1, [r0, #MAX_SGPCR3] /* for S3 */
> + str r1, [r0, #MAX_SGPCR4] /* for S4 */
> + ldr r1, =\mgpcr
> + str r1, [r0, #MAX_MGPCR0] /* for M0 */
> + str r1, [r0, #MAX_MGPCR1] /* for M1 */
> + str r1, [r0, #MAX_MGPCR2] /* for M2 */
> + str r1, [r0, #MAX_MGPCR3] /* for M3 */
> + str r1, [r0, #MAX_MGPCR4] /* for M4 */
> +.endm
>
> - write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
> - write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
> +/*
> + * M3IF setup
> + *
> + * Default argument values:
> + * - CTL:
> + * MRRP[0] = LCDC on priority list (1 << 0) = 0x00000001
> + * MRRP[1] = MAX1 not on priority list (0 << 1) = 0x00000000
> + * MRRP[2] = MAX0 not on priority list (0 << 2) = 0x00000000
> + * MRRP[3] = USBH not on priority list (0 << 3) = 0x00000000
> + * MRRP[4] = SDMA not on priority list (0 << 4) = 0x00000000
> + * MRRP[5] = eSDHC1/ATA/FEC not on priority list (0 << 5) = 0x00000000
> + * MRRP[6] = LCDC/SLCDC/MAX2 not on priority list (0 << 6) = 0x00000000
> + * MRRP[7] = CSI not on priority list (0 << 7) = 0x00000000
> + * ------------
> + * 0x00000001
> + */
> +.macro init_m3if ctl=0x00000001
> + /* M3IF Control Register (M3IFCTL) */
> + write32 IMX_M3IF_CTRL_BASE, \ctl
> .endm
>
> #endif /* __ASSEMBLY__ */
> diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> index 823df10..0421237 100644
> --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> @@ -22,37 +22,7 @@
> */
>
> #include <asm/macro.h>
> -
> -.macro init_aips
> - write32 0x43f00000, 0x77777777
> - write32 0x43f00004, 0x77777777
> - write32 0x43f00000, 0x77777777
> - write32 0x53f00004, 0x77777777
> -.endm
> -
> -.macro init_max
> - write32 0x43f04000, 0x43210
> - write32 0x43f04100, 0x43210
> - write32 0x43f04200, 0x43210
> - write32 0x43f04300, 0x43210
> - write32 0x43f04400, 0x43210
> -
> - write32 0x43f04010, 0x10
> - write32 0x43f04110, 0x10
> - write32 0x43f04210, 0x10
> - write32 0x43f04310, 0x10
> - write32 0x43f04410, 0x10
> -
> - write32 0x43f04800, 0x0
> - write32 0x43f04900, 0x0
> - write32 0x43f04a00, 0x0
> - write32 0x43f04b00, 0x0
> - write32 0x43f04c00, 0x0
> -.endm
> -
> -.macro init_m3if
> - write32 0xb8003000, 0x1
> -.endm
> +#include <asm/arch/macro.h>
>
> .macro init_clocks
> /*
> @@ -64,6 +34,8 @@
> * 0x00600000 makes CLKO parent clk the USB clk
> */
> write32 0x53f80064, 0x45600000
> +
> + /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
> write32 0x53f80008, 0x20034000
>
> /*
>
You only moved code and set default values for macros: I do not see
issues. But checkpatch reports several errors, and, even if some of them
can be related to the fact this file is in assembly, most of them can be
fixed.
Could you take a look ?
Best regards,
Stefano
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v3] mx25: Clean up lowlevel_init
2012-09-30 13:04 ` Stefano Babic
@ 2012-09-30 13:47 ` Benoît Thébaudeau
2012-10-01 19:50 ` Benoît Thébaudeau
0 siblings, 1 reply; 10+ messages in thread
From: Benoît Thébaudeau @ 2012-09-30 13:47 UTC (permalink / raw)
To: u-boot
Hi Stefano,
On Sunday, September 30, 2012 3:04:14 PM, Stefano Babic wrote:
> On 20/08/2012 21:00, Beno?t Th?baudeau wrote:
> > Clean up mx25 lowlevel_init:
> > - Add comments.
> > - Do not use write32 repeatedly with the same value in order not
> > to increase
> > code size.
> > - Make register values configurable.
> > - Use macro parameters with default values instead of literal
> > constants.
> > - Use defined macros instead of duplicating code.
> >
> > Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: John Rigby <jcrigby@gmail.com>
> > Cc: Matthias Weisser <weisserm@arcor.de>
> > ---
>
> Hi Beno?t,
>
> > Changes for v2:
> > - Use macro arguments with default values instead of #define's.
> > Changes for v3:
> > - Fix comment for default MAX MPR value.
> >
> > .../arch/arm/include/asm/arch-mx25/macro.h | 87
> > +++++++++++++++-----
> > .../board/karo/tx25/lowlevel_init.S | 34 +-------
> > 2 files changed, 68 insertions(+), 53 deletions(-)
> >
> > diff --git
> > u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> > u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> > index 3b694da..56cae36 100644
> > --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> > +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> > @@ -32,32 +32,75 @@
> >
> > #include <asm/arch/imx-regs.h>
> > #include <generated/asm-offsets.h>
> > +#include <asm/macro.h>
> >
> > -.macro init_aips
> > - write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
> > - write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
> > - write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
> > - write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
> > +/*
> > + * AIPS setup - Only setup MPROTx registers.
> > + * The PACR default values are good.
> > + *
> > + * Default argument values:
> > + * - MPR: Set all MPROTx to be non-bufferable, trusted for R/W,
> > not forced to
> > + * user-mode.
> > + */
> > +.macro init_aips mpr=0x77777777
> > + ldr r0, =IMX_AIPS1_BASE
> > + ldr r1, =\mpr
> > + str r1, [r0, #AIPS_MPR_0_7]
> > + str r1, [r0, #AIPS_MPR_8_15]
> > + ldr r2, =IMX_AIPS2_BASE
> > + str r1, [r2, #AIPS_MPR_0_7]
> > + str r1, [r2, #AIPS_MPR_8_15]
> > .endm
> >
> > -.macro init_max
> > - write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
> > - write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
> > -
> > - write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
> > - write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
> > +/*
> > + * MAX (Multi-Layer AHB Crossbar Switch) setup
> > + *
> > + * Default argument values:
> > + * - MPR: priority is IAHB > DAHB > USBOTG > RTIC > eSDHC2/SDMA
> > + * - SGPCR: always park on last master
> > + * - MGPCR: restore default values
> > + */
> > +.macro init_max mpr=0x00043210, sgpcr=0x00000010, mgpcr=0x00000000
> > + ldr r0, =IMX_MAX_BASE
> > + ldr r1, =\mpr
> > + str r1, [r0, #MAX_MPR0] /* for S0 */
> > + str r1, [r0, #MAX_MPR1] /* for S1 */
> > + str r1, [r0, #MAX_MPR2] /* for S2 */
> > + str r1, [r0, #MAX_MPR3] /* for S3 */
> > + str r1, [r0, #MAX_MPR4] /* for S4 */
> > + ldr r1, =\sgpcr
> > + str r1, [r0, #MAX_SGPCR0] /* for S0 */
> > + str r1, [r0, #MAX_SGPCR1] /* for S1 */
> > + str r1, [r0, #MAX_SGPCR2] /* for S2 */
> > + str r1, [r0, #MAX_SGPCR3] /* for S3 */
> > + str r1, [r0, #MAX_SGPCR4] /* for S4 */
> > + ldr r1, =\mgpcr
> > + str r1, [r0, #MAX_MGPCR0] /* for M0 */
> > + str r1, [r0, #MAX_MGPCR1] /* for M1 */
> > + str r1, [r0, #MAX_MGPCR2] /* for M2 */
> > + str r1, [r0, #MAX_MGPCR3] /* for M3 */
> > + str r1, [r0, #MAX_MGPCR4] /* for M4 */
> > +.endm
> >
> > - write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
> > - write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
> > +/*
> > + * M3IF setup
> > + *
> > + * Default argument values:
> > + * - CTL:
> > + * MRRP[0] = LCDC on priority list (1 << 0) = 0x00000001
> > + * MRRP[1] = MAX1 not on priority list (0 << 1) = 0x00000000
> > + * MRRP[2] = MAX0 not on priority list (0 << 2) = 0x00000000
> > + * MRRP[3] = USBH not on priority list (0 << 3) = 0x00000000
> > + * MRRP[4] = SDMA not on priority list (0 << 4) = 0x00000000
> > + * MRRP[5] = eSDHC1/ATA/FEC not on priority list (0 << 5) =
> > 0x00000000
> > + * MRRP[6] = LCDC/SLCDC/MAX2 not on priority list (0 << 6) =
> > 0x00000000
> > + * MRRP[7] = CSI not on priority list (0 << 7) = 0x00000000
> > + * ------------
> > + * 0x00000001
> > + */
> > +.macro init_m3if ctl=0x00000001
> > + /* M3IF Control Register (M3IFCTL) */
> > + write32 IMX_M3IF_CTRL_BASE, \ctl
> > .endm
> >
> > #endif /* __ASSEMBLY__ */
> > diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> > u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> > index 823df10..0421237 100644
> > --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> > +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> > @@ -22,37 +22,7 @@
> > */
> >
> > #include <asm/macro.h>
> > -
> > -.macro init_aips
> > - write32 0x43f00000, 0x77777777
> > - write32 0x43f00004, 0x77777777
> > - write32 0x43f00000, 0x77777777
> > - write32 0x53f00004, 0x77777777
> > -.endm
> > -
> > -.macro init_max
> > - write32 0x43f04000, 0x43210
> > - write32 0x43f04100, 0x43210
> > - write32 0x43f04200, 0x43210
> > - write32 0x43f04300, 0x43210
> > - write32 0x43f04400, 0x43210
> > -
> > - write32 0x43f04010, 0x10
> > - write32 0x43f04110, 0x10
> > - write32 0x43f04210, 0x10
> > - write32 0x43f04310, 0x10
> > - write32 0x43f04410, 0x10
> > -
> > - write32 0x43f04800, 0x0
> > - write32 0x43f04900, 0x0
> > - write32 0x43f04a00, 0x0
> > - write32 0x43f04b00, 0x0
> > - write32 0x43f04c00, 0x0
> > -.endm
> > -
> > -.macro init_m3if
> > - write32 0xb8003000, 0x1
> > -.endm
> > +#include <asm/arch/macro.h>
> >
> > .macro init_clocks
> > /*
> > @@ -64,6 +34,8 @@
> > * 0x00600000 makes CLKO parent clk the USB clk
> > */
> > write32 0x53f80064, 0x45600000
> > +
> > + /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
> > write32 0x53f80008, 0x20034000
> >
> > /*
> >
>
> You only moved code and set default values for macros: I do not see
> issues. But checkpatch reports several errors, and, even if some of
> them
> can be related to the fact this file is in assembly, most of them can
> be
> fixed.
>
> Could you take a look ?
Will do.
Best regards,
Beno?t
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v3] mx25: Clean up lowlevel_init
2012-09-30 13:47 ` Benoît Thébaudeau
@ 2012-10-01 19:50 ` Benoît Thébaudeau
2012-10-02 8:12 ` Stefano Babic
0 siblings, 1 reply; 10+ messages in thread
From: Benoît Thébaudeau @ 2012-10-01 19:50 UTC (permalink / raw)
To: u-boot
Hi Stefano,
On Sunday, September 30, 2012 3:47:12 PM, Beno?t Th?baudeau wrote:
> Hi Stefano,
>
> On Sunday, September 30, 2012 3:04:14 PM, Stefano Babic wrote:
> > On 20/08/2012 21:00, Beno?t Th?baudeau wrote:
> > > Clean up mx25 lowlevel_init:
> > > - Add comments.
> > > - Do not use write32 repeatedly with the same value in order not
> > > to increase
> > > code size.
> > > - Make register values configurable.
> > > - Use macro parameters with default values instead of literal
> > > constants.
> > > - Use defined macros instead of duplicating code.
> > >
> > > Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> > > Cc: Stefano Babic <sbabic@denx.de>
> > > Cc: John Rigby <jcrigby@gmail.com>
> > > Cc: Matthias Weisser <weisserm@arcor.de>
> > > ---
> >
> > Hi Beno?t,
> >
> > > Changes for v2:
> > > - Use macro arguments with default values instead of #define's.
> > > Changes for v3:
> > > - Fix comment for default MAX MPR value.
> > >
> > > .../arch/arm/include/asm/arch-mx25/macro.h | 87
> > > +++++++++++++++-----
> > > .../board/karo/tx25/lowlevel_init.S | 34
> > > +-------
> > > 2 files changed, 68 insertions(+), 53 deletions(-)
> > >
> > > diff --git
> > > u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> > > u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> > > index 3b694da..56cae36 100644
> > > --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> > > +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> > > @@ -32,32 +32,75 @@
> > >
> > > #include <asm/arch/imx-regs.h>
> > > #include <generated/asm-offsets.h>
> > > +#include <asm/macro.h>
> > >
> > > -.macro init_aips
> > > - write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
> > > - write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
> > > - write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
> > > - write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
> > > +/*
> > > + * AIPS setup - Only setup MPROTx registers.
> > > + * The PACR default values are good.
> > > + *
> > > + * Default argument values:
> > > + * - MPR: Set all MPROTx to be non-bufferable, trusted for R/W,
> > > not forced to
> > > + * user-mode.
> > > + */
> > > +.macro init_aips mpr=0x77777777
> > > + ldr r0, =IMX_AIPS1_BASE
> > > + ldr r1, =\mpr
> > > + str r1, [r0, #AIPS_MPR_0_7]
> > > + str r1, [r0, #AIPS_MPR_8_15]
> > > + ldr r2, =IMX_AIPS2_BASE
> > > + str r1, [r2, #AIPS_MPR_0_7]
> > > + str r1, [r2, #AIPS_MPR_8_15]
> > > .endm
> > >
> > > -.macro init_max
> > > - write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
> > > - write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
> > > - write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
> > > - write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
> > > - write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
> > > -
> > > - write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
> > > - write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
> > > - write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
> > > - write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
> > > - write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
> > > +/*
> > > + * MAX (Multi-Layer AHB Crossbar Switch) setup
> > > + *
> > > + * Default argument values:
> > > + * - MPR: priority is IAHB > DAHB > USBOTG > RTIC > eSDHC2/SDMA
> > > + * - SGPCR: always park on last master
> > > + * - MGPCR: restore default values
> > > + */
> > > +.macro init_max mpr=0x00043210, sgpcr=0x00000010,
> > > mgpcr=0x00000000
> > > + ldr r0, =IMX_MAX_BASE
> > > + ldr r1, =\mpr
> > > + str r1, [r0, #MAX_MPR0] /* for S0 */
> > > + str r1, [r0, #MAX_MPR1] /* for S1 */
> > > + str r1, [r0, #MAX_MPR2] /* for S2 */
> > > + str r1, [r0, #MAX_MPR3] /* for S3 */
> > > + str r1, [r0, #MAX_MPR4] /* for S4 */
> > > + ldr r1, =\sgpcr
> > > + str r1, [r0, #MAX_SGPCR0] /* for S0 */
> > > + str r1, [r0, #MAX_SGPCR1] /* for S1 */
> > > + str r1, [r0, #MAX_SGPCR2] /* for S2 */
> > > + str r1, [r0, #MAX_SGPCR3] /* for S3 */
> > > + str r1, [r0, #MAX_SGPCR4] /* for S4 */
> > > + ldr r1, =\mgpcr
> > > + str r1, [r0, #MAX_MGPCR0] /* for M0 */
> > > + str r1, [r0, #MAX_MGPCR1] /* for M1 */
> > > + str r1, [r0, #MAX_MGPCR2] /* for M2 */
> > > + str r1, [r0, #MAX_MGPCR3] /* for M3 */
> > > + str r1, [r0, #MAX_MGPCR4] /* for M4 */
> > > +.endm
> > >
> > > - write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
> > > - write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
> > > - write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
> > > - write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
> > > - write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
> > > +/*
> > > + * M3IF setup
> > > + *
> > > + * Default argument values:
> > > + * - CTL:
> > > + * MRRP[0] = LCDC on priority list (1 << 0) = 0x00000001
> > > + * MRRP[1] = MAX1 not on priority list (0 << 1) = 0x00000000
> > > + * MRRP[2] = MAX0 not on priority list (0 << 2) = 0x00000000
> > > + * MRRP[3] = USBH not on priority list (0 << 3) = 0x00000000
> > > + * MRRP[4] = SDMA not on priority list (0 << 4) = 0x00000000
> > > + * MRRP[5] = eSDHC1/ATA/FEC not on priority list (0 << 5) =
> > > 0x00000000
> > > + * MRRP[6] = LCDC/SLCDC/MAX2 not on priority list (0 << 6) =
> > > 0x00000000
> > > + * MRRP[7] = CSI not on priority list (0 << 7) = 0x00000000
> > > + * ------------
> > > + * 0x00000001
> > > + */
> > > +.macro init_m3if ctl=0x00000001
> > > + /* M3IF Control Register (M3IFCTL) */
> > > + write32 IMX_M3IF_CTRL_BASE, \ctl
> > > .endm
> > >
> > > #endif /* __ASSEMBLY__ */
> > > diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> > > u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> > > index 823df10..0421237 100644
> > > --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> > > +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> > > @@ -22,37 +22,7 @@
> > > */
> > >
> > > #include <asm/macro.h>
> > > -
> > > -.macro init_aips
> > > - write32 0x43f00000, 0x77777777
> > > - write32 0x43f00004, 0x77777777
> > > - write32 0x43f00000, 0x77777777
> > > - write32 0x53f00004, 0x77777777
> > > -.endm
> > > -
> > > -.macro init_max
> > > - write32 0x43f04000, 0x43210
> > > - write32 0x43f04100, 0x43210
> > > - write32 0x43f04200, 0x43210
> > > - write32 0x43f04300, 0x43210
> > > - write32 0x43f04400, 0x43210
> > > -
> > > - write32 0x43f04010, 0x10
> > > - write32 0x43f04110, 0x10
> > > - write32 0x43f04210, 0x10
> > > - write32 0x43f04310, 0x10
> > > - write32 0x43f04410, 0x10
> > > -
> > > - write32 0x43f04800, 0x0
> > > - write32 0x43f04900, 0x0
> > > - write32 0x43f04a00, 0x0
> > > - write32 0x43f04b00, 0x0
> > > - write32 0x43f04c00, 0x0
> > > -.endm
> > > -
> > > -.macro init_m3if
> > > - write32 0xb8003000, 0x1
> > > -.endm
> > > +#include <asm/arch/macro.h>
> > >
> > > .macro init_clocks
> > > /*
> > > @@ -64,6 +34,8 @@
> > > * 0x00600000 makes CLKO parent clk the USB clk
> > > */
> > > write32 0x53f80064, 0x45600000
> > > +
> > > + /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
> > > write32 0x53f80008, 0x20034000
> > >
> > > /*
> > >
> >
> > You only moved code and set default values for macros: I do not see
> > issues. But checkpatch reports several errors, and, even if some of
> > them
> > can be related to the fact this file is in assembly, most of them
> > can
> > be
> > fixed.
> >
> > Could you take a look ?
>
> Will do.
These errors can be classified into 3 categories:
1. "spaces required around that '=' (ctx:WxV)":
1.1. For macro argument default value, e.g.:
+.macro init_aips mpr=0x77777777
^
This is the convention used by gas' manual. It is also used by U-Boot, e.g. in
arch/arm/lib/memcpy.S. The coma is optional to separate the definition of macro
arguments, in which case not using extra spaces makes even more sense.
1.2. For register assignment, e.g.:
+ ldr r0, =IMX_AIPS1_BASE
^
This is the convention currently used by U-Boot ARM asm code. That would look
weird an unusual with an extra space.
2. "space prohibited before open square bracket '['", e.g.:
+ str r1, [r0, #AIPS_MPR_0_7]
This is the convention currently used by U-Boot ARM asm code. The purpose of
this rule for checkpatch is clearly for array subscripts, which has nothing to
do with this line. Here, the space is good to separate instruction arguments.
IMHO, 2 should definitely not be fixed, 1.2 could but should not, and that can
be discussed for 1.1 (avoiding extra spaces seems to be the general asm rule for
ARM), so that patch could be left unchanged.
What do you think?
That would be good to have official ARM asm coding style rules for U-Boot, but
the coding style used in this patch is also what is used by Linux (e.g. in [1]),
and Linux is the default reference for U-Boot.
Best regards,
Beno?t
[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/kernel/entry-armv.S;h=0f82098c9bfe3618115dcc4e8b2d74d95c7ddcf6;hb=HEAD
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v3] mx25: Clean up lowlevel_init
2012-10-01 19:50 ` Benoît Thébaudeau
@ 2012-10-02 8:12 ` Stefano Babic
0 siblings, 0 replies; 10+ messages in thread
From: Stefano Babic @ 2012-10-02 8:12 UTC (permalink / raw)
To: u-boot
On 01/10/2012 21:50, Beno?t Th?baudeau wrote:
> Hi Stefano,
>
> On Sunday, September 30, 2012 3:47:12 PM, Beno?t Th?baudeau wrote:
>> Hi Stefano,
>>
>> On Sunday, September 30, 2012 3:04:14 PM, Stefano Babic wrote:
>>> On 20/08/2012 21:00, Beno?t Th?baudeau wrote:
>>>> Clean up mx25 lowlevel_init:
>>>> - Add comments.
>>>> - Do not use write32 repeatedly with the same value in order not
>>>> to increase
>>>> code size.
>>>> - Make register values configurable.
>>>> - Use macro parameters with default values instead of literal
>>>> constants.
>>>> - Use defined macros instead of duplicating code.
>>>>
>>>> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>> Cc: John Rigby <jcrigby@gmail.com>
>>>> Cc: Matthias Weisser <weisserm@arcor.de>
>>>> ---
>>>
>>> Hi Beno?t,
>>>
>>>> Changes for v2:
>>>> - Use macro arguments with default values instead of #define's.
>>>> Changes for v3:
>>>> - Fix comment for default MAX MPR value.
>>>>
>>>> .../arch/arm/include/asm/arch-mx25/macro.h | 87
>>>> +++++++++++++++-----
>>>> .../board/karo/tx25/lowlevel_init.S | 34
>>>> +-------
>>>> 2 files changed, 68 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git
>>>> u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
>>>> u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
>>>> index 3b694da..56cae36 100644
>>>> --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
>>>> +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
>>>> @@ -32,32 +32,75 @@
>>>>
>>>> #include <asm/arch/imx-regs.h>
>>>> #include <generated/asm-offsets.h>
>>>> +#include <asm/macro.h>
>>>>
>>>> -.macro init_aips
>>>> - write32 IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
>>>> - write32 IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
>>>> - write32 IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
>>>> - write32 IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
>>>> +/*
>>>> + * AIPS setup - Only setup MPROTx registers.
>>>> + * The PACR default values are good.
>>>> + *
>>>> + * Default argument values:
>>>> + * - MPR: Set all MPROTx to be non-bufferable, trusted for R/W,
>>>> not forced to
>>>> + * user-mode.
>>>> + */
>>>> +.macro init_aips mpr=0x77777777
>>>> + ldr r0, =IMX_AIPS1_BASE
>>>> + ldr r1, =\mpr
>>>> + str r1, [r0, #AIPS_MPR_0_7]
>>>> + str r1, [r0, #AIPS_MPR_8_15]
>>>> + ldr r2, =IMX_AIPS2_BASE
>>>> + str r1, [r2, #AIPS_MPR_0_7]
>>>> + str r1, [r2, #AIPS_MPR_8_15]
>>>> .endm
>>>>
>>>> -.macro init_max
>>>> - write32 IMX_MAX_BASE + MAX_MPR0, 0x43210
>>>> - write32 IMX_MAX_BASE + MAX_MPR1, 0x43210
>>>> - write32 IMX_MAX_BASE + MAX_MPR2, 0x43210
>>>> - write32 IMX_MAX_BASE + MAX_MPR3, 0x43210
>>>> - write32 IMX_MAX_BASE + MAX_MPR4, 0x43210
>>>> -
>>>> - write32 IMX_MAX_BASE + MAX_SGPCR0, 0x10
>>>> - write32 IMX_MAX_BASE + MAX_SGPCR1, 0x10
>>>> - write32 IMX_MAX_BASE + MAX_SGPCR2, 0x10
>>>> - write32 IMX_MAX_BASE + MAX_SGPCR3, 0x10
>>>> - write32 IMX_MAX_BASE + MAX_SGPCR4, 0x10
>>>> +/*
>>>> + * MAX (Multi-Layer AHB Crossbar Switch) setup
>>>> + *
>>>> + * Default argument values:
>>>> + * - MPR: priority is IAHB > DAHB > USBOTG > RTIC > eSDHC2/SDMA
>>>> + * - SGPCR: always park on last master
>>>> + * - MGPCR: restore default values
>>>> + */
>>>> +.macro init_max mpr=0x00043210, sgpcr=0x00000010,
>>>> mgpcr=0x00000000
>>>> + ldr r0, =IMX_MAX_BASE
>>>> + ldr r1, =\mpr
>>>> + str r1, [r0, #MAX_MPR0] /* for S0 */
>>>> + str r1, [r0, #MAX_MPR1] /* for S1 */
>>>> + str r1, [r0, #MAX_MPR2] /* for S2 */
>>>> + str r1, [r0, #MAX_MPR3] /* for S3 */
>>>> + str r1, [r0, #MAX_MPR4] /* for S4 */
>>>> + ldr r1, =\sgpcr
>>>> + str r1, [r0, #MAX_SGPCR0] /* for S0 */
>>>> + str r1, [r0, #MAX_SGPCR1] /* for S1 */
>>>> + str r1, [r0, #MAX_SGPCR2] /* for S2 */
>>>> + str r1, [r0, #MAX_SGPCR3] /* for S3 */
>>>> + str r1, [r0, #MAX_SGPCR4] /* for S4 */
>>>> + ldr r1, =\mgpcr
>>>> + str r1, [r0, #MAX_MGPCR0] /* for M0 */
>>>> + str r1, [r0, #MAX_MGPCR1] /* for M1 */
>>>> + str r1, [r0, #MAX_MGPCR2] /* for M2 */
>>>> + str r1, [r0, #MAX_MGPCR3] /* for M3 */
>>>> + str r1, [r0, #MAX_MGPCR4] /* for M4 */
>>>> +.endm
>>>>
>>>> - write32 IMX_MAX_BASE + MAX_MGPCR0, 0x0
>>>> - write32 IMX_MAX_BASE + MAX_MGPCR1, 0x0
>>>> - write32 IMX_MAX_BASE + MAX_MGPCR2, 0x0
>>>> - write32 IMX_MAX_BASE + MAX_MGPCR3, 0x0
>>>> - write32 IMX_MAX_BASE + MAX_MGPCR4, 0x0
>>>> +/*
>>>> + * M3IF setup
>>>> + *
>>>> + * Default argument values:
>>>> + * - CTL:
>>>> + * MRRP[0] = LCDC on priority list (1 << 0) = 0x00000001
>>>> + * MRRP[1] = MAX1 not on priority list (0 << 1) = 0x00000000
>>>> + * MRRP[2] = MAX0 not on priority list (0 << 2) = 0x00000000
>>>> + * MRRP[3] = USBH not on priority list (0 << 3) = 0x00000000
>>>> + * MRRP[4] = SDMA not on priority list (0 << 4) = 0x00000000
>>>> + * MRRP[5] = eSDHC1/ATA/FEC not on priority list (0 << 5) =
>>>> 0x00000000
>>>> + * MRRP[6] = LCDC/SLCDC/MAX2 not on priority list (0 << 6) =
>>>> 0x00000000
>>>> + * MRRP[7] = CSI not on priority list (0 << 7) = 0x00000000
>>>> + * ------------
>>>> + * 0x00000001
>>>> + */
>>>> +.macro init_m3if ctl=0x00000001
>>>> + /* M3IF Control Register (M3IFCTL) */
>>>> + write32 IMX_M3IF_CTRL_BASE, \ctl
>>>> .endm
>>>>
>>>> #endif /* __ASSEMBLY__ */
>>>> diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
>>>> u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
>>>> index 823df10..0421237 100644
>>>> --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
>>>> +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
>>>> @@ -22,37 +22,7 @@
>>>> */
>>>>
>>>> #include <asm/macro.h>
>>>> -
>>>> -.macro init_aips
>>>> - write32 0x43f00000, 0x77777777
>>>> - write32 0x43f00004, 0x77777777
>>>> - write32 0x43f00000, 0x77777777
>>>> - write32 0x53f00004, 0x77777777
>>>> -.endm
>>>> -
>>>> -.macro init_max
>>>> - write32 0x43f04000, 0x43210
>>>> - write32 0x43f04100, 0x43210
>>>> - write32 0x43f04200, 0x43210
>>>> - write32 0x43f04300, 0x43210
>>>> - write32 0x43f04400, 0x43210
>>>> -
>>>> - write32 0x43f04010, 0x10
>>>> - write32 0x43f04110, 0x10
>>>> - write32 0x43f04210, 0x10
>>>> - write32 0x43f04310, 0x10
>>>> - write32 0x43f04410, 0x10
>>>> -
>>>> - write32 0x43f04800, 0x0
>>>> - write32 0x43f04900, 0x0
>>>> - write32 0x43f04a00, 0x0
>>>> - write32 0x43f04b00, 0x0
>>>> - write32 0x43f04c00, 0x0
>>>> -.endm
>>>> -
>>>> -.macro init_m3if
>>>> - write32 0xb8003000, 0x1
>>>> -.endm
>>>> +#include <asm/arch/macro.h>
>>>>
>>>> .macro init_clocks
>>>> /*
>>>> @@ -64,6 +34,8 @@
>>>> * 0x00600000 makes CLKO parent clk the USB clk
>>>> */
>>>> write32 0x53f80064, 0x45600000
>>>> +
>>>> + /* CCTL: ARM = 399 MHz, AHB = 133 MHz */
>>>> write32 0x53f80008, 0x20034000
>>>>
>>>> /*
>>>>
>>>
>>> You only moved code and set default values for macros: I do not see
>>> issues. But checkpatch reports several errors, and, even if some of
>>> them
>>> can be related to the fact this file is in assembly, most of them
>>> can
>>> be
>>> fixed.
>>>
>>> Could you take a look ?
>>
>> Will do.
>
> These errors can be classified into 3 categories:
>
> 1. "spaces required around that '=' (ctx:WxV)":
> 1.1. For macro argument default value, e.g.:
> +.macro init_aips mpr=0x77777777
> ^
> This is the convention used by gas' manual. It is also used by U-Boot, e.g. in
> arch/arm/lib/memcpy.S. The coma is optional to separate the definition of macro
> arguments, in which case not using extra spaces makes even more sense.
Agree.
>
> 1.2. For register assignment, e.g.:
> + ldr r0, =IMX_AIPS1_BASE
> ^
> This is the convention currently used by U-Boot ARM asm code. That would look
> weird an unusual with an extra space.
This is the convention I use, too. It makes no sense to add an extra space.
>
> 2. "space prohibited before open square bracket '['", e.g.:
> + str r1, [r0, #AIPS_MPR_0_7]
> This is the convention currently used by U-Boot ARM asm code. The purpose of
> this rule for checkpatch is clearly for array subscripts, which has nothing to
> do with this line. Here, the space is good to separate instruction arguments.
Right - much more readable with the space.
>
> IMHO, 2 should definitely not be fixed, 1.2 could but should not, and that can
> be discussed for 1.1 (avoiding extra spaces seems to be the general asm rule for
> ARM), so that patch could be left unchanged.
>
> What do you think?
Personally, it is more readable if we not fix and we leave the patch
unchanged.
> That would be good to have official ARM asm coding style rules for U-Boot, but
> the coding style used in this patch is also what is used by Linux (e.g. in [1]),
> and Linux is the default reference for U-Boot.
Right. Until a new rule is set, coding style for assembly is what Linux
uses.
Thanks for checking - I will apply the patch as it is.
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-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v3] mx25: Clean up lowlevel_init
2012-08-20 19:00 ` [U-Boot] [PATCH v3] " Benoît Thébaudeau
2012-09-30 13:04 ` Stefano Babic
@ 2012-10-02 8:31 ` Stefano Babic
1 sibling, 0 replies; 10+ messages in thread
From: Stefano Babic @ 2012-10-02 8:31 UTC (permalink / raw)
To: u-boot
On 20/08/2012 21:00, Beno?t Th?baudeau wrote:
> Clean up mx25 lowlevel_init:
> - Add comments.
> - Do not use write32 repeatedly with the same value in order not to increase
> code size.
> - Make register values configurable.
> - Use macro parameters with default values instead of literal constants.
> - Use defined macros instead of duplicating code.
>
> Signed-off-by: Beno?t Th?baudeau <benoit.thebaudeau@advansee.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: John Rigby <jcrigby@gmail.com>
> Cc: Matthias Weisser <weisserm@arcor.de>
> ---
Applied to u-boot-imx, next branch, thanks.
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-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-10-02 8:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-14 21:25 [U-Boot] [PATCH] mx25: Clean up lowlevel_init Benoît Thébaudeau
2012-08-15 6:27 ` Matthias Weißer
2012-08-15 13:47 ` Benoît Thébaudeau
2012-08-17 22:04 ` [U-Boot] [PATCH v2] " Benoît Thébaudeau
2012-08-20 19:00 ` [U-Boot] [PATCH v3] " Benoît Thébaudeau
2012-09-30 13:04 ` Stefano Babic
2012-09-30 13:47 ` Benoît Thébaudeau
2012-10-01 19:50 ` Benoît Thébaudeau
2012-10-02 8:12 ` Stefano Babic
2012-10-02 8:31 ` Stefano Babic
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox