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
