[PATCH]hxd8-nand-chip-select.patch

Ben Dooks ben at trinity.fluff.org
Fri May 18 16:52:51 CEST 2007


On Fri, May 18, 2007 at 03:54:14PM +0800, Eric Chen wrote:
> Log:
> This nand chip-select patch for HXD8, support up to 4G nand flash.

Index: linux-2.6.21/include/asm-arm/arch-s3c2440/hxd8.h
===================================================================
--- linux-2.6.21.orig/include/asm-arm/arch-s3c2440/hxd8.h	2007-05-18 13:32:17.000000000 +0800
+++ linux-2.6.21/include/asm-arm/arch-s3c2440/hxd8.h	2007-05-18 13:41:40.000000000 +0800
@@ -12,5 +12,5 @@
 #define HXD8_GPIO_PCF50606	S3C2410_GPF6
 
 #define HXD8_IRQ_PCF50606	IRQ_EINT6
-
+#define HXD8_NAND_CHIP_SELECT	1
 #endif
Index: linux-2.6.21/arch/arm/mach-s3c2440/mach-hxd8.c
===================================================================
--- linux-2.6.21.orig/arch/arm/mach-s3c2440/mach-hxd8.c	2007-05-18 13:32:17.000000000 +0800
+++ linux-2.6.21/arch/arm/mach-s3c2440/mach-hxd8.c	2007-05-18 13:49:44.000000000 +0800
@@ -74,6 +74,12 @@
 #include <asm/plat-s3c24xx/devs.h>
 #include <asm/plat-s3c24xx/cpu.h>
 #include <asm/plat-s3c24xx/pm.h>
+#include "asm-arm/delay.h"

You should use <asm/delay.h> to include this file, but I belive
the correct thing to do is to #include <linux/delay.h> which
includes <asm/delay.h> and ensures all sleep functions are defined

+#if	HXD8_NAND_CHIP_SELECT
+void	hxd8_select_chip(struct s3c2410_nand_set *,	int chip);

this really should be declared static, you don't want to be exporting
these.

+void hxd8_nand_chip_select_init();

the above point, plus the decleration is not complete without a
void in the arguments.
+#endif


 
 static struct map_desc hxd8_iodesc[] __initdata = {
 	/* ISA IO Space map (memory space selected by A24) */
@@ -133,14 +139,17 @@
 	[0] = {
 		.name		= "hxd8-nand",
 		.nr_chips	= 1,
+		.working_index = 0,
 	},
 	[1] = {
 		.name		= "hxd8-nand-1",
 		.nr_chips	= 1,
+		.working_index = 1,
 	},
 	[2] = {
 		.name		= "hxd8-nand-2",
 		.nr_chips	= 1,
+		.working_index = 2,
 	},
 };
 
@@ -154,6 +163,9 @@
 	.twrph1		= 20,
 	.nr_sets	= ARRAY_SIZE(hxd8_nand_sets),
 	.sets		= hxd8_nand_sets,
+#if	HXD8_NAND_CHIP_SELECT
+	.select_chip  = hxd8_select_chip,
+#endif	
 };

If you'd declared the code above, you could have done

#if HXD8_NAND_CHIP_SELECT
static void hxd8_select_chip(..args..)
{
	code here;
}

static void hxd8_nand_chip_select_init(void)
{
	code here;
}
#else
#define hxd8_select_chip NULL
#define hxd8_nand_chip_select_init() do { } while(0)
#endif

and then you'd not need the #if around .select_chip

 
 /* PMU configuration */
@@ -386,8 +398,78 @@
 	platform_device_register(&hxd8_pmu_dev);
 
 	s3c2410_pm_init();
+#if HXD8_NAND_CHIP_SELECT
+	hxd8_nand_chip_select_init();
+#endif	

+}
+#if	HXD8_NAND_CHIP_SELECT
+void hxd8_nand_chip_select_init()
+{
+	/*  Nand Flash 3G glue logic initialization
+	  *  nCE1 ~ nCE4 as output mode
+	  */
+	s3c2410_gpio_cfgpin(S3C2410_GPA14, S3C2410_GPA14_OUT);
+	s3c2410_gpio_cfgpin(S3C2410_GPA15, S3C2410_GPA15_OUT);
+	s3c2410_gpio_cfgpin(S3C2410_GPA16, S3C2410_GPA16_OUT);
+	s3c2410_gpio_setpin(S3C2410_GPA14, 1);
+	s3c2410_gpio_setpin(S3C2410_GPA15, 1);
+	s3c2410_gpio_setpin(S3C2410_GPA16, 1);
+	s3c2410_gpio_cfgpin(S3C2410_GPG1, S3C2410_GPG1_OUTP);
+	s3c2410_gpio_setpin(S3C2410_GPG1, 0);
+	/* RnB 2~4 as input mode */
+	s3c2410_gpio_cfgpin(S3C2410_GPC5, S3C2410_GPG5_INP);	
+	s3c2410_gpio_cfgpin(S3C2410_GPC6, S3C2410_GPG6_INP);	
+	s3c2410_gpio_cfgpin(S3C2410_GPC7, S3C2410_GPG7_INP);	
 }

If you'd put this above, you'd not have needed the
prototype in the first place. Also see notes about
removing #if statements.
 
+void	hxd8_select_chip(struct s3c2410_nand_set * nset,
+					       int chip)
+{
+
+	if (chip == -1){

space between the ) and { is usual.

+		s3c2410_gpio_setpin(S3C2410_GPG1, 0);//note here,

please put informative comments in here, this should either
be fleshed out, or removed entirely.

+		s3c2410_gpio_setpin(S3C2410_GPA14, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA15, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA16, 1);			
+		udelay(3);
+		return;
+	}
+
+	switch (nset->working_index){
+	case 0:
+		s3c2410_gpio_setpin(S3C2410_GPG1, 0);//note here,
+		s3c2410_gpio_setpin(S3C2410_GPA14, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA15, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA16, 1);			
+		udelay(3);
+		break;
+	case 1:
+		s3c2410_gpio_setpin(S3C2410_GPG1, 1);//note here,
+		s3c2410_gpio_setpin(S3C2410_GPA14, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA15, 0);
+		s3c2410_gpio_setpin(S3C2410_GPA16, 1);			
+		udelay(3);
+		break;	
+	case 2:
+		s3c2410_gpio_setpin(S3C2410_GPG1, 1);//note here,
+		s3c2410_gpio_setpin(S3C2410_GPA14, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA15, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA16, 0);			
+		udelay(3);
+		break;	
+	case 3:
+		s3c2410_gpio_setpin(S3C2410_GPG1, 1);//note here,
+		s3c2410_gpio_setpin(S3C2410_GPA14, 0);
+		s3c2410_gpio_setpin(S3C2410_GPA15, 1);
+		s3c2410_gpio_setpin(S3C2410_GPA16, 1);			
+		udelay(3);
+		break;	

another way of doing this which would have produced less code
is the following:

	int sel = (chip == -1) ? -1 : nset->working_index;

	BUG_ON(sel > 3 || sel < -1);

	s3c2410_gpio_setpin(S3C2410_GPG1, (sel == 0) ? 0 : 1);
	s3c2410_gpio_setpin(S3C2410_GPA15, (sel == 1) ? 0 : 1);
	s3c2410_gpio_setpin(S3C2410_GPA16, (sel == 2) ? 0 : 1);
	s3c2410_gpio_setpin(S3C2410_GPA14, (sel == 3) ? 0 : 1);

+	default:
+		printk("nand select error!");

BUG(); is also useful instead of a printk.
Note, the printk should have been printk(KERN_ERR "nand select error!");

+	}
+	
+}
+#endif
 MACHINE_START(HXD8, "HXD8")
 	/* Maintainer: Harald Welte <laforge at openmoko.org> */
 	.phys_io	= S3C2410_PA_UART,
Index: linux-2.6.21/include/asm-arm/arch-s3c2410/nand.h
===================================================================
--- linux-2.6.21.orig/include/asm-arm/arch-s3c2410/nand.h	2007-04-26 11:08:32.000000000 +0800
+++ linux-2.6.21/include/asm-arm/arch-s3c2410/nand.h	2007-05-18 13:55:02.000000000 +0800
@@ -20,13 +20,16 @@
  * nr_map	 = map for low-layer logical to physical chip numbers (option)
  * partitions	 = mtd partition list
 */
-
+#include	<asm-arm/arch-s3c2440/hxd8.h>
 struct s3c2410_nand_set {
 	int			nr_chips;
 	int			nr_partitions;
 	char			*name;
 	int			*nr_map;
 	struct mtd_partition	*partitions;
+#if HXD8_NAND_CHIP_SELECT
+	int			working_index;
+#endif
 };

configuration specific bits in platform code is never very good.

I belive working index could have removed by using the nr_chips
field.
 
 struct s3c2410_platform_nand {
Index: linux-2.6.21/drivers/mtd/nand/s3c2410.c
===================================================================
--- linux-2.6.21.orig/drivers/mtd/nand/s3c2410.c	2007-05-18 13:32:17.000000000 +0800
+++ linux-2.6.21/drivers/mtd/nand/s3c2410.c	2007-05-18 14:57:16.000000000 +0800
@@ -63,6 +63,7 @@
 #include <asm/arch/regs-nand.h>
 #include <asm/arch/nand.h>
 
+#include	<asm-arm/arch-s3c2440/hxd8.h>
 #ifdef CONFIG_MTD_NAND_S3C2410_HWECC
 static int hardware_ecc = 1;
 #else
@@ -254,6 +255,13 @@
 
 	if (chip == -1) {
 		cur |= info->sel_bit;
+#if HXD8_NAND_CHIP_SELECT
+		/* GPIO return to default when unselect chip*/
+		if (info->platform != NULL) {
+			if (info->platform->select_chip != NULL)
+				(info->platform->select_chip) (nmtd->set, chip);
+		}		
+#endif		
 	} else {
 		if (nmtd->set != NULL && chip > nmtd->set->nr_chips) {
 			dev_err(info->device, "invalid chip %d\n", chip);
@@ -323,9 +331,26 @@
 static int s3c2440_nand_devready(struct mtd_info *mtd)
 {
 	struct s3c2410_nand_info *info = s3c2410_nand_mtd_toinfo(mtd);
+	struct s3c2410_nand_mtd *nmtd = s3c2410_nand_mtd_toours(mtd);
+
+#if HXD8_NAND_CHIP_SELECT	
+	switch(nmtd->set->working_index){
+	case 0:
+		return readb(info->regs + S3C2440_NFSTAT) & S3C2440_NFSTAT_READY;
+	case 1:
+		return (s3c2410_gpio_getpin(S3C2410_GPC6)>>6)  & S3C2440_NFSTAT_READY;
+	case 2:
+		return (s3c2410_gpio_getpin(S3C2410_GPC7)>>7)  & S3C2440_NFSTAT_READY;
+	case 3:
+		return (s3c2410_gpio_getpin(S3C2410_GPC5)>>5)  & S3C2440_NFSTAT_READY;
+	default:
+		return readb(info->regs + S3C2440_NFSTAT) & S3C2440_NFSTAT_READY;
+	}
+#endif

Firstly, you didn't need S3C2440_NFSTAT_READY on this, it is either
zero for busy, or non-zero for ready, thus also making the shifts
redundant.

Secondly, why did the hardware engineers make 3 copies of what is
an open-collector signal, in a system where the nand controller
can only access one chip at a time? RnB should have been common
to all three chips.

I think we need to add some form of override for the
devready function passed through with the platform
data.

 	return readb(info->regs + S3C2440_NFSTAT) & S3C2440_NFSTAT_READY;
 }


-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.





More information about the openmoko-kernel mailing list