Re: [PATCH 1/2] ide: Add tx4939ide driver

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Atsushi Nemoto wrote:

>>>+static void tx4939ide_check_error_ints(ide_hwif_t *hwif, u16 stat)
>>>+{
>>>+	if (stat & TX4939IDE_INT_BUSERR) {
>>>+		unsigned long base = TX4939IDE_BASE(hwif);
>>>+		/* reset FIFO */
>>>+		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) |
>>>+				 0x4000,
>>>+				 base, Sys_Ctl);

>>   Are you sure bit 14 is self-clearing? The datashhet doesn't seem to 
>>say that...

> Well, I cannot remember...  I thought I checked that bit cleard by
> reading it, but actually the bit is write-only.  Maybe clearing
> explicitly would be a safe bet.

    It's also write-only on TC86C001, and the original driver (as well as 
mine) cleared it explicitly.

>>>+	rc = __tx4939ide_dma_setup(drive);
>>>+	if (rc == 0) {
>>>+		/* Number of sectors to transfer. */
>>>+		nframes = 0;
>>>+		for (i = 0; i < hwif->sg_nents; i++)
>>>+			nframes += sg_dma_len(&hwif->sg_table[i]);
>>>+		BUG_ON(nframes % sect_size != 0);
>>>+		nframes /= sect_size;
>>>+		BUG_ON(nframes == 0);
>>>+		TX4939IDE_writew(nframes, base, Sec_Cnt);

>>   Ugh, it looks much easier in my TC86C001 driver... doesn't 
>>hwgroup->rq->nr_sectors give you a number of 512 sectors?
>>Why bother with other (multiple of 512) sizes when you can always 
>>program transfer in 512-byte sectors? Or was I wrong there?

    Anyway, the TX3939 datasheet says that sector size must be a multiple of 
256 words when transferring more than 1 sector.

> Hmm.  Good idea.  I will try it.

    At least it worked with a CD-ROM for me. :-)

>>>+static int tx4939ide_dma_end(ide_drive_t *drive)
>>>+{
>>>+	if ((dma_stat & 7) == 0 &&
>>>+	    (ctl & (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST)) ==
>>>+	    (TX4939IDE_INT_XFEREND | TX4939IDE_INT_HOST))
>>>+		/* INT_IDE lost... bug? */
>>>+		return 0;

>>   You shouldn't fake the BMDMA interrupt. If it's not there, it's not 
>>there. Or does this actually happen?

> IIRC, Yes.

    Hum, let me think... worth printing a message if this happens.

>>>+		/*
>>>+		 * If only one of XFERINT and HOST was asserted, mask
>>>+		 * this interrupt and wait for an another one.  Note

>>   This comment somewhat contradicts the code which returns 1 if only 
>>HOST interupt is asserted if ERR is set.

    Which is not its business to test. I think you should remove that above 
check -- if there's INTRQ asserted, then it's asserted. I wonder if BMIDE 
interrupt bit gets set in that case (suspecting it's not)...

> Indeed.  I will make the comment more precise.

>>>+	case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
>>>+		dma_stat = TX4939IDE_readb(base, DMA_stat);
>>>+		if (!(dma_stat & 4))
>>>+			pr_debug("%s: weired interrupt status. "
>>>  

>>   Weird.

> Sure.  But it can happen IIRC...

    I meant the typo. :-)

>>>#ifdef __BIG_ENDIAN
>>>+/* custom iops (independent from SWAP_IO_SPACE) */
>>>  
>>>+static u8 mm_inb(unsigned long port)
>>>+{
>>>+	return (u8)readb((void __iomem *)port);
>>>+}
>>>+static void mm_outb(u8 value, unsigned long port)
>>>+{
>>>+	writeb(value, (void __iomem *)port);
>>>+}
>>>+static void mm_tf_load(ide_drive_t *drive, ide_task_t *task)
>>>+{
>>>  

>>[...]

>>>+	if (task->tf_flags & IDE_TFLAG_OUT_DEVICE) {
>>>+		unsigned long base = TX4939IDE_BASE(hwif);
>>>+		mm_outb((tf->device & HIHI) | drive->select,
>>>+			 io_ports->device_addr);

>>   I'm seeing no sense in re-defining so far...

>>>+		/* Fix ATA100 CORE System Control Register */
>>>+		TX4939IDE_writew(TX4939IDE_readw(base, Sys_Ctl) & 0x07f0,
>>>+				 base, Sys_Ctl);

>>   Ah... you're doing it here (but not in LE mode?). I think to avoid 
>>duplicating ide_tf_load() you need ot use selectproc().

> Oh, my fault.  LE mode also needs this fix.  I still need ide_tf_load
> on BE mode to support IDE_TFLAG_OUT_DATA.

   Yeah, that totally useless flag...

>>>+static void mm_insw_swap(unsigned long port, void *addr, u32 count)
>>>+{
>>>+	unsigned short *ptr = addr;
>>>+	unsigned long size = count * 2;
>>>+	port &= ~1;
>>>+	while (count--)
>>>+		*ptr++ = le16_to_cpu(__raw_readw((void __iomem *)port));
>>>+	__ide_flush_dcache_range((unsigned long)addr, size);

>>   Why is this needed BTW?

> Do you mean __ide_flush_dcache_range?  This is needed to avoid cache
> inconsistency on PIO drive.  PIO transfer only writes to cache but
> upper layers expects the data is in main memory.

    Hum, then I wonder why it's MIPS specific...

>>>+static const struct ide_tp_ops tx4939ide_tp_ops = {
>>>+	.exec_command		= ide_exec_command,
>>>+	.read_status		= ide_read_status,
>>>+	.read_altstatus		= ide_read_altstatus,
>>>+	.read_sff_dma_status	= tx4939ide_read_sff_dma_status,

>>   Hum, it should be re-defined in both LE and BE mode (but actually not 
>>called anyway).

> What do you mean?  Please elaborate?

    I mean that in LE mode you're using ide_read_sff_dma_status() but not 
setting hwif->dma_base, so it won't work. But since it shouldn't be called in 
this driver's case, this doesn't hurt.

MBR, Sergei