[PATCH 0/5] sata_mv cleanups

[PATCH 0/5] sata_mv cleanups

[Mark Lord]

Here are five new patches for sata_mv.
These serve to clean things up in preparation
for the PMP patches which will follow.

Hopefully nothing controversial here.

01_sata_mv_cosmetics.patch
02_sata_mv_clean_up_stop_edma.patch
03_sata_mv_fix_ifcfg_handling.patch
04_sata_mv_new_hardreset_handler.patch
05_sata_mv_remove_phy_reset_and_postreset.patch

I generated these against Jeff's upstream branch as of today.

Please let me know ASAP of any issues, as there's a bunch of
PMP patches for sata_mv hot on the heels of these ones.

Thanks

[PATCH 1/5] sata_mv cosmetic fixes

[Mark Lord]

Various cosmetic fixes in preparation for real code changes later on.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_mv.c	2008-03-31 16:09:08.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-03-31 17:18:15.000000000 -0400
@@ -1,6 +1,7 @@
 /*
  * sata_mv.c - Marvell SATA support
  *
+ * Copyright 2008: Marvell Corporation, all rights reserved.
  * Copyright 2005: EMC Corporation, all rights reserved.
  * Copyright 2005 Red Hat, Inc.  All rights reserved.
  *
@@ -61,7 +62,6 @@
 
 */
 
-
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -131,7 +131,7 @@
 	MV_FLAG_DUAL_HC		= (1 << 30),  /* two SATA Host Controllers */
 	MV_FLAG_IRQ_COALESCE	= (1 << 29),  /* IRQ coalescing capability */
 	/* SoC integrated controllers, no PCI interface */
-	MV_FLAG_SOC = (1 << 28),
+	MV_FLAG_SOC		= (1 << 28),
 
 	MV_COMMON_FLAGS		= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_MMIO | ATA_FLAG_NO_ATAPI |
@@ -141,6 +141,7 @@
 	CRQB_FLAG_READ		= (1 << 0),
 	CRQB_TAG_SHIFT		= 1,
 	CRQB_IOID_SHIFT		= 6,	/* CRQB Gen-II/IIE IO Id shift */
+	CRQB_PMP_SHIFT		= 12,	/* CRQB Gen-II/IIE PMP shift */
 	CRQB_HOSTQ_SHIFT	= 17,	/* CRQB Gen-II/IIE HostQueTag shift */
 	CRQB_CMD_ADDR_SHIFT	= 8,
 	CRQB_CMD_CS		= (0x2 << 11),
@@ -199,7 +200,7 @@
 	TWSI_INT		= (1 << 24),
 	HC_MAIN_RSVD		= (0x7f << 25),	/* bits 31-25 */
 	HC_MAIN_RSVD_5		= (0x1fff << 19), /* bits 31-19 */
-	HC_MAIN_RSVD_SOC 	= (0x3fffffb << 6),     /* bits 31-9, 7-6 */
+	HC_MAIN_RSVD_SOC	= (0x3fffffb << 6),     /* bits 31-9, 7-6 */
 	HC_MAIN_MASKED_IRQS	= (TRAN_LO_DONE | TRAN_HI_DONE |
 				   PORTS_0_7_COAL_DONE | GPIO_INT | TWSI_INT |
 				   HC_MAIN_RSVD),
@@ -223,13 +224,18 @@
 	SATA_STATUS_OFS		= 0x300,  /* ctrl, err regs follow status */
 	SATA_ACTIVE_OFS		= 0x350,
 	SATA_FIS_IRQ_CAUSE_OFS	= 0x364,
+	LTMODE_OFS		= 0x30c,
 	PHY_MODE3		= 0x310,
 	PHY_MODE4		= 0x314,
 	PHY_MODE2		= 0x330,
+	SATA_IFCTL_OFS		= 0x344,
+	SATA_IFSTAT_OFS		= 0x34c,
+	VENDOR_UNIQUE_FIS_OFS	= 0x35c,
+	FIS_CFG_OFS		= 0x360,
 	MV5_PHY_MODE		= 0x74,
 	MV5_LT_MODE		= 0x30,
 	MV5_PHY_CTL		= 0x0C,
-	SATA_INTERFACE_CTL	= 0x050,
+	SATA_INTERFACE_CFG	= 0x050,
 
 	MV_M2_PREAMP_MASK	= 0x7e0,
 
@@ -240,6 +246,8 @@
 	EDMA_CFG_NCQ_GO_ON_ERR	= (1 << 14),	/* continue on error */
 	EDMA_CFG_RD_BRST_EXT	= (1 << 11),	/* read burst 512B */
 	EDMA_CFG_WR_BUFF_LEN	= (1 << 13),	/* write buffer 512B */
+	EDMA_CFG_EDMA_FBS	= (1 << 16),	/* EDMA FIS-Based Switching */
+	EDMA_CFG_FBS		= (1 << 26),	/* FIS-Based Switching */
 
 	EDMA_ERR_IRQ_CAUSE_OFS	= 0x8,
 	EDMA_ERR_IRQ_MASK_OFS	= 0xc,
@@ -298,6 +306,7 @@
 				  EDMA_ERR_LNK_DATA_RX |
 				  EDMA_ERR_LNK_DATA_TX |
 				  EDMA_ERR_TRANS_PROTO,
+
 	EDMA_EH_FREEZE_5	= EDMA_ERR_D_PAR |
 				  EDMA_ERR_PRD_PAR |
 				  EDMA_ERR_DEV_DCON |
@@ -344,7 +353,6 @@
 	/* Port private flags (pp_flags) */
 	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
 	MV_PP_FLAG_NCQ_EN	= (1 << 1),	/* is EDMA set up for NCQ? */
-	MV_PP_FLAG_HAD_A_RESET	= (1 << 2),	/* 1st hard reset complete? */
 };
 
 #define IS_GEN_I(hpriv) ((hpriv)->hp_flags & MV_HP_GEN_I)
@@ -506,11 +514,11 @@
 				      void __iomem *mmio);
 static void mv_soc_reset_bus(struct ata_host *host, void __iomem *mmio);
 static void mv_reset_pci_bus(struct ata_host *host, void __iomem *mmio);
-static void mv_channel_reset(struct mv_host_priv *hpriv, void __iomem *mmio,
+static void mv_reset_channel(struct mv_host_priv *hpriv, void __iomem *mmio,
 			     unsigned int port_no);
-static void mv_edma_cfg(struct mv_port_priv *pp, struct mv_host_priv *hpriv,
-			void __iomem *port_mmio, int want_ncq);
-static int __mv_stop_dma(struct ata_port *ap);
+static int mv_stop_edma(struct ata_port *ap);
+static int mv_stop_edma_engine(struct ata_port *ap);
+static void mv_edma_cfg(struct ata_port *ap, int want_ncq);
 
 /* .sg_tablesize is (MV_MAX_SG_CT / 2) in the structures below
  * because we have to allow room for worst case splitting of
@@ -714,6 +722,14 @@
 		(mv_hardport_from_port(port) * MV_PORT_REG_SZ);
 }
 
+static void __iomem *mv5_phy_base(void __iomem *mmio, unsigned int port)
+{
+	void __iomem *hc_mmio = mv_hc_base_from_port(mmio, port);
+	unsigned long ofs = (mv_hardport_from_port(port) + 1) * 0x100UL;
+
+	return hc_mmio + ofs;
+}
+
 static inline void __iomem *mv_host_base(struct ata_host *host)
 {
 	struct mv_host_priv *hpriv = host->private_data;
@@ -789,7 +805,7 @@
 	if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 		int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0);
 		if (want_ncq != using_ncq)
-			__mv_stop_dma(ap);
+			mv_stop_edma_engine(ap);
 	}
 	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
 		struct mv_host_priv *hpriv = ap->host->private_data;
@@ -810,7 +826,7 @@
 				 hc_mmio + HC_IRQ_CAUSE_OFS);
 		}
 
-		mv_edma_cfg(pp, hpriv, port_mmio, want_ncq);
+		mv_edma_cfg(ap, want_ncq);
 
 		/* clear FIS IRQ Cause */
 		writelfl(0, port_mmio + SATA_FIS_IRQ_CAUSE_OFS);
@@ -824,7 +840,7 @@
 }
 
 /**
- *      __mv_stop_dma - Disable eDMA engine
+ *      mv_stop_edma_engine - Disable eDMA engine
  *      @ap: ATA channel to manipulate
  *
  *      Verify the local cache of the eDMA state is accurate with a
@@ -833,7 +849,7 @@
  *      LOCKING:
  *      Inherited from caller.
  */
-static int __mv_stop_dma(struct ata_port *ap)
+static int mv_stop_edma_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = mv_ap_base(ap);
 	struct mv_port_priv *pp	= ap->private_data;
@@ -866,13 +882,13 @@
 	return err;
 }
 
-static int mv_stop_dma(struct ata_port *ap)
+static int mv_stop_edma(struct ata_port *ap)
 {
 	unsigned long flags;
 	int rc;
 
 	spin_lock_irqsave(&ap->host->lock, flags);
-	rc = __mv_stop_dma(ap);
+	rc = mv_stop_edma_engine(ap);
 	spin_unlock_irqrestore(&ap->host->lock, flags);
 
 	return rc;
@@ -1007,10 +1023,12 @@
 			adev->max_sectors = ATA_MAX_SECTORS;
 }
 
-static void mv_edma_cfg(struct mv_port_priv *pp, struct mv_host_priv *hpriv,
-			void __iomem *port_mmio, int want_ncq)
+static void mv_edma_cfg(struct ata_port *ap, int want_ncq)
 {
 	u32 cfg;
+	struct mv_port_priv *pp    = ap->private_data;
+	struct mv_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio    = mv_ap_base(ap);
 
 	/* set up non-NCQ EDMA configuration */
 	cfg = EDMA_CFG_Q_DEPTH;		/* always 0x1f for *all* chips */
@@ -1118,7 +1136,7 @@
 
 	spin_lock_irqsave(&ap->host->lock, flags);
 
-	mv_edma_cfg(pp, hpriv, port_mmio, 0);
+	mv_edma_cfg(ap, 0);
 	mv_set_edma_ptrs(port_mmio, hpriv, pp);
 
 	spin_unlock_irqrestore(&ap->host->lock, flags);
@@ -1145,7 +1163,7 @@
  */
 static void mv_port_stop(struct ata_port *ap)
 {
-	mv_stop_dma(ap);
+	mv_stop_edma(ap);
 	mv_port_free_dma_mem(ap);
 }
 
@@ -1315,8 +1333,7 @@
 	    (qc->tf.protocol != ATA_PROT_NCQ))
 		return;
 
-	/* Fill in Gen IIE command request block
-	 */
+	/* Fill in Gen IIE command request block */
 	if (!(qc->tf.flags & ATA_TFLAG_WRITE))
 		flags |= CRQB_FLAG_READ;
 
@@ -1384,7 +1401,7 @@
 		 * port.  Turn off EDMA so there won't be problems accessing
 		 * shadow block, etc registers.
 		 */
-		__mv_stop_dma(ap);
+		mv_stop_edma_engine(ap);
 		return ata_qc_issue_prot(qc);
 	}
 
@@ -1407,10 +1424,10 @@
  *      @reset_allowed: bool: 0 == don't trigger from reset here
  *
  *      In most cases, just clear the interrupt and move on.  However,
- *      some cases require an eDMA reset, which is done right before
- *      the COMRESET in mv_phy_reset().  The SERR case requires a
- *      clear of pending errors in the SATA SERROR register.  Finally,
- *      if the port disabled DMA, update our cached copy to match.
+ *      some cases require an eDMA reset, which also performs a COMRESET.
+ *      The SERR case requires a clear of pending errors in the SATA
+ *      SERROR register.  Finally, if the port disabled DMA,
+ *      update our cached copy to match.
  *
  *      LOCKING:
  *      Inherited from caller.
@@ -1648,9 +1665,9 @@
 		pp = ap->private_data;
 
 		shift = port << 1;		/* (port * 2) */
-		if (port >= MV_PORTS_PER_HC) {
+		if (port >= MV_PORTS_PER_HC)
 			shift++;	/* skip bit 8 in the HC Main IRQ reg */
-		}
+
 		have_err_bits = ((PORT0_ERR << shift) & relevant);
 
 		if (unlikely(have_err_bits)) {
@@ -1739,6 +1756,7 @@
 	void __iomem *mmio = hpriv->base;
 	u32 irq_stat, irq_mask;
 
+	/* Note to self: &host->lock == &ap->host->lock == ap->lock */
 	spin_lock(&host->lock);
 
 	irq_stat = readl(hpriv->main_cause_reg_addr);
@@ -1772,14 +1790,6 @@
 	return IRQ_RETVAL(handled);
 }
 
-static void __iomem *mv5_phy_base(void __iomem *mmio, unsigned int port)
-{
-	void __iomem *hc_mmio = mv_hc_base_from_port(mmio, port);
-	unsigned long ofs = (mv_hardport_from_port(port) + 1) * 0x100UL;
-
-	return hc_mmio + ofs;
-}
-
 static unsigned int mv5_scr_offset(unsigned int sc_reg_in)
 {
 	unsigned int ofs;
@@ -1907,7 +1917,7 @@
 
 	writelfl(EDMA_DS, port_mmio + EDMA_CMD_OFS);
 
-	mv_channel_reset(hpriv, mmio, port);
+	mv_reset_channel(hpriv, mmio, port);
 
 	ZERO(0x028);	/* command */
 	writel(0x11f, port_mmio + EDMA_CFG_OFS);
@@ -2125,14 +2135,15 @@
 		m4 = readl(port_mmio + PHY_MODE4);
 
 		if (hp_flags & MV_HP_ERRATA_60X1B2)
-			tmp = readl(port_mmio + 0x310);
+			tmp = readl(port_mmio + PHY_MODE3);
 
+		/* workaround for errata FEr SATA#10 (part 1) */
 		m4 = (m4 & ~(1 << 1)) | (1 << 0);
 
 		writel(m4, port_mmio + PHY_MODE4);
 
 		if (hp_flags & MV_HP_ERRATA_60X1B2)
-			writel(tmp, port_mmio + 0x310);
+			writel(tmp, port_mmio + PHY_MODE3);
 	}
 
 	/* Revert values of pre-emphasis and signal amps to the saved ones */
@@ -2182,7 +2193,7 @@
 
 	writelfl(EDMA_DS, port_mmio + EDMA_CMD_OFS);
 
-	mv_channel_reset(hpriv, mmio, port);
+	mv_reset_channel(hpriv, mmio, port);
 
 	ZERO(0x028);		/* command */
 	writel(0x101f, port_mmio + EDMA_CFG_OFS);
@@ -2239,7 +2250,7 @@
 	return;
 }
 
-static void mv_channel_reset(struct mv_host_priv *hpriv, void __iomem *mmio,
+static void mv_reset_channel(struct mv_host_priv *hpriv, void __iomem *mmio,
 			     unsigned int port_no)
 {
 	void __iomem *port_mmio = mv_port_base(mmio, port_no);
@@ -2247,10 +2258,10 @@
 	writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);
 
 	if (IS_GEN_II(hpriv)) {
-		u32 ifctl = readl(port_mmio + SATA_INTERFACE_CTL);
+		u32 ifctl = readl(port_mmio + SATA_INTERFACE_CFG);
 		ifctl |= (1 << 7);		/* enable gen2i speed */
 		ifctl = (ifctl & 0xfff) | 0x9b1000; /* from chip spec */
-		writelfl(ifctl, port_mmio + SATA_INTERFACE_CTL);
+		writelfl(ifctl, port_mmio + SATA_INTERFACE_CFG);
 	}
 
 	udelay(25);		/* allow reset propagation */
@@ -2372,14 +2383,7 @@
 
 static int mv_prereset(struct ata_link *link, unsigned long deadline)
 {
-	struct ata_port *ap = link->ap;
-	struct mv_port_priv *pp	= ap->private_data;
-
-	mv_stop_dma(ap);
-
-	if (!(pp->pp_flags & MV_PP_FLAG_HAD_A_RESET))
-		pp->pp_flags |= MV_PP_FLAG_HAD_A_RESET;
-
+	mv_stop_edma(link->ap);
 	return 0;
 }
 
@@ -2390,10 +2394,8 @@
 	struct mv_host_priv *hpriv = ap->host->private_data;
 	void __iomem *mmio = hpriv->base;
 
-	mv_stop_dma(ap);
-
-	mv_channel_reset(hpriv, mmio, ap->port_no);
-
+	mv_stop_edma(ap);
+	mv_reset_channel(hpriv, mmio, ap->port_no);
 	mv_phy_reset(ap, class, deadline);
 
 	return 0;
@@ -2715,10 +2717,10 @@
 		if (IS_GEN_II(hpriv)) {
 			void __iomem *port_mmio = mv_port_base(mmio, port);
 
-			u32 ifctl = readl(port_mmio + SATA_INTERFACE_CTL);
+			u32 ifctl = readl(port_mmio + SATA_INTERFACE_CFG);
 			ifctl |= (1 << 7);		/* enable gen2i speed */
 			ifctl = (ifctl & 0xfff) | 0x9b1000; /* from chip spec */
-			writelfl(ifctl, port_mmio + SATA_INTERFACE_CTL);
+			writelfl(ifctl, port_mmio + SATA_INTERFACE_CFG);
 		}
 
 		hpriv->ops->phy_errata(hpriv, mmio, port);

Re: [PATCH 1/5] sata_mv cosmetic fixes

[Jeff Garzik]

Mark Lord wrote:
> Various cosmetic fixes in preparation for real code changes later on.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

applied

[PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Mark Lord]

Clean up uses of mv_stop_edma{_engine}() to match datasheet requirements.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_mv.c	2008-03-31 17:18:15.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-03-31 18:12:56.000000000 -0400
@@ -517,7 +517,7 @@
 static void mv_reset_channel(struct mv_host_priv *hpriv, void __iomem *mmio,
 			     unsigned int port_no);
 static int mv_stop_edma(struct ata_port *ap);
-static int mv_stop_edma_engine(struct ata_port *ap);
+static int mv_stop_edma_engine(void __iomem *port_mmio);
 static void mv_edma_cfg(struct ata_port *ap, int want_ncq);
 
 /* .sg_tablesize is (MV_MAX_SG_CT / 2) in the structures below
@@ -805,7 +805,7 @@
 	if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 		int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0);
 		if (want_ncq != using_ncq)
-			mv_stop_edma_engine(ap);
+			mv_stop_edma(ap);
 	}
 	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN)) {
 		struct mv_host_priv *hpriv = ap->host->private_data;
@@ -841,57 +841,41 @@
 
 /**
  *      mv_stop_edma_engine - Disable eDMA engine
- *      @ap: ATA channel to manipulate
- *
- *      Verify the local cache of the eDMA state is accurate with a
- *      WARN_ON.
+ *      @port_mmio: io base address
  *
  *      LOCKING:
  *      Inherited from caller.
  */
-static int mv_stop_edma_engine(struct ata_port *ap)
+static int mv_stop_edma_engine(void __iomem *port_mmio)
 {
-	void __iomem *port_mmio = mv_ap_base(ap);
-	struct mv_port_priv *pp	= ap->private_data;
-	u32 reg;
-	int i, err = 0;
+	int i;
 
-	if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
-		/* Disable EDMA if active.   The disable bit auto clears.
-		 */
-		writelfl(EDMA_DS, port_mmio + EDMA_CMD_OFS);
-		pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
-	} else {
-		WARN_ON(EDMA_EN & readl(port_mmio + EDMA_CMD_OFS));
-	}
+	/* Disable eDMA.  The disable bit auto clears. */
+	writelfl(EDMA_DS, port_mmio + EDMA_CMD_OFS);
 
-	/* now properly wait for the eDMA to stop */
-	for (i = 1000; i > 0; i--) {
-		reg = readl(port_mmio + EDMA_CMD_OFS);
+	/* Wait for the chip to confirm eDMA is off. */
+	for (i = 10000; i > 0; i--) {
+		u32 reg = readl(port_mmio + EDMA_CMD_OFS);
 		if (!(reg & EDMA_EN))
-			break;
-
-		udelay(100);
-	}
-
-	if (reg & EDMA_EN) {
-		ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
-		err = -EIO;
+			return 0;
+		udelay(10);
 	}
-
-	return err;
+	return -EIO;
 }
 
 static int mv_stop_edma(struct ata_port *ap)
 {
-	unsigned long flags;
-	int rc;
-
-	spin_lock_irqsave(&ap->host->lock, flags);
-	rc = mv_stop_edma_engine(ap);
-	spin_unlock_irqrestore(&ap->host->lock, flags);
+	void __iomem *port_mmio = mv_ap_base(ap);
+	struct mv_port_priv *pp = ap->private_data;
 
-	return rc;
+	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN))
+		return 0;
+	pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
+	if (mv_stop_edma_engine(port_mmio)) {
+		ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
+		return -EIO;
+	}
+	return 0;
 }
 
 #ifdef ATA_DEBUG
@@ -1401,7 +1385,7 @@
 		 * port.  Turn off EDMA so there won't be problems accessing
 		 * shadow block, etc registers.
 		 */
-		mv_stop_edma_engine(ap);
+		mv_stop_edma(ap);
 		return ata_qc_issue_prot(qc);
 	}
 
@@ -1915,8 +1899,12 @@
 {
 	void __iomem *port_mmio = mv_port_base(mmio, port);
 
-	writelfl(EDMA_DS, port_mmio + EDMA_CMD_OFS);
-
+	/*
+	 * The datasheet warns against setting ATA_RST when EDMA is active
+	 * (but doesn't say what the problem might be).  So we first try
+	 * to disable the EDMA engine before doing the ATA_RST operation.
+	 */
+	mv_stop_edma_engine(port_mmio);
 	mv_reset_channel(hpriv, mmio, port);
 
 	ZERO(0x028);	/* command */
@@ -2191,8 +2179,12 @@
 {
 	void __iomem *port_mmio = mv_port_base(mmio, port);
 
-	writelfl(EDMA_DS, port_mmio + EDMA_CMD_OFS);
-
+	/*
+	 * The datasheet warns against setting ATA_RST when EDMA is active
+	 * (but doesn't say what the problem might be).  So we first try
+	 * to disable the EDMA engine before doing the ATA_RST operation.
+	 */
+	mv_stop_edma_engine(port_mmio);
 	mv_reset_channel(hpriv, mmio, port);
 
 	ZERO(0x028);		/* command */
@@ -2250,6 +2242,10 @@
 	return;
 }
 
+/*
+ * Caller must ensure that EDMA is not active,
+ * by first doing mv_stop_edma() where needed.
+ */
 static void mv_reset_channel(struct mv_host_priv *hpriv, void __iomem *mmio,
 			     unsigned int port_no)
 {
@@ -2392,10 +2388,11 @@
 {
 	struct ata_port *ap = link->ap;
 	struct mv_host_priv *hpriv = ap->host->private_data;
+	struct mv_port_priv *pp = ap->private_data;
 	void __iomem *mmio = hpriv->base;
 
-	mv_stop_edma(ap);
 	mv_reset_channel(hpriv, mmio, ap->port_no);
+	pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
 	mv_phy_reset(ap, class, deadline);
 
 	return 0;

Re: [PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> -    /* now properly wait for the eDMA to stop */
> -    for (i = 1000; i > 0; i--) {
> -        reg = readl(port_mmio + EDMA_CMD_OFS);
> +    /* Wait for the chip to confirm eDMA is off. */
> +    for (i = 10000; i > 0; i--) {
> +        u32 reg = readl(port_mmio + EDMA_CMD_OFS);
>         if (!(reg & EDMA_EN))
> -            break;
> -
> -        udelay(100);
> -    }
> -
> -    if (reg & EDMA_EN) {
> -        ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
> -        err = -EIO;
> +            return 0;
> +        udelay(10);

Unless the hardware calls for really short polling interval, I think 
it's generally better to limit polling with jiffies and using msleep() 
instead of delays.

Also, mv_stop_edma() skips actual operation if EDMA_EN isn't set, which 
I think is the correct way to do it in hot paths but I think it's better 
to stop the edma engine unconditionally prior to reset as that's where 
we try to bring the controller back into senses.

Thanks.

-- 
tejun

Re: [PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Mark Lord]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> -    /* now properly wait for the eDMA to stop */
>> -    for (i = 1000; i > 0; i--) {
>> -        reg = readl(port_mmio + EDMA_CMD_OFS);
>> +    /* Wait for the chip to confirm eDMA is off. */
>> +    for (i = 10000; i > 0; i--) {
>> +        u32 reg = readl(port_mmio + EDMA_CMD_OFS);
>>         if (!(reg & EDMA_EN))
>> -            break;
>> -
>> -        udelay(100);
>> -    }
>> -
>> -    if (reg & EDMA_EN) {
>> -        ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
>> -        err = -EIO;
>> +            return 0;
>> +        udelay(10);
> 
> Unless the hardware calls for really short polling interval, I think 
> it's generally better to limit polling with jiffies and using msleep() 
> instead of delays.
..

Oh, absolutely.  I was just leaving Jeff's (?) original udelay() in there
for now, to avoid another possible failure while testing the new stuff.

But if we can *guarantee* that .qc_issue and .port_stop are
always invoked only from thread context, then.. no problemo.

> Also, mv_stop_edma() skips actual operation if EDMA_EN isn't set, which 
> I think is the correct way to do it in hot paths but I think it's better 
> to stop the edma engine unconditionally prior to reset as that's where 
> we try to bring the controller back into senses.
..

Harmless change.  FITNR.

Thanks

Re: [PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Jeff Garzik]

Mark Lord wrote:
> Tejun Heo wrote:
>> Hello, Mark.
>>
>> Mark Lord wrote:
>>> -    /* now properly wait for the eDMA to stop */
>>> -    for (i = 1000; i > 0; i--) {
>>> -        reg = readl(port_mmio + EDMA_CMD_OFS);
>>> +    /* Wait for the chip to confirm eDMA is off. */
>>> +    for (i = 10000; i > 0; i--) {
>>> +        u32 reg = readl(port_mmio + EDMA_CMD_OFS);
>>>         if (!(reg & EDMA_EN))
>>> -            break;
>>> -
>>> -        udelay(100);
>>> -    }
>>> -
>>> -    if (reg & EDMA_EN) {
>>> -        ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
>>> -        err = -EIO;
>>> +            return 0;
>>> +        udelay(10);
>>
>> Unless the hardware calls for really short polling interval, I think 
>> it's generally better to limit polling with jiffies and using msleep() 
>> instead of delays.
> ..
> 
> Oh, absolutely.  I was just leaving Jeff's (?) original udelay() in there
> for now, to avoid another possible failure while testing the new stuff.
> 
> But if we can *guarantee* that .qc_issue and .port_stop are
> always invoked only from thread context, then.. no problemo.

qc_issue is inside spin_lock_irqsave()

Re: [PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Hello, Mark.
>>>
>>> Mark Lord wrote:
>>>> -    /* now properly wait for the eDMA to stop */
>>>> -    for (i = 1000; i > 0; i--) {
>>>> -        reg = readl(port_mmio + EDMA_CMD_OFS);
>>>> +    /* Wait for the chip to confirm eDMA is off. */
>>>> +    for (i = 10000; i > 0; i--) {
>>>> +        u32 reg = readl(port_mmio + EDMA_CMD_OFS);
>>>>         if (!(reg & EDMA_EN))
>>>> -            break;
>>>> -
>>>> -        udelay(100);
>>>> -    }
>>>> -
>>>> -    if (reg & EDMA_EN) {
>>>> -        ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
>>>> -        err = -EIO;
>>>> +            return 0;
>>>> +        udelay(10);
>>>
>>> Unless the hardware calls for really short polling interval, I think 
>>> it's generally better to limit polling with jiffies and using 
>>> msleep() instead of delays.
>> ..
>>
>> Oh, absolutely.  I was just leaving Jeff's (?) original udelay() in there
>> for now, to avoid another possible failure while testing the new stuff.
>>
>> But if we can *guarantee* that .qc_issue and .port_stop are
>> always invoked only from thread context, then.. no problemo.
> 
> qc_issue is inside spin_lock_irqsave()
..

Okay, let's leave it alone for now.
Perhaps later it can be reworked again to find
a way to busy wait without messing up other stuff.

Nearly all of the time it exits rapidly anyway.

Cheers

Re: [PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Tejun Heo]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>> Tejun Heo wrote:
>>>> Hello, Mark.
>>>>
>>>> Mark Lord wrote:
>>>>> -    /* now properly wait for the eDMA to stop */
>>>>> -    for (i = 1000; i > 0; i--) {
>>>>> -        reg = readl(port_mmio + EDMA_CMD_OFS);
>>>>> +    /* Wait for the chip to confirm eDMA is off. */
>>>>> +    for (i = 10000; i > 0; i--) {
>>>>> +        u32 reg = readl(port_mmio + EDMA_CMD_OFS);
>>>>>         if (!(reg & EDMA_EN))
>>>>> -            break;
>>>>> -
>>>>> -        udelay(100);
>>>>> -    }
>>>>> -
>>>>> -    if (reg & EDMA_EN) {
>>>>> -        ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
>>>>> -        err = -EIO;
>>>>> +            return 0;
>>>>> +        udelay(10);
>>>>
>>>> Unless the hardware calls for really short polling interval, I think 
>>>> it's generally better to limit polling with jiffies and using 
>>>> msleep() instead of delays.
>>> ..
>>>
>>> Oh, absolutely.  I was just leaving Jeff's (?) original udelay() in 
>>> there
>>> for now, to avoid another possible failure while testing the new stuff.
>>>
>>> But if we can *guarantee* that .qc_issue and .port_stop are
>>> always invoked only from thread context, then.. no problemo.
>>
>> qc_issue is inside spin_lock_irqsave()
> ..
> 
> Okay, let's leave it alone for now.
> Perhaps later it can be reworked again to find
> a way to busy wait without messing up other stuff.
> 
> Nearly all of the time it exits rapidly anyway.

Aikk... right, it's called from the issue path too.  In that case, 
udelay() is the only way to go.

Thanks.

-- 
tejun

Re: [PATCH 2/5] sata_mv clean up mv_stop_edma usage

[Jeff Garzik]

Mark Lord wrote:
> Clean up uses of mv_stop_edma{_engine}() to match datasheet requirements.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

applied 2-3

[PATCH 3/5] sata_mv fix ifctl handling

[Mark Lord]

Fix handling of the SATA_INTERFACE_CFG register to match datasheet requirements.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_mv.c	2008-03-31 18:12:56.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-03-31 18:14:40.000000000 -0400
@@ -2242,6 +2242,16 @@
 	return;
 }
 
+static void mv_setup_ifctl(void __iomem *port_mmio, int want_gen2i)
+{
+	u32 ifctl = readl(port_mmio + SATA_INTERFACE_CFG);
+
+	ifctl = (ifctl & 0xf7f) | 0x9b1000;	/* from chip spec */
+	if (want_gen2i)
+		ifctl |= (1 << 7);		/* enable gen2i speed */
+	writelfl(ifctl, port_mmio + SATA_INTERFACE_CFG);
+}
+
 /*
  * Caller must ensure that EDMA is not active,
  * by first doing mv_stop_edma() where needed.
@@ -2253,18 +2263,17 @@
 
 	writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);
 
-	if (IS_GEN_II(hpriv)) {
-		u32 ifctl = readl(port_mmio + SATA_INTERFACE_CFG);
-		ifctl |= (1 << 7);		/* enable gen2i speed */
-		ifctl = (ifctl & 0xfff) | 0x9b1000; /* from chip spec */
-		writelfl(ifctl, port_mmio + SATA_INTERFACE_CFG);
+	if (!IS_GEN_I(hpriv)) {
+		/* Enable 3.0gb/s link speed */
+		mv_setup_ifctl(port_mmio, 1);
 	}
-
-	udelay(25);		/* allow reset propagation */
-
-	/* Spec never mentions clearing the bit.  Marvell's driver does
-	 * clear the bit, however.
+	/*
+	 * Strobing ATA_RST here causes a hard reset of the SATA transport,
+	 * link, and physical layers.  It resets all SATA interface registers
+	 * (except for SATA_INTERFACE_CFG), and issues a COMRESET to the dev.
 	 */
+	writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);
+	udelay(25);	/* allow reset propagation */
 	writelfl(0, port_mmio + EDMA_CMD_OFS);
 
 	hpriv->ops->phy_errata(hpriv, mmio, port_no);
@@ -2711,19 +2720,6 @@
 	hpriv->ops->enable_leds(hpriv, mmio);
 
 	for (port = 0; port < host->n_ports; port++) {
-		if (IS_GEN_II(hpriv)) {
-			void __iomem *port_mmio = mv_port_base(mmio, port);
-
-			u32 ifctl = readl(port_mmio + SATA_INTERFACE_CFG);
-			ifctl |= (1 << 7);		/* enable gen2i speed */
-			ifctl = (ifctl & 0xfff) | 0x9b1000; /* from chip spec */
-			writelfl(ifctl, port_mmio + SATA_INTERFACE_CFG);
-		}
-
-		hpriv->ops->phy_errata(hpriv, mmio, port);
-	}
-
-	for (port = 0; port < host->n_ports; port++) {
 		struct ata_port *ap = host->ports[port];
 		void __iomem *port_mmio = mv_port_base(mmio, port);
 

[PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Introduce mv_sata_hardreset() to perform a link hard reset
while dealing with certain chipset errata during the reset.

Also beef up error-handling in mv_prereset() to trigger a hard reset
whenever mv_stop_edma() fails.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_mv.c	2008-03-31 18:14:40.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-03-31 18:24:50.000000000 -0400
@@ -2388,7 +2388,97 @@
 
 static int mv_prereset(struct ata_link *link, unsigned long deadline)
 {
-	mv_stop_edma(link->ap);
+	if (mv_stop_edma(link->ap))
+		link->eh_context.i.action |= ATA_EH_HARDRESET;
+	return ata_std_prereset(link, deadline);
+}
+
+/**
+ *	mv_sata_hardreset - reset host port via SATA phy reset
+ *	@link: link to reset
+ *	@class: resulting class of attached device
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	SATA phy-reset host port using DET bits of SControl register,
+ *	wait for !BSY and classify the attached device.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int mv_sata_hardreset(struct ata_link *link, unsigned int *class,
+			unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+	struct mv_host_priv *hpriv = ap->host->private_data;
+	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
+	int rc, attempts = 0, extra = 0;
+	u32 sstatus;
+
+	DPRINTK("ENTER\n");
+
+	/* FIXME:
+	 * Except for the outer do-while construct below, this function
+	 * is an exact clone of sata_std_hardreset() from libata-core.c.
+	 *
+	 * Once this driver is stable, we should re-org libata so we can share
+	 * more of that code, rather than duplicating so much of it here
+	 * and in other drivers.
+	 */
+
+	/* do-while is workaround for errata FEr SATA#10 (part 2) */
+	do {
+		/* do hardreset */
+		rc = sata_link_hardreset(link, timing, deadline + extra);
+		if (rc) {
+			ata_link_printk(link, KERN_ERR,
+					"COMRESET failed (errno=%d)\n", rc);
+			return rc;
+		}
+		sata_scr_read(&ap->link, SCR_STATUS, &sstatus);
+		if (!IS_GEN_I(hpriv) && ++attempts >= 5 && sstatus == 0x121) {
+			/* Force 1.5gb/s link speed and try again */
+			mv_setup_ifctl(mv_ap_base(ap), 0);
+			if (time_after(jiffies + HZ, deadline))
+				extra = HZ; /* only extend it once, max */
+		}
+	} while (sstatus != 0x0 && sstatus != 0x113 && sstatus != 0x123);
+
+	/* TODO: phy layer with polling, timeouts, etc. */
+	if (ata_link_offline(link)) {
+		*class = ATA_DEV_NONE;
+		DPRINTK("EXIT, link offline\n");
+		return 0;
+	}
+
+	/* wait a while before checking status */
+	ata_wait_after_reset(ap, deadline + extra);
+
+	/* If PMP is supported, we have to do follow-up SRST.  Note
+	 * that some PMPs don't send D2H Reg FIS after hardreset at
+	 * all if the first port is empty.  Wait for it just for a
+	 * second and request follow-up SRST.
+	 */
+	if (ap->flags & ATA_FLAG_PMP) {
+		ata_wait_ready(ap, jiffies + HZ);
+		return -EAGAIN;
+	}
+
+	rc = ata_wait_ready(ap, deadline + extra);
+	/* link occupied, -ENODEV too is an error */
+	if (rc) {
+		ata_link_printk(link, KERN_ERR,
+				"COMRESET failed (errno=%d)\n", rc);
+		return rc;
+	}
+
+	ap->ops->dev_select(ap, 0);	/* probably unnecessary */
+
+	*class = ata_dev_try_classify(link->device, 1, NULL);
+
+	DPRINTK("EXIT, class=%u\n", *class);
 	return 0;
 }
 
@@ -2402,9 +2492,8 @@
 
 	mv_reset_channel(hpriv, mmio, ap->port_no);
 	pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
-	mv_phy_reset(ap, class, deadline);
 
-	return 0;
+	return mv_sata_hardreset(link, class, deadline);
 }
 
 static void mv_postreset(struct ata_link *link, unsigned int *classes)

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo]

Hello,

Mark Lord wrote:
> static int mv_prereset(struct ata_link *link, unsigned long deadline)
> {
> -    mv_stop_edma(link->ap);
> +    if (mv_stop_edma(link->ap))
> +        link->eh_context.i.action |= ATA_EH_HARDRESET;
> +    return ata_std_prereset(link, deadline);
> +}

libata#upstream now defaults to hardreset.  softreset is used only as 
followup-SRST in case hardreset can't classify or PMP is supported, so 
there's no reason to add reset promotion logic anymore.

> +    /* FIXME:
> +     * Except for the outer do-while construct below, this function
> +     * is an exact clone of sata_std_hardreset() from libata-core.c.
> +     *
> +     * Once this driver is stable, we should re-org libata so we can share
> +     * more of that code, rather than duplicating so much of it here
> +     * and in other drivers.
> +     */

After modularize patchsets, sata_link_hardreset() does all the chores 
needed around hardreset and sata_mv should be able to just build a loop 
around it.

Thanks.

-- 
tejun

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Tejun Heo wrote:
> Hello,
> 
> Mark Lord wrote:
>> static int mv_prereset(struct ata_link *link, unsigned long deadline)
>> {
>> -    mv_stop_edma(link->ap);
>> +    if (mv_stop_edma(link->ap))
>> +        link->eh_context.i.action |= ATA_EH_HARDRESET;
>> +    return ata_std_prereset(link, deadline);
>> +}
> 
> libata#upstream now defaults to hardreset.  softreset is used only as 
> followup-SRST in case hardreset can't classify or PMP is supported, so 
> there's no reason to add reset promotion logic anymore.
..

Super.  That completely eliminates mv_prereset, then.

>> +    /* FIXME:
>> +     * Except for the outer do-while construct below, this function
>> +     * is an exact clone of sata_std_hardreset() from libata-core.c.
>> +     *
>> +     * Once this driver is stable, we should re-org libata so we can 
>> share
>> +     * more of that code, rather than duplicating so much of it here
>> +     * and in other drivers.
>> +     */
> 
> After modularize patchsets, sata_link_hardreset() does all the chores 
> needed around hardreset and sata_mv should be able to just build a loop 
> around it.
..

Oh, that stuff is in now?  Good.  I'll rework to use it.

Thanks again!

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
..
>> +    /* FIXME:
>> +     * Except for the outer do-while construct below, this function
>> +     * is an exact clone of sata_std_hardreset() from libata-core.c.
>> +     *
>> +     * Once this driver is stable, we should re-org libata so we can 
>> share
>> +     * more of that code, rather than duplicating so much of it here
>> +     * and in other drivers.
>> +     */
> 
> After modularize patchsets, sata_link_hardreset() does all the chores 
> needed around hardreset and sata_mv should be able to just build a loop 
> around it.
..

Mmm... I don't see how this helps.

The bulk of mv_sata_hardreset() is from sata_std_hardreset().

The only part those two do *not* have in common, is that
sata_mv needs to do it's own equivalent of sata_link_hardreset(),
so sata_link_hardreset() cannot be reused here.  Wrapper or not.

Now, if we had a per-LLD .link_hardreset op, defaulting to sata_link_hardreset,
then this would be trivial.

??

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
> ..
>>> +    /* FIXME:
>>> +     * Except for the outer do-while construct below, this function
>>> +     * is an exact clone of sata_std_hardreset() from libata-core.c.
>>> +     *
>>> +     * Once this driver is stable, we should re-org libata so we can 
>>> share
>>> +     * more of that code, rather than duplicating so much of it here
>>> +     * and in other drivers.
>>> +     */
>>
>> After modularize patchsets, sata_link_hardreset() does all the chores 
>> needed around hardreset and sata_mv should be able to just build a 
>> loop around it.
> ..
> 
> Mmm... I don't see how this helps.
> 
> The bulk of mv_sata_hardreset() is from sata_std_hardreset().
> 
> The only part those two do *not* have in common, is that
> sata_mv needs to do it's own equivalent of sata_link_hardreset(),
> so sata_link_hardreset() cannot be reused here.  Wrapper or not.
> 
> Now, if we had a per-LLD .link_hardreset op, defaulting to 
> sata_link_hardreset,
> then this would be trivial.

The MV specific part is retry-if-offline w/ lower link speed, right? 
You can do that just as well by looping outside of 
sata_link_hardreset().  It will fail fast if link is offline (not very 
different from your inner loop) and do everything you need to w/ proper 
check_ready() callback on success path.  Am I missing something?

Thanks.

-- 
tejun

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>> Mark Lord wrote:
>> ..
>>>> +    /* FIXME:
>>>> +     * Except for the outer do-while construct below, this function
>>>> +     * is an exact clone of sata_std_hardreset() from libata-core.c.
>>>> +     *
>>>> +     * Once this driver is stable, we should re-org libata so we 
>>>> can share
>>>> +     * more of that code, rather than duplicating so much of it here
>>>> +     * and in other drivers.
>>>> +     */
>>>
>>> After modularize patchsets, sata_link_hardreset() does all the chores 
>>> needed around hardreset and sata_mv should be able to just build a 
>>> loop around it.
>> ..
>>
>> Mmm... I don't see how this helps.
>>
>> The bulk of mv_sata_hardreset() is from sata_std_hardreset().
>>
>> The only part those two do *not* have in common, is that
>> sata_mv needs to do it's own equivalent of sata_link_hardreset(),
>> so sata_link_hardreset() cannot be reused here.  Wrapper or not.
>>
>> Now, if we had a per-LLD .link_hardreset op, defaulting to 
>> sata_link_hardreset,
>> then this would be trivial.
> 
> The MV specific part is retry-if-offline w/ lower link speed, right? You 
> can do that just as well by looping outside of sata_link_hardreset().  
..

Yes, the code already has a loop "outside of sata_link_hardreset()"
for the speed errata handling.  So nothing new there.

And the rest of that routine is a line-by-line clone of ata_std_hardreset().
This is smaller than what other drivers have cloned for these routines,
and a lot better than the old code from sata_mv that it replaces.

The comment in my patch above is just a reminder that someday we could
go back in and address those things.  In *all* LLDs, not just sata_mv.

I guess I'd better stop adding such comments in the future..  :)

Still think it needs any changes ?

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo]

Mark Lord wrote:
>> The MV specific part is retry-if-offline w/ lower link speed, right? 
>> You can do that just as well by looping outside of 
>> sata_link_hardreset().  
> ..
> 
> Yes, the code already has a loop "outside of sata_link_hardreset()"
> for the speed errata handling.  So nothing new there.
> 
> And the rest of that routine is a line-by-line clone of 
> ata_std_hardreset().
> This is smaller than what other drivers have cloned for these routines,
> and a lot better than the old code from sata_mv that it replaces.
> 
> The comment in my patch above is just a reminder that someday we could
> go back in and address those things.  In *all* LLDs, not just sata_mv.
> 
> I guess I'd better stop adding such comments in the future..  :)
> 
> Still think it needs any changes ?

The modularize patchset is not in libata-dev#upstream yet.  Please take 
a look at the following.

http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=drivers/ata/libata-core.c;h=7646523899c0bac8b06d2d0bfcde428e4583e04d;hb=731e61759c56d564322d56b9ff6f393fda1fbec4#l3533

I meant that you wouldn't need to copy the post-reset stuff and just 
could loop around new sata_link_hardreset() after the patchset.  I 
thought about breaking sata_link_hardreset() into two such that the 
post-reset part can be used separately but couldn't find any in-tree 
driver which would need such function.

-- 
tejun

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> The MV specific part is retry-if-offline w/ lower link speed, right? 
>>> You can do that just as well by looping outside of 
>>> sata_link_hardreset().  
>> ..
>>
>> Yes, the code already has a loop "outside of sata_link_hardreset()"
>> for the speed errata handling.  So nothing new there.
>>
>> And the rest of that routine is a line-by-line clone of 
>> ata_std_hardreset().
>> This is smaller than what other drivers have cloned for these routines,
>> and a lot better than the old code from sata_mv that it replaces.
>>
>> The comment in my patch above is just a reminder that someday we could
>> go back in and address those things.  In *all* LLDs, not just sata_mv.
>>
>> I guess I'd better stop adding such comments in the future..  :)
>>
>> Still think it needs any changes ?
> 
> The modularize patchset is not in libata-dev#upstream yet.  Please take 
> a look at the following.
> 
> http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=drivers/ata/libata-core.c;h=7646523899c0bac8b06d2d0bfcde428e4583e04d;hb=731e61759c56d564322d56b9ff6f393fda1fbec4#l3533 
>
> I meant that you wouldn't need to copy the post-reset stuff and just 
> could loop around new sata_link_hardreset() after the patchset.  I 
> thought about breaking sata_link_hardreset() into two such that the 
> post-reset part can be used separately but couldn't find any in-tree 
> driver which would need such function.
..

Ah well, that's all of no use then, because sata_mv has to go upstream now for 2.6.26.
People have been waiting for this driver to improve for a very long time now.

Given the time lag of the submission pipeline I'm forced to use for it,
there likely is not enough time left to wait for another major rework
of libata, and then rework sata_mv to match.

Basically, anything I do to sata_mv has to be tested here first,
then broken into Jeff-size bites, fed to Marvell, approved by them,
then posted here, then reworked according to the whims of the day,
then resent to Marvell, reapproved by them, then reposted here, ...

It just takes too long overall.

Cheers 
 

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Mark Lord wrote:
> Tejun Heo wrote:
>
>> The modularize patchset is not in libata-dev#upstream yet.  Please 
>> take a look at the following.
>>
>> http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=drivers/ata/libata-core.c;h=7646523899c0bac8b06d2d0bfcde428e4583e04d;hb=731e61759c56d564322d56b9ff6f393fda1fbec4#l3533 
>>
>> I meant that you wouldn't need to copy the post-reset stuff and just 
>> could loop around new sata_link_hardreset() after the patchset.  I 
>> thought about breaking sata_link_hardreset() into two such that the 
>> post-reset part can be used separately but couldn't find any in-tree 
>> driver which would need such function.
> ..
> 
> Ah well, that's all of no use then, because sata_mv has to go upstream 
> now for 2.6.26.
> People have been waiting for this driver to improve for a very long time 
> now.
> 
> Given the time lag of the submission pipeline I'm forced to use for it,
> there likely is not enough time left to wait for another major rework
> of libata, and then rework sata_mv to match.
..

Though, mind you.. as soon as your rework *does* hit upstream,
I'll happily submit further sata_mv patches to take full advantage.

The less custom code in sata_mv in the end, the happier I'll be.
That's why there's a comment there right now reminding me of it.
 
> Basically, anything I do to sata_mv has to be tested here first,
> then broken into Jeff-size bites, fed to Marvell, approved by them,
> then posted here, then reworked according to the whims of the day,
> then resent to Marvell, reapproved by them, then reposted here, ...
> 
> It just takes too long overall.
> 
> Cheers
> 
> -- 
> 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

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo]

Mark Lord wrote:
> Though, mind you.. as soon as your rework *does* hit upstream,
> I'll happily submit further sata_mv patches to take full advantage.
> 
> The less custom code in sata_mv in the end, the happier I'll be.
> That's why there's a comment there right now reminding me of it.

Yeap, I fully agree.  Also, the said change will probably go into 2.6.26 
too.  It's unfortunate that your sata_mv work overlaps too much with 
recent API overhauls.  I don't know how Jeff will merge stuff but 
merging sata_mv changes first is fine by me.  I'll update API changes 
accordingly and run it by you.

Thanks.

-- 
tejun

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Though, mind you.. as soon as your rework *does* hit upstream,
>> I'll happily submit further sata_mv patches to take full advantage.
>>
>> The less custom code in sata_mv in the end, the happier I'll be.
>> That's why there's a comment there right now reminding me of it.
> 
> Yeap, I fully agree.  Also, the said change will probably go into 2.6.26 
> too.  It's unfortunate that your sata_mv work overlaps too much with 
> recent API overhauls.  I don't know how Jeff will merge stuff but 
> merging sata_mv changes first is fine by me.  I'll update API changes 
> accordingly and run it by you.
..

Mmm.. I still need a way to handle access to the port-multiplier registers.
We never did resolve that one fully on the earlier pass.

It would be great if Jeff could just merge the API stuff now,
so LLD changes for 2.6.26 could then build on top of the updated API.

Doing it all at the final-minute merge window just doesn't leave
me enough real-time to go through the Marvell review processes.

Cheers

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo]

Mark Lord wrote:
> Mmm.. I still need a way to handle access to the port-multiplier registers.
> We never did resolve that one fully on the earlier pass.

Yeah, I thought about that during the API change.  I think we should one 
of the following two.

1. Separate out actual reset part from ata_sff_softreset() and use it in 
sata_mv so that sata_mv can do...

mv_srst()
{
	check onlineness;
	set up PMP reg;
	return ata_sff_do_softreset();
}

2. Move link onlineness test before softreset into EH.  It's common to 
all drivers anyway.  This will allow mv to do.

mv_srst()
{
	set up PMP reg;
	return ata_sff_softreset();
}

I like #2 better but there can be exotic cases where link onlineness 
can't be tested before the link is actually reset.  I don't think it 
will really matter because hardreset is present and preferred if link 
onlineness test is valid && hardreset doesn't care about link onlineness.

Thanks.

-- 
tejun

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>> Mmm.. I still need a way to handle access to the port-multiplier 
>> registers.
>> We never did resolve that one fully on the earlier pass.
> 
> Yeah, I thought about that during the API change.  I think we should one 
> of the following two.
> 
> 1. Separate out actual reset part from ata_sff_softreset() and use it in 
> sata_mv so that sata_mv can do...
> 
> mv_srst()
> {
>     check onlineness;
>     set up PMP reg;
>     return ata_sff_do_softreset();
> }
> 
> 2. Move link onlineness test before softreset into EH.  It's common to 
> all drivers anyway.  This will allow mv to do.
..

Can we get just this part upstream Real Soon Now?
The sata_mv pmp stuff is ready to go, except for this one issue.

Thanks


> mv_srst()
> {
>     set up PMP reg;
>     return ata_sff_softreset();
> }
> 
> I like #2 better but there can be exotic cases where link onlineness 
> can't be tested before the link is actually reset.  I don't think it 
> will really matter because hardreset is present and preferred if link 
> onlineness test is valid && hardreset doesn't care about link onlineness.
..

Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
>>>> The MV specific part is retry-if-offline w/ lower link speed, right? 
>>>> You can do that just as well by looping outside of 
>>>> sata_link_hardreset().  
>>> ..
>>>
>>> Yes, the code already has a loop "outside of sata_link_hardreset()"
>>> for the speed errata handling.  So nothing new there.
>>>
>>> And the rest of that routine is a line-by-line clone of 
>>> ata_std_hardreset().
>>> This is smaller than what other drivers have cloned for these routines,
>>> and a lot better than the old code from sata_mv that it replaces.
>>>
>>> The comment in my patch above is just a reminder that someday we could
>>> go back in and address those things.  In *all* LLDs, not just sata_mv.
>>>
>>> I guess I'd better stop adding such comments in the future..  :)
>>>
>>> Still think it needs any changes ?
>>
>> The modularize patchset is not in libata-dev#upstream yet.  Please 
>> take a look at the following.
>>
>> http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=blob;f=drivers/ata/libata-core.c;h=7646523899c0bac8b06d2d0bfcde428e4583e04d;hb=731e61759c56d564322d56b9ff6f393fda1fbec4#l3533 
>>
>> I meant that you wouldn't need to copy the post-reset stuff and just 
>> could loop around new sata_link_hardreset() after the patchset.  I 
>> thought about breaking sata_link_hardreset() into two such that the 
>> post-reset part can be used separately but couldn't find any in-tree 
>> driver which would need such function.
> ..
> 
> Ah well, that's all of no use then, because sata_mv has to go upstream 
> now for 2.6.26.
> People have been waiting for this driver to improve for a very long time 
> now.
> 
> Given the time lag of the submission pipeline I'm forced to use for it,
> there likely is not enough time left to wait for another major rework
> of libata, and then rework sata_mv to match.
> 
> Basically, anything I do to sata_mv has to be tested here first,
> then broken into Jeff-size bites, fed to Marvell, approved by them,
> then posted here, then reworked according to the whims of the day,
> then resent to Marvell, reapproved by them, then reposted here, ...

No worries.  Just go ahead.  I'll juggle things later.

-- 
tejun

[PATCH 5/5] sata_mv remove mv_phy_reset and mv_postreset

[Mark Lord]

Remove the now unused mv_phy_reset() code,
as well as the unnecessary mv_postreset() function.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

--- old/drivers/ata/sata_mv.c	2008-03-31 18:46:44.000000000 -0400
+++ linux/drivers/ata/sata_mv.c	2008-03-31 18:46:49.000000000 -0400
@@ -481,7 +481,6 @@
 static int mv_prereset(struct ata_link *link, unsigned long deadline);
 static int mv_hardreset(struct ata_link *link, unsigned int *class,
 			unsigned long deadline);
-static void mv_postreset(struct ata_link *link, unsigned int *classes);
 static void mv_eh_freeze(struct ata_port *ap);
 static void mv_eh_thaw(struct ata_port *ap);
 static void mv6_dev_config(struct ata_device *dev);
@@ -547,7 +546,6 @@
 	.thaw			= mv_eh_thaw,
 	.prereset		= mv_prereset,
 	.hardreset		= mv_hardreset,
-	.postreset		= mv_postreset,
 	.error_handler		= ata_std_error_handler, /* avoid SFF EH */
 	.post_internal_cmd	= ATA_OP_NULL,
 
@@ -2282,110 +2280,6 @@
 		mdelay(1);
 }
 
-/**
- *      mv_phy_reset - Perform eDMA reset followed by COMRESET
- *      @ap: ATA channel to manipulate
- *
- *      Part of this is taken from __sata_phy_reset and modified to
- *      not sleep since this routine gets called from interrupt level.
- *
- *      LOCKING:
- *      Inherited from caller.  This is coded to safe to call at
- *      interrupt level, i.e. it does not sleep.
- */
-static void mv_phy_reset(struct ata_port *ap, unsigned int *class,
-			 unsigned long deadline)
-{
-	struct mv_port_priv *pp	= ap->private_data;
-	struct mv_host_priv *hpriv = ap->host->private_data;
-	void __iomem *port_mmio = mv_ap_base(ap);
-	int retry = 5;
-	u32 sstatus;
-
-	VPRINTK("ENTER, port %u, mmio 0x%p\n", ap->port_no, port_mmio);
-
-#ifdef DEBUG
-	{
-		u32 sstatus, serror, scontrol;
-
-		mv_scr_read(ap, SCR_STATUS, &sstatus);
-		mv_scr_read(ap, SCR_ERROR, &serror);
-		mv_scr_read(ap, SCR_CONTROL, &scontrol);
-		DPRINTK("S-regs after ATA_RST: SStat 0x%08x SErr 0x%08x "
-			"SCtrl 0x%08x\n", sstatus, serror, scontrol);
-	}
-#endif
-
-	/* Issue COMRESET via SControl */
-comreset_retry:
-	sata_scr_write_flush(&ap->link, SCR_CONTROL, 0x301);
-	msleep(1);
-
-	sata_scr_write_flush(&ap->link, SCR_CONTROL, 0x300);
-	msleep(20);
-
-	do {
-		sata_scr_read(&ap->link, SCR_STATUS, &sstatus);
-		if (((sstatus & 0x3) == 3) || ((sstatus & 0x3) == 0))
-			break;
-
-		msleep(1);
-	} while (time_before(jiffies, deadline));
-
-	/* work around errata */
-	if (IS_GEN_II(hpriv) &&
-	    (sstatus != 0x0) && (sstatus != 0x113) && (sstatus != 0x123) &&
-	    (retry-- > 0))
-		goto comreset_retry;
-
-#ifdef DEBUG
-	{
-		u32 sstatus, serror, scontrol;
-
-		mv_scr_read(ap, SCR_STATUS, &sstatus);
-		mv_scr_read(ap, SCR_ERROR, &serror);
-		mv_scr_read(ap, SCR_CONTROL, &scontrol);
-		DPRINTK("S-regs after PHY wake: SStat 0x%08x SErr 0x%08x "
-			"SCtrl 0x%08x\n", sstatus, serror, scontrol);
-	}
-#endif
-
-	if (ata_link_offline(&ap->link)) {
-		*class = ATA_DEV_NONE;
-		return;
-	}
-
-	/* even after SStatus reflects that device is ready,
-	 * it seems to take a while for link to be fully
-	 * established (and thus Status no longer 0x80/0x7F),
-	 * so we poll a bit for that, here.
-	 */
-	retry = 20;
-	while (1) {
-		u8 drv_stat = ata_check_status(ap);
-		if ((drv_stat != 0x80) && (drv_stat != 0x7f))
-			break;
-		msleep(500);
-		if (retry-- <= 0)
-			break;
-		if (time_after(jiffies, deadline))
-			break;
-	}
-
-	/* FIXME: if we passed the deadline, the following
-	 * code probably produces an invalid result
-	 */
-
-	/* finally, read device signature from TF registers */
-	*class = ata_dev_try_classify(ap->link.device, 1, NULL);
-
-	writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
-
-	WARN_ON(pp->pp_flags & MV_PP_FLAG_EDMA_EN);
-
-	VPRINTK("EXIT\n");
-}
-
 static int mv_prereset(struct ata_link *link, unsigned long deadline)
 {
 	if (mv_stop_edma(link->ap))
@@ -2496,28 +2390,6 @@
 	return mv_sata_hardreset(link, class, deadline);
 }
 
-static void mv_postreset(struct ata_link *link, unsigned int *classes)
-{
-	struct ata_port *ap = link->ap;
-	u32 serr;
-
-	/* print link status */
-	sata_print_link_status(link);
-
-	/* clear SError */
-	sata_scr_read(link, SCR_ERROR, &serr);
-	sata_scr_write_flush(link, SCR_ERROR, serr);
-
-	/* bail out if no device is present */
-	if (classes[0] == ATA_DEV_NONE && classes[1] == ATA_DEV_NONE) {
-		DPRINTK("EXIT, no device\n");
-		return;
-	}
-
-	/* set up device control */
-	iowrite8(ap->ctl, ap->ioaddr.ctl_addr);
-}
-
 static void mv_eh_freeze(struct ata_port *ap)
 {
 	struct mv_host_priv *hpriv = ap->host->private_data;

Re: [PATCH 5/5] sata_mv remove mv_phy_reset and mv_postreset

[Jeff Garzik]

Mark Lord wrote:
> Remove the now unused mv_phy_reset() code,
> as well as the unnecessary mv_postreset() function.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

ACK patches 4-5

However, I would prefer to finish merging Tejun's honkin' huge 
modularize patchsets, and then request that you rediff&resend sata_mv 
patches 4 and 5

Having already ack'd it, I'll push Tejun's resync as soon as I receive it.

Re: [PATCH 5/5] sata_mv remove mv_phy_reset and mv_postreset

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Remove the now unused mv_phy_reset() code,
>> as well as the unnecessary mv_postreset() function.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> ACK patches 4-5
> 
> However, I would prefer to finish merging Tejun's honkin' huge 
> modularize patchsets, and then request that you rediff&resend sata_mv 
> patches 4 and 5
> 
> Having already ack'd it, I'll push Tejun's resync as soon as I receive it.
..

Perfect!  Thanks, Jeff.

I have sata_mv PMP patches yet to come.
Plus fixes for the interrupt/error handling in sata_mv,
a side-effect of which is that hot plug should begin working.

Smaller errata fixes will follow, as I get to them.

Cheers

[PATCH] sata_mv rework hardreset sequence

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Remove the now unused mv_phy_reset() code,
>> as well as the unnecessary mv_postreset() function.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> ACK patches 4-5
> 
> However, I would prefer to finish merging Tejun's honkin' huge 
> modularize patchsets, and then request that you rediff&resend sata_mv 
> patches 4 and 5


This patch replaces the earlier patches 04/05.

* * *

Rework and simplify sata_mv's hardreset code to take advantage
of libata improvements since it was first coded.

Also, get rid of the now unnecessary prereset, postreset, and phy_reset functions.

This patch also paves the way for subsequent pmp support patches,
which will follow once this one passes muster.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- upstream/linux/drivers/ata/sata_mv.c	2008-04-10 14:30:27.000000000 -0400
+++ new/linux/drivers/ata/sata_mv.c	2008-04-10 14:31:01.000000000 -0400
@@ -478,10 +478,8 @@
 static void mv_qc_prep(struct ata_queued_cmd *qc);
 static void mv_qc_prep_iie(struct ata_queued_cmd *qc);
 static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
-static int mv_prereset(struct ata_link *link, unsigned long deadline);
 static int mv_hardreset(struct ata_link *link, unsigned int *class,
 			unsigned long deadline);
-static void mv_postreset(struct ata_link *link, unsigned int *classes);
 static void mv_eh_freeze(struct ata_port *ap);
 static void mv_eh_thaw(struct ata_port *ap);
 static void mv6_dev_config(struct ata_device *dev);
@@ -545,9 +543,7 @@
 
 	.freeze			= mv_eh_freeze,
 	.thaw			= mv_eh_thaw,
-	.prereset		= mv_prereset,
 	.hardreset		= mv_hardreset,
-	.postreset		= mv_postreset,
 	.error_handler		= ata_std_error_handler, /* avoid SFF EH */
 	.post_internal_cmd	= ATA_OP_NULL,
 
@@ -1904,7 +1900,6 @@
 	 * (but doesn't say what the problem might be).  So we first try
 	 * to disable the EDMA engine before doing the ATA_RST operation.
 	 */
-	mv_stop_edma_engine(port_mmio);
 	mv_reset_channel(hpriv, mmio, port);
 
 	ZERO(0x028);	/* command */
@@ -2184,7 +2179,6 @@
 	 * (but doesn't say what the problem might be).  So we first try
 	 * to disable the EDMA engine before doing the ATA_RST operation.
 	 */
-	mv_stop_edma_engine(port_mmio);
 	mv_reset_channel(hpriv, mmio, port);
 
 	ZERO(0x028);		/* command */
@@ -2261,6 +2255,7 @@
 {
 	void __iomem *port_mmio = mv_port_base(mmio, port_no);
 
+	mv_stop_edma_engine(port_mmio);
 	writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);
 
 	if (!IS_GEN_I(hpriv)) {
@@ -2282,116 +2277,6 @@
 		mdelay(1);
 }
 
-/**
- *      mv_phy_reset - Perform eDMA reset followed by COMRESET
- *      @ap: ATA channel to manipulate
- *
- *      Part of this is taken from __sata_phy_reset and modified to
- *      not sleep since this routine gets called from interrupt level.
- *
- *      LOCKING:
- *      Inherited from caller.  This is coded to safe to call at
- *      interrupt level, i.e. it does not sleep.
- */
-static void mv_phy_reset(struct ata_port *ap, unsigned int *class,
-			 unsigned long deadline)
-{
-	struct mv_port_priv *pp	= ap->private_data;
-	struct mv_host_priv *hpriv = ap->host->private_data;
-	void __iomem *port_mmio = mv_ap_base(ap);
-	int retry = 5;
-	u32 sstatus;
-
-	VPRINTK("ENTER, port %u, mmio 0x%p\n", ap->port_no, port_mmio);
-
-#ifdef DEBUG
-	{
-		u32 sstatus, serror, scontrol;
-
-		mv_scr_read(ap, SCR_STATUS, &sstatus);
-		mv_scr_read(ap, SCR_ERROR, &serror);
-		mv_scr_read(ap, SCR_CONTROL, &scontrol);
-		DPRINTK("S-regs after ATA_RST: SStat 0x%08x SErr 0x%08x "
-			"SCtrl 0x%08x\n", sstatus, serror, scontrol);
-	}
-#endif
-
-	/* Issue COMRESET via SControl */
-comreset_retry:
-	sata_scr_write_flush(&ap->link, SCR_CONTROL, 0x301);
-	msleep(1);
-
-	sata_scr_write_flush(&ap->link, SCR_CONTROL, 0x300);
-	msleep(20);
-
-	do {
-		sata_scr_read(&ap->link, SCR_STATUS, &sstatus);
-		if (((sstatus & 0x3) == 3) || ((sstatus & 0x3) == 0))
-			break;
-
-		msleep(1);
-	} while (time_before(jiffies, deadline));
-
-	/* work around errata */
-	if (IS_GEN_II(hpriv) &&
-	    (sstatus != 0x0) && (sstatus != 0x113) && (sstatus != 0x123) &&
-	    (retry-- > 0))
-		goto comreset_retry;
-
-#ifdef DEBUG
-	{
-		u32 sstatus, serror, scontrol;
-
-		mv_scr_read(ap, SCR_STATUS, &sstatus);
-		mv_scr_read(ap, SCR_ERROR, &serror);
-		mv_scr_read(ap, SCR_CONTROL, &scontrol);
-		DPRINTK("S-regs after PHY wake: SStat 0x%08x SErr 0x%08x "
-			"SCtrl 0x%08x\n", sstatus, serror, scontrol);
-	}
-#endif
-
-	if (ata_link_offline(&ap->link)) {
-		*class = ATA_DEV_NONE;
-		return;
-	}
-
-	/* even after SStatus reflects that device is ready,
-	 * it seems to take a while for link to be fully
-	 * established (and thus Status no longer 0x80/0x7F),
-	 * so we poll a bit for that, here.
-	 */
-	retry = 20;
-	while (1) {
-		u8 drv_stat = ata_sff_check_status(ap);
-		if ((drv_stat != 0x80) && (drv_stat != 0x7f))
-			break;
-		msleep(500);
-		if (retry-- <= 0)
-			break;
-		if (time_after(jiffies, deadline))
-			break;
-	}
-
-	/* FIXME: if we passed the deadline, the following
-	 * code probably produces an invalid result
-	 */
-
-	/* finally, read device signature from TF registers */
-	*class = ata_sff_dev_classify(ap->link.device, 1, NULL);
-
-	writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
-
-	WARN_ON(pp->pp_flags & MV_PP_FLAG_EDMA_EN);
-
-	VPRINTK("EXIT\n");
-}
-
-static int mv_prereset(struct ata_link *link, unsigned long deadline)
-{
-	mv_stop_edma(link->ap);
-	return 0;
-}
-
 static int mv_hardreset(struct ata_link *link, unsigned int *class,
 			unsigned long deadline)
 {
@@ -2399,34 +2284,33 @@
 	struct mv_host_priv *hpriv = ap->host->private_data;
 	struct mv_port_priv *pp = ap->private_data;
 	void __iomem *mmio = hpriv->base;
+	int rc, attempts = 0, extra = 0;
+	u32 sstatus;
+	bool online;
 
 	mv_reset_channel(hpriv, mmio, ap->port_no);
 	pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
-	mv_phy_reset(ap, class, deadline);
-
-	return 0;
-}
-
-static void mv_postreset(struct ata_link *link, unsigned int *classes)
-{
-	struct ata_port *ap = link->ap;
-	u32 serr;
 
-	/* print link status */
-	sata_print_link_status(link);
+	/* Workaround for errata FEr SATA#10 (part 2) */
+	do {
+		const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 
-	/* clear SError */
-	sata_scr_read(link, SCR_ERROR, &serr);
-	sata_scr_write_flush(link, SCR_ERROR, serr);
-
-	/* bail out if no device is present */
-	if (classes[0] == ATA_DEV_NONE && classes[1] == ATA_DEV_NONE) {
-		DPRINTK("EXIT, no device\n");
-		return;
-	}
+		rc = sata_link_hardreset(link, timing, deadline + extra, &online, NULL);
+		if (rc) {
+			ata_link_printk(link, KERN_ERR,
+					"COMRESET failed (errno=%d)\n", rc);
+			return rc;
+		}
+		sata_scr_read(link, SCR_STATUS, &sstatus);
+		if (!IS_GEN_I(hpriv) && ++attempts >= 5 && sstatus == 0x121) {
+			/* Force 1.5gb/s link speed and try again */
+			mv_setup_ifctl(mv_ap_base(ap), 0);
+			if (time_after(jiffies + HZ, deadline))
+				extra = HZ; /* only extend it once, max */
+		}
+	} while (sstatus != 0x0 && sstatus != 0x113 && sstatus != 0x123);
 
-	/* set up device control */
-	iowrite8(ap->ctl, ap->ioaddr.ctl_addr);
+	return online ? -EAGAIN : rc;
 }
 
 static void mv_eh_freeze(struct ata_port *ap)

Re: [PATCH] sata_mv rework hardreset sequence

[Mark Lord]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Mark Lord wrote:
>>> Remove the now unused mv_phy_reset() code,
>>> as well as the unnecessary mv_postreset() function.
>>>
>>> Signed-off-by: Mark Lord <mlord@pobox.com>
>>
>> ACK patches 4-5
>>
>> However, I would prefer to finish merging Tejun's honkin' huge 
>> modularize patchsets, and then request that you rediff&resend sata_mv 
>> patches 4 and 5
> 
> 
> This patch replaces the earlier patches 04/05.
..

Jeff -- drop this patch for now.
I'll resubmit it as part of the upcoming PMP patches.

Thanks



> 
> * * *
> 
> Rework and simplify sata_mv's hardreset code to take advantage
> of libata improvements since it was first coded.
> 
> Also, get rid of the now unnecessary prereset, postreset, and phy_reset 
> functions.
> 
> This patch also paves the way for subsequent pmp support patches,
> which will follow once this one passes muster.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- upstream/linux/drivers/ata/sata_mv.c    2008-04-10 
> 14:30:27.000000000 -0400
> +++ new/linux/drivers/ata/sata_mv.c    2008-04-10 14:31:01.000000000 -0400
> @@ -478,10 +478,8 @@
> static void mv_qc_prep(struct ata_queued_cmd *qc);
> static void mv_qc_prep_iie(struct ata_queued_cmd *qc);
> static unsigned int mv_qc_issue(struct ata_queued_cmd *qc);
> -static int mv_prereset(struct ata_link *link, unsigned long deadline);
> static int mv_hardreset(struct ata_link *link, unsigned int *class,
>             unsigned long deadline);
> -static void mv_postreset(struct ata_link *link, unsigned int *classes);
> static void mv_eh_freeze(struct ata_port *ap);
> static void mv_eh_thaw(struct ata_port *ap);
> static void mv6_dev_config(struct ata_device *dev);
> @@ -545,9 +543,7 @@
> 
>     .freeze            = mv_eh_freeze,
>     .thaw            = mv_eh_thaw,
> -    .prereset        = mv_prereset,
>     .hardreset        = mv_hardreset,
> -    .postreset        = mv_postreset,
>     .error_handler        = ata_std_error_handler, /* avoid SFF EH */
>     .post_internal_cmd    = ATA_OP_NULL,
> 
> @@ -1904,7 +1900,6 @@
>      * (but doesn't say what the problem might be).  So we first try
>      * to disable the EDMA engine before doing the ATA_RST operation.
>      */
> -    mv_stop_edma_engine(port_mmio);
>     mv_reset_channel(hpriv, mmio, port);
> 
>     ZERO(0x028);    /* command */
> @@ -2184,7 +2179,6 @@
>      * (but doesn't say what the problem might be).  So we first try
>      * to disable the EDMA engine before doing the ATA_RST operation.
>      */
> -    mv_stop_edma_engine(port_mmio);
>     mv_reset_channel(hpriv, mmio, port);
> 
>     ZERO(0x028);        /* command */
> @@ -2261,6 +2255,7 @@
> {
>     void __iomem *port_mmio = mv_port_base(mmio, port_no);
> 
> +    mv_stop_edma_engine(port_mmio);
>     writelfl(ATA_RST, port_mmio + EDMA_CMD_OFS);
> 
>     if (!IS_GEN_I(hpriv)) {
> @@ -2282,116 +2277,6 @@
>         mdelay(1);
> }
> 
> -/**
> - *      mv_phy_reset - Perform eDMA reset followed by COMRESET
> - *      @ap: ATA channel to manipulate
> - *
> - *      Part of this is taken from __sata_phy_reset and modified to
> - *      not sleep since this routine gets called from interrupt level.
> - *
> - *      LOCKING:
> - *      Inherited from caller.  This is coded to safe to call at
> - *      interrupt level, i.e. it does not sleep.
> - */
> -static void mv_phy_reset(struct ata_port *ap, unsigned int *class,
> -             unsigned long deadline)
> -{
> -    struct mv_port_priv *pp    = ap->private_data;
> -    struct mv_host_priv *hpriv = ap->host->private_data;
> -    void __iomem *port_mmio = mv_ap_base(ap);
> -    int retry = 5;
> -    u32 sstatus;
> -
> -    VPRINTK("ENTER, port %u, mmio 0x%p\n", ap->port_no, port_mmio);
> -
> -#ifdef DEBUG
> -    {
> -        u32 sstatus, serror, scontrol;
> -
> -        mv_scr_read(ap, SCR_STATUS, &sstatus);
> -        mv_scr_read(ap, SCR_ERROR, &serror);
> -        mv_scr_read(ap, SCR_CONTROL, &scontrol);
> -        DPRINTK("S-regs after ATA_RST: SStat 0x%08x SErr 0x%08x "
> -            "SCtrl 0x%08x\n", sstatus, serror, scontrol);
> -    }
> -#endif
> -
> -    /* Issue COMRESET via SControl */
> -comreset_retry:
> -    sata_scr_write_flush(&ap->link, SCR_CONTROL, 0x301);
> -    msleep(1);
> -
> -    sata_scr_write_flush(&ap->link, SCR_CONTROL, 0x300);
> -    msleep(20);
> -
> -    do {
> -        sata_scr_read(&ap->link, SCR_STATUS, &sstatus);
> -        if (((sstatus & 0x3) == 3) || ((sstatus & 0x3) == 0))
> -            break;
> -
> -        msleep(1);
> -    } while (time_before(jiffies, deadline));
> -
> -    /* work around errata */
> -    if (IS_GEN_II(hpriv) &&
> -        (sstatus != 0x0) && (sstatus != 0x113) && (sstatus != 0x123) &&
> -        (retry-- > 0))
> -        goto comreset_retry;
> -
> -#ifdef DEBUG
> -    {
> -        u32 sstatus, serror, scontrol;
> -
> -        mv_scr_read(ap, SCR_STATUS, &sstatus);
> -        mv_scr_read(ap, SCR_ERROR, &serror);
> -        mv_scr_read(ap, SCR_CONTROL, &scontrol);
> -        DPRINTK("S-regs after PHY wake: SStat 0x%08x SErr 0x%08x "
> -            "SCtrl 0x%08x\n", sstatus, serror, scontrol);
> -    }
> -#endif
> -
> -    if (ata_link_offline(&ap->link)) {
> -        *class = ATA_DEV_NONE;
> -        return;
> -    }
> -
> -    /* even after SStatus reflects that device is ready,
> -     * it seems to take a while for link to be fully
> -     * established (and thus Status no longer 0x80/0x7F),
> -     * so we poll a bit for that, here.
> -     */
> -    retry = 20;
> -    while (1) {
> -        u8 drv_stat = ata_sff_check_status(ap);
> -        if ((drv_stat != 0x80) && (drv_stat != 0x7f))
> -            break;
> -        msleep(500);
> -        if (retry-- <= 0)
> -            break;
> -        if (time_after(jiffies, deadline))
> -            break;
> -    }
> -
> -    /* FIXME: if we passed the deadline, the following
> -     * code probably produces an invalid result
> -     */
> -
> -    /* finally, read device signature from TF registers */
> -    *class = ata_sff_dev_classify(ap->link.device, 1, NULL);
> -
> -    writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
> -
> -    WARN_ON(pp->pp_flags & MV_PP_FLAG_EDMA_EN);
> -
> -    VPRINTK("EXIT\n");
> -}
> -
> -static int mv_prereset(struct ata_link *link, unsigned long deadline)
> -{
> -    mv_stop_edma(link->ap);
> -    return 0;
> -}
> -
> static int mv_hardreset(struct ata_link *link, unsigned int *class,
>             unsigned long deadline)
> {
> @@ -2399,34 +2284,33 @@
>     struct mv_host_priv *hpriv = ap->host->private_data;
>     struct mv_port_priv *pp = ap->private_data;
>     void __iomem *mmio = hpriv->base;
> +    int rc, attempts = 0, extra = 0;
> +    u32 sstatus;
> +    bool online;
> 
>     mv_reset_channel(hpriv, mmio, ap->port_no);
>     pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
> -    mv_phy_reset(ap, class, deadline);
> -
> -    return 0;
> -}
> -
> -static void mv_postreset(struct ata_link *link, unsigned int *classes)
> -{
> -    struct ata_port *ap = link->ap;
> -    u32 serr;
> 
> -    /* print link status */
> -    sata_print_link_status(link);
> +    /* Workaround for errata FEr SATA#10 (part 2) */
> +    do {
> +        const unsigned long *timing = 
> sata_ehc_deb_timing(&link->eh_context);
> 
> -    /* clear SError */
> -    sata_scr_read(link, SCR_ERROR, &serr);
> -    sata_scr_write_flush(link, SCR_ERROR, serr);
> -
> -    /* bail out if no device is present */
> -    if (classes[0] == ATA_DEV_NONE && classes[1] == ATA_DEV_NONE) {
> -        DPRINTK("EXIT, no device\n");
> -        return;
> -    }
> +        rc = sata_link_hardreset(link, timing, deadline + extra, 
> &online, NULL);
> +        if (rc) {
> +            ata_link_printk(link, KERN_ERR,
> +                    "COMRESET failed (errno=%d)\n", rc);
> +            return rc;
> +        }
> +        sata_scr_read(link, SCR_STATUS, &sstatus);
> +        if (!IS_GEN_I(hpriv) && ++attempts >= 5 && sstatus == 0x121) {
> +            /* Force 1.5gb/s link speed and try again */
> +            mv_setup_ifctl(mv_ap_base(ap), 0);
> +            if (time_after(jiffies + HZ, deadline))
> +                extra = HZ; /* only extend it once, max */
> +        }
> +    } while (sstatus != 0x0 && sstatus != 0x113 && sstatus != 0x123);
> 
> -    /* set up device control */
> -    iowrite8(ap->ctl, ap->ioaddr.ctl_addr);
> +    return online ? -EAGAIN : rc;
> }
> 
> static void mv_eh_freeze(struct ata_port *ap)