From: santosh.shilimkar@ti•com (Santosh Shilimkar)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver
Date: Fri, 15 Feb 2013 18:49:20 +0530 [thread overview]
Message-ID: <511E35D8.1030109@ti.com> (raw)
In-Reply-To: <20130215125740.GB16558@arwen.pp.htv.fi>
On Friday 15 February 2013 06:27 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 15, 2013 at 05:38:55PM +0530, Santosh Shilimkar wrote:
>> UART IP idle handling now taken care by runtime pm apis so remove
>> the hackery from the driver.
>>
>> Signed-off-by: Rajendra nayak <rnayak@ti•com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti•com>
>> ---
>> arch/arm/mach-omap2/serial.c | 31 -------------------------------
>> drivers/tty/serial/omap-serial.c | 23 -----------------------
>> 2 files changed, 54 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 04fdbc4..037e691 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -95,38 +95,9 @@ static void omap_uart_enable_wakeup(struct device *dev, bool enable)
>> omap_hwmod_disable_wakeup(od->hwmods[0]);
>> }
>>
>> -/*
>> - * Errata i291: [UART]:Cannot Acknowledge Idle Requests
>> - * in Smartidle Mode When Configured for DMA Operations.
>> - * WA: configure uart in force idle mode.
>> - */
>> -static void omap_uart_set_noidle(struct device *dev)
>> -{
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct omap_device *od = to_omap_device(pdev);
>> -
>> - omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
>> -}
>> -
>> -static void omap_uart_set_smartidle(struct device *dev)
>> -{
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct omap_device *od = to_omap_device(pdev);
>> - u8 idlemode;
>> -
>> - if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
>> - idlemode = HWMOD_IDLEMODE_SMART_WKUP;
>> - else
>> - idlemode = HWMOD_IDLEMODE_SMART;
>> -
>> - omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
>> -}
>> -
>> #else
>> static void omap_uart_enable_wakeup(struct device *dev, bool enable)
>> {}
>> -static void omap_uart_set_noidle(struct device *dev) {}
>> -static void omap_uart_set_smartidle(struct device *dev) {}
>> #endif /* CONFIG_PM */
>>
>> #ifdef CONFIG_OMAP_MUX
>> @@ -299,8 +270,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>> omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>> omap_up.flags = UPF_BOOT_AUTOCONF;
>> omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
>> - omap_up.set_forceidle = omap_uart_set_smartidle;
>> - omap_up.set_noidle = omap_uart_set_noidle;
>> omap_up.enable_wakeup = omap_uart_enable_wakeup;
>> omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
>> omap_up.dma_rx_timeout = info->dma_rx_timeout;
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 57d6b29..5722eaf 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -201,26 +201,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
>> return pdata->get_context_loss_count(up->dev);
>> }
>>
>> -static void serial_omap_set_forceidle(struct uart_omap_port *up)
>> -{
>> - struct omap_uart_port_info *pdata = up->dev->platform_data;
>> -
>> - if (!pdata || !pdata->set_forceidle)
>> - return;
>> -
>> - pdata->set_forceidle(up->dev);
>> -}
>> -
>> -static void serial_omap_set_noidle(struct uart_omap_port *up)
>> -{
>> - struct omap_uart_port_info *pdata = up->dev->platform_data;
>> -
>> - if (!pdata || !pdata->set_noidle)
>> - return;
>> -
>> - pdata->set_noidle(up->dev);
>> -}
>> -
>> static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
>> {
>> struct omap_uart_port_info *pdata = up->dev->platform_data;
>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>> serial_out(up, UART_IER, up->ier);
>> }
>>
>> - serial_omap_set_forceidle(up);
>> -
>> pm_runtime_mark_last_busy(up->dev);
>> pm_runtime_put_autosuspend(up->dev);
>> }
>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>>
>> pm_runtime_get_sync(up->dev);
>> serial_omap_enable_ier_thri(up);
>> - serial_omap_set_noidle(up);
>
> this patch is changing behavior. Currently driver has:
>
> start_tx()
> get_sync()
> set_noidle()
> put_autosuspend()
>
> ....
>
> stop_tx()
> get_sync()
> set_force_idle()
> put_autosuspend()
>
> with this change, you will have:
>
> start_tx()
> get_sync()
> set_noidle()
> put_autosuspend()
> set_force_idle()
>
> this in itself might be enough to go back to corrupted serial if
> put_autosuspend is so quick that set_force_idle() is called before all
> data has been shifted out of FIFO and through the UART lines.
>
Really. Infact the old sequence was buggy because you are setting
force_idle even before suspending the device. And that force idle
isn't really force idle but setting ip to smart idle. Just look at
what serial.c
> Before doing this, you should at least test that removing
> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
> start_tx() and removing pm_runtime_get_sync() from stop_tx() works fine.
>
Seems to work for me with above changes as well. Can you please
try out and see if you see any issue. I doubt you will...
> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
> there is still the regression of UART never being wakeup capable.
>
You are mixing the stuff here. The subject is correct.
> I wonder what are your ideas to sort that part out, I mean, how do you
> plan to implement ->set_wake() for the tty port ?
>
That is not related to module idle modes. It is for ioring and that
needs pin control support. As you can see the patch doesn't touch
omap_uart_enable_wakeup(). Thats needs pincontrol support so that
it can be made work with DT. Today hwmod handles that for you with
pin information provided by platform code.
> One last comment, to avoid conflicts, it'd be better to split driver
> part from removal of platform_data function pointer initialization, so
> that we can send driver changes in one merge window and the
> platform_data part in another.
>
Agree. We can split the patch once everybody is ok with it.
Regards
Santosh
next prev parent reply other threads:[~2013-02-15 13:19 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-15 12:08 [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver Santosh Shilimkar
2013-02-15 12:57 ` Felipe Balbi
2013-02-15 13:19 ` Santosh Shilimkar [this message]
2013-02-15 13:34 ` Felipe Balbi
2013-02-15 13:43 ` Santosh Shilimkar
2013-02-15 13:45 ` a0131647
2013-02-18 6:28 ` Bedia, Vaibhav
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=511E35D8.1030109@ti.com \
--to=santosh.shilimkar@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox