ATA: add a PCI ID for Intel Santa Rosa PATA controller

PATCH In review Chuck Ebbert 7 participants 15 messages Jun 28, 2007 – Jul 6, 2007 libata

lore

Share & tools

lore

Chuck Ebbert

author

2007-06-28 19:16 UTC #0 lore raw

From: Christian Lamparter <[email protected]>

ATA: add a PCI ID for Intel Santa Rosa PATA controller.

Signed-off-by Christian Lamparter <[email protected]>

---

[against 2.6.22-rc6; patch also attached, use the attachment]

 drivers/ata/ata_piix.c |    2 ++
 1 file changed, 2 insertions(+)

drivers/ata/ata_piix.c +2 -0

index 9c07b88..b210726 100644
@@ -200,6 +200,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
        /* ICH7/7-R (i945, i975) UDMA 100*/
        { 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
        { 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+       /* ICH8 Mobile PATA Controller */
+       { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
 
        /* NOTE: The following PCI ids must be kept in sync with the
         * list in drivers/pci/quirks.c.

From: Christian Lamparter <[email protected]>

ATA: add a PCI ID for Intel Santa Rosa PATA controller.

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/ata/ata_piix.c |    2 ++
 1 file changed, 2 insertions(+)

drivers/ata/ata_piix.c +2 -0

index 9c07b88..b210726 100644
@@ -200,6 +200,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	/* ICH7/7-R (i945, i975) UDMA 100*/
 	{ 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
 	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	/* ICH8 Mobile PATA Controller */
+	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
 
 	/* NOTE: The following PCI ids must be kept in sync with the
 	 * list in drivers/pci/quirks.c.

Chuck Ebbert

author

2007-06-28 19:47 UTC #1 lore raw

PATCH 1/1 ATA: add a PCI ID for Intel Santa Rosa PATA controller

On 06/28/2007 03:16 PM, Chuck Ebbert wrote:

[Forgot to mark subj: as a patch; attached]

Quoted 17 lines

From: Christian Lamparter <[email protected]>
ATA: add a PCI ID for Intel Santa Rosa PATA controller.
Signed-off-by Christian Lamparter <[email protected]>
---
[against 2.6.22-rc6; patch also attached, use the attachment]
 drivers/ata/ata_piix.c |    2 ++
 1 file changed, 2 insertions(+)
drivers/ata/ata_piix.c +0 -0
index 9c07b88..b210726 100644

drivers/ata/ata_piix.c static const struct pci_device_id piix_pci_tbl[] = { · line 207

Quoted 8 lines

drivers/ata/ata_piix.c +2 -0

@@ -200,6 +200,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
        /* ICH7/7-R (i945, i975) UDMA 100*/
        { 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
        { 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+       /* ICH8 Mobile PATA Controller */
+       { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
 
        /* NOTE: The following PCI ids must be kept in sync with the
         * list in drivers/pci/quirks.c.

From: Christian Lamparter <[email protected]>

ATA: add a PCI ID for Intel Santa Rosa PATA controller.

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/ata/ata_piix.c |    2 ++
 1 file changed, 2 insertions(+)

drivers/ata/ata_piix.c +2 -0

index 9c07b88..b210726 100644
@@ -200,6 +200,8 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	/* ICH7/7-R (i945, i975) UDMA 100*/
 	{ 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
 	{ 0x8086, 0x269E, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_100 },
+	/* ICH8 Mobile PATA Controller */
+	{ 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
 
 	/* NOTE: The following PCI ids must be kept in sync with the
 	 * list in drivers/pci/quirks.c.

Jeff Garzik

2007-06-29 14:31 UTC #2 lore raw

Quoted 3 lines · Chuck Ebbert

On 06/28/2007 03:16 PM, Chuck Ebbert wrote:
[Forgot to mark subj: as a patch; attached]

Sorry, using attachments (a) decreases reviewers and (b) makes your patch HARDER to apply. See #7 under Documentation/SubmittingPatches.

Jeff

Thorsten Leemhuis

2007-07-02 05:19 UTC #3 lore raw

In reply to Chuck Ebbert

Quoted 4 lines · Chuck Ebbert

On 06/28/2007 03:16 PM, Chuck Ebbert wrote:
[Forgot to mark subj: as a patch; attached]

Quoted 9 lines

Quoted 6 lines
From: Christian Lamparter <[email protected]>
ATA: add a PCI ID for Intel Santa Rosa PATA controller.
Signed-off-by Christian Lamparter <[email protected]>
---
[...]
Quoted 2 lines
drivers/ata/ata_piix.c +2 -0
+       /* ICH8 Mobile PATA Controller */
+       { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },

FYI: Patch works fine for me on my brand new Dell D630 (Santa Rosa) -- the DVD-Drive gets recognized with it applied.

CU
thl

Thorsten Leemhuis

2007-07-02 09:07 UTC #4 lore raw

Quoted 9 lines · Thorsten Leemhuis

Quoted 8 lines · Chuck Ebbert
On 06/28/2007 03:16 PM, Chuck Ebbert wrote:
[Forgot to mark subj: as a patch; attached]
Quoted 4 lines
From: Christian Lamparter <[email protected]>
ATA: add a PCI ID for Intel Santa Rosa PATA controller.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Signed-off-by Christian Lamparter <[email protected]>
---
[...]
drivers/ata/ata_piix.c static const struct pci_device_id piix_pci_tbl[] = { · line 204
Quoted 2 lines
drivers/ata/ata_piix.c +2 -0
+       /* ICH8 Mobile PATA Controller */
+       { 0x8086, 0x2850, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
FYI: Patch works fine for me on my brand new Dell D630 (Santa Rosa) -- the DVD-Drive gets recognized with it applied.

Hmmm. From looking at the ICH8-Datasheet (
http://www.intel.com/design/chipsets/datashts/313056.htm
or http://download.intel.com/design/chipsets/datashts/31305603.pdf ) page 189 and later it seems to me that the ICH8M (just as all the other Intel-Chipsets I know) officially only supports Ultra ATA 100 (and not 133!).

So is the ich_pata_133 correct in the patch (see quoted line above)?

CU
thl

Chr

2007-07-02 11:24 UTC #5 lore raw

Quoted 8 lines · you

On 02.07.2007 07:02, Thorsten Leemhuis wrote:
Hmmm. From looking at the ICH8-Datasheet (
http://www.intel.com/design/chipsets/datashts/313056.htm
or http://download.intel.com/design/chipsets/datashts/31305603.pdf ) page 189 and later it seems to me that the ICH8M (just as all the other Intel-Chipsets I know) officially only supports Ultra ATA 100 (and not 133!).

Quoted 5 lines

So is the ich_pata_133 correct in the patch (see quoted line above)?
CU
thl

(please keep the CC).

For some strange reason, I can not get the pdf...

Anyway, the original patch had "ich_pata_100".
see: http://www.mail-archive.com/linux-ide%40vger.kernel.org/msg07416.html

but Alan Cox wrote:
http://www.mail-archive.com/linux-ide%40vger.kernel.org/msg07417.html

Quoted 6 lines

Its ich_pata_133 - all the newer chips are. They work even better if you set them into AHCI mode in the BIOS and then they should "just work" with recent kernels as the AHCI driver now matches by class.
Alan

And "Gaston, Jason D" <[email protected]> didn't complain about it. it's a "bit" confusing with all "native" AHCI SATA chipset that have to emulate PATA for compatibility reasons...

So, whom can we trust? I've seen wrong specs, but you know "To err is human".

Thanks,
Chr.

Thorsten Leemhuis

2007-07-02 11:44 UTC #6 lore raw

Quoted 9 lines · Chr

Quoted 8 lines · you
On 02.07.2007 07:02, Thorsten Leemhuis wrote:
Hmmm. From looking at the ICH8-Datasheet (
http://www.intel.com/design/chipsets/datashts/313056.htm
or http://download.intel.com/design/chipsets/datashts/31305603.pdf ) page 189 and later it seems to me that the ICH8M (just as all the other Intel-Chipsets I know) officially only supports Ultra ATA 100 (and not 133!).
So is the ich_pata_133 correct in the patch (see quoted line above)?

Quoted 3 lines

For some strange reason, I can not get the pdf...
Anyway, the original patch had "ich_pata_100".
see: http://www.mail-archive.com/linux-ide%40vger.kernel.org/msg07416.html

k, missed that post.

Quoted 3 lines

but Alan Cox wrote:
http://www.mail-archive.com/linux-ide%40vger.kernel.org/msg07417.html
Quoted 1 line
Its ich_pata_133 - all the newer chips are.

Intel afaik never supported Ultra ATA 133 officially in any of the mainstream desktop or mobile chipsets.

Quoted 6 lines

Quoted 3 lines
They work even better if you
set them into AHCI mode in the BIOS and then they should "just work" with recent kernels as the AHCI driver now matches by class.
And "Gaston, Jason D" <[email protected]> didn't complain about it. it's a "bit" confusing with all "native" AHCI SATA chipset that have to emulate PATA for compatibility reasons...

Well, just FYI: on my Laptop AHCI is enabled and used for the SATA hard disk. But the DVD drive still is a pata one afaics (I'm not in front of the machine, so I can't check), connected via the pata controller -- so for me there is no emulation involved (at least afaics).

Quoted 1 line

So, whom can we trust?

I'd trust the public datasheet in this case.

CU
thl

Chr

2007-07-02 14:36 UTC #7 lore raw

Quoted 6 lines · Thorsten Leemhuis

Quoted 3 lines
but Alan Cox wrote:
http://www.mail-archive.com/linux-ide%40vger.kernel.org/msg07417.html
Quoted 1 line
Its ich_pata_133 - all the newer chips are.
Intel afaik never supported Ultra ATA 133 officially in any of the mainstream desktop or mobile chipsets.

You're probably right! But, what about Intel's ICH5 and ICH7/7-R (i945, i975)?

see ata_piix.c: line 193ff

	{ 0x8086, 0x24DB, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
[...]
	{ 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },

On the other hand, we can leave it, because of a "off-by-one error" in ata_piix.c, do_pata_set_dmamode, line ~770:

(the comment is important!)
/*
* --> UDMA is handled by a combination of clock switching and
* selection of dividers <---
* [...]
*/
u_speed = min(2 - (udma & 1), udma);
if (udma == 5)

	u_clock = 0x1000;	/* 100Mhz */ <-- wrong! it's 133Mhz.
else if (udma > 2)
	u_clock = 1;		/* 66Mhz */
else
	u_clock = 0;		/* 33Mhz */

=>
for udma = 6(133MB/s) , we get u_speed=2 and u_clock=1
for udma = 4(66MB/s), we get the "same" values! (u_speed=2 and u_clock=1)

...
so, atleast for ata_piix, UDMA6 and UDMA4 *is* the same, right?

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
They work even better if you
set them into AHCI mode in the BIOS and then they should "just work" with recent kernels as the AHCI driver now matches by class.

Quoted 9 lines

Quoted 3 lines
And "Gaston, Jason D" <[email protected]> didn't complain about it. it's a "bit" confusing with all "native" AHCI SATA chipset that have to emulate PATA for compatibility reasons...
Well, just FYI: on my Laptop AHCI is enabled and used for the SATA hard disk. But the DVD drive still is a pata one afaics (I'm not in front of the machine, so I can't check), connected via the pata controller -- so for me there is no emulation involved (at least afaics).

I know. I wanted to say something else... but it doesn't really matter (not every operating system supports AHCI-only controllers by default...)

Thanks,
Chr.

Thorsten Leemhuis

2007-07-04 14:42 UTC #8 lore raw

Quoted 4 lines · Chr

Quoted 3 lines · Thorsten Leemhuis
Quoted 3 lines
but Alan Cox wrote:
http://www.mail-archive.com/linux-ide%40vger.kernel.org/msg07417.html
Quoted 1 line
Its ich_pata_133 - all the newer chips are.

Quoted 8 lines

Quoted 2 lines
Intel afaik never supported Ultra ATA 133 officially in any of the mainstream desktop or mobile chipsets.
You're probably right! But, what about Intel's ICH5 and ICH7/7-R (i945, i975)?
see ata_piix.c: line 193ff
	{ 0x8086, 0x24DB, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },
[...]
	{ 0x8086, 0x27DF, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich_pata_133 },

Maybe Alan or someone from Intel can answer that and thus tell us how to move forward; it would be really nice to get the patch that started this thread (it got posted by Chuck again today) could go into 2.6.22, but I assume the time runs away...

Quoted 3 lines

On the other hand, we can leave it, because of a "off-by-one error" in ata_piix.c, do_pata_set_dmamode, line ~770:
[...]

/me gets confused by that stuff and leaves the answer to that part to more knowledged people...

CU
thl

Signature · 11 lines

Thorsten Leemhuis
 c't- Magazin für Computertechnik       web    http://www.heise.de/ct/
 Heise Zeitschriften Verlag GmbH&Co.KG  phone  +49 (0)511 5352 666
 Helstorfer Str. 7                      icq    140593172
 D-30625 Hannover, Germany              jabber [email protected]

/* Heise Zeitschriften Verlag GmbH & Co. KG, Registergericht:
   Amtsgericht Hannover HRA 26709; Persönlich haftende Gesellschafterin:
   Heise Zeitschriften Verlag Geschäftsführung GmbH, Registergericht:
   Amtsgericht Hannover, HRB 60405 Geschäftsführer: Ansgar Heise,
   Steven P. Steinkraus, Dr. Alfons Schräder                          */

Chr

2007-07-05 20:29 UTC #9 lore raw

Quoted 6 lines · Thorsten Leemhuis

Maybe Alan or someone from Intel can answer that and thus tell us how to move forward; it would be really nice to get the patch that started this thread (it got posted by Chuck again today) could go into 2.6.22, but I assume the time runs away...

Jeff is on holiday and Intel doesn't respond at all...

So, if someone has a ICH5 or ICH7/7-R with a ATA133 captable HDD... could you please try the following patch? (Of course, I don't know if it works,... It may cause data loss, serve corruption, or damage your controller/hdd/pc, so make backups and keep a first-aid kit handy!)

Quoted 4 lines

Quoted 3 lines
On the other hand, we can leave it, because of a "off-by-one error" in ata_piix.c, do_pata_set_dmamode, line ~770:
[...]

Thanks,
Chr.

drivers/ata/ata_piix.c +2 -2

@@ -767,8 +767,8 @@
 		 *	       except UDMA0 which is 00
 		 */
 		u_speed = min(2 - (udma & 1), udma);
-		if (udma == 5)
-			u_clock = 0x1000;	/* 100Mhz */
+		if (udma >= 5)
+			u_clock = 0x1000;	/* 133Mhz */
 		else if (udma > 2)
 			u_clock = 1;		/* 66Mhz */
 		else

Gaston, Jason D

2007-07-06 19:06 UTC #10 lore raw

Quoted 30 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Thursday, July 05, 2007 1:30 PM
To: Thorsten Leemhuis
Cc: Thorsten Leemhuis; Chuck Ebbert; Jeff Garzik; IDE/ATA development list; Gaston, Jason D; Alan Cox; [email protected]; [email protected]
Subject: Re: [patch 2.6.22-rc6] ATA: add a PCI ID for Intel Santa Rosa PATA controller
Quoted 2 lines · Thorsten Leemhuis
Maybe Alan or someone from Intel can answer that and thus
tell us how to
Quoted 1 line
move forward; it would be really nice to get the patch that
started this
Quoted 1 line
thread (it got posted by Chuck again today) could go into
2.6.22, but I
Quoted 2 lines
assume the time runs away...
Jeff is on holiday and Intel doesn't respond at all...
So, if someone has a ICH5 or ICH7/7-R with a ATA133 captable HDD... could
you please try the following patch? (Of course, I don't know if it works,...
It may cause data loss, serve corruption, or damage your controller/hdd/pc,
so make backups and keep a first-aid kit handy!)

Quoted 10 lines

Quoted 1 line
Quoted 1 line
On the other hand, we can leave it, because of a
"off-by-one error" in ata_piix.c,
Quoted 3 lines
Quoted 2 lines
do_pata_set_dmamode, line ~770:
[...]
Thanks,
Chr.

I quickly tried this patch on an ICH7-R system with an ATA133 Maxtor HD and it did not seem to do anything bad. I see no difference in function or performance with the patch.

Jason

Chr

2007-07-06 19:44 UTC #11 lore raw

Quoted 11 lines · Jason D

Quoted 10 lines
Quoted 1 line
Quoted 1 line
On the other hand, we can leave it, because of a
"off-by-one error" in ata_piix.c,
Quoted 3 lines
Quoted 2 lines
do_pata_set_dmamode, line ~770:
[...]
Thanks,
Chr.

Quoted 6 lines

I quickly tried this patch on an ICH7-R system with an ATA133 Maxtor HD and it did not seem to do anything bad. I see no difference in function or performance with the patch.
Jason

Thanks for testing, but you forgot some numbers. ;) (bonnie++? hdparm -i && hdparm -t...)

Because, I don't think that Maxtor HD's mechanics is faster than 100MB/s.. The only thing that can keep up with > 100MB/s are the few MB of Cache on every HDD... (so, does anyone have a good idea to check the HDD's cache performance? hdparm -t reads too much data)

I came up with something like:
"dd if=/dev/sdX of=/dev/null bs=4M count=1 iflag=direct"

and run it 10 times in a row.

here's a test run with a SATA I WD2500YD-01N ( hdparm -t performance: ~57MB/s) 4194304 bytes (4.2 MB) copied, 0.0897687 seconds, 46.7 MB/s 4194304 bytes (4.2 MB) copied, 0.0776731 seconds, 54.0 MB/s 4194304 bytes (4.2 MB) copied, 0.0711224 seconds, 59.0 MB/s 4194304 bytes (4.2 MB) copied, 0.0679675 seconds, 61.7 MB/s 4194304 bytes (4.2 MB) copied, 0.0603272 seconds, 69.5 MB/s 4194304 bytes (4.2 MB) copied, 0.0543068 seconds, 77.2 MB/s 4194304 bytes (4.2 MB) copied, 0.0522881 seconds, 80.2 MB/s 4194304 bytes (4.2 MB) copied, 0.0520612 seconds, 80.6 MB/s 4194304 bytes (4.2 MB) copied, 0.0444805 seconds, 94.3 MB/s 4194304 bytes (4.2 MB) copied, 0.0338407 seconds, 124 MB/s 4194304 bytes (4.2 MB) copied, 0.0333174 seconds, 126 MB/s

Thanks,
Chr

Thorsten Leemhuis

2007-07-06 20:02 UTC #12 lore raw

Quoted 9 lines · Chr

Quoted 7 lines · Jason D
Quoted 4 lines
Quoted 1 line
Quoted 1 line
On the other hand, we can leave it, because of a
"off-by-one error" in ata_piix.c,
Quoted 2 lines
Quoted 2 lines
do_pata_set_dmamode, line ~770:
[...]
I quickly tried this patch on an ICH7-R system with an ATA133 Maxtor HD and it did not seem to do anything bad. I see no difference in function or performance with the patch.

Quoted 6 lines

Thanks for testing, but you forgot some numbers. ;) (bonnie++? hdparm -i && hdparm -t...)
Because, I don't think that Maxtor HD's mechanics is faster than 100MB/s.. The only thing that can keep up with > 100MB/s are the few MB of Cache on every HDD... (so, does anyone have a good idea to check the HDD's cache performance?

Something like the good old "Coretest" might come close to it. But I'm not aware of any linux benchmark out there that has the coretest-behavior (reading the same 64k block in a loop over and over again).

But anyway: Sorry, am I the only one that gets nervous if we start to run a ATA-Hardware (Intels recent ICH southbridges in this case) with a speed grade (Ultra ATA/133) that is not specified by its manufacturer in his public data sheets (Ultra ATA/100 max)?

Doing something like that by default would imho be overclocking -- sure, it works often, but well, it can be the real pita on those systems where it doesn't. Heck, there is likely not even a real performance advantage
from Ultra ATA/133 in usual systems with one hard disk per channel, as
most PATA harddisks are afaics not much quicker then round about 70 MByte (and even that only in the early area of the disk).

CU
thl

Jeff Garzik

2007-07-06 20:43 UTC #13 lore raw

Quoted 4 lines · Thorsten Leemhuis

But anyway: Sorry, am I the only one that gets nervous if we start to run a ATA-Hardware (Intels recent ICH southbridges in this case) with a speed grade (Ultra ATA/133) that is not specified by its manufacturer in his public data sheets (Ultra ATA/100 max)?

That's why the patch I applied referenced ATA/100...

If they fix the datasheets, we can always upgrade it later.

Jeff

Alan Cox

2007-07-05 21:11 UTC #14 lore raw

In reply to Thorsten Leemhuis

Quoted 4 lines

Maybe Alan or someone from Intel can answer that and thus tell us how to move forward; it would be really nice to get the patch that started this thread (it got posted by Chuck again today) could go into 2.6.22, but I assume the time runs away...

Folks should be back Monday I suspect. Its the US indepenence day (or from UK perspective 'they day the terrorists won' 8)

I don't have suitable systems to hand to test this and investigate further.

Alan