Fix SH DMAC code to correctly handle PVR2 cascade DMA.
This updates http://lkml.org/lkml/2007/10/2/276
(I decided it was better to have the true size of the transfer put in
via the API and refactor this here. And calc_xmit_shift(chan) should
return 5 but only returns 3 so I've not used it here)
--- arch/sh/drivers/dma/dma-sh.c 2007/09/22 18:34:42 1.1
+++ arch/sh/drivers/dma/dma-sh.c 2007/10/02 20:53:49 1.3
@@ -150,6 +150,13 @@ static void sh_dmac_disable_dma(struct d
static int sh_dmac_xfer_dma(struct dma_channel *chan)
{
+ /* Handle Dreamcast PVR cascade */
+ if (mach_is_dreamcast() && chan->chan == PVR2_CASCADE_CHAN) {
+ ctrl_outl(chan->sar, SAR[chan->chan]);
+ /* Transfer in 32 byte blocks */
+ ctrl_outl((chan->count) >> 5, DMATCR[chan->chan]);
+ return 0;
+ }
/*
* If we haven't pre-configured the channel with special flags, use
* the defaults.
@@ -159,26 +166,9 @@ static int sh_dmac_xfer_dma(struct dma_c
sh_dmac_disable_dma(chan);
- /*
- * Single-address mode usage note!
- *
- * It's important that we don't accidentally write any value to SAR/DAR
- * (this includes 0) that hasn't been directly specified by the user if
- * we're in single-address mode.
- *
- * In this case, only one address can be defined, anything else will
- * result in a DMA address error interrupt (at least on the SH-4),
- * which will subsequently halt the transfer.
- *
- * Channel 2 on the Dreamcast is a special case, as this is used for
- * cascading to the PVR2 DMAC. In this case, we still need to write
- * SAR and DAR, regardless of value, in order for cascading to work.
- */
- if (chan->sar || (mach_is_dreamcast() &&
- chan->chan == PVR2_CASCADE_CHAN))
+ if (chan->sar)
ctrl_outl(chan->sar, SAR[chan->chan]);
- if (chan->dar || (mach_is_dreamcast() &&
- chan->chan == PVR2_CASCADE_CHAN))
+ if (chan->dar)
ctrl_outl(chan->dar, DAR[chan->chan]);
ctrl_outl(chan->count >> calc_xmit_shift(chan), DMATCR[chan->chan]);
On Tue, Oct 02, 2007 at 10:09:27PM +0100, Adrian McMenamin wrote:
> Fix SH DMAC code to correctly handle PVR2 cascade DMA.
>
> This updates http://lkml.org/lkml/2007/10/2/276
>
> (I decided it was better to have the true size of the transfer put in
> via the API and refactor this here. And calc_xmit_shift(chan) should
> return 5 but only returns 3 so I've not used it here)
>
It would be helpful to know why calc_xmit_shift() is broken here rather
than just coding around it, as this will have implications for the other
DMA channels on SH7091/SH7750.
Now that you've completely bypassed the rest of the SH-DMAC ->xfer_dma()
op, it's clear that the existing infrastructure needs a bit of rework for
dealing with the cascaded DMACs (especially for single-address mode only,
unidirectionally). It would be nice to get the mach-specific kludges for
cascade out of dma-sh entirely.
This can certainly be fixed for 2.6.24, though a larger overhaul is
2.6.25 material at this point.
On Wed, October 3, 2007 7:18 am, Paul Mundt wrote:
> On Tue, Oct 02, 2007 at 10:09:27PM +0100, Adrian McMenamin wrote:
>> Fix SH DMAC code to correctly handle PVR2 cascade DMA.
>>
>> This updates http://lkml.org/lkml/2007/10/2/276
>>
>> (I decided it was better to have the true size of the transfer put in
>> via the API and refactor this here. And calc_xmit_shift(chan) should
>> return 5 but only returns 3 so I've not used it here)
>>
> It would be helpful to know why calc_xmit_shift() is broken here rather
> than just coding around it, as this will have implications for the other
> DMA channels on SH7091/SH7750.
>From include/asm-sh/cpu-sh4/dma.h
53 /*
54 * The DMA count is defined as the number of bytes to transfer.
55 */
56 static unsigned int ts_shift[] __maybe_unused = {
57 [XMIT_SZ_64BIT] = 3,
58 [XMIT_SZ_8BIT] = 0,
59 [XMIT_SZ_16BIT] = 1,
60 [XMIT_SZ_32BIT] = 2,
61 [XMIT_SZ_256BIT] = 5,
62 };
63 #endif
ie ts_shift returns the number of bytes per transfer, but is then used as
a bit shift:
45 /*
46 * We determine the correct shift size based off of the CHCR transmit size
47 * for the given channel. Since we know that it will take:
48 *
49 * info->count >> ts_shift[transmit_size]
50 *
51 * iterations to complete the transfer.
52 */
53 static inline unsigned int calc_xmit_shift(struct dma_channel *chan)
54 {
55 u32 chcr = ctrl_inl(CHCR[chan->chan]);
56
57 return ts_shift[(chcr & CHCR_TS_MASK)>>CHCR_TS_SHIFT];
58 }
(From arch/sh/drivers/dma/dma-sh.c)
I'm not anywhere where I can fix this at the moment, but i am sure it
could be patched quite trivally.
On Wed, Oct 03, 2007 at 04:41:37PM +0100, Adrian McMenamin wrote:
> On Wed, October 3, 2007 7:18 am, Paul Mundt wrote:
> > On Tue, Oct 02, 2007 at 10:09:27PM +0100, Adrian McMenamin wrote:
> >> Fix SH DMAC code to correctly handle PVR2 cascade DMA.
> >>
> >> This updates http://lkml.org/lkml/2007/10/2/276
> >>
> >> (I decided it was better to have the true size of the transfer put in
> >> via the API and refactor this here. And calc_xmit_shift(chan) should
> >> return 5 but only returns 3 so I've not used it here)
> >>
> > It would be helpful to know why calc_xmit_shift() is broken here rather
> > than just coding around it, as this will have implications for the other
> > DMA channels on SH7091/SH7750.
>
>
> >From include/asm-sh/cpu-sh4/dma.h
>
> 53 /*
> 54 * The DMA count is defined as the number of bytes to transfer.
> 55 */
> 56 static unsigned int ts_shift[] __maybe_unused = {
> 57 [XMIT_SZ_64BIT] = 3,
> 58 [XMIT_SZ_8BIT] = 0,
> 59 [XMIT_SZ_16BIT] = 1,
> 60 [XMIT_SZ_32BIT] = 2,
> 61 [XMIT_SZ_256BIT] = 5,
> 62 };
> 63 #endif
>
> ie ts_shift returns the number of bytes per transfer, but is then used as
> a bit shift:
>
Er, no it doesn't, try again. ts_shift returns the transfer size _shift_.
The comment refers to the DMA count, which is a different matter. Your
32-byte transfer where you want the >> 5 shift == ts_shift[XMIT_SZ_256BIT],
and there's nothing wrong with that. So the issue becomes why you don't
get a ts_shift[] offset of XMIT_SZ_256BIT for that channel, and that's
the bug that has to be fixed.
> 45 /*
> 46 * We determine the correct shift size based off of the CHCR transmit size
> 47 * for the given channel. Since we know that it will take:
> 48 *
> 49 * info->count >> ts_shift[transmit_size]
> 50 *
> 51 * iterations to complete the transfer.
> 52 */
> 53 static inline unsigned int calc_xmit_shift(struct dma_channel *chan)
> 54 {
> 55 u32 chcr = ctrl_inl(CHCR[chan->chan]);
> 56
> 57 return ts_shift[(chcr & CHCR_TS_MASK)>>CHCR_TS_SHIFT];
> 58 }
>
So for PVR2 cascade, what does the CHCR value work out to? Both
CHCR_TS_MASK and CHCR_TS_SHIFT haven't changed for SH7750, so this
suggests that either the CHCR value is just wrong or we've had a
long-standing bug with the CHCR.TS mask.
Either way, we are not going to hardcode a ts_shift value when the CHCR
has all of the information encoded in it!
On Thu, October 4, 2007 11:01 am, Paul Mundt wrote:
> So for PVR2 cascade, what does the CHCR value work out to? Both
> CHCR_TS_MASK and CHCR_TS_SHIFT haven't changed for SH7750, so this
> suggests that either the CHCR value is just wrong or we've had a
> long-standing bug with the CHCR.TS mask.
>
> Either way, we are not going to hardcode a ts_shift value when the CHCR
> has all of the information encoded in it!
>
There *is* a long standing bug in CHCR_TS_MASK. It is meant to mask out
all but bits 6:4 but in fact only masks bits 5 and 4. If it is set to 0x70
and not 0x30 this should work - though as usual I cannot make a patch to
check this out at the moment.