2011/9/15, Andreas Barth <aba@xxxxxxxxxxxxxxx>: > * Kelvin Cheung (keguang.zhang@xxxxxxxxx) [110914 15:54]: >> 2011/9/14, Andreas Barth <aba@xxxxxxxxxxxxxxx>: >> > * keguang.zhang@xxxxxxxxx (keguang.zhang@xxxxxxxxx) [110914 12:49]: >> >> This patch adds basic support for Loongson1B >> >> including serial, timer and interrupt handler. >> > >> > I have a couple of questions. One of them is if it shouldn't be >> > possible to add this as part of the loongson-platform, and if we >> > really need a new platform. Each platform comes with some maintainence >> > costs which we should try to avoid. Making things more generic is >> > usually the right answer. >> >> I've tried to add Loongson1 to loongson-platform (acturally loongson2 >> platform), but there is essential difference between them. The >> loongson2 platform is something like the PC's architecture, which has >> north and south bridge, while the loongson1 is SoC. >> So, I think it's better that adding loongson1 as a new platform. > > I'm not convinced, but that's also not necessary. As I mentioned before, Loongson1 is a 32-bit SoC with many built-in controllers, which implements the MIPS32 release 2 instruction set, while Loongson2 is a 64-bit CPU, which only implements the MIPSIII instruction set. The Loongson1 does not have a PCI controller. And its interrupt and gpio are different from Loongson2 ... They are totally different thing. So, it is hard to reuse the code of loongson-platform. >> >> --- /dev/null >> >> +++ b/arch/mips/loongson1/common/clock.c >> >> @@ -0,0 +1,164 @@ >> >> +/* >> >> + * Copyright (c) 2011 Zhang, Keguang <keguang.zhang@xxxxxxxxx> >> > >> > Is this file not derived from any of the clock drivers we already have >> > in Linux? >> > >> > Doesn't any of the existing clock drivers work? >> > >> > Is this clock part of the CPU? Otherwise it would make sense to move >> > it out to the generic drivers section. > > What's the answer to this questions? I just did what other platform did, such as ar7, ath79, bcm63xx and jz4740. Could you have a look at their clock.c/clk.c? >> >> --- /dev/null >> >> +++ b/arch/mips/loongson1/common/irq.c >> >> @@ -0,0 +1,132 @@ >> >> +/* >> >> + * Copyright (c) 2011 Zhang, Keguang <keguang.zhang@xxxxxxxxx> >> >> + * >> >> + * Based on Copyright (C) 2009 Lemote Inc. >> > >> > same question here. Also, do you have permission from Lemote to put >> > the code within GPLv2? >> >> These code are based on the loongson platform, which is part of the >> kernel code already. > > In that case, it would make sense to say "derived from arch/mips/..." > so that other people can understand where it comes from. Thanks! I will pay attention to this. >> >> diff --git a/arch/mips/loongson1/common/prom.c >> >> b/arch/mips/loongson1/common/prom.c >> >> new file mode 100644 >> >> index 0000000..84a25f6 >> >> --- /dev/null >> >> +++ b/arch/mips/loongson1/common/prom.c >> > >> > Can't we re-use the prom-routines from the loongson platform here? Or >> > even better, factor them out somewhere else in the mips or even >> > generic linux tree? >> >> Same reason as question 1. > > Not really. Please try to de-duplicate code as far as possible, and to > generalize it's usage. Having some code of the form I did not just duplicate code, modified exactly. The prom-routines of loongson platform does not fit loongson1.For instance, the "cpu_clock_freq" parameter is not needed. The cpu freq. could be get from the PLL register directly. Moreover, the routines does not include the parameter "ethaddr", which is useful for embedded system. > + while (((readb(PORT(UART_BASE, UART_LSR)) & UART_LSR_THRE) == 0) > + && (timeout-- > 0)) > + ; > + > + writeb(c, PORT(UART_BASE, UART_TX)); > here doesn't make too much sense to me. (Also questioning why this is > part of the prom.c file). > I just did what other platform did, such as ar7, bcm47xx and jz4740. Could you have a look at their prom.c? > > Andi > -- Best Regards! Kelvin
