| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4F7AE584.3050805@mvista.com>
Date: Tue, 03 Apr 2012 15:56:52 +0400
From: Sergei Shtylyov <sshtylyov@...sta.com>
To: "Thang Q. Nguyen" <tqnguyen@....com>
CC: Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Jeff Garzik <jgarzik@...ox.com>,
Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <rob.herring@...xeda.com>,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
linux-ide@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org
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@....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@...d1000 {
> + 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@...d1800 {
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists