Hi,
This patch serie enable the NAND support on the Armada 385 Access
Point DB.
In the process, some timeouts were found when we were accessing a
freshly erased NAND page, which turned out to be an issue when
draining the read FIFO where we were not following the datasheet.
This has been fixed with the first patch, with stable CC'd. The second
patch just enables the NAND controller in the DT.
Thanks,
Maxime
Changes from v3:
- Fixed a typo in the commit log
- Reworked the FIFO draining function to not poll the RDDREQ
register on the last 32 bytes chunk, and handle non 32 bytes
aligned reads
Changes from v2:
- Read the status bits only every 32 bytes read, and not 32 bits
like was done before.
- Changed the timeout routine code not use the jiffies that won't
change in an interrupt context.
Changes from v1:
- Added a timeout to the busy waiting loop for RDDREQ
Maxime Ripard (2):
mtd: nand: pxa3xx: Fix PIO FIFO draining
ARM: mvebu: a385-db-ap: Enable the NAND
arch/arm/boot/dts/armada-385-db-ap.dts | 13 +++++++++
drivers/mtd/nand/pxa3xx_nand.c | 48 +++++++++++++++++++++++++++++-----
2 files changed, 55 insertions(+), 6 deletions(-)
--
2.3.0
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: <[email protected]> # v3.14
Signed-off-by: Maxime Ripard <[email protected]>
---
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__,
--
2.3.0
The Armada 385 Access Point Development Board has a 1GB NAND SLC chip from
Micron as its main storage. Enable it.
Signed-off-by: Maxime Ripard <[email protected]>
---
arch/arm/boot/dts/armada-385-db-ap.dts | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/armada-385-db-ap.dts b/arch/arm/boot/dts/armada-385-db-ap.dts
index b891b4c897f5..ee648fb19075 100644
--- a/arch/arm/boot/dts/armada-385-db-ap.dts
+++ b/arch/arm/boot/dts/armada-385-db-ap.dts
@@ -130,6 +130,19 @@
phy-mode = "rgmii-id";
};
+ nfc: flash@d0000 {
+ status = "okay";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ num-cs = <1>;
+ nand-ecc-strength = <4>;
+ nand-ecc-step-size = <512>;
+ marvell,nand-keep-config;
+ marvell,nand-enable-arbiter;
+ nand-on-flash-bbt;
+ };
+
usb3@f0000 {
status = "okay";
usb-phy = <&usb3_phy>;
--
2.3.0
On Wed, 18 Feb 2015 11:32:07 +0100
Maxime Ripard <[email protected]> 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: <[email protected]> # v3.14
> Signed-off-by: Maxime Ripard <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> 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
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: <[email protected]> # v3.14
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> 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
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: <[email protected]> # v3.14
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > 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
-----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: <[email protected]> # v3.14 Signed-off-by: Maxime
>>> Ripard <[email protected]> ---
>>> 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 <[email protected]>
- --
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: <[email protected]> # v3.14
> Signed-off-by: Maxime Ripard <[email protected]>
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