SATA_SIL on IXP425 workaround

SATA_SIL on IXP425 workaround

[Krzysztof Halasa]

I'm trying to add a workaround for IXP4xx CPUs to SATA SIL driver. The
problem is that IXP4xx CPUs (Intel's XScale (ARM) network-oriented
processors) are unable to perform 8 and 16-bit read from PCI MMIO, they
can only do a full 32-bit readl(); SIL chips respond to that with PCI
abort. The workaround is to use 8 and 16-bit regular IO reads (inb/inw)
instead (MMIO write is not a problem).

For SIL3x12 the workaround is simple (attached) and it works on my 3512.
I'm not sure about 3114 (the 4-port chip) - the PIO BARs have TF, CTL
and BWDMA registers which are common to channels 0 and 2, and (the other
set) to channels 1 and 3. Channel selection is done with bit 4 of
device/head TF register, this is similar (same?) as PATA master/slave.
Does that mean that I can simply treat channel 0 as PRI master, ch#2 as
PRI slave, ch#1 as SEC master and ch#3 as SEC slave, and the SFF code
will select the right device correctly? Does it need additional code?
I don't have anything based on 3114.

Note: the large PRD is not a problem here, the transfer can be started
by MMIO write. Only reads are an issue.

--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -757,7 +757,12 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
+#ifdef CONFIG_ARCH_IXP4XX
+	/* We need all 6 regions on IXP4xx */
+	rc = pcim_iomap_regions(pdev, 0x3F, DRV_NAME);
+#else
 	rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
+#endif
 	if (rc == -EBUSY)
 		pcim_pin_device(pdev);
 	if (rc)
@@ -777,10 +782,18 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		struct ata_port *ap = host->ports[i];
 		struct ata_ioports *ioaddr = &ap->ioaddr;
 
+#ifdef CONFIG_ARCH_IXP4XX
+		/* IXP4xx CPUs can't perform 8 and 16-bit MMIO reads,
+		   use normal IO from/to regions 0-5 instead */
+		ioaddr->cmd_addr = host->iomap[i * 2];
+		ioaddr->altstatus_addr = host->iomap[1 + i * 2] + 2;
+		ioaddr->bmdma_addr = host->iomap[4] + sil_port[i].bmdma;
+#else
 		ioaddr->cmd_addr = mmio_base + sil_port[i].tf;
-		ioaddr->altstatus_addr =
-		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
+		ioaddr->altstatus_addr = mmio_base + sil_port[i].ctl;
 		ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
+#endif
+		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
 		ioaddr->scr_addr = mmio_base + sil_port[i].scr;
 		ata_sff_std_ports(ioaddr);
 

-- 
Krzysztof Halasa

Re: SATA_SIL on IXP425 workaround

[Bartlomiej Zolnierkiewicz]

On Monday 09 November 2009 06:31:21 pm Krzysztof Halasa wrote:
> I'm trying to add a workaround for IXP4xx CPUs to SATA SIL driver. The
> problem is that IXP4xx CPUs (Intel's XScale (ARM) network-oriented
> processors) are unable to perform 8 and 16-bit read from PCI MMIO, they
> can only do a full 32-bit readl(); SIL chips respond to that with PCI
> abort. The workaround is to use 8 and 16-bit regular IO reads (inb/inw)
> instead (MMIO write is not a problem).
> 
> For SIL3x12 the workaround is simple (attached) and it works on my 3512.
> I'm not sure about 3114 (the 4-port chip) - the PIO BARs have TF, CTL
> and BWDMA registers which are common to channels 0 and 2, and (the other
> set) to channels 1 and 3. Channel selection is done with bit 4 of
> device/head TF register, this is similar (same?) as PATA master/slave.
> Does that mean that I can simply treat channel 0 as PRI master, ch#2 as
> PRI slave, ch#1 as SEC master and ch#3 as SEC slave, and the SFF code
> will select the right device correctly? Does it need additional code?
> I don't have anything based on 3114.
> 
> Note: the large PRD is not a problem here, the transfer can be started
> by MMIO write. Only reads are an issue.

FWIW your patch is now in my atang tree (I'm aware that Jeff is working
on generic solution but in the meantime this non-intrusive patch allows
sata_sil to work on IXP425).

> --- a/drivers/ata/sata_sil.c
> +++ b/drivers/ata/sata_sil.c
> @@ -757,7 +757,12 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (rc)
>  		return rc;
>  
> +#ifdef CONFIG_ARCH_IXP4XX
> +	/* We need all 6 regions on IXP4xx */
> +	rc = pcim_iomap_regions(pdev, 0x3F, DRV_NAME);
> +#else
>  	rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
> +#endif
>  	if (rc == -EBUSY)
>  		pcim_pin_device(pdev);
>  	if (rc)
> @@ -777,10 +782,18 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		struct ata_port *ap = host->ports[i];
>  		struct ata_ioports *ioaddr = &ap->ioaddr;
>  
> +#ifdef CONFIG_ARCH_IXP4XX
> +		/* IXP4xx CPUs can't perform 8 and 16-bit MMIO reads,
> +		   use normal IO from/to regions 0-5 instead */
> +		ioaddr->cmd_addr = host->iomap[i * 2];
> +		ioaddr->altstatus_addr = host->iomap[1 + i * 2] + 2;
> +		ioaddr->bmdma_addr = host->iomap[4] + sil_port[i].bmdma;
> +#else
>  		ioaddr->cmd_addr = mmio_base + sil_port[i].tf;
> -		ioaddr->altstatus_addr =
> -		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
> +		ioaddr->altstatus_addr = mmio_base + sil_port[i].ctl;
>  		ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
> +#endif
> +		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
>  		ioaddr->scr_addr = mmio_base + sil_port[i].scr;
>  		ata_sff_std_ports(ioaddr);

--
Bartlomiej Zolnierkiewicz

Re: SATA_SIL on IXP425 workaround

[Krzysztof Halasa]

Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> writes:

> FWIW your patch is now in my atang tree (I'm aware that Jeff is working
> on generic solution but in the meantime this non-intrusive patch allows
> sata_sil to work on IXP425).

Thanks. BTW I could help with the "general solution", but I wonder if
what we need is simply adding those other ARM archs to IXP4xx (through
Kconfig)? If they have the same problem, namely inability to do
readb/readw only, while being able to write[bwl] and all kinds of non-MM
I/O.

writeb() is important since it starts the "large PRD" transfers (it has
to use MMIO BAR to be "large").
-- 
Krzysztof Halasa

Re: SATA_SIL on IXP425 workaround

[Alan Cox]

> FWIW your patch is now in my atang tree (I'm aware that Jeff is working
> on generic solution but in the meantime this non-intrusive patch allows
> sata_sil to work on IXP425).

I think this is the wrong place. If your platform can't do MMIO properly
then the platform pci_iomap or pci quirk code should do the needed
cleaning up, not put turds into the drivers. Why not just quirk it on your
specific platform and clear the MMIO mapping.

Re: SATA_SIL on IXP425 workaround

[Krzysztof Halasa]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> I think this is the wrong place. If your platform can't do MMIO properly
> then the platform pci_iomap or pci quirk code should do the needed
> cleaning up, not put turds into the drivers. Why not just quirk it on your
> specific platform and clear the MMIO mapping.

We need to use the MMIO BAR at least for starting DMA transfers, the
I/O ones are 64KB-limited. We can't just use read[bw] if reading all
32 bits has side effects.

Most of the time there are no problems with MMIO on IXP4xx as modern
devices usually use 32-bit registers anyway, or at least they have no
problem with read[bw] always driving all four PCI byte enable lines
(write[bw] doesn't have this issue).
-- 
Krzysztof Halasa

Re: SATA_SIL on IXP425 workaround

[Alan Cox]

> We need to use the MMIO BAR at least for starting DMA transfers, the
> I/O ones are 64KB-limited. We can't just use read[bw] if reading all
> 32 bits has side effects.

Last time I instrumented this on x86 we never issued a > 64K linear block
in our s/g lists. In fact we went for years before anyone noticed we had
a bug with CS5530 and a couple of other chips that mishandled 64K segment
sizes, and that was only finally noticed in a very specific and weird
circumstance.

> Most of the time there are no problems with MMIO on IXP4xx as modern
> devices usually use 32-bit registers anyway, or at least they have no
> problem with read[bw] always driving all four PCI byte enable lines
> (write[bw] doesn't have this issue).

Fair enough

Re: SATA_SIL on IXP425 workaround

[Robert Hancock]

On 01/14/2010 03:05 PM, Alan Cox wrote:
>> We need to use the MMIO BAR at least for starting DMA transfers, the
>> I/O ones are 64KB-limited. We can't just use read[bw] if reading all
>> 32 bits has side effects.
>
> Last time I instrumented this on x86 we never issued a>  64K linear block
> in our s/g lists. In fact we went for years before anyone noticed we had
> a bug with CS5530 and a couple of other chips that mishandled 64K segment
> sizes, and that was only finally noticed in a very specific and weird
> circumstance.

Having a block over 64KB may be rare, but the other thing that the large 
block transfer feature does is remove the restriction on a block 
crossing a 64KB boundary, which based on the experiments I did when I 
worked on adding the feature, does happen fairly commonly if the driver 
allows it.

Re: SATA_SIL on IXP425 workaround

[Bartlomiej Zolnierkiewicz]

On Thursday 14 January 2010 08:22:10 pm Alan Cox wrote:
> > FWIW your patch is now in my atang tree (I'm aware that Jeff is working
> > on generic solution but in the meantime this non-intrusive patch allows
> > sata_sil to work on IXP425).
> 
> I think this is the wrong place. If your platform can't do MMIO properly
> then the platform pci_iomap or pci quirk code should do the needed
> cleaning up, not put turds into the drivers. Why not just quirk it on your
> specific platform and clear the MMIO mapping.

I think that you misinterpreted the issue -- according to Krzysztof MMIO
works just fine, only 8/16-bit MMIO reads are a problem (please note that
using mixed PIO/MMIO access is still a win over pure PIO access and also
that sata_sil doesn't support pure non-MMIO operations currently)..

However if it gets fixed in the upstream kernel in some other way I'll
simply drop the patch during the next re-base of my tree (I just collect
ATA stuff that looks useful/interesting to me and which otherwise may
become lost)..

--
Bartlomiej Zolnierkiewicz

Re: SATA_SIL on IXP425 workaround

[Alan Cox]

> I think that you misinterpreted the issue -- according to Krzysztof MMIO
> works just fine, only 8/16-bit MMIO reads are a problem (please note that
> using mixed PIO/MMIO access is still a win over pure PIO access and also
> that sata_sil doesn't support pure non-MMIO operations currently)..

Have we got timing measurements to prove that ?
> 
> However if it gets fixed in the upstream kernel in some other way I'll
> simply drop the patch during the next re-base of my tree (I just collect
> ATA stuff that looks useful/interesting to me and which otherwise may
> become lost)..

And that's a good thing. The question was aimed at Krzysztof. Really we
need to keep platform specific weirdness out of core drivers or the
rapidly turn into a mess.

Re: SATA_SIL on IXP425 workaround

[Jeff Garzik]

On 01/14/2010 04:00 PM, Alan Cox wrote:
>> I think that you misinterpreted the issue -- according to Krzysztof MMIO
>> works just fine, only 8/16-bit MMIO reads are a problem (please note that
>> using mixed PIO/MMIO access is still a win over pure PIO access and also
>> that sata_sil doesn't support pure non-MMIO operations currently)..
>
> Have we got timing measurements to prove that ?
>>
>> However if it gets fixed in the upstream kernel in some other way I'll
>> simply drop the patch during the next re-base of my tree (I just collect
>> ATA stuff that looks useful/interesting to me and which otherwise may
>> become lost)..
>
> And that's a good thing. The question was aimed at Krzysztof. Really we
> need to keep platform specific weirdness out of core drivers or the
> rapidly turn into a mess.

The lack of 8-/16-byte MMIO access is unfortunately not limited to 
Krzysztof's platform.  Other people have reported the same problem 
specifically with sata_sil (see the archives).

If the driver cannot be converted to all-32-bit accesses, we will need 
to do something like Krzysztof's patch.

	Jeff

Re: SATA_SIL on IXP425 workaround

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On 01/14/2010 10:59 AM, Bartlomiej Zolnierkiewicz wrote:
> On Monday 09 November 2009 06:31:21 pm Krzysztof Halasa wrote:
>> I'm trying to add a workaround for IXP4xx CPUs to SATA SIL driver. The
>> problem is that IXP4xx CPUs (Intel's XScale (ARM) network-oriented
>> processors) are unable to perform 8 and 16-bit read from PCI MMIO, they
>> can only do a full 32-bit readl(); SIL chips respond to that with PCI
>> abort. The workaround is to use 8 and 16-bit regular IO reads (inb/inw)
>> instead (MMIO write is not a problem).
>>
>> For SIL3x12 the workaround is simple (attached) and it works on my 3512.
>> I'm not sure about 3114 (the 4-port chip) - the PIO BARs have TF, CTL
>> and BWDMA registers which are common to channels 0 and 2, and (the other
>> set) to channels 1 and 3. Channel selection is done with bit 4 of
>> device/head TF register, this is similar (same?) as PATA master/slave.
>> Does that mean that I can simply treat channel 0 as PRI master, ch#2 as
>> PRI slave, ch#1 as SEC master and ch#3 as SEC slave, and the SFF code
>> will select the right device correctly? Does it need additional code?
>> I don't have anything based on 3114.
>>
>> Note: the large PRD is not a problem here, the transfer can be started
>> by MMIO write. Only reads are an issue.
>
> FWIW your patch is now in my atang tree (I'm aware that Jeff is working
> on generic solution but in the meantime this non-intrusive patch allows
> sata_sil to work on IXP425).

I was asking an open question, is a generic solution possible?

Something like the attached patch might work, due it is completely 
untested, and I did not verify that the BMDMA Status register is not 
stomped.  Also, the additional ioread32() calls in bmdma start/stop are 
LIKELY to be unnecessary.

	Jeff





[-- Attachment #2: patch.sata_sil-mmio32 --]
[-- Type: text/plain, Size: 2701 bytes --]

diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 3cb69d5..7fa6164 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -270,9 +270,11 @@ static void sil_bmdma_stop(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	void __iomem *mmio_base = ap->host->iomap[SIL_MMIO_BAR];
 	void __iomem *bmdma2 = mmio_base + sil_port[ap->port_no].bmdma2;
+	u32 tmp;
 
 	/* clear start/stop bit - can safely always write 0 */
-	iowrite8(0, bmdma2);
+	tmp = ioread32(bmdma2) & 0xffffff00;
+	iowrite32(tmp | 0, bmdma2);
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
 	ata_sff_dma_pause(ap);
@@ -296,14 +298,17 @@ static void sil_bmdma_start(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	void __iomem *mmio_base = ap->host->iomap[SIL_MMIO_BAR];
 	void __iomem *bmdma2 = mmio_base + sil_port[ap->port_no].bmdma2;
-	u8 dmactl = ATA_DMA_START;
+	u32 dmactl = ATA_DMA_START;
+	u32 tmp;
 
 	/* set transfer direction, start host DMA transaction
 	   Note: For Large Block Transfer to work, the DMA must be started
 	   using the bmdma2 register. */
 	if (!rw)
 		dmactl |= ATA_DMA_WR;
-	iowrite8(dmactl, bmdma2);
+
+	tmp = ioread32(bmdma2) & 0xffffff00;
+	iowrite32(tmp | dmactl, bmdma2);
 }
 
 /* The way God intended PCI IDE scatter/gather lists to look and behave... */
@@ -571,13 +576,13 @@ static void sil_freeze(struct ata_port *ap)
 	 * This is because the controller will not give us access to the
 	 * taskfile registers while a DMA is in progress
 	 */
-	iowrite8(ioread8(ap->ioaddr.bmdma_addr) & ~SIL_DMA_ENABLE,
-		 ap->ioaddr.bmdma_addr);
+	tmp = ioread32(ap->ioaddr.bmdma_addr);
+	iowrite32(tmp & ~SIL_DMA_ENABLE, ap->ioaddr.bmdma_addr);
 
 	/* According to ata_bmdma_stop, an HDMA transition requires
 	 * on PIO cycle. But we can't read a taskfile register.
 	 */
-	ioread8(ap->ioaddr.bmdma_addr);
+	ioread32(ap->ioaddr.bmdma_addr);
 }
 
 static void sil_thaw(struct ata_port *ap)
@@ -667,7 +672,7 @@ static void sil_init_controller(struct ata_host *host)
 {
 	struct pci_dev *pdev = to_pci_dev(host->dev);
 	void __iomem *mmio_base = host->iomap[SIL_MMIO_BAR];
-	u8 cls;
+	u32 cls;
 	u32 tmp;
 	int i;
 
@@ -676,9 +681,12 @@ static void sil_init_controller(struct ata_host *host)
 	if (cls) {
 		cls >>= 3;
 		cls++;  /* cls = (line_size/8)+1 */
-		for (i = 0; i < host->n_ports; i++)
-			writew(cls << 8 | cls,
+		for (i = 0; i < host->n_ports; i++) {
+			tmp = readl(mmio_base + sil_port[i].fifo_cfg) &
+			      0xffff0000;
+			writel(tmp | cls << 8 | cls,
 			       mmio_base + sil_port[i].fifo_cfg);
+		}
 	} else
 		dev_printk(KERN_WARNING, &pdev->dev,
 			   "cache line size not set.  Driver may not function\n");

Re: SATA_SIL on IXP425 workaround

[Jeff Garzik]

On 01/20/2010 11:58 PM, Jeff Garzik wrote:
> On 01/14/2010 10:59 AM, Bartlomiej Zolnierkiewicz wrote:
>> On Monday 09 November 2009 06:31:21 pm Krzysztof Halasa wrote:
>>> I'm trying to add a workaround for IXP4xx CPUs to SATA SIL driver. The
>>> problem is that IXP4xx CPUs (Intel's XScale (ARM) network-oriented
>>> processors) are unable to perform 8 and 16-bit read from PCI MMIO, they
>>> can only do a full 32-bit readl(); SIL chips respond to that with PCI
>>> abort. The workaround is to use 8 and 16-bit regular IO reads (inb/inw)
>>> instead (MMIO write is not a problem).
>>>
>>> For SIL3x12 the workaround is simple (attached) and it works on my 3512.
>>> I'm not sure about 3114 (the 4-port chip) - the PIO BARs have TF, CTL
>>> and BWDMA registers which are common to channels 0 and 2, and (the other
>>> set) to channels 1 and 3. Channel selection is done with bit 4 of
>>> device/head TF register, this is similar (same?) as PATA master/slave.
>>> Does that mean that I can simply treat channel 0 as PRI master, ch#2 as
>>> PRI slave, ch#1 as SEC master and ch#3 as SEC slave, and the SFF code
>>> will select the right device correctly? Does it need additional code?
>>> I don't have anything based on 3114.
>>>
>>> Note: the large PRD is not a problem here, the transfer can be started
>>> by MMIO write. Only reads are an issue.
>>
>> FWIW your patch is now in my atang tree (I'm aware that Jeff is working
>> on generic solution but in the meantime this non-intrusive patch allows
>> sata_sil to work on IXP425).
>
> I was asking an open question, is a generic solution possible?
>
> Something like the attached patch might work, due it is completely
> untested, and I did not verify that the BMDMA Status register is not
> stomped. Also, the additional ioread32() calls in bmdma start/stop are
> LIKELY to be unnecessary.

As I suspected, there is a W1C register in there.  But it does look 
possible to do all-32-bit accesses.

	Jeff

Re: SATA_SIL on IXP425 workaround

[Jeff Garzik]

On 01/21/2010 01:48 AM, Jeff Garzik wrote:
> On 01/20/2010 11:58 PM, Jeff Garzik wrote:
>> On 01/14/2010 10:59 AM, Bartlomiej Zolnierkiewicz wrote:
>>> On Monday 09 November 2009 06:31:21 pm Krzysztof Halasa wrote:
>>>> I'm trying to add a workaround for IXP4xx CPUs to SATA SIL driver. The
>>>> problem is that IXP4xx CPUs (Intel's XScale (ARM) network-oriented
>>>> processors) are unable to perform 8 and 16-bit read from PCI MMIO, they
>>>> can only do a full 32-bit readl(); SIL chips respond to that with PCI
>>>> abort. The workaround is to use 8 and 16-bit regular IO reads (inb/inw)
>>>> instead (MMIO write is not a problem).
>>>>
>>>> For SIL3x12 the workaround is simple (attached) and it works on my
>>>> 3512.
>>>> I'm not sure about 3114 (the 4-port chip) - the PIO BARs have TF, CTL
>>>> and BWDMA registers which are common to channels 0 and 2, and (the
>>>> other
>>>> set) to channels 1 and 3. Channel selection is done with bit 4 of
>>>> device/head TF register, this is similar (same?) as PATA master/slave.
>>>> Does that mean that I can simply treat channel 0 as PRI master, ch#2 as
>>>> PRI slave, ch#1 as SEC master and ch#3 as SEC slave, and the SFF code
>>>> will select the right device correctly? Does it need additional code?
>>>> I don't have anything based on 3114.
>>>>
>>>> Note: the large PRD is not a problem here, the transfer can be started
>>>> by MMIO write. Only reads are an issue.
>>>
>>> FWIW your patch is now in my atang tree (I'm aware that Jeff is working
>>> on generic solution but in the meantime this non-intrusive patch allows
>>> sata_sil to work on IXP425).
>>
>> I was asking an open question, is a generic solution possible?
>>
>> Something like the attached patch might work, due it is completely
>> untested, and I did not verify that the BMDMA Status register is not
>> stomped. Also, the additional ioread32() calls in bmdma start/stop are
>> LIKELY to be unnecessary.
>
> As I suspected, there is a W1C register in there. But it does look
> possible to do all-32-bit accesses.

It is definitely possible to do all 32-bit accesses...  but that 
requires activating and exclusively using the command buffering feature, 
because direct 32-bit access to the taskfile registers will result in a 
32-bit access to Data rather than the desired effect.

The chip docs are at http://gkernel.sourceforge.net/specs/sii/ for those 
unfamiliar with my doc archive.

	Jeff

Re: SATA_SIL on IXP425 workaround

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> It is definitely possible to do all 32-bit accesses...  but that
> requires activating and exclusively using the command buffering
> feature, because direct 32-bit access to the taskfile registers will
> result in a 32-bit access to Data rather than the desired effect.

Command buffering? The DS for SIL3512 lists IDEx taskfile registers
for command buffering, but I can't see any explanation there.

BTW I don't know requirements of other platforms, but IXP4xx has only
problems with 8- and 16-bit PCI memory reads. Other ops including
all memory writes, and all "port" I/O are ok.

Unfortunately I can't test any patch ATM, -ENOHW. I might be able to get
access to this hw again, but I don't know at this point.
-- 
Krzysztof Halasa