On Wed, 24 Sep 2008 15:32:19 +0400, Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> wrote:
> >>> + if (pair)
> >>> + safe = min(safe, ide_get_best_pio_mode(pair, 255, 4));
> >>> + /*
> >>> + * Update Command Transfer Mode for master/slave and Data
> >>> + * Transfer Mode for this drive.
> >>> + */
> >>> + mask = is_slave ? 0x07f00700 : 0x070007f0;
> >>> + val = (safe << 24) | (safe << 8) | (pio << (is_slave ? 20 : 4));
> >>>
> >> You are not obliged to set the same command rimings for both drives...
> >
> > I thought I should use "safe" command timings for command transfer
> > mode since taskfile registers should be considered as "shared" for
>
> Safe mode is defined as the mode not faster than the slowest drive's
> fastest mode.
>
> > both drives. At least device selection sequence should be done in
> > safe speed, isn't it?
>
> But why do you think that the PIO mode being programmed is actually
> safer for another drive than previous one which might have been slower?
Ah, so do you mean the code should be like this?
if (pair)
safe = min(safe, ide_get_best_pio_mode(pair, 255, 4));
/*
* Update Command Transfer Mode and Data Transfer Mode for this drive.
*/
mask = is_slave ? 0x07f00000 : 0x000007f0;
val = ((safe << 8) | (pio << 4)) << (is_slave ? 16 : 0);
> >>> + /* wait 12GBUSCLK (typ. 60ns @ GBUS200MHz) */
> >>> + ndelay(400);
> >>>
> >> But why wait 400 ns?
> >
> > Well, I should recalculate safe value for possible slowest gbus clock.
>
> Hm, that corresponds to 30 MHz and 6.7x that one for 200 MHz. But why
> 100 ns turns into 1 us then? Well, not that it actually matters much,
> just for consistency...
Well, while possible slowest gbus clock is 20MHz * 10 * (8 / 6) / 6 =
44MHz (not sure it _really_ work), I will choose 270ns here.
> >>> + pr_err("%s: Error interrupt %#x (%s%s%s%s )\n",
> >>> + hwif->name, stat,
> >>> + (stat & TX4939IDE_INT_ADDRERR) ?
> >>> + " Address-Error" : "",
> >>> + (stat & TX4939IDE_INT_REACHMUL) ?
> >>> + " Reach-Multiple" : "",
> >>>
> >> This is not an error condition and should only happen in so called
> >> VDMA mode iff you suspend the transfer, IIUC.
>
> I.e. when you're performing PIO transfer with drive but have
> programmed the controller for DMA transfer -- IIUC, TX4939 supports
> that. Otherwise these "break" bits don't make sense...
>
> > So just masking Reach-Multiple interrupt is better?
>
> Aren't you masking it already?
Oh yes. Then... just ignoring INT_REACHMUL is be better?
> >>> + case TX4939IDE_INT_HOST | TX4939IDE_INT_XFEREND:
> >>> + dma_stat = tx4939ide_readb(base, TX4939IDE_DMA_stat);
> >>> + if (!(dma_stat & 4))
> >>> + pr_debug("%s: weird interrupt status. "
> >>>
> >> This one is worth pr_warning() or even pr_err()...
> >>
> >>> + "DMA_stat %#02x int_ctl %#04x\n",
> >>> + hwif->name, dma_stat, ctl);
> >>>
> >> However, it's already done in the dma_end() method;.do we need
> >> really to print 2 messages?
> >
> > Yes, we don't need this usually. So I used pr_debug() instead of
> > pr_warning(). But I have no strong opinition here. I'll drop it.
>
> I suggest pr_err() or pr_warning() here and dropping it in the
> dma_end() method.
OK. I will do.
---
Atsushi Nemoto
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
