Hello, Shane.
I think it's in the right direction. Just a few nits.
Shane Huang wrote:
+static int ahci_sb600_check_ready(struct ata_link *link)
+{
+ void __iomem *port_mmio = ahci_port_base(link->ap);
+
Please kill this empty line inbetween variable declarations.
+ u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
+ u32 irq_status = readl(port_mmio + PORT_IRQ_STAT);
+
+ if (irq_status & PORT_IRQ_BAD_PMP)
+ return -EIO;
+
+ return ata_check_ready(status);
+}
+
+static int ahci_do_softreset(struct ata_link *link, unsigned int *class,
+ int pmp, unsigned long deadline,
+ int (*check_ready)(struct ata_link *link))
Why take @class if not used?
{
struct ata_port *ap = link->ap;
- int pmp = sata_srst_pmp(link);
const char *reason = NULL;
unsigned long now, msecs;
struct ata_taskfile tf;
@@ -1312,15 +1333,12 @@
ahci_exec_polled_cmd(ap, pmp, &tf, 0, 0, 0);
/* wait for link to become ready */
- rc = ata_wait_after_reset(link, deadline, ahci_check_ready);
+ rc = ata_wait_after_reset(link, deadline, check_ready);
/* link occupied, -ENODEV too is an error */
if (rc) {
reason = "device not ready";
goto fail;
}
- *class = ahci_dev_classify(ap);
-
- DPRINTK("EXIT, class=%u\n", *class);
Or maybe you can leave this part in ahci_do_softreset()?
+static int ahci_softreset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline)
+{
+ struct ata_port *ap = link->ap;
+ int pmp = sata_srst_pmp(link);
+ int rc;
+
+ DPRINTK("ENTER\n");
+
+ rc = ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
+ if (!rc) {
+ *class = ahci_dev_classify(ap);
+ DPRINTK("EXIT, class=%u\n", *class);
+ }
+ return rc;
+}
+
+static int ahci_sb600_softreset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline)
+{
+ struct ata_port *ap = link->ap;
+ void __iomem *port_mmio = ahci_port_base(ap);
+ int pmp = sata_srst_pmp(link);
+ int rc;
+ u32 irq_sts;
+
+ DPRINTK("ENTER\n");
+
+ rc = ahci_do_softreset(link, class, pmp, deadline,
+ ahci_sb600_check_ready);
+
+ /* Soft reset fails on some ATI chips with IPMS set when PMP
+ is enabled but SATA HDD/ODD is connected to SATA port,
+ do soft reset again to port 0. */
Please use
/* Blah... blah
* blah
*/
Or
/*
* Blah blah
* blah
*/
+ if (rc == -EIO) {
+ irq_sts = readl(port_mmio + PORT_IRQ_STAT);
+ if (irq_sts & PORT_IRQ_BAD_PMP) {
+ ata_link_printk(link, KERN_WARNING,
+ "softreset failed, try again\n");
Please use a bit more detailed message.
+ rc = ahci_do_softreset(link, class, 0, deadline,
+ ahci_check_ready);
+ }
+ }
+
+ if (!rc) {
+ *class = ahci_dev_classify(ap);
+ DPRINTK("EXIT, class=%u\n", *class);
+ }
+ return rc;
+}
I think it's better to organize functions in the following order.
ahci_do_softreset()
ahci_ready()
ahci_softreset()
ahci_sb600_ready()
ahci_sb600_softreset()
W/ explanation on how sb600_ready and sb600_softreset are different.
Please at least add why BAD_PMP quick exit path is necessary in
ahci_sb600_ready().
Thanks.
--
tejun
--
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
