Patchwork [2/2] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

login
register
mail settings
Submitter Ezequiel Garcia
Date Oct. 4, 2013, 6:30 p.m.
Message ID <1380911438-24240-3-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/280696/
State New
Headers show

Comments

Ezequiel Garcia - Oct. 4, 2013, 6:30 p.m.
This commit replaces the currently hardcoded buffer size, by a
dynamic detection scheme. First a small 256 bytes buffer is allocated
so the device can be detected (using READID and friends commands).

After detection, this buffer is released and a new buffer is allocated
to acommodate the page size plus out-of-band size.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 45 ++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)
Brian Norris - Oct. 15, 2013, 7:41 p.m.
On Fri, Oct 04, 2013 at 03:30:38PM -0300, Ezequiel Garcia wrote:
> This commit replaces the currently hardcoded buffer size, by a
> dynamic detection scheme. First a small 256 bytes buffer is allocated
> so the device can be detected (using READID and friends commands).
> 
> After detection, this buffer is released and a new buffer is allocated
> to acommodate the page size plus out-of-band size.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 45 ++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index a47c67f..bfb2b9f 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -39,6 +39,13 @@
>  #define NAND_STOP_DELAY		(2 * HZ/50)
>  #define PAGE_CHUNK_SIZE		(2048)
>  
> +/*
> + * Define a buffer size for the initial command that detects the flash device:
> + * STATUS, READID and PARAM. The largest of these is the PARAM command,
> + * needing 256 bytes.
> + */
> +#define INIT_BUFFER_SIZE	256
> +

Actually, PARAM tries to get 768 bytes in nand_base.c to retrieve the
redundant copies, but your current driver implementation seems to ignore
the 2nd and 3rd parameter pages by returning 0xFF.

So this is not a criticism of the current patch, but you may want to
consider improving your NAND_CMD_PARAM support (also, you don't support
NAND_CMD_RNDOUT, which is now used for extended parameter page support).

>  /* registers and bit definitions */
>  #define NDCR		(0x00) /* Control register */
>  #define NDTR0CS0	(0x04) /* Timing Parameter 0 for CS0 */

[snip]

Otherwise, this patch looks good.

Brian
Ezequiel Garcia - Oct. 16, 2013, 10:25 a.m.
On Tue, Oct 15, 2013 at 12:41:59PM -0700, Brian Norris wrote:
> On Fri, Oct 04, 2013 at 03:30:38PM -0300, Ezequiel Garcia wrote:
> > This commit replaces the currently hardcoded buffer size, by a
> > dynamic detection scheme. First a small 256 bytes buffer is allocated
> > so the device can be detected (using READID and friends commands).
> > 
> > After detection, this buffer is released and a new buffer is allocated
> > to acommodate the page size plus out-of-band size.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  drivers/mtd/nand/pxa3xx_nand.c | 45 ++++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > index a47c67f..bfb2b9f 100644
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -39,6 +39,13 @@
> >  #define NAND_STOP_DELAY		(2 * HZ/50)
> >  #define PAGE_CHUNK_SIZE		(2048)
> >  
> > +/*
> > + * Define a buffer size for the initial command that detects the flash device:
> > + * STATUS, READID and PARAM. The largest of these is the PARAM command,
> > + * needing 256 bytes.
> > + */
> > +#define INIT_BUFFER_SIZE	256
> > +
> 
> Actually, PARAM tries to get 768 bytes in nand_base.c to retrieve the
> redundant copies, but your current driver implementation seems to ignore
> the 2nd and 3rd parameter pages by returning 0xFF.
> 
> So this is not a criticism of the current patch, but you may want to
> consider improving your NAND_CMD_PARAM support (also, you don't support
> NAND_CMD_RNDOUT, which is now used for extended parameter page support).
> 

Ah, good input. I'll research a bit more and try to cook a patch, it doesn't
sound like a lot of work, and it would be nice to have better command support.

Thanks,

Patch

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index a47c67f..bfb2b9f 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -39,6 +39,13 @@ 
 #define NAND_STOP_DELAY		(2 * HZ/50)
 #define PAGE_CHUNK_SIZE		(2048)
 
+/*
+ * Define a buffer size for the initial command that detects the flash device:
+ * STATUS, READID and PARAM. The largest of these is the PARAM command,
+ * needing 256 bytes.
+ */
+#define INIT_BUFFER_SIZE	256
+
 /* registers and bit definitions */
 #define NDCR		(0x00) /* Control register */
 #define NDTR0CS0	(0x04) /* Timing Parameter 0 for CS0 */
@@ -164,6 +171,7 @@  struct pxa3xx_nand_info {
 
 	unsigned int 		buf_start;
 	unsigned int		buf_count;
+	unsigned int		buf_size;
 
 	/* DMA information */
 	int			drcmr_dat;
@@ -911,26 +919,20 @@  static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	return 0;
 }
 
-/* the maximum possible buffer size for large page with OOB data
- * is: 2048 + 64 = 2112 bytes, allocate a page here for both the
- * data buffer and the DMA descriptor
- */
-#define MAX_BUFF_SIZE	PAGE_SIZE
-
 #ifdef ARCH_HAS_DMA
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
 	struct platform_device *pdev = info->pdev;
-	int data_desc_offset = MAX_BUFF_SIZE - sizeof(struct pxa_dma_desc);
+	int data_desc_offset = info->buf_size - sizeof(struct pxa_dma_desc);
 
 	if (use_dma == 0) {
-		info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+		info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
 		if (info->data_buff == NULL)
 			return -ENOMEM;
 		return 0;
 	}
 
-	info->data_buff = dma_alloc_coherent(&pdev->dev, MAX_BUFF_SIZE,
+	info->data_buff = dma_alloc_coherent(&pdev->dev, info->buf_size,
 				&info->data_buff_phys, GFP_KERNEL);
 	if (info->data_buff == NULL) {
 		dev_err(&pdev->dev, "failed to allocate dma buffer\n");
@@ -944,7 +946,7 @@  static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 				pxa3xx_nand_data_dma_irq, info);
 	if (info->data_dma_ch < 0) {
 		dev_err(&pdev->dev, "failed to request data dma\n");
-		dma_free_coherent(&pdev->dev, MAX_BUFF_SIZE,
+		dma_free_coherent(&pdev->dev, info->buf_size,
 				info->data_buff, info->data_buff_phys);
 		return info->data_dma_ch;
 	}
@@ -962,7 +964,7 @@  static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 	struct platform_device *pdev = info->pdev;
 	if (use_dma) {
 		pxa_free_dma(info->data_dma_ch);
-		dma_free_coherent(&pdev->dev, MAX_BUFF_SIZE,
+		dma_free_coherent(&pdev->dev, info->buf_size,
 				  info->data_buff, info->data_buff_phys);
 	} else {
 		kfree(info->data_buff);
@@ -971,7 +973,7 @@  static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 #else
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
-	info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+	info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
 	if (info->data_buff == NULL)
 		return -ENOMEM;
 	return 0;
@@ -1085,7 +1087,16 @@  KEEP_CONFIG:
 	else
 		host->col_addr_cycles = 1;
 
+	/* release the initial buffer */
+	kfree(info->data_buff);
+
+	/* allocate the real data + oob buffer */
+	info->buf_size = mtd->writesize + mtd->oobsize;
+	ret = pxa3xx_nand_init_buff(info);
+	if (ret)
+		return ret;
 	info->oob_buff = info->data_buff + mtd->writesize;
+
 	if ((mtd->size >> chip->page_shift) > 65536)
 		host->row_addr_cycles = 3;
 	else
@@ -1191,9 +1202,13 @@  static int alloc_nand_resource(struct platform_device *pdev)
 	}
 	info->mmio_phys = r->start;
 
-	ret = pxa3xx_nand_init_buff(info);
-	if (ret)
+	/* Allocate a buffer to allow flash detection */
+	info->buf_size = INIT_BUFFER_SIZE;
+	info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
+	if (info->data_buff == NULL) {
+		ret = -ENOMEM;
 		goto fail_disable_clk;
+	}
 
 	/* initialize all interrupts to be disabled */
 	disable_int(info, NDSR_MASK);
@@ -1211,7 +1226,7 @@  static int alloc_nand_resource(struct platform_device *pdev)
 
 fail_free_buf:
 	free_irq(irq, info);
-	pxa3xx_nand_free_buff(info);
+	kfree(info->data_buff);
 fail_disable_clk:
 	clk_disable_unprepare(info->clk);
 	return ret;