libata-sff/pata_cmd64x problem with hardwired configurations

libata-sff/pata_cmd64x problem with hardwired configurations

[James Bottomley]

I've got a parisc system where the DVD drive is hardwired to a silicon
image controller:

00:02.0 IDE interface: Silicon Image, Inc. SiI 0649 Ultra ATA/100 PCI to
ATA Host Controller (rev 02) (prog-if 8f [Master SecP SecO PriP PriO])
        Subsystem: Silicon Image, Inc. SiI 0649 Ultra ATA/100 PCI to ATA
Host Controller
        Flags: bus master, medium devsel, latency 64, IRQ 69
        I/O ports at 0d18 [size=8]
        I/O ports at 0d24 [size=4]
        I/O ports at 0d10 [size=8]
        I/O ports at 0d20 [size=4]
        I/O ports at 0d00 [size=16]
        Capabilities: [60] Power Management version 2
        Kernel driver in use: pata_cmd64x

The specific problem is that any access to the registers where the
secondary port should be causes an instant fault on the box (I think
because the second port just isn't wired up internally, so the memory
doesn't respond), so the default libata-sff driver that pata_cmd64x is
attached to causes this by insisting on probing both ports.

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?

James

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[Sergei Shtylyov]

Hello.

On 18-04-2011 3:58, James Bottomley wrote:

> I've got a parisc system where the DVD drive is hardwired to a silicon
> image controller:

> 00:02.0 IDE interface: Silicon Image, Inc. SiI 0649 Ultra ATA/100 PCI to
> ATA Host Controller (rev 02) (prog-if 8f [Master SecP SecO PriP PriO])
>          Subsystem: Silicon Image, Inc. SiI 0649 Ultra ATA/100 PCI to ATA
> Host Controller
>          Flags: bus master, medium devsel, latency 64, IRQ 69
>          I/O ports at 0d18 [size=8]
>          I/O ports at 0d24 [size=4]
>          I/O ports at 0d10 [size=8]
>          I/O ports at 0d20 [size=4]
>          I/O ports at 0d00 [size=16]
>          Capabilities: [60] Power Management version 2
>          Kernel driver in use: pata_cmd64x

> The specific problem is that any access to the registers where the
> secondary port should be causes an instant fault on the box (I think
> because the second port just isn't wired up internally, so the memory
> doesn't respond), so the default libata-sff driver that pata_cmd64x is
> attached to causes this by insisting on probing both ports.

    Perhaps the secondary port is disabled (though it's strange that your 
lspci dump shows I/O resources for both ports allocated).

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

> James

WBR, Sergei

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[James Bottomley]

On Mon, 2011-04-18 at 14:12 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 18-04-2011 3:58, James Bottomley wrote:
> 
> > I've got a parisc system where the DVD drive is hardwired to a silicon
> > image controller:
> 
> > 00:02.0 IDE interface: Silicon Image, Inc. SiI 0649 Ultra ATA/100 PCI to
> > ATA Host Controller (rev 02) (prog-if 8f [Master SecP SecO PriP PriO])
> >          Subsystem: Silicon Image, Inc. SiI 0649 Ultra ATA/100 PCI to ATA
> > Host Controller
> >          Flags: bus master, medium devsel, latency 64, IRQ 69
> >          I/O ports at 0d18 [size=8]
> >          I/O ports at 0d24 [size=4]
> >          I/O ports at 0d10 [size=8]
> >          I/O ports at 0d20 [size=4]
> >          I/O ports at 0d00 [size=16]
> >          Capabilities: [60] Power Management version 2
> >          Kernel driver in use: pata_cmd64x
> 
> > The specific problem is that any access to the registers where the
> > secondary port should be causes an instant fault on the box (I think
> > because the second port just isn't wired up internally, so the memory
> > doesn't respond), so the default libata-sff driver that pata_cmd64x is
> > attached to causes this by insisting on probing both ports.
> 
>     Perhaps the secondary port is disabled (though it's strange that your 
> lspci dump shows I/O resources for both ports allocated).

It's a last minute wedgie into an enterprise system because they wanted
a DVD and there are no SCSI ones ... although why they didn't do USB 

> > 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 that's missing from any of
the libata stuff?  Should this be generic in libata-sff? ... 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.

James

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[Sergei Shtylyov]

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

> James

WBR, Sergei

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[Bartlomiej Zolnierkiewicz]

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(+)

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 = {

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[Alan Cox]

> 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

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[Bartlomiej Zolnierkiewicz]

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

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[Alan Cox]

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

Re: libata-sff/pata_cmd64x problem with hardwired configurations

[James Bottomley]

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