pata_cs5536: ATA driver for Geode companion chip

Hi Martin - make sure you CC me on responses, I'm not on linux-ide.

There are multiple ways to do it, simply because of the way the architecture is designed underneath. Thats why we enforce the policy
of using the PCI mechanism when ever we can to encourage everybody on the same page.

No - I think if we're going to have the different timings, they should be in their own driver, not bothering the other users of this IP.

Thats a bug, pure and simple. If the PCI space is there, but the IDE registers are not - then you are deep into breakage mode, and you need to yell very loud and very long at your BIOS vendor.

In the standard AMD VSA, writeos to PCI space 0x48 (DTC), 0x49 (CAST) and 0x50 (ETC) should work - and the change should be immediately reflected
in the MSR. You can see for yourself, the code is open:

http://dev.laptop.org/git?p=geode-vsa;a=blob;f=sysmgr/pci_wr.c;h=9a984998ba3b8ed06358a85faf16a6ddebdad4a9;hb=HEAD#l359

Not all BIOS vendors follow the AMD model, but most do.

Alan is right, the NSC/Cyrix block had issues. Thats why when we started on the 5536, we searched around in AMD's attic, and found
the 8111 IDE block under an pile of old clothes and Life magazines. We dusted it off and stuck it in. So this fundamentally different silicon then before.

I believe that the datasheet is correct - at least, there aren't any errata, and we've been pretending to be a AMD74xx for quite a while now with no ill effects in this regard.

Jordan

Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

Jordan> In the standard AMD VSA, writeos to PCI space 0x48 (DTC), 0x49 Jordan> (CAST) and 0x50 (ETC) should work - and the change should be Jordan> immediately reflected in the MSR.

I know what the problem is. I just got a BIOS update tonight which was supposed to fix the issue. However, pata_amd.c still failed spectacularly at configuring the device whereas pata_cs5536 worked fine.

For kicks I switched my driver over to using PCI config registers instead of MSRs. And it still worked!

When I changed my driver I did a quickie conversion from wrmsr() to pci_write_config_dword(). If I use pci_write_config_byte() instead (like pata_amd.c), things fail miserably. A register dump reveals that any value I write becomes a 0. Yay!

Martin K. Petersen      http://mkp.net/

Fantastic - that means we have a way to tell if you have broken firmware and which driver to pick...

At that point its probaly a good idea to take CS5536 out of AMD and use your driver for it, whether in PCI or MSR mode (or perhaps supporting either)

Alan

Agreed. It sounds like there is still breakage in the BIOS - since the reference VSA code accounts for accesses of different alignments, but its probably not worth going back and forth about it. Like I said, if Alan and Jeff are game, we should go with the 5536 driver, and now that we've debated it extensively, I'm fine with the MSR accesses - I'm satisfied that we at least considered the alternative and dismissed it for technically sound reasons.

Is there anything we can do to make the transition smooth for anybody who may be using pata_amd in 2.6.23?

Jordan

Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

Even if we use PCI accesses and keep them dword along with the right timing it makes sense to have a 5536 driver for this.

We can make pata_amd pick it up if pata_cs5536 is not selected for now

Alan> Even if we use PCI accesses and keep them dword along with the Alan> right timing it makes sense to have a 5536 driver for this.

Yeah. Let me polish the PCI version and send that out...

Martin K. Petersen      http://mkp.net/

Jordan> Agreed. It sounds like there is still breakage in the BIOS - Jordan> since the reference VSA code accounts for accesses of Jordan> different alignments, but its probably not worth going back Jordan> and forth about it. Like I said, if Alan and Jeff are game, Jordan> we should go with the 5536 driver, and now that we've debated Jordan> it extensively, I'm fine with the MSR accesses - I'm satisfied Jordan> that we at least considered the alternative and dismissed it Jordan> for technically sound reasons.

I'm going to post two patches. One using MSR accesses and one using PCI.

Mixing the two access methods was the third option but I think that's more of an academic exercise. MSR will always work so why try poking holes in PCI config space first? The BIOS I have now exhibits one problem. The one from last week had another. Instead of trying to work around bug-du-jour I'd rather not depend on the BIOS getting things right. And it's less than a handful of register writes we're talking about here. Hardly worth it to come up with fallback heuristics for something that trivial.

But both patches are working with the BIOS from yesterday. Pick your poison :)

Martin K. Petersen      http://mkp.net/

This is a driver specifically for the ATA controller on the Geode CS5536 companion chip. The PCI device ID for this device was previously claimed by pata_amd.c but the PIO timings were not correct. This driver also works around a bug in some BIOSes that emulate unaligned access to the PCI config registers poorly.

---

diff -r de183081194a drivers/ata/Kconfig
@@ -254,6 +254,15 @@ config PATA_CS5535
 
 	  If unsure, say N.
 
+config PATA_CS5536
+	tristate "CS5536 PATA support (Experimental)"
+	depends on PCI && X86 && !X86_64 && EXPERIMENTAL
+	help
+	  This option enables support for the AMD CS5536
+	  companion chip used with the Geode LX processor family.
+
+	  If unsure, say N.
+
 config PATA_CYPRESS
 	tristate "Cypress CY82C693 PATA support (Very Experimental)"
 	depends on PCI && EXPERIMENTAL

diff -r de183081194a drivers/ata/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PATA_CS5520)	+= pata_cs5520
 obj-$(CONFIG_PATA_CS5520)	+= pata_cs5520.o
 obj-$(CONFIG_PATA_CS5530)	+= pata_cs5530.o
 obj-$(CONFIG_PATA_CS5535)	+= pata_cs5535.o
+obj-$(CONFIG_PATA_CS5536)	+= pata_cs5536.o
 obj-$(CONFIG_PATA_CYPRESS)	+= pata_cypress.o
 obj-$(CONFIG_PATA_EFAR)		+= pata_efar.o
 obj-$(CONFIG_PATA_HPT366)	+= pata_hpt366.o

diff -r de183081194a drivers/ata/pata_amd.c
@@ -693,7 +693,9 @@ static const struct pci_device_id amd[] 
 	{ PCI_VDEVICE(NVIDIA,	PCI_DEVICE_ID_NVIDIA_NFORCE_MCP67_IDE),	8 },
 	{ PCI_VDEVICE(NVIDIA,	PCI_DEVICE_ID_NVIDIA_NFORCE_MCP73_IDE),	8 },
 	{ PCI_VDEVICE(NVIDIA,	PCI_DEVICE_ID_NVIDIA_NFORCE_MCP77_IDE),	8 },
+#ifndef CONFIG_PATA_CS5536	/* Temporary */
 	{ PCI_VDEVICE(AMD,	PCI_DEVICE_ID_AMD_CS5536_IDE),		9 },
+#endif
 
 	{ },
 };

This is a driver specifically for the ATA controller on the Geode CS5536 companion chip. The PCI device ID for this device was previously claimed by pata_amd.c but there were two problems with that:

- Not all Geode platforms emulate the ATA registers in PCI config space

- CS5536 PIO timings are not identical to AMD74XX/8111

pata_cs5536.c relies on Geode Machine Specific Registers to configure the ATA function and uses the correct PIO timings for the chip.

---

diff -r de183081194a drivers/ata/Kconfig
@@ -254,6 +254,15 @@ config PATA_CS5535
 
 	  If unsure, say N.
 
+config PATA_CS5536
+	tristate "CS5536 PATA support (Experimental)"
+	depends on PCI && X86 && !X86_64 && EXPERIMENTAL
+	help
+	  This option enables support for the AMD CS5536
+	  companion chip used with the Geode LX processor family.
+
+	  If unsure, say N.
+
 config PATA_CYPRESS
 	tristate "Cypress CY82C693 PATA support (Very Experimental)"
 	depends on PCI && EXPERIMENTAL

diff -r de183081194a drivers/ata/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PATA_CS5520)	+= pata_cs5520
 obj-$(CONFIG_PATA_CS5520)	+= pata_cs5520.o
 obj-$(CONFIG_PATA_CS5530)	+= pata_cs5530.o
 obj-$(CONFIG_PATA_CS5535)	+= pata_cs5535.o
+obj-$(CONFIG_PATA_CS5536)	+= pata_cs5536.o
 obj-$(CONFIG_PATA_CYPRESS)	+= pata_cypress.o
 obj-$(CONFIG_PATA_EFAR)		+= pata_efar.o
 obj-$(CONFIG_PATA_HPT366)	+= pata_hpt366.o

diff -r de183081194a drivers/ata/pata_amd.c
@@ -693,7 +693,9 @@ static const struct pci_device_id amd[] 
 	{ PCI_VDEVICE(NVIDIA,	PCI_DEVICE_ID_NVIDIA_NFORCE_MCP67_IDE),	8 },
 	{ PCI_VDEVICE(NVIDIA,	PCI_DEVICE_ID_NVIDIA_NFORCE_MCP73_IDE),	8 },
 	{ PCI_VDEVICE(NVIDIA,	PCI_DEVICE_ID_NVIDIA_NFORCE_MCP77_IDE),	8 },
+#ifndef CONFIG_PATA_CS5536	/* Temporary */
 	{ PCI_VDEVICE(AMD,	PCI_DEVICE_ID_AMD_CS5536_IDE),		9 },
+#endif
 
 	{ },
 };

Any reason for not supporting both methods ? Though I guess as you say msr is sufficient.

	!defined(FOO) && ! defined(FOO_MODULE)

otherwise ACK

Alan

ACK as well, but I put a call out on the Geode list for testing - I want to make sure that we get some time on different platforms with different BIOSes to make sure it is solid.

Jordan

Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

Alan> Any reason for not supporting both methods ? Though I guess as Alan> you say msr is sufficient.

Given that we don't have a good way to detect failure other than libata failing to set PIO timings, I think it's going to be unnecessary complexity. As I wrote, the BIOS versions all behaved differently (in a previous rev. the written value would look ok in the PCI config register but not trickle back to MSR space).

In any case the friendly firmware fairy left me a working BIOS this morning. Byte-aligned PCI config space accesses do the right thing. So as far as I'm concerned the case has been solv-ed. I'm now officially agnostic.

Jordan: You're probably dealing with issues like this more frequently than we are. What's your preference for pata_cs5536.c? MSR or PCI?

Martin K. Petersen      http://mkp.net/