On 09/12/2009 08:17 PM, Jung-Ik (John) Lee wrote:
On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock<hancockrwd@xxxxxxxxx> wrote:
On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote:
(snip)
Looks mostly reasonable to me, other than a few issues:
+static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
*adev)
+{
+ struct ata_device *peer = ata_dev_pair(adev);
+ struct atp867x_priv *dp = ap->private_data;
+ u8 speed = adev->pio_mode;
+ struct ata_timing t, p;
+ int T, UT;
+ u8 b;
+
+ T = 1000000000 / 33333;
+ UT = T/4;
+
+ switch (speed) {
+ case XFER_PIO_4:
+ case XFER_PIO_3:
+ case XFER_PIO_2:
+ case XFER_PIO_1:
+ case XFER_PIO_0:
+ case XFER_PIO_SLOW:
+ break;
+ default:
+ printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
+ " Default to XFER_PIO_0.\n", (unsigned)speed);
+ speed = XFER_PIO_0;
+ }
+
+ ata_timing_compute(adev, speed,&t, T, UT);
+ if (peer&& peer->pio_mode) {
+ ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
+ ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
+ }
+
+ b = inb(dp->dma_mode);
+ if (adev->devno& 1)
+ b = (b& ~ATP867X_IO_DMAMODE_SLAVE_MASK);
+ else
+ b = (b& ~ATP867X_IO_DMAMODE_MSTR_MASK);
+ outb(b, dp->dma_mode);
+
+#ifdef ATP867X_NO_HACK_PIOMODE
+ b = atp867x_get_active_clocks_shifted(t.active) |
+ atp867x_get_recover_clocks_shifted(t.recover);
+#else
+ /*
+ * magic value that works (from doc 6.4, 6.6.9)
+ */
+ b = 0x31;
+#endif
What's the purpose of this ifdef?
The magic value part must go. I'll update the patch.
+ if (dp->pci66mhz)
+ b += 0x10;
+
+ if (adev->devno& 1)
+ outb(b, dp->slave_piospd);
+ else
+ outb(b, dp->mstr_piospd);
+
+ /*
+ * use the same value for comand timing as for PIO timimg
+ */
+ outb(b, dp->eightb_piospd);
+}
+
+static int atp867x_cable_detect(struct ata_port *ap)
+{
+ return ATA_CBL_PATA40_SHORT;
+}
Doesn't the controller have a way to do proper 80-wire cable detection?
No programmatic way. libata.force should be used for other configurations.
Ideally you would use ATA_CBL_PATA_UNK / ata_cable_unknown, but it seems
that ATA_CBL_PATA_UNK has grown rather useless in the current kernel.
We should probably (a) update libata-core/libata-sff to do something
intelligent with ATA_CBL_PATA_UNK, and (b) use ATA_CBL_PATA_UNK in
pata_atp867x.
But yes, libata.force works as well. It just seems like we are losing
an opportunity to accept an existing BIOS-configured 80-wire cable
configuration by hardcoding ATA_CBL_PATA40_SHORT.
Also, a specific note -- atp867x_cable_detect() should be moved into
drivers/ata/libata-core.c and made generic. We have ata_cable_xxx
versions for everything except 40-short, it seems.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
