Re: [PATCH] Clean up Lemote Loongson 2E Support

[Wu Zhangjin <wuzhangjin@gmail.com>]

On Wed, 2009-04-22 at 11:15 +0200, Arnaud Patard wrote:
> Philippe Vachon <philippe@cowpig.ca> writes:
> 
> Hi,
> 
> > This patch eliminates magic numbers and adds accessors for memory-mapped
> > registers. As well, it removes some inline assembly and restructures how
> > the early printk code behaves.
> >
> > Signed-off-by: Philippe Vachon <philippe@cowpig.ca>
> > ---
> >  arch/mips/include/asm/mach-lemote/loongson2e.h |   60 +++++++++++
> >  arch/mips/lemote/lm2e/dbg_io.c                 |  125 ++---------------------
> >  arch/mips/lemote/lm2e/prom.c                   |   10 +--
> >  arch/mips/lemote/lm2e/reset.c                  |   18 ++--
> >  4 files changed, 82 insertions(+), 131 deletions(-)
> >  create mode 100644 arch/mips/include/asm/mach-lemote/loongson2e.h
> >
> > diff --git a/arch/mips/include/asm/mach-lemote/loongson2e.h b/arch/mips/include/asm/mach-lemote/loongson2e.h
> > new file mode 100644
> > index 0000000..82c1a95
> > --- /dev/null
> > +++ b/arch/mips/include/asm/mach-lemote/loongson2e.h
> > @@ -0,0 +1,60 @@
> > +/* Accessor functions for the Loongson 2E MMIO registers
> > + *
> > + * Copyright (c) 2009 Philippe Vachon <philippe@cowpig.ca>
> > + *
> > + * This program is free software; you can redistribute  it and/or modify it
> > + * under  the terms of  the GNU General  Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + *
> > + */
> > +#ifndef __ASM_MACH_LEMOTE_LOONGSON2E
> > +#define __ASM_MACH_LEMOTE_LOONGSON2E
> > +
> > +#include <linux/types.h>
> > +
> > +/* Loongson 2E Control Registers */
> > +#define LS2E_REG_BASE		0x1fe00100 /* start of config registers */
> > +#define LS2E_GENCFG_REG		(LS2E_REG_BASE + 0x04)
> > +
> > +#define LS2E_RESET_VECTOR	0x1fc00000 /* this should be obvious! */
> > +
> 
> Theses are neither Lemote nor 2E specifics. 2E and 2F controllers are
> very similar (and they're similar to the bonito stuff). Why do you need
> a specific header instead of using the Bonito64.h header for theses
> constants ? And if you really want to create it, please rename all to
> loongson and remove lemote references as it'll work on 2F and on boards
> from ST.
> 

I am working on merging the source code of fuloong+yeeloong(2f based)
and 2e, currently, the source code of fuloong & yeeloong have been
merged, and now, I am trying to merge 2e & 2f, lots of duplications have
been removed with the import of some new header files, and some MACROs,
Variables have been tuned. a first release may be out to dev.lemote.com
before this weekend. 

> > +/* UART address (16550 -- on the Fulong) */
> > +#define LS2E_UART_BASE		0x1fd003f8
> 
> It happens to be 0x1fd003f8 but it could have been at a different
> address base. For instance, on 2f, depending on the board, there's
> 0x1ff003f8 and 0x1fd003f8 (I think there's also some board with a
> different address than theses but I'm not sure).
> 

in fuloong(2f) & yeeloong(2f), the serial address is 0xbfd002f8, in
fuloong(2e), the serial address is 0xbfd003f8.

> > +/* Various system parameters passed from PMON */
> > +extern unsigned long bus_clock;
> > +extern unsigned long cpu_clock_freq;
> > +extern unsigned int memsize, highmemsize;
> > +
> > +static inline void ls2e_writeb(uint8_t value, unsigned long addr)
> > +{
> > +	*(volatile uint8_t *)addr = value;
> > +}
> > +
> 
> What about readl/writel and friends ?
> [...]
> 
> > -void debugInit(u32 baud, u8 data, u8 parity, u8 stop)
> > +void prom_putchar(char c)
> >  {
> > -	u32 divisor;
> > -
> > -	/* disable interrupts */
> > -	UART16550_WRITE(OFS_INTR_ENABLE, 0);
> > +	int timeout;
> > +	phys_addr_t uart_base = (phys_addr_t)ioremap_nocache(LS2E_UART_BASE, 8);
> > +	char reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;
> 
> hmm... I may be wrong on that but using ioremap looks here looks not a
> good idea. This code is called by early_printk so you can end up calling
> it very early in the boot process.
> Also, you're calling ioremap everytime this function is called. Why
> don't you do that only the first time ?
> 
> [...]
> 
> > diff --git a/arch/mips/lemote/lm2e/reset.c b/arch/mips/lemote/lm2e/reset.c
> > index 099387a..0989d28 100644
> > --- a/arch/mips/lemote/lm2e/reset.c
> > +++ b/arch/mips/lemote/lm2e/reset.c
> > @@ -7,20 +7,22 @@
> >   * Copyright (C) 2007 Lemote, Inc. & Institute of Computing Technology
> >   * Author: Fuxin Zhang, zhangfx@lemote.com
> >   */
> > +
> >  #include <linux/pm.h>
> > +#include <linux/io.h>
> > +#include <loongson2e.h>
> >  
> >  #include <asm/reboot.h>
> >  
> >  static void loongson2e_restart(char *command)
> >  {
> > -#ifdef CONFIG_32BIT
> > -	*(unsigned long *)0xbfe00104 &= ~(1 << 2);
> > -	*(unsigned long *)0xbfe00104 |= (1 << 2);
> > -#else
> > -	*(unsigned long *)0xffffffffbfe00104 &= ~(1 << 2);
> > -	*(unsigned long *)0xffffffffbfe00104 |= (1 << 2);
> > -#endif
> > -	__asm__ __volatile__("jr\t%0"::"r"(0xbfc00000));
> > +	uint32_t ctl =
> > +		(ls2e_readl((phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4))
> > +		& ~(1 << 2)) | 1 << 2;
> > +
> > +	ls2e_writel(ctl, (phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4));
> > +
> > +	((void (*)(void))ioremap_nocache(LS2E_RESET_VECTOR, 4))();
> 
> same remark as for the serial stuff. I'm really not sure that calling ioremap
> in such a place is a good idea (say, you've panic'ed and booted with
> panic=3, will this still work ?).
> 
> 
> Arnaud
>