linux-mips
[Top] [All Lists]

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

To: Arnaud Patard <apatard@mandriva.com>
Subject: Re: [PATCH] Clean up Lemote Loongson 2E Support
From: Philippe Vachon <philippe@cowpig.ca>
Date: Wed, 22 Apr 2009 12:16:31 -0400
Cc: linux-mips@linux-mips.org, Ralf Baechle <ralf@linux-mips.org>
In-reply-to: <m363gxgic8.fsf@anduin.mandriva.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20090422063747.GA2009@cowpig.ca> <m363gxgic8.fsf@anduin.mandriva.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.9i
Hi,

On Wed, Apr 22, 2009 at 11:15:19AM +0200, Arnaud Patard wrote:
> 
> 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.
> 

What might be worthwhile eventually is to move all the loongson code into
arch/mips/loongson. I know there is some duplication of code between 2F
and 2E as it stands, but since 2F isn't upstream yet (and probably won't
be for a while), I would cross that bridge when we get there.

Given how Loongson actually differs a lot from Bonito64 beyond a few of
the control registers' addresses, perhaps it is worthwhile to start
moving away from using bonito64.h anyways; it's a bit odd that ICT chose
to base some of the SoC on the Bonito64 design, IMO, but that's a
different conversation.

> > +/* 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).
> 

One option I was thinking of was moving that into a device-specific
header. However, of the two Loongson 2E devices I have, both have the
UART at the same address -- are there any Loongson 2E devices that have
the UART at a different location? None that I'm aware of (and on the 2F
side, I only know of the Gdium).

...

> > +static inline void ls2e_writeb(uint8_t value, unsigned long addr)
> > +{
> > +   *(volatile uint8_t *)addr = value;
> > +}
> > +
> 
> What about readl/writel and friends ?
> 

I'm ambivalent. Since they're sub-64-bit-wide reads/writes, they're
happening in a single instruction anyways. I can adjust the patch to use
them in the name of reducing code duplication.

> 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 ?
> 
> [...]

Quite wrong -- as the address is in kseg1, when compiled this actually
will end up having the same net effect as using the 'deprecated'
CKSEG1ADDR() macro. Have a look at the implementation of
__ioremap_mode() in arch/mips/include/asm/io.h. While it's being
'called' early in the boot process, the physical resource will be 
accessed via its kseg1 address. When I was speaking to Ralf about this, 
he had mentioned this was by design.

In looking at the disassembly of prom_putchar in particular:

; ioremap_nocache()
li      $v0, 0xFFFFFFF9 
dsll32  $v0, 8
ori     $v0, 0x1FD
dsll    $v0, 20
ori     $v0, 0x3FD

; load from the UART's LSR register:
lbu     $v1, 0($v0)
...

> 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 ?).
> 

Yes. See above.

I'll probably send an updated patch shortly.

Cheers,
Phil

<Prev in Thread] Current Thread [Next in Thread>