[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc
From: Feng Kan fkan@amcc.com
This is to lock down the ordering in the correction routine against the calculate routine. Otherwise, incorrect define would cause ECC errors.
Signed-off-by: Feng Kan fkan@amcc.com Acked-by: Victor Gallardo vgallardo@amcc.com --- drivers/mtd/nand/ndfc.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/nand/ndfc.c b/drivers/mtd/nand/ndfc.c index 0dd6789..88e341d 100644 --- a/drivers/mtd/nand/ndfc.c +++ b/drivers/mtd/nand/ndfc.c @@ -89,9 +89,15 @@ static int ndfc_calculate_ecc(struct mtd_info *mtdinfo,
/* The NDFC uses Smart Media (SMC) bytes order */ +#ifdef CONFIG_MTD_NAND_ECC_SMC ecc_code[0] = p[1]; ecc_code[1] = p[2]; ecc_code[2] = p[3]; +#else + ecc_code[0] = p[2]; + ecc_code[1] = p[1]; + ecc_code[2] = p[3]; +#endif
return 0; }
Dear fkan@amcc.com,
In message 1266531913-20756-1-git-send-email-fkan@amcc.com you wrote:
From: Feng Kan fkan@amcc.com
This is to lock down the ordering in the correction routine against the calculate routine. Otherwise, incorrect define would cause ECC errors.
Signed-off-by: Feng Kan fkan@amcc.com Acked-by: Victor Gallardo vgallardo@amcc.com
drivers/mtd/nand/ndfc.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/nand/ndfc.c b/drivers/mtd/nand/ndfc.c index 0dd6789..88e341d 100644 --- a/drivers/mtd/nand/ndfc.c +++ b/drivers/mtd/nand/ndfc.c @@ -89,9 +89,15 @@ static int ndfc_calculate_ecc(struct mtd_info *mtdinfo,
/* The NDFC uses Smart Media (SMC) bytes order */ +#ifdef CONFIG_MTD_NAND_ECC_SMC ecc_code[0] = p[1]; ecc_code[1] = p[2]; ecc_code[2] = p[3]; +#else
- ecc_code[0] = p[2];
- ecc_code[1] = p[1];
- ecc_code[2] = p[3];
+#endif
This patch seems wrong to me as CONFIG_MTD_NAND_ECC_SMC is nowhere defined. [Also, it's not documented anywhere.]
If this is fixing a bug, then please describe the exact problem, how to reproduce it, and how this patch is supposed to fix this problem.
As is, this makes no sense to me.
Best regards,
Wolfgang Denk
Dear Wolfgang:
The problem goes back a bit. The ordering you see in the ndfc file has been changed a few times, back and forth and cause quite a bit of problem. The define we speak of is in the driver/mtd/nand/nand_ecc.c file. The nand_correct_data function uses two ways of check ECC correctness. However the ndfc calculate only supports one ordering, although both placement method in the patch would work. It also serves to nail down the ordering depending on the define is used or not.
There is also the following in the code, should you agree, this will also need to be removed as well.
/* The PPC4xx NDFC uses Smart Media (SMC) bytes order */ #ifdef CONFIG_NAND_NDFC #define CONFIG_MTD_NAND_ECC_SMC #endif
Feng Kan
On 02/18/2010 03:13 PM, Wolfgang Denk wrote:
Dear fkan@amcc.com,
In message1266531913-20756-1-git-send-email-fkan@amcc.com you wrote:
From: Feng Kanfkan@amcc.com
This is to lock down the ordering in the correction routine against the calculate routine. Otherwise, incorrect define would cause ECC errors.
Signed-off-by: Feng Kanfkan@amcc.com Acked-by: Victor Gallardovgallardo@amcc.com
drivers/mtd/nand/ndfc.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/nand/ndfc.c b/drivers/mtd/nand/ndfc.c index 0dd6789..88e341d 100644 --- a/drivers/mtd/nand/ndfc.c +++ b/drivers/mtd/nand/ndfc.c @@ -89,9 +89,15 @@ static int ndfc_calculate_ecc(struct mtd_info *mtdinfo,
/* The NDFC uses Smart Media (SMC) bytes order */ +#ifdef CONFIG_MTD_NAND_ECC_SMC ecc_code[0] = p[1]; ecc_code[1] = p[2]; ecc_code[2] = p[3]; +#else
- ecc_code[0] = p[2];
- ecc_code[1] = p[1];
- ecc_code[2] = p[3];
+#endif
This patch seems wrong to me as CONFIG_MTD_NAND_ECC_SMC is nowhere defined. [Also, it's not documented anywhere.]
If this is fixing a bug, then please describe the exact problem, how to reproduce it, and how this patch is supposed to fix this problem.
As is, this makes no sense to me.
Best regards,
Wolfgang Denk
Dear Feng Kan,
In message 4B7DD691.8070805@amcc.com you wrote:
The problem goes back a bit. The ordering you see in the ndfc file has been changed a few times, back and forth and cause quite a bit of problem. The define we speak of is in the driver/mtd/nand/nand_ecc.c file. The nand_correct_data function uses two ways
Right, CONFIG_MTD_NAND_ECC_SMC is only ever defined and used in driver/mtd/nand/nand_ecc.c, but your patch modifies drivers/mtd/nand/ndfc.c, i. e. a different file - so this #define will never be seen there.
Either the code needs to be permanently changed, then we don't need the #ifdef stuff, or it depends on some conditions, then it's unclear what these might be.
In any case a clear description of the problem you are trying to fix is needed, and an explanation how your change is supposed to fix this problem.
Please provide a specific test case that can be used to 1) see the problem in the unchanged code and 2) verify that it's working after applying your suggested changes.
of check ECC correctness. However the ndfc calculate only supports one ordering, although both placement method in the patch would work. It also serves to nail down the ordering depending on the define is used or not.
I don;t understand what you mean here. Sorry, but I'm afraid you have to provide a bit more context.
There is also the following in the code, should you agree, this will also need to be removed as well.
/* The PPC4xx NDFC uses Smart Media (SMC) bytes order */ #ifdef CONFIG_NAND_NDFC #define CONFIG_MTD_NAND_ECC_SMC #endif
This is in another file (driver/mtd/nand/nand_ecc.c) which is not touched by your patch. If you think this file needs to be changed as well, then this change should be part of your patch. Obviously, the reason for the need to change has to be explained here as well.
Thanks.
Wolfgang Denk
Hi Feng,
On Thursday 18 February 2010 23:25:13 fkan@amcc.com wrote:
From: Feng Kan fkan@amcc.com
This is to lock down the ordering in the correction routine against the calculate routine. Otherwise, incorrect define would cause ECC errors.
It was my impression that we (finally) had done this ordering correct. The last changes were upon your request:
68e74567cf317318df52dbcb2ac170ffc5e7758a: ppc4xx: Fix ECC Correction bug with SMC ordering for NDFC driver
I don't see how this patch should fix a potential problem. Please explain which problem exactly is fixed with this change. As Wolfgang already mentioned, CONFIG_MTD_NAND_ECC_SMC will not be set in this file.
Cheers, 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
Hi Stefan:
Agreed the ordering is working now. Previously the ordering is 213 and with CONFIG_MTD_NAND_ECC_SMC defined, wrong ECC error bit position was calculated. What about the boards that are now stuck on the 213 ordering. Also in linux, the ordering is not fixed down as in u-boot. Hmm, perhaps that is another approach to the problem. Fix the ordering in linux?
Feng
On 02/18/2010 11:57 PM, Stefan Roese wrote:
Hi Feng,
On Thursday 18 February 2010 23:25:13 fkan@amcc.com wrote:
From: Feng Kanfkan@amcc.com
This is to lock down the ordering in the correction routine against the calculate routine. Otherwise, incorrect define would cause ECC errors.
It was my impression that we (finally) had done this ordering correct. The last changes were upon your request:
68e74567cf317318df52dbcb2ac170ffc5e7758a: ppc4xx: Fix ECC Correction bug with SMC ordering for NDFC driver
I don't see how this patch should fix a potential problem. Please explain which problem exactly is fixed with this change. As Wolfgang already mentioned, CONFIG_MTD_NAND_ECC_SMC will not be set in this file.
Cheers, 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
Hi Feng,
On Friday 19 February 2010 19:27:24 Feng Kan wrote:
Agreed the ordering is working now. Previously the ordering is 213 and with CONFIG_MTD_NAND_ECC_SMC defined, wrong ECC error bit position was calculated. What about the boards that are now stuck on the 213 ordering.
Which boards are stuck with this (incorrect) ordering? Please give an example.
Also in linux, the ordering is not fixed down as in u-boot.
I thought we had this fixed (or synced) in U-Boot *and* Linux.
Hmm, perhaps that is another approach to the problem. Fix the ordering in linux?
Again, I fail to see the problem here. Please give an example which board is failing here.
Thanks.
Cheers, 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
Hi Stefan:
There is not a particular board that is stuck on this 213 format. Rather, for sometime u-boot and linux both had the 213 ordering. Lets say the guy did not have the SMC define turned on, which mean the ECC would caculate correctly for him. Now, he gets a new U-boot and want to update it. He gets to prompt and programs the new u-boot and linux (in 213 ordering). The new uboot comes up (it doesn't complain since there is no error message), runs to linux, the new linux expects 123 ordering. Finds ECC error and tries to correct and crash.
I submitted this patch to support both ordering that the correction routine contains (123 and 213). Realistically, you can have any ordering you want (312 321) as long as the correction routine supports it. It is because of this reason, that the ndfc.c ordering keeps getting changed. I want the user to lock down the ordering they use. So they don't make the mistake of selecting SMC define but uses the 213 ordering (which would cause ecc errors).
Cheers, Feng
________________________________
From: Stefan Roese [mailto:sr@denx.de] Sent: Mon 2/22/2010 2:52 AM To: Feng Kan Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc
Hi Feng,
On Friday 19 February 2010 19:27:24 Feng Kan wrote:
Agreed the ordering is working now. Previously the ordering is 213 and with CONFIG_MTD_NAND_ECC_SMC defined, wrong ECC error bit position was calculated. What about the boards that are now stuck on the 213 ordering.
Which boards are stuck with this (incorrect) ordering? Please give an example.
Also in linux, the ordering is not fixed down as in u-boot.
I thought we had this fixed (or synced) in U-Boot *and* Linux.
Hmm, perhaps that is another approach to the problem. Fix the ordering in linux?
Again, I fail to see the problem here. Please give an example which board is failing here.
Thanks.
Cheers, 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 Feng Kan,
In message 2B3B2AA816369A4E87D7BE63EC9D2F260615A5EC@SDCEXCHANGE01.ad.amcc.com you wrote:
There is not a particular board that is stuck on this 213 format. Rather, for sometime u-boot and linux both had the 213 ordering. Lets say the guy did not have the SMC define turned on, which mean the ECC would caculate correctly for him. Now, he gets a new U-boot and want to update it. He gets to prompt and programs the new u-boot and linux (in 213 ordering). The new uboot comes up (it doesn't complain since there is no error message), runs to linux, the new linux expects 123 ordering. Finds ECC error and tries to correct and crash.
If this is your concern, then a compile-time setting makes little sense - you don't really expect that a user in this situation will build another U-Boot image after selecting other build options, install it (with the risk of bricking his device (keep in mind that not everybody has access to a JTAG debugger), and continue this so long until he finds a configuration that works for his combination of U-Boot and Linux settings.
I submitted this patch to support both ordering that the correction routine contains (123 and 213).
But it makes no sense as a compile time option.
If you want to help users, then this must be implemented in a way that is selectable at run-time, for example by simple setting an
Best regards,
Wolfgang Denk
Dear Wolfgang:
I will withdraw this patch.
Feng
________________________________
From: Wolfgang Denk [mailto:wd@denx.de] Sent: Mon 2/22/2010 12:54 PM To: Feng Kan Cc: Stefan Roese; u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc
Dear Feng Kan,
In message 2B3B2AA816369A4E87D7BE63EC9D2F260615A5EC@SDCEXCHANGE01.ad.amcc.com you wrote:
There is not a particular board that is stuck on this 213 format. Rather, for sometime u-boot and linux both had the 213 ordering. Lets say the guy did not have the SMC define turned on, which mean the ECC would caculate correctly for him. Now, he gets a new U-boot and want to update it. He gets to prompt and programs the new u-boot and linux (in 213 ordering). The new uboot comes up (it doesn't complain since there is no error message), runs to linux, the new linux expects 123 ordering. Finds ECC error and tries to correct and crash.
If this is your concern, then a compile-time setting makes little sense - you don't really expect that a user in this situation will build another U-Boot image after selecting other build options, install it (with the risk of bricking his device (keep in mind that not everybody has access to a JTAG debugger), and continue this so long until he finds a configuration that works for his combination of U-Boot and Linux settings.
I submitted this patch to support both ordering that the correction routine contains (123 and 213).
But it makes no sense as a compile time option.
If you want to help users, then this must be implemented in a way that is selectable at run-time, for example by simple setting an
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "Everybody is talking about the weather but nobody does anything about it." - Mark Twain
participants (4)
-
Feng Kan -
fkan＠amcc.com
-
Stefan Roese -
Wolfgang Denk