linuxsh-dev

[PATCH] Fix SH DMAC code to handle PVR2 cascade

[Adrian M. <ad...@ne...>]

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]);

Re: [PATCH] Fix SH DMAC code to handle PVR2 cascade

[Paul M. <le...@li...>]

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.

Re: [PATCH] Fix SH DMAC code to handle PVR2 cascade

[Adrian M. <ad...@ne...>]

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.

Re: [PATCH] Fix SH DMAC code to handle PVR2 cascade

[Paul M. <le...@li...>]

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!

Re: [PATCH] Fix SH DMAC code to handle PVR2 cascade

[Adrian M. <ad...@ne...>]

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.