sata_sil: Option to use IO space to access TF registers.
cover sata_sil: Option to use IO space to access TF registers.
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.
Provide an alternative way to access the TaskFile registers if mmio doesn't work.
---
drivers/ata/Kconfig | 12 ++++++++++++ drivers/ata/sata_sil.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 0 deletions(-)
drivers/ata/Kconfig +12 -0
@@ -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
drivers/ata/sata_sil.c +43 -0
75 lines not on this page · raw message
Quoted 4 lines
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
Hi Alan,
Quoted 2 lines
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
Quoted 3 lines
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 ?
Quoted 3 lines
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
Quoted 6 lines · Alan Cox
Quoted 3 lines
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.
Signature · 1 line
tejun
Quoted 2 lines · Alan Cox
I would make the switch a runtime value personally - some network drivers do this for similar reasons.
OK. Something like this?
Chris
sata_sil: Option to use IO space to access TF registers.
From: Chris Dearman <[email protected]>
Provide an alternative way to access the TaskFile registers if mmio doesn't work.
---
drivers/ata/sata_sil.c | 67 ++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 56 insertions(+), 11 deletions(-)
drivers/ata/sata_sil.c +56 -11
@@ -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 */
Hello,
Just some nitpickings.
Quoted 30 lines · Chris Dearman
sata_sil: Option to use IO space to access TF registers.From: Chris Dearman <[email protected]>
Provide an alternative way to access the TaskFile registers if mmio doesn't work.
Signed-off-by Chris Dearman <[email protected]>---
drivers/ata/sata_sil.c | 67 ++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 56 insertions(+), 11 deletions(-)drivers/ata/sata_sil.c +4
-0@@ -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 ,/*
drivers/ata/sata_sil.c static const struct { · line 243
Quoted 3 lines
drivers/ata/sata_sil.c +0
-0@@ -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?
Quoted 31 lines
drivers/ata/sata_sil.c +16
-1+/* 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)
drivers/ata/sata_sil.c static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 662
Quoted 10 lines
drivers/ata/sata_sil.c +10
-0+ 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.
Quoted 9 lines
drivers/ata/sata_sil.c +0
-2if (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;
drivers/ata/sata_sil.c static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 682
Quoted 10 lines
drivers/ata/sata_sil.c +2
-8- 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.
drivers/ata/sata_sil.c static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 688
Quoted 6 lines
drivers/ata/sata_sil.c +6
-0+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.
Signature · 1 line
tejun
Quoted 5 lines · On Thu, 30 Oct 2008 22:58:53 +0000 Chris Dearman <[email protected]> wrote:
Quoted 2 lines · Alan Cox
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