| Message ID | 1250570756.5207.15.camel@zm-desktop |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello, Shane. Sorry about the delay. Shane Huang wrote: > Implement SATA AHCI FIS-based switching support. The patch has been updated > after Tejun's review and suggestions to the 1st submission, which will not > modify libata anymore. Heh... nice that it's now contained in ahci.c proper. > @@ -286,6 +300,10 @@ struct ahci_port_priv { > unsigned int ncq_saw_dmas:1; > unsigned int ncq_saw_sdb:1; > u32 intr_mask; /* interrupts to enable */ > + bool fbs_supported; /* set iff FBS is supported */ > + bool fbs_enabled; /* set iff FBS is enabled */ > + int fbs_last_dev; /* save FBS.DEV of last FIS */ > + bool fbs_need_dec; /* need clear device error */ Why does fbs_need_dec need to be in ahci_port_priv? Can't it be a local variable of error_intr()? > /* enclosure management info per PM slot */ > struct ahci_em_priv em_priv[EM_MAX_SLOTS]; > }; > +static void ahci_fbs_dec_intr(struct ata_port *ap) > +{ > + struct ahci_port_priv *pp = ap->private_data; > + DPRINTK("ENTER\n"); > + > + if (pp->fbs_enabled) { Just do BUG_ON(!pp->fbs_enabled); > + void __iomem *port_mmio = ahci_port_base(ap); > + u32 fbs = readl(port_mmio + PORT_FBS); > + int retries = 3; > + > + /* time to wait for DEC is not specified by AHCI spec, > + * add a retry loop for safety */ > + do { > + writel(fbs | PORT_FBS_DEC, port_mmio + PORT_FBS); > + fbs = readl(port_mmio + PORT_FBS); > + retries--; > + } while ((fbs & PORT_FBS_DEC) && retries); Hmmm... shouldn't it be more like the following? writel(fbs | PORT_FBS_DEC, port_mmio + PORT_FBS); fbs = readl(port_mmio + PORT_FBS); while ((fbs & PORT_FBS_DEC) && retries--) { udelay(1); fbs = readl(port_mmio + PORT_FBS); } > + if (fbs & PORT_FBS_DEC) > + dev_printk(KERN_ERR, ap->host->dev, > + "failed to clear device error\n"); > + } else > + dev_printk(KERN_ERR, ap->host->dev, > + "FBS is disabled, no need to clear device error\n"); > +} > + > static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) > { > struct ahci_host_priv *hpriv = ap->host->private_data; > @@ -1984,10 +2042,22 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) > struct ata_eh_info *active_ehi; > u32 serror; > > - /* determine active link */ > - ata_for_each_link(link, ap, EDGE) > - if (ata_link_active(link)) > - break; > + /* determine active link with error */ > + if (pp->fbs_enabled) { > + void __iomem *port_mmio = ahci_port_base(ap); > + u32 fbs = readl(port_mmio + PORT_FBS); > + > + ata_for_each_link(link, ap, EDGE) > + if (ata_link_active(link) && (fbs & PORT_FBS_SDE) && > + (link->pmp == (fbs >> PORT_FBS_DWE_OFFSET))) { > + pp->fbs_need_dec = true; > + break; > + } You can do pmp = fbs >> PORT_FBS_DWE_OFFSET; if (pmp < ap->nr_pmp_links && ata_link_online(&ap->pmp_link[pmp])) { link = &ap->pmp_link[pmp]; pp->fbs_need_dec = true; } > + } else > + ata_for_each_link(link, ap, EDGE) > + if (ata_link_active(link)) > + break; > + > if (!link) > link = &ap->link; > > @@ -2044,8 +2114,13 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) > } > > if (irq_stat & PORT_IRQ_IF_ERR) { > - host_ehi->err_mask |= AC_ERR_ATA_BUS; > - host_ehi->action |= ATA_EH_RESET; > + if (pp->fbs_need_dec) > + active_ehi->err_mask |= AC_ERR_DEV; > + else { > + host_ehi->err_mask |= AC_ERR_ATA_BUS; > + host_ehi->action |= ATA_EH_RESET; > + } > + Are you sure IF_ERR is device specific and doesn't require host link reset? > @@ -2061,7 +2136,12 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) > if (irq_stat & PORT_IRQ_FREEZE) > ata_port_freeze(ap); > else > - ata_port_abort(ap); > + if (pp->fbs_enabled && pp->fbs_need_dec && else if (pp->fbs_need_dec) { should be enough, right? > + !ata_is_host_link(link)) { > + ata_link_abort(link); > + ahci_fbs_dec_intr(ap); > + } else > + ata_port_abort(ap); > } > > static void ahci_port_intr(struct ata_port *ap) > @@ -2113,12 +2193,19 @@ static void ahci_port_intr(struct ata_port *ap) > /* If the 'N' bit in word 0 of the FIS is set, > * we just received asynchronous notification. > * Tell libata about it. > + * > + * Lack of SNotification should not appear in > + * ahci 1.2, so the workaround is unnecessary > + * when FBS is enabled. > */ > - const __le32 *f = pp->rx_fis + RX_FIS_SDB; > - u32 f0 = le32_to_cpu(f[0]); > - > - if (f0 & (1 << 15)) > - sata_async_notification(ap); > + if (pp->fbs_enabled) > + WARN_ON(1); > + else { > + const __le32 *f = pp->rx_fis + RX_FIS_SDB; > + u32 f0 = le32_to_cpu(f[0]); > + if (f0 & (1 << 15)) > + sata_async_notification(ap); > + } > } > } > > @@ -2212,6 +2299,17 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc) > > if (qc->tf.protocol == ATA_PROT_NCQ) > writel(1 << qc->tag, port_mmio + PORT_SCR_ACT); > + > + if (pp->fbs_enabled) { > + if (pp->fbs_last_dev != qc->dev->link->pmp) { > + u32 fbs = readl(port_mmio + PORT_FBS); > + fbs &= ~(PORT_FBS_DEV_MASK | PORT_FBS_DEC); > + fbs |= qc->dev->link->pmp << PORT_FBS_DEV_OFFSET; > + writel(fbs, port_mmio + PORT_FBS); > + pp->fbs_last_dev = qc->dev->link->pmp; > + } > + } > + > writel(1 << qc->tag, port_mmio + PORT_CMD_ISSUE); > > ahci_sw_activity(qc->dev->link); > @@ -2224,6 +2322,9 @@ static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > struct ahci_port_priv *pp = qc->ap->private_data; > u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG; > > + if (pp->fbs_enabled) > + d2h_fis += (qc->dev->link->pmp) * AHCI_RX_FIS_SZ; > + > ata_tf_from_fis(d2h_fis, &qc->result_tf); > return true; > } > @@ -2272,6 +2373,77 @@ static void ahci_post_internal_cmd(struct ata_queued_cmd *qc) > ahci_kick_engine(ap); > } > > +static int ahci_enable_fbs(struct ata_port *ap) > +{ > + struct ahci_port_priv *pp = ap->private_data; > + > + if (pp->fbs_supported && !pp->fbs_enabled) { > + void __iomem *port_mmio = ahci_port_base(ap); > + u32 fbs; > + int rc = ahci_stop_engine(ap); > + if (rc) > + return rc; > + > + fbs = readl(port_mmio + PORT_FBS); > + writel(fbs | PORT_FBS_EN, port_mmio + PORT_FBS); > + fbs = readl(port_mmio + PORT_FBS); > + if (fbs & PORT_FBS_EN) { > + dev_printk(KERN_INFO, ap->host->dev, > + "FBS is enabled.\n"); > + pp->fbs_enabled = true; > + pp->fbs_last_dev = -1; /* initialization */ > + } else { > + dev_printk(KERN_ERR, ap->host->dev, > + "Failed to enable FBS\n"); > + ahci_start_engine(ap); > + return -EIO; > + } > + > + ahci_start_engine(ap); > + } else { > + dev_printk(KERN_ERR, ap->host->dev, > + "FBS is not supported or already enabled\n"); > + return -EINVAL; The above message will be printed on every !FBS ahcis, right? It would be better to do the following at the top of the function. if (!pp->fbs_supported) return; WARN_ON(pp->fbs_enabled); and drop the if/else. > +static int ahci_disable_fbs(struct ata_port *ap) > +{ > + struct ahci_port_priv *pp = ap->private_data; > + > + if (pp->fbs_enabled) { > + void __iomem *port_mmio = ahci_port_base(ap); > + u32 fbs; > + int rc = ahci_stop_engine(ap); > + if (rc) > + return rc; > + > + fbs = readl(port_mmio + PORT_FBS); > + writel(fbs & ~PORT_FBS_EN, port_mmio + PORT_FBS); > + fbs = readl(port_mmio + PORT_FBS); > + if (fbs & PORT_FBS_EN) { > + dev_printk(KERN_ERR, ap->host->dev, > + "Failed to disable FBS\n"); > + ahci_start_engine(ap); > + return -EIO; > + } else { > + dev_printk(KERN_INFO, ap->host->dev, > + "FBS is disabled.\n"); > + pp->fbs_enabled = false; > + } > + > + ahci_start_engine(ap); > + } else if (sata_pmp_attached(ap)) { > + dev_printk(KERN_ERR, ap->host->dev, > + "FBS is not supported or already disabled\n"); > + return -EINVAL; > + } Ditto. Just drop sanity checks. Upper layer should take care of them. No need to check that this deep in the stack. > static int ahci_port_start(struct ata_port *ap) > { > + struct ahci_host_priv *hpriv = ap->host->private_data; > struct device *dev = ap->host->dev; > struct ahci_port_priv *pp; > void *mem; > dma_addr_t mem_dma; > + size_t dma_sz, rx_fis_sz; > > pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); > if (!pp) > return -ENOMEM; > > - mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma, > - GFP_KERNEL); > + /* check FBS capability */ > + if ((hpriv->cap & HOST_CAP_FBS) && sata_pmp_supported(ap)) { > + void __iomem *port_mmio = ahci_port_base(ap); > + u32 cmd = readl(port_mmio + PORT_CMD); > + if (cmd & PORT_CMD_FBSCP) > + pp->fbs_supported = true; Maybe whine a bit if CAP indicates FBS but PORT_CMD doesn't? Other than the above, looks great to me. Thanks a lot.
Hi Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > > Why does fbs_need_dec need to be in ahci_port_priv? Can't it be a > local variable of error_intr()? Yes, it should be replaced by one local variable. > > +static void ahci_fbs_dec_intr(struct ata_port *ap) > > +{ > > + struct ahci_port_priv *pp = ap->private_data; > > + DPRINTK("ENTER\n"); > > + > > + if (pp->fbs_enabled) { > > Just do BUG_ON(!pp->fbs_enabled); OK. > > + void __iomem *port_mmio = ahci_port_base(ap); > > + u32 fbs = readl(port_mmio + PORT_FBS); > > + int retries = 3; > > + > > + /* time to wait for DEC is not specified by AHCI spec, > > + * add a retry loop for safety */ > > + do { > > + writel(fbs | PORT_FBS_DEC, port_mmio + > PORT_FBS); > > + fbs = readl(port_mmio + PORT_FBS); > > + retries--; > > + } while ((fbs & PORT_FBS_DEC) && retries); > > Hmmm... shouldn't it be more like the following? > > writel(fbs | PORT_FBS_DEC, port_mmio + PORT_FBS); > fbs = readl(port_mmio + PORT_FBS); > while ((fbs & PORT_FBS_DEC) && retries--) { > udelay(1); > fbs = readl(port_mmio + PORT_FBS); > } OK. > > @@ -1984,10 +2042,22 @@ static void ahci_error_intr(struct > ata_port *ap, u32 irq_stat) > > struct ata_eh_info *active_ehi; > > u32 serror; > > > > - /* determine active link */ > > - ata_for_each_link(link, ap, EDGE) > > - if (ata_link_active(link)) > > - break; > > + /* determine active link with error */ > > + if (pp->fbs_enabled) { > > + void __iomem *port_mmio = ahci_port_base(ap); > > + u32 fbs = readl(port_mmio + PORT_FBS); > > + > > + ata_for_each_link(link, ap, EDGE) > > + if (ata_link_active(link) && (fbs & > PORT_FBS_SDE) && > > + (link->pmp == (fbs >> > PORT_FBS_DWE_OFFSET))) { > > + pp->fbs_need_dec = true; > > + break; > > + } > > You can do > > pmp = fbs >> PORT_FBS_DWE_OFFSET; > if (pmp < ap->nr_pmp_links && > ata_link_online(&ap->pmp_link[pmp])) { > link = &ap->pmp_link[pmp]; > pp->fbs_need_dec = true; > } PORT_FBS_SDE also need check, because(ahci v1.2 3.3.16): Device With Error (DWE): Set by hardware to the value of the Port Multiplier port number of the device that experienced a fatal error condition. This field is only valid when PxFBS.SDE = '1'. So I'll update the code into: u32 fbs = readl(port_mmio + PORT_FBS); int pmp = fbs >> PORT_FBS_DWE_OFFSET; if ((fbs & PORT_FBS_SDE) && (pmp < ap->nr_pmp_links) && ata_link_online(&ap->pmp_link[pmp])) { link = &ap->pmp_link[pmp]; fbs_need_dec = true; } > > if (irq_stat & PORT_IRQ_IF_ERR) { > > - host_ehi->err_mask |= AC_ERR_ATA_BUS; > > - host_ehi->action |= ATA_EH_RESET; > > + if (pp->fbs_need_dec) > > + active_ehi->err_mask |= AC_ERR_DEV; > > + else { > > + host_ehi->err_mask |= AC_ERR_ATA_BUS; > > + host_ehi->action |= ATA_EH_RESET; > > + } > > + > > Are you sure IF_ERR is device specific and doesn't require host link > reset? Because I referred to the AHCI spec v1.2 9.3.6: An interface fatal error may be localized to a particular device or may be fatal to the entire port.....If the error is fatal to the entire port, then PxFBS.SDE shall be cleared to '0' by the hardware. Software should follow either the device specific or non-device specific error procedure depending on whether PxFBS.SDE is set to '1'. > > @@ -2061,7 +2136,12 @@ static void ahci_error_intr(struct > ata_port *ap, u32 irq_stat) > > if (irq_stat & PORT_IRQ_FREEZE) > > ata_port_freeze(ap); > > else > > - ata_port_abort(ap); > > + if (pp->fbs_enabled && pp->fbs_need_dec && > > else if (pp->fbs_need_dec) { > > should be enough, right? Yes, I'll update that. > > + } else { > > + dev_printk(KERN_ERR, ap->host->dev, > > + "FBS is not supported or already enabled\n"); > > + return -EINVAL; > > The above message will be printed on every !FBS ahcis, right? It > would be better to do the following at the top of the function. > > if (!pp->fbs_supported) > return; > WARN_ON(pp->fbs_enabled); > > and drop the if/else. OK, updated as below, and returns "void" instead of "int": static void ahci_enable_fbs(struct ata_port *ap) { struct ahci_port_priv *pp = ap->private_data; void __iomem *port_mmio = ahci_port_base(ap); u32 fbs; int rc; if (!pp->fbs_supported) return; WARN_ON(pp->fbs_enabled); rc = ahci_stop_engine(ap); if (rc) return; fbs = readl(port_mmio + PORT_FBS); writel(fbs | PORT_FBS_EN, port_mmio + PORT_FBS); fbs = readl(port_mmio + PORT_FBS); if (fbs & PORT_FBS_EN) { dev_printk(KERN_INFO, ap->host->dev, "FBS is enabled.\n"); pp->fbs_enabled = true; pp->fbs_last_dev = -1; /* initialization */ } else dev_printk(KERN_ERR, ap->host->dev, "Failed to enable FBS\n"); ahci_start_engine(ap); } > > +static int ahci_disable_fbs(struct ata_port *ap) > > +{ > > + struct ahci_port_priv *pp = ap->private_data; > > + > > + if (pp->fbs_enabled) { > > + void __iomem *port_mmio = ahci_port_base(ap); > > + u32 fbs; > > + int rc = ahci_stop_engine(ap); > > + if (rc) > > + return rc; > > + > > + fbs = readl(port_mmio + PORT_FBS); > > + writel(fbs & ~PORT_FBS_EN, port_mmio + PORT_FBS); > > + fbs = readl(port_mmio + PORT_FBS); > > + if (fbs & PORT_FBS_EN) { > > + dev_printk(KERN_ERR, ap->host->dev, > > + "Failed to disable FBS\n"); > > + ahci_start_engine(ap); > > + return -EIO; > > + } else { > > + dev_printk(KERN_INFO, ap->host->dev, > > + "FBS is disabled.\n"); > > + pp->fbs_enabled = false; > > + } > > + > > + ahci_start_engine(ap); > > + } else if (sata_pmp_attached(ap)) { > > + dev_printk(KERN_ERR, ap->host->dev, > > + "FBS is not supported or already > disabled\n"); > > + return -EINVAL; > > + } > > Ditto. Just drop sanity checks. Upper layer should take > care of them. > No need to check that this deep in the stack. static void ahci_disable_fbs(struct ata_port *ap) { struct ahci_port_priv *pp = ap->private_data; void __iomem *port_mmio = ahci_port_base(ap); u32 fbs; int rc; if (!pp->fbs_supported) return; WARN_ON(!pp->fbs_enabled); rc = ahci_stop_engine(ap); if (rc) return; fbs = readl(port_mmio + PORT_FBS); writel(fbs & ~PORT_FBS_EN, port_mmio + PORT_FBS); fbs = readl(port_mmio + PORT_FBS); if (fbs & PORT_FBS_EN) dev_printk(KERN_ERR, ap->host->dev, "Failed to disable FBS\n"); else { dev_printk(KERN_INFO, ap->host->dev, "FBS is disabled.\n"); pp->fbs_enabled = false; } ahci_start_engine(ap); } > > static int ahci_port_start(struct ata_port *ap) > > { > > + struct ahci_host_priv *hpriv = ap->host->private_data; > > struct device *dev = ap->host->dev; > > struct ahci_port_priv *pp; > > void *mem; > > dma_addr_t mem_dma; > > + size_t dma_sz, rx_fis_sz; > > > > pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); > > if (!pp) > > return -ENOMEM; > > > > - mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma, > > - GFP_KERNEL); > > + /* check FBS capability */ > > + if ((hpriv->cap & HOST_CAP_FBS) && sata_pmp_supported(ap)) { > > + void __iomem *port_mmio = ahci_port_base(ap); > > + u32 cmd = readl(port_mmio + PORT_CMD); > > + if (cmd & PORT_CMD_FBSCP) > > + pp->fbs_supported = true; > > Maybe whine a bit if CAP indicates FBS but PORT_CMD doesn't? Sure, updated as below: if (cmd & PORT_CMD_FBSCP) pp->fbs_supported = true; else WARN_ON(1); > Other than the above, looks great to me. OK, thanks for your nice suggestions, I'll have to submit v3 later... Best Regards, Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Shane. Huang, Shane wrote: >> You can do >> >> pmp = fbs >> PORT_FBS_DWE_OFFSET; >> if (pmp < ap->nr_pmp_links && >> ata_link_online(&ap->pmp_link[pmp])) { >> link = &ap->pmp_link[pmp]; >> pp->fbs_need_dec = true; >> } > > PORT_FBS_SDE also need check, because(ahci v1.2 3.3.16): > Device With Error (DWE): Set by hardware to the value of the > Port Multiplier port number of the device that experienced a fatal > error condition. This field is only valid when PxFBS.SDE = '1'. > > So I'll update the code into: > u32 fbs = readl(port_mmio + PORT_FBS); > int pmp = fbs >> PORT_FBS_DWE_OFFSET; > > if ((fbs & PORT_FBS_SDE) && (pmp < ap->nr_pmp_links) && > ata_link_online(&ap->pmp_link[pmp])) { > link = &ap->pmp_link[pmp]; > fbs_need_dec = true; > } Yeap, I missed the condition while trying to point out that the loop wasn't necessary there. Sorry. :-P >>> if (irq_stat & PORT_IRQ_IF_ERR) { >>> - host_ehi->err_mask |= AC_ERR_ATA_BUS; >>> - host_ehi->action |= ATA_EH_RESET; >>> + if (pp->fbs_need_dec) >>> + active_ehi->err_mask |= AC_ERR_DEV; >>> + else { >>> + host_ehi->err_mask |= AC_ERR_ATA_BUS; >>> + host_ehi->action |= ATA_EH_RESET; >>> + } >>> + >> Are you sure IF_ERR is device specific and doesn't require host link >> reset? > > Because I referred to the AHCI spec v1.2 9.3.6: > An interface fatal error may be localized to a particular device or may > be fatal to the entire port.....If the error is fatal to the entire > port, then > PxFBS.SDE shall be cleared to '0' by the hardware. Software should > follow either the device specific or non-device specific error procedure > depending on whether PxFBS.SDE is set to '1'. Ah hah... thanks for the explanation. >>> - mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma, >>> - GFP_KERNEL); >>> + /* check FBS capability */ >>> + if ((hpriv->cap & HOST_CAP_FBS) && sata_pmp_supported(ap)) { >>> + void __iomem *port_mmio = ahci_port_base(ap); >>> + u32 cmd = readl(port_mmio + PORT_CMD); >>> + if (cmd & PORT_CMD_FBSCP) >>> + pp->fbs_supported = true; >> Maybe whine a bit if CAP indicates FBS but PORT_CMD doesn't? > > Sure, updated as below: > if (cmd & PORT_CMD_FBSCP) > pp->fbs_supported = true; > else > WARN_ON(1); WARN_ON() would be a tad bit too scary. Given that on certain hardwares it would always trigger. A dev_printk() would be better. > OK, thanks for your nice suggestions, I'll have to submit v3 later... Nice work! Thanks a lot for doing it.
Hi Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > >>> + /* check FBS capability */ > >>> + if ((hpriv->cap & HOST_CAP_FBS) && sata_pmp_supported(ap)) { > >>> + void __iomem *port_mmio = ahci_port_base(ap); > >>> + u32 cmd = readl(port_mmio + PORT_CMD); > >>> + if (cmd & PORT_CMD_FBSCP) > >>> + pp->fbs_supported = true; > >> Maybe whine a bit if CAP indicates FBS but PORT_CMD doesn't? > > > > Sure, updated as below: > > if (cmd & PORT_CMD_FBSCP) > > pp->fbs_supported = true; > > else > > WARN_ON(1); > > WARN_ON() would be a tad bit too scary. Given that on certain > hardwares it would always trigger. A dev_printk() would be better. Well..., then: if (cmd & PORT_CMD_FBSCP) pp->fbs_supported = true; else dev_printk(KERN_WARNING, ap->host->dev, "The port is not capable of FBS\n"); Quoting myself: > static void ahci_disable_fbs(struct ata_port *ap) > { > struct ahci_port_priv *pp = ap->private_data; > void __iomem *port_mmio = ahci_port_base(ap); > u32 fbs; > int rc; > > if (!pp->fbs_supported) > return; > > WARN_ON(!pp->fbs_enabled); > > rc = ahci_stop_engine(ap); I find that ahci_pmp_detach() will be called for each SATA port during the initialization, right after print: > ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp fbs... so will ahci_disable_fbs() be called, which leads to the trigger of WARN_ON(). I'll also add the condition sata_pmp_attached() as below: static void ahci_disable_fbs(struct ata_port *ap) { struct ahci_port_priv *pp = ap->private_data; void __iomem *port_mmio = ahci_port_base(ap); u32 fbs; int rc; if (!pp->fbs_supported || !sata_pmp_attached(ap)) return; WARN_ON(!pp->fbs_enabled); rc = ahci_stop_engine(ap); Thanks, Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Shane. Huang, Shane wrote: > Quoting myself: >> static void ahci_disable_fbs(struct ata_port *ap) >> { >> struct ahci_port_priv *pp = ap->private_data; >> void __iomem *port_mmio = ahci_port_base(ap); >> u32 fbs; >> int rc; >> >> if (!pp->fbs_supported) >> return; >> >> WARN_ON(!pp->fbs_enabled); >> >> rc = ahci_stop_engine(ap); > > I find that ahci_pmp_detach() will be called for each SATA port > during the initialization, right after print: >> ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp fbs... > > so will ahci_disable_fbs() be called, which leads to the trigger > of WARN_ON(). Ah.. right. ahci_port_resume() does that to make sure that all PMP bits are cleared on init. Hmmm... probably it would be better to make ahci_disable_fbs() to just do it regardless of libata thinks whether PMP is attached or not. After resume from STR, we shouldn't be assuming anything about the controller state. Thanks.
Hi Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > > > I find that ahci_pmp_detach() will be called for each SATA port > > during the initialization, right after print: > >> ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp fbs... > > > > so will ahci_disable_fbs() be called, which leads to the trigger > > of WARN_ON(). > > Ah.. right. ahci_port_resume() does that to make sure that all PMP > bits are cleared on init. Hmmm... probably it would be better to make > ahci_disable_fbs() to just do it regardless of libata thinks whether > PMP is attached or not. After resume from STR, we shouldn't be > assuming anything about the controller state. I have added sata_pmp_attached() check for both ahci_enable_fbs() and ahci_disable_fbs(), because I believe it's necessary. if (!pp->fbs_supported || !sata_pmp_attached(ap)) return; What do you mean to make it better? How to? I do not catch your above words very much... Thanks, Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang, Shane wrote: > Hi Tejun, > > >> -----Original Message----- >> From: Tejun Heo [mailto:tj@kernel.org] >>> I find that ahci_pmp_detach() will be called for each SATA port >>> during the initialization, right after print: >>>> ahci 0000:00:11.0: flags: 64bit ncq sntf ilck pm led clo pmp fbs... >>> so will ahci_disable_fbs() be called, which leads to the trigger >>> of WARN_ON(). >> Ah.. right. ahci_port_resume() does that to make sure that all PMP >> bits are cleared on init. Hmmm... probably it would be better to make >> ahci_disable_fbs() to just do it regardless of libata thinks whether >> PMP is attached or not. After resume from STR, we shouldn't be >> assuming anything about the controller state. > > > I have added sata_pmp_attached() check for both ahci_enable_fbs() > and ahci_disable_fbs(), because I believe it's necessary. > > if (!pp->fbs_supported || !sata_pmp_attached(ap)) > return; > > What do you mean to make it better? How to? I do not catch your > above words very much... I meant that sata_pmp_attached() is a driver state and might not match the controller state after resume which is why the ahci driver calls detach in the first place regardless of sata_pmp_attached(), so it might be better to check the register directly and set or clear it regardless of sata_pmp_attached(). Thanks.
Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > I meant that sata_pmp_attached() is a driver state and might not match > the controller state after resume which is why the ahci driver calls > detach in the first place regardless of sata_pmp_attached(), so it > might be better to check the register directly and set or clear it > regardless of sata_pmp_attached(). OK, right, here is the updated implementation, WARN_ON() has been replaced by direct register check. static void ahci_enable_fbs(struct ata_port *ap) { struct ahci_port_priv *pp = ap->private_data; void __iomem *port_mmio = ahci_port_base(ap); u32 fbs; int rc; if (!pp->fbs_supported) return; fbs = readl(port_mmio + PORT_FBS); if (fbs & PORT_FBS_EN) return; rc = ahci_stop_engine(ap); if (rc) return; writel(fbs | PORT_FBS_EN, port_mmio + PORT_FBS); fbs = readl(port_mmio + PORT_FBS); if (fbs & PORT_FBS_EN) { dev_printk(KERN_INFO, ap->host->dev, "FBS is enabled.\n"); pp->fbs_enabled = true; pp->fbs_last_dev = -1; /* initialization */ } else dev_printk(KERN_ERR, ap->host->dev, "Failed to enable FBS\n"); ahci_start_engine(ap); } static void ahci_disable_fbs(struct ata_port *ap) { struct ahci_port_priv *pp = ap->private_data; void __iomem *port_mmio = ahci_port_base(ap); u32 fbs; int rc; if (!pp->fbs_supported) return; fbs = readl(port_mmio + PORT_FBS); if ((fbs & PORT_FBS_EN) == 0) return; rc = ahci_stop_engine(ap); if (rc) return; writel(fbs & ~PORT_FBS_EN, port_mmio + PORT_FBS); fbs = readl(port_mmio + PORT_FBS); if (fbs & PORT_FBS_EN) dev_printk(KERN_ERR, ap->host->dev, "Failed to disable FBS\n"); else { dev_printk(KERN_INFO, ap->host->dev, "FBS is disabled.\n"); pp->fbs_enabled = false; } ahci_start_engine(ap); } Thanks Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Shane. Huang, Shane wrote: > static void ahci_enable_fbs(struct ata_port *ap) > { > struct ahci_port_priv *pp = ap->private_data; > void __iomem *port_mmio = ahci_port_base(ap); > u32 fbs; > int rc; > > if (!pp->fbs_supported) > return; > > fbs = readl(port_mmio + PORT_FBS); > if (fbs & PORT_FBS_EN) > return; This can leave pp->fbs_enabled out of sync, right? Just don't check and set the bit anyway. > rc = ahci_stop_engine(ap); > if (rc) > return; > > writel(fbs | PORT_FBS_EN, port_mmio + PORT_FBS); > fbs = readl(port_mmio + PORT_FBS); > if (fbs & PORT_FBS_EN) { > dev_printk(KERN_INFO, ap->host->dev, "FBS is > enabled.\n"); > pp->fbs_enabled = true; > pp->fbs_last_dev = -1; /* initialization */ > } else > dev_printk(KERN_ERR, ap->host->dev, "Failed to enable > FBS\n"); > > ahci_start_engine(ap); > } > > > static void ahci_disable_fbs(struct ata_port *ap) > { > struct ahci_port_priv *pp = ap->private_data; > void __iomem *port_mmio = ahci_port_base(ap); > u32 fbs; > int rc; > > if (!pp->fbs_supported) > return; > > fbs = readl(port_mmio + PORT_FBS); > if ((fbs & PORT_FBS_EN) == 0) > return; Ditto. Thanks.
Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > > > > > static void ahci_disable_fbs(struct ata_port *ap) > > { > > struct ahci_port_priv *pp = ap->private_data; > > void __iomem *port_mmio = ahci_port_base(ap); > > u32 fbs; > > int rc; > > > > if (!pp->fbs_supported) > > return; > > > > fbs = readl(port_mmio + PORT_FBS); > > if ((fbs & PORT_FBS_EN) == 0) > > return; > > This can leave pp->fbs_enabled out of sync, right? Just don't check > and set the bit anyway. > Ditto. Yes, it can leave pp->fbs_enabled out of sync, but just removing the check and setting the bit anyway will still lead to the print of this message for each SATA port: ahci 0000:00:11.0: FBS is disabled. So I would suggest to keep the check and also add the sync of pp->fbs_enabled, which can also save the restart of DMA engine: static void ahci_disable_fbs(struct ata_port *ap) { ...... if (!pp->fbs_supported) return; fbs = readl(port_mmio + PORT_FBS); if ((fbs & PORT_FBS_EN) == 0) { pp->fbs_enabled = false; return; } rc = ahci_stop_engine(ap); static void ahci_enable_fbs(struct ata_port *ap) { ...... if (!pp->fbs_supported) return; fbs = readl(port_mmio + PORT_FBS); if (fbs & PORT_FBS_EN) { pp->fbs_enabled = true; pp->fbs_last_dev = -1; /* initialization */ return; } rc = ahci_stop_engine(ap); Thanks, Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang, Shane wrote: > Yes, it can leave pp->fbs_enabled out of sync, but just removing the > check and setting the bit anyway will still lead to the print of this > message for each SATA port: > ahci 0000:00:11.0: FBS is disabled. > > So I would suggest to keep the check and also add the sync of > pp->fbs_enabled, which can also save the restart of DMA engine: Yeap, sure. But in general, it doesn't really matter whether the DMA engine is started or restarted a few times during intialization. Just do whatever is safe and looks clean. Thanks.
Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > > Yes, it can leave pp->fbs_enabled out of sync, but just removing the > > check and setting the bit anyway will still lead to the > print of this > > message for each SATA port: > > ahci 0000:00:11.0: FBS is disabled. > > > > So I would suggest to keep the check and also add the sync of > > pp->fbs_enabled, which can also save the restart of DMA engine: > > Yeap, sure. But in general, it doesn't really matter whether the DMA > engine is started or restarted a few times during intialization. Just > do whatever is safe and looks clean. Yes, I agree, considering the unnecessary message for each SATA port > ahci 0000:00:11.0: FBS is disabled. might make user confused, I will keep my last update which is to "keep the check and also add the sync of pp->fbs_enabled" :-) Thanks, Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tejun, > -----Original Message----- > From: Huang, Shane > > > > +static void ahci_fbs_dec_intr(struct ata_port *ap) > > > +{ > > > + struct ahci_port_priv *pp = ap->private_data; > > > + DPRINTK("ENTER\n"); > > > + > > > + if (pp->fbs_enabled) { > > > > Just do BUG_ON(!pp->fbs_enabled); > > OK. > > > > > + void __iomem *port_mmio = ahci_port_base(ap); > > > + u32 fbs = readl(port_mmio + PORT_FBS); > > > + int retries = 3; > > > + > > > + /* time to wait for DEC is not specified by AHCI spec, > > > + * add a retry loop for safety */ > > > + do { > > > + writel(fbs | PORT_FBS_DEC, port_mmio + > > PORT_FBS); > > > + fbs = readl(port_mmio + PORT_FBS); > > > + retries--; > > > + } while ((fbs & PORT_FBS_DEC) && retries); At the 2nd thought, if (!pp->fbs_enabled) appears unfortunately (although it should not), replacement of if (pp->fbs_enabled) with BUG_ON() will lead to the execution of the followed DEC, which might lead to indeterminate result, because of AHCI v1.2 3.3.16: Device Error Clear (DEC): ....... Software shall only set this bit to '1' if PxFBS.EN is set to '1' and PxFBS.SDE is set to '1'. So I would suggest to keep my original implementation in v2, or put BUG_ON to the else case, if you insist, to replace the original dev_printk(KERN_ERR). Thanks Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Shane. Huang, Shane wrote: >>>> + /* time to wait for DEC is not specified by AHCI spec, >>>> + * add a retry loop for safety */ >>>> + do { >>>> + writel(fbs | PORT_FBS_DEC, port_mmio + >>> PORT_FBS); >>>> + fbs = readl(port_mmio + PORT_FBS); >>>> + retries--; >>>> + } while ((fbs & PORT_FBS_DEC) && retries); > > > At the 2nd thought, if (!pp->fbs_enabled) appears unfortunately > (although it should not), replacement of if (pp->fbs_enabled) with > BUG_ON() will lead to the execution of the followed DEC, Nope, it will result in the executing task being stopped with a scary stackdump. > which > might lead to indeterminate result, because of AHCI v1.2 3.3.16: > > Device Error Clear (DEC): ....... Software shall only set this bit to > '1' if > PxFBS.EN is set to '1' and PxFBS.SDE is set to '1'. > > So I would suggest to keep my original implementation in v2, or put > BUG_ON to the else case, if you insist, to replace the original > dev_printk(KERN_ERR). Problem with the original loop is that it writes PORT_FBS_DEC multiple times. Assume there are two consecutive device errors, the code might end up clearing the second one unintentionally. Thanks.
Tejun, > -----Original Message----- > From: Tejun Heo [mailto:tj@kernel.org] > > > > At the 2nd thought, if (!pp->fbs_enabled) appears unfortunately > > (although it should not), replacement of if (pp->fbs_enabled) with > > BUG_ON() will lead to the execution of the followed DEC, > > Nope, it will result in the executing task being stopped with a scary > stackdump. Yes, you are right, I will adopt your suggestion. Thanks Shane -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 273f50b..156b874 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -93,6 +93,9 @@ enum { AHCI_CMD_TBL_AR_SZ = AHCI_CMD_TBL_SZ * AHCI_MAX_CMDS, AHCI_PORT_PRIV_DMA_SZ = AHCI_CMD_SLOT_SZ + AHCI_CMD_TBL_AR_SZ + AHCI_RX_FIS_SZ, + AHCI_PORT_PRIV_FBS_DMA_SZ = AHCI_CMD_SLOT_SZ + + AHCI_CMD_TBL_AR_SZ + + (AHCI_RX_FIS_SZ * 16), AHCI_IRQ_ON_SG = (1 << 31), AHCI_CMD_ATAPI = (1 << 5), AHCI_CMD_WRITE = (1 << 6), @@ -132,6 +135,7 @@ enum { HOST_CAP_EMS = (1 << 6), /* Enclosure Management support */ HOST_CAP_SSC = (1 << 14), /* Slumber capable */ HOST_CAP_PMP = (1 << 17), /* Port Multiplier support */ + HOST_CAP_FBS = (1 << 16), /* FIS-based Switching support */ HOST_CAP_CLO = (1 << 24), /* Command List Override support */ HOST_CAP_ALPM = (1 << 26), /* Aggressive Link PM support */ HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */ @@ -155,6 +159,7 @@ enum { PORT_SCR_ERR = 0x30, /* SATA phy register: SError */ PORT_SCR_ACT = 0x34, /* SATA phy register: SActive */ PORT_SCR_NTF = 0x3c, /* SATA phy register: SNotification */ + PORT_FBS = 0x40, /* FIS-based Switching */ /* PORT_IRQ_{STAT,MASK} bits */ PORT_IRQ_COLD_PRES = (1 << 31), /* cold presence detect */ @@ -193,6 +198,7 @@ enum { PORT_CMD_ASP = (1 << 27), /* Aggressive Slumber/Partial */ PORT_CMD_ALPE = (1 << 26), /* Aggressive Link PM enable */ PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */ + PORT_CMD_FBSCP = (1 << 22), /* FBS Capable Port */ PORT_CMD_PMP = (1 << 17), /* PMP attached */ PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */ PORT_CMD_FIS_ON = (1 << 14), /* FIS DMA engine running */ @@ -207,6 +213,14 @@ enum { PORT_CMD_ICC_PARTIAL = (0x2 << 28), /* Put i/f in partial state */ PORT_CMD_ICC_SLUMBER = (0x6 << 28), /* Put i/f in slumber state */ + PORT_FBS_DWE_OFFSET = 16, /* FBS device with error offset */ + PORT_FBS_ADO_OFFSET = 12, /* FBS active dev optimization offset */ + PORT_FBS_DEV_OFFSET = 8, /* FBS device to issue offset */ + PORT_FBS_DEV_MASK = (0xf << PORT_FBS_DEV_OFFSET), /* FBS.DEV */ + PORT_FBS_SDE = (1 << 2), /* FBS single device error */ + PORT_FBS_DEC = (1 << 1), /* FBS device error clear */ + PORT_FBS_EN = (1 << 0), /* Enable FBS */ + /* hpriv->flags bits */ AHCI_HFLAG_NO_NCQ = (1 << 0), AHCI_HFLAG_IGN_IRQ_IF_ERR = (1 << 1), /* ignore IRQ_IF_ERR */ @@ -286,6 +300,10 @@ struct ahci_port_priv { unsigned int ncq_saw_dmas:1; unsigned int ncq_saw_sdb:1; u32 intr_mask; /* interrupts to enable */ + bool fbs_supported; /* set iff FBS is supported */ + bool fbs_enabled; /* set iff FBS is enabled */ + int fbs_last_dev; /* save FBS.DEV of last FIS */ + bool fbs_need_dec; /* need clear device error */ /* enclosure management info per PM slot */ struct ahci_em_priv em_priv[EM_MAX_SLOTS]; }; @@ -297,9 +315,12 @@ 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); +static int ahci_pmp_qc_defer(struct ata_queued_cmd *qc); static void ahci_qc_prep(struct ata_queued_cmd *qc); static void ahci_freeze(struct ata_port *ap); static void ahci_thaw(struct ata_port *ap); +static int ahci_enable_fbs(struct ata_port *ap); +static int ahci_disable_fbs(struct ata_port *ap); static void ahci_pmp_attach(struct ata_port *ap); static void ahci_pmp_detach(struct ata_port *ap); static int ahci_softreset(struct ata_link *link, unsigned int *class, @@ -354,7 +375,7 @@ static struct scsi_host_template ahci_sht = { static struct ata_port_operations ahci_ops = { .inherits = &sata_pmp_port_ops, - .qc_defer = sata_pmp_qc_defer_cmd_switch, + .qc_defer = ahci_pmp_qc_defer, .qc_prep = ahci_qc_prep, .qc_issue = ahci_qc_issue, .qc_fill_rtf = ahci_qc_fill_rtf, @@ -1936,6 +1957,17 @@ static unsigned int ahci_fill_sg(struct ata_queued_cmd *qc, void *cmd_tbl) return si; } +static int ahci_pmp_qc_defer(struct ata_queued_cmd *qc) +{ + struct ata_port *ap = qc->ap; + struct ahci_port_priv *pp = ap->private_data; + + if (!sata_pmp_attached(ap) || pp->fbs_enabled) + return ata_std_qc_defer(qc); + else + return sata_pmp_qc_defer_cmd_switch(qc); +} + static void ahci_qc_prep(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; @@ -1974,6 +2006,32 @@ static void ahci_qc_prep(struct ata_queued_cmd *qc) ahci_fill_cmd_slot(pp, qc->tag, opts); } +static void ahci_fbs_dec_intr(struct ata_port *ap) +{ + struct ahci_port_priv *pp = ap->private_data; + DPRINTK("ENTER\n"); + + if (pp->fbs_enabled) { + void __iomem *port_mmio = ahci_port_base(ap); + u32 fbs = readl(port_mmio + PORT_FBS); + int retries = 3; + + /* time to wait for DEC is not specified by AHCI spec, + * add a retry loop for safety */ + do { + writel(fbs | PORT_FBS_DEC, port_mmio + PORT_FBS); + fbs = readl(port_mmio + PORT_FBS); + retries--; + } while ((fbs & PORT_FBS_DEC) && retries); + + if (fbs & PORT_FBS_DEC) + dev_printk(KERN_ERR, ap->host->dev, + "failed to clear device error\n"); + } else + dev_printk(KERN_ERR, ap->host->dev, + "FBS is disabled, no need to clear device error\n"); +} + static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) { struct ahci_host_priv *hpriv = ap->host->private_data; @@ -1984,10 +2042,22 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) struct ata_eh_info *active_ehi; u32 serror; - /* determine active link */ - ata_for_each_link(link, ap, EDGE) - if (ata_link_active(link)) - break; + /* determine active link with error */ + if (pp->fbs_enabled) { + void __iomem *port_mmio = ahci_port_base(ap); + u32 fbs = readl(port_mmio + PORT_FBS); + + ata_for_each_link(link, ap, EDGE) + if (ata_link_active(link) && (fbs & PORT_FBS_SDE) && + (link->pmp == (fbs >> PORT_FBS_DWE_OFFSET))) { + pp->fbs_need_dec = true; + break; + } + } else + ata_for_each_link(link, ap, EDGE) + if (ata_link_active(link)) + break; + if (!link) link = &ap->link; @@ -2044,8 +2114,13 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) } if (irq_stat & PORT_IRQ_IF_ERR) { - host_ehi->err_mask |= AC_ERR_ATA_BUS; - host_ehi->action |= ATA_EH_RESET; + if (pp->fbs_need_dec) + active_ehi->err_mask |= AC_ERR_DEV; + else { + host_ehi->err_mask |= AC_ERR_ATA_BUS; + host_ehi->action |= ATA_EH_RESET; + } + ata_ehi_push_desc(host_ehi, "interface fatal error"); } @@ -2061,7 +2136,12 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) if (irq_stat & PORT_IRQ_FREEZE) ata_port_freeze(ap); else - ata_port_abort(ap); + if (pp->fbs_enabled && pp->fbs_need_dec && + !ata_is_host_link(link)) { + ata_link_abort(link); + ahci_fbs_dec_intr(ap); + } else + ata_port_abort(ap); } static void ahci_port_intr(struct ata_port *ap) @@ -2113,12 +2193,19 @@ static void ahci_port_intr(struct ata_port *ap) /* If the 'N' bit in word 0 of the FIS is set, * we just received asynchronous notification. * Tell libata about it. + * + * Lack of SNotification should not appear in + * ahci 1.2, so the workaround is unnecessary + * when FBS is enabled. */ - const __le32 *f = pp->rx_fis + RX_FIS_SDB; - u32 f0 = le32_to_cpu(f[0]); - - if (f0 & (1 << 15)) - sata_async_notification(ap); + if (pp->fbs_enabled) + WARN_ON(1); + else { + const __le32 *f = pp->rx_fis + RX_FIS_SDB; + u32 f0 = le32_to_cpu(f[0]); + if (f0 & (1 << 15)) + sata_async_notification(ap); + } } } @@ -2212,6 +2299,17 @@ static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc) if (qc->tf.protocol == ATA_PROT_NCQ) writel(1 << qc->tag, port_mmio + PORT_SCR_ACT); + + if (pp->fbs_enabled) { + if (pp->fbs_last_dev != qc->dev->link->pmp) { + u32 fbs = readl(port_mmio + PORT_FBS); + fbs &= ~(PORT_FBS_DEV_MASK | PORT_FBS_DEC); + fbs |= qc->dev->link->pmp << PORT_FBS_DEV_OFFSET; + writel(fbs, port_mmio + PORT_FBS); + pp->fbs_last_dev = qc->dev->link->pmp; + } + } + writel(1 << qc->tag, port_mmio + PORT_CMD_ISSUE); ahci_sw_activity(qc->dev->link); @@ -2224,6 +2322,9 @@ static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc) struct ahci_port_priv *pp = qc->ap->private_data; u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG; + if (pp->fbs_enabled) + d2h_fis += (qc->dev->link->pmp) * AHCI_RX_FIS_SZ; + ata_tf_from_fis(d2h_fis, &qc->result_tf); return true; } @@ -2272,6 +2373,77 @@ static void ahci_post_internal_cmd(struct ata_queued_cmd *qc) ahci_kick_engine(ap); } +static int ahci_enable_fbs(struct ata_port *ap) +{ + struct ahci_port_priv *pp = ap->private_data; + + if (pp->fbs_supported && !pp->fbs_enabled) { + void __iomem *port_mmio = ahci_port_base(ap); + u32 fbs; + int rc = ahci_stop_engine(ap); + if (rc) + return rc; + + fbs = readl(port_mmio + PORT_FBS); + writel(fbs | PORT_FBS_EN, port_mmio + PORT_FBS); + fbs = readl(port_mmio + PORT_FBS); + if (fbs & PORT_FBS_EN) { + dev_printk(KERN_INFO, ap->host->dev, + "FBS is enabled.\n"); + pp->fbs_enabled = true; + pp->fbs_last_dev = -1; /* initialization */ + } else { + dev_printk(KERN_ERR, ap->host->dev, + "Failed to enable FBS\n"); + ahci_start_engine(ap); + return -EIO; + } + + ahci_start_engine(ap); + } else { + dev_printk(KERN_ERR, ap->host->dev, + "FBS is not supported or already enabled\n"); + return -EINVAL; + } + + return 0; +} + +static int ahci_disable_fbs(struct ata_port *ap) +{ + struct ahci_port_priv *pp = ap->private_data; + + if (pp->fbs_enabled) { + void __iomem *port_mmio = ahci_port_base(ap); + u32 fbs; + int rc = ahci_stop_engine(ap); + if (rc) + return rc; + + fbs = readl(port_mmio + PORT_FBS); + writel(fbs & ~PORT_FBS_EN, port_mmio + PORT_FBS); + fbs = readl(port_mmio + PORT_FBS); + if (fbs & PORT_FBS_EN) { + dev_printk(KERN_ERR, ap->host->dev, + "Failed to disable FBS\n"); + ahci_start_engine(ap); + return -EIO; + } else { + dev_printk(KERN_INFO, ap->host->dev, + "FBS is disabled.\n"); + pp->fbs_enabled = false; + } + + ahci_start_engine(ap); + } else if (sata_pmp_attached(ap)) { + dev_printk(KERN_ERR, ap->host->dev, + "FBS is not supported or already disabled\n"); + return -EINVAL; + } + + return 0; +} + static void ahci_pmp_attach(struct ata_port *ap) { void __iomem *port_mmio = ahci_port_base(ap); @@ -2282,6 +2454,8 @@ static void ahci_pmp_attach(struct ata_port *ap) cmd |= PORT_CMD_PMP; writel(cmd, port_mmio + PORT_CMD); + ahci_enable_fbs(ap); + pp->intr_mask |= PORT_IRQ_BAD_PMP; writel(pp->intr_mask, port_mmio + PORT_IRQ_MASK); } @@ -2292,6 +2466,8 @@ static void ahci_pmp_detach(struct ata_port *ap) struct ahci_port_priv *pp = ap->private_data; u32 cmd; + ahci_disable_fbs(ap); + cmd = readl(port_mmio + PORT_CMD); cmd &= ~PORT_CMD_PMP; writel(cmd, port_mmio + PORT_CMD); @@ -2383,20 +2559,37 @@ static int ahci_pci_device_resume(struct pci_dev *pdev) static int ahci_port_start(struct ata_port *ap) { + struct ahci_host_priv *hpriv = ap->host->private_data; struct device *dev = ap->host->dev; struct ahci_port_priv *pp; void *mem; dma_addr_t mem_dma; + size_t dma_sz, rx_fis_sz; pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL); if (!pp) return -ENOMEM; - mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma, - GFP_KERNEL); + /* check FBS capability */ + if ((hpriv->cap & HOST_CAP_FBS) && sata_pmp_supported(ap)) { + void __iomem *port_mmio = ahci_port_base(ap); + u32 cmd = readl(port_mmio + PORT_CMD); + if (cmd & PORT_CMD_FBSCP) + pp->fbs_supported = true; + } + + if (pp->fbs_supported) { + dma_sz = AHCI_PORT_PRIV_FBS_DMA_SZ; + rx_fis_sz = AHCI_RX_FIS_SZ * 16; + } else { + dma_sz = AHCI_PORT_PRIV_DMA_SZ; + rx_fis_sz = AHCI_RX_FIS_SZ; + } + + mem = dmam_alloc_coherent(dev, dma_sz, &mem_dma, GFP_KERNEL); if (!mem) return -ENOMEM; - memset(mem, 0, AHCI_PORT_PRIV_DMA_SZ); + memset(mem, 0, dma_sz); /* * First item in chunk of DMA memory: 32-slot command table, @@ -2414,8 +2607,8 @@ static int ahci_port_start(struct ata_port *ap) pp->rx_fis = mem; pp->rx_fis_dma = mem_dma; - mem += AHCI_RX_FIS_SZ; - mem_dma += AHCI_RX_FIS_SZ; + mem += rx_fis_sz; + mem_dma += rx_fis_sz; /* * Third item: data area for storing a single command @@ -2533,7 +2726,7 @@ static void ahci_print_info(struct ata_host *host) "flags: " "%s%s%s%s%s%s%s" "%s%s%s%s%s%s%s" - "%s\n" + "%s%s\n" , cap & (1 << 31) ? "64bit " : "", @@ -2548,6 +2741,7 @@ static void ahci_print_info(struct ata_host *host) cap & (1 << 19) ? "nz " : "", cap & (1 << 18) ? "only " : "", cap & (1 << 17) ? "pmp " : "", + cap & (1 << 16) ? "fbs " : "", cap & (1 << 15) ? "pio " : "", cap & (1 << 14) ? "slum " : "", cap & (1 << 13) ? "part " : "",
Implement SATA AHCI FIS-based switching support. The patch has been updated after Tejun's review and suggestions to the 1st submission, which will not modify libata anymore. Signed-off-by: Shane Huang <shane.huang@amd.com> Cc: Tejun Heo <tj@kernel.org> --- drivers/ata/ahci.c | 232 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 213 insertions(+), 19 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html