> On Wednesday 11 February 2009, Kumar, Purushotam wrote:
> > > @@ -914,7 +927,12 @@ static inline int handle_core_command(
> > > status = readl(host->base + DAVINCI_MMCST0);
> > > if (!status)
> > > break;
> > > +
> > > qstatus |= status;
> > > + if (count++ > 40) {
> > > + dev_dbg(mmc_dev(host->mmc), "PIO timeout\n");
> > > + break;
> > > + }
> > > }
> > >
> > > if (qstatus & MMCST0_DATDNE) {
>
> > There will be no write to FIFO and so MMCST0_DXRDY/MMCST0_DRRDY
> > will not be set again and so no next interrupt which could cause
> > hang. In order to fix this bug, we need to check count value before
> > DAVINCI_MMCST0 register read. But, I feel we should not add any
> > PIO timeout here. I checked on EVM and count has never increased
> > more than 1 for high speed card.
>
> If it it only gets to one, then there's no need for the loop in
> the first place. I'm a bit curious why that loop is there in the
> first place ... it shouldn't actually be needed.
This loop was added because we have observed earlier on non-linux operating
system that MMC gives very fast interrupt when FIFO is 32 bytes which has
caused stack overflow. In my test, count has never increased more than 1 but it
may be more than 1 for some or other card. It will be better idea to write to
FIFO when DXRDY/ DRRDY bit is already set instead of going out and coming back
2nd time immediately to IRQ.
At first glance, it seems that it is unbounded while loop. But it has proper
break if "status" is zero. All bits in register DAVINCI_MMCST0 are cleared by
read and so read of DAVINCI_MMCST0 will certainly return zero after few
iteration irrespective of any condition because controller does not report same
error again. So, I feel there is no change required and we should leave it as
it is. What is your view on this?
> Note there's another unbounded loop in the driver, which I didn't
> fix: the main irq handler, mmc_davinci_irq(), which is calling
> that handle_core_command() thing. The handle_core() thing ought
> to be the main body of the IRQ handler; without looping; and
> that "spurious IRQ branch should return IRQ_NONE (and never even
> happen, for that matter, easily arranged).
>
> - Dave
Ya, You are right mmc_davinci_irq(), which is calling that
handle_core_command() in the unbounded loop. Do we really need this loop at
first place? I tried after removing this unbounded loop and it seems that
everything is working. What is your view on this? I agree with you that we
should have IRQ_NONE.
-Purushotam
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
