RE: [PATCH 1/2]Intel Moorestown NAND driver patch for mainline

[Gao, Yunpeng]

>-----Original Message-----
>From: Jiri Slaby [mailto:jirislaby@xxxxxxxxx]
>Sent: 2009年6月6日 19:12
>To: Gao, Yunpeng
>Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH 1/2]Intel Moorestown NAND driver patch for mainline
>
>On 06/06/2009 06:44 PM, Yunpeng Gao wrote:
>> diff --git a/drivers/staging/mrst_nand/ffsport.c
>b/drivers/staging/mrst_nand/ffsport.c
>> new file mode 100644
>> index 0000000..5a919f4
>> --- /dev/null
>> +++ b/drivers/staging/mrst_nand/ffsport.c
>> @@ -0,0 +1,953 @@
>> +void glob_udelay(unsigned long usecs)
>> +{
>> +	udelay(usecs);
>> +}
>> +
>> +void glob_mdelay(unsigned long msecs)
>> +{
>> +	mdelay(msecs);
>> +}
>
>Unneeded wrappers.
>
>> +u32 *GLOB_MEMMAP_NOCACHE(unsigned long addr, unsigned long size)
>> +{
>> +#if (FLASH_NAND || FLASH_CDMA)
>> +    return (u32 *)ioremap_nocache(addr, (size+0xfff)&(~0xfff));
>
>Use ALIGN() and no cast.
>
>> +#else
>> +    return (u32 *)addr;
>> +#endif
>> +}
>> +
>> +u32 *GLOB_MEMMAP_TOBUS(u32 *ptr)
>> +{
>> +#if (FLASH_NAND || FLASH_CDMA)
>> +    return (u32 *)virt_to_bus(ptr);
>
>Aiee, virt_to_bus.
>
>> +#else
>> +    return ptr;
>> +#endif
>> +}
>...
>> +#define GLOB_SBD_IOCTL_GC
>(0x7701)
>> +#define GLOB_SBD_IOCTL_WL
>(0x7702)
>> +#define GLOB_SBD_IOCTL_FORMAT
>(0x7703)
>> +#define GLOB_SBD_IOCTL_ERASE_FLASH                             (0x7704)
>> +#define GLOB_SBD_IOCTL_FLUSH_CACHE                             (0x7705)
>> +#define GLOB_SBD_IOCTL_COPY_BLK_TABLE                       (0x7706)
>> +#define GLOB_SBD_IOCTL_COPY_WEAR_LEVELING_TABLE  (0x7707)
>> +#define GLOB_SBD_IOCTL_GET_NAND_INFO                         (0x7708)
>> +#define GLOB_SBD_IOCTL_WRITE_DATA                               (0x7709)
>> +#define GLOB_SBD_IOCTL_READ_DATA                             (0x770A)
>
>Could this be defined in an ioctl-standard way?
>
>> +static int GLOB_SBD_majornum;
>> +
>> +static char *GLOB_version = GLOB_VERSION;
>
>You will save a pointer by char [] instead of char *. Also constify it.
>
>> +/* Because the driver will allocate a lot of memory and kmalloc can not */
>> +/* allocat memory more than 4M bytes, here we use static array as */
>> +/* memory pool. This is simple but ugly. It should only be used during */
>> +/* development.*/
>> +#define LOCAL_MEM_POOL_SIZE        (1024 * 1024 * 8)
>> +static u8 local_mem_pool[LOCAL_MEM_POOL_SIZE];
>
>Bah. We have vmalloc.
>
>> +struct ioctl_rw_page_info {
>> +	u8 *data;
>> +	unsigned int page;
>> +};
>
>32/64-bit incompatible.
>
>> +static struct block_device_operations GLOB_SBD_ops = {
>> +	.owner = THIS_MODULE,
>> +	.open = GLOB_SBD_open,
>> +	.release = GLOB_SBD_release,
>> +	.locked_ioctl = GLOB_SBD_ioctl,
>
>Could this be switched to an unlocked one first?
>
>> +	.getgeo = GLOB_SBD_getgeo,
>> +};
>> +
>> +static int SBD_setup_device(struct spectra_nand_dev *dev, int which)
>> +{
>> +	int res_blks;
>> +	u32 sects;
>> +
>> +	nand_dbg_print(NAND_DBG_TRACE, "%s, Line %d, Function: %s\n",
>> +		       __FILE__, __LINE__, __func__);
>> +
>> +	memset(dev, 0, sizeof(struct spectra_nand_dev));
>> +
>> +	nand_dbg_print(NAND_DBG_WARN, "Reserved %d blocks "
>> +		"for OS image, %d blocks for bad block replacement.\n",
>> +		get_res_blk_num_os(),
>> +		get_res_blk_num_bad_blk());
>> +
>> +	res_blks = get_res_blk_num_bad_blk() + get_res_blk_num_os();
>> +
>> +	dev->size = (u64)IdentifyDeviceData.PageDataSize *
>> +		IdentifyDeviceData.PagesPerBlock *
>> +		(IdentifyDeviceData.wDataBlockNum - res_blks);
>> +
>> +	res_blks_os = get_res_blk_num_os();
>> +
>> +	spin_lock_init(&dev->qlock);
>> +
>> +	dev->tmp_buf = kmalloc(IdentifyDeviceData.PageDataSize, GFP_ATOMIC);
>> +	if (!dev->tmp_buf) {
>> +		printk(KERN_ERR "Failed to kmalloc memory in %s Line %d, exit.\n",
>> +			__FILE__, __LINE__);
>> +		goto out_vfree;
>> +	}
>> +
>> +	dev->queue = blk_init_queue(GLOB_SBD_request, &dev->qlock);
>> +	if (dev->queue == NULL) {
>> +		printk(KERN_ERR
>> +		       "Spectra: Request queue could not be initialized."
>> +			" Aborting\n ");
>
>memleak? and further in that function.
>
>> +		goto out_vfree;
>
>> diff --git a/drivers/staging/mrst_nand/lld_cdma.c
>b/drivers/staging/mrst_nand/lld_cdma.c
>> new file mode 100644
>> index 0000000..19650c2
>> --- /dev/null
>> +++ b/drivers/staging/mrst_nand/lld_cdma.c
>> @@ -0,0 +1,2736 @@
>...
>> +static u16 do_ecc_for_desc(u16 c, u8 *buf,
>> +				u16 page)
>> +{
>> +	u16 event = EVENT_NONE;
>> +	u16 err_byte;
>> +	u8 err_sector;
>> +	u8 err_page = 0;
>> +	u8 err_device;
>> +	u16 ecc_correction_info;
>> +	u16 err_address;
>> +	u32 eccSectorSize;
>> +	u8 *err_pos;
>> +
>> +	eccSectorSize   = ECC_SECTOR_SIZE * (DeviceInfo.wDevicesConnected);
>> +
>> +	do {
>> +		if (0 == c)
>> +			err_page = ioread32(FlashReg + ERR_PAGE_ADDR0);
>
>FlashReg doesn't look like an iomap memory. Use readl/writel all over
>the driver.
>
>> +static int check_for_ording(struct pending_cmd (*p)[MAX_CHANS + MAX_DESCS],
>> +	int *chis, int *chMaxIndexes, int (*namb)[MAX_CHANS])
>> +{
>> +	int i, done, ch1, ch2, syncNum, syncNum2;
>> +	u8 indexchgd;
>> +
>> +	indexchgd = 0;
>> +	for (i = 0; (i < totalUsedBanks) && !done && !indexchgd; i++) {
>> +		if (!EOLIST(i) &&
>> +			get_sync_ch_pcmd(p, i, chis[i], &syncNum, &ch1)) {
>> +			debug_boundary_error(ch1, totalUsedBanks, 0);
>> +			if (!EOLIST(ch1) && get_sync_ch_pcmd(p, ch1,
>> +					chis[ch1], &syncNum2, &ch2)) {
>> +				debug_boundary_error(ch2, totalUsedBanks, 0);
>> +				if ((syncNum == syncNum2) &&
>> +					(syncNum != FORCED_ORDERED_SYNC)) {
>> +					if (ch2 != i) {
>> +						nand_dbg_print(NAND_DBG_DEBUG,
>> +						"SYNCCHECK: ILLEGAL CASE: "
>> +						"Channel %d, cmdindx %d, "
>> +						"tag %d is waiting for "
>> +						"sync number %d "
>> +						"from channel %d, "
>> +						"which is waiting for "
>> +						"the same sync number "
>> +						"from channel %d. "
>> +						"Sync checking is aborting\n",
>> +						i, chis[i],
>> +						p[i][chis[i]].Tag,
>> +						syncNum, ch1, ch2);
>> +						done = 1;
>> +					} else {
>> +						if (!(debug_sync_cnt %
>> +							DBG_SNC_PRINTEVERY)) {
>> +							nand_dbg_print(
>> +							NAND_DBG_DEBUG,
>> +							"SYNCCHECK: "
>> +							"syncnum %d "
>> +							"betn Ch %d, "
>> +							"cmdindx %d, "
>> +							"tag %d & Ch %d, "
>> +							"cmdindx %d, tag %d. "
>> +							"chis="
>> +							"{%d, %d, %d, %d}\n",
>> +							syncNum, i,
>> +							chis[i],
>> +							p[i][chis[i]].Tag,
>> +							ch1, chis[ch1],
>> +							p[ch1][chis[ch1]].Tag,
>> +							chis[0], chis[1],
>> +							chis[2], chis[3]);
>> +						}
>> +						if (!check_ordering(p, i,
>> +							namb[ch1][i]+1,
>> +							chis[i], ch1,
>> +							namb[i][ch1]+1,
>> +							chis[ch1]))
>> +							nand_dbg_print(
>> +							NAND_DBG_DEBUG,
>> +							"Above problem "
>> +							"occured when "
>> +							"analyzing "
>> +							"sync %d "
>> +							"between "
>> +							"(ch:%d, indx:%d, "
>> +							"tag:%d) & "
>> +							"(ch:%d, indx:%d, "
>> +							"tag:%d)\n",
>> +							syncNum, i, chis[i],
>> +							p[i][chis[i]].Tag,
>> +							ch1, chis[ch1],
>> +							p[ch1][chis[ch1]].Tag);
>
>Hmm, pretty unreadable. You seem to need to refactor the code.
>
>> diff --git a/drivers/staging/mrst_nand/lld_nand.c
>b/drivers/staging/mrst_nand/lld_nand.c
>> new file mode 100644
>> index 0000000..56ef843
>> --- /dev/null
>> +++ b/drivers/staging/mrst_nand/lld_nand.c
>> @@ -0,0 +1,3113 @@
>...
>> +#define CEIL_DIV(X, Y) (((X)%(Y)) ? ((X)/(Y)+1) : ((X)/(Y)))
>
>Is this DIV_ROUND_UP?
>
>> +u16 conf_parameters[] = {
>
>const?
>
>> +static u16 get_onfi_nand_para(void)
>> +{
>> +	int i;
>> +	u16 blks_lun_l, blks_lun_h, n_of_luns;
>> +	u32 blockperlun, id;
>> +
>> +	iowrite32(DEVICE_RESET__BANK0, FlashReg + DEVICE_RESET);
>> +
>> +	while (!((ioread32(FlashReg + INTR_STATUS0) &
>> +		INTR_STATUS0__RST_COMP) |
>> +		(ioread32(FlashReg + INTR_STATUS0) &
>> +		INTR_STATUS0__TIME_OUT)))
>> +		;
>
>cpu_relax(); And the same further in this function.
>
>> +static const struct pci_device_id nand_pci_ids[] = {
>> +	{
>> +	 .vendor = 0x8086,
>> +	 .device = 0x0809,
>> +	 .subvendor = PCI_ANY_ID,
>> +	 .subdevice = PCI_ANY_ID,
>
>PCI_DEVICE()
>
>> +	 },
>> +	{ /* end: all zeroes */ }
>> +};
>> +
>> +static int dump_pci_config_register(struct pci_dev *dev)
>
>What is this good for?
>
>> +static struct pci_driver nand_pci_driver = {
>
>const
>
>> +	.name = SPECTRA_NAND_NAME,
>> +	.id_table = nand_pci_ids,
>> +	.probe = nand_pci_probe,
>> +	.remove = nand_pci_remove,
>> +};
>> +
>> +u16 NAND_Flash_Init(void)
>> +{
>...
>> +	FlashReg = GLOB_MEMMAP_NOCACHE(GLOB_HWCTL_REG_BASE,
>> +			GLOB_HWCTL_REG_SIZE);
>> +	if (!FlashReg) {
>> +		printk(KERN_ERR "Spectra: ioremap_nocache failed!");
>> +		return -ENOMEM;
>> +	}
>> +	nand_dbg_print(NAND_DBG_WARN,
>> +		"Spectra: Remapped reg base address: "
>> +		"0x%p, len: %d\n",
>> +		FlashReg, GLOB_HWCTL_REG_SIZE);
>> +
>> +	FlashMem = GLOB_MEMMAP_NOCACHE(GLOB_HWCTL_MEM_BASE,
>> +			GLOB_HWCTL_MEM_SIZE);
>> +	if (!FlashMem) {
>
>unmap?
>
>> +		printk(KERN_ERR "Spectra: ioremap_nocache failed!");
>> +		return -ENOMEM;
>> +	}
>...
>> +	/* Enable the 2 lines code will enable pipeline_rw_ahead feature */
>> +	/* and improve performance for about 10%. But will also cause a */
>> +	/* 1 or 2 bit error when do a 300MB+ file copy/compare testing. */
>> +	/* Suspect it's an ECC FIFO overflow issue. -- Yunpeng 2009.03.26 */
>> +	/* iowrite32(1, FlashReg + CACHE_WRITE_ENABLE); */
>> +	/* iowrite32(1, FlashReg + CACHE_READ_ENABLE); */
>> +
>> +	retval = pci_register_driver(&nand_pci_driver);
>> +	if (retval)
>
>unmap?
>
>> +		return -ENOMEM;
>> +
>> +	return PASS;
>> +}
>> +
>> +#endif
>
>HTH.

Thank you very much for the review. All the comments are very helpful to me. As this driver is still under development (some functional feature is not implemented yet), I'll address these comments after enable some new features and then submit new patches. 

Thanks.

Rgds,
Yunpeng
韬{.n?????%?lzwm?b?Р骒r?zXЩ??{ay????j?f＂?????ア?⒎?:+v???????赙zZ+????"?!?O???v??m?鹈n?帼Y&—