Hi Vinod,
On 22.06.2017 15:00, Vinod Koul wrote:
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 :)
In this specific case, I see your point and would be willing to make
this change gladly. But in the case above, its a the largest number
of a u32 variable. I find it more confusing using GENMASK here.
Probably there is a macro already for this u32(-1) value, that I
should use instead.
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
Okay, will change either way to match "your taste". ;)
+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
This driver will (hopefully) be used on multiple archs (we test ARM
and x86), so I will stick with the current version and add some barries
and ...
Also this is good info and should be added here, will help other and
possibly you down the line :)
... and some comments. :)
+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..
Already coded here this way. This change will be included in v2.
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
