[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[fkan at amcc.com]

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;
 }
-- 
1.5.5

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Wolfgang Denk]

Dear fkan at 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

-- 
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
Don't panic.

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Feng Kan]

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

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[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

-- 
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
"I say we take off; nuke the site from orbit. It's the only way to be
sure."                                  - Corporal Hicks, in "Aliens"

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Stefan Roese]

Hi Feng,

On Thursday 18 February 2010 23:25:13 fkan at 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 at denx.de

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Feng Kan]

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 at 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 at denx.de

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Stefan Roese]

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 at denx.de

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Feng Kan]

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 at denx.de]
Sent: Mon 2/22/2010 2:52 AM
To: Feng Kan
Cc: u-boot at 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 at denx.de

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Wolfgang Denk]

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 at denx.de
"Everybody is talking about the  weather  but  nobody  does  anything
about it."                                               - Mark Twain

[U-Boot] [PATCH 1/1] ppc4xx: add support for alternate format for ndfc

[Feng Kan]

Dear Wolfgang:
 
I will withdraw this patch.
 
Feng

________________________________

From: Wolfgang Denk [mailto:wd at denx.de]
Sent: Mon 2/22/2010 12:54 PM
To: Feng Kan
Cc: Stefan Roese; u-boot at 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 at denx.de
"Everybody is talking about the  weather  but  nobody  does  anything
about it."                                               - Mark Twain