| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120828073858.GK27166@arwen.pp.htv.fi>
Date: Tue, 28 Aug 2012 10:39:01 +0300
From: Felipe Balbi <balbi@...com>
To: Hein Tibosch <hein_tibosch@...oo.es>
Cc: Viresh Kumar <viresh.kumar@...aro.org>,
Hans-Christian Egtvedt <egtvedt@...fundet.no>,
spear-devel <spear-devel@...t.st.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"ludovic.desroches" <ludovic.desroches@...el.com>,
Havard Skinnemoen <havard@...nnemoen.net>,
Nicolas Ferre <nicolas.ferre@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Arnd Bergmann <arnd.bergmann@...aro.org>
Subject: Re: [PATCH 1/2] dw_dmac: make driver endianness configurable
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@...oo.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@...oo.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
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists