| Message ID | 1424091072-7738-2-git-send-email-maxime.ripard@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
-----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-----
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
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
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
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__,
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(-)