Re: [PATCH 1/1] [v3] Add support 2 SATA ports for Maui and change filename from sata_dwc_460ex.c to sata_dwc_4xx.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

On 06-04-2012 9:31, Thang Q. Nguyen wrote:

Signed-off-by: Thang Q. Nguyen<tqnguyen@xxxxxxx>
[...]

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_4xx.c
similarity index 73%
rename from drivers/ata/sata_dwc_460ex.c
rename to drivers/ata/sata_dwc_4xx.c
index 69f7cde..07e9b36 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_4xx.c
[...]
@@ -268,22 +276,25 @@ enum {
 						 << 16)
 struct sata_dwc_device {
 	struct device		*dev;		/* generic device struct */
-	struct ata_probe_ent	*pe;		/* ptr to probe-ent */
 	struct ata_host		*host;
 	u8			*reg_base;
 	struct sata_dwc_regs	*sata_dwc_regs;	/* DW Synopsys SATA specific */
 	int			irq_dma;
+	u8			*scr_base;

Why not 'void __iomem *scr_base'? You have to cast to it anyway everytime. And 'u8 *' is just not the right type.

+	int			dma_channel;	/* DWC SATA DMA channel */
+	int			hostID;
 };
хюююъ
+/* This is used for easier trace back when having DMA channel */
+static struct sata_dwc_device *dwc_dev_list[DMA_NUM_CHANS];

I don't quite understand: isn't this dual channel device? But you declare a device per DMA channel...

+/*
+ * As we have only one DMA controller, this will be set static and global
+ * for easier to access

   "to" not needed here.

@@ -372,15 +381,15 @@ static const char *get_dma_dir_descript(int dma_dir)
  	}
  }

-static void sata_dwc_tf_dump(struct ata_taskfile *tf)
+static void sata_dwc_tf_dump(struct device *dwc_dev, struct ata_taskfile *tf)
  {
-	dev_vdbg(host_pvt.dwc_dev, "taskfile cmd: 0x%02x protocol: %s flags:"
+	dev_vdbg(dwc_dev, "taskfile cmd: 0x%02x protocol: %s flags:"

   Space missing after colon, BTW.

  		"0x%lx device: %x\n", tf->command,
  		get_prot_descript(tf->protocol), tf->flags, tf->device);
[...]

 /*
  * Function: dma_request_channel

BTW, it would be good if you changed the function comments to the kernel-doc format (in another patch).

- * arguments: None
+ * arguments: ap
  * returns channel number if available else -1
  * This function assigns the next available DMA channel from the list to the
  * requester
  */
-static int dma_request_channel(void)
+static int dma_request_channel(struct ata_port *ap)
  {
-	int i;
+	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);

[...]
+	/* Check if the channel is not currently in use */
+	if (!(in_le32(&(sata_dma_regs->dma_chan_en.low)) &\

   \ not needed. () with & not needed.

+		DMA_CHANNEL(hsdev->dma_channel)))
+		return hsdev->dma_channel;
+
+	dev_err(ap->dev, "%s Channel %d is currently in use\n", __func__,
+		hsdev->dma_channel);
  	return -1;
  }

  /*
+ * Check if the selected DMA channel is currently enabled.
+ */
+static int sata_dwc_dma_chk_en(int ch)
+{
+	u32 dma_chan_reg;

   Empty line here please.

+	/* Read the DMA channel register */
+	dma_chan_reg = in_le32(&(sata_dma_regs->dma_chan_en.low));

   () with & not needed.

+/*
+ * Handle DMA transfer complete interrupt. This checks and passes the
+ * processing to the appropriate ATA port.
+ */
+static irqreturn_t dma_dwc_handler(int irq, void *hsdev_instance)
+{
+	u32 tfr_reg, err_reg;
+	int chan;
+
+	tfr_reg = in_le32(&(sata_dma_regs->interrupt_status.tfr.low));
+	err_reg = in_le32(&(sata_dma_regs->interrupt_status.error.low));

   () with & not needed.

@@ -471,41 +517,25 @@ static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
  	spin_lock_irqsave(&host->lock, flags);
  	ap = host->ports[port];
  	hsdevp = HSDEVP_FROM_AP(ap);
-	tag = ap->link.active_tag;

-	tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
+	if (ap->link.active_tag != ATA_TAG_POISON)
+		tag = ap->link.active_tag;
+
+	tfr_reg = in_le32(&(sata_dma_regs->interrupt_status.tfr\

\ not needed. () with & not needed. And the line is too short to break it anyway.

  			.low));
-	err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
+	err_reg = in_le32(&(sata_dma_regs->interrupt_status.error\

   Same coments.

+			out_le32(&(sata_dma_regs->interrupt_clear.tfr.low),

   () with & not needed.

@@ -516,11 +546,16 @@ static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
  				err_reg);

  			/* Clear the interrupt. */
-			out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
+			out_le32(&(sata_dma_regs->interrupt_clear\

\ not needed. () with & not needed. And the line is too short to break it anyway.

  				.error.low),
[...]
@@ -629,14 +667,22 @@ static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
  			 * Set DMA addresses and lower half of control register
  			 * based on direction.
  			 */
+			if (hsdevp->hsdev->hostID == APM_821XX_SATA) {
+				sms_val = 1+hsdevp->hsdev->dma_channel;

   Spaces around + please.

+				dms_val = 0;
+			} else {
+				sms_val = 0;
+				dms_val = 1+hsdevp->hsdev->dma_channel;

   Same here.

@@ -714,70 +766,49 @@ static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
  static void dma_dwc_xfer_start(int dma_ch)
  {
  	/* Enable the DMA channel */
-	out_le32(&(host_pvt.sata_dma_regs->dma_chan_en.low),
-		 in_le32(&(host_pvt.sata_dma_regs->dma_chan_en.low)) |
+	out_le32(&(sata_dma_regs->dma_chan_en.low),
+		 in_le32(&(sata_dma_regs->dma_chan_en.low)) |

   () with & not needed.

  		 DMA_ENABLE_CHAN(dma_ch));
  }

-static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems,
-			      struct lli *lli, dma_addr_t dma_lli,
-			      void __iomem *addr, int dir)
+

   Empty line not needed...

+static int dma_dwc_xfer_setup(struct ata_port *ap, dma_addr_t dma_lli)
[...]
-	out_le32(&(host_pvt.sata_dma_regs->chan_regs[dma_ch].cfg.high),
+	out_le32(&(sata_dma_regs->chan_regs[dma_ch].cfg.high),

   () with & not needed.

+		 DMA_CFG_HW_HS_SRC(dma_ch) | DMA_CFG_HW_HS_DEST(dma_ch) | \
  		 DMA_CFG_PROTCTL | DMA_CFG_FCMOD_REQ);
-	out_le32(&(host_pvt.sata_dma_regs->chan_regs[dma_ch].cfg.low), 0);
+
+	out_le32(&(sata_dma_regs->chan_regs[dma_ch].cfg.low),

   () with & not needed.

+		DMA_CFG_HW_CH_PRIOR(dma_ch));

  	/* Program the address of the linked list */
-	out_le32(&(host_pvt.sata_dma_regs->chan_regs[dma_ch].llp.low),
+	if (hsdev->hostID == APM_460EX_SATA) {
+		out_le32(&(sata_dma_regs->chan_regs[dma_ch].llp.low),

   () with & not needed.

  		 DMA_LLP_LMS(dma_lli, DMA_LLP_AHBMASTER2));

   Please indent this like more to the right.

+	} else {
+		out_le32(&(sata_dma_regs->chan_regs[dma_ch].llp.low),

   () with & not needed.

+			DMA_LLP_LMS(dma_lli, DMA_LLP_AHBMASTER1));
+	}

  	/* Program the CTL register with src enable / dst enable */
-	out_le32(&(host_pvt.sata_dma_regs->chan_regs[dma_ch].ctl.low),
+	out_le32(&(sata_dma_regs->chan_regs[dma_ch].ctl.low),

   () with & not needed.

@@ -789,24 +820,18 @@ static int dma_dwc_init(struct sata_dwc_device *hsdev, int irq)
[...]
  	/* Enabe DMA */

   Enable.

-	out_le32(&(host_pvt.sata_dma_regs->dma_cfg.low), DMA_EN);
+	out_le32(&(sata_dma_regs->dma_cfg.low), DMA_EN);

   () with & not needed.

@@ -838,23 +863,27 @@ static int sata_dwc_scr_write(struct ata_link *link, unsigned int scr, u32 val)
  	return 0;
  }

-static u32 core_scr_read(unsigned int scr)
+static u32 core_scr_read(struct ata_port *ap, unsigned int scr)
  {
-	return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
-			(scr * 4));
+	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
+
+	return in_le32((void __iomem *)hsdev->scr_base + (scr * 4));

   () around * not needed.

  }

-static void core_scr_write(unsigned int scr, u32 val)
+
+static void core_scr_write(struct ata_port *ap, unsigned int scr, u32 val)
  {
-	out_le32((void __iomem *)(host_pvt.scr_addr_sstatus) + (scr * 4),
-		val);
+	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
+
+	out_le32((void __iomem *)hsdev->scr_base + (scr * 4), val);

   Same here.

@@ -869,7 +898,6 @@ static u32 qcmd_tag_to_mask(u8 tag)
  	return 0x00000001<<  (tag&  0x1f);
  }

-/* See ahci.c */

   Don't think this is a legal change in this patch...

  static void sata_dwc_error_intr(struct ata_port *ap,
  				struct sata_dwc_device *hsdev, uint intpr)
  {
@@ -883,24 +911,23 @@ static void sata_dwc_error_intr(struct ata_port *ap,

  	ata_ehi_clear_desc(ehi);

-	serror = core_scr_read(SCR_ERROR);
+	serror = core_scr_read(ap, SCR_ERROR);
  	status = ap->ops->sff_check_status(ap);

-	err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
+	err_reg = in_le32(&(sata_dma_regs->interrupt_status.error.\

   \ not needed. () with & not needed. And the line is too short to be broken.

  			low));
  	tag = ap->link.active_tag;

  	dev_err(ap->dev, "%s SCR_ERROR=0x%08x intpr=0x%08x status=0x%08x "
-		"dma_intp=%d pending=%d issued=%d dma_err_status=0x%08x\n",
-		__func__, serror, intpr, status, host_pvt.dma_interrupt_count,
-		hsdevp->dma_pending[tag], hsdevp->cmd_issued[tag], err_reg);
+		" pending=%d dma_err_status=0x%08x\n",

   Space not needed before "pending".

@@ -930,14 +957,14 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
  	struct sata_dwc_device *hsdev = HSDEV_FROM_HOST(host);
  	struct ata_port *ap;
  	struct ata_queued_cmd *qc;
-	unsigned long flags;
  	u8 status, tag;
-	int handled, num_processed, port = 0;
-	uint intpr, sactive, sactive2, tag_mask;
+	int handled, port = 0;
+	int num_lli;
+	uint intpr, sactive, tag_mask;
  	struct sata_dwc_device_port *hsdevp;
-	host_pvt.sata_dwc_sactive_issued = 0;
+	u32 mask;

-	spin_lock_irqsave(&host->lock, flags);
+	spin_lock(&host->lock);

   Is this change related?

+		if (qc) {
+			hsdevp->sactive_issued |= mask;
+			/* Prevent to issue more commands */
+			qc->ap->link.active_tag = tag;
+			qc->dev->link->sactive |= (1 << qc->tag);

   () not needed around <<.

+			num_lli = map_sg_to_lli(ap, qc->sg, qc->n_elem, \
+				hsdevp->llit[tag], hsdevp->llit_dma[tag], \
+				(void *__iomem)(&hsdev->sata_dwc_regs->dmadr), \

   You don't need \ to break the lines in C, unless this is in macro definition.

@@ -1012,28 +1051,12 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
  		dev_dbg(ap->dev, "%s non-NCQ cmd interrupt, protocol: %s\n",
  			__func__, get_prot_descript(qc->tf.protocol));
  DRVSTILLBUSY:
+		/* Do complete action for the current QC */
  		if (ata_is_dma(qc->tf.protocol)) {
[...]
+			sata_dwc_qc_complete(ap, qc, 1);
+		} else if ((ata_is_pio(qc->tf.protocol)) ||
+			(ata_is_nodata(qc->tf.protocol))) {

   () not needed around the operands of ||.

@@ -1049,23 +1072,18 @@ DRVSTILLBUSY:
[...]
-	if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
-					(host_pvt.sata_dwc_sactive_issued)) {
+	if ((tag_mask | hsdevp->sactive_issued) != \
+					hsdevp->sactive_issued) {
  		dev_warn(ap->dev, "Bad tag mask?  sactive=0x%08x "
-			 "(host_pvt.sata_dwc_sactive_issued)=0x%08x  tag_mask"
-			 "=0x%08x\n", sactive, host_pvt.sata_dwc_sactive_issued,
+			 "sactive_issued=0x%08x  tag_mask"

   There's no need to break the string literal here anymore.

+			 "=0x%08x\n", sactive, hsdevp->sactive_issued,
  			  tag_mask);
  	}

@@ -1073,70 +1091,21 @@ DRVSTILLBUSY:
  	status = ap->ops->sff_check_status(ap);
  	dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__, status);

-	tag = 0;
-	num_processed = 0;
-	while (tag_mask) {
-		num_processed++;
-		while (!(tag_mask&  0x00000001)) {
-			tag++;
-			tag_mask<<= 1;
-		}
-
-		tag_mask&= (~0x00000001);
-		qc = ata_qc_from_tag(ap, tag);
-
-		/* To be picked up by completion functions */
-		qc->ap->link.active_tag = tag;
-		hsdevp->cmd_issued[tag] = SATA_DWC_CMD_ISSUED_NOT;
-
-		/* Let libata/scsi layers handle error */
-		if (status & ATA_ERR) {
-			dev_dbg(ap->dev, "%s ATA_ERR (0x%x)\n", __func__,
-				status);
+	for (tag = 0; tag<  32; tag++) {
+		if (tag_mask&  qcmd_tag_to_mask(tag)) {
+			qc = ata_qc_from_tag(ap, tag);
+			if (!qc) {
+				dev_info(ap->dev, "error: Tag %d is set but " \
+					"not available\n", tag);
+				continue;
+			}
  			sata_dwc_qc_complete(ap, qc, 1);
-			handled = 1;
-			goto DONE;
  		}
-
-		/* Process completed command */
-		dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n", __func__,
-			get_prot_descript(qc->tf.protocol));
-		if (ata_is_dma(qc->tf.protocol)) {
-			host_pvt.dma_interrupt_count++;
-			if (hsdevp->dma_pending[tag] == \
-					SATA_DWC_DMA_PENDING_NONE)
-				dev_warn(ap->dev, "%s: DMA not pending?\n",
-					__func__);
-			if ((host_pvt.dma_interrupt_count % 2) == 0)
-				sata_dwc_dma_xfer_complete(ap, 1);
-		} else {
-			if (unlikely(sata_dwc_qc_complete(ap, qc, 1)))
-				goto STILLBUSY;
-		}
-		continue;
-
-STILLBUSY:
-		ap->stats.idle_irq++;
-		dev_warn(ap->dev, "STILL BUSY IRQ ata%d: irq trap\n",
-			ap->print_id);
-	} /* while tag_mask */
-
-	/*
-	 * Check to see if any commands completed while we were processing our
-	 * initial set of completed commands (read status clears interrupts,
-	 * so we might miss a completed command interrupt if one came in while
-	 * we were processing --we read status as part of processing a completed
-	 * command).
-	 */
-	sactive2 = core_scr_read(SCR_ACTIVE);
-	if (sactive2 != sactive) {
-		dev_dbg(ap->dev, "More completed - sactive=0x%x sactive2"
-			"=0x%x\n", sactive, sactive2);

You're changing the algorithm of handling command here. Is it necessary to support 2 ports?

@@ -1167,44 +1136,6 @@ static void sata_dwc_clear_dmacr(struct sata_dwc_device_port *hsdevp, u8 tag)
  	}
  }

-static void sata_dwc_dma_xfer_complete(struct ata_port *ap, u32 check_status)
-{
-	struct ata_queued_cmd *qc;
-	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
-	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
-	u8 tag = 0;
-
-	tag = ap->link.active_tag;
-	qc = ata_qc_from_tag(ap, tag);
-	if (!qc) {
-		dev_err(ap->dev, "failed to get qc");
-		return;
-	}
-
-#ifdef DEBUG_NCQ
-	if (tag > 0) {
-		dev_info(ap->dev, "%s tag=%u cmd=0x%02x dma dir=%s proto=%s "
-			 "dmacr=0x%08x\n", __func__, qc->tag, qc->tf.command,
-			 get_dma_dir_descript(qc->dma_dir),
-			 get_prot_descript(qc->tf.protocol),
-			 in_le32(&(hsdev->sata_dwc_regs->dmacr)));
-	}
-#endif
-
-	if (ata_is_dma(qc->tf.protocol)) {
-		if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_NONE) {
-			dev_err(ap->dev, "%s DMA protocol RX and TX DMA not "
-				"pending dmacr: 0x%08x\n", __func__,
-				in_le32(&(hsdev->sata_dwc_regs->dmacr)));
-		}
-
-		hsdevp->dma_pending[tag] = SATA_DWC_DMA_PENDING_NONE;
-		sata_dwc_qc_complete(ap, qc, check_status);
-		ap->link.active_tag = ATA_TAG_POISON;
-	} else {
-		sata_dwc_qc_complete(ap, qc, check_status);
-	}
-}

   Is this change related to dual port support?

@@ -1213,24 +1144,39 @@ static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc,
  	u32 mask = 0x0;
  	u8 tag = qc->tag;
  	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
-	host_pvt.sata_dwc_sactive_queued = 0;
+	int i;
+
  	dev_dbg(ap->dev, "%s checkstatus? %x\n", __func__, check_status);

-	if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_TX)
-		dev_err(ap->dev, "TX DMA PENDING\n");
-	else if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_RX)
-		dev_err(ap->dev, "RX DMA PENDING\n");
+	/* Check main status, clearing INTRQ */
+	status = ap->ops->sff_check_status(ap);
+
+	if (check_status) {
+		/* Make sure SATA port is not in BUSY state */
+		i = 0;
+		while (status & ATA_BUSY) {
+			if (++i > 10)
+				break;
+			status = ap->ops->sff_check_altstatus(ap);
+		};
+
+		if (unlikely(status & ATA_BUSY))
+			dev_err(ap->dev, "QC complete cmd=0x%02x STATUS BUSY "
+				"(0x%02x) [%d]\n", qc->tf.command, status, i);
+	}

   Is this related to dual port support?

@@ -1437,28 +1377,37 @@ static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8 tag)
  	struct ata_port *ap = qc->ap;
  	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
  	int dir = qc->dma_dir;
-	dma_chan = hsdevp->dma_chan[tag];

-	if (hsdevp->cmd_issued[tag] != SATA_DWC_CMD_ISSUED_NOT) {
+	/* Configure DMA before starting data transfer */
+	dma_chan = dma_dwc_xfer_setup(ap, hsdevp->llit_dma[tag]);
+	if (unlikely(dma_chan < 0)) {
+		dev_err(ap->dev, "%s: dma channel unavailable\n", __func__);
+		/* Offending this QC as no channel available for transfer */
+		qc->err_mask |= AC_ERR_TIMEOUT;

   Hm, is this good error code?

+		return;
+	}
+
+	/* Check if DMA should be started */
+	hsdevp->dma_chan[tag] = dma_chan;
+	if (hsdevp->sactive_queued & qcmd_tag_to_mask(tag)) {
  		start_dma = 1;
  		if (dir == DMA_TO_DEVICE)
  			hsdevp->dma_pending[tag] = SATA_DWC_DMA_PENDING_TX;
  		else
  			hsdevp->dma_pending[tag] = SATA_DWC_DMA_PENDING_RX;
[...]
@@ -1490,6 +1440,49 @@ static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
  	sata_dwc_bmdma_start_by_tag(qc, tag);
  }

+static void sata_dwc_bmdma_stop(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
+	int dma_ch, enabled;
+
+	dma_ch = hsdev->dma_channel;
+	enabled = sata_dwc_dma_chk_en(dma_ch);
+
+	if (enabled) {
+		dev_dbg(ap->dev,
+			"%s terminate DMA on channel=%d (mask=0x%08x) ...",
+			__func__, dma_ch, DMA_DISABLE_CHAN(dma_ch));
+		/* Disable the selected channel */
+		out_le32(&(sata_dma_regs->dma_chan_en.low),

   () with & not needed.

+			DMA_DISABLE_CHAN(dma_ch));
+
+		/* Wait for the channel is disabled */
+		do {
+			enabled = sata_dwc_dma_chk_en(dma_ch);
+			ndelay(1000);
+		} while (enabled);
+		dev_dbg(ap->dev, "done\n");
+	}
+}
+

   Wasn't this function necessary before dual port support?

+static u8 sata_dwc_bmdma_status(struct ata_port *ap)
+{
+	u32 status = 0;
+	u32 tfr_reg, err_reg;
+	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
+
+	/* Check DMA register for status */
+	tfr_reg = in_le32(&(sata_dma_regs->interrupt_status.tfr.low));
+	err_reg = in_le32(&(sata_dma_regs->interrupt_status.error.low));
+
+	if (unlikely(err_reg & DMA_CHANNEL(hsdev->dma_channel)))
+		status = ATA_DMA_ERR;
+	else if (tfr_reg & DMA_CHANNEL(hsdev->dma_channel))
+		status = ATA_DMA_INTR;
+	return status;
+}
+

   Is the above really related to dual port support? Wasn't it necessary before?

@@ -1500,24 +1493,22 @@ static void sata_dwc_qc_prep_by_tag(struct ata_queued_cmd *qc, u8 tag)
  {
  	struct scatterlist *sg = qc->sg;
  	struct ata_port *ap = qc->ap;
-	int dma_chan;
+	int num_lli;
  	struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
  	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);

+	if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol == ATA_PROT_PIO))

   () not neecessary around ==.

+		return;

   Wasn't this check necessary before dual port support?

  	dev_dbg(ap->dev, "%s: port=%d dma dir=%s n_elem=%d\n",
  		__func__, ap->port_no, get_dma_dir_descript(qc->dma_dir),
  		 qc->n_elem);

-	dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag],
-				      hsdevp->llit_dma[tag],
-				      (void *__iomem)(&hsdev->sata_dwc_regs->\
-				      dmadr), qc->dma_dir);
-	if (dma_chan < 0) {
-		dev_err(ap->dev, "%s: dma_dwc_xfer_setup returns err %d\n",
-			__func__, dma_chan);
-		return;
+	if (!ata_is_ncq(qc->tf.protocol)) {
+		num_lli = map_sg_to_lli(qc->ap, sg, qc->n_elem,
+			hsdevp->llit[tag], hsdevp->llit_dma[tag],
+			(void *__iomem)(&hsdev->sata_dwc_regs->dmadr),
+			qc->dma_dir);
  	}

   And what if the protocol is NCQ?

-	hsdevp->dma_chan[tag] = dma_chan;
  }

  static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
@@ -1541,19 +1532,18 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
  	sata_dwc_qc_prep_by_tag(qc, tag);

  	if (ata_is_ncq(qc->tf.protocol)) {
-		sactive = core_scr_read(SCR_ACTIVE);
+		/* Update SActive register for new command */
+		sactive = core_scr_read(qc->ap, SCR_ACTIVE);
  		sactive |= (0x00000001<<  tag);

   BTW, () not needed here. You can also use BIT() macro.

-		core_scr_write(SCR_ACTIVE, sactive);
-
-		dev_dbg(qc->ap->dev, "%s: tag=%d ap->link.sactive = 0x%08x "
-			"sactive=0x%08x\n", __func__, tag, qc->ap->link.sactive,
-			sactive);
+		core_scr_write(qc->ap, SCR_ACTIVE, sactive);
+		qc->dev->link->sactive |= 0x00000001<<  tag;

  		ap->ops->sff_tf_load(ap,&qc->tf);
-		sata_dwc_exec_command_by_tag(ap,&qc->tf, qc->tag,
-					     SATA_DWC_CMD_ISSUED_PEND);
+		sata_dwc_exec_command_by_tag(ap, &qc->tf, qc->tag);
  	} else {
-		ata_sff_qc_issue(qc);
+		/* As ata_sff_qc_issue is no longer handle DMA transfer,
+		 * change to use ata_bmdma_qc_issue instead */

   The preferred style of multi-line comments is this:

/*
 * bla
 * bla
 */

+		ata_bmdma_qc_issue(qc);

   This is a bug fix, so should be in a prior patch.

  	}
  	return 0;
  }
@@ -1582,7 +1572,12 @@ static void sata_dwc_qc_prep(struct ata_queued_cmd *qc)
  static void sata_dwc_error_handler(struct ata_port *ap)
  {
  	ap->link.flags |= ATA_LFLAG_NO_HRST;
-	ata_sff_error_handler(ap);
+	ata_bmdma_error_handler(ap);

   Same about this. I've already noted this on Friday.

+}
+
+u8 sata_dwc_check_altstatus(struct ata_port *ap)
+{
+	return ioread8(ap->ioaddr.altstatus_addr);

   This is the same as the default implementation, why you need to redefine it?

@@ -1638,13 +1637,38 @@ static int sata_dwc_probe(struct platform_device *ofdev)
  	struct ata_host *host;
  	struct ata_port_info pi = sata_dwc_port_info[0];
  	const struct ata_port_info *ppi[] = {&pi, NULL };
+	const unsigned int *dma_chan;
+
+	/* Check if device is declared in device tree */
+	if (!of_device_is_available(ofdev->dev.of_node)) {
+		printk(KERN_INFO "%s: Port disabled via device-tree\n",
+		ofdev->dev.of_node->full_name);
+		return 0;
+	}

   This check is not related to dual port support I think.


  	/* Allocate DWC SATA device */
  	hsdev = kzalloc(sizeof(*hsdev), GFP_KERNEL);

Consider switching to the managed device interface (devm_kzalloc() and friends).

  	if (hsdev == NULL) {
  		dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
  		err = -ENOMEM;
-		goto error;
+		goto error_out_5;
+	}
[...]
+	dma_chan = of_get_property(ofdev->dev.of_node, "dma-channel", NULL);

   I think you should use of_property_read_u32() instead.

@@ -1653,7 +1677,7 @@ static int sata_dwc_probe(struct platform_device *ofdev)
  		dev_err(&ofdev->dev, "ioremap failed for SATA register"
  			" address\n");
  		err = -ENODEV;
-		goto error_kmalloc;
+		goto error_out_4;
  	}
  	hsdev->reg_base = base;
  	dev_dbg(&ofdev->dev, "ioremap done for SATA register address\n");
@@ -1666,7 +1690,7 @@ static int sata_dwc_probe(struct platform_device *ofdev)
  	if (!host) {
  		dev_err(&ofdev->dev, "ata_host_alloc_pinfo failed\n");
  		err = -ENOMEM;
-		goto error_iomap;
+		goto error_out_4;

  Are you sure it's not 'error_out_3' at this point?

@@ -1688,23 +1712,30 @@ static int sata_dwc_probe(struct platform_device *ofdev)
  	if (irq == NO_IRQ) {

   You should get rid of NO_IRQ.

-	/* Initialize AHB DMAC */
-	dma_dwc_init(hsdev, irq);
+	/* Init glovbal dev list */

   s/glovbal/global/

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux