Prev: [GIT PULL] omap fixes for 2.6.35-rc3
Next: Lieber Freund!!!
From: Josh Boyer on 30 Jun 2010 16:10
On Wed, Jun 30, 2010 at 03:17:05PM -0400, Jeff Garzik wrote:
> On 06/30/2010 02:47 PM, Wolfgang Denk wrote:
>> Dear Rupjyoti Sarmah,
>>
>> In message<3b928476b2fffdcf0694e5436e8a482f(a)mail.gmail.com> you wrote:
>>>
>>> I took the mainline kernel v2.6.35-rc3 and downloaded using the git
>>> download link.
>>> I created the patch on 6/24/2010 after doing a git pull.
>>
>> I don;t think that you used v2.6.35-rc3; using this version, I still
>> get this:
>>
>> drivers/ata/sata_dwc_460ex.c:43:1: warning: "DRV_NAME" redefined
>> In file included from drivers/ata/sata_dwc_460ex.c:38:
>> drivers/ata/libata.h:31:1: warning: this is the location of the previous definition
>> drivers/ata/sata_dwc_460ex.c:44:1: warning: "DRV_VERSION" redefined
>> drivers/ata/libata.h:32:1: warning: this is the location of the previous definition
>> drivers/ata/sata_dwc_460ex.c: In function 'sata_dwc_scr_read':
>> drivers/ata/sata_dwc_460ex.c:777: error: 'struct ata_port' has no member named 'ioaddr'
>> drivers/ata/sata_dwc_460ex.c: In function 'sata_dwc_scr_write':
>> drivers/ata/sata_dwc_460ex.c:793: error: 'struct ata_port' has no member named 'ioaddr'
>> drivers/ata/sata_dwc_460ex.c: In function 'sata_dwc_error_intr':
>> drivers/ata/sata_dwc_460ex.c:844: error: 'struct ata_port_operations' has no member named 'sff_check_status'
>
> It looks like -your- build config is missing CONFIG_ATA_SFF or similar.
>
> If you actually look at include/linux/libata.h, you see that struct
> ata_port_operations most definitely has a member named sff_check_status,
> for example. Ditto ata_port and member ioaddr, etc.

The driver doesn't depend on CONFIG_ATA_SFF in it's Kconfig file, but seems to
require it at build time. Isn't that something that needs fixing in the
driver?

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Wolfgang Denk on 30 Jun 2010 19:00
Dear Josh Boyer,

In message <20100630200325.GD7756(a)zod.rchland.ibm.com> you wrote:
>
> The driver doesn't depend on CONFIG_ATA_SFF in it's Kconfig file, but seems to
> require it at build time. Isn't that something that needs fixing in the
> driver?

Right. Next question is if this is really needed for this driver.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd(a)denx.de
Copy from one, it's plagiarism; copy from two, it's research.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Rupjyoti Sarmah on 1 Jul 2010 13:50
Dear All,

The Synopsis design ware core is task file orientated so the driver would
still need CONFIG_ATA_SFF.
I would be fixing the Kconfig file to make it dependent on the
CONFIG_ATA_SFF.

Regards,
Rup



-----Original Message-----
From: Wolfgang Denk [mailto:wd(a)denx.de]
Sent: Thursday, July 01, 2010 4:25 AM
To: Josh Boyer
Cc: Jeff Garzik; linux-ide(a)vger.kernel.org; sr(a)denx.de; Rupjyoti Sarmah;
linux-kernel(a)vger.kernel.org; linuxppc-dev(a)ozlabs.org
Subject: Re: [PATCH v1]460EX on-chip SATA driver<resubmisison>

Dear Josh Boyer,

In message <20100630200325.GD7756(a)zod.rchland.ibm.com> you wrote:
>
> The driver doesn't depend on CONFIG_ATA_SFF in it's Kconfig file, but
seems to
> require it at build time. Isn't that something that needs fixing in the
> driver?

Right. Next question is if this is really needed for this driver.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd(a)denx.de
Copy from one, it's plagiarism; copy from two, it's research.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jeff Garzik on 14 Jul 2010 04:00
On 07/06/2010 07:06 AM, Rupjyoti Sarmah wrote:
> This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
>
> Signed-off-by: Rupjyoti Sarmah<rsarmah(a)appliedmicro.com>
> Signed-off-by: Mark Miesfeld<mmiesfeld(a)appliedmicro.com>
> Signed-off-by: Prodyut Hazarika<phazarika(a)appliedmicro.com>
>
> ---
> This patch incorporates the changes advised in the mailing list. The device
> tree changes were submitted as a seperate patch. Kconfig file is fixed in this version.
>
> drivers/ata/Kconfig | 9 +
> drivers/ata/Makefile | 1 +
> drivers/ata/sata_dwc_460ex.c | 1756 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1766 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/sata_dwc_460ex.c

applied


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Sergei Shtylyov on 17 Jul 2010 12:00
Hello.

Rupjyoti Sarmah wrote:

> This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.

Too bad thius has already been applied but here's my (mostly stylistic)
comments anyway:

> Signed-off-by: Rupjyoti Sarmah <rsarmah(a)appliedmicro.com>
> Signed-off-by: Mark Miesfeld <mmiesfeld(a)appliedmicro.com>
> Signed-off-by: Prodyut Hazarika <phazarika(a)appliedmicro.com>

[...]

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> new file mode 100644
> index 0000000..ea24c1e
> --- /dev/null
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -0,0 +1,1756 @@
> +/*
> + * drivers/ata/sata_dwc_460ex.c

Filenames in the heading comments have long been frowned upon.

> +#ifdef CONFIG_SATA_DWC_DEBUG

I don't see this option defined anywahere.

> +#define DEBUG
> +#endif
> +
> +#ifdef CONFIG_SATA_DWC_VDEBUG

The same about this one.

> +#define VERBOSE_DEBUG
> +#define DEBUG_NCQ
> +#endif
[...]
> +/* SATA DMA driver Globals */
> +#define DMA_NUM_CHANS 1
> +#define DMA_NUM_CHAN_REGS 8
> +
> +/* SATA DMA Register definitions */
> +#define AHB_DMA_BRST_DFLT 64 /* 16 data items burst length*/

Please put a space before */.

> +struct ahb_dma_regs {
> + struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS];
> + struct dma_interrupt_regs interrupt_raw; /* Raw Interrupt */
> + struct dma_interrupt_regs interrupt_status; /* Interrupt Status */
> + struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask */
> + struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear */
> + struct dmareg statusInt; /* Interrupt combined*/

No camelCase please, rename it to status_int.

> +#define DMA_CTL_BLK_TS(size) ((size) & 0x000000FFF) /* Blk Transfer size */
> +#define DMA_CHANNEL(ch) (0x00000001 << (ch)) /* Select channel */
> + /* Enable channel */
> +#define DMA_ENABLE_CHAN(ch) ((0x00000001 << (ch)) | \
> + ((0x000000001 << (ch)) << 8))
> + /* Disable channel */
> +#define DMA_DISABLE_CHAN(ch) (0x00000000 | ((0x000000001 << (ch)) << 8))

What's the point of OR'ing with zero?

> +/*
> + * Commonly used DWC SATA driver Macros
> + */
> +#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
> + (host)->private_data)
> +#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
> + (ap)->host->private_data)
> +#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
> + (ap)->private_data)
> +#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)

Are you sure it's '(hsdevp)', not '(p)'?

> +struct sata_dwc_host_priv {
> + void __iomem *scr_addr_sstatus;
> + u32 sata_dwc_sactive_issued ;
> + u32 sata_dwc_sactive_queued ;

Remove spaces befoer semicolons, please.

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

There's no need to use \ outside macro defintions.

> +/*
> + * Function: get_burst_length_encode
> + * arguments: datalength: length in bytes of data
> + * returns value to be programmed in register corrresponding to data length
> + * This value is effectively the log(base 2) of the length
> + */
> +static int get_burst_length_encode(int datalength)
> +{
> + int items = datalength >> 2; /* div by 4 to get lword count */
> +
> + if (items >= 64)
> + return 5;
> +
> + if (items >= 32)
> + return 4;
> +
> + if (items >= 16)
> + return 3;
> +
> + if (items >= 8)
> + return 2;
> +
> + if (items >= 4)
> + return 1;
> +
> + return 0;
> +}

Hmm, there should be a function in the kernel to calculate 2^n order from
size, something like get_count_order()...

> +/*
> + * Function: dma_dwc_interrupt
> + * arguments: irq, dev_id, pt_regs
> + * returns channel number if available else -1
> + * Interrupt Handler for DW AHB SATA DMA
> + */
> +static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
> +{
> + int chan;
> + u32 tfr_reg, err_reg;
> + unsigned long flags;
> + struct sata_dwc_device *hsdev =
> + (struct sata_dwc_device *)hsdev_instance;
> + struct ata_host *host = (struct ata_host *)hsdev->host;
> + struct ata_port *ap;
> + struct sata_dwc_device_port *hsdevp;
> + u8 tag = 0;
> + unsigned int port = 0;
> +
> + 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\

There's no need to use \ outside macro defintions.

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

Same comment here...

> + for (chan = 0; chan < DMA_NUM_CHANS; chan++) {
> + /* Check for end-of-transfer interrupt. */
> + if (tfr_reg & DMA_CHANNEL(chan)) {
> + /*
> + * Each DMA command produces 2 interrupts. Only
> + * complete the command after both interrupts have been
> + * seen. (See sata_dwc_isr())
> + */
> + host_pvt.dma_interrupt_count++;
> + sata_dwc_clear_dmacr(hsdevp, tag);
> +
> + if (hsdevp->dma_pending[tag] ==
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "DMA not pending eot=0x%08x "
> + "err=0x%08x tag=0x%02x pending=%d\n",
> + tfr_reg, err_reg, tag,
> + hsdevp->dma_pending[tag]);
> + }
> +
> + if ((host_pvt.dma_interrupt_count % 2) == 0)

Prens around % operation not necessary.

> + sata_dwc_dma_xfer_complete(ap, 1);
> +
> + /* Clear the interrupt */
> + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\

\ not needed.

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

\ not needed.

> +/*
> + * Function: map_sg_to_lli
> + * The Synopsis driver has a comment proposing that better performance
> + * is possible by only enabling interrupts on the last item in the linked list.
> + * However, it seems that could be a problem if an error happened on one of the
> + * first items. The transfer would halt, but no error interrupt would occur.
> + * Currently this function sets interrupts enabled for each linked list item:
> + * DMA_CTL_INT_EN.
> + */
> +static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *dmadr_addr, int dir)
> +{
[...]
> + /* Program the LLI CTL high register */
> + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\

\ not needed.

> + (len / 4));
> +
> + /* Program the next pointer. The next pointer must be
> + * the physical address, not the virtual address.
> + */
> + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \

Same here...

> +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)
> +{
> + int dma_ch;
> + int num_lli;

There should be an empty line here.

> +/*
> + * Function: dma_dwc_init
> + * arguments: hsdev
> + * returns status
> + * This function initializes the SATA DMA driver
> + */
> +static int dma_dwc_init(struct sata_dwc_device *hsdev, int irq)
> +{
> + int err;
> +
> + err = dma_request_interrupts(hsdev, irq);

Could be initilaizer...

> + dev_notice(host_pvt.dwc_dev, "DMA initialized\n");
> + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\

\ not needed.

> +static u32 core_scr_read(unsigned int scr)
> +{
> + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\

Not needed.

> +static void clear_serror(void)
> +{
> + u32 val;

Empty line needed here.

> + val = core_scr_read(SCR_ERROR);

This could be an initilaizer.

> +/* See ahci.c */
> +static void sata_dwc_error_intr(struct ata_port *ap,
> + struct sata_dwc_device *hsdev, uint intpr)
> +{
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct ata_eh_info *ehi = &ap->link.eh_info;
> + unsigned int err_mask = 0, action = 0;
> + struct ata_queued_cmd *qc;
> + u32 serror;
> + u8 status, tag;
> + u32 err_reg;
> +
> + ata_ehi_clear_desc(ehi);
> +
> + serror = core_scr_read(SCR_ERROR);
> + status = ap->ops->sff_check_status(ap);

Why not call ata_sff_check_status() directly?

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

\ not neeeded.

> +/*
> + * 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)
> +{
> + struct ata_host *host = (struct ata_host *)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;
> + struct sata_dwc_device_port *hsdevp;
> + host_pvt.sata_dwc_sactive_issued = 0;
[...]
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;

Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?

> + /* If no sactive issued and tag_mask is zero then this is not NCQ */
> + if (host_pvt.sata_dwc_sactive_issued == 0 && tag_mask == 0) {
> + if (ap->link.active_tag == ATA_TAG_POISON)
> + tag = 0;
> + else
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> +
> + /* DEV interrupt w/ no active qc? */
> + if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
> + dev_err(ap->dev, "%s interrupt with no active qc "
> + "qc=%p\n", __func__, qc);
> + ap->ops->sff_check_status(ap);

Call ata_sff_check_status() directly...

> + handled = 1;
> + goto DONE;
> + }
> + status = ap->ops->sff_check_status(ap);

Here too...

> +DRVSTILLBUSY:
> + if (ata_is_dma(qc->tf.protocol)) {
> + /*
> + * Each DMA transaction produces 2 interrupts. The DMAC
> + * transfer complete interrupt and the SATA controller
> + * operation done interrupt. The command should be
> + * completed only after both interrupts are seen.
> + */
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "%s: DMA not pending "
> + "intpr=0x%08x status=0x%08x pending"
> + "=%d\n", __func__, intpr, status,
> + hsdevp->dma_pending[tag]);
> + }

Curly brace not needed here. I assume you haven't run the patch thru
scripts/checkpatch.pl?

> + /*
> + * This is a NCQ command. At this point we need to figure out for which
> + * tags we have gotten a completion interrupt. One interrupt may serve
> + * as completion for more than one operation when commands are queued
> + * (NCQ). We need to process each completed command.
> + */
> +
> + /* process completed commands */
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;

Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?

> + if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \

Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not needed.

> + tag_mask > 1) {
> + dev_dbg(ap->dev, "%s NCQ:sactive=0x%08x sactive_issued=0x%08x"
> + "tag_mask=0x%08x\n", __func__, sactive,
> + host_pvt.sata_dwc_sactive_issued, tag_mask);
> + }

Curly braces not needed here.

> + if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
> + (host_pvt.sata_dwc_sactive_issued)) {

Useless parens around 'host_pvt.sata_dwc_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,
> + tag_mask);
> + }

Curly braces not needed around single statement.

> +
> + /* read just to clear ... not bad if currently still busy */
> + status = ap->ops->sff_check_status(ap);

Call ata_sff_check_status() directly.

> + 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);

Useless parens.

> + /* Process completed command */
> + dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n", __func__,
> + ata_get_cmd_descript(qc->tf.protocol));
> + if (ata_is_dma(qc->tf.protocol)) {
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \

\ not needed.

> + SATA_DWC_DMA_PENDING_NONE)
> + dev_warn(ap->dev, "%s: DMA not pending?\n",
> + __func__);
> + if ((host_pvt.dma_interrupt_count % 2) == 0)

Parens not needed around % operation.

> + sata_dwc_dma_xfer_complete(ap, 1);
> + } else {
> + if (unlikely(sata_dwc_qc_complete(ap, qc, 1)))
> + goto STILLBUSY;
> + }

You should write:

} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}

> + /*
> + * 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) {

{} not needed here.

> +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) {

Curly braces not needed around single statement.

> + 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,
> + ata_get_cmd_descript(qc->dma_dir),
> + ata_get_cmd_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) {

Curly braces not needed around single statement.

> +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 tag = qc->tag;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);

There should be an empty line here.

> + host_pvt.sata_dwc_sactive_queued = 0;
> + 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");
> + 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)));

Useless parens.

> + 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) \

\ not 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 */
> + for (i = 0; i < SATA_DWC_QCMD_MAX; i++) {

Curly braces not needed.

> + dma_free_coherent(ap->host->dev,
> + SATA_DWC_DMAC_LLI_TBL_SZ,
> + hsdevp->llit[i], hsdevp->llit_dma[i]);

Should be indented on the same level as above line

> +static void sata_dwc_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }

Curly braces not needed around single statements.

> +static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
> + int start_dma;
> + u32 reg, dma_chan;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_QC(qc);
> + struct ata_port *ap = qc->ap;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + int dir = qc->dma_dir;

There should be an empty line here.

> + if (start_dma) {
> + reg = core_scr_read(SCR_ERROR);
> + if (reg & SATA_DWC_SERROR_ERR_BITS) {

Curly braces not needed here.

> +static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }

Curly braces not needed around single statements.

> +/*
> + * Function : sata_dwc_qc_prep_by_tag
> + * arguments : ata_queued_cmd *qc, u8 tag
> + * Return value : None
> + * qc_prep for a particular queued command based on tag
> + */
> +static void sata_dwc_qc_prep_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
[...]
> + dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag],
> + hsdevp->llit_dma[tag],
> + (void *__iomem)(&hsdev->sata_dwc_regs->\

\ not needed.

> +static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
> +{
> + u32 sactive;
> + u8 tag = qc->tag;
> + struct ata_port *ap = qc->ap;
[...]
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + sactive = core_scr_read(SCR_ACTIVE);
> + sactive |= (0x00000001 << tag);

Parens not needed.

> + 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);
> +
> + ap->ops->sff_tf_load(ap, &qc->tf);

Why not call ata_sff_tf_load() directly?

> +/*
> + * Function : sata_dwc_qc_prep
> + * arguments : ata_queued_cmd *qc
> + * Return value : None
> + * qc_prep 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))

Parens not needed arounf == operation.

> + return;
> +
> +#ifdef DEBUG_NCQ
> + if (qc->tag > 0)
> + dev_info(qc->ap->dev, "%s: qc->tag=%d ap->active_tag=0x%08x\n",
> + __func__, tag, qc->ap->link.active_tag);
> +
> + return ;

Remove spade before semicolon please.

> +static const struct ata_port_info sata_dwc_port_info[] = {
> + {
> + .flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> + ATA_FLAG_MMIO | ATA_FLAG_NCQ,
> + .pio_mask = 0x1f, /* pio 0-4 */

Replace 0x1f with ATA_PIO4, please.

> +static int sata_dwc_probe(struct of_device *ofdev,
> + const struct of_device_id *match)

Shouldn't this function be '__init' or '__devinit'?

> +{
> + struct sata_dwc_device *hsdev;
> + u32 idr, versionr;
> + char *ver = (char *)&versionr;
> + u8 *base = NULL;
> + int err = 0;
> + int irq, rc;
> + struct ata_host *host;
> + struct ata_port_info pi = sata_dwc_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + /* Allocate DWC SATA device */
> + hsdev = kmalloc(sizeof(*hsdev), GFP_KERNEL);
> + if (hsdev == NULL) {
> + dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
> + err = -ENOMEM;
> + goto error_out;
> + }
> + memset(hsdev, 0, sizeof(*hsdev));

Use kzalloc() instead iof kmalloc()/memset().

> + /* Get physical SATA DMA register base address */
> + host_pvt.sata_dma_regs = of_iomap(ofdev->dev.of_node, 1);
> + if (!(host_pvt.sata_dma_regs)) {

Parens not needed around 'host_pvt.sata_dma_regs'.

> + /*
> + * Now, register with libATA core, this will also initiate the
> + * device discovery process, invoking our port_start() handler &
> + * error_handler() to execute a dummy Softreset EH session
> + */
> + rc = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> +

I think that empty line here is not needed.

> + if (rc != 0)
> + dev_err(&ofdev->dev, "failed to activate host");
> +
> + dev_set_drvdata(&ofdev->dev, host);
> + return 0;
> +
> +error_out:
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + if (base)
> + iounmap(base);

What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?

> +static int sata_dwc_remove(struct of_device *ofdev)

Shouldn't this be '__exit' or '__devexit'?

> +{
> + struct device *dev = &ofdev->dev;
> + struct ata_host *host = dev_get_drvdata(dev);
> + struct sata_dwc_device *hsdev = host->private_data;
> +
> + ata_host_detach(host);
> + dev_set_drvdata(dev, NULL);
> +
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + iounmap(hsdev->reg_base);

What about unmapping 'host_pvt.sata_dma_regs'?

> + kfree(hsdev);
> + kfree(host);

Does kfree() really pair with ata_host_alloc_pinfo()?

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: [GIT PULL] omap fixes for 2.6.35-rc3
Next: Lieber Freund!!!