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

From: Jeff Garzik
Date: Sat Sep 12 2009 - 20:56:07 EST

Next message: Irish News Centre: "Info"
Previous message: Jung-Ik (John) Lee: "Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers"
In reply to: Jung-Ik (John) Lee: "Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers"
Next in thread: Jung-Ik (John) Lee: "Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Irish News Centre: "Info"
Previous message: Jung-Ik (John) Lee: "Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers"
In reply to: Jung-Ik (John) Lee: "Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers"
Next in thread: Jung-Ik (John) Lee: "Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]