[PATCHv4 0/1] Set FORCE_CSX bit when arbitration between NAND and NOR is enabled

[PATCHv4 0/1] Set FORCE_CSX bit when arbitration between NAND and NOR is enabled

[Chris Packham]

Note: I've picked this up from Kalyan to try and get things into shape for
submission upstream.

When the arbitration between NOR and NAND flash is enabled
the <FORCE_CSX> field bit[21] in the Data Flash Control Register
needs to be set to 1 according to guidleine GL-5830741
mentioned in Marvell Errata document MV-S501377-00, Rev. D.

Set the FORCE_CSX bit in NDCR for ARMADA370 variants as the arbitration
is always enabled by default. This change does not apply for pxa3xx
variants because FORCE_CSX bit does not exist/reserved on the NFCv1.

Ran the "flash_speed" tool on NAND flash on a board with
Armada-xp based SoC which uses only one NAND chip and not using
the arbiter. There is no regression or speed penalty
introduced due to this change.

Kalyan Kinthada (1):
  mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants

 drivers/mtd/nand/pxa3xx_nand.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.15.1

[PATCHv4 1/1] mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants

[Chris Packham]

From: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>

The Armada-370 based SoCs support arbitration between the NAND Flash
interface and NOR (i.e. devbus) on the same chip select. However there
are two guidelines that must be followed to avoid data corruption in
this scenario.

First GL-6843509 which states that only "Don't Care CS" NAND devices are
supported. Secondly GL-5830741 which says that when the arbitration
between NOR and NAND flash is enabled set the FORCE_CSX bit to 1. This
prevents the chip select from being active when the data flash interface
is relinquished to the other slave.

The NDCR_ND_MODE conflicts with the new NDCR_FORCE_CSX but is unused so
remove it along with NDCR_NAND_MODE.

Signed-off-by: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
[cp: only apply when arbiter is enabled, reword commit message]
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes in v4:
- only act when the nand arbiter is enabled
- update commit message with information from Marvell

Changes in v3:
- "mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants."
  Modified commit message to mention that this change does not apply
  for pxa3xx variants.
  Fixed the missing space in comments.
  Removed unused macros "NDCR_ND_MODE" and "NDCR_NAND_MODE".

Changes in v2:
- Deleted: "dt-bindings: mtd: pxa3xx: Add "marvell,nand-force-csx" compatible string"
  Not necessary to create a new compatible string.
- "mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants."
  Modified commit message.
  This commit sets the FORCE_CSX bit for all ARMADA370 variants.

 drivers/mtd/nand/pxa3xx_nand.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 90b9a9ccbe60..1b2ae98311d2 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -73,8 +73,7 @@
 #define NDCR_DWIDTH_M		(0x1 << 26)
 #define NDCR_PAGE_SZ		(0x1 << 24)
 #define NDCR_NCSX		(0x1 << 23)
-#define NDCR_ND_MODE		(0x3 << 21)
-#define NDCR_NAND_MODE   	(0x0)
+#define NDCR_FORCE_CSX		(0x1 << 21)
 #define NDCR_CLR_PG_CNT		(0x1 << 20)
 #define NFCV1_NDCR_ARB_CNTL	(0x1 << 19)
 #define NFCV2_NDCR_STOP_ON_UNCOR	(0x1 << 19)
@@ -1477,6 +1476,10 @@ static int pxa3xx_nand_config_ident(struct pxa3xx_nand_info *info)
 	info->chunk_size = PAGE_CHUNK_SIZE;
 	info->reg_ndcr = 0x0; /* enable all interrupts */
 	info->reg_ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
+	/* Set FORCE_CSX bit for all ARMADA370 Variants. Ref#: GL-5830741 */
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
+	    pdata->enable_arbiter)
+		info->reg_ndcr |= NDCR_FORCE_CSX;
 	info->reg_ndcr |= NDCR_RD_ID_CNT(READ_ID_BYTES);
 	info->reg_ndcr |= NDCR_SPARE_EN;
 
@@ -1511,6 +1514,10 @@ static void pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	info->reg_ndcr = ndcr &
 		~(NDCR_INT_MASK | NDCR_ND_ARB_EN | NFCV1_NDCR_ARB_CNTL);
 	info->reg_ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
+	/* Set FORCE_CSX bit for all ARMADA370 Variants. Ref#: GL-5830741 */
+	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
+	    pdata->enable_arbiter)
+		info->reg_ndcr |= NDCR_FORCE_CSX;
 	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
 	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
 }
-- 
2.15.1

Re: [PATCHv4 1/1] mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants

[Miquel RAYNAL]

Hello Chris,

On Fri, 22 Dec 2017 12:19:04 +1300
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> From: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
> 
> The Armada-370 based SoCs support arbitration between the NAND Flash
> interface and NOR (i.e. devbus) on the same chip select. However there
> are two guidelines that must be followed to avoid data corruption in
> this scenario.

Sorry to bother you again with that but, do you actually face any
issue/data corruption with this scenario?

This bit is not set by the bootloaders I use and I see no difference
when I set it. But by experience on this controller: any change is
dangerous and may break users, that is why we are so careful about it.

Thank you,
Miquèl

> 
> First GL-6843509 which states that only "Don't Care CS" NAND devices
> are supported. Secondly GL-5830741 which says that when the
> arbitration between NOR and NAND flash is enabled set the FORCE_CSX
> bit to 1. This prevents the chip select from being active when the
> data flash interface is relinquished to the other slave.
> 
> The NDCR_ND_MODE conflicts with the new NDCR_FORCE_CSX but is unused
> so remove it along with NDCR_NAND_MODE.
> 
> Signed-off-by: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
> [cp: only apply when arbiter is enabled, reword commit message]
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Changes in v4:
> - only act when the nand arbiter is enabled
> - update commit message with information from Marvell
> 
> Changes in v3:
> - "mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants."
>   Modified commit message to mention that this change does not apply
>   for pxa3xx variants.
>   Fixed the missing space in comments.
>   Removed unused macros "NDCR_ND_MODE" and "NDCR_NAND_MODE".
> 
> Changes in v2:
> - Deleted: "dt-bindings: mtd: pxa3xx: Add "marvell,nand-force-csx"
> compatible string" Not necessary to create a new compatible string.
> - "mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants."
>   Modified commit message.
>   This commit sets the FORCE_CSX bit for all ARMADA370 variants.
> 
>  drivers/mtd/nand/pxa3xx_nand.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c
> b/drivers/mtd/nand/pxa3xx_nand.c index 90b9a9ccbe60..1b2ae98311d2
> 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -73,8 +73,7 @@
>  #define NDCR_DWIDTH_M		(0x1 << 26)
>  #define NDCR_PAGE_SZ		(0x1 << 24)
>  #define NDCR_NCSX		(0x1 << 23)
> -#define NDCR_ND_MODE		(0x3 << 21)
> -#define NDCR_NAND_MODE   	(0x0)
> +#define NDCR_FORCE_CSX		(0x1 << 21)
>  #define NDCR_CLR_PG_CNT		(0x1 << 20)
>  #define NFCV1_NDCR_ARB_CNTL	(0x1 << 19)
>  #define NFCV2_NDCR_STOP_ON_UNCOR	(0x1 << 19)
> @@ -1477,6 +1476,10 @@ static int pxa3xx_nand_config_ident(struct
> pxa3xx_nand_info *info) info->chunk_size = PAGE_CHUNK_SIZE;
>  	info->reg_ndcr = 0x0; /* enable all interrupts */
>  	info->reg_ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN :
> 0;
> +	/* Set FORCE_CSX bit for all ARMADA370 Variants. Ref#:
> GL-5830741 */
> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
> +	    pdata->enable_arbiter)
> +		info->reg_ndcr |= NDCR_FORCE_CSX;
>  	info->reg_ndcr |= NDCR_RD_ID_CNT(READ_ID_BYTES);
>  	info->reg_ndcr |= NDCR_SPARE_EN;
>  
> @@ -1511,6 +1514,10 @@ static void pxa3xx_nand_detect_config(struct
> pxa3xx_nand_info *info) info->reg_ndcr = ndcr &
>  		~(NDCR_INT_MASK | NDCR_ND_ARB_EN |
> NFCV1_NDCR_ARB_CNTL); info->reg_ndcr |= (pdata->enable_arbiter) ?
> NDCR_ND_ARB_EN : 0;
> +	/* Set FORCE_CSX bit for all ARMADA370 Variants. Ref#:
> GL-5830741 */
> +	if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 &&
> +	    pdata->enable_arbiter)
> +		info->reg_ndcr |= NDCR_FORCE_CSX;
>  	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
>  	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
>  }



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCHv4 1/1] mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants

[Ezequiel Garcia]

On 22 December 2017 at 12:53, Miquel RAYNAL
<miquel.raynal@free-electrons.com> wrote:
> Hello Chris,
>
> On Fri, 22 Dec 2017 12:19:04 +1300
> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
>
>> From: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
>>
>> The Armada-370 based SoCs support arbitration between the NAND Flash
>> interface and NOR (i.e. devbus) on the same chip select. However there
>> are two guidelines that must be followed to avoid data corruption in
>> this scenario.
>
> Sorry to bother you again with that but, do you actually face any
> issue/data corruption with this scenario?
>

Indeed. We need a description of a real world problem this patch is fixing.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCHv4 1/1] mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants

[Chris Packham]

Hi Miquel, Ezequiel,

On 23/12/17 05:56, Ezequiel Garcia wrote:
> On 22 December 2017 at 12:53, Miquel RAYNAL
> <miquel.raynal@free-electrons.com> wrote:
>> Hello Chris,
>>
>> On Fri, 22 Dec 2017 12:19:04 +1300
>> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
>>
>>> From: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
>>>
>>> The Armada-370 based SoCs support arbitration between the NAND Flash
>>> interface and NOR (i.e. devbus) on the same chip select. However there
>>> are two guidelines that must be followed to avoid data corruption in
>>> this scenario.
>>
>> Sorry to bother you again with that but, do you actually face any
>> issue/data corruption with this scenario?
>>
> 
> Indeed. We need a description of a real world problem this patch is fixing.
> 

I totally agree. The Marvell FAE used words like "data corruption" hence 
my re-newed interest in this. I had hoped these patches would pique the 
interest of someone from Marvell's engineering team with some more info 
on how the data corruption exhibits.

I've been running some of the mtd-utils tests on my hardware and haven't 
detected any failures yet.

I think the key would be to be doing interleaved accesses between nand 
and the parallel device. I've just kicked off something I think should 
do this on my hardware but I'm unsure as to how long I should wait for 
an issue to present itself.

I'll leave it running for as long as I can today. If I find a failure 
I'll report back otherwise we can leave this patch for the mailing list 
archives waiting for an issue to be seen.

Re: [PATCHv4 1/1] mtd: nand: pxa3xx: Set FORCE_CSX bit to ARMADA370 variants

[Chris Packham]

On 08/01/18 11:35, Chris Packham wrote:
> Hi Miquel, Ezequiel,
> 
> On 23/12/17 05:56, Ezequiel Garcia wrote:
>> On 22 December 2017 at 12:53, Miquel RAYNAL
>> <miquel.raynal@free-electrons.com> wrote:
>>> Hello Chris,
>>>
>>> On Fri, 22 Dec 2017 12:19:04 +1300
>>> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
>>>
>>>> From: Kalyan Kinthada <kalyan.kinthada@alliedtelesis.co.nz>
>>>>
>>>> The Armada-370 based SoCs support arbitration between the NAND Flash
>>>> interface and NOR (i.e. devbus) on the same chip select. However there
>>>> are two guidelines that must be followed to avoid data corruption in
>>>> this scenario.
>>>
>>> Sorry to bother you again with that but, do you actually face any
>>> issue/data corruption with this scenario?
>>>
>>
>> Indeed. We need a description of a real world problem this patch is fixing.
>>
> 
> I totally agree. The Marvell FAE used words like "data corruption" hence
> my re-newed interest in this. I had hoped these patches would pique the
> interest of someone from Marvell's engineering team with some more info
> on how the data corruption exhibits.
> 
> I've been running some of the mtd-utils tests on my hardware and haven't
> detected any failures yet.
> 
> I think the key would be to be doing interleaved accesses between nand
> and the parallel device. I've just kicked off something I think should
> do this on my hardware but I'm unsure as to how long I should wait for
> an issue to present itself.
> 
> I'll leave it running for as long as I can today. If I find a failure
> I'll report back otherwise we can leave this patch for the mailing list
> archives waiting for an issue to be seen.
> 

I've been running my test for several hours an no obvious problem has 
presented itself so I'm happy to leave it there until such time as a 
problem is observed or Marvell provide a reproduction.