| Message ID | 8b5805ff0909121726g2f1a78c1o1ff7289e059eabe5@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote: > From: John(Jung-Ik) Lee<jilee@google.com> > > 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<ericu@google.com>. > > Signed-off-by: John(Jung-Ik) Lee<jilee@google.com> > Reviewed-by: Grant Grundler<grundler@google.com> > Reviewed-by: Gwendal Gringo<gwendal@google.com> > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 12, 2009 at 5:31 PM, Jeff Garzik <jgarzik@pobox.com> wrote: > On 09/12/2009 08:26 PM, Jung-Ik (John) Lee wrote: >> >> From: John(Jung-Ik) Lee<jilee@google.com> >> >> 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<ericu@google.com>. >> >> Signed-off-by: John(Jung-Ik) Lee<jilee@google.com> >> Reviewed-by: Grant Grundler<grundler@google.com> >> Reviewed-by: Gwendal Gringo<gwendal@google.com> >> --- >> >> 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 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik <jgarzik@pobox.com> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/12/2009 10:41 PM, Jung-Ik (John) Lee wrote: > On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik<jgarzik@pobox.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 12, 2009 at 8:12 PM, Jeff Garzik <jgarzik@pobox.com> wrote: > On 09/12/2009 10:41 PM, Jung-Ik (John) Lee wrote: >> >> On Sat, Sep 12, 2009 at 5:55 PM, Jeff Garzik<jgarzik@pobox.com> 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 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 12, 2009 at 10:55 PM, Jung-Ik (John) Lee <jilee@google.com> 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.. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <jilee@google.com> + * + * 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