* Re: [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining
2015-02-18 10:32 ` [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining Maxime Ripard
@ 2015-02-18 12:52 ` Boris Brezillon
2015-02-18 13:40 ` Ezequiel Garcia
2015-02-28 9:01 ` Brian Norris
2 siblings, 0 replies; 11+ messages in thread
From: Boris Brezillon @ 2015-02-18 12:52 UTC (permalink / raw)
To: Maxime Ripard
Cc: Lior Amsalem, Andrew Lunn, Jason Cooper, Tawfik Bayouk,
Thomas Petazzoni, Seif Mazareeb, linux-kernel, stable,
Sudhakar Gundubogula, Nadav Haklai, Boris Brezillon, linux-mtd,
Ezequiel Garcia, Gregory Clement, Brian Norris, linux-arm-kernel,
Sebastian Hesselbarth
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__,
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining
2015-02-18 10:32 ` [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining Maxime Ripard
2015-02-18 12:52 ` Boris Brezillon
@ 2015-02-18 13:40 ` Ezequiel Garcia
2015-02-18 14:01 ` Maxime Ripard
2015-02-28 9:01 ` Brian Norris
2 siblings, 1 reply; 11+ messages in thread
From: Ezequiel Garcia @ 2015-02-18 13:40 UTC (permalink / raw)
To: Maxime Ripard, Gregory Clement, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Brian Norris
Cc: Lior Amsalem, Tawfik Bayouk, Thomas Petazzoni, Seif Mazareeb,
linux-kernel, stable, Sudhakar Gundubogula, Nadav Haklai,
Boris Brezillon, linux-mtd, linux-arm-kernel
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?
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining
2015-02-18 13:40 ` Ezequiel Garcia
@ 2015-02-18 14:01 ` Maxime Ripard
2015-02-18 14:06 ` Ezequiel Garcia
0 siblings, 1 reply; 11+ messages in thread
From: Maxime Ripard @ 2015-02-18 14:01 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Lior Amsalem, Andrew Lunn, Jason Cooper, Tawfik Bayouk,
Thomas Petazzoni, Seif Mazareeb, linux-kernel, stable,
Sudhakar Gundubogula, Nadav Haklai, Boris Brezillon, linux-mtd,
Gregory Clement, Brian Norris, linux-arm-kernel,
Sebastian Hesselbarth
[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]
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
--
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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining
2015-02-18 14:01 ` Maxime Ripard
@ 2015-02-18 14:06 ` Ezequiel Garcia
0 siblings, 0 replies; 11+ messages in thread
From: Ezequiel Garcia @ 2015-02-18 14:06 UTC (permalink / raw)
To: Maxime Ripard
Cc: Lior Amsalem, Andrew Lunn, Jason Cooper, Tawfik Bayouk,
Thomas Petazzoni, Seif Mazareeb, linux-kernel, stable,
Sudhakar Gundubogula, Nadav Haklai, Boris Brezillon, linux-mtd,
Gregory Clement, Brian Norris, linux-arm-kernel,
Sebastian Hesselbarth
-----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-----
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining
2015-02-18 10:32 ` [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining Maxime Ripard
2015-02-18 12:52 ` Boris Brezillon
2015-02-18 13:40 ` Ezequiel Garcia
@ 2015-02-28 9:01 ` Brian Norris
2015-03-02 16:52 ` Gregory CLEMENT
2 siblings, 1 reply; 11+ messages in thread
From: Brian Norris @ 2015-02-28 9:01 UTC (permalink / raw)
To: Maxime Ripard
Cc: Lior Amsalem, Andrew Lunn, Jason Cooper, Tawfik Bayouk,
Thomas Petazzoni, Seif Mazareeb, linux-kernel, stable,
Sudhakar Gundubogula, Nadav Haklai, Boris Brezillon, linux-mtd,
Ezequiel Garcia, Gregory Clement, linux-arm-kernel,
Sebastian Hesselbarth
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] mtd: nand: pxa3xx: Fix PIO FIFO draining
2015-02-28 9:01 ` Brian Norris
@ 2015-03-02 16:52 ` Gregory CLEMENT
0 siblings, 0 replies; 11+ messages in thread
From: Gregory CLEMENT @ 2015-03-02 16:52 UTC (permalink / raw)
To: Brian Norris, Maxime Ripard
Cc: Lior Amsalem, Andrew Lunn, Jason Cooper, Tawfik Bayouk,
Thomas Petazzoni, Seif Mazareeb, linux-kernel, stable,
Sudhakar Gundubogula, Nadav Haklai, Boris Brezillon, linux-mtd,
Ezequiel Garcia, linux-arm-kernel, Sebastian Hesselbarth
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
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 11+ messages in thread