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

[Arnaud Patard <apatard@mandriva.com>]

Philippe Vachon <philippe@cowpig.ca> writes:

Hi,

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

Arnaud