| Message ID | 515E7876.3080407@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 05 April 2013 12:38 PM, Sourav Poddar wrote: > Hi Kevin, > On Wednesday 03 April 2013 11:18 PM, Kevin Hilman wrote: >> Sourav Poddar<sourav.poddar@ti.com> writes: >> >>> Hi Kevin, >>> On Wednesday 20 March 2013 05:36 PM, Sourav Poddar wrote: >>>> Realised the list to whom the patch was send got dropped. Ccing >>>> them all.. >>>> On Wednesday 20 March 2013 05:18 PM, Sourav Poddar wrote: >>>>> Hi Kevin, >>>>> On Tuesday 19 March 2013 12:24 AM, Kevin Hilman wrote: >>>>>> Sourav Poddar<sourav.poddar@ti.com> writes: >>>>>> >>>>>>> With dt boot, uart wakeup after suspend is non functional on >>>>>>> omap4/5 while using >>>>>>> "no_console_suspend" in the bootargs. With "no_console_suspend" >>>>>>> used, od->flags >>>>>>> should be ORed with "OMAP_DEVICE_NO_IDLE_ON_SUSPEND", thereby not >>>>>>> allowing the console >>>>>>> to idle in the suspend path. For non-dt case, this was taken care >>>>>>> by platform data. >>>>>>> >>>>>>> Tested on omap5430evm, omap4430sdp. >>>>>>> >>>>>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com> >>>>>>> Cc: Felipe Balbi<balbi@ti.com> >>>>>>> Cc: Rajendra nayak<rnayak@ti.com> >>>>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com> >>>>>> This patch creates a dependency between omap_device (generic, >>>>>> device-independent code) and a specific driver (UART.) >>>>>> >>>>>> If you need to do something like this that's DT boot specific, then >>>>>> we probably need some late initcall in serial.c to handle this. >>>>>> It does >>>>>> not belong in omap_device. >>>>>> >>>>> The following function "omap_device_disable_idle_on_suspend(pdev)" >>>>> should only >>>>> be called once the omap device has been build, which in the case of >>>>> device tree is >>>>> done in omap_device.c file. Moreover, the above call should be >>>>> executed conditionally >>>>> and should depend on the following two parameter. >>>>> >>>>> [1] a. Whether "no_console_suspend" is set and >>>>> b. the device build is a console uart. >>>>> >>>>> When I look closely into the serial.c file, I realised that >>>>> "core_initcall(omap_serial_early_init)" gets called irrespective >>>>> of dt/non dt boot and will take care of most of the stuff(checking >>>>> whether >>>>> "no_console_suspend" is used and which uart is used as a console >>>>> uart) which the >>>>> $subject patch is proposing. >>>>> >>>>> But the problem is that we need to exchange the parsed information >>>>> from serial.c to the omap_device file for the condtional execution of >>>>> "omap_device_disable_idle_on_suspend" >>>>> >>>>> In this case, >>>>> from "serial.c" we need >>>>> 1. no_console_suspend = true >>>>> 2. strcpy(console_name, oh_name), where oh_name corresponds to >>>>> the console uart. >>>>> >>>>> then in "omap_device.c" do >>>>> if (no_console_suspend&& !strcmp(oh->name, console_name)) >>>>> omap_device_disable_idle_on_suspend(pdev); >>>>> >>>>> Please correct if I am understanding it incorrectly. >>>>> >>>>> If the above understanding looks good to you, is there a way we can >>>>> make this >>>>> exchange of information happen between serial.c and omap_device.c file? >>> Any input on this? >>> As I explained earlier, that there is a need to parse information in >>> serial.c and use that in >>> omap_device.c only after the device is build. >> As I explained earlier, any device specific hacks inside omap_device >> should be a red flag that something has gone wrong. >> >> How about fixing the UART driver/core to not runtime suspend if >> no_console_suspend is given? >> >> Then we can get rid of this no_idle_on_suspend hack all together since >> UART is the only remaining user. >> > > Yes, that can be done. > > I cooked up an experimental patch, based on your above suggestion. The patch is > inlined along with the mail. I tested the patch and it resolves > the issue. > Would be good to start a new thread. > Depending on whether no_console_suspend is used or not, we can set a variable > which can be used as a condition in runtime api's in serial driver. > > The information whether "no_console_suspend" is used or not is parsed in > serial.c file, and this need to be communicated to the driver layer. > WHY ? Serial core already knows about 'no_console_suspend' and thats used to manage console lock. > In my experimental patch below, I have used an extern variable > "force_console_suspend_disable". > > Do you see a better way of handling this without using extern? > > --------- > From: Sourav Poddar <sourav.poddar@ti.com> > Date: Wed, 13 Mar 2013 20:32:36 +0530 > Subject: [PATCH] arm: mach-omap2: prevent UART console idle on suspend while using "no_console_suspend" > > With dt boot, uart wakeup after suspend is non functional while using > "no_console_suspend" in the bootargs. With "no_console_suspend" used, od->flags > should be ORed with "OMAP_DEVICE_NO_IDLE_ON_SUSPEND", thereby not allowing the console > to idle in the suspend path. > > Tested on omap5430evm, omap4430sdp. > > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com> > --- > arch/arm/mach-omap2/serial.c | 6 ++++-- > drivers/tty/serial/omap-serial.c | 6 +++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c > index f660156..427c407 100644 > --- a/arch/arm/mach-omap2/serial.c > +++ b/arch/arm/mach-omap2/serial.c > @@ -51,7 +51,7 @@ > #define DEFAULT_AUTOSUSPEND_DELAY -1 > > #define MAX_UART_HWMOD_NAME_LEN 16 > - > +int force_console_suspend_disable; > struct omap_uart_state { > int num; > > @@ -207,8 +207,10 @@ static int __init omap_serial_early_init(void) > uart_name, uart->num); > } > > - if (cmdline_find_option("no_console_suspend")) > + if (cmdline_find_option("no_console_suspend")) { > + force_console_suspend_disable = 1; > no_console_suspend = true; > + } > > /* > * omap-uart can be used for earlyprintk logs > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index 08332f3..395ade8 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -43,6 +43,7 @@ > #include <linux/pinctrl/consumer.h> > #include <linux/platform_data/serial-omap.h> > > +extern int force_console_suspend_disable; This is no good. You need to use the information from core serial layer > #define OMAP_MAX_HSUART_PORTS 6 > > #define UART_BUILD_REVISION(x, y) (((x) << 8) | (y)) > @@ -1582,7 +1583,7 @@ static int serial_omap_runtime_suspend(struct device *dev) > struct uart_omap_port *up = dev_get_drvdata(dev); > struct omap_uart_port_info *pdata = dev->platform_data; > > - if (!up) > + if (!up || force_console_suspend_disable) > return -EINVAL; > > if (!pdata) > @@ -1614,6 +1615,9 @@ static int serial_omap_runtime_resume(struct device *dev) > > int loss_cnt = serial_omap_get_context_loss_count(up); > > + if (force_console_suspend_disable) > + return -EINVAL; > + > if (loss_cnt < 0) { > dev_err(dev, "serial_omap_get_context_loss_count failed : %d\n", > loss_cnt); -- 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
Hi Santosh, On Friday 05 April 2013 12:45 PM, Santosh Shilimkar wrote: > On Friday 05 April 2013 12:38 PM, Sourav Poddar wrote: >> Hi Kevin, >> On Wednesday 03 April 2013 11:18 PM, Kevin Hilman wrote: >>> Sourav Poddar<sourav.poddar@ti.com> writes: >>> >>>> Hi Kevin, >>>> On Wednesday 20 March 2013 05:36 PM, Sourav Poddar wrote: >>>>> Realised the list to whom the patch was send got dropped. Ccing >>>>> them all.. >>>>> On Wednesday 20 March 2013 05:18 PM, Sourav Poddar wrote: >>>>>> Hi Kevin, >>>>>> On Tuesday 19 March 2013 12:24 AM, Kevin Hilman wrote: >>>>>>> Sourav Poddar<sourav.poddar@ti.com> writes: >>>>>>> >>>>>>>> With dt boot, uart wakeup after suspend is non functional on >>>>>>>> omap4/5 while using >>>>>>>> "no_console_suspend" in the bootargs. With "no_console_suspend" >>>>>>>> used, od->flags >>>>>>>> should be ORed with "OMAP_DEVICE_NO_IDLE_ON_SUSPEND", thereby not >>>>>>>> allowing the console >>>>>>>> to idle in the suspend path. For non-dt case, this was taken care >>>>>>>> by platform data. >>>>>>>> >>>>>>>> Tested on omap5430evm, omap4430sdp. >>>>>>>> >>>>>>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com> >>>>>>>> Cc: Felipe Balbi<balbi@ti.com> >>>>>>>> Cc: Rajendra nayak<rnayak@ti.com> >>>>>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com> >>>>>>> This patch creates a dependency between omap_device (generic, >>>>>>> device-independent code) and a specific driver (UART.) >>>>>>> >>>>>>> If you need to do something like this that's DT boot specific, then >>>>>>> we probably need some late initcall in serial.c to handle this. >>>>>>> It does >>>>>>> not belong in omap_device. >>>>>>> >>>>>> The following function "omap_device_disable_idle_on_suspend(pdev)" >>>>>> should only >>>>>> be called once the omap device has been build, which in the case of >>>>>> device tree is >>>>>> done in omap_device.c file. Moreover, the above call should be >>>>>> executed conditionally >>>>>> and should depend on the following two parameter. >>>>>> >>>>>> [1] a. Whether "no_console_suspend" is set and >>>>>> b. the device build is a console uart. >>>>>> >>>>>> When I look closely into the serial.c file, I realised that >>>>>> "core_initcall(omap_serial_early_init)" gets called irrespective >>>>>> of dt/non dt boot and will take care of most of the stuff(checking >>>>>> whether >>>>>> "no_console_suspend" is used and which uart is used as a console >>>>>> uart) which the >>>>>> $subject patch is proposing. >>>>>> >>>>>> But the problem is that we need to exchange the parsed information >>>>>> from serial.c to the omap_device file for the condtional execution of >>>>>> "omap_device_disable_idle_on_suspend" >>>>>> >>>>>> In this case, >>>>>> from "serial.c" we need >>>>>> 1. no_console_suspend = true >>>>>> 2. strcpy(console_name, oh_name), where oh_name corresponds to >>>>>> the console uart. >>>>>> >>>>>> then in "omap_device.c" do >>>>>> if (no_console_suspend&& !strcmp(oh->name, console_name)) >>>>>> omap_device_disable_idle_on_suspend(pdev); >>>>>> >>>>>> Please correct if I am understanding it incorrectly. >>>>>> >>>>>> If the above understanding looks good to you, is there a way we can >>>>>> make this >>>>>> exchange of information happen between serial.c and omap_device.c file? >>>> Any input on this? >>>> As I explained earlier, that there is a need to parse information in >>>> serial.c and use that in >>>> omap_device.c only after the device is build. >>> As I explained earlier, any device specific hacks inside omap_device >>> should be a red flag that something has gone wrong. >>> >>> How about fixing the UART driver/core to not runtime suspend if >>> no_console_suspend is given? >>> >>> Then we can get rid of this no_idle_on_suspend hack all together since >>> UART is the only remaining user. >>> >> Yes, that can be done. >> >> I cooked up an experimental patch, based on your above suggestion. The patch is >> inlined along with the mail. I tested the patch and it resolves >> the issue. >> > Would be good to start a new thread. > I will post a new version based on your comments below. >> Depending on whether no_console_suspend is used or not, we can set a variable >> which can be used as a condition in runtime api's in serial driver. >> >> The information whether "no_console_suspend" is used or not is parsed in >> serial.c file, and this need to be communicated to the driver layer. >> > WHY ? > Serial core already knows about 'no_console_suspend' and thats used > to manage console lock. > True. >> In my experimental patch below, I have used an extern variable >> "force_console_suspend_disable". >> >> Do you see a better way of handling this without using extern? >> >> --------- >> From: Sourav Poddar<sourav.poddar@ti.com> >> Date: Wed, 13 Mar 2013 20:32:36 +0530 >> Subject: [PATCH] arm: mach-omap2: prevent UART console idle on suspend while using "no_console_suspend" >> >> With dt boot, uart wakeup after suspend is non functional while using >> "no_console_suspend" in the bootargs. With "no_console_suspend" used, od->flags >> should be ORed with "OMAP_DEVICE_NO_IDLE_ON_SUSPEND", thereby not allowing the console >> to idle in the suspend path. >> >> Tested on omap5430evm, omap4430sdp. >> >> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com> >> --- >> arch/arm/mach-omap2/serial.c | 6 ++++-- >> drivers/tty/serial/omap-serial.c | 6 +++++- >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c >> index f660156..427c407 100644 >> --- a/arch/arm/mach-omap2/serial.c >> +++ b/arch/arm/mach-omap2/serial.c >> @@ -51,7 +51,7 @@ >> #define DEFAULT_AUTOSUSPEND_DELAY -1 >> >> #define MAX_UART_HWMOD_NAME_LEN 16 >> - >> +int force_console_suspend_disable; >> struct omap_uart_state { >> int num; >> >> @@ -207,8 +207,10 @@ static int __init omap_serial_early_init(void) >> uart_name, uart->num); >> } >> >> - if (cmdline_find_option("no_console_suspend")) >> + if (cmdline_find_option("no_console_suspend")) { >> + force_console_suspend_disable = 1; >> no_console_suspend = true; >> + } >> >> /* >> * omap-uart can be used for earlyprintk logs >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c >> index 08332f3..395ade8 100644 >> --- a/drivers/tty/serial/omap-serial.c >> +++ b/drivers/tty/serial/omap-serial.c >> @@ -43,6 +43,7 @@ >> #include<linux/pinctrl/consumer.h> >> #include<linux/platform_data/serial-omap.h> >> >> +extern int force_console_suspend_disable; > This is no good. You need to use the information from > core serial layer > Ok. Will change. >> #define OMAP_MAX_HSUART_PORTS 6 >> >> #define UART_BUILD_REVISION(x, y) (((x)<< 8) | (y)) >> @@ -1582,7 +1583,7 @@ static int serial_omap_runtime_suspend(struct device *dev) >> struct uart_omap_port *up = dev_get_drvdata(dev); >> struct omap_uart_port_info *pdata = dev->platform_data; >> >> - if (!up) >> + if (!up || force_console_suspend_disable) >> return -EINVAL; >> >> if (!pdata) >> @@ -1614,6 +1615,9 @@ static int serial_omap_runtime_resume(struct device *dev) >> >> int loss_cnt = serial_omap_get_context_loss_count(up); >> >> + if (force_console_suspend_disable) >> + return -EINVAL; >> + >> if (loss_cnt< 0) { >> dev_err(dev, "serial_omap_get_context_loss_count failed : %d\n", >> loss_cnt); -- 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
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c index f660156..427c407 100644 --- a/arch/arm/mach-omap2/serial.c +++ b/arch/arm/mach-omap2/serial.c @@ -51,7 +51,7 @@ #define DEFAULT_AUTOSUSPEND_DELAY -1 #define MAX_UART_HWMOD_NAME_LEN 16 - +int force_console_suspend_disable; struct omap_uart_state { int num; @@ -207,8 +207,10 @@ static int __init omap_serial_early_init(void) uart_name, uart->num); } - if (cmdline_find_option("no_console_suspend")) + if (cmdline_find_option("no_console_suspend")) { + force_console_suspend_disable = 1; no_console_suspend = true; + } /* * omap-uart can be used for earlyprintk logs diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 08332f3..395ade8 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -43,6 +43,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/platform_data/serial-omap.h> +extern int force_console_suspend_disable; #define OMAP_MAX_HSUART_PORTS 6 #define UART_BUILD_REVISION(x, y) (((x) << 8) | (y)) @@ -1582,7 +1583,7 @@ static int serial_omap_runtime_suspend(struct device *dev) struct uart_omap_port *up = dev_get_drvdata(dev); struct omap_uart_port_info *pdata = dev->platform_data; - if (!up) + if (!up || force_console_suspend_disable) return -EINVAL;