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 dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
