Re: [PATCH v4] dmaengine: hisilicon: Add Kunpeng DMA engine support

[Zhou Wang <wangzhou1@hisilicon.com>]

On 2020/1/15 22:25, Vinod Koul wrote:
> On 02-01-20, 10:33, Zhou Wang wrote:
> 
>> +#define HISI_DMA_SQ_BASE_L(i)		(0x0 + (i) * 0x100)
>> +#define HISI_DMA_SQ_BASE_H(i)		(0x4 + (i) * 0x100)
>> +#define HISI_DMA_SQ_DEPTH(i)		(0x8 + (i) * 0x100)
>> +#define HISI_DMA_SQ_TAIL_PTR(i)		(0xc + (i) * 0x100)
>> +#define HISI_DMA_CQ_BASE_L(i)		(0x10 + (i) * 0x100)
>> +#define HISI_DMA_CQ_BASE_H(i)		(0x14 + (i) * 0x100)
>> +#define HISI_DMA_CQ_DEPTH(i)		(0x18 + (i) * 0x100)
>> +#define HISI_DMA_CQ_HEAD_PTR(i)		(0x1c + (i) * 0x100)
>> +#define HISI_DMA_CTRL0(i)		(0x20 + (i) * 0x100)
>> +#define HISI_DMA_CTRL0_QUEUE_EN_S	0
>> +#define HISI_DMA_CTRL0_QUEUE_PAUSE_S	4
>> +#define HISI_DMA_CTRL1(i)		(0x24 + (i) * 0x100)
>> +#define HISI_DMA_CTRL1_QUEUE_RESET_S	0
>> +#define HISI_DMA_Q_FSM_STS(i)		(0x30 + (i) * 0x100)
>> +#define HISI_DMA_FSM_STS_MASK		GENMASK(3, 0)
>> +#define HISI_DMA_INT_STS(i)		(0x40 + (i) * 0x100)
>> +#define HISI_DMA_INT_STS_MASK		GENMASK(12, 0)
>> +#define HISI_DMA_INT_MSK(i)		(0x44 + (i) * 0x100)
> 
> These really sound as offset + i * 0x100, so I think it might be better
> to define this as:
> 
> define HISI_DMA_SQ_BASE_L               0x0
> with HISI_DMA_OFFSET                    0x100
> 
> and then use read/write accessors:
> 
> hisi_channel_read(... , register, index)
> {
>         return readl( register + index * HISI_DMA_OFFSET)
> }
>

OK, I will modify this in v5.

>> +#define HISI_DMA_MODE			0x217c
>> +
>> +#define HISI_DMA_MSI_NUM		30
>> +#define HISI_DMA_CHAN_NUM		30
>> +#define HISI_DMA_Q_DEPTH_VAL		1024
>> +
>> +#define PCI_DEVICE_ID_HISI_DMA		0xa122
> 
> This is used only once so can be removed

OK.

> 
>> +static void hisi_dma_reset_hw_chan(struct hisi_dma_chan *chan)
>> +{
>> +	struct hisi_dma_dev *hdma_dev = chan->hdma_dev;
>> +	u32 index = chan->qp_num, tmp;
>> +	int ret;
>> +
>> +	hisi_dma_pause_dma(hdma_dev, index, true);
>> +	hisi_dma_enable_dma(hdma_dev, index, false);
>> +	hisi_dma_mask_irq(hdma_dev, index);
>> +
>> +	ret = readl_relaxed_poll_timeout(hdma_dev->base +
>> +		HISI_DMA_Q_FSM_STS(index), tmp,
>> +		FIELD_GET(HISI_DMA_FSM_STS_MASK, tmp) != RUN, 10, 1000);
>> +	if (ret) {
>> +		dev_err(&hdma_dev->pdev->dev, "disable channel timeout!\n");
>> +		BUG_ON(1);
> 
> we dont kill kernel on this! you should probably complain violently
> (dump_stack() etc)...)
> 

OK, will change to WARN_ON in v5.

Thanks for your help!
Zhou