only one drive in a port multiplier system is being recognized

only one drive in a port multiplier system is being recognized

[Greg Hennessy]

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

I have a LYCeSATA-4x pci card that is port multiplier compatible (it works fine
under win xp) that is only showing one of 5 attached drives when attached to
a Redhat 5 computer, with both  2.6.18-8.1.15.el5 and 2.6.23.1. 

The SATA link shows as up for the one recognized drive, the other four
drives show the link down. 

Any sugguestions as to what may be the problem?

A subset of what I consider the relavant portion of dmesg 
for the
  2.6.18-8.1.15.el5 kernel is:
ahci 0000:00:1f.2: AHCI 0001.0200 32 slots 6 ports 3 Gbps 0x2f impl SATA mode
ahci 0000:00:1f.2: flags: 64bit ncq pm led clo pmp pio slum part 
ata1: SATA max UDMA/133 cmd 0xF882C100 ctl 0x0 bmdma 0x0 irq 233
ata2: SATA max UDMA/133 cmd 0xF882C180 ctl 0x0 bmdma 0x0 irq 233
ata3: SATA max UDMA/133 cmd 0xF882C200 ctl 0x0 bmdma 0x0 irq 233
ata4: SATA max UDMA/133 cmd 0xF882C280 ctl 0x0 bmdma 0x0 irq 233
ata5: SATA max UDMA/133 cmd 0xF882C300 ctl 0x0 bmdma 0x0 irq 233
ata6: SATA max UDMA/133 cmd 0xF882C380 ctl 0x0 bmdma 0x0 irq 233
scsi0 : ahci
usb 1-1: new low speed USB device using uhci_hcd and address 3
usb 1-1: configuration #1 chosen from 1 choice
input: Dell Dell USB Mouse as /class/input/input1
input: USB HID v1.10 Mouse [Dell Dell USB Mouse] on usb-0000:00:1a.0-1
usb 1-2: new full speed USB device using uhci_hcd and address 4
ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
ata1.00: ATA-7, max UDMA/133, 488281250 sectors: LBA48 NCQ (depth 31/32)
ata1.00: ata1: dev 0 multi count 0
ata1.00: configured for UDMA/133
scsi1 : ahci
usb 1-2: configuration #1 chosen from 1 choice
input: Dell Dell Smart Card Reader Keyboard as /class/input/input2
input: USB HID v1.11 Keyboard [Dell Dell Smart Card Reader Keyboard] on usb-0000:00:1a.0-2
ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
ata2.00: ATA-7, max UDMA/133, 488281250 sectors: LBA48 NCQ (depth 31/32)
ata2.00: ata2: dev 0 multi count 0
ata2.00: configured for UDMA/133
scsi2 : ahci
ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata3.00: ATAPI, max UDMA/100
ata3.00: configured for UDMA/100
scsi3 : ahci
ata4: SATA link down (SStatus 4 SControl 300)
scsi4 : ahci
ata5: SATA link down (SStatus 0 SControl 0)
scsi5 : ahci
ata6: SATA link down (SStatus 4 SControl 300)
  Vendor: ATA       Model: Hitachi HDT72502  Rev: V5DO
  Type:   Direct-Access                      ANSI SCSI revision: 05
SCSI device sda: 488281250 512-byte hdwr sectors (250000 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
SCSI device sda: 488281250 512-byte hdwr sectors (250000 MB)
sda: Write Protect is off
sda: Mode Sense: 00 3a 00 00
SCSI device sda: drive cache: write back
 sda: sda1
sd 0:0:0:0: Attached scsi disk sda
  Vendor: ATA       Model: Hitachi HDT72502  Rev: V5DO
  Type:   Direct-Access                      ANSI SCSI revision: 05
SCSI device sdb: 488281250 512-byte hdwr sectors (250000 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: drive cache: write back
SCSI device sdb: 488281250 512-byte hdwr sectors (250000 MB)
sdb: Write Protect is off
sdb: Mode Sense: 00 3a 00 00
SCSI device sdb: drive cache: write back
 sdb: sdb1
sd 1:0:0:0: Attached scsi disk sdb
  Vendor: PBDS      Model: DVD+-RW DH-16W1S  Rev: 2D14
  Type:   CD-ROM                             ANSI SCSI revision: 05
sata_sil24 0000:03:02.0: version 0.3
ACPI: PCI Interrupt 0000:03:02.0[A] -> GSI 18 (level, low) -> IRQ 217
ata7: SATA max UDMA/100 cmd 0xF8840000 ctl 0x0 bmdma 0x0 irq 217
ata8: SATA max UDMA/100 cmd 0xF8842000 ctl 0x0 bmdma 0x0 irq 217
ata9: SATA max UDMA/100 cmd 0xF8844000 ctl 0x0 bmdma 0x0 irq 217
ata10: SATA max UDMA/100 cmd 0xF8846000 ctl 0x0 bmdma 0x0 irq 217
scsi6 : sata_sil24
ata7: SATA link down (SStatus 0 SControl 300)
scsi7 : sata_sil24
ata8: SATA link down (SStatus 0 SControl 300)
scsi8 : sata_sil24
ata9: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
ata9.00: ATA-7, max UDMA/133, 1953525168 sectors: LBA48 NCQ (depth 31/32)
ata9.00: ata9: dev 0 multi count 16
ata9.00: configured for UDMA/100
scsi9 : sata_sil24
ata10: SATA link down (SStatus 0 SControl 300)
  Vendor: ATA       Model: Hitachi HDS72101  Rev: GKAO
  Type:   Direct-Access                      ANSI SCSI revision: 05
SCSI device sdc: 1953525168 512-byte hdwr sectors (1000205 MB)
sdc: Write Protect is off
sdc: Mode Sense: 00 3a 00 00
SCSI device sdc: drive cache: write back
SCSI device sdc: 1953525168 512-byte hdwr sectors (1000205 MB)
sdc: Write Protect is off
sdc: Mode Sense: 00 3a 00 00
SCSI device sdc: drive cache: write back
 sdc: sdc1
sd 8:0:0:0: Attached scsi disk sdc

[root@localhost ~]# fdisk -l

Disk /dev/sda: 250.0 GB, 250000000000 bytes
255 heads, 63 sectors/track, 30394 cylinders
Units = cylinders of 16065 * 512 = 8225280 bytes

   Device Boot      Start         End      Blocks   Id  System
/dev/sda1   *           1       30394   244139773+  83  Linux

Disk /dev/sdb: 250.0 GB, 250000000000 bytes
255 heads, 63 sectors/track, 30394 cylinders
Units = cylinders of 16065 * 512 = 8225280 bytes

   Device Boot      Start         End      Blocks   Id  System
/dev/sdb1               1       30394   244139773+  83  Linux

WARNING: GPT (GUID Partition Table) detected on '/dev/sdc'! The util fdisk doesn 't support GPT. Use GNU Parted.


Disk /dev/sdc: 1000.2 GB, 1000204886016 bytes
256 heads, 63 sectors/track, 121126 cylinders
Units = cylinders of 16128 * 512 = 8257536 bytes

   Device Boot      Start         End      Blocks   Id  System
/dev/sdc1               1      121126   976760032+  83  Linux


[-- Attachment #2: greg.hennessy.vcf --]
[-- Type: text/x-vcard, Size: 278 bytes --]

begin:vcard
fn:Greg Hennessy
n:Hennessy;Greg
org:USNO;Astrometry Department
adr:;;3450 Mass. Ave. NW;Washington;DC;20392;USA
email;internet:gsh@usno.navy.mil
title:Astronomer
tel;work:(202) 762-1523
tel;fax:(202) 762-1514
url:http://ad.usno.navy.mil/~gsh
version:2.1
end:vcard

Re: only one drive in a port multiplier system is being recognized

[Tejun Heo]

Greg Hennessy wrote:
> I have a LYCeSATA-4x pci card that is port multiplier compatible (it
> works fine
> under win xp) that is only showing one of 5 attached drives when
> attached to
> a Redhat 5 computer, with both  2.6.18-8.1.15.el5 and 2.6.23.1.
> The SATA link shows as up for the one recognized drive, the other four
> drives show the link down.
> Any sugguestions as to what may be the problem?

Stock 2.6.23.1 doesn't have PMP support.  2.6.24 will.  You need patches
from the following page.

http://home-tj.org/wiki/index.php/Libata-tj-stable

-- 
tejun

What's needed for PMP support?

[Tejun Heo]

(cc'd linux-ide and changed subject a bit)

Hello,

Mark Lord wrote:
> Can you spare a minute to udpate me on what is typically needed to get
> a SATA driver to support port multipliers ?

Sure thing.

> And which LLDs currently have it already?
> I have a sata_sil24 driven ExpressCard (for notebook) here that I may
> try out..

Currently only two driver support PMP.  One is sata_sil24 and the other
is ahci.  sata_sil24 supports FIS-based switching while ahci only
supports command based switching.

The following things are needed for a LLD to support PMP.

1. PMP aware reset sequence.  The controller must issue hardreset first,
followed by softreset with PMP==15.  The first part is automatically
done if the LLD tells libata that you support PMP by setting ATA_FLAG_PMP.

2. Reset methods for downstream ports.  A LLD needs prereset, hardreset,
softreset and postreset for downstream ports.  sata_pmp_std_prereset(),
sata_pmp_std_hardreset() and sata_pmp_std_postreset() can be used
without any modification unless the controller is really weird.  The
only thing a LLD has to implement is pmp_softreset method which does the
same SRST protocol as regular softreset but with SRST set to link->pmp.

3. An error handler to combine all methods from #1 and #2.  The error
handler should call sata_pmp_do_eh() with methods from #1 and #2.

4. PMP aware command issue.  qc_prep and qc_issue should be modified
such that when it issues commands via H2D Reg FIS, it sets the PMP field
according to qc->dev->link->pmp.

5. qc_defer method.  Use sata_pmp_qc_defer_cmd_switch() if it's command
based switching.  ata_std_qc_defer() can be used if the controller is
FIS based switching without any further restriction.

6. PMP aware irq / error handling.  The irq handler should be able to
handle completions for different ports.  There doesn't need to be much
change for command switching.  Error triggering / handling needs to be
updated too.  Again, cmd-based switching won't need too much change.
One thing to note is to attribute errors to the correct link (e.g. link
errors detected by host should be recorded to ehi of the upstream link
while any error condition triggered by the device should probably be
attributed to downstream link.)

I think that's about it.  Feel free to ask if something isn't clear.

Thanks.

-- 
tejun

Re: What's needed for PMP support?

[Mark Lord]

Tejun Heo wrote:
>
> The following things are needed for a LLD to support PMP.
..
> I think that's about it.  Feel free to ask if something isn't clear.
..

Tejun, I've added PMP to sata_mv, and am now trying to get it
to work with a Marvell PM attached.

And the behaviour I see is very bizarre.

After hard+soft resets, the PM signature is found,
and libata interrogates the PM registers.

It successfully reads register 0, and then register 1.
But all subsequent registers read out (incorrectly) as zeros.

I've traced the taskfiles in/out, and it all looks proper
except for the actual data coming back from the PM.

After some experimentation, I found that all of the PM registers
were readable, if I simple inserted a  sata_pmp_read(link, 0, &junk)
in front of each issue of sata_pmp_read(link, reg, &r_val).

Then the PM is recognized and all, but fails port enumerations
probably due to either my hack or the same original bug (whatever that is?)

I'm confused.

The PM itself works fine here on sata_sil24 and AHCI(jmicron),
so it's obviously the sata_mv driver or chipset that's being weird.

Ever seen anything like this before?
I'm trying to use stock libata functions for all of this where possible,
so there's not really that much new/necessary code in sata_mv for this.
I do force the PMP value for outgoing-FIS's (non standard register for it),
but that's about all that's custom here.

The driver uses ATA register mode emulation for all commands
other than READ/WRITE disk stuff, including for the PM register accesses.

Ever seen anything like this before?

Cheers
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: What's needed for PMP support?

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> Tejun, I've added PMP to sata_mv, and am now trying to get it
> to work with a Marvell PM attached.
> 
> And the behaviour I see is very bizarre.
> 
> After hard+soft resets, the PM signature is found,
> and libata interrogates the PM registers.
> 
> It successfully reads register 0, and then register 1.
> But all subsequent registers read out (incorrectly) as zeros.

Hmmm...

> I've traced the taskfiles in/out, and it all looks proper
> except for the actual data coming back from the PM.
> 
> After some experimentation, I found that all of the PM registers
> were readable, if I simple inserted a  sata_pmp_read(link, 0, &junk)
> in front of each issue of sata_pmp_read(link, reg, &r_val).

Hmmmmmmm...

> Then the PM is recognized and all, but fails port enumerations
> probably due to either my hack or the same original bug (whatever that is?)
> 
> I'm confused.

That makes two of us.

> The PM itself works fine here on sata_sil24 and AHCI(jmicron),

Yeah, Marvell PMPs behave very nicely with sil24 and ahci.

> so it's obviously the sata_mv driver or chipset that's being weird.
> 
> Ever seen anything like this before?

No.

> I'm trying to use stock libata functions for all of this 	where possible,
> so there's not really that much new/necessary code in sata_mv for this.
> I do force the PMP value for outgoing-FIS's (non standard register for it),
> but that's about all that's custom here.
> 
> The driver uses ATA register mode emulation for all commands
> other than READ/WRITE disk stuff, including for the PM register accesses.
> 
> Ever seen anything like this before?

No but I had my fair share of problems doing PMP support for both sil24
and ahci.  Any chance you can get access to a bus analyzer?  I had a
very weird problem w/ ahci PMP support which I don't think I could fix
if I didn't have access to a bus analyzer at the time.  (I don't
remember details now tho.)

-- 
tejun

Re: What's needed for PMP support?

[Mark Lord]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> Tejun, I've added PMP to sata_mv, and am now trying to get it
>> to work with a Marvell PM attached.
>>
>> And the behaviour I see is very bizarre.
>>
>> After hard+soft resets, the PM signature is found,
>> and libata interrogates the PM registers.
>>
>> It successfully reads register 0, and then register 1.
>> But all subsequent registers read out (incorrectly) as zeros.
> 
> Hmmm...
> 
>> I've traced the taskfiles in/out, and it all looks proper
>> except for the actual data coming back from the PM.
>>
>> After some experimentation, I found that all of the PM registers
>> were readable, if I simple inserted a  sata_pmp_read(link, 0, &junk)
>> in front of each issue of sata_pmp_read(link, reg, &r_val).
> 
> Hmmmmmmm...
> 
>> Then the PM is recognized and all, but fails port enumerations
>> probably due to either my hack or the same original bug (whatever that is?)
>>
>> I'm confused.
> 
> That makes two of us.
> 
>> The PM itself works fine here on sata_sil24 and AHCI(jmicron),
..

Well, last night I found someone else to try the code
with a 0x1095:0x3726 (vendor/device?) Port-Multiplier.
It *appears* to work fine with sata_mv, unlike the Marvell one.
Hopefully the nice folks at Marvell will take a look (I've emailed them)
and report back with some useful information eventually.

> No but I had my fair share of problems doing PMP support for both sil24
> and ahci.  Any chance you can get access to a bus analyzer?  I had a
> very weird problem w/ ahci PMP support which I don't think I could fix
> if I didn't have access to a bus analyzer at the time.  (I don't
> remember details now tho.)
..

I figure they must have a few of those kicking around at Marvell..

Cheers
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[PATCH] libata-pmp: clear hob for pmp register accesses

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>> Hello, Mark.
>>
>> Mark Lord wrote:
>>> Tejun, I've added PMP to sata_mv, and am now trying to get it
>>> to work with a Marvell PM attached.
>>>
>>> And the behaviour I see is very bizarre.
>>>
>>> After hard+soft resets, the PM signature is found,
>>> and libata interrogates the PM registers.
>>>
>>> It successfully reads register 0, and then register 1.
>>> But all subsequent registers read out (incorrectly) as zeros.
..

Saeed has confirmed this behaviour with a SATA analyzer.
The Marvell port-multiplier apparently likes to see clean HOB
information when accessing PMP registers.

Since sata_mv uses PIO shadow register access, this doesn't happen
automatically, as it might in a more purely FIS-based driver (eg. ahci).

One way to fix this is to flag these commands with ATA_TFLAG_LBA48,
forcing libata to write out the HOB fields with known (zero) values.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
Acked-by: Mark Lord <mlord@pobox.com>
---
 drivers/ata/libata-pmp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index caef2bb..d91f509 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -35,7 +35,7 @@ static unsigned int sata_pmp_read(struct ata_link *link, int reg, u32 *r_val)
 	ata_tf_init(pmp_dev, &tf);
 	tf.command = ATA_CMD_PMP_READ;
 	tf.protocol = ATA_PROT_NODATA;
-	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48;
 	tf.feature = reg;
 	tf.device = link->pmp;
 
@@ -71,7 +71,7 @@ static unsigned int sata_pmp_write(struct ata_link *link, int reg, u32 val)
 	ata_tf_init(pmp_dev, &tf);
 	tf.command = ATA_CMD_PMP_WRITE;
 	tf.protocol = ATA_PROT_NODATA;
-	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48;
 	tf.feature = reg;
 	tf.device = link->pmp;
 	tf.nsect = val & 0xff;

Re: [PATCH] libata-pmp: clear hob for pmp register accesses

[saeed bishara]

Hi,
 I just what to make it clear that this is not a bug of the Marvell's
PM, the sata specification (I'm looking at version 2.5) defines the
READ/WRITE PM registers to be extended commands, specifically, the
features HOB of the READ PM register defined to be RegNum[15:8].

saeed

On Thu, Feb 21, 2008 at 10:52 PM, Mark Lord <liml@rtr.ca> wrote:
> Mark Lord wrote:
>  > Tejun Heo wrote:
>  >> Hello, Mark.
>  >>
>  >> Mark Lord wrote:
>  >>> Tejun, I've added PMP to sata_mv, and am now trying to get it
>  >>> to work with a Marvell PM attached.
>  >>>
>  >>> And the behaviour I see is very bizarre.
>  >>>
>  >>> After hard+soft resets, the PM signature is found,
>  >>> and libata interrogates the PM registers.
>  >>>
>  >>> It successfully reads register 0, and then register 1.
>  >>> But all subsequent registers read out (incorrectly) as zeros.
>  ..
>
>  Saeed has confirmed this behaviour with a SATA analyzer.
>  The Marvell port-multiplier apparently likes to see clean HOB
>  information when accessing PMP registers.
>
>  Since sata_mv uses PIO shadow register access, this doesn't happen
>  automatically, as it might in a more purely FIS-based driver (eg. ahci).
>
>  One way to fix this is to flag these commands with ATA_TFLAG_LBA48,
>  forcing libata to write out the HOB fields with known (zero) values.
>
>  Signed-off-by: Saeed Bishara <saeed@marvell.com>
>  Acked-by: Mark Lord <mlord@pobox.com>
>  ---
>   drivers/ata/libata-pmp.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
>  diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
>  index caef2bb..d91f509 100644
>  --- a/drivers/ata/libata-pmp.c
>  +++ b/drivers/ata/libata-pmp.c
>  @@ -35,7 +35,7 @@ static unsigned int sata_pmp_read(struct ata_link *link, int reg, u32 *r_val)
>         ata_tf_init(pmp_dev, &tf);
>         tf.command = ATA_CMD_PMP_READ;
>         tf.protocol = ATA_PROT_NODATA;
>  -       tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
>  +       tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48;
>         tf.feature = reg;
>         tf.device = link->pmp;
>
>  @@ -71,7 +71,7 @@ static unsigned int sata_pmp_write(struct ata_link *link, int reg, u32 val)
>         ata_tf_init(pmp_dev, &tf);
>         tf.command = ATA_CMD_PMP_WRITE;
>         tf.protocol = ATA_PROT_NODATA;
>  -       tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
>  +       tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48;
>         tf.feature = reg;
>         tf.device = link->pmp;
>         tf.nsect = val & 0xff;
>  -
>  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
>

Re: [PATCH] libata-pmp: clear hob for pmp register accesses

[Tejun Heo]

Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Hello, Mark.
>>>
>>> Mark Lord wrote:
>>>> Tejun, I've added PMP to sata_mv, and am now trying to get it
>>>> to work with a Marvell PM attached.
>>>>
>>>> And the behaviour I see is very bizarre.
>>>>
>>>> After hard+soft resets, the PM signature is found,
>>>> and libata interrogates the PM registers.
>>>>
>>>> It successfully reads register 0, and then register 1.
>>>> But all subsequent registers read out (incorrectly) as zeros.
> ..
> 
> Saeed has confirmed this behaviour with a SATA analyzer.
> The Marvell port-multiplier apparently likes to see clean HOB
> information when accessing PMP registers.
> 
> Since sata_mv uses PIO shadow register access, this doesn't happen
> automatically, as it might in a more purely FIS-based driver (eg. ahci).
> 
> One way to fix this is to flag these commands with ATA_TFLAG_LBA48,
> forcing libata to write out the HOB fields with known (zero) values.
> 
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> Acked-by: Mark Lord <mlord@pobox.com>

Acked-by: Tejun Heo <htejun@gmail.com>

I think this is correct w/ or w/o the mv problem.

-- 
tejun

Re: [PATCH] libata-pmp: clear hob for pmp register accesses

[Jeff Garzik]

Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Hello, Mark.
>>>
>>> Mark Lord wrote:
>>>> Tejun, I've added PMP to sata_mv, and am now trying to get it
>>>> to work with a Marvell PM attached.
>>>>
>>>> And the behaviour I see is very bizarre.
>>>>
>>>> After hard+soft resets, the PM signature is found,
>>>> and libata interrogates the PM registers.
>>>>
>>>> It successfully reads register 0, and then register 1.
>>>> But all subsequent registers read out (incorrectly) as zeros.
> ..
> 
> Saeed has confirmed this behaviour with a SATA analyzer.
> The Marvell port-multiplier apparently likes to see clean HOB
> information when accessing PMP registers.
> 
> Since sata_mv uses PIO shadow register access, this doesn't happen
> automatically, as it might in a more purely FIS-based driver (eg. ahci).
> 
> One way to fix this is to flag these commands with ATA_TFLAG_LBA48,
> forcing libata to write out the HOB fields with known (zero) values.
> 
> Signed-off-by: Saeed Bishara <saeed@marvell.com>
> Acked-by: Mark Lord <mlord@pobox.com>
> ---
> drivers/ata/libata-pmp.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)

applied

Re: What's needed for PMP support?

[Mark Lord]

Tejun Heo wrote:
>>
>> The following things are needed for a LLD to support PMP.
> ..
>> I think that's about it.  Feel free to ask if something isn't clear.
..

I think we need better semantics around sata_scr_{read,write}(),
or more specifically 

These need to be moved into ata_port_operations
so that LLDs can wrap them to properly manage
the host controller's global link->pmp value.

The problem I've been debugging here for the past 24hrs,
is that sata_mv sets the pmp number globally in hardware,
but then libata does a call to sata_scr_read()
which causes it to change.  Without ever changing it back.

Subsequent accesses of shadow registers now point at
the pmp==15 instead of the original PM port.

Doh!  No wonder device detection fails for me.

The LLD needs a way to properly manage the current pmp selection,
without having to clone all of the reset logic from libata-core.

I'd like to just re-use that code, but I cannot if it's going
to muck up the pmp selection.

I *think* ata_link_online() is my immediate problem.
It gets called from inside ata_std_softreset(),
and it invokes sata_scr_read().

This prevents me from re-using ata_std_softreset(),
and all of the non-exported functions that *it* calls.

There's very little that's special in the LLD for pmp support,
but the amount of code required seems huge, just to cope with
this simple problem caused.

Ugh.

If sata_scr_{read,write}() were in ata_port_operations,
then I could wrap them to save/restore the original pmp value.
But I'm not sure if that would result in a race against
other command-issue paths on the same port (?).

Tejun ???

For now, I'll try to hack it into sata_mv locally, somehow.

Suggestions?

Re: What's needed for PMP support?

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>>>
>>> The following things are needed for a LLD to support PMP.
>> ..
>>> I think that's about it.  Feel free to ask if something isn't clear.
> ..
> 
> I think we need better semantics around sata_scr_{read,write}(),
> or more specifically
> These need to be moved into ata_port_operations
> so that LLDs can wrap them to properly manage
> the host controller's global link->pmp value.
..

Heck, if .dev_select() took a *device* instead of a *port*
as it's parameter, then I could probably manage it fine in there.

-ml

Re: What's needed for PMP support?

[Tejun Heo]

Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>>>
>>>> The following things are needed for a LLD to support PMP.
>>> ..
>>>> I think that's about it.  Feel free to ask if something isn't clear.
>> ..
>>
>> I think we need better semantics around sata_scr_{read,write}(),
>> or more specifically
>> These need to be moved into ata_port_operations
>> so that LLDs can wrap them to properly manage
>> the host controller's global link->pmp value.
> ..
> 
> Heck, if .dev_select() took a *device* instead of a *port*
> as it's parameter, then I could probably manage it fine in there.

Heh... I never thought a PMP aware controller would use TF SRST, so what
you want to do is set pmp value in the register and calling
ata_std_softreset(), right?  I think the correct thing to do is to
separate out SRST sequence proper from ata_std_softreset() into, say,
ata_sff_SRST() and build custom softreset around it.  After all, the
problem here is the reset sequence not the SCR access.

Thanks.

-- 
tejun

Re: What's needed for PMP support?

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Mark Lord wrote:
>>> Tejun Heo wrote:
>>>>> The following things are needed for a LLD to support PMP.
>>>> ..
>>>>> I think that's about it.  Feel free to ask if something isn't clear.
>>> ..
>>>
>>> I think we need better semantics around sata_scr_{read,write}(),
>>> or more specifically
>>> These need to be moved into ata_port_operations
>>> so that LLDs can wrap them to properly manage
>>> the host controller's global link->pmp value.
>> ..
>>
>> Heck, if .dev_select() took a *device* instead of a *port*
>> as it's parameter, then I could probably manage it fine in there.
> 
> Heh... I never thought a PMP aware controller would use TF SRST, so what
> you want to do is set pmp value in the register and calling
> ata_std_softreset(), right?  I think the correct thing to do is to
> separate out SRST sequence proper from ata_std_softreset() into, say,
> ata_sff_SRST() and build custom softreset around it.  After all, the
> problem here is the reset sequence not the SCR access.
..

Actually, I believe the problem *is* the (pmp) SCR access.
The same issue will return again when trying to support hotplug, for example.

Any SCR access will steal the active pmp on such hosts.

I think we really do need to snoop those, somehow.

Cheers

Re: What's needed for PMP support?

[Tejun Heo]

Mark Lord wrote:
>> Heh... I never thought a PMP aware controller would use TF SRST, so what
>> you want to do is set pmp value in the register and calling
>> ata_std_softreset(), right?  I think the correct thing to do is to
>> separate out SRST sequence proper from ata_std_softreset() into, say,
>> ata_sff_SRST() and build custom softreset around it.  After all, the
>> problem here is the reset sequence not the SCR access.
> ..
> 
> Actually, I believe the problem *is* the (pmp) SCR access.
> The same issue will return again when trying to support hotplug, for
> example.

Can you elaborate a bit?

> Any SCR access will steal the active pmp on such hosts.
> 
> I think we really do need to snoop those, somehow.

Adding ->sata_pmp_scr_read/write should do but I wanna avoid that if
possible.

Thanks.

-- 
tejun

Re: What's needed for PMP support?

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> Heh... I never thought a PMP aware controller would use TF SRST, so what
>>> you want to do is set pmp value in the register and calling
>>> ata_std_softreset(), right?  I think the correct thing to do is to
>>> separate out SRST sequence proper from ata_std_softreset() into, say,
>>> ata_sff_SRST() and build custom softreset around it.  After all, the
>>> problem here is the reset sequence not the SCR access.
>> ..
>>
>> Actually, I believe the problem *is* the (pmp) SCR access.
>> The same issue will return again when trying to support hotplug, for
>> example.
> 
> Can you elaborate a bit?
..

For example: a simple call to ata_link_online() is enough to mess it up.
Say, from hotplug polling.

Or an SEMB access (if not now, then someday when we implement it)

>> Any SCR access will steal the active pmp on such hosts.
>>
>> I think we really do need to snoop those, somehow.
> 
> Adding ->sata_pmp_scr_read/write should do but I wanna avoid that if
> possible.
..

We already have .pmp_scr_{read,write} operations.
If NULL, then default to the built-ins that are there now.

I'll try it here first, though, and make sure it solves things.

Cheers

Re: What's needed for PMP support?

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
>>>> Heh... I never thought a PMP aware controller would use TF SRST, so 
>>>> what
>>>> you want to do is set pmp value in the register and calling
>>>> ata_std_softreset(), right?  I think the correct thing to do is to
>>>> separate out SRST sequence proper from ata_std_softreset() into, say,
>>>> ata_sff_SRST() and build custom softreset around it.  After all, the
>>>> problem here is the reset sequence not the SCR access.
>>> ..
>>>
>>> Actually, I believe the problem *is* the (pmp) SCR access.
>>> The same issue will return again when trying to support hotplug, for
>>> example.
>>
>> Can you elaborate a bit?
> ..
> 
> For example: a simple call to ata_link_online() is enough to mess it up.
> Say, from hotplug polling.
> 
> Or an SEMB access (if not now, then someday when we implement it)
> 
>>> Any SCR access will steal the active pmp on such hosts.
>>>
>>> I think we really do need to snoop those, somehow.
>>
>> Adding ->sata_pmp_scr_read/write should do but I wanna avoid that if
>> possible.
> ..
> 
> We already have .pmp_scr_{read,write} operations.
> If NULL, then default to the built-ins that are there now.
..

Mmm.. lost some lines there, try again:

We already have .scr_{read,write} operations, and
what I think we need are .pmp_scr_{read,write} in addition.
If NULL, then default to the built-ins that are there now.

Re: What's needed for PMP support?

[Mark Lord]

Mark Lord wrote:
> Mark Lord wrote:
>..
>> We already have .pmp_scr_{read,write} operations.
>> If NULL, then default to the built-ins that are there now.
> ..
> 
> Mmm.. lost some lines there, try again:
> 
> We already have .scr_{read,write} operations, and
> what I think we need are .pmp_scr_{read,write} in addition.
> If NULL, then default to the built-ins that are there now.

Tejun Heo wrote:
> Adding ->sata_pmp_scr_read/write should do but I wanna avoid that if possible.
..

Well, what it really needed was .pmp_read() and .pmp_write().

I implemented that, and (finally!!) sata_mv works perfectly
with the Marvell port multiplier I have here.

All other attempts to do this entirely within sata_mv
just left me (and the hardware) dazed and confused.

But being able to wrap sata_pmp_read/write() with save/restore
logic fixed things enough to work on the first try.

Whew!

I'll post a summary patch once I get it generated here.
Lots of debug code to back out first.

Thanks Tejun, Saeed!

new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Tejun,

Here's a first cut at this for discussion.

You may prefer different names for the invoking functions
inside libata-pmp.c, rather than simply pmp_read() and pmp_write(),
but I've been up too long and couldn't think of a better name.

An alternative to all this, might be to expose the "select_pmp()"
function shown in the sample code, and have libata-pmp.c call that,
instead of having the new new .pmp_{read,write} functions.

What do you think?

* * *

SATA host controllers which are not purely FIS-based require setup
of a special register to select the active pmp for taskfile accesses.

This can be done in the low-level driver for regular command issue
on a link.  But commands directed at a port-multiplier cause problems,
because they leave the original link pmp de-selected afterwards.

To fix this in a sane fashion, without tons of code duplication
from libata into the low-level driver, it is necessary to be able
to wrap the sata_pmp_read() and sata_pmp_write() functions.

This patch provides two new struct ata_port_operations methods for this,
and modifies the code in libata-pmp to use them if provided.

In practice, the low-level driver might implement them like this:

	static int pmp_read(struct ata_link *link, unsigned int reg, u32 *val)
	{
	        int old = select_pmp(link->ap, SATA_PMP_CTRL_PORT);
	        int rc  = sata_pmp_read(link, reg, val);
	        select_pmp(link->ap, old);
	        return rc;
	}
	
	static int pmp_write(struct ata_link *link, unsigned int reg, u32 val)
	{
	        int old = select_pmp(link->ap, link->pmp);
	        int rc  = sata_pmp_write(link, reg, val);
	        select_pmp(link->ap, old);
	        return rc;
	}

Note that "select_pmp" is local to the low-level driver,
and is where the chipset pmp-selection register is read/written
(the return value is the previous pmp, before writing the new one).

Something like this patch is needed for PMP support in sata_mv,
and possibly other drivers.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

 drivers/ata/libata-core.c |    2 ++
 drivers/ata/libata-pmp.c  |   36 ++++++++++++++++++++++++++----------
 include/linux/libata.h    |    5 +++++
 3 files changed, 33 insertions(+), 10 deletions(-)

diff -u --recursive --new-file --exclude-from=old/Documentation/dontdiff --exclude='*.lds' --exclude-from=old/.gitignore old/drivers/ata/libata-core.c new/drivers/ata/libata-core.c
--- old/drivers/ata/libata-core.c	2008-02-21 22:45:38.000000000 -0500
+++ new/drivers/ata/libata-core.c	2008-02-21 22:59:13.000000000 -0500
@@ -7856,6 +7856,8 @@
 EXPORT_SYMBOL_GPL(sata_pmp_std_hardreset);
 EXPORT_SYMBOL_GPL(sata_pmp_std_postreset);
 EXPORT_SYMBOL_GPL(sata_pmp_do_eh);
+EXPORT_SYMBOL_GPL(sata_pmp_read);
+EXPORT_SYMBOL_GPL(sata_pmp_write);
 
 EXPORT_SYMBOL_GPL(__ata_ehi_push_desc);
 EXPORT_SYMBOL_GPL(ata_ehi_push_desc);
diff -u --recursive --new-file --exclude-from=old/Documentation/dontdiff --exclude='*.lds' --exclude-from=old/.gitignore old/drivers/ata/libata-pmp.c new/drivers/ata/libata-pmp.c
--- old/drivers/ata/libata-pmp.c	2008-02-21 22:47:32.000000000 -0500
+++ new/drivers/ata/libata-pmp.c	2008-02-21 22:57:43.000000000 -0500
@@ -25,7 +25,7 @@
  *	RETURNS:
  *	0 on success, AC_ERR_* mask on failure.
  */
-static unsigned int sata_pmp_read(struct ata_link *link, int reg, u32 *r_val)
+unsigned int sata_pmp_read(struct ata_link *link, int reg, u32 *r_val)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_device *pmp_dev = ap->link.device;
@@ -48,6 +48,14 @@
 	return 0;
 }
 
+static unsigned int pmp_read(struct ata_link *link, int reg, u32 *r_val)
+{
+	if (link->ap->ops->pmp_read)
+		return link->ap->ops->pmp_read(link,reg,r_val);
+	else
+		return sata_pmp_read(link, reg, r_val);
+}
+
 /**
  *	sata_pmp_write - write PMP register
  *	@link: link to write PMP register for
@@ -62,7 +70,7 @@
  *	RETURNS:
  *	0 on success, AC_ERR_* mask on failure.
  */
-static unsigned int sata_pmp_write(struct ata_link *link, int reg, u32 val)
+unsigned int sata_pmp_write(struct ata_link *link, int reg, u32 val)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_device *pmp_dev = ap->link.device;
@@ -83,6 +91,14 @@
 				 SATA_PMP_SCR_TIMEOUT);
 }
 
+static unsigned int pmp_write(struct ata_link *link, int reg, u32 val)
+{
+	if (link->ap->ops->pmp_write)
+		return link->ap->ops->pmp_write(link,reg,val);
+	else
+		return sata_pmp_write(link, reg, val);
+}
+
 /**
  *	sata_pmp_qc_defer_cmd_switch - qc_defer for command switching PMP
  *	@qc: ATA command in question
@@ -135,7 +151,7 @@
 	if (reg > SATA_PMP_PSCR_CONTROL)
 		return -EINVAL;
 
-	err_mask = sata_pmp_read(link, reg, r_val);
+	err_mask = pmp_read(link, reg, r_val);
 	if (err_mask) {
 		ata_link_printk(link, KERN_WARNING, "failed to read SCR %d "
 				"(Emask=0x%x)\n", reg, err_mask);
@@ -166,7 +182,7 @@
 	if (reg > SATA_PMP_PSCR_CONTROL)
 		return -EINVAL;
 
-	err_mask = sata_pmp_write(link, reg, val);
+	err_mask = pmp_write(link, reg, val);
 	if (err_mask) {
 		ata_link_printk(link, KERN_WARNING, "failed to write SCR %d "
 				"(Emask=0x%x)\n", reg, err_mask);
@@ -332,7 +348,7 @@
 		int reg = gscr_to_read[i];
 		unsigned int err_mask;
 
-		err_mask = sata_pmp_read(dev->link, reg, &gscr[reg]);
+		err_mask = pmp_read(dev->link, reg, &gscr[reg]);
 		if (err_mask) {
 			ata_dev_printk(dev, KERN_ERR, "failed to read PMP "
 				"GSCR[%d] (Emask=0x%x)\n", reg, err_mask);
@@ -375,7 +391,7 @@
 		dev->flags |= ATA_DFLAG_AN;
 
 	/* monitor SERR_PHYRDY_CHG on fan-out ports */
-	err_mask = sata_pmp_write(dev->link, SATA_PMP_GSCR_ERROR_EN,
+	err_mask = pmp_write(dev->link, SATA_PMP_GSCR_ERROR_EN,
 				  SERR_PHYRDY_CHG);
 	if (err_mask) {
 		rc = -EIO;
@@ -387,7 +403,7 @@
 	if (gscr[SATA_PMP_GSCR_FEAT_EN] & SATA_PMP_FEAT_NOTIFY) {
 		gscr[SATA_PMP_GSCR_FEAT_EN] &= ~SATA_PMP_FEAT_NOTIFY;
 
-		err_mask = sata_pmp_write(dev->link, SATA_PMP_GSCR_FEAT_EN,
+		err_mask = pmp_write(dev->link, SATA_PMP_GSCR_FEAT_EN,
 					  gscr[SATA_PMP_GSCR_FEAT_EN]);
 		if (err_mask) {
 			rc = -EIO;
@@ -792,7 +808,7 @@
 	unsigned int err_mask;
 	u32 prod_id;
 
-	err_mask = sata_pmp_read(dev->link, SATA_PMP_GSCR_PROD_ID, &prod_id);
+	err_mask = pmp_read(dev->link, SATA_PMP_GSCR_PROD_ID, &prod_id);
 	if (err_mask) {
 		ata_dev_printk(dev, KERN_ERR, "failed to read PMP product ID "
 			       "(Emask=0x%x)\n", err_mask);
@@ -1086,7 +1102,7 @@
 	if (pmp_dev->flags & ATA_DFLAG_AN) {
 		pmp_dev->gscr[SATA_PMP_GSCR_FEAT_EN] |= SATA_PMP_FEAT_NOTIFY;
 
-		err_mask = sata_pmp_write(pmp_dev->link, SATA_PMP_GSCR_FEAT_EN,
+		err_mask = pmp_write(pmp_dev->link, SATA_PMP_GSCR_FEAT_EN,
 					  pmp_dev->gscr[SATA_PMP_GSCR_FEAT_EN]);
 		if (err_mask) {
 			ata_dev_printk(pmp_dev, KERN_ERR, "failed to write "
@@ -1097,7 +1113,7 @@
 	}
 
 	/* check GSCR_ERROR */
-	err_mask = sata_pmp_read(pmp_link, SATA_PMP_GSCR_ERROR, &gscr_error);
+	err_mask = pmp_read(pmp_link, SATA_PMP_GSCR_ERROR, &gscr_error);
 	if (err_mask) {
 		ata_dev_printk(pmp_dev, KERN_ERR, "failed to read "
 			       "PMP_GSCR_ERROR (Emask=0x%x)\n", err_mask);
diff -u --recursive --new-file --exclude-from=old/Documentation/dontdiff --exclude='*.lds' --exclude-from=old/.gitignore old/include/linux/libata.h new/include/linux/libata.h
--- old/include/linux/libata.h	2008-02-21 22:45:39.000000000 -0500
+++ new/include/linux/libata.h	2008-02-21 23:01:14.000000000 -0500
@@ -717,6 +717,9 @@
 	int (*scr_read) (struct ata_port *ap, unsigned int sc_reg, u32 *val);
 	int (*scr_write) (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
+	int (*pmp_read) (struct ata_link *link, unsigned int sc_reg, u32 *val);
+	int (*pmp_write) (struct ata_link *link, unsigned int sc_reg, u32 val);
+
 	int (*port_suspend) (struct ata_port *ap, pm_message_t mesg);
 	int (*port_resume) (struct ata_port *ap);
 	int (*enable_pm) (struct ata_port *ap, enum link_pm policy);
@@ -1043,6 +1046,8 @@
 		ata_reset_fn_t hardreset, ata_postreset_fn_t postreset,
 		ata_prereset_fn_t pmp_prereset, ata_reset_fn_t pmp_softreset,
 		ata_reset_fn_t pmp_hardreset, ata_postreset_fn_t pmp_postreset);
+extern unsigned int sata_pmp_read(struct ata_link *link, int reg, u32 *val);
+extern unsigned int sata_pmp_write(struct ata_link *link, int reg, u32 val);
 
 /*
  * EH

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Mark Lord wrote:
> Tejun,
> 
> Here's a first cut at this for discussion.
> 
> You may prefer different names for the invoking functions
> inside libata-pmp.c, rather than simply pmp_read() and pmp_write(),
> but I've been up too long and couldn't think of a better name.
> 
> An alternative to all this, might be to expose the "select_pmp()"
> function shown in the sample code, and have libata-pmp.c call that,
> instead of having the new new .pmp_{read,write} functions.
> 
> What do you think?
> 
> * * *
> 
> SATA host controllers which are not purely FIS-based require setup
> of a special register to select the active pmp for taskfile accesses.
> 
> This can be done in the low-level driver for regular command issue
> on a link.  But commands directed at a port-multiplier cause problems,
> because they leave the original link pmp de-selected afterwards.
> 
> To fix this in a sane fashion, without tons of code duplication
> from libata into the low-level driver, it is necessary to be able
> to wrap the sata_pmp_read() and sata_pmp_write() functions.
> 
> This patch provides two new struct ata_port_operations methods for this,
> and modifies the code in libata-pmp to use them if provided.
...

Note that, while this does work for sata_mv, I'm still thinking about it.

I'm not totally clear yet (more reading to do) as to how/when
the ATA shadow/taskfile registers get updated to reflect those
for the currently selected pmp..

It would seem that with other parts of libata-sff directly reading
from the ctl, status, and altstatus "registers" during polling,
command setup, and probing, that there might (?) be a loophole
somewhere in this strategy.

Is this scenario going to be possible:  somebody calls sata_pmp_read()
as part of, say, hotplug polling, and after that operation completes
we then have code that calls ata_check_status() prior to the next
tf_load / command issue ?  If so, they'll see the wrong cached 
shadow status register.

And for that matter, is it possible for sata_pmp_read() to be called
while the link is active with another command ?  Not today, it seems,
but what about when hotplug polling gets implemented ?

Mmm.. an amazing amount of complexity there for something that
ought to be very simple.

More reading to do, but comments from Tejun/others are welcomed.

Thanks.

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Mark Lord wrote:
>
> Note that, while this does work for sata_mv, I'm still thinking about it.
> 
> I'm not totally clear yet (more reading to do) as to how/when
> the ATA shadow/taskfile registers get updated to reflect those
> for the currently selected pmp..
> 
> It would seem that with other parts of libata-sff directly reading
> from the ctl, status, and altstatus "registers" during polling,
> command setup, and probing, that there might (?) be a loophole
> somewhere in this strategy.
> 
> Is this scenario going to be possible:  somebody calls sata_pmp_read()
> as part of, say, hotplug polling, and after that operation completes
> we then have code that calls ata_check_status() prior to the next
> tf_load / command issue ?  If so, they'll see the wrong cached shadow 
> status register.
..

Answering my own question here: the above scenario really doesn't matter.
They'll see 0x50 in the status register, and be happy regardless.
Because that's what should have been there before the command issue
by sata_pmp_read() in the first place.  So not-a-problem.
 
> And for that matter, is it possible for sata_pmp_read() to be called
> while the link is active with another command ?  Not today, it seems,
> but what about when hotplug polling gets implemented ?
..

That's the one I'm most concerned about.  Should I be?

-ml

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Mark Lord wrote:
> Mark Lord wrote:
...
>> And for that matter, is it possible for sata_pmp_read() to be called
>> while the link is active with another command ?  Not today, it seems,
>> but what about when hotplug polling gets implemented ?
> ..
> That's the one I'm most concerned about.  Should I be?
...

Tejun,

On a related note, I'm now looking into PMP error handling in the driver.
The obvious thing I see that I want to fix, is that after a media error
on any PMP attached drive, I get this:

	ata20.00: failed to read SCR 1 (Emask=0x40)
	ata20.01: failed to read SCR 1 (Emask=0x40)
	ata20.02: failed to read SCR 1 (Emask=0x40)
	ata20.03: failed to read SCR 1 (Emask=0x40)
	ata20.04: failed to read SCR 1 (Emask=0x40)

Okay, so those are from sata_pmp_read(), which cannot even
issue it's commands because the port was frozen by the EH.

Is this expected?  I'm not entirely clear what to do in
the EH for this driver.  The chipset docs say that
after just about any kind of error software must do
a hard reset of the channel to make it usable again.

But I suspect that PIO commands may be okay before that,
and sata_pmp_read() is trying to issue a PIO command.

How to resolve this?

Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
>>> And for that matter, is it possible for sata_pmp_read() to be called
>>> while the link is active with another command ?  Not today, it seems,
>>> but what about when hotplug polling gets implemented ?
>> ..
>> That's the one I'm most concerned about.  Should I be?
> ...
> 
> Tejun,
> 
> On a related note, I'm now looking into PMP error handling in the driver.
> The obvious thing I see that I want to fix, is that after a media error
> on any PMP attached drive, I get this:
> 
>     ata20.00: failed to read SCR 1 (Emask=0x40)
>     ata20.01: failed to read SCR 1 (Emask=0x40)
>     ata20.02: failed to read SCR 1 (Emask=0x40)
>     ata20.03: failed to read SCR 1 (Emask=0x40)
>     ata20.04: failed to read SCR 1 (Emask=0x40)
> 
> Okay, so those are from sata_pmp_read(), which cannot even
> issue it's commands because the port was frozen by the EH.

Hmm.. media error causes freeze?  Anyways, yeah, those are expected if
the port is frozen.  Those are from link autopsy.  Maybe it's better to
skip SCR access on fan-out ports if host port is frozen or at least
suppress the messages.

> Is this expected?  I'm not entirely clear what to do in
> the EH for this driver.  The chipset docs say that
> after just about any kind of error software must do
> a hard reset of the channel to make it usable again.
> 
> But I suspect that PIO commands may be okay before that,
> and sata_pmp_read() is trying to issue a PIO command.

If the controller needs to be frozen after any kind of error, I don't
think there's much left to do other than suppressing those annoying
messages.  Hmmm.. how does the controller handle ATAPI check sense?

-- 
tejun

Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
>> This patch provides two new struct ata_port_operations methods for this,
>> and modifies the code in libata-pmp to use them if provided.
> ...
> 
> Note that, while this does work for sata_mv, I'm still thinking about it.
> 
> I'm not totally clear yet (more reading to do) as to how/when
> the ATA shadow/taskfile registers get updated to reflect those
> for the currently selected pmp..
> 
> It would seem that with other parts of libata-sff directly reading
> from the ctl, status, and altstatus "registers" during polling,
> command setup, and probing, that there might (?) be a loophole
> somewhere in this strategy.

Yeah, this is an interesting problem.  There are basically multiple sets
of TF registers and the SFF way of assuming single set doesn't really
work well and I don't really think adding ->pmp_read/write is the
correct long term solution to the problem.  We need to confine direct TF
register accesses to SFF layer only as controllers which don't implement
SFF interface may or may not emulate TF registers and even when they do,
it sometimes can't really match the SFF behavior.

For command execution, TF write is already performed by qc_prep and
issue and TF read back should be performed by LLD if RESULT_TF is set.
For EH, the reset methods should be responsible whether or how the TF
registers are accessed.

Mark, for your case, I think the correct thing to do is to factor out
the necessary bare-bone part from SFF implementation and use it with
necessary PMP register setup once the necessary change is made to the
core layer.  The controller is NOT a SFF one and I don't think
stretching SFF implementation to fit mv's PMP-aware emulation of it is a
good idea.  Maybe we can fit libata to it but it's likely we'll need to
modify it again to fit different emulation from different vendor.

> Is this scenario going to be possible:  somebody calls sata_pmp_read()
> as part of, say, hotplug polling, and after that operation completes
> we then have code that calls ata_check_status() prior to the next
> tf_load / command issue ?  If so, they'll see the wrong cached shadow
> status register.

I think what should happen is that any of TF registers isn't accessed
behind LLD's back.  Then, you don't have nothing to worry about.  If the
controller emulates some of SFF interface, you can always properly wrap
SFF helpers with necessary setup and teardown added.

> Mmm.. an amazing amount of complexity there for something that
> ought to be very simple.

I wish things are a bit clearer now.  I think the problem here is that
we're assuming SFF TF access on controllers which aren't really SFF.
For sil24 and ahci, the driver emulates it and it isn't too difficult.
The picture gets more interesting for sata_mv as its hardware interface
much closer to SFF than sil24 or ahci and TF registers matter much more.
 For ahci and sil24, LLD can just fool libata core layer which assumes
ubiquitous TF access.  TF registers don't really matter to controller
operation anyway and feeding bogus values work well.  For sata_mv, it's
different.  Those registers are integral part of controller operation
and sata_mv can't really tolerate core layer stepping in w/o notifying LLD.

Thanks.

-- 
tejun

Re: new ata_port_operations for .pmp_{read,write} ?

[Jeff Garzik]

Tejun Heo wrote:
> Yeah, this is an interesting problem.  There are basically multiple sets
> of TF registers and the SFF way of assuming single set doesn't really
> work well and I don't really think adding ->pmp_read/write is the
> correct long term solution to the problem.  We need to confine direct TF
> register accesses to SFF layer only as controllers which don't implement
> SFF interface may or may not emulate TF registers and even when they do,
> it sometimes can't really match the SFF behavior.

Strongly agreed.


> I wish things are a bit clearer now.  I think the problem here is that
> we're assuming SFF TF access on controllers which aren't really SFF.
> For sil24 and ahci, the driver emulates it and it isn't too difficult.
> The picture gets more interesting for sata_mv as its hardware interface
> much closer to SFF than sil24 or ahci and TF registers matter much more.
>  For ahci and sil24, LLD can just fool libata core layer which assumes
> ubiquitous TF access.  TF registers don't really matter to controller
> operation anyway and feeding bogus values work well.  For sata_mv, it's
> different.  Those registers are integral part of controller operation
> and sata_mv can't really tolerate core layer stepping in w/o notifying LLD.

I would definitely like to move away from the model where non-SFF 
drivers have to emulate SFF in any way.

Re-reviewing the code, I don't see a lot of TF accesses outside of 
SFF-specific code, so we are already in pretty good shape.

I think the picture gets more complicated with sata_mv and similar 
drivers (soon sata_svw, too), because they use the SFF code but only for 
certain TF protocols.  The emulation question is not as clear, with 
those drivers.

	Jeff

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Mark Lord wrote:
> An alternative to all this, might be to expose the "select_pmp()"
> function shown in the sample code, and have libata-pmp.c call that,
> instead of having the new new .pmp_{read,write} functions. 
..

I wonder if this might be more viable than first thought.

Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
an option ata_operations method for "select_pmp_port(pmp)",
which the core could invoke prior to any direct manipulation
of the shadow registers.

It would really only be needed in perhaps three or four places total
(eg. sata_pmp_read/write, and before writes to the ata "ctl" register).
This could be a reasonably tidy way for the core to keep the LLD
abreast of its intent when accessing things.

I really would like to keep the LLD code small, and have good solid
core routines for non-hardware specific functionality.  All of this stuff
I'm beginning to do with sata_mv would be trivial if I wanted to bloat
the LLD, but really.. only a tiny bit of it need be custom to sata_mv.

The existing SFF reset/probe/pmp stuff is just about exactly what
sata_mv needs.. and I feel a strong desire to not clone/duplicate
that hard-won functionality.

Much of it I can see being shared with other half-and-half chipset drivers
as we add PMP functionality to those.

Maybe that's just the embedded side me showing through?

It is tricky to define the right interfaces, though, as each chipset
does throw its own unique curve balls at us.

I may prototype this select_pmp_port() concept in sata_mv(),
and see how it works out (or not).

Cheers

Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> Mark Lord wrote:
>> An alternative to all this, might be to expose the "select_pmp()"
>> function shown in the sample code, and have libata-pmp.c call that,
>> instead of having the new new .pmp_{read,write} functions. 
> ..
> 
> I wonder if this might be more viable than first thought.
> 
> Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
> an option ata_operations method for "select_pmp_port(pmp)",
> which the core could invoke prior to any direct manipulation
> of the shadow registers.

I don't really think that's a good solution.  That's the quickest
solution for sata_mv but it just works around more fundamental problem
of assuming SFF behavior in core layer which we need to drop anyway.

> I really would like to keep the LLD code small, and have good solid
> core routines for non-hardware specific functionality.  All of this stuff
> I'm beginning to do with sata_mv would be trivial if I wanted to bloat
> the LLD, but really.. only a tiny bit of it need be custom to sata_mv.
> 
> The existing SFF reset/probe/pmp stuff is just about exactly what
> sata_mv needs.. and I feel a strong desire to not clone/duplicate
> that hard-won functionality.

I strongly agree but am having difficult time agreeing with the proposed
approach.

> Much of it I can see being shared with other half-and-half chipset drivers
> as we add PMP functionality to those.

Do we have other chips which have PMP support but still uses mixed SFF
interface?

> Maybe that's just the embedded side me showing through?
> 
> It is tricky to define the right interfaces, though, as each chipset
> does throw its own unique curve balls at us.

Yeah, exactly.  I think what needs to be done is to separate out SFF
assumptions from core layer, factor out SFF-proper helpers and use them
to implement LLDs for quasi-SFF controllers.

Thanks.

-- 
tejun

Re: new ata_port_operations for .pmp_{read,write} ?

[Jeff Garzik]

Tejun Heo wrote:
> Yeah, exactly.  I think what needs to be done is to separate out SFF
> assumptions from core layer, factor out SFF-proper helpers and use them
> to implement LLDs for quasi-SFF controllers.


Thinking long term, I continue to hope that SFF support can eventually 
be separate into a sub-API, allowing people to disable (at compile time) 
SFF support completely if they don't need it.

	Jeff

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Yeah, exactly.  I think what needs to be done is to separate out SFF
>> assumptions from core layer, factor out SFF-proper helpers and use them
>> to implement LLDs for quasi-SFF controllers.
> 
> 
> Thinking long term, I continue to hope that SFF support can eventually 
> be separate into a sub-API, allowing people to disable (at compile time) 
> SFF support completely if they don't need it.
..

I can think of a few more pressing things to disable before that.

Like ACPI and PMP.  Modularizing those (or more crudely de-selecting them
at build time) would save a lot of memory on small embedded boxes.

Cheers

Re: new ata_port_operations for .pmp_{read,write} ?

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Yeah, exactly.  I think what needs to be done is to separate out SFF
>>> assumptions from core layer, factor out SFF-proper helpers and use them
>>> to implement LLDs for quasi-SFF controllers.
>>
>>
>> Thinking long term, I continue to hope that SFF support can eventually 
>> be separate into a sub-API, allowing people to disable (at compile 
>> time) SFF support completely if they don't need it.
> ..
> 
> I can think of a few more pressing things to disable before that.
> 
> Like ACPI and PMP.  Modularizing those (or more crudely de-selecting them
> at build time) would save a lot of memory on small embedded boxes.

Sure.  Make these options dependent on CONFIG_EMBEDDED, and go to town 
from there...

	Jeff

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> Mark Lord wrote:
>>> An alternative to all this, might be to expose the "select_pmp()"
>>> function shown in the sample code, and have libata-pmp.c call that,
>>> instead of having the new new .pmp_{read,write} functions. 
>> ..
>>
>> I wonder if this might be more viable than first thought.
>>
>> Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
>> an option ata_operations method for "select_pmp_port(pmp)",
>> which the core could invoke prior to any direct manipulation
>> of the shadow registers.
> 
> I don't really think that's a good solution.  That's the quickest
> solution for sata_mv but it just works around more fundamental problem
> of assuming SFF behavior in core layer which we need to drop anyway.
> 
>> I really would like to keep the LLD code small, and have good solid
>> core routines for non-hardware specific functionality.  All of this stuff
>> I'm beginning to do with sata_mv would be trivial if I wanted to bloat
>> the LLD, but really.. only a tiny bit of it need be custom to sata_mv.
>>
>> The existing SFF reset/probe/pmp stuff is just about exactly what
>> sata_mv needs.. and I feel a strong desire to not clone/duplicate
>> that hard-won functionality.
> 
> I strongly agree but am having difficult time agreeing with the proposed
> approach.
..

Okay, then.  MASSIVE code-duplication, here I come!

-ml

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> Mark Lord wrote:
>>> An alternative to all this, might be to expose the "select_pmp()"
>>> function shown in the sample code, and have libata-pmp.c call that,
>>> instead of having the new new .pmp_{read,write} functions. 
>> ..
>>
>> I wonder if this might be more viable than first thought.
>>
>> Say the LLD, be it ata_piix or sata_mv or sata_vw, were to provide
>> an option ata_operations method for "select_pmp_port(pmp)",
>> which the core could invoke prior to any direct manipulation
>> of the shadow registers.
> 
> I don't really think that's a good solution.  That's the quickest
> solution for sata_mv but it just works around more fundamental problem
> of assuming SFF behavior in core layer which we need to drop anyway.
...

No, the quickest solution for sata_mv, the one I apparently will now be using,
is to just clone about 250 lines of reset/debouce/probe code from libata-core
and change perhaps five lines of it to work around this issue plus some
chipset errata.

What I was thinking of, before, is the SATA specification which details
use of bits in the SCR to select a PMP port.  Pretty much exactly as
this chipset does it, except in the SCR rather than a proprieary port.

But no big deal.  I can clone code and not bother you any more.
In fact, some of the cloned code was already in sata_mv, and I removed
it this past week in my local working copy.  I'll just restore that,
along with another big blob so that we can select pm port where needed.

What a shame.

Re: new ata_port_operations for .pmp_{read,write} ?

[Jeff Garzik]

To repeat myself from threads past...

In particular with the Marvell 6440 (SATA/SAS, drivers/scsi/mvsas.c) 
when SATA PMP support is completed, we will want to look at passing 
things other than normal ATA commands via ->qc_issue.

Although selection isn't necessarily, talking to a PMP is fundamentally 
an asynchronous, event-driven operation just like any other FIS 
"conversation."

We don't need to be adding hooks for each new and interesting type of 
FIS sent across the wire.  That just leads to an awful API.

	Jeff

Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo]

Mark Lord wrote:
> No, the quickest solution for sata_mv, the one I apparently will now be
> using,
> is to just clone about 250 lines of reset/debouce/probe code from
> libata-core
> and change perhaps five lines of it to work around this issue plus some
> chipset errata.
> 
> What I was thinking of, before, is the SATA specification which details
> use of bits in the SCR to select a PMP port.  Pretty much exactly as
> this chipset does it, except in the SCR rather than a proprieary port.
> 
> But no big deal.  I can clone code and not bother you any more.
> In fact, some of the cloned code was already in sata_mv, and I removed
> it this past week in my local working copy.  I'll just restore that,
> along with another big blob so that we can select pm port where needed.
> 
> What a shame.

The order is somewhat reversed here and I can understand why you're
frustrated but I'm just trying to make things look right in long term,
so feel free to bother me. :-) For temporary solution, I'm okay either
way.  I'll clean things up later when the necessary core changes are made.

* Adding ->pmp_select or ->scr_pmp_rw or whatever callback to work
around the current problem.

* Duplicate code in sata_mv.

Whichever way you go, can you please mark where it's different and why?

Thanks.

-- 
tejun

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> But no big deal.  I can clone code and not bother you any more.
>> In fact, some of the cloned code was already in sata_mv, and I removed
>> it this past week in my local working copy.  I'll just restore that,
>> along with another big blob so that we can select pm port where needed.
>>
>> What a shame.
> 
> The order is somewhat reversed here and I can understand why you're
> frustrated but I'm just trying to make things look right in long term,
> so feel free to bother me. :-) For temporary solution, I'm okay either
> way.  I'll clean things up later when the necessary core changes are made.
..

MMmm.. maybe the "vendor unique FIS" mechanism of the chipset
can save the scenario here.  It would seem to be a reasonable
way to direct a FIS (anything up to 8KB) at a specific pmp,
without changing the "default" pmp on the channel.

I can have qc_issue use that mechanism for anything destined
for pmp==15.  If it works.  The Marvell proprietary driver
has some kludgey status polling wrapped around their own use of it.

One of the chip errata apparently requires this anyway for doing
the READ_LOG_EXT commands after a device error, so perhaps it will
work out to be useful in more ways.

Speaking of which.. I would like to sort out the "freeze" stuff,
so that the sata_mv EH doesn't lock out the PMP commands as it
does today.

Can you recap what a LLD should be doing in the presence of a PM
for EH purposes?  Eg. media error on a PMP drive, so what core
ATA functions should the sata_mv EH interrupt handler be calling,
and in what sequence.. so that the libata-pmp/eh code can still
succeed in it's queries to the PM?

I think James is also interested in a decent explanation of
how to tie into the libata-eh stuff.

Like I noted before, sata_mv will need to eventually hard reset
the channel regardless, but it does seem permitted to use PIO
or vendor-unique-FIS PIO (with polling for either) in the interim.

Thanks.

Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> MMmm.. maybe the "vendor unique FIS" mechanism of the chipset
> can save the scenario here.  It would seem to be a reasonable
> way to direct a FIS (anything up to 8KB) at a specific pmp,
> without changing the "default" pmp on the channel.
> 
> I can have qc_issue use that mechanism for anything destined
> for pmp==15.  If it works.  The Marvell proprietary driver
> has some kludgey status polling wrapped around their own use of it.
> 
> One of the chip errata apparently requires this anyway for doing
> the READ_LOG_EXT commands after a device error, so perhaps it will
> work out to be useful in more ways.

Hmmm...

> Speaking of which.. I would like to sort out the "freeze" stuff,
> so that the sata_mv EH doesn't lock out the PMP commands as it
> does today.
> 
> Can you recap what a LLD should be doing in the presence of a PM
> for EH purposes?  Eg. media error on a PMP drive, so what core
> ATA functions should the sata_mv EH interrupt handler be calling,
> and in what sequence.. so that the libata-pmp/eh code can still
> succeed in it's queries to the PM?

libata doesn't really put much restrictions on what a LLD should do on
entering EH and if the controller's behavior is predictable, there's no
reason to freeze the port.  If the problem is that the DMA engine isn't
usable after PMP error but it's known that the controller isn't gonna
cause irq storm, no need to freeze.  The only command EH issues before
resetting which can use DMA protocol is READ_LOG_EXT.  Maybe there needs
to be a way to force PIO protocol for READ_LOG_EXT.  Other than that, if
no-data and PIO-only commands work fine, EH autopsy should work fine.

> Like I noted before, sata_mv will need to eventually hard reset
> the channel regardless, but it does seem permitted to use PIO
> or vendor-unique-FIS PIO (with polling for either) in the interim.

Just set ATA_EH_HARDRESET (which will soon become ATA_EH_RESET with
prefer-COMRESET patchset) from the interrupt handler before requesting EH.

Thanks.

-- 
tejun

Re: new ata_port_operations for .pmp_{read,write} ?

[Mark Lord]

Tejun Heo wrote:
>
> libata doesn't really put much restrictions on what a LLD should do on
> entering EH and if the controller's behavior is predictable, there's no
> reason to freeze the port.  If the problem is that the DMA engine isn't
> usable after PMP error but it's known that the controller isn't gonna
> cause irq storm, no need to freeze.  The only command EH issues before
> resetting which can use DMA protocol is READ_LOG_EXT.  Maybe there needs
> to be a way to force PIO protocol for READ_LOG_EXT.  Other than that, if
> no-data and PIO-only commands work fine, EH autopsy should work fine.
..

Eh?  READ LOG EXT *is* a PIO command, as opposed to READ LOG DMA EXT
which uses DMA.

And the EH also issues PIO READ_BUFFER commands for PM ports, if a PM is present.

???

Re: new ata_port_operations for .pmp_{read,write} ?

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
>> libata doesn't really put much restrictions on what a LLD should do on
>> entering EH and if the controller's behavior is predictable, there's no
>> reason to freeze the port.  If the problem is that the DMA engine isn't
>> usable after PMP error but it's known that the controller isn't gonna
>> cause irq storm, no need to freeze.  The only command EH issues before
>> resetting which can use DMA protocol is READ_LOG_EXT.  Maybe there needs
>> to be a way to force PIO protocol for READ_LOG_EXT.  Other than that, if
>> no-data and PIO-only commands work fine, EH autopsy should work fine.
> ..
> 
> Eh?  READ LOG EXT *is* a PIO command, as opposed to READ LOG DMA EXT
> which uses DMA.

Ah.. right.  My bad.

> And the EH also issues PIO READ_BUFFER commands for PM ports, if a PM is
> present.

Hmmmm.... Where?

-- 
tejun

Re: What's needed for PMP support?

[Alan Cox]

> Heck, if .dev_select() took a *device* instead of a *port*
> as it's parameter, then I could probably manage it fine in there.

dev_select gets called during probing before the relevant structures are
neccessarily set up.