Re: [PATCH] MIPS: Add basic support for Loongson1B

[Andreas Barth]

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


> >> --- /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?



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

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




Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/