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

2018-01-07 Thread 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
>>  wrote:
>>> Hello Chris,
>>>
>>> On Fri, 22 Dec 2017 12:19:04 +1300
>>> Chris Packham  wrote:
>>>
 From: Kalyan Kinthada 

 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.

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

2018-01-07 Thread 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
>>  wrote:
>>> Hello Chris,
>>>
>>> On Fri, 22 Dec 2017 12:19:04 +1300
>>> Chris Packham  wrote:
>>>
 From: Kalyan Kinthada 

 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.

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

2018-01-07 Thread Chris Packham 
Hi Miquel, Ezequiel,

On 23/12/17 05:56, Ezequiel Garcia wrote:
> On 22 December 2017 at 12:53, Miquel RAYNAL
>  wrote:
>> Hello Chris,
>>
>> On Fri, 22 Dec 2017 12:19:04 +1300
>> Chris Packham  wrote:
>>
>>> From: Kalyan Kinthada 
>>>
>>> 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

2018-01-07 Thread Chris Packham 
Hi Miquel, Ezequiel,

On 23/12/17 05:56, Ezequiel Garcia wrote:
> On 22 December 2017 at 12:53, Miquel RAYNAL
>  wrote:
>> Hello Chris,
>>
>> On Fri, 22 Dec 2017 12:19:04 +1300
>> Chris Packham  wrote:
>>
>>> From: Kalyan Kinthada 
>>>
>>> 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

2017-12-22 Thread Ezequiel Garcia 
On 22 December 2017 at 12:53, Miquel RAYNAL
 wrote:
> Hello Chris,
>
> On Fri, 22 Dec 2017 12:19:04 +1300
> Chris Packham  wrote:
>
>> From: Kalyan Kinthada 
>>
>> 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

2017-12-22 Thread Ezequiel Garcia 
On 22 December 2017 at 12:53, Miquel RAYNAL
 wrote:
> Hello Chris,
>
> On Fri, 22 Dec 2017 12:19:04 +1300
> Chris Packham  wrote:
>
>> From: Kalyan Kinthada 
>>
>> 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

2017-12-22 Thread Miquel RAYNAL 
Hello Chris,

On Fri, 22 Dec 2017 12:19:04 +1300
Chris Packham  wrote:

> From: Kalyan Kinthada 
> 
> 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 
> [cp: only apply when arbiter is enabled, reword commit message]
> Signed-off-by: Chris Packham 
> ---
> 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

2017-12-22 Thread Miquel RAYNAL 
Hello Chris,

On Fri, 22 Dec 2017 12:19:04 +1300
Chris Packham  wrote:

> From: Kalyan Kinthada 
> 
> 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 
> [cp: only apply when arbiter is enabled, reword commit message]
> Signed-off-by: Chris Packham 
> ---
> 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