[RFC/PATCH v2] pxa3xx-nand: Remove custom device detection

[RFC/PATCH v2] pxa3xx-nand: Remove custom device detection

[Ezequiel Garcia]

The current pxa3xx-nand driver implements its own device detection, duplicating
a funcionality that the NAND base code provides, through nand_scan_ident() plus
nand_scan_tail().

This is not only ugly, but also wrong, for it implements a deprecated (non-ONFI)
device probing method and force the driver to keep its own list of known devices.
In addition, it settles a very bad example for future NAND drivers.

My suggestion is to simply kill the device detection and instead rely fully
on the findings of nand_scan_ident(). After the first nand_scan_iden() the driver
can configure the controller according to the detected device.

However, I'm concerned about the impact of this patch on already deployed systems,
and by the fact that we're removing the built-in "timing parameter" table.

I've noticed that the PXA maintainers have remain largely silent on the recent
patches for this pxa3xx-nand driver, but I would definitely appreciate their
point of view this time.

Although the patch removes the "keep_config" check, I'm not proposing
to discard the keep_config platform data parameter, but quite the opposite:
using "keep_config" by-passes the device detection procedure, so the effect
of this patch is to effectively set "keep_config", relying on bootloader's
settings.

Future patches will add support to re-configure the controller's timing and
other parameters. However, that's a different discussion from the current,
where I'm proposing to discard the ugly and deprecated custom device detection.

This v2 version is just a rebase, to make the patch apply on latest l2-mtd.

Thanks!

Ezequiel Garcia (1):
  mtd: nand: pxa3xx: Remove redundant device probing

 drivers/mtd/nand/pxa3xx_nand.c | 138 ++---------------------------------------
 1 file changed, 4 insertions(+), 134 deletions(-)

-- 
1.8.1.5

[RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing

[Ezequiel Garcia]

There's no need to go through this internal probe/auto-detect
procedure, since the nand core code will take care of that.
This commit removes the configuration and detection functions,
together with the built-in flash device table.

Besides being unneeded, it's also wrong to take care of such details
wich rightfully belong to the NAND base code. Removing this wrong
code, prevents the proliferation of the same mistake in future drivers.

This commit has the effect of forcing the "keep_config" option.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 138 ++---------------------------------------
 1 file changed, 4 insertions(+), 134 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index dd03dfd..bfcc588 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -212,28 +212,6 @@ static bool use_dma = 1;
 module_param(use_dma, bool, 0444);
 MODULE_PARM_DESC(use_dma, "enable DMA for data transferring to/from NAND HW");
 
-static struct pxa3xx_nand_timing timing[] = {
-	{ 40, 80, 60, 100, 80, 100, 90000, 400, 40, },
-	{ 10,  0, 20,  40, 30,  40, 11123, 110, 10, },
-	{ 10, 25, 15,  25, 15,  30, 25000,  60, 10, },
-	{ 10, 35, 15,  25, 15,  25, 25000,  60, 10, },
-};
-
-static struct pxa3xx_nand_flash builtin_flash_types[] = {
-{ "DEFAULT FLASH",      0,   0, 2048,  8,  8,    0, &timing[0] },
-{ "64MiB 16-bit",  0x46ec,  32,  512, 16, 16, 4096, &timing[1] },
-{ "256MiB 8-bit",  0xdaec,  64, 2048,  8,  8, 2048, &timing[1] },
-{ "4GiB 8-bit",    0xd7ec, 128, 4096,  8,  8, 8192, &timing[1] },
-{ "128MiB 8-bit",  0xa12c,  64, 2048,  8,  8, 1024, &timing[2] },
-{ "128MiB 16-bit", 0xb12c,  64, 2048, 16, 16, 1024, &timing[2] },
-{ "512MiB 8-bit",  0xdc2c,  64, 2048,  8,  8, 4096, &timing[2] },
-{ "512MiB 16-bit", 0xcc2c,  64, 2048, 16, 16, 4096, &timing[2] },
-{ "256MiB 16-bit", 0xba20,  64, 2048, 16, 16, 2048, &timing[3] },
-};
-
-/* Define a default flash type setting serve as flash detecting only */
-#define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
-
 #define NDTR0_tCH(c)	(min((c), 7) << 19)
 #define NDTR0_tCS(c)	(min((c), 7) << 16)
 #define NDTR0_tWH(c)	(min((c), 7) << 11)
@@ -843,53 +821,7 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
 	return 0;
 }
 
-static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
-				    const struct pxa3xx_nand_flash *f)
-{
-	struct platform_device *pdev = info->pdev;
-	struct pxa3xx_nand_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct pxa3xx_nand_host *host = info->host[info->cs];
-	uint32_t ndcr = 0x0; /* enable all interrupts */
-
-	if (f->page_size != 2048 && f->page_size != 512) {
-		dev_err(&pdev->dev, "Current only support 2048 and 512 size\n");
-		return -EINVAL;
-	}
-
-	if (f->flash_width != 16 && f->flash_width != 8) {
-		dev_err(&pdev->dev, "Only support 8bit and 16 bit!\n");
-		return -EINVAL;
-	}
-
-	/* calculate flash information */
-	host->page_size = f->page_size;
-	host->read_id_bytes = (f->page_size == 2048) ? 4 : 2;
-
-	/* calculate addressing information */
-	host->col_addr_cycles = (f->page_size == 2048) ? 2 : 1;
-
-	if (f->num_blocks * f->page_per_block > 65536)
-		host->row_addr_cycles = 3;
-	else
-		host->row_addr_cycles = 2;
-
-	ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
-	ndcr |= (host->col_addr_cycles == 2) ? NDCR_RA_START : 0;
-	ndcr |= (f->page_per_block == 64) ? NDCR_PG_PER_BLK : 0;
-	ndcr |= (f->page_size == 2048) ? NDCR_PAGE_SZ : 0;
-	ndcr |= (f->flash_width == 16) ? NDCR_DWIDTH_M : 0;
-	ndcr |= (f->dfc_width == 16) ? NDCR_DWIDTH_C : 0;
-
-	ndcr |= NDCR_RD_ID_CNT(host->read_id_bytes);
-	ndcr |= NDCR_SPARE_EN; /* enable spare by default */
-
-	info->reg_ndcr = ndcr;
-
-	pxa3xx_nand_set_timing(host, f->timing);
-	return 0;
-}
-
-static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
+static void pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 {
 	/*
 	 * We set 0 by hard coding here, for we don't support keep_config
@@ -909,7 +841,6 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
 	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
 	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
-	return 0;
 }
 
 /* the maximum possible buffer size for large page with OOB data
@@ -982,17 +913,10 @@ static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 {
 	struct mtd_info *mtd;
-	int ret;
 	mtd = info->host[info->cs]->mtd;
-	/* use the common timing to make a try */
-	ret = pxa3xx_nand_config_flash(info, &builtin_flash_types[0]);
-	if (ret)
-		return ret;
-
 	pxa3xx_nand_cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
 	if (info->is_ready)
 		return 0;
-
 	return -ENODEV;
 }
 
@@ -1000,72 +924,18 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 {
 	struct pxa3xx_nand_host *host = mtd->priv;
 	struct pxa3xx_nand_info *info = host->info_data;
-	struct platform_device *pdev = info->pdev;
-	struct pxa3xx_nand_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct nand_flash_dev pxa3xx_flash_ids[2], *def = NULL;
-	const struct pxa3xx_nand_flash *f = NULL;
 	struct nand_chip *chip = mtd->priv;
-	uint32_t id = -1;
-	uint64_t chipsize;
-	int i, ret, num;
+	int ret;
 
-	if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
-		goto KEEP_CONFIG;
+	pxa3xx_nand_detect_config(info);
 
 	ret = pxa3xx_nand_sensing(info);
 	if (ret) {
 		dev_info(&info->pdev->dev, "There is no chip on cs %d!\n",
 			 info->cs);
-
-		return ret;
-	}
-
-	chip->cmdfunc(mtd, NAND_CMD_READID, 0, 0);
-	id = *((uint16_t *)(info->data_buff));
-	if (id != 0)
-		dev_info(&info->pdev->dev, "Detect a flash id %x\n", id);
-	else {
-		dev_warn(&info->pdev->dev,
-			 "Read out ID 0, potential timing set wrong!!\n");
-
-		return -EINVAL;
-	}
-
-	num = ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1;
-	for (i = 0; i < num; i++) {
-		if (i < pdata->num_flash)
-			f = pdata->flash + i;
-		else
-			f = &builtin_flash_types[i - pdata->num_flash + 1];
-
-		/* find the chip in default list */
-		if (f->chip_id == id)
-			break;
-	}
-
-	if (i >= (ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1)) {
-		dev_err(&info->pdev->dev, "ERROR!! flash not defined!!!\n");
-
-		return -EINVAL;
-	}
-
-	ret = pxa3xx_nand_config_flash(info, f);
-	if (ret) {
-		dev_err(&info->pdev->dev, "ERROR! Configure failed\n");
 		return ret;
 	}
 
-	pxa3xx_flash_ids[0].name = f->name;
-	pxa3xx_flash_ids[0].dev_id = (f->chip_id >> 8) & 0xffff;
-	pxa3xx_flash_ids[0].pagesize = f->page_size;
-	chipsize = (uint64_t)f->num_blocks * f->page_per_block * f->page_size;
-	pxa3xx_flash_ids[0].chipsize = chipsize >> 20;
-	pxa3xx_flash_ids[0].erasesize = f->page_size * f->page_per_block;
-	if (f->flash_width == 16)
-		pxa3xx_flash_ids[0].options = NAND_BUSWIDTH_16;
-	pxa3xx_flash_ids[1].name = NULL;
-	def = pxa3xx_flash_ids;
-KEEP_CONFIG:
 	chip->ecc.mode = NAND_ECC_HW;
 	chip->ecc.size = host->page_size;
 	chip->ecc.strength = 1;
@@ -1073,7 +943,7 @@ KEEP_CONFIG:
 	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
-	if (nand_scan_ident(mtd, 1, def))
+	if (nand_scan_ident(mtd, 1, NULL))
 		return -ENODEV;
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
-- 
1.8.1.5

Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing

[Daniel Mack]

On 10.09.2013 13:17, Ezequiel Garcia wrote:
> There's no need to go through this internal probe/auto-detect
> procedure, since the nand core code will take care of that.
> This commit removes the configuration and detection functions,
> together with the built-in flash device table.
> 
> Besides being unneeded, it's also wrong to take care of such details
> wich rightfully belong to the NAND base code. Removing this wrong
> code, prevents the proliferation of the same mistake in future drivers.
> 
> This commit has the effect of forcing the "keep_config" option.

I get the following build warning with this patch:

  drivers/mtd/nand/pxa3xx_nand.c:221:13: warning:
‘pxa3xx_nand_set_timing’ defined but not used [-Wunused-function]

Apart from that, this seems to work fine on my board, but I suspect that
it would break systems where the NAND controller is not initialized from
the bootloader, right?


Thanks,
Daniel





> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 138 ++---------------------------------------
>  1 file changed, 4 insertions(+), 134 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index dd03dfd..bfcc588 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -212,28 +212,6 @@ static bool use_dma = 1;
>  module_param(use_dma, bool, 0444);
>  MODULE_PARM_DESC(use_dma, "enable DMA for data transferring to/from NAND HW");
>  
> -static struct pxa3xx_nand_timing timing[] = {
> -	{ 40, 80, 60, 100, 80, 100, 90000, 400, 40, },
> -	{ 10,  0, 20,  40, 30,  40, 11123, 110, 10, },
> -	{ 10, 25, 15,  25, 15,  30, 25000,  60, 10, },
> -	{ 10, 35, 15,  25, 15,  25, 25000,  60, 10, },
> -};
> -
> -static struct pxa3xx_nand_flash builtin_flash_types[] = {
> -{ "DEFAULT FLASH",      0,   0, 2048,  8,  8,    0, &timing[0] },
> -{ "64MiB 16-bit",  0x46ec,  32,  512, 16, 16, 4096, &timing[1] },
> -{ "256MiB 8-bit",  0xdaec,  64, 2048,  8,  8, 2048, &timing[1] },
> -{ "4GiB 8-bit",    0xd7ec, 128, 4096,  8,  8, 8192, &timing[1] },
> -{ "128MiB 8-bit",  0xa12c,  64, 2048,  8,  8, 1024, &timing[2] },
> -{ "128MiB 16-bit", 0xb12c,  64, 2048, 16, 16, 1024, &timing[2] },
> -{ "512MiB 8-bit",  0xdc2c,  64, 2048,  8,  8, 4096, &timing[2] },
> -{ "512MiB 16-bit", 0xcc2c,  64, 2048, 16, 16, 4096, &timing[2] },
> -{ "256MiB 16-bit", 0xba20,  64, 2048, 16, 16, 2048, &timing[3] },
> -};
> -
> -/* Define a default flash type setting serve as flash detecting only */
> -#define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
> -
>  #define NDTR0_tCH(c)	(min((c), 7) << 19)
>  #define NDTR0_tCS(c)	(min((c), 7) << 16)
>  #define NDTR0_tWH(c)	(min((c), 7) << 11)
> @@ -843,53 +821,7 @@ static int pxa3xx_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *this)
>  	return 0;
>  }
>  
> -static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
> -				    const struct pxa3xx_nand_flash *f)
> -{
> -	struct platform_device *pdev = info->pdev;
> -	struct pxa3xx_nand_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct pxa3xx_nand_host *host = info->host[info->cs];
> -	uint32_t ndcr = 0x0; /* enable all interrupts */
> -
> -	if (f->page_size != 2048 && f->page_size != 512) {
> -		dev_err(&pdev->dev, "Current only support 2048 and 512 size\n");
> -		return -EINVAL;
> -	}
> -
> -	if (f->flash_width != 16 && f->flash_width != 8) {
> -		dev_err(&pdev->dev, "Only support 8bit and 16 bit!\n");
> -		return -EINVAL;
> -	}
> -
> -	/* calculate flash information */
> -	host->page_size = f->page_size;
> -	host->read_id_bytes = (f->page_size == 2048) ? 4 : 2;
> -
> -	/* calculate addressing information */
> -	host->col_addr_cycles = (f->page_size == 2048) ? 2 : 1;
> -
> -	if (f->num_blocks * f->page_per_block > 65536)
> -		host->row_addr_cycles = 3;
> -	else
> -		host->row_addr_cycles = 2;
> -
> -	ndcr |= (pdata->enable_arbiter) ? NDCR_ND_ARB_EN : 0;
> -	ndcr |= (host->col_addr_cycles == 2) ? NDCR_RA_START : 0;
> -	ndcr |= (f->page_per_block == 64) ? NDCR_PG_PER_BLK : 0;
> -	ndcr |= (f->page_size == 2048) ? NDCR_PAGE_SZ : 0;
> -	ndcr |= (f->flash_width == 16) ? NDCR_DWIDTH_M : 0;
> -	ndcr |= (f->dfc_width == 16) ? NDCR_DWIDTH_C : 0;
> -
> -	ndcr |= NDCR_RD_ID_CNT(host->read_id_bytes);
> -	ndcr |= NDCR_SPARE_EN; /* enable spare by default */
> -
> -	info->reg_ndcr = ndcr;
> -
> -	pxa3xx_nand_set_timing(host, f->timing);
> -	return 0;
> -}
> -
> -static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
> +static void pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
>  {
>  	/*
>  	 * We set 0 by hard coding here, for we don't support keep_config
> @@ -909,7 +841,6 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
>  	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
>  	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
>  	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
> -	return 0;
>  }
>  
>  /* the maximum possible buffer size for large page with OOB data
> @@ -982,17 +913,10 @@ static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
>  static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
>  {
>  	struct mtd_info *mtd;
> -	int ret;
>  	mtd = info->host[info->cs]->mtd;
> -	/* use the common timing to make a try */
> -	ret = pxa3xx_nand_config_flash(info, &builtin_flash_types[0]);
> -	if (ret)
> -		return ret;
> -
>  	pxa3xx_nand_cmdfunc(mtd, NAND_CMD_RESET, 0, 0);
>  	if (info->is_ready)
>  		return 0;
> -
>  	return -ENODEV;
>  }
>  
> @@ -1000,72 +924,18 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
>  {
>  	struct pxa3xx_nand_host *host = mtd->priv;
>  	struct pxa3xx_nand_info *info = host->info_data;
> -	struct platform_device *pdev = info->pdev;
> -	struct pxa3xx_nand_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct nand_flash_dev pxa3xx_flash_ids[2], *def = NULL;
> -	const struct pxa3xx_nand_flash *f = NULL;
>  	struct nand_chip *chip = mtd->priv;
> -	uint32_t id = -1;
> -	uint64_t chipsize;
> -	int i, ret, num;
> +	int ret;
>  
> -	if (pdata->keep_config && !pxa3xx_nand_detect_config(info))
> -		goto KEEP_CONFIG;
> +	pxa3xx_nand_detect_config(info);
>  
>  	ret = pxa3xx_nand_sensing(info);
>  	if (ret) {
>  		dev_info(&info->pdev->dev, "There is no chip on cs %d!\n",
>  			 info->cs);
> -
> -		return ret;
> -	}
> -
> -	chip->cmdfunc(mtd, NAND_CMD_READID, 0, 0);
> -	id = *((uint16_t *)(info->data_buff));
> -	if (id != 0)
> -		dev_info(&info->pdev->dev, "Detect a flash id %x\n", id);
> -	else {
> -		dev_warn(&info->pdev->dev,
> -			 "Read out ID 0, potential timing set wrong!!\n");
> -
> -		return -EINVAL;
> -	}
> -
> -	num = ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1;
> -	for (i = 0; i < num; i++) {
> -		if (i < pdata->num_flash)
> -			f = pdata->flash + i;
> -		else
> -			f = &builtin_flash_types[i - pdata->num_flash + 1];
> -
> -		/* find the chip in default list */
> -		if (f->chip_id == id)
> -			break;
> -	}
> -
> -	if (i >= (ARRAY_SIZE(builtin_flash_types) + pdata->num_flash - 1)) {
> -		dev_err(&info->pdev->dev, "ERROR!! flash not defined!!!\n");
> -
> -		return -EINVAL;
> -	}
> -
> -	ret = pxa3xx_nand_config_flash(info, f);
> -	if (ret) {
> -		dev_err(&info->pdev->dev, "ERROR! Configure failed\n");
>  		return ret;
>  	}
>  
> -	pxa3xx_flash_ids[0].name = f->name;
> -	pxa3xx_flash_ids[0].dev_id = (f->chip_id >> 8) & 0xffff;
> -	pxa3xx_flash_ids[0].pagesize = f->page_size;
> -	chipsize = (uint64_t)f->num_blocks * f->page_per_block * f->page_size;
> -	pxa3xx_flash_ids[0].chipsize = chipsize >> 20;
> -	pxa3xx_flash_ids[0].erasesize = f->page_size * f->page_per_block;
> -	if (f->flash_width == 16)
> -		pxa3xx_flash_ids[0].options = NAND_BUSWIDTH_16;
> -	pxa3xx_flash_ids[1].name = NULL;
> -	def = pxa3xx_flash_ids;
> -KEEP_CONFIG:
>  	chip->ecc.mode = NAND_ECC_HW;
>  	chip->ecc.size = host->page_size;
>  	chip->ecc.strength = 1;
> @@ -1073,7 +943,7 @@ KEEP_CONFIG:
>  	if (info->reg_ndcr & NDCR_DWIDTH_M)
>  		chip->options |= NAND_BUSWIDTH_16;
>  
> -	if (nand_scan_ident(mtd, 1, def))
> +	if (nand_scan_ident(mtd, 1, NULL))
>  		return -ENODEV;
>  	/* calculate addressing information */
>  	if (mtd->writesize >= 2048)
> 

Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing

[Ezequiel Garcia]

On Tue, Sep 10, 2013 at 03:46:49PM +0200, Daniel Mack wrote:
> On 10.09.2013 13:17, Ezequiel Garcia wrote:
> > There's no need to go through this internal probe/auto-detect
> > procedure, since the nand core code will take care of that.
> > This commit removes the configuration and detection functions,
> > together with the built-in flash device table.
> > 
> > Besides being unneeded, it's also wrong to take care of such details
> > wich rightfully belong to the NAND base code. Removing this wrong
> > code, prevents the proliferation of the same mistake in future drivers.
> > 
> > This commit has the effect of forcing the "keep_config" option.
> 
> I get the following build warning with this patch:
> 
>   drivers/mtd/nand/pxa3xx_nand.c:221:13: warning:
> ‘pxa3xx_nand_set_timing’ defined but not used [-Wunused-function]
> 

Yes, that's fine. I'm keeping that until we decide what to do with
timings.

> Apart from that, this seems to work fine on my board,

Great! Which board is that?

> but I suspect that
> it would break systems where the NAND controller is not initialized from
> the bootloader, right?
> 

Right. However, since we can easily add support to configure every controller
parameter (right?) this shouldn't be a problem.

What do you think of this change, Daniel?

IMO, the code is ugly, useless and deprecated enough to consider its removal.
It forces to keep a (duplicated) list of known flash devices and it considers
only 512 and 2048 page sizes, just to name a few limitations.

But I'd love to hear a second opinion.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing

[Daniel Mack]

On 10.09.2013 15:57, Ezequiel Garcia wrote:
> On Tue, Sep 10, 2013 at 03:46:49PM +0200, Daniel Mack wrote:

>> Apart from that, this seems to work fine on my board,
> 
> Great! Which board is that?

It's a custom board featuring a PXA303 processor connected to a ST Micro
NAND:

[    2.655607] NAND device: Manufacturer ID: 0x20, Chip ID: 0xa1 (ST
Micro NAND01GR3B2CZA6), 128MiB, page size: 2048, OOB size: 64

>> but I suspect that
>> it would break systems where the NAND controller is not initialized from
>> the bootloader, right?
>>
> 
> Right. However, since we can easily add support to configure every controller
> parameter (right?) this shouldn't be a problem.
> 
> What do you think of this change, Daniel?

I always thought that this detail of the pxa nand driver is ugly :) But
I'd say before it can be merged, you need to provide code to set the
timing from parameters obtained from generic part. Are you working on
this? I'd happily test more patches.

> IMO, the code is ugly, useless and deprecated enough to consider its removal.
> It forces to keep a (duplicated) list of known flash devices and it considers
> only 512 and 2048 page sizes, just to name a few limitations.

Jup, agreed.


Thanks,
Daniel

Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing

[Ezequiel Garcia]

On Tue, Sep 10, 2013 at 04:14:25PM +0200, Daniel Mack wrote:
[..]
> 
> I always thought that this detail of the pxa nand driver is ugly :) But
> I'd say before it can be merged, you need to provide code to set the
> timing from parameters obtained from generic part.

Agreed.

> Are you working on  this? I'd happily test more patches.
> 

Yes, I'm working on this. I'll keep on sending patches then :-)

Thanks for the test and the feedback,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Setting NAND timings parameters (Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing)

[Ezequiel Garcia]

On Tue, Sep 10, 2013 at 04:14:25PM +0200, Daniel Mack wrote:
> > 
> > Right. However, since we can easily add support to configure every controller
> > parameter (right?) this shouldn't be a problem.
> > 
> > What do you think of this change, Daniel?
> 
> I always thought that this detail of the pxa nand driver is ugly :) But
> I'd say before it can be merged, you need to provide code to set the
> timing from parameters obtained from generic part. Are you working on
> this? I'd happily test more patches.
> 

Returning to this point: it seems we have two different cases: ONFI-compliant
devices and non-ONFI.

For the ONFI, we can have a timings parameter table with some index according
to the ONFI timing mode available or user-selected. This table could be
generic (as in Matthieu Castet patch [1]) or driver specific (as in
denali driver).

For the non-ONFI, we could just add platform data / device-tree bindings
to allow to user to set the timings.

[1] http://patchwork.ozlabs.org/patch/197506/

How does this sound?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: Setting NAND timings parameters (Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing)

[Brian Norris]

Hi Ezequiel,

I see that the removal of your driver-specific device table stalled on
this issue. Are you planning on picking it up?

On Tue, Sep 24, 2013 at 06:46:54PM -0300, Ezequiel Garcia wrote:
> On Tue, Sep 10, 2013 at 04:14:25PM +0200, Daniel Mack wrote:
> > > 
> > > Right. However, since we can easily add support to configure every controller
> > > parameter (right?) this shouldn't be a problem.
> > > 
> > > What do you think of this change, Daniel?
> > 
> > I always thought that this detail of the pxa nand driver is ugly :) But
> > I'd say before it can be merged, you need to provide code to set the
> > timing from parameters obtained from generic part. Are you working on
> > this? I'd happily test more patches.
> > 
> 
> Returning to this point: it seems we have two different cases: ONFI-compliant
> devices and non-ONFI.
> 
> For the ONFI, we can have a timings parameter table with some index according
> to the ONFI timing mode available or user-selected. This table could be
> generic (as in Matthieu Castet patch [1]) or driver specific (as in
> denali driver).

It looks like denali just uses a module parameter so the user can select
an ONFI timing mode. This isn't flexible or generically useful and
shouldn't be encouraged on new code.

If more drivers need timing information like this, I would like to see a
generic implementation like Matthieu's (although his can be improved, I
think). It's also worth noting that his code only gets information for
the SDR timing modes, not DDR. I don't know if any hardware is utilizing
DDR modes (I haven't tested it on mine), but it's worth considering
if/how this would fit into the framework.

> For the non-ONFI, we could just add platform data / device-tree bindings
> to allow to user to set the timings.
> 
> [1] http://patchwork.ozlabs.org/patch/197506/
> 
> How does this sound?

Brian

Re: Setting NAND timings parameters (Re: [RFC/PATCH v2] mtd: nand: pxa3xx: Remove redundant device probing)

[Ezequiel Garcia]

On Tue, Nov 26, 2013 at 03:17:09PM -0800, Brian Norris wrote:
> 
> I see that the removal of your driver-specific device table stalled on
> this issue. Are you planning on picking it up?
> 

Kind of. I'd love to fix this ugly code, but have lots of others items
on this driver's TODO list.

> On Tue, Sep 24, 2013 at 06:46:54PM -0300, Ezequiel Garcia wrote:
> > On Tue, Sep 10, 2013 at 04:14:25PM +0200, Daniel Mack wrote:
> > > > 
> > > > Right. However, since we can easily add support to configure every controller
> > > > parameter (right?) this shouldn't be a problem.
> > > > 
> > > > What do you think of this change, Daniel?
> > > 
> > > I always thought that this detail of the pxa nand driver is ugly :) But
> > > I'd say before it can be merged, you need to provide code to set the
> > > timing from parameters obtained from generic part. Are you working on
> > > this? I'd happily test more patches.
> > > 
> > 
> > Returning to this point: it seems we have two different cases: ONFI-compliant
> > devices and non-ONFI.
> > 
> > For the ONFI, we can have a timings parameter table with some index according
> > to the ONFI timing mode available or user-selected. This table could be
> > generic (as in Matthieu Castet patch [1]) or driver specific (as in
> > denali driver).
> 
> It looks like denali just uses a module parameter so the user can select
> an ONFI timing mode. This isn't flexible or generically useful and
> shouldn't be encouraged on new code.
> 

Agreed.

> If more drivers need timing information like this, I would like to see a
> generic implementation like Matthieu's (although his can be improved, I
> think). It's also worth noting that his code only gets information for
> the SDR timing modes, not DDR. I don't know if any hardware is utilizing
> DDR modes (I haven't tested it on mine), but it's worth considering
> if/how this would fit into the framework.
> 

OK, I'll see if I can push my previous work on this. I have a modified
patch in some branch around here.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com