Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 2017-09-07 14:52, Pierre-Yves MORDRET wrote:
> This patch implements the STM32 DMAMUX driver.
>
> The DMAMUX request multiplexer allows routing a DMA request line between
> the peripherals and the DMA controllers of the product. The routing
> function is ensured by a programmable multi-channel DMA request line
> multiplexer. Each channel selects a unique DMA request line,
> unconditionally or synchronously with events from its DMAMUX
> synchronization inputs. The DMAMUX may also be used as a DMA request
> generator from programmable events on its input trigger signals
>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madia...@gmail.com>
> Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mord...@st.com>
> ---
> Version history:
> v4:
> * Get rid of st,dmamux property and custom API between STM32
> DMAMUX and DMA.
> DMAMUX will read DMA masters from Device Tree from now on.
> Merely one DMAMUX node is needed now.
> * Only STM32 DMA are allowed to be connected onto DMAMUX
> * channelID is computed locally within the driver and crafted in
> dma_psec to be passed toward DMA master.
> DMAMUX router sorts out which DMA master will serve the
> request automatically.
> * This version forbids the use of DMA in standalone and DMAMUX at
> the same time : all clients need to be connected either on DMA
> or DMAMUX ; no mix up
Great that you got it working w/o a custom API!
I have one comment, which actually valid for the ti-dma-crossbar driver
as well...
> +static void *stm32_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> + struct of_dma *ofdma)
> +{
> + struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
> + struct stm32_dmamux_data *dmamux = platform_get_drvdata(pdev);
> + struct stm32_dmamux *mux;
> + u32 i, min, max, ret;
> + unsigned long flags;
> +
> + if (dma_spec->args_count != 3) {
> + dev_err(&pdev->dev, "invalid number of dma mux args\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (dma_spec->args[0] > dmamux->dmamux_requests) {
> + dev_err(&pdev->dev, "invalid mux request number: %d\n",
> + dma_spec->args[0]);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + spin_lock_irqsave(&dmamux->lock, flags);
> + mux->chan_id = find_first_zero_bit(dmamux->dma_inuse,
> + dmamux->dma_requests);
you pick the first available chan_id here under the lock.
> + spin_unlock_irqrestore(&dmamux->lock, flags);
> + if (mux->chan_id == dmamux->dma_requests) {
> + dev_err(&pdev->dev, "Run out of free DMA requests\n");
> + kfree(mux);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + /* Look for DMA Master */
> + for (i = 1, min = 0, max = dmamux->dma_reqs[i];
> + i <= dmamux->dma_reqs[0];
> + min += dmamux->dma_reqs[i], max += dmamux->dma_reqs[++i])
> + if (mux->chan_id < max)
> + break;
> + mux->master = i - 1;
> +
> + /* The of_node_put() will be done in of_dma_router_xlate function */
> + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", i - 1);
> + if (!dma_spec->np) {
> + dev_err(&pdev->dev, "can't get dma master\n");
> + kfree(mux);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + /* Set dma request */
> + spin_lock_irqsave(&dmamux->lock, flags);
> + if (!IS_ERR(dmamux->clk)) {
> + ret = clk_enable(dmamux->clk);
> + if (ret < 0) {
> + spin_unlock_irqrestore(&dmamux->lock, flags);
> + kfree(mux);
> + dev_err(&pdev->dev, "clk_prep_enable issue: %d\n", ret);
> + return ERR_PTR(ret);
> + }
> + }
> + spin_unlock_irqrestore(&dmamux->lock, flags);
> +
> + set_bit(mux->chan_id, dmamux->dma_inuse);
But nothing stops other parallel threads to pick the same chan_id since
you have released the lock (released, got the lock to protect the set
dma request and released it again). imho the find_first_zero_bit() and
the set_bit() should be done within the same lock to avoid race conditions.
- Péter
