Re: [PATCH 5/7] dw_dmac: autoconfigure data_width or get it via platform data

Andy Shevchenko
2012-09-20T09:42:18Z

On Tue, 2012-09-18 at 12:41 +0530, viresh kumar wrote:
> On Mon, Sep 17, 2012 at 1:09 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > @@ -647,7 +657,11 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
> > return NULL;
> > }
> >
> > - src_width = dst_width = dwc_fast_fls(src | dest | len);
> > + src_width = dst_width = min_t(unsigned int,
> > + /* For memory-to-memory transfers we
> > + * always use AHB master 1 */
>
> Sorry couldn't understand your comment. :(
> We use master 0 for src and master 1 for dst in memcpy.
Yeah, it might require to rewrite the logic a bit to depend on proper
AHB master properties.

I will fix it.

> > @@ -747,7 +764,9 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> > mem = sg_dma_address(sg);
> > len = sg_dma_len(sg);
> >
> > - mem_width = dwc_fast_fls(mem | len);
> > + mem_width = min_t(unsigned int,
> > + data_width,
> > + dwc_fast_fls(mem | len));
>
> can above three be merged into a single line or two lines?
I think so, if there no objections from checkpatch.pl side.

> > @@ -807,7 +828,9 @@ slave_sg_todev_fill_desc:
> > mem = sg_dma_address(sg);
> > len = sg_dma_len(sg);
> >
> > - mem_width = dwc_fast_fls(mem | len);
> > + mem_width = min_t(unsigned int,
> > + data_width,
> > + dwc_fast_fls(mem | len));
>
> ditto
See above.

> > @@ -1413,6 +1436,17 @@ static int __devinit dw_probe(struct platform_device *pdev)
> >
> > /* get hardware configuration parameters */
> > max_blk_size = dma_readl(dw, MAX_BLK_SIZE);
> > + if (autocfg)
> > + dw->nr_masters = (dw_params >> DW_PARAMS_NR_MASTER & 3) + 1;
> > + else
> > + dw->nr_masters = pdata->nr_masters;
> > + for (i = 0; i < dw->nr_masters; i++) {
> > + if (autocfg)
> > + dw->data_width[i] =
> > + (dw_params >> DW_PARAMS_DATA_WIDTH(i) & 3) + 2;
> > + else
> > + dw->data_width[i] = pdata->data_width[i];
> > + }
>
> There are many autocfg, if, else statements now in probe. Can you try
> to group these
> into as minimum if, else calls?
Ok, will try to optimize it.

> > --- a/drivers/dma/dw_dmac_regs.h
> > +++ b/drivers/dma/dw_dmac_regs.h
> > @@ -106,6 +106,12 @@ struct dw_dma_regs {
> >
> > /* Bitfields in DW_PARAMS */
> > #define DW_PARAMS_NR_CHAN 8 /* number of channels */
> > +#define DW_PARAMS_NR_MASTER 11 /* number of AHB masters */
> > +#define DW_PARAMS_DATA_WIDTH(n) (15 + 2 * (n))
> > +#define DW_PARAMS_DATA_WIDTH1 15 /* master 1 data width */
> > +#define DW_PARAMS_DATA_WIDTH2 17 /* master 2 data width */
> > +#define DW_PARAMS_DATA_WIDTH3 19 /* master 3 data width */
> > +#define DW_PARAMS_DATA_WIDTH4 21 /* master 4 data width */
> > #define DW_PARAMS_EN 28 /* encoded parameters */

> Do these changes in a single patch 2/7
>From this comment I guess you would like to have dw_dmac_regs.h changes
at one place. Am I right?

--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy