[PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

The NDDB register holds the data that are needed by the read and write
commands.

However, during a read PIO access, the datasheet specifies that after each 32
bits read in that register, when BCH is enabled, we have to make sure that the
RDDREQ bit is set in the NDSR register.

This fixes an issue that was seen on the Armada 385, and presumably other mvebu
SoCs, when a read on a newly erased page would end up in the driver reporting a
timeout from the NAND.

Cc: <stable@vger.kernel.org> # v3.14
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 47 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 96b0b1d27df1..b2d8d6960765 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -480,6 +480,41 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
 	nand_writel(info, NDCR, ndcr | int_mask);
 }
 
+static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len)
+{
+	if (info->ecc_bch) {
+		int index = 0;
+
+		while (index < (len * 4)) {
+			u32 timeout;
+
+			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
+
+			/*
+			 * According to the datasheet, when reading
+			 * from NDDB with BCH enabled, after each 32
+			 * bytes reads, we have to make sure that the
+			 * NDSR.RDDREQ bit is set
+			 */
+			for (timeout = 0;
+			     !(nand_readl(info, NDSR) & NDSR_RDDREQ);
+			     timeout++) {
+				if (timeout >= 5) {
+					dev_err(&info->pdev->dev,
+						"Timeout on RDDREQ while draining the FIFO\n");
+					return;
+				}
+
+				mdelay(1);
+			}
+
+			index += 32;
+		}
+	} else {
+		__raw_readsl(info->mmio_base + NDDB, data, len);
+	}
+}
+
 static void handle_data_pio(struct pxa3xx_nand_info *info)
 {
 	unsigned int do_bytes = min(info->data_size, info->chunk_size);
@@ -496,14 +531,14 @@ static void handle_data_pio(struct pxa3xx_nand_info *info)
 				      DIV_ROUND_UP(info->oob_size, 4));
 		break;
 	case STATE_PIO_READING:
-		__raw_readsl(info->mmio_base + NDDB,
-			     info->data_buff + info->data_buff_pos,
-			     DIV_ROUND_UP(do_bytes, 4));
+		drain_fifo(info,
+			   info->data_buff + info->data_buff_pos,
+			   DIV_ROUND_UP(do_bytes, 4));
 
 		if (info->oob_size > 0)
-			__raw_readsl(info->mmio_base + NDDB,
-				     info->oob_buff + info->oob_buff_pos,
-				     DIV_ROUND_UP(info->oob_size, 4));
+			drain_fifo(info,
+				   info->oob_buff + info->oob_buff_pos,
+				   DIV_ROUND_UP(info->oob_size, 4));
 		break;
 	default:
 		dev_err(&info->pdev->dev, "%s: invalid state %d\n", __func__,
-- 
2.3.0

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Boris Brezillon]

Hi Maxime,

On Mon, 16 Feb 2015 13:51:11 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> The NDDB register holds the data that are needed by the read and write
> commands.
> 
> However, during a read PIO access, the datasheet specifies that after each 32
> bits read in that register, when BCH is enabled, we have to make sure that the
> RDDREQ bit is set in the NDSR register.
> 
> This fixes an issue that was seen on the Armada 385, and presumably other mvebu
> SoCs, when a read on a newly erased page would end up in the driver reporting a
> timeout from the NAND.
> 
> Cc: <stable@vger.kernel.org> # v3.14
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 47 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 96b0b1d27df1..b2d8d6960765 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -480,6 +480,41 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
>  	nand_writel(info, NDCR, ndcr | int_mask);
>  }
>  
> +static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len)
> +{
> +	if (info->ecc_bch) {
> +		int index = 0;
> +
> +		while (index < (len * 4)) {
> +			u32 timeout;
> +
> +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
> +

Shouldn't you break here if you've read all the data you were
expecting ?
As I said in my previous review, I don't know what's happening if you
wait for RDDREQ when the FIFO has been fully drained.

> +			/*
> +			 * According to the datasheet, when reading
> +			 * from NDDB with BCH enabled, after each 32
> +			 * bytes reads, we have to make sure that the
> +			 * NDSR.RDDREQ bit is set
> +			 */
> +			for (timeout = 0;
> +			     !(nand_readl(info, NDSR) & NDSR_RDDREQ);
> +			     timeout++) {
> +				if (timeout >= 5) {
> +					dev_err(&info->pdev->dev,
> +						"Timeout on RDDREQ while draining the FIFO\n");
> +					return;
> +				}
> +
> +				mdelay(1);
> +			}
> +
> +			index += 32;
> +		}
> +	} else {
> +		__raw_readsl(info->mmio_base + NDDB, data, len);
> +	}
> +}

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Ezequiel Garcia]

On 02/16/2015 09:51 AM, Maxime Ripard wrote:
> The NDDB register holds the data that are needed by the read and write
> commands.
> 
> However, during a read PIO access, the datasheet specifies that after each 32
> bits read in that register, when BCH is enabled, we have to make sure that the
> RDDREQ bit is set in the NDSR register.
> 

Typo s/32 bits/32 bytes

-- 
Ezequiel Garc�a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

On Mon, Feb 16, 2015 at 10:35:50AM -0300, Ezequiel Garcia wrote:
> On 02/16/2015 09:51 AM, Maxime Ripard wrote:
> > The NDDB register holds the data that are needed by the read and write
> > commands.
> > 
> > However, during a read PIO access, the datasheet specifies that after each 32
> > bits read in that register, when BCH is enabled, we have to make sure that the
> > RDDREQ bit is set in the NDSR register.
> > 
> 
> Typo s/32 bits/32 bytes

Good catch, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Thomas Petazzoni]

Dear Maxime Ripard,

On Mon, 16 Feb 2015 13:51:11 +0100, Maxime Ripard wrote:

> +		while (index < (len * 4)) {
> +			u32 timeout;
> +
> +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);

Are you guaranteed that 'len' is a multiple of 32 bytes?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On Mon, Feb 16, 2015 at 05:27:53PM +0100, Thomas Petazzoni wrote:
> Dear Maxime Ripard,
> 
> On Mon, 16 Feb 2015 13:51:11 +0100, Maxime Ripard wrote:
> 
> > +		while (index < (len * 4)) {
> > +			u32 timeout;
> > +
> > +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
> 
> Are you guaranteed that 'len' is a multiple of 32 bytes?

I don't know if you're guaranteed of anything, but the controller
supports only 512, 2k, 4k and 8k pages, which are all mutiples of 32
bytes.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Ezequiel Garcia]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/16/2015 01:41 PM, Maxime Ripard wrote:
> On Mon, Feb 16, 2015 at 05:27:53PM +0100, Thomas Petazzoni wrote:
>> Dear Maxime Ripard,
>>
>> On Mon, 16 Feb 2015 13:51:11 +0100, Maxime Ripard wrote:
>>
>>> +		while (index < (len * 4)) {
>>> +			u32 timeout;
>>> +
>>> +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
>>
>> Are you guaranteed that 'len' is a multiple of 32 bytes?
> 
> I don't know if you're guaranteed of anything, but the controller
> supports only 512, 2k, 4k and 8k pages, which are all mutiples of 32
> bytes.
> 

'len' here comes from:

do_bytes = min(info->data_size, info->chunk_size);

and

DIV_ROUND_UP(do_bytes, 4)

Where chunk_size is the size we want to read/write in each command step
(keep in mind that with extended commands we issue multiple commands, and
read/write data in chunks for each page).

And data_size is initialized at mtd->writesize (i.e. the size of a page).

Given all the flash pages I'm aware of are multiples of 32-bytes, and
given a chunk is either a quarter or half a page... I'd say it's
guaranteed to be 32-byte multiple, but perhaps it's a good idea to enforce it.
- -- 
Ezequiel Garc�a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJU4iFfAAoJEIOKbhOEIHKiOuMQAK0yiPyjBKRcqX8qrpG9Ljcq
JVhJTjn7VdiWhoh0n9BOt5bV3K0wAvcbt3LZvpGwf1EOifBaZB+f2QskZNLDUXyC
JXPaonbdUqabEU0n9frduc9xBgbPhrwL4X0RzbJ0xZ+A2FrPt/80qUe8lsDmykH9
dyl3FOL3EQQiQ83D1VefkYbeDjaunvhA7Lfi7CcdPSFRv1FE47NQUW/8OjvZVczx
uPcvdNj4818aXtFyOJQbR9xWOhVh7nxPlU8flHZPHuJ5WVCGWBbt++/4vmK+LZkv
aZQ8W6dGiKI3ayT+PQ7nsETmoXZcjWTihq+nW+Ie2vs5PZf1iME5RYarLSKsc0Ac
4GjLnd4+0H3jeInvJ0MLw0dhkYM4PLkzp4CPo4vrH8z5F3cLXxaRkZYuv7gChden
C2VITr9C8p1OSQJ2mF8m9gWdExkEuuy7q6vURx74C4KaeQA2R4ARAROm85o6JtmN
dhozZIFrJQGwGuB5+7MI3yJj4OpFsBkxoq6U1JNDTwYnu3SnMOdwvq9kwqGXgR2I
yQlu6MO6DYHkMtmw//kkqX+vnyhGexrFoesOyG4d40mOgyGYqyk+oadV7pNh2g5Y
nXGr21Li80N65Sk+RaOFlvmIPaQ45Xn6gS7ckHcVVCZI9HAu87n5n15HEtfaj4Dc
r9FkTUgw9cqcio5EVfEs
=nkDF
-----END PGP SIGNATURE-----

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On Mon, Feb 16, 2015 at 01:57:12PM -0300, Ezequiel Garcia wrote:
> On 02/16/2015 01:41 PM, Maxime Ripard wrote:
> > On Mon, Feb 16, 2015 at 05:27:53PM +0100, Thomas Petazzoni wrote:
> >> Dear Maxime Ripard,
> >>
> >> On Mon, 16 Feb 2015 13:51:11 +0100, Maxime Ripard wrote:
> >>
> >>> +		while (index < (len * 4)) {
> >>> +			u32 timeout;
> >>> +
> >>> +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
> >>
> >> Are you guaranteed that 'len' is a multiple of 32 bytes?
> > 
> > I don't know if you're guaranteed of anything, but the controller
> > supports only 512, 2k, 4k and 8k pages, which are all mutiples of 32
> > bytes.
> > 
> 
> 'len' here comes from:
> 
> do_bytes = min(info->data_size, info->chunk_size);
> 
> and
> 
> DIV_ROUND_UP(do_bytes, 4)
> 
> Where chunk_size is the size we want to read/write in each command step
> (keep in mind that with extended commands we issue multiple commands, and
> read/write data in chunks for each page).
> 
> And data_size is initialized at mtd->writesize (i.e. the size of a page).
> 
> Given all the flash pages I'm aware of are multiples of 32-bytes, and
> given a chunk is either a quarter or half a page... I'd say it's
> guaranteed to be 32-byte multiple, but perhaps it's a good idea to enforce it.

I've fixed the function to both support non-aligned reading, just in
case, and to not poll on the last chunk, as Boris suggested.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Robert Jarzmik]

Maxime Ripard <maxime.ripard@free-electrons.com> writes:

>  drivers/mtd/nand/pxa3xx_nand.c | 47 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 96b0b1d27df1..b2d8d6960765 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -480,6 +480,41 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
>  	nand_writel(info, NDCR, ndcr | int_mask);
>  }
>  
> +static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len)
> +{
> +	if (info->ecc_bch) {
> +		int index = 0;
> +
> +		while (index < (len * 4)) {
> +			u32 timeout;
> +
> +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
> +
> +			/*
> +			 * According to the datasheet, when reading
> +			 * from NDDB with BCH enabled, after each 32
> +			 * bytes reads, we have to make sure that the
> +			 * NDSR.RDDREQ bit is set
> +			 */
> +			for (timeout = 0;
> +			     !(nand_readl(info, NDSR) & NDSR_RDDREQ);
> +			     timeout++) {
> +				if (timeout >= 5) {
> +					dev_err(&info->pdev->dev,
> +						"Timeout on RDDREQ while draining the FIFO\n");
> +					return;
> +				}
> +
> +				mdelay(1);
So in worst case, we'll end up with 4 times mdelay(1) times len / 32.
For a 2048 page, it is : 256ms where everything is stuck (mdelay and not
msleep).

I know you had no choice because this is called from interrupt handler (top
half). But having a irq handler and a irq thread handler would solve that issue,
and you'll end up with msleep(1) in this code.

I don't think an mdelay(256) is acceptable.

Cheers.

--
Robert

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Hi Robert,

On Mon, Feb 16, 2015 at 09:11:24PM +0100, Robert Jarzmik wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> 
> >  drivers/mtd/nand/pxa3xx_nand.c | 47 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index 96b0b1d27df1..b2d8d6960765 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -480,6 +480,41 @@ static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
> >  	nand_writel(info, NDCR, ndcr | int_mask);
> >  }
> >  
> > +static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len)
> > +{
> > +	if (info->ecc_bch) {
> > +		int index = 0;
> > +
> > +		while (index < (len * 4)) {
> > +			u32 timeout;
> > +
> > +			__raw_readsl(info->mmio_base + NDDB, data + index, 8);
> > +
> > +			/*
> > +			 * According to the datasheet, when reading
> > +			 * from NDDB with BCH enabled, after each 32
> > +			 * bytes reads, we have to make sure that the
> > +			 * NDSR.RDDREQ bit is set
> > +			 */
> > +			for (timeout = 0;
> > +			     !(nand_readl(info, NDSR) & NDSR_RDDREQ);
> > +			     timeout++) {
> > +				if (timeout >= 5) {
> > +					dev_err(&info->pdev->dev,
> > +						"Timeout on RDDREQ while draining the FIFO\n");
> > +					return;
> > +				}
> > +
> > +				mdelay(1);
> So in worst case, we'll end up with 4 times mdelay(1) times len / 32.
> For a 2048 page, it is : 256ms where everything is stuck (mdelay and not
> msleep).
> 
> I know you had no choice because this is called from interrupt handler (top
> half). But having a irq handler and a irq thread handler would solve that issue,
> and you'll end up with msleep(1) in this code.
> 
> I don't think an mdelay(256) is acceptable.

That's very true that this driver would need some love, but
valentine's day was last week.

I'm sorry, but this is a patch targeted for stable. This is a pure
bugfix. I won't rewrite the whole driver solely to make the driver
better, especially since that would make such a patch (or more likely
a whole serie) unsuitable for stable.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Robert Jarzmik]

Maxime Ripard <maxime.ripard@free-electrons.com> writes:

>> I don't think an mdelay(256) is acceptable.
>
> That's very true that this driver would need some love, but
> valentine's day was last week.
That doesn't cope with the 256ms mdelay. And a potential big mdelay is not what
I'd call a bug fix, see below.

> I'm sorry, but this is a patch targeted for stable. This is a pure
> bugfix. I won't rewrite the whole driver solely to make the driver
> better, especially since that would make such a patch (or more likely
> a whole serie) unsuitable for stable.

This is the rewrite I was asking for (not tested), consider it against your
"rewrite the whole driver" :

	Modified   drivers/mtd/nand/pxa3xx_nand.c
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index e512902..6e569e9 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -576,11 +576,20 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
 {}
 #endif
 
+static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
+{
+	struct pxa3xx_nand_info *info = data;
+
+	handle_data_pio(info);
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 {
 	struct pxa3xx_nand_info *info = devid;
 	unsigned int status, is_completed = 0, is_ready = 0;
 	unsigned int ready, cmd_done;
+	irqreturn_t ret = IRQ_HANDLED;
 
 	if (info->cs == 0) {
 		ready           = NDSR_FLASH_RDY;
@@ -622,7 +631,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 		} else {
 			info->state = (status & NDSR_RDDREQ) ?
 				      STATE_PIO_READING : STATE_PIO_WRITING;
-			handle_data_pio(info);
+			ret = IRQ_WAKE_THREAD;
 		}
 	}
 	if (status & cmd_done) {
@@ -663,7 +672,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 	if (is_ready)
 		complete(&info->dev_ready);
 NORMAL_IRQ_EXIT:
-	return IRQ_HANDLED;
+	return ret;
 }
 
 static inline int is_buf_blank(uint8_t *buf, size_t len)
@@ -1688,7 +1697,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
 	/* initialize all interrupts to be disabled */
 	disable_int(info, NDSR_MASK);
 
-	ret = request_irq(irq, pxa3xx_nand_irq, 0, pdev->name, info);
+	ret = request_threaded_irq(irq, pxa3xx_nand_irq,
+				   pxa3xx_nand_irq_thread, 0, pdev->name, info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to request IRQ\n");
 		goto fail_free_buf;

--
Robert

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3880 bytes --]

On Mon, Feb 16, 2015 at 10:36:02PM +0100, Robert Jarzmik wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> 
> >> I don't think an mdelay(256) is acceptable.
> >
> > That's very true that this driver would need some love, but
> > valentine's day was last week.
>
> That doesn't cope with the 256ms mdelay. And a potential big mdelay
> is not what I'd call a bug fix, see below.

I really don't care about the delay itself, really.

Just splitting the readsl into several chunks seems to slow the FIFO
draining enough so that we don't even need to poll the RDDREQ bit.

I was asked (rightfully) for a timeout, and since there's no
particular indication in the datasheet on this, I came up with a
totally random and made-up number. If you want to suggest any other
random number, I'd be happy to use it.

Plus, saying that this is strictly equivalent to mdelay(256) is just
bad faith. It is a (very very very unlikely) worst case scenario.
During my tests, I've never experienced even a single delay being
used, let alone every single reads needing to poll the bit for 5ms,
always succeeding at the last attempt.

If we really care about this, we might as well start caring about
other theoretically possible situations, such as the NAND chip being
ripped off the board by an asteroid.

> > I'm sorry, but this is a patch targeted for stable. This is a pure
> > bugfix. I won't rewrite the whole driver solely to make the driver
> > better, especially since that would make such a patch (or more likely
> > a whole serie) unsuitable for stable.
> 
> This is the rewrite I was asking for (not tested), consider it against your
> "rewrite the whole driver" :
> 
> 	Modified   drivers/mtd/nand/pxa3xx_nand.c
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index e512902..6e569e9 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -576,11 +576,20 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
>  {}
>  #endif
>  
> +static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
> +{
> +	struct pxa3xx_nand_info *info = data;
> +
> +	handle_data_pio(info);
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  {
>  	struct pxa3xx_nand_info *info = devid;
>  	unsigned int status, is_completed = 0, is_ready = 0;
>  	unsigned int ready, cmd_done;
> +	irqreturn_t ret = IRQ_HANDLED;
>  
>  	if (info->cs == 0) {
>  		ready           = NDSR_FLASH_RDY;
> @@ -622,7 +631,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  		} else {
>  			info->state = (status & NDSR_RDDREQ) ?
>  				      STATE_PIO_READING : STATE_PIO_WRITING;
> -			handle_data_pio(info);
> +			ret = IRQ_WAKE_THREAD;
>  		}
>  	}
>  	if (status & cmd_done) {
> @@ -663,7 +672,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  	if (is_ready)
>  		complete(&info->dev_ready);
>  NORMAL_IRQ_EXIT:
> -	return IRQ_HANDLED;
> +	return ret;
>  }
>  
>  static inline int is_buf_blank(uint8_t *buf, size_t len)
> @@ -1688,7 +1697,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
>  	/* initialize all interrupts to be disabled */
>  	disable_int(info, NDSR_MASK);
>  
> -	ret = request_irq(irq, pxa3xx_nand_irq, 0, pdev->name, info);
> +	ret = request_threaded_irq(irq, pxa3xx_nand_irq,
> +				   pxa3xx_nand_irq_thread, 0, pdev->name, info);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>  		goto fail_free_buf;

While I agree that this change would be needed, it is still not stable
material, so both patches will have to go through separate ways (and
one will live without the other).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]

On Mon, Feb 16, 2015 at 10:36:02PM +0100, Robert Jarzmik wrote:
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index e512902..6e569e9 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -576,11 +576,20 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
>  {}
>  #endif
>  
> +static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
> +{
> +	struct pxa3xx_nand_info *info = data;
> +
> +	handle_data_pio(info);
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  {
>  	struct pxa3xx_nand_info *info = devid;
>  	unsigned int status, is_completed = 0, is_ready = 0;
>  	unsigned int ready, cmd_done;
> +	irqreturn_t ret = IRQ_HANDLED;
>  
>  	if (info->cs == 0) {
>  		ready           = NDSR_FLASH_RDY;
> @@ -622,7 +631,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  		} else {
>  			info->state = (status & NDSR_RDDREQ) ?
>  				      STATE_PIO_READING : STATE_PIO_WRITING;
> -			handle_data_pio(info);
> +			ret = IRQ_WAKE_THREAD;
>  		}
>  	}
>  	if (status & cmd_done) {
> @@ -663,7 +672,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  	if (is_ready)
>  		complete(&info->dev_ready);
>  NORMAL_IRQ_EXIT:
> -	return IRQ_HANDLED;
> +	return ret;
>  }
>  
>  static inline int is_buf_blank(uint8_t *buf, size_t len)
> @@ -1688,7 +1697,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
>  	/* initialize all interrupts to be disabled */
>  	disable_int(info, NDSR_MASK);
>  
> -	ret = request_irq(irq, pxa3xx_nand_irq, 0, pdev->name, info);
> +	ret = request_threaded_irq(irq, pxa3xx_nand_irq,
> +				   pxa3xx_nand_irq_thread, 0, pdev->name, info);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>  		goto fail_free_buf;

I just gave this patch a try, and it blows up quite badly:
http://code.bulix.org/p96krc-87889?raw

It looks like there's more work here, most likely in the waitqueues
wake up.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Robert Jarzmik]

Maxime Ripard <maxime.ripard@free-electrons.com> writes:

> On Mon, Feb 16, 2015 at 10:36:02PM +0100, Robert Jarzmik wrote:
>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>> index e512902..6e569e9 100644
>> --- a/drivers/mtd/nand/pxa3xx_nand.c
>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> @@ -576,11 +576,20 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
>>  {}
>>  #endif
>>  
>> +static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
>> +{
>> +	struct pxa3xx_nand_info *info = data;
>> +
>> +	handle_data_pio(info);
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>  {
>>  	struct pxa3xx_nand_info *info = devid;
>>  	unsigned int status, is_completed = 0, is_ready = 0;
>>  	unsigned int ready, cmd_done;
>> +	irqreturn_t ret = IRQ_HANDLED;
>>  
>>  	if (info->cs == 0) {
>>  		ready           = NDSR_FLASH_RDY;
>> @@ -622,7 +631,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>  		} else {
>>  			info->state = (status & NDSR_RDDREQ) ?
>>  				      STATE_PIO_READING : STATE_PIO_WRITING;
>> -			handle_data_pio(info);
>> +			ret = IRQ_WAKE_THREAD;
>>  		}
>>  	}
>>  	if (status & cmd_done) {
>> @@ -663,7 +672,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>  	if (is_ready)
>>  		complete(&info->dev_ready);
>>  NORMAL_IRQ_EXIT:
>> -	return IRQ_HANDLED;
>> +	return ret;
>>  }
>>  
>>  static inline int is_buf_blank(uint8_t *buf, size_t len)
>> @@ -1688,7 +1697,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
>>  	/* initialize all interrupts to be disabled */
>>  	disable_int(info, NDSR_MASK);
>>  
>> -	ret = request_irq(irq, pxa3xx_nand_irq, 0, pdev->name, info);
>> +	ret = request_threaded_irq(irq, pxa3xx_nand_irq,
>> +				   pxa3xx_nand_irq_thread, 0, pdev->name, info);
>>  	if (ret < 0) {
>>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>>  		goto fail_free_buf;
>
> I just gave this patch a try, and it blows up quite badly:
> http://code.bulix.org/p96krc-87889?raw
Confirmed.
OTOH, replacing :
>> +			ret = IRQ_WAKE_THREAD;
With:
+			ret = IRQ_WAKE_THREAD;
+			goto NORMAL_IRQ_EXIT;

is fully working in my environment, now I got 10mn to test it.
I also added for cosmetics in pxa3xx_nand_irq_thread() a :
	info->state = STATE_CMD_DONE;

> It looks like there's more work here, most likely in the waitqueues
> wake up.
Not that much if I'm right, hein ?
And it enables the msleep() instead of mdelay().

And the flow of the driver is not changed for pxa3xx, only for Armada (ie. non
dma case) for which you need to introduce a delay anyway, and therefore change
the flow.

It will be Brian choice eventually, but if you say that you will submit that
approach for next cycle, and yours for stable, and that for next you'll convert
mdelay() to msleep(), I'll stop arguing.

Cheers.

-- 
Robert

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Ezequiel Garcia]

On 02/17/2015 02:07 PM, Robert Jarzmik wrote:
> Maxime Ripard <maxime.ripard@free-electrons.com> writes:
> 
>> On Mon, Feb 16, 2015 at 10:36:02PM +0100, Robert Jarzmik wrote:
>>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
>>> index e512902..6e569e9 100644
>>> --- a/drivers/mtd/nand/pxa3xx_nand.c
>>> +++ b/drivers/mtd/nand/pxa3xx_nand.c
>>> @@ -576,11 +576,20 @@ static void start_data_dma(struct pxa3xx_nand_info *info)
>>>  {}
>>>  #endif
>>>  
>>> +static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
>>> +{
>>> +	struct pxa3xx_nand_info *info = data;
>>> +
>>> +	handle_data_pio(info);
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>>  static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>>  {
>>>  	struct pxa3xx_nand_info *info = devid;
>>>  	unsigned int status, is_completed = 0, is_ready = 0;
>>>  	unsigned int ready, cmd_done;
>>> +	irqreturn_t ret = IRQ_HANDLED;
>>>  
>>>  	if (info->cs == 0) {
>>>  		ready           = NDSR_FLASH_RDY;
>>> @@ -622,7 +631,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>>  		} else {
>>>  			info->state = (status & NDSR_RDDREQ) ?
>>>  				      STATE_PIO_READING : STATE_PIO_WRITING;
>>> -			handle_data_pio(info);
>>> +			ret = IRQ_WAKE_THREAD;
>>>  		}
>>>  	}
>>>  	if (status & cmd_done) {
>>> @@ -663,7 +672,7 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>>>  	if (is_ready)
>>>  		complete(&info->dev_ready);
>>>  NORMAL_IRQ_EXIT:
>>> -	return IRQ_HANDLED;
>>> +	return ret;
>>>  }
>>>  
>>>  static inline int is_buf_blank(uint8_t *buf, size_t len)
>>> @@ -1688,7 +1697,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
>>>  	/* initialize all interrupts to be disabled */
>>>  	disable_int(info, NDSR_MASK);
>>>  
>>> -	ret = request_irq(irq, pxa3xx_nand_irq, 0, pdev->name, info);
>>> +	ret = request_threaded_irq(irq, pxa3xx_nand_irq,
>>> +				   pxa3xx_nand_irq_thread, 0, pdev->name, info);
>>>  	if (ret < 0) {
>>>  		dev_err(&pdev->dev, "failed to request IRQ\n");
>>>  		goto fail_free_buf;
>>
>> I just gave this patch a try, and it blows up quite badly:
>> http://code.bulix.org/p96krc-87889?raw
> Confirmed.
> OTOH, replacing :
>>> +			ret = IRQ_WAKE_THREAD;
> With:
> +			ret = IRQ_WAKE_THREAD;
> +			goto NORMAL_IRQ_EXIT;
> 
> is fully working in my environment, now I got 10mn to test it.
> I also added for cosmetics in pxa3xx_nand_irq_thread() a :
> 	info->state = STATE_CMD_DONE;
> 
>> It looks like there's more work here, most likely in the waitqueues
>> wake up.
> Not that much if I'm right, hein ?
> And it enables the msleep() instead of mdelay().
> 
> And the flow of the driver is not changed for pxa3xx, only for Armada (ie. non
> dma case) for which you need to introduce a delay anyway, and therefore change
> the flow.
> 
> It will be Brian choice eventually, but if you say that you will submit that
> approach for next cycle, and yours for stable, and that for next you'll convert
> mdelay() to msleep(), I'll stop arguing.
> 

How about you push a proper patchset with this alternative (and a nice
cover letter explaining the need for a threaded irq) so we can discuss
properly this new turn?

-- 
Ezequiel Garc�a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Brian Norris]

On Tue, Feb 17, 2015 at 02:16:43PM -0300, Ezequiel Garcia wrote:
> On 02/17/2015 02:07 PM, Robert Jarzmik wrote:
> > It will be Brian choice eventually, but if you say that you will submit that
> > approach for next cycle, and yours for stable, and that for next you'll convert
> > mdelay() to msleep(), I'll stop arguing.
> 
> How about you push a proper patchset with this alternative (and a nice
> cover letter explaining the need for a threaded irq) so we can discuss
> properly this new turn?

I think both Maxime's change (polling a new HW bit) and Robert J's
change (move to a threaded IRQ) are good. I'll take another look, but I
expect I'll take Maxime's v4 for the 4.0 cycle, and Robert J's v2 for
4.1 (or will we just jump straight to 5.0? I never know). Will I see a
patch to convert to msleep() and/or a jiffies timeout in the near
future?

Brian

Re: [PATCH v3 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

Hi Brian,

On Mon, Feb 23, 2015 at 07:45:48PM -0800, Brian Norris wrote:
> On Tue, Feb 17, 2015 at 02:16:43PM -0300, Ezequiel Garcia wrote:
> > On 02/17/2015 02:07 PM, Robert Jarzmik wrote:
> > > It will be Brian choice eventually, but if you say that you will submit that
> > > approach for next cycle, and yours for stable, and that for next you'll convert
> > > mdelay() to msleep(), I'll stop arguing.
> > 
> > How about you push a proper patchset with this alternative (and a nice
> > cover letter explaining the need for a threaded irq) so we can discuss
> > properly this new turn?
> 
> I think both Maxime's change (polling a new HW bit) and Robert J's
> change (move to a threaded IRQ) are good. I'll take another look, but I
> expect I'll take Maxime's v4 for the 4.0 cycle, and Robert J's v2 for
> 4.1 (or will we just jump straight to 5.0? I never know).

That's what I would expect too.

> Will I see a patch to convert to msleep() and/or a jiffies timeout
> in the near future?

As soon as both patches are merged.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]