Re: [Ksummit-2008-discuss] Kernel Summit request for Discussion of future of ATA (libata) and IDE

Re: [Ksummit-2008-discuss] Kernel Summit request for Discussion of future of ATA (libata) and IDE

[David Woodhouse]

On Sun, 2008-08-03 at 19:45 +0200, Rafael J. Wysocki wrote:
> Well, I know of at least one case of PC hardware in which an old IDE driver
> basically works while the libata/pata poeple have no idea why their driver
> doesn't:

I have one of those too (pdc202xx craps itself with the new drivers).

-- 
dwmw2

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Alan Cox]

> I was looking into the 32-bit PIO issue a bit yesterday. It looks like 
> some of the VLB libata drivers are doing this internally already, so it 
> shouldn't be hard to do this in the core. Only question is how we know 
> generically if the controller can do it or not? It looks like in old

You don't. Basically it is controller dependant. Pretty much all the
newer controllers support the 32bit PIO data cycles. Most PCI controllers
it makes no speed difference but host bus controllers (especially
PIIX/ICH) really benefit.

> supported. I couldn't track down where that bit was actually defined in 
> the first place, all the way back to ATA-1 it seems to be indicated as 
> reserved. Actually, I'm not sure why the drive cares in the first place, 
> it would seem like a pure host controller issue..

It goes back before IDE into the depths of the original compaq spec. When
you have a device wired basically directly to the ISA bus (original IDE)
it mattered. I don't believe it is relevant to any of the PCI controllers.

Alan

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Robert Hancock]

Alan Cox wrote:
>> * There are still corner case in libata core - PIO is dead slow
>> compared to drivers/ide/,
> 
> There are two there - libata keeps IRQs blocked for longer in PIO mode as
> well which is a factor for realtime that needs looking at, as well as
> using 16bit not 32bit I/O for most devices (which is trivial to fix). The
> IRQ masking stuff is more complex and old IDE handles it far better for
> PIO on non shared IRQ interfaces. That is actually probably the most
> complicated thing to address of the stuff you'd want to do if you were
> going to kill off old IDE.

I was looking into the 32-bit PIO issue a bit yesterday. It looks like 
some of the VLB libata drivers are doing this internally already, so it 
shouldn't be hard to do this in the core. Only question is how we know 
generically if the controller can do it or not? It looks like in old 
IDE, a few controllers explicitly disable it, but it appears that it 
doesn't default to on for any controller, so it's possible there are 
others on which it doesn't work. Presumably anything on an actual 16-bit 
bus (ISA, LPC, etc.) wouldn't like it, to start with.

There's also the matter of the identify bit to indicate whether the 
drive supports 32-bit transfers, which was reallocated to trusted 
computing in ATA-8 so any drive matching that standard will indicate not 
supported. I couldn't track down where that bit was actually defined in 
the first place, all the way back to ATA-1 it seems to be indicated as 
reserved. Actually, I'm not sure why the drive cares in the first place, 
it would seem like a pure host controller issue..

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Jeff Garzik]

Robert Hancock wrote:
> Alan Cox wrote:
>>> * There are still corner case in libata core - PIO is dead slow
>>> compared to drivers/ide/,
>>
>> There are two there - libata keeps IRQs blocked for longer in PIO mode as
>> well which is a factor for realtime that needs looking at, as well as
>> using 16bit not 32bit I/O for most devices (which is trivial to fix). The
>> IRQ masking stuff is more complex and old IDE handles it far better for
>> PIO on non shared IRQ interfaces. That is actually probably the most
>> complicated thing to address of the stuff you'd want to do if you were
>> going to kill off old IDE.
> 
> I was looking into the 32-bit PIO issue a bit yesterday. It looks like 
> some of the VLB libata drivers are doing this internally already, so it 
> shouldn't be hard to do this in the core. Only question is how we know 
> generically if the controller can do it or not? It looks like in old 
> IDE, a few controllers explicitly disable it, but it appears that it 
> doesn't default to on for any controller, so it's possible there are 
> others on which it doesn't work. Presumably anything on an actual 16-bit 
> bus (ISA, LPC, etc.) wouldn't like it, to start with.

FWIW there is already a patch from Willy Terreau (sp?) to add 32-bit I/O.

I queued it for "later" because it had some issues that Alan pointed 
out, IIRC.  I definitely want to push it in, though.

	Jeff

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Alan Cox]

> I guess that bit doesn't really make any difference with remotely modern 
> drives, then.. Could we make that ata_id_has_dword_io check always 
> return true if ata_id_is_ata returns true and only check word 48 if not?

I suspect (but need to dig further) that ata_id_has_dword_io should only
be called by pata_legacy.

> I saw Willy Tarreau's patch from February for this, I agree that we 
> should likely use a separate data_xfer method for 32-bit transfer (or if 
> enough controllers should support 32-bit, then just make it be the 
> default and make a separate 16-bit only function for those that don't), 
> rather than punting the decision to the user with hdparm.

Definitely.

> You mentioned in the thread for Willy's patch that "some
> controllers have quirky rules for 32bit xfers" - any details anywhere?

There are two main ones

- Some controllers only support 32bit I/O for a multiple of 32bit values
[sometimes 'unless the fifo is disabled']. I'd have to go back over the
docs but I think the AMD may be one of those
- Some controllers (VLB generally) require a magic sequence before the
transfer. You'll see that in the pata_legacy bits.

Alan

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Robert Hancock]

Alan Cox wrote:
>> I was looking into the 32-bit PIO issue a bit yesterday. It looks like 
>> some of the VLB libata drivers are doing this internally already, so it 
>> shouldn't be hard to do this in the core. Only question is how we know 
>> generically if the controller can do it or not? It looks like in old
> 
> You don't. Basically it is controller dependant. Pretty much all the
> newer controllers support the 32bit PIO data cycles. Most PCI controllers
> it makes no speed difference but host bus controllers (especially
> PIIX/ICH) really benefit.
> 
>> supported. I couldn't track down where that bit was actually defined in 
>> the first place, all the way back to ATA-1 it seems to be indicated as 
>> reserved. Actually, I'm not sure why the drive cares in the first place, 
>> it would seem like a pure host controller issue..
> 
> It goes back before IDE into the depths of the original compaq spec. When
> you have a device wired basically directly to the ISA bus (original IDE)
> it mattered. I don't believe it is relevant to any of the PCI controllers.

I guess that bit doesn't really make any difference with remotely modern 
drives, then.. Could we make that ata_id_has_dword_io check always 
return true if ata_id_is_ata returns true and only check word 48 if not?

I saw Willy Tarreau's patch from February for this, I agree that we 
should likely use a separate data_xfer method for 32-bit transfer (or if 
enough controllers should support 32-bit, then just make it be the 
default and make a separate 16-bit only function for those that don't), 
rather than punting the decision to the user with hdparm.

You mentioned in the thread for Willy's patch that "some
controllers have quirky rules for 32bit xfers" - any details anywhere?

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Alan Cox]

> > newer controllers support the 32bit PIO data cycles. Most PCI controllers
> > it makes no speed difference but host bus controllers (especially
> > PIIX/ICH) really benefit.
> >   
> 
>     In what way if there's no speed gain?

As in the numbers are the same before and after. The FIFO on the
controller is happily hiding the extra latencies I assume.

> >> supported. I couldn't track down where that bit was actually defined in 
> >> the first place, all the way back to ATA-1 it seems to be indicated as 
> >> reserved. Actually, I'm not sure why the drive cares in the first place, 
> >> it would seem like a pure host controller issue..
> >>     
> >
> > It goes back before IDE into the depths of the original compaq spec. When
> > you have a device wired basically directly to the ISA bus (original IDE)
> >   
> 
>    ISA has only 8/16-bit data bus, so it could not have mattered 
> there... 

Depends what a 32bit I/O looks like on the 16bit bus - timing wise.

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Robert Hancock]

Alan Cox wrote:
>> You mentioned in the thread for Willy's patch that "some
>> controllers have quirky rules for 32bit xfers" - any details anywhere?
> 
> There are two main ones
> 
> - Some controllers only support 32bit I/O for a multiple of 32bit values
> [sometimes 'unless the fifo is disabled']. I'd have to go back over the
> docs but I think the AMD may be one of those

The AMD-766 doc I have says that when the Secondary Posted Write Buffer 
or Primary Posted Write Buffer are enabled, only 32-bit writes are 
allowed to the data port. It doesn't say anything about a restriction 
with the read prefetch buffer though.

I guess it depends if any other controllers could potentially have this 
restriction. I suspect non-multiple-of-32-bit transfers are rare enough 
we could just fall back to 16-bit IO always for them, but maybe not.

> - Some controllers (VLB generally) require a magic sequence before the
> transfer. You'll see that in the pata_legacy bits.
> 
> Alan
> 

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Sergei Shtylyov]

Hello.

Alan Cox wrote:
>> I was looking into the 32-bit PIO issue a bit yesterday. It looks like 
>> some of the VLB libata drivers are doing this internally already, so it 
>> shouldn't be hard to do this in the core. Only question is how we know 
>> generically if the controller can do it or not? It looks like in old
>>     
>
> You don't. Basically it is controller dependant. Pretty much all the
> newer controllers support the 32bit PIO data cycles. Most PCI controllers
> it makes no speed difference but host bus controllers (especially
> PIIX/ICH) really benefit.
>   

    In what way if there's no speed gain?

>> supported. I couldn't track down where that bit was actually defined in 
>> the first place, all the way back to ATA-1 it seems to be indicated as 
>> reserved. Actually, I'm not sure why the drive cares in the first place, 
>> it would seem like a pure host controller issue..
>>     
>
> It goes back before IDE into the depths of the original compaq spec. When
> you have a device wired basically directly to the ISA bus (original IDE)
>   

   ISA has only 8/16-bit data bus, so it could not have mattered 
there... and I don't think that there ever was a direct connection 
between the IDE and host bus other than ISA... except maybe EISA.

MBR, Sergei

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Alan Cox]

On Mon, 04 Aug 2008 18:12:51 -0400
Mark Lord <liml@rtr.ca> wrote:

> Alan Cox wrote:
> > You don't. Basically it is controller dependant. Pretty much all the
> > newer controllers support the 32bit PIO data cycles. Most PCI controllers
> > it makes no speed difference but host bus controllers (especially
> > PIIX/ICH) really benefit.
> ..
> 
> By "most PCI controllers", did you really mean "most add-on PCI (card) controllers"?

Yes

> Nearly all onboard "PCI" controllers (integrated into the host chipset) benefit hugely,
> but I suppose that is what was meant by "host bus controllers".

Yes

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Mark Lord]

Alan Cox wrote:
> You don't. Basically it is controller dependant. Pretty much all the
> newer controllers support the 32bit PIO data cycles. Most PCI controllers
> it makes no speed difference but host bus controllers (especially
> PIIX/ICH) really benefit.
..

By "most PCI controllers", did you really mean "most add-on PCI (card) controllers"?

Nearly all onboard "PCI" controllers (integrated into the host chipset) benefit hugely,
but I suppose that is what was meant by "host bus controllers".

Cheers

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Sergei Shtylyov]

Hello.

Alan Cox wrote:
>>> newer controllers support the 32bit PIO data cycles. Most PCI controllers
>>> it makes no speed difference but host bus controllers (especially
>>> PIIX/ICH) really benefit.
>>>       

   PIIX was a pure PCI controller, IIRC. ICH is also not a "host bus" 
controller, it hangs off the I/O hub bus...

>>>   
>>>       
>>     In what way if there's no speed gain?
>>     
>
> As in the numbers are the same before and after. The FIFO on the
> controller is happily hiding the extra latencies I assume.
>   

   Depends on the PIO mode -- in the lower ones, the prefetch reads 
might really be slower than successive reads on the host bus...

>>>> supported. I couldn't track down where that bit was actually defined in 
>>>> the first place, all the way back to ATA-1 it seems to be indicated as 
>>>> reserved. Actually, I'm not sure why the drive cares in the first place, 
>>>> it would seem like a pure host controller issue..
>>>>     
>>>>         
>>> It goes back before IDE into the depths of the original compaq spec. When
>>> you have a device wired basically directly to the ISA bus (original IDE)
>>>   
>>>       
>>    ISA has only 8/16-bit data bus, so it could not have mattered 
>> there... 
>>     
>
> Depends what a 32bit I/O looks like on the 16bit bus - timing wise.
>   

   Two 16-bit reads at addresses 0x1x0 and 0x1x2 with the programmed 
recovery time, IIRC... It's just occured to me that in case of the 
16-bit bus it should be how the drive treated the accesses at address 
0x1x2 with IOCS16 asserted that could have mattered. If it honored them, 
32-bit I/O could have worked even on a dumb ISA "controller", if not -- 
no way (unless you really had *something* between the ISA and the IDE 
cable).

MBR, Sergei

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Sergei Shtylyov]

Hello, I wrote:
>>>>> supported. I couldn't track down where that bit was actually 
>>>>> defined in the first place, all the way back to ATA-1 it seems to 
>>>>> be indicated as reserved. Actually, I'm not sure why the drive 
>>>>> cares in the first place, it would seem like a pure host 
>>>>> controller issue..
>>>>>             
>>>> It goes back before IDE into the depths of the original compaq 
>>>> spec. When
>>>> you have a device wired basically directly to the ISA bus (original 
>>>> IDE)
>>>>         
>>>    ISA has only 8/16-bit data bus, so it could not have mattered 
>>> there...     
>>
>> Depends what a 32bit I/O looks like on the 16bit bus - timing wise.
>>   
>
>   Two 16-bit reads at addresses 0x1x0 and 0x1x2 with the programmed 
> recovery time, IIRC... It's just occured to me that in case of the 
> 16-bit bus it should be how the drive treated the accesses at address 
> 0x1x2 with IOCS16 asserted that could have mattered. If it honored 
> them, 32-bit I/O could have worked even on a dumb ISA "controller", if 
> not -- no way (unless you really had *something* between the ISA and 
> the IDE cable).

    Oh, -IOCS16 is driven by device, not host. I give up then. :-)

MBR, Sergei

Re: [Ksummit-2008-discuss] Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Tejun Heo]

Jeff Garzik wrote:
> Robert Hancock wrote:
>> Alan Cox wrote:
>>>> * There are still corner case in libata core - PIO is dead slow
>>>> compared to drivers/ide/,
>>> There are two there - libata keeps IRQs blocked for longer in PIO mode as
>>> well which is a factor for realtime that needs looking at, as well as
>>> using 16bit not 32bit I/O for most devices (which is trivial to fix). The
>>> IRQ masking stuff is more complex and old IDE handles it far better for
>>> PIO on non shared IRQ interfaces. That is actually probably the most
>>> complicated thing to address of the stuff you'd want to do if you were
>>> going to kill off old IDE.
>> I was looking into the 32-bit PIO issue a bit yesterday. It looks like 
>> some of the VLB libata drivers are doing this internally already, so it 
>> shouldn't be hard to do this in the core. Only question is how we know 
>> generically if the controller can do it or not? It looks like in old 
>> IDE, a few controllers explicitly disable it, but it appears that it 
>> doesn't default to on for any controller, so it's possible there are 
>> others on which it doesn't work. Presumably anything on an actual 16-bit 
>> bus (ISA, LPC, etc.) wouldn't like it, to start with.
> 
> FWIW there is already a patch from Willy Terreau (sp?) to add 32-bit I/O.
> 
> I queued it for "later" because it had some issues that Alan pointed 
> out, IIRC.  I definitely want to push it in, though.

Jeff, have your thoughts about PIO IRQ disable handling changed yet?  I
don't really see any better way than doing it like IDE.

Thanks.

-- 
tejun

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Robert Hancock]

Alan Cox wrote:
>> I guess that bit doesn't really make any difference with remotely modern 
>> drives, then.. Could we make that ata_id_has_dword_io check always 
>> return true if ata_id_is_ata returns true and only check word 48 if not?
> 
> I suspect (but need to dig further) that ata_id_has_dword_io should only
> be called by pata_legacy.
> 
>> I saw Willy Tarreau's patch from February for this, I agree that we 
>> should likely use a separate data_xfer method for 32-bit transfer (or if 
>> enough controllers should support 32-bit, then just make it be the 
>> default and make a separate 16-bit only function for those that don't), 
>> rather than punting the decision to the user with hdparm.
> 
> Definitely.
> 
>> You mentioned in the thread for Willy's patch that "some
>> controllers have quirky rules for 32bit xfers" - any details anywhere?
> 
> There are two main ones
> 
> - Some controllers only support 32bit I/O for a multiple of 32bit values
> [sometimes 'unless the fifo is disabled']. I'd have to go back over the
> docs but I think the AMD may be one of those
> - Some controllers (VLB generally) require a magic sequence before the
> transfer. You'll see that in the pata_legacy bits.

Here's my first cut at it. Compile tested only. This sets most controllers
to use 32-bit PIO except for those which could potentially be on a real ISA
or other 16-bit bus. It's a bit non-obvious what to do with some of the
drivers, so input is welcome.

This implementation doesn't check the ata_id_has_dword_io at all, since it
would only make a difference on controllers where we don't really want to
use it anyway.

It seems like regardless of whether we do 32-bit by default or not the 32-bit
data_xfer function should be added to libata core as we have several drivers
which duplicate the same code currently..

Signed-off-by: Robert Hancock <hancockr@shaw.ca>	

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 304fdc6..acb65a9 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -673,13 +673,14 @@ static inline void ata_tf_to_host(struct ata_port *ap,
 }
 
 /**
- *	ata_sff_data_xfer - Transfer data by PIO
+ *	ata_sff_data_xfer_16bit - Transfer data by PIO
  *	@dev: device to target
  *	@buf: data buffer
  *	@buflen: buffer length
  *	@rw: read/write
  *
- *	Transfer data from/to the device data register by PIO.
+ *	Transfer data from/to the device data register by PIO using only
+ *	16-bit transfers.
  *
  *	LOCKING:
  *	Inherited from caller.
@@ -687,7 +688,7 @@ static inline void ata_tf_to_host(struct ata_port *ap,
  *	RETURNS:
  *	Bytes consumed.
  */
-unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
+unsigned int ata_sff_data_xfer_16bit(struct ata_device *dev, unsigned char *buf,
 			       unsigned int buflen, int rw)
 {
 	struct ata_port *ap = dev->link->ap;
@@ -719,6 +720,51 @@ unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
 }
 
 /**
+ *	ata_sff_data_xfer - Transfer data by PIO
+ *	@dev: device to target
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@rw: read/write
+ *
+ *	Transfer data from/to the device data register by PIO.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	Bytes consumed.
+ */
+unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
+			       unsigned int buflen, int rw)
+{
+	struct ata_port *ap = dev->link->ap;
+	void __iomem *data_addr = ap->ioaddr.data_addr;
+	unsigned int words = buflen >> 2;
+	unsigned int slop = buflen & 3;
+
+	/* Transfer multiple of 4 bytes */
+	if (rw == READ)
+		ioread32_rep(data_addr, buf, words);
+	else
+		iowrite32_rep(data_addr, buf, words);
+
+	/* Transfer trailing 1 byte, if any. */
+	if (unlikely(slop)) {
+		__le32 pad = 0;
+		if (rw == READ) {
+			pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
+			memcpy(buf + buflen - slop, &pad, slop);
+		} else {
+			memcpy(&pad, buf + buflen - slop, slop);
+			iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
+		}
+		buflen += 4 - slop;
+	}
+
+	return buflen;
+}
+
+/**
  *	ata_sff_data_xfer_noirq - Transfer data by PIO
  *	@dev: device to target
  *	@buf: data buffer
@@ -748,6 +794,36 @@ unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev, unsigned char *buf,
 }
 
 /**
+ *	ata_sff_data_xfer_noirq_16bit - Transfer data by PIO
+ *	@dev: device to target
+ *	@buf: data buffer
+ *	@buflen: buffer length
+ *	@rw: read/write
+ *
+ *	Transfer data from/to the device data register by PIO. Do the
+ *	transfer with interrupts disabled using only 16-bit transfers.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ *
+ *	RETURNS:
+ *	Bytes consumed.
+ */
+unsigned int ata_sff_data_xfer_noirq_16bit(struct ata_device *dev,
+				     unsigned char *buf,
+				     unsigned int buflen, int rw)
+{
+	unsigned long flags;
+	unsigned int consumed;
+
+	local_irq_save(flags);
+	consumed = ata_sff_data_xfer_16bit(dev, buf, buflen, rw);
+	local_irq_restore(flags);
+
+	return consumed;
+}
+
+/**
  *	ata_pio_sector - Transfer a sector of data.
  *	@qc: Command on going
  *
@@ -2827,7 +2903,9 @@ EXPORT_SYMBOL_GPL(ata_sff_tf_load);
 EXPORT_SYMBOL_GPL(ata_sff_tf_read);
 EXPORT_SYMBOL_GPL(ata_sff_exec_command);
 EXPORT_SYMBOL_GPL(ata_sff_data_xfer);
+EXPORT_SYMBOL_GPL(ata_sff_data_xfer_16bit);
 EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq);
+EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq_16bit);
 EXPORT_SYMBOL_GPL(ata_sff_irq_on);
 EXPORT_SYMBOL_GPL(ata_sff_irq_clear);
 EXPORT_SYMBOL_GPL(ata_sff_hsm_move);
diff --git a/drivers/ata/pata_at32.c b/drivers/ata/pata_at32.c
index 82fb6e2..aa90b19 100644
--- a/drivers/ata/pata_at32.c
+++ b/drivers/ata/pata_at32.c
@@ -173,6 +173,7 @@ static struct scsi_host_template at32_sht = {
 static struct ata_port_operations at32_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	.cable_detect		= ata_cable_40wire,
+	.sff_data_xfer		= ata_sff_data_xfer_16bit,
 	.set_piomode		= pata_at32_set_piomode,
 };
 
diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c
index cf9e984..a1f09ca 100644
--- a/drivers/ata/pata_icside.c
+++ b/drivers/ata/pata_icside.c
@@ -336,7 +336,7 @@ static struct ata_port_operations pata_icside_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 	/* no need to build any PRD tables for DMA */
 	.qc_prep		= ata_noop_qc_prep,
-	.sff_data_xfer		= ata_sff_data_xfer_noirq,
+	.sff_data_xfer		= ata_sff_data_xfer_noirq_16bit,
 	.bmdma_setup		= pata_icside_bmdma_setup,
 	.bmdma_start		= pata_icside_bmdma_start,
 	.bmdma_stop		= pata_icside_bmdma_stop,
diff --git a/drivers/ata/pata_isapnp.c b/drivers/ata/pata_isapnp.c
index 6a111ba..6f38f76 100644
--- a/drivers/ata/pata_isapnp.c
+++ b/drivers/ata/pata_isapnp.c
@@ -26,6 +26,7 @@ static struct scsi_host_template isapnp_sht = {
 static struct ata_port_operations isapnp_port_ops = {
 	.inherits	= &ata_sff_port_ops,
 	.cable_detect	= ata_cable_40wire,
+	.sff_data_xfer	= ata_sff_data_xfer_16bit,
 };
 
 /**
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index bc037ff..d919934 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -226,12 +226,12 @@ static const struct ata_port_operations legacy_base_port_ops = {
 
 static struct ata_port_operations simple_port_ops = {
 	.inherits	= &legacy_base_port_ops,
-	.sff_data_xfer	= ata_sff_data_xfer_noirq,
+	.sff_data_xfer	= ata_sff_data_xfer_noirq_16bit,
 };
 
 static struct ata_port_operations legacy_port_ops = {
 	.inherits	= &legacy_base_port_ops,
-	.sff_data_xfer	= ata_sff_data_xfer_noirq,
+	.sff_data_xfer	= ata_sff_data_xfer_noirq_16bit,
 	.set_mode	= legacy_set_mode,
 };
 
@@ -286,39 +286,18 @@ static void pdc20230_set_piomode(struct ata_port *ap, struct ata_device *adev)
 static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw)
 {
-	if (ata_id_has_dword_io(dev->id)) {
-		struct ata_port *ap = dev->link->ap;
-		int slop = buflen & 3;
-		unsigned long flags;
+	unsigned long flags;
 
-		local_irq_save(flags);
+	local_irq_save(flags);
 
-		/* Perform the 32bit I/O synchronization sequence */
-		ioread8(ap->ioaddr.nsect_addr);
-		ioread8(ap->ioaddr.nsect_addr);
-		ioread8(ap->ioaddr.nsect_addr);
+	/* Perform the 32bit I/O synchronization sequence */
+	ioread8(ap->ioaddr.nsect_addr);
+	ioread8(ap->ioaddr.nsect_addr);
+	ioread8(ap->ioaddr.nsect_addr);
 
-		/* Now the data */
-		if (rw == READ)
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == READ) {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			} else {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			}
-			buflen += 4 - slop;
-		}
-		local_irq_restore(flags);
-	} else
-		buflen = ata_sff_data_xfer_noirq(dev, buf, buflen, rw);
+	buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
 
+	local_irq_restore(flags);
 	return buflen;
 }
 
@@ -733,33 +712,6 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
 	return ata_sff_qc_issue(qc);
 }
 
-static unsigned int vlb32_data_xfer(struct ata_device *adev, unsigned char *buf,
-					unsigned int buflen, int rw)
-{
-	struct ata_port *ap = adev->link->ap;
-	int slop = buflen & 3;
-
-	if (ata_id_has_dword_io(adev->id)) {
-		if (rw == WRITE)
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == WRITE) {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			} else {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			}
-		}
-		return (buflen + 3) & ~3;
-	} else
-		return ata_sff_data_xfer(adev, buf, buflen, rw);
-}
-
 static int qdi_port(struct platform_device *dev,
 			struct legacy_probe *lp, struct legacy_data *ld)
 {
@@ -773,19 +725,19 @@ static struct ata_port_operations qdi6500_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= qdi6500_set_piomode,
 	.qc_issue	= qdi_qc_issue,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static struct ata_port_operations qdi6580_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= qdi6580_set_piomode,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static struct ata_port_operations qdi6580dp_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= qdi6580dp_set_piomode,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static DEFINE_SPINLOCK(winbond_lock);
@@ -856,7 +808,7 @@ static int winbond_port(struct platform_device *dev,
 static struct ata_port_operations winbond_port_ops = {
 	.inherits	= &legacy_base_port_ops,
 	.set_piomode	= winbond_set_piomode,
-	.sff_data_xfer	= vlb32_data_xfer,
+	.sff_data_xfer	= ata_sff_data_xfer,
 };
 
 static struct legacy_controller controllers[] = {
diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index a9e8273..0ab2c8e 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -265,6 +265,7 @@ static struct ata_port_operations mpc52xx_ata_port_ops = {
 	.cable_detect		= ata_cable_40wire,
 	.set_piomode		= mpc52xx_ata_set_piomode,
 	.post_internal_cmd	= ATA_OP_NULL,
+	.sff_data_xfer		= ata_sff_data_xfer_16bit,
 };
 
 static int __devinit
diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index 41b4361..8cb4923 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -133,7 +133,7 @@ static struct scsi_host_template pcmcia_sht = {
 
 static struct ata_port_operations pcmcia_port_ops = {
 	.inherits	= &ata_sff_port_ops,
-	.sff_data_xfer	= ata_sff_data_xfer_noirq,
+	.sff_data_xfer	= ata_sff_data_xfer_noirq_16bit,
 	.cable_detect	= ata_cable_40wire,
 	.set_mode	= pcmcia_set_mode,
 };
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 8f65ad6..3310eb0 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -52,7 +52,7 @@ static struct scsi_host_template pata_platform_sht = {
 
 static struct ata_port_operations pata_platform_port_ops = {
 	.inherits		= &ata_sff_port_ops,
-	.sff_data_xfer		= ata_sff_data_xfer_noirq,
+	.sff_data_xfer		= ata_sff_data_xfer_noirq_16bit,
 	.cable_detect		= ata_cable_unknown,
 	.set_mode		= pata_platform_set_mode,
 	.port_start		= ATA_OP_NULL,
diff --git a/drivers/ata/pata_qdi.c b/drivers/ata/pata_qdi.c
index 63b7a1c..36cc778 100644
--- a/drivers/ata/pata_qdi.c
+++ b/drivers/ata/pata_qdi.c
@@ -124,35 +124,6 @@ static unsigned int qdi_qc_issue(struct ata_queued_cmd *qc)
 	return ata_sff_qc_issue(qc);
 }
 
-static unsigned int qdi_data_xfer(struct ata_device *dev, unsigned char *buf,
-				  unsigned int buflen, int rw)
-{
-	if (ata_id_has_dword_io(dev->id)) {
-		struct ata_port *ap = dev->link->ap;
-		int slop = buflen & 3;
-
-		if (rw == READ)
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == READ) {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			} else {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			}
-			buflen += 4 - slop;
-		}
-	} else
-		buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
-
-	return buflen;
-}
-
 static struct scsi_host_template qdi_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
@@ -160,7 +131,6 @@ static struct scsi_host_template qdi_sht = {
 static struct ata_port_operations qdi6500_port_ops = {
 	.inherits	= &ata_sff_port_ops,
 	.qc_issue	= qdi_qc_issue,
-	.sff_data_xfer	= qdi_data_xfer,
 	.cable_detect	= ata_cable_40wire,
 	.set_piomode	= qdi6500_set_piomode,
 };
diff --git a/drivers/ata/pata_winbond.c b/drivers/ata/pata_winbond.c
index a7606b0..56f5f8e 100644
--- a/drivers/ata/pata_winbond.c
+++ b/drivers/ata/pata_winbond.c
@@ -92,42 +92,12 @@ static void winbond_set_piomode(struct ata_port *ap, struct ata_device *adev)
 }
 
 
-static unsigned int winbond_data_xfer(struct ata_device *dev,
-			unsigned char *buf, unsigned int buflen, int rw)
-{
-	struct ata_port *ap = dev->link->ap;
-	int slop = buflen & 3;
-
-	if (ata_id_has_dword_io(dev->id)) {
-		if (rw == READ)
-			ioread32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-		else
-			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
-
-		if (unlikely(slop)) {
-			__le32 pad;
-			if (rw == READ) {
-				pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-				memcpy(buf + buflen - slop, &pad, slop);
-			} else {
-				memcpy(&pad, buf + buflen - slop, slop);
-				iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
-			}
-			buflen += 4 - slop;
-		}
-	} else
-		buflen = ata_sff_data_xfer(dev, buf, buflen, rw);
-
-	return buflen;
-}
-
 static struct scsi_host_template winbond_sht = {
 	ATA_PIO_SHT(DRV_NAME),
 };
 
 static struct ata_port_operations winbond_port_ops = {
 	.inherits	= &ata_sff_port_ops,
-	.sff_data_xfer	= winbond_data_xfer,
 	.cable_detect	= ata_cable_40wire,
 	.set_piomode	= winbond_set_piomode,
 };
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 06b8033..643328a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1481,8 +1481,12 @@ extern void ata_sff_exec_command(struct ata_port *ap,
 				 const struct ata_taskfile *tf);
 extern unsigned int ata_sff_data_xfer(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
+extern unsigned int ata_sff_data_xfer_16bit(struct ata_device *dev,
+			unsigned char *buf, unsigned int buflen, int rw);
 extern unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
+extern unsigned int ata_sff_data_xfer_noirq_16bit(struct ata_device *dev,
+			unsigned char *buf, unsigned int buflen, int rw);
 extern u8 ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
 extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Tejun Heo]

Robert Hancock wrote:
> Here's my first cut at it. Compile tested only. This sets most controllers
> to use 32-bit PIO except for those which could potentially be on a real ISA
> or other 16-bit bus. It's a bit non-obvious what to do with some of the
> drivers, so input is welcome.
> 
> This implementation doesn't check the ata_id_has_dword_io at all, since it
> would only make a difference on controllers where we don't really want to
> use it anyway.
> 
> It seems like regardless of whether we do 32-bit by default or not the 32-bit
> data_xfer function should be added to libata core as we have several drivers
> which duplicate the same code currently..

Great, just some minor nitpicks as I don't have much idea about 16 bit ones.

> +unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
> +			       unsigned int buflen, int rw)
> +{
> +	struct ata_port *ap = dev->link->ap;
> +	void __iomem *data_addr = ap->ioaddr.data_addr;
> +	unsigned int words = buflen >> 2;

dwords maybe?

> +	unsigned int slop = buflen & 3;
> +
> +	/* Transfer multiple of 4 bytes */
> +	if (rw == READ)
> +		ioread32_rep(data_addr, buf, words);
> +	else
> +		iowrite32_rep(data_addr, buf, words);
> +
> +	/* Transfer trailing 1 byte, if any. */

1byte?

Thanks.

-- 
tejun

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Robert Hancock]

Tejun Heo wrote:
> Robert Hancock wrote:
>> Here's my first cut at it. Compile tested only. This sets most controllers
>> to use 32-bit PIO except for those which could potentially be on a real ISA
>> or other 16-bit bus. It's a bit non-obvious what to do with some of the
>> drivers, so input is welcome.
>>
>> This implementation doesn't check the ata_id_has_dword_io at all, since it
>> would only make a difference on controllers where we don't really want to
>> use it anyway.
>>
>> It seems like regardless of whether we do 32-bit by default or not the 32-bit
>> data_xfer function should be added to libata core as we have several drivers
>> which duplicate the same code currently..
> 
> Great, just some minor nitpicks as I don't have much idea about 16 bit ones.
> 
>> +unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
>> +			       unsigned int buflen, int rw)
>> +{
>> +	struct ata_port *ap = dev->link->ap;
>> +	void __iomem *data_addr = ap->ioaddr.data_addr;
>> +	unsigned int words = buflen >> 2;
> 
> dwords maybe?
> 
>> +	unsigned int slop = buflen & 3;
>> +
>> +	/* Transfer multiple of 4 bytes */
>> +	if (rw == READ)
>> +		ioread32_rep(data_addr, buf, words);
>> +	else
>> +		iowrite32_rep(data_addr, buf, words);
>> +
>> +	/* Transfer trailing 1 byte, if any. */
> 
> 1byte?

Yeah, those are both leftovers from the 16-bit code. I'll fix them up in 
the next version, if the approach looks good..

> 
> Thanks.
> 

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Alan Cox]

> Here's my first cut at it. Compile tested only. This sets most controllers
> to use 32-bit PIO except for those which could potentially be on a real ISA
> or other 16-bit bus. It's a bit non-obvious what to do with some of the
> drivers, so input is welcome.

Only thing I would do differently and definitely want to do differently
is the assumption that we just turn on 32bit and see what occurs. We need
to read/test/change controller by controller over to 32bit not just hope.

Alan

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Sergei Shtylyov]

Hello, I wrote:

>>>>>> supported. I couldn't track down where that bit was actually 
>>>>>> defined in the first place, all the way back to ATA-1 it seems to 
>>>>>> be indicated as reserved. Actually, I'm not sure why the drive 
>>>>>> cares in the first place, it would seem like a pure host 
>>>>>> controller issue..
>>>>>>             
>>>>> It goes back before IDE into the depths of the original compaq 
>>>>> spec. When
>>>>> you have a device wired basically directly to the ISA bus 
>>>>> (original IDE)
>>>>>         
>>>>    ISA has only 8/16-bit data bus, so it could not have mattered 
>>>> there...     
>>>
>>> Depends what a 32bit I/O looks like on the 16bit bus - timing wise.
>>>   
>>
>>   Two 16-bit reads at addresses 0x1x0 and 0x1x2 with the programmed 
>> recovery time, IIRC... It's just occured to me that in case of the 
>> 16-bit bus it should be how the drive treated the accesses at address 
>> 0x1x2 with IOCS16 asserted that could have mattered. If it honored 
>> them, 32-bit I/O could have worked even on a dumb ISA "controller", 
>> if not -- no way (unless you really had *something* between the ISA 
>> and the IDE cable).
>
>    Oh, -IOCS16 is driven by device, not host. I give up then. :-)
>

   OTOH, it definitely could work if the drive asserted it for the I/O 
port 0x1x2 at least for the data transfer phase (and probably even if it 
always asserted -IOCS16 for this address).
That pre-historic word indeed could have made sense then...

MBR, Sergei

Re: Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:
>> +unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
>> +			       unsigned int buflen, int rw)
>> +{
>> +	struct ata_port *ap = dev->link->ap;
>> +	void __iomem *data_addr = ap->ioaddr.data_addr;
>> +	unsigned int words = buflen >> 2;
>>     
>
> dwords maybe?
>   

   Isn't word 32-bit in the 32-bit systems? We're not on 80[12]86 
(despite Intel/MS preference of calling 32-bit memory cells DWORDS)...

MBR, Sergei

Re: [Ksummit-2008-discuss] Kernel Summit request for Discussion of future of ATA (libata) and IDE

[Tejun Heo]

Sergei Shtylyov wrote:
> Hello.
> 
> Tejun Heo wrote:
>>> +unsigned int ata_sff_data_xfer(struct ata_device *dev, unsigned char *buf,
>>> +			       unsigned int buflen, int rw)
>>> +{
>>> +	struct ata_port *ap = dev->link->ap;
>>> +	void __iomem *data_addr = ap->ioaddr.data_addr;
>>> +	unsigned int words = buflen >> 2;
>>>     
>> dwords maybe?
>>   
> 
>    Isn't word 32-bit in the 32-bit systems? We're not on 80[12]86 
> (despite Intel/MS preference of calling 32-bit memory cells DWORDS)...

Well, as most of machines are 64-bit these days, discussing whether a
word means 16 or 32 bits is kind of moot.  I usually just think byte,
word, dword, qword and and libata and pci assume that too -
ata_id_has_dword_io() and PCI configuration accessors.  It's ultimately
a peripheral issue either way tho.

Thanks.

-- 
tejun