On Wed, May 24, 2017 at 09:00:48AM +0200, Stefan Roese wrote:
> +#define MSGDMA_MAX_TRANS_LEN 0xffffffff
GENAMSK?
> +struct msgdma_extended_desc {
> + u32 read_addr_lo; /* data buffer source address low bits */
> + u32 write_addr_lo; /* data buffer destination address low bits */
> + u32 len; /* the number of bytes to transfer
> + * per descriptor
> + */
> + u32 burst_seq_num; /* bit 31:24 write burst
> + * bit 23:16 read burst
> + * bit 15:0 sequence number
> + */
> + u32 stride; /* bit 31:16 write stride
> + * bit 15:0 read stride
> + */
> + u32 read_addr_hi; /* data buffer source address high bits */
> + u32 write_addr_hi; /* data buffer destination address high bits */
> + u32 control; /* characteristics of the transfer */
nice comments but can be made kernel-doc style
> +#define MSGDMA_DESC_CTL_TR_ERR_IRQ (0xff << 16)
GENMASK?
> +#define MSGDMA_CSR_STAT_MASK 0x3FF
> +#define MSGDMA_CSR_STAT_MASK_WITHOUT_IRQ 0x1FF
GENMASK for these two
> +
> +#define MSGDMA_CSR_STAT_BUSY_GET(v) GET_BIT_VALUE(v, 0)
This is not defined globally and seems to be an altera define but I don't
see altera_tse.h included here ??
> +/* mSGDMA response register bit definitions */
> +#define MSGDMA_RESP_EARLY_TERM BIT(8)
> +#define MSGDMA_RESP_ERR_MASK 0xFF
GENMASK
> +static dma_cookie_t msgdma_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> + struct msgdma_device *mdev = to_mdev(tx->chan);
> + struct msgdma_sw_desc *desc, *new;
> + dma_cookie_t cookie;
> +
> + new = tx_to_desc(tx);
> + spin_lock_bh(&mdev->lock);
> + cookie = dma_cookie_assign(tx);
> +
> + if (!list_empty(&mdev->pending_list)) {
> + desc = list_last_entry(&mdev->pending_list,
> + struct msgdma_sw_desc, node);
> + if (!list_empty(&desc->tx_list))
> + desc = list_last_entry(&desc->tx_list,
> + struct msgdma_sw_desc, node);
> + }
desc is not used anywhere so not really seeing the point of this code..
> +static struct dma_async_tx_descriptor *msgdma_prep_memcpy(
> + struct dma_chan *dchan, dma_addr_t dma_dst,
> + dma_addr_t dma_src, size_t len, ulong flags)
> +{
> + struct msgdma_device *mdev = to_mdev(dchan);
> + struct msgdma_sw_desc *new, *first = NULL;
> + struct msgdma_extended_desc *desc;
> + size_t copy;
> + u32 desc_cnt;
> +
> + if (len > MSGDMA_MAX_TRANS_LEN)
> + return NULL;
why :)
Nothing prevents you from splitting this to N descriptors of
MSGDMA_MAX_TRANS_LEN and one remaining one, though it would be great to have
this, but not a deal breaker
> +
> + desc_cnt = DIV_ROUND_UP(len, MSGDMA_MAX_TRANS_LEN);
and in that case why would you need this??
> +static struct dma_async_tx_descriptor *msgdma_prep_sg(
> + struct dma_chan *dchan, struct scatterlist *dst_sg,
> + unsigned int dst_sg_len, struct scatterlist *src_sg,
> + unsigned int src_sg_len, unsigned long flags)
> +{
> + struct msgdma_device *mdev = to_mdev(dchan);
> + struct msgdma_sw_desc *new, *first = NULL;
> + void *desc = NULL;
> + size_t len, dst_avail, src_avail;
> + dma_addr_t dma_dst, dma_src;
> + u32 desc_cnt = 0, i;
> + struct scatterlist *sg;
> +
> + for_each_sg(src_sg, sg, src_sg_len, i)
> + desc_cnt += DIV_ROUND_UP(sg_dma_len(sg), MSGDMA_MAX_TRANS_LEN);
> +
> + spin_lock_bh(&mdev->lock);
> + if (desc_cnt > mdev->desc_free_cnt) {
> + spin_unlock_bh(&mdev->lock);
> + dev_dbg(mdev->dev, "mdev %p descs are not available\n", mdev);
> + return NULL;
> + }
> + mdev->desc_free_cnt -= desc_cnt;
> + spin_unlock_bh(&mdev->lock);
> +
> + dst_avail = sg_dma_len(dst_sg);
> + src_avail = sg_dma_len(src_sg);
> +
> + /* Run until we are out of scatterlist entries */
> + while (true) {
> + /* Allocate and populate the descriptor */
> + new = msgdma_get_descriptor(mdev);
> +
> + desc = &new->hw_desc;
> + len = min_t(size_t, src_avail, dst_avail);
> + len = min_t(size_t, len, MSGDMA_MAX_TRANS_LEN);
> + if (len == 0)
> + goto fetch;
> + dma_dst = sg_dma_address(dst_sg) + sg_dma_len(dst_sg) -
> + dst_avail;
right justified pls
> +static struct dma_async_tx_descriptor *msgdma_prep_slave_sg(
> + struct dma_chan *dchan, struct scatterlist *sgl,
> + unsigned int sg_len, enum dma_transfer_direction dir,
> + unsigned long flags, void *context)
these should be right justified as well, its quite hard to read
> +static void msgdma_copy_one(struct msgdma_device *mdev,
> + struct msgdma_sw_desc *desc)
> +{
> + struct msgdma_extended_desc *hw_desc = mdev->desc;
> +
> + /*
> + * Check if the DESC FIFO it not full. If its full, we need to wait
> + * for at least one entry to become free again
> + */
> + while (ioread32(&mdev->csr->status) & MSGDMA_CSR_STAT_DESC_BUF_FULL)
> + mdelay(1);
> +
> + memcpy(hw_desc, &desc->hw_desc, sizeof(desc->hw_desc) - 4);
whats the magical 4 here?
> +static void msgdma_complete_descriptor(struct msgdma_device *mdev)
> +{
> + struct msgdma_sw_desc *desc;
> +
> + desc = list_first_entry_or_null(&mdev->active_list,
> + struct msgdma_sw_desc, node);
> + if (!desc)
> + return;
> + list_del(&desc->node);
> + dma_cookie_complete(&desc->async_tx);
> + list_add_tail(&desc->node, &mdev->done_list);
when do you move from done to free list
> +static void msgdma_free_descriptors(struct msgdma_device *mdev)
> +{
> + msgdma_free_desc_list(mdev, &mdev->active_list);
> + msgdma_free_desc_list(mdev, &mdev->pending_list);
> + msgdma_free_desc_list(mdev, &mdev->done_list);
btw when are the descriptors in free list freedup
> +static int msgdma_alloc_chan_resources(struct dma_chan *dchan)
> +{
> + struct msgdma_device *mdev = to_mdev(dchan);
> + struct msgdma_sw_desc *desc;
> + int i;
> +
> + mdev->sw_desq = kzalloc(sizeof(*desc) * MSGDMA_DESC_NUM, GFP_KERNEL);
GFP_NOWAIT pls
> +static void msgdma_tasklet(unsigned long data)
> +{
> + struct msgdma_device *mdev = (struct msgdma_device *)data;
> + u32 count;
> + u32 size;
> + u32 status;
> +
> + spin_lock(&mdev->lock);
> +
> + /* Read number of responses that are available */
> + count = ioread32(&mdev->csr->resp_fill_level);
> + pr_debug("%s (%d): response count=%d\n", __func__, __LINE__, count);
dev_ variants please
> +static irqreturn_t msgdma_irq_handler(int irq, void *data)
> +{
> + struct msgdma_device *mdev = data;
> +
> + tasklet_schedule(&mdev->irq_tasklet);
not submitting next descriptor here..??
> +MODULE_DESCRIPTION("Altera mSGDMA driver");
> +MODULE_AUTHOR("Stefan Roese <sr@xxxxxxx>");
> +MODULE_LICENSE("GPL");
no alias?
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
