Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
On Fri, Nov 11, 2011 at 12:32 AM, Kevin Hilman khil...@ti.com wrote: Govindraj govindraj...@gmail.com writes: [..] I have fixed this and other uart_v7 comments and have re-based the patch series on top of 3.2-rc1 along with Tero's v9 irq chaining patches and tested the same. Available here [1]. Please repost your updated series. Yes done. v8 posted [1] Can this patches series be added to a test branch for upstreaming or do you think there are still some outstanding comments that needs to be discussed? The PRCM IRQ chaining series is not yet finalized. okay fine. -- Thanks, Govindraj.R [1]: http://www.spinics.net/lists/linux-omap/msg60115.html -- To unsubscribe from this list: send the line unsubscribe linux-omap 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 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
Hi Kevin, On Wed, Nov 9, 2011 at 12:50 AM, Kevin Hilman khil...@ti.com wrote: Rajendra Nayak rna...@ti.com writes: Hi Kevin, On Saturday 05 November 2011 04:12 AM, Kevin Hilman wrote: However, as mentioned previously[1], due to a HW sleepdep between MPU and CORE, this constraint isn't actually needed for CORE UARTs, so it's a bit wasteful to go through all the constraint setting for no reason. I had a short chat with Govind on this and was trying to understand this better. Are you referring to the 'autodeps' for omap3 here, because they would prevent any clock domain from idling as long as MPU or IVA are active, No, I was thinking of HW sleepdeps. However, I looked back at the OMAP3430 TRM and see that MPU does not have a HW sleepdep on CORE like I thought. but not the other way round. Which means MPU can still idle, while CORE does not. My guess was, its probably the CORE domain idling itself thats causing the UART sluggishness, (and not MPU idling), due to higher latency, which is prevented with an active UART module in CORE, but not in PER. OK, that indeed makes sense. Thanks for correcting me. So Govind did a small experiment to prevent just CORE idling and let MPU idle alone and that did not show any sluggishness. OK, good. Now, putting a pm-qos constraint for a UART in CORE still looks redundant because the latency requirement that UART has is in some way *indirectly* met (because the active UART in CORE prevents CORE transitions in idle). But don't you think the UART driver should express its latency constraints regardless, without thinking of any indirect ways in which the same requirements would have already been met? Yes, you're right. The driver should not need to know which powerdomain a given UART is in. It's probably best (and most portable) to have UART always express its requirements all the time. Thanks for digging into this, I have fixed this and other uart_v7 comments and have re-based the patch series on top of 3.2-rc1 along with Tero's v9 irq chaining patches and tested the same. Available here [1]. Can this patches series be added to a test branch for upstreaming or do you think there are still some outstanding comments that needs to be discussed? -- Thanks, Govindraj.R [1]: git://gitorious.org/runtime_3-0/runtime_3-0.git 3.2-rc1_uart_runtime -- To unsubscribe from this list: send the line unsubscribe linux-omap 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 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
Govindraj govindraj...@gmail.com writes: Hi Kevin, On Wed, Nov 9, 2011 at 12:50 AM, Kevin Hilman khil...@ti.com wrote: Rajendra Nayak rna...@ti.com writes: Hi Kevin, On Saturday 05 November 2011 04:12 AM, Kevin Hilman wrote: However, as mentioned previously[1], due to a HW sleepdep between MPU and CORE, this constraint isn't actually needed for CORE UARTs, so it's a bit wasteful to go through all the constraint setting for no reason. I had a short chat with Govind on this and was trying to understand this better. Are you referring to the 'autodeps' for omap3 here, because they would prevent any clock domain from idling as long as MPU or IVA are active, No, I was thinking of HW sleepdeps. However, I looked back at the OMAP3430 TRM and see that MPU does not have a HW sleepdep on CORE like I thought. but not the other way round. Which means MPU can still idle, while CORE does not. My guess was, its probably the CORE domain idling itself thats causing the UART sluggishness, (and not MPU idling), due to higher latency, which is prevented with an active UART module in CORE, but not in PER. OK, that indeed makes sense. Thanks for correcting me. So Govind did a small experiment to prevent just CORE idling and let MPU idle alone and that did not show any sluggishness. OK, good. Now, putting a pm-qos constraint for a UART in CORE still looks redundant because the latency requirement that UART has is in some way *indirectly* met (because the active UART in CORE prevents CORE transitions in idle). But don't you think the UART driver should express its latency constraints regardless, without thinking of any indirect ways in which the same requirements would have already been met? Yes, you're right. The driver should not need to know which powerdomain a given UART is in. It's probably best (and most portable) to have UART always express its requirements all the time. Thanks for digging into this, I have fixed this and other uart_v7 comments and have re-based the patch series on top of 3.2-rc1 along with Tero's v9 irq chaining patches and tested the same. Available here [1]. Please repost your updated series. Can this patches series be added to a test branch for upstreaming or do you think there are still some outstanding comments that needs to be discussed? The PRCM IRQ chaining series is not yet finalized. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap 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 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
Hi Kevin,
On Saturday 05 November 2011 04:12 AM, Kevin Hilman wrote:
However, as mentioned previously[1], due to a HW sleepdep between MPU
and CORE, this constraint isn't actually needed for CORE UARTs, so it's
a bit wasteful to go through all the constraint setting for no reason.
I had a short chat with Govind on this and was trying to understand
this better.
Are you referring to the 'autodeps' for omap3 here, because they would
prevent any clock domain from idling as long as MPU or IVA are active,
but not the other way round. Which means MPU can still idle, while CORE
does not.
My guess was, its probably the CORE domain idling itself thats causing
the UART sluggishness, (and not MPU idling), due to higher latency,
which is prevented with an active UART module in CORE, but not in PER.
So Govind did a small experiment to prevent just CORE idling and let MPU
idle alone and that did not show any sluggishness.
Now, putting a pm-qos constraint for a UART in CORE still looks
redundant because the latency requirement that UART has is in
some way *indirectly* met (because the active UART in CORE prevents
CORE transitions in idle).
But don't you think the UART driver should express its
latency constraints regardless, without thinking of any indirect ways
in which the same requirements would have already been met?
regards,
Rajendra
The changelog should summarize this, and the init code should only
enable the constraints for PER UARTs, at least on OMAP3. I'm not sure
about OMAP4.
},
};
@@ -87,28 +88,6 @@ static struct omap_device_pm_latency omap_uart_latency[]
= {
};
#ifdef CONFIG_PM
-
-int omap_uart_can_sleep(void)
-{
- struct omap_uart_state *uart;
- int can_sleep = 1;
-
- list_for_each_entry(uart,uart_list, node) {
- if (!uart-clocked)
- continue;
-
- if (!uart-can_sleep) {
- can_sleep = 0;
- continue;
- }
-
- /* This UART can now safely sleep. */
- omap_uart_allow_sleep(uart);
- }
-
- return can_sleep;
-}
-
static void omap_uart_enable_wakeup(struct platform_device *pdev, bool
enable)
{
struct omap_device *od = to_omap_device(pdev);
@@ -363,6 +342,7 @@ void __init omap_serial_init_port(struct omap_board_data
*bdata,
omap_up.dma_rx_timeout = info-dma_rx_timeout;
omap_up.dma_rx_poll_rate = info-dma_rx_poll_rate;
omap_up.autosuspend_timeout = info-autosuspend_timeout;
+ omap_up.use_pm_qos = info-use_pm_qos;
/* Enable the MDR1 errata for OMAP3 */
if (cpu_is_omap34xx() !cpu_is_ti816x())
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h
b/arch/arm/plat-omap/include/plat/omap-serial.h
index 9a6879c..41eda3c 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -19,6 +19,7 @@
#includelinux/serial_core.h
#includelinux/platform_device.h
+#includelinux/pm_qos_params.h
#includeplat/mux.h
@@ -68,6 +69,7 @@ struct omap_uart_port_info {
unsigned intdma_rx_timeout;
unsigned intautosuspend_timeout;
unsigned intdma_rx_poll_rate;
+ u8 use_pm_qos;
u32 (*get_context_loss_count)(struct device *);
void (*set_forceidle)(struct platform_device *);
@@ -129,6 +131,12 @@ struct uart_omap_port {
u32 context_loss_cnt;
u32 errata;
u8 wakeups_enabled;
+
+ struct pm_qos_request_list pm_qos_request;
+ u32 latency;
+ struct work_struct work;
minor: call this qos_work.
+ u8 request_qos;
+ u8 use_pm_qos;
};
#endif /* __OMAP_SERIAL_H__ */
diff --git a/drivers/tty/serial/omap-serial.c
b/drivers/tty/serial/omap-serial.c
index 1714bd2..956736c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -51,6 +51,8 @@ static void serial_omap_rxdma_poll(unsigned long uart_no);
static int serial_omap_start_rxdma(struct uart_omap_port *up);
static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1);
+static struct workqueue_struct *serial_omap_uart_wq;
+
static inline unsigned int serial_in(struct uart_omap_port *up, int offset)
{
offset= up-port.regshift;
@@ -674,6 +676,21 @@ serial_omap_configure_xonxoff
serial_out(up, UART_LCR, up-lcr);
}
+static void serial_omap_uart_qos_work(struct work_struct *work)
+{
+ struct uart_omap_port *up = container_of(work, struct uart_omap_port,
+ work);
+
+ if (!up-request_qos) {
+ pm_qos_add_request(up-pm_qos_request,
+ PM_QOS_CPU_DMA_LATENCY, up-latency);
+
Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
Rajendra Nayak rna...@ti.com writes: Hi Kevin, On Saturday 05 November 2011 04:12 AM, Kevin Hilman wrote: However, as mentioned previously[1], due to a HW sleepdep between MPU and CORE, this constraint isn't actually needed for CORE UARTs, so it's a bit wasteful to go through all the constraint setting for no reason. I had a short chat with Govind on this and was trying to understand this better. Are you referring to the 'autodeps' for omap3 here, because they would prevent any clock domain from idling as long as MPU or IVA are active, No, I was thinking of HW sleepdeps. However, I looked back at the OMAP3430 TRM and see that MPU does not have a HW sleepdep on CORE like I thought. but not the other way round. Which means MPU can still idle, while CORE does not. My guess was, its probably the CORE domain idling itself thats causing the UART sluggishness, (and not MPU idling), due to higher latency, which is prevented with an active UART module in CORE, but not in PER. OK, that indeed makes sense. Thanks for correcting me. So Govind did a small experiment to prevent just CORE idling and let MPU idle alone and that did not show any sluggishness. OK, good. Now, putting a pm-qos constraint for a UART in CORE still looks redundant because the latency requirement that UART has is in some way *indirectly* met (because the active UART in CORE prevents CORE transitions in idle). But don't you think the UART driver should express its latency constraints regardless, without thinking of any indirect ways in which the same requirements would have already been met? Yes, you're right. The driver should not need to know which powerdomain a given UART is in. It's probably best (and most portable) to have UART always express its requirements all the time. Thanks for digging into this, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap 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 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
Govindraj.R govindraj.r...@ti.com writes:
Omap_uart_can_sleep function blocks system wide low power state until
uart is active remove this func and add qos requests to prevent
^
missing some punctuation.
MPU from transitioning while uart is active and remove qos request
if uart is auto-idled.
It would be helpful to summarize my previous comments[1] about why this
happens on some UARTs (PER) and not others (CORE.)
qos requests are blocking notifier calls so put these requests to
work queue to avoid warn on slow path warning while using qos
API's from runtime callbacks.
OK.
Rather than a vague reference to 'warn on slow path', please describe
that this is needed because the driver uses the irq_safe mode of runtime
PM, so callbacks may be run with interrupts disabled.
Flush_sync any pending qos jobs in work queue while suspending.
Nice.
Signed-off-by: Govindraj.R govindraj.r...@ti.com
---
arch/arm/mach-omap2/cpuidle34xx.c |5 ---
arch/arm/mach-omap2/pm24xx.c |2 -
arch/arm/mach-omap2/pm34xx.c | 10 --
arch/arm/mach-omap2/serial.c | 24 +-
arch/arm/plat-omap/include/plat/omap-serial.h |8 +
drivers/tty/serial/omap-serial.c | 41
-
6 files changed, 50 insertions(+), 40 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
b/arch/arm/mach-omap2/cpuidle34xx.c
index 4bf6e6e..98b7d3f 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -226,11 +226,6 @@ static int omap3_enter_idle_bm(struct cpuidle_device
*dev,
struct omap3_idle_statedata *cx;
int ret;
- if (!omap3_can_sleep()) {
- new_state = dev-safe_state;
- goto select_state;
- }
-
/*
* Prevent idle completely if CAM is active.
* CAM does not have wakeup capability in OMAP3.
diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
index a75f764..192f0a4 100644
--- a/arch/arm/mach-omap2/pm24xx.c
+++ b/arch/arm/mach-omap2/pm24xx.c
@@ -248,8 +248,6 @@ static int omap2_can_sleep(void)
{
if (omap2_fclks_active())
return 0;
- if (!omap_uart_can_sleep())
- return 0;
if (osc_ck-usecount 1)
return 0;
if (omap_dma_running())
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 6e7f276..a635fa1 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -425,21 +425,11 @@ void omap_sram_idle(void)
clkdm_allow_idle(mpu_pwrdm-pwrdm_clkdms[0]);
}
-int omap3_can_sleep(void)
-{
- if (!omap_uart_can_sleep())
- return 0;
- return 1;
-}
-
static void omap3_pm_idle(void)
{
local_irq_disable();
local_fiq_disable();
- if (!omap3_can_sleep())
- goto out;
-
if (omap_irq_pending() || need_resched())
goto out;
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 7658a03..55ce950 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -75,6 +75,7 @@ static struct omap_uart_port_info
omap_serial_default_info[] __initdata = {
.dma_rx_poll_rate = DEFAULT_RXDMA_POLLRATE,
.dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
.autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
+ .use_pm_qos = true,
You're enabling the MPU constraint for all UARTs.
However, as mentioned previously[1], due to a HW sleepdep between MPU
and CORE, this constraint isn't actually needed for CORE UARTs, so it's
a bit wasteful to go through all the constraint setting for no reason.
The changelog should summarize this, and the init code should only
enable the constraints for PER UARTs, at least on OMAP3. I'm not sure
about OMAP4.
},
};
@@ -87,28 +88,6 @@ static struct omap_device_pm_latency omap_uart_latency[] =
{
};
#ifdef CONFIG_PM
-
-int omap_uart_can_sleep(void)
-{
- struct omap_uart_state *uart;
- int can_sleep = 1;
-
- list_for_each_entry(uart, uart_list, node) {
- if (!uart-clocked)
- continue;
-
- if (!uart-can_sleep) {
- can_sleep = 0;
- continue;
- }
-
- /* This UART can now safely sleep. */
- omap_uart_allow_sleep(uart);
- }
-
- return can_sleep;
-}
-
static void omap_uart_enable_wakeup(struct platform_device *pdev, bool
enable)
{
struct omap_device *od = to_omap_device(pdev);
@@ -363,6 +342,7 @@ void __init omap_serial_init_port(struct omap_board_data
*bdata,
omap_up.dma_rx_timeout = info-dma_rx_timeout;
omap_up.dma_rx_poll_rate = info-dma_rx_poll_rate;
omap_up.autosuspend_timeout = info-autosuspend_timeout;
+
