Re: [PATCH 1/14] lemote: Loongson2F based machines support

[yanhua <yanh@lemote.com>]

Philippe Vachon 写道:
> I think this patch would be a lot better off broken up into several
> pieces, but I also have found there's a lot of code that is duplicated
> in there that needs to be tidied up (even if the implementation isn't
> identical, the functionality is). This comment in particular applies to
> the yeelong/ and fuloong/ directories.
>   
Thanks for your advice. we will merge the code to reduce duplicated 
functions.
> As well, there's pretty egregious abuse of inline asm to do things that
> really don't require inline asm, like manipulating MMIO registers.
>
> The feedback I've provided is generally at a glance, though I have some
> familiarity with Lemote's code, so some of these are issues I've
> anticipated would come up. :-)
>
> I've also avoided stylistic feedback -- I think there are people who are
> far better qualified to comment on that.
>
> On Thu, Apr 09, 2009 at 12:50:16PM +0800, yanhua wrote:
>   
>> +#ifdef CONFIG_TRACE_BOOT
>> + printk(KERN_INFO"arch_initcall:pcibios_init\n");
>> + printk(KERN_INFO"register_pci_controller :
>> %x\n",&loongson2f_pci_controller);
>> +#endif
>> +
>> +#ifdef CONFIG_64BIT
>> + loongson2f_pci_mem_resource.start = 0x50000000UL;
>> + loongson2f_pci_mem_resource.end = 0x7fffffffUL;
>> + __asm__(".set mips3\n"
>> + "dli $2,0x900000003ff00000\n"
>> + "li $3,0x40000000\n"
>> + "sd $3,0x18($2)\n"
>> + "or $3,1\n"
>> + "sd $3,0x58($2)\n"
>> + "dli $3,0xffffffffc0000000\n"
>> + "sd $3,0x38($2)\n"
>> + ".set mips0\n"
>> + :::"$2","$3","memory"
>> + );
>>     
>
> This is completely unnecessary and should be rewritten without the
> inline asm; I've prepared a header that has both register offsets and
> bases, as well as accessor macros that look a lot neater than this mess.
>
> This can be found at: 
> http://git.linux-cisco.org/?p=linux-mips.git;a=blob;f=arch/mips/include/asm/mach-stls2f/stls2f.h;h=9dbbb5d068618a1530c9396af32dfc4a394ea826;hb=refs/heads/linux-gdium
>   
Really thanks. :-)
> (pardon the long URL)
>
>   
>> +++ b/arch/mips/lemote/lm2f/fuloong/dbg_io.c
>> @@ -0,0 +1,182 @@
>>     
> *snip*
>   
>> +#ifdef CONFIG_64BIT
>> +#define BASE (0xffffffffbff003f8)
>> +#else
>> +#define BASE (0xbff003f8)
>> +#endif
>> +
>> +#else /* USE_CS5536_UART1/2 */
>> +
>> +#ifdef CONFIG_64BIT
>> +#define BASE 0xffffffffbfd002f8
>> +#else
>> +#define BASE 0xbfd002f8
>> +#endif
>> +
>> +#endif /* end of USE_GODSON2F_UART */
>>     
>
> More magic numbers that should likely be moved into a header.
>
> *more code snipped*
>
>   
>> +++ b/arch/mips/lemote/lm2f/fuloong/prom.c
>> +
>> +#ifdef CONFIG_32BIT
>> + *(volatile u32*)0xbfe00180 |= 0x7;
>> +#else
>> + *(volatile u32*)0xffffffffbfe00180 |= 0x7;
>> +#endif
>> + _rdmsr(0xe0000014, &hi, &lo);
>> + lo |= 0x00000001;
>> + _wrmsr(0xe0000014, hi, lo);
>> +
>>     
>
> More magic numbers that should have names. Accesses to MMIO registers
> should be done through accessors.
>
>   
>> + printk("Hard reset not take effect!!\n");
>> + __asm__ __volatile__ (
>> + ".long 0x3c02bfc0\n"
>> + ".long 0x00400008\n"
>> + :::"v0"
>> + );
>>     
>
> This is really REALLY scary. Perhaps it'd be even slightly neater to
> convert that jump to a void function pointer -- something like
>
> ((void (*)(void))(RESET_VECTOR)))();
>
> (note that I haven't tested that, and RESET_VECTOR would have to be
> something appropriate, i.e. CKSEG1ADDR(BONITO_BOOT_BASE) in this case,
> though I think CKSEG1ADDR is frowned upon)
>
>
>   
>> +static void loongson2f_halt(void)
>> +{
>> +#ifdef CONFIG_32BIT
>> + u32 base;
>> +#else
>> + u64 base;
>> +#endif
>> + u32 hi, lo, val;
>> +
>> + _rdmsr(0x8000000c, &hi, &lo);
>> +#ifdef CONFIG_32BIT
>> + base = (lo & 0xff00) | 0xbfd00000;
>> +#else
>> + base = (lo & 0xff00) | 0xffffffffbfd00000ULL;
>> +#endif
>> + val = (val & ~(1 << (16 + 13))) | (1 << 13);
>> + delay();
>> + *(__volatile__ u32 *)(base + 0x04) = val;
>> + delay();
>> + val = (val & ~(1 << (13))) | (1 << (16 + 13));
>> + delay();
>> + *(__volatile__ u32 *)(base + 0x00) = val;
>> + delay();
>> +}
>>     
>
> See above, re: magic numbers.
>
> *snip*
>
>   
>> +++ b/arch/mips/lemote/lm2f/fuloong/setup.c
>> +
>> +#ifdef CONFIG_64BIT
>> +#define PTR_PAD(p) ((0xffffffff00000000)|((unsigned long long)(p)))
>> +#else
>> +#define PTR_PAD(p) (p)
>> +#endif
>>     
>
> This shouldn't be necessary at all.
>
>   
>> +#ifdef CONFIG_64BIT
>> + __asm__(
>> + ".set mips3\n"
>> + "dli $2, 0x900000003ff00000\n"
>> + "dli $3, 0x0000000080000000\n"
>> + "sd $3, 0x10($2)\n"
>> + "sd $0, 0x50($2)\n"
>> + "dli $3, 0xffffffff80000000\n"
>> + "sd $3, 0x30($2)\n"
>> + ".set mips0\n"
>> + :::"$2","$3","memory");
>> + if (highmemsize > 0) {
>> + add_memory_region(0x90000000, highmemsize << 20, BOOT_MEM_RAM);
>> + }
>> +#endif
>>     
>
> This can all be done using accessors, as described above.
>
> I notice there seems to be a lot of duplicated code in the yeelong
> directory. Perhaps it is worthwhile to try to further consolidate some
> of it?
>
> My comments above seem to largely apply to the Yeelong code as well.
>
>   
>> +++ b/arch/mips/pci/fixup-fl2f.c
>> +#ifdef CONFIG_64BIT
>> + *(volatile u32*)0xffffffffbfe0004c = 0xd2000001;
>> +#else
>> + *(volatile u32*)0xbfe0004c = 0xd2000001;
>> +#endif
>>     
>
> Again, stuff like this is really unnecessary.
>
> I generally mirror Arnaud's sentiment about the structuring of the code
> -- I suspect a loongson/ directory in arch/mips makes more sense, then 
> followed by an ls2f/ and ls2e/ directory.
>
> Cheers,
> Phil
>
>   


-- 
晏华