LinuxPPC-Dev Archive on lore.kernel.org

* ppc4xx DMA fixes for simultaneous sg transfers
From: ductusrhe @ 2005-12-07 21:33 UTC (permalink / raw)
  To: linuxppc-embedded

[-- Attachment #1: Type: text/plain, Size: 12046 bytes --]

Linux 2.4.20 (probably 2.6 as well?)

Conclusion:
Scatter/gather DMA is not thread safe.

Background:
1. We run all four dma channels simultaneously in SG mode on the PPC440EP, starting and stopping them in different threads.
2. Also we need to change channel configs between different transfers, i.e. run ppc4xx_init_dma_channel() to set read or write mode.

Problem and cause:
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.
   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.
   That can cause a channel that is just done, to start again, or a newly started channel to stop.
   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.
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.
   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.

Solution:
1. - In ppc4xx_alloc_dma_handle(), do not touch the ASGC register!
   - In ppc4xx_enable_dma_sgl() and ppc4xx_disable_dma_sgl(), when setting the ASGC register, only change the given channel.
     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.
   Probably this is the way the register was intended to be handled?
2. - In ppc4xx_init_dma_channel(), only clear the correct bits of the DMASR register, not the whole register.
     (The polarity was also not set as it's supposed to in this function, but there exists a patch for that)

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). 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.

I have no idea if this has been fixed in some patch already... but I have not seen it on this list anyway.
I'm not a regular poster, just want to help others avoid some of the struggle when running many channels.

/Ronnie Hedlund

Our changed code:
In "ppc4xx_dma.c"
-----------------------
/*
 *  The comment states that this function should only be run at start-up, and never more.
 *  That is unacceptable, with the fix it can be run anywhere as long as the given channel is not running.
 */
int ppc4xx_init_dma_channel(unsigned int dmanr, ppc_dma_ch_t * p_init)
{
        unsigned int status_bits[] = { DMA_CS0 | DMA_TS0 | DMA_CH0_ERR,
                                       DMA_CS1 | DMA_TS1 | DMA_CH1_ERR,
                                       DMA_CS2 | DMA_TS2 | DMA_CH2_ERR,
                                       DMA_CS3 | DMA_TS3 | DMA_CH3_ERR};
        unsigned int polarity;
        uint32_t control = 0;
        ppc_dma_ch_t *p_dma_ch = &dma_channels[dmanr];

        DMA_MODE_READ = (unsigned long) DMA_TD; /* Peripheral to Memory */
        DMA_MODE_WRITE = 0;     /* Memory to Peripheral */
        if (!p_init) {
                printk("ppc4xx_init_dma_channel: NULL p_init\n");
                return DMA_STATUS_NULL_POINTER;
        }
        if (dmanr >= MAX_PPC4xx_DMA_CHANNELS) {
                printk("ppc4xx_init_dma_channel: bad channel %d\n", dmanr);
                return DMA_STATUS_BAD_CHANNEL;
        }
#if DCRN_POL > 0
        polarity = mfdcr(DCRN_POL);
#else
        polarity = 0;
#endif
        /* Setup the control register based on the values passed to
         * us in p_init.  Then, over-write the control register with this
         * new value.
         */
        control |= SET_DMA_CONTROL;
        switch (dmanr) {
        case 0:
                /* clear all polarity signals and then "or" in new signal levels */
                polarity &= ~GET_DMA_POLARITY(0);
                polarity |= p_init->polarity;
#if DCRN_POL > 0
                mtdcr(DCRN_POL, polarity);
#endif
                mtdcr(DCRN_DMACR0, control);
                break;
        case 1:
                polarity &= ~GET_DMA_POLARITY(1);
                polarity |= p_init->polarity;
#if DCRN_POL > 0
                mtdcr(DCRN_POL, polarity);
#endif
                mtdcr(DCRN_DMACR1, control);
                break;
        case 2:
                polarity &= ~GET_DMA_POLARITY(2);
                polarity |= p_init->polarity;
#if DCRN_POL > 0
                mtdcr(DCRN_POL, polarity);
#endif
                mtdcr(DCRN_DMACR2, control);
                break;
        case 3:
                polarity &= ~GET_DMA_POLARITY(3);
                polarity |= p_init->polarity;
#if DCRN_POL > 0
                mtdcr(DCRN_POL, polarity);
#endif
                mtdcr(DCRN_DMACR3, control);
                break;
        default:
                return DMA_STATUS_BAD_CHANNEL;
        }
        /* save these values in our dma channel structure */
        memcpy(p_dma_ch, p_init, sizeof (ppc_dma_ch_t));

        /*
         * The peripheral width values written in the control register are:
         *   PW_8                 0
         *   PW_16                1
         *   PW_32                2
         *   PW_64                3
         *
         *   Since the DMA count register takes the number of "transfers",
         *   we need to divide the count sent to us in certain
         *   functions by the appropriate number.  It so happens that our
         *   right shift value is equal to the peripheral width value.
         */
        p_dma_ch->shift = p_init->pwidth;
        /*
         * Save the control word for easy access.
         */
        p_dma_ch->control = control;
        /*
         * clear status register for the channel
         * only TS, CS and RI needs to be cleared.
         */
        mtdcr(DCRN_DMASR, status_bits[dmanr]);

        return DMA_STATUS_GOOD;
}

In "ppc4xx_sgdma.c"
-----------------------
int
ppc4xx_alloc_dma_handle(sgl_handle_t * phandle, unsigned int mode, unsigned int dmanr)
{
        sgl_list_info_t *psgl = NULL;
        ppc_dma_ch_t *p_dma_ch = &dma_channels[dmanr];
        //uint32_t sg_command;
        if (dmanr >= MAX_PPC4xx_DMA_CHANNELS) {
                printk("ppc4xx_alloc_dma_handle: invalid channel 0x%x\n", dmanr);
                return DMA_STATUS_BAD_CHANNEL;
        }
        if (!phandle) {
                printk("ppc4xx_alloc_dma_handle: null handle pointer\n");
                return DMA_STATUS_NULL_POINTER;
        }
        /* Get memory for the listinfo struct */
        psgl = kmalloc(sizeof(sgl_list_info_t), GFP_KERNEL);
        if (psgl == NULL) {
                *phandle = (sgl_handle_t) NULL;
                return DMA_STATUS_OUT_OF_MEMORY;
        }
        memset(psgl, 0, sizeof(sgl_list_info_t));

        /* dma_addr is unused now */
        psgl->dmanr = dmanr;
        /*
         * Modify and save the control word. These words will be
         * written to each sgl descriptor.  The DMA engine then
         * loads this control word into the control register
         * every time it reads a new descriptor.
         */
        psgl->control = p_dma_ch->control;
        /* Clear all mode bits */
        psgl->control &= ~(DMA_TM_MASK | DMA_TD);
        /* Save control word and mode */
        psgl->control |= (mode | DMA_CE_ENABLE);
         /* PPC Errata? DMA else ignore count on first in list */
        psgl->control |= SET_DMA_TCE(1);

        /* In MM mode, we must set ETD/TCE */
        if (mode == DMA_MODE_MM)
                psgl->control |= DMA_ETD_OUTPUT | DMA_TCE_ENABLE;
        if (p_dma_ch->int_enable) {
                /* Enable channel interrupt */
                psgl->control |= DMA_CIE_ENABLE;
        } else {
                psgl->control &= ~DMA_CIE_ENABLE;
        }
        /* we must not touch the SGC, it can cause problems to other channels! */

        psgl->sgl_control = SG_LINK;
        if (p_dma_ch->int_enable) {
                if (p_dma_ch->tce_enable)
                {       
                         /* reuse as Terminal Count Interrupt Enable on all descr. */
                        psgl->sgl_control |= SG_TCI_ENABLE;
                }
                psgl->sgl_control |= SG_ERI_ENABLE | SG_ETI_ENABLE;
        }
        *phandle = (sgl_handle_t) psgl;
        return DMA_STATUS_GOOD;
}
void
ppc4xx_enable_dma_sgl(sgl_handle_t handle)
{
        sgl_list_info_t *psgl = (sgl_list_info_t *) handle;
        ppc_dma_ch_t *p_dma_ch;
        uint32_t sg_command;
        if (!handle) {
                printk("ppc4xx_enable_dma_sgl: null handle\n");
                return;
        } else if (psgl->dmanr > (MAX_PPC4xx_DMA_CHANNELS - 1)) {
                printk("ppc4xx_enable_dma_sgl: bad channel in handle %d\n",
                       psgl->dmanr);
                return;
        } else if (!psgl->phead) {
                printk("ppc4xx_enable_dma_sgl: sg list empty\n");
                return;
        }
        p_dma_ch = &dma_channels[psgl->dmanr];
        psgl->ptail->control_count &= ~SG_LINK; /* make this the last dscrptr */
        if (p_dma_ch->int_enable)
        {
                /* Require Terminal Count interrupt on last */
                psgl->ptail->control_count |= SG_TCI_ENABLE;
        }

        /* No more changes to tail object allowed */
        //dma_cache_wback((unsigned long)psgl->ptail, sizeof(ppc_sgl_t));
        dma_cache_wback_inv((unsigned long)psgl->ptail, sizeof(ppc_sgl_t));

        ppc4xx_set_sg_addr(psgl->dmanr, virt_to_phys(psgl->phead));

        sg_command = 0;
        switch (psgl->dmanr) {
        case 0:
                sg_command = SSG0_ENABLE | SSG0_MASK_ENABLE;
                break;
        case 1:
                sg_command = SSG1_ENABLE | SSG1_MASK_ENABLE;
                break;
        case 2:
                sg_command = SSG2_ENABLE | SSG2_MASK_ENABLE;
                break;
        case 3:
                sg_command = SSG3_ENABLE | SSG3_MASK_ENABLE;
                break;
        default:
                printk("ppc4xx_enable_dma_sgl: bad channel: %d\n", psgl->dmanr);
        }

        mtdcr(DCRN_ASGC, sg_command);   /* start transfer */
}
void
ppc4xx_disable_dma_sgl(sgl_handle_t handle)
{
        sgl_list_info_t *psgl = (sgl_list_info_t *) handle;
        uint32_t sg_command;
        if (!handle) {
                printk("ppc4xx_disable_dma_sgl: null handle\n");
                return;
        } else if (psgl->dmanr > (MAX_PPC4xx_DMA_CHANNELS - 1)) {
                printk("ppc4xx_disable_dma_sgl: bad channel in handle %d\n",
                       psgl->dmanr);
                return;
        }
        sg_command = 0; //disable dma
        switch (psgl->dmanr) {
        case 0:
                sg_command = SSG0_MASK_ENABLE;
                break;
        case 1:
                sg_command = SSG1_MASK_ENABLE;
                break;
        case 2:
                sg_command = SSG2_MASK_ENABLE;
                break;
        case 3:
                sg_command = SSG3_MASK_ENABLE;
                break;
        default:
                printk("ppc4xx_disable_dma_sgl: bad channel: %d\n", psgl->dmanr);
        }
        mtdcr(DCRN_ASGC, sg_command);   /* stop transfer */
}

[-- Attachment #2: Type: text/html, Size: 28296 bytes --]

^ permalink raw reply