Re: Disabling ADMA? (was Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61)
Jeff Garzik wrote: Kuan Luo wrote: @@ -1714,3 +2761,6 @@ module_init(nv_init); module_exit(nv_exit); module_param_named(adma, adma_enabled, bool, 0444); MODULE_PARM_DESC(adma, "Enable use of ADMA (Default: true)"); +module_param_named(ncq, ncq_enabled, bool, 0444); +MODULE_PARM_DESC(ncq, "Enable use of NCQ (Default: false)"); After looking through sata_nv bug reports, I am leaning towards disabling ADMA by default, and wanted to solicit comments. While admittedly not knowing the root cause, it seems like every current outstanding sata_nv bug report that remains after switching out hardware can be solved by setting module option 'adma' to zero. That's my first suggestion upon any bugzilla sata_nv bug, and it usually works. You can look through bugs assigned to or CC'd to [EMAIL PROTECTED] (kernel.org bugs) [EMAIL PROTECTED] (redhat.com bugs) for examples. Do you have some specific bug numbers? Other than this one: http://bugzilla.kernel.org/show_bug.cgi?id=8421 That one is hotplug-related, and it would seem only affects specific boards as it works fine for me. Likely NVIDIA input would be needed on that one to get much farther. I didn't find any other sata_nv ADMA-related bugs in the kernel.org or RH Bugzillas. Most of the reports I've gotten recently have been in one of these categories: -broken NCQ implementation on the drive (usually shows up as commands timing out with CPB resp_flags of 2) -cabling or power problems (often showing errors in the SError register) There have been a few reports yet of problems with commands getting issued and seeing to go nowhere. These can be difficult to diagnose. However, in some cases there's evidence leaning toward this being a drive issue as well, ex: this thread: http://groups.google.ca/group/linux.kernel/browse_frm/thread/2ebf329e3f6470eb/0d26dff9a30d29e9?lnk=gst Here there's a report of this drive model doing this on multiple controllers. Keep in mind, in the case where a broken NCQ implementation on the drive is the cause, disabling ADMA (and hence NCQ) will appear to "fix" the problem when the proper course of action would be to blacklist NCQ on that drive globally. Given the relative scarcity of problem reports (at least those I've seen), and the fact that this is not obscure hardware (they must have made millions of these nForce4 boards, and I think some may even still be in production), I'm reluctant to say we should be switching ADMA off by default. I still need to review the SWNCQ patch in detail, but I presume it is possible to still use SWNCQ without ADMA? Yes, they're independent. On a side note, I would rather default SWNCQ to 'on'. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Disabling ADMA? (was Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61)
Kuan Luo wrote: @@ -1714,3 +2761,6 @@ module_init(nv_init); module_exit(nv_exit); module_param_named(adma, adma_enabled, bool, 0444); MODULE_PARM_DESC(adma, "Enable use of ADMA (Default: true)"); +module_param_named(ncq, ncq_enabled, bool, 0444); +MODULE_PARM_DESC(ncq, "Enable use of NCQ (Default: false)"); After looking through sata_nv bug reports, I am leaning towards disabling ADMA by default, and wanted to solicit comments. While admittedly not knowing the root cause, it seems like every current outstanding sata_nv bug report that remains after switching out hardware can be solved by setting module option 'adma' to zero. That's my first suggestion upon any bugzilla sata_nv bug, and it usually works. You can look through bugs assigned to or CC'd to [EMAIL PROTECTED] (kernel.org bugs) [EMAIL PROTECTED] (redhat.com bugs) for examples. I still need to review the SWNCQ patch in detail, but I presume it is possible to still use SWNCQ without ADMA? On a side note, I would rather default SWNCQ to 'on'. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Disabling ADMA? (was Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61)
Kuan Luo wrote: @@ -1714,3 +2761,6 @@ module_init(nv_init); module_exit(nv_exit); module_param_named(adma, adma_enabled, bool, 0444); MODULE_PARM_DESC(adma, Enable use of ADMA (Default: true)); +module_param_named(ncq, ncq_enabled, bool, 0444); +MODULE_PARM_DESC(ncq, Enable use of NCQ (Default: false)); After looking through sata_nv bug reports, I am leaning towards disabling ADMA by default, and wanted to solicit comments. While admittedly not knowing the root cause, it seems like every current outstanding sata_nv bug report that remains after switching out hardware can be solved by setting module option 'adma' to zero. That's my first suggestion upon any bugzilla sata_nv bug, and it usually works. You can look through bugs assigned to or CC'd to [EMAIL PROTECTED] (kernel.org bugs) [EMAIL PROTECTED] (redhat.com bugs) for examples. I still need to review the SWNCQ patch in detail, but I presume it is possible to still use SWNCQ without ADMA? On a side note, I would rather default SWNCQ to 'on'. Jeff - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Disabling ADMA? (was Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61)
Jeff Garzik wrote: Kuan Luo wrote: @@ -1714,3 +2761,6 @@ module_init(nv_init); module_exit(nv_exit); module_param_named(adma, adma_enabled, bool, 0444); MODULE_PARM_DESC(adma, Enable use of ADMA (Default: true)); +module_param_named(ncq, ncq_enabled, bool, 0444); +MODULE_PARM_DESC(ncq, Enable use of NCQ (Default: false)); After looking through sata_nv bug reports, I am leaning towards disabling ADMA by default, and wanted to solicit comments. While admittedly not knowing the root cause, it seems like every current outstanding sata_nv bug report that remains after switching out hardware can be solved by setting module option 'adma' to zero. That's my first suggestion upon any bugzilla sata_nv bug, and it usually works. You can look through bugs assigned to or CC'd to [EMAIL PROTECTED] (kernel.org bugs) [EMAIL PROTECTED] (redhat.com bugs) for examples. Do you have some specific bug numbers? Other than this one: http://bugzilla.kernel.org/show_bug.cgi?id=8421 That one is hotplug-related, and it would seem only affects specific boards as it works fine for me. Likely NVIDIA input would be needed on that one to get much farther. I didn't find any other sata_nv ADMA-related bugs in the kernel.org or RH Bugzillas. Most of the reports I've gotten recently have been in one of these categories: -broken NCQ implementation on the drive (usually shows up as commands timing out with CPB resp_flags of 2) -cabling or power problems (often showing errors in the SError register) There have been a few reports yet of problems with commands getting issued and seeing to go nowhere. These can be difficult to diagnose. However, in some cases there's evidence leaning toward this being a drive issue as well, ex: this thread: http://groups.google.ca/group/linux.kernel/browse_frm/thread/2ebf329e3f6470eb/0d26dff9a30d29e9?lnk=gst Here there's a report of this drive model doing this on multiple controllers. Keep in mind, in the case where a broken NCQ implementation on the drive is the cause, disabling ADMA (and hence NCQ) will appear to fix the problem when the proper course of action would be to blacklist NCQ on that drive globally. Given the relative scarcity of problem reports (at least those I've seen), and the fact that this is not obscure hardware (they must have made millions of these nForce4 boards, and I think some may even still be in production), I'm reluctant to say we should be switching ADMA off by default. I still need to review the SWNCQ patch in detail, but I presume it is possible to still use SWNCQ without ADMA? Yes, they're independent. On a side note, I would rather default SWNCQ to 'on'. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, Jeff Garzik írta: Alan Cox wrote: That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable The default DMA mask -everywhere- is 32 bits. A lot of code will break if this assumption ever changes, not just libata. Jeff thanks for clarifying this. I tested the effect of this patch on 2.6.22-rc2 + CFS-v13 with the current CVS version of PostgreSQL 8.3devel. pgbench with 25 clients and some large number of transactions to make the result stable showed substantial increase in throughput. Without NCQ, I got around 446 tps, with NCQ I got around 680 via local TCP connection. Previously, I got this level of performance only over local unix socket and smaller number of simultaneous clients. The disk is Seagate 320GB (ST3320620AS). Again, thanks for this patch. Best regards, Zoltán Böszörményi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, Jeff Garzik írta: Alan Cox wrote: That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable The default DMA mask -everywhere- is 32 bits. A lot of code will break if this assumption ever changes, not just libata. Jeff thanks for clarifying this. I tested the effect of this patch on 2.6.22-rc2 + CFS-v13 with the current CVS version of PostgreSQL 8.3devel. pgbench with 25 clients and some large number of transactions to make the result stable showed substantial increase in throughput. Without NCQ, I got around 446 tps, with NCQ I got around 680 via local TCP connection. Previously, I got this level of performance only over local unix socket and smaller number of simultaneous clients. The disk is Seagate 320GB (ST3320620AS). Again, thanks for this patch. Best regards, Zoltán Böszörményi - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Kuan Luo wrote:
Thanks for your comment, see the explaination inline.
We'll apply your advice in later patch.
...
Please don't duplicate this code in the driver, this is part of libata
core in libata-scsi.c. Add an export for these functions if you need to
use them in the driver.
[kuan]: These calls are declared in static type . I can't export them
and don't want to
modify libata code.
If you really need these functions then they should be made non-static
and exported. Duplicating the code is not a solution.
I'm not so sure you actually need all that, though. I suspect you can
likely handle the deferring of commands if you detect an FPDMA data
phase inside the qc_issue function only (like you already do in some
cases) instead of having to mess with deferring them at the SCSI layer.
I'm still puzzling out how this stuff all works, but it looks like this
code makes you stop sending new commands if:
-the port is in the FPDMA Data Phase (DMA Setup FIS received but the
transfer is not complete yet) - I assume the hardware doesn't handle
this itself, which seems rather unique
-we previously deferred a command inside of qc_issue because we were in
the FPDMA data phase
-we previously saw dhfis_flags not equal to qc_active, or we got a
BACKOUT interrupt (whatever exactly that means), both of which set some
value in the back_byte
[kuan]:
-If we got BACKOUT interrupt, it means that a command just sent by
driver
backed out.The driver should resend the command.So new commands should
be defered.
-If dhfis_flags != qc_active, it indicates that the last command doesn't
generate a device to host register FIS .
After sending some commands, I found that the last command sometimes has
this problem
but previous commands are normal.In this case, we need resend the last
command.
Both cases set back_byte.
The case where the command didn't generate a D2H FIS should likely be
investigated further, otherwise we don't necessarily know that this
workaround will work in all cases?
This code seems a bit odd. Isn't this tossing out a bunch of potential
error status, etc?
[kuan]:
If there are commands in queue, the driver can send a new command
only after receiving dhfis intr of previous command and before receiving
any dmasetup fis intr.
In this place, i do the last check before sending the command.
But the D2H FIS can contain an error indication, correct? If that
happens here it won't detect this. In this situation error handling
should be triggered.
+ done_mask = pp->qc_active ^ sactive;
+ if (unlikely(done_mask & sactive)) {
+ ata_port_printk(ap, KERN_ERR, "illegal qc_active
transition "
+ "(%08x->%08x)\n", ap->qc_active,
sactive);
+ return -EINVAL;
+ }
Shouldn't this trigger error handling if it happens instead of just
printing an error?
[kuan]:
I think the error handling can be triggered by timeout. In fact,
this case should seldom happen.
There have been reports of some drives with bad NCQ implementations that
return completion status for commands that were not issued. If we detect
this case we should raise an HSM violation which will disable NCQ on
this drive if it happens repeatedly. See the code in ahci.c in
ahci_host_intr.
This comment still applies:
Additional/general comments:
Think you need some code to handle suspend and resume (re-enable SATA
MMIO space, etc.)
--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Fri, 18 May 2007 10:34:35 -0400 Jeff Garzik <[EMAIL PROTECTED]> wrote: > Alan Cox wrote: > >> That shouldn't be a problem, libata default DMA mask is 32 bits (which > >> isn't overridden with this controller) and so the block layer will > >> bounce any data being read/written above that point with IOMMU or > >> swiotlb. The comment is a bit unnecessarily scary. > > > > Adding a BUG_ON for this would be wise. Its trivial to check and a BUG > > rather than corruption if this assumption ever changes would be far > > preferable > > The default DMA mask -everywhere- is 32 bits. > > A lot of code will break if this assumption ever changes, not just libata. Little lesson from history.. Over ten years ago someone (Eric Youngdale I guess) stuck a panic check for DMA over 16MBytes in the AHA 1542 ISA SCSI driver. Last year it triggered. The panic probably saved someone from corruption and meant the bug could be fixed. Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Alan Cox wrote: That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable The default DMA mask -everywhere- is 32 bits. A lot of code will break if this assumption ever changes, not just libata. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
> That shouldn't be a problem, libata default DMA mask is 32 bits (which > isn't overridden with this controller) and so the block layer will > bounce any data being read/written above that point with IOMMU or > swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Thanks for your comment, see the explaination inline.
We'll apply your advice in later patch.
-Original Message-
From: Robert Hancock [mailto:[EMAIL PROTECTED]
Sent: Friday, May 18, 2007 9:48 AM
To: Peer Chen
Cc: linux-kernel@vger.kernel.org; Jeff Garzik;
[EMAIL PROTECTED]; Kuan Luo; [EMAIL PROTECTED]
Subject: Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for
MCP51/MCP55/MCP61
Peer Chen wrote:
> Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
> controller.
>
> This patch base on sata_nv.c file from kernel 2.6.22-rc1
>
> See attachment for the patch.
>
> Signed-off-by: Kuan Luo <[EMAIL PROTECTED]>
> Signed-off-by: Peer Chen <[EMAIL PROTECTED]>
Good to finally see this come out. I've pasted the code below (indented)
in order to make some comments:
--- linux-2.6.22-rc1/drivers/ata/sata_nv.c.orig 2007-05-17
14:48:26.0 -0400
+++ linux-2.6.22-rc1/drivers/ata/sata_nv.c 2007-05-17
17:07:28.0 -0400
@@ -46,6 +46,8 @@
#include
#include
#include
+#include
+#include
#include
#define DRV_NAME "sata_nv"
@@ -169,6 +171,36 @@ enum {
NV_ADMA_PORT_REGISTER_MODE = (1 << 0),
NV_ADMA_ATAPI_SETUP_COMPLETE= (1 << 1),
+ /* MCP55 reg offset */
+ NV_CTL_MCP55= 0x400,
+ NV_INT_STATUS_MCP55 = 0x440,
+ NV_INT_ENABLE_MCP55 = 0x444,
+ NV_NCQ_REG_MCP55= 0x448,
+ NV_CH1_SACTIVE_MCP55= 0x0C,
+
+ /* MCP55 */
+ NV_INT_ALL_MCP55= 0x,
+ NV_INT_PORT_SHIFT_MCP55 = 16, /* each port
occupies 16 bits */
+ NV_INT_MASK_MCP55 = NV_INT_ALL_MCP55 &
0xfffd,
+
+ /* NCQ ENABLE BITS*/
+ NV_CTL_PRI_SWNCQ= 0x02,
+ NV_CTL_SEC_SWNCQ= 0x04,
+
+ /* MCP55 status bits*/
+ NV_INT_DEV_MCP55= 0x01,
+ NV_INT_PM_MCP55 = 0x02,
+ NV_INT_ADDED_MCP55 = 0x04,
+ NV_INT_REMOVED_MCP55= 0x08,
+
+ NV_INT_BACKOUT_MCP55= 0x10,
+ NV_INT_SDBFIS_MCP55 = 0x20,
+ NV_INT_DHREGFIS_MCP55 = 0x40,
+ NV_INT_DMASETUP_MCP55 = 0x80,
+
+ NV_INT_HOTPLUG_MCP55= (NV_INT_ADDED_MCP55 |
+
NV_INT_REMOVED_MCP55),
+
};
/* ADMA Physical Region Descriptor - one SG segment */
@@ -264,13 +296,118 @@ static void nv_adma_host_stop(struct ata
static void nv_adma_post_internal_cmd(struct ata_queued_cmd
*qc);
static void nv_adma_tf_read(struct ata_port *ap, struct
ata_taskfile *tf);
+static void ncq_error_handler(struct ata_port *ap);
+static void nv_mcp55_thaw(struct ata_port *ap);
+static void nv_mcp55_freeze(struct ata_port *ap);
+static void ncq_host_init(struct ata_host *host);
+static void nv_bmdma_stop(struct ata_port *ap);
+static int nv_std_qc_defer(struct ata_port *ap);
+static int nv_port_start(struct ata_port *ap);
+static void nv_port_stop(struct ata_port *ap);
+static void ncq_clear(struct ata_port *ap);
+static void nv_qc_prep(struct ata_queued_cmd *qc);
+static void nv_fill_sg(struct ata_queued_cmd *qc);
+static void ncq_sactive_start (struct ata_queued_cmd *qc);
+static u32 ncq_sactive_value (struct ata_port *ap);
+static unsigned int nv_qc_issue_prot(struct ata_queued_cmd
*qc);
+static u32 ncq_tag_value(struct ata_port *ap);
+static int nv_ncqintr_sdbfis(struct ata_port *ap);
+static int nv_ncqintr_dmasetupfis(struct ata_port *ap);
+static void ncq_clear_singlefis(struct ata_port *ap, u32 val);
+static u32 ncq_ownfisintr_value (struct ata_port *ap);
+void ncq_hotplug(struct ata_port *ap, u32 fis);
+static irqreturn_t nv_mcp55_interrupt(int irq, void
*dev_instance);
+static int ncq_interrupt(struct ata_port *ap, u32 fis);
+static int nv_scsi_queuecmd(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *));
These functions should use "mcp51" or "swncq" or something in the name
instead of "ncq", since the latter implies it may be related to ADMA as
well.
[kuan]:
I will use a better name for these function.
+
+#undef NCQ_DEBUG
+#undef NCQ_VERBOSE_DEBUG
+#ifdef NCQ_DEBUG
+#define NPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt,
__FUNCTION__, ## args)
+#ifdef NCQ_VERBOSE_DEBUG
RE: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Thanks for your comment, see the explaination inline.
We'll apply your advice in later patch.
-Original Message-
From: Robert Hancock [mailto:[EMAIL PROTECTED]
Sent: Friday, May 18, 2007 9:48 AM
To: Peer Chen
Cc: linux-kernel@vger.kernel.org; Jeff Garzik;
[EMAIL PROTECTED]; Kuan Luo; [EMAIL PROTECTED]
Subject: Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for
MCP51/MCP55/MCP61
Peer Chen wrote:
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller.
This patch base on sata_nv.c file from kernel 2.6.22-rc1
See attachment for the patch.
Signed-off-by: Kuan Luo [EMAIL PROTECTED]
Signed-off-by: Peer Chen [EMAIL PROTECTED]
Good to finally see this come out. I've pasted the code below (indented)
in order to make some comments:
--- linux-2.6.22-rc1/drivers/ata/sata_nv.c.orig 2007-05-17
14:48:26.0 -0400
+++ linux-2.6.22-rc1/drivers/ata/sata_nv.c 2007-05-17
17:07:28.0 -0400
@@ -46,6 +46,8 @@
#include linux/device.h
#include scsi/scsi_host.h
#include scsi/scsi_device.h
+#include scsi/scsi.h
+#include scsi/scsi_cmnd.h
#include linux/libata.h
#define DRV_NAME sata_nv
@@ -169,6 +171,36 @@ enum {
NV_ADMA_PORT_REGISTER_MODE = (1 0),
NV_ADMA_ATAPI_SETUP_COMPLETE= (1 1),
+ /* MCP55 reg offset */
+ NV_CTL_MCP55= 0x400,
+ NV_INT_STATUS_MCP55 = 0x440,
+ NV_INT_ENABLE_MCP55 = 0x444,
+ NV_NCQ_REG_MCP55= 0x448,
+ NV_CH1_SACTIVE_MCP55= 0x0C,
+
+ /* MCP55 */
+ NV_INT_ALL_MCP55= 0x,
+ NV_INT_PORT_SHIFT_MCP55 = 16, /* each port
occupies 16 bits */
+ NV_INT_MASK_MCP55 = NV_INT_ALL_MCP55
0xfffd,
+
+ /* NCQ ENABLE BITS*/
+ NV_CTL_PRI_SWNCQ= 0x02,
+ NV_CTL_SEC_SWNCQ= 0x04,
+
+ /* MCP55 status bits*/
+ NV_INT_DEV_MCP55= 0x01,
+ NV_INT_PM_MCP55 = 0x02,
+ NV_INT_ADDED_MCP55 = 0x04,
+ NV_INT_REMOVED_MCP55= 0x08,
+
+ NV_INT_BACKOUT_MCP55= 0x10,
+ NV_INT_SDBFIS_MCP55 = 0x20,
+ NV_INT_DHREGFIS_MCP55 = 0x40,
+ NV_INT_DMASETUP_MCP55 = 0x80,
+
+ NV_INT_HOTPLUG_MCP55= (NV_INT_ADDED_MCP55 |
+
NV_INT_REMOVED_MCP55),
+
};
/* ADMA Physical Region Descriptor - one SG segment */
@@ -264,13 +296,118 @@ static void nv_adma_host_stop(struct ata
static void nv_adma_post_internal_cmd(struct ata_queued_cmd
*qc);
static void nv_adma_tf_read(struct ata_port *ap, struct
ata_taskfile *tf);
+static void ncq_error_handler(struct ata_port *ap);
+static void nv_mcp55_thaw(struct ata_port *ap);
+static void nv_mcp55_freeze(struct ata_port *ap);
+static void ncq_host_init(struct ata_host *host);
+static void nv_bmdma_stop(struct ata_port *ap);
+static int nv_std_qc_defer(struct ata_port *ap);
+static int nv_port_start(struct ata_port *ap);
+static void nv_port_stop(struct ata_port *ap);
+static void ncq_clear(struct ata_port *ap);
+static void nv_qc_prep(struct ata_queued_cmd *qc);
+static void nv_fill_sg(struct ata_queued_cmd *qc);
+static void ncq_sactive_start (struct ata_queued_cmd *qc);
+static u32 ncq_sactive_value (struct ata_port *ap);
+static unsigned int nv_qc_issue_prot(struct ata_queued_cmd
*qc);
+static u32 ncq_tag_value(struct ata_port *ap);
+static int nv_ncqintr_sdbfis(struct ata_port *ap);
+static int nv_ncqintr_dmasetupfis(struct ata_port *ap);
+static void ncq_clear_singlefis(struct ata_port *ap, u32 val);
+static u32 ncq_ownfisintr_value (struct ata_port *ap);
+void ncq_hotplug(struct ata_port *ap, u32 fis);
+static irqreturn_t nv_mcp55_interrupt(int irq, void
*dev_instance);
+static int ncq_interrupt(struct ata_port *ap, u32 fis);
+static int nv_scsi_queuecmd(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *));
These functions should use mcp51 or swncq or something in the name
instead of ncq, since the latter implies it may be related to ADMA as
well.
[kuan]:
I will use a better name for these function.
+
+#undef NCQ_DEBUG
+#undef NCQ_VERBOSE_DEBUG
+#ifdef NCQ_DEBUG
+#define NPRINTK(fmt, args...) printk(KERN_ERR %s: fmt,
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Alan Cox wrote: That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable The default DMA mask -everywhere- is 32 bits. A lot of code will break if this assumption ever changes, not just libata. Jeff - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Fri, 18 May 2007 10:34:35 -0400 Jeff Garzik [EMAIL PROTECTED] wrote: Alan Cox wrote: That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. Adding a BUG_ON for this would be wise. Its trivial to check and a BUG rather than corruption if this assumption ever changes would be far preferable The default DMA mask -everywhere- is 32 bits. A lot of code will break if this assumption ever changes, not just libata. Little lesson from history.. Over ten years ago someone (Eric Youngdale I guess) stuck a panic check for DMA over 16MBytes in the AHA 1542 ISA SCSI driver. Last year it triggered. The panic probably saved someone from corruption and meant the bug could be fixed. Alan - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Kuan Luo wrote:
Thanks for your comment, see the explaination inline.
We'll apply your advice in later patch.
...
Please don't duplicate this code in the driver, this is part of libata
core in libata-scsi.c. Add an export for these functions if you need to
use them in the driver.
[kuan]: These calls are declared in static type . I can't export them
and don't want to
modify libata code.
If you really need these functions then they should be made non-static
and exported. Duplicating the code is not a solution.
I'm not so sure you actually need all that, though. I suspect you can
likely handle the deferring of commands if you detect an FPDMA data
phase inside the qc_issue function only (like you already do in some
cases) instead of having to mess with deferring them at the SCSI layer.
I'm still puzzling out how this stuff all works, but it looks like this
code makes you stop sending new commands if:
-the port is in the FPDMA Data Phase (DMA Setup FIS received but the
transfer is not complete yet) - I assume the hardware doesn't handle
this itself, which seems rather unique
-we previously deferred a command inside of qc_issue because we were in
the FPDMA data phase
-we previously saw dhfis_flags not equal to qc_active, or we got a
BACKOUT interrupt (whatever exactly that means), both of which set some
value in the back_byte
[kuan]:
-If we got BACKOUT interrupt, it means that a command just sent by
driver
backed out.The driver should resend the command.So new commands should
be defered.
-If dhfis_flags != qc_active, it indicates that the last command doesn't
generate a device to host register FIS .
After sending some commands, I found that the last command sometimes has
this problem
but previous commands are normal.In this case, we need resend the last
command.
Both cases set back_byte.
The case where the command didn't generate a D2H FIS should likely be
investigated further, otherwise we don't necessarily know that this
workaround will work in all cases?
This code seems a bit odd. Isn't this tossing out a bunch of potential
error status, etc?
[kuan]:
If there are commands in queue, the driver can send a new command
only after receiving dhfis intr of previous command and before receiving
any dmasetup fis intr.
In this place, i do the last check before sending the command.
But the D2H FIS can contain an error indication, correct? If that
happens here it won't detect this. In this situation error handling
should be triggered.
+ done_mask = pp-qc_active ^ sactive;
+ if (unlikely(done_mask sactive)) {
+ ata_port_printk(ap, KERN_ERR, illegal qc_active
transition
+ (%08x-%08x)\n, ap-qc_active,
sactive);
+ return -EINVAL;
+ }
Shouldn't this trigger error handling if it happens instead of just
printing an error?
[kuan]:
I think the error handling can be triggered by timeout. In fact,
this case should seldom happen.
There have been reports of some drives with bad NCQ implementations that
return completion status for commands that were not issued. If we detect
this case we should raise an HSM violation which will disable NCQ on
this drive if it happens repeatedly. See the code in ahci.c in
ahci_host_intr.
This comment still applies:
Additional/general comments:
Think you need some code to handle suspend and resume (re-enable SATA
MMIO space, etc.)
--
Robert Hancock Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Zoltan Boszormenyi wrote: Hi, thanks for publishing this. Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA controller. This patch base on sata_nv.c file from kernel 2.6.22-rc1 See attachment for the patch. Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> Signed-off-by: Peer Chen <[EMAIL PROTECTED]> == See attached file. == However, I saw this in the patch: + /* determine if physical DMA addr spans 64K boundary. +* Note h/w doesn't support 64-bit, so we unconditionally +* truncate dma_addr_t to u32. +*/ + addr = (u32) sg_dma_address(sg); Does it mean that I can't upgrade my machine to 4 GB or more without losing NCQ or risking data corruption? Can the code be made IOMMU-aware? That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. -- Robert Hancock Saskatoon, SK, Canada To email, remove "nospam" from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Peer Chen wrote:
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller.
This patch base on sata_nv.c file from kernel 2.6.22-rc1
See attachment for the patch.
Signed-off-by: Kuan Luo <[EMAIL PROTECTED]>
Signed-off-by: Peer Chen <[EMAIL PROTECTED]>
Good to finally see this come out. I've pasted the code below (indented)
in order to make some comments:
--- linux-2.6.22-rc1/drivers/ata/sata_nv.c.orig 2007-05-17
14:48:26.0 -0400
+++ linux-2.6.22-rc1/drivers/ata/sata_nv.c 2007-05-17
17:07:28.0 -0400
@@ -46,6 +46,8 @@
#include
#include
#include
+#include
+#include
#include
#define DRV_NAME "sata_nv"
@@ -169,6 +171,36 @@ enum {
NV_ADMA_PORT_REGISTER_MODE = (1 << 0),
NV_ADMA_ATAPI_SETUP_COMPLETE= (1 << 1),
+ /* MCP55 reg offset */
+ NV_CTL_MCP55= 0x400,
+ NV_INT_STATUS_MCP55 = 0x440,
+ NV_INT_ENABLE_MCP55 = 0x444,
+ NV_NCQ_REG_MCP55= 0x448,
+ NV_CH1_SACTIVE_MCP55= 0x0C,
+
+ /* MCP55 */
+ NV_INT_ALL_MCP55= 0x,
+ NV_INT_PORT_SHIFT_MCP55 = 16, /* each port occupies
16 bits */
+ NV_INT_MASK_MCP55 = NV_INT_ALL_MCP55 & 0xfffd,
+
+ /* NCQ ENABLE BITS*/
+ NV_CTL_PRI_SWNCQ= 0x02,
+ NV_CTL_SEC_SWNCQ= 0x04,
+
+ /* MCP55 status bits*/
+ NV_INT_DEV_MCP55= 0x01,
+ NV_INT_PM_MCP55 = 0x02,
+ NV_INT_ADDED_MCP55 = 0x04,
+ NV_INT_REMOVED_MCP55= 0x08,
+
+ NV_INT_BACKOUT_MCP55= 0x10,
+ NV_INT_SDBFIS_MCP55 = 0x20,
+ NV_INT_DHREGFIS_MCP55 = 0x40,
+ NV_INT_DMASETUP_MCP55 = 0x80,
+
+ NV_INT_HOTPLUG_MCP55= (NV_INT_ADDED_MCP55 |
+ NV_INT_REMOVED_MCP55),
+
};
/* ADMA Physical Region Descriptor - one SG segment */
@@ -264,13 +296,118 @@ static void nv_adma_host_stop(struct ata
static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile
*tf);
+static void ncq_error_handler(struct ata_port *ap);
+static void nv_mcp55_thaw(struct ata_port *ap);
+static void nv_mcp55_freeze(struct ata_port *ap);
+static void ncq_host_init(struct ata_host *host);
+static void nv_bmdma_stop(struct ata_port *ap);
+static int nv_std_qc_defer(struct ata_port *ap);
+static int nv_port_start(struct ata_port *ap);
+static void nv_port_stop(struct ata_port *ap);
+static void ncq_clear(struct ata_port *ap);
+static void nv_qc_prep(struct ata_queued_cmd *qc);
+static void nv_fill_sg(struct ata_queued_cmd *qc);
+static void ncq_sactive_start (struct ata_queued_cmd *qc);
+static u32 ncq_sactive_value (struct ata_port *ap);
+static unsigned int nv_qc_issue_prot(struct ata_queued_cmd *qc);
+static u32 ncq_tag_value(struct ata_port *ap);
+static int nv_ncqintr_sdbfis(struct ata_port *ap);
+static int nv_ncqintr_dmasetupfis(struct ata_port *ap);
+static void ncq_clear_singlefis(struct ata_port *ap, u32 val);
+static u32 ncq_ownfisintr_value (struct ata_port *ap);
+void ncq_hotplug(struct ata_port *ap, u32 fis);
+static irqreturn_t nv_mcp55_interrupt(int irq, void *dev_instance);
+static int ncq_interrupt(struct ata_port *ap, u32 fis);
+static int nv_scsi_queuecmd(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *));
These functions should use "mcp51" or "swncq" or something in the name
instead of "ncq", since the latter implies it may be related to ADMA as
well.
+
+#undef NCQ_DEBUG
+#undef NCQ_VERBOSE_DEBUG
+#ifdef NCQ_DEBUG
+#define NPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt,
__FUNCTION__, ## args)
+#ifdef NCQ_VERBOSE_DEBUG
+#define NVPRINTK(fmt, args...) printk(KERN_ERR "%s: " fmt,
__FUNCTION__, ## args)
+#else
+#define NVPRINTK(fmt, args...) do { } while(0)
+#endif /* NCQ_VERBOSE_DEBUG */
+#else
+#define NPRINTK(fmt, args...) do { } while(0)
+#define NVPRINTK(fmt, args...) do { } while(0)
+#endif
We don't need these private helper macros, just use the ones that libata
defines.
+
+/*cmd_stop
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On May 17 2007 13:17, Andrew Morton wrote:
>> @@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
>> WARN_ON(qc->__sg == NULL);
>> WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
>>
>> -prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
>> +prd = pp->prd + ATA_PRD_TBL_SZ*qc->tag;
>>
>> idx = 0;
>> ata_for_each_sg(sg, qc) {
>
>hm, no.
>
>ugh, I dunno what's going on here and I think I'd prefer not to. Can't
>we come up with some typesafe way of doing this without casting?
>
>Meanwhile, I'll switch the cast from u64 to long.
Maybe this?
prd = (void *)pp->prd + ATA_PRD_TBL_SZ * qc->tag
Or...
prd = pp->prd + (ATA_PRD_TBL_SZ * qc->tag) / sizeof(typeof(pp->prd));
Jan
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, thanks for publishing this. Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA controller. This patch base on sata_nv.c file from kernel 2.6.22-rc1 See attachment for the patch. Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> Signed-off-by: Peer Chen <[EMAIL PROTECTED]> == See attached file. == However, I saw this in the patch: + /* determine if physical DMA addr spans 64K boundary. +* Note h/w doesn't support 64-bit, so we unconditionally +* truncate dma_addr_t to u32. +*/ + addr = (u32) sg_dma_address(sg); Does it mean that I can't upgrade my machine to 4 GB or more without losing NCQ or risking data corruption? Can the code be made IOMMU-aware? Best regards, Zoltán Böszörményi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Thu, 17 May 2007 13:12:35 -0700
Andrew Morton <[EMAIL PROTECTED]> wrote:
> The patch generates warnings:
>
> drivers/ata/sata_nv.c:2118: warning: cast from pointer to integer of
> different size
> drivers/ata/sata_nv.c:2118: warning: cast to pointer from integer of
> different size
>
> due to some quite suspicious-looknig code:
>
> prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
>
> we have
>
> struct ata_prd {
> u32 addr;
> u32 flags_len;
> };
>
> and the code is casting a pointer to this into a pointer to u64, then
> adding stuff to it, then casting it back to the correct type.
>
> Can't we simply do this?
>
> ---
> a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix
> +++ a/drivers/ata/sata_nv.c
> @@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
> WARN_ON(qc->__sg == NULL);
> WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
>
> - prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
> + prd = pp->prd + ATA_PRD_TBL_SZ*qc->tag;
>
> idx = 0;
> ata_for_each_sg(sg, qc) {
hm, no.
ugh, I dunno what's going on here and I think I'd prefer not to. Can't
we come up with some typesafe way of doing this without casting?
Meanwhile, I'll switch the cast from u64 to long.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Thu, 17 May 2007 18:15:45 +0800
"Peer Chen" <[EMAIL PROTECTED]> wrote:
> Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
> controller.
>
> This patch base on sata_nv.c file from kernel 2.6.22-rc1
>
> See attachment for the patch.
>
> Signed-off-by: Kuan Luo <[EMAIL PROTECTED]>
> Signed-off-by: Peer Chen <[EMAIL PROTECTED]>
Please don't send patches as application/octet-stream attachments. Inlined
in the body is preferred, or at least text/plain attachments.
The patch generates warnings:
drivers/ata/sata_nv.c:2118: warning: cast from pointer to integer of different
size
drivers/ata/sata_nv.c:2118: warning: cast to pointer from integer of different
size
due to some quite suspicious-looknig code:
prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
we have
struct ata_prd {
u32 addr;
u32 flags_len;
};
and the code is casting a pointer to this into a pointer to u64, then
adding stuff to it, then casting it back to the correct type.
Can't we simply do this?
---
a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix
+++ a/drivers/ata/sata_nv.c
@@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
WARN_ON(qc->__sg == NULL);
WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
- prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
+ prd = pp->prd + ATA_PRD_TBL_SZ*qc->tag;
idx = 0;
ata_for_each_sg(sg, qc) {
_
Also, please please please do not do this:
static void nv_fill_sg(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
unsigned int idx;
struct nv_port_priv *pp = ap->private_data;
struct ata_prd *prd;
WARN_ON(qc->__sg == NULL);
WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
the random meaningless blank lines between the definitions of the local
variables makes it quite hard to read the code. I actually had to do a
text search to find the definition of "pp" because that trick hid it so
well.
Your patch added tremendous amounts of new trailing whitespace.
One function was indented an extra tabstop.
Some whitespace was broken.
diff -puN
drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix-tidy
drivers/ata/sata_nv.c
---
a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix-tidy
+++ a/drivers/ata/sata_nv.c
@@ -2107,9 +2107,7 @@ static void nv_fill_sg(struct ata_queued
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
unsigned int idx;
-
struct nv_port_priv *pp = ap->private_data;
-
struct ata_prd *prd;
WARN_ON(qc->__sg == NULL);
@@ -2194,11 +2192,10 @@ static unsigned int nv_qc_issue_prot(str
{
struct ata_port *ap = qc->ap;
struct nv_port_priv *pp = ap->private_data;
-
struct ata_taskfile *tf = &(qc->tf);
struct ata_taskfile *ttf, rtf;
-
u32 fis, stat0, stat1;
+
ttf =
if (qc->tf.protocol != ATA_PROT_NCQ)
@@ -2338,32 +2335,29 @@ static void fis_dump(struct ata_port *ap
void ncq_hotplug(struct ata_port *ap, u32 fis)
{
+ u32 serror;
+ struct ata_eh_info *ehi = >eh_info;
- u32 serror;
- struct ata_eh_info *ehi = >eh_info;
+ ata_ehi_clear_desc(ehi);
- ata_ehi_clear_desc(ehi);
-
- /* AHCI needs SError cleared; otherwise, it might lock up */
- sata_scr_read(ap, SCR_ERROR, );
- sata_scr_write(ap, SCR_ERROR, serror);
+ /* AHCI needs SError cleared; otherwise, it might lock up */
+ sata_scr_read(ap, SCR_ERROR, );
+ sata_scr_write(ap, SCR_ERROR, serror);
- /* analyze @irq_stat */
- ata_ehi_push_desc(ehi, "fis_stat 0x%08x", fis);
+ /* analyze @irq_stat */
+ ata_ehi_push_desc(ehi, "fis_stat 0x%08x", fis);
- ata_ehi_hotplugged(ehi);
+ ata_ehi_hotplugged(ehi);
- /* okay, let's hand over to EH */
- ehi->serror |= serror;
+ /* okay, let's hand over to EH */
+ ehi->serror |= serror;
- ata_port_freeze(ap);
+ ata_port_freeze(ap);
}
-
static int ncq_interrupt(struct ata_port *ap, u32 fis)
{
struct nv_port_priv *pp = ap->private_data;
-
u32 rc = 0;
u32 tag;
u8 ata_stat;
@@ -2371,7 +2365,6 @@ static int ncq_interrupt(struct ata_port
if (!fis)
return 0;
-
ata_stat = ap->ops->check_status(ap);
ap->ops->irq_clear(ap); /* clear bm irq */
@@ -2398,7 +2391,7 @@ static int ncq_interrupt(struct ata_port
set_back_byte(pp, pp->current_tag);
NPRINTK("BACK OUT FIS:%x \n", fis);
rc = 1;
-} /* first handle back out */
+ } /* first handle
[PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA controller. This patch base on sata_nv.c file from kernel 2.6.22-rc1 See attachment for the patch. Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> Signed-off-by: Peer Chen <[EMAIL PROTECTED]> == See attached file. == --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- patch-sata_nv-swncq Description: patch-sata_nv-swncq
[PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA controller. This patch base on sata_nv.c file from kernel 2.6.22-rc1 See attachment for the patch. Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] == See attached file. == --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- patch-sata_nv-swncq Description: patch-sata_nv-swncq
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Thu, 17 May 2007 18:15:45 +0800
Peer Chen [EMAIL PROTECTED] wrote:
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller.
This patch base on sata_nv.c file from kernel 2.6.22-rc1
See attachment for the patch.
Signed-off-by: Kuan Luo [EMAIL PROTECTED]
Signed-off-by: Peer Chen [EMAIL PROTECTED]
Please don't send patches as application/octet-stream attachments. Inlined
in the body is preferred, or at least text/plain attachments.
The patch generates warnings:
drivers/ata/sata_nv.c:2118: warning: cast from pointer to integer of different
size
drivers/ata/sata_nv.c:2118: warning: cast to pointer from integer of different
size
due to some quite suspicious-looknig code:
prd = (struct ata_prd*)((u64)pp-prd + ATA_PRD_TBL_SZ*qc-tag);
we have
struct ata_prd {
u32 addr;
u32 flags_len;
};
and the code is casting a pointer to this into a pointer to u64, then
adding stuff to it, then casting it back to the correct type.
Can't we simply do this?
---
a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix
+++ a/drivers/ata/sata_nv.c
@@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
WARN_ON(qc-__sg == NULL);
WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
- prd = (struct ata_prd*)((u64)pp-prd + ATA_PRD_TBL_SZ*qc-tag);
+ prd = pp-prd + ATA_PRD_TBL_SZ*qc-tag;
idx = 0;
ata_for_each_sg(sg, qc) {
_
Also, please please please do not do this:
static void nv_fill_sg(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc-ap;
struct scatterlist *sg;
unsigned int idx;
struct nv_port_priv *pp = ap-private_data;
struct ata_prd *prd;
WARN_ON(qc-__sg == NULL);
WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
the random meaningless blank lines between the definitions of the local
variables makes it quite hard to read the code. I actually had to do a
text search to find the definition of pp because that trick hid it so
well.
Your patch added tremendous amounts of new trailing whitespace.
One function was indented an extra tabstop.
Some whitespace was broken.
diff -puN
drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix-tidy
drivers/ata/sata_nv.c
---
a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix-tidy
+++ a/drivers/ata/sata_nv.c
@@ -2107,9 +2107,7 @@ static void nv_fill_sg(struct ata_queued
struct ata_port *ap = qc-ap;
struct scatterlist *sg;
unsigned int idx;
-
struct nv_port_priv *pp = ap-private_data;
-
struct ata_prd *prd;
WARN_ON(qc-__sg == NULL);
@@ -2194,11 +2192,10 @@ static unsigned int nv_qc_issue_prot(str
{
struct ata_port *ap = qc-ap;
struct nv_port_priv *pp = ap-private_data;
-
struct ata_taskfile *tf = (qc-tf);
struct ata_taskfile *ttf, rtf;
-
u32 fis, stat0, stat1;
+
ttf = rtf;
if (qc-tf.protocol != ATA_PROT_NCQ)
@@ -2338,32 +2335,29 @@ static void fis_dump(struct ata_port *ap
void ncq_hotplug(struct ata_port *ap, u32 fis)
{
+ u32 serror;
+ struct ata_eh_info *ehi = ap-eh_info;
- u32 serror;
- struct ata_eh_info *ehi = ap-eh_info;
+ ata_ehi_clear_desc(ehi);
- ata_ehi_clear_desc(ehi);
-
- /* AHCI needs SError cleared; otherwise, it might lock up */
- sata_scr_read(ap, SCR_ERROR, serror);
- sata_scr_write(ap, SCR_ERROR, serror);
+ /* AHCI needs SError cleared; otherwise, it might lock up */
+ sata_scr_read(ap, SCR_ERROR, serror);
+ sata_scr_write(ap, SCR_ERROR, serror);
- /* analyze @irq_stat */
- ata_ehi_push_desc(ehi, fis_stat 0x%08x, fis);
+ /* analyze @irq_stat */
+ ata_ehi_push_desc(ehi, fis_stat 0x%08x, fis);
- ata_ehi_hotplugged(ehi);
+ ata_ehi_hotplugged(ehi);
- /* okay, let's hand over to EH */
- ehi-serror |= serror;
+ /* okay, let's hand over to EH */
+ ehi-serror |= serror;
- ata_port_freeze(ap);
+ ata_port_freeze(ap);
}
-
static int ncq_interrupt(struct ata_port *ap, u32 fis)
{
struct nv_port_priv *pp = ap-private_data;
-
u32 rc = 0;
u32 tag;
u8 ata_stat;
@@ -2371,7 +2365,6 @@ static int ncq_interrupt(struct ata_port
if (!fis)
return 0;
-
ata_stat = ap-ops-check_status(ap);
ap-ops-irq_clear(ap); /* clear bm irq */
@@ -2398,7 +2391,7 @@ static int ncq_interrupt(struct ata_port
set_back_byte(pp, pp-current_tag);
NPRINTK(BACK OUT FIS:%x \n, fis);
rc = 1;
-} /* first handle back out */
+ } /* first handle back out */
if (fis
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Thu, 17 May 2007 13:12:35 -0700
Andrew Morton [EMAIL PROTECTED] wrote:
The patch generates warnings:
drivers/ata/sata_nv.c:2118: warning: cast from pointer to integer of
different size
drivers/ata/sata_nv.c:2118: warning: cast to pointer from integer of
different size
due to some quite suspicious-looknig code:
prd = (struct ata_prd*)((u64)pp-prd + ATA_PRD_TBL_SZ*qc-tag);
we have
struct ata_prd {
u32 addr;
u32 flags_len;
};
and the code is casting a pointer to this into a pointer to u64, then
adding stuff to it, then casting it back to the correct type.
Can't we simply do this?
---
a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix
+++ a/drivers/ata/sata_nv.c
@@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
WARN_ON(qc-__sg == NULL);
WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
- prd = (struct ata_prd*)((u64)pp-prd + ATA_PRD_TBL_SZ*qc-tag);
+ prd = pp-prd + ATA_PRD_TBL_SZ*qc-tag;
idx = 0;
ata_for_each_sg(sg, qc) {
hm, no.
ugh, I dunno what's going on here and I think I'd prefer not to. Can't
we come up with some typesafe way of doing this without casting?
Meanwhile, I'll switch the cast from u64 to long.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, thanks for publishing this. Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA controller. This patch base on sata_nv.c file from kernel 2.6.22-rc1 See attachment for the patch. Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] == See attached file. == However, I saw this in the patch: + /* determine if physical DMA addr spans 64K boundary. +* Note h/w doesn't support 64-bit, so we unconditionally +* truncate dma_addr_t to u32. +*/ + addr = (u32) sg_dma_address(sg); Does it mean that I can't upgrade my machine to 4 GB or more without losing NCQ or risking data corruption? Can the code be made IOMMU-aware? Best regards, Zoltán Böszörményi - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On May 17 2007 13:17, Andrew Morton wrote:
@@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
WARN_ON(qc-__sg == NULL);
WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
-prd = (struct ata_prd*)((u64)pp-prd + ATA_PRD_TBL_SZ*qc-tag);
+prd = pp-prd + ATA_PRD_TBL_SZ*qc-tag;
idx = 0;
ata_for_each_sg(sg, qc) {
hm, no.
ugh, I dunno what's going on here and I think I'd prefer not to. Can't
we come up with some typesafe way of doing this without casting?
Meanwhile, I'll switch the cast from u64 to long.
Maybe this?
prd = (void *)pp-prd + ATA_PRD_TBL_SZ * qc-tag
Or...
prd = pp-prd + (ATA_PRD_TBL_SZ * qc-tag) / sizeof(typeof(pp-prd));
Jan
--
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Peer Chen wrote:
Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller.
This patch base on sata_nv.c file from kernel 2.6.22-rc1
See attachment for the patch.
Signed-off-by: Kuan Luo [EMAIL PROTECTED]
Signed-off-by: Peer Chen [EMAIL PROTECTED]
Good to finally see this come out. I've pasted the code below (indented)
in order to make some comments:
--- linux-2.6.22-rc1/drivers/ata/sata_nv.c.orig 2007-05-17
14:48:26.0 -0400
+++ linux-2.6.22-rc1/drivers/ata/sata_nv.c 2007-05-17
17:07:28.0 -0400
@@ -46,6 +46,8 @@
#include linux/device.h
#include scsi/scsi_host.h
#include scsi/scsi_device.h
+#include scsi/scsi.h
+#include scsi/scsi_cmnd.h
#include linux/libata.h
#define DRV_NAME sata_nv
@@ -169,6 +171,36 @@ enum {
NV_ADMA_PORT_REGISTER_MODE = (1 0),
NV_ADMA_ATAPI_SETUP_COMPLETE= (1 1),
+ /* MCP55 reg offset */
+ NV_CTL_MCP55= 0x400,
+ NV_INT_STATUS_MCP55 = 0x440,
+ NV_INT_ENABLE_MCP55 = 0x444,
+ NV_NCQ_REG_MCP55= 0x448,
+ NV_CH1_SACTIVE_MCP55= 0x0C,
+
+ /* MCP55 */
+ NV_INT_ALL_MCP55= 0x,
+ NV_INT_PORT_SHIFT_MCP55 = 16, /* each port occupies
16 bits */
+ NV_INT_MASK_MCP55 = NV_INT_ALL_MCP55 0xfffd,
+
+ /* NCQ ENABLE BITS*/
+ NV_CTL_PRI_SWNCQ= 0x02,
+ NV_CTL_SEC_SWNCQ= 0x04,
+
+ /* MCP55 status bits*/
+ NV_INT_DEV_MCP55= 0x01,
+ NV_INT_PM_MCP55 = 0x02,
+ NV_INT_ADDED_MCP55 = 0x04,
+ NV_INT_REMOVED_MCP55= 0x08,
+
+ NV_INT_BACKOUT_MCP55= 0x10,
+ NV_INT_SDBFIS_MCP55 = 0x20,
+ NV_INT_DHREGFIS_MCP55 = 0x40,
+ NV_INT_DMASETUP_MCP55 = 0x80,
+
+ NV_INT_HOTPLUG_MCP55= (NV_INT_ADDED_MCP55 |
+ NV_INT_REMOVED_MCP55),
+
};
/* ADMA Physical Region Descriptor - one SG segment */
@@ -264,13 +296,118 @@ static void nv_adma_host_stop(struct ata
static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile
*tf);
+static void ncq_error_handler(struct ata_port *ap);
+static void nv_mcp55_thaw(struct ata_port *ap);
+static void nv_mcp55_freeze(struct ata_port *ap);
+static void ncq_host_init(struct ata_host *host);
+static void nv_bmdma_stop(struct ata_port *ap);
+static int nv_std_qc_defer(struct ata_port *ap);
+static int nv_port_start(struct ata_port *ap);
+static void nv_port_stop(struct ata_port *ap);
+static void ncq_clear(struct ata_port *ap);
+static void nv_qc_prep(struct ata_queued_cmd *qc);
+static void nv_fill_sg(struct ata_queued_cmd *qc);
+static void ncq_sactive_start (struct ata_queued_cmd *qc);
+static u32 ncq_sactive_value (struct ata_port *ap);
+static unsigned int nv_qc_issue_prot(struct ata_queued_cmd *qc);
+static u32 ncq_tag_value(struct ata_port *ap);
+static int nv_ncqintr_sdbfis(struct ata_port *ap);
+static int nv_ncqintr_dmasetupfis(struct ata_port *ap);
+static void ncq_clear_singlefis(struct ata_port *ap, u32 val);
+static u32 ncq_ownfisintr_value (struct ata_port *ap);
+void ncq_hotplug(struct ata_port *ap, u32 fis);
+static irqreturn_t nv_mcp55_interrupt(int irq, void *dev_instance);
+static int ncq_interrupt(struct ata_port *ap, u32 fis);
+static int nv_scsi_queuecmd(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *));
These functions should use mcp51 or swncq or something in the name
instead of ncq, since the latter implies it may be related to ADMA as
well.
+
+#undef NCQ_DEBUG
+#undef NCQ_VERBOSE_DEBUG
+#ifdef NCQ_DEBUG
+#define NPRINTK(fmt, args...) printk(KERN_ERR %s: fmt,
__FUNCTION__, ## args)
+#ifdef NCQ_VERBOSE_DEBUG
+#define NVPRINTK(fmt, args...) printk(KERN_ERR %s: fmt,
__FUNCTION__, ## args)
+#else
+#define NVPRINTK(fmt, args...) do { } while(0)
+#endif /* NCQ_VERBOSE_DEBUG */
+#else
+#define NPRINTK(fmt, args...) do { } while(0)
+#define NVPRINTK(fmt, args...) do { } while(0)
+#endif
We don't need these private helper macros, just use
Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Zoltan Boszormenyi wrote: Hi, thanks for publishing this. Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA controller. This patch base on sata_nv.c file from kernel 2.6.22-rc1 See attachment for the patch. Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] == See attached file. == However, I saw this in the patch: + /* determine if physical DMA addr spans 64K boundary. +* Note h/w doesn't support 64-bit, so we unconditionally +* truncate dma_addr_t to u32. +*/ + addr = (u32) sg_dma_address(sg); Does it mean that I can't upgrade my machine to 4 GB or more without losing NCQ or risking data corruption? Can the code be made IOMMU-aware? That shouldn't be a problem, libata default DMA mask is 32 bits (which isn't overridden with this controller) and so the block layer will bounce any data being read/written above that point with IOMMU or swiotlb. The comment is a bit unnecessarily scary. -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
