[PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Chris Dearman]

I'm working on a board where byte reads from the processor get
converted into word reads on the PCI bus. This doesn't mix well
with reading the TaskFile registers...

IO space reads do generate the correct byte enables so this patch
gives the option of accessing the TF registers via IO space.

I'm not sure if its worth adding this to the driver... it's a work
around for the "unfortunate" PCI behaviour on this board, but there
may be other challenged hardware out there, so it might be worth
considering. 

Chris

[PATCH 1/1] sata_sil: Option to use IO space to access TF registers.

[Chris Dearman]

Provide an alternative way to access the TaskFile registers if mmio
doesn't work.

Signed-off-by: Chris Dearman <chris@linux-mips.org>
---

 drivers/ata/Kconfig    |   12 ++++++++++++
 drivers/ata/sata_sil.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ae84949..28f7b5a 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -169,6 +169,18 @@ config SATA_SIL
 
 	  If unsure, say N.
 
+config SATA_SIL_IO
+	bool "Use IO space to access device registers"
+	depends on SATA_SIL
+	default n
+	help
+	  Some registers on this device require byte accesses. If your
+	  PCI controller promotes byte reads to word reads on the PCI bus
+	  enabling this option makes the driver use IO accesses for these
+	  registers,
+
+	  If unsure, say N.
+
 config SATA_SIS
 	tristate "SiS 964/965/966/180 SATA support"
 	depends on PCI
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 88bf421..94f4895 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -49,6 +49,12 @@
 #define DRV_VERSION	"2.3"
 
 enum {
+#ifdef CONFIG_SATA_SIL_IO
+	SIL_IO_IDE0TF_BAR	= 0,
+	SIL_IO_IDE0CTL_BAR	= 1,
+	SIL_IO_IDE1TF_BAR	= 2,
+	SIL_IO_IDE1CTL_BAR	= 3,
+#endif
 	SIL_MMIO_BAR		= 5,
 
 	/*
@@ -237,6 +243,19 @@ static const struct {
 	{ 0x2C0, 0x2CA, 0x208, 0x218, 0x244, 0x380, 0x3c8, 0x2f4, 0x3cc },
 	/* ... port 3 */
 };
+#ifdef CONFIG_SATA_SIL_IO
+static const struct {
+	unsigned int tfbar;	/* ATA taskfile BAR */
+	unsigned int tf;	/* ATA taskfile register block */
+	unsigned int ctlbar;	/* ATA control/altstatus BAR */
+	unsigned int ctl;	/* ATA control/altstatus register block */
+} sil_ioport[] = {
+	{ SIL_IO_IDE0TF_BAR, 0, SIL_IO_IDE0CTL_BAR, 2 },
+	{ SIL_IO_IDE1TF_BAR, 0, SIL_IO_IDE1CTL_BAR, 2 },
+	{ SIL_IO_IDE0TF_BAR, 0, SIL_IO_IDE0CTL_BAR, 2 },
+	{ SIL_IO_IDE1TF_BAR, 0, SIL_IO_IDE1CTL_BAR, 2 }
+};
+#endif
 
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("low-level driver for Silicon Image SATA controller");
@@ -630,7 +649,17 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
+#ifdef CONFIG_SATA_SIL_IO
+	rc = pcim_iomap_regions(pdev,
+				(1 << SIL_IO_IDE0TF_BAR) |
+				(1 << SIL_IO_IDE0CTL_BAR) |
+				(1 << SIL_IO_IDE1TF_BAR) |
+				(1 << SIL_IO_IDE1CTL_BAR) |
+				(1 << SIL_MMIO_BAR),
+				DRV_NAME);
+#else
 	rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
+#endif
 	if (rc == -EBUSY)
 		pcim_pin_device(pdev);
 	if (rc)
@@ -649,7 +678,20 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 		struct ata_ioports *ioaddr = &ap->ioaddr;
+#ifdef CONFIG_SATA_SIL_IO
+		void __iomem *tf_iobase = host->iomap[sil_ioport[i].tfbar];
+		void __iomem *ctl_iobase = host->iomap[sil_ioport[i].ctlbar];
+		ioaddr->cmd_addr = tf_iobase + sil_ioport[i].tf;
+		ioaddr->altstatus_addr =
+		ioaddr->ctl_addr = ctl_iobase + sil_ioport[i].ctl;
+		ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
+		ioaddr->scr_addr = mmio_base + sil_port[i].scr;
+		ata_sff_std_ports(ioaddr);
 
+		ata_port_pbar_desc(ap, SIL_MMIO_BAR, -1, "mmio");
+		ata_port_pbar_desc(ap, sil_ioport[i].tfbar, -1, "tf");
+		ata_port_pbar_desc(ap, sil_ioport[i].ctlbar, -1, "ctl");
+#else
 		ioaddr->cmd_addr = mmio_base + sil_port[i].tf;
 		ioaddr->altstatus_addr =
 		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
@@ -659,6 +701,7 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 		ata_port_pbar_desc(ap, SIL_MMIO_BAR, -1, "mmio");
 		ata_port_pbar_desc(ap, SIL_MMIO_BAR, sil_port[i].tf, "tf");
+#endif
 	}
 
 	/* initialize and activate */

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Alan Cox]

> I'm not sure if its worth adding this to the driver... it's a work
> around for the "unfortunate" PCI behaviour on this board, but there
> may be other challenged hardware out there, so it might be worth
> considering. 

You need to fix your ioread8/iowrite8 and other accessors to work around
this if at all possible and that will cover all your devices.

Alan

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Chris Dearman]

Hi Alan,

> You need to fix your ioread8/iowrite8 and other accessors to work around
> this if at all possible and that will cover all your devices.

 I would like to have a generic solution for all devices, but
unfortunately I don't think that's possible. There is no way to
generate a read cycle to PCI memory space on this hardware without all
of the byte enables being set. Normally this isn't a problem, a full
word is read on the PCI bus and the processor picks out the bytes it
needs, but memory mapped registers with access restrictions are a
problem.  PCI memory writes and PCI IO reads/writes do generate the
correct byte enables.

  I did consider checking for accesses to particular locations on
particular PCI devices in ioread8/ioread16 and silently converting
them into equivalent IO accesses (assuming one exists of course...),
but it gets pretty nasty. Eg on this SATA chip I would have to map
the IO BARS behind the drivers back, and it's still not a generic
solution - for each new device I would have to add affected
addresses to ioread8/ioread16. 

  The patch is a relatively clean work around for limitations in the
hardware I'm using, but I know it probably looks like just more
cruft from an outside perspective so I won't be championing it :)

Chris

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Alan Cox]

> unfortunately I don't think that's possible. There is no way to
> generate a read cycle to PCI memory space on this hardware without all
> of the byte enables being set. Normally this isn't a problem, a full

Ok so this isn't about not being able to do the workaround, you have
hardware which is just busted ? Is this devel hardware or stuff in mass
circulation ?

>   The patch is a relatively clean work around for limitations in the
> hardware I'm using, but I know it probably looks like just more
> cruft from an outside perspective so I won't be championing it :)

I would make the switch a runtime value personally - some network drivers
do this for similar reasons.

Alan

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Tejun Heo]

Alan Cox wrote:
>>   The patch is a relatively clean work around for limitations in the
>> hardware I'm using, but I know it probably looks like just more
>> cruft from an outside perspective so I won't be championing it :)
> 
> I would make the switch a runtime value personally - some network drivers
> do this for similar reasons.

I agree.  Please make it a module parameter.

Thanks.

-- 
tejun

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Chris Dearman]

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

On Thu, Oct 23, 2008 at 08:32:15AM +0100, Alan Cox wrote:
> I would make the switch a runtime value personally - some network drivers
> do this for similar reasons.

OK. Something like this?

Chris


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

sata_sil: Option to use IO space to access TF registers.

From: Chris Dearman <chris@linux-mips.org>

Provide an alternative way to access the TaskFile registers if mmio
doesn't work.

Signed-off-by: Chris Dearman <chris@linux-mips.org>
---

 drivers/ata/sata_sil.c |   67 ++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 56 insertions(+), 11 deletions(-)


diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 88bf421..79f974a 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -49,6 +49,10 @@
 #define DRV_VERSION	"2.3"
 
 enum {
+	SIL_IO_IDE0TF_BAR	= 0,
+	SIL_IO_IDE0CTL_BAR	= 1,
+	SIL_IO_IDE1TF_BAR	= 2,
+	SIL_IO_IDE1CTL_BAR	= 3,
 	SIL_MMIO_BAR		= 5,
 
 	/*
@@ -237,6 +241,18 @@ static const struct {
 	{ 0x2C0, 0x2CA, 0x208, 0x218, 0x244, 0x380, 0x3c8, 0x2f4, 0x3cc },
 	/* ... port 3 */
 };
+/* per-port IO based register offsets */
+static const struct {
+	unsigned int tfbar;	/* ATA taskfile BAR */
+	unsigned int tf;	/* ATA taskfile register block */
+	unsigned int ctlbar;	/* ATA control/altstatus BAR */
+	unsigned int ctl;	/* ATA control/altstatus register block */
+} sil_ioport[] = {
+	{ SIL_IO_IDE0TF_BAR, 0, SIL_IO_IDE0CTL_BAR, 2 },
+	{ SIL_IO_IDE1TF_BAR, 0, SIL_IO_IDE1CTL_BAR, 2 },
+	{ SIL_IO_IDE0TF_BAR, 0, SIL_IO_IDE0CTL_BAR, 2 },
+	{ SIL_IO_IDE1TF_BAR, 0, SIL_IO_IDE1CTL_BAR, 2 }
+};
 
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("low-level driver for Silicon Image SATA controller");
@@ -248,6 +264,9 @@ static int slow_down;
 module_param(slow_down, int, 0444);
 MODULE_PARM_DESC(slow_down, "Sledgehammer used to work around random problems, by limiting commands to 15 sectors (0=off, 1=on)");
 
+static int tfio;
+module_param(tfio, int, 0444);
+MODULE_PARM_DESC(tfio, "Access Taskfile registers via IO space (0=off, 1=on)");
 
 static unsigned char sil_get_device_cache_line(struct pci_dev *pdev)
 {
@@ -630,7 +649,17 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
-	rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
+	if (tfio)
+		rc = pcim_iomap_regions(pdev,
+					(1 << SIL_IO_IDE0TF_BAR) |
+					(1 << SIL_IO_IDE0CTL_BAR) |
+					(1 << SIL_IO_IDE1TF_BAR) |
+					(1 << SIL_IO_IDE1CTL_BAR) |
+					(1 << SIL_MMIO_BAR),
+					DRV_NAME);
+	else
+		rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
+
 	if (rc == -EBUSY)
 		pcim_pin_device(pdev);
 	if (rc)
@@ -649,16 +678,32 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 		struct ata_ioports *ioaddr = &ap->ioaddr;
-
-		ioaddr->cmd_addr = mmio_base + sil_port[i].tf;
-		ioaddr->altstatus_addr =
-		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
-		ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
-		ioaddr->scr_addr = mmio_base + sil_port[i].scr;
-		ata_sff_std_ports(ioaddr);
-
-		ata_port_pbar_desc(ap, SIL_MMIO_BAR, -1, "mmio");
-		ata_port_pbar_desc(ap, SIL_MMIO_BAR, sil_port[i].tf, "tf");
+		void __iomem *tf_iobase = host->iomap[sil_ioport[i].tfbar];
+		void __iomem *ctl_iobase = host->iomap[sil_ioport[i].ctlbar];
+		if (tfio) {
+			ioaddr->cmd_addr = tf_iobase + sil_ioport[i].tf;
+			ioaddr->altstatus_addr =
+			ioaddr->ctl_addr = ctl_iobase + sil_ioport[i].ctl;
+			ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
+			ioaddr->scr_addr = mmio_base + sil_port[i].scr;
+			ata_sff_std_ports(ioaddr);
+
+			ata_port_pbar_desc(ap, SIL_MMIO_BAR, -1, "mmio");
+			ata_port_pbar_desc(ap, sil_ioport[i].tfbar, -1, "tf");
+			ata_port_pbar_desc(ap, sil_ioport[i].ctlbar,
+					   -1, "ctl");
+		} else {
+			ioaddr->cmd_addr = mmio_base + sil_port[i].tf;
+			ioaddr->altstatus_addr =
+			ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
+			ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
+			ioaddr->scr_addr = mmio_base + sil_port[i].scr;
+			ata_sff_std_ports(ioaddr);
+
+			ata_port_pbar_desc(ap, SIL_MMIO_BAR, -1, "mmio");
+			ata_port_pbar_desc(ap, SIL_MMIO_BAR,
+					   sil_port[i].tf, "tf");
+		}
 	}
 
 	/* initialize and activate */

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Tejun Heo]

Hello,

Just some nitpickings.

Chris Dearman wrote:
> sata_sil: Option to use IO space to access TF registers.
> 
> From: Chris Dearman <chris@linux-mips.org>
> 
> Provide an alternative way to access the TaskFile registers if mmio
> doesn't work.
> 
> Signed-off-by: Chris Dearman <chris@linux-mips.org>
> ---
> 
>  drivers/ata/sata_sil.c |   67 ++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 56 insertions(+), 11 deletions(-)
> 
> 
> diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
> index 88bf421..79f974a 100644
> --- a/drivers/ata/sata_sil.c
> +++ b/drivers/ata/sata_sil.c
> @@ -49,6 +49,10 @@
>  #define DRV_VERSION	"2.3"
>  
>  enum {
> +	SIL_IO_IDE0TF_BAR	= 0,
> +	SIL_IO_IDE0CTL_BAR	= 1,
> +	SIL_IO_IDE1TF_BAR	= 2,
> +	SIL_IO_IDE1CTL_BAR	= 3,
>  	SIL_MMIO_BAR		= 5,
>  
>  	/*
> @@ -237,6 +241,18 @@ static const struct {
>  	{ 0x2C0, 0x2CA, 0x208, 0x218, 0x244, 0x380, 0x3c8, 0x2f4, 0x3cc },
>  	/* ... port 3 */
>  };

Care to put a blank line here?

> +/* per-port IO based register offsets */
> +static const struct {
> +	unsigned int tfbar;	/* ATA taskfile BAR */
> +	unsigned int tf;	/* ATA taskfile register block */
> +	unsigned int ctlbar;	/* ATA control/altstatus BAR */
> +	unsigned int ctl;	/* ATA control/altstatus register block */
> +} sil_ioport[] = {
> +	{ SIL_IO_IDE0TF_BAR, 0, SIL_IO_IDE0CTL_BAR, 2 },
> +	{ SIL_IO_IDE1TF_BAR, 0, SIL_IO_IDE1CTL_BAR, 2 },
> +	{ SIL_IO_IDE0TF_BAR, 0, SIL_IO_IDE0CTL_BAR, 2 },
> +	{ SIL_IO_IDE1TF_BAR, 0, SIL_IO_IDE1CTL_BAR, 2 }
> +};
>  
>  MODULE_AUTHOR("Jeff Garzik");
>  MODULE_DESCRIPTION("low-level driver for Silicon Image SATA controller");
> @@ -248,6 +264,9 @@ static int slow_down;
>  module_param(slow_down, int, 0444);
>  MODULE_PARM_DESC(slow_down, "Sledgehammer used to work around random problems, by limiting commands to 15 sectors (0=off, 1=on)");
>  
> +static int tfio;
> +module_param(tfio, int, 0444);
> +MODULE_PARM_DESC(tfio, "Access Taskfile registers via IO space (0=off, 1=on)");
>  
>  static unsigned char sil_get_device_cache_line(struct pci_dev *pdev)
>  {
> @@ -630,7 +649,17 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (rc)
>  		return rc;
>  
> -	rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
> +	if (tfio)
> +		rc = pcim_iomap_regions(pdev,
> +					(1 << SIL_IO_IDE0TF_BAR) |
> +					(1 << SIL_IO_IDE0CTL_BAR) |
> +					(1 << SIL_IO_IDE1TF_BAR) |
> +					(1 << SIL_IO_IDE1CTL_BAR) |
> +					(1 << SIL_MMIO_BAR),
> +					DRV_NAME);
> +	else
> +		rc = pcim_iomap_regions(pdev, 1 << SIL_MMIO_BAR, DRV_NAME);
> +

You can move pcim_iomap_regions() downward such that
pcim_iomap_regions() and ioaddr initialization can live in the same
if/else blocks.

>  	if (rc == -EBUSY)
>  		pcim_pin_device(pdev);
>  	if (rc)
> @@ -649,16 +678,32 @@ static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
>  		struct ata_ioports *ioaddr = &ap->ioaddr;
> -
> -		ioaddr->cmd_addr = mmio_base + sil_port[i].tf;
> -		ioaddr->altstatus_addr =
> -		ioaddr->ctl_addr = mmio_base + sil_port[i].ctl;
> -		ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
> -		ioaddr->scr_addr = mmio_base + sil_port[i].scr;
> -		ata_sff_std_ports(ioaddr);
> -
> -		ata_port_pbar_desc(ap, SIL_MMIO_BAR, -1, "mmio");
> -		ata_port_pbar_desc(ap, SIL_MMIO_BAR, sil_port[i].tf, "tf");
> +		void __iomem *tf_iobase = host->iomap[sil_ioport[i].tfbar];
> +		void __iomem *ctl_iobase = host->iomap[sil_ioport[i].ctlbar];

Please put a blank line here too.

> +		if (tfio) {
> +			ioaddr->cmd_addr = tf_iobase + sil_ioport[i].tf;
> +			ioaddr->altstatus_addr =
> +			ioaddr->ctl_addr = ctl_iobase + sil_ioport[i].ctl;
> +			ioaddr->bmdma_addr = mmio_base + sil_port[i].bmdma;
> +			ioaddr->scr_addr = mmio_base + sil_port[i].scr;

Hmm... so, mmio access to tf and ctl don't work but bmdma and scr are
okay?  Ah... it's those odd bytes accesses, right.  Anyways, please
put a brief comment explanining why it's useful and
ata_port_pbar_desc() or a dev_printk() to indicate that tfio mode is
in use.

Thanks.

-- 
tejun

Re: [PATCH 0/1] sata_sil: Option to use IO space to access TF registers.

[Alan Cox]

On Thu, 30 Oct 2008 22:58:53 +0000
Chris Dearman <chris@linux-mips.org> wrote:

> On Thu, Oct 23, 2008 at 08:32:15AM +0100, Alan Cox wrote:
> > I would make the switch a runtime value personally - some network drivers
> > do this for similar reasons.
> 
> OK. Something like this?

Yep something exactly like that. Looks good to me

Alan