Add suport for Marvell 88SE6121 in ahci

Add suport for Marvell 88SE6121 in ahci

[Jose Alberto Reguero]

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

This work for kernel 2.6.20-rc6

First apply this patch:

http://marc.theaimsgroup.com/?l=linux-ide&m=116986924301674&w=2

Then apply the patch attached.

Comments:

The Marvell 88SE6121 has three ports (0,1,2). The PATA port is port 2. (PATA 
port for 6141 is port 4). In M2V Motherboard(Marvell 88SE6121) there is only 
two SATA ports, one of them is external SATA. This two ports work well with 
this patch.

With this part:

<       if (pci_enable_msi(pdev) == 0)
---
>       if ((pdev->vendor != PCI_VENDOR_ID_MARVELL) && (pci_enable_msi(pdev) 
== 0))

you don't need to disable MSI in kernel.
MMCONFIG is disabled at startup in M2V.

dmesg:
........
PCI: Not using MMCONFIG.
........

Jose Alberto

[-- Attachment #2: ahci_6121.diff --]
[-- Type: text/x-diff, Size: 1376 bytes --]

441a442
> 	{ PCI_VDEVICE(MARVELL, 0x6121), board_ahci_mv },	/* 6121 */
714c715,718
< 		port_mmio = ahci_port_base(mmio, 4);
---
> 		if (pdev->device == 0x6121)
> 			port_mmio = ahci_port_base(mmio, 2);
> 		else
> 			port_mmio = ahci_port_base(mmio, 4);
1531,1540c1535,1558
< 		dev_printk(KERN_ERR, &pdev->dev,
< 			   "MV_AHCI HACK: port_map %x -> %x, cap_n %u -> %u\n",
< 			   hpriv->port_map,
< 			   hpriv->port_map & 0xf,
< 			   cap_n_ports,
< 			   (cap_n_ports > 4) ? 4 : cap_n_ports);
< 
< 		hpriv->port_map &= 0xf;
< 		if (cap_n_ports > 4)
< 			cap_n_ports = 4;
---
> 		if (pdev->device == 0x6121) {
> 			dev_printk(KERN_ERR, &pdev->dev,
> 				   "MV_AHCI HACK: port_map %x -> %x, cap_n %u -> %u\n",
> 				   hpriv->port_map,
> 				   hpriv->port_map & 0x3,
> 				   cap_n_ports,
> 				   (cap_n_ports > 2) ? 2 : cap_n_ports);
> 
> 			hpriv->port_map &= 0x3;
> 			if (cap_n_ports > 2)
> 				cap_n_ports = 2;
> 		}
> 		else {
> 			dev_printk(KERN_ERR, &pdev->dev,
> 				   "MV_AHCI HACK: port_map %x -> %x, cap_n %u -> %u\n",
> 				   hpriv->port_map,
> 				   hpriv->port_map & 0xf,
> 				   cap_n_ports,
> 				   (cap_n_ports > 4) ? 4 : cap_n_ports);
> 
> 			hpriv->port_map &= 0xf;
> 			if (cap_n_ports > 4)
> 				cap_n_ports = 4;
> 		}
1718c1736
< 	if (pci_enable_msi(pdev) == 0)
---
> 	if ((pdev->vendor != PCI_VENDOR_ID_MARVELL) && (pci_enable_msi(pdev) == 0))

[-- Attachment #3: dmesg --]
[-- Type: text/plain, Size: 1296 bytes --]

ahci 0000:06:00.0: version 2.0
ACPI: PCI Interrupt 0000:06:00.0[A] -> GSI 28 (level, low) -> IRQ 28
ahci 0000:06:00.0: MV_AHCI HACK: port_map 7 -> 3, cap_n 3 -> 2
PCI: Setting latency timer of device 0000:06:00.0 to 64
ahci 0000:06:00.0: AHCI 0001.0000 32 slots 3 ports 3 Gbps 0x3 impl IDE mode
ahci 0000:06:00.0: flags: 64bit ncq stag led pmp slum part
ata3: SATA max UDMA/133 cmd 0xFFFFC20000020D00 ctl 0x0 bmdma 0x0 irq 28
ata4: SATA max UDMA/133 cmd 0xFFFFC20000020D80 ctl 0x0 bmdma 0x0 irq 28
scsi2 : ahci
ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata3.00: ATA-6, max UDMA/133, 234441648 sectors: LBA48 NCQ (depth 31/32)
ata3.00: ata3: dev 0 multi count 16
ata3.00: configured for UDMA/133
scsi3 : ahci
ata4: SATA link down (SStatus 0 SControl 300)
scsi 2:0:0:0: Direct-Access     ATA      ST3120827AS      3.42 PQ: 0 ANSI: 5
SCSI device sdc: 234441648 512-byte hdwr sectors (120034 MB)
sdc: Write Protect is off
sdc: Mode Sense: 00 3a 00 00
SCSI device sdc: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdc: 234441648 512-byte hdwr sectors (120034 MB)
sdc: Write Protect is off
sdc: Mode Sense: 00 3a 00 00
SCSI device sdc: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdc: sdc1
sd 2:0:0:0: Attached scsi disk sdc

Re: Add suport for Marvell 88SE6121 in ahci

[Jeff Garzik]

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

Jose Alberto Reguero wrote:
> This work for kernel 2.6.20-rc6
> 
> First apply this patch:
> 
> http://marc.theaimsgroup.com/?l=linux-ide&m=116986924301674&w=2
> 
> Then apply the patch attached.
> 
> Comments:
> 
> The Marvell 88SE6121 has three ports (0,1,2). The PATA port is port 2. (PATA 
> port for 6141 is port 4). In M2V Motherboard(Marvell 88SE6121) there is only 
> two SATA ports, one of them is external SATA. This two ports work well with 
> this patch.
> 
> With this part:
> 
> <       if (pci_enable_msi(pdev) == 0)
> ---
>>       if ((pdev->vendor != PCI_VENDOR_ID_MARVELL) && (pci_enable_msi(pdev) 
> == 0))
> 
> you don't need to disable MSI in kernel.
> MMCONFIG is disabled at startup in M2V.
> 
> dmesg:
> ........
> PCI: Not using MMCONFIG.

Very useful data points, thanks!

For the future, please make sure to create patches using "diff -u", 
otherwise the patches are very difficult to read and apply.

I've attached your patch as a "diff -u" patch so that others can check 
it out.

	Jeff




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

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ab7ea55..c1925b2 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -439,6 +439,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 
 	/* SiS */
 	{ PCI_VDEVICE(SI, 0x1184), board_ahci }, /* SiS 966 */
+	{ PCI_VDEVICE(MARVELL, 0x6121), board_ahci_mv },	/* 6121 */
 	{ PCI_VDEVICE(SI, 0x1185), board_ahci }, /* SiS 966 */
 	{ PCI_VDEVICE(SI, 0x0186), board_ahci }, /* SiS 968 */
 
@@ -716,7 +717,10 @@ static void ahci_init_controller(void __iomem *mmio, struct pci_dev *pdev,
 	u32 tmp;
 
 	if (port_flags & AHCI_FLAG_MV_PATA) {
-		port_mmio = ahci_port_base(mmio, 4);
+		if (pdev->device == 0x6121)
+			port_mmio = ahci_port_base(mmio, 2);
+		else
+			port_mmio = ahci_port_base(mmio, 4);
 
 		writel(0, port_mmio + PORT_IRQ_MASK);
 
@@ -1570,16 +1574,30 @@ static int ahci_host_init(struct ata_probe_ent *probe_ent)
 	 * presence register, as bit 4 (counting from 0)
 	 */
 	if (probe_ent->port_flags & AHCI_FLAG_MV_PATA) {
-		dev_printk(KERN_ERR, &pdev->dev,
-			   "MV_AHCI HACK: port_map %x -> %x, cap_n %u -> %u\n",
-			   hpriv->port_map,
-			   hpriv->port_map & 0xf,
-			   cap_n_ports,
-			   (cap_n_ports > 4) ? 4 : cap_n_ports);
-
-		hpriv->port_map &= 0xf;
-		if (cap_n_ports > 4)
-			cap_n_ports = 4;
+		if (pdev->device == 0x6121) {
+			dev_printk(KERN_ERR, &pdev->dev,
+				   "MV_AHCI HACK: port_map %x -> %x, cap_n %u -> %u\n",
+				   hpriv->port_map,
+				   hpriv->port_map & 0x3,
+				   cap_n_ports,
+				   (cap_n_ports > 2) ? 2 : cap_n_ports);
+
+			hpriv->port_map &= 0x3;
+			if (cap_n_ports > 2)
+				cap_n_ports = 2;
+		}
+		else {
+			dev_printk(KERN_ERR, &pdev->dev,
+				   "MV_AHCI HACK: port_map %x -> %x, cap_n %u -> %u\n",
+				   hpriv->port_map,
+				   hpriv->port_map & 0xf,
+				   cap_n_ports,
+				   (cap_n_ports > 4) ? 4 : cap_n_ports);
+
+			hpriv->port_map &= 0xf;
+			if (cap_n_ports > 4)
+				cap_n_ports = 4;
+		}
 	}
 
 	VPRINTK("cap 0x%x  port_map 0x%x  n_ports %d\n",
@@ -1757,7 +1775,7 @@ static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_out;
 	}
 
-	if (pci_enable_msi(pdev) == 0)
+	if ((pdev->vendor != PCI_VENDOR_ID_MARVELL) && (pci_enable_msi(pdev) == 0))
 		have_msi = 1;
 	else {
 		pci_intx(pdev, 1);

Re: Add suport for Marvell 88SE6121 in ahci

[Jose Alberto Reguero]

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

El Miércoles, 31 de Enero de 2007 16:03, Jeff Garzik escribió:
> Jose Alberto Reguero wrote:
> > This work for kernel 2.6.20-rc6
> >
> > First apply this patch:
> >
> > http://marc.theaimsgroup.com/?l=linux-ide&m=116986924301674&w=2
> >
> > Then apply the patch attached.
> >
> > Comments:
> >
> > The Marvell 88SE6121 has three ports (0,1,2). The PATA port is port 2.
> > (PATA port for 6141 is port 4). In M2V Motherboard(Marvell 88SE6121)
> > there is only two SATA ports, one of them is external SATA. This two
> > ports work well with this patch.
> >
> > With this part:
> >
> > <       if (pci_enable_msi(pdev) == 0)
> > ---
> >
> >>       if ((pdev->vendor != PCI_VENDOR_ID_MARVELL) &&
> >> (pci_enable_msi(pdev)
> >
> > == 0))
> >
> > you don't need to disable MSI in kernel.
> > MMCONFIG is disabled at startup in M2V.
> >
> > dmesg:
> > ........
> > PCI: Not using MMCONFIG.
>
> Very useful data points, thanks!
>
> For the future, please make sure to create patches using "diff -u",
> otherwise the patches are very difficult to read and apply.
>
> I've attached your patch as a "diff -u" patch so that others can check
> it out.
>
> 	Jeff

Thanks.
I have another question. When attaching a external SATA disk to the external 
SATA PORT, the driver take som seconds, and recognize de disk, and work well, 
but there are some errors in dmesg. 

dmesg1 is after attaching de disk

dmesg2 is after detaching the disk

Can you look them?
They are important?

Jose Alberto

[-- Attachment #2: dmesg1 --]
[-- Type: text/plain, Size: 1201 bytes --]

ata4: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x2 frozen
ata4: (irq_stat 0x02400000, PHY RDY changed)
ata4: waiting for device to spin up (8 secs)
ata4: soft resetting port
ata4: softreset failed (port busy but CLO unavailable)
ata4: softreset failed, retrying in 5 secs
ata4: hard resetting port
ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata4.00: ATA-7, max UDMA/133, 586072368 sectors: LBA48 NCQ (depth 31/32)
ata4.00: Drive reports diagnostics failure. This may indicate a drive
ata4.00: fault or invalid emulation. Contact drive vendor for information.
ata4.00: configured for UDMA/133
ata4: EH complete
scsi 3:0:0:0: Direct-Access     ATA      ST3300831AS      3.03 PQ: 0 ANSI: 5
SCSI device sdd: 586072368 512-byte hdwr sectors (300069 MB)
sdd: Write Protect is off
sdd: Mode Sense: 00 3a 00 00
SCSI device sdd: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdd: 586072368 512-byte hdwr sectors (300069 MB)
sdd: Write Protect is off
sdd: Mode Sense: 00 3a 00 00
SCSI device sdd: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdd: sdd1
sd 3:0:0:0: Attached scsi disk sdd
sd 3:0:0:0: Attached scsi generic sg3 type 0

[-- Attachment #3: dmesg2 --]
[-- Type: text/plain, Size: 604 bytes --]

ata4: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x2 frozen
ata4: (irq_stat 0x04400000, PHY RDY changed)
ata4: soft resetting port
ata4: SATA link down (SStatus 0 SControl 300)
ata4: failed to recover some devices, retrying in 5 secs
ata4: hard resetting port
ata4: SATA link down (SStatus 0 SControl 300)
ata4: failed to recover some devices, retrying in 5 secs
ata4: hard resetting port
ata4: SATA link down (SStatus 0 SControl 300)
ata4.00: disabled
ata4: EH complete
ata4.00: detaching (SCSI 3:0:0:0)
Synchronizing SCSI cache for disk sdd:
FAILED
  status = 0, message = 00, host = 4, driver = 00

Re: Add suport for Marvell 88SE6121 in ahci

[Jose Alberto Reguero]

El Miércoles, 31 de Enero de 2007, Jeff Garzik escribió:
> Jose Alberto Reguero wrote:
> > This work for kernel 2.6.20-rc6
> >
> > First apply this patch:
> >
> > http://marc.theaimsgroup.com/?l=linux-ide&m=116986924301674&w=2
> >
> > Then apply the patch attached.
> >
> > Comments:
> >
> > The Marvell 88SE6121 has three ports (0,1,2). The PATA port is port 2.
> > (PATA port for 6141 is port 4). In M2V Motherboard(Marvell 88SE6121)
> > there is only two SATA ports, one of them is external SATA. This two
> > ports work well with this patch.
> >
> > With this part:
> >
> > <       if (pci_enable_msi(pdev) == 0)
> > ---
> >
> >>       if ((pdev->vendor != PCI_VENDOR_ID_MARVELL) &&
> >> (pci_enable_msi(pdev)
> >
> > == 0))
> >
> > you don't need to disable MSI in kernel.
> > MMCONFIG is disabled at startup in M2V.
> >
> > dmesg:
> > ........
> > PCI: Not using MMCONFIG.
>
> Very useful data points, thanks!
>
> For the future, please make sure to create patches using "diff -u",
> otherwise the patches are very difficult to read and apply.
>
> I've attached your patch as a "diff -u" patch so that others can check
> it out.
>
> 	Jeff

I found that the driver work well for Marvel 6121 without the flag 
ATA_FLAG_SKIP_D2H_BSY.
What is the meaning of this flag?

Jose Alberto

Re: Add suport for Marvell 88SE6121 in ahci

[Tejun Heo]

Jose Alberto Reguero wrote:
> I found that the driver work well for Marvel 6121 without the flag 
> ATA_FLAG_SKIP_D2H_BSY.
> What is the meaning of this flag?

This is usually meaningful with hotplug.  The problem is that, on some
SATA controllers including ahci and sata_sil24, the wait for the first
D2H Reg FIS indicating device readiness isn't reliable.  It's supposed
to work like the following (sata_sil behaves pretty good on this regard).

1. User hotplug device to port and PHY event interrupt raised, the
controller should set BSY status bit.

2. libata EH kicks in and waits for BSY to clear (current timeout 30s).
 Done by the first D2H Reg FIS.  Drives usually send this after spinup
is complete.

3. After BSY is cleared, the device is reset.

The problem with ahci and sata_sil24 is that there really is no TF
status register is non-existent or emulated in some way and they doesn't
let us reliably wait for !BSY after hotplug event.  This often makes the
wait in #2 timeout and that's 30 seconds.  :-(

So, the flag makes libata EH skip #2.  Post-reset waiting is reliable,
so there is no timeout.  But this causes problems on some devices (the
Quantum GoVault thing) which takes a looooong time to get ready and
don't respond correctly to phy reset while it's busy initializing.

The flag is there primarily for ICH ahci's.  Marvell might be different.
 Care to remove SKIP_D2H_BSY and test a LOT of hotplugging with several
different devices?  :-)

-- 
tejun

Re: Add suport for Marvell 88SE6121 in ahci

[Jose Alberto Reguero]

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

El Domingo, 18 de Marzo de 2007, Tejun Heo escribió:
> Jose Alberto Reguero wrote:
> > I found that the driver work well for Marvel 6121 without the flag
> > ATA_FLAG_SKIP_D2H_BSY.
> > What is the meaning of this flag?
>
> This is usually meaningful with hotplug.  The problem is that, on some
> SATA controllers including ahci and sata_sil24, the wait for the first
> D2H Reg FIS indicating device readiness isn't reliable.  It's supposed
> to work like the following (sata_sil behaves pretty good on this regard).
>
> 1. User hotplug device to port and PHY event interrupt raised, the
> controller should set BSY status bit.
>
> 2. libata EH kicks in and waits for BSY to clear (current timeout 30s).
>  Done by the first D2H Reg FIS.  Drives usually send this after spinup
> is complete.
>
> 3. After BSY is cleared, the device is reset.
>
> The problem with ahci and sata_sil24 is that there really is no TF
> status register is non-existent or emulated in some way and they doesn't
> let us reliably wait for !BSY after hotplug event.  This often makes the
> wait in #2 timeout and that's 30 seconds.  :-(
>
> So, the flag makes libata EH skip #2.  Post-reset waiting is reliable,
> so there is no timeout.  But this causes problems on some devices (the
> Quantum GoVault thing) which takes a looooong time to get ready and
> don't respond correctly to phy reset while it's busy initializing.
>
> The flag is there primarily for ICH ahci's.  Marvell might be different.
>  Care to remove SKIP_D2H_BSY and test a LOT of hotplugging with several
> different devices?  :-)

With  SKIP_D2H_BSY I have a old dmesg -> dmesg1 (2.6.20 I think)
I test with and a external sata disk without SKIP_D2H_BSY -> dmes2 
(2.6.21-rc4)
Both takes ~8 seconds.

Thanks
Jose Alberto



[-- Attachment #2: dmesg1 --]
[-- Type: text/plain, Size: 1201 bytes --]

ata4: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x2 frozen
ata4: (irq_stat 0x02400000, PHY RDY changed)
ata4: waiting for device to spin up (8 secs)
ata4: soft resetting port
ata4: softreset failed (port busy but CLO unavailable)
ata4: softreset failed, retrying in 5 secs
ata4: hard resetting port
ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata4.00: ATA-7, max UDMA/133, 586072368 sectors: LBA48 NCQ (depth 31/32)
ata4.00: Drive reports diagnostics failure. This may indicate a drive
ata4.00: fault or invalid emulation. Contact drive vendor for information.
ata4.00: configured for UDMA/133
ata4: EH complete
scsi 3:0:0:0: Direct-Access     ATA      ST3300831AS      3.03 PQ: 0 ANSI: 5
SCSI device sdd: 586072368 512-byte hdwr sectors (300069 MB)
sdd: Write Protect is off
sdd: Mode Sense: 00 3a 00 00
SCSI device sdd: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdd: 586072368 512-byte hdwr sectors (300069 MB)
sdd: Write Protect is off
sdd: Mode Sense: 00 3a 00 00
SCSI device sdd: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdd: sdd1
sd 3:0:0:0: Attached scsi disk sdd
sd 3:0:0:0: Attached scsi generic sg3 type 0

[-- Attachment #3: dmesg2 --]
[-- Type: text/plain, Size: 1058 bytes --]

ata4: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0x2 frozen
ata4: (irq_stat 0x02400000, PHY RDY changed)
ata4: port is slow to respond, please be patient (Status 0x80)
ata4: soft resetting port
ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata4.00: ATA-7: ST3300831AS, 3.03, max UDMA/133
ata4.00: 586072368 sectors, multi 0: LBA48 NCQ (depth 31/32)
ata4.00: configured for UDMA/133
ata4: EH complete
scsi 3:0:0:0: Direct-Access     ATA      ST3300831AS      3.03 PQ: 0 ANSI: 5
SCSI device sdd: 586072368 512-byte hdwr sectors (300069 MB)
sdd: Write Protect is off
sdd: Mode Sense: 00 3a 00 00
SCSI device sdd: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
SCSI device sdd: 586072368 512-byte hdwr sectors (300069 MB)
sdd: Write Protect is off
sdd: Mode Sense: 00 3a 00 00
SCSI device sdd: write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sdd:<6>ata4: D2H reg with I during NCQ, this message won't be printed again
 sdd1
sd 3:0:0:0: Attached scsi disk sdd
sd 3:0:0:0: Attached scsi generic sg5 type 0

Re: Add suport for Marvell 88SE6121 in ahci

[Tejun Heo]

Jose Alberto Reguero wrote:
>> The flag is there primarily for ICH ahci's.  Marvell might be different.
>>  Care to remove SKIP_D2H_BSY and test a LOT of hotplugging with several
>> different devices?  :-)
> 
> With  SKIP_D2H_BSY I have a old dmesg -> dmesg1 (2.6.20 I think)
> I test with and a external sata disk without SKIP_D2H_BSY -> dmes2 
> (2.6.21-rc4)
> Both takes ~8 seconds.

I see.  Was the disk spun up when you hotplugged it?  The condition
doesn't trigger always and more importantly, there is no way to reliably
wait for the condition in ahci.  ahci's TF status register is read from
the received D2H Reg FIS area and there is no mechanism to set the BSY
bit on a PHY event, so no matter what we do in the driver, there is an
inherent race.

ie. we can try to set the BSY bit in the interrupt handler right after a
PHY event but the hardware can always beat us and when that happens we
set BSY after the first D2H Reg FIS is received which will in turn cause
wait timeout, so I'm voting for keeping the flag.

Thanks.

-- 
tejun