On Sat, May 10, 2014 at 11:15:28AM +0200, Guennadi Liakhovetski wrote: > Hi Vinod, > > Thanks for a review. > > On Wed, 7 May 2014, Vinod Koul wrote: > > > On Sat, Apr 26, 2014 at 02:03:44PM +0200, Guennadi Liakhovetski wrote: > > > This patch adds a driver for NBPF DMAC IP cores from Renesas, designed for > > > the AMBA AXI bus. > > > > > > > > diff --git a/Documentation/devicetree/bindings/dma/nbpfaxi.txt b/Documentation/devicetree/bindings/dma/nbpfaxi.txt > > > new file mode 100644 > > > index 0000000..d5e2522 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/dma/nbpfaxi.txt > > > @@ -0,0 +1,61 @@ > > > +* Renesas "Type-AXI" NBPFAXI* DMA controllers > > > + > > > +* DMA controller > > > + > > > +Required properties > > > + > > > +- compatible: must be one of > > > + "renesas,nbpfaxi64dmac1b4" > > > + "renesas,nbpfaxi64dmac1b8" > > > + "renesas,nbpfaxi64dmac1b16" > > > + "renesas,nbpfaxi64dmac4b4" > > > + "renesas,nbpfaxi64dmac4b8" > > > + "renesas,nbpfaxi64dmac4b16" > > > + "renesas,nbpfaxi64dmac8b4" > > > + "renesas,nbpfaxi64dmac8b8" > > > + "renesas,nbpfaxi64dmac8b16" > > > +- #dma-cells: must be 2: the first integer is a terminal number, to which this > > > + slave is connected, the second one is flags. Flags is a bitmask > > > + with the following bits defined: > > > + > > > +#define NBPF_SLAVE_RQ_HIGH 1 > > > +#define NBPF_SLAVE_RQ_LOW 2 > > > +#define NBPF_SLAVE_RQ_LEVEL 4 > > > + > > > +Optional properties: > > > + > > > +You can use dma-channels and dma-requests as described in dma.txt, although they > > > +won't be used, this information is derived from the compatibility string. > > > + > > > +Example: > > > + > > > + dma: dma-controller@48000000 { > > > + compatible = "renesas,nbpfaxi64dmac8b4"; > > > + reg = <0x48000000 0x400>; > > > + interrupts = <0 12 0x4 > > > + 0 13 0x4 > > > + 0 14 0x4 > > > + 0 15 0x4 > > > + 0 16 0x4 > > > + 0 17 0x4 > > > + 0 18 0x4 > > > + 0 19 0x4>; > > > + #dma-cells = <2>; > > > + dma-channels = <8>; > > > + dma-requests = <8>; > > > + }; > > > + > > > +* DMA client > > > + > > > +Required properties: > > > + > > > +dmas and dma-names are required, as described in dma.txt. > > > + > > > +Example: > > > + > > > +#include <dt-bindings/dma/nbpfaxi.h> > > > + > > > +... > > > + dmas = <&dma 0 (NBPF_SLAVE_RQ_HIGH | NBPF_SLAVE_RQ_LEVEL) > > > + &dma 1 (NBPF_SLAVE_RQ_HIGH | NBPF_SLAVE_RQ_LEVEL)>; > > > + dma-names = "rx", "tx"; > > > > Pls split the DT bindings to seprate patch. This part needs ack from DT folks > > I thaught it would be preferable for both to go into the tree > simultaneously, i.e. as a single patch, they can review and ack this patch > too, but ok, can do, np. That helps in getting that part acked by DT folks and track independent of DMA changes. Ofocurse both will be applied togther or DT first :) > > > +static size_t nbpf_xfer_size(struct nbpf_device *nbpf, > > > + enum dma_slave_buswidth width, u32 burst) > > > +{ > > > + size_t size; > > > + > > > + if (!burst) > > > + burst = 1; > > > + > > > + switch (width) { > > > + case DMA_SLAVE_BUSWIDTH_8_BYTES: > > > + size = 8 * burst; > > > + break; > > > + case DMA_SLAVE_BUSWIDTH_4_BYTES: > > > + size = 4 * burst; > > > + break; > > > + case DMA_SLAVE_BUSWIDTH_2_BYTES: > > > + size = 2 * burst; > > why not > > size = width * burst; > > for all these three cases? > > because width is an enum, that "accidentally" coincides with the > respective numerical value. I find this confusing - if it's an enum, you > shouldn't use it in calculations. If it's a numerical value, just use it > explicitly, but mixing both... ah, i dont agree with the reasoning. We have fair examples of using enums for these calculations to covert dmaengine values to driver type. This is juts a conversion and saves code space and simplfies the routine... > > > > + break; > > > + default: > > > + pr_warn("%s(): invalid bus width %u\n", __func__, width); > > > + case DMA_SLAVE_BUSWIDTH_1_BYTE: > > > + size = burst; > > this would fit too in above :) > > > > > +static void nbpf_cookie_update(struct nbpf_channel *chan, dma_cookie_t cookie) > > > +{ > > > + dma_cookie_t forgotten = cookie - NBPF_STATUS_KEEP; > > > + > > > + if (cookie < NBPF_STATUS_KEEP && !chan->cookie_forgotten) > > > + /* Not forgetting yet */ > > > + return; > > > + > > > + if (forgotten < DMA_MIN_COOKIE) > > > + /* Wrapped */ > > > + forgotten = (DMA_MAX_COOKIE & ~NBPF_STATUS_MASK) + cookie; > > > + chan->cookie_forgotten = forgotten; > > > +} > > why dont use core handler for this? Why do you need to track forgotten? > > This has been discussed in > http://thread.gmane.org/gmane.linux.kernel/1573412, where I mentioned, > that only an extended cookie handling algorithm in respective drivers can > provide status of failed / aborted transfers. You also proposed a patch > "[PATCH] dmaengine: add error callback for reporting transaction errors" > but it has been "put on hold" too. The above is such an attempt of > extended status handling, caching several latest transactions' status and > using that cache to report back to the user. But if it's not desired, I > can remove it, np. That would be great. > > > + > > > +static bool nbpf_cookie_in_cache(struct nbpf_channel *chan, > > > + dma_cookie_t cookie, struct dma_tx_state *state) > > > +{ > > > + if (cookie > chan->cookie_forgotten) > > > + return true; > > > + > > > + /* > > > + * We still remember the cookie, if it has wrapped beyond INT_MAX: > > > + * (1) forgotten is almost INT_MAX > > > + * cookie_forgotten >> NBPF_STATUS_SHIFT == DMA_MAX_COOKIE >> NBPF_STATUS_SHIFT > > > + * and enquired is small > > > + * (2) > > > + * cookie & ~NBPF_STATUS_MASK == 0 > > Again this logic is handled so driver shoudl worry about this > > > > > + */ > > > + > > > + return chan->cookie_forgotten >> NBPF_STATUS_SHIFT == > > > + DMA_MAX_COOKIE >> NBPF_STATUS_SHIFT && > > > + !(cookie < ~NBPF_STATUS_MASK); > > > +} > > > + > > > > > +static enum dma_status nbpf_tx_status(struct dma_chan *dchan, > > > + dma_cookie_t cookie, struct dma_tx_state *state) > > > +{ > > > + struct nbpf_channel *chan = nbpf_to_chan(dchan); > > > + enum dma_status status = dma_cookie_status(dchan, cookie, state); > > > + dma_cookie_t running = chan->running ? chan->running->async_tx.cookie : -EINVAL; > > > + > > > + if (chan->paused) > > > + status = DMA_PAUSED; > > > + > > > + /* Note: we cannot return residues for old cookies */ > > ??? If cookie is completed then reside is 0. So how is this comment valid? > > For completed ones it's 0, sure. so what does comment mean here about "old" cookies? > > > > + if (state && cookie == running) { > > > + state->residue = nbpf_bytes_left(chan); > > > + dev_dbg(dchan->device->dev, "%s(): residue %u\n", __func__, > > > + state->residue); > > > + } > > > + > > > + if (status == DMA_IN_PROGRESS || status == DMA_PAUSED) > > > + return status; > > no residue here? > > I only report back residue of the currently active transfer, or do you > mean I should put the complete size as a residue for not yet started > transfers? Yes that would be the expectation as we have DMA_IN_PROGRESS for submitted as well as running ones... > > > > + > > > + if (nbpf_cookie_in_cache(chan, cookie, state)) > > > + return nbpf_cookie_cached(chan, cookie, state); > > > + > > > + return status; > > > +} > > > + > > > > > +static int nbpf_control(struct dma_chan *dchan, enum dma_ctrl_cmd cmd, > > > + unsigned long arg) > > > +{ > > > + struct nbpf_channel *chan = nbpf_to_chan(dchan); > > > + struct dma_slave_config *config; > > > + > > > + dev_dbg(dchan->device->dev, "Entry %s(%d)\n", __func__, cmd); > > > + > > > + switch (cmd) { > > > + case DMA_TERMINATE_ALL: > > > + dev_dbg(dchan->device->dev, "Terminating\n"); > > > + nbpf_chan_halt(chan); > > > + nbpf_chan_idle(chan); > > > + break; > > > + case DMA_SLAVE_CONFIG: > > > + if (!arg) > > > + return -EINVAL; > > > + config = (struct dma_slave_config *)arg; > > > + > > > + /* > > > + * We could check config->slave_id to match chan->terminal here, > > > + * but with DT they would be coming from the same source, so > > > + * such a check would be superflous > > > + */ > > > + > > > + switch (config->direction) { > > > + case DMA_MEM_TO_DEV: > > > + chan->slave_addr = config->dst_addr; > > > + chan->slave_width = nbpf_xfer_size(chan->nbpf, > > > + config->dst_addr_width, 1); > > > + chan->slave_burst = nbpf_xfer_size(chan->nbpf, > > > + config->dst_addr_width, > > > + config->dst_maxburst); > > > + break; > > > + case DMA_DEV_TO_MEM: > > > + chan->slave_addr = config->src_addr; > > > + chan->slave_width = nbpf_xfer_size(chan->nbpf, > > > + config->src_addr_width, 1); > > > + chan->slave_burst = nbpf_xfer_size(chan->nbpf, > > > + config->src_addr_width, > > > + config->src_maxburst); > > pls store all as direction makes no sense here and will be removed. > > Now based on descriptor direction you need to use one set of above. > > Hm, I find it a pity, since we don't actually need both. In this and most > other drivers, I think, only one side can be a slave device, the other > side is always RAM. So, we only need to configure addresses and burst > sizes of the slave size? That might be true for your controller but we have a fair share of controllers which are bidirectional in nature and transfer in either direction. So setting direction runtime using API makes more sense.. > > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + break; > > > + case DMA_PAUSE: > > > + chan->paused = true; > > > + nbpf_pause(chan); > > > + break; > > > + case DMA_RESUME: > > > + /* Leave unimplemented until we get a use case. */ > > why not remove? > > Ok > > > > + default: > > > + return -ENXIO; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static struct dma_async_tx_descriptor *nbpf_prep_sg(struct nbpf_channel *chan, > > > + struct scatterlist *src_sg, struct scatterlist *dst_sg, > > > + size_t len, enum dma_transfer_direction direction, > > > + unsigned long flags) > > > +{ > > > + struct nbpf_link_desc *ldesc; > > > + struct scatterlist *mem_sg; > > > + struct nbpf_desc *desc; > > > + bool inc_src, inc_dst; > > > + int i = 0; > > > + > > > + switch (direction) { > > > + case DMA_DEV_TO_MEM: > > > + mem_sg = dst_sg; > > > + inc_src = false; > > > + inc_dst = true; > > > + break; > > empty line here and similar instance pls > > Is this specified in the CodingStyle document or does checkpatch.pl > complain about such cases?... Readablity... -- ~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