On Tue, Jun 20, 2017 at 02:20:49PM +0200, Stefan Roese wrote:
> 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.
Well GENMASK(14, 11) tell me that it means mask of bit 14 thru 11, which is
way clear than 0x7800 :)
IMHO it adds readability and helps avoid bit errors while coding from specs
> Good catch, thanks. Its some relict from porting the code from the
> Xilinx DMA driver to this engine. Will remove in v2.
ha ha that was my hunch too
> >> + 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?
I prefer right justified as that's more readable and easy on (my) eye
> 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.
Looks much better, helps when you have been staring at your terminal whole
day to notice the lines are justified and not the body of function which I
would expect at that level
> >> +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.
IIUC memcpy would be arch dependent and based on ordering, we don't have
absolute guarantee that iowrite would happen after :) so you need to check
the arch and possibly add barriers
Also this is good info and should be added here, will help other and
possibly you down the line :)
> >> +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...
Yes that will greatly increase the performance of the driver, few drivers
already do so but sadly a lot more don't do..
--
~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
