[PATCH] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[PATCH] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[Robert Jarzmik]

Change the handling of the data stage in the driver : don't pump data in
the top-half interrupt, but rather schedule a thread for non dma cases.

This will enable latencies in the data pumping, especially if delays are
required. Moreover platform shall be more reactive as other interrupts
can be served while pumping data.

No throughput degradation was observed, at least on the zylonite
platform, while a slight degradation was being expected.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/nand/pxa3xx_nand.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 96b0b1d..237c92c 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -569,11 +569,25 @@ 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);
+
+	info->state = STATE_CMD_DONE;
+	nand_writel(info, NDSR, NDSR_WRDREQ | NDSR_RDDREQ);
+	enable_int(info, NDCR_INT_MASK);
+
+	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;
@@ -613,9 +627,11 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 			start_data_dma(info);
 			goto NORMAL_IRQ_EXIT;
 		} else {
+			disable_int(info, NDCR_INT_MASK);
 			info->state = (status & NDSR_RDDREQ) ?
 				      STATE_PIO_READING : STATE_PIO_WRITING;
-			handle_data_pio(info);
+			ret = IRQ_WAKE_THREAD;
+			goto NORMAL_IRQ_EXIT;
 		}
 	}
 	if (status & cmd_done) {
@@ -656,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)
@@ -1672,7 +1688,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;
-- 
2.1.0

Re: [PATCH] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[Maxime Ripard]

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

On Tue, Feb 17, 2015 at 09:06:57PM +0100, Robert Jarzmik wrote:
> Change the handling of the data stage in the driver : don't pump data in
> the top-half interrupt, but rather schedule a thread for non dma cases.
> 
> This will enable latencies in the data pumping, especially if delays are
> required. Moreover platform shall be more reactive as other interrupts
> can be served while pumping data.
> 
> No throughput degradation was observed, at least on the zylonite
> platform, while a slight degradation was being expected.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

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

On a sidenote...

> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 96b0b1d..237c92c 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -569,11 +569,25 @@ 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);
> +
> +	info->state = STATE_CMD_DONE;
> +	nand_writel(info, NDSR, NDSR_WRDREQ | NDSR_RDDREQ);
> +	enable_int(info, NDCR_INT_MASK);
> +
> +	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;
> @@ -613,9 +627,11 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  			start_data_dma(info);
>  			goto NORMAL_IRQ_EXIT;
>  		} else {
> +			disable_int(info, NDCR_INT_MASK);
>  			info->state = (status & NDSR_RDDREQ) ?
>  				      STATE_PIO_READING : STATE_PIO_WRITING;
> -			handle_data_pio(info);
> +			ret = IRQ_WAKE_THREAD;
> +			goto NORMAL_IRQ_EXIT;
>  		}
>  	}
>  	if (status & cmd_done) {
> @@ -656,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)
> @@ -1672,7 +1688,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);

Using IRQF_ONESHOT would allow you not to do the interrupt enable /
disable dance.

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] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[Robert Jarzmik]

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

> On Tue, Feb 17, 2015 at 09:06:57PM +0100, Robert Jarzmik wrote:
>> Change the handling of the data stage in the driver : don't pump data in
>> the top-half interrupt, but rather schedule a thread for non dma cases.
>> 
>> This will enable latencies in the data pumping, especially if delays are
>> required. Moreover platform shall be more reactive as other interrupts
>> can be served while pumping data.
>> 
>> No throughput degradation was observed, at least on the zylonite
>> platform, while a slight degradation was being expected.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> On a sidenote...
Thanks.

>> @@ -1672,7 +1688,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);
>
> Using IRQF_ONESHOT would allow you not to do the interrupt enable /
> disable dance.
Yes, that's a very good point. Would your Tested-by still hold with this change ?

Cheers.

-- 
Robert

Re: [PATCH] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[Maxime Ripard]

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

On Wed, Feb 18, 2015 at 06:16:17PM +0100, Robert Jarzmik wrote:
> >> @@ -1672,7 +1688,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);
> >
> > Using IRQF_ONESHOT would allow you not to do the interrupt enable /
> > disable dance.
>
> Yes, that's a very good point. Would your Tested-by still hold with
> this change ?

Yep, I actually tested it before suggesting it.

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] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[Ezequiel Garcia]

On 02/17/2015 05:06 PM, Robert Jarzmik wrote:
> Change the handling of the data stage in the driver : don't pump data in
> the top-half interrupt, but rather schedule a thread for non dma cases.
> 
> This will enable latencies in the data pumping, especially if delays are
> required. Moreover platform shall be more reactive as other interrupts
> can be served while pumping data.
> 
> No throughput degradation was observed, at least on the zylonite
> platform, while a slight degradation was being expected.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 96b0b1d..237c92c 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -569,11 +569,25 @@ 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);
> +
> +	info->state = STATE_CMD_DONE;

Are you sure you need to set the state here?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] mtd: pxa3xx-nand: handle PIO in threaded interrupt

[Robert Jarzmik]

Ezequiel Garcia <ezequiel.garcia@free-electrons.com> writes:

>> +static irqreturn_t pxa3xx_nand_irq_thread(int irq, void *data)
>> +{
>> +	struct pxa3xx_nand_info *info = data;
>> +
>> +	handle_data_pio(info);
>> +
>> +	info->state = STATE_CMD_DONE;
>
> Are you sure you need to set the state here?
Euh no, I'm not. I made it to be symmetric with dma_complete_func(). If it's not
needed, why is it needed in dma_complete_func() ?

Cheers.

-- 
Robert