[alsa-devel] [PATCH 0/4] McBSP smart idle and DMA op mode updates for ASoC
This series expands the OMAP mcbsp driver to support changing it's DMA operating mode and smart idle mode from client drivers. It's primarily aimed at lowering the power consumption for OMAP ASoC drivers by providing methods to gate clocks on the mcbsp interface at runtime.
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
Tony, can I get your ack to upstream via ALSA if it's ok
Thanks
Liam
arch/arm/plat-omap/include/plat/mcbsp.h | 17 +++ arch/arm/plat-omap/mcbsp.c | 183 ++++++++++++++++++++++++++----- sound/soc/omap/omap-mcbsp.c | 22 ++++- sound/soc/omap/omap-mcbsp.h | 2 + 4 files changed, 194 insertions(+), 30 deletions(-)
This adds a method to set the MCBSP DMA OP mode.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk --- arch/arm/plat-omap/include/plat/mcbsp.h | 2 ++ arch/arm/plat-omap/mcbsp.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mcbsp.h b/arch/arm/plat-omap/include/plat/mcbsp.h index 1bd7021..f8823f4 100644 --- a/arch/arm/plat-omap/include/plat/mcbsp.h +++ b/arch/arm/plat-omap/include/plat/mcbsp.h @@ -476,6 +476,7 @@ u16 omap_mcbsp_get_max_rx_threshold(unsigned int id); u16 omap_mcbsp_get_tx_delay(unsigned int id); u16 omap_mcbsp_get_rx_delay(unsigned int id); int omap_mcbsp_get_dma_op_mode(unsigned int id); +int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode); #else static inline void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold) { } @@ -486,6 +487,7 @@ static inline u16 omap_mcbsp_get_max_rx_threshold(unsigned int id) { return 0; } static inline u16 omap_mcbsp_get_tx_delay(unsigned int id) { return 0; } static inline u16 omap_mcbsp_get_rx_delay(unsigned int id) { return 0; } static inline int omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; } +static inline int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode) {return 0;} #endif int omap_mcbsp_request(unsigned int id); void omap_mcbsp_free(unsigned int id); diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index 4820cab..cc2b73c 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -635,6 +635,37 @@ int omap_mcbsp_get_dma_op_mode(unsigned int id) } EXPORT_SYMBOL(omap_mcbsp_get_dma_op_mode);
+/* + * omap_mcbsp_set_dma_op_mode set the current DMA + * operating mode for the mcbsp channel + */ +int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode) +{ + struct omap_mcbsp *mcbsp; + int ret = 0; + + if (mode > MCBSP_DMA_MODE_FRAME) + return -EINVAL; + + if (!omap_mcbsp_check_valid_id(id)) { + printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); + return -ENODEV; + } + mcbsp = id_to_mcbsp_ptr(id); + + spin_lock_irq(&mcbsp->lock); + if (!mcbsp->free) { + ret = -EBUSY; + goto unlock; + } + mcbsp->dma_op_mode = mode; + +unlock: + spin_unlock_irq(&mcbsp->lock); + return ret; +} +EXPORT_SYMBOL(omap_mcbsp_set_dma_op_mode); + static inline void omap34xx_mcbsp_request(struct omap_mcbsp *mcbsp) { /*
On Tue, 18 May 2010 21:13:11 +0100 Liam Girdwood lrg@slimlogic.co.uk wrote:
This adds a method to set the MCBSP DMA OP mode.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk
arch/arm/plat-omap/include/plat/mcbsp.h | 2 ++ arch/arm/plat-omap/mcbsp.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mcbsp.h b/arch/arm/plat-omap/include/plat/mcbsp.h index 1bd7021..f8823f4 100644 --- a/arch/arm/plat-omap/include/plat/mcbsp.h +++ b/arch/arm/plat-omap/include/plat/mcbsp.h @@ -476,6 +476,7 @@ u16 omap_mcbsp_get_max_rx_threshold(unsigned int id); u16 omap_mcbsp_get_tx_delay(unsigned int id); u16 omap_mcbsp_get_rx_delay(unsigned int id); int omap_mcbsp_get_dma_op_mode(unsigned int id); +int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode);
This should have been here even in private form when the get function was introduced. Let's wait for Peter's comment about op mode sysfs functionality will we go with patch 4/4 or convert it to use this function.
But anyway this patch is good to have.
Acked-by: Jarkko Nikula jhnikula@gmail.com
On Tue, May 18, 2010 at 10:13:11PM +0200, Liam Girdwood wrote:
This adds a method to set the MCBSP DMA OP mode.
OK. good. But again, why do you need set dma op mode other than inside mcbsp code?
Right, so, from what I have read briefly, the idea is to remove the user space ability to set the mode and let mcbsp clients to do it. What is not clear to me is how mcbsp clients (inside kernel) will determine if they want short delay or pm friendly. That usually is bound to upper use case though.
It would be really nice if you improve your patch descriptions.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk
arch/arm/plat-omap/include/plat/mcbsp.h | 2 ++ arch/arm/plat-omap/mcbsp.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mcbsp.h b/arch/arm/plat-omap/include/plat/mcbsp.h index 1bd7021..f8823f4 100644 --- a/arch/arm/plat-omap/include/plat/mcbsp.h +++ b/arch/arm/plat-omap/include/plat/mcbsp.h @@ -476,6 +476,7 @@ u16 omap_mcbsp_get_max_rx_threshold(unsigned int id); u16 omap_mcbsp_get_tx_delay(unsigned int id); u16 omap_mcbsp_get_rx_delay(unsigned int id); int omap_mcbsp_get_dma_op_mode(unsigned int id); +int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode); #else static inline void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold) { } @@ -486,6 +487,7 @@ static inline u16 omap_mcbsp_get_max_rx_threshold(unsigned int id) { return 0; } static inline u16 omap_mcbsp_get_tx_delay(unsigned int id) { return 0; } static inline u16 omap_mcbsp_get_rx_delay(unsigned int id) { return 0; } static inline int omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; } +static inline int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode) {return 0;} #endif int omap_mcbsp_request(unsigned int id); void omap_mcbsp_free(unsigned int id); diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index 4820cab..cc2b73c 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -635,6 +635,37 @@ int omap_mcbsp_get_dma_op_mode(unsigned int id) } EXPORT_SYMBOL(omap_mcbsp_get_dma_op_mode);
+/*
- omap_mcbsp_set_dma_op_mode set the current DMA
- operating mode for the mcbsp channel
- */
+int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode) +{
- struct omap_mcbsp *mcbsp;
- int ret = 0;
- if (mode > MCBSP_DMA_MODE_FRAME)
return -EINVAL;- if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);return -ENODEV;- }
- mcbsp = id_to_mcbsp_ptr(id);
- spin_lock_irq(&mcbsp->lock);
- if (!mcbsp->free) {
ret = -EBUSY;goto unlock;- }
- mcbsp->dma_op_mode = mode;
+unlock:
- spin_unlock_irq(&mcbsp->lock);
- return ret;
+} +EXPORT_SYMBOL(omap_mcbsp_set_dma_op_mode);
static inline void omap34xx_mcbsp_request(struct omap_mcbsp *mcbsp) { /* -- 1.7.0.4
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-05-19 at 13:50 +0300, Eduardo Valentin wrote:
On Tue, May 18, 2010 at 10:13:11PM +0200, Liam Girdwood wrote:
This adds a method to set the MCBSP DMA OP mode.
OK. good. But again, why do you need set dma op mode other than inside mcbsp code?
I think Peter has answered this.
Right, so, from what I have read briefly, the idea is to remove the user space ability to set the mode and let mcbsp clients to do it. What is not clear to me is how mcbsp clients (inside kernel) will determine if they want short delay or pm friendly. That usually is bound to upper use case though.
An example would be an ASoC client. The ASoC driver would be configured either for low latency, low power, etc. by userspace (using formal ALSA calls) and would be able to configure the mcbsp to match this request.
It would be really nice if you improve your patch descriptions.
Sorry, I thought again this was obvious.
Liam
Add a small API to configure McBSP smart idle modes to conserve power.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk --- arch/arm/plat-omap/include/plat/mcbsp.h | 15 ++++ arch/arm/plat-omap/mcbsp.c | 122 +++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mcbsp.h b/arch/arm/plat-omap/include/plat/mcbsp.h index f8823f4..3f9fb71 100644 --- a/arch/arm/plat-omap/include/plat/mcbsp.h +++ b/arch/arm/plat-omap/include/plat/mcbsp.h @@ -278,6 +278,15 @@ #define ENAWAKEUP 0x0004 #define SOFTRST 0x0002
+#define MCBSP_CLK_ACT_IOFF_POFF 0 +#define MCBSP_CLK_ACT_ION_POFF 1 +#define MCBSP_CLK_ACT_IOFF_PON 2 +#define MCBSP_CLK_ACT_ION_PON 3 + +#define MCBSP_IDLE_FORCE 0 +#define MCBSP_IDLE_NONE 1 +#define MCBSP_IDLE_SMART 2 + /********************** McBSP SSELCR bit definitions ***********************/ #define SIDETONEEN 0x0400
@@ -456,6 +465,7 @@ struct omap_mcbsp { #ifdef CONFIG_ARCH_OMAP3 struct omap_mcbsp_st_data *st_data; int dma_op_mode; + int idle_mode; u16 max_tx_thres; u16 max_rx_thres; #endif @@ -477,6 +487,11 @@ u16 omap_mcbsp_get_tx_delay(unsigned int id); u16 omap_mcbsp_get_rx_delay(unsigned int id); int omap_mcbsp_get_dma_op_mode(unsigned int id); int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode); +int omap_mcbsp_set_idle_smart(unsigned int id, unsigned int clk_activity, + unsigned int wake); +int omap_mcbsp_set_idle_none(unsigned int id); +int omap_mcbsp_set_idle_force(unsigned int id); +int omap_mcbsp_get_idle_mode(unsigned int id); #else static inline void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold) { } diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index cc2b73c..7785050 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -1743,6 +1743,128 @@ static inline void __devexit omap34xx_device_exit(struct omap_mcbsp *mcbsp) omap_st_remove(mcbsp); } } + +/* assert standby requests when idle */ +int omap_mcbsp_set_idle_smart(unsigned int id, unsigned int clk_activity, + u32 wakeup) +{ + struct omap_mcbsp *mcbsp; + u16 syscon; + int ret = 0; + + if (!omap_mcbsp_check_valid_id(id)) { + printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1); + return -ENODEV; + } + mcbsp = id_to_mcbsp_ptr(id); + + spin_lock_irq(&mcbsp->lock); + if (!mcbsp->free) { + ret = -EBUSY; + goto unlock; + } + + syscon = MCBSP_READ(mcbsp, SYSCON) & + ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); + MCBSP_WRITE(mcbsp, WAKEUPEN, wakeup); + MCBSP_WRITE(mcbsp, SYSCON, + syscon | SIDLEMODE(MCBSP_IDLE_SMART) | + CLOCKACTIVITY(clk_activity) | ENAWAKEUP); + mcbsp->idle_mode = MCBSP_IDLE_SMART; + +unlock: + spin_unlock_irq(&mcbsp->lock); + return ret; +} +EXPORT_SYMBOL(omap_mcbsp_set_idle_smart); + +/* never assert standby requests */ +int omap_mcbsp_set_idle_none(unsigned int id) +{ + struct omap_mcbsp *mcbsp; + u16 syscon; + int ret = 0; + + if (!omap_mcbsp_check_valid_id(id)) { + printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1); + return -ENODEV; + } + mcbsp = id_to_mcbsp_ptr(id); + + spin_lock_irq(&mcbsp->lock); + if (!mcbsp->free) { + ret = -EBUSY; + goto unlock; + } + + syscon = MCBSP_READ(mcbsp, SYSCON) & + ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); + + MCBSP_WRITE(mcbsp, SYSCON, syscon | SIDLEMODE(MCBSP_IDLE_NONE)); + MCBSP_WRITE(mcbsp, WAKEUPEN, 0); + mcbsp->idle_mode = MCBSP_IDLE_NONE; + +unlock: + spin_unlock_irq(&mcbsp->lock); + return ret; +} +EXPORT_SYMBOL(omap_mcbsp_set_idle_none); + +/* unconditionally assert standby requests */ +int omap_mcbsp_set_idle_force(unsigned int id) +{ + struct omap_mcbsp *mcbsp; + u16 syscon; + int ret = 0; + + if (!omap_mcbsp_check_valid_id(id)) { + printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1); + return -ENODEV; + } + mcbsp = id_to_mcbsp_ptr(id); + + spin_lock_irq(&mcbsp->lock); + if (!mcbsp->free) { + ret = -EBUSY; + goto unlock; + } + + syscon = MCBSP_READ(mcbsp, SYSCON) & + ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); + /* + * HW bug workaround - If no_idle mode is taken, we need to + * go to smart_idle before going to always_idle, or the + * device will not hit retention anymore. + */ + syscon |= SIDLEMODE(MCBSP_IDLE_SMART); + MCBSP_WRITE(mcbsp, SYSCON, syscon); + syscon &= ~(SIDLEMODE(0x03)); + + MCBSP_WRITE(mcbsp, SYSCON, syscon | SIDLEMODE(MCBSP_IDLE_FORCE)); + MCBSP_WRITE(mcbsp, WAKEUPEN, 0); + mcbsp->idle_mode = MCBSP_IDLE_FORCE; + +unlock: + spin_unlock_irq(&mcbsp->lock); + return ret; +} +EXPORT_SYMBOL(omap_mcbsp_set_idle_force); + +int omap_mcbsp_get_idle_mode(unsigned int id) +{ + struct omap_mcbsp *mcbsp; + + if (!omap_mcbsp_check_valid_id(id)) { + printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1); + return -ENODEV; + } + mcbsp = id_to_mcbsp_ptr(id); + + return mcbsp->idle_mode; +} +EXPORT_SYMBOL(omap_mcbsp_get_idle_mode); + + #else static inline void __devinit omap34xx_device_init(struct omap_mcbsp *mcbsp) {} static inline void __devexit omap34xx_device_exit(struct omap_mcbsp *mcbsp) {}
Liam Girdwood lrg@slimlogic.co.uk writes:
Add a small API to configure McBSP smart idle modes to conserve power.
Would be useful here to explain why client drivers need to change idle modes.
Also, this patch is telling me that it's time the McBSP is converted over to the omap_hwmod/omap_device infrastructure where this type of functionality would already be provided.
Not necesssarily a reason to prevent this patch for now, but this will all need to be changed when converted to hwmod.
Kevin
On Tue, 2010-05-18 at 16:01 -0700, Kevin Hilman wrote:
Liam Girdwood lrg@slimlogic.co.uk writes:
Add a small API to configure McBSP smart idle modes to conserve power.
Would be useful here to explain why client drivers need to change idle modes.
I think Peter has covered this in one of his replies.
Also, this patch is telling me that it's time the McBSP is converted over to the omap_hwmod/omap_device infrastructure where this type of functionality would already be provided.
Not necesssarily a reason to prevent this patch for now, but this will all need to be changed when converted to hwmod.
Ok, I think I'll actually just convert it to hwmod rather than adding a soon to be deprecated API.
Thanks
Liam
On Wednesday 19 May 2010 13:43:28 ext Liam Girdwood wrote:
On Tue, 2010-05-18 at 16:01 -0700, Kevin Hilman wrote:
Liam Girdwood lrg@slimlogic.co.uk writes:
Add a small API to configure McBSP smart idle modes to conserve power.
Would be useful here to explain why client drivers need to change idle modes.
I think Peter has covered this in one of his replies.
Also, this patch is telling me that it's time the McBSP is converted over to the omap_hwmod/omap_device infrastructure where this type of functionality would already be provided.
Not necesssarily a reason to prevent this patch for now, but this will all need to be changed when converted to hwmod.
Ok, I think I'll actually just convert it to hwmod rather than adding a soon to be deprecated API.
Does the hwmon supported on OMAP1, 2, 3, and 4? Do we have the needed framework covering all OMAP versions?
Thanks
Liam
Liam Girdwood lrg@slimlogic.co.uk writes:
On Tue, 2010-05-18 at 16:01 -0700, Kevin Hilman wrote:
Liam Girdwood lrg@slimlogic.co.uk writes:
Add a small API to configure McBSP smart idle modes to conserve power.
Would be useful here to explain why client drivers need to change idle modes.
I think Peter has covered this in one of his replies.
Also, this patch is telling me that it's time the McBSP is converted over to the omap_hwmod/omap_device infrastructure where this type of functionality would already be provided.
Not necesssarily a reason to prevent this patch for now, but this will all need to be changed when converted to hwmod.
Ok, I think I'll actually just convert it to hwmod rather than adding a soon to be deprecated API.
Excellent, that would certainly be my preference.
For reference, you can check out the pm-wip/uart and pm-wip/mmc branches of my tree[1] for some hwmod conversions for UART and MMC that are in progress.
Kevin
[1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git
On Tue, 18 May 2010 21:13:12 +0100 Liam Girdwood lrg@slimlogic.co.uk wrote:
Add a small API to configure McBSP smart idle modes to conserve power.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk
arch/arm/plat-omap/include/plat/mcbsp.h | 15 ++++ arch/arm/plat-omap/mcbsp.c | 122 +++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 0 deletions(-)
+#define MCBSP_IDLE_FORCE 0 +#define MCBSP_IDLE_NONE 1 +#define MCBSP_IDLE_SMART 2
...
+int omap_mcbsp_set_idle_smart(unsigned int id, unsigned int clk_activity,
unsigned int wake);+int omap_mcbsp_set_idle_none(unsigned int id); +int omap_mcbsp_set_idle_force(unsigned int id); +int omap_mcbsp_get_idle_mode(unsigned int id);
Only minor comment but IMO single set function looks better. E.g.
omap_mcbsp_set_idle_mode(unsigned int id, int mode);
Hello Lian,
On Tue, May 18, 2010 at 10:13:12PM +0200, Liam Girdwood wrote:
Add a small API to configure McBSP smart idle modes to conserve power.
I'm sorry but I didn't get the point of this patch, as you didn't add any wider description why you need to export this feature. Why do you think mcbsp clients should be aware of these bit? Would it make sense to mask it behind other feature? like the threshold size for instance..
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk
arch/arm/plat-omap/include/plat/mcbsp.h | 15 ++++ arch/arm/plat-omap/mcbsp.c | 122 +++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mcbsp.h b/arch/arm/plat-omap/include/plat/mcbsp.h index f8823f4..3f9fb71 100644 --- a/arch/arm/plat-omap/include/plat/mcbsp.h +++ b/arch/arm/plat-omap/include/plat/mcbsp.h @@ -278,6 +278,15 @@ #define ENAWAKEUP 0x0004 #define SOFTRST 0x0002
+#define MCBSP_CLK_ACT_IOFF_POFF 0 +#define MCBSP_CLK_ACT_ION_POFF 1 +#define MCBSP_CLK_ACT_IOFF_PON 2 +#define MCBSP_CLK_ACT_ION_PON 3
+#define MCBSP_IDLE_FORCE 0 +#define MCBSP_IDLE_NONE 1 +#define MCBSP_IDLE_SMART 2
/********************** McBSP SSELCR bit definitions ***********************/ #define SIDETONEEN 0x0400
@@ -456,6 +465,7 @@ struct omap_mcbsp { #ifdef CONFIG_ARCH_OMAP3 struct omap_mcbsp_st_data *st_data; int dma_op_mode;
- int idle_mode; u16 max_tx_thres; u16 max_rx_thres;
#endif @@ -477,6 +487,11 @@ u16 omap_mcbsp_get_tx_delay(unsigned int id); u16 omap_mcbsp_get_rx_delay(unsigned int id); int omap_mcbsp_get_dma_op_mode(unsigned int id); int omap_mcbsp_set_dma_op_mode(unsigned int id, unsigned int mode); +int omap_mcbsp_set_idle_smart(unsigned int id, unsigned int clk_activity,
unsigned int wake);+int omap_mcbsp_set_idle_none(unsigned int id); +int omap_mcbsp_set_idle_force(unsigned int id); +int omap_mcbsp_get_idle_mode(unsigned int id); #else static inline void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold) { } diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index cc2b73c..7785050 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -1743,6 +1743,128 @@ static inline void __devexit omap34xx_device_exit(struct omap_mcbsp *mcbsp) omap_st_remove(mcbsp); } }
+/* assert standby requests when idle */ +int omap_mcbsp_set_idle_smart(unsigned int id, unsigned int clk_activity,
u32 wakeup)+{
- struct omap_mcbsp *mcbsp;
- u16 syscon;
- int ret = 0;
- if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1);return -ENODEV;- }
- mcbsp = id_to_mcbsp_ptr(id);
- spin_lock_irq(&mcbsp->lock);
- if (!mcbsp->free) {
ret = -EBUSY;goto unlock;- }
- syscon = MCBSP_READ(mcbsp, SYSCON) &
~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03));- MCBSP_WRITE(mcbsp, WAKEUPEN, wakeup);
- MCBSP_WRITE(mcbsp, SYSCON,
syscon | SIDLEMODE(MCBSP_IDLE_SMART) |CLOCKACTIVITY(clk_activity) | ENAWAKEUP);- mcbsp->idle_mode = MCBSP_IDLE_SMART;
+unlock:
- spin_unlock_irq(&mcbsp->lock);
- return ret;
+} +EXPORT_SYMBOL(omap_mcbsp_set_idle_smart);
+/* never assert standby requests */ +int omap_mcbsp_set_idle_none(unsigned int id) +{
- struct omap_mcbsp *mcbsp;
- u16 syscon;
- int ret = 0;
- if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1);return -ENODEV;- }
- mcbsp = id_to_mcbsp_ptr(id);
- spin_lock_irq(&mcbsp->lock);
- if (!mcbsp->free) {
ret = -EBUSY;goto unlock;- }
- syscon = MCBSP_READ(mcbsp, SYSCON) &
~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03));- MCBSP_WRITE(mcbsp, SYSCON, syscon | SIDLEMODE(MCBSP_IDLE_NONE));
- MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
- mcbsp->idle_mode = MCBSP_IDLE_NONE;
+unlock:
- spin_unlock_irq(&mcbsp->lock);
- return ret;
+} +EXPORT_SYMBOL(omap_mcbsp_set_idle_none);
+/* unconditionally assert standby requests */ +int omap_mcbsp_set_idle_force(unsigned int id) +{
- struct omap_mcbsp *mcbsp;
- u16 syscon;
- int ret = 0;
- if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1);return -ENODEV;- }
- mcbsp = id_to_mcbsp_ptr(id);
- spin_lock_irq(&mcbsp->lock);
- if (!mcbsp->free) {
ret = -EBUSY;goto unlock;- }
- syscon = MCBSP_READ(mcbsp, SYSCON) &
~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03));- /*
* HW bug workaround - If no_idle mode is taken, we need to* go to smart_idle before going to always_idle, or the* device will not hit retention anymore.*/- syscon |= SIDLEMODE(MCBSP_IDLE_SMART);
- MCBSP_WRITE(mcbsp, SYSCON, syscon);
- syscon &= ~(SIDLEMODE(0x03));
- MCBSP_WRITE(mcbsp, SYSCON, syscon | SIDLEMODE(MCBSP_IDLE_FORCE));
- MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
- mcbsp->idle_mode = MCBSP_IDLE_FORCE;
+unlock:
- spin_unlock_irq(&mcbsp->lock);
- return ret;
+} +EXPORT_SYMBOL(omap_mcbsp_set_idle_force);
+int omap_mcbsp_get_idle_mode(unsigned int id) +{
- struct omap_mcbsp *mcbsp;
- if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1);return -ENODEV;- }
- mcbsp = id_to_mcbsp_ptr(id);
- return mcbsp->idle_mode;
+} +EXPORT_SYMBOL(omap_mcbsp_get_idle_mode);
#else static inline void __devinit omap34xx_device_init(struct omap_mcbsp *mcbsp) {} static inline void __devexit omap34xx_device_exit(struct omap_mcbsp *mcbsp) {} -- 1.7.0.4
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-05-19 at 13:46 +0300, Eduardo Valentin wrote:
Hello Lian,
On Tue, May 18, 2010 at 10:13:12PM +0200, Liam Girdwood wrote:
Add a small API to configure McBSP smart idle modes to conserve power.
I'm sorry but I didn't get the point of this patch, as you didn't add any wider description why you need to export this feature.
Sorry, I thought this would obvious from the patch and description.
Why do you think mcbsp clients should be aware of these bit? Would it make sense to mask it behind other feature? like the threshold size for instance..
Why do you think it should be masked behind threshold ?
Liam
On Wed, May 19, 2010 at 01:21:26PM +0200, Liam Girdwood wrote:
On Wed, 2010-05-19 at 13:46 +0300, Eduardo Valentin wrote:
Hello Lian,
On Tue, May 18, 2010 at 10:13:12PM +0200, Liam Girdwood wrote:
Add a small API to configure McBSP smart idle modes to conserve power.
I'm sorry but I didn't get the point of this patch, as you didn't add any wider description why you need to export this feature.
Sorry, I thought this would obvious from the patch and description.
Why do you think mcbsp clients should be aware of these bit? Would it make sense to mask it behind other feature? like the threshold size for instance..
Why do you think it should be masked behind threshold ?
Well, that was just an example. It could be masked there because, if client doesn't set the threshold level, you could assume you don't need to set the smart idle.
Or maybe it could be bound to dma op mode. if element mode, then no need to smart idle. I guess that was the way it was, at some point.
Liam
Freelance Developer, SlimLogic Ltd ASoC and Voltage Regulator Maintainer. http://www.slimlogic.co.uk
Add a mechanism to register a machine specific callback to calculate and set the McBSP Tx/Rx threshold.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk --- sound/soc/omap/omap-mcbsp.c | 22 +++++++++++++++++++++- sound/soc/omap/omap-mcbsp.h | 2 ++ 2 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c index 6f44cb4..9a1583d 100644 --- a/sound/soc/omap/omap-mcbsp.c +++ b/sound/soc/omap/omap-mcbsp.c @@ -51,6 +51,9 @@ struct omap_mcbsp_data { unsigned int bus_id; struct omap_mcbsp_reg_cfg regs; unsigned int fmt; + + /* optional machine set_threshold() sample value */ + void (*mach_set_threshold)(struct snd_pcm_substream *substream); /* * Flags indicating is the bus already activated and configured by * another substream @@ -306,7 +309,11 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream, } else if (cpu_is_omap343x()) { dma = omap24xx_dma_reqs[bus_id][substream->stream]; port = omap34xx_mcbsp_port[bus_id][substream->stream]; - omap_mcbsp_dai_dma_params[id][substream->stream].set_threshold = + if (mcbsp_data->mach_set_threshold) + omap_mcbsp_dai_dma_params[id][substream->stream].set_threshold = + mcbsp_data->mach_set_threshold; + else + omap_mcbsp_dai_dma_params[id][substream->stream].set_threshold = omap_mcbsp_set_threshold; /* TODO: Currently, MODE_ELEMENT == MODE_FRAME */ if (omap_mcbsp_get_dma_op_mode(bus_id) == @@ -835,6 +842,19 @@ int omap_mcbsp_st_add_controls(struct snd_soc_codec *codec, int mcbsp_id) } EXPORT_SYMBOL_GPL(omap_mcbsp_st_add_controls);
+int omap_bcbsp_set_threshold_func(struct snd_soc_dai *cpu_dai, + void (*mach_set_threshold)(struct snd_pcm_substream *substream)) +{ + struct omap_mcbsp_data *mcbsp_data = to_mcbsp(cpu_dai->private_data); + + if (!cpu_is_omap34xx()) + return -ENODEV; + + mcbsp_data->mach_set_threshold = mach_set_threshold; + return 0; +} +EXPORT_SYMBOL_GPL(omap_bcbsp_set_threshold_func); + static int __init snd_omap_mcbsp_init(void) { return snd_soc_register_dais(omap_mcbsp_dai, diff --git a/sound/soc/omap/omap-mcbsp.h b/sound/soc/omap/omap-mcbsp.h index 6c363e5..f8d8044 100644 --- a/sound/soc/omap/omap-mcbsp.h +++ b/sound/soc/omap/omap-mcbsp.h @@ -58,5 +58,7 @@ enum omap_mcbsp_div { extern struct snd_soc_dai omap_mcbsp_dai[NUM_LINKS];
int omap_mcbsp_st_add_controls(struct snd_soc_codec *codec, int mcbsp_id); +int omap_bcbsp_set_threshold_func(struct snd_soc_dai *cpu_dai, + void (*mach_set_threshold)(struct snd_pcm_substream *substream));
#endif
On Tue, May 18, 2010 at 09:13:13PM +0100, Liam Girdwood wrote:
Add a mechanism to register a machine specific callback to calculate and set the McBSP Tx/Rx threshold.
It's a shame the default implementation can't work for everyone but there we are. :(
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk
All of these:
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
alsa-devel-bounces@alsa-project.org wrote:
Add a mechanism to register a machine specific callback to calculate and set the McBSP Tx/Rx threshold.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk
sound/soc/omap/omap-mcbsp.c | 22 +++++++++++++++++++++- sound/soc/omap/omap-mcbsp.h | 2 ++ 2 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c index 6f44cb4..9a1583d 100644 --- a/sound/soc/omap/omap-mcbsp.c +++ b/sound/soc/omap/omap-mcbsp.c @@ -51,6 +51,9 @@ struct omap_mcbsp_data { unsigned int bus_id; struct omap_mcbsp_reg_cfg regs; unsigned int fmt;
- /* optional machine set_threshold() sample value */
- void (*mach_set_threshold)(struct snd_pcm_substream *substream); /* * Flags indicating is the bus already activated and
configured by * another substream @@ -306,7 +309,11 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream, } else if (cpu_is_omap343x()) { dma = omap24xx_dma_reqs[bus_id][substream->stream]; port = omap34xx_mcbsp_port[bus_id][substream->stream]; - omap_mcbsp_dai_dma_params[id][substream->stream].set_threshold =
if (mcbsp_data->mach_set_threshold)omap_mcbsp_dai_dma_params[id][substream->stream].set_threshold = + mcbsp_data->mach_set_threshold;
elseomap_mcbsp_dai_dma_params[id][substream->stream].set_threshold =
omap_mcbsp_set_threshold; /* TODO: Currently, MODE_ELEMENT == MODE_FRAME */ if (omap_mcbsp_get_dma_op_mode(bus_id) == @@ -835,6 +842,19 @@ int omap_mcbsp_st_add_controls(struct snd_soc_codec *codec, int mcbsp_id) } EXPORT_SYMBOL_GPL(omap_mcbsp_st_add_controls);
+int omap_bcbsp_set_threshold_func(struct snd_soc_dai *cpu_dai,
void (*mach_set_threshold)(structsnd_pcm_substream *substream))
I think you meant omap_mcbsp_set_threshold_func here.
+{
- struct omap_mcbsp_data *mcbsp_data =
to_mcbsp(cpu_dai->private_data);
- if (!cpu_is_omap34xx())
return -ENODEV;- mcbsp_data->mach_set_threshold = mach_set_threshold; + return 0;
+} +EXPORT_SYMBOL_GPL(omap_bcbsp_set_threshold_func);
static int __init snd_omap_mcbsp_init(void) { return snd_soc_register_dais(omap_mcbsp_dai, diff --git a/sound/soc/omap/omap-mcbsp.h b/sound/soc/omap/omap-mcbsp.h index 6c363e5..f8d8044 100644 --- a/sound/soc/omap/omap-mcbsp.h +++ b/sound/soc/omap/omap-mcbsp.h @@ -58,5 +58,7 @@ enum omap_mcbsp_div { extern struct snd_soc_dai omap_mcbsp_dai[NUM_LINKS];
int omap_mcbsp_st_add_controls(struct snd_soc_codec *codec, int mcbsp_id); +int omap_bcbsp_set_threshold_func(struct snd_soc_dai *cpu_dai,
void (*mach_set_threshold)(structsnd_pcm_substream *substream));
Also here.
#endif
On Tue, 18 May 2010 15:56:46 -0500 "Candelaria Villarreal, Jorge" jorge.candelaria@ti.com wrote:
alsa-devel-bounces@alsa-project.org wrote:
Add a mechanism to register a machine specific callback to calculate and set the McBSP Tx/Rx threshold.
...
+int omap_bcbsp_set_threshold_func(struct snd_soc_dai *cpu_dai,
void (*mach_set_threshold)(structsnd_pcm_substream *substream));
Also here.
Same comments, no any other objections but couple lines in commit log would be nice have when machine would benefit from an own callback. I.e. there could be boards that benefit from this.
On Tue, 2010-05-18 at 15:56 -0500, Candelaria Villarreal, Jorge wrote:
+int omap_bcbsp_set_threshold_func(struct snd_soc_dai *cpu_dai,
void (*mach_set_threshold)(structsnd_pcm_substream *substream))
I think you meant omap_mcbsp_set_threshold_func here.
I did indeed !
Thanks
Liam
The mcbsp DMA op mode is tightly integrated with the mcbsp client driver operation and hence is unsuitable for userspace configuration via sysfs.
Signed-off-by: Liam Girdwood lrg@slimlogic.co.uk --- arch/arm/plat-omap/mcbsp.c | 30 +----------------------------- 1 files changed, 1 insertions(+), 29 deletions(-)
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index 7785050..b7b8b5e 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -1546,35 +1546,7 @@ static ssize_t dma_op_mode_show(struct device *dev, return len; }
-static ssize_t dma_op_mode_store(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t size) -{ - struct omap_mcbsp *mcbsp = dev_get_drvdata(dev); - const char * const *s; - int i = 0; - - for (s = &dma_op_modes[i]; i < ARRAY_SIZE(dma_op_modes); s++, i++) - if (sysfs_streq(buf, *s)) - break; - - if (i == ARRAY_SIZE(dma_op_modes)) - return -EINVAL; - - spin_lock_irq(&mcbsp->lock); - if (!mcbsp->free) { - size = -EBUSY; - goto unlock; - } - mcbsp->dma_op_mode = i; - -unlock: - spin_unlock_irq(&mcbsp->lock); - - return size; -} - -static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, dma_op_mode_store); +static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, NULL);
static ssize_t st_taps_show(struct device *dev, struct device_attribute *attr, char *buf)
On Tue, 18 May 2010 21:13:10 +0100 Liam Girdwood lrg@slimlogic.co.uk wrote:
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
I've used to say that the DMA op mode is more like use-case not machine specific but I'm not sure is my point valid onemore. I've used to think that low-latency processing would need accurate DMA pointer (op mode == element) while mp3 playback would need low power consumption (op mode == threshold).
Peter: what's the status today how well can we do low-latency processing with threshold mode? IRCC, with your FIFO delay query patches, can we estimate the DMA pointer position with enough accuracy?
On Wednesday 19 May 2010 08:13:25 ext Jarkko Nikula wrote:
On Tue, 18 May 2010 21:13:10 +0100
Liam Girdwood lrg@slimlogic.co.uk wrote:
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
I've used to say that the DMA op mode is more like use-case not machine specific but I'm not sure is my point valid onemore. I've used to think that low-latency processing would need accurate DMA pointer (op mode == element) while mp3 playback would need low power consumption (op mode == threshold).
Yeah, it used to be so clear ;) I see these: element mode: if user want to have constant latency [1] threshold mode: variable latency, but possibility to save power [2]
[1] The McBSP is kept full during playback (and empty during capture) The DMA pointer moves word-by-word
[2] The McBSP FIFO fill rate changes (full, drain, refill, full, drain...) The DMA pointer moves in bursts. Between burst memories can relax, core, MPU also in theory. Smart idle helps to conserve more power here
Peter: what's the status today how well can we do low-latency processing with threshold mode? IRCC, with your FIFO delay query patches, can we estimate the DMA pointer position with enough accuracy?
The DMA pointer is easy, and it was know before as well, but according to my tests, the McBSP FIFO caused delay reporting is fairly accurate. I'll ask my users, if they have done some additional tests.
On Wednesday 19 May 2010 11:31:57 Ujfalusi Peter (Nokia-D/Tampere) wrote:
On Wednesday 19 May 2010 08:13:25 ext Jarkko Nikula wrote:
On Tue, 18 May 2010 21:13:10 +0100
Liam Girdwood lrg@slimlogic.co.uk wrote:
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
I've used to say that the DMA op mode is more like use-case not machine specific but I'm not sure is my point valid onemore. I've used to think that low-latency processing would need accurate DMA pointer (op mode == element) while mp3 playback would need low power consumption (op mode == threshold).
Yeah, it used to be so clear ;) I see these: element mode: if user want to have constant latency [1] threshold mode: variable latency, but possibility to save power [2]
[1] The McBSP is kept full during playback (and empty during capture) The DMA pointer moves word-by-word
So the latency is the length of the McBSP FIFO.
[2] The McBSP FIFO fill rate changes (full, drain, refill, full, drain...) The DMA pointer moves in bursts. Between burst memories can relax, core, MPU also in theory. Smart idle helps to conserve more power here
The latency depends on when you are asking. When the FIFO is full, than you have maximum latency, right at the end of the drain phase, you will have the shortest latency.
Peter: what's the status today how well can we do low-latency processing with threshold mode? IRCC, with your FIFO delay query patches, can we estimate the DMA pointer position with enough accuracy?
The DMA pointer is easy, and it was know before as well, but according to my tests, the McBSP FIFO caused delay reporting is fairly accurate. I'll ask my users, if they have done some additional tests.
So it is not that clear how to view them ;)
On Wed, May 19, 2010 at 10:31:57AM +0200, Ujfalusi Peter (Nokia-D/Tampere) wrote:
On Wednesday 19 May 2010 08:13:25 ext Jarkko Nikula wrote:
On Tue, 18 May 2010 21:13:10 +0100
Liam Girdwood lrg@slimlogic.co.uk wrote:
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
I've used to say that the DMA op mode is more like use-case not machine specific but I'm not sure is my point valid onemore. I've used to think that low-latency processing would need accurate DMA pointer (op mode == element) while mp3 playback would need low power consumption (op mode == threshold).
Yeah, it used to be so clear ;) I see these: element mode: if user want to have constant latency [1] threshold mode: variable latency, but possibility to save power [2]
[1] The McBSP is kept full during playback (and empty during capture) The DMA pointer moves word-by-word
[2] The McBSP FIFO fill rate changes (full, drain, refill, full, drain...) The DMA pointer moves in bursts. Between burst memories can relax, core, MPU also in theory. Smart idle helps to conserve more power here
Peter: what's the status today how well can we do low-latency processing with threshold mode? IRCC, with your FIFO delay query patches, can we estimate the DMA pointer position with enough accuracy?
The DMA pointer is easy, and it was know before as well, but according to my tests, the McBSP FIFO caused delay reporting is fairly accurate. I'll ask my users, if they have done some additional tests.
OK. fair enough. What if you really want to avoid the delay at all?
-- Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 19 May 2010 13:52:27 Valentin Eduardo (Nokia-D/Helsinki) wrote:
The DMA pointer is easy, and it was know before as well, but according to my tests, the McBSP FIFO caused delay reporting is fairly accurate. I'll ask my users, if they have done some additional tests.
OK. fair enough. What if you really want to avoid the delay at all?
You can't. the McBSP FIFO can not be bypassed. The only thing one can do is to take it into consideration.
On Wed, May 19, 2010 at 01:36:51PM +0200, Ujfalusi Peter (Nokia-D/Tampere) wrote:
On Wednesday 19 May 2010 13:52:27 Valentin Eduardo (Nokia-D/Helsinki) wrote:
The DMA pointer is easy, and it was know before as well, but according to my tests, the McBSP FIFO caused delay reporting is fairly accurate. I'll ask my users, if they have done some additional tests.
OK. fair enough. What if you really want to avoid the delay at all?
You can't. the McBSP FIFO can not be bypassed. The only thing one can do is to take it into consideration.
hmmm well, you can still do not use the fifo completely. Use only partially depending on latency requirement would still be possible.
-- Péter
On Wednesday 19 May 2010 14:46:40 Valentin Eduardo (Nokia-D/Helsinki) wrote:
On Wed, May 19, 2010 at 01:36:51PM +0200, Ujfalusi Peter (Nokia-D/Tampere)
wrote:
On Wednesday 19 May 2010 13:52:27 Valentin Eduardo (Nokia-D/Helsinki) wrote:
The DMA pointer is easy, and it was know before as well, but according to my tests, the McBSP FIFO caused delay reporting is fairly accurate. I'll ask my users, if they have done some additional tests.
OK. fair enough. What if you really want to avoid the delay at all?
You can't. the McBSP FIFO can not be bypassed. The only thing one can do is to take it into consideration.
hmmm well, you can still do not use the fifo completely. Use only partially depending on latency requirement would still be possible.
Ah, you mean the idea, where in DMA completion interrupt we configure the McBSP threshold to maximum, and on McBSP FIFO empty interrupt, we configurre back the threshold to what the level we want to use from McBSP, and start the DMA? This all has to be done 'manually', and if there is _any_ latency in the kernel, than we have channel switch, glitch, pause, and whatnot.. I remember, we tried that, and well, did not worked that reliable.
On Tue, May 18, 2010 at 10:13:10PM +0200, Liam Girdwood wrote:
This series expands the OMAP mcbsp driver to support changing it's DMA operating mode and smart idle mode from client drivers. It's primarily aimed at lowering the power consumption for OMAP ASoC drivers by providing methods to gate clocks on the mcbsp interface at runtime.
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
Yeah, I'm not sure if that would be strong enough argument though. Unfortunately, this dma op mode also couples with the usage of mcbsp internal buffer. Which in the very end will mean a compromise between delay vs. pm, as you are probably aware.
So, letting userspace to toggle this mode will also mean they can choose between pm friendly (op mode with frame and so on) or short delay friendly (element mode).
Of course, this might not be the best way of choosing it though.
Tony, can I get your ack to upstream via ALSA if it's ok
Thanks
Liam
arch/arm/plat-omap/include/plat/mcbsp.h | 17 +++ arch/arm/plat-omap/mcbsp.c | 183 ++++++++++++++++++++++++++----- sound/soc/omap/omap-mcbsp.c | 22 ++++- sound/soc/omap/omap-mcbsp.h | 2 + 4 files changed, 194 insertions(+), 30 deletions(-)
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2010-05-19 at 13:42 +0300, Eduardo Valentin wrote:
On Tue, May 18, 2010 at 10:13:10PM +0200, Liam Girdwood wrote:
This series expands the OMAP mcbsp driver to support changing it's DMA operating mode and smart idle mode from client drivers. It's primarily aimed at lowering the power consumption for OMAP ASoC drivers by providing methods to gate clocks on the mcbsp interface at runtime.
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
Yeah, I'm not sure if that would be strong enough argument though. Unfortunately, this dma op mode also couples with the usage of mcbsp internal buffer. Which in the very end will mean a compromise between delay vs. pm, as you are probably aware.
So, letting userspace to toggle this mode will also mean they can choose between pm friendly (op mode with frame and so on) or short delay friendly (element mode).
Of course, this might not be the best way of choosing it though.
The sysfs set interface implies userspace having knowledge of driver capabilities and configuration in order to safely toggle between the two DMA modes. Imo, the mcbsp client driver should be the only entity configuring it's DMA modes (in a safe manner) depending on the use case.
Liam
On Wed, May 19, 2010 at 01:07:11PM +0200, Liam Girdwood wrote:
On Wed, 2010-05-19 at 13:42 +0300, Eduardo Valentin wrote:
On Tue, May 18, 2010 at 10:13:10PM +0200, Liam Girdwood wrote:
This series expands the OMAP mcbsp driver to support changing it's DMA operating mode and smart idle mode from client drivers. It's primarily aimed at lowering the power consumption for OMAP ASoC drivers by providing methods to gate clocks on the mcbsp interface at runtime.
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
Yeah, I'm not sure if that would be strong enough argument though. Unfortunately, this dma op mode also couples with the usage of mcbsp internal buffer. Which in the very end will mean a compromise between delay vs. pm, as you are probably aware.
So, letting userspace to toggle this mode will also mean they can choose between pm friendly (op mode with frame and so on) or short delay friendly (element mode).
Of course, this might not be the best way of choosing it though.
The sysfs set interface implies userspace having knowledge of driver capabilities and configuration in order to safely toggle between the two DMA modes. Imo, the mcbsp client driver should be the only entity configuring it's DMA modes (in a safe manner) depending on the use case.
OK. That is fair enough. But it isn't clear in your patch series/description, how the client will determine which use case to use. I understand your point and I agree with it. However, the way it's being done here, it does not explain how the same issue will be handled after the removal of this interface. That's my main concern.
Liam
Freelance Developer, SlimLogic Ltd ASoC and Voltage Regulator Maintainer. http://www.slimlogic.co.uk
On Wednesday 19 May 2010 14:07:11 ext Liam Girdwood wrote:
On Wed, 2010-05-19 at 13:42 +0300, Eduardo Valentin wrote:
On Tue, May 18, 2010 at 10:13:10PM +0200, Liam Girdwood wrote:
This series expands the OMAP mcbsp driver to support changing it's DMA operating mode and smart idle mode from client drivers. It's primarily aimed at lowering the power consumption for OMAP ASoC drivers by providing methods to gate clocks on the mcbsp interface at runtime.
I've also added a patch to remove the mcbsp DMA op mode sysfs set functionality. I think DMA op mode is very specific to the mcbsp client driver _only_ and shouldn't really be changed by userspace. Please let me know if you use this feature and I'll drop this patch.
Yeah, I'm not sure if that would be strong enough argument though. Unfortunately, this dma op mode also couples with the usage of mcbsp internal buffer. Which in the very end will mean a compromise between delay vs. pm, as you are probably aware.
So, letting userspace to toggle this mode will also mean they can choose between pm friendly (op mode with frame and so on) or short delay friendly (element mode).
Of course, this might not be the best way of choosing it though.
The sysfs set interface implies userspace having knowledge of driver capabilities and configuration in order to safely toggle between the two DMA modes. Imo, the mcbsp client driver should be the only entity configuring it's DMA modes (in a safe manner) depending on the use case.
Furthermore, if there is a need for 'Use Cases', than the machine driver can provide user control to switch between them. The thing is that in most cases these are trivial, and mostly the same settings, but if you throw a codec like the tlv320dac33 into the mix, which has it's own FIFO, than things gets complicated. The user (the real one, not the developer) has several settings scattered all around the place, and those has to be configured in harmony. The only place is to do this, is in the machine driver, whihc than can build up 'scenarios', and configure the things in synchronized manner.
Liam
On Wed, 19 May 2010 15:27:35 +0300 Peter Ujfalusi peter.ujfalusi@nokia.com wrote:
The sysfs set interface implies userspace having knowledge of driver capabilities and configuration in order to safely toggle between the two DMA modes. Imo, the mcbsp client driver should be the only entity configuring it's DMA modes (in a safe manner) depending on the use case.
Furthermore, if there is a need for 'Use Cases', than the machine driver can provide user control to switch between them. The thing is that in most cases these are trivial, and mostly the same settings, but if you throw a codec like the tlv320dac33 into the mix, which has it's own FIFO, than things gets complicated. The user (the real one, not the developer) has several settings scattered all around the place, and those has to be configured in harmony. The only place is to do this, is in the machine driver, whihc than can build up 'scenarios', and configure the things in synchronized manner.
Yes and I think only very few developers know what to do with those op mode and threshold sysfs controls so most probably they are unused. Then machine drivers setting them automatically/with some control could give us more testing base.
But as those sysfs controls are there we must preserve them for a release cycle or two in case if someone is using them. At least Linus or Andrew may complain about removal of them.
participants (7)
-
Candelaria Villarreal, Jorge -
Eduardo Valentin -
Jarkko Nikula -
Kevin Hilman -
Liam Girdwood -
Mark Brown -
Peter Ujfalusi