[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.

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

[Tim Ellis]

Hi,
This change causes attached drives to no longer be detected and  
function on the PowerPC Buffalo Linkstation machines:

<7>pata_sil680 0000:00:0c.0: version 0.4.8
<6>sil680: 133MHz clock.
<6>scsi0 : pata_sil680
<6>scsi1 : pata_sil680
<6>ata1: PATA max UDMA/133 irq 18
<6>ata2: PATA max UDMA/133 irq 18

If I roll back this driver to before this change with 2.6.24.2 it works:

<7>pata_sil680 0000:00:0c.0: version 0.4.7
<6>sil680: 133MHz clock.
<6>scsi0 : pata_sil680
<6>scsi1 : pata_sil680
<6>ata1: PATA max UDMA/133 cmd 0xbffed0 ctl 0xbffed8 bmdma 0xbffef0  
irq 18
<6>ata2: PATA max UDMA/133 cmd 0xbffee0 ctl 0xbffee8 bmdma 0xbffef8  
irq 18
<6>ata1.00: ATA-6: WDC WD3200JB-00KFA0, 08.05J08, max UDMA/100
<6>ata1.00: 625142448 sectors, multi 0: LBA48
<6>ata1.00: configured for UDMA/100
<5>scsi 0:0:0:0: Direct-Access     ATA      WDC WD3200JB-00K 08.0 PQ:  
0 ANSI: 5
<5>sd 0:0:0:0: [sda] 625142448 512-byte hardware sectors (320073 MB)
<5>sd 0:0:0:0: [sda] Write Protect is off
<7>sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
<5>sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,  
doesn't support DPO or FUA
<5>sd 0:0:0:0: [sda] 625142448 512-byte hardware sectors (320073 MB)
<5>sd 0:0:0:0: [sda] Write Protect is off
<7>sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
<5>sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,  
doesn't support DPO or FUA
<6> sda: sda1 sda2 sda3 sda4
<5>sd 0:0:0:0: [sda] Attached SCSI disk
<5>sd 0:0:0:0: Attached scsi generic sg0 type 0

I have ensured the other sil680 driver is not enabled!

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

[Benjamin Herrenschmidt]

On Tue, 2008-02-12 at 15:58 +0000, Tim Ellis wrote:

> This change causes attached drives to no longer be detected and  
> function on the PowerPC Buffalo Linkstation machines:
> 
> <7>pata_sil680 0000:00:0c.0: version 0.4.8
> <6>sil680: 133MHz clock.
> <6>scsi0 : pata_sil680
> <6>scsi1 : pata_sil680
> <6>ata1: PATA max UDMA/133 irq 18
> <6>ata2: PATA max UDMA/133 irq 18
> 
> If I roll back this driver to before this change with 2.6.24.2 it works:

Hrm... and we need those patches for blades that have a sil680
controller with no working PIO on the PCI host...

That's strange though. Somebody with knowledge of that HW (or specs) who
can spot something ? Could it be an issue with timing ?

I don't have HW access to this machine. If somebody could send one to me
I could do more investigation.

Ben.

> <7>pata_sil680 0000:00:0c.0: version 0.4.7
> <6>sil680: 133MHz clock.
> <6>scsi0 : pata_sil680
> <6>scsi1 : pata_sil680
> <6>ata1: PATA max UDMA/133 cmd 0xbffed0 ctl 0xbffed8 bmdma 0xbffef0  
> irq 18
> <6>ata2: PATA max UDMA/133 cmd 0xbffee0 ctl 0xbffee8 bmdma 0xbffef8  
> irq 18
> <6>ata1.00: ATA-6: WDC WD3200JB-00KFA0, 08.05J08, max UDMA/100
> <6>ata1.00: 625142448 sectors, multi 0: LBA48
> <6>ata1.00: configured for UDMA/100
> <5>scsi 0:0:0:0: Direct-Access     ATA      WDC WD3200JB-00K 08.0 PQ:  
> 0 ANSI: 5
> <5>sd 0:0:0:0: [sda] 625142448 512-byte hardware sectors (320073 MB)
> <5>sd 0:0:0:0: [sda] Write Protect is off
> <7>sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> <5>sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,  
> doesn't support DPO or FUA
> <5>sd 0:0:0:0: [sda] 625142448 512-byte hardware sectors (320073 MB)
> <5>sd 0:0:0:0: [sda] Write Protect is off
> <7>sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> <5>sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled,  
> doesn't support DPO or FUA
> <6> sda: sda1 sda2 sda3 sda4
> <5>sd 0:0:0:0: [sda] Attached SCSI disk
> <5>sd 0:0:0:0: Attached scsi generic sg0 type 0
> 
> I have ensured the other sil680 driver is not enabled!

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

[Alan Cox]

> That's strange though. Somebody with knowledge of that HW (or specs) who
> can spot something ? Could it be an issue with timing ?
> 
> I don't have HW access to this machine. If somebody could send one to me
> I could do more investigation.

Did anyone fix all the mmio posting bugs in libata-core that were pointed
out when I originally NAKked using MMIO, or did they just add the driver.

If the latter then you need to grep the various fix this notes in
libata-core around reset/probe of an SFF controller in particular.

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

[Guennadi Liakhovetski]

On Wed, 13 Feb 2008, Benjamin Herrenschmidt wrote:

> On Tue, 2008-02-12 at 15:58 +0000, Tim Ellis wrote:
> 
> > This change causes attached drives to no longer be detected and  
> > function on the PowerPC Buffalo Linkstation machines:
> > 
> > <7>pata_sil680 0000:00:0c.0: version 0.4.8
> > <6>sil680: 133MHz clock.
> > <6>scsi0 : pata_sil680
> > <6>scsi1 : pata_sil680
> > <6>ata1: PATA max UDMA/133 irq 18
> > <6>ata2: PATA max UDMA/133 irq 18
> > 
> > If I roll back this driver to before this change with 2.6.24.2 it works:
> 
> Hrm... and we need those patches for blades that have a sil680
> controller with no working PIO on the PCI host...
> 
> That's strange though. Somebody with knowledge of that HW (or specs) who
> can spot something ? Could it be an issue with timing ?
> 
> I don't have HW access to this machine. If somebody could send one to me
> I could do more investigation.

Ben, would an ssh access to such a machine and to a terminal server 
suffice?

Thanks
Guennadi
---
Guennadi Liakhovetski

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

[Alan Cox]

> > That's strange though. Somebody with knowledge of that HW (or specs) who
> > can spot something ? Could it be an issue with timing ?
> > 
> > I don't have HW access to this machine. If somebody could send one to me
> > I could do more investigation.
> 
> Ben, would an ssh access to such a machine and to a terminal server 
> suffice?

It says clearly in the code where to start. See the FIXME notes in both
libata-sff and libata-core about MMIO. Neither the DMA transfer start or
the probe SRST sequence are correct with MMIO posting and this hasn't
been fixed as I pointed out was needed when I originally NAKked the
change.

Without those being fixed (especially SRST) on any device with heavy PCI
posting of mmio your controller *wont work*.

Alan

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

[Benjamin Herrenschmidt]

On Fri, 2008-02-15 at 15:53 +0000, Alan Cox wrote:
> > > That's strange though. Somebody with knowledge of that HW (or specs) who
> > > can spot something ? Could it be an issue with timing ?
> > > 
> > > I don't have HW access to this machine. If somebody could send one to me
> > > I could do more investigation.
> > 
> > Ben, would an ssh access to such a machine and to a terminal server 
> > suffice?
> 
> It says clearly in the code where to start. See the FIXME notes in both
> libata-sff and libata-core about MMIO. Neither the DMA transfer start or
> the probe SRST sequence are correct with MMIO posting and this hasn't
> been fixed as I pointed out was needed when I originally NAKked the
> change.
> 
> Without those being fixed (especially SRST) on any device with heavy PCI
> posting of mmio your controller *wont work*.

The dbdma start is mostly harmless (things don't get posted for -that-
long), though I suppose it's worth fixing. Would reading back dmactl do
in that case or do you foresee any kind of side effect ? (Maybe only
doing it for MMIO ?)

As for SRST, I'm not totally confident how safe it is to read back
there while doing the reset sequence, so I'm tempted to really only
do it for MMIO and use altstat rather than ctl/stat (the later tends
to have side effects which we don't want here).

What do you think ?

The main problem from here is that I don't know whether we are using
MMIO or PIO from libata-core. Maybe I can add a host flag indicate
that such flushing is needed ?

In the meantime, Guennadi, can you check if that patch helps for you
(to see if that is indeed the problem):


diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 004dae4..1451a52 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3461,10 +3461,13 @@ static int ata_bus_softreset(struct ata_port *ap, unsigned int devmask,
 
 	/* software reset.  causes dev0 to be selected */
 	iowrite8(ap->ctl, ioaddr->ctl_addr);
+	ioread16(ioaddr->nsect_addr);
 	udelay(20);	/* FIXME: flush */
 	iowrite8(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
+	ioread16(ioaddr->nsect_addr);
 	udelay(20);	/* FIXME: flush */
 	iowrite8(ap->ctl, ioaddr->ctl_addr);
+	ioread16(ioaddr->nsect_addr);
 
 	/* wait a while before checking status */
 	ata_wait_after_reset(ap, deadline);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 60cd4b1..81d5828 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -273,6 +273,7 @@ void ata_bmdma_start(struct ata_queued_cmd *qc)
 	 * FIXME: The posting of this write means I/O starts are
 	 * unneccessarily delayed for MMIO
 	 */
+	ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
 }
 
 /**

Cheers,
Ben.

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

[Alan Cox]

> The dbdma start is mostly harmless (things don't get posted for -that-
> long), though I suppose it's worth fixing. Would reading back dmactl do
> in that case or do you foresee any kind of side effect ? (Maybe only
> doing it for MMIO ?)

The dmactl read back should be just fine, or any other DMA register (eg
status).

> As for SRST, I'm not totally confident how safe it is to read back
> there while doing the reset sequence, so I'm tempted to really only
> do it for MMIO and use altstat rather than ctl/stat (the later tends
> to have side effects which we don't want here).

Agreed - we know some controllers crap themselves spectacularly on
anything which causes a SATA data transfer to be needed during a reset so
the status is probably safest. The fact its not fixed is because nobody
has sat down to figure out what is safe.

> The main problem from here is that I don't know whether we are using
> MMIO or PIO from libata-core. Maybe I can add a host flag indicate
> that such flushing is needed ?

Easier to add that to the ioxxxx ops I suspect (ioispio/ioismmio say) ?

Alan

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

[Benjamin Herrenschmidt]

On Fri, 2008-02-15 at 22:27 +0000, Alan Cox wrote:
> > The dbdma start is mostly harmless (things don't get posted for -that-
> > long), though I suppose it's worth fixing. Would reading back dmactl do
> > in that case or do you foresee any kind of side effect ? (Maybe only
> > doing it for MMIO ?)
> 
> The dmactl read back should be just fine, or any other DMA register (eg
> status).
> 
> > As for SRST, I'm not totally confident how safe it is to read back
> > there while doing the reset sequence, so I'm tempted to really only
> > do it for MMIO and use altstat rather than ctl/stat (the later tends
> > to have side effects which we don't want here).
> 
> Agreed - we know some controllers crap themselves spectacularly on
> anything which causes a SATA data transfer to be needed during a reset so
> the status is probably safest. The fact its not fixed is because nobody
> has sat down to figure out what is safe.
> 
> > The main problem from here is that I don't know whether we are using
> > MMIO or PIO from libata-core. Maybe I can add a host flag indicate
> > that such flushing is needed ?
> 
> Easier to add that to the ioxxxx ops I suspect (ioispio/ioismmio say) ?

Maybe, though that will involve fixing all the arch versions which do
different things. In fact, I'm not even sure I can tell you 100% after
the fact on ppc64, I have to double check.

I'd rather stick a flag in there to be safe, also since altstatus isn't
always there (which is why I used nsect in the test patch I sent to
Guennadi). I'm pretty sure I can rely on all MMIO controllers having an
altstatus but I'd rather still make that explicit with a host flag to
avoid unintended consequences to others.

Ben.

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

[Tim Ellis]

On 15 Feb 2008, at 21:45, Benjamin Herrenschmidt wrote:

>
> On Fri, 2008-02-15 at 15:53 +0000, Alan Cox wrote:
>>>> That's strange though. Somebody with knowledge of that HW (or  
>>>> specs) who
>>>> can spot something ? Could it be an issue with timing ?
>>>>
>>>> I don't have HW access to this machine. If somebody could send  
>>>> one to me
>>>> I could do more investigation.
>>>
>>> Ben, would an ssh access to such a machine and to a terminal server
>>> suffice?
>>
>> It says clearly in the code where to start. See the FIXME notes in  
>> both
>> libata-sff and libata-core about MMIO. Neither the DMA transfer  
>> start or
>> the probe SRST sequence are correct with MMIO posting and this hasn't
>> been fixed as I pointed out was needed when I originally NAKked the
>> change.
>>
>> Without those being fixed (especially SRST) on any device with  
>> heavy PCI
>> posting of mmio your controller *wont work*.
>
> The dbdma start is mostly harmless (things don't get posted for -that-
> long), though I suppose it's worth fixing. Would reading back dmactl  
> do
> in that case or do you foresee any kind of side effect ? (Maybe only
> doing it for MMIO ?)
>
> As for SRST, I'm not totally confident how safe it is to read back
> there while doing the reset sequence, so I'm tempted to really only
> do it for MMIO and use altstat rather than ctl/stat (the later tends
> to have side effects which we don't want here).
>
> What do you think ?
>
> The main problem from here is that I don't know whether we are using
> MMIO or PIO from libata-core. Maybe I can add a host flag indicate
> that such flushing is needed ?
>
> In the meantime, Guennadi, can you check if that patch helps for you
> (to see if that is indeed the problem):
>
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 004dae4..1451a52 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3461,10 +3461,13 @@ static int ata_bus_softreset(struct ata_port  
> *ap, unsigned int devmask,
>
> 	/* software reset.  causes dev0 to be selected */
> 	iowrite8(ap->ctl, ioaddr->ctl_addr);
> +	ioread16(ioaddr->nsect_addr);
> 	udelay(20);	/* FIXME: flush */
> 	iowrite8(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
> +	ioread16(ioaddr->nsect_addr);
> 	udelay(20);	/* FIXME: flush */
> 	iowrite8(ap->ctl, ioaddr->ctl_addr);
> +	ioread16(ioaddr->nsect_addr);
>
> 	/* wait a while before checking status */
> 	ata_wait_after_reset(ap, deadline);
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 60cd4b1..81d5828 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -273,6 +273,7 @@ void ata_bmdma_start(struct ata_queued_cmd *qc)
> 	 * FIXME: The posting of this write means I/O starts are
> 	 * unneccessarily delayed for MMIO
> 	 */
> +	ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
> }
>
> /**
>
> Cheers,
> Ben.


Unfortunately this patch appears to give same result as in the  
original post. Guennadi and I are looking into arranging access to a  
device. Thanks!

<7>pata_sil680 0000:00:0c.0: version 0.4.8
<6>sil680: 133MHz clock.
<6>scsi0 : pata_sil680
<6>scsi1 : pata_sil680
<6>ata1: PATA max UDMA/133 irq 18
<6>ata2: PATA max UDMA/133 irq 18

Tim

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

[Jeff Garzik]

Tim Ellis wrote:
> 
> On 15 Feb 2008, at 21:45, Benjamin Herrenschmidt wrote:
> 
>>
>> On Fri, 2008-02-15 at 15:53 +0000, Alan Cox wrote:
>>>>> That's strange though. Somebody with knowledge of that HW (or 
>>>>> specs) who
>>>>> can spot something ? Could it be an issue with timing ?
>>>>>
>>>>> I don't have HW access to this machine. If somebody could send one 
>>>>> to me
>>>>> I could do more investigation.
>>>>
>>>> Ben, would an ssh access to such a machine and to a terminal server
>>>> suffice?
>>>
>>> It says clearly in the code where to start. See the FIXME notes in both
>>> libata-sff and libata-core about MMIO. Neither the DMA transfer start or
>>> the probe SRST sequence are correct with MMIO posting and this hasn't
>>> been fixed as I pointed out was needed when I originally NAKked the
>>> change.
>>>
>>> Without those being fixed (especially SRST) on any device with heavy PCI
>>> posting of mmio your controller *wont work*.
>>
>> The dbdma start is mostly harmless (things don't get posted for -that-
>> long), though I suppose it's worth fixing. Would reading back dmactl do
>> in that case or do you foresee any kind of side effect ? (Maybe only
>> doing it for MMIO ?)
>>
>> As for SRST, I'm not totally confident how safe it is to read back
>> there while doing the reset sequence, so I'm tempted to really only
>> do it for MMIO and use altstat rather than ctl/stat (the later tends
>> to have side effects which we don't want here).
>>
>> What do you think ?
>>
>> The main problem from here is that I don't know whether we are using
>> MMIO or PIO from libata-core. Maybe I can add a host flag indicate
>> that such flushing is needed ?
>>
>> In the meantime, Guennadi, can you check if that patch helps for you
>> (to see if that is indeed the problem):
>>
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 004dae4..1451a52 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -3461,10 +3461,13 @@ static int ata_bus_softreset(struct ata_port 
>> *ap, unsigned int devmask,
>>
>>     /* software reset.  causes dev0 to be selected */
>>     iowrite8(ap->ctl, ioaddr->ctl_addr);
>> +    ioread16(ioaddr->nsect_addr);
>>     udelay(20);    /* FIXME: flush */
>>     iowrite8(ap->ctl | ATA_SRST, ioaddr->ctl_addr);
>> +    ioread16(ioaddr->nsect_addr);
>>     udelay(20);    /* FIXME: flush */
>>     iowrite8(ap->ctl, ioaddr->ctl_addr);
>> +    ioread16(ioaddr->nsect_addr);
>>
>>     /* wait a while before checking status */
>>     ata_wait_after_reset(ap, deadline);
>> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
>> index 60cd4b1..81d5828 100644
>> --- a/drivers/ata/libata-sff.c
>> +++ b/drivers/ata/libata-sff.c
>> @@ -273,6 +273,7 @@ void ata_bmdma_start(struct ata_queued_cmd *qc)
>>      * FIXME: The posting of this write means I/O starts are
>>      * unneccessarily delayed for MMIO
>>      */
>> +    ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
>> }
>>
>> /**
>>
>> Cheers,
>> Ben.
> 
> 
> Unfortunately this patch appears to give same result as in the original 
> post. Guennadi and I are looking into arranging access to a device. Thanks!

Yes.

Alan loves to complain about lack of MMIO flush, but in practice this is 
rarely the source of problems such as the one you describe.

But if its broken its broken, and we need to revert.  Any luck getting 
benh access to the device?

	Jeff

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

[Guennadi Liakhovetski]

On Mon, 25 Feb 2008, Jeff Garzik wrote:

> But if its broken its broken, and we need to revert.  Any luck getting benh
> access to the device?

We're working on it... We've got devices, but they have to be recovered 
with jtag first, which requires some soldering... I was told this should 
happen end of this / beginning of the next week.

Thanks
Guennadi
---
Guennadi Liakhovetski

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

[Benjamin Herrenschmidt]

> Yes.
> 
> Alan loves to complain about lack of MMIO flush, but in practice this is 
> rarely the source of problems such as the one you describe.
> 
> But if its broken its broken, and we need to revert.  Any luck getting 
> benh access to the device?
> 

Especially since reverting it will break a whole bunch of cell blades,
which wouldn't be nice (those cannot do PIO and have that controller).

In the meantime, we might "workaround" with a hack to only enable MMIO
on those cell
blades, something like:

#ifdef CONFIG_PPC64
	if (machine_is(cell))
		mmio = 1;
#endif

That might get us out of the regression until we find the proper
solution ?

Cheers,
Ben.

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

[Benjamin Herrenschmidt]

> Ben, would an ssh access to such a machine and to a terminal server 
> suffice?

If I can remote-reboot it, yes.

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.