linux-mips
[Top] [All Lists]

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

To: sshtylyov@ru.mvista.com
Subject: Re: [PATCH 1/2] ide: Add tx4939ide driver
From: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Date: Sat, 13 Sep 2008 00:59:04 +0900 (JST)
Cc: linux-mips@linux-mips.org, linux-ide@vger.kernel.org, bzolnier@gmail.com, ralf@linux-mips.org
In-reply-to: <48CA8BEE.1090305@ru.mvista.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <48C851ED.4090607@ru.mvista.com> <20080912.005243.48535230.anemo@mba.ocn.ne.jp> <48CA8BEE.1090305@ru.mvista.com>
Sender: linux-mips-bounce@linux-mips.org
On Fri, 12 Sep 2008 19:34:06 +0400, Sergei Shtylyov <sshtylyov@ru.mvista.com> 
wrote:
> >>   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.

It might be.  I'll try it.

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

Well, let me explain a bit.  The datasheed say I should wait _both_
XFERINT and HOST interrupt.  So, if only one of them was asserted, I
mask it and wait another one.  But on the error case, only HOST was
asserted and XFERINT was never asserted.  Then I could not exit from
"waiting another one" state, until timeout.

> >>>+                  pr_debug("%s: weired interrupt status. "
> >>>  
> 
> >>   Weird.
> 
> > Sure.  But it can happen IIRC...
> 
>     I meant the typo. :-)

Oh, thanks!

> >>>+  __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...

SPARC also have it.  And there were some discussions for ARM IIRC.

> >>>+  .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.

Yes, I see, thanks.

---
Atsushi Nemoto

<Prev in Thread] Current Thread [Next in Thread>