Pending experimental bits. Not necessarily ready to apply but so folks know
what is going on.
---
Alan Cox (5):
pata_piccolo: Driver for old Toshiba chipsets
pata: Update experimental tags
cmd64x: implement serialization as per notes
pata_sis: Implement MWDMA for the UDMA 133 capable chips
pata_via: Blacklist some combinations of Transcend Flash and via
drivers/ata/Kconfig | 33 +++++++---
drivers/ata/Makefile | 1
drivers/ata/ata_generic.c | 5 +-
drivers/ata/pata_cmd64x.c | 132 +++++++++++++++++++++++++++++++++++++++--
drivers/ata/pata_piccolo.c | 140 ++++++++++++++++++++++++++++++++++++++++++++
drivers/ata/pata_sis.c | 88 +++++++++++++++++++++-------
drivers/ata/pata_via.c | 27 ++++++++
include/linux/pci_ids.h | 7 +-
8 files changed, 388 insertions(+), 45 deletions(-)
create mode 100644 drivers/ata/pata_piccolo.c
--
My Git tree is full of regressions, my git tree is full of bad C
My Git tree is full of regressions oh git-clone my codebase and see
Git-clone git-clone, oh git-clone my codebase and see, and see
Git-clone git-clone, oh git-clone my codebase and see
Reported by Mikulas Patocka.
VIA VT82C586B + Transcend TS64GSSD25-M v0826 does not work in UDMA mode
Signed-off-by: Alan Cox <[email protected]>
---
drivers/ata/pata_via.c | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index 88984b8..2fb8be2 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -337,6 +337,32 @@ static void via_set_dmamode(struct ata_port *ap, struct ata_device *adev)
}
/**
+ * via_mode_filter - filter buggy device/mode pairs
+ * @dev: ATA device
+ * @mask: Mode bitmask
+ *
+ * We need to apply some minimal filtering for old controllers and at least
+ * one breed of Transcend SSD. Return the updated mask.
+ */
+
+static unsigned long via_mode_filter(struct ata_device *dev, unsigned long mask)
+{
+ struct ata_host *host = dev->link->ap->host;
+ const struct via_isa_bridge *config = host->private_data;
+ unsigned char model_num[ATA_ID_PROD_LEN + 1];
+
+ if (config->id == PCI_DEVICE_ID_VIA_82C586_0) {
+ ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
+ if (strcmp(model_num, "TS64GSSD25-M") == 0) {
+ ata_dev_printk(dev, KERN_WARNING,
+ "disabling UDMA mode due to reported lockups with this device.\n");
+ mask &= ~ ATA_MASK_UDMA;
+ }
+ }
+ return ata_bmdma_mode_filter(dev, mask);
+}
+
+/**
* via_tf_load - send taskfile registers to host controller
* @ap: Port to which output is sent
* @tf: ATA taskfile register set
@@ -427,6 +453,7 @@ static struct ata_port_operations via_port_ops = {
.prereset = via_pre_reset,
.sff_tf_load = via_tf_load,
.port_start = via_port_start,
+ .mode_filter = via_mode_filter,
};
static struct ata_port_operations via_port_ops_noirq = {
Bartlomiej pointed out that while this got fixed in the old driver whoever
did it didn't port it across.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/ata/pata_sis.c | 91 ++++++++++++++++++++++++++++++++++++------------
1 files changed, 69 insertions(+), 22 deletions(-)
diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c
index 488e77b..d70ecec 100644
--- a/drivers/ata/pata_sis.c
+++ b/drivers/ata/pata_sis.c
@@ -252,24 +252,25 @@ static void sis_100_set_piomode (struct ata_port *ap, struct ata_device *adev)
}
/**
- * sis_133_set_piomode - Initialize host controller PATA PIO timings
+ * sis_133_do_piomode - Initialize host controller PATA PIO/DMA timings
* @ap: Port whose timings we are configuring
* @adev: Device we are configuring for.
*
* Set PIO mode for device, in host controller PCI config space. This
- * function handles PIO set up for the later ATA133 devices.
+ * function handles PIO set up for the later ATA133 devices. The same
+ * timings are used for MWDMA.
*
* LOCKING:
* None (inherited from caller).
*/
-static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
+static void sis_133_do_piomode(struct ata_port *ap, struct ata_device *adev,
+ int speed)
{
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
int port = 0x40;
u32 t1;
u32 reg54;
- int speed = adev->pio_mode - XFER_PIO_0;
const u32 timing133[] = {
0x28269000, /* Recovery << 24 | Act << 16 | Ini << 12 */
@@ -305,6 +306,42 @@ static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
}
/**
+ * sis_133_set_piomode - Initialize host controller PATA PIO timings
+ * @ap: Port whose timings we are configuring
+ * @adev: Device we are configuring for.
+ *
+ * Set PIO mode for device, in host controller PCI config space. This
+ * function handles PIO set up for the later ATA133 devices.
+ *
+ * LOCKING:
+ * None (inherited from caller).
+ */
+
+static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev)
+{
+
+ sis_133_do_piomode(ap, adev, adev->pio_mode - XFER_PIO_0);
+}
+
+/**
+ * mwdma_clip_to_pio - clip MWDMA mode
+ * @adev: device
+ *
+ * As the SiS shared MWDMA and PIO timings we must program the equivalent
+ * PIO timing for the MWDMA mode but we must not program one higher than
+ * the permitted PIO timing of the device.
+ */
+
+static int mwdma_clip_to_pio(struct ata_device *adev)
+{
+ const int mwdma_to_pio[3] = {
+ XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
+ };
+ return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
+ adev->pio_mode - XFER_PIO_0);
+}
+
+/**
* sis_old_set_dmamode - Initialize host controller PATA DMA timings
* @ap: Port whose timings we are configuring
* @adev: Device to program
@@ -332,6 +369,7 @@ static void sis_old_set_dmamode (struct ata_port *ap, struct ata_device *adev)
if (adev->dma_mode < XFER_UDMA_0) {
/* bits 3-0 hold recovery timing bits 8-10 active timing and
the higher bits are dependant on the device */
+ speed = mwdma_clip_to_pio(adev);
timing &= ~0x870F;
timing |= mwdma_bits[speed];
} else {
@@ -372,6 +410,7 @@ static void sis_66_set_dmamode (struct ata_port *ap, struct ata_device *adev)
if (adev->dma_mode < XFER_UDMA_0) {
/* bits 3-0 hold recovery timing bits 8-10 active timing and
the higher bits are dependant on the device, bit 15 udma */
+ speed = mwdma_clip_to_pio(adev);
timing &= ~0x870F;
timing |= mwdma_bits[speed];
} else {
@@ -389,7 +428,7 @@ static void sis_66_set_dmamode (struct ata_port *ap, struct ata_device *adev)
* @adev: Device to program
*
* Set UDMA/MWDMA mode for device, in host controller PCI config space.
- * Handles UDMA66 and early UDMA100 devices.
+ * Handles later UDMA100 devices.
*
* LOCKING:
* None (inherited from caller).
@@ -400,21 +439,25 @@ static void sis_100_set_dmamode (struct ata_port *ap, struct ata_device *adev)
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
int speed = adev->dma_mode - XFER_MW_DMA_0;
int drive_pci = sis_old_port_base(adev);
- u8 timing;
+ u16 timing;
- const u8 udma_bits[] = { 0x8B, 0x87, 0x85, 0x83, 0x82, 0x81};
+ const u16 udma_bits[] = {
+ 0x8B00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100};
+ const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 };
- pci_read_config_byte(pdev, drive_pci + 1, &timing);
+ pci_read_config_word(pdev, drive_pci, &timing);
if (adev->dma_mode < XFER_UDMA_0) {
- /* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */
+ speed = mwdma_clip_to_pio(adev);
+ timing &= ~0x80FF;
+ timing |= mwdma_bits[speed];
} else {
/* Bit 7 is UDMA on/off, bit 0-3 are cycle time */
speed = adev->dma_mode - XFER_UDMA_0;
- timing &= ~0x8F;
+ timing &= ~0x8F00;
timing |= udma_bits[speed];
}
- pci_write_config_byte(pdev, drive_pci + 1, timing);
+ pci_write_config_word(pdev, drive_pci, timing);
}
/**
@@ -434,21 +477,26 @@ static void sis_133_early_set_dmamode (struct ata_port *ap, struct ata_device *a
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
int speed = adev->dma_mode - XFER_MW_DMA_0;
int drive_pci = sis_old_port_base(adev);
- u8 timing;
- /* Low 4 bits are timing */
- static const u8 udma_bits[] = { 0x8F, 0x8A, 0x87, 0x85, 0x83, 0x82, 0x81};
+ u16 timing;
+ /* Bits 15-12 are timing */
+ static const u16 udma_bits[] = {
+ 0x8F00, 0x8A00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100
+ };
+ static const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 };
- pci_read_config_byte(pdev, drive_pci + 1, &timing);
+ pci_read_config_word(pdev, drive_pci, &timing);
if (adev->dma_mode < XFER_UDMA_0) {
- /* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */
+ speed = mwdma_clip_to_pio(adev);
+ timing &= ~0x80FF;
+ timing = mwdma_bits[speed];
} else {
/* Bit 7 is UDMA on/off, bit 0-3 are cycle time */
speed = adev->dma_mode - XFER_UDMA_0;
- timing &= ~0x8F;
+ timing &= ~0x8F00;
timing |= udma_bits[speed];
}
- pci_write_config_byte(pdev, drive_pci + 1, timing);
+ pci_write_config_word(pdev, drive_pci, timing);
}
/**
@@ -479,13 +527,12 @@ static void sis_133_set_dmamode (struct ata_port *ap, struct ata_device *adev)
if (reg54 & 0x40000000)
port = 0x70;
port += (8 * ap->port_no) + (4 * adev->devno);
-
pci_read_config_dword(pdev, port, &t1);
if (adev->dma_mode < XFER_UDMA_0) {
- t1 &= ~0x00000004;
- /* FIXME: need data sheet to add MWDMA here. Also lacking on
- ide/pci driver */
+ speed = mwdma_clip_to_pio(adev);
+ sis_133_do_piomode(ap, adev, speed);
+ t1 &= ~4; /* UDMA off */
} else {
speed = adev->dma_mode - XFER_UDMA_0;
/* if & 8 no UDMA133 - need info for ... */
(Second attempt)
Daniela Engert pointed out that there are some implementation notes for the
643 and 646 that deal with certain serialization rules. In theory we don't
need them because they apply when the motherboard decides not to retry PCI
requests for long enough and the chip is busy doing a DMA transfer on the
other channel.
The rule basically is "don't touch the taskfile of the other channel while
a DMA is in progress". To implement that we need to
- not issue a command on a channel when there is a DMA command queued
- not issue a DMA command on a channel when there are PIO commands queued
- use the alternative access to the interrupt source so that we do not
touch altstatus or status on shared IRQ.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/ata/pata_cmd64x.c | 139 ++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 131 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
index f98dffe..48948ae 100644
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -31,7 +31,7 @@
#include <linux/libata.h>
#define DRV_NAME "pata_cmd64x"
-#define DRV_VERSION "0.2.5"
+#define DRV_VERSION "0.3.1"
/*
* CMD64x specific registers definition.
@@ -75,6 +75,13 @@ enum {
DTPR1 = 0x7C
};
+struct cmd_priv
+{
+ int dma_live; /* Channel using DMA */
+ int irq_t[2]; /* Register to check for IRQ */
+ int irq_m[2]; /* Bit to check */
+};
+
static int cmd648_cable_detect(struct ata_port *ap)
{
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
@@ -254,17 +261,114 @@ static void cmd648_bmdma_stop(struct ata_queued_cmd *qc)
}
/**
- * cmd646r1_dma_stop - DMA stop callback
+ * cmd64x_bmdma_stop - DMA stop callback
* @qc: Command in progress
*
- * Stub for now while investigating the r1 quirk in the old driver.
+ * Track the completion of live DMA commands and clear the dma_live
+ * tracking flag as we do.
*/
-static void cmd646r1_bmdma_stop(struct ata_queued_cmd *qc)
+static void cmd64x_bmdma_stop(struct ata_queued_cmd *qc)
{
+ struct ata_port *ap = qc->ap;
+ struct cmd_priv *priv = ap->host->private_data;
ata_bmdma_stop(qc);
+ WARN_ON(priv->dma_live != ap->port_no );
+ priv->dma_live = -1;
}
+/**
+ * cmd64x_qc_defer - Defer logic for chip limits
+ * @qc: queued command
+ *
+ * Decide whether we can issue the command. Called under the host lock.
+ */
+
+static int cmd64x_qc_defer(struct ata_queued_cmd *qc)
+{
+ struct ata_host *host = qc->ap->host;
+ struct cmd_priv *priv = host->private_data;
+ struct ata_port *alt = host->ports[1 ^ qc->ap->port_no];
+ int rc;
+ int dma = 0;
+
+ /* Apply the ATA rules first */
+ rc = ata_std_qc_defer(qc);
+ if (rc)
+ return rc;
+
+ if (qc->tf.protocol == ATAPI_PROT_DMA ||
+ qc->tf.protocol == ATA_PROT_DMA)
+ dma = 1;
+
+ /* If the other port is not live then issue the command */
+ if (alt == NULL || !alt->qc_active) {
+ if (dma)
+ priv->dma_live = qc->ap->port_no;
+ return 0;
+ }
+ /* If there is a live DMA command then wait */
+ if (priv->dma_live != -1)
+ return ATA_DEFER_PORT;
+ if (dma) {
+ /* Cannot overlap our DMA command */
+ if (alt->qc_active)
+ return ATA_DEFER_PORT;
+ /* Claim the DMA */
+ priv->dma_live = qc->ap->port_no;
+ }
+ return 0;
+}
+
+/**
+ * cmd64x_interrupt - ATA host interrupt handler
+ * @irq: irq line (unused)
+ * @dev_instance: pointer to our ata_host information structure
+ *
+ * Our interrupt handler for PCI IDE devices. Calls
+ * ata_sff_host_intr() for each port that is flagging an IRQ. We cannot
+ * use the defaults as we need to avoid touching status/altstatus during
+ * a DMA.
+ *
+ * LOCKING:
+ * Obtains host lock during operation.
+ *
+ * RETURNS:
+ * IRQ_NONE or IRQ_HANDLED.
+ */
+irqreturn_t cmd64x_interrupt(int irq, void *dev_instance)
+{
+ struct ata_host *host = dev_instance;
+ struct pci_dev *pdev = to_pci_dev(host->dev);
+ struct cmd_priv *priv = host->private_data;
+ unsigned int i;
+ unsigned int handled = 0;
+ unsigned long flags;
+
+ /* TODO: make _irqsave conditional on x86 PCI IDE legacy mode */
+ spin_lock_irqsave(&host->lock, flags);
+
+ for (i = 0; i < host->n_ports; i++) {
+ struct ata_port *ap;
+ u8 reg;
+
+ pci_read_config_byte(pdev, priv->irq_t[i], ®);
+ ap = host->ports[i];
+ if (ap && (reg & priv->irq_m[i]) &&
+ !(ap->flags & ATA_FLAG_DISABLED)) {
+ struct ata_queued_cmd *qc;
+
+ qc = ata_qc_from_tag(ap, ap->link.active_tag);
+ if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
+ (qc->flags & ATA_QCFLAG_ACTIVE))
+ handled |= ata_sff_host_intr(ap, qc);
+ }
+ }
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return IRQ_RETVAL(handled);
+}
static struct scsi_host_template cmd64x_sht = {
ATA_BMDMA_SHT(DRV_NAME),
};
@@ -273,6 +377,8 @@ static const struct ata_port_operations cmd64x_base_ops = {
.inherits = &ata_bmdma_port_ops,
.set_piomode = cmd64x_set_piomode,
.set_dmamode = cmd64x_set_dmamode,
+ .bmdma_stop = cmd64x_bmdma_stop,
+ .qc_defer = cmd64x_qc_defer,
};
static struct ata_port_operations cmd64x_port_ops = {
@@ -282,7 +388,6 @@ static struct ata_port_operations cmd64x_port_ops = {
static struct ata_port_operations cmd646r1_port_ops = {
.inherits = &cmd64x_base_ops,
- .bmdma_stop = cmd646r1_bmdma_stop,
.cable_detect = ata_cable_40wire,
};
@@ -290,6 +395,7 @@ static struct ata_port_operations cmd648_port_ops = {
.inherits = &cmd64x_base_ops,
.bmdma_stop = cmd648_bmdma_stop,
.cable_detect = cmd648_cable_detect,
+ .qc_defer = ata_std_qc_defer
};
static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -340,6 +446,8 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
u8 mrdmode;
int rc;
+ struct ata_host *host;
+ struct cmd_priv *cpriv;
rc = pcim_enable_device(pdev);
if (rc)
@@ -348,6 +456,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
pci_read_config_dword(pdev, PCI_CLASS_REVISION, &class_rev);
class_rev &= 0xFF;
+ cpriv = devm_kzalloc(&pdev->dev, sizeof(*cpriv), GFP_KERNEL);
+ if (cpriv == NULL)
+ return -ENOMEM;
+ cpriv->dma_live = -1;
+
+ /* Table for IRQ checking */
+ cpriv->irq_t[0] = CFR;
+ cpriv->irq_m[0] = 1 << 2;
+ cpriv->irq_t[1] = ARTTIM23;
+ cpriv->irq_m[1] = 1 << 4;
+
if (id->driver_data == 0) /* 643 */
ata_pci_bmdma_clear_simplex(pdev);
@@ -360,20 +479,24 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
ppi[0] = &cmd_info[3];
}
+
pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
pci_read_config_byte(pdev, MRDMODE, &mrdmode);
mrdmode &= ~ 0x30; /* IRQ set up */
mrdmode |= 0x02; /* Memory read line enable */
pci_write_config_byte(pdev, MRDMODE, mrdmode);
- /* Force PIO 0 here.. */
-
/* PPC specific fixup copied from old driver */
#ifdef CONFIG_PPC
pci_write_config_byte(pdev, UDIDETCR0, 0xF0);
#endif
+ rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
+ if (rc)
+ return rc;
+ host->private_data = cpriv;
- return ata_pci_sff_init_one(pdev, ppi, &cmd64x_sht, NULL);
+ pci_set_master(pdev);
+ return ata_pci_sff_activate_host(host, cmd64x_interrupt, &cmd64x_sht);
}
#ifdef CONFIG_PM
Signed-off-by: Alan Cox <[email protected]>
---
drivers/ata/Kconfig | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index f2df6e2..36931e0 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -374,7 +374,7 @@ config PATA_HPT366
If unsure, say N.
config PATA_HPT37X
- tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
+ tristate "HPT 370/370A/371/372/374/302 PATA support"
depends on PCI && EXPERIMENTAL
help
This option enables support for the majority of the later HPT
@@ -383,7 +383,7 @@ config PATA_HPT37X
If unsure, say N.
config PATA_HPT3X2N
- tristate "HPT 372N/302N PATA support (Experimental)"
+ tristate "HPT 372N/302N PATA support"
depends on PCI && EXPERIMENTAL
help
This option enables support for the N variant HPT PATA
@@ -401,7 +401,7 @@ config PATA_HPT3X3
If unsure, say N.
config PATA_HPT3X3_DMA
- bool "HPT 343/363 DMA support (Experimental)"
+ bool "HPT 343/363 DMA support"
depends on PATA_HPT3X3
help
This option enables DMA support for the HPT343/363
@@ -510,7 +510,7 @@ config PATA_NETCELL
If unsure, say N.
config PATA_NINJA32
- tristate "Ninja32/Delkin Cardbus ATA support (Experimental)"
+ tristate "Ninja32/Delkin Cardbus ATA support"
depends on PCI && EXPERIMENTAL
help
This option enables support for the Ninja32, Delkin and
We were never able to get docs for this out of Toshiba for years. Dave
Barnes produced a NetBSD driver however and from that we can fill in the
needed tables
Signed-off-by: Alan Cox <[email protected]>
---
drivers/ata/Kconfig | 25 +++++---
drivers/ata/Makefile | 1
drivers/ata/ata_generic.c | 5 +-
drivers/ata/pata_piccolo.c | 140 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci_ids.h | 7 +-
5 files changed, 166 insertions(+), 12 deletions(-)
create mode 100644 drivers/ata/pata_piccolo.c
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 36931e0..acca6b6 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -667,6 +667,15 @@ config PATA_SIS
If unsure, say N.
+config PATA_TOSHIBA
+ tristate "Toshiba Piccolo support (Experimental)"
+ depends on PCI && EXPERIMENTAL
+ help
+ Support for the Toshiba Piccolo controllers. Currently only the
+ primary channel is supported by this driver.
+
+ If unsure, say N.
+
config PATA_VIA
tristate "VIA PATA support"
depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 01e126f..d909435 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
+obj-$(CONFIG_PATA_TOSHIBA) += pata_piccolo.o
obj-$(CONFIG_PATA_VIA) += pata_via.o
obj-$(CONFIG_PATA_WINBOND) += pata_sl82c105.o
obj-$(CONFIG_PATA_WINBOND_VLB) += pata_winbond.o
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index ecfd22b..5daf507 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -168,9 +168,12 @@ static struct pci_device_id ata_generic[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C561), },
{ PCI_DEVICE(PCI_VENDOR_ID_OPTI, PCI_DEVICE_ID_OPTI_82C558), },
{ PCI_DEVICE(PCI_VENDOR_ID_CENATEK,PCI_DEVICE_ID_CENATEK_IDE), },
+#if !defined(CONFIG_PATA_TOSHIBA) && !defined(CONFIG_PATA_TOSHIBA_MODULE)
{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO), },
- { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), },
{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), },
+ { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_3), },
+ { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_5), },
+#endif
/* Must come last. If you add entries adjust this table appropriately */
{ PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL, 1},
{ 0, },
diff --git a/drivers/ata/pata_piccolo.c b/drivers/ata/pata_piccolo.c
new file mode 100644
index 0000000..1157da8
--- /dev/null
+++ b/drivers/ata/pata_piccolo.c
@@ -0,0 +1,140 @@
+/*
+ * pata_piccolo.c - Toshiba Piccolo PATA/SATA controller driver.
+ *
+ * This is basically an update to ata_generic.c to add Toshiba Piccolo support
+ * then split out to keep ata_generic "clean".
+ *
+ * Copyright 2005 Red Hat Inc, all rights reserved.
+ *
+ * Elements from ide/pci/generic.c
+ * Copyright (C) 2001-2002 Andre Hedrick <[email protected]>
+ * Portions (C) Copyright 2002 Red Hat Inc <[email protected]>
+ *
+ * May be copied or modified under the terms of the GNU General Public License
+ *
+ * The timing data tables/programming info are courtesy of the NetBSD driver
+ */
+
+#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 <scsi/scsi_host.h>
+#include <linux/libata.h>
+
+#define DRV_NAME "pata_piccolo"
+#define DRV_VERSION "0.0.1"
+
+
+
+static void tosh_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ static const u16 pio[6] = { /* For reg 0x50 low word & E088 */
+ 0x0566, 0x0433, 0x0311, 0x0201, 0x0200, 0x0100
+ };
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ u16 conf;
+ pci_read_config_word(pdev, 0x50, &conf);
+ conf &= 0xE088;
+ conf |= pio[adev->pio_mode - XFER_PIO_0];
+ pci_write_config_word(pdev, 0x50, conf);
+}
+
+static void tosh_set_dmamode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ u32 conf;
+ pci_read_config_dword(pdev, 0x5C, &conf);
+ conf &= 0x78FFE088; /* Keep the other bits */
+ if (adev->dma_mode >= XFER_UDMA_0) {
+ int udma = adev->dma_mode - XFER_UDMA_0;
+ conf |= 0x80000000;
+ conf |= (udma + 2) << 28;
+ conf |= (2 - udma) * 0x111; /* spread into three nibbles */
+ } else {
+ static const u32 mwdma[4] = {
+ 0x0655, 0x0200, 0x0200, 0x0100
+ };
+ conf |= mwdma[adev->dma_mode - XFER_MW_DMA_0];
+ }
+ pci_write_config_dword(pdev, 0x5C, conf);
+}
+
+
+static struct scsi_host_template tosh_sht = {
+ ATA_BMDMA_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations tosh_port_ops = {
+ .inherits = &ata_bmdma_port_ops,
+ .cable_detect = ata_cable_unknown,
+ .set_piomode = tosh_set_piomode,
+ .set_dmamode = tosh_set_dmamode
+};
+
+/**
+ * ata_tosh_init - attach generic IDE
+ * @dev: PCI device found
+ * @id: match entry
+ *
+ * Called each time a matching IDE interface is found. We check if the
+ * interface is one we wish to claim and if so we perform any chip
+ * specific hacks then let the ATA layer do the heavy lifting.
+ */
+
+static int ata_tosh_init_one(struct pci_dev *dev, const struct pci_device_id *id)
+{
+ static const struct ata_port_info info = {
+ .flags = ATA_FLAG_SLAVE_POSS,
+ .pio_mask = ATA_PIO5,
+ .mwdma_mask = ATA_MWDMA2,
+ .udma_mask = ATA_UDMA2,
+ .port_ops = &tosh_port_ops
+ };
+ const struct ata_port_info *ppi[] = { &info, &ata_dummy_port_info };
+ /* Just one port for the moment */
+ return ata_pci_sff_init_one(dev, ppi, &tosh_sht, NULL);
+}
+
+static struct pci_device_id ata_tosh[] = {
+ { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO), },
+ { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), },
+ { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_3), },
+ { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_5), },
+ { 0, },
+};
+
+static struct pci_driver ata_tosh_pci_driver = {
+ .name = DRV_NAME,
+ .id_table = ata_tosh,
+ .probe = ata_tosh_init_one,
+ .remove = ata_pci_remove_one,
+#ifdef CONFIG_PM
+ .suspend = ata_pci_device_suspend,
+ .resume = ata_pci_device_resume,
+#endif
+};
+
+static int __init ata_tosh_init(void)
+{
+ return pci_register_driver(&ata_tosh_pci_driver);
+}
+
+
+static void __exit ata_tosh_exit(void)
+{
+ pci_unregister_driver(&ata_tosh_pci_driver);
+}
+
+
+MODULE_AUTHOR("Alan Cox");
+MODULE_DESCRIPTION("Low level driver for Toshiba Piccolo ATA");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(pci, ata_tosh);
+MODULE_VERSION(DRV_VERSION);
+
+module_init(ata_tosh_init);
+module_exit(ata_tosh_exit);
+
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index d201220..bc449e5 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1496,9 +1496,10 @@
#define PCI_DEVICE_ID_SBE_WANXL400 0x0104
#define PCI_VENDOR_ID_TOSHIBA 0x1179
-#define PCI_DEVICE_ID_TOSHIBA_PICCOLO 0x0102
-#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_1 0x0103
-#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_2 0x0105
+#define PCI_DEVICE_ID_TOSHIBA_PICCOLO 0x0101
+#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_2 0x0102
+#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_3 0x0103
+#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_5 0x0105
#define PCI_DEVICE_ID_TOSHIBA_TOPIC95 0x060a
#define PCI_DEVICE_ID_TOSHIBA_TOPIC97 0x060f
#define PCI_DEVICE_ID_TOSHIBA_TOPIC100 0x0617
On Tuesday 17 November 2009 15:51:17 Alan Cox wrote:
> Bartlomiej pointed out that while this got fixed in the old driver whoever
> did it didn't port it across.
Please add ',' before 'whoever' and change 'the old driver' to 'sis5513'
so it is at least passable as 'information manipulation' on what I really
said instead of just being a 'straight lie'.. Thanks!
> +/**
> + * mwdma_clip_to_pio - clip MWDMA mode
> + * @adev: device
> + *
> + * As the SiS shared MWDMA and PIO timings we must program the equivalent
> + * PIO timing for the MWDMA mode but we must not program one higher than
> + * the permitted PIO timing of the device.
> + */
> +
> +static int mwdma_clip_to_pio(struct ata_device *adev)
> +{
> + const int mwdma_to_pio[3] = {
> + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> + };
> + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> + adev->pio_mode - XFER_PIO_0);
> +}
This wants to be in the generic libata code.
--
Bartlomiej Zolnierkiewicz
On Tuesday 17 November 2009 15:51:32 Alan Cox wrote:
> +struct cmd_priv
> +{
> + int dma_live; /* Channel using DMA */
> + int irq_t[2]; /* Register to check for IRQ */
> + int irq_m[2]; /* Bit to check */
> +};
irq_t and irq_m content will be identical for all host instances
so you may as well add one instance for it, use ->private_data to
store dma_live information and remove cmd_priv allocation
> + /* If the other port is not live then issue the command */
> + if (alt == NULL || !alt->qc_active) {
> + if (dma)
> + priv->dma_live = qc->ap->port_no;
> + return 0;
> + }
> + /* If there is a live DMA command then wait */
> + if (priv->dma_live != -1)
> + return ATA_DEFER_PORT;
> + if (dma) {
> + /* Cannot overlap our DMA command */
> + if (alt->qc_active)
> + return ATA_DEFER_PORT;
no need to check alt->qc_active again here
--
Bartlomiej Zolnierkiewicz
> > +static int mwdma_clip_to_pio(struct ata_device *adev)
> > +{
> > + const int mwdma_to_pio[3] = {
> > + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> > + };
> > + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> > + adev->pio_mode - XFER_PIO_0);
> > +}
>
> This wants to be in the generic libata code.
I'm not convinced because for the majority of drivers the libata timing
interface handles it. SiS needs it just because it does things by
precomputed tables. It's a one off interface.
Alan
On Tuesday 17 November 2009 18:38:23 Alan Cox wrote:
> > > +static int mwdma_clip_to_pio(struct ata_device *adev)
> > > +{
> > > + const int mwdma_to_pio[3] = {
> > > + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> > > + };
> > > + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0],
> > > + adev->pio_mode - XFER_PIO_0);
> > > +}
> >
> > This wants to be in the generic libata code.
>
> I'm not convinced because for the majority of drivers the libata timing
> interface handles it. SiS needs it just because it does things by
> precomputed tables. It's a one off interface.
Controllers based on *Intel* PIIX are in the disagreement with the above
paragraph and having generic helper to do conversion (without a clipping)
would bring us a little step closer to killing a needless code duplication
currently present in their ->set_piomode and ->set_dmamode methods.
[ In case somebody wonders: no, 'old' drivers don't have such duplication
and 'whoever did it didn't port it across' cause said 'whoever'-s were not
into the development of the square wheels.... ]
--
Bartlomiej Zolnierkiewicz
On Tue, 17 Nov 2009 18:35:05 +0100
Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> On Tuesday 17 November 2009 15:51:32 Alan Cox wrote:
>
> > +struct cmd_priv
> > +{
> > + int dma_live; /* Channel using DMA */
> > + int irq_t[2]; /* Register to check for IRQ */
> > + int irq_m[2]; /* Bit to check */
> > +};
>
> irq_t and irq_m content will be identical for all host instances
Once I've had a look at the later chip variants I'll indeed do that
providing the 648 is ok in all revs.
> so you may as well add one instance for it, use ->private_data to
> store dma_live information and remove cmd_priv allocation
>
> > + /* If the other port is not live then issue the command */
> > + if (alt == NULL || !alt->qc_active) {
> > + if (dma)
> > + priv->dma_live = qc->ap->port_no;
> > + return 0;
> > + }
> > + /* If there is a live DMA command then wait */
> > + if (priv->dma_live != -1)
> > + return ATA_DEFER_PORT;
> > + if (dma) {
> > + /* Cannot overlap our DMA command */
> > + if (alt->qc_active)
> > + return ATA_DEFER_PORT;
>
> no need to check alt->qc_active again here
Good point
Thanks for the review.
> > I'm not convinced because for the majority of drivers the libata timing
> > interface handles it. SiS needs it just because it does things by
> > precomputed tables. It's a one off interface.
>
> Controllers based on *Intel* PIIX are in the disagreement with the above
No the PIIX is quite different. You use the matching PIO timing (which is
a short lookup and shorter code than even a helper function call). You do
not use the clipping instead you set bit 3 to ensure that PIO cycles
occur at low speed but MWDMA runs at the right speed. That is usually a
win over picking a lower mode as the PIIX can do ATAPI DMA happily.
So the only thing you can "share" is what would be a 4 byte table.
> paragraph and having generic helper to do conversion (without a clipping)
> would bring us a little step closer to killing a needless code duplication
> currently present in their ->set_piomode and ->set_dmamode methods.
>
> [ In case somebody wonders: no, 'old' drivers don't have such duplication
> and 'whoever did it didn't port it across' cause said 'whoever'-s were not
> into the development of the square wheels.... ]
There is no duplication in the old drivers because they don't implement
the needed check and clipping/bit setting that I can see ;)
Alan
On Tuesday 17 November 2009 19:21:31 Alan Cox wrote:
> > > I'm not convinced because for the majority of drivers the libata timing
> > > interface handles it. SiS needs it just because it does things by
> > > precomputed tables. It's a one off interface.
> >
> > Controllers based on *Intel* PIIX are in the disagreement with the above
>
> No the PIIX is quite different. You use the matching PIO timing (which is
> a short lookup and shorter code than even a helper function call). You do
> not use the clipping instead you set bit 3 to ensure that PIO cycles
> occur at low speed but MWDMA runs at the right speed. That is usually a
> win over picking a lower mode as the PIIX can do ATAPI DMA happily.
Thank you for the detailed explanation, I certainly would still be
scratching my head over it if it wasn't for your great help.
> So the only thing you can "share" is what would be a 4 byte table.
You are of course right, I must have been confused by the old driver.
> > paragraph and having generic helper to do conversion (without a clipping)
> > would bring us a little step closer to killing a needless code duplication
> > currently present in their ->set_piomode and ->set_dmamode methods.
> >
> > [ In case somebody wonders: no, 'old' drivers don't have such duplication
> > and 'whoever did it didn't port it across' cause said 'whoever'-s were not
> > into the development of the square wheels.... ]
>
> There is no duplication in the old drivers because they don't implement
> the needed check and clipping/bit setting that I can see ;)
Seems like the whoever maintained the old driver didn't port across
this critical bugfix. :(
--
Bartlomiej Zolnierkiewicz
On 11/17/2009 09:51 AM, Alan Cox wrote:
> Signed-off-by: Alan Cox<[email protected]>
> ---
>
> drivers/ata/Kconfig | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index f2df6e2..36931e0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -374,7 +374,7 @@ config PATA_HPT366
> If unsure, say N.
>
> config PATA_HPT37X
> - tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
> + tristate "HPT 370/370A/371/372/374/302 PATA support"
> depends on PCI&& EXPERIMENTAL
When you change the help string, you must also change the 'depends' line.
Jeff
On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
> Signed-off-by: Alan Cox <[email protected]>
> ---
>
> drivers/ata/Kconfig | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index f2df6e2..36931e0 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -374,7 +374,7 @@ config PATA_HPT366
> If unsure, say N.
>
> config PATA_HPT37X
> - tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
> + tristate "HPT 370/370A/371/372/374/302 PATA support"
> depends on PCI && EXPERIMENTAL
> help
> This option enables support for the majority of the later HPT
> @@ -383,7 +383,7 @@ config PATA_HPT37X
> If unsure, say N.
>
> config PATA_HPT3X2N
> - tristate "HPT 372N/302N PATA support (Experimental)"
> + tristate "HPT 372N/302N PATA support"
> depends on PCI && EXPERIMENTAL
> help
> This option enables support for the N variant HPT PATA
Maybe they are 'stable' but when it comes to features they are behind hpt366
(i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
to understand and much smaller..
1609 drivers/ide/hpt366.c
432 drivers/ata/pata_hpt366.c
1041 drivers/ata/pata_hpt37x.c
594 drivers/ata/pata_hpt3x2n.c
2067 total
(we can still easily cut more than 100 LOC from hpt366)
Having separate drivers wasn't the best decisions from the maintainability
point-of-view. It added needless complexity (different chips share the same
PCI IDs which make detection across multiple drivers extremely painful) and
confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
while HPT302N by pata_hpt3x2n?).
--
Bartlomiej Zolnierkiewicz
> Maybe they are 'stable' but when it comes to features they are behind hpt366
> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> to understand and much smaller..
37x and 3xn lack PCI PM. Added to the TODO list.
The smaller size is a bit questionable given its mostly comments, and
three drivers versus one.
> Having separate drivers wasn't the best decisions from the maintainability
> point-of-view. It added needless complexity (different chips share the same
It was most definitely a good decision, having maintained both sets of
drivers at different times. It also makes it possible to do things the
way highpoint does rather than trying to make stuff common which HPT
themselves don't keep common. Even more importantly we get to break *one*
type of device at a time.
> PCI IDs which make detection across multiple drivers extremely painful) and
> confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> while HPT302N by pata_hpt3x2n?).
I love highpoint too for their ever weirder and more confusingly labelled
and identified chips. I still think the split is worth it, and the 'wtf
device am I' logic is needed in both cases - either to pick a driver or
pick a set of methods.
We have the it821x driver for it8211/2 but not 8213 ..
Alan
Hello.
Alan Cox wrote:
>>Having separate drivers wasn't the best decisions from the maintainability
>>point-of-view. It added needless complexity (different chips share the same
> It was most definitely a good decision, having maintained both sets of
Separating HPT36x was grounded enough decision but I can't say the same
of the separation of HPT3xxN.
> drivers at different times. It also makes it possible to do things the
> way highpoint does
Oh, don't remind me of that stupid code mostly not worth copying from...
>>PCI IDs which make detection across multiple drivers extremely painful) and
>>confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
>>while HPT302N by pata_hpt3x2n?).
How about HPT371N? ;-)
WBR, Sergei
On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> > Maybe they are 'stable' but when it comes to features they are behind hpt366
> > (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> > to understand and much smaller..
>
> 37x and 3xn lack PCI PM. Added to the TODO list.
>
> The smaller size is a bit questionable given its mostly comments, and
> three drivers versus one.
I wonder what you find 'questionable' in the numbers given..
> > Having separate drivers wasn't the best decisions from the maintainability
> > point-of-view. It added needless complexity (different chips share the same
>
> It was most definitely a good decision, having maintained both sets of
> drivers at different times. It also makes it possible to do things the
> > way highpoint does rather than trying to make stuff common which HPT
Interesting, because all other people involved in HPT PATA support
seem to disagree with you and they certainly wouldn't say that doing
all things the way HPT did is a good thing..
> themselves don't keep common. Even more importantly we get to break *one*
> type of device at a time.
I prefer to not break anything, but hey my way of doing things is not
very much welcomed here..
I also like to think that by sharing code we get better testing coverage
and are in reality able to fix problems much faster because of this..
> > PCI IDs which make detection across multiple drivers extremely painful) and
> > confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> > while HPT302N by pata_hpt3x2n?).
>
> I love highpoint too for their ever weirder and more confusingly labelled
> and identified chips. I still think the split is worth it, and the 'wtf
> device am I' logic is needed in both cases - either to pick a driver or
> pick a set of methods.
>
> We have the it821x driver for it8211/2 but not 8213 ..
it821x and it8213 share absolutely no common logic (the first chip
has ITE logic and the second one is based on Intel ICH) while in case
of HPT family there are sometimes large similarities..
pata_hpt37x.c:
...
static struct hpt_clock hpt37x_timings_66[] = {
{ XFER_UDMA_6, 0x1c869c62 },
{ XFER_UDMA_5, 0x1cae9c62 }, /* 0x1c8a9c62 */
{ XFER_UDMA_4, 0x1c8a9c62 },
{ XFER_UDMA_3, 0x1c8e9c62 },
{ XFER_UDMA_2, 0x1c929c62 },
{ XFER_UDMA_1, 0x1c9a9c62 },
{ XFER_UDMA_0, 0x1c829c62 },
{ XFER_MW_DMA_2, 0x2c829c62 },
{ XFER_MW_DMA_1, 0x2c829c66 },
{ XFER_MW_DMA_0, 0x2c829d2e },
{ XFER_PIO_4, 0x0c829c62 },
{ XFER_PIO_3, 0x0c829c84 },
{ XFER_PIO_2, 0x0c829ca6 },
{ XFER_PIO_1, 0x0d029d26 },
{ XFER_PIO_0, 0x0d029d5e }
};
...
/**
* hpt372_set_piomode - PIO setup
* @ap: ATA interface
* @adev: device on the interface
*
* Perform PIO mode setup.
*/
static void hpt372_set_piomode(struct ata_port *ap, struct ata_device *adev)
{
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
u32 addr1, addr2;
u32 reg;
u32 mode;
u8 fast;
addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
addr2 = 0x51 + 4 * ap->port_no;
/* Fast interrupt prediction disable, hold off interrupt disable */
pci_read_config_byte(pdev, addr2, &fast);
fast &= ~0x07;
pci_write_config_byte(pdev, addr2, fast);
pci_read_config_dword(pdev, addr1, ®);
mode = hpt37x_find_mode(ap, adev->pio_mode);
printk("Find mode for %d reports %X\n", adev->pio_mode, mode);
mode &= ~0x80000000; /* No FIFO in PIO */
mode &= ~0x30070000; /* Leave config bits alone */
reg &= 0x30070000; /* Strip timing bits */
pci_write_config_dword(pdev, addr1, reg | mode);
}
/**
* hpt372_set_dmamode - DMA timing setup
* @ap: ATA interface
* @adev: Device being configured
*
* Set up the channel for MWDMA or UDMA modes. Much the same as with
* PIO, load the mode number and then set MWDMA or UDMA flag.
*/
static void hpt372_set_dmamode(struct ata_port *ap, struct ata_device *adev)
{
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
u32 addr1, addr2;
u32 reg;
u32 mode;
u8 fast;
addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
addr2 = 0x51 + 4 * ap->port_no;
/* Fast interrupt prediction disable, hold off interrupt disable */
pci_read_config_byte(pdev, addr2, &fast);
fast &= ~0x07;
pci_write_config_byte(pdev, addr2, fast);
pci_read_config_dword(pdev, addr1, ®);
mode = hpt37x_find_mode(ap, adev->dma_mode);
printk("Find mode for DMA %d reports %X\n", adev->dma_mode, mode);
mode &= ~0xC0000000; /* Leave config bits alone */
mode |= 0x80000000; /* FIFO in MWDMA or UDMA */
reg &= 0xC0000000; /* Strip timing bits */
pci_write_config_dword(pdev, addr1, reg | mode);
}
/**
* hpt37x_bmdma_end - DMA engine stop
* @qc: ATA command
*
* Clean up after the HPT372 and later DMA engine
*/
static void hpt37x_bmdma_stop(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
int mscreg = 0x50 + 4 * ap->port_no;
u8 bwsr_stat, msc_stat;
pci_read_config_byte(pdev, 0x6A, &bwsr_stat);
pci_read_config_byte(pdev, mscreg, &msc_stat);
if (bwsr_stat & (1 << ap->port_no))
pci_write_config_byte(pdev, mscreg, msc_stat | 0x30);
ata_bmdma_stop(qc);
}
...
/**
* hpt37x_calibrate_dpll - Calibrate the DPLL loop
* @dev: PCI device
*
* Perform a calibration cycle on the HPT37x DPLL. Returns 1 if this
* succeeds
*/
static int hpt37x_calibrate_dpll(struct pci_dev *dev)
{
u8 reg5b;
u32 reg5c;
int tries;
for(tries = 0; tries < 0x5000; tries++) {
udelay(50);
pci_read_config_byte(dev, 0x5b, ®5b);
if (reg5b & 0x80) {
/* See if it stays set */
for(tries = 0; tries < 0x1000; tries ++) {
pci_read_config_byte(dev, 0x5b, ®5b);
/* Failed ? */
if ((reg5b & 0x80) == 0)
return 0;
}
/* Turn off tuning, we have the DPLL set */
pci_read_config_dword(dev, 0x5c, ®5c);
pci_write_config_dword(dev, 0x5c, reg5c & ~ 0x100);
return 1;
}
}
/* Never went stable */
return 0;
}
...
pata_hpt3x2n.c:
...
/* 66MHz DPLL clocks */
static struct hpt_clock hpt3x2n_clocks[] = {
{ XFER_UDMA_7, 0x1c869c62 },
{ XFER_UDMA_6, 0x1c869c62 },
{ XFER_UDMA_5, 0x1c8a9c62 },
{ XFER_UDMA_4, 0x1c8a9c62 },
{ XFER_UDMA_3, 0x1c8e9c62 },
{ XFER_UDMA_2, 0x1c929c62 },
{ XFER_UDMA_1, 0x1c9a9c62 },
{ XFER_UDMA_0, 0x1c829c62 },
{ XFER_MW_DMA_2, 0x2c829c62 },
{ XFER_MW_DMA_1, 0x2c829c66 },
{ XFER_MW_DMA_0, 0x2c829d2c },
{ XFER_PIO_4, 0x0c829c62 },
{ XFER_PIO_3, 0x0c829c84 },
{ XFER_PIO_2, 0x0c829ca6 },
{ XFER_PIO_1, 0x0d029d26 },
{ XFER_PIO_0, 0x0d029d5e },
{ 0, 0x0d029d5e }
};
...
/**
* hpt3x2n_set_piomode - PIO setup
* @ap: ATA interface
* @adev: device on the interface
*
* Perform PIO mode setup.
*/
static void hpt3x2n_set_piomode(struct ata_port *ap, struct ata_device *adev)
{
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
u32 addr1, addr2;
u32 reg;
u32 mode;
u8 fast;
addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
addr2 = 0x51 + 4 * ap->port_no;
/* Fast interrupt prediction disable, hold off interrupt disable */
pci_read_config_byte(pdev, addr2, &fast);
fast &= ~0x07;
pci_write_config_byte(pdev, addr2, fast);
pci_read_config_dword(pdev, addr1, ®);
mode = hpt3x2n_find_mode(ap, adev->pio_mode);
mode &= ~0x8000000; /* No FIFO in PIO */
mode &= ~0x30070000; /* Leave config bits alone */
reg &= 0x30070000; /* Strip timing bits */
pci_write_config_dword(pdev, addr1, reg | mode);
}
/**
* hpt3x2n_set_dmamode - DMA timing setup
* @ap: ATA interface
* @adev: Device being configured
*
* Set up the channel for MWDMA or UDMA modes. Much the same as with
* PIO, load the mode number and then set MWDMA or UDMA flag.
*/
static void hpt3x2n_set_dmamode(struct ata_port *ap, struct ata_device *adev)
{
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
u32 addr1, addr2;
u32 reg;
u32 mode;
u8 fast;
addr1 = 0x40 + 4 * (adev->devno + 2 * ap->port_no);
addr2 = 0x51 + 4 * ap->port_no;
/* Fast interrupt prediction disable, hold off interrupt disable */
pci_read_config_byte(pdev, addr2, &fast);
fast &= ~0x07;
pci_write_config_byte(pdev, addr2, fast);
pci_read_config_dword(pdev, addr1, ®);
mode = hpt3x2n_find_mode(ap, adev->dma_mode);
mode |= 0x8000000; /* FIFO in MWDMA or UDMA */
mode &= ~0xC0000000; /* Leave config bits alone */
reg &= 0xC0000000; /* Strip timing bits */
pci_write_config_dword(pdev, addr1, reg | mode);
}
/**
* hpt3x2n_bmdma_end - DMA engine stop
* @qc: ATA command
*
* Clean up after the HPT3x2n and later DMA engine
*/
static void hpt3x2n_bmdma_stop(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct pci_dev *pdev = to_pci_dev(ap->host->dev);
int mscreg = 0x50 + 2 * ap->port_no;
u8 bwsr_stat, msc_stat;
pci_read_config_byte(pdev, 0x6A, &bwsr_stat);
pci_read_config_byte(pdev, mscreg, &msc_stat);
if (bwsr_stat & (1 << ap->port_no))
pci_write_config_byte(pdev, mscreg, msc_stat | 0x30);
ata_bmdma_stop(qc);
}
...
/**
* hpt3xn_calibrate_dpll - Calibrate the DPLL loop
* @dev: PCI device
*
* Perform a calibration cycle on the HPT3xN DPLL. Returns 1 if this
* succeeds
*/
static int hpt3xn_calibrate_dpll(struct pci_dev *dev)
{
u8 reg5b;
u32 reg5c;
int tries;
for(tries = 0; tries < 0x5000; tries++) {
udelay(50);
pci_read_config_byte(dev, 0x5b, ®5b);
if (reg5b & 0x80) {
/* See if it stays set */
for(tries = 0; tries < 0x1000; tries ++) {
pci_read_config_byte(dev, 0x5b, ®5b);
/* Failed ? */
if ((reg5b & 0x80) == 0)
return 0;
}
/* Turn off tuning, we have the DPLL set */
pci_read_config_dword(dev, 0x5c, ®5c);
pci_write_config_dword(dev, 0x5c, reg5c & ~ 0x100);
return 1;
}
}
/* Never went stable */
return 0;
}
...
etc.
--
Bartlomiej Zolnierkiewicz
On Wednesday 18 November 2009 19:47:25 Sergei Shtylyov wrote:
> Hello.
>
> Alan Cox wrote:
>
> >>Having separate drivers wasn't the best decisions from the maintainability
> >>point-of-view. It added needless complexity (different chips share the same
>
> > It was most definitely a good decision, having maintained both sets of
>
> Separating HPT36x was grounded enough decision but I can't say the same
> of the separation of HPT3xxN.
When it comes to HPT36x I would agree if we would be developing completely
from scratch but due to historical reasons we share UDMA blacklists between
HPT36x and HPT37x. It is not worth it in terms of potential regressions to
try to untangle them nowadays.
> > drivers at different times. It also makes it possible to do things the
> > way highpoint does
>
> Oh, don't remind me of that stupid code mostly not worth copying from...
>
> >>PCI IDs which make detection across multiple drivers extremely painful) and
> >>confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> >>while HPT302N by pata_hpt3x2n?).
>
> How about HPT371N? ;-)
Not a problem, this one is unsupported according to help entries. ;)
--
Bartlomiej Zolnierkiewicz
On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> > PCI IDs which make detection across multiple drivers extremely painful) and
> > confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> > while HPT302N by pata_hpt3x2n?).
>
> I love highpoint too for their ever weirder and more confusingly labelled
> and identified chips. I still think the split is worth it, and the 'wtf
> device am I' logic is needed in both cases - either to pick a driver or
> pick a set of methods.
While in case of pata_hpt366.c it doesn't look that bad in case of two other
drivers it does..
pata_hpt366.c:
...
/**
* hpt36x_init_one - Initialise an HPT366/368
* @dev: PCI device
* @id: Entry in match table
*
* Initialise an HPT36x device. There are some interesting complications
* here. Firstly the chip may report 366 and be one of several variants.
* Secondly all the timings depend on the clock for the chip which we must
* detect and look up
*
* This is the known chip mappings. It may be missing a couple of later
* releases.
*
* Chip version PCI Rev Notes
* HPT366 4 (HPT366) 0 UDMA66
* HPT366 4 (HPT366) 1 UDMA66
* HPT368 4 (HPT366) 2 UDMA66
* HPT37x/30x 4 (HPT366) 3+ Other driver
*
*/
static int hpt36x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
static const struct ata_port_info info_hpt366 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA4,
.port_ops = &hpt366_port_ops
};
const struct ata_port_info *ppi[] = { &info_hpt366, NULL };
void *hpriv = NULL;
u32 class_rev;
u32 reg1;
int rc;
rc = pcim_enable_device(dev);
if (rc)
return rc;
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
class_rev &= 0xFF;
/* May be a later chip in disguise. Check */
/* Newer chips are not in the HPT36x driver. Ignore them */
if (class_rev > 2)
return -ENODEV;
...
pata_hpt37x.c:
...
/**
* hpt37x_init_one - Initialise an HPT37X/302
* @dev: PCI device
* @id: Entry in match table
*
* Initialise an HPT37x device. There are some interesting complications
* here. Firstly the chip may report 366 and be one of several variants.
* Secondly all the timings depend on the clock for the chip which we must
* detect and look up
*
* This is the known chip mappings. It may be missing a couple of later
* releases.
*
* Chip version PCI Rev Notes
* HPT366 4 (HPT366) 0 Other driver
* HPT366 4 (HPT366) 1 Other driver
* HPT368 4 (HPT366) 2 Other driver
* HPT370 4 (HPT366) 3 UDMA100
* HPT370A 4 (HPT366) 4 UDMA100
* HPT372 4 (HPT366) 5 UDMA133 (1)
* HPT372N 4 (HPT366) 6 Other driver
* HPT372A 5 (HPT372) 1 UDMA133 (1)
* HPT372N 5 (HPT372) 2 Other driver
* HPT302 6 (HPT302) 1 UDMA133
* HPT302N 6 (HPT302) 2 Other driver
* HPT371 7 (HPT371) * UDMA133
* HPT374 8 (HPT374) * UDMA133 4 channel
* HPT372N 9 (HPT372N) * Other driver
*
* (1) UDMA133 support depends on the bus clock
*/
static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
/* HPT370 - UDMA100 */
static const struct ata_port_info info_hpt370 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA5,
.port_ops = &hpt370_port_ops
};
/* HPT370A - UDMA100 */
static const struct ata_port_info info_hpt370a = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA5,
.port_ops = &hpt370a_port_ops
};
/* HPT370 - UDMA100 */
static const struct ata_port_info info_hpt370_33 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA5,
.port_ops = &hpt370_port_ops
};
/* HPT370A - UDMA100 */
static const struct ata_port_info info_hpt370a_33 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA5,
.port_ops = &hpt370a_port_ops
};
/* HPT371, 372 and friends - UDMA133 */
static const struct ata_port_info info_hpt372 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA6,
.port_ops = &hpt372_port_ops
};
/* HPT374 - UDMA100, function 1 uses different prereset method */
static const struct ata_port_info info_hpt374_fn0 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA5,
.port_ops = &hpt372_port_ops
};
static const struct ata_port_info info_hpt374_fn1 = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA5,
.port_ops = &hpt374_fn1_port_ops
};
static const int MHz[4] = { 33, 40, 50, 66 };
void *private_data = NULL;
const struct ata_port_info *ppi[] = { NULL, NULL };
u8 irqmask;
u32 class_rev;
u8 mcr1;
u32 freq;
int prefer_dpll = 1;
unsigned long iobase = pci_resource_start(dev, 4);
const struct hpt_chip *chip_table;
int clock_slot;
int rc;
rc = pcim_enable_device(dev);
if (rc)
return rc;
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
class_rev &= 0xFF;
if (dev->device == PCI_DEVICE_ID_TTI_HPT366) {
/* May be a later chip in disguise. Check */
/* Older chips are in the HPT366 driver. Ignore them */
if (class_rev < 3)
return -ENODEV;
/* N series chips have their own driver. Ignore */
if (class_rev == 6)
return -ENODEV;
switch(class_rev) {
case 3:
ppi[0] = &info_hpt370;
chip_table = &hpt370;
prefer_dpll = 0;
break;
case 4:
ppi[0] = &info_hpt370a;
chip_table = &hpt370a;
prefer_dpll = 0;
break;
case 5:
ppi[0] = &info_hpt372;
chip_table = &hpt372;
break;
default:
printk(KERN_ERR "pata_hpt37x: Unknown HPT366 subtype please report (%d).\n", class_rev);
return -ENODEV;
}
} else {
switch(dev->device) {
case PCI_DEVICE_ID_TTI_HPT372:
/* 372N if rev >= 2*/
if (class_rev >= 2)
return -ENODEV;
ppi[0] = &info_hpt372;
chip_table = &hpt372a;
break;
case PCI_DEVICE_ID_TTI_HPT302:
/* 302N if rev > 1 */
if (class_rev > 1)
return -ENODEV;
ppi[0] = &info_hpt372;
/* Check this */
chip_table = &hpt302;
break;
case PCI_DEVICE_ID_TTI_HPT371:
if (class_rev > 1)
return -ENODEV;
ppi[0] = &info_hpt372;
chip_table = &hpt371;
/* Single channel device, master is not present
but the BIOS (or us for non x86) must mark it
absent */
pci_read_config_byte(dev, 0x50, &mcr1);
mcr1 &= ~0x04;
pci_write_config_byte(dev, 0x50, mcr1);
break;
case PCI_DEVICE_ID_TTI_HPT374:
chip_table = &hpt374;
if (!(PCI_FUNC(dev->devfn) & 1))
*ppi = &info_hpt374_fn0;
else
*ppi = &info_hpt374_fn1;
break;
default:
printk(KERN_ERR "pata_hpt37x: PCI table is bogus please report (%d).\n", dev->device);
return -ENODEV;
}
}
...
pata_hpt3x2n.c:
...
/**
* hpt3x2n_init_one - Initialise an HPT37X/302
* @dev: PCI device
* @id: Entry in match table
*
* Initialise an HPT3x2n device. There are some interesting complications
* here. Firstly the chip may report 366 and be one of several variants.
* Secondly all the timings depend on the clock for the chip which we must
* detect and look up
*
* This is the known chip mappings. It may be missing a couple of later
* releases.
*
* Chip version PCI Rev Notes
* HPT372 4 (HPT366) 5 Other driver
* HPT372N 4 (HPT366) 6 UDMA133
* HPT372 5 (HPT372) 1 Other driver
* HPT372N 5 (HPT372) 2 UDMA133
* HPT302 6 (HPT302) * Other driver
* HPT302N 6 (HPT302) > 1 UDMA133
* HPT371 7 (HPT371) * Other driver
* HPT371N 7 (HPT371) > 1 UDMA133
* HPT374 8 (HPT374) * Other driver
* HPT372N 9 (HPT372N) * UDMA133
*
* (1) UDMA133 support depends on the bus clock
*
* To pin down HPT371N
*/
static int hpt3x2n_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
/* HPT372N and friends - UDMA133 */
static const struct ata_port_info info = {
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = ATA_PIO4,
.mwdma_mask = ATA_MWDMA2,
.udma_mask = ATA_UDMA6,
.port_ops = &hpt3x2n_port_ops
};
const struct ata_port_info *ppi[] = { &info, NULL };
u8 irqmask;
u32 class_rev;
unsigned int pci_mhz;
unsigned int f_low, f_high;
int adjust;
unsigned long iobase = pci_resource_start(dev, 4);
void *hpriv = NULL;
int rc;
rc = pcim_enable_device(dev);
if (rc)
return rc;
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
class_rev &= 0xFF;
switch(dev->device) {
case PCI_DEVICE_ID_TTI_HPT366:
if (class_rev < 6)
return -ENODEV;
break;
case PCI_DEVICE_ID_TTI_HPT371:
if (class_rev < 2)
return -ENODEV;
/* 371N if rev > 1 */
break;
case PCI_DEVICE_ID_TTI_HPT372:
/* 372N if rev >= 2*/
if (class_rev < 2)
return -ENODEV;
break;
case PCI_DEVICE_ID_TTI_HPT302:
if (class_rev < 2)
return -ENODEV;
break;
case PCI_DEVICE_ID_TTI_HPT372N:
break;
default:
printk(KERN_ERR "pata_hpt3x2n: PCI table is bogus please report (%d).\n", dev->device);
return -ENODEV;
}
...
and now for the comparison hpt366.c:
...
static const struct hpt_info hpt36x __devinitdata = {
.chip_name = "HPT36x",
.chip_type = HPT36x,
.udma_mask = HPT366_ALLOW_ATA66_3 ? (HPT366_ALLOW_ATA66_4 ? ATA_UDMA4 : ATA_UDMA3) : ATA_UDMA2,
.dpll_clk = 0, /* no DPLL */
.timings = &hpt36x_timings
};
static const struct hpt_info hpt370 __devinitdata = {
.chip_name = "HPT370",
.chip_type = HPT370,
.udma_mask = HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
.dpll_clk = 48,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt370a __devinitdata = {
.chip_name = "HPT370A",
.chip_type = HPT370A,
.udma_mask = HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
.dpll_clk = 48,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt374 __devinitdata = {
.chip_name = "HPT374",
.chip_type = HPT374,
.udma_mask = ATA_UDMA5,
.dpll_clk = 48,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt372 __devinitdata = {
.chip_name = "HPT372",
.chip_type = HPT372,
.udma_mask = HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 55,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt372a __devinitdata = {
.chip_name = "HPT372A",
.chip_type = HPT372A,
.udma_mask = HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 66,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt302 __devinitdata = {
.chip_name = "HPT302",
.chip_type = HPT302,
.udma_mask = HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 66,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt371 __devinitdata = {
.chip_name = "HPT371",
.chip_type = HPT371,
.udma_mask = HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 66,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt372n __devinitdata = {
.chip_name = "HPT372N",
.chip_type = HPT372N,
.udma_mask = HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 77,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt302n __devinitdata = {
.chip_name = "HPT302N",
.chip_type = HPT302N,
.udma_mask = HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 77,
.timings = &hpt37x_timings
};
static const struct hpt_info hpt371n __devinitdata = {
.chip_name = "HPT371N",
.chip_type = HPT371N,
.udma_mask = HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
.dpll_clk = 77,
.timings = &hpt37x_timings
};
...
/**
* hpt366_init_one - called when an HPT366 is found
* @dev: the hpt366 device
* @id: the matching pci id
*
* Called when the PCI registration layer (or the IDE initialization)
* finds a device matching our IDE device tables.
*/
static int __devinit hpt366_init_one(struct pci_dev *dev, const struct pci_device_id *id)
{
const struct hpt_info *info = NULL;
struct hpt_info *dyn_info;
struct pci_dev *dev2 = NULL;
struct ide_port_info d;
u8 idx = id->driver_data;
u8 rev = dev->revision;
int ret;
if ((idx == 0 || idx == 4) && (PCI_FUNC(dev->devfn) & 1))
return -ENODEV;
switch (idx) {
case 0:
if (rev < 3)
info = &hpt36x;
else {
switch (min_t(u8, rev, 6)) {
case 3: info = &hpt370; break;
case 4: info = &hpt370a; break;
case 5: info = &hpt372; break;
case 6: info = &hpt372n; break;
}
idx++;
}
break;
case 1:
info = (rev > 1) ? &hpt372n : &hpt372a;
break;
case 2:
info = (rev > 1) ? &hpt302n : &hpt302;
break;
case 3:
hpt371_init(dev);
info = (rev > 1) ? &hpt371n : &hpt371;
break;
case 4:
info = &hpt374;
break;
case 5:
info = &hpt372n;
break;
}
...
--
Bartlomiej Zolnierkiewicz
On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> > > Maybe they are 'stable' but when it comes to features they are behind hpt366
> > > (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> > > to understand and much smaller..
> >
> > 37x and 3xn lack PCI PM. Added to the TODO list.
> >
> > The smaller size is a bit questionable given its mostly comments, and
> > three drivers versus one.
>
> I wonder what you find 'questionable' in the numbers given..
BTW we can immediately reclaim 300 LOC by simply merging drivers back..
> > themselves don't keep common. Even more importantly we get to break *one*
> > type of device at a time.
>
> I prefer to not break anything, but hey my way of doing things is not
> very much welcomed here..
>
> I also like to think that by sharing code we get better testing coverage
> and are in reality able to fix problems much faster because of this..
Wait, you seem to be right!
Only pata_hpt37x has broken cable detection while pata_hpt3x2n is okay. ;)
--
Bartlomiej Zolnierkiewicz
On Wednesday 18 November 2009 20:34:19 Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
>
> > > PCI IDs which make detection across multiple drivers extremely painful) and
> > > confusion (i.e. would you have guessed that HPT302 is supported by pata_hpt37x
> > > while HPT302N by pata_hpt3x2n?).
> >
> > I love highpoint too for their ever weirder and more confusingly labelled
> > and identified chips. I still think the split is worth it, and the 'wtf
> > device am I' logic is needed in both cases - either to pick a driver or
> > pick a set of methods.
>
> While in case of pata_hpt366.c it doesn't look that bad in case of two other
> drivers it does..
>
> pata_hpt366.c:
> ...
> /**
> * hpt36x_init_one - Initialise an HPT366/368
> * @dev: PCI device
> * @id: Entry in match table
> *
> * Initialise an HPT36x device. There are some interesting complications
> * here. Firstly the chip may report 366 and be one of several variants.
> * Secondly all the timings depend on the clock for the chip which we must
> * detect and look up
> *
> * This is the known chip mappings. It may be missing a couple of later
> * releases.
> *
> * Chip version PCI Rev Notes
> * HPT366 4 (HPT366) 0 UDMA66
> * HPT366 4 (HPT366) 1 UDMA66
> * HPT368 4 (HPT366) 2 UDMA66
> * HPT37x/30x 4 (HPT366) 3+ Other driver
> *
> */
>
> static int hpt36x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> static const struct ata_port_info info_hpt366 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA4,
> .port_ops = &hpt366_port_ops
> };
> const struct ata_port_info *ppi[] = { &info_hpt366, NULL };
>
> void *hpriv = NULL;
> u32 class_rev;
> u32 reg1;
> int rc;
>
> rc = pcim_enable_device(dev);
> if (rc)
> return rc;
>
> pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
> class_rev &= 0xFF;
>
> /* May be a later chip in disguise. Check */
> /* Newer chips are not in the HPT36x driver. Ignore them */
> if (class_rev > 2)
> return -ENODEV;
> ...
>
> pata_hpt37x.c:
> ...
> /**
> * hpt37x_init_one - Initialise an HPT37X/302
> * @dev: PCI device
> * @id: Entry in match table
> *
> * Initialise an HPT37x device. There are some interesting complications
> * here. Firstly the chip may report 366 and be one of several variants.
> * Secondly all the timings depend on the clock for the chip which we must
> * detect and look up
> *
> * This is the known chip mappings. It may be missing a couple of later
> * releases.
> *
> * Chip version PCI Rev Notes
> * HPT366 4 (HPT366) 0 Other driver
> * HPT366 4 (HPT366) 1 Other driver
> * HPT368 4 (HPT366) 2 Other driver
> * HPT370 4 (HPT366) 3 UDMA100
> * HPT370A 4 (HPT366) 4 UDMA100
> * HPT372 4 (HPT366) 5 UDMA133 (1)
> * HPT372N 4 (HPT366) 6 Other driver
> * HPT372A 5 (HPT372) 1 UDMA133 (1)
> * HPT372N 5 (HPT372) 2 Other driver
> * HPT302 6 (HPT302) 1 UDMA133
> * HPT302N 6 (HPT302) 2 Other driver
> * HPT371 7 (HPT371) * UDMA133
> * HPT374 8 (HPT374) * UDMA133 4 channel
> * HPT372N 9 (HPT372N) * Other driver
> *
> * (1) UDMA133 support depends on the bus clock
> */
>
> static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> /* HPT370 - UDMA100 */
> static const struct ata_port_info info_hpt370 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA5,
> .port_ops = &hpt370_port_ops
> };
> /* HPT370A - UDMA100 */
> static const struct ata_port_info info_hpt370a = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA5,
> .port_ops = &hpt370a_port_ops
> };
> /* HPT370 - UDMA100 */
> static const struct ata_port_info info_hpt370_33 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA5,
> .port_ops = &hpt370_port_ops
> };
> /* HPT370A - UDMA100 */
> static const struct ata_port_info info_hpt370a_33 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA5,
> .port_ops = &hpt370a_port_ops
> };
> /* HPT371, 372 and friends - UDMA133 */
> static const struct ata_port_info info_hpt372 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA6,
> .port_ops = &hpt372_port_ops
> };
> /* HPT374 - UDMA100, function 1 uses different prereset method */
> static const struct ata_port_info info_hpt374_fn0 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA5,
> .port_ops = &hpt372_port_ops
> };
> static const struct ata_port_info info_hpt374_fn1 = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA5,
> .port_ops = &hpt374_fn1_port_ops
> };
>
> static const int MHz[4] = { 33, 40, 50, 66 };
> void *private_data = NULL;
> const struct ata_port_info *ppi[] = { NULL, NULL };
>
> u8 irqmask;
> u32 class_rev;
> u8 mcr1;
> u32 freq;
> int prefer_dpll = 1;
>
> unsigned long iobase = pci_resource_start(dev, 4);
>
> const struct hpt_chip *chip_table;
> int clock_slot;
> int rc;
>
> rc = pcim_enable_device(dev);
> if (rc)
> return rc;
>
> pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
> class_rev &= 0xFF;
>
> if (dev->device == PCI_DEVICE_ID_TTI_HPT366) {
> /* May be a later chip in disguise. Check */
> /* Older chips are in the HPT366 driver. Ignore them */
> if (class_rev < 3)
> return -ENODEV;
> /* N series chips have their own driver. Ignore */
> if (class_rev == 6)
> return -ENODEV;
>
> switch(class_rev) {
> case 3:
> ppi[0] = &info_hpt370;
> chip_table = &hpt370;
> prefer_dpll = 0;
> break;
> case 4:
> ppi[0] = &info_hpt370a;
> chip_table = &hpt370a;
> prefer_dpll = 0;
> break;
> case 5:
> ppi[0] = &info_hpt372;
> chip_table = &hpt372;
> break;
> default:
> printk(KERN_ERR "pata_hpt37x: Unknown HPT366 subtype please report (%d).\n", class_rev);
> return -ENODEV;
> }
> } else {
> switch(dev->device) {
> case PCI_DEVICE_ID_TTI_HPT372:
> /* 372N if rev >= 2*/
> if (class_rev >= 2)
> return -ENODEV;
> ppi[0] = &info_hpt372;
> chip_table = &hpt372a;
> break;
> case PCI_DEVICE_ID_TTI_HPT302:
> /* 302N if rev > 1 */
> if (class_rev > 1)
> return -ENODEV;
> ppi[0] = &info_hpt372;
> /* Check this */
> chip_table = &hpt302;
> break;
> case PCI_DEVICE_ID_TTI_HPT371:
> if (class_rev > 1)
> return -ENODEV;
> ppi[0] = &info_hpt372;
> chip_table = &hpt371;
> /* Single channel device, master is not present
> but the BIOS (or us for non x86) must mark it
> absent */
> pci_read_config_byte(dev, 0x50, &mcr1);
> mcr1 &= ~0x04;
> pci_write_config_byte(dev, 0x50, mcr1);
> break;
> case PCI_DEVICE_ID_TTI_HPT374:
> chip_table = &hpt374;
> if (!(PCI_FUNC(dev->devfn) & 1))
> *ppi = &info_hpt374_fn0;
> else
> *ppi = &info_hpt374_fn1;
> break;
> default:
> printk(KERN_ERR "pata_hpt37x: PCI table is bogus please report (%d).\n", dev->device);
> return -ENODEV;
> }
> }
> ...
>
> pata_hpt3x2n.c:
> ...
> /**
> * hpt3x2n_init_one - Initialise an HPT37X/302
> * @dev: PCI device
> * @id: Entry in match table
> *
> * Initialise an HPT3x2n device. There are some interesting complications
> * here. Firstly the chip may report 366 and be one of several variants.
> * Secondly all the timings depend on the clock for the chip which we must
> * detect and look up
> *
> * This is the known chip mappings. It may be missing a couple of later
> * releases.
> *
> * Chip version PCI Rev Notes
> * HPT372 4 (HPT366) 5 Other driver
> * HPT372N 4 (HPT366) 6 UDMA133
> * HPT372 5 (HPT372) 1 Other driver
> * HPT372N 5 (HPT372) 2 UDMA133
> * HPT302 6 (HPT302) * Other driver
> * HPT302N 6 (HPT302) > 1 UDMA133
> * HPT371 7 (HPT371) * Other driver
> * HPT371N 7 (HPT371) > 1 UDMA133
> * HPT374 8 (HPT374) * Other driver
> * HPT372N 9 (HPT372N) * UDMA133
> *
> * (1) UDMA133 support depends on the bus clock
> *
> * To pin down HPT371N
> */
>
> static int hpt3x2n_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> /* HPT372N and friends - UDMA133 */
> static const struct ata_port_info info = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> .mwdma_mask = ATA_MWDMA2,
> .udma_mask = ATA_UDMA6,
> .port_ops = &hpt3x2n_port_ops
> };
> const struct ata_port_info *ppi[] = { &info, NULL };
>
> u8 irqmask;
> u32 class_rev;
>
> unsigned int pci_mhz;
> unsigned int f_low, f_high;
> int adjust;
> unsigned long iobase = pci_resource_start(dev, 4);
> void *hpriv = NULL;
> int rc;
>
> rc = pcim_enable_device(dev);
> if (rc)
> return rc;
>
> pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
> class_rev &= 0xFF;
>
> switch(dev->device) {
> case PCI_DEVICE_ID_TTI_HPT366:
> if (class_rev < 6)
> return -ENODEV;
> break;
> case PCI_DEVICE_ID_TTI_HPT371:
> if (class_rev < 2)
> return -ENODEV;
> /* 371N if rev > 1 */
> break;
> case PCI_DEVICE_ID_TTI_HPT372:
> /* 372N if rev >= 2*/
> if (class_rev < 2)
> return -ENODEV;
> break;
> case PCI_DEVICE_ID_TTI_HPT302:
> if (class_rev < 2)
> return -ENODEV;
> break;
> case PCI_DEVICE_ID_TTI_HPT372N:
> break;
> default:
> printk(KERN_ERR "pata_hpt3x2n: PCI table is bogus please report (%d).\n", dev->device);
> return -ENODEV;
> }
> ...
>
>
> and now for the comparison hpt366.c:
>
>
> ...
> static const struct hpt_info hpt36x __devinitdata = {
> .chip_name = "HPT36x",
> .chip_type = HPT36x,
> .udma_mask = HPT366_ALLOW_ATA66_3 ? (HPT366_ALLOW_ATA66_4 ? ATA_UDMA4 : ATA_UDMA3) : ATA_UDMA2,
> .dpll_clk = 0, /* no DPLL */
> .timings = &hpt36x_timings
> };
>
> static const struct hpt_info hpt370 __devinitdata = {
> .chip_name = "HPT370",
> .chip_type = HPT370,
> .udma_mask = HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
> .dpll_clk = 48,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt370a __devinitdata = {
> .chip_name = "HPT370A",
> .chip_type = HPT370A,
> .udma_mask = HPT370_ALLOW_ATA100_5 ? ATA_UDMA5 : ATA_UDMA4,
> .dpll_clk = 48,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt374 __devinitdata = {
> .chip_name = "HPT374",
> .chip_type = HPT374,
> .udma_mask = ATA_UDMA5,
> .dpll_clk = 48,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt372 __devinitdata = {
> .chip_name = "HPT372",
> .chip_type = HPT372,
> .udma_mask = HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 55,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt372a __devinitdata = {
> .chip_name = "HPT372A",
> .chip_type = HPT372A,
> .udma_mask = HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 66,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt302 __devinitdata = {
> .chip_name = "HPT302",
> .chip_type = HPT302,
> .udma_mask = HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 66,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt371 __devinitdata = {
> .chip_name = "HPT371",
> .chip_type = HPT371,
> .udma_mask = HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 66,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt372n __devinitdata = {
> .chip_name = "HPT372N",
> .chip_type = HPT372N,
> .udma_mask = HPT372_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 77,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt302n __devinitdata = {
> .chip_name = "HPT302N",
> .chip_type = HPT302N,
> .udma_mask = HPT302_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 77,
> .timings = &hpt37x_timings
> };
>
> static const struct hpt_info hpt371n __devinitdata = {
> .chip_name = "HPT371N",
> .chip_type = HPT371N,
> .udma_mask = HPT371_ALLOW_ATA133_6 ? ATA_UDMA6 : ATA_UDMA5,
> .dpll_clk = 77,
> .timings = &hpt37x_timings
> };
> ...
> /**
> * hpt366_init_one - called when an HPT366 is found
> * @dev: the hpt366 device
> * @id: the matching pci id
> *
> * Called when the PCI registration layer (or the IDE initialization)
> * finds a device matching our IDE device tables.
> */
> static int __devinit hpt366_init_one(struct pci_dev *dev, const struct pci_device_id *id)
> {
> const struct hpt_info *info = NULL;
> struct hpt_info *dyn_info;
> struct pci_dev *dev2 = NULL;
> struct ide_port_info d;
> u8 idx = id->driver_data;
> u8 rev = dev->revision;
> int ret;
>
> if ((idx == 0 || idx == 4) && (PCI_FUNC(dev->devfn) & 1))
> return -ENODEV;
>
> switch (idx) {
> case 0:
> if (rev < 3)
> info = &hpt36x;
> else {
> switch (min_t(u8, rev, 6)) {
> case 3: info = &hpt370; break;
> case 4: info = &hpt370a; break;
> case 5: info = &hpt372; break;
> case 6: info = &hpt372n; break;
> }
> idx++;
> }
> break;
> case 1:
> info = (rev > 1) ? &hpt372n : &hpt372a;
> break;
> case 2:
> info = (rev > 1) ? &hpt302n : &hpt302;
> break;
> case 3:
> hpt371_init(dev);
> info = (rev > 1) ? &hpt371n : &hpt371;
> break;
> case 4:
> info = &hpt374;
> break;
> case 5:
> info = &hpt372n;
> break;
> }
> ...
On the second look it seems pata_ drivers also have something similar
to struct hpt_info (it is called struct hpt_chip) so in reality only
hpt366_init_one() code matters.
--
Bartlomiej Zolnierkiewicz
> Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> okay. ;)
I think you have that backwards according to the vendor drivers. 3x2n
looks like it needs adjusting.
> BTW we can immediately reclaim 300 LOC by simply merging drivers back..
LoC - lines of comments ? (at least half of them).
Been testing a bit before fixing up the pata_hpt drivers. Resume from S2R
doesn't work with drivers/ide/hpt366.c. So it may have code for resume
but it doesn't actually work for all the chips.
Reason is fairly obvious - the PCI speed detection logic is broken except
for the case of a PC boot where the ROM BIOS for the HPT3xx is run. That
doesn't occur on a resume from RAM so a 66MHz clocked motherboard device
comes back with the speed guessed wrongly.
Alan Cox wrote:
>>BTW we can immediately reclaim 300 LOC by simply merging drivers back..
> LoC - lines of comments ? (at least half of them).
> Been testing a bit before fixing up the pata_hpt drivers. Resume from S2R
> doesn't work with drivers/ide/hpt366.c. So it may have code for resume
> but it doesn't actually work for all the chips.
> Reason is fairly obvious - the PCI speed detection logic is broken except
> for the case of a PC boot where the ROM BIOS for the HPT3xx is run. That
Point out the breakage please. The driver has been developed and tested
on non-x86 machine with 66 MHz PCI.
> doesn't occur on a resume from RAM so a 66MHz clocked motherboard device
> comes back with the speed guessed wrongly.
WBR, Sergei
On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > okay. ;)
>
> I think you have that backwards according to the vendor drivers. 3x2n
> looks like it needs adjusting.
lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..
--
Bartlomiej Zolnierkiewicz
On Thursday 19 November 2009 15:02:59 Alan Cox wrote:
> > BTW we can immediately reclaim 300 LOC by simply merging drivers back..
>
> LoC - lines of comments ? (at least half of them).
lol, I _only_ counted whole functions + their documentation to come up with
this fairly _conservative_ number...
--
Bartlomiej Zolnierkiewicz
> Point out the breakage please. The driver has been developed and tested
> on non-x86 machine with 66 MHz PCI.
static int init_chipset_hpt366(struct pci_dev *dev)
if (chip_type >= HPT370) {
if ((temp & 0xFFFFF000) != 0xABCDE000) {
it then falls through and gets a 33MHz answer from the tests.
If I stash the MHz answer from boot then it works. So I imagine the boot
time value just needs to be dumped into a static for resume to use.
Alan
On Thu, 19 Nov 2009 15:17:14 +0100
Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > > okay. ;)
> >
> > I think you have that backwards according to the vendor drivers. 3x2n
> > looks like it needs adjusting.
>
> lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..
It doesn't need one. It still sets the cable type in the pre reset
function as the older drivers that haven't converted to a cable_detect
method do.
Alan Cox wrote:
>> Point out the breakage please. The driver has been developed and tested
>>on non-x86 machine with 66 MHz PCI.
> static int init_chipset_hpt366(struct pci_dev *dev)
>
> if (chip_type >= HPT370) {
>
> if ((temp & 0xFFFFF000) != 0xABCDE000) {
> it then falls through and gets a 33MHz answer from the tests.
> If I stash the MHz answer from boot then it works. So I imagine the boot
> time value just needs to be dumped into a static for resume to use.
Ah, that... sure, this test only works at boot if there was no smart
enough BIOS around to save the f_CNT -- then the DPLL gets reprogrammed, and
doesn't reflect the default clock anymore...
> Alan
WBR, Sergei
On Thursday 19 November 2009 15:33:51 Alan Cox wrote:
> On Thu, 19 Nov 2009 15:17:14 +0100
> Bartlomiej Zolnierkiewicz <[email protected]> wrote:
>
> > On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > > > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > > > okay. ;)
> > >
> > > I think you have that backwards according to the vendor drivers. 3x2n
> > > looks like it needs adjusting.
> >
> > lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..
>
> It doesn't need one. It still sets the cable type in the pre reset
> function as the older drivers that haven't converted to a cable_detect
> method do.
If you know about other drivers still using ->pre_reset for cable detection
please let us know because they need fixing ASAP.
->cable_detect method is there for a reason, by knowingly not using it you
already have a buggy cable detection (since ->pre_reset ignores the mandatory
by spec part of cable detection which is probing slave before master) and
make it more likely to cause breakages in the future when some other developer
do a change under assumption that all host drivers use API correctly.
--
Bartlomiej Zolnierkiewicz
On Thursday 19 November 2009 15:50:25 Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 15:33:51 Alan Cox wrote:
> > On Thu, 19 Nov 2009 15:17:14 +0100
> > Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> >
> > > On Thursday 19 November 2009 14:16:36 Alan Cox wrote:
> > > > > Only pata_hpt37x has broken cable detection while pata_hpt3x2n is
> > > > > okay. ;)
> > > >
> > > > I think you have that backwards according to the vendor drivers. 3x2n
> > > > looks like it needs adjusting.
> > >
> > > lol, just fix the damn pata_hpt37x driver _lacking_ ->cable_detect method..
> >
> > It doesn't need one. It still sets the cable type in the pre reset
> > function as the older drivers that haven't converted to a cable_detect
> > method do.
>
> If you know about other drivers still using ->pre_reset for cable detection
> please let us know because they need fixing ASAP.
No need for it any longer. I'll send patches later.
--
Bartlomiej Zolnierkiewicz
> If you know about other drivers still using ->pre_reset for cable detection
> please let us know because they need fixing ASAP.
>
> ->cable_detect method is there for a reason,
Given that I added it I may know more about it than you do. In fact from
the rubbish you spout below it seems I do.
> already have a buggy cable detection (since ->pre_reset ignores the mandatory
Wrong.
> by spec part of cable detection which is probing slave before master) and
Have a free hint. If the host detects the cable type then we don't ask
the drive. See the standard if you don't understand why. Even if we
didn't the code would still be correct because we properly evaluate
the speed configuration from all the data sources.
> make it more likely to cause breakages in the future when some other developer
> do a change under assumption that all host drivers use API correctly.
To quote Bartlomiej "Talk to the maintainer" (or use grep).
Alan
> > If you know about other drivers still using ->pre_reset for cable detection
> > please let us know because they need fixing ASAP.
>
> No need for it any longer. I'll send patches later.
Works for me - not a bug fix as you make out but it's a clean up that is
worth doing.
Alan
On Thursday 19 November 2009 16:22:45 Alan Cox wrote:
> > If you know about other drivers still using ->pre_reset for cable detection
> > please let us know because they need fixing ASAP.
> >
> > ->cable_detect method is there for a reason,
>
> Given that I added it I may know more about it than you do. In fact from
> the rubbish you spout below it seems I do.
lol, I always heard that debugging other people's code is harder than
writing it in the first place..
> > already have a buggy cable detection (since ->pre_reset ignores the mandatory
>
> Wrong.
You cannot know it unless you know how chip operates internally. That's it.
You are taking chances that the controller does what most of similar hardware
do. Unfortunately we have seen so many counterexamples of this in the past
(i.e. I wouldn't be so surprised if some hosts just snoop IDENTIFY data to get
their cable info) that I prefer to stick to safe approaches.
Especially since it cost us nothing and provides additional benefit of having
coherent API.
> > by spec part of cable detection which is probing slave before master) and
>
> Have a free hint. If the host detects the cable type then we don't ask
> the drive. See the standard if you don't understand why. Even if we
> didn't the code would still be correct because we properly evaluate
> the speed configuration from all the data sources.
Please spare your 'free hints' and preaching tone. You've completely failed
over four years span to out do the messy code even with like ~1.5 year handicap
to finalize the hostile takeover.
I'm completely fed up with the process and I'm simply fixing up your mess now,
50+ patches and counting. Turns out to be order of magnitude more productive
than even trying to discuss things with you and/or your influential friends.
--
Bartlomiej Zolnierkiewicz
> > > already have a buggy cable detection (since ->pre_reset ignores the mandatory
> >
> > Wrong.
>
> You cannot know it unless you know how chip operates internally. That's it.
Read the code. How the chip operates is irrelevant.
> You are taking chances that the controller does what most of similar hardware
> do. Unfortunately we have seen so many counterexamples of this in the past
> (i.e. I wouldn't be so surprised if some hosts just snoop IDENTIFY data to get
> their cable info) that I prefer to stick to safe approaches.
I would be very suprised indeed because neither IDE stack nor the
reference drivers would work in that case.
Even then switch to cable_detect would make no difference. Read the code.
> > Have a free hint. If the host detects the cable type then we don't ask
> > the drive. See the standard if you don't understand why. Even if we
> > didn't the code would still be correct because we properly evaluate
> > the speed configuration from all the data sources.
>
> Please spare your 'free hints' and preaching tone.
If you don't know or follow the standards you'll get bugs. Thats why I
went to so much trouble researching and digging out the rules on cable
detect in detail, and fixing them in libata so that unlike the old IDE
stack they got them right (and more stuff probes correctly). (And Davem -
those did partly - drive ordering at least get pushed into old IDE too)
The reason it doesn't matter beyond style is this. Libata doesn't mangle
drive and host cable state into one thing. The ap->cbl field reflects the
cable type as measured by the controller. The forty wire decision is made
later on a combination of both controller and drive information, trusting
the controller first because the controller is the correct source if it
knows the information (as per the spec). [1]
Thus all the rubbish you spouted earlier doesn't matter at all. The
cable_detect logic was very carefully added so it didn't break back
compatibility with old drivers or re-order stuff wrongly.
For the other stuf "Talk to the maintainer" to quote yourself. I've
been working on other things for the past couple of years.
Alan
[1] The one specific exception here is the SATA cable setting when a
drive reports that it is SATA. There isn't an obvious way to de-uglify
that aspect of it.
On Thursday 19 November 2009 17:27:38 Alan Cox wrote:
> For the other stuf "Talk to the maintainer" to quote yourself. I've
> been working on other things for the past couple of years.
Indeed.
I wonder for how long we will be recovering from your help in TTY...
--
Bartlomiej Zolnierkiewicz
On Thursday 19 November 2009 17:27:38 Alan Cox wrote:
> > > > already have a buggy cable detection (since ->pre_reset ignores the mandatory
> > >
> > > Wrong.
> >
> > You cannot know it unless you know how chip operates internally. That's it.
>
> Read the code. How the chip operates is irrelevant.
lol, there is a genuine cable detection bug in pata_hpt3x2n on closer look...
Jeff, please apply the patch below.
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] pata_hpt3x2n: fix cable detection
The detection was reversed between primary and secondary ports.
Fix it to match hpt366 and the vendor driver.
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
drivers/ata/pata_hpt3x2n.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: b/drivers/ata/pata_hpt3x2n.c
===================================================================
--- a/drivers/ata/pata_hpt3x2n.c
+++ b/drivers/ata/pata_hpt3x2n.c
@@ -133,7 +133,7 @@ static int hpt3x2n_cable_detect(struct a
/* Restore state */
pci_write_config_byte(pdev, 0x5B, scr2);
- if (ata66 & (1 << ap->port_no))
+ if (ata66 & (2 >> ap->port_no))
return ATA_CBL_PATA40;
else
return ATA_CBL_PATA80;
On Thu, 19 Nov 2009 18:38:11 +0100
Bartlomiej Zolnierkiewicz <[email protected]> wrote:
> On Thursday 19 November 2009 17:27:38 Alan Cox wrote:
> > > > > already have a buggy cable detection (since ->pre_reset ignores the mandatory
> > > >
> > > > Wrong.
> > >
> > > You cannot know it unless you know how chip operates internally. That's it.
> >
> > Read the code. How the chip operates is irrelevant.
>
> lol, there is a genuine cable detection bug in pata_hpt3x2n on closer look...
Oh you found it now I told you which one was buggy - well done. Already
patched along with adding suspend/resume and fixing a performance funny.
On Thursday 19 November 2009 18:50:51 Alan Cox wrote:
> On Thu, 19 Nov 2009 18:38:11 +0100
> Bartlomiej Zolnierkiewicz <[email protected]> wrote:
>
> > On Thursday 19 November 2009 17:27:38 Alan Cox wrote:
> > > > > > already have a buggy cable detection (since ->pre_reset ignores the mandatory
> > > > >
> > > > > Wrong.
> > > >
> > > > You cannot know it unless you know how chip operates internally. That's it.
> > >
> > > Read the code. How the chip operates is irrelevant.
> >
> > lol, there is a genuine cable detection bug in pata_hpt3x2n on closer look...
>
> Oh you found it now I told you which one was buggy - well done. Already
> patched along with adding suspend/resume and fixing a performance funny.
LOL
Fixed where? I posted the patch as soon as I noticed the problem.
Told me about it? Now this is extra funny since the bug been there for like
three years and still would be unfixed if it wasn't for this discussion.
The best part is that the bug would never have happened in the first place
if you weren't artificially splitting HPT37x and HPT3xxN support cause cable
detection for all those chipsets (except HPT374 fn 1) is identical!! :)
--
Bartlomiej Zolnierkiewicz
> Fixed where? I posted the patch as soon as I noticed the problem.
Its not posted because unlike you I don't post patches as soon as I
notice them. I test them first. Which is why for example I discovered the
bug in the drivers/ide one. Did you check the vendor driver and then
stick 40 and 80 wire cables on the system to check the bits on a 3x2N ?
No I didn't think so. You see if you had you'd have discovered something
else. You'd have discovered another bug in the old IDE one. The driver
code for these chips isn't reliable and doesn't work at all in some cases.
> Told me about it?
Yes - or do you only write replies not read them ?
NAK - the patch is inadequate. The procedure in the vendor driver does
appear to work on the newer chips however. Probably worth double checking
the HPT37x and seeing if it needs the same debounce delays.
Give the patch below a test on your HPT3x2N series device. The PCI -> io
access change doesn't appear to matter but is used by the vendor driver.
I've switched to it because we've been burned before with only some of the
I/O space being visible both ways (eg on the 371N). The critical bit other
than get the bits the right way around appears to be the 10uS delay.
Alan
--
commit d27660f440b516f58385649f705b07f10b24da94
Author: Alan Cox <[email protected]>
Date: Thu Nov 19 17:59:26 2009 +0000
pata_hpt3x2n: Fix cable detection
The version inherited from drivers/ide doesn't work on the newer chipsets
at least not reliably. The vendors own driver uses a different process and
that one appears to produce plausible numbers.
Signed-off-by: Alan Cox <[email protected]>
diff --git a/drivers/ata/pata_hpt3x2n.c b/drivers/ata/pata_hpt3x2n.c
index 3d59fe0..d92ad5b 100644
--- a/drivers/ata/pata_hpt3x2n.c
+++ b/drivers/ata/pata_hpt3x2n.c
@@ -124,16 +124,15 @@ static u32 hpt3x2n_find_mode(struct ata_port *ap, int speed)
static int hpt3x2n_cable_detect(struct ata_port *ap)
{
u8 scr2, ata66;
- struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+ void __iomem *base = ap->ioaddr.bmdma_addr;
- pci_read_config_byte(pdev, 0x5B, &scr2);
- pci_write_config_byte(pdev, 0x5B, scr2 & ~0x01);
- /* Cable register now active */
- pci_read_config_byte(pdev, 0x5A, &ata66);
- /* Restore state */
- pci_write_config_byte(pdev, 0x5B, scr2);
+ scr2 = ioread8(base + 0x7B);
+ iowrite8(scr2 & ~0x01, base + 0x7B);
+ udelay(10); /* Debounce */
+ ata66 = ioread8(base + 0x7A);
+ iowrite8(scr2, base + 0x7B);
- if (ata66 & (1 << ap->port_no))
+ if (ata66 & (2 >> ap->port_no))
return ATA_CBL_PATA40;
else
return ATA_CBL_PATA80;
Alan Cox wrote:
>>Fixed where? I posted the patch as soon as I noticed the problem.
> Its not posted because unlike you I don't post patches as soon as I
> notice them. I test them first. Which is why for example I discovered the
> bug in the drivers/ide one. Did you check the vendor driver and then
> stick 40 and 80 wire cables on the system to check the bits on a 3x2N ?
> No I didn't think so. You see if you had you'd have discovered something
> else. You'd have discovered another bug in the old IDE one. The driver
> code for these chips isn't reliable and doesn't work at all in some cases.
>>Told me about it?
> Yes - or do you only write replies not read them ?
> NAK - the patch is inadequate.
No, it was. And yours isn't quite.
> The procedure in the vendor driver does
> appear to work on the newer chips however.
> Probably worth double checking
> the HPT37x and seeing if it needs the same debounce delays.
All vendor drivers I have do call StallExec(10) when detecting cable
type, so need to add the delay to pata_hpt37x too.
> Give the patch below a test on your HPT3x2N series device. The PCI -> io
> access change doesn't appear to matter but is used by the vendor driver.
We should then switch all the driver to I/O access across all the
driver, not just here.
> I've switched to it because we've been burned before with only some of the
> I/O space being visible both ways (eg on the 371N).
This is not the case anyway. The change is pointless.
> The critical bit other
> than get the bits the right way around appears to be the 10uS delay.
That's *another* issue.
BTW I did seem to *not* need the delay on HPT371N. Though worth retesting...
> Alan
> --
>
> commit d27660f440b516f58385649f705b07f10b24da94
> Author: Alan Cox <[email protected]>
> Date: Thu Nov 19 17:59:26 2009 +0000
>
> pata_hpt3x2n: Fix cable detection
>
> The version inherited from drivers/ide doesn't work on the newer chipsets
> at least not reliably.
Please don't call it inherited when you have a bug the drivers/ide/ code
doesn't have.
> The vendors own driver uses a different process and
> that one appears to produce plausible numbers.
I don't consider such description adequate. It doesn't mention the
original bug with reversed bits at all.
> Signed-off-by: Alan Cox <[email protected]>
>
> diff --git a/drivers/ata/pata_hpt3x2n.c b/drivers/ata/pata_hpt3x2n.c
> index 3d59fe0..d92ad5b 100644
> --- a/drivers/ata/pata_hpt3x2n.c
> +++ b/drivers/ata/pata_hpt3x2n.c
> @@ -124,16 +124,15 @@ static u32 hpt3x2n_find_mode(struct ata_port *ap, int speed)
> static int hpt3x2n_cable_detect(struct ata_port *ap)
> {
> u8 scr2, ata66;
> - struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> + void __iomem *base = ap->ioaddr.bmdma_addr;
>
> - pci_read_config_byte(pdev, 0x5B, &scr2);
> - pci_write_config_byte(pdev, 0x5B, scr2 & ~0x01);
> - /* Cable register now active */
> - pci_read_config_byte(pdev, 0x5A, &ata66);
> - /* Restore state */
> - pci_write_config_byte(pdev, 0x5B, scr2);
> + scr2 = ioread8(base + 0x7B);
> + iowrite8(scr2 & ~0x01, base + 0x7B);
> + udelay(10); /* Debounce */
> + ata66 = ioread8(base + 0x7A);
> + iowrite8(scr2, base + 0x7B);
>
> - if (ata66 & (1 << ap->port_no))
> + if (ata66 & (2 >> ap->port_no))
> return ATA_CBL_PATA40;
> else
> return ATA_CBL_PATA80;
I'd suggest to address the delay by another, separate patch to both
pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for
bit reversing...
WBR, Sergei
On Thursday 19 November 2009 19:21:32 Alan Cox wrote:
> > Fixed where? I posted the patch as soon as I noticed the problem.
>
> Its not posted because unlike you I don't post patches as soon as I
> notice them. I test them first. Which is why for example I discovered the
> bug in the drivers/ide one. Did you check the vendor driver and then
> stick 40 and 80 wire cables on the system to check the bits on a 3x2N ?
> No I didn't think so. You see if you had you'd have discovered something
I did check the vendor driver but I don't have 3x2N to test it so posting
patch ASAP to make it possible for other people to verify it was the best
course of action and completely justified. Don't you agree?
> else. You'd have discovered another bug in the old IDE one. The driver
You mean 'another' like yours _three_ year old 'one'? :)
> code for these chips isn't reliable and doesn't work at all in some cases.
>
> > Told me about it?
>
> Yes - or do you only write replies not read them ?
Well, yours have low SNR and I value my time..
> NAK - the patch is inadequate. The procedure in the vendor driver does
Patch is completely adequate in what it tries to achieve (fixing three year
old problem with testing the bit for the wrong port) and you could have made
an incremental fix for 'a second issue' easily.
However I'm not into NIH so your patch is also fine and a more complete one.
> pata_hpt3x2n: Fix cable detection
>
> The version inherited from drivers/ide doesn't work on the newer chipsets
> at least not reliably. The vendors own driver uses a different process and
> that one appears to produce plausible numbers.
Well, maybe except how you decided to 'skip' in the patch description
the part about how you have managed to introduce a regression three years
ago into already unreliable cable detection taken from hpt366.. ;)
--
Bartlomiej Zolnierkiewicz
On Thursday 19 November 2009 19:38:26 Sergei Shtylyov wrote:
> Alan Cox wrote:
>
> >>Fixed where? I posted the patch as soon as I noticed the problem.
>
> > Its not posted because unlike you I don't post patches as soon as I
> > notice them. I test them first. Which is why for example I discovered the
> > bug in the drivers/ide one. Did you check the vendor driver and then
> > stick 40 and 80 wire cables on the system to check the bits on a 3x2N ?
>
> > No I didn't think so. You see if you had you'd have discovered something
> > else. You'd have discovered another bug in the old IDE one. The driver
> > code for these chips isn't reliable and doesn't work at all in some cases.
>
> >>Told me about it?
>
> > Yes - or do you only write replies not read them ?
>
> > NAK - the patch is inadequate.
>
> No, it was. And yours isn't quite.
>
> > The procedure in the vendor driver does
> > appear to work on the newer chips however.
>
> > Probably worth double checking
> > the HPT37x and seeing if it needs the same debounce delays.
>
> All vendor drivers I have do call StallExec(10) when detecting cable
> type, so need to add the delay to pata_hpt37x too.
Given this I take back my ACK to Alan's patch.
> I'd suggest to address the delay by another, separate patch to both
> pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for
> bit reversing...
Yes.
--
Bartlomiej Zolnierkiewicz
On Thursday 19 November 2009 20:03:09 Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 19:38:26 Sergei Shtylyov wrote:
> > Alan Cox wrote:
> >
> > >>Fixed where? I posted the patch as soon as I noticed the problem.
> >
> > > Its not posted because unlike you I don't post patches as soon as I
> > > notice them. I test them first. Which is why for example I discovered the
> > > bug in the drivers/ide one. Did you check the vendor driver and then
> > > stick 40 and 80 wire cables on the system to check the bits on a 3x2N ?
> >
> > > No I didn't think so. You see if you had you'd have discovered something
> > > else. You'd have discovered another bug in the old IDE one. The driver
> > > code for these chips isn't reliable and doesn't work at all in some cases.
> >
> > >>Told me about it?
> >
> > > Yes - or do you only write replies not read them ?
> >
> > > NAK - the patch is inadequate.
> >
> > No, it was. And yours isn't quite.
> >
> > > The procedure in the vendor driver does
> > > appear to work on the newer chips however.
> >
> > > Probably worth double checking
> > > the HPT37x and seeing if it needs the same debounce delays.
> >
> > All vendor drivers I have do call StallExec(10) when detecting cable
> > type, so need to add the delay to pata_hpt37x too.
>
> Given this I take back my ACK to Alan's patch.
>
> > I'd suggest to address the delay by another, separate patch to both
> > pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for
> > bit reversing...
>
> Yes.
Jeff, this is incremental to my previous fix.
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] pata_hpt{37x,3x2n}: add debounce delay to cable detection methods
Alan Cox reported that cable detection sometimes works unreliably
for HPT3xxN and that the issue is fixed by adding debounce delay
as used by the vendor driver.
Sergei Shtylyov also noticed that debounce delay is needed for all
HPT37x and HPT3xxN chipsets according to vendor drivers.
Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
---
In comparison to original patch the conversion from PCI access to
io has been dropped as it is not required for the bugfix and makes
patch easier for back-porting into -stable kernels.
drivers/ata/pata_hpt37x.c | 3 +++
drivers/ata/pata_hpt3x2n.c | 3 +++
2 files changed, 6 insertions(+)
Index: b/drivers/ata/pata_hpt37x.c
===================================================================
--- a/drivers/ata/pata_hpt37x.c
+++ b/drivers/ata/pata_hpt37x.c
@@ -324,6 +324,9 @@ static int hpt37x_pre_reset(struct ata_l
pci_read_config_byte(pdev, 0x5B, &scr2);
pci_write_config_byte(pdev, 0x5B, scr2 & ~0x01);
+
+ udelay(10); /* debounce */
+
/* Cable register now active */
pci_read_config_byte(pdev, 0x5A, &ata66);
/* Restore state */
Index: b/drivers/ata/pata_hpt3x2n.c
===================================================================
--- a/drivers/ata/pata_hpt3x2n.c
+++ b/drivers/ata/pata_hpt3x2n.c
@@ -128,6 +128,9 @@ static int hpt3x2n_cable_detect(struct a
pci_read_config_byte(pdev, 0x5B, &scr2);
pci_write_config_byte(pdev, 0x5B, scr2 & ~0x01);
+
+ udelay(10); /* debounce */
+
/* Cable register now active */
pci_read_config_byte(pdev, 0x5A, &ata66);
/* Restore state */
> I'd suggest to address the delay by another, separate patch to both
> pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for
> bit reversing...
If you can confirm the 371N is fine without switching to I/O cycles then
I agree entirely with that and I'll push the udelay(10) into all three
plus the drivers/ide one unless Bart beats me to it.
Alan
> Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> ---
> In comparison to original patch the conversion from PCI access to
> io has been dropped as it is not required for the bugfix and makes
> patch easier for back-porting into -stable kernels.
Acked-by: Alan Cox <[email protected]>
(and from what Sergey says about the 371N then the I/O conversion
is probably not needed anyway)
On Thursday 19 November 2009 20:42:13 Alan Cox wrote:
> > Signed-off-by: Bartlomiej Zolnierkiewicz <[email protected]>
> > ---
> > In comparison to original patch the conversion from PCI access to
> > io has been dropped as it is not required for the bugfix and makes
> > patch easier for back-porting into -stable kernels.
>
> Acked-by: Alan Cox <[email protected]>
>
> (and from what Sergey says about the 371N then the I/O conversion
> is probably not needed anyway)
Thanks, I'll re-sync other pata_hpt37x patches on top of this one later.
ps if you could also push the fix to hpt366.c it would be great
--
Bartlomiej Zolnierkiewicz
Hello.
Alan Cox wrote:
>> I'd suggest to address the delay by another, separate patch to both
>> pata_hpt37x and pata_hpt3x2n drivers, and accept Bart's original patch for
>> bit reversing...
>>
>
> If you can confirm the 371N is fine without switching to I/O cycles then
>
HPT371N datasheet says that the SRC1/2 registers are dual-mapped to
PCI and I/O space. Are you paranoid enough to want me to test the
drivers on the board with HPT371N too? :-)
> I agree entirely with that and I'll push the udelay(10) into all three
>
I can't say anything about pata_hpt36x, I don't think I have HPT
drivers that old...
> plus the drivers/ide one unless Bart beats me to it.
>
I could beat you to it as well. :-)
> Alan
WBR, Sergei
On 11/19/2009 02:31 PM, Bartlomiej Zolnierkiewicz wrote:
> Jeff, this is incremental to my previous fix.
>
> From: Bartlomiej Zolnierkiewicz<[email protected]>
> Subject: [PATCH] pata_hpt{37x,3x2n}: add debounce delay to cable detection methods
PLEASE update the email subject, in future emails, to reflect a patch is
buried in here!
Thanks,
Jeff
On 11/18/2009 01:19 PM, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
>> Signed-off-by: Alan Cox<[email protected]>
>> ---
>>
>> drivers/ata/Kconfig | 8 ++++----
>> 1 files changed, 4 insertions(+), 4 deletions(-)
>>
>>
>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>> index f2df6e2..36931e0 100644
>> --- a/drivers/ata/Kconfig
>> +++ b/drivers/ata/Kconfig
>> @@ -374,7 +374,7 @@ config PATA_HPT366
>> If unsure, say N.
>>
>> config PATA_HPT37X
>> - tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
>> + tristate "HPT 370/370A/371/372/374/302 PATA support"
>> depends on PCI&& EXPERIMENTAL
>> help
>> This option enables support for the majority of the later HPT
>> @@ -383,7 +383,7 @@ config PATA_HPT37X
>> If unsure, say N.
>>
>> config PATA_HPT3X2N
>> - tristate "HPT 372N/302N PATA support (Experimental)"
>> + tristate "HPT 372N/302N PATA support"
>> depends on PCI&& EXPERIMENTAL
>> help
>> This option enables support for the N variant HPT PATA
>
> Maybe they are 'stable' but when it comes to features they are behind hpt366
> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> to understand and much smaller..
That sounds like an ACK to Alan's patch, to me ;-)
A libata driver can be stable, yet not have all the features of drivers/ide.
That said, I do agree that libata drivers need to have the features
found in the drivers/ide/ drivers.
Jeff
On 11/18/2009 02:56 PM, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
>> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
>>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
>>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
>>>> to understand and much smaller..
>>>
>>> 37x and 3xn lack PCI PM. Added to the TODO list.
>>>
>>> The smaller size is a bit questionable given its mostly comments, and
>>> three drivers versus one.
>>
>> I wonder what you find 'questionable' in the numbers given..
>
> BTW we can immediately reclaim 300 LOC by simply merging drivers back..
Merging drivers is a big pain for distributions and end users, even with
the new module_alias facility.
Absent stronger justification, libata remains with separate drivers,
even if that means some code duplication.
Jeff
On 11/17/2009 09:50 AM, Alan Cox wrote:
> Pending experimental bits. Not necessarily ready to apply but so folks know
> what is going on.
Seems sane, modulo my Kconfig comment, and on-going thread resolution of
course :)
Jeff
On Thursday 19 November 2009 22:36:50 Jeff Garzik wrote:
> On 11/18/2009 01:19 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
> >> Signed-off-by: Alan Cox<[email protected]>
> >> ---
> >>
> >> drivers/ata/Kconfig | 8 ++++----
> >> 1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >>
> >> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> >> index f2df6e2..36931e0 100644
> >> --- a/drivers/ata/Kconfig
> >> +++ b/drivers/ata/Kconfig
> >> @@ -374,7 +374,7 @@ config PATA_HPT366
> >> If unsure, say N.
> >>
> >> config PATA_HPT37X
> >> - tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
> >> + tristate "HPT 370/370A/371/372/374/302 PATA support"
> >> depends on PCI&& EXPERIMENTAL
> >> help
> >> This option enables support for the majority of the later HPT
> >> @@ -383,7 +383,7 @@ config PATA_HPT37X
> >> If unsure, say N.
> >>
> >> config PATA_HPT3X2N
> >> - tristate "HPT 372N/302N PATA support (Experimental)"
> >> + tristate "HPT 372N/302N PATA support"
> >> depends on PCI&& EXPERIMENTAL
> >> help
> >> This option enables support for the N variant HPT PATA
> >
> > Maybe they are 'stable' but when it comes to features they are behind hpt366
> > (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> > to understand and much smaller..
>
> That sounds like an ACK to Alan's patch, to me ;-)
>
> A libata driver can be stable, yet not have all the features of drivers/ide.
>
> That said, I do agree that libata drivers need to have the features
> found in the drivers/ide/ drivers.
Feel free to add them Mr. Maintainer. :)
--
Bartlomiej Zolnierkiewicz
On 11/19/2009 12:38 PM, Bartlomiej Zolnierkiewicz wrote:
>
> Index: b/drivers/ata/pata_hpt3x2n.c
> ===================================================================
> --- a/drivers/ata/pata_hpt3x2n.c
> +++ b/drivers/ata/pata_hpt3x2n.c
> @@ -133,7 +133,7 @@ static int hpt3x2n_cable_detect(struct a
> /* Restore state */
> pci_write_config_byte(pdev, 0x5B, scr2);
>
> - if (ata66& (1<< ap->port_no))
> + if (ata66& (2>> ap->port_no))
> return ATA_CBL_PATA40;
> else
> return ATA_CBL_PATA80;
Applied.
Note to Alan -- even if cable detect wants more changes, this is a nice,
self-contained fix easily backported to stable@, etc. So I preferred
the above form of change.
Please do send any updates you guys (Bart | Alan) have on top of
libata-dev.git#upstream.
Thanks,
Jeff
On 11/19/2009 02:31 PM, Bartlomiej Zolnierkiewicz wrote:
> Subject: [PATCH] pata_hpt{37x,3x2n}: add debounce delay to cable detection methods
>
> Alan Cox reported that cable detection sometimes works unreliably
> for HPT3xxN and that the issue is fixed by adding debounce delay
> as used by the vendor driver.
>
> Sergei Shtylyov also noticed that debounce delay is needed for all
> HPT37x and HPT3xxN chipsets according to vendor drivers.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz<[email protected]>
> ---
> In comparison to original patch the conversion from PCI access to
> io has been dropped as it is not required for the bugfix and makes
> patch easier for back-porting into -stable kernels.
>
> drivers/ata/pata_hpt37x.c | 3 +++
> drivers/ata/pata_hpt3x2n.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> Index: b/drivers/ata/pata_hpt37x.c
> ===================================================================
> --- a/drivers/ata/pata_hpt37x.c
> +++ b/drivers/ata/pata_hpt37x.c
> @@ -324,6 +324,9 @@ static int hpt37x_pre_reset(struct ata_l
>
> pci_read_config_byte(pdev, 0x5B,&scr2);
> pci_write_config_byte(pdev, 0x5B, scr2& ~0x01);
> +
> + udelay(10); /* debounce */
> +
> /* Cable register now active */
> pci_read_config_byte(pdev, 0x5A,&ata66);
> /* Restore state */
> Index: b/drivers/ata/pata_hpt3x2n.c
> ===================================================================
> --- a/drivers/ata/pata_hpt3x2n.c
> +++ b/drivers/ata/pata_hpt3x2n.c
> @@ -128,6 +128,9 @@ static int hpt3x2n_cable_detect(struct a
>
> pci_read_config_byte(pdev, 0x5B,&scr2);
> pci_write_config_byte(pdev, 0x5B, scr2& ~0x01);
> +
> + udelay(10); /* debounce */
> +
> /* Cable register now active */
> pci_read_config_byte(pdev, 0x5A,&ata66);
> /* Restore state */
applied -- same comment as last email, regarding hpt3x2n_cable_detect() fix
On Thursday 19 November 2009 22:38:39 Jeff Garzik wrote:
> On 11/18/2009 02:56 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
> >> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
> >>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
> >>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
> >>>> to understand and much smaller..
> >>>
> >>> 37x and 3xn lack PCI PM. Added to the TODO list.
> >>>
> >>> The smaller size is a bit questionable given its mostly comments, and
> >>> three drivers versus one.
> >>
> >> I wonder what you find 'questionable' in the numbers given..
> >
> > BTW we can immediately reclaim 300 LOC by simply merging drivers back..
>
> Merging drivers is a big pain for distributions and end users, even with
> the new module_alias facility.
Could please explain in more detail 'a big pain' part?
> Absent stronger justification, libata remains with separate drivers,
> even if that means some code duplication.
I can always hold my own tree with such changes if needed.
--
Bartlomiej Zolnierkiewicz
On 11/19/2009 04:42 PM, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 22:36:50 Jeff Garzik wrote:
>> On 11/18/2009 01:19 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Tuesday 17 November 2009 15:51:39 Alan Cox wrote:
>>>> Signed-off-by: Alan Cox<[email protected]>
>>>> ---
>>>>
>>>> drivers/ata/Kconfig | 8 ++++----
>>>> 1 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>>
>>>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
>>>> index f2df6e2..36931e0 100644
>>>> --- a/drivers/ata/Kconfig
>>>> +++ b/drivers/ata/Kconfig
>>>> @@ -374,7 +374,7 @@ config PATA_HPT366
>>>> If unsure, say N.
>>>>
>>>> config PATA_HPT37X
>>>> - tristate "HPT 370/370A/371/372/374/302 PATA support (Experimental)"
>>>> + tristate "HPT 370/370A/371/372/374/302 PATA support"
>>>> depends on PCI&& EXPERIMENTAL
>>>> help
>>>> This option enables support for the majority of the later HPT
>>>> @@ -383,7 +383,7 @@ config PATA_HPT37X
>>>> If unsure, say N.
>>>>
>>>> config PATA_HPT3X2N
>>>> - tristate "HPT 372N/302N PATA support (Experimental)"
>>>> + tristate "HPT 372N/302N PATA support"
>>>> depends on PCI&& EXPERIMENTAL
>>>> help
>>>> This option enables support for the N variant HPT PATA
>>>
>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
>>> to understand and much smaller..
>>
>> That sounds like an ACK to Alan's patch, to me ;-)
>>
>> A libata driver can be stable, yet not have all the features of drivers/ide.
>>
>> That said, I do agree that libata drivers need to have the features
>> found in the drivers/ide/ drivers.
>
> Feel free to add them Mr. Maintainer. :)
Oh, I'm happy to wait until one of two scenarios occurs:
1) Bart annoys Alan sufficiently to motivate Alan to add feature X
2) Alan annoys Bart sufficiently to motivate Bart to add feature X
Personally, I think my two PATA co-maintainers are doing an excellent
job of finding and killing bugs, and adding features, even if the
mailing list traffic is contentious.
I'm happy as long as the users win in the end...
Jeff
On 11/19/2009 04:49 PM, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 19 November 2009 22:38:39 Jeff Garzik wrote:
>> On 11/18/2009 02:56 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Wednesday 18 November 2009 20:07:07 Bartlomiej Zolnierkiewicz wrote:
>>>> On Wednesday 18 November 2009 19:41:25 Alan Cox wrote:
>>>>>> Maybe they are 'stable' but when it comes to features they are behind hpt366
>>>>>> (i.e. they lack PCI PM), which is also much cleaner than your drivers, easier
>>>>>> to understand and much smaller..
>>>>>
>>>>> 37x and 3xn lack PCI PM. Added to the TODO list.
>>>>>
>>>>> The smaller size is a bit questionable given its mostly comments, and
>>>>> three drivers versus one.
>>>>
>>>> I wonder what you find 'questionable' in the numbers given..
>>>
>>> BTW we can immediately reclaim 300 LOC by simply merging drivers back..
>>
>> Merging drivers is a big pain for distributions and end users, even with
>> the new module_alias facility.
>
> Could please explain in more detail 'a big pain' part?
It makes issue tracking (bug reports, feature requests, etc.) more
difficult. The "where did my driver go?" issue, which is largely a
communication/education not technical problem. Manually specifying
drivers in /etc/modprobe.conf remains supported.
>> Absent stronger justification, libata remains with separate drivers,
>> even if that means some code duplication.
>
> I can always hold my own tree with such changes if needed.
If it really bugs you, you could take the drivers/ata/sata_promise.h
approach, or do multi-file modules.
Jeff
> Oh, I'm happy to wait until one of two scenarios occurs:
>
> 1) Bart annoys Alan sufficiently to motivate Alan to add feature X
I'm rather hard to annoy but I'm happy Bart is going over the drivers.
Whatever his motivation I am very sure he'll find more bugs in the
process. He'll even be able to correct blame me for a few I am sure.
> 2) Alan annoys Bart sufficiently to motivate Bart to add feature X
Well latest score is Bart fixes libata bug, Alan fixes the IDE tree
version. Not quite the expected outcome but who cares ;)
Alan
> > Merging drivers is a big pain for distributions and end users, even with
> > the new module_alias facility.
>
> Could please explain in more detail 'a big pain' part?
It might confuse the Fedora intall a bit but it shouldn't cause much
confusion.
> > Absent stronger justification, libata remains with separate drivers,
> > even if that means some code duplication.
>
> I can always hold my own tree with such changes if needed.
My view is that they should be separate because of all the stuff like the
hpt3x2n clocking. I've got some updates to hpt3x2n pending which I'll
post as untested too.
If you are going to try merging them then the proof will be in what the
merge looks like. If you are going to do it anyway it's got to be worth
making that decision after the merge is done.
Alan
Hello, I wrote:
>> I agree entirely with that and I'll push the udelay(10) into all three
> I can't say anything about pata_hpt36x, I don't think I have HPT
> drivers that old...
No, actually I can. On HPT36x cable detection bit simply isn't
multiplexed, so there's no point in debouncing.
>> plus the drivers/ide one unless Bart beats me to it.
> I could beat you to it as well. :-)
Working in it right now...
>> Alan
WBR, Sergei
On Thursday 19 November 2009 11:03:29 pm Jeff Garzik wrote:
> On 11/19/2009 04:49 PM, Bartlomiej Zolnierkiewicz wrote:
> >> Absent stronger justification, libata remains with separate drivers,
> >> even if that means some code duplication.
> >
> > I can always hold my own tree with such changes if needed.
>
> If it really bugs you, you could take the drivers/ata/sata_promise.h
> approach, or do multi-file modules.
One of the main problems with separate drivers is a needless complexity
of the detection logic (different chips share same PCI IDs) and relevant
code parts presenting the issue were already posted in this thread.
PS It would bug me really if I were the author or the maintainer of said
code but since I'm not I'll get to fixing it only when I'm bored enough..
--
Bartlomiej Zolnierkiewicz
On Tuesday 17 November 2009 03:52:12 pm Alan Cox wrote:
> We were never able to get docs for this out of Toshiba for years. Dave
> Barnes produced a NetBSD driver however and from that we can fill in the
> needed tables
>
> Signed-off-by: Alan Cox <[email protected]>
> ---
>
> drivers/ata/Kconfig | 25 +++++---
> drivers/ata/Makefile | 1
> drivers/ata/ata_generic.c | 5 +-
> drivers/ata/pata_piccolo.c | 140 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci_ids.h | 7 +-
> 5 files changed, 166 insertions(+), 12 deletions(-)
> create mode 100644 drivers/ata/pata_piccolo.c
[...]
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
> @@ -168,9 +168,12 @@ static struct pci_device_id ata_generic[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C561), },
> { PCI_DEVICE(PCI_VENDOR_ID_OPTI, PCI_DEVICE_ID_OPTI_82C558), },
> { PCI_DEVICE(PCI_VENDOR_ID_CENATEK,PCI_DEVICE_ID_CENATEK_IDE), },
> +#if !defined(CONFIG_PATA_TOSHIBA) && !defined(CONFIG_PATA_TOSHIBA_MODULE)
> { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO), },
> - { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), },
> { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), },
> + { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_3), },
> + { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_5), },
> +#endif
[...]
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1496,9 +1496,10 @@
> #define PCI_DEVICE_ID_SBE_WANXL400 0x0104
>
> #define PCI_VENDOR_ID_TOSHIBA 0x1179
> -#define PCI_DEVICE_ID_TOSHIBA_PICCOLO 0x0102
> -#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_1 0x0103
> -#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_2 0x0105
> +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO 0x0101
> +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_2 0x0102
> +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_3 0x0103
> +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_5 0x0105
> #define PCI_DEVICE_ID_TOSHIBA_TOPIC95 0x060a
> #define PCI_DEVICE_ID_TOSHIBA_TOPIC97 0x060f
> #define PCI_DEVICE_ID_TOSHIBA_TOPIC100 0x0617
This adds kernel regression and breaks kernel build (it is generally good to
grep kernel tree for the existing users before doing changes like the above):
drivers/ide/ide-pci-generic.c:
{ PCI_VDEVICE(TOSHIBA, PCI_DEVICE_ID_TOSHIBA_PICCOLO), 4 },
{ PCI_VDEVICE(TOSHIBA, PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), 4 },
{ PCI_VDEVICE(TOSHIBA, PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), 4 },
Please fix your patch.
--
Bartlomiej Zolnierkiewicz
> > +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO 0x0101
> > +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_2 0x0102
> > +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_3 0x0103
> > +#define PCI_DEVICE_ID_TOSHIBA_PICCOLO_5 0x0105
> > #define PCI_DEVICE_ID_TOSHIBA_TOPIC95 0x060a
> > #define PCI_DEVICE_ID_TOSHIBA_TOPIC97 0x060f
> > #define PCI_DEVICE_ID_TOSHIBA_TOPIC100 0x0617
>
> This adds kernel regression and breaks kernel build (it is generally good to
> grep kernel tree for the existing users before doing changes like the above):
>
> drivers/ide/ide-pci-generic.c:
>
> { PCI_VDEVICE(TOSHIBA, PCI_DEVICE_ID_TOSHIBA_PICCOLO), 4 },
> { PCI_VDEVICE(TOSHIBA, PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), 4 },
> { PCI_VDEVICE(TOSHIBA, PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), 4 },
>
> Please fix your patch.
Will do - I'll add the missing ones to ide-generic as well.
Alan