by Haavard Skinnemoen

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] Atmel MCI: Driver for Atmel on-chip MMC controllers

On Thu, 26 Jun 2008 15:23:23 +0200
Haavard Skinnemoen <[email protected]> wrote:

> This driver can also use PIO transfers when no DMA channels are
> available, and for transfers where using DMA may be difficult or
> impractical for some reason (e.g. the DMA setup overhead is usually
> not worth it for very short transfers, and badly aligned buffers or
> lengths are difficult to handle.)

Btw, it's probably not that hard to rip the DMA bits out and post them
as a separate patch. This would mean that:
* Pierre can merge the driver independently of the other 5 patches
* A separate patch adding DMA support would make it clearer how the
DMA slave interface is used.
* The chances of having MMC support out of the box on avr32 boards in
2.6.27 become greater, and many people have been asking about that
(including Pierre and David.)

The driver is surprisingly fast with DMA turned off (2-3 MiB/s), but
the CPU usage is of course horrible.

If that sounds like a good plan to you, I'll split the driver tomorrow.

This driver has been out of tree for way too long. I'm hoping we can
get it in before 2.6.27.

Haavard

2008-06-27 19:56:47

On Sat, 28 Jun 2008 14:43:13 +0200
Haavard Skinnemoen <[email protected]> wrote:

> Tests 15 and 17 return -EILSEQ instead of -ETIMEDOUT. The at91_mci
> driver has the same problem, and I think it's a hardware issue -- the
> controller wrongly flags a CRC error instead of a data timeout error
> if the card doesn't respond with any CRC status after a write. I don't
> know how to work around that problem.

If that's how the hardware behaves, then EILSEQ will have to do. The
test is more about forcing people to do proper error management in the
driver than anything else. Have a check that you don't report a bad
bytes_xfered though.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org

WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.

Attachments:

signature.asc (197.00 B)

2008-06-28 14:01:38

by Haavard Skinnemoen

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] Atmel MCI: Driver for Atmel on-chip MMC controllers

Pierre Ossman <[email protected]> wrote:
> On Sat, 28 Jun 2008 14:43:13 +0200
> Haavard Skinnemoen <[email protected]> wrote:
>
> > Tests 15 and 17 return -EILSEQ instead of -ETIMEDOUT. The at91_mci
> > driver has the same problem, and I think it's a hardware issue -- the
> > controller wrongly flags a CRC error instead of a data timeout error
> > if the card doesn't respond with any CRC status after a write. I don't
> > know how to work around that problem.
>
> If that's how the hardware behaves, then EILSEQ will have to do. The
> test is more about forcing people to do proper error management in the
> driver than anything else. Have a check that you don't report a bad
> bytes_xfered though.

bytes_xfered is 0 if any block failed. If I understand correctly, this
is good enough, but not optimal. I want to improve this later, but I
might need some more feedback from the DMA engine subsystem (e.g.
adding "actual" and "status" fields to the descriptor.)

The DMA slave interface isn't perfect yet, but I think the current
incarnation is actually useful and performs well even though it's very
basic. We can make incremental improvements later to improve error
reporting, offer more advanced control over the transfers, and support
other use cases better (e.g. audio.)

Haavard

2008-06-28 14:11:42

by Pierre Ossman

[permalink] [raw]

Subject: Re: [PATCH v4 6/6] Atmel MCI: Driver for Atmel on-chip MMC controllers

On Sat, 28 Jun 2008 16:01:21 +0200
Haavard Skinnemoen <[email protected]> wrote:

>
> bytes_xfered is 0 if any block failed. If I understand correctly, this
> is good enough, but not optimal. I want to improve this later, but I
> might need some more feedback from the DMA engine subsystem (e.g.
> adding "actual" and "status" fields to the descriptor.)
>

That's good enough yes. The only incorrect value is reporting more than
was actually written as that would completely undermine any attempts of
keeping data integrity in upper layers.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org

WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.

Attachments:

signature.asc (197.00 B)

2008-06-29 16:51:07

On Fri, 4 Jul 2008 16:33:53 +0100
"Sosnowski, Maciej" <[email protected]> wrote:
> Coulpe of questions and comments from my side below.
> Apart from that the code looks fine to me.
>
> Acked-by: Maciej Sosnowski <[email protected]>

Thanks a lot for reviewing!

> > +/* Called with dwc->lock held and bh disabled */
> > +static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc
> *first)
> > +{
> > + struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > +
> > + /* ASSERT: channel is idle */
> > + if (dma_readl(dw, CH_EN) & dwc->mask) {
> > + dev_err(&dwc->chan.dev,
> > + "BUG: Attempted to start non-idle channel\n");
> > + dev_err(&dwc->chan.dev,
> > + " SAR: 0x%x DAR: 0x%x LLP: 0x%x CTL:
> 0x%x:%08x\n",
> > + channel_readl(dwc, SAR),
> > + channel_readl(dwc, DAR),
> > + channel_readl(dwc, LLP),
> > + channel_readl(dwc, CTL_HI),
> > + channel_readl(dwc, CTL_LO));
> > +
> > + /* The tasklet will hopefully advance the queue... */
> > + return;
>
> Should not at this point an error status be returned
> so that it can be handled accordingly by dwc_dostart() caller?

There's not a whole lot of meaningful things to do for the caller. It
should never happen in the first place, but if the channel _is_ active
at this point, we will eventually get an xfer complete interrupt when
the currently pending transfers are done. The descriptors have already
been added to the list, so the driver should recover from this kind of
bug automatically.

I've never actually triggered this code, so I can't really say for
certain that it works, but at least in theory it makes much more sense
to fix things up when the channel eventually becomes idle.

> > + ctllo = DWC_DEFAULT_CTLLO
> > + | DWC_CTLL_DST_WIDTH(dst_width)
> > + | DWC_CTLL_SRC_WIDTH(src_width)
> > + | DWC_CTLL_DST_INC
> > + | DWC_CTLL_SRC_INC
> > + | DWC_CTLL_FC_M2M;
> > + prev = first = NULL;
> > +
> > + for (offset = 0; offset < len; offset += xfer_count <<
> src_width) {
> > + xfer_count = min_t(size_t, (len - offset) >>
> src_width,
> > + DWC_MAX_COUNT);
>
> Here it looks like the maximum xfer_count value can change - it depends
> on src_width,
> so it may be different for different transactions.
> Is that ok?

Yes, the maximum tranfer count is defined as the maximum number of
source transactions on the bus. So if the controller is set up to do 32
bits at a time on the source side, the maximum transfer _length_ is
four times the maximum transfer _count_.

The value written to the descriptor is also a transaction count, not a
byte count.

> This driver does not perform any self-test during initialization.
> What about adding some initial HW checking?

I'm not sure if it makes a lot of sense -- this device is typically
integrated on the same silicon as the CPU, so if there are any issues
with the DMA controller, they should be caught during production
testing.

I'm using the dmatest module for validating the driver, so I feel the
self-test stuff becomes somewhat redundant.

Haavard