2009-09-12 02:07:30

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

From: John(Jung-Ik) Lee <[email protected]>

This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA ctrls.
Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide codes
from Eric Uhrhane <[email protected]>.

Signed-off-by: John(Jung-Ik) Lee <[email protected]>
Reviewed-by: Grant Grundler <[email protected]>
Reviewed-by: Gwendal Gringo <[email protected]>
---

drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/pata_atp867x.c | 584 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 2 +
4 files changed, 596 insertions(+), 0 deletions(-)
create mode 100644 drivers/ata/pata_atp867x.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b17c57f..2e1e678 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -244,6 +244,15 @@ config PATA_ARTOP

If unsure, say N.

+config PATA_ATP867X
+ tristate "ARTOP/Acard ATP867X PATA support"
+ depends on PCI
+ help
+ This option enables support for ARTOP/Acard ATP867X PATA
+ controllers.
+
+ If unsure, say N.
+
config PATA_AT32
tristate "Atmel AVR32 PATA support (Experimental)"
depends on AVR32 && PLATFORM_AT32AP && EXPERIMENTAL
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 38906f9..3372007 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SATA_FSL) += sata_fsl.o
obj-$(CONFIG_PATA_ALI) += pata_ali.o
obj-$(CONFIG_PATA_AMD) += pata_amd.o
obj-$(CONFIG_PATA_ARTOP) += pata_artop.o
+obj-$(CONFIG_PATA_ATP867X) += pata_atp867x.o
obj-$(CONFIG_PATA_AT32) += pata_at32.o
obj-$(CONFIG_PATA_ATIIXP) += pata_atiixp.o
obj-$(CONFIG_PATA_CMD640_PCI) += pata_cmd640.o
diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
new file mode 100644
index 0000000..3a93a7a
--- /dev/null
+++ b/drivers/ata/pata_atp867x.c
@@ -0,0 +1,584 @@
+/*
+ * pata_atp867x.c - ARTOP 867X 64bit 4-channel UDMA133 ATA controller driver
+ *
+ * (C) 2009 Google Inc. John(Jung-Ik) Lee <[email protected]>
+ *
+ * Per Atp867 data sheet rev 1.2, Acard.
+ * Based in part on early ide code from
+ * 2003-2004 by Eric Uhrhane, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ *
+ * TODO:
+ * 1. RAID features [comparison, XOR, striping, mirroring, etc.]
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <scsi/scsi_host.h>
+#include <linux/libata.h>
+
+#define DRV_NAME "pata_atp867x"
+#define DRV_VERSION "0.7.5"
+
+/*
+ * IO Registers
+ * Note that all runtime hot ports are cached in ap private_data
+ */
+
+#define ATP867X_IO_CHANNEL_OFFSET 0x10
+#define ATP867X_IOBASE(d) pci_resource_start((d), 0)
+#define ATP867X_SYS_INFO(d) (0x3F + ATP867X_IOBASE(d))
+
+#define ATP867X_IO_PORTBASE(d, port) (0x00 + ATP867X_IOBASE(d) + \
+ (port) * ATP867X_IO_CHANNEL_OFFSET)
+#define ATP867X_IO_DMABASE(d, port) (0x40 + ATP867X_IO_PORTBASE((d), (port)))
+
+#define ATP867X_IO_STATUS(d, port) (0x07 + ATP867X_IO_PORTBASE((d),(port)))
+#define ATP867X_IO_ALTSTATUS(d, port) (0x0E + ATP867X_IO_PORTBASE((d),(port)))
+
+#define ATP867X_IO_MSTRPIOSPD(d, port) (0x08 + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_SLAVPIOSPD(d, port) (0x09 + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_8BPIOSPD(d, port) (0x0A + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_DMAMODE(d, port) (0x0B + ATP867X_IO_DMABASE((d), (port)))
+
+#define ATP867X_IO_PORTSPEED(d, port) (0x4A + ATP867X_IO_PORTBASE((d), (port)))
+#define ATP867X_IO_PREREAD(d, port) (0x4C + ATP867X_IO_PORTBASE((d), (port)))
+
+/*
+ * IO Register Bitfields
+ */
+
+#define ATP867X_IO_PIOSPD_ACTIVE_SHIFT 4
+#define ATP867X_IO_PIOSPD_RECOVER_SHIFT 0
+
+#define ATP867X_IO_DMAMODE_MSTR_SHIFT 0
+#define ATP867X_IO_DMAMODE_MSTR_MASK 0x07
+#define ATP867X_IO_DMAMODE_SLAVE_SHIFT 4
+#define ATP867X_IO_DMAMODE_SLAVE_MASK 0x70
+
+#define ATP867X_IO_DMAMODE_UDMA_6 0x07
+#define ATP867X_IO_DMAMODE_UDMA_5 0x06
+#define ATP867X_IO_DMAMODE_UDMA_4 0x05
+#define ATP867X_IO_DMAMODE_UDMA_3 0x04
+#define ATP867X_IO_DMAMODE_UDMA_2 0x03
+#define ATP867X_IO_DMAMODE_UDMA_1 0x02
+#define ATP867X_IO_DMAMODE_UDMA_0 0x01
+#define ATP867X_IO_DMAMODE_DISABLE 0x00
+
+#define ATP867X_IO_SYS_INFO_66MHZ 0x04
+#define ATP867X_IO_SYS_INFO_SLOW_UDMA5 0x02
+#define ATP867X_IO_SYS_MASK_RESERVED ~0xf1
+
+#define ATP867X_IO_PORTSPEED_VAL 0x1143
+#define ATP867X_PREREAD_VAL 0x0200
+
+#define ATP867X_NUM_PORTS 4
+#define ATP867X_BAR_IOBASE 0
+#define ATP867X_BAR_ROMBASE 6
+
+struct atp867x_priv {
+ unsigned long dma_mode;
+ unsigned long mstr_piospd;
+ unsigned long slave_piospd;
+ unsigned long eightb_piospd;
+ int pci66mhz;
+};
+
+
+static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->dma_mode;
+ u8 b;
+ u8 mode;
+
+
+ switch (speed) {
+ case XFER_UDMA_6:
+ mode = ATP867X_IO_DMAMODE_UDMA_6;
+ break;
+ case XFER_UDMA_5:
+ mode = ATP867X_IO_DMAMODE_UDMA_5;
+ break;
+ case XFER_UDMA_4:
+ mode = ATP867X_IO_DMAMODE_UDMA_4;
+ break;
+ case XFER_UDMA_3:
+ mode = ATP867X_IO_DMAMODE_UDMA_3;
+ break;
+ case XFER_UDMA_2:
+ mode = ATP867X_IO_DMAMODE_UDMA_2;
+ break;
+ case XFER_UDMA_1:
+ mode = ATP867X_IO_DMAMODE_UDMA_1;
+ break;
+ case XFER_UDMA_0:
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_UDMA_0.\n", (unsigned)speed);
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ }
+
+ /*
+ * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
+ * on 66MHz bus
+ * rev-A: UDMA_1~4 (5, 6 no change)
+ * rev-B: all UDMA modes
+ * UDMA_0 stays not to disable UDMA
+ */
+ if (dp->pci66mhz && mode > ATP867X_IO_DMAMODE_UDMA_0 &&
+ (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
+ mode < ATP867X_IO_DMAMODE_UDMA_5))
+ mode--;
+
+ b = inb(dp->dma_mode);
+ if (adev->devno & 1) {
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
+ (mode << ATP867X_IO_DMAMODE_SLAVE_SHIFT);
+ } else {
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK) |
+ (mode << ATP867X_IO_DMAMODE_MSTR_SHIFT);
+ }
+ outb(b, dp->dma_mode);
+}
+
+#define ATP867X_NO_HACK_PIOMODE
+#ifdef ATP867X_NO_HACK_PIOMODE
+static int atp867x_get_active_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 7:
+ break;
+ case 8 ... 12:
+ clocks = 7;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
+ "Using default 8clk.\n", clk);
+ clocks = 0; /* 8 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
+}
+
+static int atp867x_get_recover_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 11:
+ break;
+ case 12:
+ clocks = 0;
+ break;
+ case 13: case 14:
+ --clocks;
+ break;
+ case 15:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
+ "Using default 15clk.\n", clk);
+ clocks = 0; /* 12 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_RECOVER_SHIFT;
+}
+#endif
+
+static void atp867x_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct ata_device *peer = ata_dev_pair(adev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->pio_mode;
+ struct ata_timing t, p;
+ int T, UT;
+ u8 b;
+
+ T = 1000000000 / 33333;
+ UT = T/4;
+
+ switch (speed) {
+ case XFER_PIO_4:
+ case XFER_PIO_3:
+ case XFER_PIO_2:
+ case XFER_PIO_1:
+ case XFER_PIO_0:
+ case XFER_PIO_SLOW:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_PIO_0.\n", (unsigned)speed);
+ speed = XFER_PIO_0;
+ }
+
+ ata_timing_compute(adev, speed, &t, T, UT);
+ if (peer && peer->pio_mode) {
+ ata_timing_compute(peer, peer->pio_mode, &p, T, UT);
+ ata_timing_merge(&p, &t, &t, ATA_TIMING_8BIT);
+ }
+
+ b = inb(dp->dma_mode);
+ if (adev->devno & 1) {
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK);
+ } else {
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK);
+ }
+ outb(b, dp->dma_mode);
+
+#ifdef ATP867X_NO_HACK_PIOMODE
+ b = atp867x_get_active_clocks_shifted(t.active) |
+ atp867x_get_recover_clocks_shifted(t.recover);
+#else
+ /*
+ * magic value that works (from doc 6.4, 6.6.9)
+ */
+ b = 0x31;
+#endif
+ if (dp->pci66mhz)
+ b += 0x10;
+
+ if (adev->devno & 1) {
+ outb(b, dp->slave_piospd);
+ } else {
+ outb(b, dp->mstr_piospd);
+ }
+
+ /*
+ * use the same value for comand timing as for PIO timimg
+ */
+ outb(b, dp->eightb_piospd);
+}
+
+static int atp867x_cable_detect(struct ata_port *ap)
+{
+ return ATA_CBL_PATA40_SHORT;
+}
+
+static struct scsi_host_template atp867x_sht = {
+ ATA_BMDMA_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations atp867x_ops = {
+ .inherits = &ata_bmdma_port_ops,
+ .cable_detect = atp867x_cable_detect,
+ .set_piomode = atp867x_set_piomode,
+ .set_dmamode = atp867x_set_dmamode,
+};
+
+
+#ifdef ATP867X_DEBUG
+static void atp867x_check_res(struct pci_dev *pdev)
+{
+ int i;
+ unsigned long start, len;
+
+ /* Check the PCI resources for this channel are enabled */
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i ++) {
+ start = pci_resource_start(pdev, i);
+ len = pci_resource_len(pdev, i);
+ printk("%s: start:len=%lx:%lx\n", __func__, start, len);
+ }
+}
+
+static void atp867x_check_ports(struct ata_port *ap, struct pci_dev *pdev)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+ struct atp867x_priv *dp = ap->private_data;
+ int port = ap->port_no;
+
+ printk("atp867x: port[%d] addresses\n"
+ " cmd_addr =0x%llx, 0x%llx\n"
+ " ctl_addr =0x%llx, 0x%llx\n"
+ " bmdma_addr =0x%llx, 0x%llx\n"
+ " data_addr =0x%llx\n"
+ " error_addr =0x%llx\n"
+ " feature_addr =0x%llx\n"
+ " nsect_addr =0x%llx\n"
+ " lbal_addr =0x%llx\n"
+ " lbam_addr =0x%llx\n"
+ " lbah_addr =0x%llx\n"
+ " device_addr =0x%llx\n"
+ " status_addr =0x%llx\n"
+ " command_addr =0x%llx\n"
+ " dp->dma_mode =0x%llx\n"
+ " dp->mstr_piospd =0x%llx\n"
+ " dp->slave_piospd =0x%llx\n"
+ " dp->eightb_piospd =0x%llx\n",
+ port,
+ (unsigned long long)ioaddr->cmd_addr,
+ ATP867X_IO_PORTBASE(pdev, port),
+ (unsigned long long)ioaddr->ctl_addr,
+ ATP867X_IO_ALTSTATUS(pdev, port),
+ (unsigned long long)ioaddr->bmdma_addr,
+ ATP867X_IO_DMABASE(pdev, port),
+ (unsigned long long)ioaddr->data_addr,
+ (unsigned long long)ioaddr->error_addr,
+ (unsigned long long)ioaddr->feature_addr,
+ (unsigned long long)ioaddr->nsect_addr,
+ (unsigned long long)ioaddr->lbal_addr,
+ (unsigned long long)ioaddr->lbam_addr,
+ (unsigned long long)ioaddr->lbah_addr,
+ (unsigned long long)ioaddr->device_addr,
+ (unsigned long long)ioaddr->status_addr,
+ (unsigned long long)ioaddr->command_addr,
+ (unsigned long long)dp->dma_mode,
+ (unsigned long long)dp->mstr_piospd,
+ (unsigned long long)dp->slave_piospd,
+ (unsigned long long)dp->eightb_piospd);
+}
+#endif
+
+static inline void atp867x_get_ports(struct pci_dev *dev, unsigned int port,
+ unsigned long *base, unsigned long *control)
+{
+ *base = ATP867X_IO_PORTBASE(dev, port);
+ *control = ATP867X_IO_ALTSTATUS(dev, port);
+}
+
+static inline unsigned long atp867x_get_dma_base(struct pci_dev *dev,
+ unsigned int port)
+{
+ return ATP867X_IO_DMABASE(dev, port);
+}
+
+static int atp867x_set_priv(struct ata_port *ap)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp;
+ int port = ap->port_no;
+
+ dp = ap->private_data =
+ devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
+ if (dp == NULL)
+ return -ENOMEM;
+
+ dp->dma_mode = ATP867X_IO_DMAMODE(pdev, port);
+ dp->mstr_piospd = ATP867X_IO_MSTRPIOSPD(pdev, port);
+ dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(pdev, port);
+ dp->eightb_piospd= ATP867X_IO_8BPIOSPD(pdev, port);
+
+ dp->pci66mhz = inb(ATP867X_SYS_INFO(pdev)) & ATP867X_IO_SYS_INFO_66MHZ;
+
+ return 0;
+}
+
+static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
+{
+ struct device *gdev = host->dev;
+ struct pci_dev *pdev = to_pci_dev(gdev);
+ unsigned int mask = 0;
+ int i, rc;
+
+ /*
+ * do not map rombase
+ */
+ rc = pcim_iomap_regions(pdev, 1 << ATP867X_BAR_IOBASE, DRV_NAME);
+ if (rc == -EBUSY)
+ pcim_pin_device(pdev);
+ if (rc)
+ return rc;
+ host->iomap = pcim_iomap_table(pdev);
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_res(pdev);
+
+ for (i = 0; i < PCI_ROM_RESOURCE; i++)
+ printk("atp867x: iomap[%d]=0x%llx\n", i,
+ (unsigned long long)(host->iomap[i]));
+#endif
+
+ /*
+ * request, iomap BARs and init port addresses accordingly
+ */
+ for (i = 0; i < host->n_ports; i++) {
+ unsigned long base, control, dmabase;
+ struct ata_port *ap = host->ports[i];
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ atp867x_get_ports(pdev, i, &base, &control);
+ dmabase = atp867x_get_dma_base(pdev, i);
+
+ ioaddr->cmd_addr = devm_ioport_map(gdev, base, 8);
+ ioaddr->ctl_addr = ioaddr->altstatus_addr =
+ devm_ioport_map(gdev, control, 1);
+ ioaddr->bmdma_addr = devm_ioport_map(gdev, dmabase, 8);
+
+ ata_sff_std_ports(ioaddr);
+ rc = atp867x_set_priv(ap);
+ if (rc)
+ return rc;
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_ports(ap, pdev);
+#endif
+ ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", base, control);
+ ata_port_desc(ap, "bmdma 0x%lx", dmabase);
+
+ mask |= 1 << i;
+ }
+
+ if (!mask) {
+ dev_printk(KERN_ERR, gdev, "no available native port\n");
+ return -ENODEV;
+ }
+
+ rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+ if (rc)
+ return rc;
+
+ rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+ return rc;
+}
+
+static int atp867x_init_one(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ static int printed_version;
+ static const struct ata_port_info info_867x = {
+ .flags = ATA_FLAG_SLAVE_POSS,
+ .pio_mask = ATA_PIO4,
+ .mwdma_mask = ATA_MWDMA2,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &atp867x_ops,
+ };
+
+ struct ata_host *host;
+ const struct ata_port_info *ppi[] = { &info_867x, NULL };
+ int i, rc;
+ u8 v;
+
+ if (!printed_version++)
+ dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
+
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
+ /*
+ * Broken BIOS might not set latency high enough
+ */
+ pci_read_config_byte(pdev, PCI_LATENCY_TIMER, &v);
+ if (v < 0x80) {
+ v = 0x80;
+ pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
+ printk(KERN_DEBUG "ATP867X: set latency timer of device %s"
+ " to %d\n", pci_name(pdev), v);
+ }
+
+ /*
+ * init 8bit io ports speed(0aaarrrr) to 43h and
+ * init udma modes of master/slave to 0/0(11h)
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ outw(ATP867X_IO_PORTSPEED_VAL, ATP867X_IO_PORTSPEED(pdev, i));
+
+ /*
+ * init PreREAD counts
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ outw(ATP867X_PREREAD_VAL, ATP867X_IO_PREREAD(pdev, i));
+
+ v = inb(ATP867X_IOBASE(pdev) + 0x28);
+ v &= 0xcf; /* Enable INTA#: bit4=0 means enable */
+ v |= 0xc0; /* Enable PCI burst, MRM & not immediate interrupts */
+ outb(v, ATP867X_IOBASE(pdev) + 0x28);
+
+ /*
+ * Turn off the over clocked udma5 mode, only for Rev-B
+ */
+ v = inb(ATP867X_SYS_INFO(pdev));
+ v &= ATP867X_IO_SYS_MASK_RESERVED;
+ if (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B)
+ v |= ATP867X_IO_SYS_INFO_SLOW_UDMA5;
+ outb(v, ATP867X_SYS_INFO(pdev));
+
+ printk(KERN_INFO "ATP867X: ATP867 ATA UDMA133 controller (rev %02X)",
+ pdev->device);
+
+ host = ata_host_alloc_pinfo(&pdev->dev, ppi, ATP867X_NUM_PORTS);
+ if (!host) {
+ dev_printk(KERN_ERR, &pdev->dev,
+ "failed to allocate ATA host\n");
+ rc = -ENOMEM;
+ goto err_out;
+ }
+
+ rc = atp867x_ata_pci_sff_init_host(host);
+ if (rc) {
+ dev_printk(KERN_ERR, &pdev->dev, "failed to init host\n");
+ goto err_out;
+ }
+
+ pci_set_master(pdev);
+
+ rc = ata_host_activate(host, pdev->irq, ata_sff_interrupt,
+ IRQF_SHARED, &atp867x_sht);
+ if (rc) {
+ dev_printk(KERN_ERR, &pdev->dev, "failed to activate host\n");
+ }
+err_out:
+ return rc;
+}
+
+static struct pci_device_id atp867x_pci_tbl[] = {
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867A), 0 },
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867B), 0 },
+ { },
+};
+
+static struct pci_driver atp867x_driver = {
+ .name = DRV_NAME,
+ .id_table = atp867x_pci_tbl,
+ .probe = atp867x_init_one,
+ .remove = ata_pci_remove_one,
+};
+
+static int __init atp867x_init(void)
+{
+ return pci_register_driver(&atp867x_driver);
+}
+
+static void __exit atp867x_exit(void)
+{
+ pci_unregister_driver(&atp867x_driver);
+}
+
+MODULE_AUTHOR("John(Jung-Ik) Lee, Google Inc.");
+MODULE_DESCRIPTION("low level driver for Artop/Acard 867x ATA controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, atp867x_pci_tbl);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(atp867x_init);
+module_exit(atp867x_exit);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 73b46b6..1de1fab 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1521,6 +1521,8 @@
#define PCI_DEVICE_ID_ARTOP_ATP860R 0x0007
#define PCI_DEVICE_ID_ARTOP_ATP865 0x0008
#define PCI_DEVICE_ID_ARTOP_ATP865R 0x0009
+#define PCI_DEVICE_ID_ARTOP_ATP867A 0x000A
+#define PCI_DEVICE_ID_ARTOP_ATP867B 0x000B
#define PCI_DEVICE_ID_ARTOP_AEC7610 0x8002
#define PCI_DEVICE_ID_ARTOP_AEC7612UW 0x8010
#define PCI_DEVICE_ID_ARTOP_AEC7612U 0x8020


2009-09-12 02:13:28

by Daniel Walker

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Fri, 2009-09-11 at 19:06 -0700, Jung-Ik (John) Lee wrote:
> From: John(Jung-Ik) Lee <[email protected]>
>
> This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA
> ctrls.
> Based on the Atp867 data sheet rev 1.2, Acard, and in part on early
> ide codes
> from Eric Uhrhane <[email protected]>.

This driver has numerous checkpatch errors .. Could you run this though
scripts/checkpatch.pl and correct anything it finds?

Daniel

2009-09-12 10:59:49

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

From: John(Jung-Ik) Lee <[email protected]>

This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA ctrls.
Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide codes
from Eric Uhrhane <[email protected]>.

Signed-off-by: John(Jung-Ik) Lee <[email protected]>
Reviewed-by: Grant Grundler <[email protected]>
Reviewed-by: Gwendal Gringo <[email protected]>
---

Cleared some checkpatch.pl complaints. Thanks to Daniel.

drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/pata_atp867x.c | 588 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 2 +
4 files changed, 600 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b17c57f..2e1e678 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -244,6 +244,15 @@ config PATA_ARTOP

If unsure, say N.

+config PATA_ATP867X
+ tristate "ARTOP/Acard ATP867X PATA support"
+ depends on PCI
+ help
+ This option enables support for ARTOP/Acard ATP867X PATA
+ controllers.
+
+ If unsure, say N.
+
config PATA_AT32
tristate "Atmel AVR32 PATA support (Experimental)"
depends on AVR32 && PLATFORM_AT32AP && EXPERIMENTAL
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 38906f9..3372007 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SATA_FSL) += sata_fsl.o
obj-$(CONFIG_PATA_ALI) += pata_ali.o
obj-$(CONFIG_PATA_AMD) += pata_amd.o
obj-$(CONFIG_PATA_ARTOP) += pata_artop.o
+obj-$(CONFIG_PATA_ATP867X) += pata_atp867x.o
obj-$(CONFIG_PATA_AT32) += pata_at32.o
obj-$(CONFIG_PATA_ATIIXP) += pata_atiixp.o
obj-$(CONFIG_PATA_CMD640_PCI) += pata_cmd640.o
diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
new file mode 100644
index 0000000..c468254
--- /dev/null
+++ b/drivers/ata/pata_atp867x.c
@@ -0,0 +1,588 @@
+/*
+ * pata_atp867x.c - ARTOP 867X 64bit 4-channel UDMA133 ATA controller driver
+ *
+ * (C) 2009 Google Inc. John(Jung-Ik) Lee <[email protected]>
+ *
+ * Per Atp867 data sheet rev 1.2, Acard.
+ * Based in part on early ide code from
+ * 2003-2004 by Eric Uhrhane, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ *
+ * TODO:
+ * 1. RAID features [comparison, XOR, striping, mirroring, etc.]
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <scsi/scsi_host.h>
+#include <linux/libata.h>
+
+#define DRV_NAME "pata_atp867x"
+#define DRV_VERSION "0.7.5"
+
+/*
+ * IO Registers
+ * Note that all runtime hot ports are cached in ap private_data
+ */
+
+#define ATP867X_IO_CHANNEL_OFFSET 0x10
+#define ATP867X_IOBASE(d) pci_resource_start((d), 0)
+#define ATP867X_SYS_INFO(d) (0x3F + ATP867X_IOBASE(d))
+
+#define ATP867X_IO_PORTBASE(d, port) (0x00 + ATP867X_IOBASE(d) + \
+ (port) * ATP867X_IO_CHANNEL_OFFSET)
+#define ATP867X_IO_DMABASE(d, port) (0x40 + \
+ ATP867X_IO_PORTBASE((d), (port)))
+
+#define ATP867X_IO_STATUS(d, port) (0x07 + \
+ ATP867X_IO_PORTBASE((d), (port)))
+#define ATP867X_IO_ALTSTATUS(d, port) (0x0E + \
+ ATP867X_IO_PORTBASE((d), (port)))
+
+#define ATP867X_IO_MSTRPIOSPD(d, port) (0x08 + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_SLAVPIOSPD(d, port) (0x09 + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_8BPIOSPD(d, port) (0x0A + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_DMAMODE(d, port) (0x0B + ATP867X_IO_DMABASE((d), (port)))
+
+#define ATP867X_IO_PORTSPEED(d, port) (0x4A + \
+ ATP867X_IO_PORTBASE((d), (port)))
+#define ATP867X_IO_PREREAD(d, port) (0x4C + \
+ ATP867X_IO_PORTBASE((d), (port)))
+
+/*
+ * IO Register Bitfields
+ */
+
+#define ATP867X_IO_PIOSPD_ACTIVE_SHIFT 4
+#define ATP867X_IO_PIOSPD_RECOVER_SHIFT 0
+
+#define ATP867X_IO_DMAMODE_MSTR_SHIFT 0
+#define ATP867X_IO_DMAMODE_MSTR_MASK 0x07
+#define ATP867X_IO_DMAMODE_SLAVE_SHIFT 4
+#define ATP867X_IO_DMAMODE_SLAVE_MASK 0x70
+
+#define ATP867X_IO_DMAMODE_UDMA_6 0x07
+#define ATP867X_IO_DMAMODE_UDMA_5 0x06
+#define ATP867X_IO_DMAMODE_UDMA_4 0x05
+#define ATP867X_IO_DMAMODE_UDMA_3 0x04
+#define ATP867X_IO_DMAMODE_UDMA_2 0x03
+#define ATP867X_IO_DMAMODE_UDMA_1 0x02
+#define ATP867X_IO_DMAMODE_UDMA_0 0x01
+#define ATP867X_IO_DMAMODE_DISABLE 0x00
+
+#define ATP867X_IO_SYS_INFO_66MHZ 0x04
+#define ATP867X_IO_SYS_INFO_SLOW_UDMA5 0x02
+#define ATP867X_IO_SYS_MASK_RESERVED (~0xf1)
+
+#define ATP867X_IO_PORTSPEED_VAL 0x1143
+#define ATP867X_PREREAD_VAL 0x0200
+
+#define ATP867X_NUM_PORTS 4
+#define ATP867X_BAR_IOBASE 0
+#define ATP867X_BAR_ROMBASE 6
+
+struct atp867x_priv {
+ unsigned long dma_mode;
+ unsigned long mstr_piospd;
+ unsigned long slave_piospd;
+ unsigned long eightb_piospd;
+ int pci66mhz;
+};
+
+
+static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->dma_mode;
+ u8 b;
+ u8 mode;
+
+
+ switch (speed) {
+ case XFER_UDMA_6:
+ mode = ATP867X_IO_DMAMODE_UDMA_6;
+ break;
+ case XFER_UDMA_5:
+ mode = ATP867X_IO_DMAMODE_UDMA_5;
+ break;
+ case XFER_UDMA_4:
+ mode = ATP867X_IO_DMAMODE_UDMA_4;
+ break;
+ case XFER_UDMA_3:
+ mode = ATP867X_IO_DMAMODE_UDMA_3;
+ break;
+ case XFER_UDMA_2:
+ mode = ATP867X_IO_DMAMODE_UDMA_2;
+ break;
+ case XFER_UDMA_1:
+ mode = ATP867X_IO_DMAMODE_UDMA_1;
+ break;
+ case XFER_UDMA_0:
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_UDMA_0.\n", (unsigned)speed);
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ }
+
+ /*
+ * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
+ * on 66MHz bus
+ * rev-A: UDMA_1~4 (5, 6 no change)
+ * rev-B: all UDMA modes
+ * UDMA_0 stays not to disable UDMA
+ */
+ if (dp->pci66mhz && mode > ATP867X_IO_DMAMODE_UDMA_0 &&
+ (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
+ mode < ATP867X_IO_DMAMODE_UDMA_5))
+ mode--;
+
+ b = inb(dp->dma_mode);
+ if (adev->devno & 1) {
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
+ (mode << ATP867X_IO_DMAMODE_SLAVE_SHIFT);
+ } else {
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK) |
+ (mode << ATP867X_IO_DMAMODE_MSTR_SHIFT);
+ }
+ outb(b, dp->dma_mode);
+}
+
+#define ATP867X_NO_HACK_PIOMODE
+#ifdef ATP867X_NO_HACK_PIOMODE
+static int atp867x_get_active_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 7:
+ break;
+ case 8 ... 12:
+ clocks = 7;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
+ "Using default 8clk.\n", clk);
+ clocks = 0; /* 8 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
+}
+
+static int atp867x_get_recover_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 11:
+ break;
+ case 12:
+ clocks = 0;
+ break;
+ case 13: case 14:
+ --clocks;
+ break;
+ case 15:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
+ "Using default 15clk.\n", clk);
+ clocks = 0; /* 12 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_RECOVER_SHIFT;
+}
+#endif
+
+static void atp867x_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct ata_device *peer = ata_dev_pair(adev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->pio_mode;
+ struct ata_timing t, p;
+ int T, UT;
+ u8 b;
+
+ T = 1000000000 / 33333;
+ UT = T/4;
+
+ switch (speed) {
+ case XFER_PIO_4:
+ case XFER_PIO_3:
+ case XFER_PIO_2:
+ case XFER_PIO_1:
+ case XFER_PIO_0:
+ case XFER_PIO_SLOW:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_PIO_0.\n", (unsigned)speed);
+ speed = XFER_PIO_0;
+ }
+
+ ata_timing_compute(adev, speed, &t, T, UT);
+ if (peer && peer->pio_mode) {
+ ata_timing_compute(peer, peer->pio_mode, &p, T, UT);
+ ata_timing_merge(&p, &t, &t, ATA_TIMING_8BIT);
+ }
+
+ b = inb(dp->dma_mode);
+ if (adev->devno & 1)
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK);
+ else
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK);
+ outb(b, dp->dma_mode);
+
+#ifdef ATP867X_NO_HACK_PIOMODE
+ b = atp867x_get_active_clocks_shifted(t.active) |
+ atp867x_get_recover_clocks_shifted(t.recover);
+#else
+ /*
+ * magic value that works (from doc 6.4, 6.6.9)
+ */
+ b = 0x31;
+#endif
+ if (dp->pci66mhz)
+ b += 0x10;
+
+ if (adev->devno & 1)
+ outb(b, dp->slave_piospd);
+ else
+ outb(b, dp->mstr_piospd);
+
+ /*
+ * use the same value for comand timing as for PIO timimg
+ */
+ outb(b, dp->eightb_piospd);
+}
+
+static int atp867x_cable_detect(struct ata_port *ap)
+{
+ return ATA_CBL_PATA40_SHORT;
+}
+
+static struct scsi_host_template atp867x_sht = {
+ ATA_BMDMA_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations atp867x_ops = {
+ .inherits = &ata_bmdma_port_ops,
+ .cable_detect = atp867x_cable_detect,
+ .set_piomode = atp867x_set_piomode,
+ .set_dmamode = atp867x_set_dmamode,
+};
+
+
+#ifdef ATP867X_DEBUG
+static void atp867x_check_res(struct pci_dev *pdev)
+{
+ int i;
+ unsigned long start, len;
+
+ /* Check the PCI resources for this channel are enabled */
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ start = pci_resource_start(pdev, i);
+ len = pci_resource_len(pdev, i);
+ printk(KERN_DEBUG "ATP867X: resource start:len=%lx:%lx\n",
+ start, len);
+ }
+}
+
+static void atp867x_check_ports(struct ata_port *ap, struct pci_dev *pdev)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+ struct atp867x_priv *dp = ap->private_data;
+ int port = ap->port_no;
+
+ printk(KERN_DEBUG "ATP867X: port[%d] addresses\n"
+ " cmd_addr =0x%llx, 0x%llx\n"
+ " ctl_addr =0x%llx, 0x%llx\n"
+ " bmdma_addr =0x%llx, 0x%llx\n"
+ " data_addr =0x%llx\n"
+ " error_addr =0x%llx\n"
+ " feature_addr =0x%llx\n"
+ " nsect_addr =0x%llx\n"
+ " lbal_addr =0x%llx\n"
+ " lbam_addr =0x%llx\n"
+ " lbah_addr =0x%llx\n"
+ " device_addr =0x%llx\n"
+ " status_addr =0x%llx\n"
+ " command_addr =0x%llx\n"
+ " dp->dma_mode =0x%llx\n"
+ " dp->mstr_piospd =0x%llx\n"
+ " dp->slave_piospd =0x%llx\n"
+ " dp->eightb_piospd =0x%llx\n",
+ port,
+ (unsigned long long)ioaddr->cmd_addr,
+ ATP867X_IO_PORTBASE(pdev, port),
+ (unsigned long long)ioaddr->ctl_addr,
+ ATP867X_IO_ALTSTATUS(pdev, port),
+ (unsigned long long)ioaddr->bmdma_addr,
+ ATP867X_IO_DMABASE(pdev, port),
+ (unsigned long long)ioaddr->data_addr,
+ (unsigned long long)ioaddr->error_addr,
+ (unsigned long long)ioaddr->feature_addr,
+ (unsigned long long)ioaddr->nsect_addr,
+ (unsigned long long)ioaddr->lbal_addr,
+ (unsigned long long)ioaddr->lbam_addr,
+ (unsigned long long)ioaddr->lbah_addr,
+ (unsigned long long)ioaddr->device_addr,
+ (unsigned long long)ioaddr->status_addr,
+ (unsigned long long)ioaddr->command_addr,
+ (unsigned long long)dp->dma_mode,
+ (unsigned long long)dp->mstr_piospd,
+ (unsigned long long)dp->slave_piospd,
+ (unsigned long long)dp->eightb_piospd);
+}
+#endif
+
+static inline void atp867x_get_ports(struct pci_dev *dev, unsigned int port,
+ unsigned long *base, unsigned long *control)
+{
+ *base = ATP867X_IO_PORTBASE(dev, port);
+ *control = ATP867X_IO_ALTSTATUS(dev, port);
+}
+
+static inline unsigned long atp867x_get_dma_base(struct pci_dev *dev,
+ unsigned int port)
+{
+ return ATP867X_IO_DMABASE(dev, port);
+}
+
+static int atp867x_set_priv(struct ata_port *ap)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp;
+ int port = ap->port_no;
+
+ dp = ap->private_data =
+ devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
+ if (dp == NULL)
+ return -ENOMEM;
+
+ dp->dma_mode = ATP867X_IO_DMAMODE(pdev, port);
+ dp->mstr_piospd = ATP867X_IO_MSTRPIOSPD(pdev, port);
+ dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(pdev, port);
+ dp->eightb_piospd = ATP867X_IO_8BPIOSPD(pdev, port);
+
+ dp->pci66mhz = inb(ATP867X_SYS_INFO(pdev)) & ATP867X_IO_SYS_INFO_66MHZ;
+
+ return 0;
+}
+
+static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
+{
+ struct device *gdev = host->dev;
+ struct pci_dev *pdev = to_pci_dev(gdev);
+ unsigned int mask = 0;
+ int i, rc;
+
+ /*
+ * do not map rombase
+ */
+ rc = pcim_iomap_regions(pdev, 1 << ATP867X_BAR_IOBASE, DRV_NAME);
+ if (rc == -EBUSY)
+ pcim_pin_device(pdev);
+ if (rc)
+ return rc;
+ host->iomap = pcim_iomap_table(pdev);
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_res(pdev);
+
+ for (i = 0; i < PCI_ROM_RESOURCE; i++)
+ printk(KERN_DEBUG "ATP867X: iomap[%d]=0x%llx\n", i,
+ (unsigned long long)(host->iomap[i]));
+#endif
+
+ /*
+ * request, iomap BARs and init port addresses accordingly
+ */
+ for (i = 0; i < host->n_ports; i++) {
+ unsigned long base, control, dmabase;
+ struct ata_port *ap = host->ports[i];
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ atp867x_get_ports(pdev, i, &base, &control);
+ dmabase = atp867x_get_dma_base(pdev, i);
+
+ ioaddr->cmd_addr = devm_ioport_map(gdev, base, 8);
+ ioaddr->ctl_addr = ioaddr->altstatus_addr =
+ devm_ioport_map(gdev, control, 1);
+ ioaddr->bmdma_addr = devm_ioport_map(gdev, dmabase, 8);
+
+ ata_sff_std_ports(ioaddr);
+ rc = atp867x_set_priv(ap);
+ if (rc)
+ return rc;
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_ports(ap, pdev);
+#endif
+ ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", base, control);
+ ata_port_desc(ap, "bmdma 0x%lx", dmabase);
+
+ mask |= 1 << i;
+ }
+
+ if (!mask) {
+ dev_printk(KERN_ERR, gdev, "no available native port\n");
+ return -ENODEV;
+ }
+
+ rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+ if (rc)
+ return rc;
+
+ rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+ return rc;
+}
+
+static int atp867x_init_one(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ static int printed_version;
+ static const struct ata_port_info info_867x = {
+ .flags = ATA_FLAG_SLAVE_POSS,
+ .pio_mask = ATA_PIO4,
+ .mwdma_mask = ATA_MWDMA2,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &atp867x_ops,
+ };
+
+ struct ata_host *host;
+ const struct ata_port_info *ppi[] = { &info_867x, NULL };
+ int i, rc;
+ u8 v;
+
+ if (!printed_version++)
+ dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
+
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
+ /*
+ * Broken BIOS might not set latency high enough
+ */
+ pci_read_config_byte(pdev, PCI_LATENCY_TIMER, &v);
+ if (v < 0x80) {
+ v = 0x80;
+ pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
+ printk(KERN_DEBUG "ATP867X: set latency timer of device %s"
+ " to %d\n", pci_name(pdev), v);
+ }
+
+ /*
+ * init 8bit io ports speed(0aaarrrr) to 43h and
+ * init udma modes of master/slave to 0/0(11h)
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ outw(ATP867X_IO_PORTSPEED_VAL, ATP867X_IO_PORTSPEED(pdev, i));
+
+ /*
+ * init PreREAD counts
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ outw(ATP867X_PREREAD_VAL, ATP867X_IO_PREREAD(pdev, i));
+
+ v = inb(ATP867X_IOBASE(pdev) + 0x28);
+ v &= 0xcf; /* Enable INTA#: bit4=0 means enable */
+ v |= 0xc0; /* Enable PCI burst, MRM & not immediate interrupts */
+ outb(v, ATP867X_IOBASE(pdev) + 0x28);
+
+ /*
+ * Turn off the over clocked udma5 mode, only for Rev-B
+ */
+ v = inb(ATP867X_SYS_INFO(pdev));
+ v &= ATP867X_IO_SYS_MASK_RESERVED;
+ if (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B)
+ v |= ATP867X_IO_SYS_INFO_SLOW_UDMA5;
+ outb(v, ATP867X_SYS_INFO(pdev));
+
+ printk(KERN_INFO "ATP867X: ATP867 ATA UDMA133 controller (rev %02X)",
+ pdev->device);
+
+ host = ata_host_alloc_pinfo(&pdev->dev, ppi, ATP867X_NUM_PORTS);
+ if (!host) {
+ dev_printk(KERN_ERR, &pdev->dev,
+ "failed to allocate ATA host\n");
+ rc = -ENOMEM;
+ goto err_out;
+ }
+
+ rc = atp867x_ata_pci_sff_init_host(host);
+ if (rc) {
+ dev_printk(KERN_ERR, &pdev->dev, "failed to init host\n");
+ goto err_out;
+ }
+
+ pci_set_master(pdev);
+
+ rc = ata_host_activate(host, pdev->irq, ata_sff_interrupt,
+ IRQF_SHARED, &atp867x_sht);
+ if (rc)
+ dev_printk(KERN_ERR, &pdev->dev, "failed to activate host\n");
+
+err_out:
+ return rc;
+}
+
+static struct pci_device_id atp867x_pci_tbl[] = {
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867A), 0 },
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867B), 0 },
+ { },
+};
+
+static struct pci_driver atp867x_driver = {
+ .name = DRV_NAME,
+ .id_table = atp867x_pci_tbl,
+ .probe = atp867x_init_one,
+ .remove = ata_pci_remove_one,
+};
+
+static int __init atp867x_init(void)
+{
+ return pci_register_driver(&atp867x_driver);
+}
+
+static void __exit atp867x_exit(void)
+{
+ pci_unregister_driver(&atp867x_driver);
+}
+
+MODULE_AUTHOR("John(Jung-Ik) Lee, Google Inc.");
+MODULE_DESCRIPTION("low level driver for Artop/Acard 867x ATA controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, atp867x_pci_tbl);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(atp867x_init);
+module_exit(atp867x_exit);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 73b46b6..1de1fab 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1521,6 +1521,8 @@
#define PCI_DEVICE_ID_ARTOP_ATP860R 0x0007
#define PCI_DEVICE_ID_ARTOP_ATP865 0x0008
#define PCI_DEVICE_ID_ARTOP_ATP865R 0x0009
+#define PCI_DEVICE_ID_ARTOP_ATP867A 0x000A
+#define PCI_DEVICE_ID_ARTOP_ATP867B 0x000B
#define PCI_DEVICE_ID_ARTOP_AEC7610 0x8002
#define PCI_DEVICE_ID_ARTOP_AEC7612UW 0x8010
#define PCI_DEVICE_ID_ARTOP_AEC7612U 0x8020

2009-09-12 16:49:47

by Robert Hancock

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote:

(snip)

Looks mostly reasonable to me, other than a few issues:

> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct ata_device *peer = ata_dev_pair(adev);
> + struct atp867x_priv *dp = ap->private_data;
> + u8 speed = adev->pio_mode;
> + struct ata_timing t, p;
> + int T, UT;
> + u8 b;
> +
> + T = 1000000000 / 33333;
> + UT = T/4;
> +
> + switch (speed) {
> + case XFER_PIO_4:
> + case XFER_PIO_3:
> + case XFER_PIO_2:
> + case XFER_PIO_1:
> + case XFER_PIO_0:
> + case XFER_PIO_SLOW:
> + break;
> + default:
> + printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
> + " Default to XFER_PIO_0.\n", (unsigned)speed);
> + speed = XFER_PIO_0;
> + }
> +
> + ata_timing_compute(adev, speed,&t, T, UT);
> + if (peer&& peer->pio_mode) {
> + ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
> + ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
> + }
> +
> + b = inb(dp->dma_mode);
> + if (adev->devno& 1)
> + b = (b& ~ATP867X_IO_DMAMODE_SLAVE_MASK);
> + else
> + b = (b& ~ATP867X_IO_DMAMODE_MSTR_MASK);
> + outb(b, dp->dma_mode);
> +
> +#ifdef ATP867X_NO_HACK_PIOMODE
> + b = atp867x_get_active_clocks_shifted(t.active) |
> + atp867x_get_recover_clocks_shifted(t.recover);
> +#else
> + /*
> + * magic value that works (from doc 6.4, 6.6.9)
> + */
> + b = 0x31;
> +#endif

What's the purpose of this ifdef?

> + if (dp->pci66mhz)
> + b += 0x10;
> +
> + if (adev->devno& 1)
> + outb(b, dp->slave_piospd);
> + else
> + outb(b, dp->mstr_piospd);
> +
> + /*
> + * use the same value for comand timing as for PIO timimg
> + */
> + outb(b, dp->eightb_piospd);
> +}
> +
> +static int atp867x_cable_detect(struct ata_port *ap)
> +{
> + return ATA_CBL_PATA40_SHORT;
> +}

Doesn't the controller have a way to do proper 80-wire cable detection?

2009-09-13 00:17:31

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock <[email protected]> wrote:
> On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote:
>
> (snip)
>
> Looks mostly reasonable to me, other than a few issues:
>
>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
>> *adev)
>> +{
>> + ? ? ? struct ata_device *peer = ata_dev_pair(adev);
>> + ? ? ? struct atp867x_priv *dp = ap->private_data;
>> + ? ? ? u8 speed = adev->pio_mode;
>> + ? ? ? struct ata_timing t, p;
>> + ? ? ? int T, UT;
>> + ? ? ? u8 b;
>> +
>> + ? ? ? T = 1000000000 / 33333;
>> + ? ? ? UT = T/4;
>> +
>> + ? ? ? switch (speed) {
>> + ? ? ? case XFER_PIO_4:
>> + ? ? ? case XFER_PIO_3:
>> + ? ? ? case XFER_PIO_2:
>> + ? ? ? case XFER_PIO_1:
>> + ? ? ? case XFER_PIO_0:
>> + ? ? ? case XFER_PIO_SLOW:
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>> + ? ? ? ? ? ? ? ? ? ? ? " Default to XFER_PIO_0.\n", (unsigned)speed);
>> + ? ? ? ? ? ? ? speed = XFER_PIO_0;
>> + ? ? ? }
>> +
>> + ? ? ? ata_timing_compute(adev, speed,&t, T, UT);
>> + ? ? ? if (peer&& ?peer->pio_mode) {
>> + ? ? ? ? ? ? ? ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
>> + ? ? ? ? ? ? ? ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
>> + ? ? ? }
>> +
>> + ? ? ? b = inb(dp->dma_mode);
>> + ? ? ? if (adev->devno& ?1)
>> + ? ? ? ? ? ? ? b = (b& ?~ATP867X_IO_DMAMODE_SLAVE_MASK);
>> + ? ? ? else
>> + ? ? ? ? ? ? ? b = (b& ?~ATP867X_IO_DMAMODE_MSTR_MASK);
>> + ? ? ? outb(b, dp->dma_mode);
>> +
>> +#ifdef ATP867X_NO_HACK_PIOMODE
>> + ? ? ? b = atp867x_get_active_clocks_shifted(t.active) |
>> + ? ? ? ? ? ? ? atp867x_get_recover_clocks_shifted(t.recover);
>> +#else
>> + ? ? ? /*
>> + ? ? ? ?* magic value that works (from doc 6.4, 6.6.9)
>> + ? ? ? ?*/
>> + ? ? ? b = 0x31;
>> +#endif
>
> What's the purpose of this ifdef?

The magic value part must go. I'll update the patch.

>
>> + ? ? ? if (dp->pci66mhz)
>> + ? ? ? ? ? ? ? b += 0x10;
>> +
>> + ? ? ? if (adev->devno& ?1)
>> + ? ? ? ? ? ? ? outb(b, dp->slave_piospd);
>> + ? ? ? else
>> + ? ? ? ? ? ? ? outb(b, dp->mstr_piospd);
>> +
>> + ? ? ? /*
>> + ? ? ? ?* use the same value for comand timing as for PIO timimg
>> + ? ? ? ?*/
>> + ? ? ? outb(b, dp->eightb_piospd);
>> +}
>> +
>> +static int atp867x_cable_detect(struct ata_port *ap)
>> +{
>> + ? ? ? return ATA_CBL_PATA40_SHORT;
>> +}
>
> Doesn't the controller have a way to do proper 80-wire cable detection?

No programmatic way. libata.force should be used for other configurations.

Thanks,
-John

2009-09-13 00:26:44

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

From: John(Jung-Ik) Lee <[email protected]>

This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA ctrls.
Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide codes
from Eric Uhrhane <[email protected]>.

Signed-off-by: John(Jung-Ik) Lee <[email protected]>
Reviewed-by: Grant Grundler <[email protected]>
Reviewed-by: Gwendal Gringo <[email protected]>
---

drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/pata_atp867x.c | 578 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 2 +
4 files changed, 590 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b17c57f..2e1e678 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -244,6 +244,15 @@ config PATA_ARTOP

If unsure, say N.

+config PATA_ATP867X
+ tristate "ARTOP/Acard ATP867X PATA support"
+ depends on PCI
+ help
+ This option enables support for ARTOP/Acard ATP867X PATA
+ controllers.
+
+ If unsure, say N.
+
config PATA_AT32
tristate "Atmel AVR32 PATA support (Experimental)"
depends on AVR32 && PLATFORM_AT32AP && EXPERIMENTAL
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 38906f9..3372007 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SATA_FSL) += sata_fsl.o
obj-$(CONFIG_PATA_ALI) += pata_ali.o
obj-$(CONFIG_PATA_AMD) += pata_amd.o
obj-$(CONFIG_PATA_ARTOP) += pata_artop.o
+obj-$(CONFIG_PATA_ATP867X) += pata_atp867x.o
obj-$(CONFIG_PATA_AT32) += pata_at32.o
obj-$(CONFIG_PATA_ATIIXP) += pata_atiixp.o
obj-$(CONFIG_PATA_CMD640_PCI) += pata_cmd640.o
diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
new file mode 100644
index 0000000..4ebf555
--- /dev/null
+++ b/drivers/ata/pata_atp867x.c
@@ -0,0 +1,578 @@
+/*
+ * pata_atp867x.c - ARTOP 867X 64bit 4-channel UDMA133 ATA controller driver
+ *
+ * (C) 2009 Google Inc. John(Jung-Ik) Lee <[email protected]>
+ *
+ * Per Atp867 data sheet rev 1.2, Acard.
+ * Based in part on early ide code from
+ * 2003-2004 by Eric Uhrhane, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ *
+ * TODO:
+ * 1. RAID features [comparison, XOR, striping, mirroring, etc.]
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <scsi/scsi_host.h>
+#include <linux/libata.h>
+
+#define DRV_NAME "pata_atp867x"
+#define DRV_VERSION "0.7.5"
+
+/*
+ * IO Registers
+ * Note that all runtime hot ports are cached in ap private_data
+ */
+
+#define ATP867X_IO_CHANNEL_OFFSET 0x10
+#define ATP867X_IOBASE(d) pci_resource_start((d), 0)
+#define ATP867X_SYS_INFO(d) (0x3F + ATP867X_IOBASE(d))
+
+#define ATP867X_IO_PORTBASE(d, port) (0x00 + ATP867X_IOBASE(d) + \
+ (port) * ATP867X_IO_CHANNEL_OFFSET)
+#define ATP867X_IO_DMABASE(d, port) (0x40 + \
+ ATP867X_IO_PORTBASE((d), (port)))
+
+#define ATP867X_IO_STATUS(d, port) (0x07 + \
+ ATP867X_IO_PORTBASE((d), (port)))
+#define ATP867X_IO_ALTSTATUS(d, port) (0x0E + \
+ ATP867X_IO_PORTBASE((d), (port)))
+
+#define ATP867X_IO_MSTRPIOSPD(d, port) (0x08 + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_SLAVPIOSPD(d, port) (0x09 + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_8BPIOSPD(d, port) (0x0A + ATP867X_IO_DMABASE((d), (port)))
+#define ATP867X_IO_DMAMODE(d, port) (0x0B + ATP867X_IO_DMABASE((d), (port)))
+
+#define ATP867X_IO_PORTSPEED(d, port) (0x4A + \
+ ATP867X_IO_PORTBASE((d), (port)))
+#define ATP867X_IO_PREREAD(d, port) (0x4C + \
+ ATP867X_IO_PORTBASE((d), (port)))
+
+/*
+ * IO Register Bitfields
+ */
+
+#define ATP867X_IO_PIOSPD_ACTIVE_SHIFT 4
+#define ATP867X_IO_PIOSPD_RECOVER_SHIFT 0
+
+#define ATP867X_IO_DMAMODE_MSTR_SHIFT 0
+#define ATP867X_IO_DMAMODE_MSTR_MASK 0x07
+#define ATP867X_IO_DMAMODE_SLAVE_SHIFT 4
+#define ATP867X_IO_DMAMODE_SLAVE_MASK 0x70
+
+#define ATP867X_IO_DMAMODE_UDMA_6 0x07
+#define ATP867X_IO_DMAMODE_UDMA_5 0x06
+#define ATP867X_IO_DMAMODE_UDMA_4 0x05
+#define ATP867X_IO_DMAMODE_UDMA_3 0x04
+#define ATP867X_IO_DMAMODE_UDMA_2 0x03
+#define ATP867X_IO_DMAMODE_UDMA_1 0x02
+#define ATP867X_IO_DMAMODE_UDMA_0 0x01
+#define ATP867X_IO_DMAMODE_DISABLE 0x00
+
+#define ATP867X_IO_SYS_INFO_66MHZ 0x04
+#define ATP867X_IO_SYS_INFO_SLOW_UDMA5 0x02
+#define ATP867X_IO_SYS_MASK_RESERVED (~0xf1)
+
+#define ATP867X_IO_PORTSPEED_VAL 0x1143
+#define ATP867X_PREREAD_VAL 0x0200
+
+#define ATP867X_NUM_PORTS 4
+#define ATP867X_BAR_IOBASE 0
+#define ATP867X_BAR_ROMBASE 6
+
+struct atp867x_priv {
+ unsigned long dma_mode;
+ unsigned long mstr_piospd;
+ unsigned long slave_piospd;
+ unsigned long eightb_piospd;
+ int pci66mhz;
+};
+
+
+static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->dma_mode;
+ u8 b;
+ u8 mode;
+
+
+ switch (speed) {
+ case XFER_UDMA_6:
+ mode = ATP867X_IO_DMAMODE_UDMA_6;
+ break;
+ case XFER_UDMA_5:
+ mode = ATP867X_IO_DMAMODE_UDMA_5;
+ break;
+ case XFER_UDMA_4:
+ mode = ATP867X_IO_DMAMODE_UDMA_4;
+ break;
+ case XFER_UDMA_3:
+ mode = ATP867X_IO_DMAMODE_UDMA_3;
+ break;
+ case XFER_UDMA_2:
+ mode = ATP867X_IO_DMAMODE_UDMA_2;
+ break;
+ case XFER_UDMA_1:
+ mode = ATP867X_IO_DMAMODE_UDMA_1;
+ break;
+ case XFER_UDMA_0:
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_UDMA_0.\n", (unsigned)speed);
+ mode = ATP867X_IO_DMAMODE_UDMA_0;
+ }
+
+ /*
+ * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
+ * on 66MHz bus
+ * rev-A: UDMA_1~4 (5, 6 no change)
+ * rev-B: all UDMA modes
+ * UDMA_0 stays not to disable UDMA
+ */
+ if (dp->pci66mhz && mode > ATP867X_IO_DMAMODE_UDMA_0 &&
+ (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
+ mode < ATP867X_IO_DMAMODE_UDMA_5))
+ mode--;
+
+ b = inb(dp->dma_mode);
+ if (adev->devno & 1) {
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
+ (mode << ATP867X_IO_DMAMODE_SLAVE_SHIFT);
+ } else {
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK) |
+ (mode << ATP867X_IO_DMAMODE_MSTR_SHIFT);
+ }
+ outb(b, dp->dma_mode);
+}
+
+static int atp867x_get_active_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 7:
+ break;
+ case 8 ... 12:
+ clocks = 7;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
+ "Using default 8clk.\n", clk);
+ clocks = 0; /* 8 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
+}
+
+static int atp867x_get_recover_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 11:
+ break;
+ case 12:
+ clocks = 0;
+ break;
+ case 13: case 14:
+ --clocks;
+ break;
+ case 15:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
+ "Using default 15clk.\n", clk);
+ clocks = 0; /* 12 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_RECOVER_SHIFT;
+}
+
+static void atp867x_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct ata_device *peer = ata_dev_pair(adev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->pio_mode;
+ struct ata_timing t, p;
+ int T, UT;
+ u8 b;
+
+ T = 1000000000 / 33333;
+ UT = T/4;
+
+ switch (speed) {
+ case XFER_PIO_4:
+ case XFER_PIO_3:
+ case XFER_PIO_2:
+ case XFER_PIO_1:
+ case XFER_PIO_0:
+ case XFER_PIO_SLOW:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_PIO_0.\n", (unsigned)speed);
+ speed = XFER_PIO_0;
+ }
+
+ ata_timing_compute(adev, speed, &t, T, UT);
+ if (peer && peer->pio_mode) {
+ ata_timing_compute(peer, peer->pio_mode, &p, T, UT);
+ ata_timing_merge(&p, &t, &t, ATA_TIMING_8BIT);
+ }
+
+ b = inb(dp->dma_mode);
+ if (adev->devno & 1)
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK);
+ else
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK);
+ outb(b, dp->dma_mode);
+
+ b = atp867x_get_active_clocks_shifted(t.active) |
+ atp867x_get_recover_clocks_shifted(t.recover);
+ if (dp->pci66mhz)
+ b += 0x10;
+
+ if (adev->devno & 1)
+ outb(b, dp->slave_piospd);
+ else
+ outb(b, dp->mstr_piospd);
+
+ /*
+ * use the same value for comand timing as for PIO timimg
+ */
+ outb(b, dp->eightb_piospd);
+}
+
+static int atp867x_cable_detect(struct ata_port *ap)
+{
+ return ATA_CBL_PATA40_SHORT;
+}
+
+static struct scsi_host_template atp867x_sht = {
+ ATA_BMDMA_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations atp867x_ops = {
+ .inherits = &ata_bmdma_port_ops,
+ .cable_detect = atp867x_cable_detect,
+ .set_piomode = atp867x_set_piomode,
+ .set_dmamode = atp867x_set_dmamode,
+};
+
+
+#ifdef ATP867X_DEBUG
+static void atp867x_check_res(struct pci_dev *pdev)
+{
+ int i;
+ unsigned long start, len;
+
+ /* Check the PCI resources for this channel are enabled */
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ start = pci_resource_start(pdev, i);
+ len = pci_resource_len(pdev, i);
+ printk(KERN_DEBUG "ATP867X: resource start:len=%lx:%lx\n",
+ start, len);
+ }
+}
+
+static void atp867x_check_ports(struct ata_port *ap, struct pci_dev *pdev)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+ struct atp867x_priv *dp = ap->private_data;
+ int port = ap->port_no;
+
+ printk(KERN_DEBUG "ATP867X: port[%d] addresses\n"
+ " cmd_addr =0x%llx, 0x%llx\n"
+ " ctl_addr =0x%llx, 0x%llx\n"
+ " bmdma_addr =0x%llx, 0x%llx\n"
+ " data_addr =0x%llx\n"
+ " error_addr =0x%llx\n"
+ " feature_addr =0x%llx\n"
+ " nsect_addr =0x%llx\n"
+ " lbal_addr =0x%llx\n"
+ " lbam_addr =0x%llx\n"
+ " lbah_addr =0x%llx\n"
+ " device_addr =0x%llx\n"
+ " status_addr =0x%llx\n"
+ " command_addr =0x%llx\n"
+ " dp->dma_mode =0x%llx\n"
+ " dp->mstr_piospd =0x%llx\n"
+ " dp->slave_piospd =0x%llx\n"
+ " dp->eightb_piospd =0x%llx\n",
+ port,
+ (unsigned long long)ioaddr->cmd_addr,
+ ATP867X_IO_PORTBASE(pdev, port),
+ (unsigned long long)ioaddr->ctl_addr,
+ ATP867X_IO_ALTSTATUS(pdev, port),
+ (unsigned long long)ioaddr->bmdma_addr,
+ ATP867X_IO_DMABASE(pdev, port),
+ (unsigned long long)ioaddr->data_addr,
+ (unsigned long long)ioaddr->error_addr,
+ (unsigned long long)ioaddr->feature_addr,
+ (unsigned long long)ioaddr->nsect_addr,
+ (unsigned long long)ioaddr->lbal_addr,
+ (unsigned long long)ioaddr->lbam_addr,
+ (unsigned long long)ioaddr->lbah_addr,
+ (unsigned long long)ioaddr->device_addr,
+ (unsigned long long)ioaddr->status_addr,
+ (unsigned long long)ioaddr->command_addr,
+ (unsigned long long)dp->dma_mode,
+ (unsigned long long)dp->mstr_piospd,
+ (unsigned long long)dp->slave_piospd,
+ (unsigned long long)dp->eightb_piospd);
+}
+#endif
+
+static inline void atp867x_get_ports(struct pci_dev *dev, unsigned int port,
+ unsigned long *base, unsigned long *control)
+{
+ *base = ATP867X_IO_PORTBASE(dev, port);
+ *control = ATP867X_IO_ALTSTATUS(dev, port);
+}
+
+static inline unsigned long atp867x_get_dma_base(struct pci_dev *dev,
+ unsigned int port)
+{
+ return ATP867X_IO_DMABASE(dev, port);
+}
+
+static int atp867x_set_priv(struct ata_port *ap)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp;
+ int port = ap->port_no;
+
+ dp = ap->private_data =
+ devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
+ if (dp == NULL)
+ return -ENOMEM;
+
+ dp->dma_mode = ATP867X_IO_DMAMODE(pdev, port);
+ dp->mstr_piospd = ATP867X_IO_MSTRPIOSPD(pdev, port);
+ dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(pdev, port);
+ dp->eightb_piospd = ATP867X_IO_8BPIOSPD(pdev, port);
+
+ dp->pci66mhz = inb(ATP867X_SYS_INFO(pdev)) & ATP867X_IO_SYS_INFO_66MHZ;
+
+ return 0;
+}
+
+static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
+{
+ struct device *gdev = host->dev;
+ struct pci_dev *pdev = to_pci_dev(gdev);
+ unsigned int mask = 0;
+ int i, rc;
+
+ /*
+ * do not map rombase
+ */
+ rc = pcim_iomap_regions(pdev, 1 << ATP867X_BAR_IOBASE, DRV_NAME);
+ if (rc == -EBUSY)
+ pcim_pin_device(pdev);
+ if (rc)
+ return rc;
+ host->iomap = pcim_iomap_table(pdev);
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_res(pdev);
+
+ for (i = 0; i < PCI_ROM_RESOURCE; i++)
+ printk(KERN_DEBUG "ATP867X: iomap[%d]=0x%llx\n", i,
+ (unsigned long long)(host->iomap[i]));
+#endif
+
+ /*
+ * request, iomap BARs and init port addresses accordingly
+ */
+ for (i = 0; i < host->n_ports; i++) {
+ unsigned long base, control, dmabase;
+ struct ata_port *ap = host->ports[i];
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ atp867x_get_ports(pdev, i, &base, &control);
+ dmabase = atp867x_get_dma_base(pdev, i);
+
+ ioaddr->cmd_addr = devm_ioport_map(gdev, base, 8);
+ ioaddr->ctl_addr = ioaddr->altstatus_addr =
+ devm_ioport_map(gdev, control, 1);
+ ioaddr->bmdma_addr = devm_ioport_map(gdev, dmabase, 8);
+
+ ata_sff_std_ports(ioaddr);
+ rc = atp867x_set_priv(ap);
+ if (rc)
+ return rc;
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_ports(ap, pdev);
+#endif
+ ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", base, control);
+ ata_port_desc(ap, "bmdma 0x%lx", dmabase);
+
+ mask |= 1 << i;
+ }
+
+ if (!mask) {
+ dev_printk(KERN_ERR, gdev, "no available native port\n");
+ return -ENODEV;
+ }
+
+ rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+ if (rc)
+ return rc;
+
+ rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+ return rc;
+}
+
+static int atp867x_init_one(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ static int printed_version;
+ static const struct ata_port_info info_867x = {
+ .flags = ATA_FLAG_SLAVE_POSS,
+ .pio_mask = ATA_PIO4,
+ .mwdma_mask = ATA_MWDMA2,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &atp867x_ops,
+ };
+
+ struct ata_host *host;
+ const struct ata_port_info *ppi[] = { &info_867x, NULL };
+ int i, rc;
+ u8 v;
+
+ if (!printed_version++)
+ dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
+
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
+ /*
+ * Broken BIOS might not set latency high enough
+ */
+ pci_read_config_byte(pdev, PCI_LATENCY_TIMER, &v);
+ if (v < 0x80) {
+ v = 0x80;
+ pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
+ printk(KERN_DEBUG "ATP867X: set latency timer of device %s"
+ " to %d\n", pci_name(pdev), v);
+ }
+
+ /*
+ * init 8bit io ports speed(0aaarrrr) to 43h and
+ * init udma modes of master/slave to 0/0(11h)
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ outw(ATP867X_IO_PORTSPEED_VAL, ATP867X_IO_PORTSPEED(pdev, i));
+
+ /*
+ * init PreREAD counts
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ outw(ATP867X_PREREAD_VAL, ATP867X_IO_PREREAD(pdev, i));
+
+ v = inb(ATP867X_IOBASE(pdev) + 0x28);
+ v &= 0xcf; /* Enable INTA#: bit4=0 means enable */
+ v |= 0xc0; /* Enable PCI burst, MRM & not immediate interrupts */
+ outb(v, ATP867X_IOBASE(pdev) + 0x28);
+
+ /*
+ * Turn off the over clocked udma5 mode, only for Rev-B
+ */
+ v = inb(ATP867X_SYS_INFO(pdev));
+ v &= ATP867X_IO_SYS_MASK_RESERVED;
+ if (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B)
+ v |= ATP867X_IO_SYS_INFO_SLOW_UDMA5;
+ outb(v, ATP867X_SYS_INFO(pdev));
+
+ printk(KERN_INFO "ATP867X: ATP867 ATA UDMA133 controller (rev %02X)",
+ pdev->device);
+
+ host = ata_host_alloc_pinfo(&pdev->dev, ppi, ATP867X_NUM_PORTS);
+ if (!host) {
+ dev_printk(KERN_ERR, &pdev->dev,
+ "failed to allocate ATA host\n");
+ rc = -ENOMEM;
+ goto err_out;
+ }
+
+ rc = atp867x_ata_pci_sff_init_host(host);
+ if (rc) {
+ dev_printk(KERN_ERR, &pdev->dev, "failed to init host\n");
+ goto err_out;
+ }
+
+ pci_set_master(pdev);
+
+ rc = ata_host_activate(host, pdev->irq, ata_sff_interrupt,
+ IRQF_SHARED, &atp867x_sht);
+ if (rc)
+ dev_printk(KERN_ERR, &pdev->dev, "failed to activate host\n");
+
+err_out:
+ return rc;
+}
+
+static struct pci_device_id atp867x_pci_tbl[] = {
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867A), 0 },
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867B), 0 },
+ { },
+};
+
+static struct pci_driver atp867x_driver = {
+ .name = DRV_NAME,
+ .id_table = atp867x_pci_tbl,
+ .probe = atp867x_init_one,
+ .remove = ata_pci_remove_one,
+};
+
+static int __init atp867x_init(void)
+{
+ return pci_register_driver(&atp867x_driver);
+}
+
+static void __exit atp867x_exit(void)
+{
+ pci_unregister_driver(&atp867x_driver);
+}
+
+MODULE_AUTHOR("John(Jung-Ik) Lee, Google Inc.");
+MODULE_DESCRIPTION("low level driver for Artop/Acard 867x ATA controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, atp867x_pci_tbl);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(atp867x_init);
+module_exit(atp867x_exit);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 73b46b6..1de1fab 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1521,6 +1521,8 @@
#define PCI_DEVICE_ID_ARTOP_ATP860R 0x0007
#define PCI_DEVICE_ID_ARTOP_ATP865 0x0008
#define PCI_DEVICE_ID_ARTOP_ATP865R 0x0009
+#define PCI_DEVICE_ID_ARTOP_ATP867A 0x000A
+#define PCI_DEVICE_ID_ARTOP_ATP867B 0x000B
#define PCI_DEVICE_ID_ARTOP_AEC7610 0x8002
#define PCI_DEVICE_ID_ARTOP_AEC7612UW 0x8010
#define PCI_DEVICE_ID_ARTOP_AEC7612U 0x8020

2009-09-13 00:32:00

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote:
> From: John(Jung-Ik) Lee<[email protected]>
>
> This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA ctrls.
> Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide codes
> from Eric Uhrhane<[email protected]>.
>
> Signed-off-by: John(Jung-Ik) Lee<[email protected]>
> Reviewed-by: Grant Grundler<[email protected]>
> Reviewed-by: Gwendal Gringo<[email protected]>
> ---
>
> drivers/ata/Kconfig | 9 +
> drivers/ata/Makefile | 1 +
> drivers/ata/pata_atp867x.c | 578 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci_ids.h | 2 +
> 4 files changed, 590 insertions(+), 0 deletions(-)

What changed in this patch, versus previous patches?

(that's a question, and also an indication of what needs to be
explicitly stated in your email body, immediately following the "---"
separator, for each patch revision)

Since the merge window is open, I will likely send this driver upstream
as soon as feedback level approaches zero.

Jeff


2009-09-13 00:37:24

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Sat, Sep 12, 2009 at 5:31 PM, Jeff Garzik <[email protected]> wrote:
> On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote:
>>
>> From: John(Jung-Ik) Lee<[email protected]>
>>
>> This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA
>> ctrls.
>> Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide
>> codes
>> from Eric Uhrhane<[email protected]>.
>>
>> Signed-off-by: John(Jung-Ik) Lee<[email protected]>
>> Reviewed-by: ?Grant Grundler<[email protected]>
>> Reviewed-by: ?Gwendal Gringo<[email protected]>
>> ---
>>
>> ?drivers/ata/Kconfig ? ? ? ?| ? ?9 +
>> ?drivers/ata/Makefile ? ? ? | ? ?1 +
>> ?drivers/ata/pata_atp867x.c | ?578
>> ++++++++++++++++++++++++++++++++++++++++++++
>> ?include/linux/pci_ids.h ? ?| ? ?2 +
>> ?4 files changed, 590 insertions(+), 0 deletions(-)
>
> What changed in this patch, versus previous patches?

Only one change - removed the hard-coded value for active/recovery.

-+#ifdef ATP867X_NO_HACK_PIOMODE
+ b = atp867x_get_active_clocks_shifted(t.active) |
+ atp867x_get_recover_clocks_shifted(t.recover);
-+#else
-+ /*
-+ * magic value that works (from doc 6.4, 6.6.9)
-+ */
-+ b = 0x31;
-+#endif
+ if (dp->pci66mhz)
+ b += 0x10;
+


>
> (that's a question, and also an indication of what needs to be explicitly
> stated in your email body, immediately following the "---" separator, for
> each patch revision)
>

I'll send another with rev change.
-John


> Since the merge window is open, I will likely send this driver upstream as
> soon as feedback level approaches zero.
>
> ? ? ? ?Jeff
>
>
>
>

2009-09-13 00:38:09

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On 09/12/2009 08:17 PM, Jung-Ik (John) Lee wrote:
> On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock<[email protected]> wrote:
>> On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote:
>>
>> (snip)
>>
>> Looks mostly reasonable to me, other than a few issues:
>>
>>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
>>> *adev)
>>> +{
>>> + struct ata_device *peer = ata_dev_pair(adev);
>>> + struct atp867x_priv *dp = ap->private_data;
>>> + u8 speed = adev->pio_mode;
>>> + struct ata_timing t, p;
>>> + int T, UT;
>>> + u8 b;
>>> +
>>> + T = 1000000000 / 33333;
>>> + UT = T/4;
>>> +
>>> + switch (speed) {
>>> + case XFER_PIO_4:
>>> + case XFER_PIO_3:
>>> + case XFER_PIO_2:
>>> + case XFER_PIO_1:
>>> + case XFER_PIO_0:
>>> + case XFER_PIO_SLOW:
>>> + break;
>>> + default:
>>> + printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>>> + " Default to XFER_PIO_0.\n", (unsigned)speed);
>>> + speed = XFER_PIO_0;
>>> + }
>>> +
>>> + ata_timing_compute(adev, speed,&t, T, UT);
>>> + if (peer&& peer->pio_mode) {
>>> + ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
>>> + ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
>>> + }
>>> +
>>> + b = inb(dp->dma_mode);
>>> + if (adev->devno& 1)
>>> + b = (b& ~ATP867X_IO_DMAMODE_SLAVE_MASK);
>>> + else
>>> + b = (b& ~ATP867X_IO_DMAMODE_MSTR_MASK);
>>> + outb(b, dp->dma_mode);
>>> +
>>> +#ifdef ATP867X_NO_HACK_PIOMODE
>>> + b = atp867x_get_active_clocks_shifted(t.active) |
>>> + atp867x_get_recover_clocks_shifted(t.recover);
>>> +#else
>>> + /*
>>> + * magic value that works (from doc 6.4, 6.6.9)
>>> + */
>>> + b = 0x31;
>>> +#endif
>>
>> What's the purpose of this ifdef?
>
> The magic value part must go. I'll update the patch.
>
>>
>>> + if (dp->pci66mhz)
>>> + b += 0x10;
>>> +
>>> + if (adev->devno& 1)
>>> + outb(b, dp->slave_piospd);
>>> + else
>>> + outb(b, dp->mstr_piospd);
>>> +
>>> + /*
>>> + * use the same value for comand timing as for PIO timimg
>>> + */
>>> + outb(b, dp->eightb_piospd);
>>> +}
>>> +
>>> +static int atp867x_cable_detect(struct ata_port *ap)
>>> +{
>>> + return ATA_CBL_PATA40_SHORT;
>>> +}
>>
>> Doesn't the controller have a way to do proper 80-wire cable detection?
>
> No programmatic way. libata.force should be used for other configurations.

Ideally you would use ATA_CBL_PATA_UNK / ata_cable_unknown, but it seems
that ATA_CBL_PATA_UNK has grown rather useless in the current kernel.

We should probably (a) update libata-core/libata-sff to do something
intelligent with ATA_CBL_PATA_UNK, and (b) use ATA_CBL_PATA_UNK in
pata_atp867x.

But yes, libata.force works as well. It just seems like we are losing
an opportunity to accept an existing BIOS-configured 80-wire cable
configuration by hardcoding ATA_CBL_PATA40_SHORT.

Also, a specific note -- atp867x_cable_detect() should be moved into
drivers/ata/libata-core.c and made generic. We have ata_cable_xxx
versions for everything except 40-short, it seems.

Jeff



2009-09-13 00:51:19

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Sat, Sep 12, 2009 at 5:38 PM, Jeff Garzik <[email protected]> wrote:
> On 09/12/2009 08:17 PM, Jung-Ik (John) Lee wrote:
>>
>> On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock<[email protected]>
>> ?wrote:
>>>
>>> On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote:
>>>
>>> (snip)
>>>
>>> Looks mostly reasonable to me, other than a few issues:
>>>
>>>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
>>>> *adev)
>>>> +{
>>>> + ? ? ? struct ata_device *peer = ata_dev_pair(adev);
>>>> + ? ? ? struct atp867x_priv *dp = ap->private_data;
>>>> + ? ? ? u8 speed = adev->pio_mode;
>>>> + ? ? ? struct ata_timing t, p;
>>>> + ? ? ? int T, UT;
>>>> + ? ? ? u8 b;
>>>> +
>>>> + ? ? ? T = 1000000000 / 33333;
>>>> + ? ? ? UT = T/4;
>>>> +
>>>> + ? ? ? switch (speed) {
>>>> + ? ? ? case XFER_PIO_4:
>>>> + ? ? ? case XFER_PIO_3:
>>>> + ? ? ? case XFER_PIO_2:
>>>> + ? ? ? case XFER_PIO_1:
>>>> + ? ? ? case XFER_PIO_0:
>>>> + ? ? ? case XFER_PIO_SLOW:
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? default:
>>>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>>>> + ? ? ? ? ? ? ? ? ? ? ? " Default to XFER_PIO_0.\n", (unsigned)speed);
>>>> + ? ? ? ? ? ? ? speed = XFER_PIO_0;
>>>> + ? ? ? }
>>>> +
>>>> + ? ? ? ata_timing_compute(adev, speed,&t, T, UT);
>>>> + ? ? ? if (peer&& ? ?peer->pio_mode) {
>>>> + ? ? ? ? ? ? ? ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
>>>> + ? ? ? ? ? ? ? ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
>>>> + ? ? ? }
>>>> +
>>>> + ? ? ? b = inb(dp->dma_mode);
>>>> + ? ? ? if (adev->devno& ? ?1)
>>>> + ? ? ? ? ? ? ? b = (b& ? ?~ATP867X_IO_DMAMODE_SLAVE_MASK);
>>>> + ? ? ? else
>>>> + ? ? ? ? ? ? ? b = (b& ? ?~ATP867X_IO_DMAMODE_MSTR_MASK);
>>>> + ? ? ? outb(b, dp->dma_mode);
>>>> +
>>>> +#ifdef ATP867X_NO_HACK_PIOMODE
>>>> + ? ? ? b = atp867x_get_active_clocks_shifted(t.active) |
>>>> + ? ? ? ? ? ? ? atp867x_get_recover_clocks_shifted(t.recover);
>>>> +#else
>>>> + ? ? ? /*
>>>> + ? ? ? ?* magic value that works (from doc 6.4, 6.6.9)
>>>> + ? ? ? ?*/
>>>> + ? ? ? b = 0x31;
>>>> +#endif
>>>
>>> What's the purpose of this ifdef?
>>
>> The magic value part must go. I'll update the patch.
>>
>>>
>>>> + ? ? ? if (dp->pci66mhz)
>>>> + ? ? ? ? ? ? ? b += 0x10;
>>>> +
>>>> + ? ? ? if (adev->devno& ? ?1)
>>>> + ? ? ? ? ? ? ? outb(b, dp->slave_piospd);
>>>> + ? ? ? else
>>>> + ? ? ? ? ? ? ? outb(b, dp->mstr_piospd);
>>>> +
>>>> + ? ? ? /*
>>>> + ? ? ? ?* use the same value for comand timing as for PIO timimg
>>>> + ? ? ? ?*/
>>>> + ? ? ? outb(b, dp->eightb_piospd);
>>>> +}
>>>> +
>>>> +static int atp867x_cable_detect(struct ata_port *ap)
>>>> +{
>>>> + ? ? ? return ATA_CBL_PATA40_SHORT;
>>>> +}
>>>
>>> Doesn't the controller have a way to do proper 80-wire cable detection?
>>
>> No programmatic way. libata.force should be used for other configurations.
>
> Ideally you would use ATA_CBL_PATA_UNK / ata_cable_unknown, but it seems
> that ATA_CBL_PATA_UNK has grown rather useless in the current kernel.
>
> We should probably (a) update libata-core/libata-sff to do something
> intelligent with ATA_CBL_PATA_UNK, and (b) use ATA_CBL_PATA_UNK in
> pata_atp867x.
>
> But yes, libata.force works as well. ?It just seems like we are losing an
> opportunity to accept an existing BIOS-configured 80-wire cable
> configuration by hardcoding ATA_CBL_PATA40_SHORT.

Right. But from my experience relying on BIOS just doesn't seem to
work well in all cases though.

>
> Also, a specific note -- atp867x_cable_detect() should be moved into
> drivers/ata/libata-core.c and made generic. ?We have ata_cable_xxx versions
> for everything except 40-short, it seems.

Sounds right to me.

>
> ? ? ? ?Jeff
>
>
>
>
>

2009-09-13 00:55:58

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers



General comment:

* since you use iomap to map the region, you should use ioread{8,16,32}
/ iowrite{8,16,32} accessors. Do not use inb/outb/inl/outl/etc.

* run through scripts/checkpatch.pl


On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote:
> +/*
> + * IO Registers
> + * Note that all runtime hot ports are cached in ap private_data
> + */
> +
> +#define ATP867X_IO_CHANNEL_OFFSET 0x10
> +#define ATP867X_IOBASE(d) pci_resource_start((d), 0)
> +#define ATP867X_SYS_INFO(d) (0x3F + ATP867X_IOBASE(d))
> +
> +#define ATP867X_IO_PORTBASE(d, port) (0x00 + ATP867X_IOBASE(d) + \
> + (port) * ATP867X_IO_CHANNEL_OFFSET)
> +#define ATP867X_IO_DMABASE(d, port) (0x40 + \
> + ATP867X_IO_PORTBASE((d), (port)))
> +
> +#define ATP867X_IO_STATUS(d, port) (0x07 + \
> + ATP867X_IO_PORTBASE((d), (port)))
> +#define ATP867X_IO_ALTSTATUS(d, port) (0x0E + \
> + ATP867X_IO_PORTBASE((d), (port)))
> +
> +#define ATP867X_IO_MSTRPIOSPD(d, port) (0x08 + ATP867X_IO_DMABASE((d), (port)))
> +#define ATP867X_IO_SLAVPIOSPD(d, port) (0x09 + ATP867X_IO_DMABASE((d), (port)))
> +#define ATP867X_IO_8BPIOSPD(d, port) (0x0A + ATP867X_IO_DMABASE((d), (port)))
> +#define ATP867X_IO_DMAMODE(d, port) (0x0B + ATP867X_IO_DMABASE((d), (port)))
> +
> +#define ATP867X_IO_PORTSPEED(d, port) (0x4A + \
> + ATP867X_IO_PORTBASE((d), (port)))
> +#define ATP867X_IO_PREREAD(d, port) (0x4C + \
> + ATP867X_IO_PORTBASE((d), (port)))
> +
> +/*
> + * IO Register Bitfields
> + */
> +
> +#define ATP867X_IO_PIOSPD_ACTIVE_SHIFT 4
> +#define ATP867X_IO_PIOSPD_RECOVER_SHIFT 0
> +
> +#define ATP867X_IO_DMAMODE_MSTR_SHIFT 0
> +#define ATP867X_IO_DMAMODE_MSTR_MASK 0x07
> +#define ATP867X_IO_DMAMODE_SLAVE_SHIFT 4
> +#define ATP867X_IO_DMAMODE_SLAVE_MASK 0x70
> +
> +#define ATP867X_IO_DMAMODE_UDMA_6 0x07
> +#define ATP867X_IO_DMAMODE_UDMA_5 0x06
> +#define ATP867X_IO_DMAMODE_UDMA_4 0x05
> +#define ATP867X_IO_DMAMODE_UDMA_3 0x04
> +#define ATP867X_IO_DMAMODE_UDMA_2 0x03
> +#define ATP867X_IO_DMAMODE_UDMA_1 0x02
> +#define ATP867X_IO_DMAMODE_UDMA_0 0x01
> +#define ATP867X_IO_DMAMODE_DISABLE 0x00
> +
> +#define ATP867X_IO_SYS_INFO_66MHZ 0x04
> +#define ATP867X_IO_SYS_INFO_SLOW_UDMA5 0x02
> +#define ATP867X_IO_SYS_MASK_RESERVED (~0xf1)
> +
> +#define ATP867X_IO_PORTSPEED_VAL 0x1143
> +#define ATP867X_PREREAD_VAL 0x0200
> +
> +#define ATP867X_NUM_PORTS 4
> +#define ATP867X_BAR_IOBASE 0
> +#define ATP867X_BAR_ROMBASE 6

enums are the preferred method of creating named constants. This
potentially makes the code more readable in the debugger, imparts type
information with the constant, and makes CPP output more readable. eg.

enum {
ATP867X_NUM_PORTS = 4,
ATP867X_BAR_IOBASE = 0,
};

> +struct atp867x_priv {
> + unsigned long dma_mode;
> + unsigned long mstr_piospd;
> + unsigned long slave_piospd;
> + unsigned long eightb_piospd;
> + int pci66mhz;
> +};
> +
> +
> +static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> + struct atp867x_priv *dp = ap->private_data;
> + u8 speed = adev->dma_mode;
> + u8 b;
> + u8 mode;
> +
> +
> + switch (speed) {
> + case XFER_UDMA_6:
> + mode = ATP867X_IO_DMAMODE_UDMA_6;
> + break;
> + case XFER_UDMA_5:
> + mode = ATP867X_IO_DMAMODE_UDMA_5;
> + break;
> + case XFER_UDMA_4:
> + mode = ATP867X_IO_DMAMODE_UDMA_4;
> + break;
> + case XFER_UDMA_3:
> + mode = ATP867X_IO_DMAMODE_UDMA_3;
> + break;
> + case XFER_UDMA_2:
> + mode = ATP867X_IO_DMAMODE_UDMA_2;
> + break;
> + case XFER_UDMA_1:
> + mode = ATP867X_IO_DMAMODE_UDMA_1;
> + break;
> + case XFER_UDMA_0:
> + mode = ATP867X_IO_DMAMODE_UDMA_0;
> + break;
> + default:
> + printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
> + " Default to XFER_UDMA_0.\n", (unsigned)speed);
> + mode = ATP867X_IO_DMAMODE_UDMA_0;

a table would be nice, preferred over a switch statement. You may use
ARRAY_SIZE() macro to generate a constant at compile time for number of
elements in array.


> +
> + /*
> + * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
> + * on 66MHz bus
> + * rev-A: UDMA_1~4 (5, 6 no change)
> + * rev-B: all UDMA modes
> + * UDMA_0 stays not to disable UDMA
> + */
> + if (dp->pci66mhz&& mode> ATP867X_IO_DMAMODE_UDMA_0&&
> + (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
> + mode< ATP867X_IO_DMAMODE_UDMA_5))
> + mode--;
> +
> + b = inb(dp->dma_mode);
> + if (adev->devno& 1) {
> + b = (b& ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
> + (mode<< ATP867X_IO_DMAMODE_SLAVE_SHIFT);
> + } else {
> + b = (b& ~ATP867X_IO_DMAMODE_MSTR_MASK) |
> + (mode<< ATP867X_IO_DMAMODE_MSTR_SHIFT);

add whitespace between all tokens, eg. surrounding "&&" and ">" and "&"
operators


> + }
> + outb(b, dp->dma_mode);
> +}
> +
> +static int atp867x_get_active_clocks_shifted(unsigned int clk)
> +{
> + unsigned char clocks = clk;
> +
> + switch (clocks) {
> + case 0:
> + clocks = 1;
> + break;
> + case 1 ... 7:
> + break;
> + case 8 ... 12:
> + clocks = 7;
> + break;
> + default:
> + printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
> + "Using default 8clk.\n", clk);
> + clocks = 0; /* 8 clk */
> + break;
> + }
> + return clocks<< ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
> +}
> +
> +static int atp867x_get_recover_clocks_shifted(unsigned int clk)
> +{
> + unsigned char clocks = clk;
> +
> + switch (clocks) {
> + case 0:
> + clocks = 1;
> + break;
> + case 1 ... 11:
> + break;
> + case 12:
> + clocks = 0;
> + break;
> + case 13: case 14:
> + --clocks;
> + break;
> + case 15:
> + break;
> + default:
> + printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
> + "Using default 15clk.\n", clk);
> + clocks = 0; /* 12 clk */
> + break;
> + }
> + return clocks<< ATP867X_IO_PIOSPD_RECOVER_SHIFT;

whitespace between all tokens

> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct ata_device *peer = ata_dev_pair(adev);
> + struct atp867x_priv *dp = ap->private_data;
> + u8 speed = adev->pio_mode;
> + struct ata_timing t, p;
> + int T, UT;
> + u8 b;
> +
> + T = 1000000000 / 33333;
> + UT = T/4;

ditto


> + switch (speed) {
> + case XFER_PIO_4:
> + case XFER_PIO_3:
> + case XFER_PIO_2:
> + case XFER_PIO_1:
> + case XFER_PIO_0:
> + case XFER_PIO_SLOW:
> + break;
> + default:
> + printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
> + " Default to XFER_PIO_0.\n", (unsigned)speed);
> + speed = XFER_PIO_0;

unsupported speeds are masked out via ata_port_info. no need for
additional checks.


> + ata_timing_compute(adev, speed,&t, T, UT);
> + if (peer&& peer->pio_mode) {
> + ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
> + ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
> + }
> +
> + b = inb(dp->dma_mode);
> + if (adev->devno& 1)
> + b = (b& ~ATP867X_IO_DMAMODE_SLAVE_MASK);
> + else
> + b = (b& ~ATP867X_IO_DMAMODE_MSTR_MASK);
> + outb(b, dp->dma_mode);
> +
> + b = atp867x_get_active_clocks_shifted(t.active) |
> + atp867x_get_recover_clocks_shifted(t.recover);
> + if (dp->pci66mhz)
> + b += 0x10;
> +
> + if (adev->devno& 1)
> + outb(b, dp->slave_piospd);
> + else
> + outb(b, dp->mstr_piospd);
> +
> + /*
> + * use the same value for comand timing as for PIO timimg
> + */
> + outb(b, dp->eightb_piospd);

whitespace + ioread/iowrite


> +static int atp867x_cable_detect(struct ata_port *ap)
> +{
> + return ATA_CBL_PATA40_SHORT;
> +}
> +
> +static struct scsi_host_template atp867x_sht = {
> + ATA_BMDMA_SHT(DRV_NAME),
> +};
> +
> +static struct ata_port_operations atp867x_ops = {
> + .inherits =&ata_bmdma_port_ops,
> + .cable_detect = atp867x_cable_detect,
> + .set_piomode = atp867x_set_piomode,
> + .set_dmamode = atp867x_set_dmamode,
> +};
> +
> +
> +#ifdef ATP867X_DEBUG
> +static void atp867x_check_res(struct pci_dev *pdev)
> +{
> + int i;
> + unsigned long start, len;
> +
> + /* Check the PCI resources for this channel are enabled */
> + for (i = 0; i< DEVICE_COUNT_RESOURCE; i++) {
> + start = pci_resource_start(pdev, i);
> + len = pci_resource_len(pdev, i);
> + printk(KERN_DEBUG "ATP867X: resource start:len=%lx:%lx\n",
> + start, len);
> + }
> +}
> +
> +static void atp867x_check_ports(struct ata_port *ap, struct pci_dev *pdev)
> +{
> + struct ata_ioports *ioaddr =&ap->ioaddr;
> + struct atp867x_priv *dp = ap->private_data;
> + int port = ap->port_no;
> +
> + printk(KERN_DEBUG "ATP867X: port[%d] addresses\n"
> + " cmd_addr =0x%llx, 0x%llx\n"
> + " ctl_addr =0x%llx, 0x%llx\n"
> + " bmdma_addr =0x%llx, 0x%llx\n"
> + " data_addr =0x%llx\n"
> + " error_addr =0x%llx\n"
> + " feature_addr =0x%llx\n"
> + " nsect_addr =0x%llx\n"
> + " lbal_addr =0x%llx\n"
> + " lbam_addr =0x%llx\n"
> + " lbah_addr =0x%llx\n"
> + " device_addr =0x%llx\n"
> + " status_addr =0x%llx\n"
> + " command_addr =0x%llx\n"
> + " dp->dma_mode =0x%llx\n"
> + " dp->mstr_piospd =0x%llx\n"
> + " dp->slave_piospd =0x%llx\n"
> + " dp->eightb_piospd =0x%llx\n",
> + port,
> + (unsigned long long)ioaddr->cmd_addr,
> + ATP867X_IO_PORTBASE(pdev, port),
> + (unsigned long long)ioaddr->ctl_addr,
> + ATP867X_IO_ALTSTATUS(pdev, port),
> + (unsigned long long)ioaddr->bmdma_addr,
> + ATP867X_IO_DMABASE(pdev, port),
> + (unsigned long long)ioaddr->data_addr,
> + (unsigned long long)ioaddr->error_addr,
> + (unsigned long long)ioaddr->feature_addr,
> + (unsigned long long)ioaddr->nsect_addr,
> + (unsigned long long)ioaddr->lbal_addr,
> + (unsigned long long)ioaddr->lbam_addr,
> + (unsigned long long)ioaddr->lbah_addr,
> + (unsigned long long)ioaddr->device_addr,
> + (unsigned long long)ioaddr->status_addr,
> + (unsigned long long)ioaddr->command_addr,
> + (unsigned long long)dp->dma_mode,
> + (unsigned long long)dp->mstr_piospd,
> + (unsigned long long)dp->slave_piospd,
> + (unsigned long long)dp->eightb_piospd);
> +}
> +#endif
> +
> +static inline void atp867x_get_ports(struct pci_dev *dev, unsigned int port,
> + unsigned long *base, unsigned long *control)
> +{
> + *base = ATP867X_IO_PORTBASE(dev, port);
> + *control = ATP867X_IO_ALTSTATUS(dev, port);
> +}
> +
> +static inline unsigned long atp867x_get_dma_base(struct pci_dev *dev,
> + unsigned int port)
> +{
> + return ATP867X_IO_DMABASE(dev, port);
> +}
> +
> +static int atp867x_set_priv(struct ata_port *ap)
> +{
> + struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> + struct atp867x_priv *dp;
> + int port = ap->port_no;
> +
> + dp = ap->private_data =
> + devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
> + if (dp == NULL)
> + return -ENOMEM;
> +
> + dp->dma_mode = ATP867X_IO_DMAMODE(pdev, port);
> + dp->mstr_piospd = ATP867X_IO_MSTRPIOSPD(pdev, port);
> + dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(pdev, port);
> + dp->eightb_piospd = ATP867X_IO_8BPIOSPD(pdev, port);
> +
> + dp->pci66mhz = inb(ATP867X_SYS_INFO(pdev))& ATP867X_IO_SYS_INFO_66MHZ;
> +
> + return 0;
> +}
> +
> +static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
> +{
> + struct device *gdev = host->dev;
> + struct pci_dev *pdev = to_pci_dev(gdev);
> + unsigned int mask = 0;
> + int i, rc;
> +
> + /*
> + * do not map rombase
> + */
> + rc = pcim_iomap_regions(pdev, 1<< ATP867X_BAR_IOBASE, DRV_NAME);
> + if (rc == -EBUSY)
> + pcim_pin_device(pdev);
> + if (rc)
> + return rc;
> + host->iomap = pcim_iomap_table(pdev);
> +
> +#ifdef ATP867X_DEBUG
> + atp867x_check_res(pdev);
> +
> + for (i = 0; i< PCI_ROM_RESOURCE; i++)
> + printk(KERN_DEBUG "ATP867X: iomap[%d]=0x%llx\n", i,
> + (unsigned long long)(host->iomap[i]));
> +#endif
> +
> + /*
> + * request, iomap BARs and init port addresses accordingly
> + */
> + for (i = 0; i< host->n_ports; i++) {
> + unsigned long base, control, dmabase;
> + struct ata_port *ap = host->ports[i];
> + struct ata_ioports *ioaddr =&ap->ioaddr;
> +
> + atp867x_get_ports(pdev, i,&base,&control);
> + dmabase = atp867x_get_dma_base(pdev, i);
> +
> + ioaddr->cmd_addr = devm_ioport_map(gdev, base, 8);
> + ioaddr->ctl_addr = ioaddr->altstatus_addr =
> + devm_ioport_map(gdev, control, 1);
> + ioaddr->bmdma_addr = devm_ioport_map(gdev, dmabase, 8);

why are you mapping separately from pcim_iomap_regions?

Just do the mapping all at once, with pcim_iomap_regions, then simply
calculate addresses in the n_ports loop.




> + rc = pcim_enable_device(pdev);
> + if (rc)
> + return rc;
> +
> + /*
> + * Broken BIOS might not set latency high enough
> + */
> + pci_read_config_byte(pdev, PCI_LATENCY_TIMER,&v);
> + if (v< 0x80) {
> + v = 0x80;
> + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
> + printk(KERN_DEBUG "ATP867X: set latency timer of device %s"
> + " to %d\n", pci_name(pdev), v);
> + }

this seems pointless - pci_set_master() already does this

2009-09-13 02:42:15

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik <[email protected]> wrote:
>
>
> General comment:
>
> * since you use iomap to map the region, you should use ioread{8,16,32} /
> iowrite{8,16,32} accessors. ?Do not use inb/outb/inl/outl/etc.
.
I used them for runtime hot registers by separately mapping them
simply to avoid an extra overhead of ioread/iowrite, over the
portability.
I know it's not a good idea but in this case for these hot ports can
in/out be used?


>
> * run through scripts/checkpatch.pl
>

Weird. I don't see any WS issues you pointed below in my source code
or git diff file, except UT = T/4 below.

>
> On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote:
>>
>> +/*
>> + * IO Registers
>> + * Note that all runtime hot ports are cached in ap private_data
>> + */
>> +
>> +#define ATP867X_IO_CHANNEL_OFFSET ? ? ?0x10
>> +#define ATP867X_IOBASE(d) ? ? ? ? ? ? ?pci_resource_start((d), 0)
>> +#define ATP867X_SYS_INFO(d) ? ? ? ? ? ?(0x3F + ATP867X_IOBASE(d))
>> +
>> +#define ATP867X_IO_PORTBASE(d, port) ? (0x00 + ATP867X_IOBASE(d) + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (port) *
>> ATP867X_IO_CHANNEL_OFFSET)
>> +#define ATP867X_IO_DMABASE(d, port) ? ?(0x40 + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_PORTBASE((d), (port)))
>> +
>> +#define ATP867X_IO_STATUS(d, port) ? ? (0x07 + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_PORTBASE((d), (port)))
>> +#define ATP867X_IO_ALTSTATUS(d, port) ?(0x0E + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_PORTBASE((d), (port)))
>> +
>> +#define ATP867X_IO_MSTRPIOSPD(d, port) (0x08 + ATP867X_IO_DMABASE((d),
>> (port)))
>> +#define ATP867X_IO_SLAVPIOSPD(d, port) (0x09 + ATP867X_IO_DMABASE((d),
>> (port)))
>> +#define ATP867X_IO_8BPIOSPD(d, port) ? (0x0A + ATP867X_IO_DMABASE((d),
>> (port)))
>> +#define ATP867X_IO_DMAMODE(d, port) ? ?(0x0B + ATP867X_IO_DMABASE((d),
>> (port)))
>> +
>> +#define ATP867X_IO_PORTSPEED(d, port) ?(0x4A + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_PORTBASE((d), (port)))
>> +#define ATP867X_IO_PREREAD(d, port) ? ?(0x4C + \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_PORTBASE((d), (port)))
>> +
>> +/*
>> + * IO Register Bitfields
>> + */
>> +
>> +#define ATP867X_IO_PIOSPD_ACTIVE_SHIFT 4
>> +#define ATP867X_IO_PIOSPD_RECOVER_SHIFT ? ? ? ?0
>> +
>> +#define ATP867X_IO_DMAMODE_MSTR_SHIFT ?0
>> +#define ATP867X_IO_DMAMODE_MSTR_MASK ? 0x07
>> +#define ATP867X_IO_DMAMODE_SLAVE_SHIFT 4
>> +#define ATP867X_IO_DMAMODE_SLAVE_MASK ?0x70
>> +
>> +#define ATP867X_IO_DMAMODE_UDMA_6 ? ? ?0x07
>> +#define ATP867X_IO_DMAMODE_UDMA_5 ? ? ?0x06
>> +#define ATP867X_IO_DMAMODE_UDMA_4 ? ? ?0x05
>> +#define ATP867X_IO_DMAMODE_UDMA_3 ? ? ?0x04
>> +#define ATP867X_IO_DMAMODE_UDMA_2 ? ? ?0x03
>> +#define ATP867X_IO_DMAMODE_UDMA_1 ? ? ?0x02
>> +#define ATP867X_IO_DMAMODE_UDMA_0 ? ? ?0x01
>> +#define ATP867X_IO_DMAMODE_DISABLE ? ? 0x00
>> +
>> +#define ATP867X_IO_SYS_INFO_66MHZ ? ? ?0x04
>> +#define ATP867X_IO_SYS_INFO_SLOW_UDMA5 0x02
>> +#define ATP867X_IO_SYS_MASK_RESERVED ? (~0xf1)
>> +
>> +#define ATP867X_IO_PORTSPEED_VAL ? ? ? 0x1143
>> +#define ATP867X_PREREAD_VAL ? ? ? ? ? ?0x0200
>> +
>> +#define ATP867X_NUM_PORTS ? ? ? ? ? ? ?4
>> +#define ATP867X_BAR_IOBASE ? ? ? ? ? ? 0
>> +#define ATP867X_BAR_ROMBASE ? ? ? ? ? ?6
>
> enums are the preferred method of creating named constants. ?This
> potentially makes the code more readable in the debugger, imparts type
> information with the constant, and makes CPP output more readable. ?eg.
>
> enum {
> ? ? ? ?ATP867X_NUM_PORTS ? ? ? ? ? ? ? = 4,
> ? ? ? ?ATP867X_BAR_IOBASE ? ? ? ? ? ? ?= 0,
> };

Sure.

>
>> +struct atp867x_priv {
>> + ? ? ? unsigned long ? dma_mode;
>> + ? ? ? unsigned long ? mstr_piospd;
>> + ? ? ? unsigned long ? slave_piospd;
>> + ? ? ? unsigned long ? eightb_piospd;
>> + ? ? ? int ? ? ? ? ? ? pci66mhz;
>> +};
>> +
>> +
>> +static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device
>> *adev)
>> +{
>> + ? ? ? struct pci_dev *pdev ? ?= to_pci_dev(ap->host->dev);
>> + ? ? ? struct atp867x_priv *dp = ap->private_data;
>> + ? ? ? u8 speed = adev->dma_mode;
>> + ? ? ? u8 b;
>> + ? ? ? u8 mode;
>> +
>> +
>> + ? ? ? switch (speed) {
>> + ? ? ? case XFER_UDMA_6:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_6;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case XFER_UDMA_5:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_5;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case XFER_UDMA_4:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_4;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case XFER_UDMA_3:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_3;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case XFER_UDMA_2:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_2;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case XFER_UDMA_1:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_1;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case XFER_UDMA_0:
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_0;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>> + ? ? ? ? ? ? ? ? ? ? ? " Default to XFER_UDMA_0.\n", (unsigned)speed);
>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_0;
>
> a table would be nice, preferred over a switch statement. ?You may use
> ARRAY_SIZE() macro to generate a constant at compile time for number of
> elements in array.

OK. I had it in a pure math like mode = speed - XFER_UDMA_0 +1;

>
>
>> +
>> + ? ? ? /*
>> + ? ? ? ?* Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA
>> speed
>> + ? ? ? ?* on 66MHz bus
>> + ? ? ? ?* ? rev-A: UDMA_1~4 (5, 6 no change)
>> + ? ? ? ?* ? rev-B: all UDMA modes
>> + ? ? ? ?* ? UDMA_0 stays not to disable UDMA
>> + ? ? ? ?*/
>> + ? ? ? if (dp->pci66mhz&& ?mode> ?ATP867X_IO_DMAMODE_UDMA_0&&
>> + ? ? ? ? ?(pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
>> + ? ? ? ? ? mode< ?ATP867X_IO_DMAMODE_UDMA_5))
>> + ? ? ? ? ? ? ? mode--;
>> +
>> + ? ? ? b = inb(dp->dma_mode);
>> + ? ? ? if (adev->devno& ?1) {
>> + ? ? ? ? ? ? ? b = (b& ?~ATP867X_IO_DMAMODE_SLAVE_MASK) |
>> + ? ? ? ? ? ? ? ? ? ? ? (mode<< ?ATP867X_IO_DMAMODE_SLAVE_SHIFT);
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? b = (b& ?~ATP867X_IO_DMAMODE_MSTR_MASK) |
>> + ? ? ? ? ? ? ? ? ? ? ? (mode<< ?ATP867X_IO_DMAMODE_MSTR_SHIFT);
>
> add whitespace between all tokens, eg. surrounding "&&" and ">" and "&"
> operators

Weird. I don;t these in my source/git diff file.

>
>
>> + ? ? ? }
>> + ? ? ? outb(b, dp->dma_mode);
>> +}
>> +
>> +static int atp867x_get_active_clocks_shifted(unsigned int clk)
>> +{
>> + ? ? ? unsigned char clocks = clk;
>> +
>> + ? ? ? switch (clocks) {
>> + ? ? ? case 0:
>> + ? ? ? ? ? ? ? clocks = 1;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case 1 ... 7:
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case 8 ... 12:
>> + ? ? ? ? ? ? ? clocks = 7;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
>> + ? ? ? ? ? ? ? ? ? ? ? "Using default 8clk.\n", clk);
>> + ? ? ? ? ? ? ? clocks = 0; ? ? /* 8 clk */
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? }
>> + ? ? ? return clocks<< ?ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
>> +}
>> +
>> +static int atp867x_get_recover_clocks_shifted(unsigned int clk)
>> +{
>> + ? ? ? unsigned char clocks = clk;
>> +
>> + ? ? ? switch (clocks) {
>> + ? ? ? case 0:
>> + ? ? ? ? ? ? ? clocks = 1;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case 1 ... 11:
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case 12:
>> + ? ? ? ? ? ? ? clocks = 0;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case 13: case 14:
>> + ? ? ? ? ? ? ? --clocks;
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case 15:
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
>> + ? ? ? ? ? ? ? ? ? ? ? "Using default 15clk.\n", clk);
>> + ? ? ? ? ? ? ? clocks = 0; ? ? /* 12 clk */
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? }
>> + ? ? ? return clocks<< ?ATP867X_IO_PIOSPD_RECOVER_SHIFT;
>
> whitespace between all tokens

ditto

>
>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
>> *adev)
>> +{
>> + ? ? ? struct ata_device *peer = ata_dev_pair(adev);
>> + ? ? ? struct atp867x_priv *dp = ap->private_data;
>> + ? ? ? u8 speed = adev->pio_mode;
>> + ? ? ? struct ata_timing t, p;
>> + ? ? ? int T, UT;
>> + ? ? ? u8 b;
>> +
>> + ? ? ? T = 1000000000 / 33333;
>> + ? ? ? UT = T/4;
>
> ditto

Done.

>
>
>> + ? ? ? switch (speed) {
>> + ? ? ? case XFER_PIO_4:
>> + ? ? ? case XFER_PIO_3:
>> + ? ? ? case XFER_PIO_2:
>> + ? ? ? case XFER_PIO_1:
>> + ? ? ? case XFER_PIO_0:
>> + ? ? ? case XFER_PIO_SLOW:
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? default:
>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>> + ? ? ? ? ? ? ? ? ? ? ? " Default to XFER_PIO_0.\n", (unsigned)speed);
>> + ? ? ? ? ? ? ? speed = XFER_PIO_0;
>
> unsupported speeds are masked out via ata_port_info. ?no need for additional
> checks.

Done.

>
>
>> + ? ? ? ata_timing_compute(adev, speed,&t, T, UT);
>> + ? ? ? if (peer&& ?peer->pio_mode) {
>> + ? ? ? ? ? ? ? ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
>> + ? ? ? ? ? ? ? ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
>> + ? ? ? }
>> +
>> + ? ? ? b = inb(dp->dma_mode);
>> + ? ? ? if (adev->devno& ?1)
>> + ? ? ? ? ? ? ? b = (b& ?~ATP867X_IO_DMAMODE_SLAVE_MASK);
>> + ? ? ? else
>> + ? ? ? ? ? ? ? b = (b& ?~ATP867X_IO_DMAMODE_MSTR_MASK);
>> + ? ? ? outb(b, dp->dma_mode);
>> +
>> + ? ? ? b = atp867x_get_active_clocks_shifted(t.active) |
>> + ? ? ? ? ? ? ? atp867x_get_recover_clocks_shifted(t.recover);
>> + ? ? ? if (dp->pci66mhz)
>> + ? ? ? ? ? ? ? b += 0x10;
>> +
>> + ? ? ? if (adev->devno& ?1)
>> + ? ? ? ? ? ? ? outb(b, dp->slave_piospd);
>> + ? ? ? else
>> + ? ? ? ? ? ? ? outb(b, dp->mstr_piospd);
>> +
>> + ? ? ? /*
>> + ? ? ? ?* use the same value for comand timing as for PIO timimg
>> + ? ? ? ?*/
>> + ? ? ? outb(b, dp->eightb_piospd);
>
> whitespace + ioread/iowrite

ditto.

>
>
>> +static int atp867x_cable_detect(struct ata_port *ap)
>> +{
>> + ? ? ? return ATA_CBL_PATA40_SHORT;
>> +}
>> +
>> +static struct scsi_host_template atp867x_sht = {
>> + ? ? ? ATA_BMDMA_SHT(DRV_NAME),
>> +};
>> +
>> +static struct ata_port_operations atp867x_ops = {
>> + ? ? ? .inherits ? ? ? ? ? ? ? =&ata_bmdma_port_ops,
>> + ? ? ? .cable_detect ? ? ? ? ? = atp867x_cable_detect,
>> + ? ? ? .set_piomode ? ? ? ? ? ?= atp867x_set_piomode,
>> + ? ? ? .set_dmamode ? ? ? ? ? ?= atp867x_set_dmamode,
>> +};
>> +
>> +
>> +#ifdef ATP867X_DEBUG
>> +static void atp867x_check_res(struct pci_dev *pdev)
>> +{
>> + ? ? ? int i;
>> + ? ? ? unsigned long start, len;
>> +
>> + ? ? ? /* Check the PCI resources for this channel are enabled */
>> + ? ? ? for (i = 0; i< ?DEVICE_COUNT_RESOURCE; i++) {
>> + ? ? ? ? ? ? ? start = pci_resource_start(pdev, i);
>> + ? ? ? ? ? ? ? len ? = pci_resource_len(pdev, i);
>> + ? ? ? ? ? ? ? printk(KERN_DEBUG "ATP867X: resource start:len=%lx:%lx\n",
>> + ? ? ? ? ? ? ? ? ? ? ? start, len);
>> + ? ? ? }
>> +}
>> +
>> +static void atp867x_check_ports(struct ata_port *ap, struct pci_dev
>> *pdev)
>> +{
>> + ? ? ? struct ata_ioports *ioaddr =&ap->ioaddr;
>> + ? ? ? struct atp867x_priv *dp = ap->private_data;
>> + ? ? ? int port = ap->port_no;
>> +
>> + ? ? ? printk(KERN_DEBUG "ATP867X: port[%d] addresses\n"
>> + ? ? ? ? ? ? ? " ?cmd_addr ? ? =0x%llx, 0x%llx\n"
>> + ? ? ? ? ? ? ? " ?ctl_addr ? ? =0x%llx, 0x%llx\n"
>> + ? ? ? ? ? ? ? " ?bmdma_addr ? =0x%llx, 0x%llx\n"
>> + ? ? ? ? ? ? ? " ?data_addr ? ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?error_addr ? =0x%llx\n"
>> + ? ? ? ? ? ? ? " ?feature_addr =0x%llx\n"
>> + ? ? ? ? ? ? ? " ?nsect_addr ? =0x%llx\n"
>> + ? ? ? ? ? ? ? " ?lbal_addr ? ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?lbam_addr ? ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?lbah_addr ? ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?device_addr ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?status_addr ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?command_addr =0x%llx\n"
>> + ? ? ? ? ? ? ? " ?dp->dma_mode =0x%llx\n"
>> + ? ? ? ? ? ? ? " ?dp->mstr_piospd ? ? ?=0x%llx\n"
>> + ? ? ? ? ? ? ? " ?dp->slave_piospd ? ? =0x%llx\n"
>> + ? ? ? ? ? ? ? " ?dp->eightb_piospd ? ?=0x%llx\n",
>> + ? ? ? ? ? ? ? port,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->cmd_addr,
>> + ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_PORTBASE(pdev, port),
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->ctl_addr,
>> + ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_ALTSTATUS(pdev, port),
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->bmdma_addr,
>> + ? ? ? ? ? ? ? ? ? ? ? ATP867X_IO_DMABASE(pdev, port),
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->data_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->error_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->feature_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->nsect_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->lbal_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->lbam_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->lbah_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->device_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->status_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)ioaddr->command_addr,
>> + ? ? ? ? ? ? ? (unsigned long long)dp->dma_mode,
>> + ? ? ? ? ? ? ? (unsigned long long)dp->mstr_piospd,
>> + ? ? ? ? ? ? ? (unsigned long long)dp->slave_piospd,
>> + ? ? ? ? ? ? ? (unsigned long long)dp->eightb_piospd);
>> +}
>> +#endif
>> +
>> +static inline void atp867x_get_ports(struct pci_dev *dev, unsigned int
>> port,
>> + ? ? ? unsigned long *base, unsigned long *control)
>> +{
>> + ? ? ? *base = ATP867X_IO_PORTBASE(dev, port);
>> + ? ? ? *control = ATP867X_IO_ALTSTATUS(dev, port);
>> +}
>> +
>> +static inline unsigned long atp867x_get_dma_base(struct pci_dev *dev,
>> + ? ? ? unsigned int port)
>> +{
>> + ? ? ? return ATP867X_IO_DMABASE(dev, port);
>> +}
>> +
>> +static int atp867x_set_priv(struct ata_port *ap)
>> +{
>> + ? ? ? struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>> + ? ? ? struct atp867x_priv *dp;
>> + ? ? ? int port = ap->port_no;
>> +
>> + ? ? ? dp = ap->private_data =
>> + ? ? ? ? ? ? ? devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
>> + ? ? ? if (dp == NULL)
>> + ? ? ? ? ? ? ? return -ENOMEM;
>> +
>> + ? ? ? dp->dma_mode ? ? = ATP867X_IO_DMAMODE(pdev, port);
>> + ? ? ? dp->mstr_piospd ?= ATP867X_IO_MSTRPIOSPD(pdev, port);
>> + ? ? ? dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(pdev, port);
>> + ? ? ? dp->eightb_piospd = ATP867X_IO_8BPIOSPD(pdev, port);
>> +
>> + ? ? ? dp->pci66mhz = inb(ATP867X_SYS_INFO(pdev))&
>> ?ATP867X_IO_SYS_INFO_66MHZ;
>> +
>> + ? ? ? return 0;
>> +}
>> +
>> +static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
>> +{
>> + ? ? ? struct device *gdev = host->dev;
>> + ? ? ? struct pci_dev *pdev = to_pci_dev(gdev);
>> + ? ? ? unsigned int mask = 0;
>> + ? ? ? int i, rc;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* do not map rombase
>> + ? ? ? ?*/
>> + ? ? ? rc = pcim_iomap_regions(pdev, 1<< ?ATP867X_BAR_IOBASE, DRV_NAME);
>> + ? ? ? if (rc == -EBUSY)
>> + ? ? ? ? ? ? ? pcim_pin_device(pdev);
>> + ? ? ? if (rc)
>> + ? ? ? ? ? ? ? return rc;
>> + ? ? ? host->iomap = pcim_iomap_table(pdev);
>> +
>> +#ifdef ATP867X_DEBUG
>> + ? ? ? atp867x_check_res(pdev);
>> +
>> + ? ? ? for (i = 0; i< ?PCI_ROM_RESOURCE; i++)
>> + ? ? ? ? ? ? ? printk(KERN_DEBUG "ATP867X: iomap[%d]=0x%llx\n", i,
>> + ? ? ? ? ? ? ? ? ? ? ? (unsigned long long)(host->iomap[i]));
>> +#endif
>> +
>> + ? ? ? /*
>> + ? ? ? ?* request, iomap BARs and init port addresses accordingly
>> + ? ? ? ?*/
>> + ? ? ? for (i = 0; i< ?host->n_ports; i++) {
>> + ? ? ? ? ? ? ? unsigned long base, control, dmabase;
>> + ? ? ? ? ? ? ? struct ata_port *ap = host->ports[i];
>> + ? ? ? ? ? ? ? struct ata_ioports *ioaddr =&ap->ioaddr;
>> +
>> + ? ? ? ? ? ? ? atp867x_get_ports(pdev, i,&base,&control);
>> + ? ? ? ? ? ? ? dmabase = atp867x_get_dma_base(pdev, i);
>> +
>> + ? ? ? ? ? ? ? ioaddr->cmd_addr = devm_ioport_map(gdev, base, 8);
>> + ? ? ? ? ? ? ? ioaddr->ctl_addr = ioaddr->altstatus_addr =
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? devm_ioport_map(gdev, control, 1);
>> + ? ? ? ? ? ? ? ioaddr->bmdma_addr = devm_ioport_map(gdev, dmabase, 8);
>
> why are you mapping separately from pcim_iomap_regions?
>
> Just do the mapping all at once, with pcim_iomap_regions, then simply
> calculate addresses in the n_ports loop.

OK.

>
>
>
>
>> + ? ? ? rc = pcim_enable_device(pdev);
>> + ? ? ? if (rc)
>> + ? ? ? ? ? ? ? return rc;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* Broken BIOS might not set latency high enough
>> + ? ? ? ?*/
>> + ? ? ? pci_read_config_byte(pdev, PCI_LATENCY_TIMER,&v);
>> + ? ? ? if (v< ?0x80) {
>> + ? ? ? ? ? ? ? v = 0x80;
>> + ? ? ? ? ? ? ? pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
>> + ? ? ? ? ? ? ? printk(KERN_DEBUG "ATP867X: set latency timer of device
>> %s"
>> + ? ? ? ? ? ? ? ? ? ? ? " to %d\n", pci_name(pdev), v);
>> + ? ? ? }
>
> this seems pointless - pci_set_master() already does this
>
pci_set_master won't re-set it if BIOS set it to somewhere between 16
and 256. This controller wants 0x80.
so, if BIOS set to less than 0x80, like 0x20, pci_set_master will keep
the value.
I could do this via pci fixup or quirks but that seems too much for
this simple setting.

Will send a new rev soon.
Thanks,
-John

>

2009-09-13 03:12:54

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On 09/12/2009 10:41 PM, Jung-Ik (John) Lee wrote:
> On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik<[email protected]> wrote:
>>
>>
>> General comment:
>>
>> * since you use iomap to map the region, you should use ioread{8,16,32} /
>> iowrite{8,16,32} accessors. Do not use inb/outb/inl/outl/etc.
> .
> I used them for runtime hot registers by separately mapping them
> simply to avoid an extra overhead of ioread/iowrite, over the
> portability.
> I know it's not a good idea but in this case for these hot ports can
> in/out be used?

It is _highly_ unlikely that the overhead is even measureable above the
noise, I would think. Do you have data showing that ioread/iowrite
impose a noticeable penalty?


>
>
>>
>> * run through scripts/checkpatch.pl
>>
>
> Weird. I don't see any WS issues you pointed below in my source code
> or git diff file, except UT = T/4 below.

My apologies; most of those appear to be problems with Thunderbird. I
think it renders <tab> incorrectly.


>>> +static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device
>>> *adev)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>>> + struct atp867x_priv *dp = ap->private_data;
>>> + u8 speed = adev->dma_mode;
>>> + u8 b;
>>> + u8 mode;
>>> +
>>> +
>>> + switch (speed) {
>>> + case XFER_UDMA_6:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_6;
>>> + break;
>>> + case XFER_UDMA_5:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_5;
>>> + break;
>>> + case XFER_UDMA_4:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_4;
>>> + break;
>>> + case XFER_UDMA_3:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_3;
>>> + break;
>>> + case XFER_UDMA_2:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_2;
>>> + break;
>>> + case XFER_UDMA_1:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_1;
>>> + break;
>>> + case XFER_UDMA_0:
>>> + mode = ATP867X_IO_DMAMODE_UDMA_0;
>>> + break;
>>> + default:
>>> + printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>>> + " Default to XFER_UDMA_0.\n", (unsigned)speed);
>>> + mode = ATP867X_IO_DMAMODE_UDMA_0;
>>
>> a table would be nice, preferred over a switch statement. You may use
>> ARRAY_SIZE() macro to generate a constant at compile time for number of
>> elements in array.
>
> OK. I had it in a pure math like mode = speed - XFER_UDMA_0 +1;

That's fine too.




>>> + /*
>>> + * Broken BIOS might not set latency high enough
>>> + */
>>> + pci_read_config_byte(pdev, PCI_LATENCY_TIMER,&v);
>>> + if (v< 0x80) {
>>> + v = 0x80;
>>> + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
>>> + printk(KERN_DEBUG "ATP867X: set latency timer of device
>>> %s"
>>> + " to %d\n", pci_name(pdev), v);
>>> + }
>>
>> this seems pointless - pci_set_master() already does this
>>
> pci_set_master won't re-set it if BIOS set it to somewhere between 16
> and 256. This controller wants 0x80.
> so, if BIOS set to less than 0x80, like 0x20, pci_set_master will keep
> the value.
> I could do this via pci fixup or quirks but that seems too much for
> this simple setting.

Given your explanation, that's fine.

Jeff


2009-09-13 04:55:57

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Sat, Sep 12, 2009 at 8:12 PM, Jeff Garzik <[email protected]> wrote:
> On 09/12/2009 10:41 PM, Jung-Ik (John) Lee wrote:
>>
>> On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik<[email protected]> ?wrote:
>>>
>>>
>>> General comment:
>>>
>>> * since you use iomap to map the region, you should use ioread{8,16,32} /
>>> iowrite{8,16,32} accessors. ?Do not use inb/outb/inl/outl/etc.
>>
>> .
>> I used them for runtime hot registers by separately mapping them
>> simply to avoid an extra overhead of ioread/iowrite, over the
>> portability.
>> I know it's not a good idea but in this case for these hot ports can
>> in/out be used?
>
> It is _highly_ unlikely that the overhead is even measureable above the
> noise, I would think. ?Do you have data showing that ioread/iowrite impose a
> noticeable penalty?

I agree in that it's hard to measure/signify the additional overhead,
since those io insts are already too slow.
Anyways, the two extra "if"s and one PIO_MASK on every ioread/iowrite
are pure overhead on top of in/out insts.

Thanks,
-John

>
>
>>
>>
>>>
>>> * run through scripts/checkpatch.pl
>>>
>>
>> Weird. I don't see any WS issues you pointed below in my source code
>> or git diff file, except UT = T/4 below.
>
> My apologies; most of those appear to be problems with Thunderbird. ?I think
> it renders <tab> incorrectly.
>
>
>>>> +static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device
>>>> *adev)
>>>> +{
>>>> + ? ? ? struct pci_dev *pdev ? ?= to_pci_dev(ap->host->dev);
>>>> + ? ? ? struct atp867x_priv *dp = ap->private_data;
>>>> + ? ? ? u8 speed = adev->dma_mode;
>>>> + ? ? ? u8 b;
>>>> + ? ? ? u8 mode;
>>>> +
>>>> +
>>>> + ? ? ? switch (speed) {
>>>> + ? ? ? case XFER_UDMA_6:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_6;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? case XFER_UDMA_5:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_5;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? case XFER_UDMA_4:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_4;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? case XFER_UDMA_3:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_3;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? case XFER_UDMA_2:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_2;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? case XFER_UDMA_1:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_1;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? case XFER_UDMA_0:
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_0;
>>>> + ? ? ? ? ? ? ? break;
>>>> + ? ? ? default:
>>>> + ? ? ? ? ? ? ? printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>>>> + ? ? ? ? ? ? ? ? ? ? ? " Default to XFER_UDMA_0.\n", (unsigned)speed);
>>>> + ? ? ? ? ? ? ? mode = ATP867X_IO_DMAMODE_UDMA_0;
>>>
>>> a table would be nice, preferred over a switch statement. ?You may use
>>> ARRAY_SIZE() macro to generate a constant at compile time for number of
>>> elements in array.
>>
>> OK. I had it in a pure math like mode = speed - XFER_UDMA_0 +1;
>
> That's fine too.
>
>
>
>
>>>> + ? ? ? /*
>>>> + ? ? ? ?* Broken BIOS might not set latency high enough
>>>> + ? ? ? ?*/
>>>> + ? ? ? pci_read_config_byte(pdev, PCI_LATENCY_TIMER,&v);
>>>> + ? ? ? if (v< ? ?0x80) {
>>>> + ? ? ? ? ? ? ? v = 0x80;
>>>> + ? ? ? ? ? ? ? pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
>>>> + ? ? ? ? ? ? ? printk(KERN_DEBUG "ATP867X: set latency timer of device
>>>> %s"
>>>> + ? ? ? ? ? ? ? ? ? ? ? " to %d\n", pci_name(pdev), v);
>>>> + ? ? ? }
>>>
>>> this seems pointless - pci_set_master() already does this
>>>
>> pci_set_master won't re-set it if BIOS set it to somewhere between 16
>> and 256. This controller wants 0x80.
>> so, if BIOS set to less than 0x80, like 0x20, pci_set_master will keep
>> the value.
>> I could do this via pci fixup or quirks but that seems too much for
>> this simple setting.
>
> Given your explanation, that's fine.
>
> ? ? ? ?Jeff
>
>
>
>

2009-09-13 05:50:45

by Robert Hancock

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On Sat, Sep 12, 2009 at 10:55 PM, Jung-Ik (John) Lee <[email protected]> wrote:
> I agree in that it's hard to measure/signify the additional overhead,
> since those io insts are already too slow.
> Anyways, the two extra "if"s and one PIO_MASK on every ioread/iowrite
> are pure overhead on top of in/out insts.

I don't think that any platform currently breaks if you use the
readX/writeX instead of ioreadX/iowriteX on iomapped MMIO addresses
(and there's certainly enough code in libata that does this), but the
API definition really expects that you should be using ioread/iowrite
so it would probably be best not to add any more of these..

2009-09-15 14:53:12

by Alan

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

> Ideally you would use ATA_CBL_PATA_UNK / ata_cable_unknown, but it seems
> that ATA_CBL_PATA_UNK has grown rather useless in the current kernel.
>
> We should probably (a) update libata-core/libata-sff to do something
> intelligent with ATA_CBL_PATA_UNK,

It does - it says "Use drive side detect the controller hasn't got a
clue".

Alan

2009-09-15 04:33:02

by Jung-Ik (John) Lee

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

From: John(Jung-Ik) Lee <[email protected]>

This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA ctrls.
Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide codes
from Eric Uhrhane <[email protected]>.

Signed-off-by: John(Jung-Ik) Lee <[email protected]>
Reviewed-by: Grant Grundler <[email protected]>
Reviewed-by: Gwendal Gringo <[email protected]>
---

rev 01: Cleared some checkpatch.pl complaints. Thanks to Daniel.
rev 02: Removed hard-coded value for active/recovery. Thanks to Robert.
Use atp867x_cable_detect() until ata_cable_40short()
rev 03: changes per Jeff's comment: Thansk to Jeff.
ioread/iowrite, enum, speed-to-mode calc, single mapping, etc.

drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/pata_atp867x.c | 548 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 2 +
4 files changed, 560 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index b17c57f..2e1e678 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -244,6 +244,15 @@ config PATA_ARTOP

If unsure, say N.

+config PATA_ATP867X
+ tristate "ARTOP/Acard ATP867X PATA support"
+ depends on PCI
+ help
+ This option enables support for ARTOP/Acard ATP867X PATA
+ controllers.
+
+ If unsure, say N.
+
config PATA_AT32
tristate "Atmel AVR32 PATA support (Experimental)"
depends on AVR32 && PLATFORM_AT32AP && EXPERIMENTAL
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 38906f9..3372007 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SATA_FSL) += sata_fsl.o
obj-$(CONFIG_PATA_ALI) += pata_ali.o
obj-$(CONFIG_PATA_AMD) += pata_amd.o
obj-$(CONFIG_PATA_ARTOP) += pata_artop.o
+obj-$(CONFIG_PATA_ATP867X) += pata_atp867x.o
obj-$(CONFIG_PATA_AT32) += pata_at32.o
obj-$(CONFIG_PATA_ATIIXP) += pata_atiixp.o
obj-$(CONFIG_PATA_CMD640_PCI) += pata_cmd640.o
diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
new file mode 100644
index 0000000..7990de9
--- /dev/null
+++ b/drivers/ata/pata_atp867x.c
@@ -0,0 +1,548 @@
+/*
+ * pata_atp867x.c - ARTOP 867X 64bit 4-channel UDMA133 ATA controller driver
+ *
+ * (C) 2009 Google Inc. John(Jung-Ik) Lee <[email protected]>
+ *
+ * Per Atp867 data sheet rev 1.2, Acard.
+ * Based in part on early ide code from
+ * 2003-2004 by Eric Uhrhane, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ *
+ * TODO:
+ * 1. RAID features [comparison, XOR, striping, mirroring, etc.]
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <scsi/scsi_host.h>
+#include <linux/libata.h>
+
+#define DRV_NAME "pata_atp867x"
+#define DRV_VERSION "0.7.5"
+
+/*
+ * IO Registers
+ * Note that all runtime hot priv ports are cached in ap private_data
+ */
+
+enum {
+ ATP867X_IO_CHANNEL_OFFSET = 0x10,
+
+ /*
+ * IO Register Bitfields
+ */
+
+ ATP867X_IO_PIOSPD_ACTIVE_SHIFT = 4,
+ ATP867X_IO_PIOSPD_RECOVER_SHIFT = 0,
+
+ ATP867X_IO_DMAMODE_MSTR_SHIFT = 0,
+ ATP867X_IO_DMAMODE_MSTR_MASK = 0x07,
+ ATP867X_IO_DMAMODE_SLAVE_SHIFT = 4,
+ ATP867X_IO_DMAMODE_SLAVE_MASK = 0x70,
+
+ ATP867X_IO_DMAMODE_UDMA_6 = 0x07,
+ ATP867X_IO_DMAMODE_UDMA_5 = 0x06,
+ ATP867X_IO_DMAMODE_UDMA_4 = 0x05,
+ ATP867X_IO_DMAMODE_UDMA_3 = 0x04,
+ ATP867X_IO_DMAMODE_UDMA_2 = 0x03,
+ ATP867X_IO_DMAMODE_UDMA_1 = 0x02,
+ ATP867X_IO_DMAMODE_UDMA_0 = 0x01,
+ ATP867X_IO_DMAMODE_DISABLE = 0x00,
+
+ ATP867X_IO_SYS_INFO_66MHZ = 0x04,
+ ATP867X_IO_SYS_INFO_SLOW_UDMA5 = 0x02,
+ ATP867X_IO_SYS_MASK_RESERVED = (~0xf1),
+
+ ATP867X_IO_PORTSPD_VAL = 0x1143,
+ ATP867X_PREREAD_VAL = 0x0200,
+
+ ATP867X_NUM_PORTS = 4,
+ ATP867X_BAR_IOBASE = 0,
+ ATP867X_BAR_ROMBASE = 6,
+};
+
+#define ATP867X_IOBASE(ap) ((ap)->host->iomap[0])
+#define ATP867X_SYS_INFO(ap) (0x3F + ATP867X_IOBASE(ap))
+
+#define ATP867X_IO_PORTBASE(ap, port) (0x00 + ATP867X_IOBASE(ap) + \
+ (port) * ATP867X_IO_CHANNEL_OFFSET)
+#define ATP867X_IO_DMABASE(ap, port) (0x40 + \
+ ATP867X_IO_PORTBASE((ap), (port)))
+
+#define ATP867X_IO_STATUS(ap, port) (0x07 + \
+ ATP867X_IO_PORTBASE((ap), (port)))
+#define ATP867X_IO_ALTSTATUS(ap, port) (0x0E + \
+ ATP867X_IO_PORTBASE((ap), (port)))
+
+/*
+ * hot priv ports
+ */
+#define ATP867X_IO_MSTRPIOSPD(ap, port) (0x08 + \
+ ATP867X_IO_DMABASE((ap), (port)))
+#define ATP867X_IO_SLAVPIOSPD(ap, port) (0x09 + \
+ ATP867X_IO_DMABASE((ap), (port)))
+#define ATP867X_IO_8BPIOSPD(ap, port) (0x0A + \
+ ATP867X_IO_DMABASE((ap), (port)))
+#define ATP867X_IO_DMAMODE(ap, port) (0x0B + \
+ ATP867X_IO_DMABASE((ap), (port)))
+
+#define ATP867X_IO_PORTSPD(ap, port) (0x4A + \
+ ATP867X_IO_PORTBASE((ap), (port)))
+#define ATP867X_IO_PREREAD(ap, port) (0x4C + \
+ ATP867X_IO_PORTBASE((ap), (port)))
+
+struct atp867x_priv {
+ void __iomem *dma_mode;
+ void __iomem *mstr_piospd;
+ void __iomem *slave_piospd;
+ void __iomem *eightb_piospd;
+ int pci66mhz;
+};
+
+static inline u8 atp867x_speed_to_mode(u8 speed)
+{
+ return speed - XFER_UDMA_0 + 1;
+}
+
+static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->dma_mode;
+ u8 b;
+ u8 mode;
+
+ mode = atp867x_speed_to_mode(speed);
+
+ /*
+ * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
+ * on 66MHz bus
+ * rev-A: UDMA_1~4 (5, 6 no change)
+ * rev-B: all UDMA modes
+ * UDMA_0 stays not to disable UDMA
+ */
+ if (dp->pci66mhz && mode > ATP867X_IO_DMAMODE_UDMA_0 &&
+ (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
+ mode < ATP867X_IO_DMAMODE_UDMA_5))
+ mode--;
+
+ b = ioread8(dp->dma_mode);
+ if (adev->devno & 1) {
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
+ (mode << ATP867X_IO_DMAMODE_SLAVE_SHIFT);
+ } else {
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK) |
+ (mode << ATP867X_IO_DMAMODE_MSTR_SHIFT);
+ }
+ iowrite8(b, dp->dma_mode);
+}
+
+static int atp867x_get_active_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 7:
+ break;
+ case 8 ... 12:
+ clocks = 7;
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
+ "Using default 8clk.\n", clk);
+ clocks = 0; /* 8 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
+}
+
+static int atp867x_get_recover_clocks_shifted(unsigned int clk)
+{
+ unsigned char clocks = clk;
+
+ switch (clocks) {
+ case 0:
+ clocks = 1;
+ break;
+ case 1 ... 11:
+ break;
+ case 12:
+ clocks = 0;
+ break;
+ case 13: case 14:
+ --clocks;
+ break;
+ case 15:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
+ "Using default 15clk.\n", clk);
+ clocks = 0; /* 12 clk */
+ break;
+ }
+ return clocks << ATP867X_IO_PIOSPD_RECOVER_SHIFT;
+}
+
+static void atp867x_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct ata_device *peer = ata_dev_pair(adev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->pio_mode;
+ struct ata_timing t, p;
+ int T, UT;
+ u8 b;
+
+ T = 1000000000 / 33333;
+ UT = T / 4;
+
+ ata_timing_compute(adev, speed, &t, T, UT);
+ if (peer && peer->pio_mode) {
+ ata_timing_compute(peer, peer->pio_mode, &p, T, UT);
+ ata_timing_merge(&p, &t, &t, ATA_TIMING_8BIT);
+ }
+
+ b = ioread8(dp->dma_mode);
+ if (adev->devno & 1)
+ b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK);
+ else
+ b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK);
+ iowrite8(b, dp->dma_mode);
+
+ b = atp867x_get_active_clocks_shifted(t.active) |
+ atp867x_get_recover_clocks_shifted(t.recover);
+ if (dp->pci66mhz)
+ b += 0x10;
+
+ if (adev->devno & 1)
+ iowrite8(b, dp->slave_piospd);
+ else
+ iowrite8(b, dp->mstr_piospd);
+
+ /*
+ * use the same value for comand timing as for PIO timimg
+ */
+ iowrite8(b, dp->eightb_piospd);
+}
+
+static int atp867x_cable_detect(struct ata_port *ap)
+{
+ return ATA_CBL_PATA40_SHORT;
+}
+
+static struct scsi_host_template atp867x_sht = {
+ ATA_BMDMA_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations atp867x_ops = {
+ .inherits = &ata_bmdma_port_ops,
+ .cable_detect = atp867x_cable_detect,
+ .set_piomode = atp867x_set_piomode,
+ .set_dmamode = atp867x_set_dmamode,
+};
+
+
+#ifdef ATP867X_DEBUG
+static void atp867x_check_res(struct pci_dev *pdev)
+{
+ int i;
+ unsigned long start, len;
+
+ /* Check the PCI resources for this channel are enabled */
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+ start = pci_resource_start(pdev, i);
+ len = pci_resource_len(pdev, i);
+ printk(KERN_DEBUG "ATP867X: resource start:len=%lx:%lx\n",
+ start, len);
+ }
+}
+
+static void atp867x_check_ports(struct ata_port *ap, int port)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+ struct atp867x_priv *dp = ap->private_data;
+
+ printk(KERN_DEBUG "ATP867X: port[%d] addresses\n"
+ " cmd_addr =0x%llx, 0x%llx\n"
+ " ctl_addr =0x%llx, 0x%llx\n"
+ " bmdma_addr =0x%llx, 0x%llx\n"
+ " data_addr =0x%llx\n"
+ " error_addr =0x%llx\n"
+ " feature_addr =0x%llx\n"
+ " nsect_addr =0x%llx\n"
+ " lbal_addr =0x%llx\n"
+ " lbam_addr =0x%llx\n"
+ " lbah_addr =0x%llx\n"
+ " device_addr =0x%llx\n"
+ " status_addr =0x%llx\n"
+ " command_addr =0x%llx\n"
+ " dp->dma_mode =0x%llx\n"
+ " dp->mstr_piospd =0x%llx\n"
+ " dp->slave_piospd =0x%llx\n"
+ " dp->eightb_piospd =0x%llx\n"
+ " dp->pci66mhz =0x%lx\n",
+ port,
+ (unsigned long long)ioaddr->cmd_addr,
+ (unsigned long long)ATP867X_IO_PORTBASE(ap, port),
+ (unsigned long long)ioaddr->ctl_addr,
+ (unsigned long long)ATP867X_IO_ALTSTATUS(ap, port),
+ (unsigned long long)ioaddr->bmdma_addr,
+ (unsigned long long)ATP867X_IO_DMABASE(ap, port),
+ (unsigned long long)ioaddr->data_addr,
+ (unsigned long long)ioaddr->error_addr,
+ (unsigned long long)ioaddr->feature_addr,
+ (unsigned long long)ioaddr->nsect_addr,
+ (unsigned long long)ioaddr->lbal_addr,
+ (unsigned long long)ioaddr->lbam_addr,
+ (unsigned long long)ioaddr->lbah_addr,
+ (unsigned long long)ioaddr->device_addr,
+ (unsigned long long)ioaddr->status_addr,
+ (unsigned long long)ioaddr->command_addr,
+ (unsigned long long)dp->dma_mode,
+ (unsigned long long)dp->mstr_piospd,
+ (unsigned long long)dp->slave_piospd,
+ (unsigned long long)dp->eightb_piospd,
+ (unsigned long)dp->pci66mhz);
+}
+#endif
+
+static int atp867x_set_priv(struct ata_port *ap)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ struct atp867x_priv *dp;
+ int port = ap->port_no;
+
+ dp = ap->private_data =
+ devm_kzalloc(&pdev->dev, sizeof(*dp), GFP_KERNEL);
+ if (dp == NULL)
+ return -ENOMEM;
+
+ dp->dma_mode = ATP867X_IO_DMAMODE(ap, port);
+ dp->mstr_piospd = ATP867X_IO_MSTRPIOSPD(ap, port);
+ dp->slave_piospd = ATP867X_IO_SLAVPIOSPD(ap, port);
+ dp->eightb_piospd = ATP867X_IO_8BPIOSPD(ap, port);
+
+ dp->pci66mhz =
+ ioread8(ATP867X_SYS_INFO(ap)) & ATP867X_IO_SYS_INFO_66MHZ;
+
+ return 0;
+}
+
+static void atp867x_fixup(struct ata_host *host)
+{
+ struct pci_dev *pdev = to_pci_dev(host->dev);
+ struct ata_port *ap = host->ports[0];
+ int i;
+ u8 v;
+
+ /*
+ * Broken BIOS might not set latency high enough
+ */
+ pci_read_config_byte(pdev, PCI_LATENCY_TIMER, &v);
+ if (v < 0x80) {
+ v = 0x80;
+ pci_write_config_byte(pdev, PCI_LATENCY_TIMER, v);
+ printk(KERN_DEBUG "ATP867X: set latency timer of device %s"
+ " to %d\n", pci_name(pdev), v);
+ }
+
+ /*
+ * init 8bit io ports speed(0aaarrrr) to 43h and
+ * init udma modes of master/slave to 0/0(11h)
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ iowrite16(ATP867X_IO_PORTSPD_VAL, ATP867X_IO_PORTSPD(ap, i));
+
+ /*
+ * init PreREAD counts
+ */
+ for (i = 0; i < ATP867X_NUM_PORTS; i++)
+ iowrite16(ATP867X_PREREAD_VAL, ATP867X_IO_PREREAD(ap, i));
+
+ v = ioread8(ATP867X_IOBASE(ap) + 0x28);
+ v &= 0xcf; /* Enable INTA#: bit4=0 means enable */
+ v |= 0xc0; /* Enable PCI burst, MRM & not immediate interrupts */
+ iowrite8(v, ATP867X_IOBASE(ap) + 0x28);
+
+ /*
+ * Turn off the over clocked udma5 mode, only for Rev-B
+ */
+ v = ioread8(ATP867X_SYS_INFO(ap));
+ v &= ATP867X_IO_SYS_MASK_RESERVED;
+ if (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B)
+ v |= ATP867X_IO_SYS_INFO_SLOW_UDMA5;
+ iowrite8(v, ATP867X_SYS_INFO(ap));
+}
+
+static int atp867x_ata_pci_sff_init_host(struct ata_host *host)
+{
+ struct device *gdev = host->dev;
+ struct pci_dev *pdev = to_pci_dev(gdev);
+ unsigned int mask = 0;
+ int i, rc;
+
+ /*
+ * do not map rombase
+ */
+ rc = pcim_iomap_regions(pdev, 1 << ATP867X_BAR_IOBASE, DRV_NAME);
+ if (rc == -EBUSY)
+ pcim_pin_device(pdev);
+ if (rc)
+ return rc;
+ host->iomap = pcim_iomap_table(pdev);
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_res(pdev);
+
+ for (i = 0; i < PCI_ROM_RESOURCE; i++)
+ printk(KERN_DEBUG "ATP867X: iomap[%d]=0x%llx\n", i,
+ (unsigned long long)(host->iomap[i]));
+#endif
+
+ /*
+ * request, iomap BARs and init port addresses accordingly
+ */
+ for (i = 0; i < host->n_ports; i++) {
+ struct ata_port *ap = host->ports[i];
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ ioaddr->cmd_addr = ATP867X_IO_PORTBASE(ap, i);
+ ioaddr->ctl_addr = ioaddr->altstatus_addr
+ = ATP867X_IO_ALTSTATUS(ap, i);
+ ioaddr->bmdma_addr = ATP867X_IO_DMABASE(ap, i);
+
+ ata_sff_std_ports(ioaddr);
+ rc = atp867x_set_priv(ap);
+ if (rc)
+ return rc;
+
+#ifdef ATP867X_DEBUG
+ atp867x_check_ports(ap, i);
+#endif
+ ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
+ (unsigned long)ioaddr->cmd_addr,
+ (unsigned long)ioaddr->ctl_addr);
+ ata_port_desc(ap, "bmdma 0x%lx",
+ (unsigned long)ioaddr->bmdma_addr);
+
+ mask |= 1 << i;
+ }
+
+ if (!mask) {
+ dev_printk(KERN_ERR, gdev, "no available native port\n");
+ return -ENODEV;
+ }
+
+ atp867x_fixup(host);
+
+ rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
+ if (rc)
+ return rc;
+
+ rc = pci_set_consistent_dma_mask(pdev, ATA_DMA_MASK);
+ return rc;
+}
+
+static int atp867x_init_one(struct pci_dev *pdev,
+ const struct pci_device_id *id)
+{
+ static int printed_version;
+ static const struct ata_port_info info_867x = {
+ .flags = ATA_FLAG_SLAVE_POSS,
+ .pio_mask = ATA_PIO4,
+ .mwdma_mask = ATA_MWDMA2,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &atp867x_ops,
+ };
+
+ struct ata_host *host;
+ const struct ata_port_info *ppi[] = { &info_867x, NULL };
+ int rc;
+
+ if (!printed_version++)
+ dev_printk(KERN_INFO, &pdev->dev, "version " DRV_VERSION "\n");
+
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
+ printk(KERN_INFO "ATP867X: ATP867 ATA UDMA133 controller (rev %02X)",
+ pdev->device);
+
+ host = ata_host_alloc_pinfo(&pdev->dev, ppi, ATP867X_NUM_PORTS);
+ if (!host) {
+ dev_printk(KERN_ERR, &pdev->dev,
+ "failed to allocate ATA host\n");
+ rc = -ENOMEM;
+ goto err_out;
+ }
+
+ rc = atp867x_ata_pci_sff_init_host(host);
+ if (rc) {
+ dev_printk(KERN_ERR, &pdev->dev, "failed to init host\n");
+ goto err_out;
+ }
+
+ pci_set_master(pdev);
+
+ rc = ata_host_activate(host, pdev->irq, ata_sff_interrupt,
+ IRQF_SHARED, &atp867x_sht);
+ if (rc)
+ dev_printk(KERN_ERR, &pdev->dev, "failed to activate host\n");
+
+err_out:
+ return rc;
+}
+
+static struct pci_device_id atp867x_pci_tbl[] = {
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867A), 0 },
+ { PCI_VDEVICE(ARTOP, PCI_DEVICE_ID_ARTOP_ATP867B), 0 },
+ { },
+};
+
+static struct pci_driver atp867x_driver = {
+ .name = DRV_NAME,
+ .id_table = atp867x_pci_tbl,
+ .probe = atp867x_init_one,
+ .remove = ata_pci_remove_one,
+};
+
+static int __init atp867x_init(void)
+{
+ return pci_register_driver(&atp867x_driver);
+}
+
+static void __exit atp867x_exit(void)
+{
+ pci_unregister_driver(&atp867x_driver);
+}
+
+MODULE_AUTHOR("John(Jung-Ik) Lee, Google Inc.");
+MODULE_DESCRIPTION("low level driver for Artop/Acard 867x ATA controller");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, atp867x_pci_tbl);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(atp867x_init);
+module_exit(atp867x_exit);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 73b46b6..1de1fab 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1521,6 +1521,8 @@
#define PCI_DEVICE_ID_ARTOP_ATP860R 0x0007
#define PCI_DEVICE_ID_ARTOP_ATP865 0x0008
#define PCI_DEVICE_ID_ARTOP_ATP865R 0x0009
+#define PCI_DEVICE_ID_ARTOP_ATP867A 0x000A
+#define PCI_DEVICE_ID_ARTOP_ATP867B 0x000B
#define PCI_DEVICE_ID_ARTOP_AEC7610 0x8002
#define PCI_DEVICE_ID_ARTOP_AEC7612UW 0x8010
#define PCI_DEVICE_ID_ARTOP_AEC7612U 0x8020

2009-09-17 20:51:42

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

On 09/15/2009 12:32 AM, Jung-Ik (John) Lee wrote:
> From: John(Jung-Ik) Lee<[email protected]>
>
> This is a new pata driver for ARTOP 867X 64bit 4-channel UDMA133 ATA ctrls.
> Based on the Atp867 data sheet rev 1.2, Acard, and in part on early ide codes
> from Eric Uhrhane<[email protected]>.
>
> Signed-off-by: John(Jung-Ik) Lee<[email protected]>
> Reviewed-by: Grant Grundler<[email protected]>
> Reviewed-by: Gwendal Gringo<[email protected]>
> ---
>
> rev 01: Cleared some checkpatch.pl complaints. Thanks to Daniel.
> rev 02: Removed hard-coded value for active/recovery. Thanks to Robert.
> Use atp867x_cable_detect() until ata_cable_40short()
> rev 03: changes per Jeff's comment: Thansk to Jeff.
> ioread/iowrite, enum, speed-to-mode calc, single mapping, etc.
>
> drivers/ata/Kconfig | 9 +
> drivers/ata/Makefile | 1 +
> drivers/ata/pata_atp867x.c | 548 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci_ids.h | 2 +
> 4 files changed, 560 insertions(+), 0 deletions(-)

applied