| Message ID | 1424255528-1717-2-git-send-email-maxime.ripard@free-electrons.com |
|---|---|
| State | Accepted |
| Commit | 8dad0386b97c4bd6edd56752ca7f2e735fe5beb4 |
| Headers | show |
On Wed, 18 Feb 2015 11:32:07 +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 > bytes 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> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/mtd/nand/pxa3xx_nand.c | 48 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index 96b0b1d27df1..bc677362bc73 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -480,6 +480,42 @@ 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 timeout; > + > + /* > + * 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. > + * > + * Drain the FIFO 8 32 bits reads at a time, and skip > + * the polling on the last read. > + */ > + while (len > 8) { > + __raw_readsl(info->mmio_base + NDDB, data, 8); > + > + 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); > + } > + > + data += 32; > + len -= 8; > + } > + } > + > + __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 +532,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__,
On 02/18/2015 07:32 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 > bytes 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 | 48 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index 96b0b1d27df1..bc677362bc73 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -480,6 +480,42 @@ 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 timeout; > + > + /* > + * 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. > + * > + * Drain the FIFO 8 32 bits reads at a time, and skip > + * the polling on the last read. > + */ > + while (len > 8) { > + __raw_readsl(info->mmio_base + NDDB, data, 8); > + > + 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); This is probably a stupid nit.. but here it goes is it any difference if udelay is used here? Does this makes anything better/worse?
On Wed, Feb 18, 2015 at 10:40:02AM -0300, Ezequiel Garcia wrote: > On 02/18/2015 07:32 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 > > bytes 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 | 48 ++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > > index 96b0b1d27df1..bc677362bc73 100644 > > --- a/drivers/mtd/nand/pxa3xx_nand.c > > +++ b/drivers/mtd/nand/pxa3xx_nand.c > > @@ -480,6 +480,42 @@ 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 timeout; > > + > > + /* > > + * 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. > > + * > > + * Drain the FIFO 8 32 bits reads at a time, and skip > > + * the polling on the last read. > > + */ > > + while (len > 8) { > > + __raw_readsl(info->mmio_base + NDDB, data, 8); > > + > > + 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); > > This is probably a stupid nit.. but here it goes is it any > difference if udelay is used here? > > Does this makes anything better/worse? It doesn't make any difference. On the board I've been using, we never hit the delay. So I really don't care about the number of retries and the sleep behind them. I made these numbers up, feel free to come up with others if it makes you more comfortable, but could we settle this? Thanks, Maxime
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 02/18/2015 11:01 AM, Maxime Ripard wrote: > On Wed, Feb 18, 2015 at 10:40:02AM -0300, Ezequiel Garcia wrote: >> On 02/18/2015 07:32 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 bytes 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 | 48 >>> ++++++++++++++++++++++++++++++++++++------ 1 file changed, 42 >>> insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/pxa3xx_nand.c >>> b/drivers/mtd/nand/pxa3xx_nand.c index >>> 96b0b1d27df1..bc677362bc73 100644 --- >>> a/drivers/mtd/nand/pxa3xx_nand.c +++ >>> b/drivers/mtd/nand/pxa3xx_nand.c @@ -480,6 +480,42 @@ 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 timeout; + + >>> /* + * 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. + * + * Drain >>> the FIFO 8 32 bits reads at a time, and skip + * the polling >>> on the last read. + */ + while (len > 8) { + >>> __raw_readsl(info->mmio_base + NDDB, data, 8); + + 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); >> >> This is probably a stupid nit.. but here it goes is it any >> difference if udelay is used here? >> >> Does this makes anything better/worse? > > It doesn't make any difference. On the board I've been using, we > never hit the delay. > > So I really don't care about the number of retries and the sleep > behind them. I made these numbers up, feel free to come up with > others if it makes you more comfortable, but could we settle this? > OK, let's stop the bikeshedding. For both patches: Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> - -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJU5JxaAAoJEIOKbhOEIHKi9HYP/jxU75zI6QjNk1yGL7Y3qwEy M2UYrepXTwgRVRxAIN3nhGqERuBVOJCIVKlb3CUSiq9auNAO8rsRs6JTAossV781 LTUniAA0nIBFbTn/k2Wc6yGQizSy7iJRXu73MrLJrcSFO8JxFFqu04V1EYuZbh5s fuVpeLJEX8Gfy6gj85ybFs7+wkD/XnENKlnDzD6y/n4ewBC3yDPLNh436hZpEVDO ky8lYjGPHMQs0yEDcp1rfFejLAmxJ4SY6hVSKAxq3/Bn58S4y3Pgkm4cJtP8nHYe UN4q9UfOBIHWrQIVbBlViK//n3zyEtaPojDSKUiSvI2+Hmz9+eortlYEvpN1HCP3 Xqy1gzFto9FpP4Wp3KpyF609JNVUeQxAsUQZMXM6yaH2oz35szZhBq2xlIpsyo3C BDbjaYKFk3hVg+jAE0iitZW8BiZS+WZ/pzX4A8rwQBSTMcbrLTuRV611R4E/nEBL sfCwDWc1gxDDiM8pMJBGC97gwtHEibJqxES9y+T3LrhxtqPl1kUczHFDPgd3uoVw fT71PsZBtGUeOu3ysymNPc+mF9b9G9KRHYhSiOjnEIle9yvXh6kaGWv93ZM3RCUG JASv01+gqb+Bz5ZU3MMU+xjHxeoWBq7KfSWcshEHpD8QCuiZzNZ3yt0dZaYXao+M YsLlm5s62fDVILb2Q3+d =MA8V -----END PGP SIGNATURE-----
On Wed, Feb 18, 2015 at 11:32:07AM +0100, 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 > bytes 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> Pushed this one to linux-mtd.git. I'll try to get it out in the 4.0 cycle. I assume patch 2 (the DT addition) will go through arm-soc. Brian
On 28/02/2015 10:01, Brian Norris wrote: > On Wed, Feb 18, 2015 at 11:32:07AM +0100, 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 >> bytes 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> > > Pushed this one to linux-mtd.git. I'll try to get it out in the 4.0 > cycle. I assume patch 2 (the DT addition) will go through arm-soc. Yes, now that you took the driver part, I will apply it on mvebu and then push it to arm-soc. Thanks, Gregory > > Brian >
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 96b0b1d27df1..bc677362bc73 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -480,6 +480,42 @@ 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 timeout; + + /* + * 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. + * + * Drain the FIFO 8 32 bits reads at a time, and skip + * the polling on the last read. + */ + while (len > 8) { + __raw_readsl(info->mmio_base + NDDB, data, 8); + + 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); + } + + data += 32; + len -= 8; + } + } + + __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 +532,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 bytes 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 | 48 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 6 deletions(-)