Hi Vinod,
On 14.06.2017 10:25, Vinod Koul wrote:
> On Wed, May 24, 2017 at 09:00:48AM +0200, Stefan Roese wrote:
>
>> +#define MSGDMA_MAX_TRANS_LEN 0xffffffff
>
> GENAMSK?
I'm personally not a big fan of GENMASK. BIT(x) is good for single
bits, but GENMASK usually looks lees clear - at least to me. So if
you do not insist in this change, I would like to keep the masks.
>> +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
Sure. Will change in v2.
>> +#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 ??
Will remove in v2.
>> +/* mSGDMA response register bit definitions */
>> +#define MSGDMA_RESP_EARLY_TERM BIT(8)
>> +#define MSGDMA_RESP_ERR_MASK 0xFF
>
> GENMASK
Again, please let me know if I need to change to using GENMASK (s.o.).
>> +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..
Good catch, thanks. Its some relict from porting the code from the
Xilinx DMA driver to this engine. Will remove in v2.
>> +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 :)
This doesn't make much sense, agreed. :)
> 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
This is what the code below actually does. FWICT, I only need to
remove the first "len" check above and it should just work this way.
I'll give it a test and will change the code accordingly.
>> +
>> + desc_cnt = DIV_ROUND_UP(len, MSGDMA_MAX_TRANS_LEN);
>
> and in that case why would you need this??
All this is copied from the Xiling Zynqmp DMA driver. I'll send
a fix for this driver once all this is sorted out.
>> +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
Thats the default indentation of my Emacs Linux C-style. Do I really
need to change this?
>> +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
Hmmm, this will result in more lines for the function header, as
only one parameter will fit into one line. I could change it this way
if you prefer this:
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)
Just let know.
>> +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?
Its the size of the last word in the descriptor. By writing this last
word, the complete descriptor is flushed from the FIFO to the
DMA controller. I wanted to make sure by using a single iowrite()
after this memcpy(), that this control word will be written after
all other descriptor words have been copied.
BTW: Do you know if memcpy() is guaranteed to copy the data always
in the incrementing "direction"? If yes, I can probably drop the
iowrite32() call and only use memcpy() 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
They are moved to the free_list in msgdma_free_descriptor().
The call-list here is:
msgdma_free_chan_resources()
-> msgdma_free_descriptors()
-> msgdma_free_desc_list()
-> msgdma_free_descriptor()
>> +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
You mean, when is the memory free'ed? Its free'ed in
msgdma_free_chan_resources(). This mechanism is also copied
from the Zynqmp driver btw.
>> +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
Ah, thanks. Will change in v2.
>> +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
Sure.
>> +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..??
You mean a detection of an "idle" DMA controller state and
submitting potentially pending descriptors in this case? I'll check,
if this can be done...
>> +MODULE_DESCRIPTION("Altera mSGDMA driver");
>> +MODULE_AUTHOR("Stefan Roese <sr@xxxxxxx>");
>> +MODULE_LICENSE("GPL");
>
> no alias?
Sure, will add.
Many thanks for the detailed review.
Thanks,
Stefan
--
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
