SATAPI timing

SATAPI timing

[gp]

Jeff

I think I may have found an issue in the ATAPI code in the function 
atapi_packet_task.

 From my testing with the sata_via driver, I have been getting a timeout 
when sending the first inquiry command.


Here is the section of code of interest:


	/* make sure DRQ is set */
	status = ata_chk_status(ap);
	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
		goto err_out;

	/* send SCSI cdb */
	DPRINTK("send cdb\n");
	assert(ap->cdb_len >= 12);
	ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);

** Wait is needed here to work correctly
DPRINTK("Wait for a bit\n");

	/* if we are DMA'ing, irq handler takes over from here */
	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
		ap->ops->bmdma_start(qc);	    /* initiate bmdma */


In the test case, the dma transfer is being used. From my perusal of 
the ATA spec in section 9.8, it looks like there needs to be a definite 
state transition before issuing the bmdma_start.

Would you take a look at this and verify my theory?
Thanks for all the hard work on these drivers.

Gary

Re: SATAPI timing

[Jeff Garzik]

gp wrote:
> Jeff
> 
> I think I may have found an issue in the ATAPI code in the function 
> atapi_packet_task.
> 
>  From my testing with the sata_via driver, I have been getting a timeout 
> when sending the first inquiry command.
> 
> 
> Here is the section of code of interest:
> 
> 
>     /* make sure DRQ is set */
>     status = ata_chk_status(ap);
>     if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
>         goto err_out;
> 
>     /* send SCSI cdb */
>     DPRINTK("send cdb\n");
>     assert(ap->cdb_len >= 12);
>     ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
> 
> ** Wait is needed here to work correctly
> DPRINTK("Wait for a bit\n");
> 
>     /* if we are DMA'ing, irq handler takes over from here */
>     if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
>         ap->ops->bmdma_start(qc);        /* initiate bmdma */
> 
> 
> In the test case, the dma transfer is being used. From my perusal of the 
> ATA spec in section 9.8, it looks like there needs to be a definite 
> state transition before issuing the bmdma_start.

Yeah, it looks like there needs to be an intrq-wait state in there...

Wanna submit a patch?  :)

	Jeff

Re: SATAPI timing

[Albert Lee]

> gp wrote:
> > Jeff
> > 
> > I think I may have found an issue in the ATAPI code in the function 
> > atapi_packet_task.
> > 
> >  From my testing with the sata_via driver, I have been getting a timeout 
> > when sending the first inquiry command.
> > 
> > 
> > Here is the section of code of interest:
> > 
> > 
> >     /* make sure DRQ is set */
> >     status = ata_chk_status(ap);
> >     if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ)
> >         goto err_out;
> > 
> >     /* send SCSI cdb */
> >     DPRINTK("send cdb\n");
> >     assert(ap->cdb_len >= 12);
> >     ata_data_xfer(ap, qc->cdb, ap->cdb_len, 1);
> > 
> > ** Wait is needed here to work correctly
> > DPRINTK("Wait for a bit\n");
> > 
> >     /* if we are DMA'ing, irq handler takes over from here */
> >     if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
> >         ap->ops->bmdma_start(qc);        /* initiate bmdma */
> > 
> > 
> > In the test case, the dma transfer is being used. From my perusal of the 
> > ATA spec in section 9.8, it looks like there needs to be a definite 
> > state transition before issuing the bmdma_start.
> 
> Yeah, it looks like there needs to be an intrq-wait state in there...
> 
> Wanna submit a patch?  :)
> 

I'm a little confused about what is the "correct behavior" of ATAPI DMA INTRQ.

1. The ATA-4 flowchart and ATA-6 state diagram seems to be inconsistent.
In the ATA-4 flowchart, no wait for INTRQ is specified.

2. From the ATA-5 errata:
http://www.t13.org/project/e01122r0.pdf

"...Page 265 through 270 of NCITS 340-2000 clause 9.8 was not properly converted from the flow
charts in NCITS 317-1998 to the state diagrams. The state diagrams figures 33 and 34 and
associated text is modified to indicate that the device interrupts only at command completion. "

Notice the "device interrupts only at command completion." statement.
However, the diagrams in the errata are inconsistent with the above statement.

3. The ide-cd code does not wait for INTRQ before starting BM-DMA

        /* Arm the interrupt handler. */
        ide_set_handler(drive, handler, rq->timeout, cdrom_timer_expiry);

        /* ATAPI commands get padded out to 12 bytes minimum */
        cmd_len = COMMAND_SIZE(rq->cmd[0]);
        if (cmd_len < ATAPI_MIN_CDB_BYTES)
            cmd_len = ATAPI_MIN_CDB_BYTES;

        /* Send the command to the device. */
        HWIF(drive)->atapi_output_bytes(drive, rq->cmd, cmd_len);

        /* Start the DMA if need be */
        if (info->dma)
            hwif->dma_start(drive);

        return ide_started;

4. My CD-ROM drive does not generate INTRQ after it received the command packet.
   It only assert DMARQ.

Maybe the state diagrams in ATA-5 and ATA-6 are incorrect?  Any idea?

Albert

Re: SATAPI timing

[Jeff Garzik]

Albert Lee wrote:
> I'm a little confused about what is the "correct behavior" of ATAPI DMA INTRQ.
> 
> 1. The ATA-4 flowchart and ATA-6 state diagram seems to be inconsistent.
> In the ATA-4 flowchart, no wait for INTRQ is specified.
> 
> 2. From the ATA-5 errata:
> http://www.t13.org/project/e01122r0.pdf
> 
> "...Page 265 through 270 of NCITS 340-2000 clause 9.8 was not properly converted from the flow
> charts in NCITS 317-1998 to the state diagrams. The state diagrams figures 33 and 34 and
> associated text is modified to indicate that the device interrupts only at command completion. "
> 
> Notice the "device interrupts only at command completion." statement.
> However, the diagrams in the errata are inconsistent with the above statement.
> 
> 3. The ide-cd code does not wait for INTRQ before starting BM-DMA
> 
>         /* Arm the interrupt handler. */
>         ide_set_handler(drive, handler, rq->timeout, cdrom_timer_expiry);
> 
>         /* ATAPI commands get padded out to 12 bytes minimum */
>         cmd_len = COMMAND_SIZE(rq->cmd[0]);
>         if (cmd_len < ATAPI_MIN_CDB_BYTES)
>             cmd_len = ATAPI_MIN_CDB_BYTES;
> 
>         /* Send the command to the device. */
>         HWIF(drive)->atapi_output_bytes(drive, rq->cmd, cmd_len);
> 
>         /* Start the DMA if need be */
>         if (info->dma)
>             hwif->dma_start(drive);
> 
>         return ide_started;


This is a good point, and refreshes my memory on the subject from when 
the code was written (and updated by Bart).

I'll look into things further...

	Jeff

Re: SATAPI timing

[Jeff Garzik]

Albert Lee wrote:
> I'm a little confused about what is the "correct behavior" of ATAPI DMA INTRQ.
> 
> 1. The ATA-4 flowchart and ATA-6 state diagram seems to be inconsistent.
> In the ATA-4 flowchart, no wait for INTRQ is specified.
> 
> 2. From the ATA-5 errata:
> http://www.t13.org/project/e01122r0.pdf
> 
> "...Page 265 through 270 of NCITS 340-2000 clause 9.8 was not properly converted from the flow
> charts in NCITS 317-1998 to the state diagrams. The state diagrams figures 33 and 34 and
> associated text is modified to indicate that the device interrupts only at command completion. "
> 
> Notice the "device interrupts only at command completion." statement.
> However, the diagrams in the errata are inconsistent with the above statement.
> 
> 3. The ide-cd code does not wait for INTRQ before starting BM-DMA
> 
>         /* Arm the interrupt handler. */
>         ide_set_handler(drive, handler, rq->timeout, cdrom_timer_expiry);
> 
>         /* ATAPI commands get padded out to 12 bytes minimum */
>         cmd_len = COMMAND_SIZE(rq->cmd[0]);
>         if (cmd_len < ATAPI_MIN_CDB_BYTES)
>             cmd_len = ATAPI_MIN_CDB_BYTES;
> 
>         /* Send the command to the device. */
>         HWIF(drive)->atapi_output_bytes(drive, rq->cmd, cmd_len);
> 
>         /* Start the DMA if need be */
>         if (info->dma)
>             hwif->dma_start(drive);
> 
>         return ide_started;
> 
> 4. My CD-ROM drive does not generate INTRQ after it received the command packet.
>    It only assert DMARQ.
> 
> Maybe the state diagrams in ATA-5 and ATA-6 are incorrect?  Any idea?


As I mentioned in an earlier email, research seems to indicate the 
IDENTIFY PACKET DEVICE word 0, bits 6-5 can be used to indicate INTRQ 
will be delivered.  This is consistent with my ATA-4 specification.

Apparently this behavior is obsolete, but we still need to support it.

Thus, libata has a few ATAPI deficiencies that need correcting:

* for one value of word 0, bit 6-5, check for INTRQ bit presence

* for other values of word 0, bit 6-5, we should honor the requested 
delay before checking for DRQ

* guarantee that the total length of an ATAPI DMA transfer is a multiple 
of 4 bytes

Re: SATAPI timing

[Jens Axboe]

On Sat, Feb 19 2005, Jeff Garzik wrote:
> Albert Lee wrote:
> >I'm a little confused about what is the "correct behavior" of ATAPI DMA 
> >INTRQ.
> >
> >1. The ATA-4 flowchart and ATA-6 state diagram seems to be inconsistent.
> >In the ATA-4 flowchart, no wait for INTRQ is specified.
> >
> >2. From the ATA-5 errata:
> >http://www.t13.org/project/e01122r0.pdf
> >
> >"...Page 265 through 270 of NCITS 340-2000 clause 9.8 was not properly 
> >converted from the flow
> >charts in NCITS 317-1998 to the state diagrams. The state diagrams figures 
> >33 and 34 and
> >associated text is modified to indicate that the device interrupts only at 
> >command completion. "
> >
> >Notice the "device interrupts only at command completion." statement.
> >However, the diagrams in the errata are inconsistent with the above 
> >statement.
> >
> >3. The ide-cd code does not wait for INTRQ before starting BM-DMA
> >
> >        /* Arm the interrupt handler. */
> >        ide_set_handler(drive, handler, rq->timeout, cdrom_timer_expiry);
> >
> >        /* ATAPI commands get padded out to 12 bytes minimum */
> >        cmd_len = COMMAND_SIZE(rq->cmd[0]);
> >        if (cmd_len < ATAPI_MIN_CDB_BYTES)
> >            cmd_len = ATAPI_MIN_CDB_BYTES;
> >
> >        /* Send the command to the device. */
> >        HWIF(drive)->atapi_output_bytes(drive, rq->cmd, cmd_len);
> >
> >        /* Start the DMA if need be */
> >        if (info->dma)
> >            hwif->dma_start(drive);
> >
> >        return ide_started;
> >
> >4. My CD-ROM drive does not generate INTRQ after it received the command 
> >packet.
> >   It only assert DMARQ.
> >
> >Maybe the state diagrams in ATA-5 and ATA-6 are incorrect?  Any idea?
> 
> 
> As I mentioned in an earlier email, research seems to indicate the 
> IDENTIFY PACKET DEVICE word 0, bits 6-5 can be used to indicate INTRQ 
> will be delivered.  This is consistent with my ATA-4 specification.
> 
> Apparently this behavior is obsolete, but we still need to support it.

ide-cd uses those bits to check for interrupt delivery, see
drq_interrupt in that file.

-- 
Jens Axboe

Re: SATAPI timing

[Albert Lee]

> On Sat, Feb 19 2005, Jeff Garzik wrote:
> > Albert Lee wrote:
> > >I'm a little confused about what is the "correct behavior" of ATAPI DMA 
> > >INTRQ.
> > >
> > >1. The ATA-4 flowchart and ATA-6 state diagram seems to be inconsistent.
> > >In the ATA-4 flowchart, no wait for INTRQ is specified.
> > >
> > >2. From the ATA-5 errata:
> > >http://www.t13.org/project/e01122r0.pdf
> > >
> > >"...Page 265 through 270 of NCITS 340-2000 clause 9.8 was not properly 
> > >converted from the flow
> > >charts in NCITS 317-1998 to the state diagrams. The state diagrams figures 
> > >33 and 34 and
> > >associated text is modified to indicate that the device interrupts only at 
> > >command completion. "
> > >
> > >Notice the "device interrupts only at command completion." statement.
> > >However, the diagrams in the errata are inconsistent with the above 
> > >statement.
> > >
> > >3. The ide-cd code does not wait for INTRQ before starting BM-DMA
> > >
> > >        /* Arm the interrupt handler. */
> > >        ide_set_handler(drive, handler, rq->timeout, cdrom_timer_expiry);
> > >
> > >        /* ATAPI commands get padded out to 12 bytes minimum */
> > >        cmd_len = COMMAND_SIZE(rq->cmd[0]);
> > >        if (cmd_len < ATAPI_MIN_CDB_BYTES)
> > >            cmd_len = ATAPI_MIN_CDB_BYTES;
> > >
> > >        /* Send the command to the device. */
> > >        HWIF(drive)->atapi_output_bytes(drive, rq->cmd, cmd_len);
> > >
> > >        /* Start the DMA if need be */
> > >        if (info->dma)
> > >            hwif->dma_start(drive);
> > >
> > >        return ide_started;
> > >
> > >4. My CD-ROM drive does not generate INTRQ after it received the command 
> > >packet.
> > >   It only assert DMARQ.
> > >
> > >Maybe the state diagrams in ATA-5 and ATA-6 are incorrect?  Any idea?
> > 
> > 
> > As I mentioned in an earlier email, research seems to indicate the 
> > IDENTIFY PACKET DEVICE word 0, bits 6-5 can be used to indicate INTRQ 
> > will be delivered.  This is consistent with my ATA-4 specification.
> > 
> > Apparently this behavior is obsolete, but we still need to support it.
> 
> ide-cd uses those bits to check for interrupt delivery, see
> drq_interrupt in that file.
> 
  I've read the ide-cd code about the drq_interrupt indicated by config word 0, bits 6-5. 
The DRQ interrupt seems to be after PACKET command 0xA0 and _before_ the command packet 
is written to the device:

if (CDROM_CONFIG_FLAGS (drive)->drq_interrupt) {
  /* packet command */
  ide_execute_command(drive, WIN_PACKETCMD, handler, ATAPI_WAIT_PC, cdrom_timer_expiry);
  return ide_started;
 } else {
  unsigned long flags;

  /* packet command */
  spin_lock_irqsave(&ide_lock, flags);
  hwif->OUTBSYNC(drive, WIN_PACKETCMD, IDE_COMMAND_REG);
  ndelay(400);
  spin_unlock_irqrestore(&ide_lock, flags);

  return (*handler) (drive);
 }

  However, it seems not the same interrupt mentioned by gp and ATA-6 chap. 9.8 Figure 33.
The interrupt is _after_ the command packet is sent to the device: (HPD1:HPD3)
"When the Data register has been written, the writing of the command packet is completed,
and nIEN is cleared to zero, the host shall make a transition to the HPD3: INTRQ wait state."

Albert