Re: [PATCH 2/2] dma: Add Spreadtrum DMA controller driver

[Baolin Wang]

Hi,

On å,  7æ 22, 2017 at 01:27:31äå +0530, Vinod Koul wrote:
> On Tue, Jul 18, 2017 at 03:06:12PM +0800, Baolin Wang wrote:
> 
> > +/* DMA global registers definition */
> > +#define DMA_GLB_PAUSE			0x0
> > +#define DMA_GLB_FRAG_WAIT		0x4
> > +#define DMA_GLB_REQ_PEND0_EN		0x8
> > +#define DMA_GLB_REQ_PEND1_EN		0xc
> > +#define DMA_GLB_INT_RAW_STS		0x10
> > +#define DMA_GLB_INT_MSK_STS		0x14
> > +#define DMA_GLB_REQ_STS			0x18
> > +#define DMA_GLB_CHN_EN_STS		0x1c
> > +#define DMA_GLB_DEBUG_STS		0x20
> > +#define DMA_GLB_ARB_SEL_STS		0x24
> > +#define DMA_GLB_CHN_START_CHN_CFG1	0x28
> > +#define DMA_GLB_CHN_START_CHN_CFG2	0x2c
> > +#define DMA_CHN_LLIST_OFFSET		0x10
> > +#define DMA_GLB_REQ_CID(base, uid)	\
> > +		((unsigned long)(base) + 0x2000 + 0x4 * ((uid) - 1))
> 
> why do you need a cast here...

Since the UID registers address is calculated by the UID number. But
I can remove the cast and define the UID global macros.

> 
> > +/* DMA_CHN_INTC register definition */
> > +#define DMA_CHN_INT_MASK		GENMASK(4, 0)
> > +#define DMA_CHN_INT_CLR_OFFSET		24
> 
> Why cant this be expressed in GENMASK?

Since this is not one mask, it is one offset. If we use GENMASK(4, 3)
instead of magic number 24, it is not very clear for the offset bit.

> 
> > +/* DMA_CHN_CFG register definition */
> > +#define DMA_CHN_EN			BIT(0)
> > +#define DMA_CHN_PRIORITY_OFFSET		12
> > +#define LLIST_EN_OFFSET			4
> > +#define CHN_WAIT_BDONE			24
> > +#define DMA_DONOT_WAIT_BDONE		1
> 
> All these too...
> 
> > +static void sprd_dma_set_uid(struct sprd_dma_chn *mchan)
> > +{
> > +	struct sprd_dma_dev *sdev = to_sprd_dma_dev(&mchan->chan);
> > +	u32 dev_id = mchan->dev_id;
> > +
> > +	if (dev_id != DMA_SOFTWARE_UID)
> 
> Whats a UID?

It is for users, every user was assigned one unique hardware ID.
Then the user can trigger the DMA to transfer by the user ID.

> 
> > +		writel(mchan->chn_num + 1, (void __iomem *)DMA_GLB_REQ_CID(
> > +		       sdev->glb_base, dev_id));
> 
> And again the cast, I dont think we need casts like this...

OK.

> 
> > +static void sprd_dma_clear_int(struct sprd_dma_chn *mchan)
> > +{
> > +	u32 intc = readl(mchan->chn_base + DMA_CHN_INTC);
> > +
> > +	intc |= DMA_CHN_INT_MASK << DMA_CHN_INT_CLR_OFFSET;
> > +	writel(intc, mchan->chn_base + DMA_CHN_INTC);
> 
> How about adding a updatel macro and use that here and few other places.

OK.

> 
> > +static void sprd_dma_stop_and_disable(struct sprd_dma_chn *mchan)
> > +{
> > +	u32 cfg = readl(mchan->chn_base + DMA_CHN_CFG);
> > +	u32 pause, timeout = DMA_CHN_PAUSE_CNT;
> > +
> > +	if (!(cfg & DMA_CHN_EN))
> > +		return;
> > +
> > +	pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> > +	pause |= DMA_CHN_PAUSE_EN;
> > +	writel(pause, mchan->chn_base + DMA_CHN_PAUSE);
> > +
> > +	do {
> > +		pause = readl(mchan->chn_base + DMA_CHN_PAUSE);
> > +		if (pause & DMA_CHN_PAUSE_STS)
> > +			break;
> > +
> > +		cpu_relax();
> > +	} while (--timeout > 0);
> 
> We should check here if timeout occured and at least log the error

Yes, will add one warning meesage.

> 
> > +static enum dma_int_type sprd_dma_get_int_type(struct sprd_dma_chn *mchan)
> > +{
> > +	u32 intc_reg = readl(mchan->chn_base + DMA_CHN_INTC);
> > +
> > +	if (intc_reg & CFGERR_INT_STS)
> > +		return CONFIG_ERR;
> > +	else if (intc_reg & LLIST_INT_STS)
> > +		return LIST_DONE;
> > +	else if (intc_reg & TRSC_INT_STS)
> > +		return TRANS_DONE;
> > +	else if (intc_reg & BLK_INT_STS)
> > +		return BLK_DONE;
> > +	else if (intc_reg & FRAG_INT_STS)
> > +		return FRAG_DONE;
> 
> this seems a good case for using switch-cases

OK.

> 
> > +static int sprd_dma_chn_start_chn(struct sprd_dma_chn *mchan,
> > +				  struct sprd_dma_desc *mdesc)
> > +{
> > +	struct sprd_dma_dev *sdev = to_sprd_dma_dev(&mchan->chan);
> > +	enum dma_flags flag = mdesc->dma_flags;
> > +	int chn = mchan->chn_num + 1;
> > +	unsigned int cfg_group1, cfg_group2, start_mode = 0;
> > +
> > +	if (!(flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC | DMA_GROUP1_DST |
> > +		      DMA_GROUP2_DST)))
> > +		return 0;
> > +
> > +	if (flag & (DMA_GROUP1_SRC | DMA_GROUP2_SRC)) {
> > +		switch (flag & (DMA_MUTL_FRAG_DONE | DMA_MUTL_BLK_DONE |
> > +			DMA_MUTL_TRANS_DONE | DMA_MUTL_LIST_DONE)) {
> > +		case DMA_MUTL_FRAG_DONE:
> > +			start_mode = 0;
> > +			break;
> 
> empty line after each break helps with readability

OK.

> 
> > +/*
> > + * struct sprd_dma_cfg: DMA configuration for users
> > + * @config: salve config structure
> 
> typo

Sorry, will fix it.

> 
> > + * @chn_pri: the channel priority
> > + * @datawidth: the data width
> > + * @req_mode: the request mode
> > + * @irq_mode: the interrupt mode
> > + * @swt_mode: the switch mode
> > + * @link_cfg_v: point to the virtual memory address to save link-list DMA
> > + * configuration
> > + * @link_cfg_p: point to the physical memory address to save link-list DMA
> > + * configuration
> > + * @src_addr: the source address
> > + * @des_addr: the destination address
> > + * @fragmens_len: one fragment request length
> > + * @block_len; one block request length
> > + * @transcation_len: one transaction request length
> > + * @src_step: source side transfer step
> > + * @des_step: destination side transfer step
> > + * @src_frag_step: source fragment transfer step
> > + * @dst_frag_step: destination fragment transfer step
> > + * @src_blk_step: source block transfer step
> > + * @dst_blk_step: destination block transfer step
> > + * @wrap_ptr: wrap jump pointer address
> > + * @wrap_to: wrap jump to address
> > + * @dev_id: hardware device id to start DMA transfer
> > + * @is_end: DMA configuration end type
> > + */
> > +struct sprd_dma_cfg {
> > +	struct dma_slave_config config;
> > +	enum dma_pri_level chn_pri;
> > +	enum dma_datawidth datawidth;
> 
> why not use src/dst_addr_width

OK.

> 
> > +	enum dma_request_mode req_mode;
> > +	enum dma_int_type irq_mode;
> > +	enum dma_switch_mode swt_mode;
> 
> what do these modes mean?

I explained these structure where defining these modes.

> 
> > +	unsigned long link_cfg_v;
> > +	unsigned long link_cfg_p;
> 
> why should clients know and send this, seems internal to dma

Since there are some special cases for our DMA. This driver can
cover several DMA controllers, but for some special cases one
DMA controller just can access the memories which specified
by the user. Anyway will fix this issue in next version.

> 
> > +	unsigned long src_addr;
> > +	unsigned long des_addr;
> 
> whats wrong with src/dst_addr

Will fix this.

> 
> > +	u32 fragmens_len;
> > +	u32 block_len;
> 
> oh please, I think I will stop here now :(
> 
> > +	u32 transcation_len;
> > +	u32 src_step;
> > +	u32 des_step;
> > +	u32 src_frag_step;
> > +	u32 dst_frag_step;
> > +	u32 src_blk_step;
> > +	u32 dst_blk_step;
> > +	u32 wrap_ptr;
> > +	u32 wrap_to;
> > +	u32 dev_id;
> > +	enum dma_end_type is_end;
> 
> Looking at this I think these are overkill, many of them can be handled
> properly by current dmaengine interfaces, so please use those before you
> invent your own...
> 
> Also the code is bloated because you don't use virt-dma, pls use that. I
> skipped many parts of the driver as I feel driver needs more work.

OK. I will check the virt-dma. Thanks for your commnets.