Hello, On Mon, Jun 23, 2014 at 03:23:13PM +0530, Suman Tripathi wrote: > This patch fixes the dma state machine lockup due to the processing > of IDENTIFY DEVICE PIO mode command. The X-Gene AHCI controller > has an errata in which it cannot clear the BSY bit after > receiving the PIO setup FIS and results the dma state machine to go > into the CMFatalErrorUpdate state resulting in the dma state > machine lockup. This patch also removes the dma restart workaround > from the read_id function as the read_id function is only called by > libata layer for ATA_INTERNAL commands. But for somecases eg: > PORT MULTIPLIER and udev, the framework will enumerate using SCSI > commands and it will not call read_id function. I assume that it's verified that only IDENTIFY is affected? Please try to explicitly follow up on the previous discussions. It makes keeping track a lot easier for the reviewers. > /** > + * xgene_ahci_qc_issue - Issue commands to the device > + * @qc: Command to issue > + * > + * Due to H/W errata, for the IDENTIFY DEVICE command controller is unable to > + * clear the BSY bit after receiving the PIO setup FIS and results the dma > + * state machine to go into the CMFatalErrorUpdate state resulting in the dma > + * state machine lockup. By restarting the dma engine to move it removes the > + * controller out of lock up state. > + */ > +static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc) > +{ > + struct ata_port *ap = qc->ap; > + struct ahci_host_priv *hpriv = ap->host->private_data; > + struct xgene_ahci_context *ctx = hpriv->plat_data; > + int rc = 0; > + > + /* > + * Restart the dma engine if the last cmd issued > + * is IDENTIFY DEVICE command > + */ I think the function comment is doing a good job of explaining and we can lose the above comment. Comments which explain the code itself is useful if the logic involved is complex but I don't think that's the case here. > + if (unlikely(ctx->last_cmd[ap->port_no] == ATA_CMD_ID_ATA)) > + ahci_restart_engine(ap); > + > + rc = ahci_qc_issue(qc); > + > + /* Save the last command issued */ > + ctx->last_cmd[ap->port_no] = qc->tf.command; > + > + return rc; > +} ... > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index d1c9122..1b86cf4 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -68,7 +68,6 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state, > > static int ahci_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val); > static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val); > -static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc); > static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc); > static int ahci_port_start(struct ata_port *ap); > static void ahci_port_stop(struct ata_port *ap); > @@ -1952,7 +1951,7 @@ irqreturn_t ahci_interrupt(int irq, void *dev_instance) > } > EXPORT_SYMBOL_GPL(ahci_interrupt); > > -static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc) > +unsigned int ahci_qc_issue(struct ata_queued_cmd *qc) > { > struct ata_port *ap = qc->ap; > void __iomem *port_mmio = ahci_port_base(ap); > @@ -1981,6 +1980,7 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc) > > return 0; > } > +EXPORT_SYMBOL_GPL(ahci_qc_issue); Can we please do this in the previous patch? "implement ahci_restart_engine() and export ahci_qc_issue()" is completely fine with me. Just note in the description that a following patch will make use of them. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html