* [PATCH v7 16/21] OMAP2+: UART: Remove custom activate funcs and use generic funcs.
2011-10-18 15:35 [PATCH v7 15/21] OMAP2+: UART: Make the RX_TIMEOUT for DMA configurable for each UART Govindraj.R
@ 2011-10-18 15:35 ` Govindraj.R
2011-11-04 22:00 ` Kevin Hilman
2011-10-18 15:35 ` [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos Govindraj.R
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Govindraj.R @ 2011-10-18 15:35 UTC (permalink / raw)
To: linux-omap
Cc: Kevin Hilman, Tony Lindgren, Rajendra Nayak, Partha Basak,
Govindraj R, Santosh Shilimkar, linux-serial, Vishwanath Sripathy,
linux-arm-kernel
The custom hwmod activate and deactivate funcs does hwmod_enable
and idle same can be done with omap_device generic API's.
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
arch/arm/mach-omap2/serial.c | 18 ++----------------
1 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 7c65410..7658a03 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -78,24 +78,10 @@ static struct omap_uart_port_info omap_serial_default_info[] __initdata = {
},
};
-static int uart_idle_hwmod(struct omap_device *od)
-{
- omap_hwmod_idle(od->hwmods[0]);
-
- return 0;
-}
-
-static int uart_enable_hwmod(struct omap_device *od)
-{
- omap_hwmod_enable(od->hwmods[0]);
-
- return 0;
-}
-
static struct omap_device_pm_latency omap_uart_latency[] = {
{
- .deactivate_func = uart_idle_hwmod,
- .activate_func = uart_enable_hwmod,
+ .activate_func = omap_device_enable_hwmods,
+ .deactivate_func = omap_device_idle_hwmods,
.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
},
};
--
1.7.4.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v7 16/21] OMAP2+: UART: Remove custom activate funcs and use generic funcs.
2011-10-18 15:35 ` [PATCH v7 16/21] OMAP2+: UART: Remove custom activate funcs and use generic funcs Govindraj.R
@ 2011-11-04 22:00 ` Kevin Hilman
2011-11-07 8:39 ` Govindraj
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Hilman @ 2011-11-04 22:00 UTC (permalink / raw)
To: Govindraj.R
Cc: linux-omap, Tony Lindgren, Rajendra Nayak, Partha Basak,
Santosh Shilimkar, linux-serial, Vishwanath Sripathy,
linux-arm-kernel
"Govindraj.R" <govindraj.raja@ti.com> writes:
> The custom hwmod activate and deactivate funcs does hwmod_enable
> and idle same can be done with omap_device generic API's.
>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
This one needs a minor update for current mainline...
> ---
> arch/arm/mach-omap2/serial.c | 18 ++----------------
> 1 files changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 7c65410..7658a03 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -78,24 +78,10 @@ static struct omap_uart_port_info omap_serial_default_info[] __initdata = {
> },
> };
>
> -static int uart_idle_hwmod(struct omap_device *od)
> -{
> - omap_hwmod_idle(od->hwmods[0]);
> -
> - return 0;
> -}
> -
> -static int uart_enable_hwmod(struct omap_device *od)
> -{
> - omap_hwmod_enable(od->hwmods[0]);
> -
> - return 0;
> -}
> -
> static struct omap_device_pm_latency omap_uart_latency[] = {
> {
> - .deactivate_func = uart_idle_hwmod,
> - .activate_func = uart_enable_hwmod,
> + .activate_func = omap_device_enable_hwmods,
> + .deactivate_func = omap_device_idle_hwmods,
> .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> },
> };
If we're just using default pm_latency struct, it can be passed to
omap_device_build as NULL, and a default one will be configured.
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v7 16/21] OMAP2+: UART: Remove custom activate funcs and use generic funcs.
2011-11-04 22:00 ` Kevin Hilman
@ 2011-11-07 8:39 ` Govindraj
0 siblings, 0 replies; 15+ messages in thread
From: Govindraj @ 2011-11-07 8:39 UTC (permalink / raw)
To: Kevin Hilman
Cc: Govindraj.R, linux-omap, Tony Lindgren, Rajendra Nayak,
Partha Basak, Santosh Shilimkar, linux-serial,
Vishwanath Sripathy, linux-arm-kernel
On Sat, Nov 5, 2011 at 3:30 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> The custom hwmod activate and deactivate funcs does hwmod_enable
>> and idle same can be done with omap_device generic API's.
>>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>
> This one needs a minor update for current mainline...
>
>> ---
>> arch/arm/mach-omap2/serial.c | 18 ++----------------
>> 1 files changed, 2 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 7c65410..7658a03 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -78,24 +78,10 @@ static struct omap_uart_port_info omap_serial_default_info[] __initdata = {
>> },
>> };
>>
>> -static int uart_idle_hwmod(struct omap_device *od)
>> -{
>> - omap_hwmod_idle(od->hwmods[0]);
>> -
>> - return 0;
>> -}
>> -
>> -static int uart_enable_hwmod(struct omap_device *od)
>> -{
>> - omap_hwmod_enable(od->hwmods[0]);
>> -
>> - return 0;
>> -}
>> -
>> static struct omap_device_pm_latency omap_uart_latency[] = {
>> {
>> - .deactivate_func = uart_idle_hwmod,
>> - .activate_func = uart_enable_hwmod,
>> + .activate_func = omap_device_enable_hwmods,
>> + .deactivate_func = omap_device_idle_hwmods,
>> .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> },
>> };
>
> If we're just using default pm_latency struct, it can be passed to
> omap_device_build as NULL, and a default one will be configured.
>
yes, will update this.
--
Thanks,
Govindraj.R
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-10-18 15:35 [PATCH v7 15/21] OMAP2+: UART: Make the RX_TIMEOUT for DMA configurable for each UART Govindraj.R
2011-10-18 15:35 ` [PATCH v7 16/21] OMAP2+: UART: Remove custom activate funcs and use generic funcs Govindraj.R
@ 2011-10-18 15:35 ` Govindraj.R
2011-11-04 22:42 ` Kevin Hilman
2011-10-18 15:35 ` [PATCH v7 18/21] OMAP2+: UART: remove temporary variable used to count uart instance Govindraj.R
2011-10-18 15:35 ` [PATCH v7 19/21] OMAP2+: UART: Use custom activate func for console uart Govindraj.R
3 siblings, 1 reply; 15+ messages in thread
From: Govindraj.R @ 2011-10-18 15:35 UTC (permalink / raw)
To: linux-omap
Cc: linux-serial, linux-arm-kernel, Kevin Hilman, Tony Lindgren,
Partha Basak, Vishwanath Sripathy, Rajendra Nayak,
Santosh Shilimkar, Govindraj R
Omap_uart_can_sleep function blocks system wide low power state until
uart is active remove this func and add qos requests to prevent
MPU from transitioning while uart is active and remove qos request
if uart is auto-idled.
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. Flush_sync any pending qos jobs
in work queue while suspending.
Signed-off-by: Govindraj.R <govindraj.raja@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,
},
};
@@ -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 @@
#include <linux/serial_core.h>
#include <linux/platform_device.h>
+#include <linux/pm_qos_params.h>
#include <plat/mux.h>
@@ -68,6 +69,7 @@ struct omap_uart_port_info {
unsigned int dma_rx_timeout;
unsigned int autosuspend_timeout;
unsigned int dma_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;
+ 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);
+ up->request_qos = true;
+ } else {
+ pm_qos_remove_request(&up->pm_qos_request);
+ up->request_qos = false;
+ }
+}
+
static void
serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
struct ktermios *old)
@@ -869,6 +886,13 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
serial_omap_configure_xonxoff(up, termios);
spin_unlock_irqrestore(&up->port.lock, flags);
+
+ if (up->use_pm_qos) {
+ /* calculate wakeup latency constraint */
+ up->latency = (1000000 * up->port.fifosize) / (1000 * baud / 8);
+ schedule_work(&up->work);
+ }
+
pm_runtime_put(&up->pdev->dev);
dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
}
@@ -1144,8 +1168,11 @@ static int serial_omap_suspend(struct device *dev)
{
struct uart_omap_port *up = dev_get_drvdata(dev);
- if (up)
+ if (up) {
uart_suspend_port(&serial_omap_reg, &up->port);
+ flush_work_sync(&up->work);
+ }
+
return 0;
}
@@ -1368,6 +1395,7 @@ static int serial_omap_probe(struct platform_device *pdev)
up->port.uartclk = omap_up_info->uartclk;
up->uart_dma.uart_base = mem->start;
up->errata = omap_up_info->errata;
+ up->use_pm_qos = omap_up_info->use_pm_qos;
if (omap_up_info->dma_enabled) {
up->uart_dma.uart_dma_tx = dma_tx->start;
@@ -1382,6 +1410,11 @@ static int serial_omap_probe(struct platform_device *pdev)
up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
}
+ if (up->use_pm_qos) {
+ serial_omap_uart_wq = create_workqueue(up->name);
+ INIT_WORK(&up->work, serial_omap_uart_qos_work);
+ }
+
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev,
omap_up_info->autosuspend_timeout);
@@ -1516,6 +1549,9 @@ static int serial_omap_runtime_suspend(struct device *dev)
if (up->use_dma && pdata->set_forceidle)
pdata->set_forceidle(up->pdev);
+ if (up->use_pm_qos && up->request_qos)
+ schedule_work(&up->work);
+
return 0;
}
@@ -1535,6 +1571,9 @@ static int serial_omap_runtime_resume(struct device *dev)
/* Errata i291 */
if (up->use_dma && pdata->set_noidle)
pdata->set_noidle(up->pdev);
+
+ if (up->use_pm_qos && !up->request_qos)
+ schedule_work(&up->work);
}
return 0;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-10-18 15:35 ` [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos Govindraj.R
@ 2011-11-04 22:42 ` Kevin Hilman
2011-11-08 9:16 ` Rajendra Nayak
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Hilman @ 2011-11-04 22:42 UTC (permalink / raw)
To: Govindraj.R
Cc: linux-omap, Tony Lindgren, Rajendra Nayak, Partha Basak,
Santosh Shilimkar, linux-serial, Vishwanath Sripathy,
linux-arm-kernel
"Govindraj.R" <govindraj.raja@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.raja@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;
> + 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 @@
>
> #include <linux/serial_core.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_qos_params.h>
>
> #include <plat/mux.h>
>
> @@ -68,6 +69,7 @@ struct omap_uart_port_info {
> unsigned int dma_rx_timeout;
> unsigned int autosuspend_timeout;
> unsigned int dma_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);
> + up->request_qos = true;
->request_qos is a confusing name. ->qos_requested would help
readability, IMO.
> + } else {
> + pm_qos_remove_request(&up->pm_qos_request);
> + up->request_qos = false;
> + }
> +}
> +
> static void
> serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
> struct ktermios *old)
> @@ -869,6 +886,13 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
> serial_omap_configure_xonxoff(up, termios);
>
> spin_unlock_irqrestore(&up->port.lock, flags);
> +
> + if (up->use_pm_qos) {
> + /* calculate wakeup latency constraint */
> + up->latency = (1000000 * up->port.fifosize) / (1000 * baud / 8);
> + schedule_work(&up->work);
Is this one really needed? There's a pm_runtime_put() right after this
that will immediately remove the constraint.
Actually, it seems like it would be better to calculate the latency
earlier in this function (right after the baud rate) so that the
subsequent pm_runtime_get() sets the constraint...
> + }
> +
> pm_runtime_put(&up->pdev->dev);
...and this will remove the constraint.
> dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
> }
> @@ -1144,8 +1168,11 @@ static int serial_omap_suspend(struct device *dev)
> {
> struct uart_omap_port *up = dev_get_drvdata(dev);
>
> - if (up)
> + if (up) {
> uart_suspend_port(&serial_omap_reg, &up->port);
> + flush_work_sync(&up->work);
> + }
> +
> return 0;
> }
>
> @@ -1368,6 +1395,7 @@ static int serial_omap_probe(struct platform_device *pdev)
> up->port.uartclk = omap_up_info->uartclk;
> up->uart_dma.uart_base = mem->start;
> up->errata = omap_up_info->errata;
> + up->use_pm_qos = omap_up_info->use_pm_qos;
>
> if (omap_up_info->dma_enabled) {
> up->uart_dma.uart_dma_tx = dma_tx->start;
> @@ -1382,6 +1410,11 @@ static int serial_omap_probe(struct platform_device *pdev)
> up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
> }
>
> + if (up->use_pm_qos) {
> + serial_omap_uart_wq = create_workqueue(up->name);
> + INIT_WORK(&up->work, serial_omap_uart_qos_work);
> + }
> +
> pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_set_autosuspend_delay(&pdev->dev,
> omap_up_info->autosuspend_timeout);
> @@ -1516,6 +1549,9 @@ static int serial_omap_runtime_suspend(struct device *dev)
> if (up->use_dma && pdata->set_forceidle)
> pdata->set_forceidle(up->pdev);
>
> + if (up->use_pm_qos && up->request_qos)
> + schedule_work(&up->work);
> +
> return 0;
> }
>
> @@ -1535,6 +1571,9 @@ static int serial_omap_runtime_resume(struct device *dev)
> /* Errata i291 */
> if (up->use_dma && pdata->set_noidle)
> pdata->set_noidle(up->pdev);
> +
> + if (up->use_pm_qos && !up->request_qos)
> + schedule_work(&up->work);
> }
>
> return 0;
Kevin
[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg57577.html
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-11-04 22:42 ` Kevin Hilman
@ 2011-11-08 9:16 ` Rajendra Nayak
2011-11-08 19:20 ` Kevin Hilman
0 siblings, 1 reply; 15+ messages in thread
From: Rajendra Nayak @ 2011-11-08 9:16 UTC (permalink / raw)
To: Kevin Hilman
Cc: Govindraj.R, linux-omap, Tony Lindgren, Partha Basak,
Santosh Shilimkar, linux-serial, Vishwanath Sripathy,
linux-arm-kernel
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 @@
>> >
>> > #include<linux/serial_core.h>
>> > #include<linux/platform_device.h>
>> > +#include<linux/pm_qos_params.h>
>> >
>> > #include<plat/mux.h>
>> >
>> > @@ -68,6 +69,7 @@ struct omap_uart_port_info {
>> > unsigned int dma_rx_timeout;
>> > unsigned int autosuspend_timeout;
>> > unsigned int dma_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);
>> > + up->request_qos = true;
> ->request_qos is a confusing name. ->qos_requested would help
> readability, IMO.
>
>> > + } else {
>> > + pm_qos_remove_request(&up->pm_qos_request);
>> > + up->request_qos = false;
>> > + }
>> > +}
>> > +
>> > static void
>> > serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>> > struct ktermios *old)
>> > @@ -869,6 +886,13 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>> > serial_omap_configure_xonxoff(up, termios);
>> >
>> > spin_unlock_irqrestore(&up->port.lock, flags);
>> > +
>> > + if (up->use_pm_qos) {
>> > + /* calculate wakeup latency constraint */
>> > + up->latency = (1000000 * up->port.fifosize) / (1000 * baud / 8);
>> > + schedule_work(&up->work);
> Is this one really needed? There's a pm_runtime_put() right after this
> that will immediately remove the constraint.
>
> Actually, it seems like it would be better to calculate the latency
> earlier in this function (right after the baud rate) so that the
> subsequent pm_runtime_get() sets the constraint...
>
>> > + }
>> > +
>> > pm_runtime_put(&up->pdev->dev);
> ...and this will remove the constraint.
>
>> > dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
>> > }
>> > @@ -1144,8 +1168,11 @@ static int serial_omap_suspend(struct device *dev)
>> > {
>> > struct uart_omap_port *up = dev_get_drvdata(dev);
>> >
>> > - if (up)
>> > + if (up) {
>> > uart_suspend_port(&serial_omap_reg,&up->port);
>> > + flush_work_sync(&up->work);
>> > + }
>> > +
>> > return 0;
>> > }
>> >
>> > @@ -1368,6 +1395,7 @@ static int serial_omap_probe(struct platform_device *pdev)
>> > up->port.uartclk = omap_up_info->uartclk;
>> > up->uart_dma.uart_base = mem->start;
>> > up->errata = omap_up_info->errata;
>> > + up->use_pm_qos = omap_up_info->use_pm_qos;
>> >
>> > if (omap_up_info->dma_enabled) {
>> > up->uart_dma.uart_dma_tx = dma_tx->start;
>> > @@ -1382,6 +1410,11 @@ static int serial_omap_probe(struct platform_device *pdev)
>> > up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
>> > }
>> >
>> > + if (up->use_pm_qos) {
>> > + serial_omap_uart_wq = create_workqueue(up->name);
>> > + INIT_WORK(&up->work, serial_omap_uart_qos_work);
>> > + }
>> > +
>> > pm_runtime_use_autosuspend(&pdev->dev);
>> > pm_runtime_set_autosuspend_delay(&pdev->dev,
>> > omap_up_info->autosuspend_timeout);
>> > @@ -1516,6 +1549,9 @@ static int serial_omap_runtime_suspend(struct device *dev)
>> > if (up->use_dma&& pdata->set_forceidle)
>> > pdata->set_forceidle(up->pdev);
>> >
>> > + if (up->use_pm_qos&& up->request_qos)
>> > + schedule_work(&up->work);
>> > +
>> > return 0;
>> > }
>> >
>> > @@ -1535,6 +1571,9 @@ static int serial_omap_runtime_resume(struct device *dev)
>> > /* Errata i291 */
>> > if (up->use_dma&& pdata->set_noidle)
>> > pdata->set_noidle(up->pdev);
>> > +
>> > + if (up->use_pm_qos&& !up->request_qos)
>> > + schedule_work(&up->work);
>> > }
>> >
>> > return 0;
> Kevin
>
> [1]http://www.mail-archive.com/linux-omap@vger.kernel.org/msg57577.html
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-11-08 9:16 ` Rajendra Nayak
@ 2011-11-08 19:20 ` Kevin Hilman
2011-11-10 12:00 ` Govindraj
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Hilman @ 2011-11-08 19:20 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Govindraj.R, linux-omap, Tony Lindgren, Partha Basak,
Santosh Shilimkar, linux-serial, Vishwanath Sripathy,
linux-arm-kernel
Rajendra Nayak <rnayak@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-11-08 19:20 ` Kevin Hilman
@ 2011-11-10 12:00 ` Govindraj
2011-11-10 19:02 ` Kevin Hilman
0 siblings, 1 reply; 15+ messages in thread
From: Govindraj @ 2011-11-10 12:00 UTC (permalink / raw)
To: Kevin Hilman
Cc: Rajendra Nayak, Govindraj.R, linux-omap, Tony Lindgren,
Partha Basak, Santosh Shilimkar, linux-serial,
Vishwanath Sripathy, linux-arm-kernel
Hi Kevin,
On Wed, Nov 9, 2011 at 12:50 AM, Kevin Hilman <khilman@ti.com> wrote:
> Rajendra Nayak <rnayak@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-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-11-10 12:00 ` Govindraj
@ 2011-11-10 19:02 ` Kevin Hilman
2011-11-11 10:17 ` Govindraj
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Hilman @ 2011-11-10 19:02 UTC (permalink / raw)
To: Govindraj
Cc: Rajendra Nayak, Govindraj.R, linux-omap, Tony Lindgren,
Partha Basak, Santosh Shilimkar, linux-serial,
Vishwanath Sripathy, linux-arm-kernel
Govindraj <govindraj.ti@gmail.com> writes:
> Hi Kevin,
>
> On Wed, Nov 9, 2011 at 12:50 AM, Kevin Hilman <khilman@ti.com> wrote:
>> Rajendra Nayak <rnayak@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-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos
2011-11-10 19:02 ` Kevin Hilman
@ 2011-11-11 10:17 ` Govindraj
0 siblings, 0 replies; 15+ messages in thread
From: Govindraj @ 2011-11-11 10:17 UTC (permalink / raw)
To: Kevin Hilman
Cc: Rajendra Nayak, Govindraj.R, linux-omap, Tony Lindgren,
Partha Basak, Santosh Shilimkar, linux-serial,
Vishwanath Sripathy, linux-arm-kernel
On Fri, Nov 11, 2011 at 12:32 AM, Kevin Hilman <khilman@ti.com> wrote:
> Govindraj <govindraj.ti@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v7 18/21] OMAP2+: UART: remove temporary variable used to count uart instance
2011-10-18 15:35 [PATCH v7 15/21] OMAP2+: UART: Make the RX_TIMEOUT for DMA configurable for each UART Govindraj.R
2011-10-18 15:35 ` [PATCH v7 16/21] OMAP2+: UART: Remove custom activate funcs and use generic funcs Govindraj.R
2011-10-18 15:35 ` [PATCH v7 17/21] OMAP2+: UART: Remove omap_uart_can_sleep and add pm_qos Govindraj.R
@ 2011-10-18 15:35 ` Govindraj.R
2011-10-18 15:35 ` [PATCH v7 19/21] OMAP2+: UART: Use custom activate func for console uart Govindraj.R
3 siblings, 0 replies; 15+ messages in thread
From: Govindraj.R @ 2011-10-18 15:35 UTC (permalink / raw)
To: linux-omap
Cc: linux-serial, linux-arm-kernel, Kevin Hilman, Tony Lindgren,
Partha Basak, Vishwanath Sripathy, Rajendra Nayak,
Santosh Shilimkar, Govindraj R
Reuse the num_uarts variable itself to count number of uarts.
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
arch/arm/mach-omap2/serial.c | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 55ce950..e1eba7f 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -253,15 +253,13 @@ static void omap_serial_fill_default_pads(struct omap_board_data *bdata) {}
static int __init omap_serial_early_init(void)
{
- int i = 0;
-
do {
char oh_name[MAX_UART_HWMOD_NAME_LEN];
struct omap_hwmod *oh;
struct omap_uart_state *uart;
snprintf(oh_name, MAX_UART_HWMOD_NAME_LEN,
- "uart%d", i + 1);
+ "uart%d", num_uarts + 1);
oh = omap_hwmod_lookup(oh_name);
if (!oh)
break;
@@ -271,9 +269,8 @@ static int __init omap_serial_early_init(void)
return -ENODEV;
uart->oh = oh;
- uart->num = i++;
+ uart->num = num_uarts++;
list_add_tail(&uart->node, &uart_list);
- num_uarts++;
/*
* NOTE: omap_hwmod_setup*() has not yet been called,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v7 19/21] OMAP2+: UART: Use custom activate func for console uart.
2011-10-18 15:35 [PATCH v7 15/21] OMAP2+: UART: Make the RX_TIMEOUT for DMA configurable for each UART Govindraj.R
` (2 preceding siblings ...)
2011-10-18 15:35 ` [PATCH v7 18/21] OMAP2+: UART: remove temporary variable used to count uart instance Govindraj.R
@ 2011-10-18 15:35 ` Govindraj.R
2011-11-04 23:00 ` Kevin Hilman
3 siblings, 1 reply; 15+ messages in thread
From: Govindraj.R @ 2011-10-18 15:35 UTC (permalink / raw)
To: linux-omap
Cc: linux-serial, linux-arm-kernel, Kevin Hilman, Tony Lindgren,
Partha Basak, Vishwanath Sripathy, Rajendra Nayak,
Santosh Shilimkar, Govindraj R
Omap-uart can be used as console uart to print early boot
messages using earlyprintk so for console uart prevent
hwmod reset or idling during bootup.
Identify the console_uart set the id and use the custom
pm_latency ops for console uart for the first time
to idle console uart left enabled from bootup and then enable
them back and reset pm_latency ops to default ops.
Thanks to Kevin Hilman <khilman@ti.com> for suggesting
this approach.
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
arch/arm/mach-omap2/serial.c | 79 ++++++++++++++++++++++++++++-------------
1 files changed, 54 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index e1eba7f..55903f0 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -63,6 +63,7 @@ struct omap_uart_state {
static LIST_HEAD(uart_list);
static u8 num_uarts;
+static u8 console_uart_id = -1;
#define DEFAULT_RXDMA_POLLRATE 1 /* RX DMA polling rate (us) */
#define DEFAULT_RXDMA_BUFSIZE 4096 /* RX DMA buffer size */
@@ -87,6 +88,29 @@ static struct omap_device_pm_latency omap_uart_latency[] = {
},
};
+static int console_uart_enable_hwmod(struct omap_device *od)
+{
+ /* For early console we prevented hwmod reset and idle
+ * So before we enable the uart clocks idle and then
+ */
+ omap_hwmod_idle(od->hwmods[0]);
+ omap_hwmod_enable(od->hwmods[0]);
+
+ /* restore the default activate/deactivate funcs,
+ * since now we have set the hwmod state machine right
+ * with the idle/enable for console uart
+ */
+ od->pm_lats = omap_uart_latency;
+
+ return 0;
+}
+
+static struct omap_device_pm_latency console_uart_latency[] = {
+ {
+ .activate_func = console_uart_enable_hwmod,
+ },
+};
+
#ifdef CONFIG_PM
static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
{
@@ -251,12 +275,20 @@ static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
static void omap_serial_fill_default_pads(struct omap_board_data *bdata) {}
#endif
+char *cmdline_find_option(char *str)
+{
+ extern char *saved_command_line;
+
+ return strstr(saved_command_line, str);
+}
+
static int __init omap_serial_early_init(void)
{
do {
char oh_name[MAX_UART_HWMOD_NAME_LEN];
struct omap_hwmod *oh;
struct omap_uart_state *uart;
+ char uart_name[MAX_UART_HWMOD_NAME_LEN];
snprintf(oh_name, MAX_UART_HWMOD_NAME_LEN,
"uart%d", num_uarts + 1);
@@ -271,18 +303,23 @@ static int __init omap_serial_early_init(void)
uart->oh = oh;
uart->num = num_uarts++;
list_add_tail(&uart->node, &uart_list);
+ snprintf(uart_name, MAX_UART_HWMOD_NAME_LEN,
+ "%s%d", OMAP_SERIAL_NAME, uart->num);
+
+ if (cmdline_find_option(uart_name)) {
+ console_uart_id = uart->num;
+ /*
+ * omap-uart can be used for earlyprintk logs
+ * So if omap-uart is used as console then prevent
+ * uart reset and idle to get logs from omap-uart
+ * until uart console driver is available to take
+ * care for console messages.
+ * Idling or resetting omap-uart while printing logs
+ * early boot logs can stall the boot-up.
+ */
+ oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
+ }
- /*
- * NOTE: omap_hwmod_setup*() has not yet been called,
- * so no hwmod functions will work yet.
- */
-
- /*
- * During UART early init, device need to be probed
- * to determine SoC specific init before omap_device
- * is ready. Therefore, don't allow idle here
- */
- uart->oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
} while (1);
return 0;
@@ -311,6 +348,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
u32 pdata_size = 0;
char *name;
struct omap_uart_port_info omap_up;
+ struct omap_device *od;
if (WARN_ON(!bdata))
return;
@@ -357,6 +395,11 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
WARN(IS_ERR(pdev), "Could not build omap_device for %s: %s.\n",
name, oh->name);
+ if (console_uart_id == bdata->id) {
+ od = to_omap_device(pdev);
+ od->pm_lats = console_uart_latency;
+ }
+
omap_device_disable_idle_on_suspend(pdev);
oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
@@ -364,20 +407,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
oh->dev_attr = uart;
- console_lock(); /* in case the earlycon is on the UART */
-
- /*
- * Because of early UART probing, UART did not get idled
- * on init. Now that omap_device is ready, ensure full idle
- * before doing omap_device_enable().
- */
- omap_hwmod_idle(uart->oh);
-
- omap_device_enable(uart->pdev);
- omap_device_idle(uart->pdev);
-
- console_unlock();
-
if ((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
device_init_wakeup(&pdev->dev, true);
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v7 19/21] OMAP2+: UART: Use custom activate func for console uart.
2011-10-18 15:35 ` [PATCH v7 19/21] OMAP2+: UART: Use custom activate func for console uart Govindraj.R
@ 2011-11-04 23:00 ` Kevin Hilman
2011-11-07 8:42 ` Govindraj
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Hilman @ 2011-11-04 23:00 UTC (permalink / raw)
To: Govindraj.R
Cc: linux-omap, Tony Lindgren, Rajendra Nayak, Partha Basak,
Santosh Shilimkar, linux-serial, Vishwanath Sripathy,
linux-arm-kernel
"Govindraj.R" <govindraj.raja@ti.com> writes:
> Omap-uart can be used as console uart to print early boot
> messages using earlyprintk so for console uart prevent
> hwmod reset or idling during bootup.
>
> Identify the console_uart set the id and use the custom
> pm_latency ops for console uart for the first time
> to idle console uart left enabled from bootup and then enable
> them back and reset pm_latency ops to default ops.
>
> Thanks to Kevin Hilman <khilman@ti.com> for suggesting
> this approach.
>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
> arch/arm/mach-omap2/serial.c | 79 ++++++++++++++++++++++++++++-------------
> 1 files changed, 54 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index e1eba7f..55903f0 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -63,6 +63,7 @@ struct omap_uart_state {
>
> static LIST_HEAD(uart_list);
> static u8 num_uarts;
> +static u8 console_uart_id = -1;
>
> #define DEFAULT_RXDMA_POLLRATE 1 /* RX DMA polling rate (us) */
> #define DEFAULT_RXDMA_BUFSIZE 4096 /* RX DMA buffer size */
> @@ -87,6 +88,29 @@ static struct omap_device_pm_latency omap_uart_latency[] = {
> },
> };
>
> +static int console_uart_enable_hwmod(struct omap_device *od)
> +{
> + /* For early console we prevented hwmod reset and idle
> + * So before we enable the uart clocks idle and then
> + */
minor: fix multi-line comment style (search for multi-line in
Documentation/CodingStyle). Another one below.
Notice that you're moving existing code here, but also changing the
functionality without a description as to why. Based on the existing code:
You need a console_lock() here...
> + omap_hwmod_idle(od->hwmods[0]);
> + omap_hwmod_enable(od->hwmods[0]);
...and this should be omap_device_enable().
And you need a console_unlock() here.
Yes, you're subsequent patches avoid this problem, but we still need a real
fix other than checking for 'debug/loglevel', and when that happens,
we'll need a console lock here.
> + /* restore the default activate/deactivate funcs,
> + * since now we have set the hwmod state machine right
> + * with the idle/enable for console uart
> + */
> + od->pm_lats = omap_uart_latency;
> +
> + return 0;
> +}
> +
> +static struct omap_device_pm_latency console_uart_latency[] = {
> + {
> + .activate_func = console_uart_enable_hwmod,
> + },
> +};
> +
> #ifdef CONFIG_PM
> static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
> {
> @@ -251,12 +275,20 @@ static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
> static void omap_serial_fill_default_pads(struct omap_board_data *bdata) {}
> #endif
>
> +char *cmdline_find_option(char *str)
static inline
> +{
> + extern char *saved_command_line;
> +
> + return strstr(saved_command_line, str);
> +}
> +
> static int __init omap_serial_early_init(void)
> {
> do {
> char oh_name[MAX_UART_HWMOD_NAME_LEN];
> struct omap_hwmod *oh;
> struct omap_uart_state *uart;
> + char uart_name[MAX_UART_HWMOD_NAME_LEN];
>
> snprintf(oh_name, MAX_UART_HWMOD_NAME_LEN,
> "uart%d", num_uarts + 1);
> @@ -271,18 +303,23 @@ static int __init omap_serial_early_init(void)
> uart->oh = oh;
> uart->num = num_uarts++;
> list_add_tail(&uart->node, &uart_list);
> + snprintf(uart_name, MAX_UART_HWMOD_NAME_LEN,
> + "%s%d", OMAP_SERIAL_NAME, uart->num);
> +
> + if (cmdline_find_option(uart_name)) {
> + console_uart_id = uart->num;
> + /*
> + * omap-uart can be used for earlyprintk logs
> + * So if omap-uart is used as console then prevent
> + * uart reset and idle to get logs from omap-uart
> + * until uart console driver is available to take
> + * care for console messages.
> + * Idling or resetting omap-uart while printing logs
> + * early boot logs can stall the boot-up.
> + */
> + oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
> + }
>
> - /*
> - * NOTE: omap_hwmod_setup*() has not yet been called,
> - * so no hwmod functions will work yet.
> - */
> -
> - /*
> - * During UART early init, device need to be probed
> - * to determine SoC specific init before omap_device
> - * is ready. Therefore, don't allow idle here
> - */
> - uart->oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
> } while (1);
>
> return 0;
> @@ -311,6 +348,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
> u32 pdata_size = 0;
> char *name;
> struct omap_uart_port_info omap_up;
> + struct omap_device *od;
>
> if (WARN_ON(!bdata))
> return;
> @@ -357,6 +395,11 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
> WARN(IS_ERR(pdev), "Could not build omap_device for %s: %s.\n",
> name, oh->name);
>
> + if (console_uart_id == bdata->id) {
> + od = to_omap_device(pdev);
> + od->pm_lats = console_uart_latency;
> + }
> +
> omap_device_disable_idle_on_suspend(pdev);
> oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>
> @@ -364,20 +407,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>
> oh->dev_attr = uart;
>
> - console_lock(); /* in case the earlycon is on the UART */
> -
> - /*
> - * Because of early UART probing, UART did not get idled
> - * on init. Now that omap_device is ready, ensure full idle
> - * before doing omap_device_enable().
> - */
> - omap_hwmod_idle(uart->oh);
> -
> - omap_device_enable(uart->pdev);
> - omap_device_idle(uart->pdev);
> -
> - console_unlock();
> -
> if ((cpu_is_omap34xx() || cpu_is_omap44xx()) && bdata->pads)
> device_init_wakeup(&pdev->dev, true);
> }
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v7 19/21] OMAP2+: UART: Use custom activate func for console uart.
2011-11-04 23:00 ` Kevin Hilman
@ 2011-11-07 8:42 ` Govindraj
0 siblings, 0 replies; 15+ messages in thread
From: Govindraj @ 2011-11-07 8:42 UTC (permalink / raw)
To: Kevin Hilman
Cc: Govindraj.R, linux-omap, Tony Lindgren, Rajendra Nayak,
Partha Basak, Santosh Shilimkar, linux-serial,
Vishwanath Sripathy, linux-arm-kernel
On Sat, Nov 5, 2011 at 4:30 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> Omap-uart can be used as console uart to print early boot
>> messages using earlyprintk so for console uart prevent
>> hwmod reset or idling during bootup.
>>
>> Identify the console_uart set the id and use the custom
>> pm_latency ops for console uart for the first time
>> to idle console uart left enabled from bootup and then enable
>> them back and reset pm_latency ops to default ops.
>>
>> Thanks to Kevin Hilman <khilman@ti.com> for suggesting
>> this approach.
>>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>> ---
>> arch/arm/mach-omap2/serial.c | 79 ++++++++++++++++++++++++++++-------------
>> 1 files changed, 54 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index e1eba7f..55903f0 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -63,6 +63,7 @@ struct omap_uart_state {
>>
>> static LIST_HEAD(uart_list);
>> static u8 num_uarts;
>> +static u8 console_uart_id = -1;
>>
>> #define DEFAULT_RXDMA_POLLRATE 1 /* RX DMA polling rate (us) */
>> #define DEFAULT_RXDMA_BUFSIZE 4096 /* RX DMA buffer size */
>> @@ -87,6 +88,29 @@ static struct omap_device_pm_latency omap_uart_latency[] = {
>> },
>> };
>>
>> +static int console_uart_enable_hwmod(struct omap_device *od)
>> +{
>> + /* For early console we prevented hwmod reset and idle
>> + * So before we enable the uart clocks idle and then
>> + */
>
> minor: fix multi-line comment style (search for multi-line in
> Documentation/CodingStyle). Another one below.
>
will correct this.
> Notice that you're moving existing code here, but also changing the
> functionality without a description as to why. Based on the existing code:
>
> You need a console_lock() here...
>
>> + omap_hwmod_idle(od->hwmods[0]);
>> + omap_hwmod_enable(od->hwmods[0]);
>
> ...and this should be omap_device_enable().
>
> And you need a console_unlock() here.
>
> Yes, you're subsequent patches avoid this problem, but we still need a real
> fix other than checking for 'debug/loglevel', and when that happens,
> we'll need a console lock here.
>
yes correct, we need console_lock/unlock binding here will update this.
--
Thanks,
Govindraj.R
--
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
^ permalink raw reply [flat|nested] 15+ messages in thread