Hello, Loc. On Mon, Feb 17, 2014 at 12:17:12AM -0700, Loc Ho wrote: ... > +static int xgene_ahci_get_channel(struct ata_host *host, struct ata_port *port) > +{ > + int i; > + for (i = 0; i < host->n_ports; i++) > + if (host->ports[i] == port) > + return i; > + return -1; > +} Didn't I ask you why you weren't using ap->port_no? > +/* > + * Custom Query ID command > + * > + * Due to HW errata, we must stop and re-start the port state machine after > + * read ID command. Also disable support for DEVSLP as hardware don't support > + * it. > + */ Please use /** function comment style. > +static unsigned int xgene_ahci_read_id(struct ata_device *dev, > + struct ata_taskfile *tf, u16 *id) > +{ > + u32 err_mask; > + void __iomem *port_mmio = ahci_port_base(dev->link->ap); > + > + err_mask = ata_do_dev_read_id(dev, tf, id); > + if (err_mask) > + return err_mask; > + > + /* Mask reserved area. Bit78 spec of Link Power Management Another review point which hasn't been addressed. Please use fully-winged comments for multiline comments. > + * bit15-8: reserved > + * bit7: NCQ autosence > + * bit6: Software settings preservation supported > + * bit5: reserved > + * bit4: In-order sata delivery supported > + * bit3: DIPM requests supported > + * bit2: DMA Setup FIS Auto-Activate optimization supported > + * bit1: DMA Setup FIX non-Zero buffer offsets supported > + * bit0: Reserved > + * > + * Clear reserved bit (DEVSLP bit) as we don't support DEVSLP > + */ > + id[78] &= 0x00FF; > + > + /* Restart the port if required due to HW errata */ > + if (!readl(port_mmio + PORT_CMD_ISSUE)) { > + writel(PORT_CMD_FIS_RX, port_mmio + PORT_CMD); > + readl(port_mmio + PORT_CMD); /* Force a barrier */ > + writel(PORT_CMD_FIS_RX | PORT_CMD_START, port_mmio + PORT_CMD); > + readl(port_mmio + PORT_CMD); /* Force a barrier */ > + } > + return 0; > +} ... > +static int xgene_ahci_do_hardreset(struct ata_link *link, > + unsigned long deadline, bool *online) > +{ ... > +hardreset_retry: > + /* clear D2H reception area to properly wait for D2H FIS */ > + ata_tf_init(link->device, &tf); > + tf.command = ATA_BUSY; > + ata_tf_to_fis(&tf, 0, 0, d2h_fis); > + rc = sata_link_hardreset(link, timing, deadline, online, > + ahci_check_ready); > + > + if (*online) { > + int chan = xgene_ahci_get_channel(ap->host, ap); > + > + /* Check to ensure that the disk comes up in matching speed */ > + if (first_time) { > + u32 gen_speed; > + > + first_time = 0; > + sata_scr_read(link, SCR_STATUS, &gen_speed); > + gen_speed = (gen_speed >> 4) & 0xf; > + if (gen_speed == 1 || gen_speed == 2) { > + /* For Gen2/1 and first time, let's check again > + * with Gen2/1 PHY to ensure actual Gen2/1 disk. > + */ Another review point not addressed. 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