Re: [git patches] libata updates

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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@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 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@gmail.com>
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@gmail.com>
---
 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,
 	};