mv64x60 patches for Radstone PPC7D - for review
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi James.
Quoted 10 lines · James Chapman
Attached is a series of patches adding support for Radstone PPC7D boards and Marvell Discovery watchdog. PPC7D are rugged Marvell Discovery boards with PPC7447A CPUs, dual GigE, dual PMC and optional SCSI, VGA.
Signed-off-by James Chapman <[email protected]>The patches are split into separate pieces to aid review. Most patches are generic for mv64x60 boards and could be applied separately. Patch p9, however, requires all other patches.
From now on, please make a separate email per patch. That way the email threads/patch discussions are easier to track.
Quoted 6 lines
Patch p2 just makes fields in
/proc/interruptsline up so it is optional.p1 - fix
mv64360_pic_irq_offsetbugs when non-zero
While you're in there, would you make a #define for the IRQ 28 in include/asm-ppc/mv64x60_defs.h. Otherwise, it looks good.
Quoted 4 lines
p2 - cleanup formatting of mv64360 entries in
/proc/interrupts
[optional]
Looks fine but you may as well combine w/ patch p1.
Quoted 5 lines
p3 - add
pciauto_ignore_device()hook to allow platforms to ignore
specific devices inpciauto_bus_scan(). Should this hook be inppc_mdinstead?
Is there a reason ppc_md.pci_exclude_device doesn't work?
Quoted 3 lines
p4 - add GPP IO pin/IRQ register definitions
Looks fine.
Quoted 3 lines
p5 - add extern
i8259_pic_irq_offset
XXXX
Quoted 3 lines
p6 - add 7447A and 7457 CPU definitions
Looks okay to me but these should probably be posted to linuxppc-dev.
Quoted 3 lines
p7 - add mv64x60 watchdog support
I think it would be better to use platform_data to pass in the duration/timeout value and not a config option. Other than that, it looks okay to me but I didn't go over it in detail.
When you separate this patch into its own email, you should send it to whoever looks after the watchdog drivers, cc the appropriate mailing list, and lmkl and/or linuxppc-embedded.
Quoted 4 lines
p8 - add
mv64x60_setclr_bits()which can be used to set and clear bits
of a mv64x60 register with a single chip register write.
Is this really necessary? I have learned that many in the community really hate little routines like this. I have a todo item to get rid of the _set/clr_bits routines and replace them explicit code.
Quoted 3 lines
p9 - add Radstone PPC7D board support
I didn't go through this in great detail but I do have a few comments:
Quoted 3 lines
+++
linux-2.6/arch/ppc/boot/simple/misc-radstone_ppc7d.c+void
__attribute__((weak))mv64x60_board_init(void)
Don't make this "weak", there is one alread defined as weak that you're replacing.
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* Map 0xe0000000-0xffffffff early because we need access to SRAM + * and the ISA memory space (for serial port) here.
You mean for accessing the serial port while in the bootwrapper, right? The firmware doesn't put anything out the serial port? If so, there is likely a mapping alread setup for you to use, no?
Quoted 1 line
+++ linux-2.6/arch/ppc/Kconfig
Why bother with RADSTONE_DEBUG? Plus I'm told that pr_debug() is the preferred method for this.
Why a separate CONFIG_RADSTONE and CONFIG_RADSTONE_PPC7C? You never use CONFIG_RADSTONE.
Quoted 1 line
+++
linux-2.6/arch/ppc/platforms/radstone_ppc7d.c
It looks like you only really use the 8250 uart. Is that correct? If so, get rid of the MPSC stuff since its only clutter.
Quoted 1 line
static void
__initppc7d_early_serial_map(void)
I don't think you need this as long as you have your STD_SERIAL_PORT_DFNS setup correctly.
Quoted 1 line
TODC_ALLOC();
It doesn't look like you need this since you don't actually use a time-of-day/realtime clock.
Quoted 1 line
DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1533, ppc7d_fixup_ali1535);
You have a lot of pci fixups. Are they all necessary? I noticed a quirk already implemented for the 1533...
General note: you have a lot of lines that wrap in an 80 column window. I guess its a personal preference but I find it hard to read. Would you mind breaking them into separate lines?
Mark
Quoted 10 lines · Mark A. Greer
Hi James.
Quoted 2 lines · James Chapman
p5 - add extern
i8259_pic_irq_offsetXXXX
Oops, hit send to soon.
This looks okay but in your platform file, you never set it anyway so you don't need to access it in there.
Mark