[PATCH] libata: Add MMIO support to pata_sil680

[PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

This patch adds MMIO support to the pata_sil680 for taskfile IOs,
based on what the old siimage does.

I haven't bothered changing the chip setup stuff from PCI config
cycles to MMIO though (siimage does it), I don't think it matters,
I've only adapted it to use MMIO for taskfile accesses.

I've tested it on a Cell blade and it seems to work fine.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

 drivers/ata/pata_sil680.c |   70 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 8 deletions(-)

Index: linux-cell/drivers/ata/pata_sil680.c
===================================================================
--- linux-cell.orig/drivers/ata/pata_sil680.c	2007-05-15 15:19:08.000000000 +1000
+++ linux-cell/drivers/ata/pata_sil680.c	2007-05-15 16:06:56.000000000 +1000
@@ -35,6 +35,8 @@
 #define DRV_NAME "pata_sil680"
 #define DRV_VERSION "0.4.6"
 
+#define SIL680_MMIO_BAR		5
+
 /**
  *	sil680_selreg		-	return register base
  *	@hwif: interface
@@ -278,7 +280,7 @@ static struct ata_port_operations sil680
  *	Returns the final clock settings.
  */
 
-static u8 sil680_init_chip(struct pci_dev *pdev)
+static u8 sil680_init_chip(struct pci_dev *pdev, int *try_mmio)
 {
 	u32 class_rev	= 0;
 	u8 tmpbyte	= 0;
@@ -293,8 +295,10 @@ static u8 sil680_init_chip(struct pci_de
 
 	pci_read_config_byte(pdev, 0x8A, &tmpbyte);
 
-	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
-			tmpbyte & 1, tmpbyte & 0x30);
+	dev_dbg(&pdev->dev, "sil680: BA5_EN = %d clock = %02X\n",
+		tmpbyte & 1, tmpbyte & 0x30);
+
+	*try_mmio = (tmpbyte & 1) || pci_resource_start(pdev, 5);
 
 	switch(tmpbyte & 0x30) {
 		case 0x00:
@@ -315,8 +319,8 @@ static u8 sil680_init_chip(struct pci_de
 	}
 
 	pci_read_config_byte(pdev,   0x8A, &tmpbyte);
-	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
-			tmpbyte & 1, tmpbyte & 0x30);
+	dev_dbg(&pdev->dev, "sil680: BA5_EN = %d clock = %02X\n",
+		tmpbyte & 1, tmpbyte & 0x30);
 
 	pci_write_config_byte(pdev,  0xA1, 0x72);
 	pci_write_config_word(pdev,  0xA2, 0x328A);
@@ -339,7 +343,8 @@ static u8 sil680_init_chip(struct pci_de
 	return tmpbyte & 0x30;
 }
 
-static int sil680_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
+static int __devinit sil680_init_one(struct pci_dev *pdev,
+				     const struct pci_device_id *id)
 {
 	static const struct ata_port_info info = {
 		.sht = &sil680_sht,
@@ -359,18 +364,67 @@ static int sil680_init_one(struct pci_de
 	};
 	const struct ata_port_info *ppi[] = { &info, NULL };
 	static int printed_version;
+	struct ata_host *host;
+	void __iomem *mmio_base;
+	int rc, try_mmio;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
-	switch(sil680_init_chip(pdev))
-	{
+	switch(sil680_init_chip(pdev, &try_mmio)) {
 		case 0:
 			ppi[0] = &info_slow;
 			break;
 		case 0x30:
 			return -ENODEV;
 	}
+
+	if (!try_mmio)
+		goto use_pio;
+
+	/* Try to acquire MMIO resources and fallback to PIO if
+	 * that fails
+	 */
+	rc = pcim_enable_device(pdev);
+	if (rc)
+		return rc;
+	rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR, DRV_NAME);
+	if (rc)
+		goto use_pio;
+
+	/* Allocate host and set it up */
+	host = ata_host_alloc_pinfo(&pdev->dev, ppi, 2);
+	if (!host)
+		return -ENOMEM;
+	host->iomap = pcim_iomap_table(pdev);
+
+	/* Setup DMA masks */
+	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		return rc;
+	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		return rc;
+	pci_set_master(pdev);
+
+	/* Get MMIO base and initialize port addresses */
+	mmio_base = host->iomap[SIL680_MMIO_BAR];
+	host->ports[0]->ioaddr.bmdma_addr = mmio_base + 0x00;
+	host->ports[0]->ioaddr.cmd_addr = mmio_base + 0x80;
+	host->ports[0]->ioaddr.ctl_addr = mmio_base + 0x8a;
+	host->ports[0]->ioaddr.altstatus_addr = mmio_base + 0x8a;
+	ata_std_ports(&host->ports[0]->ioaddr);
+	host->ports[1]->ioaddr.bmdma_addr = mmio_base + 0x08;
+	host->ports[1]->ioaddr.cmd_addr = mmio_base + 0xc0;
+	host->ports[1]->ioaddr.ctl_addr = mmio_base + 0xca;
+	host->ports[1]->ioaddr.altstatus_addr = mmio_base + 0xca;
+	ata_std_ports(&host->ports[1]->ioaddr);
+
+	/* Register & activate */
+	return ata_host_activate(host, pdev->irq, ata_interrupt, IRQF_SHARED,
+				 &sil680_sht);
+
+use_pio:
 	return ata_pci_init_one(pdev, ppi);
 }
 

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Tue, 2007-05-15 at 16:12 +1000, Benjamin Herrenschmidt wrote:
> This patch adds MMIO support to the pata_sil680 for taskfile IOs,
> based on what the old siimage does.
> 
> I haven't bothered changing the chip setup stuff from PCI config
> cycles to MMIO though (siimage does it), I don't think it matters,
> I've only adapted it to use MMIO for taskfile accesses.
> 
> I've tested it on a Cell blade and it seems to work fine.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

That one is the right one. I think the only diffs vs. the previous
one that was missing a quilt ref are I wasn't initializing altstatus
address and I turned some printk's into dev_dbg() as that's really
what they are.

Cheers,
Ben.

>  drivers/ata/pata_sil680.c |   70 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 8 deletions(-)
> 
> Index: linux-cell/drivers/ata/pata_sil680.c
> ===================================================================
> --- linux-cell.orig/drivers/ata/pata_sil680.c	2007-05-15 15:19:08.000000000 +1000
> +++ linux-cell/drivers/ata/pata_sil680.c	2007-05-15 16:06:56.000000000 +1000
> @@ -35,6 +35,8 @@
>  #define DRV_NAME "pata_sil680"
>  #define DRV_VERSION "0.4.6"
>  
> +#define SIL680_MMIO_BAR		5
> +
>  /**
>   *	sil680_selreg		-	return register base
>   *	@hwif: interface
> @@ -278,7 +280,7 @@ static struct ata_port_operations sil680
>   *	Returns the final clock settings.
>   */
>  
> -static u8 sil680_init_chip(struct pci_dev *pdev)
> +static u8 sil680_init_chip(struct pci_dev *pdev, int *try_mmio)
>  {
>  	u32 class_rev	= 0;
>  	u8 tmpbyte	= 0;
> @@ -293,8 +295,10 @@ static u8 sil680_init_chip(struct pci_de
>  
>  	pci_read_config_byte(pdev, 0x8A, &tmpbyte);
>  
> -	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
> -			tmpbyte & 1, tmpbyte & 0x30);
> +	dev_dbg(&pdev->dev, "sil680: BA5_EN = %d clock = %02X\n",
> +		tmpbyte & 1, tmpbyte & 0x30);
> +
> +	*try_mmio = (tmpbyte & 1) || pci_resource_start(pdev, 5);
>  
>  	switch(tmpbyte & 0x30) {
>  		case 0x00:
> @@ -315,8 +319,8 @@ static u8 sil680_init_chip(struct pci_de
>  	}
>  
>  	pci_read_config_byte(pdev,   0x8A, &tmpbyte);
> -	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
> -			tmpbyte & 1, tmpbyte & 0x30);
> +	dev_dbg(&pdev->dev, "sil680: BA5_EN = %d clock = %02X\n",
> +		tmpbyte & 1, tmpbyte & 0x30);
>  
>  	pci_write_config_byte(pdev,  0xA1, 0x72);
>  	pci_write_config_word(pdev,  0xA2, 0x328A);
> @@ -339,7 +343,8 @@ static u8 sil680_init_chip(struct pci_de
>  	return tmpbyte & 0x30;
>  }
>  
> -static int sil680_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
> +static int __devinit sil680_init_one(struct pci_dev *pdev,
> +				     const struct pci_device_id *id)
>  {
>  	static const struct ata_port_info info = {
>  		.sht = &sil680_sht,
> @@ -359,18 +364,67 @@ static int sil680_init_one(struct pci_de
>  	};
>  	const struct ata_port_info *ppi[] = { &info, NULL };
>  	static int printed_version;
> +	struct ata_host *host;
> +	void __iomem *mmio_base;
> +	int rc, try_mmio;
>  
>  	if (!printed_version++)
>  		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
>  
> -	switch(sil680_init_chip(pdev))
> -	{
> +	switch(sil680_init_chip(pdev, &try_mmio)) {
>  		case 0:
>  			ppi[0] = &info_slow;
>  			break;
>  		case 0x30:
>  			return -ENODEV;
>  	}
> +
> +	if (!try_mmio)
> +		goto use_pio;
> +
> +	/* Try to acquire MMIO resources and fallback to PIO if
> +	 * that fails
> +	 */
> +	rc = pcim_enable_device(pdev);
> +	if (rc)
> +		return rc;
> +	rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR, DRV_NAME);
> +	if (rc)
> +		goto use_pio;
> +
> +	/* Allocate host and set it up */
> +	host = ata_host_alloc_pinfo(&pdev->dev, ppi, 2);
> +	if (!host)
> +		return -ENOMEM;
> +	host->iomap = pcim_iomap_table(pdev);
> +
> +	/* Setup DMA masks */
> +	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
> +	if (rc)
> +		return rc;
> +	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
> +	if (rc)
> +		return rc;
> +	pci_set_master(pdev);
> +
> +	/* Get MMIO base and initialize port addresses */
> +	mmio_base = host->iomap[SIL680_MMIO_BAR];
> +	host->ports[0]->ioaddr.bmdma_addr = mmio_base + 0x00;
> +	host->ports[0]->ioaddr.cmd_addr = mmio_base + 0x80;
> +	host->ports[0]->ioaddr.ctl_addr = mmio_base + 0x8a;
> +	host->ports[0]->ioaddr.altstatus_addr = mmio_base + 0x8a;
> +	ata_std_ports(&host->ports[0]->ioaddr);
> +	host->ports[1]->ioaddr.bmdma_addr = mmio_base + 0x08;
> +	host->ports[1]->ioaddr.cmd_addr = mmio_base + 0xc0;
> +	host->ports[1]->ioaddr.ctl_addr = mmio_base + 0xca;
> +	host->ports[1]->ioaddr.altstatus_addr = mmio_base + 0xca;
> +	ata_std_ports(&host->ports[1]->ioaddr);
> +
> +	/* Register & activate */
> +	return ata_host_activate(host, pdev->irq, ata_interrupt, IRQF_SHARED,
> +				 &sil680_sht);
> +
> +use_pio:
>  	return ata_pci_init_one(pdev, ppi);
>  }
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Tue, 15 May 2007 16:14:57 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> That one is the right one. I think the only diffs vs. the previous
> one that was missing a quilt ref are I wasn't initializing altstatus
> address and I turned some printk's into dev_dbg() as that's really
> what they are.

I'm going to have to NAK this on further review as it will break SRST
handling in some cases. Until ata_bus_softreset has been fixed (see
FIXME: entries) we shouldn't push this patch mainstream, or if we have
then someone should fix the software reset posting as SRST only has an
effect if the timing is honoured.

Alan

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Wed, 2007-05-23 at 14:42 +0100, Alan Cox wrote:

> I'm going to have to NAK this on further review as it will break SRST
> handling in some cases. Until ata_bus_softreset has been fixed (see
> FIXME: entries) we shouldn't push this patch mainstream, or if we have
> then someone should fix the software reset posting as SRST only has an
> effect if the timing is honoured. 

Hrm... so all MMIO drivers (PDC, etc...) are broken you think ? 

What would be the solution there ? A read to flush the posted write to
the control register would work in the middle of a soft reset ? If yes,
what register do you suggest we read ?

Ben.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

> Hrm... so all MMIO drivers (PDC, etc...) are broken you think ? 

If they use the SRST and the chipset does more than tiny amounts of mmio
posting then yes.

> What would be the solution there ? A read to flush the posted write to
> the control register would work in the middle of a soft reset ? If yes,
> what register do you suggest we read ?

Anything non taskfile, which is tricky to do arbitarily for all
controllers - this is why I didn't just stuff in a simple fix and post it.

For BMDMA controllers most of them have a load of other MMIO registers
we can read (eg the SIL680 has the PRD table address you can read
harmlessly), once we get beyond SFF BMDMA however it will be controller
dependant and we probably have to actually specify what register is used
for dummy posting reads when we set up the device. For I/O space we don't
get posting so life is easy.

Jeff ?

Alan

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Thu, 2007-05-24 at 00:31 +0100, Alan Cox wrote:
> 
> Anything non taskfile, which is tricky to do arbitarily for all
> controllers - this is why I didn't just stuff in a simple fix and post it.

We might have to provide an optional ->flush() that is device specific ?
Config space access would do the job nicely in most cases though. If
it's really only for SRST which can be slow. If it's for the 400ns of
writing the command, then we have a deeper problem but I would expect
MMIO chipsets to be smarter than that ...

> For BMDMA controllers most of them have a load of other MMIO registers
> we can read (eg the SIL680 has the PRD table address you can read
> harmlessly), once we get beyond SFF BMDMA however it will be controller
> dependant and we probably have to actually specify what register is used
> for dummy posting reads when we set up the device. For I/O space we don't
> get posting so life is easy. 

Ah yes, the PRD table pointer is a good option too... We could introduce
a ->flush() and have a default sff version that reads that pointer ?

Cheers,
Ben.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Thu, May 24, 2007 at 09:43:39AM +1000, Benjamin Herrenschmidt wrote:
> We might have to provide an optional ->flush() that is device specific ?

Probably

> Config space access would do the job nicely in most cases though. If
> it's really only for SRST which can be slow. If it's for the 400ns of
> writing the command, then we have a deeper problem but I would expect
> MMIO chipsets to be smarter than that ...

There are errata for config space and posting in some chipsets  and as you
says its slow. The 400nS command writing bit applies too - they chipsets are
not that smart in my experience.

> Ah yes, the PRD table pointer is a good option too... We could introduce
> a ->flush() and have a default sff version that reads that pointer ?

Normal SFF is I/O cycles so the default SFF one would be NULL which is 
just perfect 8). SIL680/3112 would register a PRD read and the rest can do
whatever their non SFF design does.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

> There are errata for config space and posting in some chipsets  and as you
> says its slow. The 400nS command writing bit applies too - they chipsets are
> not that smart in my experience.

Ok, I've start doing it, however, reset is the only case I've found.

Currently libata does the 400ns thingy in the inline ata_pause() which
unconditionally does ata_altstatus(); ndelay(400); (and thus does it on
PIO, MMIO, anything ...).

Do you want me to change that to use the MMIO flush hook as well ?

If we're going to do that, we probably need to be careful to have
everybody have a properly populated MMIO flush hook as there might be
current drivers (especially SATA) who rely on this. Jeff ?

Cheers,
Ben.
 

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

> Currently libata does the 400ns thingy in the inline ata_pause() which
> unconditionally does ata_altstatus(); ndelay(400); (and thus does it on
> PIO, MMIO, anything ...).

It might actually improve performance for PATA if we did as we'll get rid
of an excess (expensive) PCI read access for non MMIO cases.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Alan Cox wrote:
>> Currently libata does the 400ns thingy in the inline ata_pause() which
>> unconditionally does ata_altstatus(); ndelay(400); (and thus does it on
>> PIO, MMIO, anything ...).
> 
> It might actually improve performance for PATA if we did as we'll get rid
> of an excess (expensive) PCI read access for non MMIO cases.

The spec says you should read AltStatus.  I am definitely not inclined 
to change that part.

I agreed with Ben on IRC that the reset wants fixing.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Thu, 24 May 2007 06:52:13 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Alan Cox wrote:
> >> Currently libata does the 400ns thingy in the inline ata_pause() which
> >> unconditionally does ata_altstatus(); ndelay(400); (and thus does it on
> >> PIO, MMIO, anything ...).
> > 
> > It might actually improve performance for PATA if we did as we'll get rid
> > of an excess (expensive) PCI read access for non MMIO cases.
> 
> The spec says you should read AltStatus.  I am definitely not inclined 
> to change that part.

Which spec says that and where ?

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Alan Cox wrote:
> On Thu, 24 May 2007 06:52:13 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>> Alan Cox wrote:
>>>> Currently libata does the 400ns thingy in the inline ata_pause() which
>>>> unconditionally does ata_altstatus(); ndelay(400); (and thus does it on
>>>> PIO, MMIO, anything ...).
>>> It might actually improve performance for PATA if we did as we'll get rid
>>> of an excess (expensive) PCI read access for non MMIO cases.
>> The spec says you should read AltStatus.  I am definitely not inclined 
>> to change that part.
> 
> Which spec says that and where ?

It's buried in the host state diagrams IIRC.  I'll dig it up tomorrow if 
nobody beats me to it.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Alan Cox wrote:
> On Thu, 24 May 2007 06:52:13 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>> Alan Cox wrote:
>>>> Currently libata does the 400ns thingy in the inline ata_pause() which
>>>> unconditionally does ata_altstatus(); ndelay(400); (and thus does it on
>>>> PIO, MMIO, anything ...).
>>> It might actually improve performance for PATA if we did as we'll get rid
>>> of an excess (expensive) PCI read access for non MMIO cases.
>> The spec says you should read AltStatus.  I am definitely not inclined 
>> to change that part.
> 
> Which spec says that and where ?

Found it!  It was in the spec I consider more authoritative than any 
official ATA spec:  Hale Landis' ATADRVR (http://www.ata-atapi.com/):


// This macro provides a small delay that is used in several
// places in the ATA command protocols:
// 1) It is recommended that the host delay 400ns after
//    writing the command register.
// 2) ATA-4 has added a new requirement that the host delay
//    400ns if the DEV bit in the Device/Head register is
//    changed.  This was not recommended or required in ATA-1,
//    ATA-2 or ATA-3.  This is the easy way to do that since it
//    works in all PIO modes.
// 3) ATA-4 has added another new requirement that the host delay
//    after the last word of a data transfer before checking the
//    status register.  This was not recommended or required in
//    ATA-1, ATA-2 or ATA-3.  This is the easy to do that since it
//    works in all PIO modes.

#define DELAY400NS  { pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT );  \
                       pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT ); }


libata was originally written by following ATADRVR IO-for-IO.  You can 
still see some of the mostly-unmodified bitbang sequences in 
libata-core.c in places like dev-select or dev-chk.  Sometimes you have 
to look pretty hard, since hooks make the bitbang sequences much harder 
to follow.

Obviously libata's "pause" differs from Hale's version a bit, but the 
basic version is sound and works for everybody from ancient PATA PIO to 
modern SATA MMIO.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

> #define DELAY400NS  { pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT );  \
>                        pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT ); }
> 
> 

Totally unrelated. Hal is using the cycle time of the four I/O reads to
do the 400nS delay. Its a neat way to do the delay on boxes without
modern CPUs and nice timing features and perhaps one we should use on
those boxes but its not relevant to the question of how you post an MMIO
command write.

Alan

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Alan Cox wrote:
>> #define DELAY400NS  { pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT );  \
>>                        pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT ); }
>>
>>
> 
> Totally unrelated. Hal is using the cycle time of the four I/O reads to
> do the 400nS delay. Its a neat way to do the delay on boxes without
> modern CPUs and nice timing features and perhaps one we should use on
> those boxes but its not relevant to the question of how you post an MMIO
> command write.

It illustrates (as well as our experience to date) that AltStatus use 
for the delay is just fine.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Thu, 24 May 2007 20:51:26 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Cox wrote:
> >> #define DELAY400NS  { pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT );  \
> >>                        pio_inbyte( CB_ASTAT ); pio_inbyte( CB_ASTAT ); }
> >>
> >>
> > 
> > Totally unrelated. Hal is using the cycle time of the four I/O reads to
> > do the 400nS delay. Its a neat way to do the delay on boxes without
> > modern CPUs and nice timing features and perhaps one we should use on
> > those boxes but its not relevant to the question of how you post an MMIO
> > command write.
> 
> It illustrates (as well as our experience to date) that AltStatus use 
> for the delay is just fine.

Correct, but it is also extremely slow. No point discussing fast paths
for odd if() tests through the code when you burn 100nS unneccessarily
every time you issue a command via PIO is there.

Alan

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Alan Cox wrote:
> Correct, but it is also extremely slow. No point discussing fast paths
> for odd if() tests through the code when you burn 100nS unneccessarily
> every time you issue a command via PIO is there.

BTW if you wanna start PIO speed tuning, ISTR the device-select code 
does not cache selections.  We always unconditionally select a device 
before a command in ata_qc_issue_prot(), IIRC.

ISTR for some cases this was intentional (following ATADRVR) but I bet 
adding code to -not- select a device, if it is already selected, would 
speed things up on slow PATA machines.

ata_qc_issue_prot() and ata_dev_select() would be starting points if 
you're interested.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

MMIO has always been like this (libata-core.c):

         /* software reset.  causes dev0 to be selected */
         iowrite8(ap->ctl, ioaddr->ctl_addr);
         udelay(20);     /* FIXME: flush */
         iowrite8(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
         udelay(20);     /* FIXME: flush */
         iowrite8(ap->ctl, ioaddr->ctl_addr);

The problem is mainly in finding registers you can read without side 
effects or confusing the controller which might also be doing in-silicon 
reset procedures.

The above is not correct per PCI posting, hence the FIXME, but it works 
so far for all tested cases.

The timing is irrelevant for SATA (this merely triggers a FIS to be 
sent).  Most of PATA is not MMIO, so this problem is avoided.  Thus the 
potential affected cases are PATA MMIO, which is largely PDC and SiI, IIRC.

Ben's patch got merged because it does not change the status quo.  This 
warrants looking at -- its a core problem as shown above -- but it 
requires thinking and testing on a problematic platform :)  Maybe we can 
read a PCI config register or innocuous vendor-specific register, for 
the flush, on the few cases where it matters.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Thu, May 24, 2007 at 02:02:11AM -0400, Jeff Garzik wrote:
> Ben's patch got merged because it does not change the status quo.  This 

Yes it does - SRST timing is now wrong for the SIL680. Thats PATA and now
PATA + MMIO so the problem case.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Alan Cox wrote:
> On Thu, May 24, 2007 at 02:02:11AM -0400, Jeff Garzik wrote:
>> Ben's patch got merged because it does not change the status quo.  This 
> 
> Yes it does - Thats PATA and now
> PATA + MMIO so the problem case.

hmmm, true.

	Jeff

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Thu, 2007-05-24 at 05:55 -0400, Jeff Garzik wrote:
> Alan Cox wrote:
> > On Thu, May 24, 2007 at 02:02:11AM -0400, Jeff Garzik wrote:
> >> Ben's patch got merged because it does not change the status quo.  This 
> > 
> > Yes it does - Thats PATA and now
> > PATA + MMIO so the problem case.
> 
> hmmm, true.

I think Jeff is right. I was not completley sure about the 400ns case in
ata_pause but I think it's a good idea to use mmio_flush hook there.

The only thing that I'm wondering about a bit is that ata_pause so far
uses read of altstatus which _is_ a taskfile register. It's my
understanding that we should avoid doing so in that case.

So my patch will use the new mmio_flush hook instead, however, that
means that PIO and SATA controllers will have NULL there, thus no read.
We will have to be careful just in case something actually manages to
regress because of the loss of that read. (with IDE, anything is
possible !)


Cheers,
Ben.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Mark Lord]

Benjamin Herrenschmidt wrote:
>
> The only thing that I'm wondering about a bit is that ata_pause so far
> uses read of altstatus which _is_ a taskfile register. It's my
> understanding that we should avoid doing so in that case.

Technically, altstatus is in the control block rather than the command block,
so it doesn't really always behave the same as the regular "taskfile" regs.

Cheers

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Thu, 2007-05-24 at 16:56 -0400, Mark Lord wrote:
> Benjamin Herrenschmidt wrote:
> >
> > The only thing that I'm wondering about a bit is that ata_pause so far
> > uses read of altstatus which _is_ a taskfile register. It's my
> > understanding that we should avoid doing so in that case.
> 
> Technically, altstatus is in the control block rather than the command block,
> so it doesn't really always behave the same as the regular "taskfile" regs.

So would altstatus be good enough to flush SRST toggles as well or do
you expect problems ?

Ben.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Mark Lord]

Benjamin Herrenschmidt wrote:
> On Thu, 2007-05-24 at 16:56 -0400, Mark Lord wrote:
>> Benjamin Herrenschmidt wrote:
>>> The only thing that I'm wondering about a bit is that ata_pause so far
>>> uses read of altstatus which _is_ a taskfile register. It's my
>>> understanding that we should avoid doing so in that case.
>> Technically, altstatus is in the control block rather than the command block,
>> so it doesn't really always behave the same as the regular "taskfile" regs.
> 
> So would altstatus be good enough to flush SRST toggles as well or do
> you expect problems ?

Dunno, really.  If one didn't have a known better idea, then it's worth trying.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Thu, 2007-05-24 at 02:02 -0400, Jeff Garzik wrote:
> MMIO has always been like this (libata-core.c):
> 
>          /* software reset.  causes dev0 to be selected */
>          iowrite8(ap->ctl, ioaddr->ctl_addr);
>          udelay(20);     /* FIXME: flush */
>          iowrite8(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
>          udelay(20);     /* FIXME: flush */
>          iowrite8(ap->ctl, ioaddr->ctl_addr);

Yup, one of the things my patch adds is a mmio_flush optional hook that
I added in there just before the udelay's.

> The problem is mainly in finding registers you can read without side 
> effects or confusing the controller which might also be doing in-silicon 
> reset procedures.
> 
> The above is not correct per PCI posting, hence the FIXME, but it works 
> so far for all tested cases.

Yes. For most MMIO controllers, reading the PRD table base address is
probably a good enough way to acheive this.

> The timing is irrelevant for SATA (this merely triggers a FIS to be 
> sent).  Most of PATA is not MMIO, so this problem is avoided.  Thus the 
> potential affected cases are PATA MMIO, which is largely PDC and SiI, IIRC.
> 
> Ben's patch got merged because it does not change the status quo.  This 
> warrants looking at -- its a core problem as shown above -- but it 
> requires thinking and testing on a problematic platform :)  Maybe we can 
> read a PCI config register or innocuous vendor-specific register, for 
> the flush, on the few cases where it matters.

I'm adding a hook for that with a generic sff version that controllers
like sil can use that just reads the dbdma prd table pointer.

Now there is still the question of wether the taskfile read for the
400ns delay in ata_pause is correct or not..

Cheers,
Ben.

[PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

This patch adds MMIO support to the pata_sil680 for taskfile IOs,
based on what the old siimage does.

I haven't bothered changing the chip setup stuff from PCI config
cycles to MMIO though (siimage does it), I don't think it matters,
I've only adapted it to use MMIO for taskfile accesses.

I've tested it on a Cell blade and it seems to work fine.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

This version uses a better name "use_ioports" for the fallback
label in the probe code.

 drivers/ata/pata_sil680.c |   70 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 8 deletions(-)

Index: linux-cell/drivers/ata/pata_sil680.c
===================================================================
--- linux-cell.orig/drivers/ata/pata_sil680.c	2007-05-15 16:15:11.000000000 +1000
+++ linux-cell/drivers/ata/pata_sil680.c	2007-05-16 10:15:43.000000000 +1000
@@ -35,6 +35,8 @@
 #define DRV_NAME "pata_sil680"
 #define DRV_VERSION "0.4.6"
 
+#define SIL680_MMIO_BAR		5
+
 /**
  *	sil680_selreg		-	return register base
  *	@hwif: interface
@@ -278,7 +280,7 @@ static struct ata_port_operations sil680
  *	Returns the final clock settings.
  */
 
-static u8 sil680_init_chip(struct pci_dev *pdev)
+static u8 sil680_init_chip(struct pci_dev *pdev, int *try_mmio)
 {
 	u32 class_rev	= 0;
 	u8 tmpbyte	= 0;
@@ -293,8 +295,10 @@ static u8 sil680_init_chip(struct pci_de
 
 	pci_read_config_byte(pdev, 0x8A, &tmpbyte);
 
-	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
-			tmpbyte & 1, tmpbyte & 0x30);
+	dev_dbg(&pdev->dev, "sil680: BA5_EN = %d clock = %02X\n",
+		tmpbyte & 1, tmpbyte & 0x30);
+
+	*try_mmio = (tmpbyte & 1) || pci_resource_start(pdev, 5);
 
 	switch(tmpbyte & 0x30) {
 		case 0x00:
@@ -315,8 +319,8 @@ static u8 sil680_init_chip(struct pci_de
 	}
 
 	pci_read_config_byte(pdev,   0x8A, &tmpbyte);
-	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
-			tmpbyte & 1, tmpbyte & 0x30);
+	dev_dbg(&pdev->dev, "sil680: BA5_EN = %d clock = %02X\n",
+		tmpbyte & 1, tmpbyte & 0x30);
 
 	pci_write_config_byte(pdev,  0xA1, 0x72);
 	pci_write_config_word(pdev,  0xA2, 0x328A);
@@ -339,7 +343,8 @@ static u8 sil680_init_chip(struct pci_de
 	return tmpbyte & 0x30;
 }
 
-static int sil680_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
+static int __devinit sil680_init_one(struct pci_dev *pdev,
+				     const struct pci_device_id *id)
 {
 	static const struct ata_port_info info = {
 		.sht = &sil680_sht,
@@ -359,18 +364,67 @@ static int sil680_init_one(struct pci_de
 	};
 	const struct ata_port_info *ppi[] = { &info, NULL };
 	static int printed_version;
+	struct ata_host *host;
+	void __iomem *mmio_base;
+	int rc, try_mmio;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
-	switch(sil680_init_chip(pdev))
-	{
+	switch(sil680_init_chip(pdev, &try_mmio)) {
 		case 0:
 			ppi[0] = &info_slow;
 			break;
 		case 0x30:
 			return -ENODEV;
 	}
+
+	if (!try_mmio)
+		goto use_ioports;
+
+	/* Try to acquire MMIO resources and fallback to PIO if
+	 * that fails
+	 */
+	rc = pcim_enable_device(pdev);
+	if (rc)
+		return rc;
+	rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR, DRV_NAME);
+	if (rc)
+		goto use_ioports;
+
+	/* Allocate host and set it up */
+	host = ata_host_alloc_pinfo(&pdev->dev, ppi, 2);
+	if (!host)
+		return -ENOMEM;
+	host->iomap = pcim_iomap_table(pdev);
+
+	/* Setup DMA masks */
+	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		return rc;
+	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		return rc;
+	pci_set_master(pdev);
+
+	/* Get MMIO base and initialize port addresses */
+	mmio_base = host->iomap[SIL680_MMIO_BAR];
+	host->ports[0]->ioaddr.bmdma_addr = mmio_base + 0x00;
+	host->ports[0]->ioaddr.cmd_addr = mmio_base + 0x80;
+	host->ports[0]->ioaddr.ctl_addr = mmio_base + 0x8a;
+	host->ports[0]->ioaddr.altstatus_addr = mmio_base + 0x8a;
+	ata_std_ports(&host->ports[0]->ioaddr);
+	host->ports[1]->ioaddr.bmdma_addr = mmio_base + 0x08;
+	host->ports[1]->ioaddr.cmd_addr = mmio_base + 0xc0;
+	host->ports[1]->ioaddr.ctl_addr = mmio_base + 0xca;
+	host->ports[1]->ioaddr.altstatus_addr = mmio_base + 0xca;
+	ata_std_ports(&host->ports[1]->ioaddr);
+
+	/* Register & activate */
+	return ata_host_activate(host, pdev->irq, ata_interrupt, IRQF_SHARED,
+				 &sil680_sht);
+
+use_ioports:
 	return ata_pci_init_one(pdev, ppi);
 }
 

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Wed, May 16, 2007 at 10:21:34AM +1000, Benjamin Herrenschmidt wrote:
> This patch adds MMIO support to the pata_sil680 for taskfile IOs,
> based on what the old siimage does.
> 
> I haven't bothered changing the chip setup stuff from PCI config
> cycles to MMIO though (siimage does it), I don't think it matters,
> I've only adapted it to use MMIO for taskfile accesses.
> 
> I've tested it on a Cell blade and it seems to work fine.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Acked-by: Alan Cox <alan@redhat.com>

All we need now is >4GB support and its perfection ..

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Wed, 2007-05-16 at 08:05 -0400, Alan Cox wrote:
> On Wed, May 16, 2007 at 10:21:34AM +1000, Benjamin Herrenschmidt wrote:
> > This patch adds MMIO support to the pata_sil680 for taskfile IOs,
> > based on what the old siimage does.
> > 
> > I haven't bothered changing the chip setup stuff from PCI config
> > cycles to MMIO though (siimage does it), I don't think it matters,
> > I've only adapted it to use MMIO for taskfile accesses.
> > 
> > I've tested it on a Cell blade and it seems to work fine.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Acked-by: Alan Cox <alan@redhat.com>
> 
> All we need now is >4GB support and its perfection ..

Sorry, I have an iommu :-)

Ben.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
> This patch adds MMIO support to the pata_sil680 for taskfile IOs,
> based on what the old siimage does.
> 
> I haven't bothered changing the chip setup stuff from PCI config
> cycles to MMIO though (siimage does it), I don't think it matters,
> I've only adapted it to use MMIO for taskfile accesses.
> 
> I've tested it on a Cell blade and it seems to work fine.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> This version uses a better name "use_ioports" for the fallback
> label in the probe code.
> 
>  drivers/ata/pata_sil680.c |   70 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 8 deletions(-)

applied to #upstream (queued for 2.6.23)

[PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

This patch adds MMIO support to the pata_sil680 for taskfile IOs,
based on what the old siimage does.

I haven't bothered changing the chip setup stuff from PCI config
cycles to MMIO though (siimage does it), I don't think it matters,
I've only adapted it to use MMIO for taskfile accesses.

I've tested it on a Cell blade and it seems to work fine.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

 drivers/ata/pata_sil680.c |   64 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

Index: linux-cell/drivers/ata/pata_sil680.c
===================================================================
--- linux-cell.orig/drivers/ata/pata_sil680.c	2007-05-15 15:19:08.000000000 +1000
+++ linux-cell/drivers/ata/pata_sil680.c	2007-05-15 15:53:15.000000000 +1000
@@ -35,6 +35,8 @@
 #define DRV_NAME "pata_sil680"
 #define DRV_VERSION "0.4.6"
 
+#define SIL680_MMIO_BAR		5
+
 /**
  *	sil680_selreg		-	return register base
  *	@hwif: interface
@@ -278,7 +280,7 @@ static struct ata_port_operations sil680
  *	Returns the final clock settings.
  */
 
-static u8 sil680_init_chip(struct pci_dev *pdev)
+static u8 sil680_init_chip(struct pci_dev *pdev, int *try_mmio)
 {
 	u32 class_rev	= 0;
 	u8 tmpbyte	= 0;
@@ -296,6 +298,8 @@ static u8 sil680_init_chip(struct pci_de
 	printk(KERN_INFO "sil680: BA5_EN = %d clock = %02X\n",
 			tmpbyte & 1, tmpbyte & 0x30);
 
+	*try_mmio = (tmpbyte & 1) || pci_resource_start(pdev, 5);
+
 	switch(tmpbyte & 0x30) {
 		case 0x00:
 			/* 133 clock attempt to force it on */
@@ -339,7 +343,8 @@ static u8 sil680_init_chip(struct pci_de
 	return tmpbyte & 0x30;
 }
 
-static int sil680_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
+static int __devinit sil680_init_one(struct pci_dev *pdev,
+				     const struct pci_device_id *id)
 {
 	static const struct ata_port_info info = {
 		.sht = &sil680_sht,
@@ -359,18 +364,69 @@ static int sil680_init_one(struct pci_de
 	};
 	const struct ata_port_info *ppi[] = { &info, NULL };
 	static int printed_version;
+	struct ata_host *host;
+	void __iomem *mmio_base;
+	int rc, try_mmio;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
-	switch(sil680_init_chip(pdev))
-	{
+	switch(sil680_init_chip(pdev, &try_mmio)) {
 		case 0:
 			ppi[0] = &info_slow;
 			break;
 		case 0x30:
 			return -ENODEV;
 	}
+
+	if (!try_mmio)
+		goto use_pio;
+
+	/* Try to acquire MMIO resources and fallback to PIO if
+	 * that fails
+	 */
+	rc = pcim_enable_device(pdev);
+	if (rc)
+		return rc;
+	rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR, DRV_NAME);
+	if (rc)
+		goto use_pio;
+
+	/* Allocate host and set it up */
+	host = ata_host_alloc_pinfo(&pdev->dev, ppi, 2);
+	if (!host)
+		return -ENOMEM;
+	host->iomap = pcim_iomap_table(pdev);
+
+	/* Obsolete ? */
+	host->ports[0].flags |= ATA_FLAG_MMIO;
+	host->ports[1].flags |= ATA_FLAG_MMIO;
+
+	/* Setup DMA masks */
+	rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		return rc;
+	rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+	if (rc)
+		return rc;
+	pci_set_master(pdev);
+
+	/* Get MMIO base and initialize port addresses */
+	mmio_base = host->iomap[SIL680_MMIO_BAR];
+	host->ports[0]->ioaddr.bmdma_addr = mmio_base + 0x00;
+	host->ports[0]->ioaddr.cmd_addr = mmio_base + 0x80;
+	host->ports[0]->ioaddr.ctl_addr = mmio_base + 0x8a;
+	ata_std_ports(&host->ports[0]->ioaddr);
+	host->ports[1]->ioaddr.bmdma_addr = mmio_base + 0x08;
+	host->ports[1]->ioaddr.cmd_addr = mmio_base + 0xc0;
+	host->ports[1]->ioaddr.ctl_addr = mmio_base + 0xca;
+	ata_std_ports(&host->ports[1]->ioaddr);
+
+	/* Register & activate */
+	return ata_host_activate(host, pdev->irq, ata_interrupt, IRQF_SHARED,
+				 &sil680_sht);
+
+use_pio:
 	return ata_pci_init_one(pdev, ppi);
 }
 

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Tue, 2007-05-15 at 16:11 +1000, Benjamin Herrenschmidt wrote:
> This patch adds MMIO support to the pata_sil680 for taskfile IOs,
> based on what the old siimage does.
> 
> I haven't bothered changing the chip setup stuff from PCI config
> cycles to MMIO though (siimage does it), I don't think it matters,
> I've only adapted it to use MMIO for taskfile accesses.
> 
> I've tested it on a Cell blade and it seems to work fine.

Oops, forgot a quilt ref ... fixed patch on the way. Sorry.

Ben.

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Alan Cox]

On Tue, May 15, 2007 at 04:11:23PM +1000, Benjamin Herrenschmidt wrote:
> +	if (!try_mmio)
> +		goto use_pio;

Please use a different naming scheme "PIO" means something quite different
in ATA

Rest looks fine although I'd be interested to know if you can measure any
performance change.

Alan

Re: [PATCH] libata: Add MMIO support to pata_sil680

[Benjamin Herrenschmidt]

On Tue, 2007-05-15 at 07:08 -0400, Alan Cox wrote:
> On Tue, May 15, 2007 at 04:11:23PM +1000, Benjamin Herrenschmidt wrote:
> > +	if (!try_mmio)
> > +		goto use_pio;
> 
> Please use a different naming scheme "PIO" means something quite different
> in ATA

Indeed, the naming's a bit confusing, I'll fix that.

> Rest looks fine although I'd be interested to know if you can measure any
> performance change.

Nothing significant here on the box but I had cases of setups where
there was simply no IO space accessible on the PCI bus (one of the
reason for using a controller that does MMIO).

Cheers,
Ben.