[RFC 2.6.13.1 1/1] Cleanup for CS5535 IDE driver

[RFC 2.6.13.1 1/1] Cleanup for CS5535 IDE driver

[jayakumar.ide]

Hi Bartlomiej, IDE folk,

Appended is my patch attempting to fix up the cs5535 driver as per 
your suggestions from the thread: 

http://marc.theaimsgroup.com/?l=linux-ide&m=112533052120330&w=2

I listed what I did with respect to that todo list below:

> * cs5535_tuneproc() needs to be fixed to not abuse ->tuneproc interface
>   (which is exported to user-space), all DMA tuning should be done through
>   ->speedproc interface only
Done.

> * cs5535_set_drive() needs to use ide_rate_filter() to limit values passed
>   from the user-space
Done. btw, thanks for the eighty93 explaination yesterday. I hope I 
got that part right in this.

> * cs5535_set_drive() needs to set drive's speed unconditionally otherwise
>   drive may not be setup correctly after resume
Done.

> * cs5535_set_drive() shouldn't change drive->current_speed
Done.

> * cable detection needs to be in separate function (for hotplug - later)
I'm not sure I followed. By cable detection, I think you are pointing
to init_hwif_cs5535:

	/* if a 80 wire cable was detected */
	pci_read_config_byte(hwif->pci_dev, CS5535_CABLE_DETECT, &bit);
	hwif->udma_four = bit & 1;

I think we can leave this as is because the cs5535 is a SOC companion that 
doesn't have hotplug capability. Is that okay?

> * driver lacks device side cable detection [ see eighty_ninty_three() ]
I put eighty93 into ratemask which gets called from set_drive. 

> * driver needs to check for blacklisted DMA devices [ see ide_use_dma() ]
Done. I added it in dma_check to match what the other drivers do.

> * init_hwif_cs5535() should be marked __devinit not __init
Since 5535 has no hotplug capability, I set all to __init. 

> * cs5535_ide_init() lacks __init tag
Done.

> * .name in driver struct should be "CS5535_IDE" (w/o space)
Done.

> * what taking ide_lock in init_chipset_cs5535() is supposed to protect?
Nothing as far as I can tell so I took it out.

> * it would be simpler to always set ->autotune and tune PIO instead of
>   checking for BIOS settings
Sounds logical. So I took out the reg reads and just set autotune.

> * use 'drive->hwif' instead of 'HWIF' macro
Done.

> * use 'u8' instead of 'byte' type
Done.

> * remove unneeded includes
Done.

> * proper coding style:
Done.

I did the patch against 2.6.13.1. fyi, the part adding 5535_IDE to pci_ids.h 
duplicates a prior alsa patch I did that is in akpm's -mm. I think I
should have made the pci_ids diff separate. 

I only tested with a single drive that turned out to only be capable of
MWDMA2 since that's what I have available right now. I'll try to borrow a
UDMA4 capable drive and 80c cable if I have a chance. 

 Model=QUANTUM FIREBALL_TM1280A, FwRev=A6B.2D00, SerialNo=692716821653
 Config={ HardSect NotMFM HdSw>15uSec Fixed DTR>5Mbs TrkOff }
 RawCHS=2484/16/63, TrkSize=32256, SectSize=512, ECCbytes=4
 BuffType=DualPortCache, BuffSize=76kB, MaxMultSect=16, MultSect=16
 CurCHS=2484/16/63, CurSects=2503872, LBA=yes, LBAsects=2503872
 IORDY=on/off, tPIO={min:300,w/IORDY:120}, tDMA={min:120,rec:120}
 PIO modes:  pio0 pio1 pio2 pio3 pio4
 DMA modes:  sdma0 sdma1 sdma2 mdma0 mdma1 *mdma2
 AdvancedPM=no

 * signifies the current active mode

 Timing buffer-cache reads:   668 MB in  2.01 seconds = 332.65 MB/sec
 Timing buffered disk reads:   20 MB in  3.06 seconds =   6.54 MB/sec
 
Please let me know how it looks and if you have any feedback.

Thanks,
Jaya Kumar

Signed-off-by: Jaya Kumar <jayakumar.ide@gmail.com>

---

 drivers/ide/Kconfig      |   10 +
 drivers/ide/pci/Makefile |    1 
 drivers/ide/pci/cs5535.c |  365 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci_ids.h  |    1 
 4 files changed, 377 insertions(+)
diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/drivers/ide/Kconfig linux-2.6.13.1-ide/drivers/ide/Kconfig
--- linux-2.6.13.1-vanilla/drivers/ide/Kconfig	2005-09-15 14:35:09.000000000 +0800
+++ linux-2.6.13.1-ide/drivers/ide/Kconfig	2005-09-23 08:56:41.000000000 +0800
@@ -539,6 +539,16 @@ config BLK_DEV_CS5530
 
 	  It is safe to say Y to this question.
 
+config BLK_DEV_CS5535
+	tristate "AMD CS5535 MediaGX chipset support"
+	depends on X86 && !X86_64
+	help
+	  Include support for UDMA on the Cyrix MediaGX 5535 chipset. This
+	  will automatically be detected and configured if found. This chip
+	  is also called NSC CS5535 geode companion in some literature.
+
+	  It is safe to say Y to this question.
+
 config BLK_DEV_HPT34X
 	tristate "HPT34X chipset support"
 	help
diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/drivers/ide/pci/cs5535.c linux-2.6.13.1-ide/drivers/ide/pci/cs5535.c
--- linux-2.6.13.1-vanilla/drivers/ide/pci/cs5535.c	1970-01-01 07:30:00.000000000 +0730
+++ linux-2.6.13.1-ide/drivers/ide/pci/cs5535.c	2005-09-23 10:41:39.000000000 +0800
@@ -0,0 +1,365 @@
+/*
+ * linux/drivers/ide/pci/cs5535.c       
+ *
+ * Copyright (C) 2003           Jens Altmann <jens.altmann@amd.com>
+ *
+ * History:
+ * 09/20/2005 - Jaya Kumar <jayakumar.ide@gmail.com>
+ * - Reworked tuneproc, set_drive, misc mods to prep for mainline 
+ * - Work was sponsored by CIS (M) Sdn Bhd.
+ * Ported to Kernel 2.6.11 on June 26, 2005 by
+ *   Wolfgang Zuleger <wolfgang.zuleger@gmx.de>
+ *   Alexander Kiausch <alex.kiausch@t-online.de>
+ *
+ * Development of this chipset driver was funded
+ * by the nice folks at National Semiconductor.
+ *
+ * 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.
+ *
+ * Documentation:
+ *  CS5535 documentation available from National Semiconductor.
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/ide.h>
+
+#include "ide-timing.h"
+
+#define MSR_ATAC_BASE    	0x51300000
+#define ATAC_GLD_MSR_CAP 	(MSR_ATAC_BASE+0)
+#define ATAC_GLD_MSR_CONFIG	(MSR_ATAC_BASE+0x01)
+#define ATAC_GLD_MSR_SMI	(MSR_ATAC_BASE+0x02)
+#define ATAC_GLD_MSR_ERROR	(MSR_ATAC_BASE+0x03)
+#define ATAC_GLD_MSR_PM		(MSR_ATAC_BASE+0x04)
+#define ATAC_GLD_MSR_DIAG	(MSR_ATAC_BASE+0x05)
+#define ATAC_IO_BAR		(MSR_ATAC_BASE+0x08)
+#define ATAC_RESET		(MSR_ATAC_BASE+0x10)
+#define ATAC_CH0D0_PIO		(MSR_ATAC_BASE+0x20)
+#define ATAC_CH0D0_DMA		(MSR_ATAC_BASE+0x21)
+#define ATAC_CH0D1_PIO		(MSR_ATAC_BASE+0x22)
+#define ATAC_CH0D1_DMA		(MSR_ATAC_BASE+0x23) 
+#define ATAC_PCI_ABRTERR	(MSR_ATAC_BASE+0x24)
+#define ATAC_BM0_CMD_PRIM	0x00
+#define ATAC_BM0_STS_PRIM	0x02
+#define ATAC_BM0_PRD		0x04
+#define CS5535_CABLE_DETECT	0x48
+
+//#define DEBUG
+
+#ifdef DEBUG
+#define DPRINTK(fmt,args...) printk("cs5535-ide: " fmt, ##args)
+#else
+#define DPRINTK(fmt,args...)
+#endif
+
+/* Format I PIO settings. We seperate out cmd and data for safer timings */
+
+static unsigned int cs5535_pio_cmd_timings[5] = 
+{ 0xF7F4, 0x53F3, 0x13F1, 0x5131, 0x1131 };
+static unsigned int cs5535_pio_dta_timings[5] = 
+{ 0xF7F4, 0xF173, 0x8141, 0x5131, 0x1131 };
+
+static unsigned int cs5535_mwdma_timings[3] = 
+{ 0x7F0FFFF3, 0x7F035352, 0x7f024241 };
+
+static unsigned int cs5535_udma_timings[5] = 
+{ 0x7F7436A1, 0x7F733481, 0x7F723261, 0x7F713161, 0x7F703061 };
+
+/* Macros to check if the register is the reset value -  reset value is an
+   invalid timing and indicates the register has not been set previously */
+
+#define CS5535_BAD_PIO(timings) ( (timings&~0x80000000UL) == 0x00009172 )
+#define CS5535_BAD_DMA(timings) ( (timings & 0x000FFFFF) == 0x00077771 )
+  
+/**
+ *	cs5535_set_speed         -     Configure the chipset to the new speed
+ *	@drive: Drive to set up
+ *	@speed: desired speed
+ *
+ *	cs5535_set_speed() configures the chipset to a new speed.
+ */
+static void cs5535_set_speed(ide_drive_t *drive, u8 speed) 
+{
+	
+	u32 reg = 0, dummy;
+	int unit = drive->select.b.unit;
+
+	DPRINTK("cs5535_set_speed speed %x\n", speed);
+
+	/* Set the PIO timings */
+	
+	if ((speed & XFER_MODE) == XFER_PIO) {
+		u8 pioa; 
+		u8 piob;
+		u8 cmd; 
+
+		pioa = speed - XFER_PIO_0;
+		piob = ide_get_best_pio_mode(&(drive->hwif->drives[!unit]), 
+						255, 4, NULL);
+		cmd = pioa < piob ? pioa : piob;
+
+		/* Write the speed of the current drive */
+		reg = (cs5535_pio_cmd_timings[cmd] << 16) | 
+			cs5535_pio_dta_timings[pioa];	
+		wrmsr(unit ? ATAC_CH0D1_PIO : ATAC_CH0D0_PIO, reg, 0);
+		
+		/* And if nessesary - change the speed of the other drive */
+		rdmsr(unit ?  ATAC_CH0D0_PIO : ATAC_CH0D1_PIO, reg, dummy);    
+		
+		if (((reg >> 16) & cs5535_pio_cmd_timings[cmd]) != 
+			cs5535_pio_cmd_timings[cmd]) {
+			reg &= 0x0000FFFF;
+			reg |= cs5535_pio_cmd_timings[cmd] << 16;
+			wrmsr(unit ? ATAC_CH0D0_PIO : ATAC_CH0D1_PIO, reg, 0);
+		}
+
+		/* Set bit 31 of the DMA register for PIO format 1 timings */
+		rdmsr(unit ?  ATAC_CH0D1_DMA : ATAC_CH0D0_DMA, reg, dummy);    
+		wrmsr(unit ? ATAC_CH0D1_DMA : ATAC_CH0D0_DMA, 
+					reg | 0x80000000UL, 0);
+	} else {
+		rdmsr(unit ? ATAC_CH0D1_DMA : ATAC_CH0D0_DMA, reg, dummy);
+		
+		reg &= 0x80000000UL;  /* Preserve the PIO format bit */
+
+		if (speed >= XFER_UDMA_0 && speed <= XFER_UDMA_7) 
+			reg |= cs5535_udma_timings[speed - XFER_UDMA_0];
+		else if (speed >= XFER_MW_DMA_0 && speed <= XFER_MW_DMA_2) 
+			reg |= cs5535_mwdma_timings[speed - XFER_MW_DMA_0];
+		else 
+			return;
+		
+		wrmsr(unit ? ATAC_CH0D1_DMA : ATAC_CH0D0_DMA, reg, 0);
+	}
+}
+
+static u8 cs5535_ratemask(ide_drive_t *drive)
+{
+	/* eighty93 will return 1 if it's 80core and capable of
+	exceeding udma2, 0 otherwise. we need ratemask to set
+	the max speed and if we can > udma2 then we return 2
+	which selects speed_max as udma4 which is the 5535's max
+	speed, and 1 selects udma2 which is the max for 40c */
+	if (!eighty_ninty_three(drive))
+		return 1;
+
+	return 2;
+}
+
+
+/**
+ *	cs5535_set_drive         -     Configure the drive to the new speed
+ *	@drive: Drive to set up
+ *	@speed: desired speed
+ *
+ *	cs5535_set_drive() configures the drive and the chipset to a
+ *	new speed. It also can be called by upper layers.
+ */
+static int cs5535_set_drive(ide_drive_t *drive, u8 speed) 
+{
+	
+	DPRINTK("cs5535_set_drive speed %x\n", speed);
+
+	speed = ide_rate_filter(cs5535_ratemask(drive), speed);
+	ide_config_drive_speed(drive, speed);
+	cs5535_set_speed(drive, speed);	
+
+	return 0;
+}
+
+/**
+ *	cs5535_tuneproc    -       PIO setup
+ *	@drive: drive to set up
+ *	@pio: mode to use (255 for 'best possible')
+ *
+ *	A callback from the upper layers for PIO-only tuning.
+ */
+static void cs5535_tuneproc(ide_drive_t *drive, u8 xferspeed) 
+{ 	
+	u8 modes[] = { 	XFER_PIO_0, XFER_PIO_1, XFER_PIO_2, XFER_PIO_3,
+			XFER_PIO_4 };
+
+	/* cs5535 max pio is pio 4, best_pio will check the blacklist. 
+	i think we don't need to rate_filter the incoming xferspeed 
+	since we know we're only going to choose pio */
+	xferspeed = ide_get_best_pio_mode(drive, xferspeed, 4, NULL);
+	ide_config_drive_speed(drive, modes[xferspeed]);
+	cs5535_set_speed(drive, xferspeed);
+}
+
+#if defined(DEBUG)
+/**
+ *      init_chipset_cs5535  -       initialization handler
+ *      @dev: PCI device
+ *      @name: Name of interface
+ *
+ *      The initialization callback. We do not need it for this chipset.
+ *      It is only used for debugging purposes.
+ */
+static unsigned int __init init_chipset_cs5535(struct pci_dev *dev, 
+						const char *name)
+{
+	unsigned long flags;
+	u32 val1, val2;
+
+	/* all CPUs (there should only be one CPU with this chipset) */
+
+	rdmsr(ATAC_GLD_MSR_CAP, val1, val2);
+	DPRINTK("ATAC_GLD_MSR_CAP=0x%x\n", val1);
+	rdmsr(ATAC_GLD_MSR_CONFIG, val1, val2);
+	DPRINTK("ATAC_GLD_MSR_CONFIG=0x%x\n", val1);
+	rdmsr(ATAC_GLD_MSR_SMI, val1, val2);
+	DPRINTK("ATAC_GLD_MSR_SMI=0x%x\n", val1);
+	rdmsr(ATAC_GLD_MSR_ERROR, val1, val2);
+	DPRINTK("ATAC_GLD_MSR_ERROR=0x%x\n", val1);
+	rdmsr(ATAC_GLD_MSR_PM, val1, val2);
+	DPRINTK("ATAC_GLD_MSR_PM=0x%x\n", val1);
+	rdmsr(ATAC_GLD_MSR_DIAG, val1, val2);
+	DPRINTK("ATAC_GLD_MSR_DIAG=0x%x\n", val1);
+	rdmsr(ATAC_IO_BAR, val1, val2);
+	DPRINTK("ATAC_IO_BAR=0x%x.%x\n", val2, val1);
+	rdmsr(ATAC_RESET, val1, val2);
+	DPRINTK("ATAC_RESET=0x%x\n", val1);
+	rdmsr(ATAC_CH0D0_PIO, val1, val2);
+	DPRINTK("ATAC_CH0D0_PIO=0x%x\n", val1);
+	rdmsr(ATAC_CH0D0_DMA, val1, val2);
+	DPRINTK("ATAC_CH0D0_DMA=0x%x\n", val1);
+	rdmsr(ATAC_CH0D1_PIO, val1, val2);
+	DPRINTK("ATAC_CH0D1_PIO=0x%x\n", val1);
+	rdmsr(ATAC_CH0D1_DMA, val1, val2);
+	DPRINTK("ATAC_CH0D1_DMA=0x%x\n", val1);
+	rdmsr(ATAC_PCI_ABRTERR, val1, val2);
+	DPRINTK("ATAC_PCI_ABRTERR=0x%x\n", val1);
+
+	return 0;
+}
+#endif
+
+static int cs5535_config_drive_for_dma(ide_drive_t *drive)
+{
+	u8 speed;
+
+	speed = ide_dma_speed(drive, cs5535_ratemask(drive));
+
+	/* If no DMA speed was available then disable DMA and use PIO. */
+	if (!speed) {
+		speed = ide_get_best_pio_mode(drive, 255, 5, NULL);
+	}
+
+	cs5535_set_drive(drive, speed);
+	return ide_dma_enable(drive);
+}
+
+static int cs5535_dma_check(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif	= drive->hwif;
+	struct hd_driveid *id	= drive->id;
+	u8 speed;
+
+	drive->init_speed = 0;
+
+	if ((id->capability & 1) && drive->autodma) {
+		if (ide_use_dma(drive)) {
+			if (cs5535_config_drive_for_dma(drive))
+				return hwif->ide_dma_on(drive);
+		}
+
+		goto fast_ata_pio;
+
+	} else if ((id->capability & 8) || (id->field_valid & 2)) {
+fast_ata_pio:
+		speed = ide_get_best_pio_mode(drive, 255, 5, NULL);
+		cs5535_set_drive(drive, speed);
+		return hwif->ide_dma_off_quietly(drive);
+	}
+	/* IORDY not supported */
+	return 0;
+}
+
+/**
+ * 	init_hwif_cs5535        -       Initialize one ide cannel
+ *	@hwif: Channel descriptor
+ *
+ *	This gets invoked by the IDE driver once for each channel. It
+ *	performs channel-specific pre-initialization before drive probing.
+ *
+ */
+static void __init init_hwif_cs5535(ide_hwif_t *hwif)
+{
+	u8 bit;
+	int i;
+
+	hwif->autodma = 0;
+
+	if (hwif->mate)
+		hwif->serialized = hwif->mate->serialized = 1;
+
+	hwif->tuneproc = &cs5535_tuneproc;
+	hwif->speedproc = &cs5535_set_drive;
+	hwif->ide_dma_check = &cs5535_dma_check;
+
+	hwif->atapi_dma = 1;
+	hwif->ultra_mask = 0x1F;
+	hwif->mwdma_mask = 0x07;
+
+	/* if a 80 wire cable was detected */
+	pci_read_config_byte(hwif->pci_dev, CS5535_CABLE_DETECT, &bit);
+	hwif->udma_four = bit & 1;
+
+	if (!noautodma) 
+		hwif->autodma = 1;
+
+	/* just setting autotune and not worrying about bios timings */
+	for (i = 0; i < 2; i++) {
+		hwif->drives[i].autotune = 1;
+		hwif->drives[i].autodma = hwif->autodma;
+	}
+}
+
+static ide_pci_device_t cs5535_chipset __initdata = {
+	.name		= "CS5535",
+#if defined(DEBUG)
+	.init_chipset   = init_chipset_cs5535,
+#endif
+	.init_hwif	= init_hwif_cs5535,
+	.channels	= 1,
+	.autodma	= AUTODMA,
+	.bootable	= ON_BOARD,
+};
+
+static int __init cs5535_init_one(struct pci_dev *dev, 
+					const struct pci_device_id *id)
+{
+	return ide_setup_pci_device(dev, &cs5535_chipset);
+}
+
+static struct pci_device_id cs5535_pci_tbl[] = 
+{
+	{ PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_IDE, PCI_ANY_ID, 
+		PCI_ANY_ID, 0, 0, 0},
+	{ 0, },
+};
+
+MODULE_DEVICE_TABLE(pci, cs5535_pci_tbl);
+
+static struct pci_driver driver = {
+	.name       = "CS5535_IDE",
+	.id_table   = cs5535_pci_tbl,
+	.probe      = cs5535_init_one,
+};
+
+static int __init cs5535_ide_init(void)
+{
+	DPRINTK("cs5535_ide_init\n"); 
+	return ide_pci_register_driver(&driver);
+}
+
+module_init(cs5535_ide_init);
+
+MODULE_AUTHOR("Jens Altmann");
+MODULE_DESCRIPTION("PCI driver module for AMD/NS 5535 IDE");
+MODULE_LICENSE("GPL");
diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/drivers/ide/pci/Makefile linux-2.6.13.1-ide/drivers/ide/pci/Makefile
--- linux-2.6.13.1-vanilla/drivers/ide/pci/Makefile	2005-09-15 14:35:10.000000000 +0800
+++ linux-2.6.13.1-ide/drivers/ide/pci/Makefile	2005-09-19 15:22:13.000000000 +0800
@@ -6,6 +6,7 @@ obj-$(CONFIG_BLK_DEV_ATIIXP)		+= atiixp.
 obj-$(CONFIG_BLK_DEV_CMD64X)		+= cmd64x.o
 obj-$(CONFIG_BLK_DEV_CS5520)		+= cs5520.o
 obj-$(CONFIG_BLK_DEV_CS5530)		+= cs5530.o
+obj-$(CONFIG_BLK_DEV_CS5535)		+= cs5535.o
 obj-$(CONFIG_BLK_DEV_SC1200)		+= sc1200.o
 obj-$(CONFIG_BLK_DEV_CY82C693)		+= cy82c693.o
 obj-$(CONFIG_BLK_DEV_HPT34X)		+= hpt34x.o
diff -uprN -X dontdiff.13.1 linux-2.6.13.1-vanilla/include/linux/pci_ids.h linux-2.6.13.1-ide/include/linux/pci_ids.h
--- linux-2.6.13.1-vanilla/include/linux/pci_ids.h	2005-09-15 14:36:19.000000000 +0800
+++ linux-2.6.13.1-ide/include/linux/pci_ids.h	2005-09-19 15:22:13.000000000 +0800
@@ -402,6 +402,7 @@
 #define PCI_DEVICE_ID_NS_SC1100_SMI	0x0511
 #define PCI_DEVICE_ID_NS_SC1100_XBUS	0x0515
 #define PCI_DEVICE_ID_NS_87410		0xd001
+#define PCI_DEVICE_ID_NS_CS5535_IDE	0x002d
 
 #define PCI_VENDOR_ID_TSENG		0x100c
 #define PCI_DEVICE_ID_TSENG_W32P_2	0x3202

Re: [RFC 2.6.13.1 1/1] Cleanup for CS5535 IDE driver

[Bartlomiej Zolnierkiewicz]

On 9/23/05, jayakumar.ide@gmail.com <jayakumar.ide@gmail.com> wrote:
> Hi Bartlomiej, IDE folk,

Hi,

> Appended is my patch attempting to fix up the cs5535 driver as per
> your suggestions from the thread:
>
> http://marc.theaimsgroup.com/?l=linux-ide&m=112533052120330&w=2
>
> I listed what I did with respect to that todo list below:
>
> > * cs5535_tuneproc() needs to be fixed to not abuse ->tuneproc interface
> >   (which is exported to user-space), all DMA tuning should be done through
> >   ->speedproc interface only
> Done.
>
> > * cs5535_set_drive() needs to use ide_rate_filter() to limit values passed
> >   from the user-space
> Done. btw, thanks for the eighty93 explaination yesterday. I hope I
> got that part right in this.

Yep. :)

> > * cs5535_set_drive() needs to set drive's speed unconditionally otherwise
> >   drive may not be setup correctly after resume
> Done.
>
> > * cs5535_set_drive() shouldn't change drive->current_speed
> Done.
>
> > * cable detection needs to be in separate function (for hotplug - later)
> I'm not sure I followed. By cable detection, I think you are pointing
> to init_hwif_cs5535:
>
>         /* if a 80 wire cable was detected */
>         pci_read_config_byte(hwif->pci_dev, CS5535_CABLE_DETECT, &bit);
>         hwif->udma_four = bit & 1;
>
> I think we can leave this as is because the cs5535 is a SOC companion that
> doesn't have hotplug capability. Is that okay?

Even if chipset itself doesn't have hotplug capability please
abstract cable detection code to match future IDE core changes.

> > * driver lacks device side cable detection [ see eighty_ninty_three() ]
> I put eighty93 into ratemask which gets called from set_drive.
>
> > * driver needs to check for blacklisted DMA devices [ see ide_use_dma() ]
> Done. I added it in dma_check to match what the other drivers do.
>
> > * init_hwif_cs5535() should be marked __devinit not __init
> Since 5535 has no hotplug capability, I set all to __init.

Please use __devinit:
* there is fake PCI hotplug driver which allows you to hot(un)plug any devices
* sparse complains about wrong __init usage

> > * cs5535_ide_init() lacks __init tag
> Done.
>
> > * .name in driver struct should be "CS5535_IDE" (w/o space)
> Done.
>
> > * what taking ide_lock in init_chipset_cs5535() is supposed to protect?
> Nothing as far as I can tell so I took it out.
>
> > * it would be simpler to always set ->autotune and tune PIO instead of
> >   checking for BIOS settings
> Sounds logical. So I took out the reg reads and just set autotune.
>
> > * use 'drive->hwif' instead of 'HWIF' macro
> Done.
>
> > * use 'u8' instead of 'byte' type
> Done.
>
> > * remove unneeded includes
> Done.
>
> > * proper coding style:
> Done.
>
> I did the patch against 2.6.13.1. fyi, the part adding 5535_IDE to pci_ids.h
> duplicates a prior alsa patch I did that is in akpm's -mm. I think I
> should have made the pci_ids diff separate.
>
> I only tested with a single drive that turned out to only be capable of
> MWDMA2 since that's what I have available right now. I'll try to borrow a
> UDMA4 capable drive and 80c cable if I have a chance.
>
>  Model=QUANTUM FIREBALL_TM1280A, FwRev=A6B.2D00, SerialNo=692716821653
>  Config={ HardSect NotMFM HdSw>15uSec Fixed DTR>5Mbs TrkOff }
>  RawCHS=2484/16/63, TrkSize=32256, SectSize=512, ECCbytes=4
>  BuffType=DualPortCache, BuffSize=76kB, MaxMultSect=16, MultSect=16
>  CurCHS=2484/16/63, CurSects=2503872, LBA=yes, LBAsects=2503872
>  IORDY=on/off, tPIO={min:300,w/IORDY:120}, tDMA={min:120,rec:120}
>  PIO modes:  pio0 pio1 pio2 pio3 pio4
>  DMA modes:  sdma0 sdma1 sdma2 mdma0 mdma1 *mdma2
>  AdvancedPM=no
>
>  * signifies the current active mode
>
>  Timing buffer-cache reads:   668 MB in  2.01 seconds = 332.65 MB/sec
>  Timing buffered disk reads:   20 MB in  3.06 seconds =   6.54 MB/sec
>
> Please let me know how it looks and if you have any feedback.

two comments below

> +static int cs5535_config_drive_for_dma(ide_drive_t *drive)
> +{
> +       u8 speed;
> +
> +       speed = ide_dma_speed(drive, cs5535_ratemask(drive));
> +
> +       /* If no DMA speed was available then disable DMA and use PIO. */
> +       if (!speed) {
> +               speed = ide_get_best_pio_mode(drive, 255, 5, NULL);

just return 0 and let cs5535_dma_check() take care of tuning PIO mode

> +       }
> +
> +       cs5535_set_drive(drive, speed);
> +       return ide_dma_enable(drive);
> +}

> +static int cs5535_dma_check(ide_drive_t *drive)
> +{
> +       ide_hwif_t *hwif        = drive->hwif;
> +       struct hd_driveid *id   = drive->id;
> +       u8 speed;
> +
> +       drive->init_speed = 0;
> +
> +       if ((id->capability & 1) && drive->autodma) {
> +               if (ide_use_dma(drive)) {
> +                       if (cs5535_config_drive_for_dma(drive))
> +                               return hwif->ide_dma_on(drive);
> +               }
> +
> +               goto fast_ata_pio;
> +
> +       } else if ((id->capability & 8) || (id->field_valid & 2)) {
> +fast_ata_pio:
> +               speed = ide_get_best_pio_mode(drive, 255, 5, NULL);

wrong arguments (max PIO is 4 not 5)

> +               cs5535_set_drive(drive, speed);
> +               return hwif->ide_dma_off_quietly(drive);
> +       }
> +       /* IORDY not supported */
> +       return 0;
> +}

Otherwise it looks fine and I'll happily apply it after corrections.

Thanks,
Bartlomiej

Re: [RFC 2.6.13.1 1/1] Cleanup for CS5535 IDE driver

[jayakumar ide]

On 9/27/05, Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> On 9/23/05, jayakumar.ide@gmail.com <jayakumar.ide@gmail.com> wrote:
> > I think we can leave this as is because the cs5535 is a SOC companion that
> > doesn't have hotplug capability. Is that okay?
>
> Even if chipset itself doesn't have hotplug capability please
> abstract cable detection code to match future IDE core changes.

Ok. Will do.

>
> Please use __devinit:
> * there is fake PCI hotplug driver which allows you to hot(un)plug any devices
> * sparse complains about wrong __init usage

Will do.

> > +       /* If no DMA speed was available then disable DMA and use PIO. */
> > +       if (!speed) {
> > +               speed = ide_get_best_pio_mode(drive, 255, 5, NULL);
>
> just return 0 and let cs5535_dma_check() take care of tuning PIO mode
>

Will do.

> > +       } else if ((id->capability & 8) || (id->field_valid & 2)) {
> > +fast_ata_pio:
> > +               speed = ide_get_best_pio_mode(drive, 255, 5, NULL);
>
> wrong arguments (max PIO is 4 not 5)

Oops. Corrected. I'll send these changes shortly.

Thanks,
jaya