Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver

[M'boumba Cedric Madianga]

Hi Daniel,

2015-10-13 16:34 GMT+02:00 Daniel Thompson <daniel.thompson@xxxxxxxxxx>:
> On 13/10/15 15:05, M'boumba Cedric Madianga wrote:
>>
>> This patch adds support for the STM32 DMA controller.
>>
>> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@xxxxxxxxx>
>> ---
>>   drivers/dma/Kconfig     |   12 +
>>   drivers/dma/Makefile    |    1 +
>>   drivers/dma/stm32-dma.c | 1175
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1188 insertions(+)
>>   create mode 100644 drivers/dma/stm32-dma.c
>
>
>> diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
>> new file mode 100644
>> index 0000000..031bab7
>> --- /dev/null
>> +++ b/drivers/dma/stm32-dma.c
>
>
>> +enum stm32_dma_width {
>> +       STM32_DMA_BYTE,
>> +       STM32_DMA_HALF_WORD,
>> +       STM32_DMA_WORD,
>> +};
>> +
>> +enum stm32_dma_burst_size {
>> +       STM32_DMA_BURST_SINGLE,
>> +       STM32_DMA_BURST_INCR4,
>> +       STM32_DMA_BURST_INCR8,
>> +       STM32_DMA_BURST_INCR16,
>> +};
>> +
>> +enum stm32_dma_channel_id {
>> +       STM32_DMA_CHANNEL0,
>> +       STM32_DMA_CHANNEL1,
>> +       STM32_DMA_CHANNEL2,
>> +       STM32_DMA_CHANNEL3,
>> +       STM32_DMA_CHANNEL4,
>> +       STM32_DMA_CHANNEL5,
>> +       STM32_DMA_CHANNEL6,
>> +       STM32_DMA_CHANNEL7,
>> +};
>
>
> Why have (unused) enumerations to map numeric symbols to their own natural
> values? Using normal integers would be better.

Good point. I will remove these in the next version.
Thanks.

>
>
>> +enum stm32_dma_request_id {
>> +       STM32_DMA_REQUEST0,
>> +       STM32_DMA_REQUEST1,
>> +       STM32_DMA_REQUEST2,
>> +       STM32_DMA_REQUEST3,
>> +       STM32_DMA_REQUEST4,
>> +       STM32_DMA_REQUEST5,
>> +       STM32_DMA_REQUEST6,
>> +       STM32_DMA_REQUEST7,
>> +};
>
>
> Ditto.

As above, I will remove it in the next version. Thanks.

>
>
>> +static void stm32_dma_dump_reg(struct stm32_dma_chan *chan)
>> +{
>> +       struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> +
>> +       dev_dbg(chan2dev(chan), "SCR:   0x%08x\n",
>> +               stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id)));
>> +       dev_dbg(chan2dev(chan), "NDTR:  0x%08x\n",
>> +               stm32_dma_read(dmadev, STM32_DMA_SNDTR(chan->id)));
>> +       dev_dbg(chan2dev(chan), "SPAR:  0x%08x\n",
>> +               stm32_dma_read(dmadev, STM32_DMA_SPAR(chan->id)));
>> +       dev_dbg(chan2dev(chan), "SM0AR: 0x%08x\n",
>> +               stm32_dma_read(dmadev, STM32_DMA_SM0AR(chan->id)));
>> +       dev_dbg(chan2dev(chan), "SM1AR: 0x%08x\n",
>> +               stm32_dma_read(dmadev, STM32_DMA_SM1AR(chan->id)));
>> +       dev_dbg(chan2dev(chan), "SFCR:  0x%08x\n",
>> +               stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id)));
>> +}
>
>
> Even at dev_dbg() this looks like log spam. This debug info gets issued
> every time we set up a transfer!

Yes, this info will be logged after each transfer.
But it will complete other debug info print when DMADEVICES_DEBUG is set.

>
>
>> +static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
>> +{
>> +       struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
>> +       struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
>> +       int ret;
>> +
>> +       chan->config_init = false;
>> +       ret = clk_prepare_enable(dmadev->clk);
>> +       if (ret < 0) {
>> +               dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n",
>> ret);
>> +               return ret;
>> +       }
>> +
>> +       ret = stm32_dma_disable_chan(chan);
>> +
>> +       return ret;
>> +}
>
>
> The error path here looks like it will leak clock references.

Sorry I didn't catch it. What does it mean ?

>
>
>
>> +static int stm32_dma_probe(struct platform_device *pdev)
>> +{
>> +       struct stm32_dma_chan *chan;
>> +       struct stm32_dma_device *dmadev;
>> +       struct dma_device *dd;
>> +       const struct of_device_id *match;
>> +       unsigned int i;
>> +       struct resource *res;
>> +       int ret;
>> +
>> +       match = of_match_device(stm32_dma_of_match, &pdev->dev);
>> +       if (!match) {
>> +               dev_err(&pdev->dev, "Error: No device match found\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       dmadev = devm_kzalloc(&pdev->dev, sizeof(*dmadev), GFP_KERNEL);
>> +       if (!dmadev)
>> +               return -ENOMEM;
>> +
>> +       dd = &dmadev->ddev;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       dmadev->base = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(dmadev->base))
>> +               return PTR_ERR(dmadev->base);
>> +
>> +       dmadev->clk = devm_clk_get(&pdev->dev, NULL);
>> +       if (IS_ERR(dmadev->clk)) {
>> +               dev_err(&pdev->dev, "Error: Missing controller clock\n");
>> +               return PTR_ERR(dmadev->clk);
>> +       }
>> +
>> +       dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
>> +                                               "st,mem2mem");
>> +
>> +       dmadev->rst = devm_reset_control_get(&pdev->dev, NULL);
>> +       if (!IS_ERR(dmadev->rst)) {
>> +               reset_control_assert(dmadev->rst);
>> +               udelay(2);
>> +               reset_control_deassert(dmadev->rst);
>> +       }
>> +
>> +       dma_cap_set(DMA_SLAVE, dd->cap_mask);
>> +       dma_cap_set(DMA_PRIVATE, dd->cap_mask);
>> +       dma_cap_set(DMA_CYCLIC, dd->cap_mask);
>> +       dd->device_alloc_chan_resources = stm32_dma_alloc_chan_resources;
>> +       dd->device_free_chan_resources = stm32_dma_free_chan_resources;
>> +       dd->device_tx_status = stm32_dma_tx_status;
>> +       dd->device_issue_pending = stm32_dma_issue_pending;
>> +       dd->device_prep_slave_sg = stm32_dma_prep_slave_sg;
>> +       dd->device_prep_dma_cyclic = stm32_dma_prep_dma_cyclic;
>> +       dd->device_config = stm32_dma_slave_config;
>> +       dd->device_terminate_all = stm32_dma_terminate_all;
>> +       dd->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>> +               BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>> +               BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> +       dd->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>> +               BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>> +               BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> +       dd->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
>> +       dd->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>> +       dd->dev = &pdev->dev;
>> +       INIT_LIST_HEAD(&dd->channels);
>> +
>> +       if (dmadev->mem2mem) {
>> +               dma_cap_set(DMA_MEMCPY, dd->cap_mask);
>> +               dd->device_prep_dma_memcpy = stm32_dma_prep_dma_memcpy;
>> +               dd->directions |= BIT(DMA_MEM_TO_MEM);
>> +       }
>> +
>> +       for (i = 0; i < STM32_DMA_MAX_CHANNELS; i++) {
>> +               chan = &dmadev->chan[i];
>> +               chan->id = i;
>> +               chan->vchan.desc_free = stm32_dma_desc_free;
>> +               vchan_init(&chan->vchan, dd);
>> +       }
>> +
>> +       ret = dma_async_device_register(dd);
>> +       if (ret)
>> +               return ret;
>> +
>> +       for (i = 0; i < STM32_DMA_MAX_CHANNELS; i++) {
>> +               chan = &dmadev->chan[i];
>> +               res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>> +               if (!res) {
>> +                       ret = -EINVAL;
>> +                       dev_err(&pdev->dev, "No irq resource for chan
>> %d\n", i);
>> +                       goto err_unregister;
>> +               }
>> +               chan->irq = res->start;
>> +               ret = devm_request_irq(&pdev->dev, chan->irq,
>> +                                      stm32_dma_chan_irq, 0,
>> +                                      dev_name(chan2dev(chan)), chan);
>> +               if (ret) {
>> +                       dev_err(&pdev->dev,
>> +                               "request_irq failed with err %d channel
>> %d\n",
>> +                               ret, i);
>> +                       goto err_unregister;
>> +               }
>> +       }
>> +
>> +       ret = of_dma_controller_register(pdev->dev.of_node,
>> +                                        stm32_dma_of_xlate, dmadev);
>> +       if (ret < 0) {
>> +               dev_err(&pdev->dev,
>> +                       "STM32 DMA DMA OF registration failed %d\n", ret);
>> +               goto err_unregister;
>> +       }
>> +
>> +       platform_set_drvdata(pdev, dmadev);
>> +
>> +       dev_info(&pdev->dev, "STM32 DMA driver registered\n");
>> +
>> +       return 0;
>> +
>> +err_unregister:
>> +       dma_async_device_unregister(dd);
>> +
>> +       return ret;
>> +}
>> +
>> +static int stm32_dma_remove(struct platform_device *pdev)
>> +{
>> +       struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
>> +
>> +       of_dma_controller_free(pdev->dev.of_node);
>> +
>> +       dma_async_device_unregister(&dmadev->ddev);
>> +
>> +       clk_disable_unprepare(dmadev->clk);
>
>
> What is the purpose of disabling/unpreparing the clock here?
> stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources() should
> pair up and the clock should already be stopped.

I agree with you. I will remove it in the next version. Thanks

>
>
> Daniel.

Thanks for your review.

BR,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/