[Top] [All Lists]

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

To: Philippe Vachon <>
Subject: Re: [PATCH] Clean up Lemote Loongson 2E Support
From: Arnaud Patard <>
Date: Thu, 23 Apr 2009 10:23:47 +0200
Cc:, Ralf Baechle <>
In-reply-to: <> (Philippe Vachon's message of "Wed, 22 Apr 2009 12:16:31 -0400")
Organization: Mandriva
Original-recipient: rfc822;
References: <> <> <>
User-agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/22.1 (gnu/linux)
Philippe Vachon <> writes:


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

That's not a good reason to repeat wrong naming from the
past. Moverover, Lemote is trying to get 2f support merged so I would
say that you're pessimistic.

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

1 is more than 0 so, please take 2f into account. I don't see the
benefit of creating yet an other header containing only difference
something like "UART_BASE 0x1ff003f8" 

> ...
>> > +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.

ok, I read the code too quickly. thanks.

Please note also that my remark about calling it everytime still
stands. Reading a variable is taking less instructions than calling


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