Re: [PATCH] libata: PIIX3 support

Re: [PATCH] libata: PIIX3 support

[Robert Hancock]

Mikael Pettersson wrote:
> On Wed, 10 Jan 2007 17:13:38 +0000, Alan <alan@lxorguk.ukuu.org.uk> wrote:
>> This I believe completes the PIIX range of support for libata
>>
>> This adds the table entries needed for the PIIX3, both a new PCI
>> identifier and a new mode list. It also fixes an erroneous access to PCI
>> configuration 0x48 on non UDMA capable chips.
> 
> Works fine here on a 430HX box (ASUS T2P4).
> I'm appending kernel messages for boots with the IDE driver and
> with the updated libata driver, in case you want to compare them.
> 
> I did notice that ata_piix identified the disk as
> "QUANTUM FIREBALL A5U." when IDE correctly identified it as
> "QUANTUM FIREBALL CR8.4A".

I believe libata truncates the ATA device ID string to fit the max 
allowable for SCSI. The A5U. part is presumably the drive's firmware 
revision.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [PATCH] libata: PIIX3 support

[Jeff Garzik]

Alan wrote:
> This I believe completes the PIIX range of support for libata
> 
> This adds the table entries needed for the PIIX3, both a new PCI
> identifier and a new mode list. It also fixes an erroneous access to PCI
> configuration 0x48 on non UDMA capable chips.
> 
> Signed-off-by: Alan Cox <alan@redhat.com>

applied.

Please submit clean, warning-free C code in the future, rather than 
relying on Andrew to clean up after you.

	Jeff

Re: [PATCH] libata: PIIX3 support

[Alan]

On Fri, 12 Jan 2007 02:08:12 +0100 (MET)
Mikael Pettersson <mikpe@it.uu.se> wrote:

> On Wed, 10 Jan 2007 17:13:38 +0000, Alan <alan@lxorguk.ukuu.org.uk> wrote:
> >This I believe completes the PIIX range of support for libata
> >
> >This adds the table entries needed for the PIIX3, both a new PCI
> >identifier and a new mode list. It also fixes an erroneous access to PCI
> >configuration 0x48 on non UDMA capable chips.
> 
> Works fine here on a 430HX box (ASUS T2P4).
> I'm appending kernel messages for boots with the IDE driver and
> with the updated libata driver, in case you want to compare them.

Thanks a lot. That all looks good and fits the other test reports nicely
(except qemu which appears to have PIIX3 emulation issues which Tejun's
quite fussy and careful eh code picks up on)

Alan

Re: [PATCH] libata: PIIX3 support

[Mikael Pettersson]

On Wed, 10 Jan 2007 17:13:38 +0000, Alan <alan@lxorguk.ukuu.org.uk> wrote:
>This I believe completes the PIIX range of support for libata
>
>This adds the table entries needed for the PIIX3, both a new PCI
>identifier and a new mode list. It also fixes an erroneous access to PCI
>configuration 0x48 on non UDMA capable chips.

Works fine here on a 430HX box (ASUS T2P4).
I'm appending kernel messages for boots with the IDE driver and
with the updated libata driver, in case you want to compare them.

I did notice that ata_piix identified the disk as
"QUANTUM FIREBALL A5U." when IDE correctly identified it as
"QUANTUM FIREBALL CR8.4A".

/Mikael

[2.6.20-rc4 with CONFIG_BLK_DEV_PIIX=y]
Uniform Multi-Platform E-IDE driver Revision: 7.00alpha2
ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
PIIX3: IDE controller at PCI slot 0000:00:07.1
PIIX3: chipset revision 0
PIIX3: not 100% native mode: will probe irqs later
    ide0: BM-DMA at 0xe800-0xe807, BIOS settings: hda:DMA, hdb:pio
    ide1: BM-DMA at 0xe808-0xe80f, BIOS settings: hdc:pio, hdd:pio
Probing IDE interface ide0...
hda: QUANTUM FIREBALL CR8.4A, ATA DISK drive
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
Probing IDE interface ide1...
hda: max request size: 128KiB
hda: 16514064 sectors (8455 MB) w/418KiB Cache, CHS=16383/16/63, (U)DMA
hda: cache flushes not supported
 hda: hda1 hda2 hda3 hda4 < hda5 hda6 >

[2.6.20-rc4 + alan's patch with CONFIG_ATA_PIIX=y]
ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max MWDMA2 cmd 0x1F0 ctl 0x3F6 bmdma 0xE800 irq 14
ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xE808 irq 15
scsi0 : ata_piix
ata1.00: ATA-4, max UDMA/66, 16514064 sectors: LBA 
ata1.00: ata1: dev 0 multi count 16
ata1.00: configured for MWDMA2
scsi1 : ata_piix
scsi 0:0:0:0: Direct-Access     ATA      QUANTUM FIREBALL A5U. PQ: 0 ANSI: 5
SCSI device sda: 16514064 512-byte hdwr sectors (8455 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sda: 16514064 512-byte hdwr sectors (8455 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sda: sda1 sda2 sda3 sda4 < sda5 sda6 >
sd 0:0:0:0: Attached scsi disk sda

Re: [PATCH] libata: PIIX3 support

[Jeff Garzik]

Alan wrote:
> @@ -786,7 +797,8 @@
>  			    { 2, 3 }, };
>  
>  	pci_read_config_word(dev, master_port, &master_data);
> -	pci_read_config_byte(dev, 0x48, &udma_enable);
> +	if (ap->udma_mask)
> +		pci_read_config_byte(dev, 0x48, &udma_enable);
>  
>  	if (speed >= XFER_UDMA_0) {
>  		unsigned int udma = adev->dma_mode - XFER_UDMA_0;

This creates a situation where the code modifies an uninitialized 
variable.  Ultimately irrelevant, but please init udma_enable to zero so 
that at least it is valid C code.

Otherwise ACK.

	Jeff

[PATCH] libata: PIIX3 support

[Alan]

This I believe completes the PIIX range of support for libata

This adds the table entries needed for the PIIX3, both a new PCI
identifier and a new mode list. It also fixes an erroneous access to PCI
configuration 0x48 on non UDMA capable chips.

Signed-off-by: Alan Cox <alan@redhat.com>

--- linux.vanilla-2.6.20-rc3-mm1/drivers/ata/ata_piix.c	2007-01-05 13:09:36.000000000 +0000
+++ linux-2.6.20-rc3-mm1/drivers/ata/ata_piix.c	2007-01-10 16:37:56.840128000 +0000
@@ -118,7 +118,7 @@
 	PIIX_80C_SEC		= (1 << 7) | (1 << 6),
 
 	/* controller IDs */
-	piix_pata_33		= 0,	/* PIIX3 or 4 at 33Mhz */
+	piix_pata_33		= 0,	/* PIIX4 at 33Mhz */
 	ich_pata_33		= 1,	/* ICH up to UDMA 33 only */
 	ich_pata_66		= 2,	/* ICH up to 66 Mhz */
 	ich_pata_100		= 3,	/* ICH up to UDMA 100 */
@@ -128,6 +128,7 @@
 	ich6_sata_ahci		= 7,
 	ich6m_sata_ahci		= 8,
 	ich8_sata_ahci		= 9,
+	piix_pata_mwdma		= 10,	/* PIIX3 MWDMA only */
 
 	/* constants for mapping table */
 	P0			= 0,  /* port 0 */
@@ -165,6 +166,8 @@
 
 static const struct pci_device_id piix_pci_tbl[] = {
 #ifdef ATA_ENABLE_PATA
+	/* Intel PIIX3 for the 430HX etc */
+	{ 0x8086, 0x7010, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_mwdma },
 	/* Intel PIIX4 for the 430TX/440BX/MX chipset: UDMA 33 */
 	/* Also PIIX4E (fn3 rev 2) and PIIX4M (fn3 rev 3) */
 	{ 0x8086, 0x7111, PCI_ANY_ID, PCI_ANY_ID, 0, 0, piix_pata_33 },
@@ -441,7 +444,7 @@
 };
 
 static struct ata_port_info piix_port_info[] = {
-	/* piix_pata_33: 0:  PIIX3 or 4 at 33MHz */
+	/* piix_pata_33: 0:  PIIX4 at 33MHz */
 	{
 		.sht		= &piix_sht,
 		.flags		= PIIX_PATA_FLAGS,
@@ -543,6 +546,14 @@
 		.port_ops	= &piix_sata_ops,
 	},
 
+	/* piix_pata_mwdma: 10:  PIIX3 MWDMA only */
+	{
+		.sht		= &piix_sht,
+		.flags		= PIIX_PATA_FLAGS,
+		.pio_mask	= 0x1f,	/* pio0-4 */
+		.mwdma_mask	= 0x06, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
+		.port_ops	= &piix_pata_ops,
+	},
 };
 
 static struct pci_bits piix_enable_bits[] = {
@@ -786,7 +797,8 @@
 			    { 2, 3 }, };
 
 	pci_read_config_word(dev, master_port, &master_data);
-	pci_read_config_byte(dev, 0x48, &udma_enable);
+	if (ap->udma_mask)
+		pci_read_config_byte(dev, 0x48, &udma_enable);
 
 	if (speed >= XFER_UDMA_0) {
 		unsigned int udma = adev->dma_mode - XFER_UDMA_0;