diff mbox

libata-sff/pata_cmd64x problem with hardwired configurations

Message ID 201104191120.31061.bzolnier@gmail.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Bartlomiej Zolnierkiewicz April 19, 2011, 9:20 a.m. UTC 
Hi,

Sergei Shtylyov wrote:

> Hello.
> 
> James Bottomley wrote:
> 
> >>> I can get all of this working by fixing up all the hard coded knowledge
> >>> in libata-sff only to use a single port.
> >>> However, I can't fix the libata-sff driver until I know how to tell
> >>> there's only one port wired.  Does anyone with cmd649 knowledge have any
> >>> idea how I might tell this?
> 
> >>     The secondary port is enabled in the PCI config. space: register 0x51 bit 
> >> 3 controls this. Unfortunately, pata_cmd64x driver still doesn't check the 
> >> channel enable bits; the cmd64x driver does though, so it might be worth trying...
> 
> > So this is the enablebits code in driver/ide
> 
>     We still need to ensure that these are indeed the enable bits that are at 
> play here...
> 
> > that's missing from any of
> > the libata stuff?
> 
>     Not from any. It's handled in some drivers, like pata_hpt37x, for example -- 
> in their prereset() methods.
> 
> > Should this be generic in libata-sff? ...
> 
>     There's even libata core function, pci_test_config_bits() that facilitates 
> this check.
> 
> > I mean even on an x86 where arbitrary memory can be poked without consequence,
> > trying to activate a disabled port will still produce lots of noise.
> 
>     The story with pata_cmd64x is somewhat convoluted: originally (when the 
> libata driver was created) in the IDE driver there were only enable bits for the 
> secondary channel of PCI0646 (and even those wrong); I fixed those and added the 
> enable bits for other chips back in 2007 but it never got propagated to 
> pata_cmd64x. Well, I have some plans for changes to this driver but not much 
> time to actually do them currently...

From atang tree, orginally posted in 2009:

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH v2] pata_cmd64x: add enablebits checking

Fixes IDE -> libata regression.

IDE's cmd64x host driver has been supporting enablebits checking
since the initial driver's merge.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/pata_cmd64x.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)





--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Alan Cox April 19, 2011, 10:08 a.m. UTC | #1 
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH v2] pata_cmd64x: add enablebits checking
> 
> Fixes IDE -> libata regression.

And causes a regression too

With the check for parent bridge not being a split bridge I think however
it'll do the job nicely.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz April 19, 2011, 11:50 a.m. UTC | #2 
On Tue, Apr 19, 2011 at 12:08 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Subject: [PATCH v2] pata_cmd64x: add enablebits checking
>>
>> Fixes IDE -> libata regression.
>
> And causes a regression too

Could you be more explicit?

Regression in this patch would mean that old driver currently doesn't
work which is not true..

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox April 19, 2011, 12:29 p.m. UTC | #3 
On Tue, 19 Apr 2011 13:50:12 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> On Tue, Apr 19, 2011 at 12:08 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >> Subject: [PATCH v2] pata_cmd64x: add enablebits checking
> >>
> >> Fixes IDE -> libata regression.
> >
> > And causes a regression too
> 
> Could you be more explicit?
> 
> Regression in this patch would mean that old driver currently doesn't
> work which is not true..

The old driver fails on some of the hot plug boxes because the enable
bits don't seem to be valid. See the rest of the discussion on this thread

Hence the comment I made about needing to also check the parent bridge

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley April 19, 2011, 1:59 p.m. UTC | #4 
On Tue, 2011-04-19 at 11:20 +0200, Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH v2] pata_cmd64x: add enablebits checking
> 
> Fixes IDE -> libata regression.
> 
> IDE's cmd64x host driver has been supporting enablebits checking
> since the initial driver's merge.

Actually, the thread discussing the proposed patches is here:

http://marc.info/?t=130315227100005

I much prefer the dummy interface approach to the prereset one because
it prevents any possible poke at the registers which will crash the box.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: b/drivers/ata/pata_cmd64x.c
===================================================================
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -79,6 +79,40 @@  static int cmd648_cable_detect(struct at
 }
 
 /**
+ *	cmd64x_prereset	-	perform reset handling
+ *	@link: ATA link
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Reset sequence checking enable bits to see which ports are
+ *	active.
+ */
+
+static int cmd64x_prereset(struct ata_link *link, unsigned long deadline)
+{
+	static const struct pci_bits cmd64x_enable_bits[] = {
+		{ 0x51, 1, 0x04, 0x04 },
+		{ 0x51, 1, 0x08, 0x08 }
+	};
+
+	struct ata_port *ap = link->ap;
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+
+	/*
+	 * The original PCI0643 and PCI0646 didn't have the primary
+	 * channel enable bit, it appeared starting with PCI0646U
+	 * (i.e. revision ID 3).
+	 */
+	if (ap->port_no == 0 && (pdev->device == PCI_DEVICE_ID_CMD_643 ||
+	    (pdev->device == PCI_DEVICE_ID_CMD_646 && pdev->revision < 3)))
+		goto out;
+
+	if (!pci_test_config_bits(pdev, &cmd64x_enable_bits[ap->port_no]))
+		return -ENOENT;
+out:
+	return ata_sff_prereset(link, deadline);
+}
+
+/**
  *	cmd64x_set_timing	-	set PIO and MWDMA timing
  *	@ap: ATA interface
  *	@adev: ATA device
@@ -266,6 +300,7 @@  static const struct ata_port_operations
 	.inherits	= &ata_bmdma_port_ops,
 	.set_piomode	= cmd64x_set_piomode,
 	.set_dmamode	= cmd64x_set_dmamode,
+	.prereset	= cmd64x_prereset,
 };
 
 static struct ata_port_operations cmd64x_port_ops = {