diff mbox

[03/12] mtd: nand: omap: Move IRQ handling from GPMC to NAND driver

Message ID 55BB4C1D.5080905@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Quadros July 31, 2015, 10:21 a.m. UTC 
On 29/07/15 15:06, Roger Quadros wrote:
> Tony,
> 
> On 13/07/15 15:40, Tony Lindgren wrote:
>> * Roger Quadros <rogerq@ti.com> [150713 03:07]:
>>> Tony,
>>>
>>> On 13/07/15 10:10, Tony Lindgren wrote:
>>>> * Roger Quadros <rogerq@ti.com> [150710 05:26]:
>>>>> Since the Interrupt Events are used only by the NAND driver,
>>>>> there is no point in managing the Interrupt registers
>>>>> in the GPMC driver and complicating it with irqchip modeling.
>>>>
>>>> I don't think it's a good idea to allow external drivers to
>>>> tinker directly with GPMC registers. How about just set up GPMC
>>>> as an irqchip for the edge detection interrupts?
>>>>
>>>> I think we already have devices with multiple NAND chips. And
>>>> there's nothing stopping other drivers from using the edge
>>>> detection interrupts.
>>>
>>> OK. The GPMC_IRQ registers manage 2 NAND specific interrupts
>>> (terminalcount and fifo) and 'n' WAIT pin edge interrupts.
>>>
>>>  So we can model this as a irqchip with 'n + 2' interrupts.
>>
>> OK
> 
> For the wait pins irqchip is not sufficient and it needs to be gpiochip
> with irqchip. Waitpin status can be read from GPIO_STATUS register.
> 
> Just getting the interrupt is not enough and we want to know if the
> line is high or low. That is how nand->dev_ready works.
> 
> How about having 2 IRQ domains?
> One is irqchip with 2 interrupts (terminalcount and fifo) and second is
> gpiochip + irqchip for the n wait pins.
> 
> The nand driver can then be modified to use GPIO to get Read/Busy
> pin status from the wait pin.

One more observation I've had is that using irqchip modelling for
the 2 NAND events causes a performance impact.

Using mtd_oobtest I see the following on dra7-evm

1) v4.2-rc4 with prefetch-polled (no IRQs used)
mtd_speedtest: eraseblock write speed is 7142 KiB/s
mtd_speedtest: eraseblock read speed is 13721 KiB/s

2) v4.2-rc4 with prefetch-irq (IRQchip model)
eraseblock write speed is 5475 KiB/s
eraseblock read speed is 6420 KiB/s

3) this series (*) with prefetch-irq (no IRQchip model, nand driver
directly accesses irqstatus/irqenable)
eraseblock write speed is 6564 KiB/s
eraseblock read speed is 10850 KiB/s

(*) diff at the end is required on top to fix an issue with this series.

So should we continue IRQchip modelling for the NAND events
or use the GPMC interrupt as shared and add APIs to access
the NAND bits of the IRQSTATUS/ENABLE register.

cheers,
-roger

--


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tony Lindgren Aug. 3, 2015, 1:08 p.m. UTC | #1 
* Roger Quadros <rogerq@ti.com> [150731 03:24]:
> 
> One more observation I've had is that using irqchip modelling for
> the 2 NAND events causes a performance impact.
> 
> Using mtd_oobtest I see the following on dra7-evm
> 
> 1) v4.2-rc4 with prefetch-polled (no IRQs used)
> mtd_speedtest: eraseblock write speed is 7142 KiB/s
> mtd_speedtest: eraseblock read speed is 13721 KiB/s
> 
> 2) v4.2-rc4 with prefetch-irq (IRQchip model)
> eraseblock write speed is 5475 KiB/s
> eraseblock read speed is 6420 KiB/s
> 
> 3) this series (*) with prefetch-irq (no IRQchip model, nand driver
> directly accesses irqstatus/irqenable)
> eraseblock write speed is 6564 KiB/s
> eraseblock read speed is 10850 KiB/s
> 
> (*) diff at the end is required on top to fix an issue with this series.
> 
> So should we continue IRQchip modelling for the NAND events
> or use the GPMC interrupt as shared and add APIs to access
> the NAND bits of the IRQSTATUS/ENABLE register.

In the long run chained IRQ would be the most flexible solution
for sure.

It might be worth checking why we have so much overhead with
the irqchip modelling compared to shared IRQ. If the overhead
is really all the extra IRQ handling then it seems the shared
interrupt plus an API to access IRQSTATUS/ENABLE is the way
to go. Up to you.

Regards,

Tony

 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index fecc054..26ef2bd 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1832,13 +1832,14 @@ static int omap_get_dt_info(struct device *dev, struct omap_nand_info *info)
>  		for (i = 0; i < ARRAY_SIZE(nand_xfer_types); i++) {
>  			if (!strcasecmp(s, nand_xfer_types[i])) {
>  				info->xfer_type = i;
> -				break;
> +				goto next;
>  			}
>  		}
>  
>  		dev_err(dev, "unrecognized value for ti,nand-xfer-type\n");
>  		return -EINVAL;
>  	}
> +next:
>  
>  	of_get_nand_on_flash_bbt(child);
>  
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 26ef2bd..e8bdff5 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -670,17 +670,12 @@ static irqreturn_t omap_nand_irq(int this_irq, void *dev)
>  			goto done;
>  	}
>  
> -	/* Clear FIFOEVENT STATUS */
> -	irqstatus &= ~GPMC_IRQ_FIFOEVENT;
> -	writel(irqstatus, info->reg.gpmc_irqstatus);
> -
>  	return IRQ_HANDLED;
>  
>  done:
>  	complete(&info->comp);
>  
> -	/* Clear FIFOEVENT and TERMCOUNT STATUS */
> -	irqstatus &= ~(GPMC_IRQ_TERMCOUNT | GPMC_IRQ_FIFOEVENT);
> +	/* Clear IRQ STATUS */
>  	writel(irqstatus, info->reg.gpmc_irqstatus);
>  
>  	/* Disable Interrupt generation */
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index fecc054..26ef2bd 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1832,13 +1832,14 @@  static int omap_get_dt_info(struct device *dev, struct omap_nand_info *info)
 		for (i = 0; i < ARRAY_SIZE(nand_xfer_types); i++) {
 			if (!strcasecmp(s, nand_xfer_types[i])) {
 				info->xfer_type = i;
-				break;
+				goto next;
 			}
 		}
 
 		dev_err(dev, "unrecognized value for ti,nand-xfer-type\n");
 		return -EINVAL;
 	}
+next:
 
 	of_get_nand_on_flash_bbt(child);
 
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 26ef2bd..e8bdff5 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -670,17 +670,12 @@  static irqreturn_t omap_nand_irq(int this_irq, void *dev)
 			goto done;
 	}
 
-	/* Clear FIFOEVENT STATUS */
-	irqstatus &= ~GPMC_IRQ_FIFOEVENT;
-	writel(irqstatus, info->reg.gpmc_irqstatus);
-
 	return IRQ_HANDLED;
 
 done:
 	complete(&info->comp);
 
-	/* Clear FIFOEVENT and TERMCOUNT STATUS */
-	irqstatus &= ~(GPMC_IRQ_TERMCOUNT | GPMC_IRQ_FIFOEVENT);
+	/* Clear IRQ STATUS */
 	writel(irqstatus, info->reg.gpmc_irqstatus);
 
 	/* Disable Interrupt generation */