Re: [PATCH 1/2] dw_dmac: make driver endianness configurable

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5638 bytes --]

On Tue, Aug 28, 2012 at 02:55:35PM +0800, Hein Tibosch wrote:
> On 8/28/2012 11:23 AM, Viresh Kumar wrote:
> > On 27 August 2012 20:28, Hein Tibosch <hein_tibosch@yahoo.es> wrote:
> >>>> +config DW_DMAC_MEM_64_BIT
> >>>> +    bool "Allow 64-bit memory transfers"
> >>>> +    default y if !AVR32
> >>>> +    depends on DW_DMAC
> >>>> +    help
> >>>> +      Say yes if the DMA controller may do 64-bit memory transfers
> >>>> +      For AVR32, say no because only up to 32-bit transfers are
> >>>> +      defined
> >>> Is this sane to add? Could some non-AVR32 platforms use 64-bit and 32-bit
> >>> depending on runtime configuration? E.g. if you build a kernel with support
> >>> for multiple boards/processors, and there is a mix of 32-bit and 64-bit wide
> >>> DMA support.
> >>>
> >>> I think it is better to select 32/64-bit at runtime.
> >> I did that in the first patch, adding a new property to the dw_dma_slave
> >> structure. It had the small disadvantage that some arch code had to be
> >> adapted (at32ap700x.c).
> >>
> >> Viresh, what do you think? Add a property called "mem_64_bit_access" or so?
> >>
> >> Or should it be passed as a member of 'dw_dma_platform_data', because it
> >> is a property of the (entire) DMA controller?
> > I think second option is better. But can there be some supportive scenarios of
> > first option?
> >
> > We have a system, with two different memory controllers, one supporting 32 bit
> > and other 64 bit?
> >
> > Or what we can do now is: go with option 2, i.e. update dw_dma_platform_data
> > and if some platform like what i mentioned above comes, then we can move it
> > to slave data.
> What about this:
> 
> In case of AVR32, the arch code will indicate:
> 
> static struct dw_dma_platform_data dw_dmac0_data = {
> 	.nr_channels    = 3,
> 	/* DMAC supports up to 32-bit memory access */
> 	.mem_access_32_bit_only = true,
> };
> 
> ARM users won't have to change anything because mem_access_32_bit_only
> will be false and therefor 'dw->mem_64_bit' will become true
> 
> Signed-off-by: Hein Tibosch <hein_tibosch@yahoo.es>
> 
> ---
>  drivers/dma/dw_dmac.c      |   11 ++++++++---
>  drivers/dma/dw_dmac_regs.h |    2 ++
>  include/linux/dw_dmac.h    |    2 ++
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index 7212961..a37053c 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -618,6 +618,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
>  		size_t len, unsigned long flags)
>  {
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
> +	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
>  	struct dw_desc		*desc;
>  	struct dw_desc		*first;
>  	struct dw_desc		*prev;
> @@ -639,7 +640,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
>  	 * We can be a lot more clever here, but this should take care
>  	 * of the most common optimization.
>  	 */
> -	if (!((src | dest  | len) & 7))
> +	if (dw->mem_64_bit && !((src | dest  | len) & 7))
>  		src_width = dst_width = 3;
>  	else if (!((src | dest  | len) & 3))
>  		src_width = dst_width = 2;
> @@ -710,6 +711,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
>  	struct dw_dma_slave	*dws = chan->private;
>  	struct dma_slave_config	*sconfig = &dwc->dma_sconfig;
> +	struct dw_dma		*dw = to_dw_dma(dwc->chan.device);
>  	struct dw_desc		*prev;
>  	struct dw_desc		*first;
>  	u32			ctllo;
> @@ -746,7 +748,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  			mem = sg_dma_address(sg);
>  			len = sg_dma_len(sg);
>  
> -			if (!((mem | len) & 7))
> +			if (dw->mem_64_bit && !((mem | len) & 7))
>  				mem_width = 3;
>  			else if (!((mem | len) & 3))
>  				mem_width = 2;
> @@ -813,7 +815,7 @@ slave_sg_todev_fill_desc:
>  			mem = sg_dma_address(sg);
>  			len = sg_dma_len(sg);
>  
> -			if (!((mem | len) & 7))
> +			if (dw->mem_64_bit && !((mem | len) & 7))
>  				mem_width = 3;
>  			else if (!((mem | len) & 3))
>  				mem_width = 2;
> @@ -1419,6 +1421,9 @@ static int __init dw_probe(struct platform_device *pdev)
>  		goto err_kfree;
>  	}
>  
> +	/* Remember if 64-bit access to memory is allowed */
> +	dw->mem_64_bit = !pdata->mem_access_32_bit_only;
> +
>  	dw->regs = ioremap(io->start, DW_REGLEN);
>  	if (!dw->regs) {
>  		err = -ENOMEM;
> diff --git a/drivers/dma/dw_dmac_regs.h b/drivers/dma/dw_dmac_regs.h
> index 9758651..e24562e 100644
> --- a/drivers/dma/dw_dmac_regs.h
> +++ b/drivers/dma/dw_dmac_regs.h
> @@ -199,6 +199,8 @@ struct dw_dma {
>  	struct clk		*clk;
>  
>  	u8			all_chan_mask;
> +	/* 64-bit access to memory is allowed */
> +	bool			mem_64_bit;
>  
>  	struct dw_dma_chan	chan[0];
>  };
> diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h
> index 2412e02..d01d63f 100644
> --- a/include/linux/dw_dmac.h
> +++ b/include/linux/dw_dmac.h
> @@ -29,6 +29,8 @@ struct dw_dma_platform_data {
>  #define CHAN_PRIORITY_ASCENDING		0	/* chan0 highest */
>  #define CHAN_PRIORITY_DESCENDING	1	/* chan7 highest */
>  	unsigned char	chan_priority;
> +	/* Make true if 64-bit access to memory is not implemented */
> +	bool		mem_access_32_bit_only;
>  };

Can you not read this from any internal register ? Synopsys generally
adds a set of read only registers which we can use to guess which
features where enabled in the IP when configuring it with their IP
configuration tool.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]