From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Fri, 21 Nov 2008 18:14:54 +0000 (GMT) Received: from h155.mvista.com ([63.81.120.155]:29204 "EHLO imap.sh.mvista.com") by ftp.linux-mips.org with ESMTP id S23819605AbYKUSOv (ORCPT ); Fri, 21 Nov 2008 18:14:51 +0000 Received: from [192.168.1.234] (unknown [10.150.0.9]) by imap.sh.mvista.com (Postfix) with ESMTP id EF7213ECB; Fri, 21 Nov 2008 10:14:44 -0800 (PST) Message-ID: <4926FA97.201@ru.mvista.com> Date: Fri, 21 Nov 2008 21:14:47 +0300 From: Sergei Shtylyov Organization: MontaVista Software Inc. User-Agent: Mozilla/5.0 (X11; U; Linux i686; rv:1.7.2) Gecko/20040803 X-Accept-Language: ru, en-us, en-gb MIME-Version: 1.0 To: David Daney Cc: linux-ide@vger.kernel.org, linux-mips Subject: Re: [PATCH] ide: New libata driver for OCTEON SOC Compact Flash interface. References: <49261BE5.2010406@caviumnetworks.com> <4926E467.9020305@ru.mvista.com> <4926EFEE.2010701@caviumnetworks.com> In-Reply-To: <4926EFEE.2010701@caviumnetworks.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 21373 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: sshtylyov@ru.mvista.com Precedence: bulk X-list: linux-mips Hello. David Daney wrote: >>> The register definitions are part of the chip support patch set >>> mentioned above, and are not included here. >>> At this point I would like to get feedback on the patch and would >>> expect that it would merge via the linux-mips tree along with the rest >>> of the chip support. >> Why a libata driver should be merged via the linus-mips tree? > I don't have a strong preference. The driver depends on the processor > support *and* it is only relevant to OCTEON based boards. If the Most SoC IDE drivers are like that. > linux-ide maintainers would prefer to merge it via their tree that is > fine with me. >>> Thanks, >>> Signed-off-by: David Daney >>> +static void octeon_cf_set_dmamode(struct ata_port *ap, struct >>> ata_device *dev) >>> +{ >> [...] >>> + switch (dev->dma_mode) { >>> + case XFER_MW_DMA_0: >>> + dma_ackh = 20; /* Tj */ >>> + To = 480; >>> + Td = 215; >>> + Tkr = 50; >> It's safer to use Tkw instead as it's 215 ns minimum in this mode >> (in other modes Tkr and Tkw minimums are the same). >> Again, To, Td, and Tkw are returned by ata_timing_find_mode(). >>> + /* Td (Seem to need more margin here.... */ >>> + oe_a = Td + 20; >> Well, you shouldn't... > Hmm... Shouldn't need more margin -- that why those a minimum timings. >>> +/** >>> + * Handle an I/O request. >>> + * >>> + * @cf: Device to access >>> + * @lba_sector: Starting sector >>> + * @num_sectors: >>> + * Number of sectors to transfer >>> + * @buffer: Data buffer >>> + * @write: Is the a write. Default to a read >>> + */ >>> +static unsigned int octeon_cf_data_xfer(struct ata_device *dev, >>> + unsigned char *buffer, >>> + unsigned int buflen, >>> + int rw) >>> +{ >>> + struct ata_port *ap = dev->link->ap; >>> + struct octeon_cf_data *ocd = ap->dev->platform_data; >>> + void __iomem *data_addr = ap->ioaddr.data_addr; >>> + unsigned int words; >>> + unsigned int count; >>> + >>> + /* >>> + * Odd lengths are not supported. We should always be a >>> + * multiple of 512. >>> + */ >>> + BUG_ON(buflen & 1); >>> + if (ocd->is16bit) { >>> + words = buflen / 2; >>> + if (rw) { >>> + count = 16; >>> + while (words--) { >>> + iowrite16(*(uint16_t *)buffer, data_addr); >> >> >> Not seeing the reason to use iowrite16() and not writew() as >> registers are always memory mapped... > I will consider that. Now that I think again, neither of those is correct. We must treat the data as a stream of bytes. >>> + buffer += sizeof(uint16_t); >>> + /* >>> + * Every 16 writes do a read so the >>> + * bootbus FIFO doesn't fill up. >>> + */ >>> + if (--count == 0) { >>> + ioread8(ap->ioaddr.altstatus_addr); >>> + count = 16; >>> + } >> This is a strange/slow way of doing anything every 16 data writes. > Oh, how would you do something every 16 writes? count = words > 16 ? 16 : words; words -= count; iowrite16_rep(ptr, data_addr, count); ioread(ap->ioaddr.altstatus_addr); >> Why not use iowrite16_rep()? > Two reasons. It is broken, and we need to do something every 16 writes. It's not broken -- using iowrite16() is wrong. > We need to be careful not to be anti-social by filling the FIFO. Once > the FIFO is filled other cores will be starved of I/O resources. It's not the first time I'm seeing this driver, so I'm familiar with the trick. Altho I didn't realize that this abuses the shared write buffer. > [...] >>> +static void octeon_cf_tf_read16(struct ata_port *ap, struct >>> ata_taskfile *tf) >>> +{ >>> + u16 blob; >>> + /* The base of the registers is at ioaddr.data_addr. */ >>> + void __iomem *base = ap->ioaddr.data_addr; >>> + >>> + blob = __raw_readw(base + 0xc); >>> + tf->feature = blob >> 8; >> How come the error register gets mapped at offset 0xC? >> (Well, that would explain alike part in another CF driver.) > In 16 bit mode it overlaps the data register, so it must be accessed via > the 0xc address. Actually, these register *always* overlap. That's the way that IDE thing works. :-) But I'm getting it now -- no way to differ the data read/write from the error/feature register read/write in 16-bit mode. >>> +/** >>> + * Check if any queued commands have more DMAs, if so start the next >>> + * transfer, else do standard handling. >>> + */ >>> +irqreturn_t octeon_cf_interrupt(int irq, void *dev_instance) >> Is that a normal IDE interrupt, or some Octeon specific interrupt? > No, It is OCTEON specific. It fires when a DMA operation is complete. > As I mentioned earlier, the normal IDE interrupt is not connected to > anything on most boards. That's not exactly a good design desicion... >>> +{ >>> + struct ata_host *host = dev_instance; >>> + struct octeon_cf_port *cf_port; >>> + int i; >>> + unsigned int handled = 0; >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&host->lock, flags); >>> + >>> + DPRINTK("ENTER\n"); "ENTER" needs "EXIT", no? >>> + for (i = 0; i < host->n_ports; i++) { >>> + struct ata_port *ap; >>> + struct ata_queued_cmd *qc; >>> + >>> + ap = host->ports[i]; >>> + if (!ap || (ap->flags & ATA_FLAG_DISABLED)) >>> + continue; >>> + >>> + qc = ata_qc_from_tag(ap, ap->link.active_tag); >>> + if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) && >>> + (qc->flags & ATA_QCFLAG_ACTIVE)) { >>> + u8 status = octeon_cf_bmdma_status(ap); >>> + if ((status & ATA_DMA_INTR) >>> + && !(status & ATA_DMA_ACTIVE) >>> + && !sg_is_last(qc->cursg)) { >>> + qc->cursg = sg_next(qc->cursg); >>> + handled = 1; >>> + octeon_cf_bmdma_start(qc); >>> + } else { >>> + status = ioread8(ap->ioaddr.altstatus_addr); >>> + if (status & ATA_BUSY) { >> If it's normal IDE interrupt, > Which it is not. So... >> BSY bit will always be 0 > ...this doesn't hold, thus all this code. Not necessarily. Depends on when the DMA interrupt occurs. Although it probably predates the IDE interrupt... >>> +static int __devinit octeon_cf_probe(struct platform_device *pdev) >>> +{ >>> + struct resource *res_cs0, *res_cs1; >>> + >>> + void __iomem *cs0; >>> + void __iomem *cs1; >> [...] >>> + >>> + res_cs0 = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + res_cs1 = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> [...] >>> + base = cs0 + ocd->base_region_bias; >>> + if (!ocd->is16bit) { >> [...] >>> + ap->ioaddr.altstatus_addr = base + 0xe; >>> + ap->ioaddr.ctl_addr = base + 0xe; >> Wait, why have 2 MMIO resources then? > The boards that are 'TrueIDE' use two resources, others only one. But your probe will abort seeing only one resource. I think this needs to be changed. MBR, Sergei