Message-ID: <1493049305.25985.4.camel@synopsys.com>
Date:   Mon, 24 Apr 2017 15:55:06 +0000
From:   Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>
To:     "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>
CC:     "vinod.koul@...el.com" <vinod.koul@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "Alexey.Brodkin@...opsys.com" <Alexey.Brodkin@...opsys.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "Eugeniy.Paltsev@...opsys.com" <Eugeniy.Paltsev@...opsys.com>,
        "linux-snps-arc@...ts.infradead.org" 
        <linux-snps-arc@...ts.infradead.org>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver

Hi,
On Fri, 2017-04-21 at 18:13 +0300, Andy Shevchenko wrote:
> On Fri, 2017-04-21 at 14:29 +0000, Eugeniy Paltsev wrote:
> > On Tue, 2017-04-18 at 15:31 +0300, Andy Shevchenko wrote:
> > > On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:
> > > > This patch adds support for the DW AXI DMAC controller.
> > > > +#define AXI_DMA_BUSWIDTHS		  \
> > > > +	(DMA_SLAVE_BUSWIDTH_1_BYTE	| \
> > > > +	DMA_SLAVE_BUSWIDTH_2_BYTES	| \
> > > > +	DMA_SLAVE_BUSWIDTH_4_BYTES	| \
> > > > +	DMA_SLAVE_BUSWIDTH_8_BYTES	| \
> > > > +	DMA_SLAVE_BUSWIDTH_16_BYTES	| \
> > > > +	DMA_SLAVE_BUSWIDTH_32_BYTES	| \
> > > > +	DMA_SLAVE_BUSWIDTH_64_BYTES)
> > > > +/* TODO: check: do we need to use BIT() macro here? */
> > >
> > > Still TODO? I remember I answered to this on the first round.
> >
> > Yes, I remember it.
> > I left this TODO as a reminder because src_addr_widths and
> > dst_addr_widths are
> > not used anywhere and they are set differently in different drivers
> > (with or without BIT macro).
>
> Strange. AFAIK they are representing bits (which is not the best
> idea) in the resulting u64 field. So, anything bigger than 63 doesn't
> make sense.
They are u32 fields!
From dmaengine.h :
struct dma_device {
...
    u32 src_addr_widths;
    u32 dst_addr_widths;
};

> In drivers where they are not bits quite likely a bug is hidden.
For example (from pxa_dma.c):
const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE |
DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES;

And there are a lot of drivers, which don't use BIT for this fields.
sh/shdmac.c
sh/rcar-dmac.c
qcom/bam_dma.c
mmp_pdma.c
ste_dma40.c
And many others...

> >
> > > > +
> > > > +static inline void
> > > > +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val)
> > > > +{
> > > > +	iowrite32(val, chip->regs + reg);
> > >
> > > Are you going to use IO ports for this IP? I don't think so.
> > > Wouldn't be better to call readl()/writel() instead?
> >
> > As I understand, it's better to use ioread/iowrite as more
> > universal
> > IO
> > access way. Am I wrong?
>
> As I said above the ioreadX/iowriteX makes only sense when your IP
> would be accessed via IO region or MMIO. I'm pretty sure IO is not
> the case at all for this IP.
MMIO? This IP works exactly via memory-mapped I/O.

> > > > +static inline void axi_chan_irq_disable(struct axi_dma_chan
> > > > *chan,
> > > > u32 irq_mask)
> > > > +{
> > > > +	u32 val;
> > > > +
> > > > +	if (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) {
> > > > +		axi_chan_iowrite32(chan, CH_INTSTATUS_ENA,
> > > > DWAXIDMAC_IRQ_NONE);
> > > > +	} else {
> > >
> > > I don't see the benefit. (Yes, I see one read-less path, I think
> > > it
> > > makes perplexity for nothing here)
> >
> > This function is called mostly with irq_mask = DWAXIDMAC_IRQ_ALL.
> > Actually it is called only with irq_mask = DWAXIDMAC_IRQ_ALL until
> > I add DMA_SLAVE support.
> > But I can cut off this 'if' statment, if it is necessery.
>
> It's not necessary, but from my point of view increases readability
> of the code a lot for the price of an additional readl().
Ok.

> >
> > > > +		val = axi_chan_ioread32(chan,
> > > > CH_INTSTATUS_ENA);
> > > > +		val &= ~irq_mask;
> > > > +		axi_chan_iowrite32(chan, CH_INTSTATUS_ENA,
> > > > val);
> > > > +	}
> > > > +
> > > > +	return min_t(size_t, __ffs(sdl), max_width);
> > > > +}
> > > > +static void axi_desc_put(struct axi_dma_desc *desc)
> > > > +{
> > > > +	struct axi_dma_chan *chan = desc->chan;
> > > > +	struct dw_axi_dma *dw = chan->chip->dw;
> > > > +	struct axi_dma_desc *child, *_next;
> > > > +	unsigned int descs_put = 0;
> > > > +	list_for_each_entry_safe(child, _next, &desc-
> > > > >xfer_list,
> > > > xfer_list) {
> > >
> > > xfer_list looks redundant.
> > > Can you elaborate why virtual channel management is not working
> > > for
> > > you?
> >
> > Each virtual descriptor encapsulates several hardware descriptors,
> > which belong to same transfer.
> > This list (xfer_list) is used only for allocating/freeing these
> > descriptors and it doesn't affect on virtual dma work logic.
> > I can see this approach in several drivers with VirtDMA (but they
> > mostly use array instead of list)
>
> You described how most of the DMA drivers are implemented, though
> they
> are using just sg_list directly. I would recommend to do the same and
> get rid of this list.
This IP can be (ans is) configured with small block size.
(note, that I am not saying about runtime HW configuration)

And there is opportunity what we can't use sg_list directly and need to
split sg_list to a smaller chunks.

> > > Btw, are you planning to use priority at all? For now on I didn't
> > > see
> > > a single driver (from the set I have checked, like 4-5 of them)
> > > that
> > > uses priority anyhow. It makes driver more complex for nothing.
> >
> > Only for dma slave operations.
>
> So, in other words you *have* an actual two or more users that *need*
> prioritization?
As I remember there was an idea to give higher priority to audio dma
chanels.

> > > > +	if (unlikely(dst_nents == 0 || src_nents == 0))
> > > > +		return NULL;
> > > > +
> > > > +	if (unlikely(dst_sg == NULL || src_sg == NULL))
> > > > +		return NULL;
> > >
> > > If we need those checks they should go to
> > > dmaengine.h/dmaengine.c.
> >
> > I checked several drivers, which implements device_prep_dma_sg and
> > they
> > implements this checkers.
> > Should I add something like "dma_sg_desc_invalid" function to
> > dmaengine.h ?
>
> I suppose it's better to implement them in the corresponding helpers
> in dmaengine.h.
Ok.

> > > > +static void axi_chan_list_dump_lli(struct axi_dma_chan *chan,
> > > > +				   struct axi_dma_desc
> > > > *desc_head)
> > > > +{
> > > > +	struct axi_dma_desc *desc;
> > > > +
> > > > +	axi_chan_dump_lli(chan, desc_head);
> > > > +	list_for_each_entry(desc, &desc_head->xfer_list,
> > > > xfer_list)
> > > > +		axi_chan_dump_lli(chan, desc);
> > > > +}
> > > > +
> > > > +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32
> > > > status)
> > > > +{
> > > > +	/* WARN about bad descriptor */
> > > >
> > > > +	dev_err(chan2dev(chan),
> > > > +		"Bad descriptor submitted for %s, cookie: %d,
> > > > irq:
> > > > 0x%08x\n",
> > > > +		axi_chan_name(chan), vd->tx.cookie, status);
> > > > +	axi_chan_list_dump_lli(chan, vd_to_axi_desc(vd));
> > >
> > > As I said earlier dw_dmac is *bad* example of the (virtual
> > > channel
> > > based) DMA driver.
> > >
> > > I guess you may just fail the descriptor and don't pretend it has
> > > been processed successfully.
> >
> > What do you mean by saying "fail the descriptor"?
> > After I get error I cancel current transfer and free all
> > descriptors
> > from it (by calling vchan_cookie_complete).
> > I can't store error status in descriptor structure because it will
> > be
> > freed by vchan_cookie_complete.
> > I can't store error status in channel structure because it will be
> > overwritten by next transfer.
>
> Better not to pretend that it has been processed successfully. Don't
> call callback on it and set its status to DMA_ERROR (that's why
> descriptors in many drivers have dma_status field). When user asks
> for
> status (using cookie) the saved value would be returned until
> descriptor
> is active.
>
> Do you have some other workflow in mind?

Hmm...
Do you mean I should left error descriptors in desc_issued list
or I should create another list (like desc_error) in my driver and move
error descriptors to desc_error list?

And when exactly should I free error descriptors?

I checked hsu/hsu.c dma driver implementation:
  vdma descriptor is deleted from desc_issued list when transfer
  starts. When descriptor marked as error descriptor
  vchan_cookie_complete isn't called for this descriptor. And this
  descriptor isn't placed in any list. So error descriptors *never*
  will be freed.
  I don't actually like this approach.

> > > > +
> > > > +static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> > > > +	SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend,
> > > > axi_dma_runtime_resume, NULL)
> > > > +};
> > >
> > > Have you tried to build with CONFIG_PM disabled?
> >
> > Yes.
> >
> > > I'm pretty sure you need __maybe_unused applied to your PM ops.
> >
> > I call axi_dma_runtime_suspend / axi_dma_runtime_resume even I
> > dont't
> > use PM.
> > (I call them in probe / remove function.)
>
> Hmm... I didn't check your ->probe() and ->remove(). Do you mean you
> call them explicitly by those names?
>
> If so, please don't do that. Use pm_runtime_*() instead. And...
>
> > So I don't need to declare them with __maybe_unused.
>
> ...in that case it's possible you have them defined but not used.
>
From my ->probe() function:

pm_runtime_get_noresume(chip->dev);
ret = axi_dma_runtime_resume(chip->dev);

Firstly I only incrememt counter.
Secondly explicitly call my resume function.

I call them explicitly because I need driver to work also without
Runtime PM. So I can't just call pm_runtime_get here instead of
pm_runtime_get_noresume + axi_dma_runtime_resume.

Of course I can copy *all* code from axi_dma_runtime_resume
to ->probe() function, but I don't really like this idea.

> > > > +struct axi_dma_chan {
> > > > +	struct axi_dma_chip		*chip;
> > > > +	void __iomem			*chan_regs;
> > > > +	u8				id;
> > > > +	atomic_t			descs_allocated;
> > > > +
> > > > +	struct virt_dma_chan		vc;
> > > > +
> > > > +	/* these other elements are all protected by vc.lock
> > > > */
> > > > +	bool				is_paused;
> > >
> > > I still didn't get (already forgot) why you can't use dma_status
> > > instead for the active descriptor?
> >
> > As I said before, I checked several driver, which have status
> > variable
> > in their channel structure - it is used *only* for determinating is
> > channel paused or not. So there is no much sense in replacing
> > "is_paused" to "status" and I left "is_paused" variable untouched.
>
> Not only (see above), the errored descriptor keeps that status.
>
> > (I described above why we can't use status in channel structure for
> > error handling)
>
> Ah, I'm talking about descriptor.

Again - PAUSED is per-channel flag. So, even if we have status field in
each descriptor, it is simpler to use one per-channel flag instead of
plenty per-descriptor flags.
When we pausing/resuming dma channel it is simpler to set only one flag
instead of writing DMA_PAUSED to *each* descriptor status field.

> > > > Status Fetch Addr */
> > > > +#define CH_INTSTATUS_ENA	0x080 /* R/W Chan Interrupt
> > > > Status
> > > > Enable */
> > > > +#define CH_INTSTATUS		0x088 /* R/W Chan
> > > > Interrupt
> > > > Status */
> > > > +#define CH_INTSIGNAL_ENA	0x090 /* R/W Chan Interrupt
> > > > Signal
> > > > Enable */
> > > > +#define CH_INTCLEAR		0x098 /* W Chan Interrupt
> > > > Clear
> > > > */
> > >
> > > I'm wondering if you can use regmap API instead.
> >
> > Is it really necessary? It'll make driver more complex for nothing.
>
> That's why I'm not suggesting but wondering.
> >
> > > > +};
> > >
> > > Hmm... do you need them in the header?
> >
> > I use some of these definitions in my code so I guess yes.
> > /* Maybe I misunderstood your question... */
>
> I mean, who are the users of them? If it's only one module, there is
> no need to put them in header.
Yes, only one module.
Should I move all this definitions to axi_dma_platform.c file and rid
of both axi_dma_platform_reg.h and axi_dma_platform.h headers?

> >
> > > > +#define CH_CFG_H_TT_FC_POS	0
> > > > +enum {
> > > > +	DWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC	= 0x0,
> > > > +	DWAXIDMAC_TT_FC_MEM_TO_PER_DMAC,
> > > > +	DWAXIDMAC_TT_FC_PER_TO_MEM_DMAC,
> > > > +	DWAXIDMAC_TT_FC_PER_TO_PER_DMAC,
> > > > +	DWAXIDMAC_TT_FC_PER_TO_MEM_SRC,
> > > > +	DWAXIDMAC_TT_FC_PER_TO_PER_SRC,
> > > > +	DWAXIDMAC_TT_FC_MEM_TO_PER_DST,
> > > > +	DWAXIDMAC_TT_FC_PER_TO_PER_DST
> > > > +};
> > >
> > > Some of definitions are the same as for dw_dmac, right? We might
> > > split them to a common header, though I have no strong opinion
> > > about
> >
> > it.
> > > Vinod?
> >
> > APB DMAC and AXI DMAC have completely different regmap. So there is
> > no
> > much sense to do that.
>
> I'm not talking about registers, I'm talking about definitions like
> above. Though it would be indeed little sense to split some common
> code between those two.
This definitions are the fields of some registers. So they are also
different.

--
 Eugeniy Paltsev

Powered by blists - more mailing lists