"Mark A. Greer" [off-list ref] wrote:
This patch adds core support for a line of host bridges from Marvell
(formerly Galileo). This code has been tested with a GT64260a,
GT64260b, MV64360, and MV64460. Patches for platforms that use these
bridges will be sent separately.
Shouldn't these guys:
+ u32 cpu2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+ { MV64x60_CPU2MEM_0_BASE, MV64x60_CPU2MEM_0_SIZE },
+ { MV64x60_CPU2MEM_1_BASE, MV64x60_CPU2MEM_1_SIZE },
+ { MV64x60_CPU2MEM_2_BASE, MV64x60_CPU2MEM_2_SIZE },
+ { MV64x60_CPU2MEM_3_BASE, MV64x60_CPU2MEM_3_SIZE }
+ };
+ u32 com2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+ { MV64360_MPSC2MEM_0_BASE, MV64360_MPSC2MEM_0_SIZE },
+ { MV64360_MPSC2MEM_1_BASE, MV64360_MPSC2MEM_1_SIZE },
+ { MV64360_MPSC2MEM_2_BASE, MV64360_MPSC2MEM_2_SIZE },
+ { MV64360_MPSC2MEM_3_BASE, MV64360_MPSC2MEM_3_SIZE }
+ };
+ u32 dram_selects[MV64x60_CPU2MEM_WINDOWS] = { 0xe, 0xd, 0xb, 0x7 };
be static, and maybe __devinitdata? Right now, the CPU has to populate
them by hand at runtime.
+wait_for_ownership(int chan)
+{
+ int i;
+
+ for (i=0; i<MAX_TX_WAIT; i++) {
+ if ((MV64x60_REG_READ(sdma_regs[chan].sdcm) &
+ SDMA_SDCM_TXD) == 0)
+ break;
+
+ udelay(1000);
ow, big busywait. Can't use a sleep in here? I guess not :(
+ * arch/ppc/boot/simple/mv64x60_tty.c
hm. Normally we put arch-specific drivers like this into drivers/serial
and do the appropriate Kconfig work. Is there a reason why this serial
driver is buried under arch/ppc?
+#include "../../../../drivers/serial/mpsc_defs.h"
erk.
+struct mv64x60_rx_desc {
+ volatile u16 bufsize;
+ volatile u16 bytecnt;
+ volatile u32 cmd_stat;
+ volatile u32 next_desc_ptr;
+ volatile u32 buffer;
+};
+
+struct mv64x60_tx_desc {
+ volatile u16 bytecnt;
+ volatile u16 shadow;
+ volatile u32 cmd_stat;
+ volatile u32 next_desc_ptr;
+ volatile u32 buffer;
+};
Do these need to be volatile? If so, it indicates that the driver is doing
something wrong.
+gt64260_register_hdlrs(void)
+{
+ /* Register CPU interface error interrupt handler */
+ request_irq(MV64x60_IRQ_CPU_ERR, gt64260_cpu_error_int_handler,
+ SA_INTERRUPT, CPU_INTR_STR, 0);
request_irq() can fail.
+int
+mv64360_get_irq(struct pt_regs *regs)
+{
+ int irq;
+ int irq_gpp;
+
+#ifdef CONFIG_SMP
+ /*
+ * Second CPU gets only doorbell (message) interrupts.
+ * The doorbell interrupt is BIT28 in the main interrupt low cause reg.
+ */
+ int cpu_nr = smp_processor_id();
This function has no callers, so I am unable to determine whether it is
called from non-preemptible code. If it is called from preemptible code
then that smp_processor_id() is buggy, because you can switch CPUs at any
time.
+static struct platform_device mpsc_shared_device = { /* Shared device */
+ .name = MPSC_SHARED_NAME,
+ .id = 0,
+ .num_resources = ARRAY_SIZE(mv64x60_mpsc_shared_resources),
+ .resource = mv64x60_mpsc_shared_resources,
+ .dev = {
+ .driver_data = (void *)&mv64x60_mpsc_shared_pd_dd,
+ },
+};
The cast to void* is unnecessary.
+ (void)mv64x60_setup_for_chip(&bh);
how come you always stick that (void) in there?
+mv64x60_config_cpu2mem_windows(struct mv64x60_handle *bh,
+ struct mv64x60_setup_info *si,
+ u32 mem_windows[MV64x60_CPU2MEM_WINDOWS][2])
+{
+ u32 i, win;
+ u32 prot_tab[] = {
+ MV64x60_CPU_PROT_0_WIN, MV64x60_CPU_PROT_1_WIN,
+ MV64x60_CPU_PROT_2_WIN, MV64x60_CPU_PROT_3_WIN
+ };
+ u32 cpu_snoop_tab[] = {
+ MV64x60_CPU_SNOOP_0_WIN, MV64x60_CPU_SNOOP_1_WIN,
+ MV64x60_CPU_SNOOP_2_WIN, MV64x60_CPU_SNOOP_3_WIN
+ };
static initialisation?
+mv64x60_config_cpu2pci_windows(struct mv64x60_handle *bh,
+ struct mv64x60_pci_info *pi, u32 bus)
+{
+ int i;
+ u32 win_tab[2][4] = {
+ { MV64x60_CPU2PCI0_IO_WIN, MV64x60_CPU2PCI0_MEM_0_WIN,
+ MV64x60_CPU2PCI0_MEM_1_WIN,
+ MV64x60_CPU2PCI0_MEM_2_WIN },
+ { MV64x60_CPU2PCI1_IO_WIN, MV64x60_CPU2PCI1_MEM_0_WIN,
+ MV64x60_CPU2PCI1_MEM_1_WIN,
+ MV64x60_CPU2PCI1_MEM_2_WIN },
+ };
+ u32 remap_tab[2][4] = {
+ { MV64x60_CPU2PCI0_IO_REMAP_WIN,
+ MV64x60_CPU2PCI0_MEM_0_REMAP_WIN,
+ MV64x60_CPU2PCI0_MEM_1_REMAP_WIN,
+ MV64x60_CPU2PCI0_MEM_2_REMAP_WIN },
+ { MV64x60_CPU2PCI1_IO_REMAP_WIN,
+ MV64x60_CPU2PCI1_MEM_0_REMAP_WIN,
+ MV64x60_CPU2PCI1_MEM_1_REMAP_WIN,
+ MV64x60_CPU2PCI1_MEM_2_REMAP_WIN }
+ };
+
ditto
+mv64x60_config_pci2mem_windows(struct mv64x60_handle *bh,
and here
+mv64360_set_pci2mem_window(struct pci_controller *hose, u32 bus, u32 window,
and here
+mv64360_config_io2mem_windows(struct mv64x60_handle *bh,
and here
Anyway, I'll stick this as-is in -mm. Feel free to send an incremental
patch, or a replacement.
Andrew Morton wrote:
"Mark A. Greer" [off-list ref] wrote:
quoted
This patch adds core support for a line of host bridges from Marvell
(formerly Galileo). This code has been tested with a GT64260a,
GT64260b, MV64360, and MV64460. Patches for platforms that use these
bridges will be sent separately.
Shouldn't these guys:
+ u32 cpu2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+ { MV64x60_CPU2MEM_0_BASE, MV64x60_CPU2MEM_0_SIZE },
+ { MV64x60_CPU2MEM_1_BASE, MV64x60_CPU2MEM_1_SIZE },
+ { MV64x60_CPU2MEM_2_BASE, MV64x60_CPU2MEM_2_SIZE },
+ { MV64x60_CPU2MEM_3_BASE, MV64x60_CPU2MEM_3_SIZE }
+ };
+ u32 com2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+ { MV64360_MPSC2MEM_0_BASE, MV64360_MPSC2MEM_0_SIZE },
+ { MV64360_MPSC2MEM_1_BASE, MV64360_MPSC2MEM_1_SIZE },
+ { MV64360_MPSC2MEM_2_BASE, MV64360_MPSC2MEM_2_SIZE },
+ { MV64360_MPSC2MEM_3_BASE, MV64360_MPSC2MEM_3_SIZE }
+ };
+ u32 dram_selects[MV64x60_CPU2MEM_WINDOWS] = { 0xe, 0xd, 0xb, 0x7 };
be static, and maybe __devinitdata? Right now, the CPU has to populate
them by hand at runtime.
Yes. I'll fix that.
+wait_for_ownership(int chan)
+{
+ int i;
+
+ for (i=0; i<MAX_TX_WAIT; i++) {
+ if ((MV64x60_REG_READ(sdma_regs[chan].sdcm) &
+ SDMA_SDCM_TXD) == 0)
+ break;
+
+ udelay(1000);
ow, big busywait. Can't use a sleep in here? I guess not :(
This code is in the ppc bootwrapper which is glue code to get the
hardware (and bd_info, etc.) from whatever state the f/w left it in into
something the kernel can work with. IOW, its not in the kernel and
there is no sleep mechanism...or even a thread/process entity to put to
sleep.
+ * arch/ppc/boot/simple/mv64x60_tty.c
hm. Normally we put arch-specific drivers like this into drivers/serial
and do the appropriate Kconfig work. Is there a reason why this serial
driver is buried under arch/ppc?
It isn't a part of the kernel so I don't think it belongs in
drivers/serial. This particular serial driver is required for cmd_line
editing when booting a zImage.
+#include "../../../../drivers/serial/mpsc_defs.h"
erk.
Ah, yeah, I'll fix that too :)
+struct mv64x60_rx_desc {
+ volatile u16 bufsize;
+ volatile u16 bytecnt;
+ volatile u32 cmd_stat;
+ volatile u32 next_desc_ptr;
+ volatile u32 buffer;
+};
+
+struct mv64x60_tx_desc {
+ volatile u16 bytecnt;
+ volatile u16 shadow;
+ volatile u32 cmd_stat;
+ volatile u32 next_desc_ptr;
+ volatile u32 buffer;
+};
Do these need to be volatile? If so, it indicates that the driver is doing
something wrong.
I didn't spend much time looking at this code. I'll clean it up.
+gt64260_register_hdlrs(void)
+{
+ /* Register CPU interface error interrupt handler */
+ request_irq(MV64x60_IRQ_CPU_ERR, gt64260_cpu_error_int_handler,
+ SA_INTERRUPT, CPU_INTR_STR, 0);
request_irq() can fail.
OK.
+int
+mv64360_get_irq(struct pt_regs *regs)
+{
+ int irq;
+ int irq_gpp;
+
+#ifdef CONFIG_SMP
+ /*
+ * Second CPU gets only doorbell (message) interrupts.
+ * The doorbell interrupt is BIT28 in the main interrupt low cause reg.
+ */
+ int cpu_nr = smp_processor_id();
This function has no callers, so I am unable to determine whether it is
called from non-preemptible code. If it is called from preemptible code
then that smp_processor_id() is buggy, because you can switch CPUs at any
time.
Its called via ppc_md.get_irq() (see arch/ppc/kernel/irq.c:do_IRQ()).
ppc_md.get_irq is set up in the platform files.
+static struct platform_device mpsc_shared_device = { /* Shared device */
+ .name = MPSC_SHARED_NAME,
+ .id = 0,
+ .num_resources = ARRAY_SIZE(mv64x60_mpsc_shared_resources),
+ .resource = mv64x60_mpsc_shared_resources,
+ .dev = {
+ .driver_data = (void *)&mv64x60_mpsc_shared_pd_dd,
+ },
+};
The cast to void* is unnecessary.
OK.
+ (void)mv64x60_setup_for_chip(&bh);
how come you always stick that (void) in there?
I did that b/c the routine returns an 'int' but I'm ignoring it. I
probably shouldn't be ignoring it...
+mv64x60_config_cpu2mem_windows(struct mv64x60_handle *bh,
+ struct mv64x60_setup_info *si,
+ u32 mem_windows[MV64x60_CPU2MEM_WINDOWS][2])
+{
+ u32 i, win;
+ u32 prot_tab[] = {
+ MV64x60_CPU_PROT_0_WIN, MV64x60_CPU_PROT_1_WIN,
+ MV64x60_CPU_PROT_2_WIN, MV64x60_CPU_PROT_3_WIN
+ };
+ u32 cpu_snoop_tab[] = {
+ MV64x60_CPU_SNOOP_0_WIN, MV64x60_CPU_SNOOP_1_WIN,
+ MV64x60_CPU_SNOOP_2_WIN, MV64x60_CPU_SNOOP_3_WIN
+ };
static initialisation?
Yep.
+mv64x60_config_cpu2pci_windows(struct mv64x60_handle *bh,
+ struct mv64x60_pci_info *pi, u32 bus)
+{
+ int i;
+ u32 win_tab[2][4] = {
+ { MV64x60_CPU2PCI0_IO_WIN, MV64x60_CPU2PCI0_MEM_0_WIN,
+ MV64x60_CPU2PCI0_MEM_1_WIN,
+ MV64x60_CPU2PCI0_MEM_2_WIN },
+ { MV64x60_CPU2PCI1_IO_WIN, MV64x60_CPU2PCI1_MEM_0_WIN,
+ MV64x60_CPU2PCI1_MEM_1_WIN,
+ MV64x60_CPU2PCI1_MEM_2_WIN },
+ };
+ u32 remap_tab[2][4] = {
+ { MV64x60_CPU2PCI0_IO_REMAP_WIN,
+ MV64x60_CPU2PCI0_MEM_0_REMAP_WIN,
+ MV64x60_CPU2PCI0_MEM_1_REMAP_WIN,
+ MV64x60_CPU2PCI0_MEM_2_REMAP_WIN },
+ { MV64x60_CPU2PCI1_IO_REMAP_WIN,
+ MV64x60_CPU2PCI1_MEM_0_REMAP_WIN,
+ MV64x60_CPU2PCI1_MEM_1_REMAP_WIN,
+ MV64x60_CPU2PCI1_MEM_2_REMAP_WIN }
+ };
+
ditto
+mv64x60_config_pci2mem_windows(struct mv64x60_handle *bh,
and here
+mv64360_set_pci2mem_window(struct pci_controller *hose, u32 bus, u32 window,
and here
+mv64360_config_io2mem_windows(struct mv64x60_handle *bh,
and here
Yes, several times. I'll fix it.
Anyway, I'll stick this as-is in -mm. Feel free to send an incremental
patch, or a replacement.
Thanks for the feedback. I'll clean it up & resend.
Mark