Bug in nand_select_chip?

Bug in nand_select_chip?

[llandre]

Hi,

I have a question about the nand_select_chip function. I wrote a driver for 
a Samsung NAND
Flash device (K9F5608U0C - 32MB). Everything worked fine with old MTD 
sources (July 2003).
After updating the MTD with latest CVS (2004-03-17) and applying the ilookup
patch 
(http://www.infradead.org/cgi-bin/cvsweb.cgi/~checkout~/mtd/patches/ilookup-2.4.23.patch?rev=1.1),
the same driver did not work anymore.
Then I had a look at the new sources and I noted that the nand_select_chip 
changed completely.

The old function, with chip==0, sets the nCE pin low:

static void nand_select_chip(struct mtd_info *mtd, int chip)
{
         struct nand_chip *this = mtd->priv;
         if (chip)
                 this->hwcontrol(mtd, NAND_CTL_SETNCE);
         else
                 this->hwcontrol(mtd, NAND_CTL_CLRNCE);

}


The new function, with chip==0, sets the nCE pin high:

static void nand_select_chip(struct mtd_info *mtd, int chip)
{
         struct nand_chip *this = mtd->priv;
         switch(chip) {
         case -1:
                 this->hwcontrol(mtd, NAND_CTL_CLRNCE);
                 break;
         case 0:
                 this->hwcontrol(mtd, NAND_CTL_SETNCE);
                 break;

         default:
                 BUG();
         }
}

For this reason the chip was clearly not detected at all.
After changing the new function as follows, everything works fine again:

static void nand_select_chip(struct mtd_info *mtd, int chip)
{
         struct nand_chip *this = mtd->priv;
         switch(chip) {
         case -1:
                 /* llandre - inverted default behaviour */
                 this->hwcontrol(mtd, NAND_CTL_SETNCE);
                 break;
         case 0:
                 /* llandre - inverted default behaviour */
                 this->hwcontrol(mtd, NAND_CTL_CLRNCE);
                 break;

         default:
                 BUG();
         }
}


1) Why did the code change this way?
2) I think the best way to overcome the problem is to define a proprietary 
nand_select_chip function
in the low-level driver and to make the select_chip pointer in the struct 
nand_chip to point to it. Correct?





Many thanks in advance,

llandre 

Re: Bug in nand_select_chip?

[David Woodhouse]

On Wed, 2004-03-17 at 16:44 +0100, llandre wrote:
> The old function, with chip==0, sets the nCE pin low:

> The new function, with chip==0, sets the nCE pin high:

> 1) Why did the code change this way?

To handle multiple chips. More correctly, should should say
"The new function, with chip == N, sets nCE on chip #N high."

To deselect _all_ chips you use chip == -1.

All callers of nand_select_chip() were changed at the same time
though... this shouldn't cause a problem.

-- 
dwmw2

Re: Bug in nand_select_chip?

[Thomas Gleixner]

On Wednesday 17 March 2004 16:44, llandre wrote:
> Hi,

The calling functions were changed, so there is no problem at all.

> The new function, with chip==0, sets the nCE pin high:

Wrong. SETNCE means set nCE active, which sets the physical pin low.
Therefor it is called _n_CE and SET_N_CE.

> 1) Why did the code change this way?

To handle multiple chips.

> 2) I think the best way to overcome the problem is to define a proprietary
> nand_select_chip function
> in the low-level driver and to make the select_chip pointer in the struct
> nand_chip to point to it. Correct?

You need only a seperate select function, if you have non standard hardware. 
The hwcontrol function should be enough adjustment for most boards, where you 
have 1 NAND chip to select.

-- 
Thomas
________________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: tglx@linutronix.de

Re: Bug in nand_select_chip?

[llandre]

>On Wed, 2004-03-17 at 16:44 +0100, llandre wrote:
> > The old function, with chip==0, sets the nCE pin low:
>
> > The new function, with chip==0, sets the nCE pin high:
>
> > 1) Why did the code change this way?
>
>To handle multiple chips. More correctly, should should say
>"The new function, with chip == N, sets nCE on chip #N high."
>
>To deselect _all_ chips you use chip == -1.
>
>All callers of nand_select_chip() were changed at the same time
>though... this shouldn't cause a problem.

I fixed the driver as shown in the NAND Flash document:
- nCE pin low when hwcontrol is invoked with NAND_CTL_SETNCE
- nCE pin high when hwcontrol is invoked with NAND_CTL_CLRNCE

and now the driver works with the new MTD code.




Regards,

llandre 

Re: Bug in nand_select_chip?

[Thomas Gleixner]

On Wednesday 17 March 2004 17:51, llandre wrote:
>
> I fixed the driver as shown in the NAND Flash document:
> - nCE pin low when hwcontrol is invoked with NAND_CTL_SETNCE
> - nCE pin high when hwcontrol is invoked with NAND_CTL_CLRNCE
> and now the driver works with the new MTD code.

That's the reason why documents are written. :)

But the driver should work with the old code too, as this never changed since 
we started NAND support in JFFS2.

-- 
Thomas
________________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: tglx@linutronix.de

Re: Bug in nand_select_chip?

[llandre]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]


> > I fixed the driver as shown in the NAND Flash document:
> > - nCE pin low when hwcontrol is invoked with NAND_CTL_SETNCE
> > - nCE pin high when hwcontrol is invoked with NAND_CTL_CLRNCE
> > and now the driver works with the new MTD code.
>
>That's the reason why documents are written. :)
>
>But the driver should work with the old code too, as this never changed since
>we started NAND support in JFFS2.

In my understanding the driver does not work with the old code because
the old nand_select_chip() implementation called this->hwcontrol(mtd, 
NAND_CTL_CLRNCE),
with chip==0, setting the nCE pin high.

By the way, as described in the mentioned document, I send you the driver 
source code in order to
include it in the public source tree. It supports the chip K9F5608U0B 
(32MB) by Samsung, mounted on
the PPChameleonEVB board. To build it, it is necessary to add the following 
line in the Makefile:
obj-$(CONFIG_MTD_NAND_PPCHAMELEONEVB)   += ppchameleonevb.o

and the following lines in the Config.in:

if [ "$CONFIG_PPCHAMELEONEVB" = "y" ]; then
    dep_tristate '  NAND Flash device on PPChameleonEVB board' 
CONFIG_MTD_NAND_PPCHAMELEONEVB $CONFIG_MTD_NAND
fi

We tested it with the kernel 2.4.20 included the linuxppc_2_4_devel tree 
hosted by Denx Software
Engineering (www.denx.de).



Best regards,

llandre 

[-- Attachment #2: ppchameleonevb.c --]
[-- Type: text/plain, Size: 6278 bytes --]

/*
 *  drivers/mtd/nand/ppchameleonevb.c
 *
 *  Copyright (C) 2003 DAVE Srl (info@wawnet.biz)
 *
 *  Derived from drivers/mtd/nand/edb7312.c
 *
 *
 * $Id: ppchameleonevb.c,v 1.0 2003/09/26 12:58:16 mag Exp $
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 *
 *  Overview:
 *   This is a device driver for the NAND flash device found on the
 *   PPChameleonEVB board which utilizes the Samsung K9F5608U0B part. This is
 *   a 256Mibit (32MiB x 8 bits) NAND flash device.
 */

#include <linux/init.h>
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
#include <linux/mtd/partitions.h>
#include <asm/io.h>
#include <platforms/PPChameleonEVB.h>

/* handy sizes */
#define SZ_4M                           0x00400000

/* GPIO pins used to drive NAND chip */
#define NAND_nCE_GPIO_PIN (1 << 14)
#define NAND_CLE_GPIO_PIN (1 << 15)
#define NAND_ALE_GPIO_PIN (1 << 16)

/*
 * MTD structure for PPChameleonEVB board
 */
static struct mtd_info *ppchameleonevb_mtd = NULL;

/*
 * Module stuff
 */
static int ppchameleonevb_fio_pbase = CFG_NAND1_PADDR;
#ifdef MODULE
MODULE_PARM(ppchameleonevb_fio_pbase, "i");

__setup("ppchameleonevb_fio_pbase=",ppchameleonevb_fio_pbase);
#endif

#ifdef CONFIG_MTD_PARTITIONS
/*
 * Define static partitions for flash device
 */
static struct mtd_partition partition_info[] = {
	{ name: "PPChameleonEVB Nand Flash",
		  offset: 0,
		  size: 32*1024*1024 }
};
#define NUM_PARTITIONS 1

extern int parse_cmdline_partitions(struct mtd_info *master,
				    struct mtd_partition **pparts,
				    const char *mtd_id);
#endif


/*
 *	hardware specific access to control-lines
 */
static void ppchameleonevb_hwcontrol(struct mtd_info *mtdinfo, int cmd)
{
	switch(cmd) {

	case NAND_CTL_SETCLE:
        	MACRO_NAND_CTL_SETCLE(CFG_NAND1_PADDR);
		break;
	case NAND_CTL_CLRCLE:
        	MACRO_NAND_CTL_CLRCLE(CFG_NAND1_PADDR);
		break;
	case NAND_CTL_SETALE:
        	MACRO_NAND_CTL_SETALE(CFG_NAND1_PADDR);
		break;
	case NAND_CTL_CLRALE:
        	MACRO_NAND_CTL_CLRALE(CFG_NAND1_PADDR);
		break;
	case NAND_CTL_SETNCE:
        	MACRO_NAND_ENABLE_CE(CFG_NAND1_PADDR);
		break;
	case NAND_CTL_CLRNCE:
        	MACRO_NAND_DISABLE_CE(CFG_NAND1_PADDR);
		break;
	}
}

#if 0
/*
 *	read device ready pin
 */
static int ppchameleonevb_device_ready(struct mtd_info *minfo)
{
	return 1;
}
#endif

#ifdef CONFIG_MTD_PARTITIONS
const char *part_probes_evb[] = { "cmdlinepart", NULL };
#endif

/*
 * Main initialization routine
 */
static int __init ppchameleonevb_init (void)
{
	struct nand_chip *this;
	const char *part_type = 0;
	int mtd_parts_nb = 0;
	struct mtd_partition *mtd_parts = 0;
	int ppchameleonevb_fio_base;


	/* Allocate memory for MTD device structure and private data */
	ppchameleonevb_mtd = kmalloc(sizeof(struct mtd_info) +
			     sizeof(struct nand_chip),
			     GFP_KERNEL);
	if (!ppchameleonevb_mtd) {
		printk("Unable to allocate PPChameleonEVB NAND MTD device structure.\n");
		return -ENOMEM;
	}

	/* map physical address */
	ppchameleonevb_fio_base = (unsigned long)ioremap(ppchameleonevb_fio_pbase, SZ_4M);
	if(!ppchameleonevb_fio_base) {
		printk("ioremap PPChameleonEVB NAND flash failed\n");
		kfree(ppchameleonevb_mtd);
		return -EIO;
	}

	/* Get pointer to private data */
	this = (struct nand_chip *) (&ppchameleonevb_mtd[1]);

	/* Initialize structures */
	memset((char *) ppchameleonevb_mtd, 0, sizeof(struct mtd_info));
	memset((char *) this, 0, sizeof(struct nand_chip));

	/* Link the private data with the MTD structure */
	ppchameleonevb_mtd->priv = this;

        /* Initialize GPIOs */
	/* Pin mapping for NAND chip */
	/*
		CE	GPIO_14
		CLE	GPIO_15
		ALE	GPIO_16
		R/B	GPIO_31 (still not used)
	*/
	/* output select */
	out_be32(GPIO0_OSRH, in_be32(GPIO0_OSRH) & 0xFFFFFFF0);
	out_be32(GPIO0_OSRL, in_be32(GPIO0_OSRL) & 0x3FFFFFFF);
	/* three-state select */
	out_be32(GPIO0_TSRH, in_be32(GPIO0_TSRH) & 0xFFFFFFF0);
	out_be32(GPIO0_TSRL, in_be32(GPIO0_TSRL) & 0x3FFFFFFF);
	/* enable output driver */
	out_be32(GPIO0_TCR, in_be32(GPIO0_TCR) | NAND_nCE_GPIO_PIN | NAND_CLE_GPIO_PIN | NAND_ALE_GPIO_PIN);

	/* insert callbacks */
	this->IO_ADDR_R = ppchameleonevb_fio_base;
	this->IO_ADDR_W = ppchameleonevb_fio_base;
	this->hwcontrol = ppchameleonevb_hwcontrol;
	/*this->dev_ready = ppchameleonevb_device_ready;*/
        this->dev_ready = NULL;
	/* 12 us command delay time */
	this->chip_delay = 12;
	/* ECC mode */
	this->eccmode = NAND_ECC_SOFT;


	/* Scan to find existence of the device */
	if (nand_scan (ppchameleonevb_mtd, 1)) {
		iounmap((void *)ppchameleonevb_fio_base);
		kfree (ppchameleonevb_mtd);
		return -ENXIO;
	}



	/* Allocate memory for internal data buffer */
	this->data_buf = kmalloc (sizeof(u_char) * (ppchameleonevb_mtd->oobblock + ppchameleonevb_mtd->oobsize), GFP_KERNEL);
	if (!this->data_buf) {
		printk("Unable to allocate NAND data buffer for PPChameleonEVB.\n");
		iounmap((void *)ppchameleonevb_fio_base);
		kfree (ppchameleonevb_mtd);
		return -ENOMEM;

	}


#ifdef CONFIG_MTD_PARTITIONS
	ppchameleonevb_mtd->name = "ppchameleonevb-nand";
	mtd_parts_nb = parse_mtd_partitions(ppchameleonevb_mtd, part_probes_evb, &mtd_parts, 0);
	if (mtd_parts_nb > 0)
	  part_type = "command line";
	else
	  mtd_parts_nb = 0;
#endif
	if (mtd_parts_nb == 0)
	{
		mtd_parts = partition_info;
		mtd_parts_nb = NUM_PARTITIONS;
		part_type = "static";
	}



	/* Register the partitions */
	printk(KERN_NOTICE "Using %s partition definition\n", part_type);
	add_mtd_partitions(ppchameleonevb_mtd, mtd_parts, mtd_parts_nb);

	/* Return happy */
	return 0;
}
module_init(ppchameleonevb_init);

/*
 * Clean up routine
 */
static void __exit ppchameleonevb_cleanup (void)
{
	struct nand_chip *this = (struct nand_chip *) &ppchameleonevb_mtd[1];

	/* Unregister the device */
	del_mtd_device (ppchameleonevb_mtd);

	/* Free internal data buffer */
	kfree (this->data_buf);

	/* Free the MTD device structure */
	kfree (ppchameleonevb_mtd);
}
module_exit(ppchameleonevb_cleanup);

MODULE_LICENSE("GPL");
MODULE_AUTHOR("llandre <llandre@wawnet.biz>");
MODULE_DESCRIPTION("MTD map driver for DAVE Srl PPChameleonEVB board");

Re: Bug in nand_select_chip?

[Thomas Gleixner]

On Thursday 18 March 2004 09:11, llandre wrote:
> > > I fixed the driver as shown in the NAND Flash document:
> > > - nCE pin low when hwcontrol is invoked with NAND_CTL_SETNCE
> > > - nCE pin high when hwcontrol is invoked with NAND_CTL_CLRNCE
> > > and now the driver works with the new MTD code.
> >
> >That's the reason why documents are written. :)
> >
> >But the driver should work with the old code too, as this never changed
> > since we started NAND support in JFFS2.
>
> In my understanding the driver does not work with the old code because
> the old nand_select_chip() implementation called this->hwcontrol(mtd,
> NAND_CTL_CLRNCE),
> with chip==0, setting the nCE pin high.

That's correct, but you unfortunaly relied on code, which was in a heavy 
modifciation phase. If you use leading edge code, be aware, that things might 
be broken from time to time. The fix for this was committed 10 days after the 
first rework. 

-- 
Thomas
________________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: tglx@linutronix.de

Re: Bug in nand_select_chip?

[llandre]

>On Thursday 18 March 2004 09:11, llandre wrote:
> > > > I fixed the driver as shown in the NAND Flash document:
> > > > - nCE pin low when hwcontrol is invoked with NAND_CTL_SETNCE
> > > > - nCE pin high when hwcontrol is invoked with NAND_CTL_CLRNCE
> > > > and now the driver works with the new MTD code.
> > >
> > >That's the reason why documents are written. :)
> > >
> > >But the driver should work with the old code too, as this never changed
> > > since we started NAND support in JFFS2.
> >
> > In my understanding the driver does not work with the old code because
> > the old nand_select_chip() implementation called this->hwcontrol(mtd,
> > NAND_CTL_CLRNCE),
> > with chip==0, setting the nCE pin high.
>
>That's correct, but you unfortunaly relied on code, which was in a heavy
>modifciation phase. If you use leading edge code, be aware, that things might
>be broken from time to time. The fix for this was committed 10 days after the
>first rework.

Ok, thanks for the advice.
To avoid such problems, is it possible to download "stable" releases? Are they
tagged in the CVS repository?



Best regards,

llandre 

Re: Bug in nand_select_chip?

[Thomas Gleixner]

On Thursday 18 March 2004 10:56, llandre wrote:
> >That's correct, but you unfortunaly relied on code, which was in a heavy
> >modifciation phase. If you use leading edge code, be aware, that things
> > might be broken from time to time. The fix for this was committed 10 days
> > after the first rework.
>
> Ok, thanks for the advice.
> To avoid such problems, is it possible to download "stable" releases? Are
> they tagged in the CVS repository?

Yep, but the NAND stuff is quite new and not always up to date in the stable 
versions.

You can subscribe on the CVS mailinglist, which informs you of all changes in 
the repository.
http://lists.infradead.org/mailman/listinfo/linux-mtd-cvs

-- 
Thomas
________________________________________________________________________
linutronix - competence in embedded & realtime linux
http://www.linutronix.de
mail: tglx@linutronix.de