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

From: Sergei Shtylyov
Date: Sun Apr 08 2012 - 15:37:59 EST

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/