Messages in this thread | | | Date | Tue, 03 Apr 2012 15:56:52 +0400 | From | Sergei Shtylyov <> | Subject | Re: [PATCH 1/1] Add support 2 SATA ports for Maui and change filename from sata_dwc_460ex.c to sata_dwc_4xx.c |
| |
Hello.
On 03-04-2012 14:12, Thang Q. Nguyen wrote:
> Signed-off-by: Thang Q. Nguyen<tqnguyen@apm.com> > --- > Changes for v2: > - Use git rename feature to change the driver to the newname and for > easier review.
> arch/powerpc/boot/dts/bluestone.dts | 21 + > drivers/ata/Makefile | 2 +- > drivers/ata/{sata_dwc_460ex.c => sata_dwc_4xx.c} | 1371 ++++++++++++++-------- > 3 files changed, 904 insertions(+), 490 deletions(-) > rename drivers/ata/{sata_dwc_460ex.c => sata_dwc_4xx.c} (56%)
You submitted a magapatch doing several things at once (some even needlessly) and even in two areas of the kernel. This needs proper splitting/description.
> diff --git a/arch/powerpc/boot/dts/bluestone.dts b/arch/powerpc/boot/dts/bluestone.dts > index cfa23bf..803fda6 100644 > --- a/arch/powerpc/boot/dts/bluestone.dts > +++ b/arch/powerpc/boot/dts/bluestone.dts > @@ -155,6 +155,27 @@ > /*RXDE*/ 0x5 0x4>; > }; > > + /* SATA DWC devices */ > + SATA0: sata@bffd1000 { > + compatible = "amcc,sata-apm821xx"; > + reg =<4 0xbffd1000 0x800 /* SATA0 */ > + 4 0xbffd0800 0x400>; /* AHBDMA */ > + dma-channel=<0>; > + interrupt-parent =<&UIC0>; > + interrupts =<26 4 /* SATA0 */ > + 25 4>; /* AHBDMA */ > + }; > + > + SATA1: sata@bffd1800 { > + compatible = "amcc,sata-apm821xx"; > + reg =<4 0xbffd1800 0x800 /* SATA1 */ > + 4 0xbffd0800 0x400>; /* AHBDMA */ > + dma-channel=<1>; > + interrupt-parent =<&UIC0>; > + interrupts =<27 4 /* SATA1 */ > + 25 4>; /* AHBDMA */ > + }; > + > POB0: opb { > compatible = "ibm,opb"; > #address-cells =<1>;
The above should be in a separate patch for PPC people, of course.
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_4xx.c > similarity index 56% > rename from drivers/ata/sata_dwc_460ex.c > rename to drivers/ata/sata_dwc_4xx.c > index 69f7cde..bdbb35a 100644 > --- a/drivers/ata/sata_dwc_460ex.c > +++ b/drivers/ata/sata_dwc_4xx.c > @@ -1,5 +1,5 @@ > /* > - * drivers/ata/sata_dwc_460ex.c > + * drivers/ata/sata_dwc_4xx.c
This line should be removed altogether.
> * > * Synopsys DesignWare Cores (DWC) SATA host driver > * [...] > @@ -135,13 +146,12 @@ enum { > DMA_CTL_LLP_DSTEN = 0x08000000, /* Blk chain enable Dst */ > }; > > -#define DMA_CTL_BLK_TS(size) ((size)& 0x000000FFF) /* Blk Transfer size */ > +#define DMA_CTL_BLK_TS(size) ((size)& 0x000000FFF) /* Blk Transfer size */
Avoid random whitespoace changes.
> #define DMA_CHANNEL(ch) (0x00000001<< (ch)) /* Select channel */ > /* Enable channel */ > -#define DMA_ENABLE_CHAN(ch) ((0x00000001<< (ch)) | \ > - ((0x000000001<< (ch))<< 8)) > +#define DMA_ENABLE_CHAN(ch) (0x00000101<< (ch)) > /* Disable channel */ > -#define DMA_DISABLE_CHAN(ch) (0x00000000 | ((0x000000001<< (ch))<< 8)) > +#define DMA_DISABLE_CHAN(ch) (0x000000100<< (ch)) > /* Transfer Type& Flow Controller */
These cleanups are not related to adding support for 2 channels
> @@ -298,43 +313,32 @@ struct sata_dwc_device_port { > #define HSDEV_FROM_QC(qc) ((struct sata_dwc_device *)\ > (qc)->ap->host->private_data) > #define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\ > - (hsdevp)->hsdev) > + (hsdevp)->hsdev)
Avoid random whitespoace changes.
> +/* > + * Globals > + */ > +static struct sata_dwc_device *dwc_dev_list[2]; > +static struct ahb_dma_regs *sata_dma_regs;
This assumes that the system only has single controller, doesn't it?
> /* > - * Function: get_burst_length_encode > - * arguments: datalength: length in bytes of data > - * returns value to be programmed in register corresponding to data length > + * Calculate value to be programmed in register corresponding to data length > * This value is effectively the log(base 2) of the length > */ > -static int get_burst_length_encode(int datalength) > +static int get_burst_length_encode(int datalength)
Is it releated to adding support to 2 ports?
> { > int items = datalength>> 2; /* div by 4 to get lword count */ > > @@ -414,152 +416,205 @@ static int get_burst_length_encode(int datalength) > return 0; > } > > -static void clear_chan_interrupts(int c) > +/* > + * Clear channel interrupt. No interrupt for the specified channel > + * generated until it is enabled again. > + */ > +static void clear_chan_interrupts(int c) > { > - out_le32(&(host_pvt.sata_dma_regs->interrupt_clear.tfr.low), > - DMA_CHANNEL(c)); > - out_le32(&(host_pvt.sata_dma_regs->interrupt_clear.block.low), > + out_le32(&(sata_dma_regs->interrupt_clear.tfr.low), DMA_CHANNEL(c)); > + out_le32(&(sata_dma_regs->interrupt_clear.block.low), DMA_CHANNEL(c)); > + out_le32(&(sata_dma_regs->interrupt_clear.srctran.low), > DMA_CHANNEL(c)); > - out_le32(&(host_pvt.sata_dma_regs->interrupt_clear.srctran.low), > - DMA_CHANNEL(c)); > - out_le32(&(host_pvt.sata_dma_regs->interrupt_clear.dsttran.low), > - DMA_CHANNEL(c)); > - out_le32(&(host_pvt.sata_dma_regs->interrupt_clear.error.low), > + out_le32(&(sata_dma_regs->interrupt_clear.dsttran.low), > DMA_CHANNEL(c)); > + out_le32(&(sata_dma_regs->interrupt_clear.error.low), DMA_CHANNEL(c));
() with & are not necessary.
> } > > /* > - * Function: dma_request_channel > - * arguments: None > - * returns channel number if available else -1 > - * This function assigns the next available DMA channel from the list to the > - * requester > + * Check if the selected DMA channel is currently enabled. > */ > -static int dma_request_channel(void) > +static int sata_dwc_dma_chk_en(int ch) > { > - int i; > + u32 dma_chan_reg; > + /* Read the DMA channel register */ > + dma_chan_reg = in_le32(&(sata_dma_regs->dma_chan_en.low)); > + /* Check if it is currently enabled */ > + if (dma_chan_reg & DMA_CHANNEL(ch)) > + return 1; > + return 0; > +}
Is this related to the claimed subject of adding support for 2 ports?
> > - for (i = 0; i< DMA_NUM_CHANS; i++) { > - if (!(in_le32(&(host_pvt.sata_dma_regs->dma_chan_en.low))&\ > - DMA_CHANNEL(i))) > - return i; > +/* > + * Terminate the current DMA transfer > + * > + * Refer to the "Abnormal Transfer Termination" section > + * Disable the corresponding bit in the ChEnReg register > + * and poll that register to until the channel is terminated. > + */ > +static void sata_dwc_dma_terminate(struct ata_port *ap, int dma_ch) > +{ > + int enabled = sata_dwc_dma_chk_en(dma_ch); > + /* If the channel is currenly in use, release it. */ > + if (enabled) { > + dev_dbg(ap->dev, > + "%s terminate DMA on channel=%d (mask=0x%08x) ...", > + __func__, dma_ch, DMA_DISABLE_CHAN(dma_ch)); > + dev_dbg(ap->dev, "ChEnReg=0x%08x\n", > + in_le32(&(sata_dma_regs->dma_chan_en.low))); > + /* Disable the selected channel */ > + out_le32(&(sata_dma_regs->dma_chan_en.low), > + 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"); > } > - dev_err(host_pvt.dwc_dev, "%s NO channel chan_en: 0x%08x\n", __func__, > - in_le32(&(host_pvt.sata_dma_regs->dma_chan_en.low))); > +}
Same question.
> + > +/* > + * Check if the DMA channel is currently available for transferring data > + * on the specified ata_port. > + */ > +static int dma_request_channel(struct ata_port *ap) > +{ > + 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))&\ > + 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; > }
Same question.
> +/* > + * Registers ISR for a particular DMA channel interrupt > + */ > +static int dma_request_interrupts(struct sata_dwc_device *hsdev, int irq) > +{ > + int retval; > + > + /* Unmask error interrupt */ > + out_le32(&sata_dma_regs->interrupt_mask.error.low, > + in_le32(&sata_dma_regs->interrupt_mask.error.low) | > + DMA_ENABLE_CHAN(hsdev->dma_channel)); > + > + /* Unmask end-of-transfer interrupt */ > + out_le32(&sata_dma_regs->interrupt_mask.tfr.low, > + in_le32(&sata_dma_regs->interrupt_mask.tfr.low) | > + DMA_ENABLE_CHAN(hsdev->dma_channel)); > + > + retval = request_irq(irq, dma_dwc_handler, IRQF_SHARED, "SATA DMA", > + hsdev); > if (retval) { > - dev_err(host_pvt.dwc_dev, "%s: could not get IRQ %d\n", > + dev_err(hsdev->dev, "%s: could not get IRQ %d\n",\ > __func__, irq); > return -ENODEV; > } > > /* Mark this interrupt as requested */ > hsdev->irq_dma = irq; > + > return 0; > }
Same question.
> /* > - * Function: dma_dwc_exit > - * arguments: None > - * returns status > - * This function exits the SATA DMA driver > - */ > -static void dma_dwc_exit(struct sata_dwc_device *hsdev) > -{ > - dev_dbg(host_pvt.dwc_dev, "%s:\n", __func__); > - if (host_pvt.sata_dma_regs) { > - iounmap(host_pvt.sata_dma_regs); > - host_pvt.sata_dma_regs = NULL; > - } > - > - if (hsdev->irq_dma) { > - free_irq(hsdev->irq_dma, hsdev); > - hsdev->irq_dma = 0; > - } > -}
Same question.
> static int sata_dwc_scr_read(struct ata_link *link, unsigned int scr, u32 *val) > { > - if (scr> SCR_NOTIFICATION) { > + if (unlikely(scr> SCR_NOTIFICATION)) { > dev_err(link->ap->dev, "%s: Incorrect SCR offset 0x%02x\n", > __func__, scr); > return -EINVAL; > } > > *val = in_le32((void *)link->ap->ioaddr.scr_addr + (scr * 4)); > - dev_dbg(link->ap->dev, "%s: id=%d reg=%d val=val=0x%08x\n", > + dev_dbg(link->ap->dev, "%s: id=%d reg=%d val=0x%08x\n", > __func__, link->ap->print_id, scr, *val); > > return 0; > @@ -828,7 +867,7 @@ static int sata_dwc_scr_write(struct ata_link *link, unsigned int scr, u32 val) > { > dev_dbg(link->ap->dev, "%s: id=%d reg=%d val=val=0x%08x\n", > __func__, link->ap->print_id, scr, val); > - if (scr> SCR_NOTIFICATION) { > + if (unlikely(scr> SCR_NOTIFICATION)) { > dev_err(link->ap->dev, "%s: Incorrect SCR offset 0x%02x\n", > __func__, scr); > return -EINVAL;
Same question.
> @@ -838,23 +877,24 @@ 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);
Insert empty line here, please.
> + return in_le32((void __iomem *)hsdev->scr_base + (scr * 4)); > } > > -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);
And here.
> + out_le32((void __iomem *)hsdev->scr_base + (scr * 4), val); > } > > -static void clear_serror(void) > +static void clear_serror(struct ata_port *ap) > { > - u32 val; > - val = core_scr_read(SCR_ERROR); > - core_scr_write(SCR_ERROR, val); > + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
And here.
> + out_le32((void __iomem *)hsdev->scr_base + 4, > + in_le32((void __iomem *)hsdev->scr_base + 4)); > > } > > @@ -864,12 +904,105 @@ static void clear_interrupt_bit(struct sata_dwc_device *hsdev, u32 bit) > in_le32(&hsdev->sata_dwc_regs->intpr)); > } > > +/* > + * Porting the ata_bus_softreset function from the libata-sff.c library. > + */ > +static int sata_dwc_bus_softreset(struct ata_port *ap, unsigned int devmask, > + unsigned long deadline) > +{ > + struct ata_ioports *ioaddr =&ap->ioaddr; > + > + DPRINTK("ata%u: bus reset via SRST\n", ap->print_id); > + > + /* Software reset. causes dev0 to be selected */ > + iowrite8(ap->ctl, ioaddr->ctl_addr); > + udelay(20); /* FIXME: flush */ > + iowrite8(ap->ctl | ATA_SRST, ioaddr->ctl_addr); > + udelay(20); /* FIXME: flush */ > + iowrite8(ap->ctl, ioaddr->ctl_addr); > + ap->last_ctl = ap->ctl; > + > + /* Wait the port to become ready */ > + return ata_sff_wait_after_reset(&ap->link, devmask, deadline); > +}
I don't see
> + > +/* > + * Do soft reset on the current SATA link. > + */ > +static int sata_dwc_softreset(struct ata_link *link, unsigned int *classes, > + unsigned long deadline) > +{ > + int rc; > + u8 err; > + struct ata_port *ap = link->ap; > + unsigned int devmask = 0;
Why delcare it at all if it's always 0?
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap); > + > + /* Select device 0 again */ > + ap->ops->sff_dev_select(ap, 0); > + > + DPRINTK("about to softreset, devmask=%x\n", devmask); > + rc = sata_dwc_bus_softreset(ap, devmask, deadline); > + > + /* If link is occupied, -ENODEV too is an error */ > + if (rc && (rc != -ENODEV || sata_scr_valid(link))) { > + ata_link_printk(link, KERN_ERR, "SRST failed(errno=%d)\n", rc); > + return rc; > + } > + > + /* Determine by signature whether we have ATA or ATAPI devices */ > + classes[0] = ata_sff_dev_classify(&link->device[0], > + devmask& (1<< 0),&err);
Always 0, and it should be 1.
> + DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
classes[1] will always be empty.
> + clear_serror(link->ap); > + > + /* Terminate DMA if it is currently in use */ > + sata_dwc_dma_terminate(link->ap, hsdev->dma_channel);
Isn't it too late?
> + > + return rc; > +} > + > +/* > + * Reset all internal parameters to default value. > + * This function should be called in hardreset > + */ > +static void dwc_reset_internal_params(struct ata_port *ap) > +{ > + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap); > + int tag;
Empty line here please.
> + for (tag = 0; tag< SATA_DWC_QCMD_MAX; tag++) > + hsdevp->dma_pending[tag] = SATA_DWC_DMA_PENDING_NONE; > + > + hsdevp->sata_dwc_sactive_issued = 0; > + hsdevp->sata_dwc_sactive_queued = 0; > +} > + > +static int sata_dwc_hardreset(struct ata_link *link, unsigned int *classes, > + unsigned long deadline) > +{ > + int rc; > + const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context); > + bool online; > + > + /* Reset internal parameters to default values */ > + dwc_reset_internal_params(link->ap); > + > + /* Call standard hard reset */ > + rc = sata_link_hardreset(link, timing, deadline,&online, NULL); > + > + /* Reconfigure the port after hard reset */ > + if (ata_link_online(link)) > + sata_dwc_init_port(link->ap); > + > + return online ? -EAGAIN : rc; > +} > +
What does this have to do with adding support for 2 ports again?
> @@ -918,11 +1049,7 @@ static void sata_dwc_error_intr(struct ata_port *ap, > } > > /* > - * Function : sata_dwc_isr > - * arguments : irq, void *dev_instance, struct pt_regs *regs > - * Return value : irqreturn_t - status of IRQ > * This Interrupt handler called via port ops registered function. > - * .irq_handler = sata_dwc_isr > */ > static irqreturn_t sata_dwc_isr(int irq, void *dev_instance) > { > @@ -930,14 +1057,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); > > /* Read the interrupt register */ > intpr = in_le32(&hsdev->sata_dwc_regs->intpr); > @@ -958,38 +1085,61 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance) > /* Check for DMA SETUP FIS (FP DMA) interrupt */ > if (intpr& SATA_DWC_INTPR_NEWFP) { > clear_interrupt_bit(hsdev, SATA_DWC_INTPR_NEWFP); > + if (ap->qc_allocated == 0x0) { > + handled = 1; > + goto DONE; > + } > > tag = (u8)(in_le32(&hsdev->sata_dwc_regs->fptagr)); > + mask = qcmd_tag_to_mask(tag); > dev_dbg(ap->dev, "%s: NEWFP tag=%d\n", __func__, tag); > - if (hsdevp->cmd_issued[tag] != SATA_DWC_CMD_ISSUED_PEND) > + if ((hsdevp->sata_dwc_sactive_queued& mask) == 0) > dev_warn(ap->dev, "CMD tag=%d not pending?\n", tag); > > - host_pvt.sata_dwc_sactive_issued |= qcmd_tag_to_mask(tag); > - > qc = ata_qc_from_tag(ap, tag); > /* > * Start FP DMA for NCQ command. At this point the tag is the > * active tag. It is the tag that matches the command about to > * be completed. > */ > - qc->ap->link.active_tag = tag; > - sata_dwc_bmdma_start_by_tag(qc, tag); > + if (qc) { > + hsdevp->sata_dwc_sactive_issued |= mask; > + /* Prevent to issue more commands */ > + qc->ap->link.active_tag = tag; > + qc->dev->link->sactive |= (1<< qc->tag); > + 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), \ > + qc->dma_dir); > + sata_dwc_bmdma_start_by_tag(qc, tag); > + wmb(); > + qc->ap->hsm_task_state = HSM_ST_LAST; > + } else { > + hsdevp->sata_dwc_sactive_issued&= ~mask; > + dev_warn(ap->dev, "No QC available for tag %d (intpr=" > + "0x%08x, qc_allocated=0x%08x, qc_active=0x%08x)\n", tag,\ > + intpr, ap->qc_allocated, ap->qc_active);
Indent the above preperly with tabs.
> + } > [...] > @@ -1167,70 +1245,51 @@ 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); > - } > -}
What does this chenge have to do with the claimed target of thye patch.
> static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc, > u32 check_status) > { > - u8 status = 0; > - u32 mask = 0x0; > + u8 status; > + int i; > u8 tag = qc->tag; > struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap); > - host_pvt.sata_dwc_sactive_queued = 0; > + u32 serror; > 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) { > + 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); > + serror = core_scr_read(ap, SCR_ERROR); > + if (unlikely(serror& SATA_DWC_SERROR_ERR_BITS)) > + dev_err(ap->dev, "****** SERROR=0x%08x ******\n", > + serror); > + } > dev_dbg(ap->dev, "QC complete cmd=0x%02x status=0x%02x ata%u:" > " protocol=%d\n", qc->tf.command, status, ap->print_id, > qc->tf.protocol); > > - /* clear active bit */ > - mask = (~(qcmd_tag_to_mask(tag))); > - host_pvt.sata_dwc_sactive_queued = (host_pvt.sata_dwc_sactive_queued) \ > - & mask; > - host_pvt.sata_dwc_sactive_issued = (host_pvt.sata_dwc_sactive_issued) \ > - & mask; > - ata_qc_complete(qc); > + hsdevp->sata_dwc_sactive_issued&= ~qcmd_tag_to_mask(tag); > + > + /* Complete taskfile transaction (does not read SCR registers) */ > + if (ata_is_atapi(qc->tf.protocol)) > + ata_sff_hsm_move(ap, qc, status, 0); > + else > + ata_qc_complete(qc); > + > + if (hsdevp->sata_dwc_sactive_queued == 0) > + ap->link.active_tag = ATA_TAG_POISON; > + > return 0; > }
Same question.
> +static void sata_dwc_init_port(struct ata_port *ap) > +{ > + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap); > + > + /* Configure DMA */ > + if (ap->port_no == 0) { > + dev_dbg(ap->dev, "%s: clearing TXCHEN, RXCHEN in DMAC\n", > + __func__); > + > + /* Clear all transmit/receive bits */ > + out_le32(&hsdev->sata_dwc_regs->dmacr, > + SATA_DWC_DMACR_TXRXCH_CLEAR); > + > + dev_dbg(ap->dev, "%s: setting burst size DBTSR\n", __func__); > + out_le32(&hsdev->sata_dwc_regs->dbtsr, > + (SATA_DWC_DBTSR_MWR(AHB_DMA_BRST_DFLT) | > + SATA_DWC_DBTSR_MRD(AHB_DMA_BRST_DFLT)));
Why not put the above init. in a separate function, instead of associating with channehl 0?
> + } > + > + /* Enable interrupts */ > + sata_dwc_enable_interrupts(hsdev); > +} > + > static void sata_dwc_setup_port(struct ata_ioports *port, unsigned long base) > { > port->cmd_addr = (void *)base + 0x00; > @@ -1276,10 +1359,7 @@ static void sata_dwc_setup_port(struct ata_ioports *port, unsigned long base) > } > > /* > - * Function : sata_dwc_port_start > - * arguments : struct ata_ioports *port > - * Return value : returns 0 if success, error code otherwise > - * This function allocates the scatter gather LLI table for AHB DMA > + * Allocates the scatter gather LLI table for AHB DMA > */ > static int sata_dwc_port_start(struct ata_port *ap) > { > @@ -1287,6 +1367,7 @@ static int sata_dwc_port_start(struct ata_port *ap) > struct sata_dwc_device *hsdev; > struct sata_dwc_device_port *hsdevp = NULL; > struct device *pdev; > + u32 sstatus; > int i; > > hsdev = HSDEV_FROM_AP(ap); > @@ -1308,12 +1389,10 @@ static int sata_dwc_port_start(struct ata_port *ap) > err = -ENOMEM; > goto CLEANUP; > } > + memset(hsdevp, 0, sizeof(*hsdevp));
We already called kzalloc(), so the allocated buffer is already cleared.
> hsdevp->hsdev = hsdev; > > - for (i = 0; i< SATA_DWC_QCMD_MAX; i++) > - hsdevp->cmd_issued[i] = SATA_DWC_CMD_ISSUED_NOT; > - > - ap->bmdma_prd = 0; /* set these so libata doesn't use them */ > + ap->bmdma_prd = 0; /* set these so libata doesn't use them */
Avoid random whitespace changes.
> @@ -1347,32 +1426,47 @@ static int sata_dwc_port_start(struct ata_port *ap) > } > > /* Clear any error bits before libata starts issuing commands */ > - clear_serror(); > + clear_serror(ap); > ap->private_data = hsdevp; > + > + /* Are we in Gen I or II */ > + sstatus = core_scr_read(ap, SCR_STATUS); > + switch (SATA_DWC_SCR0_SPD_GET(sstatus)) { > + case 0x0: > + dev_info(ap->dev, "**** No neg speed (nothing attached?)\n"); > + break; > + case 0x1: > + dev_info(ap->dev, "**** GEN I speed rate negotiated\n"); > + break; > + case 0x2: > + dev_info(ap->dev, "**** GEN II speed rate negotiated\n"); > + break; > + } > +
libata will negoptiate the speed, why this is needed?
> static void sata_dwc_port_stop(struct ata_port *ap) > { > int i; > - struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap); > struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap); > > dev_dbg(ap->dev, "%s: ap->id = %d\n", __func__, ap->print_id); > > - if (hsdevp&& hsdev) { > - /* deallocate LLI table */ > + if (hsdevp) { > + /* De-allocate LLI table */ > for (i = 0; i< SATA_DWC_QCMD_MAX; i++) { > dma_free_coherent(ap->host->dev, > - SATA_DWC_DMAC_LLI_TBL_SZ, > - hsdevp->llit[i], hsdevp->llit_dma[i]); > + SATA_DWC_DMAC_LLI_TBL_SZ, > + hsdevp->llit[i], hsdevp->llit_dma[i]);
It was properly indented before.
> @@ -1381,15 +1475,76 @@ static void sata_dwc_port_stop(struct ata_port *ap) > } > > /* > - * Function : sata_dwc_exec_command_by_tag > - * arguments : ata_port *ap, ata_taskfile *tf, u8 tag, u32 cmd_issued > - * Return value : None > - * This function keeps track of individual command tag ids and calls > - * ata_exec_command in libata > + * As our SATA is master only, no dev_select function needed. > + * This just overwrite the ata_sff_dev_select() function in > + * libata-sff > + */ > +void sata_dwc_dev_select(struct ata_port *ap, unsigned int device) > +{ > + ndelay(100);
Why?
> +} > + > +/** > + * Filter ATAPI cmds which are unsuitable for DMA. > + * > + * The bmdma engines cannot handle speculative data sizes > + * (bytecount under/over flow). So only allow DMA for > + * data transfer commands with known data sizes. > + */ > +static int sata_dwc_check_atapi_dma(struct ata_queued_cmd *qc) > +{ > + struct scsi_cmnd *scmd = qc->scsicmd; > + int pio = 1; /* ATAPI DMA disabled by default */ > + unsigned int lba; > + > + if (scmd) { > + switch (scmd->cmnd[0]) { > + case WRITE_6: > + case WRITE_10: > + case WRITE_12: > + case READ_6: > + case READ_10: > + case READ_12: > + pio = 0; /* DMA is safe */ > + break; > + } > + > + /* Command WRITE_10 with LBA between -45150 (FFFF4FA2) > + * and -1 (FFFFFFFF) shall use PIO mode */ > + if (scmd->cmnd[0] == WRITE_10) { > + lba = (scmd->cmnd[2]<< 24) | > + (scmd->cmnd[3]<< 16) | > + (scmd->cmnd[4]<< 8) | > + scmd->cmnd[5]; > + if (lba>= 0xFFFF4FA2) > + pio = 1; > + } > + /* > + * WORK AROUND: Fix DMA issue when blank CD/DVD disc > + * in the drive and user use the 'fdisk -l' command. > + * No DMA data returned so we can not complete the QC. > + */ > + if (scmd->cmnd[0] == READ_10) { > + lba = (scmd->cmnd[2]<< 24) | > + (scmd->cmnd[3]<< 16) | > + (scmd->cmnd[4]<< 8) | > + scmd->cmnd[5]; > + if (lba< 0x20) > + pio = 1; > + } > + } > + dev_dbg(qc->ap->dev, "%s - using %s mode for command cmd=0x%02x\n", \ > + __func__, (pio ? "PIO" : "DMA"), scmd->cmnd[0]); > + return pio; > +}
ATAPI support is a different matter then 2-port support. Needs to be in a separate patch.
> @@ -1437,42 +1588,54 @@ static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8 tag) [...] > dev_dbg(ap->dev, "%s qc=%p tag: %x cmd: 0x%02x dma_dir: %s " > "start_dma? %x\n", __func__, qc, tag, qc->tf.command, > get_dma_dir_descript(qc->dma_dir), start_dma); > - sata_dwc_tf_dump(&(qc->tf)); > + sata_dwc_tf_dump(hsdev->dev, &(qc->tf));
() with & not necessary.
> > + /* Enable to start DMA transfer */ > if (start_dma) { > - reg = core_scr_read(SCR_ERROR); > - if (reg& SATA_DWC_SERROR_ERR_BITS) { > + reg = core_scr_read(ap, SCR_ERROR); > + if (unlikely(reg& SATA_DWC_SERROR_ERR_BITS)) { > dev_err(ap->dev, "%s: ****** SError=0x%08x ******\n", > __func__, reg);
libata will print SError register...
> } > > - if (dir == DMA_TO_DEVICE) > + if (dir == DMA_TO_DEVICE) { > out_le32(&hsdev->sata_dwc_regs->dmacr, > SATA_DWC_DMACR_TXCHEN); > - else > + } else { > out_le32(&hsdev->sata_dwc_regs->dmacr, > SATA_DWC_DMACR_RXCHEN); > + } > > /* Enable AHB DMA transfer on the specified channel */ > - dma_dwc_xfer_start(dma_chan); > + dwc_dma_xfer_start(dma_chan); > + hsdevp->sata_dwc_sactive_queued&= ~qcmd_tag_to_mask(tag); > } > } > @@ -1490,34 +1653,98 @@ static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc) > sata_dwc_bmdma_start_by_tag(qc, tag); > } > > +static u8 sata_dwc_dma_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 | ATA_DMA_INTR;
Error bit in BMIDE (SFF-8038i) specification doesn't cause interrupt.
> + else if (tfr_reg& DMA_CHANNEL(hsdev->dma_channel)) > + status = ATA_DMA_INTR; > + return status; > +} > + [...] > + > +int sata_dwc_qc_defer(struct ata_queued_cmd *qc) > +{ > + struct ata_port *ap = qc->ap; > + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap); > + u8 status; > + int ret; > + > + dev_dbg(qc->ap->dev, "%s -\n", __func__); > + ret = ata_std_qc_defer(qc); > + if (ret) { > + printk(KERN_DEBUG "STD Defer %s cmd %s tag=%d\n", > + (ret == ATA_DEFER_LINK) ? "LINK" : "PORT", > + ata_get_cmd_descript(qc->tf.command), qc->tag); > + return ret; > + } > + > + /* Check the SATA host for busy status */ > + if (ata_is_ncq(qc->tf.protocol)) { > + status = ap->ops->sff_check_altstatus(ap); > + if (status& ATA_BUSY) { > + dev_dbg(ap->dev, > + "Defer PORT cmd %s tag=%d as host is busy\n", > + ata_get_cmd_descript(qc->tf.command), qc->tag); > + return ATA_DEFER_PORT;/*HOST BUSY*/ > + } > + > + /* This will prevent collision error */ > + if (hsdevp->sata_dwc_sactive_issued) { > + dev_dbg(ap->dev, "Defer PORT cmd %s with tag %d " \ > + "because another dma xfer is outstanding\n", > + ata_get_cmd_descript(qc->tf.command), qc->tag);
What kind of NCQ is this if you can't start another comamnd when some are active already?!
> + > + return ATA_DEFER_PORT;/*DEVICE&HOST BUSY*/ > + } > + > + } > + > + return 0; > +}
> +void sata_dwc_exec_command(struct ata_port *ap, const struct ata_taskfile *tf) > +{ > + iowrite8(tf->command, ap->ioaddr.command_addr); > + /* If we have an mmio device with no ctl and no altstatus > + * method, this will fail. No such devices are known to exist. > + */ > + if (ap->ioaddr.altstatus_addr)
Isn't it always set?
> + ioread8(ap->ioaddr.altstatus_addr); > + > + ndelay(400); > }
Why duplicate the standard sff_exec_command() method at all?
> @@ -1525,6 +1752,8 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc) > u32 sactive; > u8 tag = qc->tag; > struct ata_port *ap = qc->ap; > + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(qc->ap); > + u8 status; > > #ifdef DEBUG_NCQ > if (qc->tag> 0 || ap->link.sactive> 1) > @@ -1541,50 +1770,148 @@ 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); > + status = ap->ops->sff_check_altstatus(ap); > + if (status& ATA_BUSY) { > + /* Ignore the QC when device is BUSY */ > + sactive = core_scr_read(qc->ap, SCR_ACTIVE); > + dev_info(ap->dev, "Ignore current QC as device BUSY" > + "tag=%d, sactive=0x%08x)\n", qc->tag, sactive); > + return AC_ERR_SYSTEM; > + } > + > + if (hsdevp->sata_dwc_sactive_issued) > + return AC_ERR_SYSTEM;
Very strange NCQ... was there a point in implementing it at all?
> + > + sactive = core_scr_read(qc->ap, SCR_ACTIVE); > sactive |= (0x00000001<< tag); > - core_scr_write(SCR_ACTIVE, sactive); > + qc->dev->link->sactive |= (0x00000001<< tag);
() not needed.
> + core_scr_write(qc->ap, 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=0x%x\n", __func__, tag, qc->ap->link.sactive,
Why?
> sactive); > > 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); > + ap->link.active_tag = qc->tag; > + /* Pass QC to libata-sff to process */ > + ata_bmdma_qc_issue(qc);
This don't have to do with the claimed subject of the patch.
> } > return 0; > } > > /* > - * Function : sata_dwc_qc_prep > - * arguments : ata_queued_cmd *qc > - * Return value : None > - * qc_prep for a particular queued command > + * Prepare for a particular queued command > */ > > static void sata_dwc_qc_prep(struct ata_queued_cmd *qc) > { > - if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol == ATA_PROT_PIO)) > + if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol == ATA_PROT_PIO) > + || (qc->tf.protocol == ATAPI_PROT_PIO))
Adding support for ATAPI is another matter than adding support for two ports. Should be in a patch of its own.
> return; > > #ifdef DEBUG_NCQ > if (qc->tag> 0) > dev_info(qc->ap->dev, "%s: qc->tag=%d ap->active_tag=0x%08x\n", > __func__, qc->tag, qc->ap->link.active_tag); > - > - return ; > #endif > } > > +/* > + * Get the QC currently used for transferring data > + */ > +static struct ata_queued_cmd *sata_dwc_get_active_qc(struct ata_port *ap) > +{ > + struct ata_queued_cmd *qc; > + > + qc = ata_qc_from_tag(ap, ap->link.active_tag); > + if (qc&& !(qc->tf.flags& ATA_TFLAG_POLLING)) > + return qc; > + return NULL; > +} > + > +/* > + * dwc_lost_interrupt - check and process if interrupt is lost. > + * @ap: ATA port > + * > + * Process the command when it is timeout. > + * Check to see if interrupt is lost. If yes, complete the qc. > + */ > +static void sata_dwc_lost_interrupt(struct ata_port *ap) > +{ > + u8 status; > + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap); > + struct ata_queued_cmd *qc; > + > + dev_dbg(ap->dev, "%s -\n", __func__); > + /* Only one outstanding command per SFF channel */ > + qc = sata_dwc_get_active_qc(ap); > + /* We cannot lose an interrupt on a non-existent or polled command */ > + if (!qc) > + return; > + > + /* See if the controller thinks it is still busy - if so the command > + isn't a lost IRQ but is still in progress */ > + status = ap->ops->sff_check_altstatus(ap); > + if (status& ATA_BUSY) { > + ata_port_printk(ap, KERN_INFO, "%s - ATA_BUSY\n", __func__); > + return; > + } > + > + /* There was a command running, we are no longer busy and we have > + no interrupt. */ > + ata_link_printk(qc->dev->link, KERN_WARNING, > + "lost interrupt (Status 0x%x)\n", status); > + > + if (sata_dwc_dma_chk_en(hsdev->dma_channel)) { > + /* When DMA does transfer does not complete, > + see if DMA fails */ > + qc->err_mask |= AC_ERR_DEV; > + ap->hsm_task_state = HSM_ST_ERR; > + sata_dwc_dma_terminate(ap, hsdev->dma_channel); > + } > + sata_dwc_qc_complete(ap, qc, 1); > +} > + > + > static void sata_dwc_error_handler(struct ata_port *ap) > { > - ap->link.flags |= ATA_LFLAG_NO_HRST; > + bool thaw = false; > + struct ata_queued_cmd *qc; > + u8 status = ap->ops->bmdma_status(ap); > + > + qc = sata_dwc_get_active_qc(ap); > + /* In case of DMA timeout, process it. */ > + if (qc && ata_is_dma(qc->tf.protocol)) { > + if ((qc->err_mask == AC_ERR_TIMEOUT) > + && (status & ATA_DMA_ERR)) { > + qc->err_mask = AC_ERR_HOST_BUS; > + thaw = true; > + } > + > + if (thaw) { > + ap->ops->sff_check_status(ap); > + if (ap->ops->sff_irq_clear) > + ap->ops->sff_irq_clear(ap); > + } > + } > + if (thaw) > + ata_eh_thaw_port(ap); > + > ata_sff_error_handler(ap); > } >
I don't think this goes well with adding support for 2 ports. Seems to be material for another patch.
[...] > +u8 sata_dwc_check_status(struct ata_port *ap) > +{ > + return ioread8(ap->ioaddr.status_addr); > +}
This method is equivalent to ata_sff_check_status(), why redefine it?
> + > +u8 sata_dwc_check_altstatus(struct ata_port *ap) > +{ > + return ioread8(ap->ioaddr.altstatus_addr); > +}
This method is optional. The above is equivalent to the default implementnation, why redefine it?
> @@ -1604,7 +1931,10 @@ static struct ata_port_operations sata_dwc_ops = { > .inherits =&ata_sff_port_ops, > > .error_handler = sata_dwc_error_handler, > + .softreset = sata_dwc_softreset, > + .hardreset = sata_dwc_hardreset, > > + .qc_defer = sata_dwc_qc_defer, > .qc_prep = sata_dwc_qc_prep, > .qc_issue = sata_dwc_qc_issue, > > @@ -1614,8 +1944,17 @@ static struct ata_port_operations sata_dwc_ops = { > .port_start = sata_dwc_port_start, > .port_stop = sata_dwc_port_stop, > > + .check_atapi_dma = sata_dwc_check_atapi_dma, > .bmdma_setup = sata_dwc_bmdma_setup, > .bmdma_start = sata_dwc_bmdma_start, > + .bmdma_status = sata_dwc_dma_status, > + > + .sff_dev_select = sata_dwc_dev_select, > + .sff_check_status = sata_dwc_check_status, > + .sff_check_altstatus = sata_dwc_check_altstatus, > + .sff_exec_command = sata_dwc_exec_command, > + > + .lost_interrupt = sata_dwc_lost_interrupt, > }; > > static const struct ata_port_info sata_dwc_port_info[] = { > @@ -1639,21 +1978,49 @@ static int sata_dwc_probe(struct platform_device *ofdev) > struct ata_port_info pi = sata_dwc_port_info[0]; > const struct ata_port_info *ppi[] = {&pi, NULL }; >
Why empty line here?
> + const unsigned int *dma_chan; > + > /* Allocate DWC SATA device */ > - hsdev = kzalloc(sizeof(*hsdev), GFP_KERNEL); > + hsdev = kmalloc(sizeof(*hsdev), GFP_KERNEL);
Why change kzalloc() to kmalloc() if you do memset() later?
> if (hsdev == NULL) { > dev_err(&ofdev->dev, "kmalloc failed for hsdev\n"); > err = -ENOMEM; > - goto error; > + goto error_out_5; > } > + memset(hsdev, 0, sizeof(*hsdev)); > [...] > + /* Identify SATA controller index from the cell-index property */
Comment don't match the code?
> + dma_chan = of_get_property(ofdev->dev.of_node, "dma-channel", NULL); > + if (dma_chan) { > + dev_notice(&ofdev->dev, "Getting DMA channel %d\n", *dma_chan); > + hsdev->dma_channel = *dma_chan; > + } else { > + hsdev->dma_channel = 0; > + } > + [...] > @@ -1777,7 +2159,18 @@ static struct platform_driver sata_dwc_driver = { > .remove = sata_dwc_remove, > }; > > -module_platform_driver(sata_dwc_driver); > +static int __init sata_dwc_init(void) > +{ > + return platform_driver_register(&sata_dwc_driver); > +} > + > +static void __exit sata_dwc_exit(void) > +{ > + platform_driver_unregister(&sata_dwc_driver); > +} > + > +module_init(sata_dwc_init); > +module_exit(sata_dwc_exit);
Why you changed this from module_platfrom_driver()?
MBR, Sergei
| |