* Högander Jouni <jouni.hogander@xxxxxxxxx> [080820 08:50]:
> "ext Russell King - ARM Linux" <linux@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
