[PATCH 2/2 v1] pata_via.c: Patch the behavior of via chipsets.

[PATCH 2/2 v1] pata_via.c: Patch the behavior of via chipsets.

[JosephChan]

This patch is to patch the behavior of via chipsets, which will reset the DEV 
register to be reset value after changing the IEN bit on CTL register to cause
channel reset. For some controllers, the pata_via default processing flow will
work OK, because the controller will cache the value of the registers until it
receives a set device operation (write the device register). By following the
ATA/ATAPI protocol, DEV bit should be set before issuing ATA/ATAPI commands to
device. And this patch is to set DEV bit after IEN bit is set of VIA chipset
VX700/CX700, VX800/V820 and VX855 series.


Signed-off-by: Joseph Chan <josephchan@via.com.tw>

--- a/drivers/ata/pata_via.c	2009-01-16 02:36:58.000000000 +0800
+++ a/drivers/ata/pata_via.c	2009-01-16 02:39:04.000000000 +0800
@@ -347,12 +347,11 @@
  */
 static void via_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)
 {
-	struct ata_taskfile tmp_tf;
+	struct ata_ioports *ioaddr = &ap->ioaddr;
 
-	if (ap->ctl != ap->last_ctl && !(tf->flags & ATA_TFLAG_DEVICE)) {
-		tmp_tf = *tf;
-		tmp_tf.flags |= ATA_TFLAG_DEVICE;
-		tf = &tmp_tf;
+	if (tf->ctl != ap->last_ctl) {
+		iowrite8(tf->ctl, ioaddr->ctl_addr);
+		iowrite8(tf->device, ioaddr->device_addr);
 	}
 	ata_sff_tf_load(ap, tf);
 } 

Re: [PATCH 2/2 v1] pata_via.c: Patch the behavior of via chipsets.

[Tejun Heo]

Hello, Joseph.

JosephChan@via.com.tw wrote:
> This patch is to patch the behavior of via chipsets, which will reset the DEV 
> register to be reset value after changing the IEN bit on CTL register to cause
> channel reset. For some controllers, the pata_via default processing flow will
> work OK, because the controller will cache the value of the registers until it
> receives a set device operation (write the device register). By following the
> ATA/ATAPI protocol, DEV bit should be set before issuing ATA/ATAPI commands to
> device. And this patch is to set DEV bit after IEN bit is set of VIA chipset
> VX700/CX700, VX800/V820 and VX855 series.
> 
> Signed-off-by: Joseph Chan <josephchan@via.com.tw>
> 
> --- a/drivers/ata/pata_via.c	2009-01-16 02:36:58.000000000 +0800
> +++ a/drivers/ata/pata_via.c	2009-01-16 02:39:04.000000000 +0800
> @@ -347,12 +347,11 @@
>   */
>  static void via_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)
>  {
> -	struct ata_taskfile tmp_tf;
> +	struct ata_ioports *ioaddr = &ap->ioaddr;
>  
> -	if (ap->ctl != ap->last_ctl && !(tf->flags & ATA_TFLAG_DEVICE)) {
> -		tmp_tf = *tf;
> -		tmp_tf.flags |= ATA_TFLAG_DEVICE;
> -		tf = &tmp_tf;
> +	if (tf->ctl != ap->last_ctl) {
> +		iowrite8(tf->ctl, ioaddr->ctl_addr);
> +		iowrite8(tf->device, ioaddr->device_addr);

Oh, I see.  So the controller requires that the DEVICE register is
always loaded whenever ctl changes && it should be loaded before the
LBA regs.  Interesting.  Please note that the device select bit is
always loaded before any of these happens.

Can you please add comments to explain it?  Also, it would be nice if
you do ap->ctl = ap->last_ctl and turn off ATA_TFLAG_DEVICE to avoid
loading them yet again.

Thanks.

-- 
tejun

Re: [PATCH 2/2 v1] pata_via.c: Patch the behavior of via chipsets.

[Alan Cox]

> Can you please add comments to explain it?  Also, it would be nice if
> you do ap->ctl = ap->last_ctl and turn off ATA_TFLAG_DEVICE to avoid
> loading them yet again.

I don't think you can turn off ATA_TFLAG_DEVICE, you have no guarantee it
won't be referred to again anywhere else in the stack - tf->flags isn't
owned by the driver so its asking for trouble to do that.

Alan

Re: [PATCH 2/2 v1] pata_via.c: Patch the behavior of via chipsets.

[Tejun Heo]

Alan Cox wrote:
>> Can you please add comments to explain it?  Also, it would be nice if
>> you do ap->ctl = ap->last_ctl and turn off ATA_TFLAG_DEVICE to avoid
>> loading them yet again.
> 
> I don't think you can turn off ATA_TFLAG_DEVICE, you have no guarantee it
> won't be referred to again anywhere else in the stack - tf->flags isn't
> owned by the driver so its asking for trouble to do that.

The original code copied tf to tmp_tf and turned it off there.  New code
can do about the same thing.

-- 
tejun

Re: [PATCH 2/2 v1] pata_via.c: Patch the behavior of via chipsets.

[Tejun Heo]

Tejun Heo wrote:
> Alan Cox wrote:
>>> Can you please add comments to explain it?  Also, it would be nice if
>>> you do ap->ctl = ap->last_ctl and turn off ATA_TFLAG_DEVICE to avoid
>>> loading them yet again.
>> I don't think you can turn off ATA_TFLAG_DEVICE, you have no guarantee it
>> won't be referred to again anywhere else in the stack - tf->flags isn't
>> owned by the driver so its asking for trouble to do that.
> 
> The original code copied tf to tmp_tf and turned it off there.  New code
> can do about the same thing.

But then again, maybe it's just better to open code it at this point.

-- 
tejun