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

[Tony Lindgren <tony@atomide.com>]

* 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