<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN"><HTML DIR=ltr><HEAD><META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=iso-8859-1"></HEAD><BODY><DIV><FONT face="Courier New" color=#000000 size=2>Linux 2.4.20 (probably 2.6 as well?)</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>Conclusion:<BR>Scatter/gather
DMA is not thread safe.</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>Background:<BR>1. We run all
four dma channels simultaneously in SG mode on the PPC440EP, </FONT><FONT
face="Courier New" color=#000000 size=2>starting and stopping them in different
threads.<BR>2. Also we need to change channel configs between different
transfers, i.e. run ppc4xx_init_dma_channel() to set read or write
mode.</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>Problem and cause:<BR>1. All
is well when running one channel at a time, but when starting/stopping a new
channel during the time another is active can - if you are unlucky - cause
problems.<BR> Because all channels SG start/stop bits are in the
same register (ASGC), it must not be read then changed and written to the way it
was done.<BR> That can cause a channel that is just done, to start
again, or a newly started channel to stop.<BR> The register includes
a feature that can be used as a semaphore - the read enable (mask) bits, but it
was not used correctly in the code, it was enabled for all channels in the
start-up.<BR>2. It is said in the comment of ppc4xx_init_dma_channel() that it
should only be used once in the start-up. If you comply, you will not have any
problem with it.<BR> However, when running this for a channel when
other channels are active, can cause channels to stop or maybe never give an
interrupt or false interrupts. The method clears the entire status register, not
only the bits for the given channel.</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>Solution:<BR>1. - In
ppc4xx_alloc_dma_handle(), do not touch the ASGC register!<BR> - In
ppc4xx_enable_dma_sgl() and ppc4xx_disable_dma_sgl(), when setting the ASGC
register, only change the given channel.<BR> That's done
without reading the register at all, just set/clear the enable bit of the
channel to change, then set the MASK_ENABLE for the same
channel.<BR> Probably this is the way the register was intended to
be handled?</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>2. - In
ppc4xx_init_dma_channel(), only clear the correct bits of the DMASR register,
not the whole register.<BR> (The polarity was also not
set as it's supposed to in this function, but there exists a patch for
that)</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New"><FONT color=#000000 size=2>I can send a patch with
our changes, that works very well and stable, but there are many changes and not
all of them in line with the current official version of the files (we handle
the sg descriptor list differently). </FONT><FONT color=#000000 size=2>Maybe
someone feeling for it will take a look at the changes and make them into the
real code, since the above fixes does not interfere with the usage, only
improves thread safety.</FONT></FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>I have no idea if this has
been fixed in some patch already... but I have not seen it on this list
anyway.<BR>I'm not a regular poster, just want to help others avoid some of the
struggle when running many channels.</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2></FONT> </DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>/Ronnie
Hedlund<BR></FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>Our changed code:<BR>In
"ppc4xx_dma.c"<BR>-----------------------<BR>/*<BR> * The comment
states that this function should only be run at start-up, and never
more.<BR> * That is unacceptable, with the fix it can be run anywhere
as long as the given channel is not running.<BR> */<BR>int
ppc4xx_init_dma_channel(unsigned int dmanr, ppc_dma_ch_t *
p_init)<BR>{<BR> unsigned int
status_bits[] = { DMA_CS0 | DMA_TS0 |
DMA_CH0_ERR,<BR>
DMA_CS1 | DMA_TS1 |
DMA_CH1_ERR,<BR>
DMA_CS2 | DMA_TS2 |
DMA_CH2_ERR,<BR>
DMA_CS3 | DMA_TS3 | DMA_CH3_ERR};</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> unsigned int
polarity;<BR> uint32_t control =
0;<BR> ppc_dma_ch_t *p_dma_ch =
&dma_channels[dmanr];<BR>
<BR> DMA_MODE_READ = (unsigned long)
DMA_TD; /* Peripheral to Memory */<BR>
DMA_MODE_WRITE = 0; /* Memory to Peripheral
*/</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (!p_init)
{<BR>
printk("ppc4xx_init_dma_channel: NULL
p_init\n");<BR>
return DMA_STATUS_NULL_POINTER;<BR>
}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (dmanr >=
MAX_PPC4xx_DMA_CHANNELS)
{<BR>
printk("ppc4xx_init_dma_channel: bad channel %d\n",
dmanr);<BR>
return DMA_STATUS_BAD_CHANNEL;<BR>
}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2>#if DCRN_POL >
0<BR> polarity =
mfdcr(DCRN_POL);<BR>#else<BR> polarity
= 0;<BR>#endif</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> /* Setup the control register
based on the values passed
to<BR> * us in p_init.
Then, over-write the control register with
this<BR> * new
value.<BR>
*/<BR> control |=
SET_DMA_CONTROL;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> switch (dmanr)
{<BR> case
0:<BR>
/* clear all polarity signals and then "or" in new signal levels
*/<BR>
polarity &=
~GET_DMA_POLARITY(0);<BR>
polarity |= p_init->polarity;<BR>#if DCRN_POL >
0<BR>
mtdcr(DCRN_POL,
polarity);<BR>#endif<BR>
mtdcr(DCRN_DMACR0,
control);<BR>
break;<BR> case
1:<BR>
polarity &=
~GET_DMA_POLARITY(1);<BR>
polarity |= p_init->polarity;<BR>#if DCRN_POL >
0<BR>
mtdcr(DCRN_POL,
polarity);<BR>#endif<BR>
mtdcr(DCRN_DMACR1,
control);<BR>
break;<BR> case
2:<BR>
polarity &=
~GET_DMA_POLARITY(2);<BR>
polarity |= p_init->polarity;<BR>#if DCRN_POL >
0<BR>
mtdcr(DCRN_POL,
polarity);<BR>#endif<BR>
mtdcr(DCRN_DMACR2,
control);<BR>
break;<BR> case
3:<BR>
polarity &=
~GET_DMA_POLARITY(3);<BR>
polarity |= p_init->polarity;<BR>#if DCRN_POL >
0<BR>
mtdcr(DCRN_POL,
polarity);<BR>#endif<BR>
mtdcr(DCRN_DMACR3,
control);<BR>
break;<BR>
default:<BR>
return DMA_STATUS_BAD_CHANNEL;<BR>
}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> /* save these values in our
dma channel structure */<BR>
memcpy(p_dma_ch, p_init, sizeof
(ppc_dma_ch_t));<BR>
<BR>
/*<BR> * The peripheral width
values written in the control register
are:<BR> *
PW_8
0<BR> *
PW_16
1<BR> *
PW_32
2<BR> *
PW_64
3<BR>
*<BR> * Since the
DMA count register takes the number of
"transfers",<BR> *
we need to divide the count sent to us in
certain<BR> *
functions by the appropriate number. It so happens that
our<BR> * right
shift value is equal to the peripheral width
value.<BR>
*/<BR> p_dma_ch->shift =
p_init->pwidth;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>
/*<BR> * Save the control word
for easy access.<BR>
*/<BR> p_dma_ch->control =
control;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>
/*<BR> * clear status register
for the channel<BR> * only TS,
CS and RI needs to be
cleared.<BR>
*/<BR> mtdcr(DCRN_DMASR,
status_bits[dmanr]);<BR>
<BR> return
DMA_STATUS_GOOD;<BR>}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000 size=2><BR>In
"ppc4xx_sgdma.c"<BR>-----------------------</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>int<BR>ppc4xx_alloc_dma_handle(sgl_handle_t * phandle, unsigned int mode,
unsigned int dmanr)<BR>{<BR>
sgl_list_info_t *psgl = NULL;<BR>
ppc_dma_ch_t *p_dma_ch =
&dma_channels[dmanr];<BR>
//uint32_t sg_command;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (dmanr >=
MAX_PPC4xx_DMA_CHANNELS)
{<BR>
printk("ppc4xx_alloc_dma_handle: invalid channel 0x%x\n",
dmanr);<BR>
return DMA_STATUS_BAD_CHANNEL;<BR>
}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (!phandle)
{<BR>
printk("ppc4xx_alloc_dma_handle: null handle
pointer\n");<BR>
return DMA_STATUS_NULL_POINTER;<BR>
}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> /* Get memory for the listinfo
struct */<BR> psgl =
kmalloc(sizeof(sgl_list_info_t),
GFP_KERNEL);<BR> if (psgl == NULL)
{<BR>
*phandle = (sgl_handle_t)
NULL;<BR>
return DMA_STATUS_OUT_OF_MEMORY;<BR>
}<BR> memset(psgl, 0,
sizeof(sgl_list_info_t));<BR>
<BR> /* dma_addr is unused now
*/<BR> psgl->dmanr =
dmanr;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>
/*<BR> * Modify and save the
control word. These words will
be<BR> * written to each sgl
descriptor. The DMA engine
then<BR> * loads this control
word into the control
register<BR> * every time it
reads a new descriptor.<BR>
*/<BR> psgl->control =
p_dma_ch->control;<BR> /* Clear all
mode bits */<BR> psgl->control
&= ~(DMA_TM_MASK | DMA_TD);<BR> /*
Save control word and mode */<BR>
psgl->control |= (mode |
DMA_CE_ENABLE);<BR> /* PPC
Errata? DMA else ignore count on first in list
*/<BR> psgl->control |=
SET_DMA_TCE(1);<BR>
<BR> /* In MM mode, we must set
ETD/TCE */<BR> if (mode ==
DMA_MODE_MM)<BR>
psgl->control |= DMA_ETD_OUTPUT |
DMA_TCE_ENABLE;<BR> if
(p_dma_ch->int_enable)
{<BR>
/* Enable channel interrupt
*/<BR>
psgl->control |=
DMA_CIE_ENABLE;<BR> } else
{<BR>
psgl->control &=
~DMA_CIE_ENABLE;<BR> }</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> /* we must not touch the SGC,
it can cause problems to other channels! */</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>
<BR> psgl->sgl_control =
SG_LINK;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (p_dma_ch->int_enable)
{<BR>
if
(p_dma_ch->tce_enable)<BR>
{
<BR>
/* reuse as Terminal Count Interrupt Enable on all descr.
*/<BR>
psgl->sgl_control |=
SG_TCI_ENABLE;<BR>
}<BR>
psgl->sgl_control |= SG_ERI_ENABLE |
SG_ETI_ENABLE;<BR> }</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> *phandle = (sgl_handle_t)
psgl;<BR> return
DMA_STATUS_GOOD;<BR>}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>void<BR>ppc4xx_enable_dma_sgl(sgl_handle_t
handle)<BR>{<BR> sgl_list_info_t *psgl
= (sgl_list_info_t *) handle;<BR>
ppc_dma_ch_t *p_dma_ch;<BR> uint32_t
sg_command;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (!handle)
{<BR>
printk("ppc4xx_enable_dma_sgl: null
handle\n");<BR>
return;<BR> } else if (psgl->dmanr
> (MAX_PPC4xx_DMA_CHANNELS - 1))
{<BR>
printk("ppc4xx_enable_dma_sgl: bad channel in handle
%d\n",<BR>
psgl->dmanr);<BR>
return;<BR> } else if
(!psgl->phead)
{<BR>
printk("ppc4xx_enable_dma_sgl: sg list
empty\n");<BR>
return;<BR> }</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> p_dma_ch =
&dma_channels[psgl->dmanr];<BR>
psgl->ptail->control_count &= ~SG_LINK; /* make this the last dscrptr
*/<BR> if
(p_dma_ch->int_enable)<BR>
{<BR>
/* Require Terminal Count interrupt on last
*/<BR>
psgl->ptail->control_count |=
SG_TCI_ENABLE;<BR>
}<BR>
<BR> /* No more changes to tail object
allowed */<BR>
//dma_cache_wback((unsigned long)psgl->ptail,
sizeof(ppc_sgl_t));<BR>
dma_cache_wback_inv((unsigned long)psgl->ptail,
sizeof(ppc_sgl_t));<BR>
<BR>
ppc4xx_set_sg_addr(psgl->dmanr,
virt_to_phys(psgl->phead));<BR>
<BR> sg_command =
0;<BR> switch (psgl->dmanr)
{<BR> case
0:<BR>
sg_command = SSG0_ENABLE |
SSG0_MASK_ENABLE;<BR>
break;<BR> case
1:<BR>
sg_command = SSG1_ENABLE |
SSG1_MASK_ENABLE;<BR>
break;<BR> case
2:<BR>
sg_command = SSG2_ENABLE |
SSG2_MASK_ENABLE;<BR>
break;<BR> case
3:<BR>
sg_command = SSG3_ENABLE |
SSG3_MASK_ENABLE;<BR>
break;<BR>
default:<BR>
printk("ppc4xx_enable_dma_sgl: bad channel: %d\n",
psgl->dmanr);<BR>
}<BR>
<BR> mtdcr(DCRN_ASGC,
sg_command); /* start transfer */<BR>}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2>void<BR>ppc4xx_disable_dma_sgl(sgl_handle_t
handle)<BR>{<BR> sgl_list_info_t *psgl
= (sgl_list_info_t *) handle;<BR>
uint32_t sg_command;</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> if (!handle)
{<BR>
printk("ppc4xx_disable_dma_sgl: null
handle\n");<BR>
return;<BR> } else if (psgl->dmanr
> (MAX_PPC4xx_DMA_CHANNELS - 1))
{<BR>
printk("ppc4xx_disable_dma_sgl: bad channel in handle
%d\n",<BR>
psgl->dmanr);<BR>
return;<BR> }</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> sg_command = 0; //disable
dma<BR> switch (psgl->dmanr)
{<BR> case
0:<BR>
sg_command =
SSG0_MASK_ENABLE;<BR>
break;<BR> case
1:<BR>
sg_command =
SSG1_MASK_ENABLE;<BR>
break;<BR> case
2:<BR>
sg_command =
SSG2_MASK_ENABLE;<BR>
break;<BR> case
3:<BR>
sg_command =
SSG3_MASK_ENABLE;<BR>
break;<BR>
default:<BR>
printk("ppc4xx_disable_dma_sgl: bad channel: %d\n",
psgl->dmanr);<BR> }</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> mtdcr(DCRN_ASGC,
sg_command); /* stop transfer */<BR>}</FONT></DIV>
<DIV><FONT face="Courier New" color=#000000
size=2> </DIV></FONT></BODY></HTML>