Re: pata_cs5536: ATA driver for Geode companion chip

Re: pata_cs5536: ATA driver for Geode companion chip

[Jordan Crouse]

On 03/10/07 13:28 -0400, Martin K. Petersen wrote:
> 
> 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:
 
Hmm - I had to ruminate on this for a while.   As a general rule of thumb
I try to discourage working around the PCI virtualization - both because
it encumbers Linux with one off drivers such as this one, and we lose
any benefit from bug fixes in the firmware (not to mention bug fixes in
the common shared driver).  AMD has gone to a lot of trouble and expense
to make a decidedly non-PCI bus Just Work (TM) like a PCI bus, and it
takes a lot to convince me to diverge from that.

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

If you system has any sort of PCI virtualization support at all,
and it doesn't support these registers, then thats a big fat
bug, IMHO, and should be resolved post haste.  But that said, 
the VSA doesn't do any sort of messy workaround here, so the direct
route isn't going to lose us anything in terms of bug fixes.

>  - CS5536 PIO timings are not identical to AMD74XX/8111

This is true, and it was brought up when we first pushed the drivers.
Tests showed that the timings generated by the Thor driver were within
the optimal tolerances for the 5536 controller, and we decided that a
custom driver wasn't indicated.  But if we *are* going to have a custom
driver, then we should get these right.

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

I have to discourage this patch just on principle, because I don't like
the idea of working around the VSA.  That said, it seems that Alan is 
willing to take on the extra code, and there isn't anything technically
deficient here, so I'm fine with this going in.

Jordan

Re: pata_cs5536: ATA driver for Geode companion chip

[Alan Cox]

> I have to discourage this patch just on principle, because I don't like
> the idea of working around the VSA.  That said, it seems that Alan is 
> willing to take on the extra code, and there isn't anything technically
> deficient here, so I'm fine with this going in.

I'd prefer to go via the PCI virtualisation(but with the timings fixed)
but there are boxes out there where this doesn't work.

Ideally what I'd like would be a suggestion of how to tell if the VSA is
active on this so that we can use VSA if present.

Re: pata_cs5536: ATA driver for Geode companion chip

[Jordan Crouse]

On 03/10/07 21:59 +0100, Alan Cox wrote:
> > I have to discourage this patch just on principle, because I don't like
> > the idea of working around the VSA.  That said, it seems that Alan is 
> > willing to take on the extra code, and there isn't anything technically
> > deficient here, so I'm fine with this going in.
> 
> I'd prefer to go via the PCI virtualisation(but with the timings fixed)
> but there are boxes out there where this doesn't work.
> 
> Ideally what I'd like would be a suggestion of how to tell if the VSA is
> active on this so that we can use VSA if present.

int geode_has_vsa() {
	outw(0xFC53, 0xAC1C);
	outw(0x0003, 0xAC1C);

	rev = inw(0xAC1E);
	return (rev == 0x4132) ? 1 : 0;
}

That should work on any AMD VSA derivatives.  There are some who may have
rolled their own, and they don't support the same virtual registers.  in
that case, you can check if SMIs are enabled - that would be a reasonable
indication that there is something VSAish out there.

But you bring up another point - I'm not sure that Martin _doesn't_ have
VSA.  He is initializing this as a PCI driver, so he has something out
there listening to CF8/CFC, or he's emulating the OLPC spoof mechanism [1].
If its the former, and it doesn't handle the 5536 IDE config space, then
thats something that should be fixed by the BIOS vendor just on general
principle (other OSes aren't as lucky as we are working around the PCI
space).  If its the OLPC method, then I could argue that fudging the
registers belongs in that code, and not in the driver.

Jordan

[1] - http://dev.laptop.org/git?p=olpc-2.6;a=blob;f=arch/i386/pci/olpc.c;h=1518d254c5e3a58a27a396afa3f27d06785fc337;hb=HEAD

(I refer this only as an example, and not something to be emulated by other
platforms.  This works great for OLPC, but it would be a nightmare for
arbitrary platforms).

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

Re: pata_cs5536: ATA driver for Geode companion chip

[Martin K. Petersen]

>>>>> "Jordan" == Jordan Crouse <jordan.crouse@amd.com> writes:

Jordan> That should work on any AMD VSA derivatives.  There are some
Jordan> who may have rolled their own, and they don't support the same
Jordan> virtual registers.  in that case, you can check if SMIs are
Jordan> enabled - that would be a reasonable indication that there is
Jordan> something VSAish out there.

I'll try this later and see what comes up.  But I'm not sure I see the
advantage of having two ways to configure the 5536.

I was simply interested in getting things configured correctly.  I
agree that ideally all platforms would simply Do The Right Thing.
However, I took the pragmatic/safe approach and used the MSRs to
configure the beast.  The only reason I even tried is that this is a
setup issue exclusively.  Had the actual I/O interface been hiding in
a similar fashion I wouldn't have bothered.

Early on I also had some funny results with PIO so another reason I
wrote pata_cs5536 was to use the values specified in the databook.  It
seemed cleaner to have a tiny separate driver than to wedge another
set of timings into pata_amd.c.

I agree that it would be silly to have two drivers if there was actual
functionality lurking in them.  But as it stands most of the pata_*
drivers are simply configuration templates and nothing more.

Anyway.  That's my personal take on it.  If there's a general
consensus that the timings should be in pata_amd.c then so be it.


Jordan> But you bring up another point - I'm not sure that Martin
Jordan> _doesn't_ have VSA.  He is initializing this as a PCI driver,
Jordan> so he has something out there listening to CF8/CFC, or he's
Jordan> emulating the OLPC spoof mechanism [1]. 

The board I'm using has a BIOS which exposes the IDE registers in PCI
space but doesn't appear to listen when they are written to.  I'm
hoping that will get fixed eventually.


PS. So what's the story wrt. a PRD length of 0?

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

Re: pata_cs5536: ATA driver for Geode companion chip

[Jeff Garzik]

Martin K. Petersen wrote:
> PS. So what's the story wrt. a PRD length of 0?

That's what I'm quite interested in hearing, too...

	Jeff

Re: pata_cs5536: ATA driver for Geode companion chip

[Jordan Crouse]

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

> I'll try this later and see what comes up.  But I'm not sure I see the
> advantage of having two ways to configure the 5536.

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.

> Anyway.  That's my personal take on it.  If there's a general
> consensus that the timings should be in pata_amd.c then so be it.

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.

> The board I'm using has a BIOS which exposes the IDE registers in PCI
> space but doesn't appear to listen when they are written to.  I'm
> hoping that will get fixed eventually.

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.

> PS. So what's the story wrt. a PRD length of 0?

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.

Re: pata_cs5536: ATA driver for Geode companion chip

[Martin K. Petersen]

>>>>> "Jordan" == Jordan Crouse <jordan.crouse@amd.com> writes:

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/

Re: pata_cs5536: ATA driver for Geode companion chip

[Alan Cox]

> 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!

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

Re: pata_cs5536: ATA driver for Geode companion chip

[Alan Cox]

> 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!

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

Re: pata_cs5536: ATA driver for Geode companion chip

[Jordan Crouse]

On 04/10/07 16:58 +0100, Alan Cox wrote:
> > 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!
> 
> 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)

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.

Re: pata_cs5536: ATA driver for Geode companion chip

[Alan Cox]

> 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.

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.
> 
> Is there anything we can do to make the transition smooth for anybody who
> may be using pata_amd in 2.6.23?

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

Re: pata_cs5536: ATA driver for Geode companion chip

[Martin K. Petersen]

>>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

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/

Re: pata_cs5536: ATA driver for Geode companion chip

[Martin K. Petersen]

>>>>> "Jordan" == Jordan Crouse <jordan.crouse@amd.com> writes:

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/

[PATCH] pata_cs5536: ATA driver for Geode companion chip (PCI)

[Martin K. Petersen]

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.

Signed-off-by: Martin K. Petersen <mkp@mkp.net>

---

diff -r de183081194a drivers/ata/Kconfig
--- a/drivers/ata/Kconfig	Tue Oct 02 18:00:56 2007 +0000
+++ b/drivers/ata/Kconfig	Fri Oct 05 01:04:49 2007 -0400
@@ -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
--- a/drivers/ata/Makefile	Tue Oct 02 18:00:56 2007 +0000
+++ b/drivers/ata/Makefile	Fri Oct 05 01:04:49 2007 -0400
@@ -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
--- a/drivers/ata/pata_amd.c	Tue Oct 02 18:00:56 2007 +0000
+++ b/drivers/ata/pata_amd.c	Fri Oct 05 01:04:49 2007 -0400
@@ -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
 
 	{ },
 };
diff -r de183081194a drivers/ata/pata_cs5536.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/ata/pata_cs5536.c	Fri Oct 05 01:04:49 2007 -0400
@@ -0,0 +1,295 @@
+/*
+ * pata_cs5536.c	- CS5536 PATA for new ATA layer
+ *			  (C) 2007 Martin K. Petersen <mkp@mkp.net>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Documentation:
+ *	Available from AMD web site.
+ *
+ * The IDE timing registers for the CS5536 live in the Geode Machine
+ * Specific Register file and not PCI config space.  Most BIOSes
+ * virtualize the PCI registers so the chip looks like a standard IDE
+ * controller.  Unfortunately not all implementations get this right.
+ * In particular some have problems with unaligned accesses to the
+ * virtualized PCI registers.  This driver always does full dword
+ * writes to work around the issue.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/libata.h>
+#include <scsi/scsi_host.h>
+
+#define DRV_NAME	"pata_cs5536"
+#define DRV_VERSION	"0.0.4"
+
+enum {
+	IDE_CFG			= 0x40,
+	IDE_CFG_CHANEN		= 0x2,
+	IDE_CFG_CABLE		= 0x42,
+
+	IDE_D0_SHIFT		= 24,
+	IDE_D1_SHIFT		= 16,
+	IDE_DRV_MASK		= 0xff,
+
+	IDE_DTC			= 0x48,
+
+	IDE_CAST		= 0x4c,
+	IDE_CAST_D0_SHIFT	= 6,
+	IDE_CAST_D1_SHIFT	= 4,
+	IDE_CAST_DRV_MASK	= 0x3,
+	IDE_CAST_CMD_MASK	= 0xff,
+	IDE_CAST_CMD_SHIFT	= 24,
+
+	IDE_ETC			= 0x50,
+	IDE_ETC_NODMA		= 0x03,
+};
+
+/**
+ *	cs5536_cable_detect	-	detect cable type
+ *	@ap: Port to detect on
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Perform cable detection for ATA66 capable cable. Return a libata
+ *	cable type.
+ */
+
+static int cs5536_cable_detect(struct ata_port *ap)
+{
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	u8 reg;
+
+	pci_read_config_byte(pdev, IDE_CFG_CABLE, &reg);
+
+	if (reg & (1 << ap->port_no))
+		return ATA_CBL_PATA80;
+	else
+		return ATA_CBL_PATA40;
+}
+
+/**
+ *	cs5536_set_piomode		-	PIO setup
+ *	@ap: ATA interface
+ *	@adev: device on the interface
+ */
+
+static void cs5536_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	static const u8 drv_timings[5] = {
+		0x98, 0x55, 0x32, 0x21, 0x20
+	};
+
+	static const u8 addr_timings[5] = {
+		0x2, 0x1, 0x0, 0x0, 0x0
+	};
+
+	static const u8 cmd_timings[5] = {
+		0x99, 0x92, 0x90, 0x22, 0x20
+	};
+
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	struct ata_device *pair = ata_dev_pair(adev);
+	int mode = adev->pio_mode - XFER_PIO_0;
+	int cmdmode = mode;
+	int dshift = ap->port_no ? IDE_D1_SHIFT : IDE_D0_SHIFT;
+	int cshift = ap->port_no ? IDE_CAST_D1_SHIFT : IDE_CAST_D0_SHIFT;
+	u32 dtc, cast, etc;
+
+	if (pair)
+		cmdmode = min(mode, pair->pio_mode - XFER_PIO_0);
+
+	pci_read_config_dword(pdev, IDE_DTC, &dtc);
+	pci_read_config_dword(pdev, IDE_CAST, &cast);
+	pci_read_config_dword(pdev, IDE_ETC, &etc);
+
+	dtc &= ~(IDE_DRV_MASK << dshift);
+	dtc |= drv_timings[mode] << dshift;
+
+	cast &= ~(IDE_CAST_DRV_MASK << cshift);
+	cast |= addr_timings[mode] << cshift;
+
+	cast &= ~(IDE_CAST_CMD_MASK << IDE_CAST_CMD_SHIFT);
+	cast |= cmd_timings[cmdmode] << IDE_CAST_CMD_SHIFT;
+
+	etc &= ~(IDE_DRV_MASK << dshift);
+	etc |= IDE_ETC_NODMA << dshift;
+
+	pci_write_config_dword(pdev, IDE_DTC, dtc);
+	pci_write_config_dword(pdev, IDE_CAST, cast);
+	pci_write_config_dword(pdev, IDE_ETC, etc);
+}
+
+/**
+ *	cs5536_set_dmamode		-	DMA timing setup
+ *	@ap: ATA interface
+ *	@adev: Device being configured
+ *
+ */
+
+static void cs5536_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+	static const u8 udma_timings[6] = {
+		0xc2, 0xc1, 0xc0, 0xc4, 0xc5, 0xc6
+	};
+
+	static const u8 mwdma_timings[3] = {
+		0x67, 0x21, 0x20
+	};
+
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	u32 dtc, etc;
+	int mode = adev->dma_mode;
+	int dshift = ap->port_no ? IDE_D1_SHIFT  : IDE_D0_SHIFT;
+
+	if (mode >= XFER_UDMA_0) {
+		pci_read_config_dword(pdev, IDE_ETC, &etc);
+
+		etc &= ~(IDE_DRV_MASK << dshift);
+		etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
+
+		pci_write_config_dword(pdev, IDE_ETC, etc);
+	} else { /* MWDMA */
+		pci_read_config_dword(pdev, IDE_DTC, &dtc);
+
+		dtc &= ~(IDE_DRV_MASK << dshift);
+		dtc |= mwdma_timings[mode] << dshift;
+
+		pci_write_config_dword(pdev, IDE_DTC, dtc);
+	}
+}
+
+static struct scsi_host_template cs5536_sht = {
+	.module			= THIS_MODULE,
+	.name			= DRV_NAME,
+	.ioctl			= ata_scsi_ioctl,
+	.queuecommand		= ata_scsi_queuecmd,
+	.can_queue		= ATA_DEF_QUEUE,
+	.this_id		= ATA_SHT_THIS_ID,
+	.sg_tablesize		= LIBATA_MAX_PRD,
+	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
+	.emulated		= ATA_SHT_EMULATED,
+	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.proc_name		= DRV_NAME,
+	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.slave_configure	= ata_scsi_slave_config,
+	.slave_destroy		= ata_scsi_slave_destroy,
+	.bios_param		= ata_std_bios_param,
+};
+
+static struct ata_port_operations cs5536_port_ops = {
+	.port_disable	= ata_port_disable,
+	.set_piomode	= cs5536_set_piomode,
+	.set_dmamode	= cs5536_set_dmamode,
+	.mode_filter	= ata_pci_default_filter,
+
+	.tf_load	= ata_tf_load,
+	.tf_read	= ata_tf_read,
+	.check_status	= ata_check_status,
+	.exec_command	= ata_exec_command,
+	.dev_select	= ata_std_dev_select,
+
+	.freeze		= ata_bmdma_freeze,
+	.thaw		= ata_bmdma_thaw,
+	.error_handler	= ata_bmdma_error_handler,
+	.post_internal_cmd = ata_bmdma_post_internal_cmd,
+	.cable_detect	= cs5536_cable_detect,
+
+	.bmdma_setup	= ata_bmdma_setup,
+	.bmdma_start	= ata_bmdma_start,
+	.bmdma_stop	= ata_bmdma_stop,
+	.bmdma_status	= ata_bmdma_status,
+
+	.qc_prep	= ata_qc_prep,
+	.qc_issue	= ata_qc_issue_prot,
+
+	.data_xfer	= ata_data_xfer,
+
+	.irq_handler	= ata_interrupt,
+	.irq_clear	= ata_bmdma_irq_clear,
+	.irq_on		= ata_irq_on,
+	.irq_ack	= ata_irq_ack,
+
+	.port_start	= ata_port_start,
+};
+
+/**
+ *	cs5536_init_one
+ *	@dev: PCI device
+ *	@id: Entry in match table
+ *
+ */
+
+static int cs5536_init_one(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	static const struct ata_port_info info = {
+		.sht = &cs5536_sht,
+		.flags = ATA_FLAG_SLAVE_POSS,
+		.pio_mask = 0x1f,
+		.mwdma_mask = 0x07,
+		.udma_mask = ATA_UDMA5,
+		.port_ops = &cs5536_port_ops
+	};
+
+	const struct ata_port_info *ppi[] = { &info, &ata_dummy_port_info };
+	u8 reg;
+
+	pci_read_config_byte(dev, IDE_CFG, &reg);
+
+	if ((reg & IDE_CFG_CHANEN) == 0) {
+		printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
+		return -ENODEV;
+	}
+
+	return ata_pci_init_one(dev, ppi);
+}
+
+static const struct pci_device_id cs5536[] = {
+	{ PCI_VDEVICE(AMD,	PCI_DEVICE_ID_AMD_CS5536_IDE), },
+	{ },
+};
+
+static struct pci_driver cs5536_pci_driver = {
+	.name		= DRV_NAME,
+	.id_table	= cs5536,
+	.probe		= cs5536_init_one,
+	.remove		= ata_pci_remove_one,
+#ifdef CONFIG_PM
+	.suspend	= ata_pci_device_suspend,
+	.resume		= ata_pci_device_resume,
+#endif
+};
+
+static int __init cs5536_init(void)
+{
+	return pci_register_driver(&cs5536_pci_driver);
+}
+
+static void __exit cs5536_exit(void)
+{
+	pci_unregister_driver(&cs5536_pci_driver);
+}
+
+MODULE_AUTHOR("Martin K. Petersen");
+MODULE_DESCRIPTION("low-level driver for the CS5536 IDE controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, cs5536);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(cs5536_init);
+module_exit(cs5536_exit);

[PATCH] pata_cs5536: ATA driver for Geode companion chip (MSR)

[Martin K. Petersen]

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.

Signed-off-by: Martin K. Petersen <mkp@mkp.net>

---

diff -r de183081194a drivers/ata/Kconfig
--- a/drivers/ata/Kconfig	Tue Oct 02 18:00:56 2007 +0000
+++ b/drivers/ata/Kconfig	Fri Oct 05 01:20:22 2007 -0400
@@ -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
--- a/drivers/ata/Makefile	Tue Oct 02 18:00:56 2007 +0000
+++ b/drivers/ata/Makefile	Fri Oct 05 01:20:22 2007 -0400
@@ -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
--- a/drivers/ata/pata_amd.c	Tue Oct 02 18:00:56 2007 +0000
+++ b/drivers/ata/pata_amd.c	Fri Oct 05 01:20:22 2007 -0400
@@ -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
 
 	{ },
 };
diff -r de183081194a drivers/ata/pata_cs5536.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/ata/pata_cs5536.c	Fri Oct 05 01:20:22 2007 -0400
@@ -0,0 +1,298 @@
+/*
+ * pata_cs5536.c	- CS5536 PATA for new ATA layer
+ *			  (C) 2007 Martin K. Petersen <mkp@mkp.net>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Documentation:
+ *	Available from AMD web site.
+ *
+ * Like its predecessor (CS5535), CS5536 exposes its IDE configuration
+ * registers to the Geode Machine Specific Register file instead of
+ * PCI config space.  The 5536 IDE registers are modeled after the AMD
+ * 74xx PCI IDE controller although some of the timing knobs are
+ * subtly different.  Initially the CS5536 was supported by the
+ * pata_amd.c driver as some Geode platforms emulate the PCI config
+ * register support in firmware.  This driver, however, does not
+ * depend on PCI config emulation and accesses the MSR registers
+ * directly.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/libata.h>
+#include <scsi/scsi_host.h>
+#include <asm/msr.h>
+
+#define DRV_NAME	"pata_cs5536"
+#define DRV_VERSION	"0.0.3"
+
+enum {
+	MSR_IDE_BASE		= 0x51300000,
+
+	MSR_IDE_CFG		= (MSR_IDE_BASE + 0x10),
+	MSR_IDE_DTC		= (MSR_IDE_BASE + 0x12),
+	MSR_IDE_CAST		= (MSR_IDE_BASE + 0x13),
+	MSR_IDE_ETC		= (MSR_IDE_BASE + 0x14),
+
+	IDE_CFG_CHANEN          = 0x2,
+	IDE_CFG_CABLE           = 0x10000,
+	IDE_CFG_CABLE_MASK      = 0x30000,
+
+	IDE_D0_SHIFT		= 24,
+	IDE_D1_SHIFT		= 16,
+	IDE_DRV_MASK		= 0xff,
+
+	IDE_CAST_D0_SHIFT	= 6,
+	IDE_CAST_D1_SHIFT	= 4,
+	IDE_CAST_DRV_MASK	= 0x3,
+	IDE_CAST_CMD_MASK	= 0xff,
+	IDE_CAST_CMD_SHIFT	= 24,
+
+	IDE_ETC_NODMA		= 0x03,
+};
+
+/**
+ *	cs5536_cable_detect	-	detect cable type
+ *	@ap: Port to detect on
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Perform cable detection for ATA66 capable cable. Return a libata
+ *	cable type.
+ */
+
+static int cs5536_cable_detect(struct ata_port *ap)
+{
+	u32 reg, dummy;
+
+	rdmsr(MSR_IDE_CFG, reg, dummy);
+
+	if (reg & (IDE_CFG_CABLE << ap->port_no))
+		return ATA_CBL_PATA80;
+	else
+		return ATA_CBL_PATA40;
+}
+
+/**
+ *	cs5536_set_piomode		-	PIO setup
+ *	@ap: ATA interface
+ *	@adev: device on the interface
+ */
+
+static void cs5536_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+	static const u8 drv_timings[5] = {
+		0x98, 0x55, 0x32, 0x21, 0x20
+	};
+
+	static const u8 addr_timings[5] = {
+		0x2, 0x1, 0x0, 0x0, 0x0
+	};
+
+	static const u8 cmd_timings[5] = {
+		0x99, 0x92, 0x90, 0x22, 0x20
+	};
+
+	struct ata_device *pair = ata_dev_pair(adev);
+	int mode = adev->pio_mode - XFER_PIO_0;
+	int cmdmode = mode;
+	int dshift = ap->port_no ? IDE_D1_SHIFT : IDE_D0_SHIFT;
+	int cshift = ap->port_no ? IDE_CAST_D1_SHIFT : IDE_CAST_D0_SHIFT;
+	u32 dtc, cast, etc, dummy;
+
+	if (pair)
+		cmdmode = min(mode, pair->pio_mode - XFER_PIO_0);
+
+	rdmsr(MSR_IDE_DTC, dtc, dummy);
+	rdmsr(MSR_IDE_CAST, cast, dummy);
+	rdmsr(MSR_IDE_ETC, etc, dummy);
+
+	dtc &= ~(IDE_DRV_MASK << dshift);
+	dtc |= drv_timings[mode] << dshift;
+
+	cast &= ~(IDE_CAST_DRV_MASK << cshift);
+	cast |= addr_timings[mode] << cshift;
+
+	cast &= ~(IDE_CAST_CMD_MASK << IDE_CAST_CMD_SHIFT);
+	cast |= cmd_timings[cmdmode] << IDE_CAST_CMD_SHIFT;
+
+	etc &= ~(IDE_DRV_MASK << dshift);
+	etc |= IDE_ETC_NODMA << dshift;
+
+	wrmsr(MSR_IDE_DTC, dtc, dummy);
+	wrmsr(MSR_IDE_CAST, cast, dummy);
+	wrmsr(MSR_IDE_ETC, etc, dummy);
+}
+
+/**
+ *	cs5536_set_dmamode		-	DMA timing setup
+ *	@ap: ATA interface
+ *	@adev: Device being configured
+ *
+ */
+
+static void cs5536_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+	static const u8 udma_timings[6] = {
+		0xc2, 0xc1, 0xc0, 0xc4, 0xc5, 0xc6
+	};
+
+	static const u8 mwdma_timings[3] = {
+		0x67, 0x21, 0x20
+	};
+
+	u32 dtc, etc, dummy;
+	int mode = adev->dma_mode;
+	int dshift = ap->port_no ? IDE_D1_SHIFT  : IDE_D0_SHIFT;
+
+	if (mode >= XFER_UDMA_0) {
+		rdmsr(MSR_IDE_ETC, etc, dummy);
+
+		etc &= ~(IDE_DRV_MASK << dshift);
+		etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
+
+		wrmsr(MSR_IDE_ETC, etc, dummy);
+	} else { /* MWDMA */
+		rdmsr(MSR_IDE_DTC, dtc, dummy);
+
+		dtc &= ~(IDE_DRV_MASK << dshift);
+		dtc |= mwdma_timings[mode] << dshift;
+
+		wrmsr(MSR_IDE_DTC, dtc, dummy);
+	}
+}
+
+static struct scsi_host_template cs5536_sht = {
+	.module			= THIS_MODULE,
+	.name			= DRV_NAME,
+	.ioctl			= ata_scsi_ioctl,
+	.queuecommand		= ata_scsi_queuecmd,
+	.can_queue		= ATA_DEF_QUEUE,
+	.this_id		= ATA_SHT_THIS_ID,
+	.sg_tablesize		= LIBATA_MAX_PRD,
+	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
+	.emulated		= ATA_SHT_EMULATED,
+	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.proc_name		= DRV_NAME,
+	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.slave_configure	= ata_scsi_slave_config,
+	.slave_destroy		= ata_scsi_slave_destroy,
+	.bios_param		= ata_std_bios_param,
+};
+
+static struct ata_port_operations cs5536_port_ops = {
+	.port_disable	= ata_port_disable,
+	.set_piomode	= cs5536_set_piomode,
+	.set_dmamode	= cs5536_set_dmamode,
+	.mode_filter	= ata_pci_default_filter,
+
+	.tf_load	= ata_tf_load,
+	.tf_read	= ata_tf_read,
+	.check_status	= ata_check_status,
+	.exec_command	= ata_exec_command,
+	.dev_select	= ata_std_dev_select,
+
+	.freeze		= ata_bmdma_freeze,
+	.thaw		= ata_bmdma_thaw,
+	.error_handler	= ata_bmdma_error_handler,
+	.post_internal_cmd = ata_bmdma_post_internal_cmd,
+	.cable_detect	= cs5536_cable_detect,
+
+	.bmdma_setup	= ata_bmdma_setup,
+	.bmdma_start	= ata_bmdma_start,
+	.bmdma_stop	= ata_bmdma_stop,
+	.bmdma_status	= ata_bmdma_status,
+
+	.qc_prep	= ata_qc_prep,
+	.qc_issue	= ata_qc_issue_prot,
+
+	.data_xfer	= ata_data_xfer,
+
+	.irq_handler	= ata_interrupt,
+	.irq_clear	= ata_bmdma_irq_clear,
+	.irq_on		= ata_irq_on,
+	.irq_ack	= ata_irq_ack,
+
+	.port_start	= ata_port_start,
+};
+
+/**
+ *	cs5536_init_one
+ *	@dev: PCI device
+ *	@id: Entry in match table
+ *
+ */
+
+static int cs5536_init_one(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	static const struct ata_port_info info = {
+		.sht = &cs5536_sht,
+		.flags = ATA_FLAG_SLAVE_POSS,
+		.pio_mask = 0x1f,
+		.mwdma_mask = 0x07,
+		.udma_mask = ATA_UDMA5,
+		.port_ops = &cs5536_port_ops
+	};
+
+	const struct ata_port_info *ppi[] = { &info, &ata_dummy_port_info };
+	u32 cfg, dummy;
+
+	rdmsr(MSR_IDE_CFG, cfg, dummy);
+
+	if ((cfg & IDE_CFG_CHANEN) == 0) {
+		printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
+		return -ENODEV;
+	}
+
+	return ata_pci_init_one(dev, ppi);
+}
+
+static const struct pci_device_id cs5536[] = {
+	{ PCI_VDEVICE(AMD,	PCI_DEVICE_ID_AMD_CS5536_IDE), },
+	{ },
+};
+
+static struct pci_driver cs5536_pci_driver = {
+	.name		= DRV_NAME,
+	.id_table	= cs5536,
+	.probe		= cs5536_init_one,
+	.remove		= ata_pci_remove_one,
+#ifdef CONFIG_PM
+	.suspend	= ata_pci_device_suspend,
+	.resume		= ata_pci_device_resume,
+#endif
+};
+
+static int __init cs5536_init(void)
+{
+	return pci_register_driver(&cs5536_pci_driver);
+}
+
+static void __exit cs5536_exit(void)
+{
+	pci_unregister_driver(&cs5536_pci_driver);
+}
+
+MODULE_AUTHOR("Martin K. Petersen");
+MODULE_DESCRIPTION("low-level driver for the CS5536 IDE controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, cs5536);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(cs5536_init);
+module_exit(cs5536_exit);

Re: [PATCH] pata_cs5536: ATA driver for Geode companion chip (MSR)

[Alan Cox]

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

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


> +#ifndef CONFIG_PATA_CS5536	/* Temporary */

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

otherwise ACK


Alan

Re: pata_cs5536: ATA driver for Geode companion chip (MSR)

[Jordan Crouse]

On 05/10/07 13:01 +0100, Alan Cox wrote:
> > pata_cs5536.c relies on Geode Machine Specific Registers to configure
> > the ATA function and uses the correct PIO timings for the chip.
> 
> Any reason for not supporting both methods ? Though I guess as you say
> msr is sufficient.
> 
> 
> > +#ifndef CONFIG_PATA_CS5536	/* Temporary */
> 
> 	!defined(FOO) && ! defined(FOO_MODULE)
> 
> otherwise ACK

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.

Re: [PATCH] pata_cs5536: ATA driver for Geode companion chip (MSR)

[Martin K. Petersen]

>>>>> "Alan" == Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

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

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/

Re: pata_cs5536: ATA driver for Geode companion chip

[Jordan Crouse]

On 10/10/07 00:33 -0400, Martin K. Petersen wrote:
> >>>>> "Jeff" == Jeff Garzik <jeff@garzik.org> writes:
> 
> Jeff> Just FWIW...  one you guys are happy with things, I'm ready to
> Jeff> merge the driver whenever.
> 
> I'm waiting for Jordan to get back to me with feedback on test results
> on other boards as well as preference wrt. MSR or PCI configuration...

I never heard back from anybody - so either nobody is using pata_amd
(which I suspect), or they didn't have any problems.  Go ahead and merge,
and I'll do a sanity check on a few boards next week just to make sure.

Thanks,
Jordan

> -- 
> Martin K. Petersen	Oracle Linux Engineering
> 
> 
> 

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

Re: pata_cs5536: ATA driver for Geode companion chip

[Andrew Paprocki]

On 10/10/07, Jordan Crouse <jordan.crouse@amd.com> wrote:
> I never heard back from anybody - so either nobody is using pata_amd
> (which I suspect), or they didn't have any problems.  Go ahead and merge,
> and I'll do a sanity check on a few boards next week just to make sure.

I am using the CS5536 currently with pata_amd and it worked without
issue. I only used the PATA port for a short time, though, before
switching to USB + SATA. I will now be going back to using the PATA
this week, so I'll try out the new driver in place of pata_amd and
write back if there are any problems.

Thanks,
-Andrew

Re: pata_cs5536: ATA driver for Geode companion chip

[Andrew Paprocki]

Martin,

Just wanted to report that the 2.6.23 libata-dev with the MSR
pata_cs5536 applied to it is working fine with my LX board. I am using
the first PATA port as my boot/root drive right now.

Thanks,
-Andrew

scsi4 : pata_cs5536
scsi5 : pata_cs5536
ata5: PATA max UDMA/100 cmd 0x000101f0 ctl 0x000103f6 bmdma 0x0001ff00 irq 14
ata6: DUMMY
ata5.00: ATA-0: , 060729DA, max MWDMA2
ata5.00: 256000 sectors, multi 1: LBA
ata5.00: configured for MWDMA2
scsi 4:0:0:0: Direct-Access     ATA                       0607 PQ: 0 ANSI: 5
sd 4:0:0:0: [sdc] 256000 512-byte hardware sectors (131 MB)
sd 4:0:0:0: [sdc] Write Protect is off
sd 4:0:0:0: [sdc] Mode Sense: 00 3a 00 00
sd 4:0:0:0: [sdc] Write cache: disabled, read cache: enabled, doesn't
support DPO or FUA