Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote: > > Or alternatively we could publish the limitations of the channel using > > capabilities so SPI knows I have a dmaengine channel and it can transfer > > max N > > length transfers so would be able to break rather than guessing it or coding > > in DT. Yes it may come from DT but that should be dmaengine driver rather > > than client driver :) > > > > This can be done by dma_get_slave_caps(chan, &caps) > > > > And we add max_length as one more parameter to existing set > > > > Also all this could be handled in generic SPI-dmaengine layer so that > > individual drivers don't have to code it in > > > > Let me know if this idea is okay, I can push the dmaengine bits... > > It would be ok if there was a fixed limit. However, the limit depends > on SPI slave settings. Presumably for other buses using the dmaengine > the limit would depend on the bus or slave settings as well. I do not > see a sane way of passing this all the way to the dmaengine driver. I don't see why this should be client (SPI) dependent. The max length supported is a dmaengine constraint, typically flowing from max blocks/length it can transfer. Know this limit can allow clients to split transfers. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.
On Sun, Jul 19, 2015 at 09:01:34PM +0200, Michal Suchanek wrote: > Hello, > > On 15 July 2015 at 17:59, Brian Norris wrote: > > Hi Michal, > > > > On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote: > >> The problem is, if you add a new DT binding, you'd have to support it > >> forever, no matter how bad idea that binding turned out to be. > > > > Agreed, and a solid NAK to this patch. I could have sworn I gave such a > > response when this was originally being discussed a month ago. > > > > AFAICT, you have one of two general approaches available to you: > > > > 1. Fix up the SPI driver so that it knows how to break large SPI > > transfers up into smaller segments that its constituent hardware (DMA > > controllers, fast clocks, etc.) can handle. > > > > 2. Utilize/create a parameter within the SPI subsystem to communicate > > that the SPI master has a limited max transfer size (notably: NOT a > > per-device DT property, but a SPI API property), and modify SPI device > > drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I > > thought he suggested it somewhere. > > It is not known what exactly is limited here. > > It seems that the pl330 fails but it is not possible to transfer that > much data over the spi bus in one go without the help of the pl330. > > With either approach the limit depends on the SPI transfer settings > which are known the the SPI driver. The pl330 driver is oblivious to > these because it just transfers data from one port to another port and > has no idea that the port is wired to SPI in the SoC. > > On the other hand, AFAICT the SPI driver only allocates a DMA channel > which it receives through DT binding and is oblivious to the fact the > DMA channel lives on a pl330. It could probably determine that the > channel is indeed driven by a pl330. I don't think it's a great idea > to add device-specific handling to a generic dmaengine driver or a > dmaengine-spiecific handling to a SPI driver. > > It's technically possible to pass SPI transfer parameters to the > dmaengine driver prior to transfer and the dmaengine could impose some > limitation based on those parameters. However, generalising this to > drivers other than SPI might be problematic. Should this interface > also handle i2c parameters, VE parameters, audio parameters, ethernet > parameters, etc? Or alternatively we could publish the limitations of the channel using capabilities so SPI knows I have a dmaengine channel and it can transfer max N length transfers so would be able to break rather than guessing it or coding in DT. Yes it may come from DT but that should be dmaengine driver rather than client driver :) This can be done by dma_get_slave_caps(chan, &caps) And we add max_length as one more parameter to existing set Also all this could be handled in generic SPI-dmaengine layer so that individual drivers don't have to code it in Let me know if this idea is okay, I can push the dmaengine bits... Thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: exynos4412: Audio dies after one day on kernel 4.0
On Wed, Jun 10, 2015 at 05:22:07PM +0900, Krzysztof Kozlowski wrote: > > I can now say for sure that the commits 88987d2c75 and aee4d1fac8 are > > the cause for the problem. Audio was working now for 7 days. > > +CC Vinod > > Gabriel, > > I sent a patch which should fix the issue. Could you give it a try? Of > course don't revert the other patches and don't use other workarounds. > Just apply the patch on clean (vanilla would be the best) kernel. > [RFT PATCH] dmaengine: Fix choppy sound because of unimplemented resume I have already applied this one, can you check the -next and see if this issue is seen -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/11] dma: pl330: fix wording in mcbufsz message
On Wed, Jun 03, 2015 at 09:26:41PM +, Michal Suchanek wrote: > The kernel is not trying to increase mcbufsz. It suggests you should try > doing so. Also print the calculated required size of mcbufsz. pls use right subsystem name in the patches I have applied this now -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: SAMSUNG: remove unused DMA infrastructure
On Thu, Jan 15, 2015 at 04:16:03PM +0100, Arnd Bergmann wrote: > Everything uses dmaengine now, so there is no reason to > keep this around any longer. Thanks to everyone who was involved > in moving the users over to use the dmaengine APIs. Very good indeed :) Acked-by: Vinod Koul -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On Tue, Dec 09, 2014 at 08:48:04PM +0530, Jassi Brar wrote: > As Russell pointed out, that ain't the case either. > So we are yet to figure out benefits of having explicit > issue_pending() after tx_submit(). callback ? -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On Mon, Dec 08, 2014 at 02:23:21PM +, Russell King - ARM Linux wrote: > On Mon, Dec 08, 2014 at 06:37:27PM +0530, Vinod Koul wrote: > > I actually like the split model, you can also prepare txn ahead of time and > > submit them when needed. > > Actually, you can't - that's not permitted. I have email(s) from Dan > explicitly stating that it is permitted for a driver to take a spinlock > in their prepare callback, and release it when the descriptor is > submitted. Several DMA engine drivers (particularly those in for > async_tx) do exactly that. > > The reason that submit is separate from prepare is to allow DMA engine > users to set a callback - if it weren't for that, there wouldn't be a > submit step, prepare would have done everything. Yes thats right. Do you mind pointing to thread Dan replied, I would like to add these bits and anything else missing to Documentation Thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On Sat, Dec 06, 2014 at 12:31:01PM +0530, Jassi Brar wrote: > It does, though, create an "awkward situation" when a channel is > active while new requests are submitted - why would the channel want > to stop after current transfer and await fresh issue_pending()? It's > not that the request can be modified after submission. > > And it isn't that tx_submit() is meant for 'sleepable' preparation for > the transfer and we need another call to be issued from atomic > context. From what I see most drivers don't need to sleep in > tx_submit(). In fact, from a quick look most clients seem to suffer > from the ritual i.e, there's nothing between tx_submit() and > issue_pending() calls. And when there is indeed some code, it seems > that can be moved just before tx_submit(). > > Last and apparently the least of all, we can never enforce the same > behavior of the api on Async dma and have uniform behavior over the > api. > > So, if all tx_submit() does is put the request in controller driver's > internal queue and the client can't touch the request after > tx_submit(), why not merge the tx_submit() and issue_pending()? You are thinking from an API point and not what can be done with this API. Today this API allows you to collate all pending txn and start while dmaengine driver can collate and submit as a batch to hardware and not waste time in getting irq and then setting next one. Sadly none of the drivers use this feature... I actually like the split model, you can also prepare txn ahead of time and submit them when needed. If you don't like this, then please do as most implementation do today, call prepare and submit in series. Flexiblity in API is a better option IMO Thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
> On 3 December 2014 at 10:17, Padma Venkat wrote:
> > Hi Lars,
> >
> > [snip]
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + if (ret == DMA_COMPLETE || !txstate)
> + return ret;
> +
> + used = txstate->used;
> +
> + spin_lock_irqsave(&pch->lock, flags);
> + sar = readl(regs + SA(thrd->id));
> + dar = readl(regs + DA(thrd->id));
> +
> + list_for_each_entry(desc, &pch->work_list, node) {
> + if (desc->status == BUSY) {
> + current_c = desc->txd.cookie;
> + if (first) {
> + first_c = desc->txd.cookie;
> + first = false;
> + }
> +
> + if (first_c < current_c)
> + residue += desc->px.bytes;
> + else {
> + if (desc->rqcfg.src_inc &&
> pl330_src_addr_in_desc(desc, sar)) {
> + residue += desc->px.bytes;
> + residue -= sar - desc->px.src_addr;
> + } else if (desc->rqcfg.dst_inc &&
> pl330_dst_addr_in_desc(desc, dar))
> {
> + residue += desc->px.bytes;
> + residue -= dar - desc->px.dst_addr;
> + }
> + }
> + } else if (desc->status == PREP)
> + residue += desc->px.bytes;
> +
> + if (desc->txd.cookie == used)
> + break;
> + }
> + spin_unlock_irqrestore(&pch->lock, flags);
> + dma_set_residue(txstate, residue);
> + return ret;
> }
> > [snip]
> >>>
> >>> Any comment on this patch?
> >>
> >> Well it doesn't break audio, but I don't think it has the correct haviour
> >> for all cases yet.
> >
> > OK. Any way of testing other cases like scatter-gather and memcopy. I
> > verified memcopy in dmatest but it seems not doing anything with
> > residue bytes.
> >
> >>
> >> Again, the semantics are that it should return the progress of the transfer
> >>
> >> for which the allocation function returned the cookie that is passe to this
> >
> > May be my understanding is wrong. For clarification..In the
> > snd_dmaengine_pcm_pointer it is subtracting the residue bytes from the
> > total buffer bytes not from period bytes. So how it expects
> > the progress of the transfer of the passed cookie which just holds period
> > bytes?
> >
> >>
> >> function. You have to consider that there might be multiple different
> >> descriptors submitted and in the work list, not just the one we want to
> >> know
> >
> > Even though there are multiple descriptors in the work list, at a time
> > only two descriptors are in busy state(as per the documentation in the
> > driver) and all the descriptors cookie number is in incremental order.
> > Not sure for other cases how it will be.
> >
> Yes.
>
> Tracing the history ... I think we could have done without
>
> 04abf5daf7d dma: pl330: Differentiate between submitted and issued
> descriptors
>
> The pl330 dmaengine driver currently does not differentiate
> between submitted
> and issued descriptors. It won't start transferring a newly submitted
> descriptor until issue_pending() is called, but only if it is idle. If it
> is
> active and a new descriptor is submitted before it goes idle it will
> happily
> start the newly submitted descriptor once all earlier submitted
> descriptors have
> been completed. This is not a 100% correct with regards to the dmaengine
> interface semantics. A descriptor is not supposed to be started
> until the next
> issue_pending() call after the descriptor has been submitted.
>
>
> because the reasoning above seems incorrect considering the following
> documentation...
>
> Documentation/crypto/async-tx-api.txt says
> " Once a driver-specific threshold is met the driver
> automatically issues pending operations. An application can force this
> event by calling async_tx_issue_pending_all(). "
>
> And
>
> include/linux/dmaengine.h says
> dma_async_tx_descriptor.tx_submit(): set the prepared descriptor(s)
> to be executed by the engine
"to be" here can lead to different conculsion. I will reword this :)
@tx_submit: accept the descriptor and assign ordered cookie and mark the
descriptor pending. To be pushed on .issue_pending() call
--
~Vinod
>
> so theoretically a driver, not starting transfer until
> issue_pending(), is "broken".
> At best the patch@04abf5daf7d makes the driver slightly more
> complicated and the reason behind confusion such as in this thread.
>
> -jassi
--
--
To unsubscribe from this list: send the line "unsubscribe linu
Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
On Tue, Dec 02, 2014 at 06:25:49PM +0100, Lars-Peter Clausen wrote: > On 12/02/2014 06:38 AM, Padma Venkat wrote: > > Well it doesn't break audio, but I don't think it has the correct > haviour for all cases yet. > > Again, the semantics are that it should return the progress of the > transfer for which the allocation function returned the cookie that > is passe to this function. You have to consider that there might be > multiple different descriptors submitted and in the work list, not > just the one we want to know the status of. The big problem with the > pl330 driver is that the current structure of the driver makes it > not so easy to implement the residue reporting correctly. well you can use the completed_cookie as hint. This was last trasaction which was issues, so calculate remianing bytes for this and subsequent ones resiude would only be size of transaction -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: pl08x: Use correct specifier for size_t values
On Fri, Aug 01, 2014 at 06:09:48PM +0100, Mark Brown wrote: > From: Mark Brown > > When printing size_t values we should use the %zd or %zx format specifier > in order to ensure the value is displayed correctly and avoid warnings from > sparse. Applied, thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] dmaengine: s3c24xx-dma: use DMA_COMPLETE for dma completion status
On Thu, Nov 28, 2013 at 02:33:22PM -0800, Dan Williams wrote: > On Wed, Nov 27, 2013 at 11:25 PM, Vinod Koul wrote: > > On Wed, Nov 13, 2013 at 02:28:58PM +0530, Vinod Koul wrote: > >> On Tue, Nov 12, 2013 at 07:28:10PM +0900, Kukjin Kim wrote: > >> > Vinod Koul wrote: > >> > > > >> > > On Thu, Oct 31, 2013 at 10:48:09AM +0530, Sachin Kamat wrote: > >> > > > Use the recently introduced DMA_COMPLETE instead of DMA_SUCCESS. > >> > > > Without this patch we get the following build error: > >> > > > drivers/dma/s3c24xx-dma.c: In function ‘s3c24xx_dma_tx_status’: > >> > > > drivers/dma/s3c24xx-dma.c:798:13: error: ‘DMA_SUCCESS’ undeclared > >> > > > (first use in this function) > >> > > I missed it because this is not in my tree! This needs to go thru > >> > > kgene's > >> > > tree > >> > > and for that you can merge dma_complete branch from my tree. I will not > >> > > rebase > >> > > that > >> > > > >> > > Acked-by: Vinod Koul > >> > > > >> > > >> > Oops, I missed :( > >> > > >> > Vinod, since the s3c24xx-dma is in Linus' tree now, you can go ahead > >> > with my ack into your fixes. > >> > > >> > If you want me to take this into samsung tree, let me know :-) > >> Okay, I will apply this after merge window > > Applied now, > > > > This driver also missed the unmap rework. Fixing now. yup, this is a new driver and got merged thru samsung tree > Vinod, I'll send you the pile after I finish my build regression suite. sure, i think these should go into fixes -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] dmaengine: s3c24xx-dma: use DMA_COMPLETE for dma completion status
On Wed, Nov 13, 2013 at 02:28:58PM +0530, Vinod Koul wrote: > On Tue, Nov 12, 2013 at 07:28:10PM +0900, Kukjin Kim wrote: > > Vinod Koul wrote: > > > > > > On Thu, Oct 31, 2013 at 10:48:09AM +0530, Sachin Kamat wrote: > > > > Use the recently introduced DMA_COMPLETE instead of DMA_SUCCESS. > > > > Without this patch we get the following build error: > > > > drivers/dma/s3c24xx-dma.c: In function ‘s3c24xx_dma_tx_status’: > > > > drivers/dma/s3c24xx-dma.c:798:13: error: ‘DMA_SUCCESS’ undeclared > > > > (first use in this function) > > > I missed it because this is not in my tree! This needs to go thru kgene's > > > tree > > > and for that you can merge dma_complete branch from my tree. I will not > > > rebase > > > that > > > > > > Acked-by: Vinod Koul > > > > > > > Oops, I missed :( > > > > Vinod, since the s3c24xx-dma is in Linus' tree now, you can go ahead with > > my ack into your fixes. > > > > If you want me to take this into samsung tree, let me know :-) > Okay, I will apply this after merge window Applied now, Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] dmaengine: s3c24xx-dma: use DMA_COMPLETE for dma completion status
On Tue, Nov 12, 2013 at 07:28:10PM +0900, Kukjin Kim wrote:
> Vinod Koul wrote:
> >
> > On Thu, Oct 31, 2013 at 10:48:09AM +0530, Sachin Kamat wrote:
> > > Use the recently introduced DMA_COMPLETE instead of DMA_SUCCESS.
> > > Without this patch we get the following build error:
> > > drivers/dma/s3c24xx-dma.c: In function ‘s3c24xx_dma_tx_status’:
> > > drivers/dma/s3c24xx-dma.c:798:13: error: ‘DMA_SUCCESS’ undeclared
> > > (first use in this function)
> > I missed it because this is not in my tree! This needs to go thru kgene's
> > tree
> > and for that you can merge dma_complete branch from my tree. I will not
> > rebase
> > that
> >
> > Acked-by: Vinod Koul
> >
>
> Oops, I missed :(
>
> Vinod, since the s3c24xx-dma is in Linus' tree now, you can go ahead with my
> ack into your fixes.
>
> If you want me to take this into samsung tree, let me know :-)
Okay, I will apply this after merge window
--
~Vinod
>
> Thanks,
> Kukjin
>
> > --
> > ~Vinod
> > >
> > > Signed-off-by: Sachin Kamat
> > > Cc: Heiko Stuebner
> > > Cc: Vinod Koul
> > > ---
> > > drivers/dma/s3c24xx-dma.c |2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
> > > index 4cb127978636..085da4fe6613 100644
> > > --- a/drivers/dma/s3c24xx-dma.c
> > > +++ b/drivers/dma/s3c24xx-dma.c
> > > @@ -795,7 +795,7 @@ static enum dma_status s3c24xx_dma_tx_status(struct
> > dma_chan *chan,
> > >
> > > spin_lock_irqsave(&s3cchan->vc.lock, flags);
> > > ret = dma_cookie_status(chan, cookie, txstate);
> > > - if (ret == DMA_SUCCESS) {
> > > + if (ret == DMA_COMPLETE) {
> > > spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
> > > return ret;
> > > }
> > > --
> > > 1.7.9.5
> > >
> >
> > --
>
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] dmaengine: s3c24xx-dma: use DMA_COMPLETE for dma completion status
On Thu, Oct 31, 2013 at 10:48:09AM +0530, Sachin Kamat wrote:
> Use the recently introduced DMA_COMPLETE instead of DMA_SUCCESS.
> Without this patch we get the following build error:
> drivers/dma/s3c24xx-dma.c: In function ‘s3c24xx_dma_tx_status’:
> drivers/dma/s3c24xx-dma.c:798:13: error: ‘DMA_SUCCESS’ undeclared
> (first use in this function)
I missed it because this is not in my tree! This needs to go thru kgene's tree
and for that you can merge dma_complete branch from my tree. I will not rebase
that
Acked-by: Vinod Koul
--
~Vinod
>
> Signed-off-by: Sachin Kamat
> Cc: Heiko Stuebner
> Cc: Vinod Koul
> ---
> drivers/dma/s3c24xx-dma.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
> index 4cb127978636..085da4fe6613 100644
> --- a/drivers/dma/s3c24xx-dma.c
> +++ b/drivers/dma/s3c24xx-dma.c
> @@ -795,7 +795,7 @@ static enum dma_status s3c24xx_dma_tx_status(struct
> dma_chan *chan,
>
> spin_lock_irqsave(&s3cchan->vc.lock, flags);
> ret = dma_cookie_status(chan, cookie, txstate);
> - if (ret == DMA_SUCCESS) {
> + if (ret == DMA_COMPLETE) {
> spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
> return ret;
> }
> --
> 1.7.9.5
>
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 0/3] ARM: S3C24XX: add missing dma-devices and warning fix
On Wed, Oct 16, 2013 at 07:32:20AM +0900, Kukjin Kim wrote: > On 10/14/13 03:11, Heiko Stübner wrote: > >Am Sonntag, 13. Oktober 2013, 16:56:42 schrieb Vinod Koul: > >>On Fri, Oct 11, 2013 at 10:59:19AM +0200, Heiko Stübner wrote: > >>>[I messed up the linux-arm-kernel list address yesterday, so I resend it > >>>with a fixed address, sorry for the noise] > >>> > >>>When Olof reported the warning about the unused s3c2410_dma_resource, I > >>>thought the best way forward would be to simply implement the missing > >>>pieces and so it has users :-) . > >>> > >>>Therefore this series adds the necessary platform-devices for s3c2410, > >>>s3c2440 and s3c2442. This especially also includes the > >>>channel-constraints of those socs. > >>> > >>>As I do not have access to any of those socs these changes are of course > >>>compile-tested only. > >> > >>Do you want this to be applied thru dma or arm tree? > > > >hmm, the original series went into the samsung tree for 3.13, so maybe this > >addition should go the same way? > > > I think so, let me pick this up into Samsung tree for v3.13. Okay, sounds good. Ack for dmaengine patch sent ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 2/3] dmaengine: s3c24xx-dma: add support for the s3c2410 type of controller
On Fri, Oct 11, 2013 at 11:01:04AM +0200, Heiko Stübner wrote:
> The earliest variants of the dma controller did not contain support for
> controlling clocks.
Acked-by: Vinod Koul
>
> Signed-off-by: Heiko Stuebner
> ---
> drivers/dma/s3c24xx-dma.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
> index 56c9253..4cb1279 100644
> --- a/drivers/dma/s3c24xx-dma.c
> +++ b/drivers/dma/s3c24xx-dma.c
> @@ -1078,6 +1078,13 @@ static void s3c24xx_dma_free_virtual_channels(struct
> dma_device *dmadev)
> list_del(&chan->vc.chan.device_node);
> }
>
> +/* s3c2410, s3c2440 and s3c2442 have a 0x40 stride without separate clocks */
> +static struct soc_data soc_s3c2410 = {
> + .stride = 0x40,
> + .has_reqsel = false,
> + .has_clocks = false,
> +};
> +
> /* s3c2412 and s3c2413 have a 0x40 stride and dmareqsel mechanism */
> static struct soc_data soc_s3c2412 = {
> .stride = 0x40,
> @@ -1094,6 +1101,9 @@ static struct soc_data soc_s3c2443 = {
>
> static struct platform_device_id s3c24xx_dma_driver_ids[] = {
> {
> + .name = "s3c2410-dma",
> + .driver_data= (kernel_ulong_t)&soc_s3c2410,
> + }, {
> .name = "s3c2412-dma",
> .driver_data= (kernel_ulong_t)&soc_s3c2412,
> }, {
> --
> 1.7.10.4
>
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 0/3] ARM: S3C24XX: add missing dma-devices and warning fix
On Fri, Oct 11, 2013 at 10:59:19AM +0200, Heiko Stübner wrote: > [I messed up the linux-arm-kernel list address yesterday, so I resend it > with a fixed address, sorry for the noise] > > When Olof reported the warning about the unused s3c2410_dma_resource, I > thought the best way forward would be to simply implement the missing > pieces and so it has users :-) . > > Therefore this series adds the necessary platform-devices for s3c2410, > s3c2440 and s3c2442. This especially also includes the channel-constraints > of those socs. > > As I do not have access to any of those socs these changes are of course > compile-tested only. Do you want this to be applied thru dma or arm tree? ~Vinod > > Heiko Stuebner (3): > ARM: S3C24XX: Fix possible dma selection warning > dmaengine: s3c24xx-dma: add support for the s3c2410 type of controller > ARM: S3C24XX: add dma platformdata for s3c2410, s3c2440 and s3c2442 > > arch/arm/mach-s3c24xx/Kconfig |3 +- > arch/arm/mach-s3c24xx/common.c| 100 > + > arch/arm/mach-s3c24xx/common.h|2 + > drivers/dma/s3c24xx-dma.c | 10 +++ > include/linux/platform_data/dma-s3c24xx.h |3 + > sound/soc/samsung/Kconfig |2 +- > 6 files changed, 118 insertions(+), 2 deletions(-) > > -- > 1.7.10.4 > -- -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 3/3] ARM: S3C24XX: add dma pdata for s3c2410, s3c2440 and s3c2442
On Fri, Oct 11, 2013 at 11:01:33AM +0200, Heiko Stübner wrote:
> s3c2410 and s3c2442 share the same dma channels while s3c2440 has
> slight differences. But on all three the reachable sources per dma
> channel has constraints attached and thus encodes the usable
> combinations using the S3C24XX_DMA_CHANREQ macro.
>
> This also fixes the warning about s3c2410_dma_resource being unused
> as reported by Olof Johansson.
Perhpas you should have used Reported-by tag for that..
~Vinod
>
> Signed-off-by: Heiko Stuebner
> ---
> arch/arm/mach-s3c24xx/common.c| 100
> +
> arch/arm/mach-s3c24xx/common.h|2 +
> include/linux/platform_data/dma-s3c24xx.h |3 +
> 3 files changed, 105 insertions(+)
>
> diff --git a/arch/arm/mach-s3c24xx/common.c b/arch/arm/mach-s3c24xx/common.c
> index 16ac669..4cfe7a4 100644
> --- a/arch/arm/mach-s3c24xx/common.c
> +++ b/arch/arm/mach-s3c24xx/common.c
> @@ -343,6 +343,50 @@ static struct resource s3c2410_dma_resource[] = {
> };
> #endif
>
> +#if defined(CONFIG_CPU_S3C2410) || defined(CONFIG_CPU_S3C2442)
> +static struct s3c24xx_dma_channel s3c2410_dma_channels[DMACH_MAX] = {
> + [DMACH_XD0] = { S3C24XX_DMA_AHB, true, S3C24XX_DMA_CHANREQ(0, 0), },
> + [DMACH_XD1] = { S3C24XX_DMA_AHB, true, S3C24XX_DMA_CHANREQ(0, 1), },
> + [DMACH_SDI] = { S3C24XX_DMA_APB, false, S3C24XX_DMA_CHANREQ(2, 0) |
> + S3C24XX_DMA_CHANREQ(2, 2) |
> + S3C24XX_DMA_CHANREQ(1, 3),
> + },
> + [DMACH_SPI0] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(3, 1), },
> + [DMACH_SPI1] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(2, 3), },
> + [DMACH_UART0] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(1, 0), },
> + [DMACH_UART1] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(1, 1), },
> + [DMACH_UART2] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(0, 3), },
> + [DMACH_TIMER] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(3, 0) |
> + S3C24XX_DMA_CHANREQ(3, 2) |
> + S3C24XX_DMA_CHANREQ(3, 3),
> + },
> + [DMACH_I2S_IN] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(2, 1) |
> + S3C24XX_DMA_CHANREQ(1, 2),
> + },
> + [DMACH_I2S_OUT] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(0, 2), },
> + [DMACH_USB_EP1] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(4, 0), },
> + [DMACH_USB_EP2] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(4, 1), },
> + [DMACH_USB_EP3] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(4, 2), },
> + [DMACH_USB_EP4] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(4, 3), },
> +};
> +
> +static struct s3c24xx_dma_platdata s3c2410_dma_platdata = {
> + .num_phy_channels = 4,
> + .channels = s3c2410_dma_channels,
> + .num_channels = DMACH_MAX,
> +};
> +
> +struct platform_device s3c2410_device_dma = {
> + .name = "s3c2410-dma",
> + .id = 0,
> + .num_resources = ARRAY_SIZE(s3c2410_dma_resource),
> + .resource = s3c2410_dma_resource,
> + .dev= {
> + .platform_data = &s3c2410_dma_platdata,
> + },
> +};
> +#endif
> +
> #ifdef CONFIG_CPU_S3C2412
> static struct s3c24xx_dma_channel s3c2412_dma_channels[DMACH_MAX] = {
> [DMACH_XD0] = { S3C24XX_DMA_AHB, true, 17 },
> @@ -384,6 +428,62 @@ struct platform_device s3c2412_device_dma = {
> };
> #endif
>
> +#if defined(CONFIG_CPU_S3C2440)
> +static struct s3c24xx_dma_channel s3c2440_dma_channels[DMACH_MAX] = {
> + [DMACH_XD0] = { S3C24XX_DMA_AHB, true, S3C24XX_DMA_CHANREQ(0, 0), },
> + [DMACH_XD1] = { S3C24XX_DMA_AHB, true, S3C24XX_DMA_CHANREQ(0, 1), },
> + [DMACH_SDI] = { S3C24XX_DMA_APB, false, S3C24XX_DMA_CHANREQ(2, 0) |
> + S3C24XX_DMA_CHANREQ(6, 1) |
> + S3C24XX_DMA_CHANREQ(2, 2) |
> + S3C24XX_DMA_CHANREQ(1, 3),
> + },
> + [DMACH_SPI0] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(3, 1), },
> + [DMACH_SPI1] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(2, 3), },
> + [DMACH_UART0] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(1, 0), },
> + [DMACH_UART1] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(1, 1), },
> + [DMACH_UART2] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(0, 3), },
> + [DMACH_TIMER] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(3, 0) |
> + S3C24XX_DMA_CHANREQ(3, 2) |
> + S3C24XX_DMA_CHANREQ(3, 3),
> + },
> + [DMACH_I2S_IN] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(2, 1) |
> + S3C24XX_DMA_CHANREQ(1, 2),
> + },
> + [DMACH_I2S_OUT] = { S3C24XX_DMA_APB, true, S3C24XX_DMA_CHANREQ(5,
Re: [PATCH 1/3] dmaengine: pl330: Set residue in tx_status callback.
On Wed, Oct 09, 2013 at 01:37:56PM -0700, Dylan Reid wrote: > On Sun, Oct 6, 2013 at 8:48 PM, Vinod Koul wrote: > > On Mon, Oct 07, 2013 at 10:39:34AM +0900, Chanho Park wrote: > >> > > Why didn't you use a cookie value to track the request? > >> > > The cookie is assigned when each transfer is submitted. > >> > > If you save the value in the desc, we can find the request easily. > >> > > >> > If there are several cyclic desc in the work list, is there a better way > >> > to find the "current" one? The chan struct tracks the last completed and > >> > last submitted cookies, but these will be the first and last > >> > respectively as long as the cyclic transfer is active. Is there an > >> > "active" cookie stored somewhere that I missed? > >> > >> Assume there are three cookies. If you want to get the second cookie not > >> latest cookie, your way can be also correct in such case? > >> I think tx_status API is to get dma status of the given cookie. > >> You are only considering a cyclic case. > > For cyclic case you would have possible same descriptor running till you > > terminate. > > The cyclic case that makes this interesting is when there are multiple > cyclic descriptors in the list. The cookie and completed_cookie > markers don't help to determine which of the descriptors in the list > is currently active. dma_cookie_complete isn't called for a cyclic > desc, the desc is just pushed to the end of the list. Yes the cyclic is a very different case. I think driver can still return for cyclic case which was txstate->last and that will give clue to client which is getting processed now! > > For non cyclic, if you have 3 descriptors submitted, the cookie value can > > be, say > > 7, 8 and 9. If you query the status of any descriptor and pass the last > > optional > > txstate arg then you will know the last cookie completed. so if > > txstate->last is > > 7, then 7th was completed and 8 should be running and 9 in queue! > > Got it, but the correct desc for cookie 8 will still have to be > searched for, correct? Yes -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/4] dmaengine: add driver for Samsung s3c24xx SoCs
On Wed, Sep 18, 2013 at 09:19:54PM +0200, Heiko Stübner wrote: > This adds a new driver to support the s3c24xx dma using the dmaengine > and makes the old one in mach-s3c24xx obsolete in the long run. > > Conceptually the s3c24xx-dma feels like a distant relative of the pl08x > with numerous virtual channels being mapped to a lot less physical ones. > The driver therefore borrows a lot from the amba-pl08x driver in this > regard. Functionality-wise the driver gains a memcpy ability in addition > to the slave_sg one. > > The driver supports both the method for requesting the peripheral used > by SoCs before the S3C2443 and the different method for S3C2443 and later. > > On earlier SoCs the hardware channels usable for specific peripherals is > constrainted while on later SoCs all channels can be used for any peripheral. > > Tested on a s3c2416-based board, memcpy using the dmatest module and > slave_sg partially using the spi-s3c64xx driver. > > Signed-off-by: Heiko Stuebner > Acked-by: Linus Walleij Acked-by: Vinod Koul -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] dmaengine: pl330: Set residue in tx_status callback.
On Mon, Oct 07, 2013 at 10:39:34AM +0900, Chanho Park wrote: > > > Why didn't you use a cookie value to track the request? > > > The cookie is assigned when each transfer is submitted. > > > If you save the value in the desc, we can find the request easily. > > > > If there are several cyclic desc in the work list, is there a better way > > to find the "current" one? The chan struct tracks the last completed and > > last submitted cookies, but these will be the first and last > > respectively as long as the cyclic transfer is active. Is there an > > "active" cookie stored somewhere that I missed? > > Assume there are three cookies. If you want to get the second cookie not > latest cookie, your way can be also correct in such case? > I think tx_status API is to get dma status of the given cookie. > You are only considering a cyclic case. For cyclic case you would have possible same descriptor running till you terminate. For non cyclic, if you have 3 descriptors submitted, the cookie value can be, say 7, 8 and 9. If you query the status of any descriptor and pass the last optional txstate arg then you will know the last cookie completed. so if txstate->last is 7, then 7th was completed and 8 should be running and 9 in queue! > > Looking for the first buffer with status == BUSY is an improvement I'll > > make. Any way to avoid looking through the list? Its already there :) -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 24/51] DMA-API: dma: pl330: add dma_set_mask_and_coherent() call
On Sat, Sep 21, 2013 at 09:00:00PM +0100, Russell King - ARM Linux wrote: > On Fri, Sep 20, 2013 at 07:26:27PM +0200, Heiko Stübner wrote: > > Am Donnerstag, 19. September 2013, 23:49:01 schrieb Russell King: > > > The DMA API requires drivers to call the appropriate dma_set_mask() > > > functions before doing any DMA mapping. Add this required call to > > > the AMBA PL08x driver. > > ^--- copy and paste error - should of course be PL330 > > Fixed, thanks. with fixed changelog... Acked-by: Vinod Koul ~Vinod -- -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 43/51] DMA-API: dma: edma.c: no need to explicitly initialize DMA masks
On Fri, Sep 20, 2013 at 12:15:39AM +0100, Russell King wrote:
> register_platform_device_full() can setup the DMA mask provided the
> appropriate member is set in struct platform_device_info. So lets
> make that be the case. This avoids a direct reference to the DMA
> masks by this driver.
>
> Signed-off-by: Russell King
Acked-by: Vinod Koul
This also brings me question that should we force the driver to use the
dma_set_mask_and_coherent() API or they have below flexiblity too?
~Vinod
> ---
> drivers/dma/edma.c |6 ++
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index ff50ff4..7f9fe30 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -702,11 +702,13 @@ static struct platform_device *pdev0, *pdev1;
> static const struct platform_device_info edma_dev_info0 = {
> .name = "edma-dma-engine",
> .id = 0,
> + .dma_mask = DMA_BIT_MASK(32),
> };
>
> static const struct platform_device_info edma_dev_info1 = {
> .name = "edma-dma-engine",
> .id = 1,
> + .dma_mask = DMA_BIT_MASK(32),
> };
>
> static int edma_init(void)
> @@ -720,8 +722,6 @@ static int edma_init(void)
> ret = PTR_ERR(pdev0);
> goto out;
> }
> - pdev0->dev.dma_mask = &pdev0->dev.coherent_dma_mask;
> - pdev0->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> }
>
> if (EDMA_CTLRS == 2) {
> @@ -731,8 +731,6 @@ static int edma_init(void)
> platform_device_unregister(pdev0);
> ret = PTR_ERR(pdev1);
> }
> - pdev1->dev.dma_mask = &pdev1->dev.coherent_dma_mask;
> - pdev1->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> }
>
> out:
> --
> 1.7.4.4
>
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH 23/51] DMA-API: dma: pl08x: add dma_set_mask_and_coherent() call
On Thu, Sep 19, 2013 at 10:48:01PM +0100, Russell King wrote:
> The DMA API requires drivers to call the appropriate dma_set_mask()
> functions before doing any DMA mapping. Add this required call to
> the AMBA PL08x driver.
>
> Signed-off-by: Russell King
Acked-by: Vinod Koul
~Vinod
> ---
> drivers/dma/amba-pl08x.c |5 +
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index fce46c5..e51a983 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -2055,6 +2055,11 @@ static int pl08x_probe(struct amba_device *adev, const
> struct amba_id *id)
> if (ret)
> return ret;
>
> + /* Ensure that we can do DMA */
> + ret = dma_set_mask_and_coherent(&adev->dev, DMA_BIT_MASK(32));
> + if (ret)
> + goto out_no_pl08x;
> +
> /* Create the driver state holder */
> pl08x = kzalloc(sizeof(*pl08x), GFP_KERNEL);
> if (!pl08x) {
> --
> 1.7.4.4
>
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] dmaengine: pl330: Set residue in tx_status callback.
On Wed, Sep 11, 2013 at 11:38:03AM +0530, Padmavathi Venna wrote:
> From: Dylan Reid
>
> Fill txstate.residue with the amount of bytes remaining in the current
> transfer if the transfer is not complete. This will be of particular
> use to i2s DMA transfers, providing more accurate hw_ptr values to ASoC.
>
> Signed-off-by: Dylan Reid
> Reviewed-by: Olof Johansson
> Signed-off-by: Padmavathi Venna
> ---
> drivers/dma/pl330.c | 55
> ++-
> 1 files changed, 54 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 593827b..7ab9136 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2476,11 +2476,64 @@ static void pl330_free_chan_resources(struct dma_chan
> *chan)
> spin_unlock_irqrestore(&pch->lock, flags);
> }
>
> +static inline int
> +pl330_src_addr_in_desc(struct dma_pl330_desc *desc, unsigned int sar)
> +{
> + return ((desc->px.src_addr <= sar) &&
> + (sar <= (desc->px.src_addr + desc->px.bytes)));
> +}
> +
> +static inline int
> +pl330_dst_addr_in_desc(struct dma_pl330_desc *desc, unsigned int dar)
> +{
> + return ((desc->px.dst_addr <= dar) &&
> + (dar <= (desc->px.dst_addr + desc->px.bytes)));
> +}
> +
> +static unsigned int pl330_tx_residue(struct dma_chan *chan)
> +{
> + struct dma_pl330_chan *pch = to_pchan(chan);
> + void __iomem *regs = pch->dmac->pif.base;
> + struct pl330_thread *thrd = pch->pl330_chid;
> + struct dma_pl330_desc *desc;
> + unsigned int sar, dar;
> + unsigned int residue = 0;
> + unsigned long flags;
> +
> + sar = readl(regs + SA(thrd->id));
> + dar = readl(regs + DA(thrd->id));
> +
> + spin_lock_irqsave(&pch->lock, flags);
> +
> + /* Find the desc related to the current buffer. */
> + list_for_each_entry(desc, &pch->work_list, node) {
> + if (desc->rqcfg.src_inc && pl330_src_addr_in_desc(desc, sar)) {
> + residue = desc->px.bytes - (sar - desc->px.src_addr);
> + goto found_unlock;
> + }
> + if (desc->rqcfg.dst_inc && pl330_dst_addr_in_desc(desc, dar)) {
> + residue = desc->px.bytes - (dar - desc->px.dst_addr);
> + goto found_unlock;
> + }
> + }
> +
> +found_unlock:
> + spin_unlock_irqrestore(&pch->lock, flags);
> +
> + return residue;
> +}
> +
> static enum dma_status
> pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>struct dma_tx_state *txstate)
> {
> - return dma_cookie_status(chan, cookie, txstate);
> + enum dma_status ret;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + if (ret != DMA_SUCCESS) /* Not complete, check amount left. */
if will return DMA_IN_PROGRESS for two cases
a) when the descriptor is in queue
b) when the descriptor is submitted and running
You need to take different paths for these two cases. For former just return
residue
as complete size of descriptor. For latter you cna read sar/dar and check
remaining bytes in sg_list.
Hint: use the cookie values to find the state
> + dma_set_residue(txstate, pl330_tx_residue(chan));
> +
> + return ret;
> }
>
> static void pl330_issue_pending(struct dma_chan *chan)
~Vinod
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/4] dmaengine: add driver for Samsung s3c24xx SoCs
On Thu, Sep 05, 2013 at 12:35:23AM +0200, Heiko Stübner wrote:
> This adds a new driver to support the s3c24xx dma using the dmaengine
> and makes the old one in mach-s3c24xx obsolete in the long run.
>
> Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> with numerous virtual channels being mapped to a lot less physical ones.
> The driver therefore borrows a lot from the amba-pl08x driver in this
> regard. Functionality-wise the driver gains a memcpy ability in addition
> to the slave_sg one.
>
> The driver supports both the method for requesting the peripheral used
> by SoCs before the S3C2443 and the different method for S3C2443 and later.
>
> On earlier SoCs the hardware channels usable for specific peripherals is
> constrainted while on later SoCs all channels can be used for any peripheral.
>
> Tested on a s3c2416-based board, memcpy using the dmatest module and
> slave_sg partially using the spi-s3c64xx driver.
>
> Signed-off-by: Heiko Stuebner
> Acked-by: Linus Walleij
> ---
> +static enum dma_status s3c24xx_dma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> + struct s3c24xx_dma_chan *s3cchan = to_s3c24xx_dma_chan(chan);
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> + enum dma_status ret;
> + size_t bytes = 0;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + if (ret == DMA_SUCCESS)
> + return ret;
> +
> + /*
> + * There's no point calculating the residue if there's
> + * no txstate to store the value.
> + */
> + if (!txstate)
> + return ret;
> +
> + spin_lock_irqsave(&s3cchan->vc.lock, flags);
> + ret = dma_async_is_complete(cookie, txstate->last, txstate->used);
Did you see what dma_cookie_status() does. There is no need to call
dma_async_is_complete() here!
> + if (ret != DMA_SUCCESS) {
> + struct s3c24xx_txd *txd;
> + struct s3c24xx_sg *dsg;
> +
> + vd = vchan_find_desc(&s3cchan->vc, cookie);
> + if (vd) {
> + /* On the issued list, so hasn't been processed yet */
> + txd = to_s3c24xx_txd(&vd->tx);
> +
> + list_for_each_entry(dsg, &txd->dsg_list, node)
> + bytes += dsg->len;
> + } else {
> + /*
> + * Currently running, so sum over the pending sg's and
> + * the currently active one.
> + */
> + txd = s3cchan->at;
> +
> + dsg = list_entry(txd->at, struct s3c24xx_sg, node);
> + list_for_each_entry_from(dsg, &txd->dsg_list, node);
> + bytes += dsg->len;
> +
> + bytes += s3c24xx_dma_getbytes_chan(s3cchan);
> + }
> + }
> + spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
> +
> + /*
> + * This cookie not complete yet
> + * Get number of bytes left in the active transactions and queue
> + */
> + dma_set_residue(txstate, bytes);
> +
> + /* Whether waiting or running, we're in progress */
> + return ret;
Aprt form this driver looks fine to me...
~Vinod
--
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] dmaengine: add driver for Samsung s3c24xx SoCs
On Wed, Aug 28, 2013 at 12:13:51AM +0200, Heiko Stübner wrote:
> This adds a new driver to support the s3c24xx dma using the dmaengine
> and makes the old one in mach-s3c24xx obsolete in the long run.
>
> Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> with numerous virtual channels being mapped to a lot less physical ones.
> The driver therefore borrows a lot from the amba-pl08x driver in this
> regard. Functionality-wise the driver gains a memcpy ability in addition
> to the slave_sg one.
>
> The driver supports both the method for requesting the peripheral used
> by SoCs before the S3C2443 and the different method for S3C2443 and later.
>
> On earlier SoCs the hardware channels usable for specific peripherals is
> constrainted while on later SoCs all channels can be used for any peripheral.
>
> Tested on a s3c2416-based board, memcpy using the dmatest module and
> slave_sg partially using the spi-s3c64xx driver.
>
> +static int s3c24xx_dma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> + unsigned long arg)
> +{
> + struct s3c24xx_dma_chan *s3cchan = to_s3c24xx_dma_chan(chan);
> + struct s3c24xx_dma_engine *s3cdma = s3cchan->host;
> + unsigned long flags;
> + int ret = 0;
> +
> + /* Controls applicable to inactive channels */
> + if (cmd == DMA_SLAVE_CONFIG)
> + return s3c24xx_dma_set_runtime_config(s3cchan,
> + (struct dma_slave_config *)arg);
> +
> + /*
> + * Anything succeeds on channels with no physical allocation and
> + * no queued transfers.
> + */
> + spin_lock_irqsave(&s3cchan->vc.lock, flags);
> + if (!s3cchan->phy && !s3cchan->at) {
> + spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
> + return 0;
this can be bad cmd or terminating already closed channel, return 0 doesnt make
sense
> + }
> +
> + switch (cmd) {
> + case DMA_TERMINATE_ALL:
why not DMA_SLAVE_CONFIG in this switch to make it look simpler...
> +static enum dma_status s3c24xx_dma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> + struct s3c24xx_dma_chan *s3cchan = to_s3c24xx_dma_chan(chan);
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> + enum dma_status ret;
> + size_t bytes = 0;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + if (ret == DMA_SUCCESS)
> + return ret;
> +
> + /*
> + * There's no point calculating the residue if there's
> + * no txstate to store the value.
> + */
> + if (!txstate)
> + return ret;
> +
> + spin_lock_irqsave(&s3cchan->vc.lock, flags);
> + ret = dma_cookie_status(chan, cookie, txstate);
you are checking the cookie status twice, if you check txstate (which should be
valid anyways), then you avoid the duplication
> + if (ret != DMA_SUCCESS) {
> + struct s3c24xx_txd *txd;
> + struct s3c24xx_sg *dsg;
> +
> + vd = vchan_find_desc(&s3cchan->vc, cookie);
> + if (vd) {
> + /* On the issued list, so hasn't been processed yet */
> + txd = to_s3c24xx_txd(&vd->tx);
> +
> + list_for_each_entry(dsg, &txd->dsg_list, node)
> + bytes += dsg->len;
> + } else {
> + /*
> + * Currently running, so sum over the pending sg's and
> + * the currently active one.
> + */
> + txd = s3cchan->at;
> +
> + dsg = list_entry(txd->at, struct s3c24xx_sg, node);
> + list_for_each_entry_from(dsg, &txd->dsg_list, node);
> + bytes += dsg->len;
> +
> + bytes += s3c24xx_dma_getbytes_chan(s3cchan);
> + }
> + }
> + spin_unlock_irqrestore(&s3cchan->vc.lock, flags);
> +
> + /*
> + * This cookie not complete yet
> + * Get number of bytes left in the active transactions and queue
> + */
> + dma_set_residue(txstate, bytes);
> +
> + /* Whether waiting or running, we're in progress */
> + return ret;
> +}
> +
> +/*
> + * Initialize a descriptor to be used by memcpy submit
> + */
> +static struct dma_async_tx_descriptor *s3c24xx_dma_prep_memcpy(
> + struct dma_chan *chan, dma_addr_t dest, dma_addr_t src,
> + size_t len, unsigned long flags)
> +{
> + struct s3c24xx_dma_chan *s3cchan = to_s3c24xx_dma_chan(chan);
> + struct s3c24xx_dma_engine *s3cdma = s3cchan->host;
> + struct s3c24xx_txd *txd;
> + struct s3c24xx_sg *dsg;
> +
> + dev_dbg(&s3cdma->pdev->dev, "prepare memcpy of %d bytes from %s\n",
> + len, s3cchan->name);
> +
> + if ((len & S3C24XX_DCON_TC_MASK) != len) {
> + dev_err(&s3cdma->pdev->dev, "memcpy size %d to large\n"
Re: [PATCH 00/18] ARM: s3c64xx: Let amba-pl08x driver handle DMA
On Sun, Aug 11, 2013 at 07:59:12PM +0200, Tomasz Figa wrote: > This is first non-RFC version of my patches extending support of > amba-pl08x DMA engine driver to PL080S DMA engine (PL080 modified by > Samsung) found in Samsung S3C64xx SoCs. > > Due to changes scattered across different areas of kernel, patches are > based on merged 3 branches: > - for-next of Kgene's Samsung tree, > - clk-next of Mike's clock tree, > - next of Vinod's slave DMA tree. > > To ease testing I have prepared a branch in my private tree for anyone > willing to check the patches out: > git://github.com/tom3q/linux.git v3.12-pl080 > > Dependencies (already applied in my branch): > - for patches 14 and 16 - CCF-based clock driver for s3c64xx. > > Some of the patches not related to the amba-pl08x driver itself > can be likely applied into appropriate trees separately, namely: > - 09/18 - ASoC: Samsung: Do not queue cyclic buffers multiple times, > - 14/18 - clk: samsung: s3c64xx: Add aliases for DMA clocks. > > After patch 14/18, both old and new DMA drivers can be supported on > S3C64xx, depending on Kconfig options. Patches 15-18 remove the old driver > leaving support only for the generic pl08x driver. Feel free to drop those > patches for now if we want more testing, but I don't suspect any problems. > > On S3C64xx-based Mini6410 and SMDK6410 boards, with I2S audio > playback and capture (including full duplex operation) and also SPI > using spidev: Applied 1-8 to slave-dma tree, thanks ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] dmaengine: add driver for Samsung s3c24xx SoCs
On Tue, Aug 20, 2013 at 10:23:49AM +0200, Heiko Stübner wrote:
> Hi Vinod,
>
> Am Montag, 19. August 2013, 06:48:12 schrieb Vinod Koul:
> > On Wed, Aug 14, 2013 at 02:00:25PM +0200, Heiko Stübner wrote:
> > > This adds a new driver to support the s3c24xx dma using the dmaengine
> > > and makes the old one in mach-s3c24xx obsolete in the long run.
> > >
> > > Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> > > with numerous virtual channels being mapped to a lot less physical ones.
> > > The driver therefore borrows a lot from the amba-pl08x driver in this
> > > regard. Functionality-wise the driver gains a memcpy ability in addition
> > > to the slave_sg one.
> >
> > If that is the case why cant we have this driver supported from pl08x
> > driver? If the delta is only mapping then can that be seprated or both
> > mapping hanlded? Maybe you and Linus have already though about that?
>
> Yes we have ... As Tomasz has already written the hardware itself is very
> much
> different. It's only the concept of mapping virtual channels to physical
> channels that is somehow similar.
>
> It seems my patch message is lacking in making this clearer ;-) .
The above made me believ they are similar contrlllers with differnt mapping!,
hence the question...
> > > +#define DMASKTRIG_STOP (1 << 2)
> > > +#define DMASKTRIG_ON (1 << 1)
> > > +#define DMASKTRIG_SWTRIG (1 << 0)
> > > +
> > > +#define DMAREQSEL(0x24)
> > > +#define DMAREQSEL_HW (1 << 0)
> >
> > This is proper namespacing...
>
> Hmm, I don't understand meaning of this sentence. Is it a suggestion to
> change
> anything?
Sorry above should be read as "this need proper namespacing". The macros like
DMAREQSEL asre farliy egneric and can collide with others. SO the recommendation
is to use something like S3_DMAREQSEL etc
> > > +static irqreturn_t s3c24xx_dma_irq(int irq, void *data)
> > > +{
> > > + struct s3c24xx_dma_phy *phy = data;
> > > + struct s3c24xx_dma_chan *s3cchan = phy->serving;
> > > + struct s3c24xx_txd *txd;
> > > +
> > > + dev_dbg(&phy->host->pdev->dev, "interrupt on channel %d\n", phy->id);
> > > +
> > > + if (!s3cchan) {
> > > + dev_err(&phy->host->pdev->dev, "interrupt on unused channel
> %d\n",
> > > + phy->id);
> > > + return IRQ_NONE;
> >
> > hmmm, these channles belong to you. So if one of them is behvaing badly,
> > then not handling the interrupt will make things worse...
>
> hmm ... I'm not sure what a valid handling would be for this.
>
> The interrupt is only asserted when a transfer is completed - there are no
> other interrupt-triggers. But when phy->serving is NULL, this also means that
> the clock of the channel is disabled at this time. So this _should_ never
> happen.
if that is the case we dont need above, but you added that just for the small
iota of if
> And as written above, the interrupt is only triggered when a transfer was
> completed and the channel is idle again, so if there is no virtual channel
> being served, there is nothing else to do.
But if we do get such an interrupt, it means:
a) bug in SW
b) erratic hw behaviour
if you handle and dump the issue at least you have recovered. Rather than
returning and controller asserting interrupt again and again as it is not
cleared.
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] dmaengine: add driver for Samsung s3c24xx SoCs
On Wed, Aug 14, 2013 at 02:00:25PM +0200, Heiko Stübner wrote:
> This adds a new driver to support the s3c24xx dma using the dmaengine
> and makes the old one in mach-s3c24xx obsolete in the long run.
>
> Conceptually the s3c24xx-dma feels like a distant relative of the pl08x
> with numerous virtual channels being mapped to a lot less physical ones.
> The driver therefore borrows a lot from the amba-pl08x driver in this
> regard. Functionality-wise the driver gains a memcpy ability in addition
> to the slave_sg one.
If that is the case why cant we have this driver supported from pl08x driver? If
the delta is only mapping then can that be seprated or both mapping hanlded?
Maybe you and Linus have already though about that?
> The driver supports both the method for requesting the peripheral used
> by SoCs before the S3C2443 and the different method for S3C2443 and later.
>
> On earlier SoCs the hardware channels usable for specific peripherals is
> constrainted while on later SoCs all channels can be used for any peripheral.
>
> Tested on a s3c2416-based board, memcpy using the dmatest module and
> slave_sg partially using the spi-s3c64xx driver.
>
> Signed-off-by: Heiko Stuebner
> +#define DISRC(0x00)
> +#define DISRCC (0x04)
> +#define DISRCC_INC_INCREMENT (0 << 0)
> +#define DISRCC_INC_FIXED (1 << 0)
> +#define DISRCC_LOC_AHB (0 << 1)
> +#define DISRCC_LOC_APB (1 << 1)
> +
> +#define DIDST(0x08)
> +#define DIDSTC (0x0C)
> +#define DIDSTC_INC_INCREMENT (0 << 0)
> +#define DIDSTC_INC_FIXED (1 << 0)
> +#define DIDSTC_LOC_AHB (0 << 1)
> +#define DIDSTC_LOC_APB (1 << 1)
> +#define DIDSTC_INT_TC0 (0 << 2)
> +#define DIDSTC_INT_RELOAD(1 << 2)
> +
> +#define DCON (0x10)
> +
> +#define DCON_TC_MASK 0xf
> +#define DCON_DSZ_BYTE(0 << 20)
> +#define DCON_DSZ_HALFWORD(1 << 20)
> +#define DCON_DSZ_WORD(2 << 20)
> +#define DCON_DSZ_MASK(3 << 20)
> +#define DCON_DSZ_SHIFT 20
> +#define DCON_AUTORELOAD (0 << 22)
> +#define DCON_NORELOAD(1 << 22)
> +#define DCON_HWTRIG (1 << 23)
> +#define DCON_HWSRC_SHIFT 24
> +#define DCON_SERV_SINGLE (0 << 27)
> +#define DCON_SERV_WHOLE (1 << 27)
> +#define DCON_TSZ_UNIT(0 << 28)
> +#define DCON_TSZ_BURST4 (1 << 28)
> +#define DCON_INT (1 << 29)
> +#define DCON_SYNC_PCLK (0 << 30)
> +#define DCON_SYNC_HCLK (1 << 30)
> +#define DCON_DEMAND (0 << 31)
> +#define DCON_HANDSHAKE (1 << 31)
> +
> +#define DSTAT(0x14)
> +#define DSTAT_STAT_BUSY (1 << 20)
> +#define DSTAT_CURRTC_MASK0xf
> +
> +#define DMASKTRIG(0x20)
> +#define DMASKTRIG_STOP (1 << 2)
> +#define DMASKTRIG_ON (1 << 1)
> +#define DMASKTRIG_SWTRIG (1 << 0)
> +
> +#define DMAREQSEL(0x24)
> +#define DMAREQSEL_HW (1 << 0)
This is proper namespacing...
> +static int s3c24xx_dma_set_runtime_config(struct s3c24xx_dma_chan *s3cchan,
> + struct dma_slave_config *config)
> +{
> + if (!s3cchan->slave)
> + return -EINVAL;
> +
> + /* Reject definitely invalid configurations */
> + if (config->src_addr_width == DMA_SLAVE_BUSWIDTH_8_BYTES ||
> + config->dst_addr_width == DMA_SLAVE_BUSWIDTH_8_BYTES)
> + return -EINVAL;
> +
> + s3cchan->cfg = *config;
you are takinga ref to client pointer without a clue on when that would be
freed. I dont think its a good idea!
> +static irqreturn_t s3c24xx_dma_irq(int irq, void *data)
> +{
> + struct s3c24xx_dma_phy *phy = data;
> + struct s3c24xx_dma_chan *s3cchan = phy->serving;
> + struct s3c24xx_txd *txd;
> +
> + dev_dbg(&phy->host->pdev->dev, "interrupt on channel %d\n", phy->id);
> +
> + if (!s3cchan) {
> + dev_err(&phy->host->pdev->dev, "interrupt on unused channel
> %d\n",
> + phy->id);
> + return IRQ_NONE;
hmmm, these channles belong to you. So if one of them is behvaing badly, then
not handling the interrupt will make things worse...
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 05/12] dmaengine: PL08x: Add support for different maximum transfer size
On Sat, Jun 22, 2013 at 10:42:37PM +0200, Tomasz Figa wrote: > PL080S has separate register to store transfer size in, allowing single > transfer to be much larger than in standard PL080. > > This patch makes the amba-pl08x driver aware of this and removes writing > transfer size to reserved bits of CH_CONTROL register on PL080S, which > was not a problem witn transfer sizes fitting the original bitfield > of PL080, but now would overwrite other fields. > > Signed-off-by: Tomasz Figa Acked by: Vinod Koul -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 02/12] dmaengine: PL08x: Add support for different offset of CONFIG register
On Sat, Jun 22, 2013 at 10:42:34PM +0200, Tomasz Figa wrote: > Some variants of PL08x (namely PL080S, found in Samsung S3C64xx SoCs) > have CONFIG register at different offset. This patch makes the driver > use offset from vendor data struct. > > Signed-off-by: Tomasz Figa Acked-by: Vinod Koul -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 01/12] dmaengine: PL08x: Refactor pl08x_getbytes_chan() to lower indentation
On Sat, Jun 22, 2013 at 10:42:33PM +0200, Tomasz Figa wrote:
> Further patch will introduce support for PL080S, which requires some
> things to be done conditionally, thus increasing indentation level of
> some functions even more.
>
> This patch reduces indentation level of pl08x_getbytes_chan() function
> by inverting several conditions and returning from function wherever
> possible.
>
> Signed-off-by: Tomasz Figa
> ---
> drivers/dma/amba-pl08x.c | 53
> ++--
> 1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index 06fe45c..6a12392 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -469,47 +469,52 @@ static inline u32 get_bytes_in_cctl(u32 cctl)
> /* The channel should be paused when calling this */
> static u32 pl08x_getbytes_chan(struct pl08x_dma_chan *plchan)
> {
> + struct pl08x_lli *llis_va;
> struct pl08x_phy_chan *ch;
> + dma_addr_t llis_bus;
> struct pl08x_txd *txd;
> - size_t bytes = 0;
> + size_t bytes;
> + int index;
> + u32 clli;
>
> ch = plchan->phychan;
> txd = plchan->at;
>
> + if (!ch || !txd)
> + return 0;
shouldnt this be err return
> +
> /*
>* Follow the LLIs to get the number of remaining
>* bytes in the currently active transaction.
>*/
> - if (ch && txd) {
> - u32 clli = readl(ch->base + PL080_CH_LLI) & ~PL080_LLI_LM_AHB2;
> + clli = readl(ch->base + PL080_CH_LLI) & ~PL080_LLI_LM_AHB2;
>
> - /* First get the remaining bytes in the active transfer */
> - bytes = get_bytes_in_cctl(readl(ch->base + PL080_CH_CONTROL));
> + /* First get the remaining bytes in the active transfer */
> + bytes = get_bytes_in_cctl(readl(ch->base + PL080_CH_CONTROL));
>
> - if (clli) {
> - struct pl08x_lli *llis_va = txd->llis_va;
> - dma_addr_t llis_bus = txd->llis_bus;
> - int index;
> + if (!clli)
> + return bytes;
>
> - BUG_ON(clli < llis_bus || clli >= llis_bus +
> + llis_va = txd->llis_va;
> + llis_bus = txd->llis_bus;
> +
> + BUG_ON(clli < llis_bus || clli >= llis_bus +
> sizeof(struct pl08x_lli) * MAX_NUM_TSFR_LLIS);
IMO BUG_ON is too much for this. Perhaps returning error and logging error would
be okay
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] DMA: PL330: Add check if device tree compatible
On Tue, Apr 02, 2013 at 04:46:45PM +0530, Padmavathi Venna wrote:
> This patch register the dma controller with generic dma helpers only
> in DT case. This also adds some extra error handling in the driver.
>
> Signed-off-by: Padmavathi Venna
> Reported-by: Sachin Kamat
> ---
>
> Based on Vinod Koul next branch.
>
> Changes since V2:
> - Removed pl330_free_chan_resources for error handling in probe.
>
> Changes since V1:
> - return silently when of_dma_controller_register fails, as
> suggested by Arnd.
>
> drivers/dma/pl330.c | 33 ++---
> 1 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 345e2a1..fa388c1c 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2882,7 +2882,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> {
> struct dma_pl330_platdata *pdat;
> struct dma_pl330_dmac *pdmac;
> - struct dma_pl330_chan *pch;
> + struct dma_pl330_chan *pch, *_p;
> struct pl330_info *pi;
> struct dma_device *pd;
> struct resource *res;
> @@ -2984,7 +2984,17 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> ret = dma_async_device_register(pd);
> if (ret) {
> dev_err(&adev->dev, "unable to register DMAC\n");
> - goto probe_err2;
> + goto probe_err3;
> + }
> +
> + if (adev->dev.of_node) {
> + ret = of_dma_controller_register(adev->dev.of_node,
> + of_dma_pl330_xlate, pdmac);
> + if (ret) {
> + dev_err(&adev->dev,
> + "unable to register DMA to the generic"
> + "DT DMA helpers\n");
This could have been in a single line
I have applied it to fixes.
--
~Vinod
> + }
> }
>
> dev_info(&adev->dev,
> @@ -2995,16 +3005,15 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> pi->pcfg.data_bus_width / 8, pi->pcfg.num_chan,
> pi->pcfg.num_peri, pi->pcfg.num_events);
>
> - ret = of_dma_controller_register(adev->dev.of_node,
> - of_dma_pl330_xlate, pdmac);
> - if (ret) {
> - dev_err(&adev->dev,
> - "unable to register DMA to the generic DT DMA helpers\n");
> - goto probe_err2;
> - }
> -
> return 0;
> +probe_err3:
> + amba_set_drvdata(adev, NULL);
>
> + /* Idle the DMAC */
> + list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels,
> + chan.device_node)
> + /* Remove the channel */
> + list_del(&pch->chan.device_node);
> probe_err2:
> pl330_del(pi);
> probe_err1:
> @@ -3023,8 +3032,10 @@ static int pl330_remove(struct amba_device *adev)
> if (!pdmac)
> return 0;
>
> - of_dma_controller_free(adev->dev.of_node);
> + if (adev->dev.of_node)
> + of_dma_controller_free(adev->dev.of_node);
>
> + dma_async_device_unregister(&pdmac->ddma);
> amba_set_drvdata(adev, NULL);
>
> /* Idle the DMAC */
> --
> 1.7.4.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] DMA: PL330: Add check if device tree compatible
On Mon, Apr 01, 2013 at 08:13:31AM -0500, Rob Herring wrote:
> On Thu, Mar 21, 2013 at 4:39 AM, Vinod Koul wrote:
> > On Tue, Mar 05, 2013 at 02:55:31PM +0530, Padmavathi Venna wrote:
> >> This patch register the dma controller with generic dma helpers only
> >> in DT case. This also adds some extra error handling in the driver.
> >>
> >> Signed-off-by: Padmavathi Venna
> >> Reported-by: Sachin Kamat
>
> What's the status on this? It is needed for 3.9 to fix pl330 on highbank.
Well the status is that submitter has been rude. I had few questions and
comments on this patch and they are yet to be addressed.
--
~Vinod
>
> Rob
>
> >> ---
> >>
> >> Based on Vinod Koul next branch.
> >>
> >> Changes since V1:
> >> - return silently when of_dma_controller_register fails, as
> >> suggested by Arnd.
> >>
> >> drivers/dma/pl330.c | 38 +++---
> >> 1 files changed, 27 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> >> index 7181531..5dbc594 100644
> >> --- a/drivers/dma/pl330.c
> >> +++ b/drivers/dma/pl330.c
> >> @@ -2882,7 +2882,7 @@ pl330_probe(struct amba_device *adev, const struct
> >> amba_id *id)
> >> {
> >> struct dma_pl330_platdata *pdat;
> >> struct dma_pl330_dmac *pdmac;
> >> - struct dma_pl330_chan *pch;
> >> + struct dma_pl330_chan *pch, *_p;
> >> struct pl330_info *pi;
> >> struct dma_device *pd;
> >> struct resource *res;
> >> @@ -2984,7 +2984,16 @@ pl330_probe(struct amba_device *adev, const struct
> >> amba_id *id)
> >> ret = dma_async_device_register(pd);
> >> if (ret) {
> >> dev_err(&adev->dev, "unable to register DMAC\n");
> >> - goto probe_err2;
> >> + goto probe_err3;
> >> + }
> >> +
> >> + if (adev->dev.of_node) {
> >> + ret = of_dma_controller_register(adev->dev.of_node,
> >> + of_dma_pl330_xlate, pdmac);
> >> + if (ret) {
> >> + dev_err(&adev->dev,
> >> + "unable to register DMA to the generic DT DMA
> >> helpers\n");
> > Indent?
> >> + }
> >> }
> >>
> >> dev_info(&adev->dev,
> >> @@ -2995,16 +3004,21 @@ pl330_probe(struct amba_device *adev, const struct
> >> amba_id *id)
> >> pi->pcfg.data_bus_width / 8, pi->pcfg.num_chan,
> >> pi->pcfg.num_peri, pi->pcfg.num_events);
> >>
> >> - ret = of_dma_controller_register(adev->dev.of_node,
> >> - of_dma_pl330_xlate, pdmac);
> >> - if (ret) {
> >> - dev_err(&adev->dev,
> >> - "unable to register DMA to the generic DT DMA helpers\n");
> >> - goto probe_err2;
> >> - }
> >> -
> >> return 0;
> >> +probe_err3:
> >> + amba_set_drvdata(adev, NULL);
> >>
> >> + /* Idle the DMAC */
> >> + list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels,
> >> + chan.device_node) {
> >> +
> >> + /* Remove the channel */
> >> + list_del(&pch->chan.device_node);
> >> +
> >> + /* Flush the channel */
> >> + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
> >> + pl330_free_chan_resources(&pch->chan);
> > free_chan for error handling in probe?
> >
> >> + }
> >> probe_err2:
> >> pl330_del(pi);
> >> probe_err1:
> >> @@ -3023,8 +3037,10 @@ static int pl330_remove(struct amba_device *adev)
> >> if (!pdmac)
> >> return 0;
> >>
> >> - of_dma_controller_free(adev->dev.of_node);
> >> + if (adev->dev.of_node)
> >> + of_dma_controller_free(adev->dev.of_node);
> >>
> >> + dma_async_device_unregister(&pdmac->ddma);
> >> amba_set_drvdata(adev, NULL);
> >>
> >> /* Idle the DMAC */
> >> --
> >> 1.7.4.4
> >>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] DMA: PL330: Add check if device tree compatible
On Tue, Mar 05, 2013 at 02:55:31PM +0530, Padmavathi Venna wrote:
> This patch register the dma controller with generic dma helpers only
> in DT case. This also adds some extra error handling in the driver.
>
> Signed-off-by: Padmavathi Venna
> Reported-by: Sachin Kamat
> ---
>
> Based on Vinod Koul next branch.
>
> Changes since V1:
> - return silently when of_dma_controller_register fails, as
> suggested by Arnd.
>
> drivers/dma/pl330.c | 38 +++---
> 1 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 7181531..5dbc594 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2882,7 +2882,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> {
> struct dma_pl330_platdata *pdat;
> struct dma_pl330_dmac *pdmac;
> - struct dma_pl330_chan *pch;
> + struct dma_pl330_chan *pch, *_p;
> struct pl330_info *pi;
> struct dma_device *pd;
> struct resource *res;
> @@ -2984,7 +2984,16 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> ret = dma_async_device_register(pd);
> if (ret) {
> dev_err(&adev->dev, "unable to register DMAC\n");
> - goto probe_err2;
> + goto probe_err3;
> + }
> +
> + if (adev->dev.of_node) {
> + ret = of_dma_controller_register(adev->dev.of_node,
> + of_dma_pl330_xlate, pdmac);
> + if (ret) {
> + dev_err(&adev->dev,
> + "unable to register DMA to the generic DT DMA
> helpers\n");
Indent?
> + }
> }
>
> dev_info(&adev->dev,
> @@ -2995,16 +3004,21 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> pi->pcfg.data_bus_width / 8, pi->pcfg.num_chan,
> pi->pcfg.num_peri, pi->pcfg.num_events);
>
> - ret = of_dma_controller_register(adev->dev.of_node,
> - of_dma_pl330_xlate, pdmac);
> - if (ret) {
> - dev_err(&adev->dev,
> - "unable to register DMA to the generic DT DMA helpers\n");
> - goto probe_err2;
> - }
> -
> return 0;
> +probe_err3:
> + amba_set_drvdata(adev, NULL);
>
> + /* Idle the DMAC */
> + list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels,
> + chan.device_node) {
> +
> + /* Remove the channel */
> + list_del(&pch->chan.device_node);
> +
> + /* Flush the channel */
> + pl330_control(&pch->chan, DMA_TERMINATE_ALL, 0);
> + pl330_free_chan_resources(&pch->chan);
free_chan for error handling in probe?
> + }
> probe_err2:
> pl330_del(pi);
> probe_err1:
> @@ -3023,8 +3037,10 @@ static int pl330_remove(struct amba_device *adev)
> if (!pdmac)
> return 0;
>
> - of_dma_controller_free(adev->dev.of_node);
> + if (adev->dev.of_node)
> + of_dma_controller_free(adev->dev.of_node);
>
> + dma_async_device_unregister(&pdmac->ddma);
> amba_set_drvdata(adev, NULL);
>
> /* Idle the DMAC */
> --
> 1.7.4.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: EXYNOS: support burst mode for for dev-to-mem and dev-to-mem transmit
On Tue, Feb 19, 2013 at 11:02:09AM +0900, Boojin Kim wrote:
> This patch adds to support burst mode for for dev-to-mem and dev-to-mem
> transmit
>
> Signed-off-by: Boojin Kim
> ---
> arch/arm/plat-samsung/dma-ops.c | 10 --
> arch/arm/plat-samsung/include/plat/dma-ops.h |1 +
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
> index d088afa..c25e842 100644
> --- a/arch/arm/plat-samsung/dma-ops.c
> +++ b/arch/arm/plat-samsung/dma-ops.c
> @@ -54,14 +54,20 @@ static int samsung_dmadev_config(unsigned ch,
> slave_config.direction = param->direction;
> slave_config.src_addr = param->fifo;
> slave_config.src_addr_width = param->width;
> - slave_config.src_maxburst = 1;
Hi Boojin,
what do you mean by the busrt mode here?
fwiw the meanining of above maxburst is:
@src_maxburst: the maximum number of words (note: words, as in
units of the src_addr_width member, not bytes) that can be sent
in one burst to the device. Typically something like half the
FIFO depth on I/O peripherals so you don't overflow it. This
may or may not be applicable on memory sources.
> + if (param->maxburst)
> + slave_config.src_maxburst = param->maxburst;
> + else
> + slave_config.src_maxburst = 1;
> dmaengine_slave_config(chan, &slave_config);
> } else if (param->direction == DMA_MEM_TO_DEV) {
> memset(&slave_config, 0, sizeof(struct dma_slave_config));
> slave_config.direction = param->direction;
> slave_config.dst_addr = param->fifo;
> slave_config.dst_addr_width = param->width;
> - slave_config.dst_maxburst = 1;
> + if (param->maxburst)
> + slave_config.dst_maxburst = param->maxburst;
> + else
> + slave_config.dst_maxburst = 1;
> dmaengine_slave_config(chan, &slave_config);
> } else {
> pr_warn("unsupported direction\n");
> diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h
> b/arch/arm/plat-samsung/include/plat/dma-ops.h
> index f5144cd..95893c7 100644
> --- a/arch/arm/plat-samsung/include/plat/dma-ops.h
> +++ b/arch/arm/plat-samsung/include/plat/dma-ops.h
> @@ -35,6 +35,7 @@ struct samsung_dma_prep {
> struct samsung_dma_config {
> enum dma_transfer_direction direction;
> enum dma_slave_buswidth width;
> + u32 maxburst;
> dma_addr_t fifo;
> };
>
> --
> 1.7.5.4
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 0/4] Add generic DMA DT binding support
On Thu, Feb 14, 2013 at 09:10:04AM +0530, Padmavathi Venna wrote: Logically this should have been v5 :) Applied all, thanks -- ~Vinod > Changes since V3: > - Make dma-cells property optional as suggested by Rob Herring > - Add dma-requests and dma-channels properties to DMA controller > as suggested by Arnd for future-proof > - Add Acked-by for some of the patches > > Changes since V2: > - Add new filter function for DT case as suggested by Arnd > - Add xlate as static function > - Use newly added filter function in xlate. > - Add Acked-by for some of the patches > > Changes since V1: > - Address the review comments by Arnd Bergmann as below > - Wording of the properties. > - Pass pdmac as third parameter to of_dma_controller_register > - Filter the dma channel based on channel number and dma_device > > This patch set adds support for generic dma device tree bindings for > Samsung platforms and is dependent on the following patches from > Vinod Koul next branch > 1)of: Add generic device tree DMA helpers > 2)dmaengine: add helper function to request a slave DMA channel > > This patch set is made based on Vinod Koul next branch > > Padmavathi Venna (4): > DMA: PL330: Add new pl330 filter for DT case. > DMA: PL330: Add xlate function > DMA: PL330: Register the DMA controller with the generic DMA helpers > ARM: dts: pl330: Add #dma-cells for generic dma binding support > > .../devicetree/bindings/dma/arm-pl330.txt | 21 +-- > arch/arm/boot/dts/exynos5250.dtsi | 12 > drivers/dma/pl330.c| 64 +++ > 3 files changed, 78 insertions(+), 19 deletions(-) > > -- > 1.7.4.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 0/5] Add generic DMA DT binding support
On Thu, Feb 14, 2013 at 11:12:51AM +0530, Padma Venkat wrote: > Hi Vinod, > > On Thu, Feb 14, 2013 at 10:17 AM, Vinod Koul wrote: > > On Wed, Feb 13, 2013 at 09:52:30AM +0530, Padma Venkat wrote: > >> Hi Vinod, > >> > >> On Tue, Feb 12, 2013 at 8:19 PM, Vinod Koul wrote: > >> > On Mon, Feb 11, 2013 at 02:08:20PM +0530, Padmavathi Venna wrote: > >> > > >> > This looks fine, I have only question. The code seems to assume that > >> > pl330 dma > >> > controller always uses DT. But I dont see that as dependency for pl330. > > Have you checked this? > > Sorry I didn't see this comment previously. I verified on SMDK6410 > board which doesn't have DT support. > I didn't see any build error and it detected the sound card. Okay, sound good... I will try the series now. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 0/5] Add generic DMA DT binding support
On Wed, Feb 13, 2013 at 09:52:30AM +0530, Padma Venkat wrote: > Hi Vinod, > > On Tue, Feb 12, 2013 at 8:19 PM, Vinod Koul wrote: > > On Mon, Feb 11, 2013 at 02:08:20PM +0530, Padmavathi Venna wrote: > > > > This looks fine, I have only question. The code seems to assume that pl330 > > dma > > controller always uses DT. But I dont see that as dependency for pl330. Have you checked this? > > Something tells me withot OF the driver may not build, have you checked it? > > > >> This patch set adds support for generic dma device tree bindings for > >> Samsung platforms and is dependent on the following patches from > >> Vinod Koul next branch > >> 1)of: Add generic device tree DMA helpers > >> 2)dmaengine: add helper function to request a slave DMA channel > >> > >> This patch set is made based Mark Brown next branch > > Is this targetted for ASoC tree, if so why? It would fail to build if > > applied > > there > > I have done this for my testing purpose (I expected all will directly > apply in your branch). But 3rd patch is not applied directly. I will > resend the patches after re-basing on your tree. 5th patch has to go > in ASoC tree because it has dependency on "ARM: SAMSUNG: Make dma > request compatible to generic dma bindings" which is there in ASoC > tree. > > Hi Mark, > > Can you please take the 5th patch in your tree(if there is no issue) > or should I resend it as a separate patch? > > > > > -- > > ~Vinod > >> > >> Padmavathi Venna (5): > >> DMA: PL330: Add new pl330 filter for DT case. > >> DMA: PL330: Add xlate function > >> DMA: PL330: Register the DMA controller with the generic DMA helpers > >> ARM: dts: pl330: Add #dma-cells for generic dma binding support > >> ARM: SAMSUNG: dma: Remove unnecessary code > >> > >> .../devicetree/bindings/dma/arm-pl330.txt | 21 +-- > >> arch/arm/boot/dts/exynos5250.dtsi | 12 > >> arch/arm/mach-s3c24xx/include/mach/dma.h |1 - > >> arch/arm/mach-s3c64xx/include/mach/dma.h |1 - > >> arch/arm/plat-samsung/dma-ops.c| 10 +--- > >> arch/arm/plat-samsung/include/plat/dma-ops.h |1 - > >> arch/arm/plat-samsung/include/plat/dma-pl330.h |1 - > >> drivers/dma/pl330.c| 64 > >> +++ > >> 8 files changed, 79 insertions(+), 32 deletions(-) > >> > >> -- > >> 1.7.4.4 > >> > Thanks > Padma -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 0/5] Add generic DMA DT binding support
On Mon, Feb 11, 2013 at 02:08:20PM +0530, Padmavathi Venna wrote: This looks fine, I have only question. The code seems to assume that pl330 dma controller always uses DT. But I dont see that as dependency for pl330. Something tells me withot OF the driver may not build, have you checked it? > This patch set adds support for generic dma device tree bindings for > Samsung platforms and is dependent on the following patches from > Vinod Koul next branch > 1)of: Add generic device tree DMA helpers > 2)dmaengine: add helper function to request a slave DMA channel > > This patch set is made based Mark Brown next branch Is this targetted for ASoC tree, if so why? It would fail to build if applied there -- ~Vinod > > Padmavathi Venna (5): > DMA: PL330: Add new pl330 filter for DT case. > DMA: PL330: Add xlate function > DMA: PL330: Register the DMA controller with the generic DMA helpers > ARM: dts: pl330: Add #dma-cells for generic dma binding support > ARM: SAMSUNG: dma: Remove unnecessary code > > .../devicetree/bindings/dma/arm-pl330.txt | 21 +-- > arch/arm/boot/dts/exynos5250.dtsi | 12 > arch/arm/mach-s3c24xx/include/mach/dma.h |1 - > arch/arm/mach-s3c64xx/include/mach/dma.h |1 - > arch/arm/plat-samsung/dma-ops.c| 10 +--- > arch/arm/plat-samsung/include/plat/dma-ops.h |1 - > arch/arm/plat-samsung/include/plat/dma-pl330.h |1 - > drivers/dma/pl330.c| 64 +++ > 8 files changed, 79 insertions(+), 32 deletions(-) > > -- > 1.7.4.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V6 10/10] dmaengine: Fix compilation error in non-DT case
On Fri, Jan 18, 2013 at 05:17:09PM +0530, Padmavathi Venna wrote:
> Signed-off-by: Padmavathi Venna
> ---
> include/linux/dmaengine.h |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8cd0e25..c88f302 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -992,7 +992,7 @@ static inline struct dma_chan
> *__dma_request_channel(dma_cap_mask_t *mask,
> static inline struct dma_chan *dma_request_slave_channel(struct device *dev,
>char *name)
> {
> - return NULL
> + return NULL;
what tree was this generated against? this was fixed by 678bd8
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Add generic DMA DT binding support
On Fri, Jan 18, 2013 at 05:03:40PM +0530, Padmavathi Venna wrote: > This patch set adds support for generic dma device tree bindings for > Samsung platforms and is dependent on the following patches from > Vinod Koul next branch > 1)of: Add generic device tree DMA helpers > 2)dmaengine: add helper function to request a slave DMA channel Changes look fairly decent. I need somone with better knowldge of DT to akc this before this is applied. Arnd...? > > This patch set is made based Kukjin Kim for-next branch > > Padmavathi Venna (4): > DMA: PL330: Add xlate function > DMA: PL330: Register the DMA controller with the generic DMA helpers > ARM: dts: Add #dma-cells for generic dma binding support > DMA: PL330: Modify pl330 filter based on new generic dma dt bindings. > > .../devicetree/bindings/dma/arm-pl330.txt |9 +++- > arch/arm/boot/dts/exynos5250.dtsi |4 ++ > drivers/dma/pl330.c| 48 > > 3 files changed, 49 insertions(+), 12 deletions(-) > > -- > 1.7.4.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] DMA: PL330: fix locking in pl330_free_chan_resources()
On Fri, 2012-11-30 at 11:59 +0100, Bartlomiej Zolnierkiewicz wrote: > > Acked-by: Jassi Brar > > Vinod/Dan could you please pick this patch for 3.8? Thanks! I will check and queue it up today -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe
On Sat, 2012-10-27 at 15:50 +0530, Inderpal Singh wrote: > Hi Vinod, > > On 26 October 2012 10:15, Vinod Koul wrote: > > On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote: > >> > >> This code will get executed only in case of force removal of the > >> module which was discussed in the first version of the patch at [1]. > >> Now, if we do not have to think about force removal then this patch > >> will go back to the first version. > > But why are you doing force removal of driver even when client is > > holding a reference to you. > > > > What happens when client finally tries to free the channel? > Since we return EBUSY so forced removal won't succeed. Client can free > the channel eventually. And that is my concern. You have forcefully removed the dma module. What happens then? How will the free calll get executed, wont you hit a panic. > > > > > What is the problem you are trying to solve? > >> > > There was a long discussion about it in the first version of the > patch. Allow me to explain it to you. > > The existing driver does DMA_TERMINATE_ALL and frees resources for all > the channels in the _remove function. Which for starters may not be right thing to do. Shouldn't you first make sure client has freed all references to your driver and then only remove. Freeing resources in .remove without keeping client in sync doesn't sound to be good idea to me. > The first version of patch > removed this flushing and freeing of channel resources because they > are not getting allocated in the probe. Jassi pointed out that manual > flushing is needed if a force removal happens and some client is > queued. Then it was agreed that flushing is not needed, instead we > should return EBUSY if client is queued on some channel (this will > happen only in force removal case). Hence this additional check in v2 > version so that force removal does not succeeds if any client is > queued. > > If you think force removal is not a practical scenario and we should > not be bothering about it, this check can be removed and the patch > will go back to first version which just removes flushing and freeing > of channels beacues they are not getting allocated in probe. > > Let me know your view. > > Regards, > Inder > > > >> Let me know your view. > >> > >> [1] https://patchwork.kernel.org/patch/1503171/ > >> > > > > > > -- > > Vinod Koul > > Intel Corp. > > -- Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe
On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote: > > This code will get executed only in case of force removal of the > module which was discussed in the first version of the patch at [1]. > Now, if we do not have to think about force removal then this patch > will go back to the first version. But why are you doing force removal of driver even when client is holding a reference to you. What happens when client finally tries to free the channel? What is the problem you are trying to solve? > > Let me know your view. > > [1] https://patchwork.kernel.org/patch/1503171/ > -- Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DMA: PL330: Add runtime pm support
On Wed, 2012-10-17 at 09:42 +0530, Inderpal Singh wrote:
> At the pl330's probe point the device is already in runtime resume state.
> Hence to manage the device with runtime, the probe should do pm_runtime_put
> and remove should do pm_runtime_get to balance with probe.
>
> And in between, the device is being get/put at alloc_chan_resources and
> free_chan_resources.
>
> Signed-off-by: Inderpal Singh
> ---
> This patch is based on slave-dma's next branch and on top
> of the clean up patches at [1].
>
> [1] https://lkml.org/lkml/2012/10/5/169
>
> drivers/dma/pl330.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 2ee1538..5ae19ea 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dmaengine.h"
> #define PL330_MAX_CHAN 8
> @@ -2384,6 +2385,8 @@ static int pl330_alloc_chan_resources(struct dma_chan
> *chan)
> struct dma_pl330_dmac *pdmac = pch->dmac;
> unsigned long flags;
>
> + pm_runtime_get_sync(pdmac->ddma.dev);
it would be really good if we move this to tx_submit. That way you
resume the dmaengine when actual transactions happen, not when someone
requests a channel.
Yes thats a little more work, though initially we can have this way.
> +
> spin_lock_irqsave(&pch->lock, flags);
>
> dma_cookie_init(chan);
> @@ -2392,6 +2395,7 @@ static int pl330_alloc_chan_resources(struct dma_chan
> *chan)
> pch->pl330_chid = pl330_request_channel(&pdmac->pif);
> if (!pch->pl330_chid) {
> spin_unlock_irqrestore(&pch->lock, flags);
> + pm_runtime_put(pdmac->ddma.dev);
> return -ENOMEM;
> }
>
> @@ -2470,6 +2474,8 @@ static void pl330_free_chan_resources(struct dma_chan
> *chan)
> list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);
>
> spin_unlock_irqrestore(&pch->lock, flags);
> +
> + pm_runtime_put(pch->dmac->ddma.dev);
> }
>
> static enum dma_status
> @@ -2974,6 +2980,8 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> pi->pcfg.data_bus_width / 8, pi->pcfg.num_chan,
> pi->pcfg.num_peri, pi->pcfg.num_events);
>
> + pm_runtime_put(pd->dev);
pm_runtime_put_noidle is better. Yous should also do pm_runtime_allow
here and pm_runtime_forbid in your remove.
> +
> return 0;
>
> probe_err5:
> @@ -3017,6 +3025,8 @@ static int __devexit pl330_remove(struct amba_device
> *adev)
> return -EBUSY;
> }
>
> + pm_runtime_get(pdmac->ddma.dev);
> +
> dma_async_device_unregister(&pdmac->ddma);
>
> amba_set_drvdata(adev, NULL);
--
Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] DMA: PL330: Free memory allocated for peripheral channels
On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
> The allocated memory for peripheral channels is not being freed upon
> failure in probe and in module's remove funtion. It will lead to memory
> leakage. Hence free the allocated memory.
>
> Signed-off-by: Inderpal Singh
> ---
> drivers/dma/pl330.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 2ebd4cd..10c6b6a 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> ret = dma_async_device_register(pd);
> if (ret) {
> dev_err(&adev->dev, "unable to register DMAC\n");
> - goto probe_err4;
> + goto probe_err5;
> }
>
> dev_info(&adev->dev,
> @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
>
> return 0;
>
> +probe_err5:
> + kfree(pdmac->peripherals);
> probe_err4:
> pl330_del(pi);
> probe_err3:
> @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device
> *adev)
> res = &adev->res;
> release_mem_region(res->start, resource_size(res));
>
> + kfree(pdmac->peripherals);
> kfree(pdmac);
>
> return 0;
This looks fine, but if you use devm_ functions then you dont need to do
all this.
Can you do that conversion instead?
--
Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function
On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote: > unregister dma_device in module's remove function. > > Signed-off-by: Inderpal Singh > --- > drivers/dma/pl330.c |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 4b7a34d..e7dc040 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device > *adev) > return -EBUSY; > } > > + dma_async_device_unregister(&pdmac->ddma); > + > amba_set_drvdata(adev, NULL); > > list_for_each_entry_safe(pch, _p, &pdmac->ddma.channels, Ok with this one :) Tried applying but didn't work out. You would need to regenerate this one. Thanks -- Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe
On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
> Since peripheral channel resources are not being allocated at probe,
> no need to flush the channels and free the resources in remove function.
> In case, the channel is in use by some client, return EBUSY.
>
> Signed-off-by: Inderpal Singh
> ---
> drivers/dma/pl330.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index bf71ff7..4b7a34d 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device
> *adev)
> if (!pdmac)
> return 0;
>
> + /* check if any client is using any channel */
> + list_for_each_entry(pch, &pdmac->ddma.channels,
> + chan.device_node) {
> +
> + if (pch->chan.client_count)
> + return -EBUSY;
> + }
> +
> while (!list_empty(&pdmac->desc_pool)) {
Did you get this code executed?
I think No.
The dmaengine holds the reference to channels, so if they are in use and
not freed by client your remove wont be called. So this check is
redundant
--
Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] DMA: PL330: Change allocation method to properly free DMA descriptors
On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
> In probe, memory for multiple DMA descriptors were being allocated at once
> and then it was being split and added into DMA pool one by one. The address
> of this memory allocation is not being saved anywhere. To free this memory,
> the address is required. Initially the first node of the pool will be
> pointed by this address but as we use this pool the descs will shuffle and
> hence we will lose the track of the address.
>
> This patch does following:
>
> 1. Allocates DMA descs one by one and then adds them to pool so that all
>descs can be fetched and memory freed one by one. This way runtime
>added descs can also be freed.
> 2. Free DMA descs in case of error in probe and in module's remove function
For probe, again you have cleaner code by using devm_kzalloc.
On 1, if we use the devm_kzalloc then we don't need to allocate in
chunks right?
>
> Signed-off-by: Inderpal Singh
> ---
> drivers/dma/pl330.c | 35 +--
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 10c6b6a..bf71ff7 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2541,20 +2541,20 @@ static int add_desc(struct dma_pl330_dmac *pdmac,
> gfp_t flg, int count)
> if (!pdmac)
> return 0;
>
> - desc = kmalloc(count * sizeof(*desc), flg);
> - if (!desc)
> - return 0;
> + for (i = 0; i < count; i++) {
> + desc = kmalloc(sizeof(*desc), flg);
> + if (!desc)
> + break;
> + _init_desc(desc);
>
> - spin_lock_irqsave(&pdmac->pool_lock, flags);
> + spin_lock_irqsave(&pdmac->pool_lock, flags);
>
> - for (i = 0; i < count; i++) {
> - _init_desc(&desc[i]);
> - list_add_tail(&desc[i].node, &pdmac->desc_pool);
> - }
> + list_add_tail(&desc->node, &pdmac->desc_pool);
>
> - spin_unlock_irqrestore(&pdmac->pool_lock, flags);
> + spin_unlock_irqrestore(&pdmac->pool_lock, flags);
> + }
>
> - return count;
> + return i;
> }
>
> static struct dma_pl330_desc *
> @@ -2857,6 +2857,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> struct dma_pl330_platdata *pdat;
> struct dma_pl330_dmac *pdmac;
> struct dma_pl330_chan *pch;
> + struct dma_pl330_desc *desc;
> struct pl330_info *pi;
> struct dma_device *pd;
> struct resource *res;
> @@ -2978,6 +2979,12 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> probe_err5:
> kfree(pdmac->peripherals);
> probe_err4:
> + while (!list_empty(&pdmac->desc_pool)) {
> + desc = list_entry(pdmac->desc_pool.next,
> + struct dma_pl330_desc, node);
> + list_del(&desc->node);
> + kfree(desc);
> + }
> pl330_del(pi);
> probe_err3:
> free_irq(irq, pi);
> @@ -2994,6 +3001,7 @@ static int __devexit pl330_remove(struct amba_device
> *adev)
> {
> struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
> struct dma_pl330_chan *pch, *_p;
> + struct dma_pl330_desc *desc;
> struct pl330_info *pi;
> struct resource *res;
> int irq;
> @@ -3015,6 +3023,13 @@ static int __devexit pl330_remove(struct amba_device
> *adev)
> pl330_free_chan_resources(&pch->chan);
> }
>
> + while (!list_empty(&pdmac->desc_pool)) {
> + desc = list_entry(pdmac->desc_pool.next,
> + struct dma_pl330_desc, node);
> + list_del(&desc->node);
> + kfree(desc);
> + }
> +
> pi = &pdmac->pif;
>
> pl330_del(pi);
--
Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe
On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote: > If we fail pl330_remove while some client is queued, the force unload > will fail and the > force unload will lose its purpose. > How about conditionally DMA_TERMINATE_ALL and free resources like > below ? Why would you want to remove the driver when it is doing something useful? You have to ensure driver is not doing anything. What is point here? -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] DMA: PL330: Clock and runtime cleanup
On Fri, 2012-09-07 at 12:14 +0530, Inderpal Singh wrote: > The controller clock is being managed at AMBA bus level probe/remove and > pm_runtime/suspend functions. The existing driver does the clock > enable/disable > again in the same code paths, which unneccessarily increments the usage count > of > the clock for the same device. > > The following patches remove the redundant clock enable/disable from the > driver. Applied thanks -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] DMA: PL330: Clock and runtime cleanup
On Fri, 2012-09-07 at 12:14 +0530, Inderpal Singh wrote: > The controller clock is being managed at AMBA bus level probe/remove and > pm_runtime/suspend functions. The existing driver does the clock > enable/disable > again in the same code paths, which unneccessarily increments the usage count > of > the clock for the same device. > > The following patches remove the redundant clock enable/disable from the > driver. Looks good, any tested by before I apply this.. Kukjin? > > Inderpal Singh (2): > DMA: PL330: Remove controller clock enable/disable > DMA: PL330: Remove redundant runtime_suspend/resume functions > > drivers/dma/pl330.c | 73 > --- > 1 file changed, 5 insertions(+), 68 deletions(-) > -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 0/2] ARM: EXYNOS: Set the capability of pdm0 and pdm1 as DMA_PRIVATE
On Fri, 2012-09-07 at 14:30 +0900, Kukjin Kim wrote: > Vinod Koul wrote: > > > > On Wed, 2012-08-29 at 10:16 +0530, Tushar Behera wrote: > > > DMA clients pdma0 and pdma1 are internal to the SoC and are used only > > > by dedicated peripherals. Since they cannot be used for generic > > > purpose, their capability should be set as DMA_PRIVATE. > > > > > > The patches are rebased on top of v3.6-rc3. > > Kukjin, if you ack them I can take thru my tree, other way round is fine > > with me too. > > Hi Vinod, > > Looks good to me, please pick them into your tree with my ack. > > Acked-by: Kukjin Kim Okay applied both. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] ARM: EXYNOS: Set the capability of pdm0 and pdm1 as DMA_PRIVATE
On Wed, 2012-08-29 at 10:16 +0530, Tushar Behera wrote: > DMA clients pdma0 and pdma1 are internal to the SoC and are used only > by dedicated peripherals. Since they cannot be used for generic > purpose, their capability should be set as DMA_PRIVATE. > > The patches are rebased on top of v3.6-rc3. Kukjin, if you ack them I can take thru my tree, other way round is fine with me too. > > Tushar Behera (2): > ARM: EXYNOS: Set the capability of pdm0 and pdm1 as DMA_PRIVATE > DMA: PL330: Set the capability of pdm0 and pdm1 as DMA_PRIVATE > > arch/arm/mach-exynos/dma.c |2 ++ > drivers/dma/pl330.c|1 + > 2 files changed, 3 insertions(+), 0 deletions(-) > -- ~Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dma: pl330: assign a new cookie when restarting tx descriptors in cyclic mode
On Thu, 2012-05-03 at 17:50 -0700, Thomas Abraham wrote:
> The cookie of completed transfer descriptors are marked as zero. In case of
> cyclic transfers, a new cookie needs to be assigned to the transfer
> descriptors
> which are picked up from the work_list list before they are re-enabled for
> transfer. This prevents hitting BUG_ON in dma_cookie_complete function when
> transfer descriptors are recycled from the work_list list.
>
> Signed-off-by: Thomas Abraham
> ---
> drivers/dma/pl330.c |1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 2ee6e23..7f8f422 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2265,6 +2265,7 @@ static inline void handle_cyclic_desc_list(struct
> list_head *list)
> dma_async_tx_callback callback;
>
> /* Change status to reload it */
> + dma_cookie_assign(&desc->txd);
> desc->status = PREP;
> pch = desc->pchan;
> callback = desc->txd.callback;
Nope, this doesn't seem correct to me for two reasons:
1. client doesnt know this new descriptor, so how will it keep track
2. What does marking cyclic descriptor complete mean... Nothing IMO
So it is better *NOT* to mark the descriptor complete.
And below is a better fix this for you, Please test.
--
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ee6e23..fa3fb21 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2322,7 +2322,8 @@ static void pl330_tasklet(unsigned long data)
/* Pick up ripe tomatoes */
list_for_each_entry_safe(desc, _dt, &pch->work_list, node)
if (desc->status == DONE) {
- dma_cookie_complete(&desc->txd);
+ if (pch->cyclic)
+ dma_cookie_complete(&desc->txd);
list_move_tail(&desc->node, &list);
}
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] DMA: PL330: Merge PL330 driver into drivers/dma/
On Wed, 2012-02-22 at 08:01 +0530, Jassi Brar wrote: > On Wed, Feb 22, 2012 at 7:41 AM, Kukjin Kim wrote: Sadly, what a mess!!! Jassi, you don't own the copyright, your company did, as they employed you to do the job. So both your and Kukjin are not correct in claiming the copyright!! Jassi, your claim on being author is certainly right and you name should be there as MODULE_AUTHOR. Also, If you had a Authors line added and that was removed, then I would have agreed with you. And Kukjin, It wont hurt by giving credit to your former colleagues, by doing something like: http://permalink.gmane.org/gmane.linux.alsa.devel/94905 -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DMA: PL330: Merge PL330 drivers
On Thu, 2011-12-08 at 10:10 +0900, Boojin Kim wrote: > In-Reply-To: > > PL330 driver is divided into 2 parts. > First is the PL330 API driver that located on driver/dma/. > Second is the low-level PL330 driver that is located on arch/arm/common/. > But, It's not needed anymore to divided PL330 driver into 2 parts > Low-level PL330 driver is only used for PL330 API driver on driver/dma > because samsung specific S3C-PL330 driver was gone. > So, This patch merges 'arch/arm/common/pl330' into 'driver/dma/pl330.c'. > > [PATCH 1/2] DMA: PL330: Merge PL330 drivers > [PATCH 2/2] DMA: PL330: Remove an unused function You should try git --cover-letter for this :) -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] DMA: PL330: Remove pm_runtime_xxx calls from pl330 probe/remove
On Thu, 2011-12-08 at 16:43 +0900, Kukjin Kim wrote: > Vinod Koul wrote: > > > > On Tue, 2011-12-06 at 16:15 +0530, Tushar Behera wrote: > > > amba_probe() now calls pm_runtime_get_noresume() and pm_runtime_enable() > > > for the devices before the device probe is called. Hence we don't need > > > to call pm_runtime_get_xxx and pm_runtime_enable() in device probe again. > > > In the same way, since amba_remove() calls the respective pm_runtime > > > functions, those functions need not be called from device remove. > > > > > > This patch fixes following run time error with pl330 driver. > > > > > > dma-pl330 dma-pl330.0: Unbalanced pm_runtime_enable! > > > dma-pl330 dma-pl330.0: failed to get runtime pm > > > > > > Signed-off-by: Giridhar Maruthy > > > Signed-off-by: Tushar Behera > > Looks fine to me. Do you want these to go thru slave-dma or samsung > > tree. > > Hi Vinod, > > I think, this patch can be sent to upstream via slave-dma tree and > second one via Samsung tree separately and you can add my and Boojin > Kim's ack(actually, she replied) on this when you apply. Okay, I have applied this one only -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DMA: PL330: Remove pm_runtime_xxx calls from pl330 probe/remove
On Tue, 2011-12-06 at 16:15 +0530, Tushar Behera wrote:
> amba_probe() now calls pm_runtime_get_noresume() and pm_runtime_enable()
> for the devices before the device probe is called. Hence we don't need
> to call pm_runtime_get_xxx and pm_runtime_enable() in device probe again.
> In the same way, since amba_remove() calls the respective pm_runtime
> functions, those functions need not be called from device remove.
>
> This patch fixes following run time error with pl330 driver.
>
> dma-pl330 dma-pl330.0: Unbalanced pm_runtime_enable!
> dma-pl330 dma-pl330.0: failed to get runtime pm
>
> Signed-off-by: Giridhar Maruthy
> Signed-off-by: Tushar Behera
Looks fine to me. Do you want these to go thru slave-dma or samsung
tree.
For latter:
Acked-by: Vinod Koul
> ---
> drivers/dma/pl330.c | 17 ++---
> 1 files changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index a626e15..cd07121 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -834,17 +834,7 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
>
> amba_set_drvdata(adev, pdmac);
>
> -#ifdef CONFIG_PM_RUNTIME
> - /* to use the runtime PM helper functions */
> - pm_runtime_enable(&adev->dev);
> -
> - /* enable the power domain */
> - if (pm_runtime_get_sync(&adev->dev)) {
> - dev_err(&adev->dev, "failed to get runtime pm\n");
> - ret = -ENODEV;
> - goto probe_err1;
> - }
> -#else
> +#ifndef CONFIG_PM_RUNTIME
> /* enable dma clk */
> clk_enable(pdmac->clk);
> #endif
> @@ -977,10 +967,7 @@ static int __devexit pl330_remove(struct amba_device
> *adev)
> res = &adev->res;
> release_mem_region(res->start, resource_size(res));
>
> -#ifdef CONFIG_PM_RUNTIME
> - pm_runtime_put(&adev->dev);
> - pm_runtime_disable(&adev->dev);
> -#else
> +#ifndef CONFIG_PM_RUNTIME
> clk_disable(pdmac->clk);
> #endif
>
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DMA: PL330: Fix build warning
On Thu, 2011-11-03 at 15:48 +0900, Boojin Kim wrote:
> This patch adds to fix the build warning as following.
>
> drivers/dma/pl330.c: In function 'pl330_probe':
> drivers/dma/pl330.c:859: warning: comparison of distinct pointer types lacks
> a cast
>
> Signed-off-by: Boojin Kim
> ---
> drivers/dma/pl330.c |3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 621134f..379d928 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -856,7 +856,8 @@ pl330_probe(struct amba_device *adev, const struct
> amba_id *id)
> INIT_LIST_HEAD(&pd->channels);
>
> /* Initialize channel parameters */
> - num_chan = max(pdat ? pdat->nr_valid_peri : 0, (u8)pi->pcfg.num_chan);
> + num_chan = max(pdat ? (int)pdat->nr_valid_peri : 0,
> + (int)pi->pcfg.num_chan);
> pdmac->peripherals = kzalloc(num_chan * sizeof(*pch), GFP_KERNEL);
>
> for (i = 0; i < num_chan; i++) {
Applied Thanks
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 00/10] Add device tree support for PL330 dma controller driver
On Wed, 2011-10-12 at 14:03 +0900, Kukjin Kim wrote: > Vinod Koul wrote: > > > > On Tue, 2011-10-11 at 21:06 +0900, Kukjin Kim wrote: > > > Thomas Abraham wrote: > > > > > > Hi, > > > > > > Looks ok to me and if required, > > > Acked-by: Kukjin Kim > > > > > > And I hope since this includes many changes of arch/arm/Samsung stuff, > > > this > > > would be sent to upstream via Samsung tree after ack from Vinod for dma > > > stuff. > > Sure, looks fine to me > > Acked-by: Vinod Koul > > > Hi Vinod, > > OK, I will apply this series to send to upstream via Samsung tree. If > you need branch to apply in your tree, please let me know. If you send to linus and there are no dependencies then I don't need to do anything... -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 00/10] Add device tree support for PL330 dma controller driver
On Tue, 2011-10-11 at 21:06 +0900, Kukjin Kim wrote: > Thomas Abraham wrote: > > Hi, > > Looks ok to me and if required, > Acked-by: Kukjin Kim > > And I hope since this includes many changes of arch/arm/Samsung stuff, this > would be sent to upstream via Samsung tree after ack from Vinod for dma > stuff. Sure, looks fine to me Acked-by: Vinod Koul -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v8 00/16] To use DMA generic APIs for Samsung DMA
On Mon, 2011-09-19 at 19:23 +0900, Kukjin Kim wrote: > Vinod Koul wrote: > > > > On Wed, 2011-09-14 at 17:03 +0530, Jassi Brar wrote: > > > On 14 September 2011 16:47, Vinod Koul wrote: > > > > > > >> The changelog for [PATCH v8 04/16] is misleading - we don't need any > > > >> modification for the reason mentioned in changelog. But the > > > >> modification > > > >> has positive side-effect of preventing callbacks during terminate_all > > > >> which > > > >> is no way understood from the changelog. So I would like to changelog > > > >> corrected. > > > > I thought change log was correct in depicting what patch does and Boojin > > > > had replied I will check again... > > > > > > I didn't reply because I ran out of ways to explain the same thing in > > > different words. > > I checked again the patch, change log and your comments. > > I agree with current change log, and also your observation is right but > > that is just a side effect, which IMO should be best left to developer > > to choose or not, in this case she ignored it > > > > So no changes to this and I a ready to merge it to my next in a day or > > two... > > > So as a note, pulled git://git.infradead.org/users/vkoul/slave-dma.git > samsung_dma for other regarding dma patches in arch/arm/ samsung stuff. > > If any problems, please let me know. > I am pushing them to next now.. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/16] To use DMA generic APIs for Samsung DMA
On Wed, 2011-09-14 at 17:03 +0530, Jassi Brar wrote: > On 14 September 2011 16:47, Vinod Koul wrote: > > >> The changelog for [PATCH v8 04/16] is misleading - we don't need any > >> modification for the reason mentioned in changelog. But the modification > >> has positive side-effect of preventing callbacks during terminate_all which > >> is no way understood from the changelog. So I would like to changelog > >> corrected. > > I thought change log was correct in depicting what patch does and Boojin > > had replied I will check again... > > I didn't reply because I ran out of ways to explain the same thing in > different words. I checked again the patch, change log and your comments. I agree with current change log, and also your observation is right but that is just a side effect, which IMO should be best left to developer to choose or not, in this case she ignored it So no changes to this and I a ready to merge it to my next in a day or two... -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/16] To use DMA generic APIs for Samsung DMA
On Wed, 2011-09-14 at 12:00 +0530, Jassi Brar wrote: > On Wed, Sep 14, 2011 at 11:31 AM, Vinod Koul wrote: > > On Fri, 2011-09-02 at 09:44 +0900, Boojin Kim wrote: > >> This patchset adds support DMA generic APIs for samsung DMA. > >> > >> The changes from V7 is following: > >> - Divides patch file. > >> : The 03 patch on V7 patchset is divided into the 03 and 04 patch on V8 > >> patchset. > >> The O3 patch is only for DMA_SLAVE_CONFIG command. > >> The 04 patch is only for DMA_TERMINATE_ALL command. > >> - Code clean-up > >> : Remove unnecessary code on 05 patch. > >> > >> [PATCH v8 01/16] DMA: PL330: Add support runtime PM for PL330 DMAC > >> [PATCH v8 02/16] DMA: PL330: Update PL330 DMA API driver > >> [PATCH v8 03/16] DMA: PL330: Support DMA_SLAVE_CONFIG command > >> [PATCH v8 04/16] DMA: PL330: Remove the start operation for handling > >> DMA_TERMINATE_ALL command > >> [PATCH v8 05/16] DMA: PL330: Add DMA_CYCLIC capability > >> [PATCH v8 06/16] ARM: SAMSUNG: Update to use PL330-DMA driver > >> [PATCH v8 07/16] ARM: SAMSUNG: Add common DMA operations > >> [PATCH v8 08/16] ARM: EXYNOS4: Use generic DMA PL330 driver > >> [PATCH v8 09/16] ARM: S5PV210: Use generic DMA PL330 driver > >> [PATCH v8 10/16] ARM: S5PC100: Use generic DMA PL330 driver > >> [PATCH v8 11/16] ARM: S5P64X0: Use generic DMA PL330 driver > >> [PATCH v8 12/16] ARM: SAMSUNG: Remove S3C-PL330-DMA driver > >> [PATCH v8 13/16] spi/s3c64xx: Add support DMA engine API > >> [PATCH v8 14/16] spi/s3c64xx: Merge dma control code > >> [PATCH v8 15/16] ASoC: Samsung: Update DMA interface > >> [PATCH v8 16/16] ARM: SAMSUNG: Remove Samsung specific enum type for dma > >> direction > > Thanks, > > > > I have pushed these into my samsung-dma branch, Fixed the typos in the > > changelog of patch 4. > > > > Please check and let me know if all are fine, I will push them to my > > next tomorrow > > > > If anyone has concerns on this series please let me know by tomorrow > > > > I have no concern about patches modifying samsung specific code. > > Of the patches touching common pl330 code, I had acked Patch-1,2,3 & 5 > in previous revisions already. So for them, you may add my > Acked-by: Jassi Brar Ok > > The changelog for [PATCH v8 04/16] is misleading - we don't need any > modification for the reason mentioned in changelog. But the modification > has positive side-effect of preventing callbacks during terminate_all which > is no way understood from the changelog. So I would like to changelog > corrected. I thought change log was correct in depicting what patch does and Boojin had replied I will check again... -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 00/16] To use DMA generic APIs for Samsung DMA
On Fri, 2011-09-02 at 09:44 +0900, Boojin Kim wrote: > This patchset adds support DMA generic APIs for samsung DMA. > > The changes from V7 is following: > - Divides patch file. > : The 03 patch on V7 patchset is divided into the 03 and 04 patch on V8 > patchset. > The O3 patch is only for DMA_SLAVE_CONFIG command. > The 04 patch is only for DMA_TERMINATE_ALL command. > - Code clean-up > : Remove unnecessary code on 05 patch. > > [PATCH v8 01/16] DMA: PL330: Add support runtime PM for PL330 DMAC > [PATCH v8 02/16] DMA: PL330: Update PL330 DMA API driver > [PATCH v8 03/16] DMA: PL330: Support DMA_SLAVE_CONFIG command > [PATCH v8 04/16] DMA: PL330: Remove the start operation for handling > DMA_TERMINATE_ALL command > [PATCH v8 05/16] DMA: PL330: Add DMA_CYCLIC capability > [PATCH v8 06/16] ARM: SAMSUNG: Update to use PL330-DMA driver > [PATCH v8 07/16] ARM: SAMSUNG: Add common DMA operations > [PATCH v8 08/16] ARM: EXYNOS4: Use generic DMA PL330 driver > [PATCH v8 09/16] ARM: S5PV210: Use generic DMA PL330 driver > [PATCH v8 10/16] ARM: S5PC100: Use generic DMA PL330 driver > [PATCH v8 11/16] ARM: S5P64X0: Use generic DMA PL330 driver > [PATCH v8 12/16] ARM: SAMSUNG: Remove S3C-PL330-DMA driver > [PATCH v8 13/16] spi/s3c64xx: Add support DMA engine API > [PATCH v8 14/16] spi/s3c64xx: Merge dma control code > [PATCH v8 15/16] ASoC: Samsung: Update DMA interface > [PATCH v8 16/16] ARM: SAMSUNG: Remove Samsung specific enum type for dma > direction Thanks, I have pushed these into my samsung-dma branch, Fixed the typos in the changelog of patch 4. Please check and let me know if all are fine, I will push them to my next tomorrow If anyone has concerns on this series please let me know by tomorrow -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 04/16] DMA: PL330: Remove the start operation for handling DMA_TERMINATE_ALL command
On Fri, 2011-09-02 at 09:44 +0900, Boojin Kim wrote:
> Origianl code carries out the start operation after flush operation.
Orignal
> But start operation is not required for DMA_TERMINATE_ALL command.
> So, This patch removes the unnecessary start operation and only carries out
this should be all lowercase
> the flush oeration for handling DMA_TERMINATE_ALL command.
operation
>
> Signed-off-by: Boojin Kim
> Acked-by: Linus Walleij
> Acked-by: Vinod Koul
> Cc: Dan Williams
> Signed-off-by: Kukjin Kim
> ---
> drivers/dma/pl330.c | 11 +++
> 1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index e7f9d1d..59943ec 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -262,10 +262,11 @@ static int pl330_alloc_chan_resources(struct dma_chan
> *chan)
> static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> unsigned long arg)
> {
> struct dma_pl330_chan *pch = to_pchan(chan);
> - struct dma_pl330_desc *desc;
> + struct dma_pl330_desc *desc, *_dt;
> unsigned long flags;
> struct dma_pl330_dmac *pdmac = pch->dmac;
> struct dma_slave_config *slave_config;
> + LIST_HEAD(list);
>
> switch (cmd) {
> case DMA_TERMINATE_ALL:
> @@ -275,12 +276,14 @@ static int pl330_control(struct dma_chan *chan, enum
> dma_ctrl_cmd cmd, unsigned
> pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
>
> /* Mark all desc done */
> - list_for_each_entry(desc, &pch->work_list, node)
> + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) {
> desc->status = DONE;
> + pch->completed = desc->txd.cookie;
> + list_move_tail(&desc->node, &list);
> + }
>
> + list_splice_tail_init(&list, &pdmac->desc_pool);
> spin_unlock_irqrestore(&pch->lock, flags);
> -
> - pl330_tasklet((unsigned long) pch);
> break;
> case DMA_SLAVE_CONFIG:
> slave_config = (struct dma_slave_config *)arg;
If this is the only fix required in entire series, I can fix this up
while applying
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Add device tree support for PL330 dma controller driver
On Tue, 2011-08-30 at 17:58 +0530, Thomas Abraham wrote: > Hi Vinod, > > On 29 August 2011 22:59, Vinod Koul wrote: > > On Fri, 2011-08-26 at 14:10 +0530, Thomas Abraham wrote: > >> This patchset adds device tree support for PL330 driver and uses it > >> to add device tree support for Samsung platforms, specifically Exynos4. > > The DMA patches looks good to me. Do you want this to go thru slave-dma > > tree or somewhere else. > > I will need acks on 3, 5 and 6th patch to carry them. > > There are couple of changes required in patch 4/6 of this series. I > will do that and resubmit. > > Since these patches are based on Boojin's pl330 driver update patches, > it would be easier to take the same route which Boojin's patches will > take. I will try to get acks' for the patches that you have listed. > And I think these might need to be rebased based on Boojin's latest version -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] Add device tree support for PL330 dma controller driver
On Fri, 2011-08-26 at 14:10 +0530, Thomas Abraham wrote: > This patchset adds device tree support for PL330 driver and uses it > to add device tree support for Samsung platforms, specifically Exynos4. The DMA patches looks good to me. Do you want this to go thru slave-dma tree or somewhere else. I will need acks on 3, 5 and 6th patch to carry them. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/15] DMA: PL330: Update PL330 DMA API driver
On Mon, 2011-08-29 at 19:52 +0530, Vinod Koul wrote:
> On Mon, 2011-08-29 at 19:25 +0530, Vinod Koul wrote:
> > On Thu, 2011-08-25 at 11:13 +0900, Boojin Kim wrote:
> > > This patch updates following 3 items.
> > > @@ -69,6 +70,10 @@ struct dma_pl330_chan {
> > >* NULL if the channel is available to be acquired.
> > >*/
> > > void *pl330_chid;
> > > +
> > > + /* For D-to-M and M-to-D channels */
> > > + int burst_sz; /* the peripheral fifo width */
> > > + dma_addr_t fifo_addr;
> > > };
> > Why should you store peripheral address and burst size in channel
> > structure. You should take these from the API and dma_slave_structure
> Ignore my comment for burst size, but is valid for peripheral address
Never mind this one as well, I need some more coffee...
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/15] DMA: PL330: Update PL330 DMA API driver
On Mon, 2011-08-29 at 19:25 +0530, Vinod Koul wrote:
> On Thu, 2011-08-25 at 11:13 +0900, Boojin Kim wrote:
> > This patch updates following 3 items.
> > @@ -69,6 +70,10 @@ struct dma_pl330_chan {
> > * NULL if the channel is available to be acquired.
> > */
> > void *pl330_chid;
> > +
> > + /* For D-to-M and M-to-D channels */
> > + int burst_sz; /* the peripheral fifo width */
> > + dma_addr_t fifo_addr;
> > };
> Why should you store peripheral address and burst size in channel
> structure. You should take these from the API and dma_slave_structure
Ignore my comment for burst size, but is valid for peripheral address
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/15] DMA: PL330: Update PL330 DMA API driver
On Thu, 2011-08-25 at 11:13 +0900, Boojin Kim wrote:
> This patch updates following 3 items.
> @@ -69,6 +70,10 @@ struct dma_pl330_chan {
>* NULL if the channel is available to be acquired.
>*/
> void *pl330_chid;
> +
> + /* For D-to-M and M-to-D channels */
> + int burst_sz; /* the peripheral fifo width */
> + dma_addr_t fifo_addr;
> };
Why should you store peripheral address and burst size in channel
structure. You should take these from the API and dma_slave_structure
>
> struct dma_pl330_dmac {
> @@ -456,7 +461,7 @@ static struct dma_pl330_desc *pl330_get_desc(struct
> dma_pl330_chan *pch)
>
> if (peri) {
> desc->req.rqtype = peri->rqtype;
> - desc->req.peri = peri->peri_id;
> + desc->req.peri = pch->chan.chan_id;
> } else {
> desc->req.rqtype = MEMTOMEM;
> desc->req.peri = 0;
> @@ -582,7 +587,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct
> scatterlist *sgl,
> struct dma_pl330_peri *peri = chan->private;
> struct scatterlist *sg;
> unsigned long flags;
> - int i, burst_size;
> + int i;
> dma_addr_t addr;
>
> if (unlikely(!pch || !sgl || !sg_len || !peri))
> @@ -598,8 +603,7 @@ pl330_prep_slave_sg(struct dma_chan *chan, struct
> scatterlist *sgl,
> return NULL;
> }
>
> - addr = peri->fifo_addr;
> - burst_size = peri->burst_sz;
> + addr = pch->fifo_addr;
what you removed is the correct way to do...
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve slave/cyclic DMA engine documentation (was: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability)
On Tue, 2011-07-26 at 23:25 +0530, Jassi Brar wrote: > On Tue, Jul 26, 2011 at 10:50 PM, Russell King - ARM Linux > wrote: > > On Tue, Jul 26, 2011 at 08:07:44PM +0530, Jassi Brar wrote: > >> Dear Vinod, > >> > >> Since it came from the RMK, most probably it'll be the best. > >> > >> But applying patches upon personal timeout seems very dangerous. > > > > Ehh what? Is there any contention over this documentation patch? > I haven't yet read it... even after reading I would object only if I found > your patch disturbing enough to disrupt my bowel movements. Which > I don't think would be the case. Jassi, Since this was a doc patch, I applied it soon enough, doesn't sound dangerous to me ! If you have any updates you would like, pls feel free to send a patch. > > I just observed it is second time that Vinod applied a patch without any > ack or prior alert. And you can't ignore the fact that you had almost _two_ weeks to ack the patch from Rob, but You will get ample time to ack a patch, if you don't you can always point out and I will do the right thing, which in this case was to revert. > > > > >> People not responding doesn't mean only either people agree completely > >> or they don't care. Some might be interested but too busy with current > >> tasks > >> that they need time to check... please make some policy for such cases. > >> > >> It already happened with the patch from Rob, which you probably have to > >> revert. > >> > >> IMHO, if nobody replied, maybe you could first ack the patch and wait > >> for, say a week, before applying? > >> That way people will know they have to hurry if they care otherwise > >> the patch is going upstream as such. > > > > A week is far too long. That's how patches get lost and missed. > > > He may decide to wait shorter, but imho a week after the first ack > isn't that long. > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V4 12/14] spi/s3c64xx: Add support DMA engine API
On Tue, 2011-07-26 at 18:31 +0900, Boojin Kim wrote:
> Vinod Koul Wrote:
> > Sent: Monday, July 25, 2011 8:17 PM
> > To: Boojin Kim
> > Cc: vinod.k...@intel.com; linux-arm-ker...@lists.infradead.org; linux-
> > samsung-...@vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely;
> > Mark Brown; Dan Williams
> > Subject: Re: [PATCH V4 12/14] spi/s3c64xx: Add support DMA engine API
> >
> > On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> > > This patch adds to support DMA generic API to transfer raw
> > > SPI data. Basiclly the spi driver uses DMA generic API if
> > > architecture supports it. Otherwise, uses Samsung specific
> > > S3C-PL330 APIs.
> > >
> > > Signed-off-by: Boojin Kim
> > > Cc: Grant Likely
> > > Signed-off-by: Kukjin Kim
> > > ---
> > > drivers/spi/spi_s3c64xx.c | 141 ++-
> > --
> > > 1 files changed, 69 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
> > > index 8945e20..a4cf76a 100644
> > > --- a/drivers/spi/spi_s3c64xx.c
> > > +++ b/drivers/spi/spi_s3c64xx.c
> > > @@ -172,6 +172,9 @@ struct s3c64xx_spi_driver_data {
> > > unsignedstate;
> > > unsignedcur_mode, cur_bpw;
> > > unsignedcur_speed;
> > > + unsignedrx_ch;
> > > + unsignedtx_ch;
> > > + struct samsung_dma_ops *ops;
> > > };
> > >
> > > static struct s3c2410_dma_client s3c64xx_spi_dma_client = {
> > > @@ -227,6 +230,38 @@ static void flush_fifo(struct
> > s3c64xx_spi_driver_data *sdd)
> > > writel(val, regs + S3C64XX_SPI_CH_CFG);
> > > }
> > >
> > > +static void s3c64xx_spi_dma_rxcb(void *data)
> > > +{
> > > + struct s3c64xx_spi_driver_data *sdd
> > > + = (struct s3c64xx_spi_driver_data *)data;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&sdd->lock, flags);
> > > +
> > > + sdd->state &= ~RXBUSY;
> > > + /* If the other done */
> > > + if (!(sdd->state & TXBUSY))
> > > + complete(&sdd->xfer_completion);
> > > +
> > > + spin_unlock_irqrestore(&sdd->lock, flags);
> > > +}
> > > +
> > > +static void s3c64xx_spi_dma_txcb(void *data)
> > > +{
> > > + struct s3c64xx_spi_driver_data *sdd
> > > + = (struct s3c64xx_spi_driver_data *)data;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&sdd->lock, flags);
> > > +
> > > + sdd->state &= ~TXBUSY;
> > > + /* If the other done */
> > > + if (!(sdd->state & RXBUSY))
> > > + complete(&sdd->xfer_completion);
> > > +
> > > + spin_unlock_irqrestore(&sdd->lock, flags);
> > > +}
> > I don't see much diff in these two functions and you should be able to
> > use a generic one which takes care of both TX and RX, does your
> > callback
> > data know if the channel is for TX or RX? If not then a helper to do
> > above should take care well and making code simpler
> I'm very agree with you.
> But, I think it isn't deeply related to this patch series. So, I wish to make
> and submit it after this patch series is finished.
>
Since you are reworking this driver would make sense to do now, it
doesn't sound to be a big change.
--
~Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve slave/cyclic DMA engine documentation (was: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability)
llback function may wish to terminate the > + DMA via dmaengine_terminate_all(). > + > + Therefore, it is important that DMA engine drivers drop any > + locks before calling the callback function which may cause a > + deadlock. > + > + Note that callbacks will always be invoked from the DMA > + engines tasklet, never from interrupt context. > + > +4. Submit the transaction > + > + Once the descriptor has been prepared and the callback information > + added, it must be placed on the DMA engine drivers pending queue. > + > + Interface: > + dma_cookie_t dmaengine_submit(struct dma_async_tx_descriptor *desc) > + > + This returns a cookie can be used to check the progress of DMA engine > + activity via other DMA engine calls not covered in this document. > + > + dmaengine_submit() will not start the DMA operation, it merely adds > + it to the pending queue. For this, see step 5, dma_async_issue_pending. > + > +5. Issue pending DMA requests and wait for callback notification > + > + The transactions in the pending queue can be activated by calling the > + issue_pending API. If channel is idle then the first transaction in > + queue is started and subsequent ones queued up. > + > + On completion of each DMA operation, the next in queue is started and > + a tasklet triggered. The tasklet will then call the client driver > + completion callback routine for notification, if set. > + > + Interface: > + void dma_async_issue_pending(struct dma_chan *chan); > + > +Further APIs: > + > +1. int dmaengine_terminate_all(struct dma_chan *chan) > + > + This causes all activity for the DMA channel to be stopped, and may > + discard data in the DMA FIFO which hasn't been fully transferred. > + No callback functions will be called for any incomplete transfers. > + > +2. int dmaengine_pause(struct dma_chan *chan) > + > + This pauses activity on the DMA channel without data loss. > + > +3. int dmaengine_resume(struct dma_chan *chan) > + > + Resume a previously paused DMA channel. It is invalid to resume a > + channel which is not currently paused. > + > +4. enum dma_status dma_async_is_tx_complete(struct dma_chan *chan, > +dma_cookie_t cookie, dma_cookie_t *last, dma_cookie_t *used) > + > + This can be used to check the status of the channel. Please see > + the documentation in include/linux/dmaengine.h for a more complete > + description of this API. > + > + This can be used in conjunction with dma_async_is_complete() and > + the cookie returned from 'descriptor->submit()' to check for > + completion of a specific DMA transaction. > + > + Note: > + Not all DMA engine drivers can return reliable information for > + a running DMA channel. It is recommended that DMA engine users > + pause or stop (via dmaengine_terminate_all) the channel before > + using this API. > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- ~Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Mon, 2011-07-25 at 21:51 +0900, Boojin Kim wrote:
> Vinod Koul Wrote:
> > Sent: Monday, July 25, 2011 7:27 PM
> > To: Boojin Kim
> > Cc: vinod.k...@intel.com; linux-arm-ker...@lists.infradead.org; linux-
> > samsung-...@vger.kernel.org; Kukjin Kim; Jassi Brar; Grant Likely;
> > Mark Brown; Dan Williams
> > Subject: Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG
> > command
> >
> > On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> > > This patch adds support DMA_SLAVE_CONFIG command.
> > >
> > > Signed-off-by: Boojin Kim
> > > ---
> > > drivers/dma/pl330.c | 53 +
> > -
> > > 1 files changed, 39 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > > index 586ab39..880f010 100644
> > > --- a/drivers/dma/pl330.c
> > > +++ b/drivers/dma/pl330.c
> > > @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct
> > dma_chan *chan)
> > > static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd
> > cmd, unsigned long arg)
> > > {
> > > struct dma_pl330_chan *pch = to_pchan(chan);
> > > - struct dma_pl330_desc *desc;
> > > + struct dma_pl330_desc *desc, *_dt;
> > > unsigned long flags;
> > > + struct dma_pl330_dmac *pdmac = pch->dmac;
> > > + struct dma_slave_config *slave_config;
> > > + struct dma_pl330_peri *peri;
> > > + LIST_HEAD(list);
> > >
> > > - /* Only supports DMA_TERMINATE_ALL */
> > > - if (cmd != DMA_TERMINATE_ALL)
> > > - return -ENXIO;
> > > -
> > > - spin_lock_irqsave(&pch->lock, flags);
> > > -
> > > - /* FLUSH the PL330 Channel thread */
> > > - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > > + switch (cmd) {
> > > + case DMA_TERMINATE_ALL:
> > > + spin_lock_irqsave(&pch->lock, flags);
> > >
> > > - /* Mark all desc done */
> > > - list_for_each_entry(desc, &pch->work_list, node)
> > > - desc->status = DONE;
> > > + /* FLUSH the PL330 Channel thread */
> > > + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> > >
> > > - spin_unlock_irqrestore(&pch->lock, flags);
> > > + /* Mark all desc done */
> > > + list_for_each_entry_safe(desc, _dt, &pch->work_list , node)
> > {
> > > + desc->status = DONE;
> > > + pch->completed = desc->txd.cookie;
> > > + list_move_tail(&desc->node, &list);
> > > + }
> > >
> > > - pl330_tasklet((unsigned long) pch);
> > > + list_splice_tail_init(&list, &pdmac->desc_pool);
> > > + spin_unlock_irqrestore(&pch->lock, flags);
> > > + break;
> > > + case DMA_SLAVE_CONFIG:
> > > + slave_config = (struct dma_slave_config *)arg;
> > > + peri = pch->chan.private;
> > > +
> > > + if (slave_config->direction == DMA_TO_DEVICE) {
> > > + if (slave_config->dst_addr)
> > > + peri->fifo_addr = slave_config->dst_addr;
> > > + if (slave_config->dst_addr_width)
> > > + peri->burst_sz = __ffs(slave_config-
> > >dst_addr_width);
> > This doesn't sound right, why can't you use xxx_maxburst field instead?
> Can you explain the problem in more detail?
> Do you mean that 'xxx_maxburst' takes place of 'xxx_addr_width' or __ffs()
> isn't proper here?
>
> Additionally, I will modify this code to consider '_maxburst' field.
__ffs is okay and sensible.
I meant you should use proper fields for passing info, which in this
case would be dst_maxburst for specifying destination burst sizes
--
~Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability
On Mon, 2011-07-25 at 12:39 +0100, Russell King - ARM Linux wrote: > On Mon, Jul 25, 2011 at 04:31:57PM +0530, Vinod Koul wrote: > > > I know that very well thank you. Please look at my previous post in the > > > previous round of patches, and the response from Boojin Kim to see why > > > I used this as an *EXAMPLE* to get this fixed. > > Russell, > > Sorry that was not intended for you but for the author of patch Boojin > > Kim... Agree on your EXAMPLE, just wanted to ensure authors have read it > > as going thru this patch made it clear that they haven't. > > That wasn't obvious because of the To: and the use of 'you' in your reply. Laziness on a rainy monday evening caused me to miss that > > In any case, let's fix up the documentation to be a little more complete > and expansive about these conditions, and improve its formatting so it's > easier to read. > > A few points: > > 1. This really should describe the dma_slave_config structure in detail >(or refer to the documentation in dmaengine.h) especially describing >what is mandatory there. I think you missed mandatory fields here, Pls note I am dropping the direction field in this, as we have discussed it makes no sense > > 2. Note the filter function thing - I think this should be mandatory >for slave and cyclic channels. These typically have a DMA request >signal associated with each channel, and so 'any random channel' isn't >really acceptable like it is for the async_tx APIs. > > 3. I think this should specify that dmaengine_slave_config() should be >specified as mandatory after obtaining the DMA engine channel. > > Any comments on this before I prepare this as a proper patch? > > diff --git a/Documentation/dmaengine.txt b/Documentation/dmaengine.txt > index 5a0cb1e..8ba8d3c 100644 > --- a/Documentation/dmaengine.txt > +++ b/Documentation/dmaengine.txt > @@ -10,87 +10,163 @@ > Below is a guide to device driver writers on how to use the Slave-DMA API of > the > DMA Engine. This is applicable only for slave DMA usage only. > > -The slave DMA usage consists of following steps > +The slave DMA usage consists of following steps: > 1. Allocate a DMA slave channel > 2. Set slave and controller specific parameters > 3. Get a descriptor for transaction > 4. Submit the transaction and wait for callback notification > +5. Issue pending requests > > 1. Allocate a DMA slave channel > -Channel allocation is slightly different in the slave DMA context, client > -drivers typically need a channel from a particular DMA controller only and > even > -in some cases a specific channel is desired. To request a channel > -dma_request_channel() API is used. > - > -Interface: > -struct dma_chan *dma_request_channel(dma_cap_mask_t mask, > - dma_filter_fn filter_fn, > - void *filter_param); > -where dma_filter_fn is defined as: > -typedef bool (*dma_filter_fn)(struct dma_chan *chan, void *filter_param); > - > -When the optional 'filter_fn' parameter is set to NULL dma_request_channel > -simply returns the first channel that satisfies the capability mask. > Otherwise, > -when the mask parameter is insufficient for specifying the necessary channel, > -the filter_fn routine can be used to disposition the available channels in > the > -system. The filter_fn routine is called once for each free channel in the > -system. Upon seeing a suitable channel filter_fn returns DMA_ACK which flags > -that channel to be the return value from dma_request_channel. A channel > -allocated via this interface is exclusive to the caller, until > -dma_release_channel() is called. > + > + Channel allocation is slightly different in the slave DMA context, > + client drivers typically need a channel from a particular DMA > + controller only and even in some cases a specific channel is desired. > + To request a channel dma_request_channel() API is used. > + > + Interface: > + struct dma_chan *dma_request_channel(dma_cap_mask_t mask, > + dma_filter_fn filter_fn, > + void *filter_param); > + where dma_filter_fn is defined as: > + typedef bool (*dma_filter_fn)(struct dma_chan *chan, void > *filter_param); > + > + The 'filter_fn' parameter is optional, but highly recommended for > + slave and cyclic channels as they typically need to obtain a specific > + DMA channel. > + > + When the optional 'filter_fn' parameter is NULL, dma_request_channel() > + simply returns the first channel that satisfies the capability mask. > + > + Otherwise, the 'filter_fn' routine will
Re: [PATCH V4 12/14] spi/s3c64xx: Add support DMA engine API
xfer->rx_dma, xfer->len);
> - s3c2410_dma_ctrl(sdd->rx_dmach, S3C2410_DMAOP_START);
> + info.cap = DMA_SLAVE;
> + info.direction = DMA_FROM_DEVICE;
> + info.buf = xfer->rx_dma;
> + info.len = xfer->len;
> + info.fp = s3c64xx_spi_dma_rxcb;
> + info.fp_param = sdd;
> + sdd->ops->prepare(sdd->rx_ch, &info);
> + sdd->ops->trigger(sdd->rx_ch);
> }
> }
>
> @@ -483,46 +527,6 @@ static void s3c64xx_spi_config(struct
> s3c64xx_spi_driver_data *sdd)
> }
> }
>
> -static void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
> - int size, enum s3c2410_dma_buffresult res)
> -{
> - struct s3c64xx_spi_driver_data *sdd = buf_id;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&sdd->lock, flags);
> -
> - if (res == S3C2410_RES_OK)
> - sdd->state &= ~RXBUSY;
> - else
> - dev_err(&sdd->pdev->dev, "DmaAbrtRx-%d\n", size);
> -
> - /* If the other done */
> - if (!(sdd->state & TXBUSY))
> - complete(&sdd->xfer_completion);
> -
> - spin_unlock_irqrestore(&sdd->lock, flags);
> -}
> -
> -static void s3c64xx_spi_dma_txcb(struct s3c2410_dma_chan *chan, void *buf_id,
> - int size, enum s3c2410_dma_buffresult res)
> -{
> - struct s3c64xx_spi_driver_data *sdd = buf_id;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&sdd->lock, flags);
> -
> - if (res == S3C2410_RES_OK)
> - sdd->state &= ~TXBUSY;
> - else
> - dev_err(&sdd->pdev->dev, "DmaAbrtTx-%d \n", size);
> -
> - /* If the other done */
> - if (!(sdd->state & RXBUSY))
> - complete(&sdd->xfer_completion);
> -
> - spin_unlock_irqrestore(&sdd->lock, flags);
> -}
> -
> #define XFER_DMAADDR_INVALID DMA_BIT_MASK(32)
>
> static int s3c64xx_spi_map_mssg(struct s3c64xx_spi_driver_data *sdd,
> @@ -697,12 +701,10 @@ static void handle_msg(struct s3c64xx_spi_driver_data
> *sdd,
> if (use_dma) {
> if (xfer->tx_buf != NULL
> && (sdd->state & TXBUSY))
> - s3c2410_dma_ctrl(sdd->tx_dmach,
> - S3C2410_DMAOP_FLUSH);
> + sdd->ops->stop(sdd->tx_ch);
> if (xfer->rx_buf != NULL
> && (sdd->state & RXBUSY))
> - s3c2410_dma_ctrl(sdd->rx_dmach,
> - S3C2410_DMAOP_FLUSH);
> + sdd->ops->stop(sdd->rx_ch);
> }
>
> goto out;
> @@ -742,24 +744,19 @@ out:
>
> static int acquire_dma(struct s3c64xx_spi_driver_data *sdd)
> {
> - if (s3c2410_dma_request(sdd->rx_dmach,
> - &s3c64xx_spi_dma_client, NULL) < 0) {
> - dev_err(&sdd->pdev->dev, "cannot get RxDMA\n");
> - return 0;
> - }
> - s3c2410_dma_set_buffdone_fn(sdd->rx_dmach, s3c64xx_spi_dma_rxcb);
> - s3c2410_dma_devconfig(sdd->rx_dmach, S3C2410_DMASRC_HW,
> - sdd->sfr_start + S3C64XX_SPI_RX_DATA);
> -
> - if (s3c2410_dma_request(sdd->tx_dmach,
> - &s3c64xx_spi_dma_client, NULL) < 0) {
> - dev_err(&sdd->pdev->dev, "cannot get TxDMA\n");
> - s3c2410_dma_free(sdd->rx_dmach, &s3c64xx_spi_dma_client);
> - return 0;
> - }
> - s3c2410_dma_set_buffdone_fn(sdd->tx_dmach, s3c64xx_spi_dma_txcb);
> - s3c2410_dma_devconfig(sdd->tx_dmach, S3C2410_DMASRC_MEM,
> - sdd->sfr_start + S3C64XX_SPI_TX_DATA);
> +
> + struct samsung_dma_info info;
> + sdd->ops = samsung_dma_get_ops();
> +
> + info.cap = DMA_SLAVE;
> + info.client = &s3c64xx_spi_dma_client;
> + info.direction = DMA_FROM_DEVICE;
> + info.fifo = sdd->sfr_start + S3C64XX_SPI_RX_DATA;
> + info.width = sdd->cur_bpw / 8;
> + sdd->rx_ch = sdd->ops->request(sdd->rx_dmach, &info);
> + info.direction = DMA_TO_DEVICE;
> + info.fifo = sdd->sfr_start + S3C64XX_SPI_TX_DATA;
> + sdd->tx_ch = sdd->ops->request(sdd->tx_dmach, &info);
>
> return 1;
> }
> @@ -800,8 +797,8 @@ static void s3c64xx_spi_work(struct work_struct *work)
> spin_unlock_irqrestore(&sdd->lock, flags);
>
> /* Free DMA channels */
> - s3c2410_dma_free(sdd->tx_dmach, &s3c64xx_spi_dma_client);
> - s3c2410_dma_free(sdd->rx_dmach, &s3c64xx_spi_dma_client);
> + sdd->ops->release(sdd->rx_ch, &s3c64xx_spi_dma_client);
> + sdd->ops->release(sdd->tx_ch, &s3c64xx_spi_dma_client);
> }
>
> static int s3c64xx_spi_transfer(struct spi_device *spi,
--
~Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND] [PATCH V4 0/14] To use DMA generic APIs for Samsung DMA
On Mon, 2011-07-25 at 15:47 +0530, Vinod Koul wrote: > On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote: > > This patchset adds support DMA generic APIs for Samsung DMA > > and is re-sent by the request of DMA maintainer, Vinod Koul. > > > > V4 has the changes about 3 patches from V3. > > Changes from V3: > > - Divided '[03/13 patch] DMA: PL330: Add DMA capabilites' on V3 patchset > > into two patches. > > - One is '03/14 patch' for supporting DMA_SLAVE_CONFIG command. > > - Another is '04/14 patch' for supporting DMA_CYCLIC capability. > > - Code clean-up > > - Remove unnecessary vairable referance on '01/14 patch'. > > - Remove redunancy code on '06/14 patch' > > > > [PATCH V4 01/14] DMA: PL330: Add support runtime PM for PL330 DMAC > > [PATCH V4 02/14] DMA: PL330: Update PL330 DMA API driver > > [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command > > [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability > > [PATCH V4 05/14] ARM: SAMSUNG: Update to use PL330-DMA driver > > [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations > > [PATCH V4 07/14] ARM: EXYNOS4: Use generic DMA PL330 driver > > [PATCH V4 08/14] ARM: S5PV210: Use generic DMA PL330 driver > > [PATCH V4 09/14] ARM: S5PC100: Use generic DMA PL330 driver > > [PATCH V4 10/14] ARM: S5P64X0: Use generic DMA PL330 driver > > [PATCH V4 11/14] ARM: SAMSUNG: Remove S3C-PL330-DMA driver > > [PATCH V4 12/14] spi/s3c64xx: Add support DMA engine API > > [PATCH V4 13/14] ASoC: Samsung: Update DMA interface > > [PATCH V4 14/14] ARM: SAMSUNG: Remove Samsung specific enum type for dma > > direction > > > Thanks for resending I will review them now, You should use git > --cover-letter to generate this > Also I noticed the whole patch set doesn't have any acks from previous posting. If a patch has got ack from someone and it is not changed at all in next posting then you can carry that ack. Makes things easier for me to track while applying -- ~Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability
On Mon, 2011-07-25 at 11:57 +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 25, 2011 at 04:18:04PM +0530, Vinod Koul wrote:
> > On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote:
> > > > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote:
> > > > > > +static void pl330_tasklet_cyclic(unsigned long data)
> > > > > > +{
> > > > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
> > > > > > + struct dma_pl330_desc *desc, *_dt;
> > > > > > + unsigned long flags;
> > > > > > + LIST_HEAD(list);
> > > > > > +
> > > > > > + spin_lock_irqsave(&pch->lock, flags);
> > > > > ...
> > > > > > + callback = desc->txd.callback;
> > > > > > + if (callback)
> > > > > > + callback(desc->txd.callback_param);
> > > > >
> > > > > On this again - what if the callback wants to terminate the DMA
> > > > > activity
> > > > > because there's no more audio data to be sent/received from the
> > > > > device?
> > > >
> > > > Do you mean what is happened if the callback() is called after channel
> > > > is
> > > > terminated ?
> > > > Or What is happened if Callback() calls 'dma_release_channel()' to
> > > > terminate
> > > > DMA?
> > >
> > > No. I mean what if the callback wants to call dmaengine_terminate_all().
> > you are supposed to drop the lock here, that way callback can call any
> > DMA API, otherwise it will result in deadlock.
> > This make me wonder you haven't read the documentation at all, please
> > ensure you have read Documentation/dmaengine.txt before next posting
>
> I know that very well thank you. Please look at my previous post in the
> previous round of patches, and the response from Boojin Kim to see why
> I used this as an *EXAMPLE* to get this fixed.
Russell,
Sorry that was not intended for you but for the author of patch Boojin
Kim... Agree on your EXAMPLE, just wanted to ensure authors have read it
as going thru this patch made it clear that they haven't.
--
~Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 05/14] ARM: SAMSUNG: Update to use PL330-DMA driver
> + DMACH_MTOM_4, > + DMACH_MTOM_5, > + DMACH_MTOM_6, > + DMACH_MTOM_7, > /* END Marker, also used to denote a reserved channel */ > DMACH_MAX, > }; > @@ -95,4 +102,4 @@ static inline bool s3c_dma_has_circular(void) > > #include > > -#endif /* __S3C_DMA_PL330_H_ */ > +#endif /* __DMA_PL330_H_ */ > diff --git a/arch/arm/plat-samsung/include/plat/s3c-pl330-pdata.h > b/arch/arm/plat-samsung/include/plat/s3c-pl330-pdata.h > index bf5e2a9..64fdf66 100644 > --- a/arch/arm/plat-samsung/include/plat/s3c-pl330-pdata.h > +++ b/arch/arm/plat-samsung/include/plat/s3c-pl330-pdata.h > @@ -12,7 +12,7 @@ > #ifndef __S3C_PL330_PDATA_H > #define __S3C_PL330_PDATA_H > > -#include > +#include > > /* > * Every PL330 DMAC has max 32 peripheral interfaces, -- ~Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability
On Mon, 2011-07-25 at 11:36 +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 25, 2011 at 07:31:45PM +0900, Boojin Kim wrote:
> > > On Mon, Jul 25, 2011 at 10:28:22AM +0900, Boojin Kim wrote:
> > > > +static void pl330_tasklet_cyclic(unsigned long data)
> > > > +{
> > > > + struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
> > > > + struct dma_pl330_desc *desc, *_dt;
> > > > + unsigned long flags;
> > > > + LIST_HEAD(list);
> > > > +
> > > > + spin_lock_irqsave(&pch->lock, flags);
> > > ...
> > > > + callback = desc->txd.callback;
> > > > + if (callback)
> > > > + callback(desc->txd.callback_param);
> > >
> > > On this again - what if the callback wants to terminate the DMA activity
> > > because there's no more audio data to be sent/received from the device?
> >
> > Do you mean what is happened if the callback() is called after channel is
> > terminated ?
> > Or What is happened if Callback() calls 'dma_release_channel()' to terminate
> > DMA?
>
> No. I mean what if the callback wants to call dmaengine_terminate_all().
you are supposed to drop the lock here, that way callback can call any
DMA API, otherwise it will result in deadlock.
This make me wonder you haven't read the documentation at all, please
ensure you have read Documentation/dmaengine.txt before next posting
>
> > > > + if (!pch->cyclic_task) {
> > > > + pch->cyclic_task =
> > > > + kmalloc(sizeof(struct tasklet_struct),
> > > > GFP_KERNEL);
> > > > + tasklet_init(pch->cyclic_task,
> > > > + pl330_tasklet_cyclic, (unsigned int)pch);
> > >
> > > Here you allocate memory for the cyclic task. Above you set this pointer
> > > to NULL. That sounds like a memory leak to me. Why are you kmallocing
> > > this memory - why can't it be part of the dma_pl330_chan structure? It's
> > > only 28 bytes.
> >
> > It's my mistake. I should have been free of the memory.
> >
> > And the reason why I use kmalloc for 'cyclic_task' is following.
> > This memory size for 'cyclic_tasklet' is the 896 bytes ( = the number of
> > channel * sizeof(struct tasklet_struct)= 32*28) for each DMAC. And This
> > memory size is increased according to the number of DMAC.
> > And Samsung has the DMAC that is dedicated for Mem-to-Mem operation. If I
> > make 'cyclic_task' be part of dma_pl330_chan, this DMAC that is dedicated
> > for Mem-to-Mem operation should hold unused data.
> > So, I think it's loss that all dma channels hold own 'cyclic_task'.
>
> Could you re-use the tasklet that already exists?
--
~Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote:
> This patch adds support DMA_SLAVE_CONFIG command.
>
> Signed-off-by: Boojin Kim
> ---
> drivers/dma/pl330.c | 53 +-
> 1 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 586ab39..880f010 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -256,25 +256,50 @@ static int pl330_alloc_chan_resources(struct dma_chan
> *chan)
> static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> unsigned long arg)
> {
> struct dma_pl330_chan *pch = to_pchan(chan);
> - struct dma_pl330_desc *desc;
> + struct dma_pl330_desc *desc, *_dt;
> unsigned long flags;
> + struct dma_pl330_dmac *pdmac = pch->dmac;
> + struct dma_slave_config *slave_config;
> + struct dma_pl330_peri *peri;
> + LIST_HEAD(list);
>
> - /* Only supports DMA_TERMINATE_ALL */
> - if (cmd != DMA_TERMINATE_ALL)
> - return -ENXIO;
> -
> - spin_lock_irqsave(&pch->lock, flags);
> -
> - /* FLUSH the PL330 Channel thread */
> - pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
> + switch (cmd) {
> + case DMA_TERMINATE_ALL:
> + spin_lock_irqsave(&pch->lock, flags);
>
> - /* Mark all desc done */
> - list_for_each_entry(desc, &pch->work_list, node)
> - desc->status = DONE;
> + /* FLUSH the PL330 Channel thread */
> + pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH);
>
> - spin_unlock_irqrestore(&pch->lock, flags);
> + /* Mark all desc done */
> + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) {
> + desc->status = DONE;
> + pch->completed = desc->txd.cookie;
> + list_move_tail(&desc->node, &list);
> + }
>
> - pl330_tasklet((unsigned long) pch);
> + list_splice_tail_init(&list, &pdmac->desc_pool);
> + spin_unlock_irqrestore(&pch->lock, flags);
> + break;
> + case DMA_SLAVE_CONFIG:
> + slave_config = (struct dma_slave_config *)arg;
> + peri = pch->chan.private;
> +
> + if (slave_config->direction == DMA_TO_DEVICE) {
> + if (slave_config->dst_addr)
> + peri->fifo_addr = slave_config->dst_addr;
> + if (slave_config->dst_addr_width)
> + peri->burst_sz =
> __ffs(slave_config->dst_addr_width);
This doesn't sound right, why can't you use xxx_maxburst field instead?
> + } else if (slave_config->direction == DMA_FROM_DEVICE) {
> + if (slave_config->src_addr)
> + peri->fifo_addr = slave_config->src_addr;
> + if (slave_config->src_addr_width)
> + peri->burst_sz =
> __ffs(slave_config->src_addr_width);
ditto...
> + }
> + break;
> + default:
> + dev_err(pch->dmac->pif.dev, "Not supported command.\n");
> + return -ENXIO;
> + }
>
> return 0;
> }
--
~Vinod Koul
Intel Corp.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND] [PATCH V4 0/14] To use DMA generic APIs for Samsung DMA
On Mon, 2011-07-25 at 10:28 +0900, Boojin Kim wrote: > This patchset adds support DMA generic APIs for Samsung DMA > and is re-sent by the request of DMA maintainer, Vinod Koul. > > V4 has the changes about 3 patches from V3. > Changes from V3: > - Divided '[03/13 patch] DMA: PL330: Add DMA capabilites' on V3 patchset > into two patches. > - One is '03/14 patch' for supporting DMA_SLAVE_CONFIG command. > - Another is '04/14 patch' for supporting DMA_CYCLIC capability. > - Code clean-up > - Remove unnecessary vairable referance on '01/14 patch'. > - Remove redunancy code on '06/14 patch' > > [PATCH V4 01/14] DMA: PL330: Add support runtime PM for PL330 DMAC > [PATCH V4 02/14] DMA: PL330: Update PL330 DMA API driver > [PATCH V4 03/14] DMA: PL330: Support DMA_SLAVE_CONFIG command > [PATCH V4 04/14] DMA: PL330: Add DMA_CYCLIC capability > [PATCH V4 05/14] ARM: SAMSUNG: Update to use PL330-DMA driver > [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations > [PATCH V4 07/14] ARM: EXYNOS4: Use generic DMA PL330 driver > [PATCH V4 08/14] ARM: S5PV210: Use generic DMA PL330 driver > [PATCH V4 09/14] ARM: S5PC100: Use generic DMA PL330 driver > [PATCH V4 10/14] ARM: S5P64X0: Use generic DMA PL330 driver > [PATCH V4 11/14] ARM: SAMSUNG: Remove S3C-PL330-DMA driver > [PATCH V4 12/14] spi/s3c64xx: Add support DMA engine API > [PATCH V4 13/14] ASoC: Samsung: Update DMA interface > [PATCH V4 14/14] ARM: SAMSUNG: Remove Samsung specific enum type for dma > direction > Thanks for resending I will review them now, You should use git --cover-letter to generate this -- ~Vinod Koul Intel Corp. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
