On Thursday 17 September 2009 22:49:35 Jeff Garzik wrote: > > Bug fixes, and a new driver. > > > > Please pull from 'upstream-linus' branch of > master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/libata-dev.git upstream-linus > > to receive the following updates: > > drivers/ata/Kconfig | 9 + > drivers/ata/Makefile | 1 + > drivers/ata/ahci.c | 4 +- > drivers/ata/libata-core.c | 4 +- > drivers/ata/pata_amd.c | 3 + > drivers/ata/pata_atp867x.c | 548 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/ata/sata_promise.c | 155 +++++++++++-- > include/linux/pci_ids.h | 2 + > 8 files changed, 704 insertions(+), 22 deletions(-) > create mode 100644 drivers/ata/pata_atp867x.c > > John(Jung-Ik) Lee (1): > libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers That was really fast.. Not necessarily a bad thing but this driver would benefit from few polishing touches.. > 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 <jilee@xxxxxxxxxx> > + * > + * 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); The driver currently doesn't support MWDMA modes but claims otherwise (fixed in the attached patch). > + /* > + * 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; Shouldn't "clocks = 0" (the default case) be used here? Otherwise it seems to result in underclocked timings for dp->pci66mhz == 0. > + 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; Is "clocks == 14" a reserved setting? If so a comment documenting it would be appreciated. > + case 15: > + break; > + default: > + printk(KERN_WARNING "ATP867X: recover %dclk is invalid. " > + "Using default 15clk.\n", clk); > + clocks = 0; /* 12 clk */ Shouldn't it use "clocks == 15" setting? > + 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; What is the purpose of the above hack? AFAICS (I don't have a datasheet) it may result in invalid active clocks being used for t.active > 12 and 0x80 bit being set incorrectly for t.active values 7..12 (unless it was the purpose of the hack). > + 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); This is incorrect if slave/master devices use different PIO modes or if PIO mode <= 2 is used by any device. Timing based on t.act8b and t.rec8b values should be used instead. On the somehow related note: * I don't see how PIO0-2 command timings can be met with only 3 bits used for active clocks. Could it be that dp->eight_piospd should be programmed in a slightly different way than dp->{mstr,slave}_piospd? * The controller allows higher clocks values for recovery timings but ata_timing_compute() tries to fairly increase both recovery and active timings to meet the required cycle timing. > +} > + > +static int atp867x_cable_detect(struct ata_port *ap) > +{ > + return ATA_CBL_PATA40_SHORT; > +} As noticed by Robert and Alan already: This should use ATA_CBL_PATA_UNK and rely on the driver-side cable detection. One last thing: Power Management support is missing from this driver (I tried addressing this in the separately posted patch but it needs testing by somebody with the hardware). MWDMA fix: From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> Subject: [PATCH] pata_atp867x: fix it to not claim MWDMA support MWDMA modes are not supported by this driver currently. Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> --- drivers/ata/pata_atp867x.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) Index: b/drivers/ata/pata_atp867x.c =================================================================== --- a/drivers/ata/pata_atp867x.c +++ b/drivers/ata/pata_atp867x.c @@ -118,20 +118,13 @@ struct atp867x_priv { 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); + u8 mode = speed - XFER_UDMA_0 + 1; /* * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed @@ -471,7 +464,6 @@ static int atp867x_init_one(struct pci_d 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, }; -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html