[0/2] Add sdma driver for HiSilicon Ascend platform

This is for the System Direct Memory Access(SDMA) hardware
used by HiSilicon Ascend families. The dma controller supports
data transfers between memory and memory or between memory and device.

Guo Mengqi (2):
  dmaengine: Add HiSilicon Ascend SDMA engine support
  dt-bindings: dma: hisi: Add bindings for Hisi Ascend sdma

 .../bindings/dma/hisi,ascend-sdma.yaml        |  82 ++
 drivers/dma/Kconfig                           |   9 +
 drivers/dma/Makefile                          |   1 +
 drivers/dma/ascend_sdma.c                     | 810 ++++++++++++++++++
 4 files changed, 902 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/hisi,ascend-sdma.yaml
 create mode 100644 drivers/dma/ascend_sdma.c

On 11/08/2023 05:48, Guo Mengqi wrote:
> This patch adds a driver for HiSilicon Ascend SDMA engine.
> 
> The DMA controller can do transfers between device and memory
> or memory to memory. Currently, the controller only support
> single copy. Drives can pass a substreamid to the DMA engine,
> which will enable transfers in user-space addresses.

...

> +
> +static int sdma_device_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev;
> +	struct sdma_dev *sdev;
> +	struct sdma_hardware_info info;
> +
> +	dev = &pdev->dev;
> +
> +	if (!pdev->dev.bus) {
> +		pr_debug("the sdma dev bus is NULL\n");

How is this possible?

> +		return -EPROBE_DEFER;
> +	}
> +
> +	if (!pdev->dev.bus->iommu_ops) {
> +		pr_debug("defer probe sdma device\n");

Anyway, no pr_xxx, but dev_xxx. Is it really, really possible?


> +		return -EPROBE_DEFER;> +	}
> +
> +	sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
> +	if (!sdev) {
> +		pr_err("alloc sdma_device failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	sdev->pdev = pdev;
> +	dev_set_drvdata(&pdev->dev, sdev);

Come on, you just stored pdev->dev under dev!
> +
> +	ret = of_sdma_collect_info(pdev, &info);
> +	if (ret < 0) {
> +		pr_err("collect device info failed, %d\n", ret);

dev_err

Please work on this driver to start looking like other kernel drivers.


> +		return ret;
> +	}
> +
> +	sdev->io_base = ioremap(info.base_addr, SDMA_IOMEM_SIZE);
> +	if (!sdev->io_base) {
> +		pr_err("ioremap failed\n");
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	/* Fill in dmaengine */
> +	sdma_init_dma_device(&sdev->dma_dev, dev);
> +
> +	ret = sdma_init_channels(sdev, &info);
> +	if (ret < 0)
> +		goto unmap_iobase;
> +
> +	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
> +	if (ret) {
> +		pr_err("iommu failed to init iopf, %d\n", ret);
> +		goto destroy_channels;
> +	}
> +
> +	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
> +	if (ret) {
> +		pr_err("iommu failed to init sva, %d\n", ret);
> +		goto disable_iopf;
> +	}
> +
> +	sdev->streamid = pdev->dev.iommu->fwspec->ids[0];
> +
> +	snprintf(sdev->name, SDMA_DEVICE_NAME_LENGTH_MAX, "sdma%d", sdev->idx);
> +	pr_info("%s device probe success\n", sdev->name);

No, drop.

> +
> +	ret = dma_async_device_register(&sdev->dma_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register slave DMA engine: %d\n", ret);
> +		goto disable_sva;
> +	}
> +
> +	return 0;
> +
> +disable_sva:
> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
> +disable_iopf:
> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
> +destroy_channels:
> +	sdma_destroy_channels(sdev);
> +unmap_iobase:
> +	iounmap(sdev->io_base);
> +	return ret;
> +}
> +
> +static int sdma_device_remove(struct platform_device *pdev)
> +{
> +	struct sdma_dev *psdma_dev = dev_get_drvdata(&pdev->dev);
> +
> +	dma_async_device_unregister(&psdma_dev->dma_dev);
> +
> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
> +
> +	sdma_destroy_channels(psdma_dev);
> +
> +	iounmap(psdma_dev->io_base);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sdma_of_match[] = {
> +	{ .compatible = "hisilicon,sdma" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, sdma_of_match);
> +
> +static struct platform_driver sdma_driver = {
> +	.probe    = sdma_device_probe,
> +	.remove   = sdma_device_remove,
> +	.driver   = {
> +		.name           = SDMA_DEVICE_NAME,
> +		.of_match_table = sdma_of_match,
> +	},
> +};
> +
> +static int __init sdma_driver_init(void)
> +{
> +	return platform_driver_register(&sdma_driver);
> +}
> +module_init(sdma_driver_init);
> +
> +static void sdma_driver_exit(void)
> +{
> +	platform_driver_unregister(&sdma_driver);
> +}
> +module_exit(sdma_driver_exit);

module_platform_driver


Best regards,
Krzysztof

在 2023/8/14 16:54, Krzysztof Kozlowski 写道:
> On 11/08/2023 05:48, Guo Mengqi wrote:
>> This patch adds a driver for HiSilicon Ascend SDMA engine.
>>
>> The DMA controller can do transfers between device and memory
>> or memory to memory. Currently, the controller only support
>> single copy. Drives can pass a substreamid to the DMA engine,
>> which will enable transfers in user-space addresses.
> ...
>
>> +
>> +static int sdma_device_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct device *dev;
>> +	struct sdma_dev *sdev;
>> +	struct sdma_hardware_info info;
>> +
>> +	dev = &pdev->dev;
>> +
>> +	if (!pdev->dev.bus) {
>> +		pr_debug("the sdma dev bus is NULL\n");
> How is this possible?
    This shall not be possible. Removed.
>
>> +		return -EPROBE_DEFER;
>> +	}
>> +
>> +	if (!pdev->dev.bus->iommu_ops) {
>> +		pr_debug("defer probe sdma device\n");
> Anyway, no pr_xxx, but dev_xxx. Is it really, really possible?
>
If iommu driver is loaded later than this driver, then here iommu_ops 
may be uninitialized.
>> +		return -EPROBE_DEFER;> +	}
>> +
>> +	sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
>> +	if (!sdev) {
>> +		pr_err("alloc sdma_device failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	sdev->pdev = pdev;
>> +	dev_set_drvdata(&pdev->dev, sdev);
> Come on, you just stored pdev->dev under dev!
>> +
>> +	ret = of_sdma_collect_info(pdev, &info);
>> +	if (ret < 0) {
>> +		pr_err("collect device info failed, %d\n", ret);
> dev_err
>
> Please work on this driver to start looking like other kernel drivers.
>
>
>> +		return ret;
>> +	}
>> +
>> +	sdev->io_base = ioremap(info.base_addr, SDMA_IOMEM_SIZE);
>> +	if (!sdev->io_base) {
>> +		pr_err("ioremap failed\n");
>> +		ret = -ENOMEM;
>> +		return ret;
>> +	}
>> +
>> +	/* Fill in dmaengine */
>> +	sdma_init_dma_device(&sdev->dma_dev, dev);
>> +
>> +	ret = sdma_init_channels(sdev, &info);
>> +	if (ret < 0)
>> +		goto unmap_iobase;
>> +
>> +	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
>> +	if (ret) {
>> +		pr_err("iommu failed to init iopf, %d\n", ret);
>> +		goto destroy_channels;
>> +	}
>> +
>> +	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
>> +	if (ret) {
>> +		pr_err("iommu failed to init sva, %d\n", ret);
>> +		goto disable_iopf;
>> +	}
>> +
>> +	sdev->streamid = pdev->dev.iommu->fwspec->ids[0];
>> +
>> +	snprintf(sdev->name, SDMA_DEVICE_NAME_LENGTH_MAX, "sdma%d", sdev->idx);
>> +	pr_info("%s device probe success\n", sdev->name);
> No, drop.
>
>> +
>> +	ret = dma_async_device_register(&sdev->dma_dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register slave DMA engine: %d\n", ret);
>> +		goto disable_sva;
>> +	}
>> +
>> +	return 0;
>> +
>> +disable_sva:
>> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
>> +disable_iopf:
>> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
>> +destroy_channels:
>> +	sdma_destroy_channels(sdev);
>> +unmap_iobase:
>> +	iounmap(sdev->io_base);
>> +	return ret;
>> +}
>> +
>> +static int sdma_device_remove(struct platform_device *pdev)
>> +{
>> +	struct sdma_dev *psdma_dev = dev_get_drvdata(&pdev->dev);
>> +
>> +	dma_async_device_unregister(&psdma_dev->dma_dev);
>> +
>> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
>> +	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
>> +
>> +	sdma_destroy_channels(psdma_dev);
>> +
>> +	iounmap(psdma_dev->io_base);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id sdma_of_match[] = {
>> +	{ .compatible = "hisilicon,sdma" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sdma_of_match);
>> +
>> +static struct platform_driver sdma_driver = {
>> +	.probe    = sdma_device_probe,
>> +	.remove   = sdma_device_remove,
>> +	.driver   = {
>> +		.name           = SDMA_DEVICE_NAME,
>> +		.of_match_table = sdma_of_match,
>> +	},
>> +};
>> +
>> +static int __init sdma_driver_init(void)
>> +{
>> +	return platform_driver_register(&sdma_driver);
>> +}
>> +module_init(sdma_driver_init);
>> +
>> +static void sdma_driver_exit(void)
>> +{
>> +	platform_driver_unregister(&sdma_driver);
>> +}
>> +module_exit(sdma_driver_exit);
> module_platform_driver
>
>
> Best regards,
> Krzysztof
>
> .

Hi

Thank you for carefully reviewing this patch and give all these advices.

I had sent a version 2 of the patch and fixed these points in dts 
bindings and driver code.

If any thing else is wrong, please let me know.


Best regards,

Guo Mengqi