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 devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
