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@xxxxxxxxxxxxxxx/msg57577.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
