| Message ID | 1408819047-17224-4-git-send-email-stripathi@apm.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Aug 24, 2014 at 12:07:27AM +0530, Suman Tripathi wrote: > This patch addresses two HW erratas as described below by retrying the > COMRESET: > > 1. During speed negotiation, controller is not able to detect ALIGN > at GEN3(6Gbps) within 54.6us and results in a timeout. This issue can > be recovered by issuing a COMRESET. > > 2. Although ALIGN detection is successful, 8b/10b and disparity bit > errors can occur which result in the signature FIS not received > successfully by the Host controller. Due to this, the PHY communication > between the host and drive is not established because of CDR(clock and > data recovery) circuit doesn't lock. This issue can be recoverd by issuing > a COMRESET. > > The above retries are issued only if the port status register PXSTATUS > reports device presence detected but PHY communication not established. > The maximum retry attempts are 3. Didn't I ask you to update the comment to explain what's going on? Or is the existing comment already sufficient? Thanks.
On Tue, Aug 26, 2014 at 12:17:35PM +0530, Suman Tripathi wrote: > Didn't I ask you to update the comment to explain what's going on? > [suman] : can you specifically tell which part of the comment is not clear > and need more explanation? The comment on top of the function doesn't seem to match what's being implemented. In addition, it's generally not very useful to list the actual algorithm in text. Put algorithm in code and explain the summary and rationales for it in the comments. Nothing explains why the retries are being done. > is the existing comment already sufficient? > [suman] : The existing comment is sufficient . No, this isn't. You don't have to include a novel to explain it but there's something different going on here and you should provide information on why this sort of deviation is necessary.
diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c index fd9c137..894fa33 100644 --- a/drivers/ata/ahci_xgene.c +++ b/drivers/ata/ahci_xgene.c @@ -78,6 +78,9 @@ #define CFG_MEM_RAM_SHUTDOWN 0x00000070 #define BLOCK_MEM_RDY 0x00000074 +/* Max retry for link down */ +#define MAX_LINK_DOWN_RETRY 3 + struct xgene_ahci_context { struct ahci_host_priv *hpriv; struct device *dev; @@ -276,15 +279,23 @@ static int xgene_ahci_do_hardreset(struct ata_link *link, u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG; void __iomem *port_mmio = ahci_port_base(ap); struct ata_taskfile tf; + int link_down_retry = 0; int rc; - u32 val; - - /* 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, + u32 val, sstatus; + + do { + /* 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) + break; + + sata_scr_read(link, SCR_STATUS, &sstatus); + } while (link_down_retry++ < MAX_LINK_DOWN_RETRY && + (sstatus & 0xff) == 0x1); val = readl(port_mmio + PORT_SCR_ERR); if (val & (SERR_DISPARITY | SERR_10B_8B_ERR))