On Tue, Oct 21, 2008 at 08:04:00PM +0200, Holger Freyther wrote:
> On Tuesday 21 October 2008 11:22:31 Harald Welte wrote:
>
> > I'm really lost here, don't know what else to do. I'll get some profiles
> > on a soft-ECC and on a non-irq-based-NAND kernel to compare the results and
> > see if they also show this 'artefact'. Maybe 'top' is actually wrong? Any
> > ideas?
>
> @oprofile: You could catch Richard on irc, IIRC he did work on oprofile for
> ARM and XScale and might know if some thing are not accounted because of
> missing hooks.
I'll catch up with him once I get back to this topic, right now I'm afraid I'll
have to work on other stuff for some time.
Right now I just discovered that for some reason some part of the profile is
accounted to 'vma 00000000' instead of the vmlinux vma. But if I resolve the
symbol adresses manually in 'objdump -d' output, it makes a lot of sense.
Still, it sounds like more than 50% CPU is burnt in the actual PIO read from
the controller, and some 27% are in default_idle.
Comparing interrupt based and busy-wait based profiles shows almost zero
difference. busy-waiting in nand_wait_ready() is about 0.238 percent (!),
s3c2410_nand_devready() accounts for only 0.0069%
I believe the profiles in as far as there is little difference between
interrupt and busy-wait based NAND read.
What I still don't get is
1) why is there so much default_idle in the profiles but top says ~ 100%
2) why is there so much time in default_idle rather than s3c24xx_default_idle
3) why is the CPU idle that much time, given that the NAND chip supposedly is
much faster
> @nand read:
> - If we can not read faster, we can read fewer? The untested ideas are
> attached. On S3C2442 there is a 2nd ECC hardware register bank, by setting
> the ecc.size higher we might kill half of your read call's (if we can
> actually read 512 byte at a time...) and the second ECC register is actually
> doing something.
I don't think it changes much. As indicated before, the NAND flash caches an
entire page + oob (2048+64 = 2112) bytes internally, so you don't pay the
penalty of fetching the data twice from the actual NAND cells.
Also, they way how I understand the current code, it actually executes one
READ0 command and then just calls read_buf four times (256bytes each) plus
once for the 64bytes ECC.
So this actually should never read any data multiple times. It just fetches
different parts of the 2112 bytes page that was read into different buffers.
> maybe this sounds stupid or you already have tested these...
I haven't tested it,
> nice that you work on this
As indicated, it's unlikely I'll get back to this during the remaining week.
If anyone is interested in my interrupt patches, I'll attach them to this mail.
They're for testing only, not for merging them.
--
- Harald Welte <[EMAIL PROTECTED]> http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone
>From 0235e866985aa187c4d03ce6202ddb55f01763bf Mon Sep 17 00:00:00 2001
From: Harald Welte <[EMAIL PROTECTED]>
Date: Mon, 20 Oct 2008 20:56:39 +0200
Subject: [PATCH] [MTD] NAND: Add support for driver-specific wait_ready function
This enables a driver to implement RnB-interrupt based waiting
for the RnB line to become ready, as opposed to the busy-waiting
polling implementation in the core NAND layer.
This is particularly important on battery-powered devices where
you try to save CPU cycles where you can.
Signed-off-by: Harald Welte <[EMAIL PROTECTED]>
---
drivers/mtd/nand/nand_base.c | 10 ++++++----
include/linux/mtd/nand.h | 2 ++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e29c1da..81996b3 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -533,7 +533,7 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
* any case on any machine. */
ndelay(100);
- nand_wait_ready(mtd);
+ chip->wait_ready(mtd);
}
/**
@@ -654,7 +654,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
* any case on any machine. */
ndelay(100);
- nand_wait_ready(mtd);
+ chip->wait_ready(mtd);
}
/**
@@ -1033,7 +1033,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
if (!chip->dev_ready)
udelay(chip->chip_delay);
else
- nand_wait_ready(mtd);
+ chip->wait_ready(mtd);
}
} else {
memcpy(buf, chip->buffers->databuf + col, bytes);
@@ -1318,7 +1318,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
if (!chip->dev_ready)
udelay(chip->chip_delay);
else
- nand_wait_ready(mtd);
+ chip->wait_ready(mtd);
}
readlen -= len;
@@ -2211,6 +2211,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
chip->verify_buf = busw ? nand_verify_buf16 : nand_verify_buf;
if (!chip->scan_bbt)
chip->scan_bbt = nand_default_bbt;
+ if (!chip->wait_ready)
+ chip->wait_ready = nand_wait_ready;
if (!chip->controller) {
chip->controller = &chip->hwcontrol;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c42bc7f..ac0fd71 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -320,6 +320,7 @@ struct nand_buffers {
* @dev_ready: [BOARDSPECIFIC] hardwarespecific function for accesing device ready/busy line
* If set to NULL no access to ready/busy is available and the ready/busy information
* is read from the chip status register
+ * @wait_ready: [REPLACEABLE] hardwarespecific function for waiting read/busy line
* @cmdfunc: [REPLACEABLE] hardwarespecific function for writing commands to the chip
* @waitfunc: [REPLACEABLE] hardwarespecific function for wait on ready
* @ecc: [BOARDSPECIFIC] ecc control ctructure
@@ -377,6 +378,7 @@ struct nand_chip {
void (*cmd_ctrl)(struct mtd_info *mtd, int dat,
unsigned int ctrl);
int (*dev_ready)(struct mtd_info *mtd);
+ void (*wait_ready)(struct mtd_info *mtd);
void (*cmdfunc)(struct mtd_info *mtd, unsigned command, int column, int page_addr);
int (*waitfunc)(struct mtd_info *mtd, struct nand_chip *this);
void (*erase_cmd)(struct mtd_info *mtd, int page);
--
1.5.6.5
>From 403d3ed21020736554732d916a67499f3721c17a Mon Sep 17 00:00:00 2001
From: Harald Welte <[EMAIL PROTECTED]>
Date: Tue, 21 Oct 2008 22:49:07 +0200
Subject: [PATCH] [MTD] S3C24XX NAND: Use interrupt based RnB
With this patch, you can define USE_IRQ and switch the driver
from busy-waiting for RnB to an interrupt + completion based
method.
Signed-off-by: Harald Welte <[EMAIL PROTECTED]>
---
drivers/mtd/nand/s3c2410.c | 73 +++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 72 insertions(+), 1 deletions(-)
diff --git a/drivers/mtd/nand/s3c2410.c b/drivers/mtd/nand/s3c2410.c
index eeb48ed..cb4c00e 100644
--- a/drivers/mtd/nand/s3c2410.c
+++ b/drivers/mtd/nand/s3c2410.c
@@ -52,6 +52,8 @@
#include <linux/err.h>
#include <linux/slab.h>
#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -119,6 +121,7 @@ struct s3c2410_nand_info {
void __iomem *sel_reg;
int sel_bit;
int mtd_count;
+ struct completion rnb_completion;
enum s3c_cpu_type cpu_type;
};
@@ -225,9 +228,14 @@ static int s3c2410_nand_inithw(struct s3c2410_nand_info *info,
cfg |= S3C2440_NFCONF_TWRPH0(twrph0 - 1);
cfg |= S3C2440_NFCONF_TWRPH1(twrph1 - 1);
+#ifdef USE_IRQ
+ /* enable the controller, enable RnB IRQ and de-assert nFCE */
+ writel(S3C2440_NFCONT_ENABLE | S3C2440_NFCONT_RNBINT_EN,
+ info->regs + S3C2440_NFCONT);
+#else
/* enable the controller and de-assert nFCE */
-
writel(S3C2440_NFCONT_ENABLE, info->regs + S3C2440_NFCONT);
+#endif
}
dev_dbg(info->device, "NF_CONF is 0x%lx\n", cfg);
@@ -505,12 +513,56 @@ static void s3c2440_nand_write_buf(struct mtd_info *mtd, const u_char *buf, int
writesl(info->regs + S3C2440_NFDATA, buf, len / 4);
}
+#ifdef USE_IRQ
+static irqreturn_t s3c2440_nand_irq(int irq, void *dev_id)
+{
+ struct s3c2410_nand_info *info = dev_id;
+ u_int8_t stat = readb(info->regs + S3C2440_NFSTAT);
+
+ if (stat & S3C2440_NFSTAT_RnB_CHANGE) {
+ //dev_err(info->device, "RnB IRQ\n");
+ /* clear the RnB change status */
+ writeb(stat | S3C2440_NFSTAT_RnB_CHANGE,
+ info->regs + S3C2440_NFSTAT);
+ /* FIXME: should we check RnB status ? */
+ complete(&info->rnb_completion);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void s3c2440_nand_wait_ready(struct mtd_info *mtd)
+{
+ struct s3c2410_nand_info *info = s3c2410_nand_mtd_toinfo(mtd);
+
+ /* FIXME: do we have a race condition? How to solve it? */
+ if (s3c2440_nand_devready(mtd)) {
+ dev_err(info->device, "immediately ready\n");
+ return;
+ }
+
+ //dev_err(info->device, "waiting for completion\n");
+ wait_for_completion(&info->rnb_completion);
+ //dev_err(info->device, "completed\n");
+}
+#endif /* USE_IRQ */
+
/* device management functions */
static int s3c2410_nand_remove(struct platform_device *pdev)
{
struct s3c2410_nand_info *info = to_nand_info(pdev);
+ switch (info->cpu_type) {
+ case TYPE_S3C2440:
+#ifdef USE_IRQ
+ free_irq(IRQ_NFCON, s3c2440_nand_irq);
+#endif /* USE_IRQ */
+ break;
+ default:
+ break;
+ }
+
platform_set_drvdata(pdev, NULL);
if (info == NULL)
@@ -633,6 +685,9 @@ static void s3c2410_nand_init_chip(struct s3c2410_nand_info *info,
info->sel_bit = S3C2440_NFCONT_nFCE;
chip->cmd_ctrl = s3c2440_nand_hwcontrol;
chip->dev_ready = s3c2440_nand_devready;
+#ifdef USE_IRQ
+ chip->wait_ready= s3c2440_nand_wait_ready;
+#endif
chip->read_buf = s3c2440_nand_read_buf;
chip->write_buf = s3c2440_nand_write_buf;
break;
@@ -777,6 +832,7 @@ static int s3c24xx_nand_probe(struct platform_device *pdev,
info->platform = plat;
info->regs = ioremap(res->start, size);
info->cpu_type = cpu_type;
+ init_completion(&info->rnb_completion);
if (info->regs == NULL) {
dev_err(&pdev->dev, "cannot reserve register region\n");
@@ -786,6 +842,21 @@ static int s3c24xx_nand_probe(struct platform_device *pdev,
dev_dbg(&pdev->dev, "mapped registers at %p\n", info->regs);
+ switch (cpu_type) {
+ case TYPE_S3C2440:
+#ifdef USE_IRQ
+ if (request_irq(IRQ_NFCON, s3c2440_nand_irq, 0,
+ "s3c2440_nand", info)) {
+ dev_err(&pdev->dev, "cannot request interrupt");
+ err = -EIO;
+ goto exit_error;
+ }
+#endif
+ break;
+ default:
+ break;
+ }
+
/* initialise the hardware */
err = s3c2410_nand_inithw(info, pdev);
--
1.5.6.5
