pata_sch: fix PSTS register reading.
pata_sch: fix PSTS register reading.
According to Poulsbo SCH datasheet (p. 353): "Reading reserved bits returns an indeterminate, inconsistent value".
Bit 7 of PSTS register read as 1 when there is slave disk drive attached, despite being described as reserved with zero value. This causes wrong feature decoding in ata_sff_tf_read.
Fix it by implementing pata_sch specific sff_tf_read with appropriate bit masks on PSTS register.
drivers/ata/pata_sch.c 2010-01-20 11:19:16.000000000 +0200 renamed from drivers/ata/pata_sch.c 2010-01-13 07:15:00.000000000 +0200 +0 -0
@@ -49,6 +49,11 @@ USD = (1 << 31), /* Use Synchronous DMA */
};
+/* see SCH datasheet page 353 */
+enum {
+ PSTSM = 0x7f, /* PSTS Bit Mask */
+};
+
static int sch_init_one(struct pci_dev *pdev,
const struct pci_device_id *ent);
static void sch_set_piomode(struct ata_port *ap, struct ata_device *adev); @@ -75,11 +80,47 @@
ATA_BMDMA_SHT(DRV_NAME),
};
+/**
+ * sch_tf_read - input device's ATA taskfile shadow registers + * @ap: Port from which input is read + * @tf: ATA taskfile register set for storing input
+ *
+ * Note: Original code is ata_sff_tf_read().
+ */
+
+static void sch_tf_read(struct ata_port *ap, struct ata_taskfile *tf) +{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ tf->command = ata_sff_check_status(ap);
+ tf->feature = ioread8(ioaddr->error_addr) && PSTSM;
+ tf->nsect = ioread8(ioaddr->nsect_addr);
+ tf->lbal = ioread8(ioaddr->lbal_addr);
+ tf->lbam = ioread8(ioaddr->lbam_addr);
+ tf->lbah = ioread8(ioaddr->lbah_addr);
+ tf->device = ioread8(ioaddr->device_addr);
+
+ if (tf->flags & ATA_TFLAG_LBA48) {
+ if (likely(ioaddr->ctl_addr)) {
+ iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
+ tf->hob_feature = ioread8(ioaddr->error_addr) && PSTSM;
+ tf->hob_nsect = ioread8(ioaddr->nsect_addr);
+ tf->hob_lbal = ioread8(ioaddr->lbal_addr);
+ tf->hob_lbam = ioread8(ioaddr->lbam_addr);
+ tf->hob_lbah = ioread8(ioaddr->lbah_addr);
+ iowrite8(tf->ctl, ioaddr->ctl_addr);
+ ap->last_ctl = tf->ctl;+ } else
+ WARN_ON_ONCE(1);
+ }
+}
+
static struct ata_port_operations sch_pata_ops = {
.inherits = &ata_bmdma_port_ops,
.cable_detect = ata_cable_unknown,
.set_piomode = sch_set_piomode,
.set_dmamode = sch_set_dmamode,
+ .sff_tf_read = sch_tf_read,
};
static struct ata_port_info sch_port_info = {
O> According to Poulsbo SCH datasheet (p. 353): "Reading reserved bits returns
Quoted 1 line
an indeterminate, inconsistent value".
Ok but
Quoted 3 lines
Bit 7 of PSTS register read as 1 when there is slave disk drive attached, despite being described as reserved with zero value. This causes wrong feature decoding in
ata_sff_tf_read.
The feature/error register doesn't have reserved bits so your patch and diagnostic don't seem to make sense.
If the top bit is set in your error register post reset it is because you have a device flagging a fail
0x81 means the slave flagged a fail
0x80/82-FF means both devices flagged a fail
Hello.
Quoted 7 lines · Denis Turischev
pata_sch: fix PSTS register reading.According to Poulsbo SCH datasheet (p. 353): "Reading reserved bits returns an indeterminate, inconsistent value".
Bit 7 of PSTS register read as 1 when there is slave disk drive attached, despite being described as reserved with zero value. This causes wrong feature decoding inata_sff_tf_read.
ata_sff_tf_read() doesn't decode anything.
Quoted 2 lines
Fix it by implementing
pata_schspecificsff_tf_readwith appropriate bit masks on PSTS register.
Your patch doesn't make any sense to me -- sorry...
Quoted 27 lines
Signed-off-by Denis Turischev <[email protected]>---
linux-2.6.33-rc4.orig/drivers/ata/pata_sch.c2010-01-13
07:15:00.000000000 +0200
+++linux-2.6.33-rc4/drivers/ata/pata_sch.c2010-01-20
11:19:16.000000000 +0200(quoted diff) +9
-0@@ -49,6 +49,11 @@ USD = (1 << 31), /* Use Synchronous DMA */ }; +/* see SCH datasheet page 353 */ +enum { + PSTSM = 0x7f, /* PSTS Bit Mask */ +}; + static int sch_init_one(struct pci_dev *pdev, const struct pci_device_id *ent); static void sch_set_piomode(struct ata_port *ap, struct ata_device *adev);@@ -75,11 +80,47 @@ ATA_BMDMA_SHT(DRV_NAME), }; +/** + * sch_tf_read - input device's ATA taskfile shadow registers + * @ap: Port from which input is read + * @tf: ATA taskfile register set for storing input
Review line 96
Quoted 10 lines
(quoted diff) +10
-0+ * + * Note: Original code is ata_sff_tf_read(). + */ + +static void sch_tf_read(struct ata_port *ap, struct ata_taskfile *tf) +{ + struct ata_ioports *ioaddr = &ap->ioaddr; + + tf->command = ata_sff_check_status(ap); + tf->feature = ioread8(ioaddr->error_addr) && PSTSM;
1) you want a bitwise AND here (&);
2) what you're doing is wrong, you can't mask the features register
like that -- are you sure that *this* is the PSTS register?
Review line 104
Quoted 9 lines
(quoted diff) +8
-0+ tf->nsect = ioread8(ioaddr->nsect_addr); + tf->lbal = ioread8(ioaddr->lbal_addr); + tf->lbam = ioread8(ioaddr->lbam_addr); + tf->lbah = ioread8(ioaddr->lbah_addr); + tf->device = ioread8(ioaddr->device_addr); + + if (tf->flags & ATA_TFLAG_LBA48) { + if (likely(ioaddr->ctl_addr)) {
Useless check I think -- ioaddr->ctl_addr should always be non-zero
with pata_sch.
Review line 106
Quoted 2 lines
(quoted diff) +2
-0+ iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr); + tf->hob_feature = ioread8(ioaddr->error_addr) && PSTSM;
Same comments here.
Quoted 9 lines
(quoted diff) +9
-0+ tf->hob_nsect = ioread8(ioaddr->nsect_addr); + tf->hob_lbal = ioread8(ioaddr->lbal_addr); + tf->hob_lbam = ioread8(ioaddr->lbam_addr); + tf->hob_lbah = ioread8(ioaddr->lbah_addr); + iowrite8(tf->ctl, ioaddr->ctl_addr); + ap->last_ctl = tf->ctl; + } else + WARN_ON_ONCE(1); + }
Review line 125
Quoted 10 lines
(quoted diff) +3
-0+} + static struct ata_port_operations sch_pata_ops = { .inherits = &ata_bmdma_port_ops, .cable_detect = ata_cable_unknown, .set_piomode = sch_set_piomode, .set_dmamode = sch_set_dmamode, + .sff_tf_read = sch_tf_read, };
MBR, Sergei