NAND Flash support for Intel IXP4xx platform
Subsystems (2)
PATCH 1/1 NAND Flash support for Intel IXP4xx platform
The patch adds glue logic to support MTD for NAND flash devices on Intel IXP4xx based platform
-----
drivers/mtd/nand/Kconfig +0 -0
@@ -143,6 +143,13 @@ config MTD_NAND_S3C2410_CLKSTOP
when the is NAND chip selected or released, but will save approximately 5mA of power when there is nothing happening.
+config MTD_NAND_IXP4XX
+ tristate "Support for NAND Flash on Intel IXP4XX based systems"
+ depends on MTD_NAND && ARCH_IXP4XX
+ help
+ This enable the use of NAND flash on Intel IXP400 based platforms.
+ Currently this is only supported on KIXRP435 platform.
+
config MTD_NAND_DISKONCHIP
tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation) (EXPERIMENTAL)" depends on MTD_NAND && EXPERIMENTAL
drivers/mtd/nand/Makefile +0 -0
@@ -25,6 +25,7 @@ obj-$(CONFIG_MTD_NAND_CS553X) += cs553x
obj-$(CONFIG_MTD_NAND_NDFC) += ndfc.o
obj-$(CONFIG_MTD_NAND_AT91) += at91_nand.o
obj-$(CONFIG_MTD_NAND_BASLER_EXCITE) += excite_nandflash.o
+obj-$(CONFIG_MTD_NAND_IXP4XX) += ixp4xx_nand.o
nand-objs := nand_base.o nand_bbt.o
cafe_nand-objs := cafe.o cafe_ecc.o
drivers/mtd/nand/ixp4xx_nand.c new file +221 -0
221 lines not on this page · raw message
drivers/mtd/nand/ixp4xx_nand.c line 37
Quoted 5 lines · Ruslan V. Sushko
drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+static struct ixp4xx_faddr_t { +int offset; +void (*chip_select)(unsigned int ctrl); +} ixp4xx_faddr_info = {0 ,NULL };
ixp4xx_faddr_t is a nice name for a structure ;)
What the initialization to {0, NULL} is for?
Also, if this is a per-chip thing as might be concluded by placing its pointer into this->priv, why do you make it static structure? Same goes for
drivers/mtd/nand/ixp4xx_nand.c line 54
Quoted 7 lines
drivers/mtd/nand/ixp4xx_nand.c new file +5
-0+static struct mtd_info *ixp4xx_mtd =NULL ; +static struct mtd_partition *ixp4xx_nand_parts =NULL ; + +for ( i =0 ; i < len ; i++ ) +writeb (buf[i], this->IO_ADDR_W + addr_info->offset);
Wrong whitespacing.
Quoted 8 lines
drivers/mtd/nand/ixp4xx_nand.c new file +7
-0+} + +static void +ixp4xx_hwcontrol (struct mtd_info *mtd,int cmd,unsigned int ctrl) +{ +struct nand_chip *this = mtd->priv; +struct ixp4xx_faddr_t *addr_info = this->priv;
drivers/mtd/nand/ixp4xx_nand.c line 71
Quoted 10 lines
drivers/mtd/nand/ixp4xx_nand.c new file +10
-0+if (ctrl &NAND_CTRL_CHANGE ) { + addr_info->offset = (ctrl &NAND_CLE ) ?1 :0 ; + addr_info->offset |= (ctrl &NAND_ALE ) ?2 :0 ; +if (addr_info->chip_select) + addr_info->chip_select (ctrl); + } + +if (cmd !=NAND_CMD_NONE ) +writeb (cmd, this->IO_ADDR_W + addr_info->offset); +}
Wait please, can you explain the logic here?
It looks like you'll be writing by ixp_write_buf to different base addresses depending on how ctrl was set. Is this the expected behavior?
drivers/mtd/nand/ixp4xx_nand.c line 147
Quoted 2 lines
drivers/mtd/nand/ixp4xx_nand.c new file +1
-0+if ( nb_of_parts <=0 ) {/* No partition from parsing, use default */
Wrong whitespacing.
drivers/mtd/nand/ixp4xx_nand.c line 158
Quoted 12 lines
drivers/mtd/nand/ixp4xx_nand.c new file +11
-0+printk (KERN_INFO "Loading default partition table\n" ); + ixp4xx_nand_parts = plat->parts; + nb_of_parts = plat->nr_parts; + } + +/* Register the partitions */ + err =add_mtd_partitions (ixp4xx_mtd, ixp4xx_nand_parts, nb_of_parts); +if (err) { +printk (KERN_ERR "Failed to add MTD partitions\n" ); +if ( ixp4xx_nand_parts != plat->parts ) +kfree (ixp4xx_nand_parts);
Wrong indentation.
Moreover, if the number of partitions obtained by parse_mtd_partitions is the same as the one provided by platform data, you'll get leakage here.
Vitaly
Quoted 8 lines · Vitaly Wool
drivers/mtd/nand/ixp4xx_nand.c line 37
Quoted 5 lines · Ruslan V. Sushko
drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+static struct ixp4xx_faddr_t { +int offset; +void (*chip_select)(unsigned int ctrl); +} ixp4xx_faddr_info = {0 ,NULL };
ixp4xx_faddr_tis a nice name for a structure ;)
What the initialization to {0, NULL} is for?
just through habit. What's wrong here?
Quoted 10 lines
Also, if this is a per-chip thing as might be concluded by placing its pointer into
this->priv, why do you make it static structure? Same goes fordrivers/mtd/nand/ixp4xx_nand.c line 54
Quoted 6 lines
drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+static struct mtd_info *ixp4xx_mtd =NULL ; +static struct mtd_partition *ixp4xx_nand_parts =NULL ; + + for ( i = 0 ; i < len ; i++ ) +writeb (buf[i], this->IO_ADDR_W + addr_info->offset);Wrong whitespacing.
Will fixed
Quoted 13 lines
Quoted 13 lines
drivers/mtd/nand/ixp4xx_nand.c new file +13
-0+} + +static void +ixp4xx_hwcontrol (struct mtd_info *mtd,int cmd,unsigned int ctrl) +{ +struct nand_chip *this = mtd->priv; +struct ixp4xx_faddr_t *addr_info = this->priv; +if (ctrl &NAND_CTRL_CHANGE ) { + addr_info->offset = (ctrl &NAND_CLE ) ?1 :0 ; + addr_info->offset |= (ctrl &NAND_ALE ) ?2 :0 ; +if (addr_info->chip_select) + addr_info->chip_select (ctrl); + }
Quoted 8 lines
drivers/mtd/nand/ixp4xx_nand.c line 71
Quoted 5 lines
drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+ +if (cmd !=NAND_CMD_NONE ) +writeb (cmd, this->IO_ADDR_W + addr_info->offset); +}Wait please, can you explain the logic here?
It looks like you'll be writing byixp_write_bufto different base addresses depending on how ctrl was set. Is this the expected behavior?
Yes it is. Two low bits of address bus is connected to ALE/CLE signals. Generally it not necessary for wrietbuff routine, but it will not be error on this level, by the way I'll remove it. The hwcontrol must work in this way.
Quoted 5 lines
Quoted 4 lines
+ if (
nb_of_parts<= 0 ) { /* No partition from parsing, use
default */Wrong whitespacing.
will fix
Quoted 13 lines
drivers/mtd/nand/ixp4xx_nand.c line 158
Quoted 13 lines
drivers/mtd/nand/ixp4xx_nand.c new file +7
-0+printk (KERN_INFO "Loading default partition table\n" ); + ixp4xx_nand_parts = plat->parts; + nb_of_parts = plat->nr_parts; + } + +/* Register the partitions */ + err = add_mtd_partitions(ixp4xx_mtd, ixp4xx_nand_parts,
nb_of_parts);drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+if (err) { +printk (KERN_ERR "Failed to add MTD partitions\n" ); +if ( ixp4xx_nand_parts != plat->parts ) +kfree (ixp4xx_nand_parts);
Quoted 2 lines
Wrong indentation.
Moreover, if the number of partitions obtained byparse_mtd_partitionsis the same as the one provided by platform data, you'll get leakage here.
how this can happens? Please describe.
Quoted 3 lines
Vitaly
Best reagards,
Ruslan Sushko
Quoted 2 lines
Quoted 1 line
What the initialization to {0, NULL} is for?
just through habit. What's wrong here?
It's just useless.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/mtd/nand/ixp4xx_nand.c new file +3
-0+ +/* Register the partitions */ + err = add_mtd_partitions(ixp4xx_mtd, ixp4xx_nand_parts,
nb_of_parts);drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+if (err) { +printk (KERN_ERR "Failed to add MTD partitions\n" ); +if ( ixp4xx_nand_parts != plat->parts ) +kfree (ixp4xx_nand_parts);
Quoted 5 lines
Quoted 4 lines
Wrong indentation.
Moreover, if the number of partitions obtained byparse_mtd_partitionsis the same as the one provided by platform data, you'll get leakage here.how this can happens? Please describe.
Suppose that the number of partitions obtained by parse_mtd_partitions is the same as configured statically. If add_mtd_partitions fails, ixp4xx_nand_parts != plat->parts is false and kfree won't be called.
Vitaly
Quoted 13 lines · Vitaly Wool
Quoted 13 lines
Quoted 13 lines
drivers/mtd/nand/ixp4xx_nand.c line 158
Quoted 9 lines
drivers/mtd/nand/ixp4xx_nand.c new file +3
-0+ +/* Register the partitions */ + err = add_mtd_partitions(ixp4xx_mtd, ixp4xx_nand_parts,
nb_of_parts);drivers/mtd/nand/ixp4xx_nand.c new file +4
-0+if (err) { +printk (KERN_ERR "Failed to add MTD partitions\n" ); +if ( ixp4xx_nand_parts != plat->parts ) +kfree (ixp4xx_nand_parts);Wrong indentation.
Moreover, if the number of partitions obtained byparse_mtd_partitionsis the same as the one provided by platform data, you'll get leakage here.
Quoted 7 lines
Quoted 1 line
how this can happens? Please describe.
Suppose that the number of partitions obtained by
parse_mtd_partitionsis the same as configured statically. Ifadd_mtd_partitionsfails,ixp4xx_nand_parts!=plat->partsis false and kfree won't be called.Vitaly
It's impossible because ixp4xx_nand_parts and plat->parts hold pointers to mtd_partions structure, not number of partions. Number of partitions held in nb_of_parts variable.
Thank you,
Ruslan
Quoted 6 lines · Ruslan V. Sushko
Quoted 3 lines
Suppose that the number of partitions obtained by
parse_mtd_partitionsis the same as configured statically. Ifadd_mtd_partitionsfails,ixp4xx_nand_parts!=plat->partsis false and kfree won't be called.It's impossible because
ixp4xx_nand_partsandplat->partshold pointers tomtd_partionsstructure, not number of partions. Number of partitions held innb_of_partsvariable.
Oh shoot, right, thanks!
Vitaly
The patch adds glue logic to support MTD for NAND flash devices on Intel IXP4xx based platform The patch made against the 2.6.21-rc4 kernel
Updated after Vitaly's comments
drivers/mtd/nand/Kconfig +7 -0
@@ -143,6 +143,13 @@ config MTD_NAND_S3C2410_CLKSTOP when the is NAND chip selected or released, but will save approximately 5mA of power when there is nothing happening. +config MTD_NAND_IXP4XX + tristate "Support for NAND Flash on Intel IXP4XX based systems" + depends on MTD_NAND && ARCH_IXP4XX + help + This enable the use of NAND flash on Intel IXP400 based platforms. + Currently this is only supported on KIXRP435 platform. +
config MTD_NAND_DISKONCHIP
tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND
reimplementation) (EXPERIMENTAL)"
depends on MTD_NAND && EXPERIMENTAL
drivers/mtd/nand/Makefile +0 -0
@@ -25,6 +25,7 @@ obj-$(CONFIG_MTD_NAND_CS553X) += cs553x
obj-$(CONFIG_MTD_NAND_NDFC) += ndfc.o
obj-$(CONFIG_MTD_NAND_AT91) += at91_nand.o
obj-$(CONFIG_MTD_NAND_BASLER_EXCITE) += excite_nandflash.o
+obj-$(CONFIG_MTD_NAND_IXP4XX) += ixp4xx_nand.o
nand-objs := nand_base.o nand_bbt.o
cafe_nand-objs := cafe.o cafe_ecc.o
drivers/mtd/nand/ixp4xx_nand.c new file +221 -0
221 lines not on this page · raw message
Quoted 2 lines · Ruslan V. Sushko
The patch adds glue logic to support MTD for NAND flash devices on Intel IXP4xx based platform
Hmm, the IXP4xx doesn't have an on-chip NAND controller, does it? Is this for NAND flashes connected to the expansion bus? Maybe we need something a la physmap for NAND flashes..
Quoted 11 lines · Lennert Buytenhek
Quoted 3 lines · Ruslan V. Sushko
The patch adds glue logic to support MTD for NAND flash devices on Intel IXP4xx based platform
Hmm, the IXP4xx doesn't have an on-chip NAND controller, does it? Is this for NAND flashes connected to the expansion bus? Maybe we need something a la physmap for NAND flashes..
Yes, It's true the NAND flash is connected to expansion bus. The driver has two parts: The common glue logic submitted here and the platform specific (evaluation board) definition submitted to http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4323/2
All maps defined in last patch
Quoted 7 lines · Ruslan V. Sushko
Quoted 6 lines
Quoted 2 lines
The patch adds glue logic to support MTD for NAND flash devices on Intel IXP4xx based platform
Hmm, the IXP4xx doesn't have an on-chip NAND controller, does it? Is this for NAND flashes connected to the expansion bus? Maybe we need something a la physmap for NAND flashes..
Quoted 5 lines
Yes, It's true the NAND flash is connected to expansion bus. The driver has two parts: The common glue logic submitted here and the platform specific (evaluation board) definition submitted to
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4323/2
All maps defined in last patch
I wonder how much sense it makes to call this mtd driver "the ixp4xx NAND flash driver" if there's hardly anything ixp4xx-specific in it.
For that matter, I wonder how much sense it makes to submit a separate NAND driver for every ARM SoC. For NOR flashes we have physmap, we should have something similar for NAND.
Quoted 7 lines · Lennert Buytenhek
I wonder how much sense it makes to call this mtd driver "the ixp4xx NAND flash driver" if there's hardly anything ixp4xx-specific in it.
For that matter, I wonder how much sense it makes to submit a separate NAND driver for every ARM SoC. For NOR flashes we have physmap, we should have something similar for NAND.
It won't do for all the cases, but for the drivers where only cmd_ctrl differs that might really make sense.
Vitaly
Quoted 10 lines · Vitaly Wool
Quoted 7 lines
I wonder how much sense it makes to call this mtd driver "the ixp4xx NAND flash driver" if there's hardly anything ixp4xx-specific in it.
For that matter, I wonder how much sense it makes to submit a separate NAND driver for every ARM SoC. For NOR flashes we have physmap, we should have something similar for NAND.
It won't do for all the cases, but for the drivers where only
cmd_ctrldiffers that might really make sense.
Yes, exactly. For a unified NAND driver, you can just assume that the data register will be memory-mapped. This is probably true in 99% of the cases. In cases where this isn't true, people can still write their own driver.
Quoted 5 lines · Lennert Buytenhek
Yes, exactly. For a unified NAND driver, you can just assume that the data register will be memory-mapped. This is probably true in 99% of the cases. In cases where this isn't true, people can still write their own driver.
Seem it should be ok for most drivers. In this case we need to drive Chip Select signal "manually" through(Not all signal connected to expansion bus). At the same time different vendor make create CS in different way.
Quoted 11 lines · Lennert Buytenhek
Quoted 6 lines
Quoted 3 lines
For that matter, I wonder how much sense it makes to submit a separate NAND driver for every ARM SoC. For NOR flashes we have physmap, we should have something similar for NAND.
It won't do for all the cases, but for the drivers where only
cmd_ctrldiffers that might really make sense.Yes, exactly. For a unified NAND driver, you can just assume that the data register will be memory-mapped. This is probably true in 99% of the cases. In cases where this isn't true, people can still write their own driver.
Yes, but the "write your own" case will also be applicable to the cases where the controller has HW ECC capabilities, for instance.
Vitaly