This is the second part of my review of the architecture, based on the
revised patch at
http://mprc.pku.edu.cn/~guanxuetao/linux/2.6.37/patch-2.6.37-rc2-uc32-gxt2-arch.bz2
I'm skipping the parts that I commented in the first review, but will
look at them again once we have resolved the points raised in this review
and Guan Xuetao posts the series to the mailing list.
The recurring theme with the unicore code is that it uses unnecessary
abstractions because it copies code from other architectures, mainly
the ARM tree, but does not actually require the complexity of the
original code. All the code that is required does seem to be there
and looks quite good for the most part, but there are substantial
parts that should just not be included.
The name of this file is somewhat misleading, it seems to refer to the
way that x86 originally did this using BIOS calls, which is not the case
on unicore. I would just name it pci.c.
Does not seem to be used anywhere, just remove this and
pcibios_bus_report_status.
This seems to be specific to some ARM IDE controller, I don't think
you need it here.
As I mentioned when discussing the header files, the pci_sys_data
abstraction is not useful on unicore, since you only have one
PCI controller.
Also all the hw_pci abstraction, which adds another indirection here.
When you start using the generic unistd.h file, you can also replace
this table with something like arch/tile/kernel/sys.c.
This should set PER_LINUX for the native personality, not
PER_LINUX_32BIT.
Then just remove irq_finish.
This also looks unnecessary, just rename puv3_init_irq to init_IRQ.
Please move all extern declarations to header files.
I don't think disable_hlt is used anywhere, it's rather x86 specific,
so I'd just drop it.
All of these can probably go away, since the code is not dynamic on
unicore.
Another extern that should be in a header.
Do you need to export these symbols? AFAICT they are only
used in nonmodular code.
Do you have a PC-stype CMOS RTC or is this just copied from ARM?
The symbol is disabled in defconfig, so I expect you don't need this.
AFAICT, you can remove all this if you just use GENERIC_CLOCKEVENT
unconditionally.
This function looks very obscure and it seems to deal with
legacy ARM issues that were copied here.
Better make the unicore specific interfaces regular syscalls
(sys_breakpoint, sys_cacheflush, sys_cmpxchg, ...).
This is unusual. If a syscall is not implemented, just return -ENOSYS,
rather than sending a signal.
The idea of this function is to cause a link error if it ever
gets called, so you must have no definition in the code!
This is a copy of the ARM-optimized memcpy code. Is it really better
than the libgcc version on unicore?
A similar question can be asked for much of the assembly code in
arch/unicore/lib. The code is rather old and compilers have gotten
a lot better in the meantime, so I could imagine that you'd be
better off just using the C versions.
Hmm, when the architecture was being defined, why didn't you ask
for a cycle counter? It really improves the delay code a lot.
If you have a good time base by now, you should use it. Is the
OST_OSCR something you could use here?
I would not plan to have multiple mach- directories if you only need one
today. It makes sense on ARM, which has dozens of different machines,
but in your case, I would hope that you control it well enough to
keep machines mostly compatible with each other.
If you start seeing incompatible SoCs in the future, the chances are
quite high that the level of abstraction does not match the differences
between them well. In particular, we are currently trying very hard
to get ARM into a state where you can build one kernel that uses
multiple machine directories together. If you do this right from
the start, it will be much easier.
There are currently patches under discussion for the ARM architecture
that unify the various struct clk definitions. It probably makes
sense for you to use the same definition.
It's usually more efficient to turn code like this into table lookups,
like
struct {
unsigned int rate;
unsigned int cfg;
unsigned int div;
} vga_clk_table = {
{ .rate = 25175000, .cfg = 0x00002001, .div = 0x9 }, /* 640x480@60 250M/10 */
...
};
Please have a look at the flattened device tree format as
a way to define the SoC components. Defining all the platform
data statically is generally not a scalable way to support
multiple SOCs in the long run, or even multiple boards
based on the same SOC in different configurations.
Statically defining platform devices is deprecated. Please use
platform_device_register_resndata or platform_device_register_simple
to create the platform_device for you.
I would not bother splitting the assembly code into so many CPU and
function specific files (abort, cache, ...) if all you have is a
single CPU type.
Please don't introduce new architectures specific procfs files.
If you need to track alignment exceptions, you could make that
a perf event.
Why do you even need to fix up alignment exceptions, can't you
just always send SIGBUS in this case?
This won't be needed once you get rid of highmem.
This file will look very different when you use swiotlb. Given
the restrictions on your PCI bus, I don't think your current
version even works in general for PCI devices.
This is a very complex definition for something very simple ;-)
#define IO_PORT_BASE (void __iomem *)0x80030000
return IO_PORT_BASE + (port & 0xffff);
This apparently never happened. The only place where vmregion is
actually use is managing coherent DMA ranges. Do you actually need
to do this? I don't see any architecture besides ARM using something
like this, and they have all sorts of complex problems including
noncoherent DMA devices and VIPT caches with aliasing problems.
Hmm, we don't have any precedent for putting ramfs contents into
the architecture tree. It's not necessarily a bad idea, but please
split this out into a separate patch so we can discuss this
independent of the architecture contents.
If people agree that this is a good thing to have in the kernel
sources, it should probably go into an architecture independent
place.
Please also split out the f64 code into a separate patch.
The code looks ok to me, but I have no clue what it actually
needs to do, but there is a large amount of code in it that
otherwise clutters up the review process.
When you have addressed the review comments you got up to
now, it would be good to start posting the code as separate
patches by email to the linux-kernel and linux-arch mailing
lists as described in the Documentation/SubmittingPatches
file. I recommend posting at most 10-20 patches at a time,
and if you have patches to include/asm-generic or other
common kernel code, please make them self-contained with
a proper changelog so we can apply them independent of the
architecture tree.
Arnd
--
