[PATCH] mtd: nand: pxa3xx: Fix READOOB implementation

[PATCH] mtd: nand: pxa3xx: Fix READOOB implementation

[Boris Brezillon]

In the current driver, OOB bytes are accessed in raw mode, and when a
page access is done with NDCR_SPARE_EN set and NDCR_ECC_EN cleared, the
driver must read the whole spare area (64 bytes in case of a 2k page,
16 bytes for a 512 page). The driver was only reading the free OOB
bytes, which was leaving some unread data in the FIFO and was somehow
leading to a timeout.

We could patch the driver to read ->spare_size + ->ecc_size instead of
just ->spare_size when READOOB is requested, but we'd better make
in-band and OOB accesses consistent.
Since the driver is always accessing in-band data in non-raw mode (with
the ECC engine enabled), we should also access OOB data in this mode.
That's particularly useful when using the BCH engine because in this
mode the free OOB bytes are also ECC protected.

Fixes: 43bcfd2bb24a ("mtd: nand: pxa3xx: Add driver-specific ECC BCH support")
Cc: <stable@kernel.vger.org>
Reported-by: Sean Nyekjær <sean.nyekjaer@prevas.dk>
Tested-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 021374fe59dc..d1979c7dbe7e 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -961,6 +961,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 
 	switch (command) {
 	case NAND_CMD_READ0:
+	case NAND_CMD_READOOB:
 	case NAND_CMD_PAGEPROG:
 		info->use_ecc = 1;
 		break;
-- 
2.11.0

Re: [PATCH] mtd: nand: pxa3xx: Fix READOOB implementation

[Ezequiel Garcia]

On 18 December 2017 at 07:32, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> In the current driver, OOB bytes are accessed in raw mode, and when a
> page access is done with NDCR_SPARE_EN set and NDCR_ECC_EN cleared, the
> driver must read the whole spare area (64 bytes in case of a 2k page,
> 16 bytes for a 512 page). The driver was only reading the free OOB
> bytes, which was leaving some unread data in the FIFO and was somehow
> leading to a timeout.
>
> We could patch the driver to read ->spare_size + ->ecc_size instead of
> just ->spare_size when READOOB is requested, but we'd better make
> in-band and OOB accesses consistent.
> Since the driver is always accessing in-band data in non-raw mode (with
> the ECC engine enabled), we should also access OOB data in this mode.
> That's particularly useful when using the BCH engine because in this
> mode the free OOB bytes are also ECC protected.
>
> Fixes: 43bcfd2bb24a ("mtd: nand: pxa3xx: Add driver-specific ECC BCH support")
> Cc: <stable@kernel.vger.org>
> Reported-by: Sean Nyekjær <sean.nyekjaer@prevas.dk>
> Tested-by: Willy Tarreau <w@1wt.eu>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 021374fe59dc..d1979c7dbe7e 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -961,6 +961,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
>
>         switch (command) {
>         case NAND_CMD_READ0:
> +       case NAND_CMD_READOOB:
>         case NAND_CMD_PAGEPROG:
>                 info->use_ecc = 1;
>                 break;
> --
> 2.11.0
>

Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks a lot Boris!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] mtd: nand: pxa3xx: Fix READOOB implementation

[Sean Nyekjær]

On 2017-12-18 13:33, Ezequiel Garcia wrote:
> On 18 December 2017 at 07:32, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>> In the current driver, OOB bytes are accessed in raw mode, and when a
>> page access is done with NDCR_SPARE_EN set and NDCR_ECC_EN cleared, the
>> driver must read the whole spare area (64 bytes in case of a 2k page,
>> 16 bytes for a 512 page). The driver was only reading the free OOB
>> bytes, which was leaving some unread data in the FIFO and was somehow
>> leading to a timeout.
>>
>> We could patch the driver to read ->spare_size + ->ecc_size instead of
>> just ->spare_size when READOOB is requested, but we'd better make
>> in-band and OOB accesses consistent.
>> Since the driver is always accessing in-band data in non-raw mode (with
>> the ECC engine enabled), we should also access OOB data in this mode.
>> That's particularly useful when using the BCH engine because in this
>> mode the free OOB bytes are also ECC protected.
>>
>> Fixes: 43bcfd2bb24a ("mtd: nand: pxa3xx: Add driver-specific ECC BCH support")
>> Cc: <stable@kernel.vger.org>
>> Reported-by: Sean Nyekjær <sean.nyekjaer@prevas.dk>
>> Tested-by: Willy Tarreau <w@1wt.eu>
>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> ---
>>   drivers/mtd/nand/pxa3xx_nand.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> index 021374fe59dc..d1979c7dbe7e 100644
>> --- a/drivers/mtd/nand/pxa3xx_nand.c
>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> @@ -961,6 +961,7 @@ static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
>>
>>          switch (command) {
>>          case NAND_CMD_READ0:
>> +       case NAND_CMD_READOOB:
>>          case NAND_CMD_PAGEPROG:
>>                  info->use_ecc = 1;
>>                  break;
>> --
>> 2.11.0
>>
> Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Thanks a lot Boris!

Tested-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>

Thanks

Re: [PATCH] mtd: nand: pxa3xx: Fix READOOB implementation

[Robert Jarzmik]

Boris Brezillon <boris.brezillon@free-electrons.com> (by way of Boris Brezillon
<boris.brezillon@free-electrons.com>) writes:

> In the current driver, OOB bytes are accessed in raw mode, and when a
> page access is done with NDCR_SPARE_EN set and NDCR_ECC_EN cleared, the
> driver must read the whole spare area (64 bytes in case of a 2k page,
> 16 bytes for a 512 page). The driver was only reading the free OOB
> bytes, which was leaving some unread data in the FIFO and was somehow
> leading to a timeout.

That's a very good catch.
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert