[PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Tejun Heo]

Make pdc_atapi_pkt() use values from qc->tf instead of creating its
own.  This is to ease future ATAPI handling changes.

DONT APPLY YET
---
Mikael, would this work?  Values other than lbam and lbah remain the
same.  Does sata_promise have strict requirements for lbam and lbah?

Thanks.

 drivers/ata/sata_promise.c |   34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

Index: work/drivers/ata/sata_promise.c
===================================================================
--- work.orig/drivers/ata/sata_promise.c
+++ work/drivers/ata/sata_promise.c
@@ -450,7 +450,7 @@ static void pdc_atapi_pkt(struct ata_que
 	struct pdc_port_priv *pp = ap->private_data;
 	u8 *buf = pp->pkt;
 	u32 *buf32 = (u32 *) buf;
-	unsigned int dev_sel, feature, nbytes;
+	unsigned int dev_sel, feature;
 
 	/* set control bits (byte 0), zero delay seq id (byte 3),
 	 * and seq id (byte 2)
@@ -473,45 +473,37 @@ static void pdc_atapi_pkt(struct ata_que
 	buf32[2] = 0;				/* no next-packet */
 
 	/* select drive */
-	if (sata_scr_valid(&ap->link)) {
+	if (sata_scr_valid(&ap->link))
 		dev_sel = PDC_DEVICE_SATA;
-	} else {
-		dev_sel = ATA_DEVICE_OBS;
-		if (qc->dev->devno != 0)
-			dev_sel |= ATA_DEV1;
-	}
+	else
+		dev_sel = qc->tf.device;
+
 	buf[12] = (1 << 5) | ATA_REG_DEVICE;
 	buf[13] = dev_sel;
 	buf[14] = (1 << 5) | ATA_REG_DEVICE | PDC_PKT_CLEAR_BSY;
 	buf[15] = dev_sel; /* once more, waiting for BSY to clear */
 
 	buf[16] = (1 << 5) | ATA_REG_NSECT;
-	buf[17] = 0x00;
+	buf[17] = qc->tf.nsect;
 	buf[18] = (1 << 5) | ATA_REG_LBAL;
-	buf[19] = 0x00;
+	buf[19] = qc->tf.lbal;
 
 	/* set feature and byte counter registers */
-	if (qc->tf.protocol != ATA_PROT_ATAPI_DMA) {
+	if (qc->tf.protocol != ATA_PROT_ATAPI_DMA)
 		feature = PDC_FEATURE_ATAPI_PIO;
-		/* set byte counter register to real transfer byte count */
-		nbytes = qc->nbytes;
-		if (nbytes > 0xffff)
-			nbytes = 0xffff;
-	} else {
+	else
 		feature = PDC_FEATURE_ATAPI_DMA;
-		/* set byte counter register to 0 */
-		nbytes = 0;
-	}
+
 	buf[20] = (1 << 5) | ATA_REG_FEATURE;
 	buf[21] = feature;
 	buf[22] = (1 << 5) | ATA_REG_BYTEL;
-	buf[23] = nbytes & 0xFF;
+	buf[23] = qc->tf.lbam;
 	buf[24] = (1 << 5) | ATA_REG_BYTEH;
-	buf[25] = (nbytes >> 8) & 0xFF;
+	buf[25] = qc->tf.lbah;
 
 	/* send ATAPI packet command 0xA0 */
 	buf[26] = (1 << 5) | ATA_REG_CMD;
-	buf[27] = ATA_CMD_PACKET;
+	buf[27] = qc->tf.command;
 
 	/* select drive and check DRQ */
 	buf[28] = (1 << 5) | ATA_REG_DEVICE | PDC_PKT_WAIT_DRDY;

Re: [PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Mikael Pettersson]

On Mon, 26 Nov 2007 20:34:38 +0900, Tejun Heo wrote:
> Make pdc_atapi_pkt() use values from qc->tf instead of creating its
> own.  This is to ease future ATAPI handling changes.
> 
> DONT APPLY YET
> ---
> Mikael, would this work?  Values other than lbam and lbah remain the
> same.  Does sata_promise have strict requirements for lbam and lbah?

...

>  	/* set feature and byte counter registers */
> -	if (qc->tf.protocol != ATA_PROT_ATAPI_DMA) {
> +	if (qc->tf.protocol != ATA_PROT_ATAPI_DMA)
>  		feature = PDC_FEATURE_ATAPI_PIO;
> -		/* set byte counter register to real transfer byte count */
> -		nbytes = qc->nbytes;
> -		if (nbytes > 0xffff)
> -			nbytes = 0xffff;
> -	} else {
> +	else
>  		feature = PDC_FEATURE_ATAPI_DMA;
> -		/* set byte counter register to 0 */
> -		nbytes = 0;
> -	}
> +
>  	buf[20] = (1 << 5) | ATA_REG_FEATURE;
>  	buf[21] = feature;
>  	buf[22] = (1 << 5) | ATA_REG_BYTEL;
> -	buf[23] = nbytes & 0xFF;
> +	buf[23] = qc->tf.lbam;
>  	buf[24] = (1 << 5) | ATA_REG_BYTEH;
> -	buf[25] = (nbytes >> 8) & 0xFF;
> +	buf[25] = qc->tf.lbah;

The original code matches what Promise' own driver does, including
the "set byte counter register to real transfer byte count" comment.
It's certainly possible that if lbah/lbam don't match ->nbytes,
the HW will go nuts. Their data sheets are very quiet about ATAPI.

I can test your proposed change next weekend when I'm back to where
my sata_promise test equipment is.

/Mikael

Re: [PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Tejun Heo]

Hello,

Mikael Pettersson wrote:
>>  	buf[20] = (1 << 5) | ATA_REG_FEATURE;
>>  	buf[21] = feature;
>>  	buf[22] = (1 << 5) | ATA_REG_BYTEL;
>> -	buf[23] = nbytes & 0xFF;
>> +	buf[23] = qc->tf.lbam;
>>  	buf[24] = (1 << 5) | ATA_REG_BYTEH;
>> -	buf[25] = (nbytes >> 8) & 0xFF;
>> +	buf[25] = qc->tf.lbah;
> 
> The original code matches what Promise' own driver does, including
> the "set byte counter register to real transfer byte count" comment.
> It's certainly possible that if lbah/lbam don't match ->nbytes,
> the HW will go nuts. Their data sheets are very quiet about ATAPI.

Yeah, now core code sets the value properly and I just posted a patch to
massage transfer chunk size a bit so I was curious whether now
sata_promise can use the result of core layer instead.

> I can test your proposed change next weekend when I'm back to where
> my sata_promise test equipment is.

Thank you.

-- 
tejun

Re: [PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Jeff Garzik]

Tejun Heo wrote:
> Make pdc_atapi_pkt() use values from qc->tf instead of creating its
> own.  This is to ease future ATAPI handling changes.
> 
> DONT APPLY YET
> ---
> Mikael, would this work?  Values other than lbam and lbah remain the
> same.  Does sata_promise have strict requirements for lbam and lbah?
> 
> Thanks.
> 
>  drivers/ata/sata_promise.c |   34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)

what was the outcome of this discussion?

I haven't looked over the Promise datasheet nor checked my brain for 
details, hoping Mikael would do that for me ;-)

Re: [PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Mikael Pettersson]

On Sat, 01 Dec 2007 18:23:44 -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Make pdc_atapi_pkt() use values from qc->tf instead of creating its
> > own.  This is to ease future ATAPI handling changes.
> > 
> > DONT APPLY YET
> > ---
> > Mikael, would this work?  Values other than lbam and lbah remain the
> > same.  Does sata_promise have strict requirements for lbam and lbah?
> > 
> > Thanks.
> > 
> >  drivers/ata/sata_promise.c |   34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> what was the outcome of this discussion?
> 
> I haven't looked over the Promise datasheet nor checked my brain for 
> details, hoping Mikael would do that for me ;-)

I've now tested this on top of 2.6.24-rc3, with no observable
regressions. Blanking, writing, and mounting/reading CD-RWs
on both SATAPI and PATAPI works (tested on a 300 TX2plus card).

I can't find anything in Promise's public docs or reference driver
about non-standard requirements on lbam/lbah in ATAPI packets.

/Mikael

Re: [PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Tejun Heo]

Mikael Pettersson wrote:
>> what was the outcome of this discussion?
>>
>> I haven't looked over the Promise datasheet nor checked my brain for 
>> details, hoping Mikael would do that for me ;-)
> 
> I've now tested this on top of 2.6.24-rc3, with no observable
> regressions. Blanking, writing, and mounting/reading CD-RWs
> on both SATAPI and PATAPI works (tested on a 300 TX2plus card).
> 
> I can't find anything in Promise's public docs or reference driver
> about non-standard requirements on lbam/lbah in ATAPI packets.

The values set by core layer should be good enough.  The only thing I'm
worried about is setting transfer chunk size when protocol is DMA.  As
setting this value hasn't caused any problem for other controllers and
it seems sata_promise doesn't seem to have problem with it either, I'm
leaning toward keeping this value but if setting this value to zero is
the right thing to do, we can definitely change that in the core layer.
 One way or the other, I'd really like to keep sata_promise's behavior
in line with other libata drivers.

So, Mikael, do you think it would be okay to include the patch for
#upstream and see how it works in -mm?

Thanks.

-- 
tejun

Re: [PATCH RFC] sata_promise: make pdc_atapi_pkt() use values from qc->tf

[Mikael Pettersson]

On Mon, 03 Dec 2007 13:49:36 +0900, Tejun Heo wrote:
> Mikael Pettersson wrote:
> >> what was the outcome of this discussion?
> >>
> >> I haven't looked over the Promise datasheet nor checked my brain for 
> >> details, hoping Mikael would do that for me ;-)
> > 
> > I've now tested this on top of 2.6.24-rc3, with no observable
> > regressions. Blanking, writing, and mounting/reading CD-RWs
> > on both SATAPI and PATAPI works (tested on a 300 TX2plus card).
> > 
> > I can't find anything in Promise's public docs or reference driver
> > about non-standard requirements on lbam/lbah in ATAPI packets.
> 
> The values set by core layer should be good enough.  The only thing I'm
> worried about is setting transfer chunk size when protocol is DMA.  As
> setting this value hasn't caused any problem for other controllers and
> it seems sata_promise doesn't seem to have problem with it either, I'm
> leaning toward keeping this value but if setting this value to zero is
> the right thing to do, we can definitely change that in the core layer.
>  One way or the other, I'd really like to keep sata_promise's behavior
> in line with other libata drivers.
> 
> So, Mikael, do you think it would be okay to include the patch for
> #upstream and see how it works in -mm?

Yes

/Mikael