ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

PATCH New fkan at amcc.com 6 participants 15 messages Aug 13, 2008

lore

Share & tools

lore

fkan at amcc.com

author

2008-08-13 04:43 UTC #0 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

From: Prodyut Hazarika <[email protected]>

Signed-off-by Prodyut Hazarika <[email protected]>

Acked-by Feng Kan <[email protected]>

---
 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(-)

cpu/ppc4xx/44x_spd_ddr2.c +23 -4

index 1c36324..0431754 100644
@@ -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
 }

cpu/ppc4xx/cpu_init.c +15 -1

index e2d0402..c7c429e 100644
@@ -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 */
 }
 
 /*

include/asm-ppc/ppc4xx-sdram.h +33 ~~-17~~

56 lines not on this page · raw message

include/ppc440.h +0 ~~-47~~

53 lines not on this page · raw message

include/ppc4xx.h +55 -0

61 lines not on this page · raw message

Signature · 1 line

1.5.5

Wolfgang Denk

NAK

2008-08-13 05:37 UTC #1 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear Feng Kan,

Quoted 5 lines · in message <[email protected]> you wrote:

From: Prodyut Hazarika <[email protected]>
Signed-off-by Prodyut Hazarika <[email protected]>
Acked-by Feng Kan <[email protected]>
---

It would be nice if the commit messages contained at least a minimal explanation of the reasons for the changes.

Quoted 48 lines

include/asm-ppc/ppc4xx-sdram.h +23 ~~-17~~

index df787b3..d2e3b42 100644
@@ -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 */

include/asm-ppc/ppc4xx-sdram.h line 294

Quoted 10 lines

include/asm-ppc/ppc4xx-sdram.h +10 -0

+#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.

Quoted 113 lines

include/asm-ppc/ppc4xx-sdram.h +0 -0

 #if !defined(CONFIG_405EX)
 /*

include/ppc440.h +0 ~~-47~~

index 92db15f..3584fd2 100644
@@ -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 */

include/ppc4xx.h +45 -0

index c71da60..154956e 100644
@@ -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

include/ppc4xx.h line 103

Quoted 10 lines

include/ppc4xx.h +10 -0

+#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

Signature · 6 lines

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
Man is the best computer we can put aboard a spacecraft ...  and  the
only one that can be mass produced with unskilled labor.
                                                  - Wernher von Braun

Stefan Roese

2008-08-13 06:26 UTC #2 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Quoted 8 lines · Wolfgang Denk

Quoted 5 lines
From: Prodyut Hazarika <[email protected]>
Signed-off-by Prodyut Hazarika <[email protected]>
Acked-by Feng Kan <[email protected]>
---
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>

Quoted 59 lines

*/ diff --git a/include/ppc4xx.h b/include/ppc4xx.h index c71da60..154956e 100644

include/ppc4xx.h +51 -0

@@ -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)

Quoted 8 lines

include/ppc4xx.h line 103
Quoted 4 lines
include/ppc4xx.h +4 -0
+#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 at denx.de
=====================================================================

Wolfgang Denk

2008-08-13 06:44 UTC #3 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear Stefan Roese,

Quoted 7 lines · In message <[email protected]> you wrote:

Quoted 7 lines

include/ppc4xx.h line 54

Quoted 7 lines

include/ppc4xx.h +6 -0

+/* 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)

--------------------------------------------^^^^^^^^^^^^^
...

Quoted 8 lines

Quoted 3 lines
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

Signature · 5 lines

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
Let's say the docs present a simplified view of reality...    :-)
                      - Larry Wall in  <[email protected]>

Prodyut Hazarika

2008-08-13 13:59 UTC #4 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

In reply to Stefan Roese

Quoted 5 lines

Quoted 3 lines
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.

Quoted 10 lines

Quoted 9 lines
include/ppc4xx.h line 103
Quoted 5 lines
include/ppc4xx.h +5 -0
+#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.

Quoted 4 lines

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.

Stefan Roese

2008-08-13 14:24 UTC #5 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Quoted 10 lines · Prodyut Hazarika

Quoted 8 lines
Quoted 3 lines
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

Scott Wood

2008-08-13 17:13 UTC #6 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

In reply to Wolfgang Denk

Quoted 7 lines · Wolfgang Denk

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". :-)

Wolfgang Denk

2008-08-13 18:31 UTC #7 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear Scott Wood,

Quoted 6 lines · In message <[email protected]> you wrote:

Quoted 4 lines
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

Quoted 5 lines

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.

Quoted 4 lines

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

Signature · 5 lines

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
To live is always desirable.
	-- Eleen the Capellan, "Friday's Child", stardate 3498.9

Scott Wood

2008-08-13 18:44 UTC #8 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Quoted 10 lines · Wolfgang Denk

Quoted 4 lines · In message <[email protected]> you wrote:
Quoted 3 lines
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
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

prodyut hazarika

2008-08-13 19:04 UTC #9 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

In reply to Wolfgang Denk

Dear Wolfgang,
Please see comments below.

Quoted 5 lines

Quoted 5 lines
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$

Prodyut Hazarika

2008-08-13 19:16 UTC #10 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear Wolfgang,
One more comment.

Quoted 5 lines

Quoted 5 lines
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
=====================
--------------------------------------------------------

Wolfgang Denk

2008-08-13 20:08 UTC #11 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear "Prodyut Hazarika",

Quoted 11 lines · In message <0CA0A16855646F4FA96D25A158E299D604DB40C1@SDCEXCHANGE01.ad.amcc.com>…

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:

Quoted 18 lines

include/asm-ppc/ppc4xx-sdram.h +0 ~~-10~~

index df787b3..d2e3b42 100644
@@ -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   */

include/asm-ppc/ppc4xx-sdram.h line 264

Quoted 10 lines

include/asm-ppc/ppc4xx-sdram.h +3 -7

-#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

include/asm-ppc/ppc4xx-sdram.h line 274

Quoted 10 lines

include/asm-ppc/ppc4xx-sdram.h +10 -0

+#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

Signature · 8 lines

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
Making files is easy under  the  UNIX  operating  system.  Therefore,
users  tend  to  create  numerous  files  using large amounts of file
space. It has been said that the only standard thing about  all  UNIX
systems  is  the  message-of-the-day  telling users to clean up their
files.                             - System V.2 administrator's guide

Prodyut Hazarika

2008-08-13 20:19 UTC #12 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear Wolfgang,
Thanks for the clarification. I will change include/asm-ppc/ppc4xx-sdram.h to tabs and resubmit.

But one more question.

Quoted 6 lines

Quoted 4 lines
#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
=====================
--------------------------------------------------------

Wolfgang Denk

2008-08-13 20:56 UTC #13 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

Dear "Prodyut Hazarika",

Quoted 8 lines · In message <0CA0A16855646F4FA96D25A158E299D604DB40F8@SDCEXCHANGE01.ad.amcc.com>…

Quoted 6 lines
Quoted 4 lines
#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.

Quoted 3 lines

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

Signature · 6 lines

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
Es sind ?berhaupt nur die Dummk?pfe, die sich den Befehlen der  M?ch-
tigen  widersetzen.  Um  sie  zu ruinieren ist es genug, ihre Befehle
treu zu erf?llen.                  - Peter Hacks: "Die sch?ne Helena"

Wolfgang Denk

2008-08-13 20:01 UTC #14 lore raw

PATCH 1/1 ppc4xx: Read pipeline depth set to 4 for PPC440SP/SPE, PPC405EX, PPC460EX/GT/SX processors

In reply to prodyut hazarika

Dear "prodyut hazarika",

Quoted 12 lines · In message <[email protected]> you…

Quoted 6 lines
Quoted 6 lines
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.:

Quoted 5 lines

#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

Signature · 6 lines

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
Quotation, n. The act of repeating erroneously the words of  another.
The  words  erroneously  repeated.
                            - Ambrose Bierce _The Devil's Dictionary_