| Message ID | 1337392391-26822-1-git-send-email-yorksun@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andy Fleming |
| Headers | show |
Please copy me on any 85xx U-Boot patches. > > +#define HWCONFIG_BUFFER_SIZE 128 [...] > + char buffer[HWCONFIG_BUFFER_SIZE]; > + char *buf = NULL; > + > + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) > + buf = buffer; This seems fragile. I suppose there's no way to ensure that this succeeds, but we should at least check to see if the buffer we pass in was big enough, and let the user know that we were looking for the A011 erratum setting, and may not have found it because the hwconfig string was more than 128 characters. > + > + if (hwconfig_arg_cmp_f("fsl_cpu_a011", "enable", buf) > 0) { > + enable_cpu_a011_workaround = > + (SVR_SOC_VER(svr) != SVR_P4080 || SVR_MAJ(svr) < 3); Please merge this patch with its follow-on that changes the default to be enabled. Andy
On Wed, 2012-08-08 at 15:09 -0500, Andy Fleming wrote: > Please copy me on any 85xx U-Boot patches. > > > > > > +#define HWCONFIG_BUFFER_SIZE 128 > > > [...] > > > + char buffer[HWCONFIG_BUFFER_SIZE]; > > + char *buf = NULL; > > + > > + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) > > + buf = buffer; > > > This seems fragile. I suppose there's no way to ensure that this > succeeds, but we should at least check to see if the buffer we pass in > was big enough, and let the user know that we were looking for the > A011 erratum setting, and may not have found it because the hwconfig > string was more than 128 characters. > If the buffer isn't big enough, getenv_f() will print a message saying that. Is that enough? Or do you prefer another warning when fsl_cpu_a011 is not detected? It seems a little bit redundant. York
On Wed, Aug 8, 2012 at 6:16 PM, York Sun <yorksun@freescale.com> wrote: > On Wed, 2012-08-08 at 15:09 -0500, Andy Fleming wrote: >> Please copy me on any 85xx U-Boot patches. >> >> >> > >> > +#define HWCONFIG_BUFFER_SIZE 128 >> >> >> [...] >> >> > + char buffer[HWCONFIG_BUFFER_SIZE]; >> > + char *buf = NULL; >> > + >> > + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) >> > + buf = buffer; >> >> >> This seems fragile. I suppose there's no way to ensure that this >> succeeds, but we should at least check to see if the buffer we pass in >> was big enough, and let the user know that we were looking for the >> A011 erratum setting, and may not have found it because the hwconfig >> string was more than 128 characters. >> > > If the buffer isn't big enough, getenv_f() will print a message saying > that. Is that enough? Or do you prefer another warning when fsl_cpu_a011 > is not detected? It seems a little bit redundant. Well, I think that the user will want to know that it's fsl_cpu_a011 that isn't being found. The code is able to know that, whereas the user would have to dig around for a while to figure out what was wrong. Especially if the user adds random stuff to the beginning, and that causes the erratum variable to fall off the end. Andy
Dear York Sun, In message <1344467781.18960.41.camel@oslab-l1> you wrote: > > If the buffer isn't big enough, getenv_f() will print a message saying ... but only if it can (i. e. if he call is _after_ the initialization of the console driver has been completed (which is actually pretty late). Best regards, Wolfgang Denk
diff --git a/arch/powerpc/cpu/mpc85xx/cmd_errata.c b/arch/powerpc/cpu/mpc85xx/cmd_errata.c index 4e1a54a..858b3f8 100644 --- a/arch/powerpc/cpu/mpc85xx/cmd_errata.c +++ b/arch/powerpc/cpu/mpc85xx/cmd_errata.c @@ -27,6 +27,9 @@ static int do_errata(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { +#ifdef CONFIG_SYS_FSL_ERRATUM_NMG_CPU_A011 + extern int enable_cpu_a011_workaround; +#endif __maybe_unused u32 svr = get_svr(); #if defined(CONFIG_FSL_SATA_V2) && defined(CONFIG_FSL_SATA_ERRATUM_A001) @@ -56,8 +59,9 @@ static int do_errata(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) /* * NMG_CPU_A011 applies to P4080 rev 1.0, 2.0, fixed in 3.0 * also applies to P3041 rev 1.0, 1.1, P2041 rev 1.0, 1.1 + * The SVR has been checked by cpu_init_r(). */ - if (SVR_SOC_VER(svr) != SVR_P4080 || SVR_MAJ(svr) < 3) + if (enable_cpu_a011_workaround) puts("Work-around for Erratum CPU-A011 enabled\n"); #endif #if defined(CONFIG_SYS_FSL_ERRATUM_CPU_A003999) diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c b/arch/powerpc/cpu/mpc85xx/cpu_init.c index 4f03d25..10f142e 100644 --- a/arch/powerpc/cpu/mpc85xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c @@ -38,6 +38,7 @@ #include <asm/fsl_law.h> #include <asm/fsl_serdes.h> #include <asm/fsl_srio.h> +#include <hwconfig.h> #include <linux/compiler.h> #include "mp.h" #ifdef CONFIG_SYS_QE_FMAN_FW_IN_NAND @@ -47,6 +48,8 @@ #include "../../../../drivers/block/fsl_sata.h" +#define HWCONFIG_BUFFER_SIZE 128 + DECLARE_GLOBAL_DATA_PTR; #ifdef CONFIG_QE @@ -311,11 +314,30 @@ int cpu_init_r(void) #if defined(CONFIG_SYS_P4080_ERRATUM_CPU22) || \ defined(CONFIG_SYS_FSL_ERRATUM_NMG_CPU_A011) /* + * CPU22 and NMG_CPU_A011 share the same workaround. * CPU22 applies to P4080 rev 1.0, 2.0, fixed in 3.0 * NMG_CPU_A011 applies to P4080 rev 1.0, 2.0, fixed in 3.0 * also applies to P3041 rev 1.0, 1.1, P2041 rev 1.0, 1.1 + * NMG_CPU_A011 is activated by hwconfig with syntax: + * fsl_cpu_a011:enable */ - if (SVR_SOC_VER(svr) != SVR_P4080 || SVR_MAJ(svr) < 3) { + extern int enable_cpu_a011_workaround; +#ifdef CONFIG_SYS_P4080_ERRATUM_CPU22 + enable_cpu_a011_workaround = + (SVR_SOC_VER(svr) != SVR_P4080 || SVR_MAJ(svr) < 3); +#else + char buffer[HWCONFIG_BUFFER_SIZE]; + char *buf = NULL; + + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) + buf = buffer; + + if (hwconfig_arg_cmp_f("fsl_cpu_a011", "enable", buf) > 0) { + enable_cpu_a011_workaround = + (SVR_SOC_VER(svr) != SVR_P4080 || SVR_MAJ(svr) < 3); + } +#endif + if (enable_cpu_a011_workaround) { flush_dcache(); mtspr(L1CSR2, (mfspr(L1CSR2) | L1CSR2_DCWS)); sync(); diff --git a/arch/powerpc/cpu/mpc85xx/release.S b/arch/powerpc/cpu/mpc85xx/release.S index 1860684..b8d4a37 100644 --- a/arch/powerpc/cpu/mpc85xx/release.S +++ b/arch/powerpc/cpu/mpc85xx/release.S @@ -163,6 +163,12 @@ __secondary_start_page: cmpw r3,r5 bge 2f 1: +#ifdef CONFIG_SYS_FSL_ERRATUM_NMG_CPU_A011 + lis r3,toreset(enable_cpu_a011_workaround)@ha + lwz r3,toreset(enable_cpu_a011_workaround)@l(r3) + cmpwi r3,0 + beq 2f +#endif mfspr r3,L1CSR2 oris r3,r3,(L1CSR2_DCWS)@h mtspr L1CSR2,r3 @@ -346,6 +352,15 @@ __bootpg_addr: __spin_table: .space CONFIG_MAX_CPUS*ENTRY_SIZE + /* + * This variable is set by cpu_init_r() after parsing hwconfig + * to enable workaround for erratum NMG_CPU_A011. + */ + .align L1_CACHE_SHIFT + .global enable_cpu_a011_workaround +enable_cpu_a011_workaround: + .long 0 + /* Fill in the empty space. The actual reset vector is * the last word of the page */ __secondary_start_code_end:
Workaround for erratum NMG_CPU_A011 can be enabled by hwconfig with syntax fsl_cpu_a011:enable This workaround is not enabled by default. Enabling this workaround may degrade performance. P4080 erratum CPU22 shares the same workaround. So it is always enabled for P4080. Signed-off-by: York Sun <yorksun@freescale.com> --- arch/powerpc/cpu/mpc85xx/cmd_errata.c | 6 +++++- arch/powerpc/cpu/mpc85xx/cpu_init.c | 24 +++++++++++++++++++++++- arch/powerpc/cpu/mpc85xx/release.S | 15 +++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-)