diff mbox

[U-Boot,2/2] powerpc/85xx: Fix the work-arounds for errata SERDES-8 & SERDES-A001 on p4080

Message ID 1309492845-13117-2-git-send-email-galak@kernel.crashing.org
State Accepted
Delegated to: Kumar Gala
Headers show

Commit Message

Kumar Gala July 1, 2011, 4 a.m. UTC 
From: Timur Tabi <timur@freescale.com>

Erratum SERDES-8 says that the clocks for bank 3 needs to be enabled if
bank 2 is enabled, but this was not being done for SERDES protocols 0xF
and 0x10.  The bank reset that was being done for erratum SERDES4 (a
left-over work-around that was removed in "powerpc/85xx: remove SERDES4
soft-reset work-around") also happened to enable bank 3 (apparently an
undocumented feature).  Now that the reset has been removed, bank 3 was not
being enabled for these two SERDES protocols.

It turns out that every time we call enable_bank(), we do want at least one
lane of the bank enabled, either because the bank is supposed to be enabled,
or because we need the clock from that bank enabled.

For erratum SERDES-A001, we don't want to modify srds_lpd_b[] when we
call enable_bank(), because that array is used elsewhere to determine if
the bank is available.

Also fix an off-by-one error in a printf().

Note that the side effect of these changes is that the work-arounds for these
two errata are now linked.  Specifically, if SERDES-A001 is enabled, then we
need SERDES-8 enabled as well.

Signed-off-by: Timur Tabi <timur@freescale.com>
Acked-by: Ed Swarthout <swarthou@freescale.com>
Acked-by: Scott Wood <scottwood@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c |   68 ++++++++++++++++++-------
 1 files changed, 49 insertions(+), 19 deletions(-)

Comments

Tabi Timur-B04825 July 1, 2011, 12:43 p.m. UTC | #1 
Kumar Gala wrote:
> Erratum SERDES-8 says that the clocks for bank 3 needs to be enabled if
> bank 2 is enabled, but this was not being done for SERDES protocols 0xF
> and 0x10.  The bank reset that was being done for erratum SERDES4 (a
> left-over work-around that was removed in "powerpc/85xx: remove SERDES4
> soft-reset work-around") also happened to enable bank 3 (apparently an
> undocumented feature).  Now that the reset has been removed, bank 3 was not
> being enabled for these two SERDES protocols.

Kumar, these two patches should be merged, since the first one breaks 
SERDES, and this one fixes the problem that the first one exposes.  If you 
add them as separate commits, then you'll break git-bisect.
Kumar Gala July 1, 2011, 1:06 p.m. UTC | #2 
On Jul 1, 2011, at 7:43 AM, Tabi Timur-B04825 wrote:

> Kumar Gala wrote:
>> Erratum SERDES-8 says that the clocks for bank 3 needs to be enabled if
>> bank 2 is enabled, but this was not being done for SERDES protocols 0xF
>> and 0x10.  The bank reset that was being done for erratum SERDES4 (a
>> left-over work-around that was removed in "powerpc/85xx: remove SERDES4
>> soft-reset work-around") also happened to enable bank 3 (apparently an
>> undocumented feature).  Now that the reset has been removed, bank 3 was not
>> being enabled for these two SERDES protocols.
> 
> Kumar, these two patches should be merged, since the first one breaks 
> SERDES, and this one fixes the problem that the first one exposes.  If you 
> add them as separate commits, then you'll break git-bisect.


Ok, Can you send a commit message for when I merge them or repost a merged patch.

- k
Tabi Timur-B04825 July 1, 2011, 2:06 p.m. UTC | #3 
Kumar Gala wrote:
> Ok, Can you send a commit message for when I merge them or repost a merged patch.

powerpc/85xx: remove SERDES4 soft-reset work-around

Some P4080 rev1 errata work-arounds, notably erratum SERDES4, required a
bank soft-reset after the bank was configured and enabled, even though 
enabling a bank causes it to reset.  Because the reset was required for 
multiple errata, it was not properly enclosed in an #ifdef, and so was not 
removed with all the other rev1 errata work-arounds.

Erratum SERDES-8 says that the clocks for bank 3 needs to be enabled if
bank 2 is enabled, but this was not being done for SERDES protocols 0xF
and 0x10.  The bank reset also happened to enable bank 3 (apparently an
undocumented feature).  Simply removing the reset breaks these two protocols.

It turns out that every time we call enable_bank(), we do want at least 
one lane of the bank enabled, either because the bank is supposed to be 
enabled, or because we need the clock from that bank enabled.

For erratum SERDES-A001, we don't want to modify srds_lpd_b[] when we
call enable_bank(), because that array is used elsewhere to determine if
the bank is available.

Note that the side effect of these changes is that the work-arounds for 
these two errata are now linked.  Specifically, if SERDES-A001 is enabled, 
then we need SERDES-8 enabled as well.

Because this was the only SERDES bank soft-reset, there is no need to 
implement a work-around for erratum SERDES-A003.

Also fix an off-by-one error in a printf().
Kumar Gala July 11, 2011, 4:13 p.m. UTC | #4 
On Jul 1, 2011, at 9:06 AM, Tabi Timur-B04825 wrote:

> Kumar Gala wrote:
>> Ok, Can you send a commit message for when I merge them or repost a merged patch.
> 
> powerpc/85xx: remove SERDES4 soft-reset work-around
> 
> Some P4080 rev1 errata work-arounds, notably erratum SERDES4, required a
> bank soft-reset after the bank was configured and enabled, even though 
> enabling a bank causes it to reset.  Because the reset was required for 
> multiple errata, it was not properly enclosed in an #ifdef, and so was not 
> removed with all the other rev1 errata work-arounds.
> 
> Erratum SERDES-8 says that the clocks for bank 3 needs to be enabled if
> bank 2 is enabled, but this was not being done for SERDES protocols 0xF
> and 0x10.  The bank reset also happened to enable bank 3 (apparently an
> undocumented feature).  Simply removing the reset breaks these two protocols.
> 
> It turns out that every time we call enable_bank(), we do want at least 
> one lane of the bank enabled, either because the bank is supposed to be 
> enabled, or because we need the clock from that bank enabled.
> 
> For erratum SERDES-A001, we don't want to modify srds_lpd_b[] when we
> call enable_bank(), because that array is used elsewhere to determine if
> the bank is available.
> 
> Note that the side effect of these changes is that the work-arounds for 
> these two errata are now linked.  Specifically, if SERDES-A001 is enabled, 
> then we need SERDES-8 enabled as well.
> 
> Because this was the only SERDES bank soft-reset, there is no need to 
> implement a work-around for erratum SERDES-A003.
> 
> Also fix an off-by-one error in a printf().

merged commits, updated commit message, applied to 85xx

- k
Wolfgang Denk Oct. 6, 2011, 10:12 p.m. UTC | #5 
Dear Kumar Gala,

In message <1309492845-13117-2-git-send-email-galak@kernel.crashing.org> you wrote:
> From: Timur Tabi <timur@freescale.com>
> 
> Erratum SERDES-8 says that the clocks for bank 3 needs to be enabled if
> bank 2 is enabled, but this was not being done for SERDES protocols 0xF
> and 0x10.  The bank reset that was being done for erratum SERDES4 (a
> left-over work-around that was removed in "powerpc/85xx: remove SERDES4
> soft-reset work-around") also happened to enable bank 3 (apparently an
> undocumented feature).  Now that the reset has been removed, bank 3 was not
> being enabled for these two SERDES protocols.
> 
> It turns out that every time we call enable_bank(), we do want at least one
> lane of the bank enabled, either because the bank is supposed to be enabled,
> or because we need the clock from that bank enabled.
> 
> For erratum SERDES-A001, we don't want to modify srds_lpd_b[] when we
> call enable_bank(), because that array is used elsewhere to determine if
> the bank is available.
> 
> Also fix an off-by-one error in a printf().
> 
> Note that the side effect of these changes is that the work-arounds for these
> two errata are now linked.  Specifically, if SERDES-A001 is enabled, then we
> need SERDES-8 enabled as well.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> Acked-by: Ed Swarthout <swarthou@freescale.com>
> Acked-by: Scott Wood <scottwood@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
>  arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c |   68 ++++++++++++++++++-------
>  1 files changed, 49 insertions(+), 19 deletions(-)


Checkpatch says:

WARNING: line over 80 characters
#127: FILE: arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c:43:
+#error "CONFIG_SYS_P4080_ERRATUM_SERDES_A001 requires CONFIG_SYS_P4080_ERRATUM_SERDES8"


Please clean up and resubmit.  Thanks.

Best regards,

Wolfgang Denk
Wolfgang Denk Oct. 6, 2011, 10:13 p.m. UTC | #6 
Dear Kumar Gala,

In message <28544AC3-1F7F-47D5-B534-C0A7D6540597@kernel.crashing.org> you wrote:
> 
> merged commits, updated commit message, applied to 85xx

Please undo / fix the line over 80 characters

Best regards,

Wolfgang Denk
Timur Tabi Oct. 6, 2011, 10:15 p.m. UTC | #7 
Wolfgang Denk wrote:
> WARNING: line over 80 characters
> #127: FILE: arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c:43:
> +#error "CONFIG_SYS_P4080_ERRATUM_SERDES_A001 requires CONFIG_SYS_P4080_ERRATUM_SERDES8"

Lines like these would be accepted into the Linux kernel.  Are the standards for
U-boot stricter than for Linux?  It appears that you are requiring exact
conformance to checkpatch, even when it complains about something that isn't
really that bad.
diff mbox

Patch

diff --git a/arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c b/arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c
index e78d32d..4307a4c 100644
--- a/arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c
+++ b/arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c
@@ -32,6 +32,18 @@ 
 #include <asm/errno.h>
 #include "fsl_corenet_serdes.h"
 
+/*
+ * The work-arounds for erratum SERDES8 and SERDES-A001 are linked together.
+ * The code is already very complicated as it is, and separating the two
+ * completely would just make things worse.  We try to keep them as separate
+ * as possible, but for now we require SERDES8 if SERDES_A001 is defined.
+ */
+#ifdef CONFIG_SYS_P4080_ERRATUM_SERDES_A001
+#ifndef CONFIG_SYS_P4080_ERRATUM_SERDES8
+#error "CONFIG_SYS_P4080_ERRATUM_SERDES_A001 requires CONFIG_SYS_P4080_ERRATUM_SERDES8"
+#endif
+#endif
+
 static u32 serdes_prtcl_map;
 
 #define HWCONFIG_BUFFER_SIZE	128
@@ -259,9 +271,28 @@  void serdes_reset_rx(enum srds_prtcl device)
 #endif
 
 #ifdef CONFIG_SYS_P4080_ERRATUM_SERDES8
+/*
+ * Enable a SERDES bank that was disabled via the RCW
+ *
+ * We only call this function for SERDES8 and SERDES-A001 in cases we really
+ * want to enable the bank, whether we actually want to use the lanes or not,
+ * so make sure at least one lane is enabled.  We're only enabling this one
+ * lane to satisfy errata requirements that the bank be enabled.
+ *
+ * We use a local variable instead of srds_lpd_b[] because we want drivers to
+ * think that the lanes actually are disabled.
+ */
 static void enable_bank(ccsr_gur_t *gur, int bank)
 {
 	u32 rcw5;
+	u32 temp_lpd_b = srds_lpd_b[bank];
+
+	/*
+	 * If we're asked to disable all lanes, just pretend we're doing
+	 * that.
+	 */
+	if (temp_lpd_b == 0xF)
+		temp_lpd_b = 0xE;
 
 	/*
 	 * Enable the lanes SRDS_LPD_Bn.  The RCW bits are read-only in
@@ -270,10 +301,10 @@  static void enable_bank(ccsr_gur_t *gur, int bank)
 	rcw5 = in_be32(gur->rcwsr + 5);
 	if (bank == FSL_SRDS_BANK_2) {
 		rcw5 &= ~FSL_CORENET_RCWSRn_SRDS_LPD_B2;
-		rcw5 |= srds_lpd_b[bank] << 26;
+		rcw5 |= temp_lpd_b << 26;
 	} else if (bank == FSL_SRDS_BANK_3) {
 		rcw5 &= ~FSL_CORENET_RCWSRn_SRDS_LPD_B3;
-		rcw5 |= srds_lpd_b[bank] << 18;
+		rcw5 |= temp_lpd_b << 18;
 	} else {
 		printf("SERDES: enable_bank: bad bank %d\n", bank + 1);
 		return;
@@ -343,8 +374,6 @@  static void p4080_erratum_serdes8(serdes_corenet_t *regs, ccsr_gur_t *gur,
 		 */
 		setbits_be32(&regs->bank[FSL_SRDS_BANK_3].pllcr1,
 			     SRDS_PLLCR1_PLL_BWSEL);
-
-		enable_bank(gur, FSL_SRDS_BANK_3);
 		break;
 
 	case 0x0f:
@@ -379,10 +408,9 @@  static void p4080_erratum_serdes8(serdes_corenet_t *regs, ccsr_gur_t *gur,
 				SRDS_PLLCR0_FRATE_SEL_MASK,
 				SRDS_PLLCR0_FRATE_SEL_6_25);
 		break;
-	default:
-		enable_bank(gur, FSL_SRDS_BANK_3);
 	}
 
+	enable_bank(gur, FSL_SRDS_BANK_3);
 }
 #endif
 
@@ -451,7 +479,7 @@  static void wait_for_rstdone(unsigned int bank)
 	} while (end_tick > get_ticks());
 
 	if (!(rstctl & SRDS_RSTCTL_RSTDONE))
-		printf("SERDES: timeout resetting bank %u\n", bank);
+		printf("SERDES: timeout resetting bank %u\n", bank + 1);
 }
 
 void fsl_serdes_init(void)
@@ -523,6 +551,14 @@  void fsl_serdes_init(void)
 			srds_lpd_b[bank] =
 				simple_strtoul(srds_lpd_arg, NULL, 0) & 0xf;
 	}
+
+	if ((cfg == 0xf) || (cfg == 0x10)) {
+		/*
+		 * For SERDES protocols 0xF and 0x10, force bank 3 to be
+		 * disabled, because it is not supported.
+		 */
+		srds_lpd_b[FSL_SRDS_BANK_3] = 0xF;
+	}
 #endif
 
 	/* Look for banks with all lanes disabled, and power down the bank. */
@@ -546,7 +582,10 @@  void fsl_serdes_init(void)
 #ifdef CONFIG_SYS_P4080_ERRATUM_SERDES_A001
 	/*
 	 * The work-aroud for erratum SERDES-A001 is needed only if bank two
-	 * is disabled and bank three is enabled.
+	 * is disabled and bank three is enabled.  The converse is also true,
+	 * but SERDES8 ensures that bank 3 is always enabled if bank 2 is
+	 * enabled, so there's no point in complicating the code to handle
+	 * that situation.
 	 */
 	need_serdes_a001 =
 		!have_bank[FSL_SRDS_BANK_2] && have_bank[FSL_SRDS_BANK_3];
@@ -724,7 +763,7 @@  void fsl_serdes_init(void)
 			p4080_erratum_serdes8(srds_regs, gur, serdes8_devdisr,
 					      serdes8_devdisr2, cfg);
 		} else if (idx == 2) {
-			/* Eable bank two now that bank three is enabled. */
+			/* Enable bank two now that bank three is enabled. */
 			enable_bank(gur, FSL_SRDS_BANK_2);
 		}
 #endif
@@ -734,16 +773,7 @@  void fsl_serdes_init(void)
 
 #ifdef CONFIG_SYS_P4080_ERRATUM_SERDES_A001
 	if (need_serdes_a001) {
-		/*
-		 * Bank three has been enabled, so enable bank two and then
-		 * disable it.
-		 */
-		srds_lpd_b[FSL_SRDS_BANK_2] = 0;
-		enable_bank(gur, FSL_SRDS_BANK_2);
-
-		wait_for_rstdone(FSL_SRDS_BANK_2);
-
-		/* Disable bank 2 */
+		/* Bank 3 has been enabled, so now we can disable bank 2 */
 		setbits_be32(&srds_regs->bank[FSL_SRDS_BANK_2].rstctl,
 			     SRDS_RSTCTL_SDPD);
 	}