Re: [PATCH 1/2] dmaengine: ingenic: add Ingenic PDMA controller support.

[Krzysztof Kozlowski]

On 21/03/2024 09:02, bin.yao@xxxxxxxxxxx wrote:
> From: "bin.yao" <bin.yao@xxxxxxxxxxx>
> 
> This module can be found on ingenic victory soc.
> 
> Signed-off-by: bin.yao <bin.yao@xxxxxxxxxxx>
> ---
>  drivers/dma/Kconfig        |    6 +
>  drivers/dma/Makefile       |    1 +
>  drivers/dma/ingenic-pdma.c | 1356 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 1363 insertions(+)
>  create mode 100644 drivers/dma/ingenic-pdma.c

..

> +static int ingenic_dma_chan_init(struct ingenic_dma_engine *dma, int id)
> +{
> +	struct ingenic_dma_chan *dmac = NULL;
> +
> +	if ((id < 0) || (id >= INGENIC_DMA_CHAN_CNT))
> +		return -EINVAL;
> +
> +	dmac = devm_kzalloc(dma->dev, sizeof(*dmac), GFP_KERNEL);
> +	if (!dmac)
> +		return -ENOMEM;
> +
> +	dmac->id = id;
> +	dmac->iomem = dma->iomem + dmac->id * DMACH_OFF;
> +	dmac->engine = dma;
> +
> +	spin_lock_init(&dmac->hdesc_lock);
> +	init_completion(&dmac->completion);
> +
> +	dmac->slave_id = pdma_maps[id] & INGENIC_DMA_TYPE_REQ_MSK;
> +	dma->chan[id] = dmac;
> +	INIT_LIST_HEAD(&dmac->ingenic_dma_sdesc_list_submitted);
> +	INIT_LIST_HEAD(&dmac->ingenic_dma_sdesc_list_issued);
> +
> +	dma_cookie_init(&dmac->chan);
> +	dmac->chan.device = &dma->dma_device;
> +	dmac->working_sdesc = NULL;
> +	list_add_tail(&dmac->chan.device_node, &dma->dma_device.channels);
> +	tasklet_init(&dmac->task, ingenic_dma_complete, (unsigned long)dmac);
> +
> +	return 0;
> +}
> +
> +static int ingenic_dma_probe(struct platform_device *pdev)
> +{
> +	struct ingenic_dma_engine *ingenic_dma = NULL;
> +	unsigned int reg_dmac = DMAC_DMAE;
> +	struct device *dev = &pdev->dev;
> +	struct resource *iores;
> +	int i, ret = 0;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "This driver must be probed from devicetree\n");

This driver cannot be probed other way. Drop.

> +		return -EINVAL;
> +	}
> +
> +	ingenic_dma = devm_kzalloc(&pdev->dev, sizeof(struct ingenic_dma_engine),

ssizeof(*(

> +				   GFP_KERNEL);
> +	if (!ingenic_dma)
> +		return -ENOMEM;
> +
> +	ingenic_dma->dma_data = (struct ingenic_dma_data *)device_get_match_data(dev);

Why the cast?

> +	if (!ingenic_dma->dma_data)
> +		return -EINVAL;
> +
> +	/*
> +	 * Obtaining parameters from the device tree.
> +	 */

Drop obvious comments.

> +	ingenic_dma->dev = dev;
> +	if (!of_property_read_u32(pdev->dev.of_node, "programed-chs",

No, there is no such property. NAK.

Upstream your DTS or I will be NAKing your patches. Because otherwise
you try to sneak a lot of undocumented stuff here.

> +				  &ingenic_dma->chan_programed))
> +		ingenic_dma->chan_reserved |= ingenic_dma->chan_programed;
> +
> +	if (HWATTR_SPECIAL_CH01_SUP(ingenic_dma->dma_data->hwattr) &&
> +	    of_property_read_bool(pdev->dev.of_node, "special-chs")) {
> +		ingenic_dma->chan_reserved  |= DMA_SPECAIL_CHS;
> +		ingenic_dma->chan_programed |= DMA_SPECAIL_CHS;
> +		ingenic_dma->special_ch = true;
> +	}
> +
> +	ingenic_dma->intc_ch = -1;
> +	if (HWATTR_INTC_IRQ_SUP(ingenic_dma->dma_data->hwattr) &&
> +	    !of_property_read_u32(pdev->dev.of_node, "intc-ch",
> +				  &ingenic_dma->intc_ch)) {
> +
> +		if (BIT(ingenic_dma->intc_ch) & ingenic_dma->chan_reserved)
> +			dev_warn(ingenic_dma->dev,
> +				 "intc irq channel %d is already reserved\n",
> +				 ingenic_dma->intc_ch);
> +
> +		ingenic_dma->chan_reserved |= BIT(ingenic_dma->intc_ch);
> +	}
> +
> +	/*
> +	 * obtaining the base address of the DMA peripheral.
> +	 */

Drop obvious comments.

> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (iores) {
> +		ingenic_dma->iomem = devm_ioremap_resource(&pdev->dev, iores);

Combine these two, there's helper for this.

> +		if (IS_ERR(ingenic_dma->iomem))
> +			return PTR_ERR(ingenic_dma->iomem);
> +	} else {
> +		dev_err(dev, "Failed to get I/O memory\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Get PDMA interrupt.
> +	 */

Drop

> +	ingenic_dma->irq_pdma = platform_get_irq_byname(pdev, "pdma");
> +	if (ingenic_dma->irq_pdma < 0) {
> +		dev_err(dev, "Unable to get pdma irq\n");
> +		return ingenic_dma->irq_pdma;

Syntax is return dev_err_probe().

> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, ingenic_dma->irq_pdma,
> +			       pdma_int_handler, 0, "pdma", ingenic_dma);
> +	if (ret) {
> +		dev_err(dev, "Failed to request pdma irq\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Get PDMA descriptor interrupt.
> +	 */
> +	if (HWATTR_DESC_INTER_SUP(ingenic_dma->dma_data->hwattr)) {
> +		ingenic_dma->irq_pdmad = platform_get_irq_byname(pdev, "pdmad");

NAK, you must be joking. You do not have second interrupt.

You must clean your code before upstream from all downstream weirdness
and incorrectness. You cannot send different binding than driver.

What's more, I don't trust that you will send usable binding, judging by
the driver, so I expect to see upstreamed DTS.

> +		if (ingenic_dma->irq_pdmad < 0) {
> +			dev_err(&pdev->dev, "Unable to get pdmad irq\n");
> +			return ingenic_dma->irq_pdmad;
> +		}
> +		ret = devm_request_irq(&pdev->dev, ingenic_dma->irq_pdmad,
> +				       pdmad_int_handler, 0, "pdmad", ingenic_dma);
> +		if (ret) {
> +			dev_err(dev, "Failed to request pdmad irq\n");
> +			return ret;
> +		}
> +	}
> +
> +	/*
> +	 * Initialize dma channel.
> +	 */
> +	INIT_LIST_HEAD(&ingenic_dma->dma_device.channels);
> +	for (i = 0; i < ingenic_dma->dma_data->nb_channels; i++) {
> +		/*reserved one channel for intc interrupt*/
> +		if (ingenic_dma->intc_ch == i)
> +			continue;
> +		ingenic_dma_chan_init(ingenic_dma, i);
> +	}
> +
> +	dma_cap_set(DMA_MEMCPY, ingenic_dma->dma_device.cap_mask);
> +	dma_cap_set(DMA_SLAVE, ingenic_dma->dma_device.cap_mask);
> +	dma_cap_set(DMA_CYCLIC, ingenic_dma->dma_device.cap_mask);
> +
> +	ingenic_dma->dma_device.dev = &pdev->dev;
> +	ingenic_dma->dma_device.device_alloc_chan_resources = ingenic_dma_alloc_chan_resources;
> +	ingenic_dma->dma_device.device_free_chan_resources = ingenic_dma_free_chan_resources;
> +	ingenic_dma->dma_device.device_tx_status = ingenic_dma_tx_status;
> +	ingenic_dma->dma_device.device_prep_slave_sg = ingenic_dma_prep_slave_sg;
> +	ingenic_dma->dma_device.device_prep_dma_cyclic = ingenic_dma_prep_dma_cyclic;
> +	ingenic_dma->dma_device.device_prep_dma_memcpy = ingenic_dma_prep_dma_memcpy;
> +	ingenic_dma->dma_device.device_config = ingenic_dma_config;
> +	ingenic_dma->dma_device.device_terminate_all = ingenic_dma_terminate_all;
> +	ingenic_dma->dma_device.device_issue_pending = ingenic_dma_issue_pending;
> +	ingenic_dma->dma_device.copy_align = DMAENGINE_ALIGN_4_BYTES;
> +	ingenic_dma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> +						  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +						  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +	ingenic_dma->dma_device.dst_addr_widths = ingenic_dma->dma_device.src_addr_widths;
> +	ingenic_dma->dma_device.directions = BIT(DMA_DEV_TO_MEM) |
> +					     BIT(DMA_MEM_TO_DEV) |
> +					     BIT(DMA_MEM_TO_MEM);
> +	ingenic_dma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> +
> +	dma_set_max_seg_size(ingenic_dma->dma_device.dev, DTC_TC_MSK);
> +
> +	ingenic_dma->gate_clk = devm_clk_get(&pdev->dev, "gate_pdma");
> +	if (IS_ERR(ingenic_dma->gate_clk))
> +		return PTR_ERR(ingenic_dma->gate_clk);
> +
> +	ret = dma_async_device_register(&ingenic_dma->dma_device);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to register dma\n");
> +		clk_disable(ingenic_dma->gate_clk);
> +		return ret;
> +	}
> +
> +	of_ingenic_dma_info.dma_cap = ingenic_dma->dma_device.cap_mask;
> +	ret = of_dma_controller_register(pdev->dev.of_node,
> +					 of_dma_simple_xlate,
> +					 &of_ingenic_dma_info);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Unable to register dma to device tree\n");
> +		dma_async_device_unregister(&ingenic_dma->dma_device);
> +		clk_disable(ingenic_dma->gate_clk);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, ingenic_dma);
> +
> +	clk_prepare_enable(ingenic_dma->gate_clk);
> +
> +	if (ingenic_dma->chan_programed)
> +		writel(ingenic_dma->chan_programed, ingenic_dma->iomem + DMACP);
> +	if (ingenic_dma->intc_ch >= 0)
> +		reg_dmac |= DMAC_INTCE |
> +			    ((ingenic_dma->intc_ch << DMAC_INTCC_SFT) & DMAC_INTCC_MSK);
> +	if (ingenic_dma->special_ch)
> +		reg_dmac |= DMAC_CH01;
> +	writel(reg_dmac, ingenic_dma->iomem + DMAC);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_dma_suspend(struct device *dev)
> +{
> +	struct ingenic_dma_engine *ingenic_dma = dev_get_drvdata(dev);
> +	struct ingenic_dma_chan *dmac;
> +	unsigned long flg;
> +	int i;
> +
> +	for (i = 0; i < ingenic_dma->dma_data->nb_channels; i++) {
> +		dmac = ingenic_dma->chan[i];
> +		spin_lock_irqsave(&dmac->hdesc_lock, flg);
> +		if (!list_empty(&dmac->ingenic_dma_sdesc_list_submitted)) {
> +			spin_unlock_irqrestore(&dmac->hdesc_lock, flg);
> +			return -EBUSY;
> +		}
> +		if (!list_empty(&dmac->ingenic_dma_sdesc_list_issued)) {
> +			spin_unlock_irqrestore(&dmac->hdesc_lock, flg);
> +			return -EBUSY;
> +		}
> +		spin_unlock_irqrestore(&dmac->hdesc_lock, flg);
> +	}
> +	clk_disable_unprepare(ingenic_dma->gate_clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_dma_resume(struct device *dev)
> +{
> +	struct ingenic_dma_engine *ingenic_dma = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(ingenic_dma->gate_clk);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ingenic_dma_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ingenic_dma_suspend, ingenic_dma_resume)
> +};
> +
> +static const struct ingenic_dma_data t33_soc_data = {
> +	.nb_channels = 32,
> +};
> +
> +static const struct of_device_id ingenic_dma_dt_match[] = {
> +	{ .compatible = "ingenic,t33-pdma",   .data = &t33_soc_data },

Don't merge lines. Open existing code and look how it is done there.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_dma_dt_match);
> +
> +static struct platform_driver ingenic_dma_driver = {
> +	.probe			= ingenic_dma_probe,
> +	.driver = {
> +		.name		= "ingenic-dma",
> +		.of_match_table = ingenic_dma_dt_match,
> +		.pm		= &ingenic_dma_dev_pm_ops,
> +	},
> +};
> +
> +static int __init ingenic_dma_init(void)
> +{
> +	return platform_driver_register(&ingenic_dma_driver);
> +}
> +subsys_initcall(ingenic_dma_init);
> +
> +static void __exit ingenic_dma_exit(void)
> +{
> +	platform_driver_unregister(&ingenic_dma_driver);
> +}
> +module_exit(ingenic_dma_exit);
> +
> +MODULE_AUTHOR("bin.yao <bin.yao@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Ingenic dma driver");

Why do you add second Ingenic dma driver? That's duplication.

Best regards,
Krzysztof