OMAP2+: UART: Modify omap_uart_can_sleep function

Move the errata handling mechanism from serial.c to omap-serial file and utilise the same func in driver file.

Errata i202, i291 are moved to be handled with omap-serial Moving the errata macro from serial.c file to driver header file as from on errata will be handled in driver file itself.

Corrected errata id from chapter reference 2.15 to errata id i291.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

Quoted 11 lines · Govindraj.R

Move the errata handling mechanism from serial.c to omap-serial file and utilise the same func in driver file.

Errata i202, i291 are moved to be handled with omap-serial Moving the errata macro from serial.c file to driver header file as from on errata will be handled in driver file itself.

Corrected errata id from chapter reference 2.15 to errata id i291.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

The errata handling for i291 is moved from device code to driver code but it's functionality is also changed (but not described.)

In the current mainline code, The workaround for i291 is done whenever UART clocks are about to be cut. ?When the clocks are (re)enabled, the device is put back into no-idle.

This patch puts the device into force-idle just before the idle/suspend transistion, but never puts it back into no-idle. ?So after the first idle/suspend transition, the IP remains in force-idle forever. ?I don't think that's what we want.

From: Deepak K <[email protected]>

The following UART parameters are defined within the UART driver:

1). Whether the UART uses DMA (dma_enabled), by default set to 0 2). The size of dma buffer (set to 4096 bytes)
3). The time after which the dma should stop if no more data is received. 4). The auto suspend delay that will be passed for pm_runtime_autosuspend
where uart will be disabled after timeout

Different UARTs may be used for different purpose such as the console, for interfacing bluetooth chip, for interfacing to a modem chip, etc. Therefore, it is necessary to be able to customize the above settings for a given board on a per UART basis.

This change allows these parameters to be configured from the board file and allows the parameters to be configured for each UART independently.

If a board does not define its own custom parameters for the UARTs, then use the default parameters in the structure "omap_serial_default_info". The default parameters are defined to be the same as the current settings in the UART driver to avoid breaking the UART for any cuurnelty supported boards. By default, make all boards use the default UART parameters.

Signed-off-by Deepak K <[email protected]>

Signed-off-by Jon Hunter <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---
This patch is derived and reworked from a custom 2.6.35 kernel Available here:
http://git.omapzoom.org/?p=kernel/omap.git;
a=commitdiff;h=584ef316542f77312be7ba96a0f3013c8f64852b; hp=7233a76cb362c0fc603f773274159adff91d3513

 arch/arm/mach-omap2/board-n8x0.c              |    6 +-
 arch/arm/mach-omap2/serial.c                  |   56 ++++++++++++++++++++----
 arch/arm/plat-omap/include/plat/omap-serial.h |    7 ++-
 arch/arm/plat-omap/include/plat/serial.h      |    5 ++-
 drivers/tty/serial/omap-serial.c              |    8 +--
 5 files changed, 61 insertions(+), 21 deletions(-)

arch/arm/mach-omap2/board-n8x0.c +3 -3

index e11f0c5..3408726 100644
@@ -656,15 +656,15 @@ static inline void board_serial_init(void)
 	bdata.pads_cnt = 0;
 
 	bdata.id = 0;
-	omap_serial_init_port(&bdata);
+	omap_serial_init_port(&bdata, NULL);
 
 	bdata.id = 1;
-	omap_serial_init_port(&bdata);
+	omap_serial_init_port(&bdata, NULL);
 
 	bdata.id = 2;
 	bdata.pads = serial2_pads;
 	bdata.pads_cnt = ARRAY_SIZE(serial2_pads);
-	omap_serial_init_port(&bdata);
+	omap_serial_init_port(&bdata, NULL);
 }
 
 #else

arch/arm/mach-omap2/serial.c +4 -5

index 0731575..78f7051 100644
@@ -43,17 +43,29 @@
 #include "mux.h"
 
 /*
- * NOTE: By default the serial timeout is disabled as it causes lost characters
- * over the serial ports. This means that the UART clocks will stay on until
- * disabled via sysfs. This also causes that any deeper omap sleep states are
- * blocked. 
+ * NOTE: By default the serial auto_suspend timeout is disabled as it causes
+ * lost characters over the serial ports. This means that the UART clocks will
+ * stay on until power/autosuspend_delay is set for the uart from sysfs.
+ * This also causes that any deeper omap sleep states are blocked.
  */
-#define DEFAULT_TIMEOUT 0

arch/arm/mach-omap2/serial.c +7 -0

+		.dma_rx_buf_size = DEFAULT_RXDMA_BUFSIZE,
+		.dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
+		.autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
+	},
+};
+
 static int uart_idle_hwmod(struct omap_device *od)
 {
 	omap_hwmod_idle(od->hwmods[0]);

@@ -298,6 +310,7 @@ core_initcall(omap_serial_early_init);
 /**
  * omap_serial_init_port() - initialize single serial port
  * @bdata: port specific board data pointer
+ * @info: platform specific data pointer
  *
  * This function initialies serial driver for given port only.
  * Platforms can call this function instead of omap_serial_init()

Quoted 24 lines · Govindraj.R

From: Deepak K <[email protected]>

The following UART parameters are defined within the UART driver:

1). Whether the UART uses DMA (dma_enabled), by default set to 0 2). The size of dma buffer (set to 4096 bytes)
3). The time after which the dma should stop if no more data is received. 4). The auto suspend delay that will be passed for pm_runtime_autosuspend ? ? where uart will be disabled after timeout

Different UARTs may be used for different purpose such as the console, for interfacing bluetooth chip, for interfacing to a modem chip, etc. Therefore, it is necessary to be able to customize the above settings for a given board on a per UART basis.

This change allows these parameters to be configured from the board file and allows the parameters to be configured for each UART independently.

If a board does not define its own custom parameters for the UARTs, then use the default parameters in the structure "omap_serial_default_info". The default parameters are defined to be the same as the current settings in the UART driver to avoid breaking the UART for any cuurnelty supported boards. By default, make all boards use the default UART parameters.

Quoted 3 lines

Signed-off-by Deepak K <[email protected]>

Signed-off-by Jon Hunter <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

A couple minor comments below...

Quoted 59 lines

---
This patch is derived and reworked from a custom 2.6.35 kernel Available here:
http://git.omapzoom.org/?p=kernel/omap.git;
a=commitdiff;h=584ef316542f77312be7ba96a0f3013c8f64852b; hp=7233a76cb362c0fc603f773274159adff91d3513

?arch/arm/mach-omap2/board-n8x0.c ? ? ? ? ? ? ?| ? ?6 +-
?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 56 ++++++++++++++++++++----
?arch/arm/plat-omap/include/plat/omap-serial.h | ? ?7 ++-
?arch/arm/plat-omap/include/plat/serial.h ? ? ?| ? ?5 ++-
?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ? ?8 +--

?5 files changed, 61 insertions(+), 21 deletions(-)

arch/arm/mach-omap2/board-n8x0.c +3 -3

index e11f0c5..3408726 100644
@@ -656,15 +656,15 @@ static inline void board_serial_init(void)
 ? ? ? bdata.pads_cnt = 0;
 
 ? ? ? bdata.id = 0;
- ? ? omap_serial_init_port(&bdata);
+ ? ? omap_serial_init_port(&bdata, NULL);
 
 ? ? ? bdata.id = 1;
- ? ? omap_serial_init_port(&bdata);
+ ? ? omap_serial_init_port(&bdata, NULL);
 
 ? ? ? bdata.id = 2;
 ? ? ? bdata.pads = serial2_pads;
 ? ? ? bdata.pads_cnt = ARRAY_SIZE(serial2_pads);
- ? ? omap_serial_init_port(&bdata);
+ ? ? omap_serial_init_port(&bdata, NULL);
 ?}
 

?#else

arch/arm/mach-omap2/serial.c +0 -0

index 0731575..78f7051 100644
@@ -43,17 +43,29 @@

?#include "mux.h"

?/*
- * NOTE: By default the serial timeout is disabled as it causes lost characters
- * over the serial ports. This means that the UART clocks will stay on until
- * disabled via sysfs. This also causes that any deeper omap sleep states are
- * blocked.
+ * NOTE: By default the serial auto_suspend timeout is disabled as it causes
+ * lost characters over the serial ports. This means that the UART clocks will
+ * stay on until power/autosuspend_delay is set for the uart from sysfs.
+ * This also causes that any deeper omap sleep states are blocked. ? */

arch/arm/mach-omap2/serial.c +1 -1

-#define DEFAULT_TIMEOUT 0
+#define DEFAULT_AUTOSUSPEND_DELAY ? ?-1

?#define MAX_UART_HWMOD_NAME_LEN ? ? ? ? ? ? ?16

arch/arm/mach-omap2/serial.c line 60

Quoted 6 lines

?static u8 num_uarts;

arch/arm/mach-omap2/serial.c +4 -0

+#define DEFAULT_RXDMA_TIMEOUT ? ? ? ? ? ? ? ?1 ? ? ? /* RX DMA polling rate (us) */
+#define DEFAULT_RXDMA_BUFSIZE ? ? ? ? ? ? ? ?4096 ? ?/* RX DMA buffer size */
+
+static struct omap_uart_port_info omap_serial_default_info[] = {

This could be __initdata

Quoted 2 lines

+ ? ? {
+ ? ? ? ? ? ? .dma_enabled ? ?= 0,

This field is a bool, use 'false' instead of 0.

Quoted 22 lines

+ ? ? ? ? ? ? .dma_rx_buf_size = DEFAULT_RXDMA_BUFSIZE,
+ ? ? ? ? ? ? .dma_rx_timeout = DEFAULT_RXDMA_TIMEOUT,
+ ? ? ? ? ? ? .autosuspend_timeout = DEFAULT_AUTOSUSPEND_DELAY,
+ ? ? },

arch/arm/mach-omap2/serial.c +2 -0

+};
+

?static int uart_idle_hwmod(struct omap_device *od) ?{
? ? ? omap_hwmod_idle(od->hwmods[0]);

arch/arm/mach-omap2/serial.c +0 -0

@@ -298,6 +310,7 @@ core_initcall(omap_serial_early_init);

?/**
? * omap_serial_init_port() - initialize single serial port ? * @bdata: port specific board data pointer
+ * @info: platform specific data pointer
? *
? * This function initialies serial driver for given port only. ? * Platforms can call this function instead of omap_serial_init()

arch/arm/mach-omap2/serial.c +0 -0

@@ -306,7 +319,8 @@ core_initcall(omap_serial_early_init);
 ? * Don't mix calls to omap_serial_init_port() and omap_serial_init(),

? * use only one of the two.
? */

arch/arm/mach-omap2/serial.c core_initcall(omap_serial_early_init);

Quoted 3 lines

arch/arm/mach-omap2/serial.c +2 -1

-void __init omap_serial_init_port(struct omap_board_data *bdata)
+void __init omap_serial_init_port(struct omap_board_data *bdata,
+ ? ? ? ? ? ? ? ? ? ? struct omap_uart_port_info *info)

alignment. ?2nd argument should align with 1st

Quoted 7 lines

?{
? ? ? struct omap_hwmod *oh;
? ? ? struct platform_device *pdev;

arch/arm/mach-omap2/serial.c +0 -0

@@ -325,6 +339,9 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
 ? ? ? if (!oh)
 ? ? ? ? ? ? ? return;
 

arch/arm/mach-omap2/serial.c void __init omap_serial_init_port(struct omap_board_data *bdata) · line 342

Quoted 1 line

arch/arm/mach-omap2/serial.c +1 -0

+ ? ? if (info == NULL)

if (!info)

there's another one of these elsewhere in the patch too.

In suspend path the console_lock is taken by uart_port_suspend however when no_console_suspend is used console_lock is not taken.

During system wide suspend omap_pwr_domain hooks cut all clocks that are left enabled. So its unsafe to proceed printing after clocks are cut by pwr_domain hooks.

Also pm_runtime will be disabled after dpm_suspend devices happens.

So buffer all prints in suspend path by taking console_lock and print them back safely after power domain hooks re-enable clocks back.

Use CONFIG_SERIAL_OMAP_CONSOLE macro check to take console_lock since console ops are available only if omap console is defined. omap-serial can be built as module without console support.

Quoted 6 lines · Govindraj.R

In suspend path the console_lock is taken by uart_port_suspend however when no_console_suspend is used console_lock is not taken.

During system wide suspend omap_pwr_domain hooks cut all clocks that are left enabled. So its unsafe to proceed printing after clocks are cut by pwr_domain hooks.

As I've mentioned in previous reviews, when no_console_suspend is enabled, the user has explicitly requested console output during suspend. ?In order to support that, we should not be cutting clocks at all in that mode.

One way to address this would be to just disable runtime PM in the ->prepare method of the driver if no_console_suspend is enabled.

Quoted 14 lines · Kevin Hilman

Quoted 6 lines · Govindraj.R

In suspend path the console_lock is taken by uart_port_suspend however when no_console_suspend is used console_lock is not taken.

During system wide suspend omap_pwr_domain hooks cut all clocks that are left enabled. So its unsafe to proceed printing after clocks are cut by pwr_domain hooks.

As I've mentioned in previous reviews, when no_console_suspend is enabled, the user has explicitly requested console output during suspend. ?In order to support that, we should not be cutting clocks at all in that mode.

Quoted 3 lines

One way to address this would be to just disable runtime PM in the ->prepare method of the driver if no_console_suspend is enabled.

Okay fine exploring this option, right API's would be to use pm_runtime_forbid/allow.

<<SNIP>>

(quoted diff) +11 -0

+static int serial_omap_runtime_prepare(struct device *dev)
+{
+       if (!console_suspend_enabled)
+               pm_runtime_forbid(dev);
+
+       return 0;
+}
+
+static void serial_omap_runtime_complete(struct device *dev)
+{
+       if (!console_suspend_enabled)

But to either use runtime forbid or disable we have ensure that power_domain hooks don't go ahead and disable
the clocks with omap_device_idle as *runtime forbid or disable will not set runtime_status to RPM_SUSPENDED*
and will stay in RPM_ACTIVE if we call runtime disable or forbid from active state.

in power_domain hooks we just check the pm_runtime_status_suspended this will be false even if
we do runtime disable/forbid and it will cut uart clocks always.

So we may need below check also:

arch/arm/plat-omap/omap_device.c +0 -0

index 26aee5c..286a534 100644

Quoted 15 lines · Govindraj

Quoted 14 lines · Kevin Hilman

Quoted 6 lines · Govindraj.R

In suspend path the console_lock is taken by uart_port_suspend however when no_console_suspend is used console_lock is not taken.

During system wide suspend omap_pwr_domain hooks cut all clocks that are left enabled. So its unsafe to proceed printing after clocks are cut by pwr_domain hooks.

As I've mentioned in previous reviews, when no_console_suspend is enabled, the user has explicitly requested console output during suspend. ?In order to support that, we should not be cutting clocks at all in that mode.

Quoted 6 lines

Quoted 3 lines

One way to address this would be to just disable runtime PM in the ->prepare method of the driver if no_console_suspend is enabled.

Okay fine exploring this option, right API's would be to use pm_runtime_forbid/allow.

Yes.

Quoted 16 lines

<<SNIP>>

(quoted diff) +14 -0

+static int serial_omap_runtime_prepare(struct device *dev)
+{
+ ? ? ? if (!console_suspend_enabled)
+ ? ? ? ? ? ? ? pm_runtime_forbid(dev);
+
+ ? ? ? return 0;
+}
+
+static void serial_omap_runtime_complete(struct device *dev)
+{
+ ? ? ? if (!console_suspend_enabled)
+ ? ? ? ? ? ? ? pm_runtime_allow(dev);
+}
+

Quoted 7 lines

?static const struct dev_pm_ops serial_omap_dev_pm_ops = {
? ? ? ? SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume) ? ? ? ? SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? serial_omap_runtime_resume, NULL)
+ ? ? ? .prepare = serial_omap_runtime_prepare,
+ ? ? ? .complete = ?serial_omap_runtime_complete,
?};

OK, but please add comments to these functions about exactly why they are needed.

Quoted 7 lines

But to either use runtime forbid or disable we have ensure that power_domain hooks don't go ahead and disable
the clocks with omap_device_idle as *runtime forbid or disable will not set runtime_status to RPM_SUSPENDED*
and will stay in RPM_ACTIVE if we call runtime disable or forbid from active state.

Correct.

Quoted 11 lines

in power_domain hooks we just check the pm_runtime_status_suspended this will be false even if
we do runtime disable/forbid and it will cut uart clocks always.

So we may need below check also:

arch/arm/plat-omap/omap_device.c +0 -0

index 26aee5c..286a534 100644

arch/arm/plat-omap/omap_device.c static int _od_suspend_noirq(struct device *dev) · line 599

Quoted 9 lines

arch/arm/plat-omap/omap_device.c +2 -1

@@ -592,7 +592,8 @@ static int _od_suspend_noirq(struct device *dev)
 
 ? ? ? ? ret = pm_generic_suspend_noirq(dev);
 
- ? ? ? if (!ret && !pm_runtime_status_suspended(dev)) {
+ ? ? ? if (!ret && pm_runtime_enabled(dev) &&
+ ? ? ? ? ? ? ? ? ? ? ? !pm_runtime_status_suspended(dev)) {
 ? ? ? ? ? ? ? ? if (pm_generic_runtime_suspend(dev) == 0) {
 ? ? ? ? ? ? ? ? ? ? ? ? omap_device_idle(pdev);
 ? ? ? ? ? ? ? ? ? ? ? ? od->flags |= OMAP_DEVICE_SUSPENDED;

This isn't right either because devices that may not yet be initialized (or loaded) may not have runtime PM enabled, so those devices may not be properly idled.

We have an omap_device API to disable this feature at the PM domain level:
omap_device_disable_idle_on_suspend(). ?All you should have to do is to use this API in the device init code on the console UART if no_console_suspend has been enabled.

For the early console probing we had avoided hwmod reset and idling and uart was idled using hwmod API and enabled back using omap_device API after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and use runtime API to enable back UART.

Signed-off-by Govindraj.R <[email protected]>

Quoted 8 lines · Govindraj.R

For the early console probing we had avoided hwmod reset and idling and uart was idled using hwmod API and enabled back using omap_device API after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and use runtime API to enable back UART.

Signed-off-by Govindraj.R <[email protected]>

Now that the driver is using runtime PM. ?Why do we still need HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?

The comment in the code says:

? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? * 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 ? ? ? ? ? ? ? ? */

This was true when using the 8250 driver because it was not using runtime PM so could not know how to (re)enable the device.

However, since the driver is now runtime PM adapted, any device access should be contained within a runtime PM get/put block, so there should no longer be a reason not allow the IP blocks to be reset during boot.

Quoted 25 lines · Kevin Hilman

Quoted 8 lines · Govindraj.R

For the early console probing we had avoided hwmod reset and idling and uart was idled using hwmod API and enabled back using omap_device API after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and use runtime API to enable back UART.

Signed-off-by Govindraj.R <[email protected]>

Now that the driver is using runtime PM. ?Why do we still need HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?

The comment in the code says:

? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? * 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 ? ? ? ? ? ? ? ? */

This was true when using the 8250 driver because it was not using runtime PM so could not know how to (re)enable the device.

Quoted 4 lines

However, since the driver is now runtime PM adapted, any device access should be contained within a runtime PM get/put block, so there should no longer be a reason not allow the IP blocks to be reset during boot.

Forgot to add, this is still needed for
earlyprintk(CONFIG_EARLY_PRINTK) use case,

The initial boot prints until a console driver is available is from "arch/arm/kernel/early_printk.c" which does a tx on uart console and relies on configuration from bootloader.

during bootup earlyprink does a tx on uart console and if uart driver is not available yet
uart reset or idle done by hwmod layer can cause boot failures.

--> put_char from earlyprintk.c

     --> reset/idle from hwmod layer
          --> put_char from earlyprintk.c

So console_uart reset or clock gating must be done only after uart driver is available or be prevented using these available hwmod_flags.

Quoted 26 lines · Govindraj

Quoted 25 lines · Kevin Hilman

Quoted 8 lines · Govindraj.R

For the early console probing we had avoided hwmod reset and idling and uart was idled using hwmod API and enabled back using omap_device API after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and use runtime API to enable back UART.

Signed-off-by Govindraj.R <[email protected]>

Now that the driver is using runtime PM. ?Why do we still need HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?

The comment in the code says:

? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? * 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 ? ? ? ? ? ? ? ? */

This was true when using the 8250 driver because it was not using runtime PM so could not know how to (re)enable the device.

Quoted 7 lines

Quoted 4 lines

However, since the driver is now runtime PM adapted, any device access should be contained within a runtime PM get/put block, so there should no longer be a reason not allow the IP blocks to be reset during boot.

Forgot to add, this is still needed for
earlyprintk(CONFIG_EARLY_PRINTK) use case,

Ah, right. ?I forgot about that. ?Please update the changelog (and comment in the code) to reflect that.

Quoted 8 lines

The initial boot prints until a console driver is available is from "arch/arm/kernel/early_printk.c" which does a tx on uart console and relies on configuration from bootloader.

during bootup earlyprink does a tx on uart console and if ?uart driver is not available yet
uart reset or idle done by hwmod layer can cause boot failures.

Quoted 7 lines

--> put_char from earlyprintk.c
? ? ?--> reset/idle from hwmod layer
? ? ? ? ? --> put_char from earlyprintk.c

So console_uart reset or clock gating must be done only after uart driver is available or be prevented using these available hwmod_flags.

So why not leave the driver out of it and solve it like the current code does?

The current codes use the hwmod flags, then waits until the UART driver is available (after omap_device_build) and uses omap_hwmod_idle() to do an clean idle of the device.

Notably this is inside a console_lock/console_unlock block so that prints are buffered.

The current code then does an omap_device_enable() to re-enable the device, but you shouldn't need that after the driver is converted to runtime PM.

Quoted 62 lines · Kevin Hilman

Quoted 26 lines · Govindraj

Quoted 25 lines · Kevin Hilman

Quoted 8 lines · Govindraj.R

For the early console probing we had avoided hwmod reset and idling and uart was idled using hwmod API and enabled back using omap_device API after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and use runtime API to enable back UART.

Signed-off-by Govindraj.R <[email protected]>

Now that the driver is using runtime PM. ?Why do we still need HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?

The comment in the code says:

? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? * 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 ? ? ? ? ? ? ? ? */

This was true when using the 8250 driver because it was not using runtime PM so could not know how to (re)enable the device.

Quoted 7 lines

Quoted 4 lines

However, since the driver is now runtime PM adapted, any device access should be contained within a runtime PM get/put block, so there should no longer be a reason not allow the IP blocks to be reset during boot.

Forgot to add, this is still needed for
earlyprintk(CONFIG_EARLY_PRINTK) use case,

Ah, right. ?I forgot about that. ?Please update the changelog (and comment in the code) to reflect that.

Quoted 8 lines

The initial boot prints until a console driver is available is from "arch/arm/kernel/early_printk.c" which does a tx on uart console and relies on configuration from bootloader.

during bootup earlyprink does a tx on uart console and if ?uart driver is not available yet
uart reset or idle done by hwmod layer can cause boot failures.

Quoted 7 lines

--> put_char from earlyprintk.c
? ? ?--> reset/idle from hwmod layer
? ? ? ? ? --> put_char from earlyprintk.c

So console_uart reset or clock gating must be done only after uart driver is available or be prevented using these available hwmod_flags.

So why not leave the driver out of it and solve it like the current code does?

The current codes use the hwmod flags, then waits until the UART driver is available (after omap_device_build) and uses omap_hwmod_idle() to do an clean idle of the device.

Quoted 6 lines

Notably this is inside a console_lock/console_unlock block so that prints are buffered.

The current code then does an omap_device_enable() to re-enable the device, but you shouldn't need that after the driver is converted to runtime PM.

Yes similar approach here, We are not doing hwmod idle until console driver is available, once omap-serial is available from probe doing hwmod_idle* and then get_sync.

hwmod idle in serial.c will still cause problems if ealryprintk tries to print until omap-uart console driver is not available,

as now with rumtime adaptation only driver can enable back clocks.

So have added a function pointer to pdata which calls hwmod_idle implemented in serial.c calling omap_hwmod_idle.

Quoted 70 lines · Govindraj

Quoted 62 lines · Kevin Hilman

Quoted 26 lines · Govindraj

Quoted 25 lines · Kevin Hilman

Quoted 8 lines · Govindraj.R

For the early console probing we had avoided hwmod reset and idling and uart was idled using hwmod API and enabled back using omap_device API after omap_device registration.

Now since we are using runtime API's to enable back uart, move hwmod idling and use runtime API to enable back UART.

Signed-off-by Govindraj.R <[email protected]>

Now that the driver is using runtime PM. ?Why do we still need HWMOD_INIT_NO_IDLE and HWMOD_INIT_NO_RESET?

The comment in the code says:

? ? ? ? ? ? ? ?/*
? ? ? ? ? ? ? ? * 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 ? ? ? ? ? ? ? ? */

This was true when using the 8250 driver because it was not using runtime PM so could not know how to (re)enable the device.

Quoted 7 lines

Quoted 4 lines

However, since the driver is now runtime PM adapted, any device access should be contained within a runtime PM get/put block, so there should no longer be a reason not allow the IP blocks to be reset during boot.

Forgot to add, this is still needed for
earlyprintk(CONFIG_EARLY_PRINTK) use case,

Ah, right. ?I forgot about that. ?Please update the changelog (and comment in the code) to reflect that.

Quoted 8 lines

The initial boot prints until a console driver is available is from "arch/arm/kernel/early_printk.c" which does a tx on uart console and relies on configuration from bootloader.

during bootup earlyprink does a tx on uart console and if ?uart driver is not available yet
uart reset or idle done by hwmod layer can cause boot failures.

Quoted 7 lines

--> put_char from earlyprintk.c
? ? ?--> reset/idle from hwmod layer
? ? ? ? ? --> put_char from earlyprintk.c

So console_uart reset or clock gating must be done only after uart driver is available or be prevented using these available hwmod_flags.

So why not leave the driver out of it and solve it like the current code does?

The current codes use the hwmod flags, then waits until the UART driver is available (after omap_device_build) and uses omap_hwmod_idle() to do an clean idle of the device.

Quoted 6 lines

Notably this is inside a console_lock/console_unlock block so that prints are buffered.

The current code then does an omap_device_enable() to re-enable the device, but you shouldn't need that after the driver is converted to runtime PM.

Quoted 6 lines

Yes similar approach here, We are not doing hwmod idle until console driver is available, once omap-serial is available from probe doing hwmod_idle* and then get_sync.

hwmod idle in serial.c will still cause problems if ealryprintk tries to print until omap-uart console driver is not available,

It will try, but note that current code takes the console_lock() during that time, so those prints will be buffered.

Quoted 1 line

as now with rumtime adaptation only driver can enable back clocks.

I'm not sure why you're calling this "enable back clocks." ?This patch is just trying to decide where to idle the hwmod (that is, disable the clocks.)

Re: only the driver can do it, I can think of at least 2 ways to keep this out of the driver.

1) Use the a custom activate_func in the omap_device pm_lats struct ? to idle the first time.

2) Use a bus notifier so the device init can be notified when the ? real driver is available. ?I think you're probably wanting ? the BUS_NOTIFY_BIND_DRIVER event, which would happen right ? before probe. ?There's also BUS_NOTIFY_BOUND_DRIVER which ? happens right after probe. ? You might actually want to use ? both. ?e.g. ? console_lock(); omap_hwmod_idle() in BIND ? and console_unlock() in 'BOUND'.

Quoted 2 lines

So have added a function pointer to pdata which calls hwmod_idle implemented in serial.c calling omap_hwmod_idle.

Yes, I saw that in the patch, and that's what I don't like.

Fixing up after earlyprintk is not the responsiblity of the driver, and the driver should not know or care whether earlyprintk was or wasn't used before it was loaded. ?This needs to be handled in device init code as it is today.

Kevin

I did some study into the approaches you suggested, Below are my observation, Please correct me if I missed out any thing in my below observation,

Quoted 3 lines

1) Use the a custom activate_func in the omap_device pm_lats struct ? to idle the first time.

same activate func gets called for all uarts, how to determine the activate func. was called for first time for the given uart.

(use od->pdev->dev.name strcmp with uart name for first name and maintain a state machine ? confused)

Then should we have different activate funcs?

Quoted 7 lines

2) Use a bus notifier so the device init can be notified when the ? real driver is available. ?I think you're probably wanting ? the BUS_NOTIFY_BIND_DRIVER event, which would happen right ? before probe. ?There's also BUS_NOTIFY_BOUND_DRIVER which ? happens right after probe. ? You might actually want to use ? both. ?e.g. ? console_lock(); omap_hwmod_idle() in BIND ? and console_unlock() in 'BOUND'.

bus_register_notifier is for all drivers within the bus, omap-uart is registered as platform bus and there a lot more devices that register under platform bus,

So registering the notifier for platform bus will call notifier for all probes withing platform bus,

Is there a way to have notifier per device or given device?

Modify the omap_uart_can_sleep function to check uart is active or not to be used by pm code to enter low power states.

Removing this check can cause console response little sluggish.

However no characters will be lost until uart clocks are gated and woken up using rx-pad. UART interface clocks can be auto gated this can make response on uart slower. This behaviour was observed only on some of OMAP3 boards(beagleboard xm rev c).

Reported-by Tero Kristo <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---
 arch/arm/mach-omap2/serial.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/serial.c +0 -3

index 6725caf..ccf3550 100644
@@ -156,23 +156,20 @@ static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
 
 int omap_uart_can_sleep(void)
 {
-	struct omap_uart_state *uart;
-	int can_sleep = 1;
-

arch/arm/mach-omap2/serial.c +1 -3

+		if (!oh)
 			continue;
-		}
 
-		/* This UART can now safely sleep. */
-		omap_uart_allow_sleep(uart);

Quoted 2 lines · Govindraj.R

Modify the omap_uart_can_sleep function to check uart is active or not to be used by pm code to enter low power states.

Doesn't the driver now control when the UART clocks are gated (using runtime PM autosuspend)?

IMO, this check should be completely removed and the driver should be managing this with the autosuspend timeout.

Quoted 1 line

Removing this check can cause console response little sluggish.

Sluggish in what way?

If the driver is runtime suspended, it should only be sluggish for the first character. ?After that, the autosuspend timeout should prevent it from feeling sluggish.

Quoted 28 lines

However no characters will be lost until uart clocks are gated and woken up using rx-pad. UART interface clocks can be auto gated this can make response on uart slower. This behaviour was observed only on some of OMAP3 boards(beagleboard xm rev c).

Reported-by Tero Kristo <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---
?arch/arm/mach-omap2/serial.c | ? 21 +++++++++------------ ?1 files changed, 9 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/serial.c +2 -6

index 6725caf..ccf3550 100644
@@ -156,23 +156,20 @@ static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
 
 ?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;
+ ? ? struct omap_hwmod *oh;
+ ? ? u8 i, ret = true;
 

arch/arm/mach-omap2/serial.c static void omap_uart_smart_idle_enable(struct omap_uart_state *uart, · line 163

Quoted 4 lines

arch/arm/mach-omap2/serial.c +2 -2

- ? ? ? ? ? ? if (!uart->can_sleep) {
- ? ? ? ? ? ? ? ? ? ? can_sleep = 0;
+ ? ? for (i = 0; i < num_uarts; i++) {
+ ? ? ? ? ? ? oh = omap_uart_hwmod_lookup(i);

This is a heavy operation to add for *every* entry into idle.

Quoted 8 lines · Kevin Hilman

Quoted 2 lines · Govindraj.R

Modify the omap_uart_can_sleep function to check uart is active or not to be used by pm code to enter low power states.

Doesn't the driver now control when the UART clocks are gated (using runtime PM autosuspend)?

Quoted 7 lines

IMO, this check should be completely removed and the driver should be managing this with the autosuspend timeout.

Quoted 1 line

Removing this check can cause console response little sluggish.

Sluggish in what way?

response is slower like when we type something or cat debugfs/pm_count see things little slower on console, there is no character loss.

Happens even though we have not set the autosuspend timeout and uart clocks are active, which basically means allowing mpu to enter retention keeping uart active.

this delay in response or sluggishness is not there on my 3430SDP or 3630zoom board but I was able to see this behavior on a beagle board(xm rev c).

retaining this uart_can_sleep check in omap3_can_sleep ensures a better console user experience. (not allowing mpu to enter retention until uart clocks are cut)

Quoted 17 lines · Govindraj

Quoted 8 lines · Kevin Hilman

Quoted 2 lines · Govindraj.R

Modify the omap_uart_can_sleep function to check uart is active or not to be used by pm code to enter low power states.

Doesn't the driver now control when the UART clocks are gated (using runtime PM autosuspend)?

Quoted 7 lines

IMO, this check should be completely removed and the driver should be managing this with the autosuspend timeout.

Quoted 1 line

Removing this check can cause console response little sluggish.

Sluggish in what way?

Quoted 6 lines

response is slower like when we type something or cat debugfs/pm_count see things little slower on console, there is no character loss.

Happens even though we have not set the autosuspend timeout and uart clocks are active, which basically means allowing mpu to enter retention keeping uart active.

OK, I see now.

Quoted 3 lines

this delay in response or sluggishness is not there on my 3430SDP or 3630zoom board but I was able to see this behavior on a beagle board(xm rev c).

Here's why:

The difference is the powerdomain that the console UART is on for these boards. ?UART1,2 are in CORE, UART2/3 are in PER. ? SDP uses UART1 (CORE), Zoom3 doesn't use OMAP UARTs at all, and Beagle uses UART3 (PER).

Due to a HW sleepdep between MPU and CORE, MPU will not transition until CORE does, which means MPU will not transition until UART 1 & 2 are idle.

On Beagle, the console is ttyO2 (UART3) which is in PER, and since the MPU is free to transition independently of PER, that is what is happening, resulting in slower response time on for any boards that have PER-UART consoles.

Quoted 3 lines

retaining this uart_can_sleep check in omap3_can_sleep ensures a better console user experience. (not allowing mpu to enter retention until uart clocks are cut)

Yes, but obviously comes at the expense of power savings. ?IOW, This is a hard-coded power vs. performance trade off that we are trying to get away from.

So, the root of the problem is that the MPU wakeup latency is causing a "sluggish" console. ?The solution? ?request an MPU wakeup latency constraint.

This is a classic use-case for such a constraint, and the serial driver should have the option of requesting a constraint to prevent the sluggish console. ?The constraint only needs to be held until the auto-suspend delay expires, so should be relased in the ->runtime_suspend() method of the driver.

This constraint needs to be configurable, probably from the board file, so that it is optional, and so users who don't care about sluggish consoles (or non-console UART users who don't care about response time) have the option of preferring power savings over UART responsiveness.

As a reference, the i2c driver is currently doing something similar in that it request an MPU constraint to prevent the MPU from going into retention/off while waiting for an i2c interrupt to arrive.

Quoted 9 lines

Yes, but obviously comes at the expense of power savings. ?IOW, This is a hard-coded power vs. performance trade off that we are trying to get away from.

So, the root of the problem is that the MPU wakeup latency is causing a "sluggish" console. ?The solution? ?request an MPU wakeup latency constraint.

Okay, Will explore this.

Quoted 11 lines

This is a classic use-case for such a constraint, and the serial driver should have the option of requesting a constraint to prevent the sluggish console. ?The constraint only needs to be held until the auto-suspend delay expires, so should be relased in the ->runtime_suspend() method of the driver.

This constraint needs to be configurable, probably from the board file, so that it is optional, and so users who don't care about sluggish consoles (or non-console UART users who don't care about response time) have the option of preferring power savings over UART responsiveness.

Quoted 4 lines

As a reference, the i2c driver is currently doing something similar in that it request an MPU constraint to prevent the MPU from going into retention/off while waiting for an i2c interrupt to arrive.

Thanks, will check and try to use the mpu constraints

--
Govindraj.R.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html