nForce4 ADMA with NCQ: It's aliiiive..

I've been working on the patch for sata_nv ADMA support for nForce4 that

make the default 1). I only enabled ADMA on those chipsets and not MCP51, MCP55 or MCP61 since that was all that the original NVIDIA version did. I assume there was a reason for this, though maybe not.

Am Samstag 07 Oktober 2006 10:08 schrieb Robert Hancock:

Quoted 1 line

I've been working on the patch for sata_nv ADMA support for nForce4 that

Nice!

Quoted 3 lines

make the default 1). I only enabled ADMA on those chipsets and not MCP51, MCP55 or MCP61 since that was all that the original NVIDIA version did. I assume there was a reason for this, though maybe not.

Unfortunately it doesn't work for me on MCP51 if I change GENERIC to ADMA. So I wonder whether MCP51 has ADMA mode or what needs to be done to get NCQ working. :-(

Quoted 1 line

Unfortunately it doesn't work for me on MCP51 if I change

GENERIC to

Quoted 1 line

ADMA. So I wonder whether MCP51 has ADMA mode or what needs

to be done

Quoted 1 line

to get NCQ working. :-(

What happened when you tried it? It would be useful if you could change the #undef in these lines:

53 #undef ATA_DEBUG /* debugging output */
54 #undef ATA_VERBOSE_DEBUG /* yet more debugging output */

in include/linux/libata.h to #define and rebuild and try it then, that will spew out a bunch more output and I can see if any reasonable looking values are showing up at all. I was capturing this output the crude way, booting with vga=6 to get a smaller font and taking a picture of the screen :-) Also, maybe post the lspci -v output from the SATA controller..

If that doesn't provide any insight, maybe the docs Jeff has provide the answer for whether or not the MCP5x/MCP61 controllers have the same interface as the CK804/MCP04..

Quoted 10 lines

Quoted 1 line

Unfortunately it doesn't work for me on MCP51 if I change

GENERIC to

Quoted 1 line

ADMA. So I wonder whether MCP51 has ADMA mode or what needs

to be done

Quoted 1 line

to get NCQ working. :-(

Quoted 3 lines

If that doesn't provide any insight, maybe the docs Jeff has provide the answer for whether or not the MCP5x/MCP61 controllers have the same interface as the CK804/MCP04..

No, only CK804 and MCP04 support ADMA. We'll be publishing some patches for NCQ support for MCP55/MCP61 soon.

No, only CK804 and MCP04 support ADMA. We'll be publishing some patches for NCQ support for MCP55/MCP61 soon.

Am Dienstag 10 Oktober 2006 08:44 schrieb Allen Martin:

Quoted 2 lines

No, only CK804 and MCP04 support ADMA. We'll be publishing some patches for NCQ support for MCP55/MCP61 soon.

Just wondering whether I missed them? It is now 1,5 month later...

I've been working on the patch for sata_nv ADMA support for nForce4 that Jeff Garzik has in a git branch. I've gotten it into a state where the ADMA and NCQ features appear to be working with no obvious problems. I've attached a patch against 2.6.18-mm2.

The code was mostly in a working state for the non-NCQ case but there were a number of heinous bugs that prevented NCQ from working, like in the sg list to APRD conversion code and in the interrupt handler.

This is still in quite an experimental state. It has survived system boots into Fedora Core 5 and Bonnie++ benchmark runs without blowing up, but there could still be bugs that could corrupt data, etc. so test with caution.

There is a module parameter adma_enabled which has to be set to 1 to enable ADMA on CK804/MCP04 chipsets (either that or hack the code to make the default 1). I only enabled ADMA on those chipsets and not MCP51, MCP55 or MCP61 since that was all that the original NVIDIA version did. I assume there was a reason for this, though maybe not. Someone with one of these chipsets should probably try it out (replacing the GENERIC type with CK804 in the PCI device table may be all it takes).

A few outstanding issues:

-Error handling likely needs work. EH works well enough to get past
drive detection but that's likely about all. When I ran into errors while debugging, it usually locked up the machine when trying to do a soft reset.

-Error handling is also noisy at the moment (it dumps a bunch of controller state information).

-Jeff will probably cringe at how I implemented the
bmdma_stop/start/status/setup functions. This kludge of toggling ATA_FLAG_MMIO off for the call into libata was needed since this controller is almost what libata calls ATA_FLAG_MMIO, but not quite (the ATA taskfile registers are MMIO but the BMDMA registers are PIO). This is also why I needed the patch to libata-sff.c to use the adapter's bmdma_status function rather than hardcoded ata_bmdma_status.

Quoted 2 lines

-Jeff will probably cringe at how I implemented the
bmdma_stop/start/status/setup functions. This kludge of toggling ATA_FLAG_MMIO off for the call into libata was needed since this controller is almost what libata calls ATA_FLAG_MMIO, but not quite (the ATA taskfile registers are MMIO but the BMDMA registers are PIO). This is also why I needed the patch to libata-sff.c to use the adapter's bmdma_status function rather than hardcoded ata_bmdma_status.

*shrug* I don't cringe if that's the most expedient way to do something.

But I really don't think that is necessary. I will take a look at docs and see how things match up, when I am much more awake. Most likely you need to be using another set of registers, and be all MMIO, all the time.

Jeff

Quoted 5 lines

-Jeff will probably cringe at how I implemented the bmdma_stop/start/status/setup functions. This kludge of toggling ATA_FLAG_MMIO off for the call into libata was needed since this controller is almost what libata calls ATA_FLAG_MMIO, but not quite (the ATA taskfile registers are MMIO but the BMDMA

registers are PIO).

Quoted 2 lines

This is also why I needed the patch to libata-sff.c to use the adapter's bmdma_status function rather than hardcoded

ata_bmdma_status.

*shrug* I don't cringe if that's the most expedient way to do something.

But I really don't think that is necessary. I will take a look at docs and see how things match up, when I am much more awake. Most likely you need to be using another set of registers, and be all MMIO, all the time.

Quoted 1 line

But I really don't think that is necessary. I will take a look at docs and see how things match up, when I am much more awake. Most likely you need to be using another set of registers, and be all MMIO, all the time.

You shouldn't be touching BM registers when ADMA is enabled, it can cause bad things to happen.

You should be using BM registers when doing ATAPI protocol though, as it doesn't work through ADMA. So I wouldn't say you should be using MMIO all the time.

-Allen

Quoted 5 lines · Allen Martin

Quoted 4 lines

But I really don't think that is necessary. I will take a look at docs and see how things match up, when I am much more awake. Most likely you need to be using another set of registers, and be all MMIO, all the time.

Quoted 8 lines

You shouldn't be touching BM registers when ADMA is enabled, it can cause bad things to happen.

You should be using BM registers when doing ATAPI protocol though, as it doesn't work through ADMA. So I wouldn't say you should be using MMIO all the time.

-Allen

OK, I've updated the code to take this into account, an updated patch is attached. However, this does raise an issue. If we have to fall back to legacy mode to do ATAPI DMA, this means that we can't do 64-bit DMA for such transfers. Since by the time the driver gets a request the SGs have already been created based on the set DMA mask, the only way I can see to handle this is to either allow ATAPI DMA or 64-bit DMA, not both. I've chosen to default to 64-bit DMA in this version, but there is a module parameter which allows overriding this if you care more about using ATAPI devices than efficiency with over 4GB of RAM. I'm open to suggestions on a better way to handle this..

Quoted 9 lines · Robert Hancock

Quoted 8 lines · Allen Martin

Quoted 1 line

But I really don't think that is necessary. I will take a look at docs and see how things match up, when I am much more awake. Most likely you need to be using another set of registers, and be all MMIO, all the time.

You shouldn't be touching BM registers when ADMA is enabled, it can cause bad things to happen.

You should be using BM registers when doing ATAPI protocol though, as it doesn't work through ADMA. So I wouldn't say you should be using MMIO all the time.

Quoted 2 lines

Quoted 1 line

-Allen

OK, I've updated the code to take this into account, an updated patch is attached. However, this does raise an issue. If we have to fall back to legacy mode to do ATAPI DMA, this means that we can't do 64-bit DMA for such transfers. Since by the time the driver gets a request the SGs have already been created based on the set DMA mask, the only way I can see to handle this is to either allow ATAPI DMA or 64-bit DMA, not both. I've chosen to default to 64-bit DMA in this version, but there is a module parameter which allows overriding this if you care more about using ATAPI devices than efficiency with over 4GB of RAM. I'm open to suggestions on a better way to handle this..

Should be easily fixable - in general, set 64-bit dma mask. Then when you detect an atapi device, lower the dma mask settings to 32-bit dma for that device only. So the pci device in question gets a full 64-bit dma mask, the attached scsi devices can have lower masks if necessary. I'd suggest doing this off slave config.

Quoted 25 lines · Jens Axboe

Quoted 13 lines · Robert Hancock

Quoted 4 lines · Allen Martin

Quoted 4 lines

But I really don't think that is necessary. I will take a look at docs and see how things match up, when I am much more awake. Most likely you need to be using another set of registers, and be all MMIO, all the time.

Quoted 8 lines

You shouldn't be touching BM registers when ADMA is enabled, it can cause bad things to happen.

You should be using BM registers when doing ATAPI protocol though, as it doesn't work through ADMA. So I wouldn't say you should be using MMIO all the time.

-Allen

Quoted 10 lines

OK, I've updated the code to take this into account, an updated patch is attached. However, this does raise an issue. If we have to fall back to legacy mode to do ATAPI DMA, this means that we can't do 64-bit DMA for such transfers. Since by the time the driver gets a request the SGs have already been created based on the set DMA mask, the only way I can see to handle this is to either allow ATAPI DMA or 64-bit DMA, not both. I've chosen to default to 64-bit DMA in this version, but there is a module parameter which allows overriding this if you care more about using ATAPI devices than efficiency with over 4GB of RAM. I'm open to suggestions on a better way to handle this..

Quoted 6 lines

Should be easily fixable - in general, set 64-bit dma mask. Then when you detect an atapi device, lower the dma mask settings to 32-bit dma for that device only. So the pci device in question gets a full 64-bit dma mask, the attached scsi devices can have lower masks if necessary. I'd suggest doing this off slave config.

I think that should be feasible.. However, one problem is that slave_config only has access to the struct scsi_device and the ata_scsi_find_dev function to turn that into a struct ata_device isn't exported, which it would need to be in order to do anything useful inside the driver for slave_config. We could export it, or I suppose the other place we could do this handling would be postreset, as at that point we should know what kind of device is attached.. any comments?

Also, how is the driver supposed to be setting the DMA mask for the SCSI device? I suppose blk_queue_bounce_limit would work, but it seems a bit odd to use block layer calls at the libata driver level.

I also noticed that I'm still using the default 64KB libata dma_boundary value, this should be 4GB for ADMA mode (but fixed up back to the default if an ATAPI device is connected, same as with the DMA mask).

Quoted 1 line

I think that should be feasible.. However, one problem is that slave_config only has access to the struct scsi_device and the ata_scsi_find_dev function to turn that into a struct ata_device isn't exported, which it would need to be in order to do anything useful inside the driver for slave_config. We could export it, or I suppose the other place we could do this handling would be postreset, as at that point we should know what kind of device is attached.. any comments?

What else do you need? From the scsi device, call the blk_queue_bounce_limit() on the queue tored in there. That shold be it.

Quoted 1 line

Also, how is the driver supposed to be setting the DMA mask for the SCSI device? I suppose blk_queue_bounce_limit would work, but it seems a bit odd to use block layer calls at the libata driver level.

That is the right thing to do.

Quoted 1 line

I also noticed that I'm still using the default 64KB libata dma_boundary value, this should be 4GB for ADMA mode (but fixed up back to the default if an ATAPI device is connected, same as with the DMA mask).

You can set that from the same location as the bounce limit.

drivers/ata/sata_nv.c +8 -0

+/* ADMA Physical Region Descriptor - one SG segment */
+struct nv_adma_prd {
+	__le64			addr;
+	__le32			len;
+	u8			flags;
+	u8			packet_len;
+	__le16			reserved;
+};

drivers/ata/sata_nv.c +8 -0

+/* ADMA Command Parameter Block
+   The first 5 SG segments are stored inside the Command Parameter Block itself.
+   If there are more than 5 segments the remainder are stored in a separate
+   memory area indicated by next_aprd. */
+struct nv_adma_cpb {
+	u8			resp_flags;    //0
+	u8			reserved1;     //1
+	u8			ctl_flags;     //2

Quoted 9 lines · Robert Hancock

drivers/ata/sata_nv.c +8 -0

+/* ADMA Physical Region Descriptor - one SG segment */
+struct nv_adma_prd {
+    __le64            addr;
+    __le32            len;
+    u8            flags;
+    u8            packet_len;
+    __le16            reserved;
+};

..

Quoted 13 lines

(quoted diff) +2 -0

+/* ADMA Command Parameter Block
+   The first 5 SG segments are stored inside the Command Parameter 

Block itself.
+ If there are more than 5 segments the remainder are stored in a
separate

drivers/ata/sata_nv.c +7 -0

+   memory area indicated by next_aprd. */
+struct nv_adma_cpb {
+    u8            resp_flags;    //0
+    u8            reserved1;     //1
+    u8            ctl_flags;     //2
+    // len is length of taskfile in 64 bit words
+     u8            len;           //3 +    u8            

tag; //4

drivers/ata/sata_nv.c enum { · line 203

Quoted 7 lines

drivers/ata/sata_nv.c +7 -0

+    u8            next_cpb_idx;  //5
+    __le16            reserved2;     //6-7
+    __le16            tf[12];        //8-31
+    struct nv_adma_prd    aprd[5];       //32-111
+    __le64            next_aprd;     //112-119
+    __le64            reserved3;     //120-127
+};

Are those CPB / PRD structs endian-safe when using a big-endian CPU?

Cheers

Quoted 8 lines · Mark Lord

Quoted 7 lines · Robert Hancock

drivers/ata/sata_nv.c +6 -0

+/* ADMA Physical Region Descriptor - one SG segment */
+struct nv_adma_prd {
+    __le64            addr;
+    __le32            len;
+    u8            flags;
+    u8            packet_len;

Quoted 8 lines

Quoted 8 lines

drivers/ata/sata_nv.c +6 -0

+struct nv_adma_cpb {
+    u8            resp_flags;    //0
+    u8            reserved1;     //1
+    u8            ctl_flags;     //2
+    // len is length of taskfile in 64 bit words
+     u8            len;           //3 +    u8            

tag; //4
+ u8 next_cpb_idx; //5

..

Quoted 3 lines

Are those CPB / PRD structs endian-safe when using a big-endian CPU?

Cheers

They should be, I believe cpu_to_leXX is used whenever the multi-byte elements are being written.

Mark> I was thinking more about the non wordsized fields, such as Mark> the various u8 bytes that gcc will lay out differently Mark> depending upon endianess.

I don't know of any gcc version that changes the order of struct fields. You might be thinking of bitfields, which are laid out in an endian-dependent way.

I also noticed that I'm still using the default 64KB libata dma_boundary value, this should be 4GB for ADMA mode (but fixed up back to the default if an ATAPI device is connected, same as with the DMA mask).

Quoted 2 lines · Robert Hancock

I also noticed that I'm still using the default 64KB libata dma_boundary value, this should be 4GB for ADMA mode (but fixed up back to the default if an ATAPI device is connected, same as with the DMA mask).

Be careful of that. The original PDC hardware for ADMA still had the "don't cross a 64KB boundary" requirement.

Cheers

Quoted 1 line · Mark Lord

Robert Hancock wrote:

Quoted 2 lines

Be careful of that. The original PDC hardware for ADMA still had the "don't cross a 64KB boundary" requirement.

That is part of the ADMA spec - but in that case, how come the pdc_adma.c driver sets the dma_boundary in the SCSI host template to 4GB? That seems wrong.