Hello,
Just some nitpickings.
Chris Dearman wrote:
> sata_sil: Option to use IO space to access TF registers.
>
> From: Chris Dearman <chris@xxxxxxxxxxxxxx>
>
> Provide an alternative way to access the TaskFile registers if mmio
> doesn't work.
>
> Signed-off-by: Chris Dearman <chris@xxxxxxxxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
