[PATCH 00/16] Omap2 patches for post 2.6.26

[PATCH 00/16] Omap2 patches for post 2.6.26

[Tony Lindgren]

Hi all,

This patch series contains various omap2 patchs from linux-omap tree.
Big chunk of the patches still improve the clock code. Also more
clean-up to allow compiling omap2 and omap3 support into the same
kernel. The patch also adds core omap3430 support, but no board
files or PM at at this point.

This patch series depends on earlier patch series:

1. Omap upstream patches for post 2.6.26

2. Omap2 clock patches for post 2.6.26

This patch series also available via git at:

git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6.git omap2-upstream

Regards,

Tony

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

[PATCH 01/16] ARM: OMAP2 Provide function to enable/disable uart clocks

[Tony Lindgren]

From: Jouni Hogander <jouni.hogander@nokia.com>

This patch adds common function to enable/disable omap2/3 uart
clocks. Enabled uarts are passed by bootloader in atags and clocks for
these enabled uarts are touched.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index e9c367f..b0fa582 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -3,7 +3,7 @@
  *
  * OMAP2 serial support.
  *
- * Copyright (C) 2005 Nokia Corporation
+ * Copyright (C) 2005-2008 Nokia Corporation
  * Author: Paul Mundt <paul.mundt@nokia.com>
  *
  * Based off of arch/arm/mach-omap/omap1/serial.c
@@ -18,43 +18,39 @@
 #include <linux/serial_reg.h>
 #include <linux/clk.h>
 
-#include <asm/io.h>
+#include <linux/io.h>
 
 #include <asm/arch/common.h>
 #include <asm/arch/board.h>
 
-static struct clk * uart1_ick = NULL;
-static struct clk * uart1_fck = NULL;
-static struct clk * uart2_ick = NULL;
-static struct clk * uart2_fck = NULL;
-static struct clk * uart3_ick = NULL;
-static struct clk * uart3_fck = NULL;
+static struct clk *uart_ick[OMAP_MAX_NR_PORTS];
+static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
 
 static struct plat_serial8250_port serial_platform_data[] = {
 	{
-		.membase	= (char *)IO_ADDRESS(OMAP_UART1_BASE),
+		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
 		.mapbase	= (unsigned long)OMAP_UART1_BASE,
 		.irq		= 72,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-		.uartclk	= OMAP16XX_BASE_BAUD * 16,
+		.uartclk	= OMAP24XX_BASE_BAUD * 16,
 	}, {
-		.membase	= (char *)IO_ADDRESS(OMAP_UART2_BASE),
+		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART2_BASE),
 		.mapbase	= (unsigned long)OMAP_UART2_BASE,
 		.irq		= 73,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-		.uartclk	= OMAP16XX_BASE_BAUD * 16,
+		.uartclk	= OMAP24XX_BASE_BAUD * 16,
 	}, {
-		.membase	= (char *)IO_ADDRESS(OMAP_UART3_BASE),
+		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART3_BASE),
 		.mapbase	= (unsigned long)OMAP_UART3_BASE,
 		.irq		= 74,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-		.uartclk	= OMAP16XX_BASE_BAUD * 16,
+		.uartclk	= OMAP24XX_BASE_BAUD * 16,
 	}, {
 		.flags		= 0
 	}
@@ -71,7 +67,7 @@ static inline void serial_write_reg(struct plat_serial8250_port *p, int offset,
 				    int value)
 {
 	offset <<= p->regshift;
-	__raw_writeb(value, (unsigned long)(p->membase + offset));
+	__raw_writeb(value, p->membase + offset);
 }
 
 /*
@@ -87,10 +83,27 @@ static inline void __init omap_serial_reset(struct plat_serial8250_port *p)
 	serial_write_reg(p, UART_OMAP_SYSC, (0x02 << 3) | (1 << 2) | (1 << 0));
 }
 
-void __init omap_serial_init()
+void omap_serial_enable_clocks(int enable)
+{
+	int i;
+	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
+		if (uart_ick[i] && uart_fck[i]) {
+			if (enable) {
+				clk_enable(uart_ick[i]);
+				clk_enable(uart_fck[i]);
+			} else {
+				clk_disable(uart_ick[i]);
+				clk_disable(uart_fck[i]);
+			}
+		}
+	}
+}
+
+void __init omap_serial_init(void)
 {
 	int i;
 	const struct omap_uart_config *info;
+	char name[16];
 
 	/*
 	 * Make sure the serial ports are muxed on at this point.
@@ -98,8 +111,7 @@ void __init omap_serial_init()
 	 * if not needed.
 	 */
 
-	info = omap_get_config(OMAP_TAG_UART,
-			       struct omap_uart_config);
+	info = omap_get_config(OMAP_TAG_UART, struct omap_uart_config);
 
 	if (info == NULL)
 		return;
@@ -108,58 +120,26 @@ void __init omap_serial_init()
 		struct plat_serial8250_port *p = serial_platform_data + i;
 
 		if (!(info->enabled_uarts & (1 << i))) {
-			p->membase = 0;
+			p->membase = NULL;
 			p->mapbase = 0;
 			continue;
 		}
 
-		switch (i) {
-		case 0:
-			uart1_ick = clk_get(NULL, "uart1_ick");
-			if (IS_ERR(uart1_ick))
-				printk("Could not get uart1_ick\n");
-			else {
-				clk_enable(uart1_ick);
-			}
-
-			uart1_fck = clk_get(NULL, "uart1_fck");
-			if (IS_ERR(uart1_fck))
-				printk("Could not get uart1_fck\n");
-			else {
-				clk_enable(uart1_fck);
-			}
-			break;
-		case 1:
-			uart2_ick = clk_get(NULL, "uart2_ick");
-			if (IS_ERR(uart2_ick))
-				printk("Could not get uart2_ick\n");
-			else {
-				clk_enable(uart2_ick);
-			}
-
-			uart2_fck = clk_get(NULL, "uart2_fck");
-			if (IS_ERR(uart2_fck))
-				printk("Could not get uart2_fck\n");
-			else {
-				clk_enable(uart2_fck);
-			}
-			break;
-		case 2:
-			uart3_ick = clk_get(NULL, "uart3_ick");
-			if (IS_ERR(uart3_ick))
-				printk("Could not get uart3_ick\n");
-			else {
-				clk_enable(uart3_ick);
-			}
-
-			uart3_fck = clk_get(NULL, "uart3_fck");
-			if (IS_ERR(uart3_fck))
-				printk("Could not get uart3_fck\n");
-			else {
-				clk_enable(uart3_fck);
-			}
-			break;
-		}
+		sprintf(name, "uart%d_ick", i+1);
+		uart_ick[i] = clk_get(NULL, name);
+		if (IS_ERR(uart_ick[i])) {
+			printk(KERN_ERR "Could not get uart%d_ick\n", i+1);
+			uart_ick[i] = NULL;
+		} else
+			clk_enable(uart_ick[i]);
+
+		sprintf(name, "uart%d_fck", i+1);
+		uart_fck[i] = clk_get(NULL, name);
+		if (IS_ERR(uart_fck[i])) {
+			printk(KERN_ERR "Could not get uart%d_fck\n", i+1);
+			uart_fck[i] = NULL;
+		} else
+			clk_enable(uart_fck[i]);
 
 		omap_serial_reset(p);
 	}
diff --git a/include/asm-arm/arch-omap/common.h b/include/asm-arm/arch-omap/common.h
index 8ac0307..e015e94 100644
--- a/include/asm-arm/arch-omap/common.h
+++ b/include/asm-arm/arch-omap/common.h
@@ -34,6 +34,7 @@ struct sys_timer;
 extern void omap_map_common_io(void);
 extern struct sys_timer omap_timer;
 extern void omap_serial_init(void);
+extern void omap_serial_enable_clocks(int enable);
 #ifdef CONFIG_I2C_OMAP
 extern int omap_register_i2c_bus(int bus_id, u32 clkrate,
 				 struct i2c_board_info const *info,
diff --git a/include/asm-arm/arch-omap/serial.h b/include/asm-arm/arch-omap/serial.h
index 79a5297..b6771b4 100644
--- a/include/asm-arm/arch-omap/serial.h
+++ b/include/asm-arm/arch-omap/serial.h
@@ -20,11 +20,17 @@
 #define OMAP_UART1_BASE		0x4806a000
 #define OMAP_UART2_BASE		0x4806c000
 #define OMAP_UART3_BASE		0x4806e000
+#elif defined(CONFIG_ARCH_OMAP3)
+/* OMAP3 serial ports */
+#define OMAP_UART1_BASE		0x4806a000
+#define OMAP_UART2_BASE		0x4806c000
+#define OMAP_UART3_BASE		0x49020000
 #endif
 
 #define OMAP_MAX_NR_PORTS	3
 #define OMAP1510_BASE_BAUD	(12000000/16)
 #define OMAP16XX_BASE_BAUD	(48000000/16)
+#define OMAP24XX_BASE_BAUD	(48000000/16)
 
 #define is_omap_port(p)	({int __ret = 0;			\
 			if (p == IO_ADDRESS(OMAP_UART1_BASE) ||	\

Re: [PATCH 01/16] ARM: OMAP2 Provide function to enable/disable uart clocks

[Russell King - ARM Linux]

On Fri, Jun 06, 2008 at 06:47:42PM -0700, Tony Lindgren wrote:
> This patch adds common function to enable/disable omap2/3 uart
> clocks. Enabled uarts are passed by bootloader in atags and clocks for
> these enabled uarts are touched.

In some ways, this patch is a good thing, in others it's outrageous.

> -static struct clk * uart1_ick = NULL;
> -static struct clk * uart1_fck = NULL;
> -static struct clk * uart2_ick = NULL;
> -static struct clk * uart2_fck = NULL;
> -static struct clk * uart3_ick = NULL;
> -static struct clk * uart3_fck = NULL;
> +static struct clk *uart_ick[OMAP_MAX_NR_PORTS];
> +static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
>  
>  static struct plat_serial8250_port serial_platform_data[] = {
>  	{
> -		.membase	= (char *)IO_ADDRESS(OMAP_UART1_BASE),
> +		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),

__force in drivers is a big no no, immediate patch rejection.  Drivers
do not use __force.

The reason for adding __iomem is to pick up where people are doing
stupid things with pointers, and __force is added in _private_ code
(eg, functions supporting IO, mapping functions, etc) to provide a
way of saying "this is part of the infrastructure and its permitted
to do this conversion."

The correct place for that cast, in its entirety, is inside the
IO_ADDRESS macro.

> @@ -71,7 +67,7 @@ static inline void serial_write_reg(struct plat_serial8250_port *p, int offset,
>  				    int value)
>  {
>  	offset <<= p->regshift;
> -	__raw_writeb(value, (unsigned long)(p->membase + offset));
> +	__raw_writeb(value, p->membase + offset);

Good.

> @@ -87,10 +83,27 @@ static inline void __init omap_serial_reset(struct plat_serial8250_port *p)
>  	serial_write_reg(p, UART_OMAP_SYSC, (0x02 << 3) | (1 << 2) | (1 << 0));
>  }
>  
> -void __init omap_serial_init()
> +void omap_serial_enable_clocks(int enable)
> +{
> +	int i;
> +	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
> +		if (uart_ick[i] && uart_fck[i]) {
> +			if (enable) {
> +				clk_enable(uart_ick[i]);
> +				clk_enable(uart_fck[i]);
> +			} else {
> +				clk_disable(uart_ick[i]);
> +				clk_disable(uart_fck[i]);
> +			}
> +		}
> +	}
> +}

Urgh.  Why enable all clocks?  I thought OMAP was about being as lean
and mean as possible as far as power management goes, so why enable
everything and disable everything?

Looking at the omapzoom tree, this function isn't even referenced by
another part of the kernel, so maybe this function is just cruft?

> +		sprintf(name, "uart%d_ick", i+1);
> +		uart_ick[i] = clk_get(NULL, name);
> +		if (IS_ERR(uart_ick[i])) {
> +			printk(KERN_ERR "Could not get uart%d_ick\n", i+1);
> +			uart_ick[i] = NULL;
> +		} else
> +			clk_enable(uart_ick[i]);
> +
> +		sprintf(name, "uart%d_fck", i+1);
> +		uart_fck[i] = clk_get(NULL, name);
> +		if (IS_ERR(uart_fck[i])) {
> +			printk(KERN_ERR "Could not get uart%d_fck\n", i+1);
> +			uart_fck[i] = NULL;
> +		} else
> +			clk_enable(uart_fck[i]);

Definitely an improvement, but still some way to go yet... but not a
reason not to get this in (once the above issues have been resolved.)

Re: [PATCH 01/16] ARM: OMAP2 Provide function to enable/disable uart clocks

[Högander Jouni]

"ext Russell King - ARM Linux" <linux@arm.linux.org.uk> writes:

> On Fri, Jun 06, 2008 at 06:47:42PM -0700, Tony Lindgren wrote:
>> This patch adds common function to enable/disable omap2/3 uart
>> clocks. Enabled uarts are passed by bootloader in atags and clocks for
>> these enabled uarts are touched.
>
> In some ways, this patch is a good thing, in others it's outrageous.
>
>> -static struct clk * uart1_ick = NULL;
>> -static struct clk * uart1_fck = NULL;
>> -static struct clk * uart2_ick = NULL;
>> -static struct clk * uart2_fck = NULL;
>> -static struct clk * uart3_ick = NULL;
>> -static struct clk * uart3_fck = NULL;
>> +static struct clk *uart_ick[OMAP_MAX_NR_PORTS];
>> +static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
>>  
>>  static struct plat_serial8250_port serial_platform_data[] = {
>>  	{
>> -		.membase	= (char *)IO_ADDRESS(OMAP_UART1_BASE),
>> +		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
>
> __force in drivers is a big no no, immediate patch rejection.  Drivers
> do not use __force.
>
> The reason for adding __iomem is to pick up where people are doing
> stupid things with pointers, and __force is added in _private_ code
> (eg, functions supporting IO, mapping functions, etc) to provide a
> way of saying "this is part of the infrastructure and its permitted
> to do this conversion."
>
> The correct place for that cast, in its entirety, is inside the
> IO_ADDRESS macro.
>
>> @@ -71,7 +67,7 @@ static inline void serial_write_reg(struct plat_serial8250_port *p, int offset,
>>  				    int value)
>>  {
>>  	offset <<= p->regshift;
>> -	__raw_writeb(value, (unsigned long)(p->membase + offset));
>> +	__raw_writeb(value, p->membase + offset);
>
> Good.
>
>> @@ -87,10 +83,27 @@ static inline void __init omap_serial_reset(struct plat_serial8250_port *p)
>>  	serial_write_reg(p, UART_OMAP_SYSC, (0x02 << 3) | (1 << 2) | (1 << 0));
>>  }
>>  
>> -void __init omap_serial_init()
>> +void omap_serial_enable_clocks(int enable)
>> +{
>> +	int i;
>> +	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
>> +		if (uart_ick[i] && uart_fck[i]) {
>> +			if (enable) {
>> +				clk_enable(uart_ick[i]);
>> +				clk_enable(uart_fck[i]);
>> +			} else {
>> +				clk_disable(uart_ick[i]);
>> +				clk_disable(uart_fck[i]);
>> +			}
>> +		}
>> +	}
>> +}
>
> Urgh.  Why enable all clocks?  I thought OMAP was about being as lean
> and mean as possible as far as power management goes, so why enable
> everything and disable everything?

Not everything, just those that are marked as a enabled by a
bootloader. This mean basically serial-console uart. If all three omap
uarts are used as a serial console, then this function will
disable/enable clocks for all of them.

> Looking at the omapzoom tree, this function isn't even referenced by
> another part of the kernel, so maybe this function is just cruft?

This function is added for dynamic PM to have a mean to enable/disable
serial console clocks. My intention is to be able to have PM when
serial console is used without need to reboot the device and changing
atags from the bootloader. There are patches on linux-omap list which are
using this. They are not pushed yet.

>
>> +		sprintf(name, "uart%d_ick", i+1);
>> +		uart_ick[i] = clk_get(NULL, name);
>> +		if (IS_ERR(uart_ick[i])) {
>> +			printk(KERN_ERR "Could not get uart%d_ick\n", i+1);
>> +			uart_ick[i] = NULL;
>> +		} else
>> +			clk_enable(uart_ick[i]);
>> +
>> +		sprintf(name, "uart%d_fck", i+1);
>> +		uart_fck[i] = clk_get(NULL, name);
>> +		if (IS_ERR(uart_fck[i])) {
>> +			printk(KERN_ERR "Could not get uart%d_fck\n", i+1);
>> +			uart_fck[i] = NULL;
>> +		} else
>> +			clk_enable(uart_fck[i]);
>
> Definitely an improvement, but still some way to go yet... but not a
> reason not to get this in (once the above issues have been resolved.)
>
>

-- 
Jouni Högander

--
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: [PATCH 01/16] ARM: OMAP2 Provide function to enable/disable uart clocks

[Tony Lindgren]

* Högander Jouni <jouni.hogander@nokia.com> [080820 08:50]:
> "ext Russell King - ARM Linux" <linux@arm.linux.org.uk> writes:
> 
> > On Fri, Jun 06, 2008 at 06:47:42PM -0700, Tony Lindgren wrote:
> >> This patch adds common function to enable/disable omap2/3 uart
> >> clocks. Enabled uarts are passed by bootloader in atags and clocks for
> >> these enabled uarts are touched.
> >
> > In some ways, this patch is a good thing, in others it's outrageous.
> >
> >> -static struct clk * uart1_ick = NULL;
> >> -static struct clk * uart1_fck = NULL;
> >> -static struct clk * uart2_ick = NULL;
> >> -static struct clk * uart2_fck = NULL;
> >> -static struct clk * uart3_ick = NULL;
> >> -static struct clk * uart3_fck = NULL;
> >> +static struct clk *uart_ick[OMAP_MAX_NR_PORTS];
> >> +static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
> >>  
> >>  static struct plat_serial8250_port serial_platform_data[] = {
> >>  	{
> >> -		.membase	= (char *)IO_ADDRESS(OMAP_UART1_BASE),
> >> +		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
> >
> > __force in drivers is a big no no, immediate patch rejection.  Drivers
> > do not use __force.
> >
> > The reason for adding __iomem is to pick up where people are doing
> > stupid things with pointers, and __force is added in _private_ code
> > (eg, functions supporting IO, mapping functions, etc) to provide a
> > way of saying "this is part of the infrastructure and its permitted
> > to do this conversion."
> >
> > The correct place for that cast, in its entirety, is inside the
> > IO_ADDRESS macro.
> >
> >> @@ -71,7 +67,7 @@ static inline void serial_write_reg(struct plat_serial8250_port *p, int offset,
> >>  				    int value)
> >>  {
> >>  	offset <<= p->regshift;
> >> -	__raw_writeb(value, (unsigned long)(p->membase + offset));
> >> +	__raw_writeb(value, p->membase + offset);
> >
> > Good.
> >
> >> @@ -87,10 +83,27 @@ static inline void __init omap_serial_reset(struct plat_serial8250_port *p)
> >>  	serial_write_reg(p, UART_OMAP_SYSC, (0x02 << 3) | (1 << 2) | (1 << 0));
> >>  }
> >>  
> >> -void __init omap_serial_init()
> >> +void omap_serial_enable_clocks(int enable)
> >> +{
> >> +	int i;
> >> +	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
> >> +		if (uart_ick[i] && uart_fck[i]) {
> >> +			if (enable) {
> >> +				clk_enable(uart_ick[i]);
> >> +				clk_enable(uart_fck[i]);
> >> +			} else {
> >> +				clk_disable(uart_ick[i]);
> >> +				clk_disable(uart_fck[i]);
> >> +			}
> >> +		}
> >> +	}
> >> +}
> >
> > Urgh.  Why enable all clocks?  I thought OMAP was about being as lean
> > and mean as possible as far as power management goes, so why enable
> > everything and disable everything?
> 
> Not everything, just those that are marked as a enabled by a
> bootloader. This mean basically serial-console uart. If all three omap
> uarts are used as a serial console, then this function will
> disable/enable clocks for all of them.

Or marked as enabled in board-*.c file. The omap ATAGs are not going
to mainline tree as discussed earlier and will get removed from l-o
tree eventually also. Any bootloader tags must be generic, such as
ATAG_UART or something similar.

> > Looking at the omapzoom tree, this function isn't even referenced by
> > another part of the kernel, so maybe this function is just cruft?
> 
> This function is added for dynamic PM to have a mean to enable/disable
> serial console clocks. My intention is to be able to have PM when
> serial console is used without need to reboot the device and changing
> atags from the bootloader. There are patches on linux-omap list which are
> using this. They are not pushed yet.

The long term solution is to switch to omap-uart instead of 8250.c so we
can support DMA and dynamic power and clocking.

> 
> >
> >> +		sprintf(name, "uart%d_ick", i+1);
> >> +		uart_ick[i] = clk_get(NULL, name);
> >> +		if (IS_ERR(uart_ick[i])) {
> >> +			printk(KERN_ERR "Could not get uart%d_ick\n", i+1);
> >> +			uart_ick[i] = NULL;
> >> +		} else
> >> +			clk_enable(uart_ick[i]);
> >> +
> >> +		sprintf(name, "uart%d_fck", i+1);
> >> +		uart_fck[i] = clk_get(NULL, name);
> >> +		if (IS_ERR(uart_fck[i])) {
> >> +			printk(KERN_ERR "Could not get uart%d_fck\n", i+1);
> >> +			uart_fck[i] = NULL;
> >> +		} else
> >> +			clk_enable(uart_fck[i]);
> >
> > Definitely an improvement, but still some way to go yet... but not a
> > reason not to get this in (once the above issues have been resolved.)
> >
> >
> 
> -- 
> Jouni Högander
> 
--
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: [PATCH 01/16] ARM: OMAP2 Provide function to enable/disable uart clocks

[Tony Lindgren]

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

* Tony Lindgren <tony@atomide.com> [080820 00:28]:
> * Högander Jouni <jouni.hogander@nokia.com> [080820 08:50]:
> > "ext Russell King - ARM Linux" <linux@arm.linux.org.uk> writes:
> > 
> > > On Fri, Jun 06, 2008 at 06:47:42PM -0700, Tony Lindgren wrote:
> > >> This patch adds common function to enable/disable omap2/3 uart
> > >> clocks. Enabled uarts are passed by bootloader in atags and clocks for
> > >> these enabled uarts are touched.
> > >
> > > In some ways, this patch is a good thing, in others it's outrageous.
> > >
> > >> -static struct clk * uart1_ick = NULL;
> > >> -static struct clk * uart1_fck = NULL;
> > >> -static struct clk * uart2_ick = NULL;
> > >> -static struct clk * uart2_fck = NULL;
> > >> -static struct clk * uart3_ick = NULL;
> > >> -static struct clk * uart3_fck = NULL;
> > >> +static struct clk *uart_ick[OMAP_MAX_NR_PORTS];
> > >> +static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
> > >>  
> > >>  static struct plat_serial8250_port serial_platform_data[] = {
> > >>  	{
> > >> -		.membase	= (char *)IO_ADDRESS(OMAP_UART1_BASE),
> > >> +		.membase	= (__force void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
> > >
> > > __force in drivers is a big no no, immediate patch rejection.  Drivers
> > > do not use __force.
> > >
> > > The reason for adding __iomem is to pick up where people are doing
> > > stupid things with pointers, and __force is added in _private_ code
> > > (eg, functions supporting IO, mapping functions, etc) to provide a
> > > way of saying "this is part of the infrastructure and its permitted
> > > to do this conversion."
> > >
> > > The correct place for that cast, in its entirety, is inside the
> > > IO_ADDRESS macro.
> > >
> > >> @@ -71,7 +67,7 @@ static inline void serial_write_reg(struct plat_serial8250_port *p, int offset,
> > >>  				    int value)
> > >>  {
> > >>  	offset <<= p->regshift;
> > >> -	__raw_writeb(value, (unsigned long)(p->membase + offset));
> > >> +	__raw_writeb(value, p->membase + offset);
> > >
> > > Good.
> > >
> > >> @@ -87,10 +83,27 @@ static inline void __init omap_serial_reset(struct plat_serial8250_port *p)
> > >>  	serial_write_reg(p, UART_OMAP_SYSC, (0x02 << 3) | (1 << 2) | (1 << 0));
> > >>  }
> > >>  
> > >> -void __init omap_serial_init()
> > >> +void omap_serial_enable_clocks(int enable)
> > >> +{
> > >> +	int i;
> > >> +	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
> > >> +		if (uart_ick[i] && uart_fck[i]) {
> > >> +			if (enable) {
> > >> +				clk_enable(uart_ick[i]);
> > >> +				clk_enable(uart_fck[i]);
> > >> +			} else {
> > >> +				clk_disable(uart_ick[i]);
> > >> +				clk_disable(uart_fck[i]);
> > >> +			}
> > >> +		}
> > >> +	}
> > >> +}
> > >
> > > Urgh.  Why enable all clocks?  I thought OMAP was about being as lean
> > > and mean as possible as far as power management goes, so why enable
> > > everything and disable everything?
> > 
> > Not everything, just those that are marked as a enabled by a
> > bootloader. This mean basically serial-console uart. If all three omap
> > uarts are used as a serial console, then this function will
> > disable/enable clocks for all of them.
> 
> Or marked as enabled in board-*.c file. The omap ATAGs are not going
> to mainline tree as discussed earlier and will get removed from l-o
> tree eventually also. Any bootloader tags must be generic, such as
> ATAG_UART or something similar.
> 
> > > Looking at the omapzoom tree, this function isn't even referenced by
> > > another part of the kernel, so maybe this function is just cruft?
> > 
> > This function is added for dynamic PM to have a mean to enable/disable
> > serial console clocks. My intention is to be able to have PM when
> > serial console is used without need to reboot the device and changing
> > atags from the bootloader. There are patches on linux-omap list which are
> > using this. They are not pushed yet.
> 
> The long term solution is to switch to omap-uart instead of 8250.c so we
> can support DMA and dynamic power and clocking.
> 
> > 
> > >
> > >> +		sprintf(name, "uart%d_ick", i+1);
> > >> +		uart_ick[i] = clk_get(NULL, name);
> > >> +		if (IS_ERR(uart_ick[i])) {
> > >> +			printk(KERN_ERR "Could not get uart%d_ick\n", i+1);
> > >> +			uart_ick[i] = NULL;
> > >> +		} else
> > >> +			clk_enable(uart_ick[i]);
> > >> +
> > >> +		sprintf(name, "uart%d_fck", i+1);
> > >> +		uart_fck[i] = clk_get(NULL, name);
> > >> +		if (IS_ERR(uart_fck[i])) {
> > >> +			printk(KERN_ERR "Could not get uart%d_fck\n", i+1);
> > >> +			uart_fck[i] = NULL;
> > >> +		} else
> > >> +			clk_enable(uart_fck[i]);
> > >
> > > Definitely an improvement, but still some way to go yet... but not a
> > > reason not to get this in (once the above issues have been resolved.)
> > >
> > >
> > 

Here's this one updated with __force removed.

Tony

[-- Attachment #2: 0001-serial --]
[-- Type: text/plain, Size: 6803 bytes --]

>From bb49b3cd1da2f23b344fcaf32607d280731d2653 Mon Sep 17 00:00:00 2001
From: Jouni Hogander <jouni.hogander@nokia.com>
Date: Sat, 23 Aug 2008 15:19:46 -0700
Subject: [PATCH] ARM: OMAP2 Provide function to enable/disable uart clocks

This patch adds common function to enable/disable omap2/3 uart
clocks. Enabled uarts are passed by bootloader in atags and clocks for
these enabled uarts are touched.

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index adc8a26..bd6f8c9 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -3,7 +3,7 @@
  *
  * OMAP2 serial support.
  *
- * Copyright (C) 2005 Nokia Corporation
+ * Copyright (C) 2005-2008 Nokia Corporation
  * Author: Paul Mundt <paul.mundt@nokia.com>
  *
  * Based off of arch/arm/mach-omap/omap1/serial.c
@@ -18,43 +18,39 @@
 #include <linux/serial_reg.h>
 #include <linux/clk.h>
 
-#include <asm/io.h>
+#include <linux/io.h>
 
 #include <mach/common.h>
 #include <mach/board.h>
 
-static struct clk * uart1_ick = NULL;
-static struct clk * uart1_fck = NULL;
-static struct clk * uart2_ick = NULL;
-static struct clk * uart2_fck = NULL;
-static struct clk * uart3_ick = NULL;
-static struct clk * uart3_fck = NULL;
+static struct clk *uart_ick[OMAP_MAX_NR_PORTS];
+static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
 
 static struct plat_serial8250_port serial_platform_data[] = {
 	{
-		.membase	= (char *)IO_ADDRESS(OMAP_UART1_BASE),
+		.membase	= (void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
 		.mapbase	= (unsigned long)OMAP_UART1_BASE,
 		.irq		= 72,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-		.uartclk	= OMAP16XX_BASE_BAUD * 16,
+		.uartclk	= OMAP24XX_BASE_BAUD * 16,
 	}, {
-		.membase	= (char *)IO_ADDRESS(OMAP_UART2_BASE),
+		.membase	= (void __iomem *)IO_ADDRESS(OMAP_UART2_BASE),
 		.mapbase	= (unsigned long)OMAP_UART2_BASE,
 		.irq		= 73,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-		.uartclk	= OMAP16XX_BASE_BAUD * 16,
+		.uartclk	= OMAP24XX_BASE_BAUD * 16,
 	}, {
-		.membase	= (char *)IO_ADDRESS(OMAP_UART3_BASE),
+		.membase	= (void __iomem *)IO_ADDRESS(OMAP_UART3_BASE),
 		.mapbase	= (unsigned long)OMAP_UART3_BASE,
 		.irq		= 74,
 		.flags		= UPF_BOOT_AUTOCONF,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-		.uartclk	= OMAP16XX_BASE_BAUD * 16,
+		.uartclk	= OMAP24XX_BASE_BAUD * 16,
 	}, {
 		.flags		= 0
 	}
@@ -71,7 +67,7 @@ static inline void serial_write_reg(struct plat_serial8250_port *p, int offset,
 				    int value)
 {
 	offset <<= p->regshift;
-	__raw_writeb(value, (unsigned long)(p->membase + offset));
+	__raw_writeb(value, p->membase + offset);
 }
 
 /*
@@ -87,10 +83,27 @@ static inline void __init omap_serial_reset(struct plat_serial8250_port *p)
 	serial_write_reg(p, UART_OMAP_SYSC, (0x02 << 3) | (1 << 2) | (1 << 0));
 }
 
-void __init omap_serial_init()
+void omap_serial_enable_clocks(int enable)
+{
+	int i;
+	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
+		if (uart_ick[i] && uart_fck[i]) {
+			if (enable) {
+				clk_enable(uart_ick[i]);
+				clk_enable(uart_fck[i]);
+			} else {
+				clk_disable(uart_ick[i]);
+				clk_disable(uart_fck[i]);
+			}
+		}
+	}
+}
+
+void __init omap_serial_init(void)
 {
 	int i;
 	const struct omap_uart_config *info;
+	char name[16];
 
 	/*
 	 * Make sure the serial ports are muxed on at this point.
@@ -98,8 +111,7 @@ void __init omap_serial_init()
 	 * if not needed.
 	 */
 
-	info = omap_get_config(OMAP_TAG_UART,
-			       struct omap_uart_config);
+	info = omap_get_config(OMAP_TAG_UART, struct omap_uart_config);
 
 	if (info == NULL)
 		return;
@@ -108,58 +120,26 @@ void __init omap_serial_init()
 		struct plat_serial8250_port *p = serial_platform_data + i;
 
 		if (!(info->enabled_uarts & (1 << i))) {
-			p->membase = 0;
+			p->membase = NULL;
 			p->mapbase = 0;
 			continue;
 		}
 
-		switch (i) {
-		case 0:
-			uart1_ick = clk_get(NULL, "uart1_ick");
-			if (IS_ERR(uart1_ick))
-				printk("Could not get uart1_ick\n");
-			else {
-				clk_enable(uart1_ick);
-			}
-
-			uart1_fck = clk_get(NULL, "uart1_fck");
-			if (IS_ERR(uart1_fck))
-				printk("Could not get uart1_fck\n");
-			else {
-				clk_enable(uart1_fck);
-			}
-			break;
-		case 1:
-			uart2_ick = clk_get(NULL, "uart2_ick");
-			if (IS_ERR(uart2_ick))
-				printk("Could not get uart2_ick\n");
-			else {
-				clk_enable(uart2_ick);
-			}
-
-			uart2_fck = clk_get(NULL, "uart2_fck");
-			if (IS_ERR(uart2_fck))
-				printk("Could not get uart2_fck\n");
-			else {
-				clk_enable(uart2_fck);
-			}
-			break;
-		case 2:
-			uart3_ick = clk_get(NULL, "uart3_ick");
-			if (IS_ERR(uart3_ick))
-				printk("Could not get uart3_ick\n");
-			else {
-				clk_enable(uart3_ick);
-			}
-
-			uart3_fck = clk_get(NULL, "uart3_fck");
-			if (IS_ERR(uart3_fck))
-				printk("Could not get uart3_fck\n");
-			else {
-				clk_enable(uart3_fck);
-			}
-			break;
-		}
+		sprintf(name, "uart%d_ick", i+1);
+		uart_ick[i] = clk_get(NULL, name);
+		if (IS_ERR(uart_ick[i])) {
+			printk(KERN_ERR "Could not get uart%d_ick\n", i+1);
+			uart_ick[i] = NULL;
+		} else
+			clk_enable(uart_ick[i]);
+
+		sprintf(name, "uart%d_fck", i+1);
+		uart_fck[i] = clk_get(NULL, name);
+		if (IS_ERR(uart_fck[i])) {
+			printk(KERN_ERR "Could not get uart%d_fck\n", i+1);
+			uart_fck[i] = NULL;
+		} else
+			clk_enable(uart_fck[i]);
 
 		omap_serial_reset(p);
 	}
diff --git a/arch/arm/plat-omap/include/mach/common.h b/arch/arm/plat-omap/include/mach/common.h
index 0609311..515c628 100644
--- a/arch/arm/plat-omap/include/mach/common.h
+++ b/arch/arm/plat-omap/include/mach/common.h
@@ -34,6 +34,7 @@ struct sys_timer;
 extern void omap_map_common_io(void);
 extern struct sys_timer omap_timer;
 extern void omap_serial_init(void);
+extern void omap_serial_enable_clocks(int enable);
 #ifdef CONFIG_I2C_OMAP
 extern int omap_register_i2c_bus(int bus_id, u32 clkrate,
 				 struct i2c_board_info const *info,
diff --git a/arch/arm/plat-omap/include/mach/serial.h b/arch/arm/plat-omap/include/mach/serial.h
index cc6bfa5..abea6ef 100644
--- a/arch/arm/plat-omap/include/mach/serial.h
+++ b/arch/arm/plat-omap/include/mach/serial.h
@@ -20,11 +20,17 @@
 #define OMAP_UART1_BASE		0x4806a000
 #define OMAP_UART2_BASE		0x4806c000
 #define OMAP_UART3_BASE		0x4806e000
+#elif defined(CONFIG_ARCH_OMAP3)
+/* OMAP3 serial ports */
+#define OMAP_UART1_BASE		0x4806a000
+#define OMAP_UART2_BASE		0x4806c000
+#define OMAP_UART3_BASE		0x49020000
 #endif
 
 #define OMAP_MAX_NR_PORTS	3
 #define OMAP1510_BASE_BAUD	(12000000/16)
 #define OMAP16XX_BASE_BAUD	(48000000/16)
+#define OMAP24XX_BASE_BAUD	(48000000/16)
 
 #define is_omap_port(p)	({int __ret = 0;			\
 			if (p == IO_ADDRESS(OMAP_UART1_BASE) ||	\