[PATCH] ahci: Fix long standing Marvell regressions

[PATCH] ahci: Fix long standing Marvell regressions

[Alan Cox]

pata_marvell: Undo the regressions Jeff caused

From: Alan Cox <alan@redhat.com>

I've been chasing Jeff about this for months so am now giving up and sending
it directly. Jeff added the Marvell device identifiers to the ahci driver
without making the AHCI driver handle the PATA port. This means a lot of users
can't use current kernels and in most distro cases can't even install. This has
been going on since March 2008 for the 6121 Marvell, and late 2007 for the 6145.

This was all pointed out at the time and repeatedly ignored. Bugs assigned
to Jeff about this are ignored also.

To quote Jeff in email

"Just switch the order of 'ahci' and 'pata_marvell' in
/etc/modprobe.conf, then use Fedora's tools regenerate the initrd.

See?  It's not rocket science, and the current configuration can be
easily made to work for Fedora users."

(Which isn't trivial, isn't end user, shouldn't be needed, and as it usually
breaks at install time is in fact impossible)

To quote Jeff in August 2007

"   mv-ahci-pata
Marvell 6121/6141 PATA support.  Needs fixing in the 'PATA controller
command' area before it is usable, and can go upstream."

Only he applied it all anyway later and broke everything.

The actual fix for the moment is very simple. If the user has included
the pata_marvell driver let it drive the ports. If they've only selected
for SATA support give them the AHCI driver which will run the port a fraction
faster.


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

 drivers/ata/Kconfig |    6 ++++--
 drivers/ata/ahci.c  |    5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)


diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ae84949..11c8c19 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -448,8 +448,10 @@ config PATA_MARVELL
 	tristate "Marvell PATA support via legacy mode"
 	depends on PCI
 	help
-	  This option enables limited support for the Marvell 88SE6145 ATA
-	  controller.
+	  This option enables limited support for the Marvell 88SE61xx ATA
+	  controllers. If you wish to use only the SATA ports then select
+	  the AHCI driver alone. If you wish to the use the PATA port or
+	  both SATA and PATA include this driver.
 
 	  If unsure, say N.
 
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c729e69..546865a 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -581,9 +581,14 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(SI, 0x1185), board_ahci },		/* SiS 968 */
 	{ PCI_VDEVICE(SI, 0x0186), board_ahci },		/* SiS 968 */
 
+#if  !defined(CONFIG_PATA_MARVELL) && !defined(CONFIG_PATA_MARVELL_MODULE)
 	/* Marvell */
+	/* The AHCI driver can only drive the SATA ports, the PATA driver
+	   can drive them all so if both drivers are selected make sure
+	   AHCI stays out of the way */
 	{ PCI_VDEVICE(MARVELL, 0x6145), board_ahci_mv },	/* 6145 */
 	{ PCI_VDEVICE(MARVELL, 0x6121), board_ahci_mv },	/* 6121 */
+#endif
 
 	/* Generic, PCI class code for AHCI */
 	{ PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 3740 bytes --]

Alan Cox wrote:
> pata_marvell: Undo the regressions Jeff caused
> 
> From: Alan Cox <alan@redhat.com>
> 
> I've been chasing Jeff about this for months so am now giving up and sending
> it directly. Jeff added the Marvell device identifiers to the ahci driver
> without making the AHCI driver handle the PATA port. This means a lot of users
> can't use current kernels and in most distro cases can't even install. This has
> been going on since March 2008 for the 6121 Marvell, and late 2007 for the 6145.
> 
> This was all pointed out at the time and repeatedly ignored. Bugs assigned
> to Jeff about this are ignored also.
> 
> To quote Jeff in email
> 
> "Just switch the order of 'ahci' and 'pata_marvell' in
> /etc/modprobe.conf, then use Fedora's tools regenerate the initrd.
> 
> See?  It's not rocket science, and the current configuration can be
> easily made to work for Fedora users."
> 
> (Which isn't trivial, isn't end user, shouldn't be needed, and as it usually
> breaks at install time is in fact impossible)
> 
> To quote Jeff in August 2007
> 
> "   mv-ahci-pata
> Marvell 6121/6141 PATA support.  Needs fixing in the 'PATA controller
> command' area before it is usable, and can go upstream."
> 
> Only he applied it all anyway later and broke everything.

Completely and utterly false.  libata-dev.git#mv-ahci-pata has not yet 
gone upstream, as is blatantly clear if you had bothered to LOOK.

It helps to look at source code before posting, Alan:

         /*
          * Temporary Marvell 6145 hack: PATA port presence
          * is asserted through the standard AHCI port
          * presence register, as bit 4 (counting from 0)
          */
         if (hpriv->flags & AHCI_HFLAG_MV_PATA) {
                 if (pdev->device == 0x6121)
                         mv = 0x3;
                 else
                         mv = 0xf;

Clearly PATA support is not applied, because the code masks out the PATA 
ports.


> The actual fix for the moment is very simple. If the user has included
> the pata_marvell driver let it drive the ports. If they've only selected
> for SATA support give them the AHCI driver which will run the port a fraction
> faster.

You omit details that hurt your case, namely that a ton of users were 
asking for AHCI support EVEN IF IT IS SATA ONLY, because the SATA 
support via pata_marvell legacy mode was so poor and problematic.

Among the pata_marvell complaints I received that were solved were:

* bloody awful error handling, with no hotplug support
* problems with suspend/resume (apparently some BIOS assumed you were in 
AHCI/enhanced mode)
* problems with speed negotiation of 3.0 Gbps SATA devices

So any claim that SATA always worked wonderfully under pata_marvell is 
specious.



Finally,

> +#if  !defined(CONFIG_PATA_MARVELL) && !defined(CONFIG_PATA_MARVELL_MODULE)
>  	/* Marvell */

this prevents users from choosing the driver that works best for them, 
including bringing back all the SATA problems just as you bring back PATA.

The attached patch is more reasonable, but neither your nor my patch 
actually address the relevant problem:  distros choosing the module.

So at this point, applying your patch would create regressions just as 
it purports to solve regressions, since Marvell AHCI is out there in 
active use as well.


How to move forward?  I recommend,

1) Applying my attached patch

2) Communicating with distros

3) See if we can get Marvell docs to someone willing to code in support. 
  Marvell has been very responsive in the past year with regards to 
'mvsas', so maybe they can help here too.

I kept hoping Marvell would pick up my unfinished #mv-ahci-pata work and 
polish it up.  They indicated interest, but kept putting it off.

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 729 bytes --]

diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 674965f..56462ee 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -1,6 +1,8 @@
 
 obj-$(CONFIG_ATA)		+= libata.o
 
+obj-$(CONFIG_PATA_MARVELL)	+= pata_marvell.o
+
 obj-$(CONFIG_SATA_AHCI)		+= ahci.o
 obj-$(CONFIG_SATA_SVW)		+= sata_svw.o
 obj-$(CONFIG_ATA_PIIX)		+= ata_piix.o
@@ -47,7 +49,6 @@ obj-$(CONFIG_PATA_NS87415)	+= pata_ns87415.o
 obj-$(CONFIG_PATA_OPTI)		+= pata_opti.o
 obj-$(CONFIG_PATA_OPTIDMA)	+= pata_optidma.o
 obj-$(CONFIG_PATA_MPC52xx)	+= pata_mpc52xx.o
-obj-$(CONFIG_PATA_MARVELL)	+= pata_marvell.o
 obj-$(CONFIG_PATA_MPIIX)	+= pata_mpiix.o
 obj-$(CONFIG_PATA_OLDPIIX)	+= pata_oldpiix.o
 obj-$(CONFIG_PATA_PCMCIA)	+= pata_pcmcia.o

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Alan Cox]

> Completely and utterly false.  libata-dev.git#mv-ahci-pata has not yet 
> gone upstream, as is blatantly clear if you had bothered to LOOK.

Oh I beg your pardon - you only pushed the PCI identifiers that broke
stuff rather than the code that went with it you never finished. So does
that actually sound better "hey this was broken, not merged but I pushed
the IDs part *knowing* it would cause serious regressions" ?

Bit rich telling me to LOOK (emphasis yours) when your last email says

"Situation before ahci mod:

	Users with PATA may use pata_marvell.
	Users wanting SATA screwed."

Jeff - pata_marvell has always supported SATA.

> You omit details that hurt your case, namely that a ton of users were 
> asking for AHCI support EVEN IF IT IS SATA ONLY, because the SATA 
> support via pata_marvell legacy mode was so poor and problematic.

Actually it just works. You don't get Phy diagnostics that is about all
you lose (and NCQ for performance).

> Among the pata_marvell complaints I received that were solved were:
 
> * bloody awful error handling, with no hotplug support

Full SFF error handling. Warmplug but not automatic hotplug detect. But
hey at least you *can install the damned machine* and the machine that
was working in FC 8 might actually still be working.

(But you clearly didn't know this because your last email to linux-ide
claimed the pata_marvell driver didn't support the SATA ports)

> * problems with suspend/resume (apparently some BIOS assumed you were in 
> AHCI/enhanced mode)

Some assume you are not too.

> > +#if  !defined(CONFIG_PATA_MARVELL) && !defined(CONFIG_PATA_MARVELL_MODULE)
> >  	/* Marvell */
> 
> this prevents users from choosing the driver that works best for them, 
> including bringing back all the SATA problems just as you bring back PATA.

Not really, its in the config script. Oh that hard for users, then tell
me why 'editing the initrd' before installing is acceptable ?

At runtime it might make sense to allow ahci.marvell=1. I can add that in
about 60 seconds if you want now you've finally been forced to stop
ignoring the issue. That would allow the distros to ship pata_marvell and
users to say "actually I want full SATA support".

Unfortunately there isn't a documented way to check the 6121 PATA port
has a drive attached so pata_marvell can punt if it isn't.

> The attached patch is more reasonable, but neither your nor my patch 
> actually address the relevant problem:  distros choosing the module.

Currently distributions can't compile in pata_marvell and remove the
broken ahci support for it. That is a *huge* problem.

> So at this point, applying your patch would create regressions just as 
> it purports to solve regressions, since Marvell AHCI is out there in 
> active use as well.

A mess *you* created by repeatedly ignoring the problems that were
pointed out to you and then ignoring attempts to raise it. Meanwhile I've
been getting zillions of bug reports and emails about it. You dug the
hole and ran away with your fingers in your ears while I got all the bugs
and complaints.

> 1) Applying my attached patch

Your patch is a no-op. No distribution of any note builds them
non-modular. You know this so I find it incredulous you propose this
no-op change. Trying to pull the wool over Linus eyes ?

> 2) Communicating with distros

Well you've been ignoring bugzilla for months so doing something about
that would be a start. It isn't the distros who've not been communicating.

> 3) See if we can get Marvell docs to someone willing to code in support. 
>   Marvell has been very responsive in the past year with regards to 
> 'mvsas', so maybe they can help here too.

or to provide docs on the pata port. I'm assuming its probably as trivial
as "issue one command at a time". In SFF mode the chip does all the
management itself so it's obvious smart enough to do the work.

But your proposed patch is a joke and you *know* it.

Alan

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Alan Cox]

> 1) Applying my attached patch

This is another way of doing it that again unlike your joke 'patch'
actually isn't a pointless no-op. Push the decision into a config
variable with module option. Also document that the PATA port is being
disabled so the end user knows who screwed up their machine and how to
undo it.

Default is pata_marvell = use this driver, no pata_marvell use AHCI, but
the default can be reversed in either direction using
ahci.marvell_enable=0/1.

--

pata_marvell: Undo multiple ahci caused regressions

From: Alan Cox <alan@redhat.com>

I've been chasing Jeff about this for months so am now giving up and sending
it directly. Jeff added the Marvell device identifiers to the ahci driver
without making the AHCI driver handle the PATA port. This means a lot of users
can't use current kernels and in most distro cases can't even install.

This has been going on since March 2008 for the 6121 Marvell, and late 2007
for the 6145!!!

This was all pointed out at the time and repeatedly ignored. Bugs assigned
to Jeff about this are ignored also.

To quote Jeff in email

"Just switch the order of 'ahci' and 'pata_marvell' in
/etc/modprobe.conf, then use Fedora's tools regenerate the initrd.

See?  It's not rocket science, and the current configuration can be
easily made to work for Fedora users."

(Which isn't trivial, isn't end user, shouldn't be needed, and as it usually
breaks at install time is in fact impossible)

To quote Jeff in August 2007

"   mv-ahci-pata
Marvell 6121/6141 PATA support.  Needs fixing in the 'PATA controller
command' area before it is usable, and can go upstream."

Only he add the ids anyway later and caused regressions, adding a further
id in March causing more regresions.

The actual fix for the moment is very simple. If the user has included
the pata_marvell driver let it drive the ports. If they've only selected
for SATA support give them the AHCI driver which will run the port a fraction
faster. Allow the user to control this decision via ahci.marvell_enable as
a module parameter so that distributions can ship 'it works' defaults and
smarter users (or config tools) can then flip it over it desired.


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

 drivers/ata/Kconfig |    6 ++++--
 drivers/ata/ahci.c  |   19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)


diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ae84949..11c8c19 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -448,8 +448,10 @@ config PATA_MARVELL
 	tristate "Marvell PATA support via legacy mode"
 	depends on PCI
 	help
-	  This option enables limited support for the Marvell 88SE6145 ATA
-	  controller.
+	  This option enables limited support for the Marvell 88SE61xx ATA
+	  controllers. If you wish to use only the SATA ports then select
+	  the AHCI driver alone. If you wish to the use the PATA port or
+	  both SATA and PATA include this driver.
 
 	  If unsure, say N.
 
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c729e69..91d2a47 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -610,6 +610,15 @@ module_param(ahci_em_messages, int, 0444);
 MODULE_PARM_DESC(ahci_em_messages,
 	"Set AHCI Enclosure Management Message type (0 = disabled, 1 = LED");
 
+#if defined(CONFIG_PATA_MARVELL) || defined(CONFIG_PATA_MARVELL_MODULE)
+static int marvell_enable;
+#else
+static int marvell_enable = 1;
+#endif
+module_param(marvell_enable, int, 0644);
+MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
+
+
 static inline int ahci_nr_ports(u32 cap)
 {
 	return (cap & 0x1f) + 1;
@@ -732,6 +741,8 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 			   "MV_AHCI HACK: port_map %x -> %x\n",
 			   port_map,
 			   port_map & mv);
+		dev_printk(KERN_ERR, &pdev->dev,
+			  "Disabling your PATA port. Use the boot option 'ahci.marvell_enable=0' to avoid this.\n");
 
 		port_map &= mv;
 	}
@@ -2533,6 +2544,14 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
+#if  !defined(CONFIG_PATA_MARVELL) && !defined(CONFIG_PATA_MARVELL_MODULE)
+	/* The AHCI driver can only drive the SATA ports, the PATA driver
+	   can drive them all so if both drivers are selected make sure
+	   AHCI stays out of the way */
+	if (pdev->vendor == PCI_VENDOR_ID_MARVELL && !marvell_enable)
+		return -ENODEV;
+#endif
+
 	/* acquire resources */
 	rc = pcim_enable_device(pdev);
 	if (rc)

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Jeff Garzik]

Alan Cox wrote:
> The actual fix for the moment is very simple. If the user has included
> the pata_marvell driver let it drive the ports. If they've only selected
> for SATA support give them the AHCI driver which will run the port a fraction
> faster. Allow the user to control this decision via ahci.marvell_enable as
> a module parameter so that distributions can ship 'it works' defaults and
> smarter users (or config tools) can then flip it over it desired.
> 
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> ---
> 
>  drivers/ata/Kconfig |    6 ++++--
>  drivers/ata/ahci.c  |   19 +++++++++++++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)

[snip]

That's far more reasonable, and I'd be happy to queue that with your 
full email text...

Unless you object, I'll roll this up with a couple other libata fixes 
for 2.6.27-rc.

	Jeff

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Alan Cox]

And here's a rev 3

- Fix the option so it works both ways
- We can using the 6145 detect PATA enabled so implement a facility in
pata_marvell to punt to AHCI if disabled

We can't automate that for the other devices as we don't actually know
how to check the PATA port on them.

--

pata_marvell: Undo multiple ahci caused regressions

From: Alan Cox <alan@redhat.com>

I've been chasing Jeff about this for months so am now giving up and sending
it directly. Jeff added the Marvell device identifiers to the ahci driver
without making the AHCI driver handle the PATA port. This means a lot of users
can't use current kernels and in most distro cases can't even install.

This has been going on since March 2008 for the 6121 Marvell, and late 2007
for the 6145!!!

This was all pointed out at the time and repeatedly ignored. Bugs assigned
to Jeff about this are ignored also.

To quote Jeff in email

"Just switch the order of 'ahci' and 'pata_marvell' in
/etc/modprobe.conf, then use Fedora's tools regenerate the initrd.

See?  It's not rocket science, and the current configuration can be
easily made to work for Fedora users."

(Which isn't trivial, isn't end user, shouldn't be needed, and as it usually
breaks at install time is in fact impossible)

To quote Jeff in August 2007

"   mv-ahci-pata
Marvell 6121/6141 PATA support.  Needs fixing in the 'PATA controller
command' area before it is usable, and can go upstream."

Only he add the ids anyway later and caused regressions, adding a further
id in March causing more regresions.

The actual fix for the moment is very simple. If the user has included
the pata_marvell driver let it drive the ports. If they've only selected
for SATA support give them the AHCI driver which will run the port a fraction
faster. Allow the user to control this decision via ahci.marvell_enable as
a module parameter so that distributions can ship 'it works' defaults and
smarter users (or config tools) can then flip it over it desired.


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

 drivers/ata/Kconfig        |    6 +++--
 drivers/ata/ahci.c         |   17 +++++++++++++++
 drivers/ata/pata_marvell.c |   51 +++++++++++++++++++++++++++++++++-----------
 3 files changed, 59 insertions(+), 15 deletions(-)


diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ae84949..11c8c19 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -448,8 +448,10 @@ config PATA_MARVELL
 	tristate "Marvell PATA support via legacy mode"
 	depends on PCI
 	help
-	  This option enables limited support for the Marvell 88SE6145 ATA
-	  controller.
+	  This option enables limited support for the Marvell 88SE61xx ATA
+	  controllers. If you wish to use only the SATA ports then select
+	  the AHCI driver alone. If you wish to the use the PATA port or
+	  both SATA and PATA include this driver.
 
 	  If unsure, say N.
 
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c729e69..bce26ee 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -610,6 +610,15 @@ module_param(ahci_em_messages, int, 0444);
 MODULE_PARM_DESC(ahci_em_messages,
 	"Set AHCI Enclosure Management Message type (0 = disabled, 1 = LED");
 
+#if defined(CONFIG_PATA_MARVELL) || defined(CONFIG_PATA_MARVELL_MODULE)
+static int marvell_enable;
+#else
+static int marvell_enable = 1;
+#endif
+module_param(marvell_enable, int, 0644);
+MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
+
+
 static inline int ahci_nr_ports(u32 cap)
 {
 	return (cap & 0x1f) + 1;
@@ -732,6 +741,8 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 			   "MV_AHCI HACK: port_map %x -> %x\n",
 			   port_map,
 			   port_map & mv);
+		dev_printk(KERN_ERR, &pdev->dev,
+			  "Disabling your PATA port. Use the boot option 'ahci.marvell_enable=0' to avoid this.\n");
 
 		port_map &= mv;
 	}
@@ -2533,6 +2544,12 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
+	/* The AHCI driver can only drive the SATA ports, the PATA driver
+	   can drive them all so if both drivers are selected make sure
+	   AHCI stays out of the way */
+	if (pdev->vendor == PCI_VENDOR_ID_MARVELL && !marvell_enable)
+		return -ENODEV;
+
 	/* acquire resources */
 	rc = pcim_enable_device(pdev);
 	if (rc)
diff --git a/drivers/ata/pata_marvell.c b/drivers/ata/pata_marvell.c
index 24a011b..0d87eec 100644
--- a/drivers/ata/pata_marvell.c
+++ b/drivers/ata/pata_marvell.c
@@ -20,29 +20,30 @@
 #include <linux/ata.h>
 
 #define DRV_NAME	"pata_marvell"
-#define DRV_VERSION	"0.1.4"
+#define DRV_VERSION	"0.1.6"
 
 /**
- *	marvell_pre_reset	-	check for 40/80 pin
- *	@link: link
- *	@deadline: deadline jiffies for the operation
+ *	marvell_pata_active	-	check if PATA is active
+ *	@pdev: PCI device
  *
- *	Perform the PATA port setup we need.
+ *	Returns 1 if the PATA port may be active. We know how to check this
+ *	for the 6145 but not the other devices
  */
 
-static int marvell_pre_reset(struct ata_link *link, unsigned long deadline)
+static int marvell_pata_active(struct pci_dev *pdev)
 {
-	struct ata_port *ap = link->ap;
-	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	int i;
 	u32 devices;
 	void __iomem *barp;
-	int i;
 
-	/* Check if our port is enabled */
+	/* We don't yet know how to do this for other devices */
+	if (pdev->device != 0x6145)
+		return 1;	
 
 	barp = pci_iomap(pdev, 5, 0x10);
 	if (barp == NULL)
 		return -ENOMEM;
+
 	printk("BAR5:");
 	for(i = 0; i <= 0x0F; i++)
 		printk("%02X:%02X ", i, ioread8(barp + i));
@@ -51,9 +52,27 @@ static int marvell_pre_reset(struct ata_link *link, unsigned long deadline)
 	devices = ioread32(barp + 0x0C);
 	pci_iounmap(pdev, barp);
 
-	if ((pdev->device == 0x6145) && (ap->port_no == 0) &&
-	    (!(devices & 0x10)))	/* PATA enable ? */
-		return -ENOENT;
+	if (devices & 0x10)
+		return 1;
+	return 0;
+}
+
+/**
+ *	marvell_pre_reset	-	check for 40/80 pin
+ *	@link: link
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Perform the PATA port setup we need.
+ */
+
+static int marvell_pre_reset(struct ata_link *link, unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+
+	if (pdev->device == 0x6145 && ap->port_no == 0 &&
+		!marvell_pata_active(pdev))	/* PATA enable ? */
+			return -ENOENT;
 
 	return ata_sff_prereset(link, deadline);
 }
@@ -128,6 +147,12 @@ static int marvell_init_one (struct pci_dev *pdev, const struct pci_device_id *i
 	if (pdev->device == 0x6101)
 		ppi[1] = &ata_dummy_port_info;
 
+#if defined(CONFIG_AHCI) || defined(CONFIG_AHCI_MODULE)
+	if (!marvell_pata_active(pdev)) {
+		printk(KERN_INFO DRV_NAME ": PATA port not active, deferring to AHCI driver.\n");
+		return -ENODEV;
+	}
+#endif
 	return ata_pci_sff_init_one(pdev, ppi, &marvell_sht, NULL);
 }
 

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Jeff Garzik]

Alan Cox wrote:
> And here's a rev 3
> 
> - Fix the option so it works both ways
> - We can using the 6145 detect PATA enabled so implement a facility in
> pata_marvell to punt to AHCI if disabled
> 
> We can't automate that for the other devices as we don't actually know
> how to check the PATA port on them.

Nice... superceding your rev 2 in my queue for 2.6.27-rc.

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Richard A Nelson]

On Wed, 3 Sep 2008, Alan Cox wrote:

> We can't automate that for the other devices as we don't actually know
> how to check the PATA port on them.

I happen to have one of the others:
 	Marvell Technology Group Ltd. 88SE6121 SATA II Controller (rev b1)

It is not in use, likely as a result of the issues at hand, I don't
recall exactly what transpired at the time.

I have no specs, but would be happy to run any software diagnostics
if they'd be of help
-- 
Rick Nelson
"I would rather spend 10 hours reading someone else's source code than
10 minutes listening to Musak waiting for technical support which isn't."
(By Dr. Greg Wettstein, Roger Maris Cancer Center)

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Alan Cox]

On Wed, 3 Sep 2008 11:54:36 -0700 (PDT)
Richard A Nelson <cowboy@cavein.org> wrote:

> On Wed, 3 Sep 2008, Alan Cox wrote:
> 
> > We can't automate that for the other devices as we don't actually know
> > how to check the PATA port on them.
> 
> I happen to have one of the others:
>  	Marvell Technology Group Ltd. 88SE6121 SATA II Controller (rev b1)
> 
> It is not in use, likely as a result of the issues at hand, I don't
> recall exactly what transpired at the time.
> 
> I have no specs, but would be happy to run any software diagnostics
> if they'd be of help

Load the pata_marvell driver and it dumps out BAR5: .... lists of values

Can you do that with a PATA device attached and with no PATA device
attached and see what changes.

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 84 bytes --]

applied.  I edited the text a bit, for formatting and clarity only (see 
attached)


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 6646 bytes --]

commit 5b66c829bf5c65663b2f68ee6b42f6e834cd39cd
Author: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date:   Wed Sep 3 14:48:34 2008 +0100

    ahci, pata_marvell: play nicely together
    
    I've been chasing Jeff about this for months.  Jeff added the Marvell
    device identifiers to the ahci driver without making the AHCI driver
    handle the PATA port. This means a lot of users can't use current
    kernels and in most distro cases can't even install.
    
    This has been going on since March 2008 for the 6121 Marvell, and late 2007
    for the 6145!!!
    
    This was all pointed out at the time and repeatedly ignored. Bugs assigned
    to Jeff about this are ignored also.
    
    To quote Jeff in email
    
    > "Just switch the order of 'ahci' and 'pata_marvell' in
    > /etc/modprobe.conf, then use Fedora's tools regenerate the initrd.
    
    > See?  It's not rocket science, and the current configuration can be
    > easily made to work for Fedora users."
    
    (Which isn't trivial, isn't end user, shouldn't be needed, and as it usually
    breaks at install time is in fact impossible)
    
    To quote Jeff in August 2007
    
    > "   mv-ahci-pata
    > Marvell 6121/6141 PATA support.  Needs fixing in the 'PATA controller
    > command' area before it is usable, and can go upstream."
    
    Only he add the ids anyway later and caused regressions, adding a further
    id in March causing more regresions.
    
    The actual fix for the moment is very simple. If the user has included
    the pata_marvell driver let it drive the ports. If they've only selected
    for SATA support give them the AHCI driver which will run the port a fraction
    faster. Allow the user to control this decision via ahci.marvell_enable as
    a module parameter so that distributions can ship 'it works' defaults and
    smarter users (or config tools) can then flip it over it desired.
    
    Signed-off-by: Alan Cox <alan@redhat.com>
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ae84949..11c8c19 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -448,8 +448,10 @@ config PATA_MARVELL
 	tristate "Marvell PATA support via legacy mode"
 	depends on PCI
 	help
-	  This option enables limited support for the Marvell 88SE6145 ATA
-	  controller.
+	  This option enables limited support for the Marvell 88SE61xx ATA
+	  controllers. If you wish to use only the SATA ports then select
+	  the AHCI driver alone. If you wish to the use the PATA port or
+	  both SATA and PATA include this driver.
 
 	  If unsure, say N.
 
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c729e69..bce26ee 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -610,6 +610,15 @@ module_param(ahci_em_messages, int, 0444);
 MODULE_PARM_DESC(ahci_em_messages,
 	"Set AHCI Enclosure Management Message type (0 = disabled, 1 = LED");
 
+#if defined(CONFIG_PATA_MARVELL) || defined(CONFIG_PATA_MARVELL_MODULE)
+static int marvell_enable;
+#else
+static int marvell_enable = 1;
+#endif
+module_param(marvell_enable, int, 0644);
+MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
+
+
 static inline int ahci_nr_ports(u32 cap)
 {
 	return (cap & 0x1f) + 1;
@@ -732,6 +741,8 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
 			   "MV_AHCI HACK: port_map %x -> %x\n",
 			   port_map,
 			   port_map & mv);
+		dev_printk(KERN_ERR, &pdev->dev,
+			  "Disabling your PATA port. Use the boot option 'ahci.marvell_enable=0' to avoid this.\n");
 
 		port_map &= mv;
 	}
@@ -2533,6 +2544,12 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
+	/* The AHCI driver can only drive the SATA ports, the PATA driver
+	   can drive them all so if both drivers are selected make sure
+	   AHCI stays out of the way */
+	if (pdev->vendor == PCI_VENDOR_ID_MARVELL && !marvell_enable)
+		return -ENODEV;
+
 	/* acquire resources */
 	rc = pcim_enable_device(pdev);
 	if (rc)
diff --git a/drivers/ata/pata_marvell.c b/drivers/ata/pata_marvell.c
index 24a011b..0d87eec 100644
--- a/drivers/ata/pata_marvell.c
+++ b/drivers/ata/pata_marvell.c
@@ -20,29 +20,30 @@
 #include <linux/ata.h>
 
 #define DRV_NAME	"pata_marvell"
-#define DRV_VERSION	"0.1.4"
+#define DRV_VERSION	"0.1.6"
 
 /**
- *	marvell_pre_reset	-	check for 40/80 pin
- *	@link: link
- *	@deadline: deadline jiffies for the operation
+ *	marvell_pata_active	-	check if PATA is active
+ *	@pdev: PCI device
  *
- *	Perform the PATA port setup we need.
+ *	Returns 1 if the PATA port may be active. We know how to check this
+ *	for the 6145 but not the other devices
  */
 
-static int marvell_pre_reset(struct ata_link *link, unsigned long deadline)
+static int marvell_pata_active(struct pci_dev *pdev)
 {
-	struct ata_port *ap = link->ap;
-	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	int i;
 	u32 devices;
 	void __iomem *barp;
-	int i;
 
-	/* Check if our port is enabled */
+	/* We don't yet know how to do this for other devices */
+	if (pdev->device != 0x6145)
+		return 1;	
 
 	barp = pci_iomap(pdev, 5, 0x10);
 	if (barp == NULL)
 		return -ENOMEM;
+
 	printk("BAR5:");
 	for(i = 0; i <= 0x0F; i++)
 		printk("%02X:%02X ", i, ioread8(barp + i));
@@ -51,9 +52,27 @@ static int marvell_pre_reset(struct ata_link *link, unsigned long deadline)
 	devices = ioread32(barp + 0x0C);
 	pci_iounmap(pdev, barp);
 
-	if ((pdev->device == 0x6145) && (ap->port_no == 0) &&
-	    (!(devices & 0x10)))	/* PATA enable ? */
-		return -ENOENT;
+	if (devices & 0x10)
+		return 1;
+	return 0;
+}
+
+/**
+ *	marvell_pre_reset	-	check for 40/80 pin
+ *	@link: link
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Perform the PATA port setup we need.
+ */
+
+static int marvell_pre_reset(struct ata_link *link, unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+
+	if (pdev->device == 0x6145 && ap->port_no == 0 &&
+		!marvell_pata_active(pdev))	/* PATA enable ? */
+			return -ENOENT;
 
 	return ata_sff_prereset(link, deadline);
 }
@@ -128,6 +147,12 @@ static int marvell_init_one (struct pci_dev *pdev, const struct pci_device_id *i
 	if (pdev->device == 0x6101)
 		ppi[1] = &ata_dummy_port_info;
 
+#if defined(CONFIG_AHCI) || defined(CONFIG_AHCI_MODULE)
+	if (!marvell_pata_active(pdev)) {
+		printk(KERN_INFO DRV_NAME ": PATA port not active, deferring to AHCI driver.\n");
+		return -ENODEV;
+	}
+#endif
 	return ata_pci_sff_init_one(pdev, ppi, &marvell_sht, NULL);
 }
 

Re: [PATCH] ahci: Fix long standing Marvell regressions

[Chuck Ebbert]

Jeff Garzik wrote:
> applied.  I edited the text a bit, for formatting and clarity only (see 
> attached)
> 
 >   ahci, pata_marvell: play nicely together

I can't see how that's going to work with a 6145 adapter and no PATA devices
attached. The AHCI driver will always defer to pata_marvell (unless
marvell_enable=1 is passed) and pata_marvell will defer to ahci when it doesn't
see any PATA devices.