From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:32:58
From: Martin Sperl <redacted>
This patchset fixes several issues:
* missing residue_granularity to allow better support for I2S
* dma-channels are artificially restricted (channel 0 and 2)
* added correct support for dma-channels 11-14 by using the correct
shared irq line
* dma-channel 12 used the "trigger for all" irq resulting in
a lockup when used
* note that care was taken so that no change to the device-tree
was necessary to avoid a break...
With this now 11 DMA channels available instead of 7 - the
additional HW-DMA-channels are:
* 0, 2 (masking of channels)
* 13, 14 (shared interrupts - see also note on channel 12)
It also adds several new features:
* slave_sg support
* dma_memcopy support
For these a consolidated method for creating standard control-block
chains was created to share common between cyclic, slave_sg and
memcopy.
Additional support for memset, memset_sg and interleave
are in the pipeline.
Testing:
* slave_sg
* spi-bcm2835:
* fb_st7735r framebuffer
* tested using mplayer
* cyclic
* bcm2835-i2s
* Audio output with a Hifiberry I2S card.
* tested using mplayer and aplay
* memcopy
* dmatest
Tests using both slave_sg and cyclic concurrently were also conducted
using mplayer to play BigBuckBunny.
To test that interrupt line sharing was working propperly the same
audio/video test was run only using dma channel 11 to 14 - these
channels are sharing the same interrupt line -
Note that the bcm2835-i2s driver that is currenlty in the kernel
fails to work since:
commit 94cb7f76caa0b337 ("Switch to using the new clock driver support")
and requires modifications to use the clock-driver framework.
For testing a quick and dirty patch had to be used - a patch to use
the clock framework is in the pipeline.
Martin Sperl (8):
dmaengine: bcm2835: set residue_granularity field
dmaengine: bcm2835: remove unnecessary masking of dma channels
dmaengine: bcm2835: use shared interrupt for channel 11 to 14.
dmaengine: bcm2835: add additional defines for DMA-registers
dmaengine: bcm2835: move cyclic member from bcm2835_chan into
bcm2835_desc
dmaengine: bcm2835: move controlblock chain generation into separate
method
dmaengine: bcm2835: add slave_sg support to bcm2835-dma
dmaengine: bcm2835: add dma_memcopy support to bcm2835-dma
drivers/dma/bcm2835-dma.c | 541 +++++++++++++++++++++++++++++++++++----------
1 file changed, 419 insertions(+), 122 deletions(-)
--
1.7.10.4
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:32:59
From: Martin Sperl <redacted>
bcm2835-dma supports residue reporting at burst level but didn't report
this via the residue_granularity field.
See also:
https://github.com/raspberrypi/linux/commit/b015555327afa402f70ddc86e3632f59df1cd9d7
for the downstream patch.
Signed-off-by: Matthias Reichl <redacted>
Signed-off-by: Noralf Tr?nnes <redacted>
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 1 +
1 file changed, 1 insertion(+)
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:33:00
From: Martin Sperl <redacted>
The original patch contained 3 dma channels that were masked out.
These - as far as research and discussions show - are a
artefacts remaining from the downstream legacy dma-api.
Right now down-stream still includes a legacy api used only
in a single (downstream only) driver (bcm2708_fb) that requires
2D DMA for speedup (DMA-channel 0).
Formerly the sd-card support driver also was using this legacy
api (DMA-channel 2), but since has been moved over to use
dmaengine directly.
The DMA-channel 3 is already masked out in the devicetree in
the default property "brcm,dma-channel-mask = <0x7f35>;"
So we can remove the whole masking of DMA channels.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 9 ---------
1 file changed, 9 deletions(-)
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:33:01
From: Martin Sperl <redacted>
The bcm2835 dma channel 11 to 14 only have a single shared irq line,
so this patch implements shared interrupts for these channels.
To avoid changes to the device tree (with regards to interrupts listed,
which reflects on pdev->num_resources) a fixed channel count had
to get used in the code instead.
With this patch applied we now have 11 dma channels available to
the ARM side of the SOC.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 46 ++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 41 insertions(+), 5 deletions(-)
@@ -127,6 +129,19 @@ struct bcm2835_desc {#define BCM2835_DMA_CHAN(n) ((n) << 8) /* Base address */#define BCM2835_DMA_CHANIO(base, n) ((base) + BCM2835_DMA_CHAN(n))+/*+*numberofdmachannelswesupport+*wedonotsupportDMAchannel15,asitisinaseparateIOrange,+*doesnothaveaseparateIRQlineexceptforthe"catch all IRQ line"+*finallythischannelisusedbythefirmwaresoisnotavailable+*/+#define BCM2835_DMA_MAX_CHANNEL_NUMBER 14++/* the DMA channels 11 to 14 share a common interrupt */+#define BCM2835_DMA_IRQ_SHARED_MASK (BIT(11) | BIT(12) | BIT(13) | BIT(14))+#define BCM2835_DMA_IRQ_SHARED 11+#define BCM2835_DMA_IRQ_ALL 12+staticinlinestructbcm2835_dmadev*to_bcm2835_dma_dev(structdma_device*d){returncontainer_of(d,structbcm2835_dmadev,ddev);
@@ -215,6 +230,15 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)structbcm2835_desc*d;unsignedlongflags;+/* check the shared interrupt */+if(c->irq_flags&IRQF_SHARED){+/* check if the interrupt is enabled */+flags=readl(c->chan_base+BCM2835_DMA_CS);+/* if not set then we are not the reason for the irq */+if(!(flags&BCM2835_DMA_INT))+returnIRQ_NONE;+}+spin_lock_irqsave(&c->vc.lock,flags);/* Acknowledge interrupt */
@@ -526,7 +551,8 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)return0;}-staticintbcm2835_dma_chan_init(structbcm2835_dmadev*d,intchan_id,intirq)+staticintbcm2835_dma_chan_init(structbcm2835_dmadev*d,intchan_id,+intirq,unsignedlongirq_flags){structbcm2835_chan*c;
@@ -541,6 +567,7 @@ static int bcm2835_dma_chan_init(struct bcm2835_dmadev *d, int chan_id, int irq)c->chan_base=BCM2835_DMA_CHANIO(d->base,chan_id);c->ch=chan_id;c->irq_number=irq;+c->irq_flags=irq_flags;return0;}
@@ -586,6 +613,7 @@ static int bcm2835_dma_probe(struct platform_device *pdev)intrc;inti;intirq;+unsignedlongirq_flags;uint32_tchans_available;if(!pdev->dev.dma_mask)
@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev)gotoerr_no_dma;}-for(i=0;i<pdev->num_resources;i++){-irq=platform_get_irq(pdev,i);+for(i=0;i<=BCM2835_DMA_MAX_CHANNEL_NUMBER;i++){+if(BCM2835_DMA_IRQ_SHARED_MASK&BIT(i)){+irq=platform_get_irq(pdev,+BCM2835_DMA_IRQ_SHARED);+irq_flags=IRQF_SHARED;+}else{+irq=platform_get_irq(pdev,i);+irq_flags=0;+}+if(irq<0)break;if(chans_available&(1<<i)){-rc=bcm2835_dma_chan_init(od,i,irq);+rc=bcm2835_dma_chan_init(od,i,irq,irq_flags);if(rc)gotoerr_no_dma;}
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:33:02
From: Martin Sperl <redacted>
Add additional defines describing the DMA registers
as well as adding some more documentation to those registers.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 54 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 46 insertions(+), 8 deletions(-)
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:33:03
From: Martin Sperl <redacted>
In preparation to consolidating code we move the cyclic member
into the bcm_2835_desc structure.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:33:04
From: Martin Sperl <redacted>
In preparation of adding slave_sg functionality this patch moves the
generation/allocation of bcm2835_desc and the building of
the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic
into the newly created method bcm2835_dma_create_cb_chain.
This new code also takes the limits of LITE channels into account
and splits the control-block transfers at the correct location.
Tested Audio output with a Hifiberry I2S card.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 288 ++++++++++++++++++++++++++++++---------------
1 file changed, 191 insertions(+), 97 deletions(-)
@@ -181,6 +181,22 @@ struct bcm2835_desc {#define BCM2835_DMA_IRQ_SHARED 11#define BCM2835_DMA_IRQ_ALL 12+/* the max dma length for different channels */+#define MAX_DMA_LEN SZ_1G+#define MAX_LITE_DMA_LEN (SZ_64K - 4)++staticinlineboolbcm2835_dma_is_lite(structbcm2835_chan*c)+{+/* dma channels >= 7 are LITE channels */+return(c->ch>=7);+}++staticinlinesize_tbcm2835_dma_max_frame_length(structbcm2835_chan*c)+{+/* lite and normal channels have different max frame length */+returnbcm2835_dma_is_lite(c)?MAX_LITE_DMA_LEN:MAX_DMA_LEN;+}+staticinlinestructbcm2835_dmadev*to_bcm2835_dma_dev(structdma_device*d){returncontainer_of(d,structbcm2835_dmadev,ddev);
@@ -197,19 +213,140 @@ static inline struct bcm2835_desc *to_bcm2835_dma_desc(returncontainer_of(t,structbcm2835_desc,vd.tx);}-staticvoidbcm2835_dma_desc_free(structvirt_dma_desc*vd)+staticvoidbcm2835_dma_free_cb_chain(structbcm2835_desc*desc){-structbcm2835_desc*desc=container_of(vd,structbcm2835_desc,vd);-inti;+size_ti;for(i=0;i<desc->frames;i++)dma_pool_free(desc->c->cb_pool,desc->cb_list[i].cb,desc->cb_list[i].paddr);-kfree(desc->cb_list);kfree(desc);}+staticvoidbcm2835_dma_desc_free(structvirt_dma_desc*vd)+{+bcm2835_dma_free_cb_chain(+container_of(vd,structbcm2835_desc,vd));+}++staticinlinesize_tbcm2835_dma_frames_for_length(size_tlen,+size_tperiod_len)+{+returnDIV_ROUND_UP(len,period_len);+}++/**+*bcm2835_dma_create_cb_chain-createacontrolblockandfillsdatain+*+*@chan:the@dma_chanforwhichwerunthis+*@direction:thedirectioninwhichwetransfer+*@cyclic:itisacyclictransfer+*@info:thedefaultinfobitstoapplypercontrolblock+*@finalextrainfo:additionalbitsinlastcontrolblock+*(orwhenperiod_lenisreachedincaseofcyclic)+*@frames:numberofcontrolblockstoallocate+*@src:thesrcaddresstoassign(iftheS_INCbitisset+*in@info,thenitgetsincremented)+*@dst:thedstaddresstoassign(iftheD_INCbitisset+*in@info,thenitgetsincremented)+*@buf_len:thefullbufferlength(mayalsobe0)+*@period_len:theperiodlengthwhentoapply@finalextrainfo+*inadditiontothelasttransfer+*thiswillalsobreaksomecontrol-blocksearly+*@gfp:theGFPflagtouseforallocation+*/+staticstructbcm2835_desc*bcm2835_dma_create_cb_chain(+structdma_chan*chan,enumdma_transfer_directiondirection,+boolcyclic,u32info,u32finalextrainfo,size_tframes,+dma_addr_tsrc,dma_addr_tdst,size_tbuf_len,+size_tperiod_len,gfp_tgfp)+{+structbcm2835_chan*c=to_bcm2835_dma_chan(chan);+size_tlen=buf_len,total_len;+size_tframe;+structbcm2835_desc*d;+structbcm2835_cb_entry*cb_entry;+structbcm2835_dma_cb*control_block;+size_tmax_len=bcm2835_dma_max_frame_length(c);++/* allocate and setup the descriptor. */+d=kzalloc(sizeof(*d)+frames*sizeof(structbcm2835_cb_entry),+gfp);+if(!d)+returnNULL;++d->c=c;+d->dir=direction;+d->cyclic=cyclic;++/*+*Iterateoverallframes,createacontrolblock+*foreachframeandlinkthemtogether.+*/+for(frame=0,total_len=0;frame<frames;d->frames++,frame++){+cb_entry=&d->cb_list[frame];+cb_entry->cb=dma_pool_alloc(c->cb_pool,gfp,+&cb_entry->paddr);+if(!cb_entry->cb)+gotoerror_cb;++/* fill in the control block */+control_block=cb_entry->cb;+control_block->info=info;+control_block->src=src;+control_block->dst=dst;+if(buf_len){+control_block->length=min(max_len,len);+if(period_len&&+(total_len+control_block->length>=+period_len)){+/* set to end of period_len */+control_block->length=+period_len-total_len;+/* add extrainfo when cyclic */+if(cyclic)+control_block->info|=+finalextrainfo;+/* and reset total_len */+total_len=0;+}+}+control_block->stride=0;+control_block->next=0;++/* link this the last controlblock */+if(frame)+d->cb_list[frame-1].cb->next=cb_entry->paddr;++/* update src and dst and length */+if(buf_len)+len-=control_block->length;+if(src&&(info&BCM2835_DMA_S_INC))+src+=control_block->length;+if(dst&&(info&BCM2835_DMA_D_INC))+dst+=control_block->length;++/* Length of total transfer */+d->size+=control_block->length;+}++/* the last frame requires extra flags */+d->cb_list[d->frames-1].cb->info|=finalextrainfo;++/* check the size - if there is some missmatch,+*thenthisisdetectedhere+*/+if(buf_len&&(d->size!=buf_len))+gotoerror_cb;++returnd;+error_cb:+bcm2835_dma_free_cb_chain(d);++returnNULL;+}+staticintbcm2835_dma_abort(void__iomem*chan_base){unsignedlongcs;
@@ -413,117 +550,74 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(unsignedlongflags){structbcm2835_chan*c=to_bcm2835_dma_chan(chan);-enumdma_slave_buswidthdev_width;structbcm2835_desc*d;-dma_addr_tdev_addr;-unsignedintes,sync_type;-unsignedintframe;-inti;+dma_addr_tsrc,dst;+u32info=BCM2835_DMA_WAIT_RESP;+u32extra=BCM2835_DMA_INT_EN;+size_tmax_len=bcm2835_dma_max_frame_length(c);+size_tframes;/* Grab configuration */if(!is_slave_direction(direction)){-dev_err(chan->device->dev,"%s: bad direction?\n",__func__);+dev_err(chan->device->dev,+"%s: bad direction?\n",__func__);returnNULL;}-if(direction==DMA_DEV_TO_MEM){-dev_addr=c->cfg.src_addr;-dev_width=c->cfg.src_addr_width;-sync_type=BCM2835_DMA_S_DREQ;-}else{-dev_addr=c->cfg.dst_addr;-dev_width=c->cfg.dst_addr_width;-sync_type=BCM2835_DMA_D_DREQ;-}--/* Bus width translates to the element size (ES) */-switch(dev_width){-caseDMA_SLAVE_BUSWIDTH_4_BYTES:-es=BCM2835_DMA_DATA_TYPE_S32;-break;-default:+if(!buf_len){+dev_err(chan->device->dev,+"%s: bad buffer length (= 0)\n",__func__);returnNULL;}-/* Now allocate and setup the descriptor. */-d=kzalloc(sizeof(*d),GFP_NOWAIT);-if(!d)-returnNULL;+/* warn if buf_len is not a multiple of period_lenas this may leed+*tounexpectedlatenciesforinterruptsandthusaudiableclicks+*/+if(buf_len%period_len)+dev_warn(chan->device->dev,+"%s: buffer_length (%zd) is not a multiple of period_len (%zd)\n",+__func__,buf_len,period_len);-d->c=c;-d->dir=direction;-d->frames=buf_len/period_len;-d->cyclic=true;+/* Setup DREQ channel */+if(c->dreq!=0)+info|=BCM2835_DMA_PER_MAP(c->dreq);-d->cb_list=kcalloc(d->frames,sizeof(*d->cb_list),GFP_KERNEL);-if(!d->cb_list){-kfree(d);-returnNULL;+if(direction==DMA_DEV_TO_MEM){+if(c->cfg.src_addr_width!=DMA_SLAVE_BUSWIDTH_4_BYTES)+returnNULL;+src=c->cfg.src_addr;+dst=buf_addr;+info|=BCM2835_DMA_S_DREQ|BCM2835_DMA_D_INC;+}else{+if(c->cfg.dst_addr_width!=DMA_SLAVE_BUSWIDTH_4_BYTES)+returnNULL;+dst=c->cfg.dst_addr;+src=buf_addr;+info|=BCM2835_DMA_D_DREQ|BCM2835_DMA_S_INC;}-/* Allocate memory for control blocks */-for(i=0;i<d->frames;i++){-structbcm2835_cb_entry*cb_entry=&d->cb_list[i];-cb_entry->cb=dma_pool_zalloc(c->cb_pool,GFP_ATOMIC,-&cb_entry->paddr);-if(!cb_entry->cb)-gotoerror_cb;-}+/* calculate number of frames */+frames=/* number of periods */+DIV_ROUND_UP(buf_len,period_len)*+/* number of frames per period */+bcm2835_dma_frames_for_length(period_len,max_len);/*-*Iterateoverallframes,createacontrolblock-*foreachframeandlinkthemtogether.+*allocatetheCBchain+*notethatweneedtouseGFP_ATOMIC,astheALSAi2sdmaengine+*implementationcallsprep_dma_cyclicwithinterruptsdisabled.*/-for(frame=0;frame<d->frames;frame++){-structbcm2835_dma_cb*control_block=d->cb_list[frame].cb;--/* Setup adresses */-if(d->dir==DMA_DEV_TO_MEM){-control_block->info=BCM2835_DMA_D_INC;-control_block->src=dev_addr;-control_block->dst=buf_addr+frame*period_len;-}else{-control_block->info=BCM2835_DMA_S_INC;-control_block->src=buf_addr+frame*period_len;-control_block->dst=dev_addr;-}--/* Enable interrupt */-control_block->info|=BCM2835_DMA_INT_EN;--/* Setup synchronization */-if(sync_type!=0)-control_block->info|=sync_type;--/* Setup DREQ channel */-if(c->dreq!=0)-control_block->info|=-BCM2835_DMA_PER_MAP(c->dreq);--/* Length of a frame */-control_block->length=period_len;-d->size+=control_block->length;+d=bcm2835_dma_create_cb_chain(chan,direction,true,+info,extra,+frames,src,dst,buf_len,+period_len,GFP_ATOMIC);+if(!d)+returnNULL;-/*-*Nextblockisthenextframe.-*ThisDMAenginedrivercurrentlyonlysupportscyclicDMA.-*Therefore,wraparoundatnumberofframes.-*/-control_block->next=d->cb_list[((frame+1)%d->frames)].paddr;-}+/* wrap around into a loop */+d->cb_list[d->frames-1].cb->next=d->cb_list[0].paddr;returnvchan_tx_prep(&c->vc,&d->vd,flags);-error_cb:-i--;-for(;i>=0;i--){-structbcm2835_cb_entry*cb_entry=&d->cb_list[i];--dma_pool_free(c->cb_pool,cb_entry->cb,cb_entry->paddr);-}--kfree(d->cb_list);-kfree(d);-returnNULL;}staticintbcm2835_dma_slave_config(structdma_chan*chan,--
From: kernel at martin.sperl.org <hidden> Date: 2016-01-07 17:33:05
From: Martin Sperl <redacted>
Add slave_sg support to bcm2835-dma using shared allocation
code for bcm2835_desc and DMA-control blocks already used by
dma_cyclic.
Note that bcm2835_dma_callback had to get modified to support
both modes of operation (cyclic and non-cyclic).
Tested using:
* Hifiberry I2S card (using cyclic DMA)
* fb_st7735r SPI-framebuffer (using slave_sg DMA via spi-bcm2835)
playing BigBuckBunny for audio and video.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 113 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 108 insertions(+), 5 deletions(-)
@@ -423,12 +466,18 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)d=c->desc;if(d){-/* TODO Only works for cyclic DMA */-vchan_cyclic_callback(&d->vd);-}+if(d->cyclic){+/* call the cyclic callback */+vchan_cyclic_callback(&d->vd);-/* Keep the DMA engine running */-writel(BCM2835_DMA_ACTIVE,c->chan_base+BCM2835_DMA_CS);+/* Keep the DMA engine running */+writel(BCM2835_DMA_ACTIVE,+c->chan_base+BCM2835_DMA_CS);+}else{+vchan_cookie_complete(&c->desc->vd);+bcm2835_dma_start_desc(c);+}+}spin_unlock_irqrestore(&c->vc.lock,flags);
@@ -544,6 +593,58 @@ static void bcm2835_dma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore(&c->vc.lock,flags);}+staticstructdma_async_tx_descriptor*bcm2835_dma_prep_slave_sg(+structdma_chan*chan,+structscatterlist*sgl,unsignedintsg_len,+enumdma_transfer_directiondirection,+unsignedlongflags,void*context)+{+structbcm2835_chan*c=to_bcm2835_dma_chan(chan);+structbcm2835_desc*d;+dma_addr_tsrc=0,dst=0;+u32info=BCM2835_DMA_WAIT_RESP;+u32extra=BCM2835_DMA_INT_EN;+size_tframes;++if(!is_slave_direction(direction)){+dev_err(chan->device->dev,+"%s: bad direction?\n",__func__);+returnNULL;+}++if(c->dreq!=0)+info|=BCM2835_DMA_PER_MAP(c->dreq);++if(direction==DMA_DEV_TO_MEM){+if(c->cfg.src_addr_width!=DMA_SLAVE_BUSWIDTH_4_BYTES)+returnNULL;+src=c->cfg.src_addr;+info|=BCM2835_DMA_S_DREQ|BCM2835_DMA_D_INC;+}else{+if(c->cfg.dst_addr_width!=DMA_SLAVE_BUSWIDTH_4_BYTES)+returnNULL;+dst=c->cfg.dst_addr;+info|=BCM2835_DMA_D_DREQ|BCM2835_DMA_S_INC;+}++/* count frames in sg list */+frames=bcm2835_dma_count_frames_for_sg(c,sgl,sg_len);++/* allocate the CB chain */+d=bcm2835_dma_create_cb_chain(chan,direction,false,+info,extra,+frames,src,dst,0,0,+GFP_KERNEL);+if(!d)+returnNULL;++/* fill in frames with scatterlist pointers */+bcm2835_dma_fill_cb_chain_with_sg(chan,direction,d->cb_list,+sgl,sg_len);++returnvchan_tx_prep(&c->vc,&d->vd,flags);+}+staticstructdma_async_tx_descriptor*bcm2835_dma_prep_dma_cyclic(structdma_chan*chan,dma_addr_tbuf_addr,size_tbuf_len,size_tperiod_len,enumdma_transfer_directiondirection,
@@ -772,11 +873,13 @@ static int bcm2835_dma_probe(struct platform_device *pdev)dma_cap_set(DMA_SLAVE,od->ddev.cap_mask);dma_cap_set(DMA_PRIVATE,od->ddev.cap_mask);dma_cap_set(DMA_CYCLIC,od->ddev.cap_mask);+dma_cap_set(DMA_SLAVE,od->ddev.cap_mask);od->ddev.device_alloc_chan_resources=bcm2835_dma_alloc_chan_resources;od->ddev.device_free_chan_resources=bcm2835_dma_free_chan_resources;od->ddev.device_tx_status=bcm2835_dma_tx_status;od->ddev.device_issue_pending=bcm2835_dma_issue_pending;od->ddev.device_prep_dma_cyclic=bcm2835_dma_prep_dma_cyclic;+od->ddev.device_prep_slave_sg=bcm2835_dma_prep_slave_sg;od->ddev.device_config=bcm2835_dma_slave_config;od->ddev.device_terminate_all=bcm2835_dma_terminate_all;od->ddev.src_addr_widths=BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
@@ -287,6 +287,9 @@ static struct bcm2835_desc *bcm2835_dma_create_cb_chain(structbcm2835_dma_cb*control_block;size_tmax_len=bcm2835_dma_max_frame_length(c);+if(!frames)+returnNULL;+/* allocate and setup the descriptor. */d=kzalloc(sizeof(*d)+frames*sizeof(structbcm2835_cb_entry),gfp);
@@ -593,6 +596,34 @@ static void bcm2835_dma_issue_pending(struct dma_chan *chan)spin_unlock_irqrestore(&c->vc.lock,flags);}+structdma_async_tx_descriptor*bcm2835_dma_prep_dma_memcpy(+structdma_chan*chan,dma_addr_tdst,dma_addr_tsrc,+size_tlen,unsignedlongflags)+{+structbcm2835_chan*c=to_bcm2835_dma_chan(chan);+structbcm2835_desc*d;+u32info=BCM2835_DMA_D_INC|BCM2835_DMA_S_INC;+u32extra=BCM2835_DMA_INT_EN|BCM2835_DMA_WAIT_RESP;+size_tmax_len=bcm2835_dma_max_frame_length(c);+size_tframes;++/* if src, dst or len is not given return with an error */+if(!src||!dst||!len)+returnNULL;++/* calculate number of frames */+frames=bcm2835_dma_frames_for_length(len,max_len);++/* allocate the CB chain - this also fills in the pointers */+d=bcm2835_dma_create_cb_chain(chan,DMA_MEM_TO_MEM,false,+info,extra,frames,+src,dst,len,0,GFP_KERNEL);+if(!d)+returnNULL;++returnvchan_tx_prep(&c->vc,&d->vd,flags);+}+staticstructdma_async_tx_descriptor*bcm2835_dma_prep_slave_sg(structdma_chan*chan,structscatterlist*sgl,unsignedintsg_len,
@@ -874,17 +905,20 @@ static int bcm2835_dma_probe(struct platform_device *pdev)dma_cap_set(DMA_PRIVATE,od->ddev.cap_mask);dma_cap_set(DMA_CYCLIC,od->ddev.cap_mask);dma_cap_set(DMA_SLAVE,od->ddev.cap_mask);+dma_cap_set(DMA_MEMCPY,od->ddev.cap_mask);od->ddev.device_alloc_chan_resources=bcm2835_dma_alloc_chan_resources;od->ddev.device_free_chan_resources=bcm2835_dma_free_chan_resources;od->ddev.device_tx_status=bcm2835_dma_tx_status;od->ddev.device_issue_pending=bcm2835_dma_issue_pending;od->ddev.device_prep_dma_cyclic=bcm2835_dma_prep_dma_cyclic;od->ddev.device_prep_slave_sg=bcm2835_dma_prep_slave_sg;+od->ddev.device_prep_dma_memcpy=bcm2835_dma_prep_dma_memcpy;od->ddev.device_config=bcm2835_dma_slave_config;od->ddev.device_terminate_all=bcm2835_dma_terminate_all;od->ddev.src_addr_widths=BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);od->ddev.dst_addr_widths=BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);-od->ddev.directions=BIT(DMA_DEV_TO_MEM)|BIT(DMA_MEM_TO_DEV);+od->ddev.directions=BIT(DMA_DEV_TO_MEM)|BIT(DMA_MEM_TO_DEV)|+BIT(DMA_MEM_TO_MEM);od->ddev.residue_granularity=DMA_RESIDUE_GRANULARITY_BURST;od->ddev.dev=&pdev->dev;INIT_LIST_HEAD(&od->ddev.channels);
On Thu, Jan 07, 2016 at 05:33:01PM +0000, kernel at martin.sperl.org wrote:
quoted hunk
@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev) goto err_no_dma; }- for (i = 0; i < pdev->num_resources; i++) {- irq = platform_get_irq(pdev, i);+ for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {+ if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {
Ideally this should be done thru DT data and not hard coded in kernel. I
dont think this assumption will hold good for next gen of this device, so
better to get this from DT!
--
~Vinod
On Thu, Jan 07, 2016 at 05:33:04PM +0000, kernel at martin.sperl.org wrote:
quoted hunk
+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)+{+ /* dma channels >= 7 are LITE channels */+ return (c->ch >= 7);
Why not DT data here as well
quoted hunk
+}++static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)+{+ /* lite and normal channels have different max frame length */+ return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;
+ /* fill in the control block */+ control_block = cb_entry->cb;+ control_block->info = info;+ control_block->src = src;+ control_block->dst = dst;+ if (buf_len) {+ control_block->length = min(max_len, len);+ if (period_len &&+ (total_len + control_block->length >=+ period_len)) {+ /* set to end of period_len */+ control_block->length =+ period_len - total_len;+ /* add extrainfo when cyclic */+ if (cyclic)+ control_block->info |=+ finalextrainfo;+ /* and reset total_len */+ total_len = 0;+ }
this looks hard to read, perhpas a helper will make it look better
quoted hunk
+ /* the last frame requires extra flags */+ d->cb_list[d->frames - 1].cb->info |= finalextrainfo;++ /* check the size - if there is some missmatch,+ * then this is detected here+ */
this is not kernel style for multi-line comments
quoted hunk
/* Grab configuration */ if (!is_slave_direction(direction)) {- dev_err(chan->device->dev, "%s: bad direction?\n", __func__);+ dev_err(chan->device->dev,+ "%s: bad direction?\n", __func__);
unrelated change
quoted hunk
- /* Now allocate and setup the descriptor. */- d = kzalloc(sizeof(*d), GFP_NOWAIT);- if (!d)- return NULL;+ /* warn if buf_len is not a multiple of period_lenas this may leed+ * to unexpected latencies for interrupts and thus audiable clicks+ */
here too
quoted hunk
/*- * Iterate over all frames, create a control block- * for each frame and link them together.+ * allocate the CB chain+ * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine
dmaengine drivers use GFP_NOWAIT in these cases
--
~Vinod
From: Martin Sperl <hidden> Date: 2016-01-13 13:30:32
On 13.01.2016 13:26, Vinod Koul wrote:
On Thu, Jan 07, 2016 at 05:33:01PM +0000, kernel at martin.sperl.org wrote:
quoted
@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev) goto err_no_dma; }- for (i = 0; i < pdev->num_resources; i++) {- irq = platform_get_irq(pdev, i);+ for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {+ if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {
Ideally this should be done thru DT data and not hard coded in kernel. I
dont think this assumption will hold good for next gen of this device, so
better to get this from DT!
The ideal solution would be breaking the DT in such a way that we could
define a register range and interrupt per dma-channel looking something
like this:
@@ -31,8 +31,28 @@dma:dmaat7e007000{compatible="brcm,bcm2835-dma";-reg=<0x7e0070000xf00>;-interrupts=<116>,+reg=<0x7e007f000x100>,/* status reg */+/* dma channel 0-14 base addresses */+<0x7e0070000x100>,+<0x7e0071000x100>,+<0x7e0072000x100>,+<0x7e0073000x100>,+<0x7e0074000x100>,+<0x7e0075000x100>,+<0x7e0076000x100>,+<0x7e0077000x100>,+<0x7e0078000x100>,+<0x7e0079000x100>,+<0x7e007a000x100>,+<0x7e007b000x100>,+<0x7e007c000x100>,+<0x7e007d000x100>,+<0x7e007e000x100>,+/* dma channel 15 uses a different base */+<0x7ee050000x100>;+interrupts=<128>,/* catch all DMA-interrupts */+/* dma channel 0-10 interrupts */+<116>,<117>,<118>,<119>,
@@ -43,9 +63,30 @@<124>,<125>,<126>,+/* dma channel 11-14 share irq */<127>,-<128>;-+<127>,+<127>,+<127>,+/* no irq support for dma channel 15 */+<0>;+dma-names="shared",+"dma0",+"dma1",+"dma2",+"dma3",+"dma4",+"dma5",+"dma6",+"dma7",+"dma8",+"dma9",+"dma10",+"dma11",+"dma12",+"dma13",+"dma14",+"dma15";#dma-cells=<1>;brcm,dma-channel-mask=<0x7f35>;
(or similar)
This actually would allow us to make "brcm,dma-channel-mask" redundant,
as we could remove those dma channels that are owned by the firmware
directly from the list.
That way we could also map other capabilities via the DT.
It would also allow a transparent addition of additional dma channels
with newer versions of the HW - mostly - by modifying the DT.
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
It is not optimal, but at least it works with the least amount of
change to the DT - and what about all those assumptions that we
would need to hard-code to be backwards compatible to the DT without?
I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */
#define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
i++) {
So which way would you prefer this to go - I got another few days
before I leave on vacation.
Thanks,
Martin
From: Martin Sperl <hidden> Date: 2016-01-13 13:38:40
On 13.01.2016 14:23, Vinod Koul wrote:
On Thu, Jan 07, 2016 at 05:33:04PM +0000, kernel at martin.sperl.org wrote:
quoted
+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)+{+ /* dma channels >= 7 are LITE channels */+ return (c->ch >= 7);
Why not DT data here as well
See other email with comments on DT.
quoted
+}++static inline size_t bcm2835_dma_max_frame_length(struct bcm2835_chan *c)+{+ /* lite and normal channels have different max frame length */+ return bcm2835_dma_is_lite(c) ? MAX_LITE_DMA_LEN : MAX_DMA_LEN;
Or rather get length from DT..
See other email with comments on DT.
quoted
+ /* allocate and setup the descriptor. */+ d = kzalloc(sizeof(*d) + frames * sizeof(struct bcm2835_cb_entry),+ gfp);
odd style.. btw should flag be GFP_NOWAIT ..?
gfp was used as a method-argument - see your comment below.
quoted
+ /* fill in the control block */+ control_block = cb_entry->cb;+ control_block->info = info;+ control_block->src = src;+ control_block->dst = dst;+ if (buf_len) {+ control_block->length = min(max_len, len);+ if (period_len &&+ (total_len + control_block->length >=+ period_len)) {+ /* set to end of period_len */+ control_block->length =+ period_len - total_len;+ /* add extrainfo when cyclic */+ if (cyclic)+ control_block->info |=+ finalextrainfo;+ /* and reset total_len */+ total_len = 0;+ }
this looks hard to read, perhpas a helper will make it look better
Perhaps - I can look...
quoted
+ /* the last frame requires extra flags */+ d->cb_list[d->frames - 1].cb->info |= finalextrainfo;++ /* check the size - if there is some missmatch,+ * then this is detected here+ */
this is not kernel style for multi-line comments
I know - this came up with a different patch-set
quoted
/* Grab configuration */ if (!is_slave_direction(direction)) {- dev_err(chan->device->dev, "%s: bad direction?\n", __func__);+ dev_err(chan->device->dev,+ "%s: bad direction?\n", __func__);
unrelated change
OK
quoted
- /* Now allocate and setup the descriptor. */- d = kzalloc(sizeof(*d), GFP_NOWAIT);- if (!d)- return NULL;+ /* warn if buf_len is not a multiple of period_lenas this may leed+ * to unexpected latencies for interrupts and thus audiable clicks+ */
here too
as this code is new and there are a few more issues that have been seen
it was included here...
quoted
/*- * Iterate over all frames, create a control block- * for each frame and link them together.+ * allocate the CB chain+ * note that we need to use GFP_ATOMIC, as the ALSA i2s dmaengine
dmaengine drivers use GFP_NOWAIT in these cases
OK we were not sure if this was "permissible" or would result in rare
issues...
I will wait on the generic DT discussion before sending any updates
incorporating both...
On Wed, Jan 13, 2016 at 02:30:32PM +0100, Martin Sperl wrote:
quoted hunk
On 13.01.2016 13:26, Vinod Koul wrote:
quoted
On Thu, Jan 07, 2016 at 05:33:01PM +0000, kernel at martin.sperl.org wrote:
quoted
@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev) goto err_no_dma; }- for (i = 0; i < pdev->num_resources; i++) {- irq = platform_get_irq(pdev, i);+ for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {+ if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {
Ideally this should be done thru DT data and not hard coded in kernel. I
dont think this assumption will hold good for next gen of this device, so
better to get this from DT!
The ideal solution would be breaking the DT in such a way that we could
define a register range and interrupt per dma-channel looking something
like this:
@@ -31,8 +31,28 @@dma:dmaat7e007000{compatible="brcm,bcm2835-dma";-reg=<0x7e0070000xf00>;-interrupts=<116>,+reg=<0x7e007f000x100>,/* status reg */+<0x7e0070000x100>,+<0x7e0071000x100>,+<0x7e0072000x100>,+<0x7e0073000x100>,+<0x7e0074000x100>,+<0x7e0075000x100>,+<0x7e0076000x100>,+<0x7e0077000x100>,+<0x7e0078000x100>,+<0x7e0079000x100>,+<0x7e007a000x100>,+<0x7e007b000x100>,+<0x7e007c000x100>,+<0x7e007d000x100>,+<0x7e007e000x100>,+/* dma channel 15 uses a different base */+<0x7ee050000x100>;+interrupts=<128>,/* catch all DMA-interrupts */+/* dma channel 0-10 interrupts */+<116>,<117>,<118>,<119>,
@@ -43,9 +63,30 @@<124>,<125>,<126>,+/* dma channel 11-14 share irq */<127>,-<128>;-+<127>,+<127>,+<127>,+/* no irq support for dma channel 15 */+<0>;+dma-names="shared",+"dma0",+"dma1",+"dma2",+"dma3",+"dma4",+"dma5",+"dma6",+"dma7",+"dma8",+"dma9",+"dma10",+"dma11",+"dma12",+"dma13",+"dma14",+"dma15";#dma-cells=<1>;brcm,dma-channel-mask=<0x7f35>;
(or similar)
This actually would allow us to make "brcm,dma-channel-mask" redundant,
as we could remove those dma channels that are owned by the firmware
directly from the list.
That way we could also map other capabilities via the DT.
It would also allow a transparent addition of additional dma channels
with newer versions of the HW - mostly - by modifying the DT.
Precisely
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
It is not optimal, but at least it works with the least amount of
change to the DT - and what about all those assumptions that we
would need to hard-code to be backwards compatible to the DT without?
I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */
#define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
i++) {
So which way would you prefer this to go - I got another few days
before I leave on vacation.
I still think DT is the right way to go here, unless I hear some other
convincing answer..
--
~Vinod
From: Martin Sperl <hidden> Date: 2016-01-13 14:24:59
On 13.01.2016 14:43, Vinod Koul wrote:
On Wed, Jan 13, 2016 at 02:30:32PM +0100, Martin Sperl wrote:
quoted
On 13.01.2016 13:26, Vinod Koul wrote:
quoted
On Thu, Jan 07, 2016 at 05:33:01PM +0000, kernel at martin.sperl.org wrote:
quoted
@@ -638,13 +666,21 @@ static int bcm2835_dma_probe(struct platform_device *pdev) goto err_no_dma; }- for (i = 0; i < pdev->num_resources; i++) {- irq = platform_get_irq(pdev, i);+ for (i = 0; i <= BCM2835_DMA_MAX_CHANNEL_NUMBER; i++) {+ if (BCM2835_DMA_IRQ_SHARED_MASK & BIT(i)) {
Ideally this should be done thru DT data and not hard coded in kernel. I
dont think this assumption will hold good for next gen of this device, so
better to get this from DT!
The ideal solution would be breaking the DT in such a way that we could
define a register range and interrupt per dma-channel looking something
like this:
@@ -31,8 +31,28 @@dma:dmaat7e007000{compatible="brcm,bcm2835-dma";-reg=<0x7e0070000xf00>;-interrupts=<116>,+reg=<0x7e007f000x100>,/* status reg */+<0x7e0070000x100>,+<0x7e0071000x100>,+<0x7e0072000x100>,+<0x7e0073000x100>,+<0x7e0074000x100>,+<0x7e0075000x100>,+<0x7e0076000x100>,+<0x7e0077000x100>,+<0x7e0078000x100>,+<0x7e0079000x100>,+<0x7e007a000x100>,+<0x7e007b000x100>,+<0x7e007c000x100>,+<0x7e007d000x100>,+<0x7e007e000x100>,+/* dma channel 15 uses a different base */+<0x7ee050000x100>;+interrupts=<128>,/* catch all DMA-interrupts */+/* dma channel 0-10 interrupts */+<116>,<117>,<118>,<119>,
@@ -43,9 +63,30 @@<124>,<125>,<126>,+/* dma channel 11-14 share irq */<127>,-<128>;-+<127>,+<127>,+<127>,+/* no irq support for dma channel 15 */+<0>;+dma-names="shared",+"dma0",+"dma1",+"dma2",+"dma3",+"dma4",+"dma5",+"dma6",+"dma7",+"dma8",+"dma9",+"dma10",+"dma11",+"dma12",+"dma13",+"dma14",+"dma15";#dma-cells=<1>;brcm,dma-channel-mask=<0x7f35>;
(or similar)
This actually would allow us to make "brcm,dma-channel-mask" redundant,
as we could remove those dma channels that are owned by the firmware
directly from the list.
That way we could also map other capabilities via the DT.
It would also allow a transparent addition of additional dma channels
with newer versions of the HW - mostly - by modifying the DT.
Precisely
quoted
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?
We still need to be backwards-compatible - at least that is what
everyone tells me, so I need to hard-code fallbacks for those values.
quoted
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
that could be a DT property, yes.
quoted
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
OK - how would you define that "mapping" in a "sane" manner in the DT
that allows us to have multiple such mappings in the future?
quoted
It is not optimal, but at least it works with the least amount of
change to the DT - and what about all those assumptions that we
would need to hard-code to be backwards compatible to the DT without?
I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */
#define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
i++) {
So which way would you prefer this to go - I got another few days
before I leave on vacation.
I still think DT is the right way to go here, unless I hear some other
convincing answer..
The point is that the way the DT is right now it becomes very hard to
extend it in a sane manner - It would be bitmaps/lists here,
bitmaps/lists there...
Breaking the DT would make all of it go away.
If I think of it (reading your other comments), then here how the
"new" DT could look like:
dma: dma at 7e007f00 {
/* new compatible name to map to new driver */
compatible = "brcm,bcm2835-dma-v2";
/* the status/enable registers */
reg = <0x7e007f00 0x100>;
/* the catch all interrupt */
interrupts = <1 28>;
dma0: dma-channel at 0x7e007000 {
reg = <0x7e007000 0x100>;
interrupts = <1 16>;
};
...
dma7: dma-channel at 0x7e007700 {
reg = <0x7e007700 0x100>;
interrupts = <1 23>;
dma-channel-type = <BCM2835_DMA_LITE>;
dma-max-length = <65532>; /* 64K - 4 */
};
...
dma11: dma-channel at 0x7e007b00 {
reg = <0x7e007b00 0x100>;
interrupts = <1 27>;
shared-interrupt;
dma-channel-type = <BCM2835_DMA_LITE>;
};
...
dma14: dma-channel at 0x7e007e00 {
reg = <0x7e007e00 0x100>;
interrupts = <1 27>;
shared-interrupt;
dma-channel-type = <BCM2835_DMA_LITE>;
};
dma15: dma-channel at 0x7ee05000 {
reg = <0x7ee05000 0x100>;
};
};
Adding additional "features" would make it easy like:
* dma-channel priority on AXI bus
* setting warn/alert levels for DREQs
* eventually mapping of drivers to the explicit dma channel
e.g: SPI has a limit of 65534 bytes per dma transfer in HW,
so the use of a DMA-LITE engine would be sufficient - no need
to waste a "normal" DMA-engine with 2D support,...
So the spi dt node could then look like this:
dmas = <&dma11 BCM2835_DREQ_SPI_TX>, <&dma12 BCM2835_DREQ_SPI_RX>;
dma-names = "tx", "rx";
Is this the approach I should try to take?
Or do you want me to continue on the "patchwork" route:
dma: dma at 7e007000 {
interrupt-names = "dma0", ..., "dma10", "shared", "catch-all";
bcrm,dma-lite-channel-mask = <0x7f80>;
bcrm,dma-max-channels = 14;
bcrm,catch-all-interrupt-name = "catch-all";
bcrm,shared-interrupt-name = "shared";
bcrm,shared-interrupt-channel-mapping = <0x7800>;
}
For all of the above we would need to define defaults in the driver
to make it work in a backwards compatible way.
That would not support the registers for each dma channel and not lend
itself towards extending only via the DT - say support DMA channel 15.
I have another 5 days before I leave on vacation - there is only so
much I can do...
Thanks,
Martin
On Wed, Jan 13, 2016 at 03:24:59PM +0100, Martin Sperl wrote:
quoted
quoted
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?
We still need to be backwards-compatible - at least that is what
everyone tells me, so I need to hard-code fallbacks for those values.
IMO hard code for falling back is okay as that supports old cases and new
platforms use geric DT info and new devices can be supported generically,
please check with DT folks..
quoted
quoted
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
that could be a DT property, yes.
quoted
quoted
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
OK - how would you define that "mapping" in a "sane" manner in the DT
that allows us to have multiple such mappings in the future?
You are hard coding the flags for each channel, we can pass this for each
channel in the interrupt configi, a flag share/none..? Please run this thru
DT experts and I am not one of them..
quoted
quoted
It is not optimal, but at least it works with the least amount of
change to the DT - and what about all those assumptions that we
would need to hard-code to be backwards compatible to the DT without?
I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */
#define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
i++) {
So which way would you prefer this to go - I got another few days
before I leave on vacation.
I still think DT is the right way to go here, unless I hear some other
convincing answer..
The point is that the way the DT is right now it becomes very hard to
extend it in a sane manner - It would be bitmaps/lists here,
bitmaps/lists there...
Breaking the DT would make all of it go away.
If I think of it (reading your other comments), then here how the
"new" DT could look like:
dma: dma at 7e007f00 {
/* new compatible name to map to new driver */
compatible = "brcm,bcm2835-dma-v2";
/* the status/enable registers */
reg = <0x7e007f00 0x100>;
/* the catch all interrupt */
interrupts = <1 28>;
dma0: dma-channel at 0x7e007000 {
reg = <0x7e007000 0x100>;
interrupts = <1 16>;
};
...
dma7: dma-channel at 0x7e007700 {
reg = <0x7e007700 0x100>;
interrupts = <1 23>;
dma-channel-type = <BCM2835_DMA_LITE>;
dma-max-length = <65532>; /* 64K - 4 */
};
...
dma11: dma-channel at 0x7e007b00 {
reg = <0x7e007b00 0x100>;
interrupts = <1 27>;
shared-interrupt;
dma-channel-type = <BCM2835_DMA_LITE>;
};
...
dma14: dma-channel at 0x7e007e00 {
reg = <0x7e007e00 0x100>;
interrupts = <1 27>;
shared-interrupt;
dma-channel-type = <BCM2835_DMA_LITE>;
};
dma15: dma-channel at 0x7ee05000 {
reg = <0x7ee05000 0x100>;
};
};
This looks okay to me to start with...
Adding additional "features" would make it easy like:
* dma-channel priority on AXI bus
* setting warn/alert levels for DREQs
* eventually mapping of drivers to the explicit dma channel
e.g: SPI has a limit of 65534 bytes per dma transfer in HW,
so the use of a DMA-LITE engine would be sufficient - no need
to waste a "normal" DMA-engine with 2D support,...
So the spi dt node could then look like this:
dmas = <&dma11 BCM2835_DREQ_SPI_TX>, <&dma12 BCM2835_DREQ_SPI_RX>;
dma-names = "tx", "rx";
Is this the approach I should try to take?
Or do you want me to continue on the "patchwork" route:
dma: dma at 7e007000 {
interrupt-names = "dma0", ..., "dma10", "shared", "catch-all";
bcrm,dma-lite-channel-mask = <0x7f80>;
bcrm,dma-max-channels = 14;
bcrm,catch-all-interrupt-name = "catch-all";
bcrm,shared-interrupt-name = "shared";
bcrm,shared-interrupt-channel-mapping = <0x7800>;
}
For all of the above we would need to define defaults in the driver
to make it work in a backwards compatible way.
That would not support the registers for each dma channel and not lend
itself towards extending only via the DT - say support DMA channel 15.
I have another 5 days before I leave on vacation - there is only so
much I can do...
Thanks,
Martin
From: Martin Sperl <hidden> Date: 2016-01-14 08:48:56
On 14.01.2016 05:07, Vinod Koul wrote:
On Wed, Jan 13, 2016 at 03:24:59PM +0100, Martin Sperl wrote:
quoted
quoted
quoted
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?
We still need to be backwards-compatible - at least that is what
everyone tells me, so I need to hard-code fallbacks for those values.
IMO hard code for falling back is okay as that supports old cases and new
platforms use geric DT info and new devices can be supported generically,
please check with DT folks..
quoted
quoted
quoted
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
that could be a DT property, yes.
quoted
quoted
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
OK - how would you define that "mapping" in a "sane" manner in the DT
that allows us to have multiple such mappings in the future?
You are hard coding the flags for each channel, we can pass this for each
channel in the interrupt configi, a flag share/none..? Please run this thru
DT experts and I am not one of them..
quoted
quoted
quoted
It is not optimal, but at least it works with the least amount of
change to the DT - and what about all those assumptions that we
would need to hard-code to be backwards compatible to the DT without?
I guess we could replace BCM2835_DMA_MAX_CHANNEL_NUMBER with:
/* we do not support dma channel 15 with this driver */
#define BCM2835_DMA_MAX_CHANNEL_SUPPORTED 14
...
for (i = 0;
i <= min_t(int, flv(chans_available),
BCM2835_DMA_MAX_CHANNEL_SUPPORTED);
i++) {
So which way would you prefer this to go - I got another few days
before I leave on vacation.
I still think DT is the right way to go here, unless I hear some other
convincing answer..
The point is that the way the DT is right now it becomes very hard to
extend it in a sane manner - It would be bitmaps/lists here,
bitmaps/lists there...
Breaking the DT would make all of it go away.
If I think of it (reading your other comments), then here how the
"new" DT could look like:
dma: dma at 7e007f00 {
/* new compatible name to map to new driver */
compatible = "brcm,bcm2835-dma-v2";
/* the status/enable registers */
reg = <0x7e007f00 0x100>;
/* the catch all interrupt */
interrupts = <1 28>;
dma0: dma-channel at 0x7e007000 {
reg = <0x7e007000 0x100>;
interrupts = <1 16>;
};
...
dma7: dma-channel at 0x7e007700 {
reg = <0x7e007700 0x100>;
interrupts = <1 23>;
dma-channel-type = <BCM2835_DMA_LITE>;
dma-max-length = <65532>; /* 64K - 4 */
};
...
dma11: dma-channel at 0x7e007b00 {
reg = <0x7e007b00 0x100>;
interrupts = <1 27>;
shared-interrupt;
dma-channel-type = <BCM2835_DMA_LITE>;
};
...
dma14: dma-channel at 0x7e007e00 {
reg = <0x7e007e00 0x100>;
interrupts = <1 27>;
shared-interrupt;
dma-channel-type = <BCM2835_DMA_LITE>;
};
dma15: dma-channel at 0x7ee05000 {
reg = <0x7ee05000 0x100>;
};
};
This looks okay to me to start with...
Can we please agree on something over the next 2 weeks, so that I can
implement it when I get back from vacation?
The point is that it would be a much more explicit device-tree
with the way I had proposed it above.
I guess we still could use the same driver (compatiblity)
with some reorganization. To detect old and new DTs we could
just check the size of regs or number of interrupts and if they
are reg.len > 256 bytes or count(interrupts) = 13 then fall back
to legacy probe...
Thanks,
Martin
From: Eric Anholt <hidden> Date: 2016-01-28 19:53:00
kernel at martin.sperl.org writes:
From: Martin Sperl <redacted>
bcm2835-dma supports residue reporting at burst level but didn't report
this via the residue_granularity field.
See also:
https://github.com/raspberrypi/linux/commit/b015555327afa402f70ddc86e3632f59df1cd9d7
for the downstream patch.
Signed-off-by: Matthias Reichl <redacted>
Signed-off-by: Noralf Tr?nnes <redacted>
Signed-off-by: Martin Sperl <redacted>
From: Eric Anholt <hidden> Date: 2016-01-30 03:08:45
kernel at martin.sperl.org writes:
From: Martin Sperl <redacted>
The original patch contained 3 dma channels that were masked out.
These - as far as research and discussions show - are a
artefacts remaining from the downstream legacy dma-api.
Right now down-stream still includes a legacy api used only
in a single (downstream only) driver (bcm2708_fb) that requires
2D DMA for speedup (DMA-channel 0).
Formerly the sd-card support driver also was using this legacy
api (DMA-channel 2), but since has been moved over to use
dmaengine directly.
The DMA-channel 3 is already masked out in the devicetree in
the default property "brcm,dma-channel-mask = <0x7f35>;"
Delayed the rest of the review, because I went digging around in the
firmware, and missed the ifdef mess in it that produced 0x7f35. As far
as I can tell, though, we should be using just that value to control
which channels we expose (other than the shared-irq issue). If the
firmware wants to use a different set of channels some day, they'll have
to edit our DT.
Reviewed-by: Eric Anholt <redacted>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160129/8188ea20/attachment-0001.sig>
From: Eric Anholt <hidden> Date: 2016-02-18 03:24:49
kernel at martin.sperl.org writes:
quoted hunk
From: Martin Sperl <redacted>
In preparation of adding slave_sg functionality this patch moves the
generation/allocation of bcm2835_desc and the building of
the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic
into the newly created method bcm2835_dma_create_cb_chain.
This new code also takes the limits of LITE channels into account
and splits the control-block transfers at the correct location.
Tested Audio output with a Hifiberry I2S card.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 288 ++++++++++++++++++++++++++++++---------------
1 file changed, 191 insertions(+), 97 deletions(-)
@@ -181,6 +181,22 @@ struct bcm2835_desc {#define BCM2835_DMA_IRQ_SHARED 11#define BCM2835_DMA_IRQ_ALL 12+/* the max dma length for different channels */+#define MAX_DMA_LEN SZ_1G+#define MAX_LITE_DMA_LEN (SZ_64K - 4)++staticinlineboolbcm2835_dma_is_lite(structbcm2835_chan*c)+{+/* dma channels >= 7 are LITE channels */+return(c->ch>=7);+}
You can ask a channel if it's a LITE engine by checking if the DEBUG reg
has bit 28 set. Then you don't need to have the software/DT guessing
about it.
However, I'm disappointed to see these changes for adding LITE support
included in the same commit as refactoring chain generation into a
separate function.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160217/2763337d/attachment.sig>
From: Eric Anholt <hidden> Date: 2016-02-18 04:09:23
kernel at martin.sperl.org writes:
From: Martin Sperl <redacted>
The bcm2835 dma channel 11 to 14 only have a single shared irq line,
so this patch implements shared interrupts for these channels.
To avoid changes to the device tree (with regards to interrupts listed,
which reflects on pdev->num_resources) a fixed channel count had
to get used in the code instead.
With this patch applied we now have 11 dma channels available to
the ARM side of the SOC.
Signed-off-by: Martin Sperl <redacted>
Particularly given the existing DT bindings we have, I actually think
this is a good way to go. This is internal to the hardware module about
how its features map to interrupt lines that leave the hardware module
(which are exposed in DT appropriately).
I thought about "what if we had the last interrupt line hooked up to a
different interrupt handler that read out and decided which channels to
wake up", but this seems much simpler.
Reviewed-by: Eric Anholt <redacted>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160217/ca03aa66/attachment.sig>
From: Eric Anholt <hidden> Date: 2016-02-18 04:18:24
kernel at martin.sperl.org writes:
quoted hunk
From: Martin Sperl <redacted>
Add additional defines describing the DMA registers
as well as adding some more documentation to those registers.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 54 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 46 insertions(+), 8 deletions(-)
s/STIDE/STRIDE/
With that fixed, and optionally Vinod's comment about reformatting the
comment changed,
Reviewed-by: Eric Anholt <redacted>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160217/6a256ca6/attachment.sig>
From: Eric Anholt <hidden> Date: 2016-02-18 04:39:25
kernel at martin.sperl.org writes:
From: Martin Sperl <redacted>
Add slave_sg support to bcm2835-dma using shared allocation
code for bcm2835_desc and DMA-control blocks already used by
dma_cyclic.
Note that bcm2835_dma_callback had to get modified to support
both modes of operation (cyclic and non-cyclic).
Tested using:
* Hifiberry I2S card (using cyclic DMA)
* fb_st7735r SPI-framebuffer (using slave_sg DMA via spi-bcm2835)
playing BigBuckBunny for audio and video.
Signed-off-by: Martin Sperl <redacted>
I would have expected bcm2835_dma_create_cb_chain()'s changes to support
slave DMA to be in this patch rather than the refactor patch. However,
the content of this one and 8/8 are:
Reviewed-by: Eric Anholt <redacted>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160217/9d8c8c3b/attachment.sig>
From: Martin Sperl <hidden> Date: 2016-02-29 17:10:53
On 14.01.2016, at 05:07, Vinod Koul [off-list ref] wrote:
On Wed, Jan 13, 2016 at 03:24:59PM +0100, Martin Sperl wrote:
quoted
quoted
quoted
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?
We still need to be backwards-compatible - at least that is what
everyone tells me, so I need to hard-code fallbacks for those values.
IMO hard code for falling back is okay as that supports old cases and new
platforms use geric DT info and new devices can be supported generically,
please check with DT folks..
quoted
quoted
quoted
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
that could be a DT property, yes.
quoted
quoted
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
OK - how would you define that "mapping" in a "sane" manner in the DT
that allows us to have multiple such mappings in the future?
You are hard coding the flags for each channel, we can pass this for each
channel in the interrupt configi, a flag share/none..? Please run this thru
DT experts and I am not one of them..
So here a ?simple? approach with the following device tree addition (plus defaults):
interrupts = <1 16>, /* dma 0 irq */
<1 17>, /* dma 1 irq */
...
<1 26>, /* dma 10 irq */
<1 27>, /* dma 11-14 irq */
<1 28>; /* dma all irq */
brcm,dma-channel-mask = <0x7f35>;
/* new properties below - also the defaults */
brcm,dma-channel-shared-mask = <0x7800>;
brcm,dma-shared-irq-index = <11>;
Is this sufficient to move forward?
Thanks,
Martin
P.s: this would also mean (for later patches):
brcm,dma-lite-channel-mask = <0x7f00>;
From: Martin Sperl <hidden> Date: 2016-02-29 18:14:19
On 18.02.2016, at 04:24, Eric Anholt [off-list ref] wrote:
kernel at martin.sperl.org writes:
quoted
From: Martin Sperl <redacted>
In preparation of adding slave_sg functionality this patch moves the
generation/allocation of bcm2835_desc and the building of
the corresponding DMA-control-block chain from bcm2835_dma_prep_dma_cyclic
into the newly created method bcm2835_dma_create_cb_chain.
This new code also takes the limits of LITE channels into account
and splits the control-block transfers at the correct location.
Tested Audio output with a Hifiberry I2S card.
Signed-off-by: Martin Sperl <redacted>
---
drivers/dma/bcm2835-dma.c | 288 ++++++++++++++++++++++++++++++---------------
1 file changed, 191 insertions(+), 97 deletions(-)
#define BCM2835_DMA_IRQ_SHARED 11
#define BCM2835_DMA_IRQ_ALL 12
+/* the max dma length for different channels */
+#define MAX_DMA_LEN SZ_1G
+#define MAX_LITE_DMA_LEN (SZ_64K - 4)
+
+static inline bool bcm2835_dma_is_lite(struct bcm2835_chan *c)
+{
+ /* dma channels >= 7 are LITE channels */
+ return (c->ch >= 7);
+}
You can ask a channel if it's a LITE engine by checking if the DEBUG reg
has bit 28 set. Then you don't need to have the software/DT guessing
about it.
I will implement that - it is simple.
However, I'm disappointed to see these changes for adding LITE support
included in the same commit as refactoring chain generation into a
separate function.
I will separate this ?LITE-channel handling? out into a separate patch.
Martin
On Mon, Feb 29, 2016 at 06:10:53PM +0100, Martin Sperl wrote:
quoted
On 14.01.2016, at 05:07, Vinod Koul [off-list ref] wrote:
On Wed, Jan 13, 2016 at 03:24:59PM +0100, Martin Sperl wrote:
quoted
quoted
quoted
But that would be frowned upon, so I had to come up with the approach
taken, which makes the following assumptions:
DT was designed to move this info and hardcoding from kernel into
DT, so why cant we do that?
We still need to be backwards-compatible - at least that is what
everyone tells me, so I need to hard-code fallbacks for those values.
IMO hard code for falling back is okay as that supports old cases and new
platforms use geric DT info and new devices can be supported generically,
please check with DT folks..
quoted
quoted
quoted
* the DT maps only the interrupts that are assigned to the HW block
* the driver knows about the number of DMA channels in HW
that could be a DT property, yes.
quoted
quoted
* the driver knows about the mapping of shared interrupts
(11-14 share irq).
OK - how would you define that "mapping" in a "sane" manner in the DT
that allows us to have multiple such mappings in the future?
You are hard coding the flags for each channel, we can pass this for each
channel in the interrupt configi, a flag share/none..? Please run this thru
DT experts and I am not one of them..
So here a ?simple? approach with the following device tree addition (plus defaults):
interrupts = <1 16>, /* dma 0 irq */
<1 17>, /* dma 1 irq */
...
<1 26>, /* dma 10 irq */
<1 27>, /* dma 11-14 irq */
<1 28>; /* dma all irq */
brcm,dma-channel-mask = <0x7f35>;
/* new properties below - also the defaults */
brcm,dma-channel-shared-mask = <0x7800>;
brcm,dma-shared-irq-index = <11>;
Is this sufficient to move forward?
This looks okay to me for now, though am not a DT expert. Please send
patches and CC DT folks as relevant and we can discuss..
--
~Vinod