[U-Boot] [PATCH] ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors
From: Prodyut Hazarika phazarika@amcc.com
Signed-off-by: Prodyut Hazarika phazarika@amcc.com Acked-by: Feng Kan fkan@amcc.com --- cpu/ppc4xx/44x_spd_ddr2.c | 27 ++++++++++++++++--- cpu/ppc4xx/cpu_init.c | 16 +++++++++++- include/asm-ppc/ppc4xx-sdram.h | 48 +++++++++++++++++++++++----------- include/ppc440.h | 47 ---------------------------------- include/ppc4xx.h | 55 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 125 insertions(+), 68 deletions(-)
diff --git a/cpu/ppc4xx/44x_spd_ddr2.c b/cpu/ppc4xx/44x_spd_ddr2.c index 1c36324..0431754 100644 --- a/cpu/ppc4xx/44x_spd_ddr2.c +++ b/cpu/ppc4xx/44x_spd_ddr2.c @@ -2172,6 +2172,11 @@ static void program_memory_queue(unsigned long *dimm_populated, unsigned long i; unsigned long bank_0_populated = 0; phys_size_t total_size = 0; +#if defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \ + defined(CONFIG_460EX) || defined(CONFIG_460GT) || \ + defined(CONFIG_460SX) + unsigned long val; +#endif
/*------------------------------------------------------------------ * Reset the rank_base_address. @@ -2249,17 +2254,31 @@ static void program_memory_queue(unsigned long *dimm_populated, } }
-#if defined(CONFIG_460EX) || defined(CONFIG_460GT) +#if defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \ + defined(CONFIG_460EX) || defined(CONFIG_460GT) || \ + defined(CONFIG_460SX) + /* - * Enable high bandwidth access on 460EX/GT. - * This should/could probably be done on other - * PPC's too, like 440SPe. + * Enable high bandwidth access * This is currently not used, but with this setup * it is possible to use it later on in e.g. the Linux * EMAC driver for performance gain. */ mtdcr(SDRAM_PLBADDULL, 0x00000000); /* MQ0_BAUL */ mtdcr(SDRAM_PLBADDUHB, 0x00000008); /* MQ0_BAUH */ + + /* + * Set optimal value for Memory Queue HB/LL Configuration registers + */ + + val = (mfdcr(SDRAM_CONF1HB) | SDRAM_CONF1HB_AAFR | SDRAM_CONF1HB_RPEN | SDRAM_CONF1HB_RFTE); + mtdcr(SDRAM_CONF1HB, val); + + val = (mfdcr(SDRAM_CONF1LL) | SDRAM_CONF1LL_AAFR | SDRAM_CONF1LL_RPEN | SDRAM_CONF1LL_RFTE); + mtdcr(SDRAM_CONF1LL, val); + + val = (mfdcr(SDRAM_CONFPATHB) | SDRAM_CONFPATHB_TPEN); + mtdcr(SDRAM_CONFPATHB, val); #endif }
diff --git a/cpu/ppc4xx/cpu_init.c b/cpu/ppc4xx/cpu_init.c index e2d0402..c7c429e 100644 --- a/cpu/ppc4xx/cpu_init.c +++ b/cpu/ppc4xx/cpu_init.c @@ -138,7 +138,9 @@ void reconfigure_pll(u32 new_cpu_freq) void cpu_init_f (void) { -#if defined(CONFIG_WATCHDOG) || defined(CONFIG_440GX) || defined(CONFIG_460EX) +#if defined(CONFIG_WATCHDOG) || defined(CONFIG_440GX) || defined(CONFIG_460EX) || \ + defined(CONFIG_440SP) || defined(CONFIG_440SPE) || defined(CONFIG_405EX) || \ + defined(CONFIG_460GT) || defined(CONFIG_460SX) u32 val; #endif
@@ -301,6 +303,18 @@ cpu_init_f (void) val |= 0x400; mtsdr(SDR0_USB2HOST_CFG, val); #endif /* CONFIG_460EX */ + +#if defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \ + defined(CONFIG_460EX) || defined(CONFIG_460GT) || \ + defined(CONFIG_405EX) || defined(CONFIG_460SX) + /* + * Set PLB4 arbiter (Segment 0 and 1) to 4 deep pipeline read + */ + val = (mfdcr(plb0_acr) & ~plb0_acr_rdp_mask) | plb0_acr_rdp_4deep; + mtdcr(plb0_acr, val); + val = (mfdcr(plb1_acr) & ~plb1_acr_rdp_mask) | plb1_acr_rdp_4deep; + mtdcr(plb1_acr, val); +#endif /* CONFIG_440SP/SPE || CONFIG_460EX/GT || CONFIG_405EX */ }
/* diff --git a/include/asm-ppc/ppc4xx-sdram.h b/include/asm-ppc/ppc4xx-sdram.h index df787b3..d2e3b42 100644 --- a/include/asm-ppc/ppc4xx-sdram.h +++ b/include/asm-ppc/ppc4xx-sdram.h @@ -259,23 +259,39 @@ /* * Memory queue defines */ -#define SDRAMQ_DCR_BASE 0x040 - -#define SDRAM_R0BAS (SDRAMQ_DCR_BASE+0x0) /* rank 0 base address & size */ -#define SDRAM_R1BAS (SDRAMQ_DCR_BASE+0x1) /* rank 1 base address & size */ -#define SDRAM_R2BAS (SDRAMQ_DCR_BASE+0x2) /* rank 2 base address & size */ -#define SDRAM_R3BAS (SDRAMQ_DCR_BASE+0x3) /* rank 3 base address & size */ -#define SDRAM_CONF1HB (SDRAMQ_DCR_BASE+0x5) /* configuration 1 HB */ -#define SDRAM_ERRSTATHB (SDRAMQ_DCR_BASE+0x7) /* error status HB */ -#define SDRAM_ERRADDUHB (SDRAMQ_DCR_BASE+0x8) /* error address upper 32 HB */ -#define SDRAM_ERRADDLHB (SDRAMQ_DCR_BASE+0x9) /* error address lower 32 HB */ -#define SDRAM_PLBADDULL (SDRAMQ_DCR_BASE+0xA) /* PLB base address upper 32 LL */ -#define SDRAM_CONF1LL (SDRAMQ_DCR_BASE+0xB) /* configuration 1 LL */ -#define SDRAM_ERRSTATLL (SDRAMQ_DCR_BASE+0xC) /* error status LL */ -#define SDRAM_ERRADDULL (SDRAMQ_DCR_BASE+0xD) /* error address upper 32 LL */ -#define SDRAM_ERRADDLLL (SDRAMQ_DCR_BASE+0xE) /* error address lower 32 LL */ -#define SDRAM_CONFPATHB (SDRAMQ_DCR_BASE+0xF) /* configuration between paths */ -#define SDRAM_PLBADDUHB (SDRAMQ_DCR_BASE+0x10) /* PLB base address upper 32 LL */ +#define SDRAMQ_DCR_BASE 0x040 + +#define SDRAM_R0BAS (SDRAMQ_DCR_BASE+0x0) /* rank 0 base address & size */ +#define SDRAM_R1BAS (SDRAMQ_DCR_BASE+0x1) /* rank 1 base address & size */ +#define SDRAM_R2BAS (SDRAMQ_DCR_BASE+0x2) /* rank 2 base address & size */ +#define SDRAM_R3BAS (SDRAMQ_DCR_BASE+0x3) /* rank 3 base address & size */ +#define SDRAM_CONF1HB (SDRAMQ_DCR_BASE+0x5) /* configuration 1 HB */ +#define SDRAM_CONF1HB_AAFR 0x80000000 /* Address Ack on First Request - Bit 0 */ +#define SDRAM_CONF1HB_PRPD 0x00080000 /* PLB Read pipeline Disable - Bit 12 */ +#define SDRAM_CONF1HB_PWPD 0x00040000 /* PLB Write pipeline Disable - Bit 13 */ +#define SDRAM_CONF1HB_PRW 0x00020000 /* PLB Read Wait - Bit 14 */ +#define SDRAM_CONF1HB_RPEN 0x00000800 /* Read Passing Enable - Bit 20 */ +#define SDRAM_CONF1HB_RFTE 0x00000400 /* Read Flow Through Enable - Bit 21 */ + +#define SDRAM_ERRSTATHB (SDRAMQ_DCR_BASE+0x7) /* error status HB */ +#define SDRAM_ERRADDUHB (SDRAMQ_DCR_BASE+0x8) /* error address upper 32 HB */ +#define SDRAM_ERRADDLHB (SDRAMQ_DCR_BASE+0x9) /* error address lower 32 HB */ +#define SDRAM_PLBADDULL (SDRAMQ_DCR_BASE+0xA) /* PLB base address upper 32 LL */ +#define SDRAM_CONF1LL (SDRAMQ_DCR_BASE+0xB) /* configuration 1 LL */ +#define SDRAM_CONF1LL_AAFR 0x80000000 /* Address Ack on First Request - Bit 0 */ +#define SDRAM_CONF1LL_PRPD 0x00080000 /* PLB Read pipeline Disable - Bit 12 */ +#define SDRAM_CONF1LL_PWPD 0x00040000 /* PLB Write pipeline Disable - Bit 13 */ +#define SDRAM_CONF1LL_PRW 0x00020000 /* PLB Read Wait - Bit 14 */ +#define SDRAM_CONF1LL_RPEN 0x00000800 /* Read Passing Enable - Bit 20 */ +#define SDRAM_CONF1LL_RFTE 0x00000400 /* Read Flow Through Enable - Bit 21 */ + +#define SDRAM_ERRSTATLL (SDRAMQ_DCR_BASE+0xC) /* error status LL */ +#define SDRAM_ERRADDULL (SDRAMQ_DCR_BASE+0xD) /* error address upper 32 LL */ +#define SDRAM_ERRADDLLL (SDRAMQ_DCR_BASE+0xE) /* error address lower 32 LL */ +#define SDRAM_CONFPATHB (SDRAMQ_DCR_BASE+0xF) /* configuration between paths */ +#define SDRAM_CONFPATHB_TPEN 0x08000000 /* Transaction Passing Enable - Bit 4 */ + +#define SDRAM_PLBADDUHB (SDRAMQ_DCR_BASE+0x10) /* PLB base address upper 32 LL */
#if !defined(CONFIG_405EX) /* diff --git a/include/ppc440.h b/include/ppc440.h index 92db15f..3584fd2 100644 --- a/include/ppc440.h +++ b/include/ppc440.h @@ -341,53 +341,6 @@
#define PLB4_ACR_WRP (0x80000000 >> 7)
-/* Nebula PLB4 Arbiter - PowerPC440EP */ -#define PLB_ARBITER_BASE 0x80 - -#define plb0_revid (PLB_ARBITER_BASE+ 0x00) -#define plb0_acr (PLB_ARBITER_BASE+ 0x01) -#define plb0_acr_ppm_mask 0xF0000000 -#define plb0_acr_ppm_fixed 0x00000000 -#define plb0_acr_ppm_fair 0xD0000000 -#define plb0_acr_hbu_mask 0x08000000 -#define plb0_acr_hbu_disabled 0x00000000 -#define plb0_acr_hbu_enabled 0x08000000 -#define plb0_acr_rdp_mask 0x06000000 -#define plb0_acr_rdp_disabled 0x00000000 -#define plb0_acr_rdp_2deep 0x02000000 -#define plb0_acr_rdp_3deep 0x04000000 -#define plb0_acr_rdp_4deep 0x06000000 -#define plb0_acr_wrp_mask 0x01000000 -#define plb0_acr_wrp_disabled 0x00000000 -#define plb0_acr_wrp_2deep 0x01000000 - -#define plb0_besrl (PLB_ARBITER_BASE+ 0x02) -#define plb0_besrh (PLB_ARBITER_BASE+ 0x03) -#define plb0_bearl (PLB_ARBITER_BASE+ 0x04) -#define plb0_bearh (PLB_ARBITER_BASE+ 0x05) -#define plb0_ccr (PLB_ARBITER_BASE+ 0x08) - -#define plb1_acr (PLB_ARBITER_BASE+ 0x09) -#define plb1_acr_ppm_mask 0xF0000000 -#define plb1_acr_ppm_fixed 0x00000000 -#define plb1_acr_ppm_fair 0xD0000000 -#define plb1_acr_hbu_mask 0x08000000 -#define plb1_acr_hbu_disabled 0x00000000 -#define plb1_acr_hbu_enabled 0x08000000 -#define plb1_acr_rdp_mask 0x06000000 -#define plb1_acr_rdp_disabled 0x00000000 -#define plb1_acr_rdp_2deep 0x02000000 -#define plb1_acr_rdp_3deep 0x04000000 -#define plb1_acr_rdp_4deep 0x06000000 -#define plb1_acr_wrp_mask 0x01000000 -#define plb1_acr_wrp_disabled 0x00000000 -#define plb1_acr_wrp_2deep 0x01000000 - -#define plb1_besrl (PLB_ARBITER_BASE+ 0x0A) -#define plb1_besrh (PLB_ARBITER_BASE+ 0x0B) -#define plb1_bearl (PLB_ARBITER_BASE+ 0x0C) -#define plb1_bearh (PLB_ARBITER_BASE+ 0x0D) - /* Pin Function Control Register 1 */ #define SDR0_PFC1 0x4101 #define SDR0_PFC1_U1ME_MASK 0x02000000 /* UART1 Mode Enable */ diff --git a/include/ppc4xx.h b/include/ppc4xx.h index c71da60..154956e 100644 --- a/include/ppc4xx.h +++ b/include/ppc4xx.h @@ -46,6 +46,61 @@ #define CONFIG_SDRAM_PPC4xx_IBM_DDR2 /* IBM DDR(2) controller */ #endif
+/* PLB4 CrossBar Arbiter Core supported across PPC4xx families */ +#if defined(CONFIG_440EP) || defined(CONFIG_440EPX) || \ + defined(CONFIG_440GR) || defined(CONFIG_440GRX) || \ + defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \ + defined(CONFIG_460EX) || defined(CONFIG_460GT) || \ + defined(CONFIG_460SX) || defined(CONFIG_405EX) + +#define PLB_ARBITER_BASE 0x80 + +#define plb0_revid (PLB_ARBITER_BASE+ 0x00) +#define plb0_acr (PLB_ARBITER_BASE+ 0x01) +#define plb0_acr_ppm_mask 0xF0000000 +#define plb0_acr_ppm_fixed 0x00000000 +#define plb0_acr_ppm_fair 0xD0000000 +#define plb0_acr_hbu_mask 0x08000000 +#define plb0_acr_hbu_disabled 0x00000000 +#define plb0_acr_hbu_enabled 0x08000000 +#define plb0_acr_rdp_mask 0x06000000 +#define plb0_acr_rdp_disabled 0x00000000 +#define plb0_acr_rdp_2deep 0x02000000 +#define plb0_acr_rdp_3deep 0x04000000 +#define plb0_acr_rdp_4deep 0x06000000 +#define plb0_acr_wrp_mask 0x01000000 +#define plb0_acr_wrp_disabled 0x00000000 +#define plb0_acr_wrp_2deep 0x01000000 + +#define plb0_besrl (PLB_ARBITER_BASE+ 0x02) +#define plb0_besrh (PLB_ARBITER_BASE+ 0x03) +#define plb0_bearl (PLB_ARBITER_BASE+ 0x04) +#define plb0_bearh (PLB_ARBITER_BASE+ 0x05) +#define plb0_ccr (PLB_ARBITER_BASE+ 0x08) + +#define plb1_acr (PLB_ARBITER_BASE+ 0x09) +#define plb1_acr_ppm_mask 0xF0000000 +#define plb1_acr_ppm_fixed 0x00000000 +#define plb1_acr_ppm_fair 0xD0000000 +#define plb1_acr_hbu_mask 0x08000000 +#define plb1_acr_hbu_disabled 0x00000000 +#define plb1_acr_hbu_enabled 0x08000000 +#define plb1_acr_rdp_mask 0x06000000 +#define plb1_acr_rdp_disabled 0x00000000 +#define plb1_acr_rdp_2deep 0x02000000 +#define plb1_acr_rdp_3deep 0x04000000 +#define plb1_acr_rdp_4deep 0x06000000 +#define plb1_acr_wrp_mask 0x01000000 +#define plb1_acr_wrp_disabled 0x00000000 +#define plb1_acr_wrp_2deep 0x01000000 + +#define plb1_besrl (PLB_ARBITER_BASE+ 0x0A) +#define plb1_besrh (PLB_ARBITER_BASE+ 0x0B) +#define plb1_bearl (PLB_ARBITER_BASE+ 0x0C) +#define plb1_bearh (PLB_ARBITER_BASE+ 0x0D) + +#endif /* 440EP/EPX 440GR/GRX 440SP/SPE 460EX/GT/SX 405EX*/ + #if defined(CONFIG_440) /* * Enable long long (%ll ...) printf format on 440 PPC's since most of
Dear Feng Kan,
in message 1218602619-19293-1-git-send-email-fkan@amcc.com you wrote:
From: Prodyut Hazarika phazarika@amcc.com
Signed-off-by: Prodyut Hazarika phazarika@amcc.com Acked-by: Feng Kan fkan@amcc.com
It would be nice if the commit messages contained at least a minimal explanation of the reasons for the changes.
diff --git a/include/asm-ppc/ppc4xx-sdram.h b/include/asm-ppc/ppc4xx-sdram.h index df787b3..d2e3b42 100644 --- a/include/asm-ppc/ppc4xx-sdram.h +++ b/include/asm-ppc/ppc4xx-sdram.h @@ -259,23 +259,39 @@ /*
- Memory queue defines
*/ -#define SDRAMQ_DCR_BASE 0x040
-#define SDRAM_R0BAS (SDRAMQ_DCR_BASE+0x0) /* rank 0 base address & size */ -#define SDRAM_R1BAS (SDRAMQ_DCR_BASE+0x1) /* rank 1 base address & size */ -#define SDRAM_R2BAS (SDRAMQ_DCR_BASE+0x2) /* rank 2 base address & size */ -#define SDRAM_R3BAS (SDRAMQ_DCR_BASE+0x3) /* rank 3 base address & size */ -#define SDRAM_CONF1HB (SDRAMQ_DCR_BASE+0x5) /* configuration 1 HB */ -#define SDRAM_ERRSTATHB (SDRAMQ_DCR_BASE+0x7) /* error status HB */ -#define SDRAM_ERRADDUHB (SDRAMQ_DCR_BASE+0x8) /* error address upper 32 HB */ -#define SDRAM_ERRADDLHB (SDRAMQ_DCR_BASE+0x9) /* error address lower 32 HB */ -#define SDRAM_PLBADDULL (SDRAMQ_DCR_BASE+0xA) /* PLB base address upper 32 LL */ -#define SDRAM_CONF1LL (SDRAMQ_DCR_BASE+0xB) /* configuration 1 LL */ -#define SDRAM_ERRSTATLL (SDRAMQ_DCR_BASE+0xC) /* error status LL */ -#define SDRAM_ERRADDULL (SDRAMQ_DCR_BASE+0xD) /* error address upper 32 LL */ -#define SDRAM_ERRADDLLL (SDRAMQ_DCR_BASE+0xE) /* error address lower 32 LL */ -#define SDRAM_CONFPATHB (SDRAMQ_DCR_BASE+0xF) /* configuration between paths */ -#define SDRAM_PLBADDUHB (SDRAMQ_DCR_BASE+0x10) /* PLB base address upper 32 LL */ +#define SDRAMQ_DCR_BASE 0x040
+#define SDRAM_R0BAS (SDRAMQ_DCR_BASE+0x0) /* rank 0 base address & size */ +#define SDRAM_R1BAS (SDRAMQ_DCR_BASE+0x1) /* rank 1 base address & size */ +#define SDRAM_R2BAS (SDRAMQ_DCR_BASE+0x2) /* rank 2 base address & size */ +#define SDRAM_R3BAS (SDRAMQ_DCR_BASE+0x3) /* rank 3 base address & size */ +#define SDRAM_CONF1HB (SDRAMQ_DCR_BASE+0x5) /* configuration 1 HB */ +#define SDRAM_CONF1HB_AAFR 0x80000000 /* Address Ack on First Request - Bit 0 */ +#define SDRAM_CONF1HB_PRPD 0x00080000 /* PLB Read pipeline Disable - Bit 12 */ +#define SDRAM_CONF1HB_PWPD 0x00040000 /* PLB Write pipeline Disable - Bit 13 */ +#define SDRAM_CONF1HB_PRW 0x00020000 /* PLB Read Wait - Bit 14 */ +#define SDRAM_CONF1HB_RPEN 0x00000800 /* Read Passing Enable - Bit 20 */ +#define SDRAM_CONF1HB_RFTE 0x00000400 /* Read Flow Through Enable - Bit 21 */
+#define SDRAM_ERRSTATHB (SDRAMQ_DCR_BASE+0x7) /* error status HB */ +#define SDRAM_ERRADDUHB (SDRAMQ_DCR_BASE+0x8) /* error address upper 32 HB */ +#define SDRAM_ERRADDLHB (SDRAMQ_DCR_BASE+0x9) /* error address lower 32 HB */ +#define SDRAM_PLBADDULL (SDRAMQ_DCR_BASE+0xA) /* PLB base address upper 32 LL */ +#define SDRAM_CONF1LL (SDRAMQ_DCR_BASE+0xB) /* configuration 1 LL */ +#define SDRAM_CONF1LL_AAFR 0x80000000 /* Address Ack on First Request - Bit 0 */ +#define SDRAM_CONF1LL_PRPD 0x00080000 /* PLB Read pipeline Disable - Bit 12 */ +#define SDRAM_CONF1LL_PWPD 0x00040000 /* PLB Write pipeline Disable - Bit 13 */ +#define SDRAM_CONF1LL_PRW 0x00020000 /* PLB Read Wait - Bit 14 */ +#define SDRAM_CONF1LL_RPEN 0x00000800 /* Read Passing Enable - Bit 20 */ +#define SDRAM_CONF1LL_RFTE 0x00000400 /* Read Flow Through Enable - Bit 21 */
+#define SDRAM_ERRSTATLL (SDRAMQ_DCR_BASE+0xC) /* error status LL */ +#define SDRAM_ERRADDULL (SDRAMQ_DCR_BASE+0xD) /* error address upper 32 LL */ +#define SDRAM_ERRADDLLL (SDRAMQ_DCR_BASE+0xE) /* error address lower 32 LL */ +#define SDRAM_CONFPATHB (SDRAMQ_DCR_BASE+0xF) /* configuration between paths */ +#define SDRAM_CONFPATHB_TPEN 0x08000000 /* Transaction Passing Enable - Bit 4 */
+#define SDRAM_PLBADDUHB (SDRAMQ_DCR_BASE+0x10) /* PLB base address upper 32 LL */
This change seems to be completely unrelated to above described changes, so if it was a valid modification, it would have to be split off into a separate commit.
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Please do not do this.
NAK.
#if !defined(CONFIG_405EX) /* diff --git a/include/ppc440.h b/include/ppc440.h index 92db15f..3584fd2 100644 --- a/include/ppc440.h +++ b/include/ppc440.h @@ -341,53 +341,6 @@
#define PLB4_ACR_WRP (0x80000000 >> 7)
-/* Nebula PLB4 Arbiter - PowerPC440EP */ -#define PLB_ARBITER_BASE 0x80
-#define plb0_revid (PLB_ARBITER_BASE+ 0x00) -#define plb0_acr (PLB_ARBITER_BASE+ 0x01) -#define plb0_acr_ppm_mask 0xF0000000 -#define plb0_acr_ppm_fixed 0x00000000 -#define plb0_acr_ppm_fair 0xD0000000 -#define plb0_acr_hbu_mask 0x08000000 -#define plb0_acr_hbu_disabled 0x00000000 -#define plb0_acr_hbu_enabled 0x08000000 -#define plb0_acr_rdp_mask 0x06000000 -#define plb0_acr_rdp_disabled 0x00000000 -#define plb0_acr_rdp_2deep 0x02000000 -#define plb0_acr_rdp_3deep 0x04000000 -#define plb0_acr_rdp_4deep 0x06000000 -#define plb0_acr_wrp_mask 0x01000000 -#define plb0_acr_wrp_disabled 0x00000000 -#define plb0_acr_wrp_2deep 0x01000000
-#define plb0_besrl (PLB_ARBITER_BASE+ 0x02) -#define plb0_besrh (PLB_ARBITER_BASE+ 0x03) -#define plb0_bearl (PLB_ARBITER_BASE+ 0x04) -#define plb0_bearh (PLB_ARBITER_BASE+ 0x05) -#define plb0_ccr (PLB_ARBITER_BASE+ 0x08)
-#define plb1_acr (PLB_ARBITER_BASE+ 0x09) -#define plb1_acr_ppm_mask 0xF0000000 -#define plb1_acr_ppm_fixed 0x00000000 -#define plb1_acr_ppm_fair 0xD0000000 -#define plb1_acr_hbu_mask 0x08000000 -#define plb1_acr_hbu_disabled 0x00000000 -#define plb1_acr_hbu_enabled 0x08000000 -#define plb1_acr_rdp_mask 0x06000000 -#define plb1_acr_rdp_disabled 0x00000000 -#define plb1_acr_rdp_2deep 0x02000000 -#define plb1_acr_rdp_3deep 0x04000000 -#define plb1_acr_rdp_4deep 0x06000000 -#define plb1_acr_wrp_mask 0x01000000 -#define plb1_acr_wrp_disabled 0x00000000 -#define plb1_acr_wrp_2deep 0x01000000
-#define plb1_besrl (PLB_ARBITER_BASE+ 0x0A) -#define plb1_besrh (PLB_ARBITER_BASE+ 0x0B) -#define plb1_bearl (PLB_ARBITER_BASE+ 0x0C) -#define plb1_bearh (PLB_ARBITER_BASE+ 0x0D)
/* Pin Function Control Register 1 */ #define SDR0_PFC1 0x4101 #define SDR0_PFC1_U1ME_MASK 0x02000000 /* UART1 Mode Enable */ diff --git a/include/ppc4xx.h b/include/ppc4xx.h index c71da60..154956e 100644 --- a/include/ppc4xx.h +++ b/include/ppc4xx.h @@ -46,6 +46,61 @@ #define CONFIG_SDRAM_PPC4xx_IBM_DDR2 /* IBM DDR(2) controller */ #endif
+/* PLB4 CrossBar Arbiter Core supported across PPC4xx families */ +#if defined(CONFIG_440EP) || defined(CONFIG_440EPX) || \
- defined(CONFIG_440GR) || defined(CONFIG_440GRX) || \
- defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \
- defined(CONFIG_460EX) || defined(CONFIG_460GT) || \
- defined(CONFIG_460SX) || defined(CONFIG_405EX)
+#define PLB_ARBITER_BASE 0x80
+#define plb0_revid (PLB_ARBITER_BASE+ 0x00) +#define plb0_acr (PLB_ARBITER_BASE+ 0x01) +#define plb0_acr_ppm_mask 0xF0000000 +#define plb0_acr_ppm_fixed 0x00000000 +#define plb0_acr_ppm_fair 0xD0000000 +#define plb0_acr_hbu_mask 0x08000000 +#define plb0_acr_hbu_disabled 0x00000000 +#define plb0_acr_hbu_enabled 0x08000000 +#define plb0_acr_rdp_mask 0x06000000 +#define plb0_acr_rdp_disabled 0x00000000 +#define plb0_acr_rdp_2deep 0x02000000 +#define plb0_acr_rdp_3deep 0x04000000 +#define plb0_acr_rdp_4deep 0x06000000 +#define plb0_acr_wrp_mask 0x01000000 +#define plb0_acr_wrp_disabled 0x00000000 +#define plb0_acr_wrp_2deep 0x01000000
+#define plb0_besrl (PLB_ARBITER_BASE+ 0x02) +#define plb0_besrh (PLB_ARBITER_BASE+ 0x03) +#define plb0_bearl (PLB_ARBITER_BASE+ 0x04) +#define plb0_bearh (PLB_ARBITER_BASE+ 0x05) +#define plb0_ccr (PLB_ARBITER_BASE+ 0x08)
+#define plb1_acr (PLB_ARBITER_BASE+ 0x09) +#define plb1_acr_ppm_mask 0xF0000000 +#define plb1_acr_ppm_fixed 0x00000000 +#define plb1_acr_ppm_fair 0xD0000000 +#define plb1_acr_hbu_mask 0x08000000 +#define plb1_acr_hbu_disabled 0x00000000 +#define plb1_acr_hbu_enabled 0x08000000 +#define plb1_acr_rdp_mask 0x06000000 +#define plb1_acr_rdp_disabled 0x00000000 +#define plb1_acr_rdp_2deep 0x02000000 +#define plb1_acr_rdp_3deep 0x04000000 +#define plb1_acr_rdp_4deep 0x06000000 +#define plb1_acr_wrp_mask 0x01000000 +#define plb1_acr_wrp_disabled 0x00000000 +#define plb1_acr_wrp_2deep 0x01000000
+#define plb1_besrl (PLB_ARBITER_BASE+ 0x0A) +#define plb1_besrh (PLB_ARBITER_BASE+ 0x0B) +#define plb1_bearl (PLB_ARBITER_BASE+ 0x0C) +#define plb1_bearh (PLB_ARBITER_BASE+ 0x0D)
+#endif /* 440EP/EPX 440GR/GRX 440SP/SPE 460EX/GT/SX 405EX*/
Here you move 44x specific code from a 44x specific header file into a 4xx generic header file which requires you to add a 44x specific #ifdef's.
That's a change to the worse. Please don't do that.
Best regards,
Wolfgang Denk
On Wednesday 13 August 2008, Wolfgang Denk wrote:
From: Prodyut Hazarika phazarika@amcc.com
Signed-off-by: Prodyut Hazarika phazarika@amcc.com Acked-by: Feng Kan fkan@amcc.com
It would be nice if the commit messages contained at least a minimal explanation of the reasons for the changes.
Yes, the original comment from Prodyut is gone. Please add it again.
<snip>
*/ diff --git a/include/ppc4xx.h b/include/ppc4xx.h index c71da60..154956e 100644 --- a/include/ppc4xx.h +++ b/include/ppc4xx.h @@ -46,6 +46,61 @@ #define CONFIG_SDRAM_PPC4xx_IBM_DDR2 /* IBM DDR(2) controller */ #endif
+/* PLB4 CrossBar Arbiter Core supported across PPC4xx families */ +#if defined(CONFIG_440EP) || defined(CONFIG_440EPX) || \
- defined(CONFIG_440GR) || defined(CONFIG_440GRX) || \
- defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \
- defined(CONFIG_460EX) || defined(CONFIG_460GT) || \
- defined(CONFIG_460SX) || defined(CONFIG_405EX)
+#define PLB_ARBITER_BASE 0x80
+#define plb0_revid (PLB_ARBITER_BASE+ 0x00) +#define plb0_acr (PLB_ARBITER_BASE+ 0x01) +#define plb0_acr_ppm_mask 0xF0000000 +#define plb0_acr_ppm_fixed 0x00000000 +#define plb0_acr_ppm_fair 0xD0000000 +#define plb0_acr_hbu_mask 0x08000000 +#define plb0_acr_hbu_disabled 0x00000000 +#define plb0_acr_hbu_enabled 0x08000000 +#define plb0_acr_rdp_mask 0x06000000 +#define plb0_acr_rdp_disabled 0x00000000 +#define plb0_acr_rdp_2deep 0x02000000 +#define plb0_acr_rdp_3deep 0x04000000 +#define plb0_acr_rdp_4deep 0x06000000 +#define plb0_acr_wrp_mask 0x01000000 +#define plb0_acr_wrp_disabled 0x00000000 +#define plb0_acr_wrp_2deep 0x01000000
+#define plb0_besrl (PLB_ARBITER_BASE+ 0x02) +#define plb0_besrh (PLB_ARBITER_BASE+ 0x03) +#define plb0_bearl (PLB_ARBITER_BASE+ 0x04) +#define plb0_bearh (PLB_ARBITER_BASE+ 0x05) +#define plb0_ccr (PLB_ARBITER_BASE+ 0x08)
+#define plb1_acr (PLB_ARBITER_BASE+ 0x09) +#define plb1_acr_ppm_mask 0xF0000000 +#define plb1_acr_ppm_fixed 0x00000000 +#define plb1_acr_ppm_fair 0xD0000000 +#define plb1_acr_hbu_mask 0x08000000 +#define plb1_acr_hbu_disabled 0x00000000 +#define plb1_acr_hbu_enabled 0x08000000 +#define plb1_acr_rdp_mask 0x06000000 +#define plb1_acr_rdp_disabled 0x00000000 +#define plb1_acr_rdp_2deep 0x02000000 +#define plb1_acr_rdp_3deep 0x04000000 +#define plb1_acr_rdp_4deep 0x06000000 +#define plb1_acr_wrp_mask 0x01000000 +#define plb1_acr_wrp_disabled 0x00000000 +#define plb1_acr_wrp_2deep 0x01000000
+#define plb1_besrl (PLB_ARBITER_BASE+ 0x0A) +#define plb1_besrh (PLB_ARBITER_BASE+ 0x0B) +#define plb1_bearl (PLB_ARBITER_BASE+ 0x0C) +#define plb1_bearh (PLB_ARBITER_BASE+ 0x0D)
+#endif /* 440EP/EPX 440GR/GRX 440SP/SPE 460EX/GT/SX 405EX*/
Here you move 44x specific code from a 44x specific header file into a 4xx generic header file which requires you to add a 44x specific #ifdef's.
These defines are also used on the 405EX (and possibly future 405 variants as well). It makes sense to move them to ppc4xx.h so we don't have to duplicate those defines in 2 headers. This has been a big problem in the past with the ppc405.h and ppc440.h headers.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Dear Stefan Roese,
In message 200808130826.57511.sr@denx.de you wrote:
+/* PLB4 CrossBar Arbiter Core supported across PPC4xx families */ +#if defined(CONFIG_440EP) || defined(CONFIG_440EPX) || \
- defined(CONFIG_440GR) || defined(CONFIG_440GRX) || \
- defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \
- defined(CONFIG_460EX) || defined(CONFIG_460GT) || \
- defined(CONFIG_460SX) || defined(CONFIG_405EX)
--------------------------------------------^^^^^^^^^^^^^ ...
Here you move 44x specific code from a 44x specific header file into a 4xx generic header file which requires you to add a 44x specific #ifdef's.
These defines are also used on the 405EX (and possibly future 405 variants as well). It makes sense to move them to ppc4xx.h so we don't have to duplicate those defines in 2 headers. This has been a big problem in the past with the ppc405.h and ppc440.h headers.
You are right, I missed that single 405 there.
May I please ask to sort the list, then?
Best regards,
Wolfgang Denk
It would be nice if the commit messages contained at least a minimal explanation of the reasons for the changes.
Yes, the original comment from Prodyut is gone. Please add it again.
The original comments got omitted by mistake. I will resubmit it again.
+#define plb1_bearl (PLB_ARBITER_BASE+ 0x0C) +#define plb1_bearh (PLB_ARBITER_BASE+ 0x0D)
+#endif /* 440EP/EPX 440GR/GRX 440SP/SPE 460EX/GT/SX 405EX*/
Here you move 44x specific code from a 44x specific header file into a 4xx generic header file which requires you to add a 44x specific #ifdef's.
These defines are also used on the 405EX (and possibly future 405 variants as well). It makes sense to move them to ppc4xx.h so we don't have to duplicate those defines in 2 headers. This has been a big problem in the past with the ppc405.h and ppc440.h headers.
I will move the PPC405EX to the beginning of the list and resubmit.
Best regards, Prodyut Hazarika
========================= Staff Software Engineer AMCC ========================= --------------------------------------------------------
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
On Wednesday 13 August 2008, Prodyut Hazarika wrote:
Here you move 44x specific code from a 44x specific header file into a 4xx generic header file which requires you to add a 44x specific #ifdef's.
These defines are also used on the 405EX (and possibly future 405 variants as well). It makes sense to move them to ppc4xx.h so we don't have to duplicate those defines in 2 headers. This has been a big problem in the past with the ppc405.h and ppc440.h headers.
I will move the PPC405EX to the beginning of the list and resubmit.
Thanks. Please rebase your patch now against the master branch of my u-boot-ppc4xx repository. I've merged the next branch in the master branch already.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
On Wed, Aug 13, 2008 at 07:37:40AM +0200, Wolfgang Denk wrote:
This change seems to be completely unrelated to above described changes, so if it was a valid modification, it would have to be split off into a separate commit.
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Where? I see no mention of alignment in the coding style documentation.
I know it's common practice, but could someone please explain *why* TABs are mandated for alignment? It makes sense for indentation, as it allows adjusting the tab size[1], requires fewer keystrokes to change indentation level, and conveys meaning with respect to the structure of the code.
The only thing that using TABs for vertical alignment gets you is some slight compression of the source code when not compressed by other means, an annoying 1/8 chance of the column shifting by 8 characters when the code to the left is changed, the removal of flexibility with respect to tab size, and the breakage of the alignment in patches when the tabs start at the leftmost column (due to the prepended [+- ] characters).
-Scott
[1] Yes, I know that 8 characters is canonical -- but why go out of your way to break other sizes? What "makes things easier to read" is a matter of personal preference -- and if that preference can be accomodated without hardcoding it into the source code, it should be, even if some see it as a "heretic movement". :-)
Dear Scott Wood,
In message 20080813171332.GC24225@ld0162-tx32.am.freescale.net you wrote:
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Where? I see no mention of alignment in the coding style documentation.
See http://www.denx.de/wiki/U-Boot/CodingStyle, bullet 4.2:
Use TAB characters for indentation and vertical alignment, not spaces
I know it's common practice, but could someone please explain *why* TABs are mandated for alignment? It makes sense for indentation, as it allows adjusting the tab size[1], requires fewer keystrokes to change indentation level, and conveys meaning with respect to the structure of the code.
The very same reasons apply for vertical alignment.
The only thing that using TABs for vertical alignment gets you is some slight compression of the source code when not compressed by other means, an annoying 1/8 chance of the column shifting by 8 characters when the code to the left is changed, the removal of flexibility with respect to
That's 8 times better than the 100% chance that it will shift when using spaces for alignment.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 20080813171332.GC24225@ld0162-tx32.am.freescale.net you wrote:
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Where? I see no mention of alignment in the coding style documentation.
See http://www.denx.de/wiki/U-Boot/CodingStyle, bullet 4.2:
Use TAB characters for indentation and vertical alignment, not spaces
OK, I was looking at the Coding Standards section of the README.
I know it's common practice, but could someone please explain *why* TABs are mandated for alignment? It makes sense for indentation, as it allows adjusting the tab size[1], requires fewer keystrokes to change indentation level, and conveys meaning with respect to the structure of the code.
The very same reasons apply for vertical alignment.
They do not. It specifically *disallows* changing the tab size, and has no correlation to code structure.
The only thing that using TABs for vertical alignment gets you is some slight compression of the source code when not compressed by other means, an annoying 1/8 chance of the column shifting by 8 characters when the code to the left is changed, the removal of flexibility with respect to
That's 8 times better than the 100% chance that it will shift when using spaces for alignment.
I disagree. It's much easier to edit when I know how many spaces I'll have to add/remove beforehand.
-Scott
Dear Wolfgang, Please see comments below.
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Please do not do this.
The problem is that lot of existing code use spaces to align the defines. You can see include/ppc4xx.h and lot of other header files. I have seen that spaces are used only in defines. As an example in ppc4xx.h. I can send thousands of other places.:
#if defined(CONFIG_405EX) || $ defined(CONFIG_440SP) || defined(CONFIG_440SPE) || $ defined(CONFIG_460EX) || defined(CONFIG_460GT)$ #define CONFIG_SDRAM_PPC4xx_IBM_DDR2^I/* IBM DDR(2) controller */$ #endif$
Dear Wolfgang, One more comment.
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Please do not do this.
I basically moved the defines from include/ppc440.h to include/ppc4xx.h. If you go to include/ppc440.h, you will see that the original define was aligned With spaces. I just followed whatever was it.
Here is the original code in ppc440.h
#if defined(CONFIG_440EP) || defined (CONFIG_440GR) || \ ^^ space here ^^ space here defined(CONFIG_440EPX) || defined(CONFIG_440GR) ^^^^^space here
Please advice what is the best way to proceed. As you can see, this is certainly not my invention.
Regards, Prodyut Hazarika
===================== Staff S/w Engineer ===================== --------------------------------------------------------
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
Dear "Prodyut Hazarika",
In message 0CA0A16855646F4FA96D25A158E299D604DB40C1@SDCEXCHANGE01.ad.amcc.com you wrote:
I basically moved the defines from include/ppc440.h to include/ppc4xx.h. If you go to include/ppc440.h, you will see that the original define was aligned With spaces. I just followed whatever was it.
Here is the original code in ppc440.h
#if defined(CONFIG_440EP) || defined (CONFIG_440GR) || \ ^^ space here ^^ space here defined(CONFIG_440EPX) || defined(CONFIG_440GR) ^^^^^space here
This is not what we are talking about. This part of the code is OK.
I was complaining about this part:
diff --git a/include/asm-ppc/ppc4xx-sdram.h b/include/asm-ppc/ppc4xx-sdram.h index df787b3..d2e3b42 100644 --- a/include/asm-ppc/ppc4xx-sdram.h +++ b/include/asm-ppc/ppc4xx-sdram.h @@ -259,23 +259,39 @@ /*
- Memory queue defines
*/ -#define SDRAMQ_DCR_BASE<<<>>>0x040
-#define SDRAM_R0BAS<>SDRAMQ_DCR_BASE+0x0)<<>>/* rank 0 base address & size */ -#define SDRAM_R1BAS<>(SDRAMQ_DCR_BASE+0x1)<>>/* rank 1 base address & size */ -#define SDRAM_R2BAS<>(SDRAMQ_DCR_BASE+0x2)<>>/* rank 2 base address & size */ -#define SDRAM_R3BAS<>(SDRAMQ_DCR_BASE+0x3)<>>/* rank 3 base address & size */ -#define SDRAM_CONF1HB<<<<>>>>(SDRAMQ_DCR_BASE+0x5)<>>/* configuration 1 HB */ -#define SDRAM_ERRSTATHB<<<>>>(SDRAMQ_DCR_BASE+0x7)<>>/* error status HB */ -#define SDRAM_ERRADDUHB<<<>>>(SDRAMQ_DCR_BASE+0x8)<>>/* error address upper 32 HB */ -#define SDRAM_ERRADDLHB<<<>>>(SDRAMQ_DCR_BASE+0x9)<>>/* error address lower 32 HB */ -#define SDRAM_PLBADDULL<<<>>>(SDRAMQ_DCR_BASE+0xA)<>>/* PLB base address upper 32 LL */ -#define SDRAM_CONF1LL<<<<>>>>(SDRAMQ_DCR_BASE+0xB)<>>/* configuration 1 LL */ -#define SDRAM_ERRSTATLL<<<>>>(SDRAMQ_DCR_BASE+0xC)<>>/* error status LL */ -#define SDRAM_ERRADDULL<<<>>>(SDRAMQ_DCR_BASE+0xD)<>>/* error address upper 32 LL */ -#define SDRAM_ERRADDLLL<<<>>>(SDRAMQ_DCR_BASE+0xE)<>>/* error address lower 32 LL */ -#define SDRAM_CONFPATHB<<<>>>(SDRAMQ_DCR_BASE+0xF)<>>/* configuration between paths */ -#define SDRAM_PLBADDUHB<<<>>>(SDRAMQ_DCR_BASE+0x10)<>/* PLB base address upper 32 LL */ +#define SDRAMQ_DCR_BASE 0x040
+#define SDRAM_R0BAS (SDRAMQ_DCR_BASE+0x0) /* rank 0 base address & size */
----------------------^^^^^^--------------------^^^ all spaces here
+#define SDRAM_R1BAS (SDRAMQ_DCR_BASE+0x1) /* rank 1 base address & size */ +#define SDRAM_R2BAS (SDRAMQ_DCR_BASE+0x2) /* rank 2 base address & size */ +#define SDRAM_R3BAS (SDRAMQ_DCR_BASE+0x3) /* rank 3 base address & size */ +#define SDRAM_CONF1HB (SDRAMQ_DCR_BASE+0x5) /* configuration 1 HB */ +#define SDRAM_CONF1HB_AAFR 0x80000000 /* Address Ack on First Request - Bit 0 */ +#define SDRAM_CONF1HB_PRPD 0x00080000 /* PLB Read pipeline Disable - Bit 12 */ +#define SDRAM_CONF1HB_PWPD 0x00040000 /* PLB Write pipeline Disable - Bit 13 */ +#define SDRAM_CONF1HB_PRW 0x00020000 /* PLB Read Wait - Bit 14 */ +#define SDRAM_CONF1HB_RPEN 0x00000800 /* Read Passing Enable - Bit 20 */ +#define SDRAM_CONF1HB_RFTE 0x00000400 /* Read Flow Through Enable - Bit 21 */
... -------------------------^^^----------^^^^^^^^^ allspeace here
I marked the places in the original part that were actually TAB characters by sequences of "<<>>" so you can see what I mean.
[Actually, the comment-end markers "*/" should be aligned by TABs, too.]
Best regards,
Wolfgang Denk
Dear Wolfgang, Thanks for the clarification. I will change include/asm-ppc/ppc4xx-sdram.h to tabs and resubmit.
But one more question.
#if defined(CONFIG_440EP) || defined (CONFIG_440GR) || \ ^^ space here ^^ space here defined(CONFIG_440EPX) || defined(CONFIG_440GR)
This is not what we are talking about. This part of the code is OK.
I want to understand why is space alignment okay here. The coding guidelines state that we must use tabs for vertical alignment. Does this rule not apply to defines? If so, the coding guidelines doc should be updated.
Best regards, Prodyut Hazarika
===================== Staff S/W Engineer AMCC ===================== --------------------------------------------------------
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries. It is to be used solely for the purpose of furthering the parties' business relationship. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.
Dear "Prodyut Hazarika",
In message 0CA0A16855646F4FA96D25A158E299D604DB40F8@SDCEXCHANGE01.ad.amcc.com you wrote:
#if defined(CONFIG_440EP) || defined (CONFIG_440GR) || \ ^^ space here ^^ space here defined(CONFIG_440EPX) || defined(CONFIG_440GR)
This is not what we are talking about. This part of the code is OK.
I want to understand why is space alignment okay here. The coding
The intention of vertical alignment is to make code look "nice". Com- puters don't care (at least to the best of my knowledge), but humans seem to find it easier if code is neatly aligned in columns.
For example, compare this example (randomly selected - it's from include/4xx_i2c.h):
(1) #define IIC_OK 0 #define IIC_NOK 1 #define IIC_NOK_LA 2 /* Lost arbitration */ #define IIC_NOK_ICT 3 /* Incomplete transfer */ #define IIC_NOK_XFRA 4 /* Transfer aborted */ #define IIC_NOK_DATA 5 /* No data in buffer */ #define IIC_NOK_TOUT 6 /* Transfer timeout */
#define IIC_TIMEOUT 1 /* 1 second */
against:
(2) #define IIC_OK 0 #define IIC_NOK 1 #define IIC_NOK_LA 2 /* Lost arbitration */ #define IIC_NOK_ICT 3 /* Incomplete transfer */ #define IIC_NOK_XFRA 4 /* Transfer aborted */ #define IIC_NOK_DATA 5 /* No data in buffer */ #define IIC_NOK_TOUT 6 /* Transfer timeout */
#define IIC_TIMEOUT 1 /* 1 second */
Which is nicer, and easier to read - (1) or (2)?
Now if you have something that starts "#if defined(..)" in the first line, then it seems only natural to me that (assuming we have to add another line, which actually is an indication of a coding problem in the first place), it should be aligned such that the "defined()" parts start in the same column. So the second line would follow like this:
#if defined(foo) ... || \ defined(bar)
There is no law, no written rule for that, just common sense and a a certain amount of esthetical sensation.
guidelines state that we must use tabs for vertical alignment. Does this rule not apply to defines? If so, the coding guidelines doc should be updated.
I'm sorry, but it would exceed my capabilities as an author to put that in writing. Feel free to add to the wiki and improve the description there yourself if you feel it needs it.
Best regards,
Wolfgang Denk
Dear "prodyut hazarika",
In message 49c0ff980808131204hdc940cel1dcc28d607429f78@mail.gmail.com you wrote:
But then, you are changing good TAB chanracters that were used for vertical alignment into spaces. This is incorrect - please read the Coding Style requirements.
Please do not do this.
The problem is that lot of existing code use spaces to align the defines. You can see include/ppc4xx.h and lot of other header files. I have seen that spaces are used only in defines. As an example in ppc4xx.h. I can send thousands of other places.:
#if defined(CONFIG_405EX) || $ defined(CONFIG_440SP) || defined(CONFIG_440SPE) || $ defined(CONFIG_460EX) || defined(CONFIG_460GT)$ #define CONFIG_SDRAM_PPC4xx_IBM_DDR2^I/* IBM DDR(2) controller */$ #endif$
Here it makes sense to align the 'define's vertically, and it seems obvious that only spaces can be used here.
Yes, I am aware that there are lots of bad examples around, but please take the good ones as a guide, not the bad ones.
Finally, no matter what any examples were that you might have followed when writing new code. What I am complaining about is that you changed good code and converted it into bad one.
Best regards,
Wolfgang Denk
participants (6)
-
fkan＠amcc.com -
Prodyut Hazarika -
prodyut hazarika -
Scott Wood -
Stefan Roese -
Wolfgang Denk