On Wed, 2009-04-22 at 11:15 +0200, Arnaud Patard wrote:
> Philippe Vachon <philippe@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
> > + *
> > + * 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@xxxxxxxxxx
> > */
> > +
> > #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
>
