Re: [Patch v4 2/2] dmaengine: Add ADM driver

[Vinod Koul <vinod.koul@intel.com>]

On Wed, Feb 11, 2015 at 11:46:05PM -0600, Andy Gross wrote:
> +++ b/drivers/dma/qcom_adm.c
> @@ -0,0 +1,901 @@
> +/*
> + * Copyright (c) 2013-2014, The Linux Foundation. All rights reserved.
shouldn't this be 15 :)

 +/* ADM registers - calculated from channel number and security domain */
> +#define HI_CH_CMD_PTR(chan, ee)		(4*chan + 0x20800*ee)
> +#define HI_CH_RSLT(chan, ee)		(0x40 + 4*chan + 0x20800*ee)
> +#define HI_CH_FLUSH_STATE0(chan, ee)	(0x80 + 4*chan + 0x20800*ee)
> +#define HI_CH_FLUSH_STATE1(chan, ee)	(0xc0 + 4*chan + 0x20800*ee)
> +#define HI_CH_FLUSH_STATE2(chan, ee)	(0x100 + 4*chan + 0x20800*ee)
> +#define HI_CH_FLUSH_STATE3(chan, ee)	(0x140 + 4*chan + 0x20800*ee)
> +#define HI_CH_FLUSH_STATE4(chan, ee)	(0x180 + 4*chan + 0x20800*ee)
> +#define HI_CH_FLUSH_STATE5(chan, ee)	(0x1c0 + 4*chan + 0x20800*ee)
> +#define HI_CH_STATUS_SD(chan, ee)	(0x200 + 4*chan + 0x20800*ee)
> +#define HI_CH_CONF(chan)		(0x240 + 4*chan)
> +#define HI_CH_RSLT_CONF(chan, ee)	(0x300 + 4*chan + 0x20800*ee)
> +#define HI_SEC_DOMAIN_IRQ_STATUS(ee)	(0x380 + 0x20800*ee)
> +#define HI_CI_CONF(ci)			(0x390 + 4*ci)
> +#define HI_CRCI_CONF0			0x3d0
> +#define HI_CRCI_CONF1			0x3d4
> +#define HI_GP_CTL			0x3d8
> +#define HI_CRCI_CTL(crci, ee)		(0x400 + 0x4*crci + 0x20800*ee)
two things, one the names are quite generic and may cause conflicts so pls
fix that. Second the values, what is the deal with 4*chan, should that be a
define as well. Also rather than copy pasting a macros would be better for
this expansion

> +
> +/* channel status */
> +#define CH_STATUS_VALID	BIT(1)
> +
> +/* channel result */
> +#define CH_RSLT_VALID	BIT(31)
> +#define CH_RSLT_ERR	BIT(3)
> +#define CH_RSLT_FLUSH	BIT(2)
> +#define CH_RSLT_TPD	BIT(1)
> +
> +/* channel conf */
> +#define CH_CONF_MPU_DISABLE	BIT(11)
> +#define CH_CONF_PERM_MPU_CONF	BIT(9)
> +#define CH_CONF_FLUSH_RSLT_EN	BIT(8)
> +#define CH_CONF_FORCE_RSLT_EN	BIT(7)
> +#define CH_CONF_IRQ_EN		BIT(6)
> +
> +/* channel result conf */
> +#define CH_RSLT_CONF_FLUSH_EN	BIT(1)
> +#define CH_RSLT_CONF_IRQ_EN	BIT(0)
> +
> +/* CRCI CTL */
> +#define CRCI_CTL_MUX_SEL	BIT(18)
> +#define CRCI_CTL_RST		BIT(17)
> +
> +/* CI configuration */
> +#define CI_RANGE_END(x)		(x << 24)
> +#define CI_RANGE_START(x)	(x << 16)
> +#define CI_BURST_4_WORDS	0x4
> +#define CI_BURST_8_WORDS	0x8
shouldn't you be consistent is usage of BIT()

> +
> +/* GP CTL */
> +#define GP_CTL_LP_EN		BIT(12)
> +#define GP_CTL_LP_CNT(x)	(x << 8)
> +
> +/* Command pointer list entry */
> +#define CPLE_LP			BIT(31)
> +#define CPLE_CMD_PTR_LIST	BIT(29)
> +
> +/* Command list entry */
> +#define CMD_LC			BIT(31)
> +#define CMD_DST_CRCI(n)		(((n) & 0xf) << 7)
> +#define CMD_SRC_CRCI(n)		(((n) & 0xf) << 3)
> +
> +#define CMD_TYPE_SINGLE		0x0
> +#define CMD_TYPE_BOX		0x3a
naming issues...
> +static int adm_alloc_chan(struct dma_chan *chan)
> +{
> +	return 0;
> +}
This is no longer mandatory, so can be dropped

> +static int adm_get_blksize(unsigned int burst)
> +{
> +	int ret;
> +
> +	switch (burst) {
> +	case 16:
> +		ret = 0;
> +		break;
> +	case 32:
> +		ret = 1;
> +		break;
> +	case 64:
> +		ret = 2;
> +		break;
> +	case 128:
> +		ret = 3;
> +		break;
> +	case 192:
> +		ret = 4;
> +		break;
> +	case 256:
> +		ret = 5;
> +		break;
ffs(burst>>4) ?

> +static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
> +	struct scatterlist *sgl, unsigned int sg_len,
> +	enum dma_transfer_direction direction, unsigned long flags,
> +	void *context)
> +{
> +	struct adm_chan *achan = to_adm_chan(chan);
> +	struct adm_device *adev = achan->adev;
> +	struct adm_async_desc *async_desc;
> +	struct scatterlist *sg;
> +	u32 i;
> +	u32 single_count = 0, box_count = 0, desc_offset = 0, crci = 0;
> +	struct adm_desc_hw_box *box_desc;
> +	struct adm_desc_hw_single *single_desc;
> +	void *desc;
> +	u32 *cple, *last_cmd;
> +	u32 burst;
> +	int blk_size = 0;
> +
> +
> +	if (!is_slave_direction(direction)) {
> +		dev_err(adev->dev, "invalid dma direction\n");
> +		return NULL;
> +	}
> +
> +	/*
> +	 * get burst value from slave configuration
> +	 * If zero, default to maximum burst size
> +	 * If larger than the max transfer size, set to ADM_MAX_XFER
> +	 */
> +	burst = (direction == DMA_MEM_TO_DEV) ?
> +		achan->slave.dst_maxburst :
> +		achan->slave.src_maxburst;
> +
> +	if (!burst || burst > ADM_MAX_XFER)
> +		burst = ADM_MAX_XFER;
For slave, we should send error here. The DMA settings are matched with
slave FIFO, so any change here can cause issues

> +
> +	/* if using flow control, validate burst and crci values */
> +	if (achan->slave.device_fc) {
> +
> +		blk_size = adm_get_blksize(burst);
> +		if (blk_size < 0) {
> +			dev_err(adev->dev, "invalid burst value w/ crci: %d\n",
> +				burst);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		crci = achan->slave.slave_id & 0xf;
> +		if (!crci || achan->slave.slave_id > 0x1f) {
> +			dev_err(adev->dev, "invalid crci value\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +	}
> +
> +	/* iterate through sgs and compute allocation size of structures */
> +	for_each_sg(sgl, sg, sg_len, i)
> +		if (sg_dma_len(sg) % burst)
> +			single_count += DIV_ROUND_UP(sg_dma_len(sg), burst);
> +		else
> +			box_count += DIV_ROUND_UP(sg_dma_len(sg) / burst,
> +							ADM_MAX_ROWS);
> +
> +	async_desc = kzalloc(sizeof(*async_desc), GFP_NOWAIT);
> +	if (!async_desc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (crci)
> +		async_desc->mux = achan->slave.slave_id >> 4 ?
> +					CRCI_CTL_MUX_SEL : 0;
> +	async_desc->crci = crci;
> +	async_desc->blk_size = blk_size;
> +	async_desc->dma_len = single_count * sizeof(*single_desc) +
> +				box_count * sizeof(*box_desc) + sizeof(*cple) +
> +				2*ADM_DESC_ALIGN;
> +
> +	async_desc->cpl = dma_alloc_writecombine(adev->dev, async_desc->dma_len,
> +				&async_desc->dma_addr, GFP_NOWAIT);
> +
> +	if (!async_desc->cpl) {
> +		kfree(async_desc);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	async_desc->adev = adev;
> +
> +	/* both command list entry and descriptors must be 8 byte aligned */
> +	cple = PTR_ALIGN(async_desc->cpl, ADM_DESC_ALIGN);
> +	desc = PTR_ALIGN(cple + 1, ADM_DESC_ALIGN);
> +	last_cmd = desc;
> +
> +	/* init cmd list */
> +	*cple = CPLE_LP;
> +	*cple |= (desc - async_desc->cpl + async_desc->dma_addr) >> 3;
> +
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		unsigned int xfer_len, row_len, rows;
> +		unsigned int remainder = sg_dma_len(sg);
> +		unsigned int offset = 0;
> +
> +		do {
> +			/* use single if length is not multiple of burst */
> +			if (remainder % burst) {
> +				single_desc = desc + desc_offset;
> +				last_cmd = &single_desc->cmd;
> +				single_desc->cmd = CMD_TYPE_SINGLE;
> +				xfer_len = (remainder > ADM_MAX_XFER) ?
> +					ADM_MAX_XFER : remainder;
> +				if (direction == DMA_DEV_TO_MEM) {
> +					single_desc->dst_addr =
> +						sg_dma_address(sg) + offset;
> +					single_desc->src_addr =
> +						achan->slave.src_addr;
> +					single_desc->len = xfer_len;
> +					single_desc->cmd |= CMD_SRC_CRCI(crci);
> +				} else {
> +					single_desc->src_addr =
> +						sg_dma_address(sg) + offset;
> +					single_desc->dst_addr =
> +						achan->slave.dst_addr;
> +					single_desc->len = xfer_len;
> +					single_desc->cmd |= CMD_DST_CRCI(crci);
> +				}
> +
> +				remainder -= xfer_len;
> +				async_desc->length += xfer_len;
> +				offset += xfer_len;
> +				desc_offset += sizeof(*single_desc);
> +			} else {
> +				box_desc = desc + desc_offset;
> +				last_cmd = &box_desc->cmd;
> +				box_desc->cmd = CMD_TYPE_BOX;
> +				box_desc->row_offset = 0;
> +
> +				if (direction == DMA_DEV_TO_MEM) {
> +					box_desc->dst_addr =
> +						sg_dma_address(sg) + offset;
> +					box_desc->src_addr =
> +						achan->slave.src_addr;
> +					box_desc->cmd |= CMD_SRC_CRCI(crci);
> +					box_desc->row_offset = burst;
> +
> +				} else {
> +					box_desc->src_addr =
> +						sg_dma_address(sg) + offset;
> +					box_desc->dst_addr =
> +						achan->slave.dst_addr;
> +					box_desc->cmd |= CMD_DST_CRCI(crci);
> +					box_desc->row_offset = burst << 16;
> +				}
till this both look quite similar, also you are way deeply nested. I would
split this up to make it look better as well try reusing common parts for
both here

> +static int adm_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> +	unsigned long arg)
> +{
this is removed, so you need to rebase this

Thanks
-- 
~Vinod