On Thu, Jun 22, 2017 at 04:50:57PM +0200, Stefan Roese wrote:
> 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.
I think there is something, but can't recall which one. Andy?
> >>>>+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 ...
That makes sense
--
~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
