[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Dirk Behme]

Convert readl/writel to base + offset style. Replace hardcoded values with
macros.

No functional change.

Signed-off-by: Dirk Behme <dirk.behme@googlemail.com>

---
 cpu/arm_cortexa8/omap3/board.c      |   56 ++++++++-------
 cpu/arm_cortexa8/omap3/clock.c      |   47 ++++++++-----
 cpu/arm_cortexa8/omap3/interrupts.c |   14 +--
 cpu/arm_cortexa8/omap3/mem.c        |   53 ++++++++------
 cpu/arm_cortexa8/omap3/sys_info.c   |   14 ++-
 include/asm-arm/arch-omap3/cpu.h    |  129 +++++++++++++++++++-----------------
 include/asm-arm/arch-omap3/mem.h    |    8 +-
 include/asm-arm/arch-omap3/omap3.h  |    2 
 8 files changed, 180 insertions(+), 143 deletions(-)

Index: u-boot-arm/cpu/arm_cortexa8/omap3/mem.c
===================================================================
--- u-boot-arm.orig/cpu/arm_cortexa8/omap3/mem.c
+++ u-boot-arm/cpu/arm_cortexa8/omap3/mem.c
@@ -106,6 +106,8 @@ u32 *onenand_cs_base;
 
 #endif
 
+static u32 *sdrc_base = (u32 *)OMAP34XX_SDRC_BASE;
+
 /**************************************************************************
  * make_cs1_contiguous() - for es2 and above remap cs1 behind cs0 to allow
  *  command line mem=xyz use all memory with out discontinuous support
@@ -120,7 +122,7 @@ void make_cs1_contiguous(void)
 	size /= SZ_32M;			/* find size to offset CS1 */
 	a_add_high = (size & 3) << 8;	/* set up low field */
 	a_add_low = (size & 0x3C) >> 2;	/* set up high field */
-	writel((a_add_high | a_add_low), SDRC_CS_CFG);
+	writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));
 
 }
 
@@ -175,36 +177,43 @@ void do_sdrc_init(u32 offset, u32 early)
 
 	if (early) {
 		/* reset sdrc controller */
-		writel(SOFTRESET, SDRC_SYSCONFIG);
-		wait_on_value(RESETDONE, RESETDONE, SDRC_STATUS, 12000000);
-		writel(0, SDRC_SYSCONFIG);
+		writel(SOFTRESET, sdrc_base + OFFS(SDRC_SYSCONFIG));
+		wait_on_value(RESETDONE, RESETDONE, SDRC_STATUS_REG, 12000000);
+		writel(0, sdrc_base + OFFS(SDRC_SYSCONFIG));
 
 		/* setup sdrc to ball mux */
-		writel(SDP_SDRC_SHARING, SDRC_SHARING);
+		writel(SDP_SDRC_SHARING, sdrc_base + OFFS(SDRC_SHARING));
 
-		/* Disble Power Down of CKE cuz of 1 CKE on combo part */
-		writel(0x00000081, SDRC_POWER);
+		/* Disable Power Down of CKE cuz of 1 CKE on combo part */
+		writel(SRFRONRESET | PAGEPOLICY_HIGH,
+			sdrc_base + OFFS(SDRC_POWER));
 
-		writel(0x0000A, SDRC_DLLA_CTRL);
+		writel(ENADLL | DLLPHASE_90, sdrc_base + OFFS(SDRC_DLLA_CTRL));
 		sdelay(0x20000);
 	}
 
-	writel(0x02584099,	SDRC_MCFG_0 + offset);
-	writel(0x4e201,		SDRC_RFR_CTRL + offset);
-	writel(0xaa9db4c6,	SDRC_ACTIM_CTRLA_0 + actim_offs);
-	writel(0x11517,		SDRC_ACTIM_CTRLB_0 + actim_offs);
-
-	writel(CMD_NOP,		SDRC_MANUAL_0 + offset);
-	writel(CMD_PRECHARGE,	SDRC_MANUAL_0 + offset);
-	writel(CMD_AUTOREFRESH,	SDRC_MANUAL_0 + offset);
-	writel(CMD_AUTOREFRESH,	SDRC_MANUAL_0 + offset);
-
-	/*  CAS latency 3, Write Burst = Read Burst, Serial Mode,
-	    Burst length = 4 */
-	writel(0x00000032,	SDRC_MR_0 + offset);
+	writel(RASWIDTH_13BITS | CASWIDTH_10BITS | ADDRMUXLEGACY |
+		RAMSIZE_128 | BANKALLOCATION | B32NOT16 | B32NOT16 |
+		DEEPPD | DDR_SDRAM, sdrc_base + OFFS(SDRC_MCFG_0 + offset));
+	writel(ARCV | ARE_ARCV_1, sdrc_base + OFFS(SDRC_RFR_CTRL + offset));
+	writel(V_ACTIMA_165, sdrc_base +
+			OFFS(SDRC_ACTIM_CTRLA_0 + actim_offs));
+	writel(V_ACTIMB_165, sdrc_base +
+			OFFS(SDRC_ACTIM_CTRLB_0 + actim_offs));
+
+	writel(CMD_NOP, sdrc_base + OFFS(SDRC_MANUAL_0 + offset));
+	writel(CMD_PRECHARGE, sdrc_base + OFFS(SDRC_MANUAL_0 + offset));
+	writel(CMD_AUTOREFRESH, sdrc_base + OFFS(SDRC_MANUAL_0 + offset));
+	writel(CMD_AUTOREFRESH, sdrc_base + OFFS(SDRC_MANUAL_0 + offset));
+
+	/*
+	 * CAS latency 3, Write Burst = Read Burst, Serial Mode,
+	 * Burst length = 4
+	 */
+	writel(CASL3 | BURSTLENGTH4, sdrc_base + OFFS(SDRC_MR_0 + offset));
 
 	if (!mem_ok(offset))
-		writel(0, SDRC_MCFG_0 + offset);
+		writel(0, sdrc_base + OFFS(SDRC_MCFG_0 + offset));
 }
 
 void enable_gpmc_config(u32 *gpmc_config, u32 *gpmc_cs_base, u32 base,
Index: u-boot-arm/cpu/arm_cortexa8/omap3/sys_info.c
===================================================================
--- u-boot-arm.orig/cpu/arm_cortexa8/omap3/sys_info.c
+++ u-boot-arm/cpu/arm_cortexa8/omap3/sys_info.c
@@ -33,6 +33,8 @@
 
 extern omap3_sysinfo sysinfo;
 static u32 *gpmc_base = (u32 *)GPMC_BASE;
+static u32 *sdrc_base = (u32 *)OMAP34XX_SDRC_BASE;
+static u32 *ctrl_base = (u32 *)OMAP34XX_CTRL_BASE;
 
 /******************************************
  * get_cpu_rev(void) - extract version info
@@ -59,7 +61,8 @@ u32 get_cpu_rev(void)
  ****************************************************/
 u32 is_mem_sdr(void)
 {
-	if (readl(SDRC_MR_0 + SDRC_CS0_OSET) == SDP_SDRC_MR_0_SDR)
+	if (readl(sdrc_base + OFFS(SDRC_MR_0 + SDRC_CS0_OSET)) ==
+			SDP_SDRC_MR_0_SDR)
 		return 1;
 	return 0;
 }
@@ -72,7 +75,7 @@ u32 get_sdr_cs_size(u32 offset)
 	u32 size;
 
 	/* get ram size field */
-	size = readl(SDRC_MCFG_0 + offset) >> 8;
+	size = readl(sdrc_base + OFFS(SDRC_MCFG_0 + offset)) >> 8;
 	size &= 0x3FF;		/* remove unwanted bits */
 	size *= SZ_2M;		/* find size in MB */
 	return size;
@@ -88,7 +91,7 @@ u32 get_sdr_cs_offset(u32 cs)
 	if (!cs)
 		return 0;
 
-	offset = readl(SDRC_CS_CFG);
+	offset = readl(sdrc_base + OFFS(SDRC_CS_CFG));
 	offset = (offset & 15) << 27 | (offset & 0x30) >> 17;
 
 	return offset;
@@ -240,7 +243,7 @@ u32 is_running_in_sdram(void)
  ***************************************************************/
 u32 get_boot_type(void)
 {
-	return (readl(CONTROL_STATUS) & SYSBOOT_MASK);
+	return (readl(ctrl_base + OFFS(CONTROL_STATUS)) & SYSBOOT_MASK);
 }
 
 /*************************************************************
@@ -248,5 +251,6 @@ u32 get_boot_type(void)
  *************************************************************/
 u32 get_device_type(void)
 {
-	return ((readl(CONTROL_STATUS) & (DEVICE_MASK)) >> 8);
+	return ((readl(ctrl_base + OFFS(CONTROL_STATUS)) &
+		(DEVICE_MASK)) >> 8);
 }
Index: u-boot-arm/include/asm-arm/arch-omap3/cpu.h
===================================================================
--- u-boot-arm.orig/include/asm-arm/arch-omap3/cpu.h
+++ u-boot-arm/include/asm-arm/arch-omap3/cpu.h
@@ -29,14 +29,7 @@
 
 /* Register offsets of common modules */
 /* Control */
-#define CONTROL_STATUS			(OMAP34XX_CTRL_BASE + 0x2F0)
-#define OMAP34XX_MCR			(OMAP34XX_CTRL_BASE + 0x8C)
-#define CONTROL_SCALABLE_OMAP_STATUS	(OMAP34XX_CTRL_BASE + 0x44C)
-#define CONTROL_SCALABLE_OMAP_OCP	(OMAP34XX_CTRL_BASE + 0x534)
-
-/* Tap Information */
-#define TAP_IDCODE_REG		(OMAP34XX_TAP_BASE + 0x204)
-#define PRODUCTION_ID		(OMAP34XX_TAP_BASE + 0x208)
+#define CONTROL_STATUS		0x2F0
 
 /* device type */
 #define DEVICE_MASK		(0x7 << 8)
@@ -104,36 +97,52 @@
 						/* (actual size small port) */
 
 /* SMS */
-#define SMS_SYSCONFIG		(OMAP34XX_SMS_BASE + 0x10)
-#define SMS_RG_ATT0		(OMAP34XX_SMS_BASE + 0x48)
-#define SMS_CLASS_ARB0		(OMAP34XX_SMS_BASE + 0xD0)
+#define SMS_SYSCONFIG		0x10
+#define SMS_RG_ATT0		0x48
+#define SMS_CLASS_ARB0		0xD0
 #define BURSTCOMPLETE_GROUP7	(0x1 << 31)
 
 /* SDRC */
-#define SDRC_SYSCONFIG		(OMAP34XX_SDRC_BASE + 0x10)
-#define SDRC_STATUS		(OMAP34XX_SDRC_BASE + 0x14)
-#define SDRC_CS_CFG		(OMAP34XX_SDRC_BASE + 0x40)
-#define SDRC_SHARING		(OMAP34XX_SDRC_BASE + 0x44)
-#define SDRC_DLLA_CTRL		(OMAP34XX_SDRC_BASE + 0x60)
-#define SDRC_DLLA_STATUS	(OMAP34XX_SDRC_BASE + 0x64)
-#define SDRC_DLLB_CTRL		(OMAP34XX_SDRC_BASE + 0x68)
-#define SDRC_DLLB_STATUS	(OMAP34XX_SDRC_BASE + 0x6C)
-#define DLLPHASE		(0x1 << 1)
+#define SDRC_SYSCONFIG		0x10
+#define SDRC_STATUS		0x14
+#define SDRC_STATUS_REG		(OMAP34XX_SDRC_BASE + SDRC_STATUS)
+#define SDRC_CS_CFG		0x40
+#define SDRC_SHARING		0x44
+#define SDRC_DLLA_CTRL		0x60
+#define SDRC_DLLA_STATUS	0x64
+#define SDRC_DLLB_CTRL		0x68
+#define SDRC_DLLB_STATUS	0x6C
+#define DLLPHASE_90		(0x1 << 1)
 #define LOADDLL			(0x1 << 2)
+#define ENADLL			(0x1 << 3)
 #define DLL_DELAY_MASK		0xFF00
 #define DLL_NO_FILTER_MASK	((0x1 << 9) | (0x1 << 8))
 
-#define SDRC_POWER		(OMAP34XX_SDRC_BASE + 0x70)
+#define SDRC_POWER		0x70
+#define PAGEPOLICY_HIGH		(0x1 << 0)
+#define SRFRONRESET		(0x1 << 7)
 #define WAKEUPPROC		(0x1 << 26)
 
-#define SDRC_MCFG_0		(OMAP34XX_SDRC_BASE + 0x80)
-#define SDRC_MR_0		(OMAP34XX_SDRC_BASE + 0x84)
-#define SDRC_ACTIM_CTRLA_0	(OMAP34XX_SDRC_BASE + 0x9C)
-#define SDRC_ACTIM_CTRLB_0	(OMAP34XX_SDRC_BASE + 0xA0)
-#define SDRC_ACTIM_CTRLA_1	(OMAP34XX_SDRC_BASE + 0xC4)
-#define SDRC_ACTIM_CTRLB_1	(OMAP34XX_SDRC_BASE + 0xC8)
-#define SDRC_RFR_CTRL		(OMAP34XX_SDRC_BASE + 0xA4)
-#define SDRC_MANUAL_0		(OMAP34XX_SDRC_BASE + 0xA8)
+#define SDRC_MCFG_0		0x80
+#define DDR_SDRAM		(0x1 << 0)
+#define DEEPPD			(0x1 << 3)
+#define B32NOT16		(0x1 << 4)
+#define BANKALLOCATION		(0x2 << 6)
+#define RAMSIZE_128		(0x40 << 8) /* RAM size in 2MB chunks */
+#define ADDRMUXLEGACY		(0x1 << 19)
+#define CASWIDTH_10BITS		(0x5 << 20)
+#define RASWIDTH_13BITS		(0x2 << 24)
+#define SDRC_MR_0		0x84
+#define BURSTLENGTH4		(0x2 << 0)
+#define CASL3			(0x3 << 4)
+#define SDRC_ACTIM_CTRLA_0	0x9C
+#define SDRC_ACTIM_CTRLB_0	0xA0
+#define SDRC_ACTIM_CTRLA_1	0xC4
+#define SDRC_ACTIM_CTRLB_1	0xC8
+#define SDRC_RFR_CTRL		0xA4
+#define ARE_ARCV_1		(0x1 << 0)
+#define ARCV			(0x4e2 << 8) /* Autorefresh count */
+#define SDRC_MANUAL_0		0xA8
 #define OMAP34XX_SDRC_CS0	0x80000000
 #define OMAP34XX_SDRC_CS1	0xA0000000
 #define CMD_NOP			0x0
@@ -174,6 +183,7 @@
 #define WD_UNLOCK2		0x5555
 
 /* PRCM */
+#define PRCM_BASE		0x48004000
 #define CM_FCLKEN_IVA2		0x48004000
 #define CM_CLKEN_PLL_IVA2	0x48004004
 #define CM_IDLEST_PLL_IVA2	0x48004024
@@ -187,16 +197,22 @@
 #define CM_ICLKEN1_CORE		0x48004a10
 #define CM_ICLKEN2_CORE		0x48004a14
 #define CM_CLKSEL_CORE		0x48004a40
+#define CLKSEL_CORE		0xa40
 #define CM_FCLKEN_GFX		0x48004b00
 #define CM_ICLKEN_GFX		0x48004b10
 #define CM_CLKSEL_GFX		0x48004b40
 #define CM_FCLKEN_WKUP		0x48004c00
+#define FCLKEN_WKUP		0xc00
 #define CM_ICLKEN_WKUP		0x48004c10
+#define ICLKEN_WKUP		0xc10
 #define CM_CLKSEL_WKUP		0x48004c40
+#define CLKSEL_WKUP		0xc40
 #define CM_IDLEST_WKUP		0x48004c20
 #define CM_CLKEN_PLL		0x48004d00
+#define CLKEN_PLL		0xd00
 #define CM_IDLEST_CKGEN		0x48004d20
 #define CM_CLKSEL1_PLL		0x48004d40
+#define CLKSEL1_PLL		0xd40
 #define CM_CLKSEL2_PLL		0x48004d44
 #define CM_CLKSEL3_PLL		0x48004d48
 #define CM_FCLKEN_DSS		0x48004e00
@@ -210,9 +226,11 @@
 #define CM_CLKSEL_PER		0x48005040
 #define CM_CLKSEL1_EMU		0x48005140
 
+#define PRM_BASE		0x48306000
 #define PRM_CLKSEL		0x48306d40
 #define PRM_RSTCTRL		0x48307250
 #define PRM_CLKSRC_CTRL		0x48307270
+#define CLKSRC_CTRL		0x1270
 #define SYSCLKDIV_1		(0x1 << 6)
 #define SYSCLKDIV_2		(0x1 << 7)
 
@@ -244,35 +262,30 @@
 #define PM_OCM_ROM_BASE_ADDR_ARM	(SMX_APE_BASE + 0x12C00)
 #define PM_IVA2_BASE_ADDR_ARM		(SMX_APE_BASE + 0x14000)
 
-#define RT_REQ_INFO_PERMISSION_1	(PM_RT_APE_BASE_ADDR_ARM + 0x68)
-#define RT_READ_PERMISSION_0		(PM_RT_APE_BASE_ADDR_ARM + 0x50)
-#define RT_WRITE_PERMISSION_0		(PM_RT_APE_BASE_ADDR_ARM + 0x58)
-#define RT_ADDR_MATCH_1			(PM_RT_APE_BASE_ADDR_ARM + 0x60)
-
-#define GPMC_REQ_INFO_PERMISSION_0	(PM_GPMC_BASE_ADDR_ARM + 0x48)
-#define GPMC_READ_PERMISSION_0		(PM_GPMC_BASE_ADDR_ARM + 0x50)
-#define GPMC_WRITE_PERMISSION_0		(PM_GPMC_BASE_ADDR_ARM + 0x58)
-
-#define OCM_REQ_INFO_PERMISSION_0	(PM_OCM_RAM_BASE_ADDR_ARM + 0x48)
-#define OCM_READ_PERMISSION_0		(PM_OCM_RAM_BASE_ADDR_ARM + 0x50)
-#define OCM_WRITE_PERMISSION_0		(PM_OCM_RAM_BASE_ADDR_ARM + 0x58)
-#define OCM_ADDR_MATCH_2		(PM_OCM_RAM_BASE_ADDR_ARM + 0x80)
-
-#define IVA2_REQ_INFO_PERMISSION_0	(PM_IVA2_BASE_ADDR_ARM + 0x48)
-#define IVA2_READ_PERMISSION_0		(PM_IVA2_BASE_ADDR_ARM + 0x50)
-#define IVA2_WRITE_PERMISSION_0		(PM_IVA2_BASE_ADDR_ARM + 0x58)
-
-#define IVA2_REQ_INFO_PERMISSION_1	(PM_IVA2_BASE_ADDR_ARM + 0x68)
-#define IVA2_READ_PERMISSION_1		(PM_IVA2_BASE_ADDR_ARM + 0x70)
-#define IVA2_WRITE_PERMISSION_1		(PM_IVA2_BASE_ADDR_ARM + 0x78)
-
-#define IVA2_REQ_INFO_PERMISSION_2	(PM_IVA2_BASE_ADDR_ARM + 0x88)
-#define IVA2_READ_PERMISSION_2		(PM_IVA2_BASE_ADDR_ARM + 0x90)
-#define IVA2_WRITE_PERMISSION_2		(PM_IVA2_BASE_ADDR_ARM + 0x98)
-
-#define IVA2_REQ_INFO_PERMISSION_3	(PM_IVA2_BASE_ADDR_ARM + 0xA8)
-#define IVA2_READ_PERMISSION_3		(PM_IVA2_BASE_ADDR_ARM + 0xB0)
-#define IVA2_WRITE_PERMISSION_3		(PM_IVA2_BASE_ADDR_ARM + 0xB8)
+#define RT_REQ_INFO_PERMISSION_1	0x68
+#define RT_READ_PERMISSION_0		0x50
+#define RT_WRITE_PERMISSION_0		0x58
+#define RT_ADDR_MATCH_1			0x60
+
+#define GPMC_REQ_INFO_PERMISSION_0	0x48
+#define GPMC_READ_PERMISSION_0		0x50
+#define GPMC_WRITE_PERMISSION_0		0x58
+
+#define OCM_REQ_INFO_PERMISSION_0	0x48
+#define OCM_READ_PERMISSION_0		0x50
+#define OCM_WRITE_PERMISSION_0		0x58
+#define OCM_ADDR_MATCH_2		0x80
+
+#define IVA2_REQ_INFO_PERMISSION_0	0x48
+#define IVA2_READ_PERMISSION_0		0x50
+#define IVA2_WRITE_PERMISSION_0		0x58
+
+/* Permission values for registers -Full fledged permissions to all */
+#define UNLOCK_1			0xFFFFFFFF
+#define UNLOCK_2			0x00000000
+#define UNLOCK_3			0x0000FFFF
+
+#define NOT_EARLY			0
 
 /* I2C base */
 #define I2C_BASE1		(OMAP34XX_CORE_L4_IO_BASE + 0x70000)
Index: u-boot-arm/include/asm-arm/arch-omap3/mem.h
===================================================================
--- u-boot-arm.orig/include/asm-arm/arch-omap3/mem.h
+++ u-boot-arm/include/asm-arm/arch-omap3/mem.h
@@ -80,16 +80,16 @@ typedef enum {
 #define TRP_165		3
 #define TRAS_165	7
 #define TRC_165		10
-#define TRFC_165	12
+#define TRFC_165	21
 #define V_ACTIMA_165	((TRFC_165 << 27) | (TRC_165 << 22) | \
 			(TRAS_165 << 18) | (TRP_165 << 15) |  \
 			(TRCD_165 << 12) | (TRRD_165 << 9) |  \
 			(TDPL_165 << 6) | (TDAL_165))
 
 #define TWTR_165	1
-#define TCKE_165	2
-#define TXP_165		2
-#define XSR_165		20
+#define TCKE_165	1
+#define TXP_165		5
+#define XSR_165		23
 #define V_ACTIMB_165	(((TCKE_165 << 12) | (XSR_165 << 0)) |	\
 			(TXP_165 << 8) | (TWTR_165 << 16))
 
Index: u-boot-arm/cpu/arm_cortexa8/omap3/board.c
===================================================================
--- u-boot-arm.orig/cpu/arm_cortexa8/omap3/board.c
+++ u-boot-arm/cpu/arm_cortexa8/omap3/board.c
@@ -37,13 +37,6 @@
 #include <asm/arch/sys_proto.h>
 #include <asm/arch/mem.h>
 
-#define NOT_EARLY 0
-
-/* Permission values for registers -Full fledged permissions to all */
-#define UNLOCK_1 0xFFFFFFFF
-#define UNLOCK_2 0x00000000
-#define UNLOCK_3 0x0000FFFF
-
 extern omap3_sysinfo sysinfo;
 
 /******************************************************************************
@@ -63,27 +56,34 @@ static inline void delay(unsigned long l
  *****************************************************************************/
 void secure_unlock_mem(void)
 {
+	u32 *pm_rt_ape_base = (u32 *)PM_RT_APE_BASE_ADDR_ARM;
+	u32 *pm_gpmc_base = (u32 *)PM_GPMC_BASE_ADDR_ARM;
+	u32 *pm_ocm_ram_base = (u32 *)PM_OCM_RAM_BASE_ADDR_ARM;
+	u32 *pm_iva2_base = (u32 *)PM_IVA2_BASE_ADDR_ARM;
+	u32 *sms_base = (u32 *)OMAP34XX_SMS_BASE;
+
 	/* Protection Module Register Target APE (PM_RT) */
-	writel(UNLOCK_1, RT_REQ_INFO_PERMISSION_1);
-	writel(UNLOCK_1, RT_READ_PERMISSION_0);
-	writel(UNLOCK_1, RT_WRITE_PERMISSION_0);
-	writel(UNLOCK_2, RT_ADDR_MATCH_1);
-
-	writel(UNLOCK_3, GPMC_REQ_INFO_PERMISSION_0);
-	writel(UNLOCK_3, GPMC_READ_PERMISSION_0);
-	writel(UNLOCK_3, GPMC_WRITE_PERMISSION_0);
-
-	writel(UNLOCK_3, OCM_REQ_INFO_PERMISSION_0);
-	writel(UNLOCK_3, OCM_READ_PERMISSION_0);
-	writel(UNLOCK_3, OCM_WRITE_PERMISSION_0);
-	writel(UNLOCK_2, OCM_ADDR_MATCH_2);
+	writel(UNLOCK_1, pm_rt_ape_base + OFFS(RT_REQ_INFO_PERMISSION_1));
+	writel(UNLOCK_1, pm_rt_ape_base + OFFS(RT_READ_PERMISSION_0));
+	writel(UNLOCK_1, pm_rt_ape_base + OFFS(RT_WRITE_PERMISSION_0));
+	writel(UNLOCK_2, pm_rt_ape_base + OFFS(RT_ADDR_MATCH_1));
+
+	writel(UNLOCK_3, pm_gpmc_base + OFFS(GPMC_REQ_INFO_PERMISSION_0));
+	writel(UNLOCK_3, pm_gpmc_base + OFFS(GPMC_READ_PERMISSION_0));
+	writel(UNLOCK_3, pm_gpmc_base + OFFS(GPMC_WRITE_PERMISSION_0));
+
+	writel(UNLOCK_3, pm_ocm_ram_base + OFFS(OCM_REQ_INFO_PERMISSION_0));
+	writel(UNLOCK_3, pm_ocm_ram_base + OFFS(OCM_READ_PERMISSION_0));
+	writel(UNLOCK_3, pm_ocm_ram_base + OFFS(OCM_WRITE_PERMISSION_0));
+	writel(UNLOCK_2, pm_ocm_ram_base + OFFS(OCM_ADDR_MATCH_2));
 
 	/* IVA Changes */
-	writel(UNLOCK_3, IVA2_REQ_INFO_PERMISSION_0);
-	writel(UNLOCK_3, IVA2_READ_PERMISSION_0);
-	writel(UNLOCK_3, IVA2_WRITE_PERMISSION_0);
+	writel(UNLOCK_3, pm_iva2_base + OFFS(IVA2_REQ_INFO_PERMISSION_0));
+	writel(UNLOCK_3, pm_iva2_base + OFFS(IVA2_READ_PERMISSION_0));
+	writel(UNLOCK_3, pm_iva2_base + OFFS(IVA2_WRITE_PERMISSION_0));
 
-	writel(UNLOCK_1, SMS_RG_ATT0);	/* SDRC region 0 public */
+	/* SDRC region 0 public */
+	writel(UNLOCK_1, sms_base + OFFS(SMS_RG_ATT0));
 }
 
 /******************************************************************************
@@ -210,7 +210,7 @@ void s_init(void)
 #endif
 	/*
 	 * Writing to AuxCR in U-boot using SMI for GP DEV
-	 * Currently SMI in Kernel on ES2 devices seems to have an isse
+	 * Currently SMI in Kernel on ES2 devices seems to have an issue
 	 * Once that is resolved, we can postpone this config to kernel
 	 */
 	if (get_device_type() == GP_DEVICE)
@@ -245,6 +245,8 @@ void wait_for_command_complete(unsigned 
  *****************************************************************************/
 void watchdog_init(void)
 {
+	u32 *wd2_base = (u32 *)WD2_BASE;
+
 	/*
 	 * There are 3 watch dogs WD1=Secure, WD2=MPU, WD3=IVA. WD1 is
 	 * either taken care of by ROM (HS/EMU) or not accessible (GP).
@@ -256,9 +258,9 @@ void watchdog_init(void)
 	sr32(CM_ICLKEN_WKUP, 5, 1, 1);
 	wait_on_value(ST_WDT2, 0x20, CM_IDLEST_WKUP, 5); /* some issue here */
 
-	writel(WD_UNLOCK1, WD2_BASE + WSPR);
+	writel(WD_UNLOCK1, wd2_base + OFFS(WSPR));
 	wait_for_command_complete(WD2_BASE);
-	writel(WD_UNLOCK2, WD2_BASE + WSPR);
+	writel(WD_UNLOCK2, wd2_base + OFFS(WSPR));
 }
 
 /******************************************************************************
Index: u-boot-arm/cpu/arm_cortexa8/omap3/interrupts.c
===================================================================
--- u-boot-arm.orig/cpu/arm_cortexa8/omap3/interrupts.c
+++ u-boot-arm/cpu/arm_cortexa8/omap3/interrupts.c
@@ -38,9 +38,6 @@
 
 #define TIMER_LOAD_VAL 0
 
-/* macro to read the 32 bit timer */
-#define READ_TIMER (*(volatile ulong *)(CONFIG_SYS_TIMERBASE+TCRR))
-
 #ifdef CONFIG_USE_IRQ
 /* enable IRQ interrupts */
 void enable_interrupts(void)
@@ -170,15 +167,16 @@ void do_irq(struct pt_regs *pt_regs)
 
 static ulong timestamp;
 static ulong lastinc;
+static u32 *timer_base = (u32 *)CONFIG_SYS_TIMERBASE;
 
 /* nothing really to do with interrupts, just starts up a counter. */
 int interrupt_init(void)
 {
 	/* start the counter ticking up, reload value on overflow */
-	writel(TIMER_LOAD_VAL, CONFIG_SYS_TIMERBASE + TLDR);
+	writel(TIMER_LOAD_VAL, timer_base + OFFS(TLDR));
 	/* enable timer */
 	writel((CONFIG_SYS_PVT << 2) | TCLR_PRE | TCLR_AR | TCLR_ST,
-		CONFIG_SYS_TIMERBASE + TCLR);
+		timer_base + OFFS(TCLR));
 
 	reset_timer_masked();	/* init the timestamp and lastinc value */
 
@@ -233,14 +231,14 @@ void udelay(unsigned long usec)
 
 void reset_timer_masked(void)
 {
-	/* reset time */
-	lastinc = READ_TIMER;	/* capture current incrementer value time */
+	/* reset time, capture current incrementer value time */
+	lastinc = readl(timer_base + OFFS(TCRR));
 	timestamp = 0;		/* start "advancing" time stamp from 0 */
 }
 
 ulong get_timer_masked(void)
 {
-	ulong now = READ_TIMER;	/* current tick value */
+	ulong now = readl(timer_base + OFFS(TCRR)); /* current tick value */
 
 	if (now >= lastinc)	/* normal mode (non roll) */
 		/* move stamp fordward with absoulte diff ticks */
Index: u-boot-arm/cpu/arm_cortexa8/omap3/clock.c
===================================================================
--- u-boot-arm.orig/cpu/arm_cortexa8/omap3/clock.c
+++ u-boot-arm/cpu/arm_cortexa8/omap3/clock.c
@@ -41,36 +41,46 @@
 u32 get_osc_clk_speed(void)
 {
 	u32 start, cstart, cend, cdiff, val;
+	u32 *prcm_base = (u32 *)PRCM_BASE;
+	u32 *prm_base = (u32 *)PRM_BASE;
+	u32 *gpt1_base = (u32 *)OMAP34XX_GPT1;
+	u32 *s32k_base = (u32 *)SYNC_32KTIMER_BASE;
 
-	val = readl(PRM_CLKSRC_CTRL);
+	val = readl(prm_base + OFFS(CLKSRC_CTRL));
 
 	/* If SYS_CLK is being divided by 2, remove for now */
 	val = (val & (~SYSCLKDIV_2)) | SYSCLKDIV_1;
-	writel(val, PRM_CLKSRC_CTRL);
+	writel(val, prm_base + OFFS(CLKSRC_CTRL));
 
 	/* enable timer2 */
-	val = readl(CM_CLKSEL_WKUP) | CLKSEL_GPT1;
-	writel(val, CM_CLKSEL_WKUP);	/* select sys_clk for GPT1 */
+	val = readl(prcm_base + OFFS(CLKSEL_WKUP)) | CLKSEL_GPT1;
+
+	/* select sys_clk for GPT1 */
+	writel(val, prcm_base + OFFS(CLKSEL_WKUP));
 
 	/* Enable I and F Clocks for GPT1 */
-	val = readl(CM_ICLKEN_WKUP) | EN_GPT1 | EN_32KSYNC;
-	writel(val, CM_ICLKEN_WKUP);
-	val = readl(CM_FCLKEN_WKUP) | EN_GPT1;
-	writel(val, CM_FCLKEN_WKUP);
+	val = readl(prcm_base + OFFS(ICLKEN_WKUP)) | EN_GPT1 | EN_32KSYNC;
+	writel(val, prcm_base + OFFS(ICLKEN_WKUP));
+	val = readl(prcm_base + OFFS(FCLKEN_WKUP)) | EN_GPT1;
+	writel(val, prcm_base + OFFS(FCLKEN_WKUP));
 
-	writel(0, OMAP34XX_GPT1 + TLDR);	/* start counting at 0 */
-	writel(GPT_EN, OMAP34XX_GPT1 + TCLR);	/* enable clock */
+	writel(0, gpt1_base + OFFS(TLDR));	/* start counting at 0 */
+	writel(GPT_EN, gpt1_base + OFFS(TCLR));	/* enable clock */
 
 	/* enable 32kHz source, determine sys_clk via gauging */
-	start = 20 + readl(S32K_CR);		/* start time in 20 cycles */
-	while (readl(S32K_CR) < start) ;	/* dead loop till start time */
+
+	/* start time in 20 cycles */
+	start = 20 + readl(s32k_base + OFFS(S32K_CR));
+
+	/* dead loop till start time */
+	while (readl(s32k_base + OFFS(S32K_CR)) < start);
 
 	/* get start sys_clk count */
-	cstart = readl(OMAP34XX_GPT1 + TCRR);
+	cstart = readl(gpt1_base + OFFS(TCRR));
 
 	/* wait for 40 cycles */
-	while (readl(S32K_CR) < (start + 20)) ;
-	cend = readl(OMAP34XX_GPT1 + TCRR);	/* get end sys_clk count */
+	while (readl(s32k_base + OFFS(S32K_CR)) < (start + 20)) ;
+	cend = readl(gpt1_base + OFFS(TCRR));	/* get end sys_clk count */
 	cdiff = cend - cstart;			/* get elapsed ticks */
 
 	/* based on number of ticks assign speed */
@@ -123,6 +133,7 @@ void prcm_init(void)
 	int xip_safe, p0, p1, p2, p3;
 	u32 osc_clk = 0, sys_clkin_sel;
 	u32 clk_index, sil_index;
+	u32 *prcm_base = (u32 *)PRCM_BASE;
 	dpll_param *dpll_param_p;
 
 	f_lock_pll = (void *) ((u32) &_end_vect - (u32) &_start +
@@ -199,17 +210,17 @@ void prcm_init(void)
 		 * if running from flash, jump to small relocated code
 		 * area in SRAM.
 		 */
-		p0 = readl(CM_CLKEN_PLL);
+		p0 = readl(prcm_base + OFFS(CLKEN_PLL));
 		sr32((u32) &p0, 0, 3, PLL_FAST_RELOCK_BYPASS);
 		sr32((u32) &p0, 4, 4, dpll_param_p->fsel);	/* FREQSEL */
 
-		p1 = readl(CM_CLKSEL1_PLL);
+		p1 = readl(prcm_base + OFFS(CLKSEL1_PLL));
 		sr32((u32) &p1, 27, 2, dpll_param_p->m2);	/* Set M2 */
 		sr32((u32) &p1, 16, 11, dpll_param_p->m);	/* Set M */
 		sr32((u32) &p1, 8, 7, dpll_param_p->n);		/* Set N */
 		sr32((u32) &p1, 6, 1, 0);	/* set source for 96M */
 
-		p2 = readl(CM_CLKSEL_CORE);
+		p2 = readl(prcm_base + OFFS(CLKSEL_CORE));
 		sr32((u32) &p2, 8, 4, CORE_SSI_DIV);	/* ssi */
 		sr32((u32) &p2, 4, 2, CORE_FUSB_DIV);	/* fsusb */
 		sr32((u32) &p2, 2, 2, CORE_L4_DIV);	/* l4 */
Index: u-boot-arm/include/asm-arm/arch-omap3/omap3.h
===================================================================
--- u-boot-arm.orig/include/asm-arm/arch-omap3/omap3.h
+++ u-boot-arm/include/asm-arm/arch-omap3/omap3.h
@@ -75,7 +75,7 @@
 
 /* 32KTIMER */
 #define SYNC_32KTIMER_BASE		0x48320000
-#define S32K_CR				(SYNC_32KTIMER_BASE + 0x10)
+#define S32K_CR				0x10
 
 /* OMAP3 GPIO registers */
 #define OMAP34XX_GPIO1_BASE		0x48310000

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Wolfgang Denk]

Dear Dirk,

In message <1227445293-23887-1-git-send-email-dirk.behme@googlemail.com> you wrote:
> Convert readl/writel to base + offset style. Replace hardcoded values with
> macros.

I think repeating the sequence "base + offset(foo)" again  and  again
is not exactly nice or easy to read.

> -	writel((a_add_high | a_add_low), SDRC_CS_CFG);
> +	writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));

I find this new code is even less readable.

At this point I was tempted to compain that you should not re-invent
the offsetof() macro when I realized that the actual definition of
OFFS is

	#define OFFS(x)    ((x) >> 2)

Argh... that's awfully error prone. So you rely  on  havnig  to  deal
with  32  bit  register  accesses  only,  without any way of compiler
supported type checking?

That's awful.

I am aware that many areas of ARM code use such a  horrible  program-
ming  syle  of  defining only register offsets instead of proper data
structures like for example PowerPC has been doing for ages.

Do you see a chance to convert the OMAP3 code to use data  structures
instead of offset definitions to describe the processor registers and
peripherals?

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: wd at denx.de
Nearly everyone is in favor of going  to  heaven  but  too  many  are
hoping  they'll  live  long  enough  to see an easing of the entrance
requirements. Never appeal to a man's "better nature." he  might  not
have one.

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Dirk Behme]

Dear Wolfgang,

Wolfgang Denk wrote:
> Dear Dirk,
> 
> In message <1227445293-23887-1-git-send-email-dirk.behme@googlemail.com> you wrote:
> 
>>Convert readl/writel to base + offset style. Replace hardcoded values with
>>macros.
> 
> 
> I think repeating the sequence "base + offset(foo)" again  and  again
> is not exactly nice or easy to read.

But it's what the patch does ;) Anyway, if I remember correctly this 
is why you asked me to re-send OMAP3 patch series to mailing list 
again once all the review comments are fixed and not directly git pull 
from OMAP3 branch. To get rid of all these "fix here, clean up there" 
comments.

>>-	writel((a_add_high | a_add_low), SDRC_CS_CFG);
>>+	writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));
> 
> 
> I find this new code is even less readable.
> 
> At this point I was tempted to compain that you should not re-invent
> the offsetof() macro when I realized that the actual definition of
> OFFS is
> 
> 	#define OFFS(x)    ((x) >> 2)
> 
> Argh... that's awfully error prone. So you rely  on  havnig  to  deal
> with  32  bit  register  accesses  only,  without any way of compiler
> supported type checking?
> 
> That's awful.

This style is what Jean-Christophe and Scott asked me to use [1]. I 
was asked to convert all OMAP3 read/write access to this style to get 
the patches accepted.

Disussing this at IRC, we agreed on

-- cut --
#define GPMC_BASE     0x6E000000

OFFS(x)  ((x) >> 2)

#define GPMC_ECC_CONFIG		0x1F4

static uint32_t *gpmc_base = (uint32_t *)GPMC_BASE;
...
writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG));
-- cut --

style for 32-bit accesses. OFFS is used to be able to use offset 
macros in assembly, too, and to be able to directly use offset 
addresses from TRM not needing them to divide by 4 "manually" (error 
prone).

[1] http://lists.denx.de/pipermail/u-boot/2008-October/042483.html

> I am aware that many areas of ARM code use such a  horrible  program-
> ming  syle  of  defining only register offsets instead of proper data
> structures like for example PowerPC has been doing for ages.
> 
> Do you see a chance to convert the OMAP3 code to use data  structures
> instead of offset definitions to describe the processor registers and
> peripherals?

Looking at the time I already spent into converting to the style used 
now, no, unfortunately, not. This was the last read/write converting 
patch, btw.

Best regards

Dirk

Btw.: Did you notice how e.g. Nomadik patch [2] does it?

-- cut --
#define NOMADIK_GPIO1_BASE	0x101E5000
...
writel(0xC37800F0, NOMADIK_GPIO1_BASE + 0x20);
-- cut --

Having this patch applied in this format I slightly wonder

a) why I have to convert all hardcoded values to macros

b) why I have to convert all writex/readx

Style used by Nomadik patch is the same OMAP3 initially used and that 
wasn't accepted...

[2] http://lists.denx.de/pipermail/u-boot/2008-November/043651.html

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Wolfgang Denk]

Dear Dirk,

In message <4929A58A.5060709@googlemail.com> you wrote:
> 
> > I think repeating the sequence "base + offset(foo)" again  and  again
> > is not exactly nice or easy to read.
...
> >>-	writel((a_add_high | a_add_low), SDRC_CS_CFG);
> >>+	writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));
...
> > Argh... that's awfully error prone. So you rely  on  havnig  to  deal
> > with  32  bit  register  accesses  only,  without any way of compiler
> > supported type checking?
...
> This style is what Jean-Christophe and Scott asked me to use [1]. I 
> was asked to convert all OMAP3 read/write access to this style to get 
> the patches accepted.

I don't see a request for this specific style in the message you
referenced.

Anyway - I put both Jean-Christophe and Scott on Cc:, and hereby ask
to discuss this again.

> Disussing this at IRC, we agreed on
> 
> -- cut --
> #define GPMC_BASE     0x6E000000
> 
> OFFS(x)  ((x) >> 2)
> 
> #define GPMC_ECC_CONFIG		0x1F4
> 
> static uint32_t *gpmc_base = (uint32_t *)GPMC_BASE;
> ...
> writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG));
> -- cut --

But this is ugly and error prone, and I really do not want to continue
adding such code to U-Boot.

> style for 32-bit accesses. OFFS is used to be able to use offset 
> macros in assembly, too, and to be able to directly use offset 
> addresses from TRM not needing them to divide by 4 "manually" (error 
> prone).

It may be true that the ARM  referecne  manuals  just  list  register
offsets - PowerPC manuals do the same. But this is in no way a reason
to access the registers through a "base address + offset" style thing
without type checking.

> [1] http://lists.denx.de/pipermail/u-boot/2008-October/042483.html


> > Do you see a chance to convert the OMAP3 code to use data  structures
> > instead of offset definitions to describe the processor registers and
> > peripherals?
> 
> Looking at the time I already spent into converting to the style used 
> now, no, unfortunately, not. This was the last read/write converting 
> patch, btw.

Well, if we do not clean this up now,  more  boards  will  get  added
which all use this style, and we will never be able to find resources
to clean it up.

Instead of declaring lists of offsets like this:

	#define GPMC_ECC_CONFIG         0x1F4
	#define GPMC_ECC_CONTROL        0x1F8
	#define GPMC_ECC_SIZE_CONFIG    0x1FC
	#define GPMC_ECC1_RESULT        0x200
	#define GPMC_ECC2_RESULT        0x204
	#define GPMC_ECC3_RESULT        0x208
	#define GPMC_ECC4_RESULT        0x20C
	#define GPMC_ECC5_RESULT        0x210
	#define GPMC_ECC6_RESULT        0x214
	#define GPMC_ECC7_RESULT        0x218
	#define GPMC_ECC8_RESULT        0x21C
	#define GPMC_ECC9_RESULT        0x220

you should really use structures like this:

	struct ecc_control {
		u32 ecc_config;
		u32 ecc_control;
		u32 ecc_size_config;
		u32 ecc1_result;
		u32 ecc2_result;
		u32 ecc3_result;
		u32 ecc4_result;
		u32 ecc5_result;
		u32 ecc6_result;
		u32 ecc7_result;
		u32 ecc8_result;
		u32 ecc9_result;
	};

so that instead of 

	writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG));

you can write

	writel(0x000, ptr->ecc_config);

or similar - and the compiler will actually perform type checking.

[Actually thius should be done for ALL such offset lists for all
systems that use such stuff, i. e. for  probably all ARM systems.]

I am aware that this is a major style change, but it really seems
necessary to me.


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: wd at denx.de
Only a fool fights in a burning house.
	-- Kank the Klingon, "Day of the Dove", stardate unknown

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Graeme Russ]

On Mon, Nov 24, 2008 at 6:47 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Dirk,
>

> you should really use structures like this:
>
>        struct ecc_control {
>                u32 ecc_config;
>                u32 ecc_control;
>                u32 ecc_size_config;
>                u32 ecc1_result;
>                u32 ecc2_result;
>                u32 ecc3_result;
>                u32 ecc4_result;
>                u32 ecc5_result;
>                u32 ecc6_result;
>                u32 ecc7_result;
>                u32 ecc8_result;
>                u32 ecc9_result;
>        };
>
> so that instead of
>
>        writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG));
>
> you can write
>
>        writel(0x000, ptr->ecc_config);
>
> or similar - and the compiler will actually perform type checking.

Wolfgang,

Do you mean to instance the ecc_control struct at a specific memory
location and use writel(0x000, &ptr->ecc_config);? If so, why not
simply use ptr->ecc_config = 0x0000?

Example:

The AMD sc520 has 4k of Memory Mapped Control Registers (MMCR) that range
in size from 8 to 32 bits located at a fixed memory address (0xfffef000)

I am thinking of replacing the read/write_mmcr_byte/word/long () functions
into a struct e.g.

struct sc520_mmcr {
	/* CPU */
	u16	revid;
	u8	cpuctl;
	u8	pad_0x003[0xd];

	/* SDRAM Controller */
	u8	drcctl;
	
	<etc>

} __attribute__((packed));

struct struct sc520_mmcr* mmcrptr = (volatile struct sc520_mmcr*)0xfffef000;

and replacing:
	write_mmcr_byte(SC520_DRCCTL, \
			(read_mmcr_byte(SC520_DRCCTL) & 0xcf) | (val<<4));

with:
	mmcrptr->drcctl = (mmcrptr->drcctl & 0xcf) | (val<<4);

Is this appropriate?

Regards,

Graeme

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Wolfgang Denk]

Dear Graeme,

in message <d66caabb0811231324q5bcf3076p698840e2f73c0564@mail.gmail.com> you wrote:
>
> Do you mean to instance the ecc_control struct at a specific memory
> location and use writel(0x000, &ptr->ecc_config);? If so, why not

Yes - this, and all other blocks of registers.

> simply use ptr->ecc_config = 0x0000?

Because in general it will not work. You may make  it  work  in  many
situations  by  making  sure  that  the  poointer  has the "volatile"
attribute, but depending on CPu  propoerties  and/or  compiler  opti-
mization  this  may  or may not be sufficient. Please see for example
Documentation/volatile-considered-harmful.txt  in  the  Linux  kernel
tree.

The only clean and portable and thus reliable way is to use the (CPU
specific) accessor macros or functions.

> and replacing:
> 	write_mmcr_byte(SC520_DRCCTL, \
> 			(read_mmcr_byte(SC520_DRCCTL) & 0xcf) | (val<<4));
> with:
> 	mmcrptr->drcctl = (mmcrptr->drcctl & 0xcf) | (val<<4);
> 
> Is this appropriate?

Probably not. Check what the Linux kenrel is doing (but then, x86 may
be a poor example; these processors are simply not advanced enough).

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: wd at denx.de
"Plan to throw one away. You will anyway."
                              - Fred Brooks, "The Mythical Man Month"

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Graeme Russ]

Wolfgang,

On Mon, Nov 24, 2008 at 10:04 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Graeme,
>
> in message <d66caabb0811231324q5bcf3076p698840e2f73c0564@mail.gmail.com> you wrote:
>>
>> Do you mean to instance the ecc_control struct at a specific memory
>> location and use writel(0x000, &ptr->ecc_config);? If so, why not
>
> Yes - this, and all other blocks of registers.
>
>> simply use ptr->ecc_config = 0x0000?
>
> mization  this  may  or may not be sufficient. Please see for example
> Documentation/volatile-considered-harmful.txt  in  the  Linux  kernel
> tree.

Have done

>
> The only clean and portable and thus reliable way is to use the (CPU
> specific) accessor macros or functions.

Will do :)

It seems to me that more and more of these 'style' issues are
cropping up. Should there be a document that covers them so we can
start to systematically improve the consistency of the code?

Thanks

Graeme

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Wolfgang Denk]

Dear Graeme,

In message <d66caabb0811231536k455da2e6wc0ce1a774d37e4@mail.gmail.com> you wrote:
> 
> > The only clean and portable and thus reliable way is to use the (CPU
> > specific) accessor macros or functions.
> 
> Will do :)

thanks.

> It seems to me that more and more of these 'style' issues are
> cropping up. Should there be a document that covers them so we can
> start to systematically improve the consistency of the code?

Well, there is: http://www.denx.de/wiki/U-Boot/CodingStyle and
Linux/Documentation/

The thing is, that only now people start to realize the importance of
some of these issues. The PowerPC tree may be well ahead compared  to
other  architectures,  because that's where both big fat 64bit server
machines on one side and low-end (50 MHz) embedded processors on  the
other  side  have  been  supported  for a long time. I think no other
architecture does that in a  similar  way.  [x86  may  be  different,
because it always is.]

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: wd at denx.de
We do not colonize. We conquer. We rule. There is no  other  way  for
us.
	-- Rojan, "By Any Other Name", stardate 4657.5

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Graeme Russ]

On Mon, Nov 24, 2008 at 10:47 AM, Wolfgang Denk <wd@denx.de> wrote:

> Linux/Documentation/

Hmmm, thats a big mouthful to chew for such a small bootloader

Maybe we could start to pick out particular specific pieces that
apply directly to the U-Boot source?

> architecture does that in a  similar  way.  [x86  may  be  different,
> because it always is.]

One could argue that x86 is always the same and everyone else is
(thankfully) different ;)

Cheers,


Graeme

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Dirk Behme]

Dear Jean-Christophe,

> In message <1227445293-23887-1-git-send-email-dirk.behme@googlemail.com> you wrote:
> 
>>Convert readl/writel to base + offset style. Replace hardcoded values with
>>macros.

It would be quite nice if you could apply this patch to 
u-boot-arm/omap3 branch independent of read/write discussion. As this 
is the last clean up patch, after applying this patch we would have 
the omap3 branch at least in a consistent style for the moment.

Many thanks

Dirk

[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Kim Phillips]

On Mon, 24 Nov 2008 10:53:46 +1100
"Graeme Russ" <graeme.russ@gmail.com> wrote:

> On Mon, Nov 24, 2008 at 10:47 AM, Wolfgang Denk <wd@denx.de> wrote:
> 
> > Linux/Documentation/
> 
> Hmmm, thats a big mouthful to chew for such a small bootloader

I believe he meant linux/Documentation/CodingStyle

Kim