Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback
Hi Vinod, Hi Russell, On 11 December 2014 at 11:42, Jassi Brar jaswinder.si...@linaro.org wrote: On 11 December 2014 at 10:17, Vinod Koul vinod.k...@intel.com wrote: 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 ? The callback is set after prep() and before tx_submit(), but here we talk after tx_submit(). Perhaps the idea dates back to async-only days, when client drivers would prepare and queue descriptors in the controller driver rather than having to manage the dependency queues themselves (?). Today ~95% clients are slave and I am yet to find one that really can't work with submit and issue_pending tied together. Not to forget the 100% of the controller drivers have to manage 'submitted' and 'active' queues -- only to have arguably negative side-effects. If we agree that clubbing submit and issue_pending is the right thing to do, I can start converting the ~90 client drivers. Please let me know either way. Cheers! -- 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 11 December 2014 at 10:17, Vinod Koul vinod.k...@intel.com wrote: 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 ? The callback is set after prep() and before tx_submit(), but here we talk after tx_submit(). -- 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 8 December 2014 at 18:37, Vinod Koul vinod.k...@intel.com wrote: 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. awkward situation and ritual suffering above, are two practical issues that we see. 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... So we at least agree that the feature is unused so far. And I doubt if it would ever make sense to batch transfers in slave dma (unlike offloading on async-dma) because the user has to also setup the master for each queued request, mostly if not always. 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 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(). -Jassi -- 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 5 December 2014 at 20:48, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote: because the reasoning above seems incorrect considering the following documentation... Documentation/crypto/async-tx-api.txt says async-tx-api is not the DMA slave API. 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(). ^^ DMA slave users do not call this function. This documentation is not relevant for DMA slave. The APIs (device_issue_pending, tx_submit etc) are same for Slave and Async. async_tx_issue_pending_all() for Async and dma_async_issue_pending() for Slave both boil down to device_issue_pending() include/linux/dmaengine.h says dma_async_tx_descriptor.tx_submit(): set the prepared descriptor(s) to be executed by the engine It doesn't say when. :) OK so theoretically a driver, not starting transfer until issue_pending(), is broken. It isn't. DMA slave engine implementations have been needing the issue_pending() call since their dawn, so it's something that they've always needed. At best the patch@04abf5daf7d makes the driver slightly more complicated and the reason behind confusion such as in this thread. That may be, and yes, it _might_ be worth discussing whether this should be relaxed or not, but that should be done as a proposal, not trying to hide it as a bug fix. It isn't. 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()? 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] dmaengine: pl330: Set residue in tx_status callback
On 3 December 2014 at 10:17, Padma Venkat padma@gmail.com 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
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 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] cpufreq: exynos: Add exynos5420 cpufreq driver
On Tue, Dec 10, 2013 at 10:02 PM, Lukasz Majewski l.majew...@samsung.com wrote: Actually these values are not for PLL, but for the dividers. If you see below, the PLL rate setting is done through clk_set_rate() going via CCF. But I found an issue if the divider values are set via clk_set_rate API. What I found is, the system goes into freeze if all the divider values are not set in one shot. So we cannot call multiple clk_set_rate()'s on each divider. Thats why I continued with non-CCF way of setting the divider. I see. I'm not an expert on common clock framework (CCF), but for me conceptually clock dividers setting shall be handled by CCF. However, I've poked a bit at CCF. There isn't any out of the box solutions available. A virtual clock can be defined and inside its implementation we can atomically set dividers. Another solution would be to hack the current CCF to provide atomic clock operations CCF isn't only for clocks that have single divider or gate control register. One could define a clock that manipulates more than one divider/gate in one CCF callback. It is already abstract enough. So implementing a virtual clock is the solution, imho. BTW, on my platform linux needs to send a 'non-atomic' IPC message to a master co-processor to scale up/down cpufreq. I just define a virtual clock and use the generic bL cpufreq driver drivers/cpufreq/arm_big_little.c -- 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] Support device_prep_dma_sg for pl330
On Tue, Apr 2, 2013 at 5:31 PM, Chanho Park chanho61.p...@samsung.com wrote: This patchset support device_prep_dma_sg functionality for pl330 dmaengine driver. The prep_dma_sg function prepares descriptors to transfer between src and dest memory which consists of scatter/gather list. The patchset contains code clean-up to eliminate duplications. Chanho Park (2): dma: pl330: split off common code to give back descriptors dma: pl330: add pl330_prep_dma_sg to transfer from sglist to sglist drivers/dma/pl330.c | 209 ++- 1 file changed, 157 insertions(+), 52 deletions(-) Acked-by : Jassi Brar jassisinghb...@gmail.com -- 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 4/4] DMA: PL330: add device tree property for DMA_MEMCPY capability
On Fri, Nov 30, 2012 at 4:26 PM, Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com wrote: Thank you for explaining it. Here is a patch implementing the idea: From: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com Subject: [PATCH] DMA: PL330: add peripherals map to the device tree Add device tree (DT) property (peri-map) for storing indices of peripherals connected to DMAC and fix DT nodes of client drivers to use 'dma peripheral id' instead of 'dma request id'. Also instead of setting DMA_MEMCPY capability unconditionally in pl330_probe() do it only when peri-map DT property is present (idea from Jassi Brar). It fixes the issue on ARM EXYNOS platforms using DT where pdma controller erroneously was used for DMA_MEMCPY operations instead of mdma one (it seems to work correctly but at the cost of worse performance). Sorry, we need to change it as per new generic dma DT bindings http://git.infradead.org/users/vkoul/slave-dma.git/blob/refs/heads/next:/Documentation/devicetree/bindings/dma/dma.txt 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 4/4] DMA: PL330: add device tree property for DMA_MEMCPY capability
On 30 October 2012 14:51, Bartlomiej Zolnierkiewicz
b.zolnier...@samsung.com wrote:
Hi,
On Monday 29 October 2012 22:45:48 Jassi Brar wrote:
On Mon, Oct 29, 2012 at 10:59 AM, Bartlomiej Zolnierkiewicz
b.zolnier...@samsung.com wrote:
* Add device tree (DT) property (pl330,dma-memcpy) for DMA_MEMCPY
capability and instead of setting this capability unconditionally
in pl330_probe() do it only when property is present.
Perhaps we should pass the array of peripheral interfaces via DT, the
lack of which could imply MEMCPY capability ? (while it works, I doubt
if pl330 is supposed to have SLAVE and MEMCPY capabilities in any
instance)
In case of PL330 on EXYNOS4 we have two interfaces with SLAVE capability
and one interface with MEMCPY capability. Could you please explain more
the idea of passing the array of peripherals through DT so we can detect
which interface has MEMCPY capability?
The DT node of a 'pdma' should have the array of indices of
peripherals it caters to (what is currently peri_id of 'struct
dma_pl330_platdata'). The array would be missing in the DT node of
'mdma' since all channels are equal.
During probe if the array, say as property 'peri_map', is missing from
DT node of the dmac, that would imply the dmac is 'mdma' and hence the
pl330.c sets DMA_MEMCPY in its cap_mask. Otherwise the peri_map
implies a 'pdma' and hence SLAVE|CYCLIC is set.
That would also be a step towards discarding struct dma_pl330_platdata.
I don't know if getting rid of struct dma_pl330_platdata is possible
but we still need to come up with some way to pass the needed information
through DT. Do you have an idea how it could be done?
struct dma_pl330_platdata {
u8 nr_valid_peri;
u8 *peri_id;
As explain above, these two should move to DT node of the dma controller.
dma_cap_mask_t cap_mask;
Should be set in pl330.c : MEMCPY for mdma, SLAVE|CYCLIC for pdma
unsigned mcbuf_sz;
Currently unused and already safe enough default value set in driver.
}
--
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 4/4] DMA: PL330: add device tree property for DMA_MEMCPY capability
On Mon, Oct 29, 2012 at 10:59 AM, Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com wrote: * Add device tree (DT) property (pl330,dma-memcpy) for DMA_MEMCPY capability and instead of setting this capability unconditionally in pl330_probe() do it only when property is present. Perhaps we should pass the array of peripheral interfaces via DT, the lack of which could imply MEMCPY capability ? (while it works, I doubt if pl330 is supposed to have SLAVE and MEMCPY capabilities in any instance) That would also be a step towards discarding struct dma_pl330_platdata. -- 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 Mon, Oct 29, 2012 at 10:59 AM, Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com wrote: tasklet_kill() may sleep so call it before taking pch-lock. Fixes following lockup: [ 345.47] BUG: scheduling while atomic: cat/2383/0x0002 [ 345.47] Modules linked in: [ 345.47] [c0015858] (unwind_backtrace+0x0/0xfc) from [c004d980] (__schedule_bug+0x4c/0x58) [ 345.47] [c004d980] (__schedule_bug+0x4c/0x58) from [c0360b6c] (__schedule+0x690/0x6e0) [ 345.47] [c0360b6c] (__schedule+0x690/0x6e0) from [c004f2b4] (sys_sched_yield+0x70/0x78) [ 345.47] [c004f2b4] (sys_sched_yield+0x70/0x78) from [c002acec] (tasklet_kill+0x34/0x8c) [ 345.47] [c002acec] (tasklet_kill+0x34/0x8c) from [c01da4cc] (pl330_free_chan_resources+0x24/0x88) [ 345.47] [c01da4cc] (pl330_free_chan_resources+0x24/0x88) from [c01d81f4] (dma_chan_put+0x4c/0x50) [ 345.47] [c01d81f4] (dma_chan_put+0x4c/0x50) from [c01d82c0] (dma_release_channel+0x28/0x98) [...] [ 368.335000] BUG: spinlock lockup suspected on CPU#0, swapper/0/0 [ 368.34] lock: 0xe52aa04c, .magic: dead4ead, .owner: cat/2383, .owner_cpu: 1 [ 368.35] [c0015858] (unwind_backtrace+0x0/0xfc) from [c01b3d78] (do_raw_spin_lock+0x194/0x204) [ 368.36] [c01b3d78] (do_raw_spin_lock+0x194/0x204) from [c0361adc] (_raw_spin_lock_irqsave+0x20/0x28) [ 368.365000] [c0361adc] (_raw_spin_lock_irqsave+0x20/0x28) from [c01da80c] (pl330_tasklet+0x2c/0x5a8) [ 368.375000] [c01da80c] (pl330_tasklet+0x2c/0x5a8) from [c002ac04] (tasklet_action+0xfc/0x114) [ 368.385000] [c002ac04] (tasklet_action+0xfc/0x114) from [c002b204] (__do_softirq+0xe4/0x19c) [ 368.395000] [c002b204] (__do_softirq+0xe4/0x19c) from [c002b398] (irq_exit+0x98/0x9c) [ 368.405000] [c002b398] (irq_exit+0x98/0x9c) from [c0013ebc] (handle_IPI+0x124/0x16c) [ 368.41] [c0013ebc] (handle_IPI+0x124/0x16c) from [c000857c] (gic_handle_irq+0x64/0x68) [ 368.42] [c000857c] (gic_handle_irq+0x64/0x68) from [c000e740] (__irq_svc+0x40/0x70) [ 368.43] Exception stack(0xc04a3f00 to 0xc04a3f48) [ 368.435000] 3f00: c04a3f48 6f9e23e8 0050 c07492c8 c04a3f48 c04ccc88 [ 368.44] 3f20: 6f9dbac3 0050 6f9e23e8 0050 3b9aca00 c04a3f48 c005cfa4 c02946d4 [ 368.45] 3f40: 6013 [ 368.455000] [c000e740] (__irq_svc+0x40/0x70) from [c02946d4] (cpuidle_wrap_enter+0x4c/0xa0) [ 368.46] [c02946d4] (cpuidle_wrap_enter+0x4c/0xa0) from [c02940dc] (cpuidle_enter_state+0x18/0x68) [ 368.47] [c02940dc] (cpuidle_enter_state+0x18/0x68) from [c02948e0] (cpuidle_idle_call+0xac/0xe0) [ 368.48] [c02948e0] (cpuidle_idle_call+0xac/0xe0) from [c00102f8] (cpu_idle+0xac/0xf0) [ 368.49] [c00102f8] (cpu_idle+0xac/0xf0) from [c04796a0] (start_kernel+0x28c/0x294) Cc: Jassi Brar jassisinghb...@gmail.com Cc: Vinod Koul vinod.k...@linux.intel.com Cc: Tomasz Figa t.f...@samsung.com Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/dma/pl330.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 665668b..db7574b 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -2459,10 +2459,10 @@ static void pl330_free_chan_resources(struct dma_chan *chan) struct dma_pl330_chan *pch = to_pchan(chan); unsigned long flags; - spin_lock_irqsave(pch-lock, flags); - tasklet_kill(pch-task); + spin_lock_irqsave(pch-lock, flags); + pl330_release_channel(pch-pl330_chid); pch-pl330_chid = NULL; Thanks. Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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 0/4] DMA: PL330: Fix mem leaks and balance probe/remove
On Fri, Oct 5, 2012 at 3:47 PM, Inderpal Singh inderpal.si...@linaro.org wrote: The first 2 patches of this series fix memory leaks because the memory allocated for peripheral channels and DMA descriptors were not getting freed. The last 2 patches balance the module's remove function. This series depends on 61c6e7531d3b66b3 DMA: PL330: Check the pointer returned by kzalloc which is on slave-dma's fixes branch. Hence slave-dma tree's next branch was merged with fixes and applied patch at [1] to fix the build error. [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274 Changes since v1: - Protect only list_add_tail with spin_locks - Return EBUSY from remove if channel is in use - unregister dma_device in remove Inderpal Singh (4): DMA: PL330: Free memory allocated for peripheral channels DMA: PL330: Change allocation method to properly free DMA descriptors DMA: PL330: Balance module remove function with probe DMA: PL330: unregister dma_device in module's remove function drivers/dma/pl330.c | 53 --- 1 file changed, 38 insertions(+), 15 deletions(-) All seem fine. Acked-by: Jassi Brar jassisinghb...@gmail.com 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 3/3] DMA: PL330: Balance module remove function with probe
On Fri, Sep 28, 2012 at 10:03 AM, Inderpal Singh
inderpal.si...@linaro.org wrote:
Now, if we have to check if any client is using the channel and then
decide. We will have to traverse the channel list twice once to check
the usage and second time to delete the nodes from the list if we go
ahead with remove.
The remove will look like below:
@@ -3008,18 +3008,19 @@ 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_safe(pch, _p, pdmac-ddma.channels,
+ chan.device_node) {
+
+ if (pch-chan.client_count)
+ return -EBUSY;
+ }
+
The iteration doesn't have to be the 'safe' version here. Other than
that it seems OK.
--
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, Sep 26, 2012 at 12:11 PM, Inderpal Singh
inderpal.si...@linaro.org wrote:
How about conditionally DMA_TERMINATE_ALL and free resources like below ?
@@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
amba_device *adev)
/* 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);
+ if (pch-chan.client_count != 0) {
+ /* Flush the channel */
+ pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
+ pl330_free_chan_resources(pch-chan);
+ }
}
It is perfectly safe to flush the channels that have client_count == 0
as well. Which is already the case.
--
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/3] DMA: PL330: Change allocation method to properly free DMA descriptors
On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
inderpal.si...@linaro.org 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
Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
drivers/dma/pl330.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..04d83e6 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,19 @@ 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;
-
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);
+ desc = kmalloc(sizeof(*desc), flg);
+ if (!desc)
+ break;
+ _init_desc(desc);
+ list_add_tail(desc-node, pdmac-desc_pool);
}
spin_unlock_irqrestore(pdmac-pool_lock, flags);
- return count;
+ return i;
}
OK, but the kmalloc shouldn't be done with irqs disabled. Please
protect only the list_add_tail()
thanks.
static struct dma_pl330_desc *
@@ -2857,6 +2856,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 +2978,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 +3000,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 +3022,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);
--
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 3/3] DMA: PL330: Balance module remove function with probe
On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
inderpal.si...@linaro.org wrote:
Since peripheral channel resources are not being allocated at probe,
no need to flush the channels and free the resources in remove function.
Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
drivers/dma/pl330.c |8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 04d83e6..6f06080 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device
*adev)
/* Idle the DMAC */
list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
- chan.device_node) {
-
+ 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);
- }
-
while (!list_empty(pdmac-desc_pool)) {
desc = list_entry(pdmac-desc_pool.next,
struct dma_pl330_desc, node);
I am not sure about this patch. The DMA_TERMINATE_ALL is only called
by the client and if the pl330 module is forced unloaded while some
client is queued, we have to manually do DMA_TERMINATE_ALL.
A better option could be to simply fail pl330_remove if some client is
queued on the DMAC.
-jassi
--
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: return ENOMEM instead of 0 from pl330_alloc_chan_resources
On Mon, Sep 17, 2012 at 9:57 AM, Inderpal Singh
inderpal.si...@linaro.org wrote:
Since 0 is not considered as error at dmaengine level, return ENOMEM
from pl330_alloc_chan_resources in case of failure.
Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
Acked-by: Jassi Brar jassisinghb...@gmail.com
---
drivers/dma/pl330.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index e4feba6..14d881c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2393,7 +2393,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);
- return 0;
+ return -ENOMEM;
}
tasklet_init(pch-task, pl330_tasklet, (unsigned long) pch);
--
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 07/10] spi: s3c64xx: Remove the 'set_level' callback from controller data
On Wed, May 9, 2012 at 3:34 AM, Thomas Abraham thomas.abra...@linaro.org wrote: The set_level callback in the controller data, which is used to configure the slave select line, cannot be supported when migrating the driver to device tree based discovery. Since all the platforms currently use gpio as the slave select line, this callback can be removed from the controller data and replaced with call to gpio_set_value in the driver. This patch is ok. Separately, you might also want to see if we really need the air-tight protection of spin_lock_irqsave around enable_cs - IIRC someone had a CS coming out of an gpio extender connected over I2C -- 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 00/10] spi: s3c64xx: add support for device tree
On Wed, May 9, 2012 at 3:34 AM, Thomas Abraham
thomas.abra...@linaro.org wrote:
This patch series adds device tree based discovery support for Samsung's
s3c64xx compatible spi controller. This is mainly tested for Exynos4210
and Exynos5250 with onboard spi nor flash device.
This patch series is based on Linux 3.4-rc5 with the following two
patch series applied.
[1]
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg10494.html
[PATCH 00/20] ARM: Samsung: Add support for Exynos5250 Rev1.0
[2]
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg09640.html
[PATCH 0/6] S3C24XX: Add support for HSSPI on S3C2416/S3C2443
Thomas Abraham (10):
spi: s3c64xx: remove unused S3C64XX_SPI_ST_TRLCNTZ macro
spi: s3c64xx: move controller information into driver data
ARM: Samsung: Remove spi hardware controller information from platform data
ARM: Samsung: Remove pdev pointer paremeter from spi gpio setup functions
ARM: Samsung: Update the device names for spi clock lookup
ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
spi: s3c64xx: Remove the 'set_level' callback from controller data
ARM: Exynos4: Fix the incorrect hierarchy of spi controller bus clock
ARM: Exynos5: Add spi clock support
spi: s3c64xx: add device tree support
Look ok to me. FWIW, Acked-by: Jassi Brar jassisinghb...@gmail.com
--
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 07/10] spi: s3c64xx: Remove the 'set_level' callback from controller data
On 9 May 2012 14:50, Heiko Stübner he...@sntech.de wrote: Am Mittwoch, 9. Mai 2012, 00:04:51 schrieb Thomas Abraham: The set_level callback in the controller data, which is used to configure the slave select line, cannot be supported when migrating the driver to device tree based discovery. Since all the platforms currently use gpio as the slave select line, this callback can be removed from the controller data and replaced with call to gpio_set_value in the driver. I was quite fond of the set_level callback. On one of the machines I'm working on there is some sort of multiplexer (TI-sn74cbtlv3257) sitting between the controller and spi devices to provide support for more than one device. So I was doing the switch to the correct device in the set_level callback, which worked quite well. I suppose you should still be able to do that defining virtual gpios backed by appropriate physical mux'ing of lines ? -jassi -- 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: spi-s3c64xx: problems with SPI_MODE_3 transfers
On Mon, Apr 9, 2012 at 12:09 AM, Heiko Stübner he...@sntech.de wrote: [@jassi: I know that you do not work at this stuff any longer, but I included you in the list of recipients in the hope that you might be able to give me a pointer on where to look for the culprit of the problem :-) ] Hi, while working on the beginnings of a wireless driver connected via SPI [1], the s3c64xx-spi-driver produced strange problems while doing MODE_3 (CPOL | CPHA) transfers: [2] [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 [read] spi cur_bpw: 8: 0x00 0x00 0x00 0x00 [read] spi cur_bpw: 8: 0x00 0x00 0x00 0x00 (chip_id = 0x, chip_rev = 0x00) mapped channel 24 to 0 mapped channel 23 to 1 [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 [read] spi cur_bpw: 8: 0x00 0x00 0x00 0x00 [read] spi cur_bpw: 8: 0x00 0x00 0x00 0x00 CIR: 0 [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 [read] spi cur_bpw: 8: 0x00 0x00 0x00 0x00 mt592x_spi spi0.0: I/O Error: rx-1 tx-0 res:rx-f tx-p len-4 CIR: 0 [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 mt592x_spi spi0.0: I/O Error: rx-0 tx-1 res:rx-p tx-f len-4 CIR: 0 [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 mt592x_spi spi0.0: I/O Error: rx-0 tx-1 res:rx-p tx-f len-4 CIR: 0 [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 mt592x_spi spi0.0: I/O Error: rx-0 tx-1 res:rx-p tx-f len-4 CIR: 0 where the expected result would be: [write] spi cur_bpw: 8: 0x00 0x00 0x04 0x00 [read] spi cur_bpw: 8: 0x00 0x00 0x00 0x00 [read] spi cur_bpw: 8: 0x21 0x59 0x91 0x00 The pattern of either empty results or read errors also varies with each invocation, i.e. sometimes all fail or all result in wrong values and so on. In contrast when doing this with the spi-gpio driver, everything works as expected and I get the expected results. Also the device-family I'm working on contains a variant with a Marvell 8686 wlan chip [= the rest of the system is the same]. Using the spi-s3c64xx with the libertas driver (MODE_0) works as expected. As I'm not sure where I should look for errors in the driver, I would be glad to get pointers in the right direction. You might want to play around tuning the 'fb_delay' and maybe signal strength too. -j -- 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, Feb 22, 2012 at 3:57 PM, Vinod Koul vinod.k...@intel.com 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!! Read the thread again. Nobody is claiming the copyright, it rests with Samsung as before. Kukjin thinks merging two files is a serious enough change to warrant co-authorship, which I disagree. (The line below 'Copyright' tells the author, and its email, of the file). Jassi, your claim on being author is certainly right and you name should be there as MODULE_AUTHOR. Please check, it is there indeed. Also, If you had a Authors line added and that was removed, then I would have agreed with you. Equally objectionable is any tom, dick or harry claiming co-authorship of a code(f3.c) that is almost an equivalent of 'cat f1.c f2.c f3.c' I am equally pissed off by the fact that Kukjin/Boojin sneaked in the co-authorship in this revision while carrying over the Acked-by's from previous revision as if it has already been approved by others. -- 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, Feb 22, 2012 at 10:21 PM, Kukjin Kim kgene@samsung.com wrote: * Copyright (c) 2012 Samsung Electronics Co., Ltd. * http://www.samsung.com * * Copyright (C) 2010 Samsung Electronics Co. Ltd. * Jaswinder Singh jassi.b...@samsung.com * The following is more appropriate - * Copyright (C) 2010 Samsung Electronics Co. Ltd. + * Copyright (C) 2010 - 2012 Samsung Electronics Co. Ltd. * Jaswinder Singh jassi.b...@samsung.com * Based on: * arch/arm/common/pl330.c, arch/arm/include/asm/hardware/pl330.h * Copyright (C) 2010 Samsung Electronics Co Ltd. Doesn't make any sense because arch/arm/common/pl330.c and arch/arm/include/asm/hardware/pl330.h wouldn't exist anymore, and the changelog already provides that info. -- 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, Feb 22, 2012 at 10:59 PM, Kukjin Kim kgene@samsung.com wrote: * Copyright (c) 2012 Samsung Electronics Co., Ltd. * http://www.samsung.com * * Copyright (C) 2010 Samsung Electronics Co. Ltd. * Jaswinder Singhjassi.b...@samsung.com * The following is more appropriate - * Copyright (C) 2010 Samsung Electronics Co. Ltd. + * Copyright (C) 2010 - 2012 Samsung Electronics Co. Ltd. * Jaswinder Singhjassi.b...@samsung.com Well, as I know, I don't have to change/update original your copyright. If required, you should do it, next time. FYI that that is not my copyright. That is Samsung's copyright. Jaswinder Singhjassi.b...@samsung.com only claims to be the author. And I don't expect you to correct my email id (we have to suffer an extra mundane patch) I think you can agree following. Let's finish this talking. * Copyright (c) 2012 Samsung Electronics Co., Ltd. * http://www.samsung.com * and 'original copyright'... Doesn't make sense. Go look at other files in the kernel to find how it's done (if you must update the year). By your logic, next year someone would add + * Copyright (c) 2013 Samsung Electronics Co., Ltd. + * http://www.samsung.com and another year later + * Copyright (c) 2014 Samsung Electronics Co., Ltd. + * http://www.samsung.com and so on As I said, simply updating the current year in the same line works. - * Copyright (C) 2010 Samsung Electronics Co. Ltd. + * Copyright (C) 2010 - 2012 Samsung Electronics Co. Ltd. * Jaswinder Singhjassi.b...@samsung.com And don't worry, you won't be doing me any favor by doing that. -- 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] ARM: S3C24XX: Move s3c2443-clock.c to mach-s3c24xx
On 22 February 2012 22:58, Heiko Stübner he...@sntech.de wrote: No objections on the patch, but please use 'git format-patch -M' when moving files so that we can see which parts have changed in the process. the results of -M look really nice, thanks for the hint and I will try to remember it :-) The -M option is as easy to forget as it is useful. I wonder if that could be made the default with format-patch ? -j -- 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 Thu, Feb 16, 2012 at 2:57 AM, Kukjin Kim kgene@samsung.com wrote: From: Boojin Kim boojin@samsung.com Currently there were two part of DMAC PL330 driver for support old styled s3c-pl330 which has been merged into drivers/dma/pl330.c driver. Actually, there is no reason to separate them now. Basically this patch merges arch/arm/common/pl330.c into drivers/dma/pl330.c driver and removes useless exported symbol, externed function and so on. The newer pl330 driver tested on SMDKV310 and SMDK4212 boards Cc: Jassi Brar jassisinghb...@gmail.com Cc: Russell King rmk+ker...@arm.linux.org.uk Acked-by: Linus Walleij linus.wall...@linaro.org Acked-by: Vinod Koul vinod.k...@intel.com Signed-off-by: Boojin Kim boojin@samsung.com Signed-off-by: Kukjin Kim kgene@samsung.com --- Changes since v2: - Address comments from Russell King the 'asm/hardware/pl330.h' is moved to 'drivers/dma/pl330.h' which is used only in drivers/dma/ arch/arm/common/Kconfig | 3 - arch/arm/common/Makefile | 1 - arch/arm/common/pl330.c | 1959 - arch/arm/include/asm/hardware/pl330.h | 217 drivers/dma/Kconfig | 1 - drivers/dma/pl330.c | 1892 +++- drivers/dma/pl330.h | 251 + include/linux/amba/pl330.h | 1 - 8 files changed, 2142 insertions(+), 2183 deletions(-) delete mode 100644 arch/arm/common/pl330.c delete mode 100644 arch/arm/include/asm/hardware/pl330.h create mode 100644 drivers/dma/pl330.h I repeat yet again, the drivers/dma/pl330.h is included _only_ by drivers/dma/pl330.c so it'll be better to simply move its content in the c file and reduce file count by 1. diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index b8ec03e..65c5f24 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -1,4 +1,7 @@ -/* linux/drivers/dma/pl330.c +/* + * Copyright (c) 2012 Samsung Electronics Co., Ltd. + * http://www.samsung.com + * Boojin Kim boojin@samsung.com * I object. If simply moving the code around grants you share of authorship, then every person who contributed even a single actual bug fix or feature qualifies too. Javi Merino from ARM Ltd contributed far more important patches to PL330 and he never claimed authorship (though I think, if not me, he would have done the driver just as well or better). -- 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, Feb 22, 2012 at 5:52 AM, Kukjin Kim kgene@samsung.com wrote: And since she has been doing these updates on behalf of Samsung, she has added attribution to Samsung also (which you never did while at Samsung). What the bloody fuck ?!! Can't you read this ? . - /* linux/drivers/dma/pl330.c * * Copyright (C) 2010 Samsung Electronics Co. Ltd. * Jaswinder Singh jassi.b...@samsung.com * - I challenge you to show 1 file that I wrote while in Samsung which doesn't show Samsung as the copyright holder. And, I don't think some patches are more important than the other. All patches, simple or complex, help in improving the code. Bloody rant. Who said any type of patch is useless or doesn't improve code?! I said, and still say, merging two files written by same person doesn't grant you the right to add your own name as one of the authors. And if you think Boojin's patches were any great (even after discounting the number of iterations they took), Javi Merino's name should be added before Boojin's. -- 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, Feb 22, 2012 at 7:41 AM, Kukjin Kim kgene@samsung.com wrote: Can't you read this ? . As I know, the Copyright can belong to a person or company. See my patches in mainline. I meant that. Of course, it depends on writer. - /* linux/drivers/dma/pl330.c * * Copyright (C) 2010 Samsung Electronics Co. Ltd. * Jaswinder Singh jassi.b...@samsung.com * - I challenge you to show 1 file that I wrote while in Samsung which doesn't show Samsung as the copyright holder. See above. What crap, dude ?! So you mean (a) is correct crediting while (b), which already exists, doesn't give enough credit to Samsung ? Bloody double standards ! (a) + * Copyright (c) 2012 Samsung Electronics Co., Ltd. + * http://www.samsung.com + * Boojin Kim boojin@samsung.com (b) * Copyright (C) 2010 Samsung Electronics Co. Ltd. * Jaswinder Singh jassi.b...@samsung.com -- 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: PL330: Fix driver freeze
On 12 December 2011 00:57, Javi Merino javi.mer...@arm.com wrote:
Add a req_running field to the pl330_thread to track which request (if
any) has been submitted to the DMA. This mechanism replaces the old
one in which we tried to guess the same by looking at the PC of the
DMA, which could prevent the driver from sending more requests if it
didn't guess correctly.
Signed-off-by: Javi Merino javi.mer...@arm.com
Cc: Jassi Brar jaswinder.si...@linaro.org
---
arch/arm/common/pl330.c | 116
---
1 files changed, 49 insertions(+), 67 deletions(-)
diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..8d8df74 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -221,17 +221,6 @@
*/
#define MCODE_BUFF_PER_REQ 256
-/*
- * Mark a _pl330_req as free.
- * We do it by writing DMAEND as the first instruction
- * because no valid request is going to have DMAEND as
- * its first instruction to execute.
- */
-#define MARK_FREE(req) do { \
- _emit_END(0, (req)-mc_cpu); \
- (req)-mc_len = 0; \
- } while (0)
-
/* If the _pl330_req is available to the client */
#define IS_FREE(req) (*((u8 *)((req)-mc_cpu)) == CMD_DMAEND)
@@ -301,8 +290,10 @@ struct pl330_thread {
struct pl330_dmac *dmac;
/* Only two at a time */
struct _pl330_req req[2];
- /* Index of the last submitted request */
+ /* Index of the last enqueued request */
unsigned lstenq;
+ /* Index of the last submitted request or -1 if the DMA is stopped */
+ int req_running;
};
enum pl330_dmac_state {
@@ -778,6 +769,22 @@ static inline void _execute_DBGINSN(struct pl330_thread
*thrd,
writel(0, regs + DBGCMD);
}
+/*
+ * Mark a _pl330_req as free.
+ * We do it by writing DMAEND as the first instruction
+ * because no valid request is going to have DMAEND as
+ * its first instruction to execute.
+ */
+static void mark_free(struct pl330_thread *thrd, int idx)
+{
+ struct _pl330_req *req = thrd-req[idx];
+
+ _emit_END(0, req-mc_cpu);
+ req-mc_len = 0;
+
+ thrd-req_running = -1;
+}
+
static inline u32 _state(struct pl330_thread *thrd)
{
void __iomem *regs = thrd-dmac-pinfo-base;
@@ -836,31 +843,6 @@ static inline u32 _state(struct pl330_thread *thrd)
}
}
-/* If the request 'req' of thread 'thrd' is currently active */
-static inline bool _req_active(struct pl330_thread *thrd,
- struct _pl330_req *req)
-{
- void __iomem *regs = thrd-dmac-pinfo-base;
- u32 buf = req-mc_bus, pc = readl(regs + CPC(thrd-id));
-
- if (IS_FREE(req))
- return false;
-
- return (pc = buf pc = buf + req-mc_len) ? true : false;
-}
-
-/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
-static inline unsigned _thrd_active(struct pl330_thread *thrd)
-{
- if (_req_active(thrd, thrd-req[0]))
- return 1; /* First req active */
-
- if (_req_active(thrd, thrd-req[1]))
- return 2; /* Second req active */
-
- return 0;
-}
-
static void _stop(struct pl330_thread *thrd)
{
void __iomem *regs = thrd-dmac-pinfo-base;
@@ -892,17 +874,22 @@ static bool _trigger(struct pl330_thread *thrd)
struct _arg_GO go;
unsigned ns;
u8 insn[6] = {0, 0, 0, 0, 0, 0};
+ int idx;
/* Return if already ACTIVE */
if (_state(thrd) != PL330_STATE_STOPPED)
return true;
- if (!IS_FREE(thrd-req[1 - thrd-lstenq]))
- req = thrd-req[1 - thrd-lstenq];
- else if (!IS_FREE(thrd-req[thrd-lstenq]))
- req = thrd-req[thrd-lstenq];
- else
- req = NULL;
+ idx = 1 - thrd-lstenq;
+ if (!IS_FREE(thrd-req[idx]))
+ req = thrd-req[idx];
+ else {
+ idx = thrd-lstenq;
+ if (!IS_FREE(thrd-req[idx]))
+ req = thrd-req[idx];
+ else
+ req = NULL;
+ }
/* Return if no request */
if (!req || !req-r)
@@ -933,6 +920,8 @@ static bool _trigger(struct pl330_thread *thrd)
/* Only manager can execute GO */
_execute_DBGINSN(thrd, insn, true);
+ thrd-req_running = idx;
+
return true;
}
@@ -1382,8 +1371,8 @@ static void pl330_dotask(unsigned long data)
thrd-req[0].r = NULL;
thrd-req[1].r = NULL;
- MARK_FREE(thrd-req[0]);
- MARK_FREE(thrd-req[1]);
+ mark_free(thrd, 0);
+ mark_free(thrd, 1);
/* Clear the reset flag */
pl330-dmac_tbd.reset_chan = ~(1 i);
@@ -1461,14 +1450,12
Re: [PATCH 2/2] DMA: PL330: Removes useless function
On Thu, Dec 8, 2011 at 1:53 PM, Kukjin Kim kgene@samsung.com wrote:
From: Boojin Kim boojin@samsung.com
Cc: Jassi Brar jassisinghb...@gmail.com
Cc: Linus Walleij linus.wall...@linaro.org
Cc: Vinod Koul vinod.k...@intel.com
Signed-off-by: Boojin Kim boojin@samsung.com
Signed-off-by: Kukjin Kim kgene@samsung.com
---
drivers/dma/pl330.c | 47 ---
1 files changed, 0 insertions(+), 47 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 35f0904..b917477 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1512,53 +1512,6 @@ static int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)
return ret;
}
-static int pl330_chan_status(void *ch_id, struct pl330_chanstatus *pstatus)
-{
- struct pl330_thread *thrd = ch_id;
- struct pl330_dmac *pl330;
- struct pl330_info *pi;
- void __iomem *regs;
- int active;
- u32 val;
-
- if (!pstatus || !thrd || thrd-free)
- return -EINVAL;
-
- pl330 = thrd-dmac;
- pi = pl330-pinfo;
- regs = pi-base;
-
- /* The client should remove the DMAC and add again */
- if (pl330-state == DYING)
- pstatus-dmac_halted = true;
- else
- pstatus-dmac_halted = false;
-
- val = readl(regs + FSC);
- if (val (1 thrd-id))
- pstatus-faulting = true;
- else
- pstatus-faulting = false;
-
- active = _thrd_active(thrd);
-
- if (!active) {
- /* Indicate that the thread is not running */
- pstatus-top_req = NULL;
- pstatus-wait_req = NULL;
- } else {
- active--;
- pstatus-top_req = thrd-req[active].r;
- pstatus-wait_req = !IS_FREE(thrd-req[1 - active])
- ? thrd-req[1 - active].r : NULL;
- }
-
- pstatus-src_addr = readl(regs + SA(thrd-id));
- pstatus-dst_addr = readl(regs + DA(thrd-id));
-
- return 0;
-}
NAK.
Recently Samsung ASoC regressed to update DMA pointer only at period boundaries.
I am sure other platforms would like to have low audio latencies and
hence need this function.
--
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] ARM: pl330: Fix a race condition
On Sat, Dec 10, 2011 at 1:20 AM, Javi Merino javi.mer...@arm.com wrote:
What about properly tracking what we have sent to the DMA? Something
like the following (warning *ugly* and untested code ahead, may eat your
kitten):
diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index f407a6b..3652c4b 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -303,6 +303,7 @@ struct pl330_thread {
struct _pl330_req req[2];
/* Index of the last submitted request */
unsigned lstenq;
+ int req_running;
};
enum pl330_dmac_state {
@@ -836,31 +837,6 @@ static inline u32 _state(struct pl330_thread *thrd)
}
}
-/* If the request 'req' of thread 'thrd' is currently active */
-static inline bool _req_active(struct pl330_thread *thrd,
- struct _pl330_req *req)
-{
- void __iomem *regs = thrd-dmac-pinfo-base;
- u32 buf = req-mc_bus, pc = readl(regs + CPC(thrd-id));
-
- if (IS_FREE(req))
- return false;
-
- return (pc = buf pc = buf + req-mc_len) ? true : false;
-}
-
-/* Returns 0 if the thread is inactive, ID of active req + 1 otherwise */
-static inline unsigned _thrd_active(struct pl330_thread *thrd)
-{
- if (_req_active(thrd, thrd-req[0]))
- return 1; /* First req active */
-
- if (_req_active(thrd, thrd-req[1]))
- return 2; /* Second req active */
-
- return 0;
-}
-
static void _stop(struct pl330_thread *thrd)
{
void __iomem *regs = thrd-dmac-pinfo-base;
@@ -897,11 +873,13 @@ static bool _trigger(struct pl330_thread *thrd)
if (_state(thrd) != PL330_STATE_STOPPED)
return true;
- if (!IS_FREE(thrd-req[1 - thrd-lstenq]))
+ if (!IS_FREE(thrd-req[1 - thrd-lstenq])) {
req = thrd-req[1 - thrd-lstenq];
- else if (!IS_FREE(thrd-req[thrd-lstenq]))
+ thrd-req_running = 2 - thrd-lstenq;
+ } else if (!IS_FREE(thrd-req[thrd-lstenq])) {
req = thrd-req[thrd-lstenq];
- else
+ thrd-req_running = 1 + thrd-lstenq;
+ } else
req = NULL;
/* Return if no request */
@@ -1384,6 +1362,7 @@ static void pl330_dotask(unsigned long data)
thrd-req[1].r = NULL;
MARK_FREE(thrd-req[0]);
MARK_FREE(thrd-req[1]);
+ thrd-req_running = 0;
/* Clear the reset flag */
pl330-dmac_tbd.reset_chan = ~(1 i);
@@ -1461,7 +1440,7 @@ int pl330_update(const struct pl330_info *pi)
thrd = pl330-channels[id];
- active = _thrd_active(thrd);
+ active = thrd-req_running;
if (!active) /* Aborted */
continue;
@@ -1469,6 +1448,7 @@ int pl330_update(const struct pl330_info *pi)
rqdone = thrd-req[active];
MARK_FREE(rqdone);
+ thrd-req_running = 0;
/* Get going again ASAP */
_start(thrd);
@@ -1527,10 +1507,11 @@ int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)
thrd-req[1].r = NULL;
MARK_FREE(thrd-req[0]);
MARK_FREE(thrd-req[1]);
+ thrd-req_running = 0;
break;
case PL330_OP_ABORT:
- active = _thrd_active(thrd);
+ active = thrd-req_running;
/* Make sure the channel is stopped */
_stop(thrd);
@@ -1543,10 +1524,11 @@ int pl330_chan_ctrl(void *ch_id, enum
pl330_chan_op op)
thrd-req[active].r = NULL;
MARK_FREE(thrd-req[active]);
+ thrd-req_running = 0;
/* Start the next */
case PL330_OP_START:
- if (!_thrd_active(thrd) !_start(thrd))
+ if (!thrd-req_running !_start(thrd))
ret = -EIO;
break;
@@ -1587,7 +1569,7 @@ int pl330_chan_status(void *ch_id, struct
pl330_chanstatus *pstatus)
else
pstatus-faulting = false;
- active = _thrd_active(thrd);
+ active = thrd-req_running;
if (!active) {
/* Indicate that the thread is not running */
@@ -1662,6 +1644,7 @@ void *pl330_request_channel(const struct
pl330_info *pi)
MARK_FREE(thrd-req[0]);
thrd-req[1].r = NULL;
MARK_FREE(thrd-req[1]);
+ thrd-req_running = 0;
break;
}
}
@@ -1775,6 +1758,8 @@ static inline void _reset_thread(struct
pl330_thread *thrd)
+ pi-mcbufsz / 2;
thrd-req[1].r = NULL;
Re: [PATCH v2] ARM: pl330: Fix a race condition
On 11 December 2011 20:39, Javi Merino javi.mer...@arm.com wrote: What about properly tracking what we have sent to the DMA? Something like the following (warning *ugly* and untested code ahead, may eat your kitten): Yeah, this is like I said 'marker' method. Though we can clean it up a bit. 1) Pack req_running and lstenq together. Make lsteng return invalid value should there be no buff programmed, otherwise 0 or 1. This can lead to starvation. If lstenq is -1 when the DMA hasn't been programmed yet, in _trigger() you don't know which buffer is the oldest, so you may end up always starting the new buffer and forgetting about the other one. lstenq as it is right now prevents that. Sorry I don't understand. If lstenq is -1 that means there's no req programmed so trigger need not do anything. I didn't say we don't need to do anything else. Though it's just an idea I haven't implemented and it may not work out. Just please give it a try if you can. 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 v2] ARM: pl330: Fix a race condition
Hi Javi, On 9 December 2011 17:28, Javi Merino javi.mer...@arm.com wrote: Javi, could you please check if you too get the memcpy failure with dmatest ? Ok, I think I've just reproduced it in my end with the kernel's dmatest module. After the first transaction it looks like the dma test wasn't able to issue any more transactions. I'll have a look at it tomorrow and try to answer my own questions ;) The problem is that pl330_submit_req() always puts the request in buffer 0 if both buffers are free. There should be nothing wrong in that. It is but natural to prefer 0 if both 0 and 1 are available. If you submit a transaction, it finishes and there's nothing else to run, pl330_update() calls _start() but there is nothing to send. This is all right. Then, if another transaction is submitted, pl330_submit_req() will put it in buffer 0 again. This time, the PC of the DMA is in the last instruction of buffer 0 (the DMAEND of the *previous* transaction that finished long ago) so _thrd_active() thinks that this buffer is active, Many thanks for the in-depth analysis. Though before the PC check, the IS_FREE() should return true since pl330_update() does MARK_FREE() Could you please check if the client's callback function called successfully for the first submitted transfer ? Then we can decide upon one of the following 2 options (I *think* either should fix the issue) diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index 97912fa..f550d46 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -846,7 +846,7 @@ static inline bool _req_active(struct pl330_thread *thrd, if (IS_FREE(req)) return false; - return (pc = buf pc = buf + req-mc_len) ? true : false; + return (pc = buf pc buf + req-mc_len) ? true : false; } --OR-- diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index 97912fa..ad85a75 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -233,7 +233,7 @@ } while (0) /* If the _pl330_req is available to the client */ -#define IS_FREE(req) (*((u8 *)((req)-mc_cpu)) == CMD_DMAEND) +#define IS_FREE(req) ((req)-mc_len == 0) /* Use this _only_ to wait on transient states */ #define UNTIL(t, s)while (!(_state(t) (s))) cpu_relax(); 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 v2] ARM: pl330: Fix a race condition
On Fri, Dec 9, 2011 at 8:22 PM, Javi Merino javi.mer...@arm.com wrote: I think the best solution would be to revert ee3f615819404a9438b2dd01b7a39f276d2737f2 and go back to my original patch (in the beginning of this thread): http://article.gmane.org/gmane.linux.ports.arm.kernel/133110 What do you think? Well, we have to resort to that if we can't find something simpler. What do you think about ... diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c index f407a6b..3a51cdd 100644 --- a/arch/arm/common/pl330.c +++ b/arch/arm/common/pl330.c @@ -1546,7 +1546,7 @@ int pl330_chan_ctrl(void *ch_id, enum pl330_chan_op op) /* Start the next */ case PL330_OP_START: - if (!_thrd_active(thrd) !_start(thrd)) + if (_state(thrd) == PL330_STATE_STOPPED !_start(thrd)) ret = -EIO; break; Thanks -jassi [Sorry I don't have any pl330 platform handy] -- 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] ARM: pl330: Fix a race condition
On 29 November 2011 15:23, Javi Merino javi.mer...@arm.com wrote: On Samsung's Exynos4 platform, while testing audio playback with i2s interface, the above change causes the playback to freeze. The _thrd_active(thrd) call always returns '1' and hence _start(thrd) is not getting called. If _thrd_active(thrd) returns '1', that means there is an active transfer still running or, if it has finished, you haven't called pl330_update() to acknowledge that. pl330_update() calls _start() as soon as it can. drivers/dma/pl330.c registers the irq handler in pl330_probe(), so when the transaction finishes, pl330_update() should clear it and call _start(). If there is any outstanding transaction, it should start straight away. If there isn't, it would mark the channel as free, so _thrd_active() should return '0'. If _thrd_active() is still '1', then something has gone wrong in the way. Does this shed some light? Your patch makes the memcpy operation on dmatest.c and net DMA be frozen too as well as Samsung audio playback. Is the IRQ correctly registered in drivers/dma/pl330.c:pl330_probe()? Do you get interrupts when the transfer finish? Sure. IRQ works well. Ok, so can you check if pl330_update() is correctly marking the request as free? Do you know if there is another request in the queue when that happens? Thomas, you said in a previous email that _thrd_active() always returned '1'. Was that after a request in req[0] finished? - If so, can you check that MARK_FREE was actually called for that request in pl330_update()? - If it was after a request in req[1] finished and there was a request already waiting in req[0], can you debug why _start() didn't activate it. Javi, could you please check if you too get the memcpy failure with dmatest ? 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 v8 00/16] To use DMA generic APIs for Samsung DMA
On 14 September 2011 16:47, Vinod Koul vinod.k...@intel.com 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. -- 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 1/6] DMA: PL330: move filter function into driver
On Mon, Sep 12, 2011 at 11:59 PM, Thomas Abraham thomas.abra...@linaro.org wrote: The dma channel selection filter function is moved from plat-samsung into the pl330 driver. In additon to that, a check is added in the filter function to ensure that the channel on which the filter has been invoked is pl330 channel instance (and avoid any incorrect access of chan-private in a system with multiple types of DMA drivers). Suggested-by: Russell King rmk+ker...@arm.linux.org.uk Signed-off-by: Thomas Abraham thomas.abra...@linaro.org Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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 4/6] DMA: PL330: Add device tree support
On Mon, Sep 12, 2011 at 11:59 PM, Thomas Abraham thomas.abra...@linaro.org wrote: For PL330 dma controllers instantiated from device tree, the channel lookup is based on phandle of the dma controller and dma request id specified by the client node. During probe, the private data of each channel of the controller is set to point to the device node of the dma controller. The 'chan_id' of the each channel is used as the dma request id. Client driver requesting dma channels specify the phandle of the dma controller and the request id. The pl330 filter function converts the phandle to the device node pointer and matches that with channel's private data. If a match is found, the request id from the client node and the 'chan_id' of the channel is matched. A channel is found if both the values match. Cc: Jassi Brar jassisinghb...@gmail.com Cc: Boojin Kim boojin@samsung.com Signed-off-by: Thomas Abraham thomas.abra...@linaro.org Reviewed-by: Rob Herring rob.herr...@calxeda.com --- Acked-by: Jassi Brar jassisinghb...@gmail.com thanks -jassi -- 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 7 September 2011 12:53, Kukjin Kim kgene@samsung.com wrote:
Jassi Brar wrote:
On 6 September 2011 17:57, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Tue, Sep 06, 2011 at 05:52:19PM +0530, Jassi Brar wrote:
On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim boojin@samsung.com wrote:
Origianl code carries out the start operation after flush operation.
But start operation is not required for DMA_TERMINATE_ALL command.
So, This patch removes the unnecessary start operation and only carries
out
the flush oeration for handling DMA_TERMINATE_ALL command.
Not exactly. The 'start' is impotent when called from this path because
there
is nothing left queued after the call to 'flush'.
pl330_tasklet() is called because that is a common function that does the
house-keeping acc to what's presently in 'work' and 'done' lists.
Though as a side-effect, your patch does avoid doing callbacks on
submitted
xfers during DMA_TERMINATE_ALL - which may or may not be the right thing
to do.
It is defined that DMA_TERMINATE_ALL does not call the callbacks:
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.
Ok, thanks for the info.
Boojin Kim, please re-write the changelog to state only preventing
callbacks during DMA_TERMINATE_ALL as the reason.
As above, we don't need callback as well start operation in DMA_TERMIANTE_ALL
command and her patch just removed them for DMA_TERMINATE_ALL here. So
current git message seems to have proper behavior of patch.
Nopes.
No modification to current code is needed if only the following is to
be the reason.
{
Origianl code carries out the start operation after flush operation.
But start operation is not required for DMA_TERMINATE_ALL command.
So, This patch removes the unnecessary start operation and only carries out
the flush oeration for handling DMA_TERMINATE_ALL command.
}
The patch, however, has unintended good effect of preventing callbacks
from DMA_TERMINATE_ALL.
The only valid reason, and which is no way implied by the changelog.
--
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, Sep 2, 2011 at 6:14 AM, Boojin Kim boojin@samsung.com wrote: Origianl code carries out the start operation after flush operation. But start operation is not required for DMA_TERMINATE_ALL command. So, This patch removes the unnecessary start operation and only carries out the flush oeration for handling DMA_TERMINATE_ALL command. Not exactly. The 'start' is impotent when called from this path because there is nothing left queued after the call to 'flush'. pl330_tasklet() is called because that is a common function that does the house-keeping acc to what's presently in 'work' and 'done' lists. Though as a side-effect, your patch does avoid doing callbacks on submitted xfers during DMA_TERMINATE_ALL - which may or may not be the right thing to do. I remember seeing some dmac drivers doing the callbacks while some specifically avoiding them during DMA_TERMINATE_ALL. While I personally prefer the former, I guess it's for maintainers to decide. 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 6 September 2011 17:57, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Sep 06, 2011 at 05:52:19PM +0530, Jassi Brar wrote: On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim boojin@samsung.com wrote: Origianl code carries out the start operation after flush operation. But start operation is not required for DMA_TERMINATE_ALL command. So, This patch removes the unnecessary start operation and only carries out the flush oeration for handling DMA_TERMINATE_ALL command. Not exactly. The 'start' is impotent when called from this path because there is nothing left queued after the call to 'flush'. pl330_tasklet() is called because that is a common function that does the house-keeping acc to what's presently in 'work' and 'done' lists. Though as a side-effect, your patch does avoid doing callbacks on submitted xfers during DMA_TERMINATE_ALL - which may or may not be the right thing to do. It is defined that DMA_TERMINATE_ALL does not call the callbacks: 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. Ok, thanks for the info. Boojin Kim, please re-write the changelog to state only preventing callbacks during DMA_TERMINATE_ALL as the reason. -- 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, Aug 25, 2011 at 7:43 AM, Boojin Kim boojin@samsung.com wrote: This patch updates following 3 items. 1. Removes unneccessary code. 2. Add AMBA, PL330 configuration 3. Change the meaning of 'peri_id' variable from PL330 event number to specific dma id by user. Signed-off-by: Boojin Kim boojin@samsung.com Acked-by: Linus Walleij linus.wall...@linaro.org Acked-by: Vinod Koul vinod.k...@intel.com Cc: Dan Williams dan.j.willi...@intel.com Signed-off-by: Kukjin Kim kgene@samsung.com Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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 03/15] DMA: PL330: Modify device_control()
On Thu, Aug 25, 2011 at 7:43 AM, Boojin Kim boojin@samsung.com wrote: This patch modifies device_control() to support both DMA_TERMINATE_ALL and DMA_SLAVE_CONFIG command. First, modify the flush control for DMA_TERMINATE_ALL command. Second, add the slave configuration control for DMA_SLAVE_CONFIG command. I repeat what I said for v6. * Apart from the alleged purpose Support DMA_SLAVE_CONFIG command this patch also modifies the behavior of DMA_TERMINATE_ALL. If the change is intended, please split it into two patches and explain the reason. Otherwise, restore the behavior. * -- 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 04/15] DMA: PL330: Add DMA_CYCLIC capability
On Thu, Aug 25, 2011 at 7:43 AM, Boojin Kim boojin@samsung.com wrote:
static void pl330_issue_pending(struct dma_chan *chan)
{
- pl330_tasklet((unsigned long) to_pchan(chan));
+ struct dma_pl330_chan *pch = to_pchan(chan);
+
+ pl330_tasklet((unsigned long) pch);
}
This is a useless churn since v6.
Please fix this and add
Acked-by: Jassi Brar jassisinghb...@gmail.com
--
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 04/15] DMA: PL330: Add DMA_CYCLIC capability
On Tue, Aug 23, 2011 at 12:38 PM, Boojin Kim boojin@samsung.com wrote:
Jassi Brar [mailto:jassisinghb...@gmail.com]
Sent: Tuesday, August 23, 2011 2:42 PM
To: Boojin Kim
Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
s...@vger.kernel.org; Vinod Koul; Kukjin Kim; Dan Williams; Mark Brown;
Grant Likely; Russell King
Subject: Re: [PATCH v6 04/15] DMA: PL330: Add DMA_CYCLIC capability
On Mon, Aug 22, 2011 at 5:33 PM, Boojin Kim boojin@samsung.com
wrote:
+static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
+ struct dma_chan *chan, dma_addr_t dma_addr, size_t
len,
+ size_t period_len, enum dma_data_direction
direction)
+{
+ struct dma_pl330_desc *desc;
+ struct dma_pl330_chan *pch = to_pchan(chan);
+ dma_addr_t dst;
+ dma_addr_t src;
+
+ desc = pl330_get_desc(pch);
+ if (!desc) {
+ dev_err(pch-dmac-pif.dev, %s:%d Unable to fetch
desc\n,
+ __func__, __LINE__);
+ return NULL;
+ }
+
+ switch (direction) {
+ case DMA_TO_DEVICE:
+ desc-rqcfg.src_inc = 1;
+ desc-rqcfg.dst_inc = 0;
+ src = dma_addr;
+ dst = pch-fifo_addr;
+ break;
+ case DMA_FROM_DEVICE:
+ desc-rqcfg.src_inc = 0;
+ desc-rqcfg.dst_inc = 1;
+ src = pch-fifo_addr;
+ dst = dma_addr;
+ break;
+ default:
+ dev_err(pch-dmac-pif.dev, %s:%d Invalid dma
direction\n,
+ __func__, __LINE__);
+ return NULL;
+ }
+
+ desc-rqcfg.brst_size = pch-burst_sz;
+ desc-rqcfg.brst_len = 1;
+
+ if (!pch-cyclic)
+ pch-cyclic = CYCLIC_PREP;
The need for check here seems suspicious.
Is it really needed? If not, please remove it.
It's required because Cyclic operation is passed from CYCLIC_PREP to
CYCLIC_TRIGGER.
I don't see why you need to differentiate between PREP and TRIGGER in
cyclic mode. I think, you should simply have 'bool cyclic' instead of
enum.
On cyclic mode, Pl330_tasklet() operation is changed according to the value of
'cyclic'.
In case of CYCLIC_PREP, Pl330_tasklet()operation is same with normal
operation.
By 'normal' you mean non-cyclic, right? That doesn't seem correct to do.
Once the desc has been marked cyclic by device_prep_dma_cyclic(), you
shouldn't treat it like a non-cyclic anymore.
The sequence is following.
device_prep_dma_cyclic() - set CYCLIC_PREP - device_issue_pending() -
Pl330_tasklet() with CYCLIC_PREP - set CYCLIC_TRIGGER - PL330 IRQ -
Pl330_tasklet() with CYCLIC_TRIGGER.
device_issue_pending() -Pl330_tasklet() with CYCLIC_PREP is no different
from device_issue_pending() -Pl330_tasklet() with CYCLIC_TRIGGER
because the list of 'done' xfers would be null yet.
--
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] DMA: PL330: Infer transfer direction from transfer request instead of platform data
On Tue, Aug 23, 2011 at 3:20 AM, Thomas Abraham thomas.abra...@linaro.org wrote: The transfer direction for a channel can be inferred from the transfer request and the need for specifying transfer direction in platfrom data can be eliminated. So the structure definition 'struct dma_pl330_peri' is no longer required. With the 'struct dma_pl330_peri' removed, the dma controller transfer capabilities cannot be inferred any longer. Hence, the dma controller capabilities is specified using platforme data. Cc: Jassi Brar jassisinghb...@gmail.com Cc: Boojin Kim boojin@samsung.com Signed-off-by: Thomas Abraham thomas.abra...@linaro.org Looks ok. Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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 04/15] DMA: PL330: Add DMA_CYCLIC capability
On Fri, Aug 19, 2011 at 2:24 PM, Boojin Kim boojin@samsung.com wrote:
@@ -324,6 +362,9 @@ static void pl330_free_chan_resources(struct dma_chan
*chan)
pl330_release_channel(pch-pl330_chid);
pch-pl330_chid = NULL;
+ if (pch-cyclic)
+ list_splice_tail_init(pch-work_list, pch-dmac-desc_pool);
'cyclic' member is 'enum cyclic_mode', please observe the rule and compare
it only against the enum values.
+static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
+ struct dma_chan *chan, dma_addr_t dma_addr, size_t len,
+ size_t period_len, enum dma_data_direction direction)
+{
+ struct dma_pl330_desc *desc;
+ struct dma_pl330_chan *pch = to_pchan(chan);
+ dma_addr_t dst;
+ dma_addr_t src;
+
+ desc = pl330_get_desc(pch);
+ if (!desc) {
+ dev_err(pch-dmac-pif.dev, %s:%d Unable to fetch desc\n,
+ __func__, __LINE__);
+ return NULL;
+ }
+
+ switch (direction) {
+ case DMA_TO_DEVICE:
+ desc-rqcfg.src_inc = 1;
+ desc-rqcfg.dst_inc = 0;
+ src = dma_addr;
+ dst = pch-fifo_addr;
+ break;
+ case DMA_FROM_DEVICE:
+ desc-rqcfg.src_inc = 0;
+ desc-rqcfg.dst_inc = 1;
+ src = pch-fifo_addr;
+ dst = dma_addr;
+ break;
+ default:
+ dev_err(pch-dmac-pif.dev, %s:%d Invalid dma direction\n,
+ __func__, __LINE__);
+ return NULL;
+ }
+
+ desc-rqcfg.brst_size = pch-burst_sz;
+ desc-rqcfg.brst_len = 1;
+
+ if (!pch-cyclic)
+ pch-cyclic = CYCLIC_PREP;
The need for check here seems suspicious.
Is it really needed? If not, please remove it.
Re: [PATCH v6 02/15] DMA: PL330: Update PL330 DMA API driver
On Fri, Aug 19, 2011 at 2:24 PM, Boojin Kim boojin@samsung.com wrote: This patch updates following 3 items. 1. Removes unneccessary code. 2. Add AMBA, PL330 configuration 3. Change the meaning of 'peri_id' variable from PL330 event number to specific dma id by user. Signed-off-by: Boojin Kim boojin@samsung.com Acked-by: Linus Walleij linus.wall...@linaro.org Acked-by: Vinod Koul vinod.k...@intel.com Cc: Dan Williams dan.j.willi...@intel.com Signed-off-by: Kukjin Kim kgene@samsung.com Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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 03/15] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Fri, Aug 19, 2011 at 2:24 PM, Boojin Kim boojin@samsung.com wrote:
Signed-off-by: Boojin Kim boojin@samsung.com
Acked-by: Linus Walleij linus.wall...@linaro.org
Acked-by: Vinod Koul vinod.k...@intel.com
Cc: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Kukjin Kim kgene@samsung.com
---
drivers/dma/pl330.c | 56 ++
1 files changed, 42 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index d5829c7..59943ec 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -73,6 +73,7 @@ struct dma_pl330_chan {
/* For D-to-M and M-to-D channels */
int burst_sz; /* the peripheral fifo width */
+ int burst_len; /* the number of burst */
dma_addr_t fifo_addr;
};
@@ -261,25 +262,52 @@ 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);
- /* 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;
+
+ if (slave_config-direction == DMA_TO_DEVICE) {
+ if (slave_config-dst_addr)
+ pch-fifo_addr = slave_config-dst_addr;
+ if (slave_config-dst_addr_width)
+ pch-burst_sz =
__ffs(slave_config-dst_addr_width);
+ if (slave_config-dst_maxburst)
+ pch-burst_len = slave_config-dst_maxburst;
+ } else if (slave_config-direction == DMA_FROM_DEVICE) {
+ if (slave_config-src_addr)
+ pch-fifo_addr = slave_config-src_addr;
+ if (slave_config-src_addr_width)
+ pch-burst_sz =
__ffs(slave_config-src_addr_width);
+ if (slave_config-src_maxburst)
+ pch-burst_len = slave_config-src_maxburst;
+ }
+ break;
+ default:
+ dev_err(pch-dmac-pif.dev, Not supported command.\n);
+ return -ENXIO;
+ }
Apart from the alleged purpose Support DMA_SLAVE_CONFIG command
this patch also modifies the behavior of DMA_TERMINATE_ALL.
If the change is intended, please split it into two patches and
explain the reason.
Otherwise, restore the behavior.
--
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 14/15] ASoC: Samsung: Update DMA interface
On Fri, Aug 19, 2011 at 2:24 PM, Boojin Kim boojin@samsung.com wrote:
This patch adds to support the DMA PL330 driver that uses
DMA generic API. Samsung sound driver uses DMA generic API
if architecture supports it. Otherwise, use samsung specific
S3C-PL330 API driver to transfer PCM data.
Signed-off-by: Boojin Kim boojin@samsung.com
Acked-by: Linus Walleij linus.wall...@linaro.org
Acked-by: Vinod Koul vinod.k...@intel.com
Cc: Jassi Brar jassisinghb...@gmail.com
Cc: Liam Girdwood l...@ti.com
Acked-by: Mark Brown broo...@opensource.wolfsonmicro.com
[kgene@samsung.com: removed useless variable]
Signed-off-by: Kukjin Kim kgene@samsung.com
---
arch/arm/mach-s3c2410/include/mach/dma.h | 10 +-
arch/arm/mach-s3c64xx/include/mach/dma.h | 2 +-
arch/arm/plat-samsung/include/plat/dma-pl330.h | 2 +-
sound/soc/samsung/ac97.c | 10 ++-
sound/soc/samsung/dma.c | 146
++--
sound/soc/samsung/dma.h | 4 +-
6 files changed, 78 insertions(+), 96 deletions(-)
diff --git a/arch/arm/mach-s3c2410/include/mach/dma.h
b/arch/arm/mach-s3c2410/include/mach/dma.h
index e61a91f..4e485ba 100644
--- a/arch/arm/mach-s3c2410/include/mach/dma.h
+++ b/arch/arm/mach-s3c2410/include/mach/dma.h
@@ -50,6 +50,11 @@ enum dma_ch {
DMACH_MAX, /* the end entry */
};
+static inline bool samsung_dma_has_circular(void)
+{
+ return false;
+}
+
static inline bool samsung_dma_is_dmadev(void)
{
return false;
@@ -202,9 +207,4 @@ struct s3c2410_dma_chan {
typedef unsigned long dma_device_t;
-static inline bool s3c_dma_has_circular(void)
-{
- return false;
-}
-
#endif /* __ASM_ARCH_DMA_H */
diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h
b/arch/arm/mach-s3c64xx/include/mach/dma.h
index 49c3a53..74fdf25 100644
--- a/arch/arm/mach-s3c64xx/include/mach/dma.h
+++ b/arch/arm/mach-s3c64xx/include/mach/dma.h
@@ -58,7 +58,7 @@ enum dma_ch {
DMACH_MAX /* the end */
};
-static __inline__ bool s3c_dma_has_circular(void)
+static inline bool samsung_dma_has_circular(void)
{
return true;
}
diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h
b/arch/arm/plat-samsung/include/plat/dma-pl330.h
index 9a1dadb..2e55e59 100644
--- a/arch/arm/plat-samsung/include/plat/dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -89,7 +89,7 @@ struct s3c2410_dma_client {
char *name;
};
-static inline bool s3c_dma_has_circular(void)
+static inline bool samsung_dma_has_circular(void)
{
return true;
}
diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
index f97110e..b4f9b00 100644
--- a/sound/soc/samsung/ac97.c
+++ b/sound/soc/samsung/ac97.c
@@ -271,7 +271,10 @@ static int s3c_ac97_trigger(struct snd_pcm_substream
*substream, int cmd,
writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
- s3c2410_dma_ctrl(dma_data-channel, S3C2410_DMAOP_STARTED);
+ if (!dma_data-ops)
+ dma_data-ops = samsung_dma_get_ops();
+
+ dma_data-ops-started(dma_data-channel);
return 0;
}
@@ -317,7 +320,10 @@ static int s3c_ac97_mic_trigger(struct snd_pcm_substream
*substream,
writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
- s3c2410_dma_ctrl(dma_data-channel, S3C2410_DMAOP_STARTED);
+ if (!dma_data-ops)
+ dma_data-ops = samsung_dma_get_ops();
+
+ dma_data-ops-started(dma_data-channel);
return 0;
}
diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
index 9465588..851346f 100644
--- a/sound/soc/samsung/dma.c
+++ b/sound/soc/samsung/dma.c
@@ -54,7 +54,6 @@ struct runtime_data {
spinlock_t lock;
int state;
unsigned int dma_loaded;
- unsigned int dma_limit;
unsigned int dma_period;
dma_addr_t dma_start;
dma_addr_t dma_pos;
@@ -62,77 +61,79 @@ struct runtime_data {
struct s3c_dma_params *params;
};
+static void audio_buffdone(void *data);
+
/* dma_enqueue
*
* place a dma buffer onto the queue for the dma system
* to handle.
-*/
+ */
static void dma_enqueue(struct snd_pcm_substream *substream)
{
struct runtime_data *prtd = substream-runtime-private_data;
dma_addr_t pos = prtd-dma_pos;
unsigned int limit;
- int ret;
+ struct samsung_dma_prep_info dma_info;
pr_debug(Entered %s\n, __func__);
- if (s3c_dma_has_circular())
- limit = (prtd-dma_end - prtd-dma_start) / prtd-dma_period;
- else
- limit = prtd-dma_limit;
+ limit = (prtd-dma_end - prtd-dma_start) / prtd-dma_period;
pr_debug(%s: loaded %d, limit %d\n,
__func__, prtd-dma_loaded, limit);
- while (prtd-dma_loaded limit) {
- unsigned long len
Re: [PATCH] DMA: PL330: Merge PL330 drivers
On Mon, Aug 22, 2011 at 10:22 AM, Boojin Kim boojin@samsung.com wrote: This patch merges 'arch/arm/common/pl330.c' with 'driver/dma/pl330.c' NOTE: this patch is made on following patches. - [PATCH v6] To use DMA generic APIs for Samsung DMA - [PATCH v4] amba: consolidate PrimeCell magic This doesn't make sense in changelog. Such notes usually go below the '---' line below last SOB Signed-off-by: Boojin Kim boojin@samsung.com --- arch/arm/common/Kconfig | 3 - arch/arm/common/Makefile | 1 - arch/arm/common/pl330.c | 1931 --- arch/arm/include/asm/hardware/pl330.h | 217 drivers/dma/Kconfig | 1 - drivers/dma/pl330.c | 2036 + include/linux/amba/pl330.h | 11 +- 7 files changed, 2046 insertions(+), 2154 deletions(-) delete mode 100644 arch/arm/common/pl330.c delete mode 100644 arch/arm/include/asm/hardware/pl330.h diff --git a/arch/arm/include/asm/hardware/pl330.h b/arch/arm/include/asm/hardware/pl330.h deleted file mode 100644 index 575fa81..000 --- a/arch/arm/include/asm/hardware/pl330.h +++ /dev/null @@ -1,217 +0,0 @@ -/* linux/include/asm/hardware/pl330.h - * - * Copyright (C) 2010 Samsung Electronics Co. Ltd. - * Jaswinder Singh jassi.b...@samsung.com While at it, please also modify the instances of author id to 'Jassi Brar jassisinghb...@gmail.com I think we can accommodate the trivial change. -- 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 04/15] DMA: PL330: Add DMA_CYCLIC capability
On Mon, Aug 22, 2011 at 5:33 PM, Boojin Kim boojin@samsung.com wrote:
+static struct dma_async_tx_descriptor *pl330_prep_dma_cyclic(
+ struct dma_chan *chan, dma_addr_t dma_addr, size_t len,
+ size_t period_len, enum dma_data_direction direction)
+{
+ struct dma_pl330_desc *desc;
+ struct dma_pl330_chan *pch = to_pchan(chan);
+ dma_addr_t dst;
+ dma_addr_t src;
+
+ desc = pl330_get_desc(pch);
+ if (!desc) {
+ dev_err(pch-dmac-pif.dev, %s:%d Unable to fetch
desc\n,
+ __func__, __LINE__);
+ return NULL;
+ }
+
+ switch (direction) {
+ case DMA_TO_DEVICE:
+ desc-rqcfg.src_inc = 1;
+ desc-rqcfg.dst_inc = 0;
+ src = dma_addr;
+ dst = pch-fifo_addr;
+ break;
+ case DMA_FROM_DEVICE:
+ desc-rqcfg.src_inc = 0;
+ desc-rqcfg.dst_inc = 1;
+ src = pch-fifo_addr;
+ dst = dma_addr;
+ break;
+ default:
+ dev_err(pch-dmac-pif.dev, %s:%d Invalid dma
direction\n,
+ __func__, __LINE__);
+ return NULL;
+ }
+
+ desc-rqcfg.brst_size = pch-burst_sz;
+ desc-rqcfg.brst_len = 1;
+
+ if (!pch-cyclic)
+ pch-cyclic = CYCLIC_PREP;
The need for check here seems suspicious.
Is it really needed? If not, please remove it.
It's required because Cyclic operation is passed from CYCLIC_PREP to
CYCLIC_TRIGGER.
I don't see why you need to differentiate between PREP and TRIGGER in
cyclic mode. I think, you should simply have 'bool cyclic' instead of enum.
--
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 2/2] spi: s3c64xx: Use clkdev for bus clock lookup
On Wed, Aug 10, 2011 at 5:33 PM, padma venkat padma@gmail.com wrote: Hi Jassi, On Tue, Aug 9, 2011 at 6:13 PM, Jassi Brar jassisinghb...@gmail.com wrote: On Tue, Aug 9, 2011 at 7:28 PM, Padmavathi Venna padm...@samsung.com wrote: SPI driver is modified to lookup the bus clock using the alias name instead of getting clock name and clock number from platform data. Cool. Driver is modified to get the best source clock among the available source clocks for the required frequency. I am not sure if this driver should be deciding which clock is 'best' for it. Because ... 1) Usually it's the board designer who decides which clock to run at what speed based upon target device. So ideally, based upon use-case the driver should simply get the 'best' clock from board via platform in a format that is compliant to the 'generic clock api'. As per your comment I modified the code for board designer to supply the required list of source clocks. If this list is NULL then it uses the all available clock sources. I will resubmit this patch. No dear. Not a list of clocks (that is property of the platform - not a board). But get just one 'generic clock api' compliant clock from the board and use it. That will keep the driver simple for sure and IIUIC, in future anyways DT would specify source clock for each peripheral. Grant ? -- 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 2/2] spi: s3c64xx: Use clkdev for bus clock lookup
On Tue, Aug 9, 2011 at 7:28 PM, Padmavathi Venna padm...@samsung.com wrote: SPI driver is modified to lookup the bus clock using the alias name instead of getting clock name and clock number from platform data. Cool. Driver is modified to get the best source clock among the available source clocks for the required frequency. I am not sure if this driver should be deciding which clock is 'best' for it. Because ... 1) Usually it's the board designer who decides which clock to run at what speed based upon target device. So ideally, based upon use-case the driver should simply get the 'best' clock from board via platform in a format that is compliant to the 'generic clock api'. 2) We are not changing source clock rates(and we should not). So the 'best' clock found, might still be way off in accuracy. And when we can't anyway guarantee accuracy, why not leave the decision to the board designer who might, say, select the source clock good enough to be useful to more than one controller yet not absolutely accurate for any ? 3) It keeps enabled 3 unused clocks all the time. diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c index 8945e20..d7c979d 100644 --- a/drivers/spi/spi_s3c64xx.c +++ b/drivers/spi/spi_s3c64xx.c @@ -132,6 +132,9 @@ #define RXBUSY (12) #define TXBUSY (13) +#define MAX_SPI_BUS_CLK (4) +#define MAX_PSR (256) Btw, #define MAX_SPI_BUS_CLK 4 #define MAX_PSR 256 is safe ... even safer than with two on! -- 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 12/15] spi/s3c64xx: Add support DMA engine API
On Wed, Jul 27, 2011 at 11:01 AM, Boojin Kim boojin@samsung.com 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 boojin@samsung.com
Acked-by: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Kukjin Kim kgene@samsung.com
---
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 {
unsigned state;
unsigned cur_mode, cur_bpw;
unsigned cur_speed;
+ unsigned rx_ch;
+ unsigned tx_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;
void* doesn't need typecasting (same for all such occurances)
IIRC someone already pointed it out?
+ 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);
+}
+
static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
struct spi_device *spi,
struct spi_transfer *xfer, int dma_mode)
@@ -234,6 +269,7 @@ static void enable_datapath(struct
s3c64xx_spi_driver_data *sdd,
struct s3c64xx_spi_info *sci = sdd-cntrlr_info;
void __iomem *regs = sdd-regs;
u32 modecfg, chcfg;
+ struct samsung_dma_prep_info info;
modecfg = readl(regs + S3C64XX_SPI_MODE_CFG);
modecfg = ~(S3C64XX_SPI_MODE_TXDMA_ON | S3C64XX_SPI_MODE_RXDMA_ON);
@@ -259,10 +295,14 @@ static void enable_datapath(struct
s3c64xx_spi_driver_data *sdd,
chcfg |= S3C64XX_SPI_CH_TXCH_ON;
if (dma_mode) {
modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
- s3c2410_dma_config(sdd-tx_dmach, sdd-cur_bpw / 8);
- s3c2410_dma_enqueue(sdd-tx_dmach, (void *)sdd,
- xfer-tx_dma, xfer-len);
- s3c2410_dma_ctrl(sdd-tx_dmach, S3C2410_DMAOP_START);
+ info.cap = DMA_SLAVE;
+ info.direction = DMA_TO_DEVICE;
+ info.buf = xfer-tx_dma;
+ info.len = xfer-len;
+ info.fp = s3c64xx_spi_dma_txcb;
+ info.fp_param = sdd;
+ sdd-ops-prepare(sdd-tx_ch, info);
+ sdd-ops-trigger(sdd-tx_ch);
} else {
switch (sdd-cur_bpw) {
case 32:
@@ -294,10 +334,14 @@ static void enable_datapath(struct
s3c64xx_spi_driver_data *sdd,
writel(((xfer-len * 8 / sdd-cur_bpw) 0x)
| S3C64XX_SPI_PACKET_CNT_EN,
regs + S3C64XX_SPI_PACKET_CNT);
- s3c2410_dma_config(sdd-rx_dmach, sdd-cur_bpw / 8);
- s3c2410_dma_enqueue(sdd-rx_dmach, (void *)sdd,
- 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
Re: [PATCH 12/15] spi/s3c64xx: Add support DMA engine API
On Mon, Aug 8, 2011 at 11:25 PM, Heiko Stübner he...@sntech.de wrote: Am Montag 08 August 2011, 19:47:58 schrieb Jassi Brar: Btw, this spi driver is for S3C64xx(with pl080) and S5P(with pl330) series, both of which have DMAENGINE drivers. May be it could be directly switched to using those, rather than via Samsung's wrapper driver. Or am I overlooking something ? yep, at least the S3C2416/2450 HSSPI which should use this driver. Seems you keep some patches private ;) the spi driver isn't enabled for 2416 in mainline. -- 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 13/15] spi/s3c64xx: Merge dma control code
On Wed, Jul 27, 2011 at 11:01 AM, Boojin Kim boojin@samsung.com wrote: This patch modifies to merge the dma control code. Original s3c64xx spi driver has each dma control code for rx and tx channel. This patch merges these dma control codes into one. With this patch, a dma setup function and callback function handle for both rx and tx channel. This is nothing really to do with underlying DMAENGINE API. And, apparently it actually bloats the driver ? 76 insertions(+), 64 deletions(-) Yet, I am not very against the idea. If you must do it, maybe It would have been better to move it early in the patchset, which would have resulted in a smaller and cleaner spi/s3c64xx: Add support DMA engine API -- 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 14/15] ASoC: Samsung: Update DMA interface
On Wed, Jul 27, 2011 at 11:01 AM, Boojin Kim boojin@samsung.com wrote:
static void dma_enqueue(struct snd_pcm_substream *substream)
{
struct runtime_data *prtd = substream-runtime-private_data;
dma_addr_t pos = prtd-dma_pos;
unsigned int limit;
- int ret;
+ struct samsung_dma_prep_info dma_info;
pr_debug(Entered %s\n, __func__);
- if (s3c_dma_has_circular())
- limit = (prtd-dma_end - prtd-dma_start) / prtd-dma_period;
- else
- limit = prtd-dma_limit;
+ limit = (prtd-dma_end - prtd-dma_start) / prtd-dma_period;
'dma_limit' is rendered useless, so you might want to remove it from
'struct runtime_data'
as well.
pr_debug(%s: loaded %d, limit %d\n,
__func__, prtd-dma_loaded, limit);
- while (prtd-dma_loaded limit) {
- unsigned long len = prtd-dma_period;
+ dma_info.cap = (samsung_dma_has_circular() ? DMA_CYCLIC : DMA_SLAVE);
+ dma_info.direction =
+ (substream-stream == SNDRV_PCM_STREAM_PLAYBACK
+ ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
+ dma_info.fp = audio_buffdone;
+ dma_info.fp_param = substream;
+ dma_info.period = prtd-dma_period;
+ dma_info.len = prtd-dma_period*limit;
+ while (prtd-dma_loaded limit) {
pr_debug(dma_loaded: %d\n, prtd-dma_loaded);
- if ((pos + len) prtd-dma_end) {
- len = prtd-dma_end - pos;
- pr_debug(%s: corrected dma len %ld\n, __func__,
len);
+ if ((pos + dma_info.period) prtd-dma_end) {
+ dma_info.period = prtd-dma_end - pos;
+ pr_debug(%s: corrected dma len %ld\n,
+ __func__, dma_info.period);
}
- ret = s3c2410_dma_enqueue(prtd-params-channel,
- substream, pos, len);
+ dma_info.buf = pos;
+ prtd-params-ops-prepare(prtd-params-ch, dma_info);
- if (ret == 0) {
- prtd-dma_loaded++;
- pos += prtd-dma_period;
- if (pos = prtd-dma_end)
- pos = prtd-dma_start;
- } else
- break;
+ prtd-dma_loaded++;
+ pos += prtd-dma_period;
+ if (pos = prtd-dma_end)
+ pos = prtd-dma_start;
}
prtd-dma_pos = pos;
}
-static void audio_buffdone(struct s3c2410_dma_chan *channel,
- void *dev_id, int size,
- enum s3c2410_dma_buffresult result)
+static void audio_buffdone(void *data)
{
- struct snd_pcm_substream *substream = dev_id;
- struct runtime_data *prtd;
+ struct snd_pcm_substream *substream = data;
+ struct runtime_data *prtd = substream-runtime-private_data;
pr_debug(Entered %s\n, __func__);
- if (result == S3C2410_RES_ABORT || result == S3C2410_RES_ERR)
- return;
-
- prtd = substream-runtime-private_data;
+ if (prtd-state ST_RUNNING) {
+ prtd-dma_pos += prtd-dma_period;
+ if (prtd-dma_pos = prtd-dma_end)
+ prtd-dma_pos = prtd-dma_start;
- if (substream)
- snd_pcm_period_elapsed(substream);
+ if (substream)
+ snd_pcm_period_elapsed(substream);
- spin_lock(prtd-lock);
- if (prtd-state ST_RUNNING !s3c_dma_has_circular()) {
- prtd-dma_loaded--;
- dma_enqueue(substream);
+ spin_lock(prtd-lock);
+ if (!samsung_dma_has_circular()) {
+ prtd-dma_loaded--;
+ dma_enqueue(substream);
+ }
+ spin_unlock(prtd-lock);
}
-
- spin_unlock(prtd-lock);
}
Since we set integer-constraint on number of periods, you could also
discard bothering fractional boundaries. That would make things a lot
simpler.
@@ -265,14 +250,14 @@ static int dma_trigger(struct snd_pcm_substream
*substream, int cmd)
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
prtd-state |= ST_RUNNING;
- s3c2410_dma_ctrl(prtd-params-channel, S3C2410_DMAOP_START);
+ prtd-params-ops-trigger(prtd-params-ch);
break;
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
prtd-state = ~ST_RUNNING;
- s3c2410_dma_ctrl(prtd-params-channel, S3C2410_DMAOP_STOP);
+ prtd-params-ops-stop(prtd-params-ch);
break;
I wish you agreed and used
prtd-params-ops-cmd(ch, enum
Re: [RFC PATCH] DMA: PL330: Update PL330 DMAC to support runtime PM
On Wed, Jul 27, 2011 at 11:44 AM, Chanwoo Choi cw00.c...@samsung.com wrote:
This patch update runtime PM for PL330 DMAC to reduce power consumption.
Signed-off-by: Chanwoo Choi cw00.c...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
The following patch support runtime PM for PL330 DMAC, but the clock
of PL330 is always on. If the clock of PL330 is always on, additional power
(10mA) is consumed.
Yes, we need to do this, but please do it in drivers/dma/pl330.c so that
every platform benefits.
diff --git a/include/linux/amba/pl330.h b/include/linux/amba/pl330.h
index 17b0ada..d9a63fd 100644
--- a/include/linux/amba/pl330.h
+++ b/include/linux/amba/pl330.h
@@ -12,6 +12,9 @@
#ifndef __AMBA_PL330_H_
#define __AMBA_PL330_H_
+#include linux/dmaengine.h
+#include linux/interrupt.h
+
#include asm/hardware/pl330.h
struct dma_pl330_peri {
@@ -42,4 +45,55 @@ struct dma_pl330_platdata {
unsigned mcbuf_sz;
};
+struct dma_pl330_chan {
+ /* Schedule desc completion */
+ struct tasklet_struct task;
+
+ /* DMA-Engine Channel */
+ struct dma_chan chan;
+
+ /* Last completed cookie */
+ dma_cookie_t completed;
+
+ /* List of to be xfered descriptors */
+ struct list_head work_list;
+
+ /* Pointer to the DMAC that manages this channel,
+ * NULL if the channel is available to be acquired.
+ * As the parent, this DMAC also provides descriptors
+ * to the channel.
+ */
+ struct dma_pl330_dmac *dmac;
+
+ /* To protect channel manipulation */
+ spinlock_t lock;
+
+ /* Token of a hardware channel thread of PL330 DMAC
+ * NULL if the channel is available to be acquired.
+ */
+ void *pl330_chid;
+
+ /* taks for cyclic capability */
+ struct tasklet_struct *cyclic_task;
+
+ bool cyclic;
+};
+
+struct dma_pl330_dmac {
+ struct pl330_info pif;
+
+ /* DMA-Engine Device */
+ struct dma_device ddma;
+
+ /* Pool of descriptors available for the DMAC's channels */
+ struct list_head desc_pool;
+ /* To protect desc_pool manipulation */
+ spinlock_t pool_lock;
+
+ /* Peripheral channels connected to this DMAC */
+ struct dma_pl330_chan peripherals[0]; /* keep at end */
+
+ struct clk *clk;
+};
+
#endif /* __AMBA_PL330_H_ */
struct dma_pl330_dmac and struct dma_pl330_chan are internal to the
pl330 dmac driver. Nobody from outside should ever need them.
Thanks,
Jassi
--
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 01/15] DMA: PL330: Add support runtime PM for PL330 DMAC
On Wed, Jul 27, 2011 at 11:01 AM, Boojin Kim boojin@samsung.com wrote: Signed-off-by: Boojin Kim boojin@samsung.com Cc: Vinod Koul vinod.k...@intel.com Cc: Dan Williams dan.j.willi...@intel.com Signed-off-by: Kukjin Kim kgene@samsung.com Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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 06/14] ARM: SAMSUNG: Add common DMA operations
On Wed, Jul 27, 2011 at 10:47 AM, Boojin Kim boojin@samsung.com wrote:
Jassi Brar wrote:
Sent: Wednesday, July 27, 2011 10:34 AM
To: Boojin Kim
Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
Likely; Mark Brown
Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim boojin@samsung.com
wrote:
Jassi Brar Wrote:
Sent: Monday, July 25, 2011 8:52 PM
To: Boojin Kim
Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
Likely; Mark Brown
Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA
operations
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim
boojin@samsung.com
wrote:
+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+ struct dma_pl330_peri *peri = (struct dma_pl330_peri
*)chan-
private;
+ unsigned dma_ch = (unsigned)param;
+
+ if (peri-peri_id != dma_ch)
+ return false;
+
+ return true;
+}
This is what I meant... if we keep chan_id for paltform assigned
IDs,
these filter functions could simply become
static bool pl330_filter(struct dma_chan *chan, void *param)
{
return chan-chan_id == param
}
And ideally in the long run, we could just drop the filter callback
and add expected channel ID to the request_channel call.
The chan_id is set by dmaengine. So, We don't use it to hold the
user specific
Id.
Not for long.
See https://lkml.org/lkml/2011/7/26/245
+
+static inline int s3c_dma_trigger(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
+}
+
+static inline int s3c_dma_started(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
+}
+
+static inline int s3c_dma_flush(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
+}
+
+static inline int s3c_dma_stop(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
+}
+
+static struct samsung_dma_ops s3c_dma_ops = {
+ .request = s3c_dma_request,
+ .release = s3c_dma_release,
+ .prepare = s3c_dma_prepare,
+ .trigger = s3c_dma_trigger,
+ .started = s3c_dma_started,
+ .flush = s3c_dma_flush,
+ .stop = s3c_dma_stop,
These last 4 should be gnereallized into one callback with OP
argument.
I don't have any idea about it. Can you explain it in more detail?
static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or
similar*/ op)
{
return s3c2410_dma_ctrl(ch, op);
}
static struct samsung_dma_ops s3c_dma_ops = {
.request = s3c_dma_request,
.release = s3c_dma_release,
.prepare = s3c_dma_prepare,
.control = s3c_dma_control,
'Struct samsung_dma_ops' is used for both 'S3C-DMA-OPS' for 's3c dma' and
'DMA-OPS' for 'dmaengine'.
If I modify Struct samsung_dma_ops as your guide, It gives un-expectable
effect to DMA-OPS.
Actually, We don't want the client with 'DMA-OPS' uses 'enum s3c2410_chan_op'
operation anymore.
enum s3c2410_chan_op /* or similar */ note 'or similar'
Defining a callback for each value of the argument doesn't make sense.
--
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, Jul 26, 2011 at 3:05 PM, Vinod Koul vk...@infradead.org wrote: On Tue, 2011-07-26 at 08:57 +0100, Russell King - ARM Linux wrote: Here's an updated patch. 8-- From: Russell King rmk+ker...@arm.linux.org.uk DMAEngine: Improve slave/cyclic documentation Improve the documentation for the slave and cyclic DMA engine support reformatting it for easier reading, adding further APIs, splitting it into five steps, and including references to the documentation in dmaengine.h. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk --- Documentation/dmaengine.txt | 211 ++- 1 files changed, 146 insertions(+), 65 deletions(-) diff --git a/Documentation/dmaengine.txt b/Documentation/dmaengine.txt index 5a0cb1e..8c2e888 100644 --- a/Documentation/dmaengine.txt +++ b/Documentation/dmaengine.txt @@ -10,87 +10,168 @@ 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 Thanks Russell, Applied with change to 4 above. Moved and wait for callback notification to 5. Dear Vinod, Since it came from the RMK, most probably it'll be the best. But applying patches upon personal timeout seems very dangerous. 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. Thanks -Jassi -- 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 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 11:44 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: 1. Arrange for individual DMA engine drivers to provide a filter function - eg, pl08x_filter_id() for pl08x channels. 2. Have the filter function check chan-device-dev-driver == its own struct driver as the very first thing, and reject on failure. 3. Have its own matching algorithm using whatever data is appropriate for the DMA engine driver. IMHO it should be possible to have client drivers not use platform specific details while choosing a channel. Otherwise, they are not really 'generic' 'platform-independent' client drivers. I hope we agree, clients (esp 'generic' ones of the future) should not ask for channels from a particular dmac. They should simply tell DMA API, what capabilities they expect from the allocated channel - of whatever index on whichever dmac. And the platform should have specified capabilities of a channel while introducing it to the DMA API. Yes, imho, DMA API should only work with a pool of channels and not dmacs - it shouldn't matter from which dmac a channel comes. You're quite clearly confused. DMA API has not very much to do with the subject we're discussing. The DMA API is the API for mapping and unmapping buffers for DMA. It has not much to do with the DMA engine API. So I'm not going to bother trying to decode what you said above until you start using the right terminology. I'm a stickler for that, get used to it. No point talking about how round bananas are when you're actually discussing orange fruit. Sorry, my carelessness. I should have written DMAENGINE API, instead of DMA API. If you want I can repost with s/DMA API/DMAENGINE API/ So when a client asks for a channel, the dmaengine would return the first free channel that has just as much the requested capability. No. For slave DMA, peripheral drivers normally need a specific channel on the DMA controller. Of course they do. And It is the responsibility of platform to encode such information as a 'capability' of the channel. Capability to reach a particular peripheral, say. Peripheral drivers normally do not know which channel they need, or even which DMA controller they require. That information is specific to the platform and SoC. For that very reason I proposed, say, if MMC client driver is probed for 2nd instance of MMC controller, it should ask for the following capabilities (MMC TYPE_SHFT) | (2 INST_SHFT)//I don't show other fields for simplicity It is the job of _platform_ to set TYPE and INSTANCE fields to 'MMC' and 2 resp _only_ for the channel(s) that can reach its MMC_2 controller. That way, when the client 'generically' asks for DMA_MMC_2, it will be given only one of those channels that can talk to second instance of MMC controller. That is, the generic filter function would look equivalent of :- return is_free(chan) (cap_requested == cap_of(chan) cap_requested) Now it all boils down to defining the set of _practically_ possible capabilities and a method to exchange them between the API, Client and DMAC drivers. What are these capabilities? I had mentioned the list below under 'Assuming'. We may add more. The above is the overall approach. The following is my idea of implementing it as of now, I am open to suggestions. Assuming :- *** There are no more than 256 types of DMA'able devices (MMC, USB, SPI etc) -- [8bits] A type of device never has more than 16 instances in a platform (MMC-0, MMC-1, MMC-2 etc) -- [4bits] Mem-Mem, Mem-Dev, Dev-Mem, Dev-Dev capability in [4bits] No. You're being far too specific to your own DMA controller. Many DMA controllers (eg, PL08x) have a number of physical channels, all of which can be programmed for M2M, M2P, P2M and P2P. There is nothing in the design which restricts what a channel can do. I am not being specific, at least not on this point. Btw, I have worked on pl080 as well and I have tried to be not specific to not only pl330 but also pl080. If you notice I have assigned 4bits to the field. Each of the 4 bits can be set independently. So, a capable pl08x channel can set all 4 bits to indicate that it can do all 4 types - M2M, M2P, P2M and P2P. Max_Burst_Len for any channel is less than 64KB, in power of two [4bits] Support programmable Max_Burst_Len(tunable in geometric steps of 2) [1bit] Max_RW_width/fifo_width is less than 128, in power of two -- [3bits] Support programmable Max_RW_Width(tunable in geometric steps of 2) [1bit] Finally, No platform has more than 128 channels with identicial capabilities [7bits] There is absolutely no way that this will work in reality - are you on a different planet? As has already been described to you, MMCI can use one DMA channel, and programs it according to the DMA direction, and has in the past adjusted the burst size according to what's going on with the driver. With your model, it would
Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Tue, Jul 26, 2011 at 3:05 PM, Boojin Kim boojin@samsung.com wrote:
Jassi Brar Wrote:
Sent: Monday, July 25, 2011 8:52 PM
To: Boojin Kim
Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-
s...@vger.kernel.org; Vinod Koul; Dan Williams; Kukjin Kim; Grant
Likely; Mark Brown
Subject: Re: [PATCH V4 06/14] ARM: SAMSUNG: Add common DMA operations
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim boojin@samsung.com
wrote:
+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+ struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan-
private;
+ unsigned dma_ch = (unsigned)param;
+
+ if (peri-peri_id != dma_ch)
+ return false;
+
+ return true;
+}
This is what I meant... if we keep chan_id for paltform assigned IDs,
these filter functions could simply become
static bool pl330_filter(struct dma_chan *chan, void *param)
{
return chan-chan_id == param
}
And ideally in the long run, we could just drop the filter callback
and add expected channel ID to the request_channel call.
The chan_id is set by dmaengine. So, We don't use it to hold the user specific
Id.
Not for long.
See https://lkml.org/lkml/2011/7/26/245
+
+static inline int s3c_dma_trigger(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
+}
+
+static inline int s3c_dma_started(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
+}
+
+static inline int s3c_dma_flush(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
+}
+
+static inline int s3c_dma_stop(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
+}
+
+static struct samsung_dma_ops s3c_dma_ops = {
+ .request = s3c_dma_request,
+ .release = s3c_dma_release,
+ .prepare = s3c_dma_prepare,
+ .trigger = s3c_dma_trigger,
+ .started = s3c_dma_started,
+ .flush = s3c_dma_flush,
+ .stop = s3c_dma_stop,
These last 4 should be gnereallized into one callback with OP argument.
I don't have any idea about it. Can you explain it in more detail?
static int s3c_dma_control(unsigned ch, enum s3c2410_chan_op/*or similar*/ op)
{
return s3c2410_dma_ctrl(ch, op);
}
static struct samsung_dma_ops s3c_dma_ops = {
.request= s3c_dma_request,
.release= s3c_dma_release,
.prepare= s3c_dma_prepare,
.control= s3c_dma_control,
--
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, Jul 25, 2011 at 6:58 AM, Boojin Kim boojin@samsung.com wrote:
This patch adds DMA_CYCLIC capability that is used for audio driver.
DMA driver with DMA_CYCLIC capability reuses the dma requests that
were submitted through tx_submit().
Signed-off-by: Boojin Kim boojin@samsung.com
---
drivers/dma/pl330.c | 111
+++
1 files changed, 111 insertions(+), 0 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 880f010..121c75a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -69,6 +69,11 @@ struct dma_pl330_chan {
* NULL if the channel is available to be acquired.
*/
void *pl330_chid;
+
+ /* taks for cyclic capability */
+ struct tasklet_struct *cyclic_task;
+
+ bool cyclic;
};
We already have a tasklet_struct member here.
This 'cyclic' flag can indicate if we are doing cyclic or serial
transfers. So, this cyclic_task seems unnecessary.
+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);
+
+ /* Pick up ripe tomatoes */
+ list_for_each_entry_safe(desc, _dt, pch-work_list, node)
+ if (desc-status == DONE) {
+ dma_async_tx_callback callback;
+
+ list_move_tail(desc-node, pch-work_list);
+ pch-completed = desc-txd.cookie;
+
+ desc-status = PREP;
+
+ /* Try to submit a req imm.
+ next to the last completed cookie */
+ fill_queue(pch);
+
+ /* Make sure the PL330 Channel thread is active */
+ pl330_chan_ctrl(pch-pl330_chid, PL330_OP_START);
+
+ callback = desc-txd.callback;
+ if (callback)
+ callback(desc-txd.callback_param);
+
+ }
+
+ spin_unlock_irqrestore(pch-lock, flags);
+}
Please merge this with pl330_tasklet and use 'cyclic' flag to differentiate.
@@ -227,6 +267,9 @@ static void dma_pl330_rqcb(void *token, enum pl330_op_err
err)
spin_unlock_irqrestore(pch-lock, flags);
+ if (pch-cyclic_task)
+ tasklet_schedule(pch-cyclic_task);
+ else
tasklet_schedule(pch-task);
A tab here please, and check for 'cyclic' flag.
@@ -316,6 +359,15 @@ static void pl330_free_chan_resources(struct dma_chan
*chan)
pl330_release_channel(pch-pl330_chid);
pch-pl330_chid = NULL;
+ if (pch-cyclic) {
+ pch-cyclic = false;
+ list_splice_tail_init(pch-work_list, pch-dmac-desc_pool);
+ if (pch-cyclic_task) {
+ tasklet_kill(pch-cyclic_task);
+ pch-cyclic_task = NULL;
+ }
+ }
+
spin_unlock_irqrestore(pch-lock, flags);
}
To be explicit, please initialize 'cyclic' flag to false in
pl330_alloc_chan_resources
--
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
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim boojin@samsung.com wrote:
diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
index 4d79519..cb170a6 100644
--- a/arch/arm/plat-samsung/Kconfig
+++ b/arch/arm/plat-samsung/Kconfig
@@ -300,6 +300,14 @@ config S3C_PL330_DMA
help
S3C DMA API Driver for PL330 DMAC.
+config DMADEV_PL330
+ bool
+ select DMADEVICES
+ select PL330_DMA
+ select ARM_AMBA
+ help
+ Use DMA device engine for PL330 DMAC.
+
If this is for the 'wrapper' dma driver, please use S3C or similar prefix.
diff --git a/arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h
b/arch/arm/plat-samsung/include/plat/dma-pl330.h
similarity index 84%
rename from arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h
rename to arch/arm/plat-samsung/include/plat/dma-pl330.h
index 8107442..c402719 100644
--- a/arch/arm/plat-samsung/include/plat/s3c-dma-pl330.h
+++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h
@@ -8,19 +8,18 @@
* (at your option) any later version.
*/
-#ifndef __S3C_DMA_PL330_H_
-#define __S3C_DMA_PL330_H_
-
-#define S3C2410_DMAF_AUTOSTART (1 0)
-#define S3C2410_DMAF_CIRCULAR (1 1)
+#ifndef __DMA_PL330_H_
+#define __DMA_PL330_H_ __FILE__
To respect namespaces, please preserve S3C prefix because
the file is Samsung specific.
@@ -84,6 +83,14 @@ enum dma_ch {
DMACH_SLIMBUS4_TX,
DMACH_SLIMBUS5_RX,
DMACH_SLIMBUS5_TX,
+ DMACH_MTOM_0,
+ DMACH_MTOM_1,
+ DMACH_MTOM_2,
+ DMACH_MTOM_3,
+ DMACH_MTOM_4,
+ DMACH_MTOM_5,
+ DMACH_MTOM_6,
+ DMACH_MTOM_7,
/* END Marker, also used to denote a reserved channel */
DMACH_MAX,
};
Naming mem-mem channels should be unnecessary.
Not sure what you have in mind.
--
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 06/14] ARM: SAMSUNG: Add common DMA operations
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim boojin@samsung.com wrote:
+
+static bool pl330_filter(struct dma_chan *chan, void *param)
+{
+ struct dma_pl330_peri *peri = (struct dma_pl330_peri *)chan-private;
+ unsigned dma_ch = (unsigned)param;
+
+ if (peri-peri_id != dma_ch)
+ return false;
+
+ return true;
+}
This is what I meant... if we keep chan_id for paltform assigned IDs,
these filter functions could simply become
static bool pl330_filter(struct dma_chan *chan, void *param)
{
return chan-chan_id == param
}
And ideally in the long run, we could just drop the filter callback
and add expected channel ID to the request_channel call.
+
+static inline int s3c_dma_trigger(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_START);
+}
+
+static inline int s3c_dma_started(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STARTED);
+}
+
+static inline int s3c_dma_flush(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_FLUSH);
+}
+
+static inline int s3c_dma_stop(unsigned ch)
+{
+ return s3c2410_dma_ctrl(ch, S3C2410_DMAOP_STOP);
+}
+
+static struct samsung_dma_ops s3c_dma_ops = {
+ .request = s3c_dma_request,
+ .release = s3c_dma_release,
+ .prepare = s3c_dma_prepare,
+ .trigger = s3c_dma_trigger,
+ .started = s3c_dma_started,
+ .flush = s3c_dma_flush,
+ .stop = s3c_dma_stop,
These last 4 should be gnereallized into one callback with OP argument.
--
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 07/14] ARM: EXYNOS4: Use generic DMA PL330 driver
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim boojin@samsung.com wrote:
diff --git a/arch/arm/mach-exynos4/dma.c b/arch/arm/mach-exynos4/dma.c
index 564bb53..e1c00cf 100644
--- a/arch/arm/mach-exynos4/dma.c
+++ b/arch/arm/mach-exynos4/dma.c
@@ -21,151 +21,250 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
-#include linux/platform_device.h
#include linux/dma-mapping.h
+#include linux/amba/bus.h
+#include linux/amba/pl330.h
+#include asm/irq.h
#include plat/devs.h
#include plat/irqs.h
#include mach/map.h
#include mach/irqs.h
-
-#include plat/s3c-pl330-pdata.h
+#include mach/dma.h
static u64 dma_dmamask = DMA_BIT_MASK(32);
-static struct resource exynos4_pdma0_resource[] = {
- [0] = {
- .start = EXYNOS4_PA_PDMA0,
- .end = EXYNOS4_PA_PDMA0 + SZ_4K,
- .flags = IORESOURCE_MEM,
- },
- [1] = {
- .start = IRQ_PDMA0,
- .end = IRQ_PDMA0,
- .flags = IORESOURCE_IRQ,
+struct dma_pl330_peri pdma0_peri[32] = {
Please remove 32 from the list (it seems you don't always pass 32 elements)
Also for completeness, please explicitly write element indices below because
it's not some array of randomly placed elements.
Same for every other such occurance.
+ {
+ .peri_id = (u8)DMACH_PCM0_RX,
+ .rqtype = DEVTOMEM,
+ }, {
+ .peri_id = (u8)DMACH_PCM0_TX,
+ .rqtype = MEMTODEV,
+ }, {
+ .peri_id = (u8)DMACH_PCM2_RX,
+ .rqtype = DEVTOMEM,
+ }, {
+ .peri_id = (u8)DMACH_PCM2_TX,
+ .rqtype = MEMTODEV,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_MSM_REQ0,
+ }, {
+ .peri_id = (u8)DMACH_MSM_REQ2,
+ }, {
+ .peri_id = (u8)DMACH_SPI0_RX,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 1,
+ }, {
+ .peri_id = (u8)DMACH_SPI0_TX,
+ .rqtype = MEMTODEV,
+ .burst_sz = 1,
+ }, {
+ .peri_id = (u8)DMACH_SPI2_RX,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 1,
+ }, {
+ .peri_id = (u8)DMACH_SPI2_TX,
+ .rqtype = MEMTODEV,
+ .burst_sz = 1,
+ }, {
+ .peri_id = (u8)DMACH_I2S0S_TX,
+ .rqtype = MEMTODEV,
+ }, {
+ .peri_id = (u8)DMACH_I2S0_RX,
+ .rqtype = DEVTOMEM,
+ }, {
+ .peri_id = (u8)DMACH_I2S0_TX,
+ .rqtype = MEMTODEV,
+ }, {
+ .peri_id = (u8)DMACH_UART0_RX,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_UART0_TX,
+ .rqtype = MEMTODEV,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_UART2_RX,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_UART2_TX,
+ .rqtype = MEMTODEV,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_UART4_RX,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_UART4_TX,
+ .rqtype = MEMTODEV,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_SLIMBUS0_RX,
+ .rqtype = DEVTOMEM,
+ }, {
+ .peri_id = (u8)DMACH_SLIMBUS0_TX,
+ .rqtype = MEMTODEV,
+ }, {
+ .peri_id = (u8)DMACH_SLIMBUS2_RX,
+ .rqtype = DEVTOMEM,
+ }, {
+ .peri_id = (u8)DMACH_SLIMBUS2_TX,
+ .rqtype = MEMTODEV,
+ }, {
+ .peri_id = (u8)DMACH_SLIMBUS4_RX,
+ .rqtype = DEVTOMEM,
+ }, {
+ .peri_id = (u8)DMACH_SLIMBUS4_TX,
+ .rqtype = MEMTODEV,
+ }, {
+ .peri_id = (u8)DMACH_AC97_MICIN,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_AC97_PCMIN,
+ .rqtype = DEVTOMEM,
+ .burst_sz = 4,
+ }, {
+ .peri_id = (u8)DMACH_AC97_PCMOUT,
+ .rqtype = MEMTODEV,
+ .burst_sz = 4,
},
};
-static struct s3c_pl330_platdata exynos4_pdma0_pdata = {
- .peri = {
- [0] = DMACH_PCM0_RX,
- [1] = DMACH_PCM0_TX,
- [2] = DMACH_PCM2_RX,
- [3] = DMACH_PCM2_TX,
- [4] = DMACH_MSM_REQ0,
- [5] = DMACH_MSM_REQ2,
- [6] = DMACH_SPI0_RX,
- [7] = DMACH_SPI0_TX,
- [8] = DMACH_SPI2_RX,
- [9] = DMACH_SPI2_TX,
- [10] =
Re: [PATCH V4 14/14] ARM: SAMSUNG: Remove Samsung specific enum type for dma direction
On Mon, Jul 25, 2011 at 6:58 AM, Boojin Kim boojin@samsung.com wrote: This patch removes the samsung specific enum type 's3c2410_dmasrc' and uses 'dma_data_direction' instead. Signed-off-by: Boojin Kim boojin@samsung.com Signed-off-by: Kukjin Kim kgene@samsung.com Acked-by: Jassi Brar jassisinghb...@gmail.com If this patch was first in the patchset, probably it could have been already merged. -- 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-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Thu, Jul 21, 2011 at 12:04 PM, Boojin Kim boojin@samsung.com wrote:
Jassi Brar wrote:
Sent: Thursday, July 21, 2011 2:00 PM
To: Boojin Kim
Cc: linux-arm-ker...@lists.infradead.org; linux-samsung-soc@vger.kernel.org;
Vinod Koul; Dan Williams; Kukjin Kim
Subject: Re: [PATCH V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim boojin@samsung.com wrote:
- pl330_tasklet((unsigned long) pch);
+ list_splice_tail_init(list, pdmac-desc_pool);
+ spin_unlock_irqrestore(pch-lock, flags);
+ break;
+ case DMA_SLAVE_CONFIG:
Please protect this section too using spin_lock.
Why is spin_lock need here?
This code just sets configuration data into own channel structure.
It does seem unncessary, but the configuration that is set here is read
in other parts of the driver. However unlikely but there is theoretical
possibility of race here - one shouldn't count upon goodness of client
drivers.
Yes, Client driver doesn't afflict the configuration data again.
So, I think spin_lock isn't required here.
+ 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);
+ } 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);
+ }
PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.
First, DMA machine code should know the FIFO address of all client drivers
to
set via platform data.
In this case, Problem is that the definition of FIFO address is almost
declared to the header file of client driver.
For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of
fifo
address as following.
#define S3C2412_IISTXD (0x10)
#define S3C2412_IISRXD (0x14)
These definitions should be referred to DMA machine code to set fifo
address
through platform data.
I think it's not good method.
Crap!
Client drivers don't conjure up the fifo address - if not hardcoded they
are gotten via platform_data already.
For it, DMA machine code should include all header files of client driver that
has definition of FIFO address.
The number of header file would be more than 10. And I think it make the code
a little complicated.
And, SLAVE_CONFIG command exist to pass slave configuration data from
client
driver to DMA driver.
So, I think that it's proper to pass fifo address through SLAVE_CONFIG
command.
If it's still not clear, read the para above definition of 'struct
dma_slave_config'
in include/linux/dmaengine.h
Other DMA client drivers in mainline code already use SLAVE_CONFIG command to
pass fifo address as following.
Please refer to Sound/soc/imx/imx-pcm-dma/mx2.c, Driver/mmc/host/mmci.c,
Drivers/mmc/host/mxcmmc.c and so on.
And, Other DMA drivers also support to SLAVE_CONFIG command for it.
Please refer to amba-pl08x.c, coh901318.c, ste_dma40.c and so on in
'driver/dma' directory.
So, In my opinion, this is proper method to handle the client's fifo address.
NAK.
--
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-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Thu, Jul 21, 2011 at 3:53 PM, Linus Walleij linus.wall...@linaro.org wrote:
On Thu, Jul 21, 2011 at 11:14 AM, Jassi Brar jassisinghb...@gmail.com wrote:
On Thu, Jul 21, 2011 at 1:41 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Thu, Jul 21, 2011 at 12:47:49AM +0530, Jassi Brar wrote:
PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.
That's utter crap, and isn't what the DMA engine API is about.
The above looks correctly implemented. Slave DMA engine users are
supposed to supply the device DMA register address via this
DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA
device is braindead.
Rather than have 32 client drivers expect fifo_address from the
platform and then
provide to the DMAC, IMHO it is better for a single driver(DMAC) to
get 32 addresses
from the platform.
My idea (when I implemented it) is that the device driver knows the
physical and virtual base and thus the address where DMA data is
destined. It knows all other registers, it remaps them, noone else should
be bothered with containing the knowledge of adress ranges for the
specific hardware block.
Passing this through platform data requires the machine to keep track
not only of the base adress of the peripheral (as is generally contained
in the machine or device tree) but also the offset of specific registers
in that memory region, which is too much.
Usually this means the offsets are defined in files like mach/perfoo.h
which means they can't be pushed down into drivers/foo/foo.c and
creates unnecessary bindings and de-encapsulation of the driver.
We want to get rid of such stuff. (I think?)
Therefore I introduced this to confine the different registers into
the driver itself and ask the DMA engine to transfer to a specific
address.
While that does make sense, it makes mandatory the ritual of calling
DMA_SLAVE_CONFIG mandatory for most of the cases.
And I think the effort to set fifo_addr for peripherals is overrated.
Esp when the DMAC driver already expect similar h/w
parameter -- *direction*.
I don't understand why is 'fifo_address' a property much different
than 'direction' of the
channel ?
struct dma_slave_config {
enum dma_data_direction direction;
dma_addr_t src_addr;
dma_addr_t dst_addr;
enum dma_slave_buswidth src_addr_width;
enum dma_slave_buswidth dst_addr_width;
u32 src_maxburst;
u32 dst_maxburst;
};
We do support changing direction as well as src and dst address
(i.e. FIFO endpoint) at runtime.
Check drivers/mmc/host/mmci.c for an example of how we switch
a single channel from TX to RX in runtime using this one property.
However it has been noted by Russell that the direction member
is unnecessary since the device_prep_slave_sg() function in the
dmaengine vtable already takes a direction argument like this:
struct dma_async_tx_descriptor *(*device_prep_slave_sg)(
struct dma_chan *chan, struct scatterlist *sgl,
unsigned int sg_len, enum dma_data_direction direction,
unsigned long flags);
Therefore the direction setting needs to go from either the config
struct or the device_prep_slave_sg() call, as right now the channel
is being told about the direction twice which is not elegant and
confusing.
So you even have two ways of changing direction at runtime...
Of course there are ways to set the direction... but whatever we do
it can't really be changed from what h/w can only do.
And that is my point. Let clients not bother trying to 'configure' what is
the only supported option by h/w.
And I don't suggest dropping the DMA_SLAVE_CONFIG callback - just
keep it for rarer situations when we have configurable channels.
And I assumed that was your original idea too.
---
* @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers
* that need to runtime reconfigure the slave channels (as opposed to passing
* configuration data in statically from the platform). An additional
* argument of struct dma_slave_config must be passed in with this
* command.
---
Regards,
-j
Yours,
Linus Walleij
--
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-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: PL330 has fixed channels to peripherals. So FIFO addresses(burst_sz too?) should already be set via platform data. Client drivers shouldn't bother. That's utter crap, and isn't what the DMA engine API is about. The above looks correctly implemented. Slave DMA engine users are supposed to supply the device DMA register address via this DMA_SLAVE_CONFIG call. Doing this via platform data for the DMA device is braindead. Rather than have 32 client drivers expect fifo_address from the platform and then provide to the DMAC, IMHO it is better for a single driver(DMAC) to get 32 addresses from the platform. Esp when the DMAC driver already expect similar h/w parameter -- *direction*. Conversely, when a platform doesn't know where the FIFOs are because they're located inside the actual devices, and the device driver does, then it makes much more sense for the device driver to provide the DMA engine with the bus address of that. Yes, that is a case to consider. Perhaps we already do something like that while setting i2c addresses of attached devices in board files... and similarly it might be possible for the developer to know what the fifo address is going to be after the device is up and running esp when it is interfaced with an internal component like DMA. Does your hardware have a hardware block from the device itself containing all the systems FIFOs ? I am not sure what you ask, but let me say what I know. In this case atleast, all PL330 DMA channels have fixed source/destination address on the device side. So it's not like developer doesn't know fifo_addr here. I don't understand why is 'fifo_address' a property much different than 'direction' of the channel ? Some channels can do memory-to-peripheral and peripheral-to-memory transfers. Some peripherals provide a single set of DMA request/ack lines to perform this function. Some peripherals have different addresses for the TX and RX FIFOs within the device. Likewise we had something like that for I2S IP of S3C2440(or A0 ?) a single fifo address shared by TX and RX channel. Depending if it's full/half duplex capable we can have both or one 'virtual' channel active at a time. If a channel is flexible enough to change it's 'fifo_address', most probably it could also change direction of transfers. There are DMA engines and setups where that's true. Of course, and I think this 'runtime' configuration should be done only for such cases. thnx -- 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-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Thu, Jul 21, 2011 at 8:53 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Thu, Jul 21, 2011 at 08:42:40PM +0530, Jassi Brar wrote: On Thu, Jul 21, 2011 at 4:59 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: Does your hardware have a hardware block from the device itself containing all the systems FIFOs ? I am not sure what you ask, but let me say what I know. In this case atleast, all PL330 DMA channels have fixed source/destination address on the device side. So it's not like developer doesn't know fifo_addr here. Even so, your approach is _conceptually_ wrong. Think about it. You declare your devices giving the bus address where they're located. So, lets say for argument that your UART is located at 0x10001000. Your UART driver knows that the FIFO register is at offset 0x20 from the base address. Your platform data provides the UART driver with a DMA match function and data specific for that match function. This data encodes the specific DMA channel. Now, why should you have to encode into the DMA drivers platform data that DMA channel X has its FIFO at 0x10001020? Not only do you have to declare the base address of the UART but also you have to know the offset into the UART. Why not just let the UART driver - which knows that the base address is 0x10001000, and the FIFO is at offset 0x20 above that - tell the DMA driver that's where the FIFO is located? Yes I understand, the idea was to avoid optional DMA_SLAVE_CONFIG call. Apparently we give different weightage to the pros and cons. Anyways, I accept your opinion. Though you might want to consider changing the DMA_SLAVE_CONFIG API from optional to mandatory for Slave capable DMACs. Otherwise I don't see common client drivers working over different DMACs. 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 V4 03-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim boojin@samsung.com wrote:
Signed-off-by: Boojin Kim boojin@samsung.com
Signed-off-by: Kukjin Kim kgene@samsung.com
---
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);
+ } 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);
+ }
+ break;
As discussed yesterday, DMA_SLAVE_CONFIG is assumed to be madatory,
so please dismantle the 'struct dma_pl330_peri', move fifo_addr,
burst_sz and rqtype
to 'struct dma_pl330_chan' and poison them - that will force clients
to do DMA_SLAVE_CONFIG. Move 'peri_id' to 'struct dma_pl330_platdata'
And protect the changes to channel parameters with lock.
--
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 01/13] DMA: PL330: Add support runtime PM for PL330 DMAC
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim boojin@samsung.com wrote:
Signed-off-by: Boojin Kim boojin@samsung.com
Cc: Vinod Koul vinod.k...@intel.com
Cc: Dan Williams dan.j.willi...@intel.com
Signed-off-by: Kukjin Kim kgene@samsung.com
Acked-by: Jassi Brar jassisinghb...@gmail.com
---
drivers/dma/pl330.c | 75 +-
1 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 6abe1ec..b7ecf47 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -17,6 +17,7 @@
#include linux/interrupt.h
#include linux/amba/bus.h
#include linux/amba/pl330.h
+#include linux/pm_runtime.h
#define NR_DEFAULT_DESC 16
@@ -83,6 +84,8 @@ struct dma_pl330_dmac {
/* Peripheral channels connected to this DMAC */
struct dma_pl330_chan peripherals[0]; /* keep at end */
+
+ struct clk *clk;
};
struct dma_pl330_desc {
@@ -696,6 +699,30 @@ pl330_probe(struct amba_device *adev, const struct
amba_id *id)
goto probe_err1;
}
+ pdmac-clk = clk_get(adev-dev, dma);
+ if (IS_ERR(pdmac-clk)) {
+ dev_err(adev-dev, Cannot get operation clock.\n);
+ ret = -EINVAL;
+ goto probe_err1;
+ }
+
+ 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
+ /* enable dma clk */
+ clk_enable(pdmac-clk);
+#endif
+
irq = adev-irq[0];
ret = request_irq(irq, pl330_irq_handler, 0,
dev_name(adev-dev), pi);
@@ -763,8 +790,6 @@ pl330_probe(struct amba_device *adev, const struct
amba_id *id)
goto probe_err4;
}
- amba_set_drvdata(adev, pdmac);
-
dev_info(adev-dev,
Loaded driver for PL330 DMAC-%d\n, adev-periphid);
dev_info(adev-dev,
@@ -825,6 +850,13 @@ 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
+ clk_disable(pdmac-clk);
+#endif
+
kfree(pdmac);
return 0;
@@ -838,10 +870,49 @@ static struct amba_id pl330_ids[] = {
{ 0, 0 },
};
+#ifdef CONFIG_PM_RUNTIME
+static int pl330_runtime_suspend(struct device *dev)
+{
+ struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
+
+ if (!pdmac) {
+ dev_err(dev, failed to get dmac\n);
+ return -ENODEV;
+ }
+
+ clk_disable(pdmac-clk);
+
+ return 0;
+}
+
+static int pl330_runtime_resume(struct device *dev)
+{
+ struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
+
+ if (!pdmac) {
+ dev_err(dev, failed to get dmac\n);
+ return -ENODEV;
+ }
+
+ clk_enable(pdmac-clk);
+
+ return 0;
+}
+#else
+#define pl330_runtime_suspend NULL
+#define pl330_runtime_resume NULL
+#endif /* CONFIG_PM_RUNTIME */
+
+static const struct dev_pm_ops pl330_pm_ops = {
+ .runtime_suspend = pl330_runtime_suspend,
+ .runtime_resume = pl330_runtime_resume,
+};
+
static struct amba_driver pl330_driver = {
.drv = {
.owner = THIS_MODULE,
.name = dma-pl330,
+ .pm = pl330_pm_ops,
},
.id_table = pl330_ids,
.probe = pl330_probe,
--
1.7.1
--
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-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Wed, Jul 20, 2011 at 4:16 PM, Boojin Kim boojin@samsung.com wrote:
Signed-off-by: Boojin Kim boojin@samsung.com
Signed-off-by: Kukjin Kim kgene@samsung.com
---
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:
Please protect this section too using spin_lock.
+ 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);
+ } 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);
+ }
PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.
will review remaining patches soon, gotta go now
--
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-1/13] DMA: PL330: Support DMA_SLAVE_CONFIG command
On Thu, Jul 21, 2011 at 9:43 AM, Boojin Kim boojin@samsung.com wrote:
- pl330_tasklet((unsigned long) pch);
+ list_splice_tail_init(list, pdmac-desc_pool);
+ spin_unlock_irqrestore(pch-lock, flags);
+ break;
+ case DMA_SLAVE_CONFIG:
Please protect this section too using spin_lock.
Why is spin_lock need here?
This code just sets configuration data into own channel structure.
It does seem unncessary, but the configuration that is set here is read
in other parts of the driver. However unlikely but there is theoretical
possibility of race here - one shouldn't count upon goodness of client drivers.
+ 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);
+ } 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);
+ }
PL330 has fixed channels to peripherals.
So FIFO addresses(burst_sz too?) should already be set via platform data.
Client drivers shouldn't bother.
First, DMA machine code should know the FIFO address of all client drivers to
set via platform data.
In this case, Problem is that the definition of FIFO address is almost
declared to the header file of client driver.
For example, sound\soc\samsung\regs-i2s-v2.h only has the definition of fifo
address as following.
#define S3C2412_IISTXD (0x10)
#define S3C2412_IISRXD (0x14)
These definitions should be referred to DMA machine code to set fifo address
through platform data.
I think it's not good method.
Crap!
Client drivers don't conjure up the fifo address - if not hardcoded they
are gotten via platform_data already.
And, SLAVE_CONFIG command exist to pass slave configuration data from client
driver to DMA driver.
So, I think that it's proper to pass fifo address through SLAVE_CONFIG
command.
If it's still not clear, read the para above definition of 'struct
dma_slave_config'
in include/linux/dmaengine.h
Second, 'burst_sz' isn't fixed value. Namely, Client driver changes 'burst_sz'
according to its usecase.
For example, I2S driver sets 'burst_sz' to 4 in case of stereo
playback/recording. But in case of mono playback/recording, It changes
'burst_sz' to 2.
So, Client driver should use SLAVE_CONFIG command to set 'burst_sz' because
it's not fixed value.
Yeah yeah, ok, that's why I put that with a ? in bracket.
I just don't remember testing with fixed burst_sz with pl330.
--
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] ASoC: SAMSUNG: 24-bit audio playback on Exynos4210
On Wed, Jul 13, 2011 at 4:52 PM, Giridhar Maruthy giridhar.maru...@linaro.org wrote: Using 256fs or 512fs will result in distortion of 24-bit audio samples. This is because the lrclk generated is not proper. Using 384 fs generates proper output. Signed-off-by: Giridhar Maruthy giridhar.maru...@linaro.org --- sound/soc/samsung/smdk_wm8994.c | 5 - 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/sound/soc/samsung/smdk_wm8994.c b/sound/soc/samsung/smdk_wm8994.c index e7c1009..45fbe2b 100644 --- a/sound/soc/samsung/smdk_wm8994.c +++ b/sound/soc/samsung/smdk_wm8994.c @@ -8,6 +8,7 @@ */ #include ../codecs/wm8994.h +#include sound/pcm_params.h /* * Default CFG switch settings to use this driver: @@ -44,7 +45,9 @@ static int smdk_hw_params(struct snd_pcm_substream *substream, int ret; /* AIF1CLK should be =3MHz for optimal performance */ - if (params_rate(params) == 8000 || params_rate(params) == 11025) + if (params_format(params) == SNDRV_PCM_FORMAT_S24_LE) + pll_out = params_rate(params) * 384; + else if (params_rate(params) == 8000 || params_rate(params) == 11025) pll_out = params_rate(params) * 512; else pll_out = params_rate(params) * 256; It might be a good idea to maintain a matrix of sample rates and sizes tested cleanly. Thanks, Jassi -- 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/7] DMA: PL330: Add DMA capabilities
On Thu, Jul 14, 2011 at 6:27 AM, boojin boojin@samsung.com wrote:
Jassi Brar wrote:
On Tue, Jul 5, 2011 at 12:03 PM, Chanho Park parkc...@gmail.com wrote:
Kukjin Kim kgene.kim at samsung.com writes:
(snip)
+ 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) {
+ i = 0;
+ while (slave_config-dst_addr_width != (1
i))
+ i++;
+ peri-burst_sz = i;
+ }
+ } 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) {
+ i = 0;
+ while (slave_config-src_addr_width != (1
i))
+ i++;
+ peri-burst_sz = i;
Re-send including cc and mailing lists
--
pl330 dmac only supports 1/2/4/8/16 bytes burst size.
If some bad D/D doesn't use powers of 2 width,
dmaengine is going to infinite loop.
You'd better check it instead of running loop.
It might be even better to start with max possible width and
keep decreasing 'i' in the loop.
I will addressed your comment on next release.
Actually, best would be to simply do
peri-burst_sz = __ffs(slave_config-src_addr_width);
And sorry I haven't yet reviewed the patches because I have been
very busy lately. Will be more active in a few days.
Re: [PATCH 6/7] spi/s3c64xx: Add support DMA engine API
On Tue, Jul 5, 2011 at 12:35 PM, Kukjin Kim kgene@samsung.com wrote: Grant Likely wrote: On Mon, Jul 04, 2011 at 09:51:43PM +0200, Heiko Stübner wrote: Am Montag 04 Juli 2011, 19:02:17 schrieb Grant Likely: On Mon, Jul 04, 2011 at 06:59:11PM +0200, Heiko Stübner wrote: Am Montag 04 Juli 2011, 18:42:51 schrieb Grant Likely: Wow. A lot of #ifdefs here. It does not look multiplatform friendly at all. Are the s3c2410_dma functions obsolete when DMADEV_PL330 is selected? If so, can they be removed entirely, or are they required to support certain hardware? The spi_s3c64xx driver can also support the SPI controller of the S3C2416/S3C2443 line of SoCs. As I'm currently working on a S3C2416 based project, my small wish from the sidelines would be to not break this support with whatever solution you will decide on :-) . I will not merge a patch that either breaks a platform, or requires a compile time either/or choice of device support (enabling support for one device should not break support for another). The patch above seems to contain the support for both SoCs (i.e. s3c2410_dma_* for S3C2416 etc), so it wouldn't break the support. But this form will definitly break future multiplatform kernels when the 2416 and some variant using the DMADEV_PL330 are selected at the same time. Yes, that's the breakage I'm talking about. The 2416/2443 seem to be the first Samsung-SoCs to have a SPI-controller of this type. Implementing the DMA engine stuff for these SoCs would obviously solve the ifdef-problem but I don't know if this is possible to do. Of course it's possible, it's just software. :-D If a config option is either/or then it needs to be really well justified before I will accept it. Most either/or options I've seen aren't for any strong technical reason other than it was easier to code that way. Hi all, Yes I know your concerns on this. First of all, please see below which is block diagram of Samsung DMA usage and 1st step will be finished with other patches for S5PC100 and S5PV210 soon. Basically we need more time maybe 2 or 3 days to test on boards. +-+ | Each drivers which uses DMA | +-+ | S3C DMA API (such as s3c2410_dma_) | +---+-+ | PL080 DMA driver | S3C PL330 DMA API driver | | for S3C24XX and S3C64XX | (arch/arm/plat-samsung/s3c-pl330.c) | | +-+ | (arch/arm/plat-s3c24xx/dma.c) | Common DMA core driver | | (arch/arm/mach-s3c64xx/dma.c) | (arch/arm/common/pl330.c) | +---+-+ || (1st step. removing S3C DMA API for PL330) || \/ +-+ | Each drivers which uses DMA | +---+-+ | S3C DMA API(s3c2410_dma_xxx) | DMA generic API for PL330 | +---+-+ | PL080 DMA driver | PL330 DMA API driver | | for S3C24XX and S3C64XX | (drivers/dma/pl330.c) | | +-+ | (arch/arm/plat-s3c24xx/dma.c) | Common DMA core driver | | (arch/arm/mach-s3c64xx/dma.c) | (arch/arm/common/pl330.c) | +---+-+ As you saw, the S3C_DMA_API is still used for PL080 DMA for compatibilities with this patches in each driver, e.g., spi and audio. As a note, S3C24XX and S3C64XX include PL080 DMAC not PL330. Not exactly. S3C24XX use 2-3 different versions of Samsung's DMA implementations. S3C6400/10 use PL080 S5P and later series use PL330 Of course, if we can remove every S3C_DMA_API, that can be the best solution but it needs more time. I'd like to go to our goal step by step. Ideally, like PL330 for S5P, drivers/dma/pl080 should be enabled for s3c64xx And new generic dma api drivers written for S3C24xx in drivers/dma/ -j -- 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 6/7] spi/s3c64xx: Add support DMA engine API
On Tue, Jul 5, 2011 at 4:57 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Jul 05, 2011 at 04:46:47PM +0530, Jassi Brar wrote: Ideally, like PL330 for S5P, drivers/dma/pl080 should be enabled for s3c64xx And new generic dma api drivers written for S3C24xx in drivers/dma/ Another solution would be to write a DMA engine driver which massages the Samsung DMA API into the DMA engine way of doing things. You can then convert all the Samsung drivers to using DMA engine, and then sort out the Samsung DMA API as a separate issue. OK, but before that we need to see if the effort involved in writing the api 'mapping' driver would be sufficiently lesser than that of writing a common driver for 24xx, which IIRC are variations of each other. -- 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] SPI: SAMSUNG: Bug fix for SPI with different FIFO level
On Fri, Jul 1, 2011 at 11:29 AM, padma venkat padma@gmail.com wrote: Hi Jassi, On Fri, Jul 1, 2011 at 11:22 AM, Jassi Brar jassisinghb...@gmail.com wrote: On Fri, Jul 1, 2011 at 11:16 AM, padma venkat padma@gmail.com wrote: Hi Tony, On Thu, Jun 30, 2011 at 4:30 PM, Tony Nadackal ton...@gmail.com wrote: Hi Padma, With regards to your patch, even though one can check the tx done status using the TX_DONE bit, the present macro itself would work perfectly fine if the 'fifo_lvl_mask' is set properly. For example in 6450 channel 1, the fifo_lvl_mask should be 0x1ff (for 9bits, 15:23), while even in your patch, it is wrongly set as 0x7f(only 7bits). Thus, if this fifo_lvl_mask was defined correctly, the existing macro would itself have worked. Thanks for your comment. I considered changing to the fifo_lvl_mask to 1ff as you mentioned. But I think that the fifo_lvl_mask reflects the actual FIFO capacity in the SPI driver. For the failing channels the FIFO trigger level is 64 bytes and so i retained that value. In the driver it polls till the FIFO capacity level otherwise it goes for DMA.So if we keep the FIFO level as 1ff when the actual capacity is 7f then it fails. Jassi what do you think about this? 'fifo_lvl_mask' is h/w specific and can't be set for convenience. I don't have access to post-s3c64xx datasheets. Please check and reply if TX_DONE bit is at same offset for all channels of an SoC, because I suspect it's otherwise. Yes. The TX_DONE bit is at the same offset for all the channels of an SoC. in S5P64X0,S5PV210 and S5PV310 it is at offset 25. Then, Patches-1,2 Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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] SPI: SAMSUNG: Bug fix for SPI with different FIFO level
On Thu, Jun 30, 2011 at 6:08 PM, Padmavathi Venna padm...@samsung.com wrote: Fixed the bug in transmission status check for 64 bytes FIFO level. Signed-off-by: Padmavathi Venna padm...@samsung.com --- drivers/spi/spi_s3c64xx.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c index 795828b..8945e20 100644 --- a/drivers/spi/spi_s3c64xx.c +++ b/drivers/spi/spi_s3c64xx.c @@ -116,9 +116,7 @@ (((i)-fifo_lvl_mask + 1))) \ ? 1 : 0) -#define S3C64XX_SPI_ST_TX_DONE(v, i) v) (i)-rx_lvl_offset) \ - (((i)-fifo_lvl_mask + 1) 1)) \ - ? 1 : 0) +#define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) (1 (i)-tx_st_done)) ? 1 : 0) IIRC the macro is already designed to deduct tx-done levels from other fields. Could you please _explain_ with one example where it fails ? It is difficult to see without numbers. Thanks, -Jassi -- 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/7] ARM: SAMSUNG: Cleanup SPI platform specific code
On Thu, Jun 30, 2011 at 5:55 PM, Padmavathi Venna padm...@samsung.com wrote: This patchset does the following: 1. Move duplicated code to common place [PATCH 1/7] ARM: SAMSUNG: Move SPI device definitions to plat-samsung SPI platform devices are defined in respective machine folder of Samsung S3C64XX and S5P series SoCs.This is duplicated for every SoC. So all SPI platform devices are moved to a common place. The machine specific code is put in machine specific location for some reason. And the code is not duplicated, it's mostly data structures initialized with machine specific values. Have you considered if it would still be possible to build kernel image supporting more than 1 soc after your changes ? -- 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/7] ARM: SAMSUNG: Add SPI clock definitions for SAMSUNG SoCs
On Thu, Jun 30, 2011 at 3:00 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Thu, Jun 30, 2011 at 08:25:19AM -0400, Padmavathi Venna wrote: SPI Clocks were defined in dev-spi.c of machine specific folder. To make SPI devices common across all SoCs,dev-spi.c is moved from machine specific folder to plat-samsung. So SPI clock definitions has been moved from dev-spi.c to SoC specific machine folder for S3C64XX,S5P64X0,S5PC100 and S5PV210 This just shows how sick the Samsung code is by abusing the clk API. If it stopped that abuse and switched over to clkdev none of this would be necessary. I am anyways not for the patch set. Though this 'sickness' predates clkdev, and I agree it should be changed now. -- 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] SPI: SAMSUNG: Bug fix for SPI with different FIFO level
On Thu, Jun 30, 2011 at 2:35 PM, padma venkat padma@gmail.com wrote: Hi, On Thu, Jun 30, 2011 at 12:38 PM, Jassi Brar jassisinghb...@gmail.com wrote: On Thu, Jun 30, 2011 at 6:08 PM, Padmavathi Venna padm...@samsung.com wrote: Fixed the bug in transmission status check for 64 bytes FIFO level. Signed-off-by: Padmavathi Venna padm...@samsung.com --- drivers/spi/spi_s3c64xx.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.hc index 795828b..8945e20 100644 --- a/drivers/spi/spi_s3c64xx.c +++ b/drivers/spi/spi_s3c64xx.c @@ -116,9 +116,7 @@ (((i)-fifo_lvl_mask + 1))) \ ? 1 : 0) -#define S3C64XX_SPI_ST_TX_DONE(v, i) v) (i)-rx_lvl_offset) \ - (((i)-fifo_lvl_mask + 1) 1)) \ - ? 1 : 0) +#define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) (1 (i)-tx_st_done)) ? 1 : 0) IIRC the macro is already designed to deduct tx-done levels from other fields. Could you please _explain_ with one example where it fails ? It is difficult to see without numbers. The existing macro fails for following scenarios. 1) S5P64X0 channel 1 2) S5PV210 channel 1 3) S5PV310 channel 1 and channel 2 The FIFO data level supported in the above SoCs either 64 or 256 bytes depending on the channel. Because of this the TX_DONE is the 25 bit in the status register. The existing macro works for the following scenarios 1) S3C6410 all channels 2) S5PC100 all channels The FIFO data level supported in the above SoCs 64 bytes on all the channels. Because of this the TX_DONE is the 21 bit in the status register. So when we use the existing macro for the non-working SoCs it is not anding with the TX_DONE bit but it is only anding the bits earlier to TX_DONE bit. I see. I don't have access to post s3c64xx datasheets. Please confirm if TX_DONE bit at same offset for all channels of an SoC. If so, I am OK with these 2 patches. Thanks, Jassi -- 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/7] ARM: SAMSUNG: Cleanup SPI platform specific code
On Thu, Jun 30, 2011 at 2:57 PM, padma venkat padma@gmail.com wrote: Hi, On Thu, Jun 30, 2011 at 1:01 PM, Jassi Brar jassisinghb...@gmail.com wrote: On Thu, Jun 30, 2011 at 5:55 PM, Padmavathi Venna padm...@samsung.com wrote: This patchset does the following: 1. Move duplicated code to common place [PATCH 1/7] ARM: SAMSUNG: Move SPI device definitions to plat-samsung SPI platform devices are defined in respective machine folder of Samsung S3C64XX and S5P series SoCs.This is duplicated for every SoC. So all SPI platform devices are moved to a common place. The machine specific code is put in machine specific location for some reason. And the code is not duplicated, it's mostly data structures initialized with machine specific values. Have you considered if it would still be possible to build kernel image supporting more than 1 soc after your changes ? I didn't consider the single image scenario. Because as far as I know, the existing Samsung code doesn't have support for building a single kernel image for multiple SoCs. Samsung did use to support during 24xx days. Unfortunately not much with new generation SoCs. This patch-set only takes us further away from having multiple SoCs supported in a single kernel image some day. The intention behind my changes were 1) To reuse the dev-spi files for all the SoCs, as this reduces the code size. I already decided in favor of being able to support multiple SoCs in single image. Also future SoCs with SPI can use the same file. Only if they have _exactly_ same SPI block. Then most probably they'll have most other blocks identical too (not sure if I can name such examples in public) and can be supported under the same SoC name. Otherwise different SoCs spanning last 5yrs are already supported by the same SPI driver. We must provide SoC specific bits to the driver either via one place or the other. -j -- 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: DMA Cleanup as per sparse
On Thu, Jun 30, 2011 at 3:46 PM, Sangwook Lee sangwook@linaro.org wrote: Function declaration differs between file:s3c-pl330.c and file:dma.h and SPARSE (Documentation/sparse.txt) gives error messages Signed-off-by: Sangwook Lee sangwook@linaro.org Acked-by: Jassi Brar jassisinghb...@gmail.com --- arch/arm/plat-samsung/include/plat/dma.h | 16 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/arch/arm/plat-samsung/include/plat/dma.h b/arch/arm/plat-samsung/include/plat/dma.h index 2e8f8c6..7365f46 100644 --- a/arch/arm/plat-samsung/include/plat/dma.h +++ b/arch/arm/plat-samsung/include/plat/dma.h @@ -62,7 +62,7 @@ typedef int (*s3c2410_dma_opfn_t)(struct s3c2410_dma_chan *, * request a dma channel exclusivley */ -extern int s3c2410_dma_request(unsigned int channel, +extern int s3c2410_dma_request(enum dma_ch id, struct s3c2410_dma_client *, void *dev); @@ -71,14 +71,14 @@ extern int s3c2410_dma_request(unsigned int channel, * change the state of the dma channel */ -extern int s3c2410_dma_ctrl(unsigned int channel, enum s3c2410_chan_op op); +extern int s3c2410_dma_ctrl(enum dma_ch id, enum s3c2410_chan_op op); /* s3c2410_dma_setflags * * set the channel's flags to a given state */ -extern int s3c2410_dma_setflags(unsigned int channel, +extern int s3c2410_dma_setflags(enum dma_ch id, unsigned int flags); /* s3c2410_dma_free @@ -86,7 +86,7 @@ extern int s3c2410_dma_setflags(unsigned int channel, * free the dma channel (will also abort any outstanding operations) */ -extern int s3c2410_dma_free(unsigned int channel, struct s3c2410_dma_client *); +extern int s3c2410_dma_free(enum dma_ch id, struct s3c2410_dma_client *); /* s3c2410_dma_enqueue * @@ -95,7 +95,7 @@ extern int s3c2410_dma_free(unsigned int channel, struct s3c2410_dma_client *); * drained before the buffer is given to the DMA system. */ -extern int s3c2410_dma_enqueue(unsigned int channel, void *id, +extern int s3c2410_dma_enqueue(enum dma_ch idx, void *id, dma_addr_t data, int size); /* s3c2410_dma_config @@ -103,14 +103,14 @@ extern int s3c2410_dma_enqueue(unsigned int channel, void *id, * configure the dma channel */ -extern int s3c2410_dma_config(unsigned int channel, int xferunit); +extern int s3c2410_dma_config(enum dma_ch id, int xferunit); /* s3c2410_dma_devconfig * * configure the device we're talking to */ -extern int s3c2410_dma_devconfig(unsigned int channel, +extern int s3c2410_dma_devconfig(enum dma_ch id, enum s3c2410_dmasrc source, unsigned long devaddr); /* s3c2410_dma_getposition @@ -118,7 +118,7 @@ extern int s3c2410_dma_devconfig(unsigned int channel, * get the position that the dma transfer is currently at */ -extern int s3c2410_dma_getposition(unsigned int channel, +extern int s3c2410_dma_getposition(enum dma_ch id, dma_addr_t *src, dma_addr_t *dest); extern int s3c2410_dma_set_opfn(unsigned int, s3c2410_dma_opfn_t rtn); -- 1.7.4.1 ___ 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 2/2] SPI: SAMSUNG: Bug fix for SPI with different FIFO level
On Fri, Jul 1, 2011 at 11:16 AM, padma venkat padma@gmail.com wrote: Hi Tony, On Thu, Jun 30, 2011 at 4:30 PM, Tony Nadackal ton...@gmail.com wrote: Hi Padma, With regards to your patch, even though one can check the tx done status using the TX_DONE bit, the present macro itself would work perfectly fine if the 'fifo_lvl_mask' is set properly. For example in 6450 channel 1, the fifo_lvl_mask should be 0x1ff (for 9bits, 15:23), while even in your patch, it is wrongly set as 0x7f(only 7bits). Thus, if this fifo_lvl_mask was defined correctly, the existing macro would itself have worked. Thanks for your comment. I considered changing to the fifo_lvl_mask to 1ff as you mentioned. But I think that the fifo_lvl_mask reflects the actual FIFO capacity in the SPI driver. For the failing channels the FIFO trigger level is 64 bytes and so i retained that value. In the driver it polls till the FIFO capacity level otherwise it goes for DMA.So if we keep the FIFO level as 1ff when the actual capacity is 7f then it fails. Jassi what do you think about this? 'fifo_lvl_mask' is h/w specific and can't be set for convenience. I don't have access to post-s3c64xx datasheets. Please check and reply if TX_DONE bit is at same offset for all channels of an SoC, because I suspect it's otherwise. -- 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] ASoC: SAMSUNG: Modify I2S driver to support idma
On Wed, Jun 15, 2011 at 2:13 PM, Sangbeom Kim sbki...@samsung.com wrote:
+#define AHB_INTENLVL0 (1 24)
+#define AHB_LVL0INT (1 20)
+#define AHB_CLRLVL0INT (1 16)
+#define AHB_DMARLD (1 5)
+#define AHB_INTMASK (1 3)
+#define AHB_DMAEN (1 0)
+#define AHB_LVLINTMASK (0xf 20)
+
+#define I2SSIZE_TRNMSK (0x)
+#define I2SSIZE_SHIFT (16)
Not serious but
a) Let us please get over our infatuation with such parentheses :)
b) Ideally these _new_ definitions should have been added separate to
this moving.
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 992a732..2fc2428 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -9,7 +9,7 @@
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*/
-
+#define DEBUG
Usually we don't do it by default.
+#define ST_RUNNING (10)
+#define ST_OPENED (11)
why do we need these two defines ?
@@ -646,6 +548,7 @@ static int i2s_hw_params(struct snd_pcm_substream
*substream,
{
struct i2s_dai *i2s = to_info(dai);
u32 mod = readl(i2s-addr + I2SMOD);
+ u32 ahb = readl(i2s-addr + I2SAHB);
Please realize that this function is common for even s3c24xx, for
which this is invalid AHB read
and all other subsequent ops.
@@ -702,6 +605,13 @@ static int i2s_hw_params(struct snd_pcm_substream
*substream,
params_format(params));
return -EINVAL;
}
+
+ if (is_secondary(i2s)) {
+ ahb |= (AHB_DMARLD | AHB_INTMASK);
+ mod |= MOD_TXS_IDMA;
+ }
+
+ writel(ahb, i2s-addr + I2SAHB);
... this too.
--
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 4/5] ARM: EXYNOS4: Add SPDIF for SMDKV310
On Fri, Jun 10, 2011 at 12:04 PM, Naveen Krishna Chatradhi
ch.nav...@samsung.com wrote:
This patch adds spdif to the machine supported device list for
SMDKV310.
Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
---
arch/arm/mach-exynos4/mach-smdkv310.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-exynos4/mach-smdkv310.c
b/arch/arm/mach-exynos4/mach-smdkv310.c
index 7e3ee6c..966c6f2 100644
--- a/arch/arm/mach-exynos4/mach-smdkv310.c
+++ b/arch/arm/mach-exynos4/mach-smdkv310.c
@@ -190,6 +190,7 @@ static struct platform_device *smdkv310_devices[]
__initdata = {
exynos4_device_pd[PD_CAM],
exynos4_device_pd[PD_TV],
exynos4_device_pd[PD_GPS],
+ exynos4_device_spdif,
exynos4_device_sysmmu,
exynos4_device_pcm0,
samsung_asoc_dma,
Acked-by: Jassi Brar jassisinghb...@gmail.com
--
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 5/5] ASoC: SMDKV310: Enable SPDIF device
On Fri, Jun 10, 2011 at 12:04 PM, Naveen Krishna Chatradhi ch.nav...@samsung.com wrote: Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com --- sound/soc/samsung/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig index d155cbb..92236eb 100644 --- a/sound/soc/samsung/Kconfig +++ b/sound/soc/samsung/Kconfig @@ -158,7 +158,7 @@ config SND_SOC_GONI_AQUILA_WM8994 config SND_SOC_SAMSUNG_SMDK_SPDIF tristate SoC S/PDIF Audio support for SMDK - depends on SND_SOC_SAMSUNG (MACH_SMDKC100 || MACH_SMDKC110 || MACH_SMDKV210) + depends on SND_SOC_SAMSUNG (MACH_SMDKC100 || MACH_SMDKC110 || MACH_SMDKV210 || MACH_SMDKV310) select SND_SAMSUNG_SPDIF help Say Y if you want to add support for SoC S/PDIF audio on the SMDK. Acked-by: Jassi Brar jassisinghb...@gmail.com -- 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/4] ASoC: SAMSUNG: Change platform driver for SMDKs
On Thu, Jun 9, 2011 at 1:39 PM, Sangbeom Kim sbki...@samsung.com wrote: Change platform driver to support internal dma for smdk_wm8994 and smdk_wm8580 Signed-off-by: Sangbeom Kim sbki...@samsung.com --- NAK. It will break the most common user - SMDK6410, as s3c6410 doesn't have any I2S internal DMA. Please try to test the changes before submitting. I know it's boring, but it's even more important :) Thanks -j -- 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 3/4] ASoC: SAMSUNG: Change platform driver for SMDKs
On Fri, Jun 10, 2011 at 12:46 PM, Sangbeom Kim sbki...@samsung.com wrote: On Thu, Jun 9, 2011 at 4:04 PM, Jassi Brar wrote: NAK. It will break the most common user - SMDK6410, as s3c6410 doesn't have any I2S internal DMA. Please try to test the changes before submitting. I know it's boring, but it's even more important :) Thanks I only focused on S5PV210 and S5PV310. I didn't consider s3c and s5p series. In the next version, I will delete it. Though if then everything works fine with system and internal DMA, maybe it is better to assign platform_name string at runtime from some module parameter, and leaving the default to normal system DMA ? But I had found some problem and want to share. Like this patch, If different platform driver is used for each channel, 1st channel is overwritten by platform driver of 2nd channel. Have you ever experienced before like this problem? IIRC, I successfully tested using system DMA with primary and iDMA with secondary channel. Only the sec chan would keep playing when the system went to suspend. Though tt was long time ago and I don't remember which Samsung kernel version. -- 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/4] ASoC: SAMSUNG: Add I2S0 internal dma driver
On Thu, Jun 9, 2011 at 1:39 PM, Sangbeom Kim sbki...@samsung.com wrote:
I2S in Exynos4 and S5PC110(S5PV210) has a internal dma.
It can be used low power audio mode and 2nd channel transfer.
For my convenience, could you please tell how does it differ from my
original implementation? Most things look same, except for a few
variables.
@@ -16,6 +17,7 @@ obj-$(CONFIG_SND_S3C_I2SV2_SOC) += snd-soc-s3c-i2s-v2.o
obj-$(CONFIG_SND_SAMSUNG_SPDIF) += snd-soc-samsung-spdif.o
obj-$(CONFIG_SND_SAMSUNG_PCM) += snd-soc-pcm.o
obj-$(CONFIG_SND_SAMSUNG_I2S) += snd-soc-i2s.o
+obj-$(CONFIG_SND_SAMSUNG_I2S) += snd-soc-idma.o
Please check that building only for s3c64xx doesn't break by this.
+
+static struct idma_info {
+ spinlock_t lock;
+ void __iomem *regs;
+ int trigger_stat;
The role of trigger_stat is not necessary.
+
+ /* Start address0 of I2S internal DMA operation. */
+ val = readl(idma.regs + I2SSTR0);
Why read when you immediately overwrite it ?
+ val = LP_TXBUFF_ADDR;
+ writel(val, idma.regs + I2SSTR0);
+
+static int idma_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ struct snd_pcm_runtime *runtime = substream-runtime;
+ struct idma_ctrl *prtd = substream-runtime-private_data;
+
+ pr_debug(Entered %s\n, __func__);
can we have all the pr_debug's converted to dev_debug ?
+ snd_pcm_set_runtime_buffer(substream, substream-dma_buffer);
+ runtime-dma_bytes = params_buffer_bytes(params);
+ memset(runtime-dma_area, 0, runtime-dma_bytes);
Is it really needed ?
+
+ if (iisahb AHB_LVL0INT)
+ val = AHB_CLRLVL0INT;
+ else
+ val = 0;
val = (iisahb AHB_LVL0INT) ? AHB_CLRLVL0INT : 0; looks better
+
+ if (val) {
+ iisahb |= val;
+ writel(iisahb, idma.regs + I2SAHB);
+
+ addr = readl(idma.regs + I2SLVL0ADDR);
+ addr += prtd-periodsz;
+
+ if (addr = prtd-end)
+ addr = LP_TXBUFF_ADDR;
This will break if ring buffer is not a multiple of period size.
Either set the constraint in open or do modulo operation here.
+
+static int idma_close(struct snd_pcm_substream *substream)
+{
+ struct snd_pcm_runtime *runtime = substream-runtime;
+ struct idma_ctrl *prtd = runtime-private_data;
+
+ pr_debug(Entered %s, prtd = %p\n, __func__, prtd);
+
+ free_irq(IRQ_I2S0, prtd);
+
+ if (!prtd)
+ pr_err(idma_close called with prtd == NULL\n);
+
+ kfree(prtd);
Also runtime-private_data = NULL;
diff --git a/sound/soc/samsung/idma.h b/sound/soc/samsung/idma.h
new file mode 100644
index 000..2b0ac37
--- /dev/null
+++ b/sound/soc/samsung/idma.h
@@ -0,0 +1,28 @@
+/*
+ * idma.h -- I2S0's Internal Dma driver
+ *
+ * Copyright (c) 2010 Samsung Electronics Co. Ltd
Copyright 2011 ?
+/* idma_state */
+#define LPAM_DMA_STOP 0
+#define LPAM_DMA_START 1
These are internal to idma.c, please move them there.
--
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 3/4] ASoC: SAMSUNG: Change platform driver for SMDKs
On Fri, Jun 10, 2011 at 3:19 PM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Fri, Jun 10, 2011 at 12:56:45PM +0530, Jassi Brar wrote: On Fri, Jun 10, 2011 at 12:46 PM, Sangbeom Kim sbki...@samsung.com wrote: I only focused on S5PV210 and S5PV310. I didn't consider s3c and s5p series. In the next version, I will delete it. Though if then everything works fine with system and internal DMA, maybe it is better to assign platform_name string at runtime from some module parameter, and leaving the default to normal system DMA ? If iDMA is always a better choice on CPUs that support it then the cpu_is_() macros might be a good way to make the selection? Actually I remembered just after I posted - iDMA wouldn't support capture. So iDMA is a bad choice after all. -- 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] ASoC: SAMSUNG: Modify I2S driver to support idma
On Thu, Jun 9, 2011 at 1:39 PM, Sangbeom Kim sbki...@samsung.com wrote: case 2: + if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) + i2s-dma_playback.dma_size = 4; + else + i2s-dma_capture.dma_size = 4; + break; Why do we need this ? + case 1: + if (substream-stream == SNDRV_PCM_STREAM_PLAYBACK) + i2s-dma_playback.dma_size = 2; + else + i2s-dma_capture.dma_size = 2; I2S doesn't support Mono. #define SAMSUNG_I2S_RCLKSRC_0 0 #define SAMSUNG_I2S_RCLKSRC_1 1 -#define SAMSUNG_I2S_CDCLK 2 +#define SAMSUNG_I2S_CDCLK 2 Please avoid inconsequential changes. + +#define I2SCON 0x0 +#define I2SMOD 0x4 +#define I2SFIC 0x8 +#define I2SPSR 0xc +#define I2STXD 0x10 +#define I2SRXD 0x14 +#define I2SFICS 0x18 +#define I2STXDS 0x1c +#define I2SAHB 0x20 +#define I2SSTR0 0x24 +#define I2SSIZE 0x28 +#define I2STRNCNT 0x2c +#define I2SLVL0ADDR 0x30 +#define I2SLVL1ADDR 0x34 +#define I2SLVL2ADDR 0x38 +#define I2SLVL3ADDR 0x3c + +#define CON_RSTCLR (1 31) +#define CON_FRXOFSTATUS (1 26) +#define CON_FRXORINTEN (1 25) +#define CON_FTXSURSTAT (1 24) +#define CON_FTXSURINTEN (1 23) +#define CON_TXSDMA_PAUSE (1 20) +#define CON_TXSDMA_ACTIVE (1 18) + +#define CON_FTXURSTATUS (1 17) +#define CON_FTXURINTEN (1 16) +#define CON_TXFIFO2_EMPTY (1 15) +#define CON_TXFIFO1_EMPTY (1 14) +#define CON_TXFIFO2_FULL (1 13) +#define CON_TXFIFO1_FULL (1 12) + +#define CON_LRINDEX (1 11) +#define CON_TXFIFO_EMPTY (1 10) +#define CON_RXFIFO_EMPTY (1 9) +#define CON_TXFIFO_FULL (1 8) +#define CON_RXFIFO_FULL (1 7) +#define CON_TXDMA_PAUSE (1 6) +#define CON_RXDMA_PAUSE (1 5) +#define CON_TXCH_PAUSE (1 4) +#define CON_RXCH_PAUSE (1 3) +#define CON_TXDMA_ACTIVE (1 2) +#define CON_RXDMA_ACTIVE (1 1) +#define CON_ACTIVE (1 0) + +#define MOD_OPCLK_CDCLK_OUT (0 30) +#define MOD_OPCLK_CDCLK_IN (1 30) +#define MOD_OPCLK_BCLK_OUT (2 30) +#define MOD_OPCLK_PCLK (3 30) +#define MOD_OPCLK_MASK (3 30) +#define MOD_TXS_IDMA (1 28) + +#define MOD_BLCS_SHIFT 26 +#define MOD_BLCS_16BIT (0 MOD_BLCS_SHIFT) +#define MOD_BLCS_8BIT (1 MOD_BLCS_SHIFT) +#define MOD_BLCS_24BIT (2 MOD_BLCS_SHIFT) +#define MOD_BLCS_MASK (3 MOD_BLCS_SHIFT) +#define MOD_BLCP_SHIFT 24 +#define MOD_BLCP_16BIT (0 MOD_BLCP_SHIFT) +#define MOD_BLCP_8BIT (1 MOD_BLCP_SHIFT) +#define MOD_BLCP_24BIT (2 MOD_BLCP_SHIFT) +#define MOD_BLCP_MASK (3 MOD_BLCP_SHIFT) + +#define MOD_C2DD_HHALF (1 21) /* Discard Higher-half */ +#define MOD_C2DD_LHALF (1 20) /* Discard Lower-half */ +#define MOD_C1DD_HHALF (1 19) +#define MOD_C1DD_LHALF (1 18) +#define MOD_DC2_EN (1 17) +#define MOD_DC1_EN (1 16) +#define MOD_BLC_16BIT (0 13) +#define MOD_BLC_8BIT (1 13) +#define MOD_BLC_24BIT (2 13) +#define MOD_BLC_MASK (3 13) + +#define MOD_IMS_SYSMUX (1 10) +#define MOD_SLAVE (1 11) +#define MOD_TXONLY (0 8) +#define MOD_RXONLY (1 8) +#define MOD_TXRX (2 8) +#define MOD_MASK (3 8) +#define MOD_LR_LLOW (0 7) +#define MOD_LR_RLOW (1 7) +#define MOD_SDF_IIS (0 5) +#define MOD_SDF_MSB (1 5) +#define MOD_SDF_LSB (2 5) +#define MOD_SDF_MASK (3 5) +#define MOD_RCLK_256FS (0 3) +#define MOD_RCLK_512FS (1 3) +#define MOD_RCLK_384FS (2 3) +#define MOD_RCLK_768FS (3 3) +#define MOD_RCLK_MASK (3 3) +#define MOD_BCLK_32FS (0 1) +#define MOD_BCLK_48FS (1 1) +#define MOD_BCLK_16FS (2 1) +#define MOD_BCLK_24FS (3 1) +#define MOD_BCLK_MASK (3 1) +#define MOD_8BIT (1 0) + +#define MOD_CDCLKCON (1 12) + +#define PSR_PSREN (1 15) + +#define FIC_TX2COUNT(x) (((x) 24) 0xf) +#define FIC_TX1COUNT(x) (((x) 16) 0xf) + +#define FIC_TXFLUSH (1 15) +#define FIC_RXFLUSH (1 7) + +#define FIC_TXCOUNT(x) (((x) 8) 0x7f) +#define
Re: [PATCH] ARM:SAMSUNG: Move S3C DMA driver to drivers/dma
On Tue, Jun 7, 2011 at 1:18 PM, root alim.akh...@samsung.com wrote: Signed-off-by: alim.akhtar alim.akh...@samsung.com --- arch/arm/configs/exynos4_defconfig | 1 + arch/arm/configs/s5p64x0_defconfig | 1 + arch/arm/configs/s5pc100_defconfig | 1 + arch/arm/configs/s5pv210_defconfig | 1 + arch/arm/plat-samsung/Kconfig | 6 - arch/arm/plat-samsung/Makefile | 2 - arch/arm/plat-samsung/s3c-pl330.c | 1244 drivers/dma/Kconfig | 8 + drivers/dma/Makefile | 2 + drivers/dma/s3c-pl330.c | 1244 10 files changed, 1258 insertions(+), 1252 deletions(-) delete mode 100644 arch/arm/plat-samsung/s3c-pl330.c create mode 100644 drivers/dma/s3c-pl330.c NAK The drivers/dma/ is place for generic DMA API con formant drivers. S3C-PL330.c is implementation of Samsung's DMA API. -- 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: Move S3C DMA driver to drivers/dma
On Tue, Jun 7, 2011 at 1:30 PM, Kyungmin Park kmp...@infradead.org wrote: As I know there's are PL330 DMA implementation by MODULE_AUTHOR(Jaswinder Singh jassi.b...@samsung.com); Thanks for CC'ing me, though my current contact is jaswinder.si...@linaro.org or jassisinghb...@gmail.com Perhaps I need to change the email in all locations. Doesn't it better to use the generic PL330 instead of Samsung specific PL330 implementation? Unfortunately, no. There are some features of Samsung's DMA API that the drivers have come to rely upon. Besides, I am not particularly a fan of the generic API. And IIRC, neither is Ben Dooks, the designer of the Samsung's DMA API. As I remember Jassi has a plan to use generic one? No plans, sorry. thnx -j -- 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: Move S3C DMA driver to drivers/dma
On Tue, Jun 7, 2011 at 1:45 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Jun 07, 2011 at 01:39:43PM +0530, Jassi Brar wrote: Unfortunately, no. There are some features of Samsung's DMA API that the drivers have come to rely upon. Besides, I am not particularly a fan of the generic API. And IIRC, neither is Ben Dooks, the designer of the Samsung's DMA API. We are now at the point where this is non-optional. If the generic API doesn't fit what Samsung needs, then that needs to be discussed and whatever problems there are need to be resolved. The discussion did take off a few months ago, but we didn't reach anywhere. Being able to queue request from the 'done' callback, the need of circular buffer API (possibly a free-running one too) and callbacks in irq-context, as they happen, were a few requirements for having fast peripherals with shallow fifo work without underruns. -- 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
