Withdraw IGN_SERR_INTERNAL for SB800 SATA

PATCH In review Shane Huang 2 participants 4 messages Nov 18, 2008 – Nov 19, 2008 libata

Share & tools

Shane Huang

author

2008-11-18 09:27 UTC #0 lore raw

PATCH 1/1 Withdraw IGN_SERR_INTERNAL for SB800 SATA

There is an issue in ATI SB600/SB700 SATA that PxSERR.E should not be set on some conditions, which will lead to many SATA ODD error messages. commit 55a61604cd1354e1783364e1c901034f2f474b7d is the workaround. Since SB800 fixed this HW issue, IGN_SERR_INTERNAL should be withdrawn for SB800.

Signed-off-by Shane Huang <[email protected]>

drivers/ata/ahci.c +6 -2

diff -ruN c/drivers/ata/ahci.c d/drivers/ata/ahci.c
@@ -105,7 +105,7 @@
 	board_ahci_ign_iferr	= 2,
 	board_ahci_sb600	= 3,
 	board_ahci_mv		= 4,
-	board_ahci_sb700	= 5,
+	board_ahci_sb700	= 5, /* for SB700 and SB800 */
 	board_ahci_mcp65	= 6,
 	board_ahci_nopmp	= 7,

@@ -439,7 +439,7 @@
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	/* board_ahci_sb700 */
+	/* board_ahci_sb700, for SB700 and SB800 */
 	{
 		AHCI_HFLAGS	(AHCI_HFLAG_IGN_SERR_INTERNAL),
 		.flags		= AHCI_FLAG_COMMON,

@@ -2610,6 +2610,10 @@
 	    (pdev->revision == 0xa1 || pdev->revision == 0xa2))
 		hpriv->flags |= AHCI_HFLAG_NO_MSI;
 
+	/* SB800 does NOT need the workaround to ignore SERR_INTERNAL */
+	if (board_id == board_ahci_sb700 && pdev->revision >= 0x40)
+		hpriv->flags &= ~AHCI_HFLAG_IGN_SERR_INTERNAL;
+
 	if ((hpriv->flags & AHCI_HFLAG_NO_MSI) || pci_enable_msi(pdev))
 		pci_intx(pdev, 1);

Tejun Heo

2008-11-18 16:09 UTC #1 lore raw

Quoted 30 lines · Shane Huang

There is an issue in ATI SB600/SB700 SATA that PxSERR.E should not be set on some conditions, which will lead to many SATA ODD error messages. commit 55a61604cd1354e1783364e1c901034f2f474b7d is the workaround. Since SB800 fixed this HW issue, IGN_SERR_INTERNAL should be withdrawn for SB800.
Signed-off-by Shane Huang <[email protected]>
diff -ruN c/drivers/ata/ahci.c d/drivers/ata/ahci.c
drivers/ata/ahci.c +2 -2
@@ -105,7 +105,7 @@
 	board_ahci_ign_iferr	= 2,
 	board_ahci_sb600	= 3,
 	board_ahci_mv		= 4,
-	board_ahci_sb700	= 5,
+	board_ahci_sb700	= 5, /* for SB700 and SB800 */
 	board_ahci_mcp65	= 6,
 	board_ahci_nopmp	= 7,
 
@@ -439,7 +439,7 @@
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	/* board_ahci_sb700 */
+	/* board_ahci_sb700, for SB700 and SB800 */
 	{
 		AHCI_HFLAGS	(AHCI_HFLAG_IGN_SERR_INTERNAL),
 		.flags		= AHCI_FLAG_COMMON,

drivers/ata/ahci.c line 2618

Quoted 9 lines

drivers/ata/ahci.c +4 -0

@@ -2610,6 +2610,10 @@
 	    (pdev->revision == 0xa1 || pdev->revision == 0xa2))
 		hpriv->flags |= AHCI_HFLAG_NO_MSI;
 
+	/* SB800 does NOT need the workaround to ignore SERR_INTERNAL */
+	if (board_id == board_ahci_sb700 && pdev->revision >= 0x40)
+		hpriv->flags &= ~AHCI_HFLAG_IGN_SERR_INTERNAL;
+
 	if ((hpriv->flags & AHCI_HFLAG_NO_MSI) || pci_enable_msi(pdev))
 		pci_intx(pdev, 1);

Great, but please use separate board_ahci_* entry. ie. board_ahci_sb700_ign_serr for the ones with the problem and board_ahci_sb700 for other sb700s and sb800s.

Thanks.

Signature · 1 line

tejun

Huang, Shane

author

2008-11-19 02:41 UTC #2 lore raw

Hi Tejun,

Quoted 13 lines

-----Original Message-----
From: Tejun Heo [mailto:[email protected]]
drivers/ata/ahci.c line 2618
Quoted 7 lines
drivers/ata/ahci.c +4 -0
+	/* SB800 does NOT need the workaround to ignore SERR_INTERNAL */
+	if (board_id == board_ahci_sb700 && pdev->revision >= 0x40)
+		hpriv->flags &= ~AHCI_HFLAG_IGN_SERR_INTERNAL;
+
 	if ((hpriv->flags & AHCI_HFLAG_NO_MSI) || pci_enable_msi(pdev))
 		pci_intx(pdev, 1);
Great, but please use separate board_ahci_* entry. ie. board_ahci_sb700_ign_serr for the ones with the problem and board_ahci_sb700 for other sb700s and sb800s.

Thanks for your suggestion.

Different SATA controller revisions with/without the problem have SAME PCI device ID,
so it's difficult to distinguish them with board_ahci_sb700_ign_serr and board_ahci_sb700 at the beginning.

Although we can distinguish them in ahci_init_one() and reevaluate board_id
as well as ent->driver_data, I still suggest we keep my submitted patch because it seems to be neater. :-)

And I also added some comments for better maintainance like /* board_ahci_sb700, for SB700 and SB800 */
/* SB800 does NOT need the workaround to ignore SERR_INTERNAL */ It should be much clear to us now...

Thanks
Shane

Tejun Heo

Acked-by

2008-11-19 02:49 UTC #3 lore raw

Quoted 28 lines · Shane

Hi Tejun,
Quoted 12 lines
-----Original Message-----
From: Tejun Heo [mailto:[email protected]]
drivers/ata/ahci.c line 2618
Quoted 7 lines
drivers/ata/ahci.c +4 -0
+	/* SB800 does NOT need the workaround to ignore SERR_INTERNAL */
+	if (board_id == board_ahci_sb700 && pdev->revision >= 0x40)
+		hpriv->flags &= ~AHCI_HFLAG_IGN_SERR_INTERNAL;
+
 	if ((hpriv->flags & AHCI_HFLAG_NO_MSI) || pci_enable_msi(pdev))
 		pci_intx(pdev, 1);
Great, but please use separate board_ahci_* entry. ie. board_ahci_sb700_ign_serr for the ones with the problem and board_ahci_sb700 for other sb700s and sb800s.
Thanks for your suggestion.
Different SATA controller revisions with/without the problem have SAME PCI device ID,
so it's difficult to distinguish them with board_ahci_sb700_ign_serr and board_ahci_sb700 at the beginning.
Although we can distinguish them in ahci_init_one() and reevaluate board_id
as well as ent->driver_data, I still suggest we keep my submitted patch because it seems to be neater. :-)

Quoted 4 lines

And I also added some comments for better maintainance like /* board_ahci_sb700, for SB700 and SB800 */
/* SB800 does NOT need the workaround to ignore SERR_INTERNAL */ It should be much clear to us now...

Ah.. right, can't match revision. Yeap, then,

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun