[v3,05/15] mtd: nand: pxa3xx: Support command buffer #3

Some newer controllers support a fourth command buffer. This additional
command buffer allows to set an arbitrary length count, using the
NDCB3.NDLENCNT field, to perform non-standard length operations
such as the ONFI parameter page read.

In controllers without this register, the operation has no effect.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)

On Sat, Aug 10, 2013 at 04:34:55PM -0300, Ezequiel Garcia wrote:
> Some newer controllers support a fourth command buffer. This additional
> command buffer allows to set an arbitrary length count, using the
> NDCB3.NDLENCNT field, to perform non-standard length operations
> such as the ONFI parameter page read.
> 
> In controllers without this register, the operation has no effect.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Tested-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 3a3e042..83a7187 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -197,6 +197,7 @@ struct pxa3xx_nand_info {
>  	uint32_t		ndcb0;
>  	uint32_t		ndcb1;
>  	uint32_t		ndcb2;
> +	uint32_t		ndcb3;
>  };
>  
>  static bool use_dma = 1;
> @@ -496,6 +497,10 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  		nand_writel(info, NDCB0, info->ndcb0);
>  		nand_writel(info, NDCB0, info->ndcb1);
>  		nand_writel(info, NDCB0, info->ndcb2);
> +
> +		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
> +		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> +			nand_writel(info, NDCB0, info->ndcb3);

Pardon my unfamiliarity with your hardware, but why do you have the
following three register definitions if you're only using NDCB0?

#define NDCB0           (0x48) /* Command Buffer0 */
#define NDCB1           (0x4C) /* Command Buffer1 */
#define NDCB2           (0x50) /* Command Buffer2 */

>  	}
>  
>  	/* clear NDSR to let the controller exit the IRQ */
> @@ -554,6 +559,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>  	default:
>  		info->ndcb1 = 0;
>  		info->ndcb2 = 0;
> +		info->ndcb3 = 0;
>  		break;
>  	}
>  

Brian

Dear Ezequiel Garcia,

On Sat, 10 Aug 2013 16:34:55 -0300, Ezequiel Garcia wrote:
> Some newer controllers support a fourth command buffer. This additional
> command buffer allows to set an arbitrary length count, using the
> NDCB3.NDLENCNT field, to perform non-standard length operations
> such as the ONFI parameter page read.
> 
> In controllers without this register, the operation has no effect.

Are you sure this is true? I thought you had this statement in earlier
revisions of your patch set, but one of the comment was precisely that
this patch was breaking platforms that did not have this register, and
this lead you to introduce the separate compatible string.

I must admit, I'm also a bit confused by the existing code:

 		nand_writel(info, NDCB0, info->ndcb0);
 		nand_writel(info, NDCB0, info->ndcb1);
 		nand_writel(info, NDCB0, info->ndcb2);

but it's probably because I don't know much about NAND and the
registers of this controller.

Thomas

On Sat, Aug 10, 2013 at 11:48:14PM +0200, Thomas Petazzoni wrote:
> On Sat, 10 Aug 2013 16:34:55 -0300, Ezequiel Garcia wrote:
> > Some newer controllers support a fourth command buffer. This additional
> > command buffer allows to set an arbitrary length count, using the
> > NDCB3.NDLENCNT field, to perform non-standard length operations
> > such as the ONFI parameter page read.
> > 
> > In controllers without this register, the operation has no effect.
> 
> Are you sure this is true? I thought you had this statement in earlier
> revisions of your patch set, but one of the comment was precisely that
> this patch was breaking platforms that did not have this register, and
> this lead you to introduce the separate compatible string.

It appears as if he didn't change the commit message properly. He does
now protect the fourth command buffer (?) with this:

+		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
+		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+			nand_writel(info, NDCB0, info->ndcb3);

> I must admit, I'm also a bit confused by the existing code:
> 
>  		nand_writel(info, NDCB0, info->ndcb0);
>  		nand_writel(info, NDCB0, info->ndcb1);
>  		nand_writel(info, NDCB0, info->ndcb2);
> 
> but it's probably because I don't know much about NAND and the
> registers of this controller.

Brian

On Sat, Aug 10, 2013 at 07:33:18PM -0700, Brian Norris wrote:
> On Sat, Aug 10, 2013 at 11:48:14PM +0200, Thomas Petazzoni wrote:
> > On Sat, 10 Aug 2013 16:34:55 -0300, Ezequiel Garcia wrote:
> > > Some newer controllers support a fourth command buffer. This additional
> > > command buffer allows to set an arbitrary length count, using the
> > > NDCB3.NDLENCNT field, to perform non-standard length operations
> > > such as the ONFI parameter page read.
> > > 
> > > In controllers without this register, the operation has no effect.
> > 
> > Are you sure this is true? I thought you had this statement in earlier
> > revisions of your patch set, but one of the comment was precisely that
> > this patch was breaking platforms that did not have this register, and
> > this lead you to introduce the separate compatible string.
> 
> It appears as if he didn't change the commit message properly. He does
> now protect the fourth command buffer (?) with this:
> 
> +		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
> +		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> +			nand_writel(info, NDCB0, info->ndcb3);
> 

Indeed, I just forgot to update the commit message.

I'll prepare a new version to fix this.

> > I must admit, I'm also a bit confused by the existing code:
> > 
> >  		nand_writel(info, NDCB0, info->ndcb0);
> >  		nand_writel(info, NDCB0, info->ndcb1);
> >  		nand_writel(info, NDCB0, info->ndcb2);
> > 

In fact, I got confused as well when I first saw it.

However this behavior is specified in the Armada 370 (I can't find it in the public
PXA specs), which says that software must write either 12 or 16 bytes directly to
NDCB0, four bytes at a time to load the commands in NDCB0, NDCB1, NDCB2, (and optionally
NDCB3).

Writes to NDCB1, NDCB2 and NDCB3 are ignored but each NDCBx register can be read.

I'll add this information as a comment to the above confusing sequence
in the next soon to come version.