[pm-wip/uart][PATCH v3] Serial: Avoid using hwmod lookup using name string

[pm-wip/uart][PATCH v3] Serial: Avoid using hwmod lookup using name string

[Govindraj.R]

[-- Attachment #1: Type: text/plain, Size: 3745 bytes --]

Avoid using hwmod lookup using name string rather
retreive port info using the hwmod class interface.
Aviod populating uart_list in early init phase,
leave the uart_list empty and keep the omap hwmod info
in a seperate uart_oh list and if board file calls
serial init use this uart_oh list info to fill uart_list.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c |   90 ++++++++++++++++++++---------------------
 1 files changed, 44 insertions(+), 46 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 38a74d0..68cbd16 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -55,8 +55,6 @@
  */
 #define DEFAULT_TIMEOUT 0

-#define MAX_UART_HWMOD_NAME_LEN		16
-
 struct omap_uart_state {
 	int num;
 	int can_sleep;
@@ -96,6 +94,12 @@ struct omap_uart_state {
 #endif
 };

+struct uart_oh {
+	struct list_head node;
+	struct omap_hwmod *oh;
+};
+
+static LIST_HEAD(uart_oh_list);
 static LIST_HEAD(uart_list);
 static u8 num_uarts;

@@ -568,52 +572,37 @@ static void serial_out_override(struct uart_port *up, int offset, int value)
 }
 #endif

-void __init omap_serial_early_init(void)
+static int omap_serial_port_init(struct omap_hwmod *oh, void *user)
 {
-	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);
-		oh = omap_hwmod_lookup(oh_name);
-		if (!oh)
-			break;
-
-		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
-		if (WARN_ON(!uart))
-			return;
-
-		uart->oh = oh;
-		uart->num = i++;
-		list_add_tail(&uart->node, &uart_list);
-		num_uarts++;
-
-		/*
-		 * NOTE: omap_hwmod_init() has not yet been called,
-		 *       so no hwmod functions will work yet.
-		 */
+	struct uart_oh *uoh;

-		/*
-		 * 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;
-
-		/*
-		 * Since UART hwmod is idle/enabled inside the
-		 * idle loop, interrupts are already disabled and
-		 * thus no locking is needed.  Since the mutex-based
-		 * locking in hwmod might sleep, allowing locking
-		 * may introduce problems.
-		 */
-		uart->oh->flags |= HWMOD_NO_IDLE_LOCKING;
+	uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
+	if (WARN_ON(!uoh))
+		return -ENOMEM;
+	/*
+	 * 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
+	 */
+	oh->flags |= HWMOD_INIT_NO_IDLE;
+	/*
+	 * Since UART hwmod is idle/enabled inside the
+	 * idle loop, interrupts are already disabled and
+	 * thus no locking is needed.  Since the mutex-based
+	 * locking in hwmod might sleep, allowing locking
+	 * may introduce problems.
+	 */
+	oh->flags |= HWMOD_NO_IDLE_LOCKING;
+	list_add_tail(&uoh->node, &uart_oh_list);
+	uoh->oh = oh;
+	num_uarts++;
+	return 0;
+}

-	} while (1);
+void __init omap_serial_early_init(void)
+{
+	omap_hwmod_for_each_by_class("uart",
+		omap_serial_port_init, NULL);
 }

 /**
@@ -769,7 +758,16 @@ void __init omap_serial_init_port(int port)
 void __init omap_serial_init(void)
 {
 	struct omap_uart_state *uart;
+	struct uart_oh *uoh;
+	int i = 0;

-	list_for_each_entry(uart, &uart_list, node)
+	list_for_each_entry(uoh, &uart_oh_list, node) {
+		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
+		if (WARN_ON(!uart))
+			return;
+		list_add_tail(&uart->node, &uart_list);
+		uart->num = i++;
+		uart->oh = uoh->oh;
 		omap_serial_init_port(uart->num);
+	}
 }
-- 
1.6.3.3

[-- Attachment #2: patch.zip --]
[-- Type: application/x-zip-compressed, Size: 1788 bytes --]

Re: [pm-wip/uart][PATCH v3] Serial: Avoid using hwmod lookup using name string

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> Avoid using hwmod lookup using name string rather
> retreive port info using the hwmod class interface.
> Aviod populating uart_list in early init phase,
> leave the uart_list empty and keep the omap hwmod info
> in a seperate uart_oh list and if board file calls
> serial init use this uart_oh list info to fill uart_list.
>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
>  arch/arm/mach-omap2/serial.c |   90 ++++++++++++++++++++---------------------
>  1 files changed, 44 insertions(+), 46 deletions(-)

Hi Govindraj,

Thanks for this update.

> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 38a74d0..68cbd16 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -55,8 +55,6 @@
>   */
>  #define DEFAULT_TIMEOUT 0
>
> -#define MAX_UART_HWMOD_NAME_LEN		16
> -
>  struct omap_uart_state {
>  	int num;
>  	int can_sleep;
> @@ -96,6 +94,12 @@ struct omap_uart_state {
>  #endif
>  };
>
> +struct uart_oh {
> +	struct list_head node;
> +	struct omap_hwmod *oh;
> +};
> +
> +static LIST_HEAD(uart_oh_list);
>  static LIST_HEAD(uart_list);
>  static u8 num_uarts;
>
> @@ -568,52 +572,37 @@ static void serial_out_override(struct uart_port *up, int offset, int value)
>  }
>  #endif
>
> -void __init omap_serial_early_init(void)
> +static int omap_serial_port_init(struct omap_hwmod *oh, void *user)
>  {
> -	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);
> -		oh = omap_hwmod_lookup(oh_name);
> -		if (!oh)
> -			break;
> -
> -		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
> -		if (WARN_ON(!uart))
> -			return;
> -
> -		uart->oh = oh;
> -		uart->num = i++;
> -		list_add_tail(&uart->node, &uart_list);
> -		num_uarts++;
> -
> -		/*
> -		 * NOTE: omap_hwmod_init() has not yet been called,
> -		 *       so no hwmod functions will work yet.
> -		 */
> +	struct uart_oh *uoh;
>
> -		/*
> -		 * 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;

Minor issue... in the current pm-wip/uart (just changed today), this
part has been removed, so please refresh this against current
pm-wip/uart. 

> -
> -		/*
> -		 * Since UART hwmod is idle/enabled inside the
> -		 * idle loop, interrupts are already disabled and
> -		 * thus no locking is needed.  Since the mutex-based
> -		 * locking in hwmod might sleep, allowing locking
> -		 * may introduce problems.
> -		 */
> -		uart->oh->flags |= HWMOD_NO_IDLE_LOCKING;
> +	uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
> +	if (WARN_ON(!uoh))
> +		return -ENOMEM;
> +	/*
> +	 * 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
> +	 */
> +	oh->flags |= HWMOD_INIT_NO_IDLE;
> +	/*
> +	 * Since UART hwmod is idle/enabled inside the
> +	 * idle loop, interrupts are already disabled and
> +	 * thus no locking is needed.  Since the mutex-based
> +	 * locking in hwmod might sleep, allowing locking
> +	 * may introduce problems.
> +	 */
> +	oh->flags |= HWMOD_NO_IDLE_LOCKING;
> +	list_add_tail(&uoh->node, &uart_oh_list);
> +	uoh->oh = oh;
> +	num_uarts++;
> +	return 0;
> +}
>
> -	} while (1);
> +void __init omap_serial_early_init(void)
> +{
> +	omap_hwmod_for_each_by_class("uart",
> +		omap_serial_port_init, NULL);
>  }
>
>  /**
> @@ -769,7 +758,16 @@ void __init omap_serial_init_port(int port)
>  void __init omap_serial_init(void)
>  {
>  	struct omap_uart_state *uart;
> +	struct uart_oh *uoh;
> +	int i = 0;
>
> -	list_for_each_entry(uart, &uart_list, node)
> +	list_for_each_entry(uoh, &uart_oh_list, node) {
> +		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
> +		if (WARN_ON(!uart))
> +			return;
> +		list_add_tail(&uart->node, &uart_list);
> +		uart->num = i++;
> +		uart->oh = uoh->oh;
>  		omap_serial_init_port(uart->num);
> +	}
>  }

This works if a board calls omap_serial_init(), but doesn't work if a
board only wants to init a single UART and calls omap_serial_init_port()
directly.

I think you have to move the kzalloc, list_add etc. into
omap_serial_init_port()

Then, you'll have to add a 'num' field to the 'struct uart_oh' so you
can call omap_serial_init_port() with a valid number.

If we have to add a number to 'struct uart_oh', then it makes more sense
(to me) to just keep the hwmod lookup by name, since knowing the UART
number is rather important in this case.

So maybe just leave the hwmod lookup by name, but fix the problems with
empty lists that this patch also addresses.

Kevin

[pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early init phase

[Govindraj.R]

[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]

Leave the uart_list empty and keep the omap hwmod info
in a seperate uart_oh list and if board file calls
serial init use this uart_oh list info to fill uart_list.
The board file can also call just init_port to initialise
a single uart instance, so make init_port flexible to handle
individual uart instance by avoiding filling uart_list
in any non port_init function.

Cc: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
As per earlier dicussion:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31157.html

 arch/arm/mach-omap2/serial.c |   60 +++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 24b8c60..246ae02 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -97,6 +97,13 @@ struct omap_uart_state {
 #endif
 };

+struct uart_oh {
+	struct list_head node;
+	struct omap_hwmod *oh;
+	int uart_num;
+};
+
+static LIST_HEAD(uart_oh_list);
 static LIST_HEAD(uart_list);
 static u8 num_uarts;

@@ -593,39 +600,32 @@ static void serial_out_override(struct uart_port *up, int offset, int value)

 void __init omap_serial_early_init(void)
 {
-	int i = 0;
+	struct omap_hwmod *oh;
+	struct uart_oh *uoh;
+	char oh_name[MAX_UART_HWMOD_NAME_LEN];

 	do {
-		char oh_name[MAX_UART_HWMOD_NAME_LEN];
-		struct omap_hwmod *oh;
-		struct omap_uart_state *uart;
+		uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
+		if (WARN_ON(!uoh))
+			return;

 		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;
-
-		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
-		if (WARN_ON(!uart))
-			return;
-
-		uart->oh = oh;
-		uart->num = i++;
-		list_add_tail(&uart->node, &uart_list);
-		num_uarts++;
-
 		/*
 		 * NOTE: omap_hwmod_init() has not yet been called,
-		 *       so no hwmod functions will work yet.
-		 */
-
-		/*
+		 * 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
+		 * is ready. Therefore, don't allow idle here
 		 */
-		uart->oh->flags |= HWMOD_INIT_NO_IDLE;
+		oh->flags |= HWMOD_INIT_NO_IDLE;
+		uoh->oh = oh;
+		uoh->uart_num = num_uarts;
+		list_add_tail(&uoh->node, &uart_oh_list);
+		num_uarts++;
 	} while (1);
 }

@@ -645,6 +645,7 @@ void __init omap_serial_init_port(int port)
 	struct omap_uart_state *uart;
 	struct omap_hwmod *oh;
 	struct omap_device *od;
+	struct uart_oh *uoh;
 	void *pdata = NULL;
 	u32 pdata_size = 0;
 	char *name;
@@ -663,6 +664,17 @@ void __init omap_serial_init_port(int port)
 	if (WARN_ON(port >= num_uarts))
 		return;

+	uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
+	if (WARN_ON(!uart))
+		return;
+
+	uart->num = port;
+	list_add_tail(&uart->node, &uart_list);
+	list_for_each_entry(uoh, &uart_oh_list, node)
+		if (port == uoh->uart_num)
+			break;
+
+	uart->oh = uoh->oh;
 	list_for_each_entry(uart, &uart_list, node)
 		if (port == uart->num)
 			break;
@@ -781,8 +793,8 @@ void __init omap_serial_init_port(int port)
  */
 void __init omap_serial_init(void)
 {
-	struct omap_uart_state *uart;
+	struct uart_oh *uoh;

-	list_for_each_entry(uart, &uart_list, node)
-		omap_serial_init_port(uart->num);
+	list_for_each_entry(uoh, &uart_oh_list, node)
+		omap_serial_init_port(uoh->uart_num);
 }
-- 
1.6.3.3

[-- Attachment #2: patch.zip --]
[-- Type: application/x-zip-compressed, Size: 1673 bytes --]

Re: [pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early init phase

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> Leave the uart_list empty and keep the omap hwmod info
> in a seperate uart_oh list and if board file calls
> serial init use this uart_oh list info to fill uart_list.
> The board file can also call just init_port to initialise
> a single uart instance, so make init_port flexible to handle
> individual uart instance by avoiding filling uart_list
> in any non port_init function.
>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
> As per earlier dicussion:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31157.html

Thanks.

Your inline patch is still getting mangled somehow.  It looks like
you're using a webmail client (SquirrelMail) that seems to be altering
the whitespace in the patch.  The attached .zip worked fine.  Please
spend some time figuring out how to use git send-email directly

However, I just discovered another snag...

I did some more testing on Zoom3 and discovered that because we do the
HWMOD_INIT_NO_IDLE, and omap_serial_init[_port] is never called on Zoom3
(since it only uses debug board UART) the UART hwmods are left active 
are never disabled (since they are not used.)  This prevents retention :(

So, here's an idea... (I know, you're growing more and more to not like
my ideas that are going around in circles... sorry about that)

The ultimate goal is fix up all this serial.c hackery by getting your
OMAP serial driver upstream and moving PM handling there, so lets focus
on that instead of continuing to make this hacky layer perfect.

So for now, lets accept that with our current serial.c layer, we have to
always initialize all the UARTs for our PM core to work.  Currently,
Zoom2/3 are the only ones not doing that, so lets just change that until
we get your OMAP serial driver merged.

The patch below does it for Zoom3, can you test and do similar for
Zoom2?  Note that the default console will need to be changed to ttyS4
(Zoom3) and ttyS3 (Zoom2) after this patch.

Thanks,

Kevin

diff --git a/arch/arm/mach-omap2/board-zoom-debugboard.c b/arch/arm/mach-omap2/board-zoom-debugboard.c
index 1d7f827..9307f58 100644
--- a/arch/arm/mach-omap2/board-zoom-debugboard.c
+++ b/arch/arm/mach-omap2/board-zoom-debugboard.c
@@ -96,7 +96,7 @@ static struct plat_serial8250_port serial_platform_data[] = {
 
 static struct platform_device zoom_debugboard_serial_device = {
 	.name			= "serial8250",
-	.id			= PLAT8250_DEV_PLATFORM,
+	.id			= PLAT8250_DEV_PLATFORM + 4,
 	.dev			= {
 		.platform_data	= serial_platform_data,
 	},
diff --git a/arch/arm/mach-omap2/board-zoom3.c b/arch/arm/mach-omap2/board-zoom3.c
index 3314704..ff3b144 100644
--- a/arch/arm/mach-omap2/board-zoom3.c
+++ b/arch/arm/mach-omap2/board-zoom3.c
@@ -65,6 +65,7 @@ static const struct ehci_hcd_omap_platform_data ehci_pdata __initconst = {
 static void __init omap_zoom_init(void)
 {
 	omap3_mux_init(board_mux, OMAP_PACKAGE_CBP);
+	omap_serial_init();
 	zoom_peripherals_init();
 	zoom_debugboard_init();
 

Re: [pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early init phase

[Govindraj]

>
> Your inline patch is still getting mangled somehow.  It looks like
> you're using a webmail client (SquirrelMail) that seems to be altering
> the whitespace in the patch.  The attached .zip worked fine.  Please
> spend some time figuring out how to use git send-email directly
>

Have been trying to setup git-send quite some time, but firewall is blocking.
Will try to get this enabled somehow.

> However, I just discovered another snag...
>
> I did some more testing on Zoom3 and discovered that because we do the
> HWMOD_INIT_NO_IDLE, and omap_serial_init[_port] is never called on Zoom3
> (since it only uses debug board UART) the UART hwmods are left active
> are never disabled (since they are not used.)  This prevents retention :(
>

Yes I was able to reproduce this as said.

But I was trying to set this option from flags field from uart_hmwod
in omap_hwmod data file as below

.flags          = (HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET),

But strangely it doesn't work, but works only when we set the flag
from serial_early_init phase
any idea why so?

It was hitting retention for zoom2 with earlier suggested changes as
below: [ttyS3->console]

diff --git a/arch/arm/mach-omap2/board-zoom-debugboard.c
b/arch/arm/mach-omap2/board-zoom-debugboard.c
index 1d7f827..9307f58 100644
--- a/arch/arm/mach-omap2/board-zoom-debugboard.c
+++ b/arch/arm/mach-omap2/board-zoom-debugboard.c
@@ -96,7 +96,7 @@ static struct plat_serial8250_port serial_platform_data[] = {

 static struct platform_device zoom_debugboard_serial_device = {
        .name                   = "serial8250",
-       .id                     = PLAT8250_DEV_PLATFORM,
+       .id                     = PLAT8250_DEV_PLATFORM + 4,
        .dev                    = {
                .platform_data  = serial_platform_data,
        },
diff --git a/arch/arm/mach-omap2/board-zoom2.c
b/arch/arm/mach-omap2/board-zoom2.c
index 803ef14..c858b43 100644
--- a/arch/arm/mach-omap2/board-zoom2.c
+++ b/arch/arm/mach-omap2/board-zoom2.c
@@ -80,6 +80,7 @@ static struct omap_board_mux board_mux[] __initdata = {
 static void __init omap_zoom2_init(void)
 {
        omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
+       omap_serial_init();
        zoom_peripherals_init();
        zoom_debugboard_init();
 }


---
Regards,
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

RE: [pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early init phase

[DebBarma, Tarun Kanti]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Govindraj.R
> Sent: Friday, June 25, 2010 7:12 PM
> To: linux-omap@vger.kernel.org
> Cc: Kevin Hilman
> Subject: [pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early
> init phase
> 
> Leave the uart_list empty and keep the omap hwmod info
> in a seperate uart_oh list and if board file calls
> serial init use this uart_oh list info to fill uart_list.
> The board file can also call just init_port to initialise
> a single uart instance, so make init_port flexible to handle
> individual uart instance by avoiding filling uart_list
> in any non port_init function.
> 
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
> As per earlier dicussion:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31157.html
> 
>  arch/arm/mach-omap2/serial.c |   60 +++++++++++++++++++++++++------------
> ----
>  1 files changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 24b8c60..246ae02 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -97,6 +97,13 @@ struct omap_uart_state {
>  #endif
>  };
> 
> +struct uart_oh {
> +	struct list_head node;
> +	struct omap_hwmod *oh;
> +	int uart_num;
> +};
> +
> +static LIST_HEAD(uart_oh_list);
>  static LIST_HEAD(uart_list);
>  static u8 num_uarts;
> 
> @@ -593,39 +600,32 @@ static void serial_out_override(struct uart_port
> *up, int offset, int value)
> 
>  void __init omap_serial_early_init(void)
>  {
> -	int i = 0;
> +	struct omap_hwmod *oh;
> +	struct uart_oh *uoh;
> +	char oh_name[MAX_UART_HWMOD_NAME_LEN];
> 
>  	do {
> -		char oh_name[MAX_UART_HWMOD_NAME_LEN];
> -		struct omap_hwmod *oh;
> -		struct omap_uart_state *uart;
> +		uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
> +		if (WARN_ON(!uoh))
> +			return;
> 
>  		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;

[Tarun Kanti DebBarma] 
What happens to the successfully allocated memory (uoh)? Are we not supposed to free it before breaking?

In fact, is there any problem if we do the allocation at this point? 
+		uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
+		if (WARN_ON(!uoh))
+			return;

This would mean that we allocate and do the rest of initialization only when omap_hwmod_lookup() is successful.

> -
> -		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
> -		if (WARN_ON(!uart))
> -			return;
> -
> -		uart->oh = oh;
> -		uart->num = i++;
> -		list_add_tail(&uart->node, &uart_list);
> -		num_uarts++;
> -
>  		/*
>  		 * NOTE: omap_hwmod_init() has not yet been called,
> -		 *       so no hwmod functions will work yet.
> -		 */
> -
> -		/*
> +		 * 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
> +		 * is ready. Therefore, don't allow idle here
>  		 */
> -		uart->oh->flags |= HWMOD_INIT_NO_IDLE;
> +		oh->flags |= HWMOD_INIT_NO_IDLE;
> +		uoh->oh = oh;
> +		uoh->uart_num = num_uarts;
> +		list_add_tail(&uoh->node, &uart_oh_list);
> +		num_uarts++;
>  	} while (1);
>  }
> 
> @@ -645,6 +645,7 @@ void __init omap_serial_init_port(int port)
>  	struct omap_uart_state *uart;
>  	struct omap_hwmod *oh;
>  	struct omap_device *od;
> +	struct uart_oh *uoh;
>  	void *pdata = NULL;
>  	u32 pdata_size = 0;
>  	char *name;
> @@ -663,6 +664,17 @@ void __init omap_serial_init_port(int port)
>  	if (WARN_ON(port >= num_uarts))
>  		return;
[Tarun Kanti DebBarma]
I am not sure if this check is needed at all since this is called for all valid ports in uart_oh_list. In other words validation of port number has happened already while creating the uart_oh_list. 

> 
> +	uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
> +	if (WARN_ON(!uart))
> +		return;
> +
> +	uart->num = port;
> +	list_add_tail(&uart->node, &uart_list);
> +	list_for_each_entry(uoh, &uart_oh_list, node)
> +		if (port == uoh->uart_num)
> +			break;
> +
[Tarun Kanti DebBarma] 
BTW, why can't we pass (uoh) to this function instead of port number? In that case we would any way get the port number using uoh->uart_num as well as avoid the above loop for parsing the uart_oh_list for identifying the respective uoh just for making the below assignment.

> +	uart->oh = uoh->oh;
>  	list_for_each_entry(uart, &uart_list, node)
>  		if (port == uart->num)
>  			break;
> @@ -781,8 +793,8 @@ void __init omap_serial_init_port(int port)
>   */
>  void __init omap_serial_init(void)
>  {
> -	struct omap_uart_state *uart;
> +	struct uart_oh *uoh;
> 
> -	list_for_each_entry(uart, &uart_list, node)
> -		omap_serial_init_port(uart->num);
> +	list_for_each_entry(uoh, &uart_oh_list, node)
> +		omap_serial_init_port(uoh->uart_num);
[Tarun Kanti DebBarma] 
As mentioned above, can we pass (uoh) directly instead of (uoh->uart_num) ?

>  }
> --
> 1.6.3.3

Re: [pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early init phase

[Govindraj]

On Sat, Jun 26, 2010 at 9:38 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
>
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Govindraj.R
>> Sent: Friday, June 25, 2010 7:12 PM
>> To: linux-omap@vger.kernel.org
>> Cc: Kevin Hilman
>> Subject: [pm-wip/uart][PATCH] Serial: Avoid populating uart_list in early
>> init phase
>>
>> Leave the uart_list empty and keep the omap hwmod info
>> in a seperate uart_oh list and if board file calls
>> serial init use this uart_oh list info to fill uart_list.
>> The board file can also call just init_port to initialise
>> a single uart instance, so make init_port flexible to handle
>> individual uart instance by avoiding filling uart_list
>> in any non port_init function.
>>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>> ---
>> As per earlier dicussion:
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31157.html
>>
>>  arch/arm/mach-omap2/serial.c |   60 +++++++++++++++++++++++++------------
>> ----
>>  1 files changed, 36 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 24b8c60..246ae02 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -97,6 +97,13 @@ struct omap_uart_state {
>>  #endif
>>  };
>>
>> +struct uart_oh {
>> +     struct list_head node;
>> +     struct omap_hwmod *oh;
>> +     int uart_num;
>> +};
>> +
>> +static LIST_HEAD(uart_oh_list);
>>  static LIST_HEAD(uart_list);
>>  static u8 num_uarts;
>>
>> @@ -593,39 +600,32 @@ static void serial_out_override(struct uart_port
>> *up, int offset, int value)
>>
>>  void __init omap_serial_early_init(void)
>>  {
>> -     int i = 0;
>> +     struct omap_hwmod *oh;
>> +     struct uart_oh *uoh;
>> +     char oh_name[MAX_UART_HWMOD_NAME_LEN];
>>
>>       do {
>> -             char oh_name[MAX_UART_HWMOD_NAME_LEN];
>> -             struct omap_hwmod *oh;
>> -             struct omap_uart_state *uart;
>> +             uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
>> +             if (WARN_ON(!uoh))
>> +                     return;
>>
>>               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;
>
> [Tarun Kanti DebBarma]
> What happens to the successfully allocated memory (uoh)? Are we not supposed to free it before breaking?
>

oh is based on port instance if one instance of device is successful
we should retain
its instance rather warn only on the port instance where we failed.


> In fact, is there any problem if we do the allocation at this point?
> +               uoh = kzalloc(sizeof(struct uart_oh), GFP_KERNEL);
> +               if (WARN_ON(!uoh))
> +                       return;
>
> This would mean that we allocate and do the rest of initialization only when omap_hwmod_lookup() is >successful.
>
>> -
>> -             uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
>> -             if (WARN_ON(!uart))
>> -                     return;
>> -
>> -             uart->oh = oh;
>> -             uart->num = i++;
>> -             list_add_tail(&uart->node, &uart_list);
>> -             num_uarts++;
>> -
>>               /*
>>                * NOTE: omap_hwmod_init() has not yet been called,
>> -              *       so no hwmod functions will work yet.
>> -              */
>> -
>> -             /*
>> +              * 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
>> +              * is ready. Therefore, don't allow idle here
>>                */
>> -             uart->oh->flags |= HWMOD_INIT_NO_IDLE;
>> +             oh->flags |= HWMOD_INIT_NO_IDLE;
>> +             uoh->oh = oh;
>> +             uoh->uart_num = num_uarts;
>> +             list_add_tail(&uoh->node, &uart_oh_list);
>> +             num_uarts++;
>>       } while (1);
>>  }
>>
>> @@ -645,6 +645,7 @@ void __init omap_serial_init_port(int port)
>>       struct omap_uart_state *uart;
>>       struct omap_hwmod *oh;
>>       struct omap_device *od;
>> +     struct uart_oh *uoh;
>>       void *pdata = NULL;
>>       u32 pdata_size = 0;
>>       char *name;
>> @@ -663,6 +664,17 @@ void __init omap_serial_init_port(int port)
>>       if (WARN_ON(port >= num_uarts))
>>               return;
> [Tarun Kanti DebBarma]
> I am not sure if this check is needed at all since this is called for all valid ports in uart_oh_list. In other >words validation of port number has happened already while creating the uart_oh_list.
>
>>
>> +     uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
>> +     if (WARN_ON(!uart))
>> +             return;
>> +
>> +     uart->num = port;
>> +     list_add_tail(&uart->node, &uart_list);
>> +     list_for_each_entry(uoh, &uart_oh_list, node)
>> +             if (port == uoh->uart_num)
>> +                     break;
>> +
> [Tarun Kanti DebBarma]
> BTW, why can't we pass (uoh) to this function instead of port number? In that case we would any way get >the port number using uoh->uart_num as well as avoid the above loop for parsing the uart_oh_list for >identifying the respective uoh just for making the below assignment.
>
>> +     uart->oh = uoh->oh;
>>       list_for_each_entry(uart, &uart_list, node)
>>               if (port == uart->num)
>>                       break;
>> @@ -781,8 +793,8 @@ void __init omap_serial_init_port(int port)
>>   */
>>  void __init omap_serial_init(void)
>>  {
>> -     struct omap_uart_state *uart;
>> +     struct uart_oh *uoh;
>>
>> -     list_for_each_entry(uart, &uart_list, node)
>> -             omap_serial_init_port(uart->num);
>> +     list_for_each_entry(uoh, &uart_oh_list, node)
>> +             omap_serial_init_port(uoh->uart_num);
> [Tarun Kanti DebBarma]
> As mentioned above, can we pass (uoh) directly instead of (uoh->uart_num) ?

the reason for not passing uoh to init_port is because init_port can
be called directly to initialize any single
instance of the port with a port number based on the board used.

Regards,
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