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@xxxxxxxxxx>
> ---
> 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
