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

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

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

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

> 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