Re: [PATCH v4 1/2] dmaengine: Add HiSilicon Ascend SDMA engine support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 13/09/2023 10:28, 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.
> 
> Signed-off-by: Guo Mengqi <guomengqi3@huawei.com>
> ---
>  drivers/dma/Kconfig            |   9 +
>  drivers/dma/Makefile           |   1 +
>  drivers/dma/hisi-ascend-sdma.c | 810 +++++++++++++++++++++++++++++++++
>  3 files changed, 820 insertions(+)
>  create mode 100644 drivers/dma/hisi-ascend-sdma.c
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 4ccae1a3b884..afc2b648dcd2 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -244,6 +244,15 @@ config FSL_RAID
>  	  the capability to offload memcpy, xor and pq computation
>  	  for raid5/6.
>  
> +config HISI_ASCEND_SDMA
> +	tristate "HiSilicon Ascend SDMA Engine support"
> +	depends on ARCH_HISI && ARM64

Missing compile testing.

> +/*
> + * struct ascend_sdma_chip_data - Ascend chip specific data
> + * @channel_iomem_size: Size of channel register space
> + */
> +struct ascend_sdma_chip_data {
> +	unsigned int channel_iomem_size;
> +};
> +
> +void set_sdma_channel_info(struct dma_chan *c, int pasid);

Why is this needed? There is no usage before definition.

> +
> +static u32 sdma_queue_count(u32 head, u32 tail, u32 len)

Do not declare functions before data structures. You need to properly
organize this code.

> +{
> +	return (tail - head) & (len - 1);
> +}
> +
> +static int iommu_enabled;

No static (file-scope) variables. Drop.

> +
> +struct sdma_sq_entry {
> +	u32 opcode          : 8;
> +	u32 ie              : 1;
> +	u32 sssv            : 1;
> +	u32 dssv            : 1;
> +	u32 sns             : 1;
> +	u32 dns             : 1;
> +	u32 qos             : 4;
> +	u32 sro             : 1;
> +	u32 dro             : 1;
> +	u32 partid          : 4;
> +	u32 mpamns          : 1;
> +	u32 reserved0       : 8;
> +	u32 src_streamid    : 16;
> +	u32 src_substreamid : 16;
> +	u32 dst_streamid    : 16;
> +	u32 dst_substreamid : 16;
> +	u32 length;
> +	union {
> +		u64 src_addr;
> +		struct {
> +			u32 src_addr_l;
> +			u32 src_addr_h;
> +		};
> +	};
> +	union {
> +		u64 dst_addr;
> +		struct {
> +			u32 dst_addr_l;
> +			u32 dst_addr_h;
> +		};
> +	};
> +};
> +
> +struct sdma_cq_entry {
> +	u32 reserved1;
> +	u32 reserved2;
> +	u32 sqhd      : 16;
> +	u32 reserved3 : 16;
> +	u32 reserved4 : 16;
> +	u32 vld       : 1;
> +	u32 status    : 15;
> +};
> +
> +/*
> + * struct sdma_desc - sdma descriptor to manage transfer requests.
> + */
> +struct sdma_desc {
> +	int pasid;
> +	struct virt_dma_desc vd;
> +	struct sdma_sq_entry entry;
> +};
> +
> +/*
> + * struct sdma_chan - sdma channel information
> + */
> +struct sdma_chan {
> +	u16			idx;
> +	u16			cq_vld;
> +
> +	u16			sq_head;
> +	u16			sq_tail;
> +	u16			cq_head;
> +	u16			cq_tail;
> +
> +	/* must be page-aligned and continuous physical memory */
> +	struct sdma_sq_entry	*sq_base;
> +	struct sdma_cq_entry	*cq_base;
> +
> +	/* used for discrete copy, pre-alloc the buffer, reserved for now */
> +	unsigned long		*src_addr;
> +	unsigned long		*dst_addr;
> +	unsigned long		*len;
> +
> +	void __iomem *io_base;
> +
> +	int id;
> +	struct virt_dma_chan vc;
> +	struct sdma_dev *sdev;
> +
> +	struct sdma_desc *desc;
> +	char *name;
> +	int pasid;
> +};
> +
> +#define SDMA_DEVICE_NAME_LENGTH_MAX 20
> +/*
> + * struct sdma_dev - sdma controller information
> + */
> +struct sdma_dev {
> +	struct	dma_device dma_dev;
> +	struct	device	*dev;
> +	void __iomem *io_base;
> +
> +	u16		idx;
> +	u16		nr_channel;

Indentation is a mess.

> +	DECLARE_BITMAP(channel_map, SDMA_MAX_CHANNEL_NUM);
> +	u32		streamid;
> +
> +	const struct ascend_sdma_chip_data *cdata;
> +
> +	char	name[SDMA_DEVICE_NAME_LENGTH_MAX];
> +	struct	sdma_chan *channels;
> +};
> +
> +static inline struct sdma_chan *to_sdma_chan(struct dma_chan *c)
> +{
> +	return container_of(c, struct sdma_chan, vc.chan);
> +}
> +
> +static inline struct sdma_desc *to_sdma_desc(struct virt_dma_desc *vd)
> +{
> +	return container_of(vd, struct sdma_desc, vd);
> +}
> +
> +/* sdma supports sva transfer via iommu.
> + * client must first set the pasid.
> + */
> +void set_sdma_channel_info(struct dma_chan *c, int pasid)

This is not used.

> +{
> +	struct sdma_chan *sc = to_sdma_chan(c);
> +
> +	sc->pasid = pasid;
> +}
> +EXPORT_SYMBOL_GPL(set_sdma_channel_info);

No, you cannot export unused stuff. Drop entire function.

> +
> +struct sdma_hardware_info {
> +	unsigned long	channel_map;
> +	u64		base_addr; /* physical address */
> +};
> +
> +static int of_sdma_collect_info(struct platform_device *pdev, struct sdma_hardware_info *info)

This should be next to probe, not in totally different place.

> +{
> +	int ret;
> +	u32 chan_mask[2] = {0};
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	ret = of_property_read_variable_u32_array(np, "dma-channel-mask",
> +			chan_mask, 1, 2);
> +	if (ret < 0) {
> +		dev_err(dev, "get dma channel mask from dtb failed, %d\n", ret);
> +		return ret;
> +	}
> +	bitmap_from_arr32(&info->channel_map, chan_mask, SDMA_MAX_CHANNEL_NUM);
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret < 0) {
> +		dev_err(dev, "get io_base info from dtb failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	info->base_addr = res.start;
> +	if (resource_size(&res) != SDMA_IOMEM_SIZE)
> +		dev_warn(dev, "reg size %#llx check failed, use %#x\n",
> +				resource_size(&res), SDMA_IOMEM_SIZE);
> +
> +	return 0;
> +}
> +
> +static int sdma_channel_alloc_sq_cq(struct sdma_chan *sc)
> +{
> +	unsigned long *buf;
> +	struct device *dev = sc->sdev->dev;
> +
> +	sc->sq_base = (struct sdma_sq_entry *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +			get_order(SDMA_SQ_SIZE));
> +	if (!sc->sq_base) {
> +		dev_err(dev, "channel%d: alloc sq_memory failed\n", sc->idx);

Why do you print errors on memory allocation failures?

> +		return -ENOMEM;
> +	}
> +
> +	sc->cq_base = (struct sdma_cq_entry *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +			get_order(SDMA_CQ_SIZE));
> +	if (!sc->cq_base) {
> +		dev_err(dev, "channel%d: alloc cq_memory failed\n", sc->idx);


Same question.

> +		free_pages((unsigned long)sc->sq_base, get_order(SDMA_SQ_SIZE));
> +		return -ENOMEM;
> +	}
> +
> +	buf = vmalloc(sizeof(unsigned long) * SDMA_SQ_LENGTH * 3);
> +	if (!buf) {
> +		dev_err(dev, "channel%d: alloc user_buf failed\n", sc->idx);

Same question.


> +		free_pages((unsigned long)sc->sq_base, get_order(SDMA_SQ_SIZE));
> +		free_pages((unsigned long)sc->cq_base, get_order(SDMA_CQ_SIZE));
> +		return -ENOMEM;
> +	}
> +	sc->src_addr = buf;
> +	sc->dst_addr = buf + SDMA_SQ_LENGTH;
> +	sc->len      = buf + SDMA_SQ_LENGTH * 2;
> +
> +	return 0;
> +}
> +


Best regards,
Krzysztof